Returning an error doesn't seem right here because this stops cleanup.

I... kind-of want to stop cleanup and fail, because this means (unless Atoi has other failure cases) that the Windows snapshotter behaviour has changed, and this code wasn't updated. I'd want that to fail CI.

To be clear, the only circumstances I could hit this with the current code-base is an rm-<digits> directory, created during func (s *snapshotter) Remove, and having fallen through either "Failed to rename after failed commit" or "Failed to remove root filesystem". In that case, it seems quite likely we will fail to call cleanupWCOWLayer on it anyway.

I could refactor this to collect all the directories, best-effort sort them (I'd have to strip the rm- prefix so that I remove child layers before parent layers), and then record any cleanupWCOWLayer failure but still try it on all of them. The outcome would be that if a child layer is stuck, it and all its parent layers would report errors during clean-up, and we'd probably stumble over panic that prompted this PR in the first place, as we'd be trying to remove a parent layer while its child is activated.

Huh, and rebasing to master, I managed to trigger exactly this code-path on CI:

2021-03-20T00:25:28.7332957Z failed to remove test root dir failed to cleanup WCOW layers in C:\Program Files\containerd\root-test\io.containerd.snapshotter.v1.windows\snapshots: strconv.Atoi: parsing "rm-26": invalid syntax
2021-03-20T00:25:28.8327339Z exit status 1
2021-03-20T00:25:28.8328816Z FAIL	github.com/containerd/containerd/integration/client	358.389s
2021-03-20T00:25:28.9091215Z mingw32-make: *** [Makefile:181: integration] Error 1

The layer itself doesn't seem to have caused any failures in the tests, so my guess this hit the "Failed to remove root filesystem" Warnf and that doesn't turn into a test failure (or get logged anywhere...). Annoyingly, there is a hcsshim::DeactivateLayer span, but not a hcsshim::DestroyLayer span, so I can't definitively tie this to a particular test.

I suspect this represents a gap in the containerd Windows snapshotter, as it's never cleaning up left-over rm- layer directories... Particularly because if I'm reading the test logs correctly, the directory in question came from "Integration 1", but the cleanup failure was the end of "Integration 2".

Thinking about it, it might be reasonable to, during this walk, just os.RemoveAll or cleanupWCOWLayer any rm-* layers we encounter during the Walk. My main concern is that if a stack of layers fails removal, and ends up all being renamed to rm-*, we'll walk them in the wrong order and hit the panic again.

Upshot for me is that if this routine fails as-is, we should have failed earlier because the tests have left the Snapshotter in an inconsistent state, and I'm not sure if trying to force the clean-up harder is productive at that point, as it seems like clean-up will fail for the same reasons that left the Snapshotter state inconsistent.