fix(shim): bound teardown waits so a leftover bundle can't wedge a container

internal/erofs/vmdk.go

@@ -33,6 +33,64 @@ const (
 	hwVersion           = "4"
 )
 
+// The names the shim writes into a bundle directory. They live here, beside
+// the writers that derive them, so that shim teardown — which has to recognise
+// and remove them — cannot drift from what was written.
+//
+// There is deliberately no "_tail.bin" suffix: the GPT disk is synthetic and
+// read-only, so no secondary GPT is written.
+const (
+	bundleDescriptorPrefix = "merged_fs"
+	GPTDescriptorName      = bundleDescriptorPrefix + "_gpt.vmdk"
+
+	auxHeaderSuffix = "_header.bin"
+	auxPadSuffix    = "_pad.bin"
+)
+
+var auxSuffixes = []string{auxHeaderSuffix, auxPadSuffix}
+
+// FlatDescriptorName is the basename of the flat-concat VMDK descriptor for a
+// multi-device erofs mount assigned the given disk letter. The letter keeps
+// several such mounts in one bundle from overwriting each other.
+func FlatDescriptorName(letter byte) string {
+	return fmt.Sprintf("%s_%c.vmdk", bundleDescriptorPrefix, letter)
+}
+
+// auxFilePath names one auxiliary blob after its descriptor, replacing the
+// descriptor's extension with suffix.
+func auxFilePath(vmdkPath, suffix string) string {
+	dir := filepath.Dir(vmdkPath)
+	base := strings.TrimSuffix(filepath.Base(vmdkPath), filepath.Ext(vmdkPath))
+	return filepath.Join(dir, base+suffix)
+}
+
+// IsBundleArtifact reports whether name — a bare directory entry name, not a
+// path — is a descriptor or auxiliary blob the shim wrote into a bundle.
+//
+// Shim teardown removes these before containerd deletes the bundle, because on
+// Windows a descriptor extent still mapped by the VM cannot be unlinked, and one
+// leftover file makes containerd's recursive bundle removal fail — permanently,
+// since the stale bundle then also blocks every later start of that container.
+//
+// Matching by name rather than globbing is deliberate: a bundle path holding a
+// glob metacharacter would make the pattern match nothing and report no error,
+// and on Windows it cannot even be escaped, the separator being the escape
+// character.
+func IsBundleArtifact(name string) bool {
+	if !strings.HasPrefix(name, bundleDescriptorPrefix) {
+		return false
+	}
+	if filepath.Ext(name) == ".vmdk" {
+		return true
+	}
+	for _, suffix := range auxSuffixes {
+		if strings.HasSuffix(name, suffix) {
+			return true
+		}
+	}
+	return false
+}
+
 // vmdkDescAddExtent writes extent lines to the writer.
 // Each extent line follows the format: RW <count> FLAT "<filename>" <offset>
 // A single device > 2 GiB is split across multiple RW lines, each referencing
@@ -229,9 +287,7 @@ func DumpGPTVMDKDescriptorToFile(vmdkPath string, cid uint32, devices []string)
 		return err
 	}
 
-	dir := filepath.Dir(vmdkPath)
-	base := strings.TrimSuffix(filepath.Base(vmdkPath), filepath.Ext(vmdkPath))
-	headerPath := filepath.Join(dir, base+"_header.bin")
+	headerPath := auxFilePath(vmdkPath, auxHeaderSuffix)
 
 	if err := writeBlob(headerPath, layout.WriteHeader); err != nil {
 		return err
@@ -248,7 +304,7 @@ func DumpGPTVMDKDescriptorToFile(vmdkPath string, cid uint32, devices []string)
 	if !gptUseZeroExtents {
 		// The maximum single padding region is bounded by gptAlignSectors
 		// (1 MiB minus a sector); a 1 MiB pad file always suffices.
-		padFile = filepath.Join(dir, base+"_pad.bin")
+		padFile = auxFilePath(vmdkPath, auxPadSuffix)
 		if err := writePadFile(padFile, gptAlignSectors*gptSectorSize); err != nil {
 			cleanup()
 			return err

internal/erofs/vmdk_names_test.go

@@ -0,0 +1,134 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package erofs
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// bundleArtifacts lists the entries of dir that IsBundleArtifact classifies as
+// shim-written, as full paths — the same enumeration shim teardown performs.
+func bundleArtifacts(t *testing.T, dir string) []string {
+	t.Helper()
+	entries, err := os.ReadDir(dir)
+	require.NoError(t, err)
+	var matched []string
+	for _, e := range entries {
+		if IsBundleArtifact(e.Name()) {
+			matched = append(matched, filepath.Join(dir, e.Name()))
+		}
+	}
+	return matched
+}
+
+// TestBundleArtifactsMatchWhatIsWritten is the anti-drift test: it writes the
+// descriptors the shim actually writes into a bundle and asserts the
+// classifier used by teardown matches every one of them. If a writer starts
+// emitting a new name, this fails rather than silently leaving a file behind
+// that would wedge the container id.
+func TestBundleArtifactsMatchWhatIsWritten(t *testing.T) {
+	t.Cleanup(func() {
+		gptUseZeroExtents = true
+	})
+	// Disable ZERO extents so the pad file is written too, putting the full set
+	// of auxiliary blobs in front of the classifier.
+	gptUseZeroExtents = false
+
+	bundle := t.TempDir()
+	devices := []string{
+		makeLayerFile(t, bundle, "a.img", 4*1024*1024),
+		makeLayerFile(t, bundle, "b.img", 4*1024*1024),
+	}
+
+	// The GPT descriptor plus its auxiliary blobs.
+	gptPath := filepath.Join(bundle, GPTDescriptorName)
+	require.NoError(t, DumpGPTVMDKDescriptorToFile(gptPath, 0xfffffffe, devices))
+
+	// A flat-concat descriptor, as written for a multi-device erofs mount.
+	flatPath := filepath.Join(bundle, FlatDescriptorName('b'))
+	require.NoError(t, DumpVMDKDescriptorToFile(flatPath, 0xfffffffe, devices))
+
+	want := []string{
+		gptPath,
+		filepath.Join(bundle, "merged_fs_gpt_header.bin"),
+		filepath.Join(bundle, "merged_fs_gpt_pad.bin"),
+		flatPath,
+	}
+	for _, p := range want {
+		require.FileExists(t, p, "expected the writers to produce this file")
+	}
+
+	matched := bundleArtifacts(t, bundle)
+
+	assert.ElementsMatch(t, want, matched,
+		"IsBundleArtifact must match exactly the descriptors and blobs written into the bundle")
+
+	// Classification must not sweep up the layer images, which live outside
+	// the bundle in production and are not the shim's to delete.
+	for _, dev := range devices {
+		assert.NotContains(t, matched, dev)
+	}
+}
+
+// TestIsBundleArtifactWithGlobMetacharactersInPath guards the reason teardown
+// enumerates instead of globbing: a bundle directory whose path contains a glob
+// metacharacter must still have its artifacts found. filepath.Glob would return
+// no matches and no error here, silently leaving the files that wedge the
+// container id.
+func TestIsBundleArtifactWithGlobMetacharactersInPath(t *testing.T) {
+	bundle := filepath.Join(t.TempDir(), "state[1]")
+	require.NoError(t, os.MkdirAll(bundle, 0o755))
+
+	devices := []string{
+		makeLayerFile(t, bundle, "a.img", 4*1024*1024),
+		makeLayerFile(t, bundle, "b.img", 4*1024*1024),
+	}
+	gptPath := filepath.Join(bundle, GPTDescriptorName)
+	require.NoError(t, DumpGPTVMDKDescriptorToFile(gptPath, 0xfffffffe, devices))
+
+	assert.ElementsMatch(t,
+		[]string{gptPath, filepath.Join(bundle, "merged_fs_gpt_header.bin")},
+		bundleArtifacts(t, bundle))
+}
+
+func TestBundleArtifactsRemoveEverything(t *testing.T) {
+	bundle := t.TempDir()
+	devices := []string{
+		makeLayerFile(t, bundle, "a.img", 4*1024*1024),
+		makeLayerFile(t, bundle, "b.img", 4*1024*1024),
+	}
+	require.NoError(t, DumpGPTVMDKDescriptorToFile(
+		filepath.Join(bundle, GPTDescriptorName), 0xfffffffe, devices))
+
+	for _, p := range bundleArtifacts(t, bundle) {
+		require.NoError(t, os.RemoveAll(p))
+	}
+
+	entries, err := os.ReadDir(bundle)
+	require.NoError(t, err)
+	var left []string
+	for _, e := range entries {
+		left = append(left, e.Name())
+	}
+	assert.ElementsMatch(t, []string{"a.img", "b.img"}, left,
+		"only the layer images should survive; every shim-written artifact must be gone")
+}

internal/shim/task/mount.go

@@ -136,7 +136,7 @@ func transformMounts(ctx context.Context, id string, ms []*types.Mount, da *disk
 				// Use the disk letter in the filename so that multiple
 				// multi-device erofs mounts within the same bundle each get
 				// a distinct descriptor and don't overwrite each other.
-				mergedfsPath := filepath.Join(bundleDir, fmt.Sprintf("merged_fs_%c.vmdk", letter))
+				mergedfsPath := filepath.Join(bundleDir, erofs.FlatDescriptorName(letter))
 				if err := erofs.DumpVMDKDescriptorToFile(mergedfsPath, 0xfffffffe, devices); err != nil {
 					log.G(ctx).WithError(err).WithField("path", mergedfsPath).Warn("failed to generate erofs vmdk descriptor")
 					return nil, nil, fmt.Errorf("erofs vmdk: %w", errdefs.ErrNotImplemented)
@@ -268,7 +268,7 @@ func finalizeErofsCandidates(ctx context.Context, id string, da *diskAllocator,
 		// shim does not mutate the source image directories.  Cache by
 		// stat-check: setupMounts may be called more than once per bundle
 		// (e.g. on restore) but the layer set for a given bundle is fixed.
-		gptPath := filepath.Join(bundleDir, "merged_fs_gpt.vmdk")
+		gptPath := filepath.Join(bundleDir, erofs.GPTDescriptorName)
 		if _, err := os.Stat(gptPath); err != nil {
 			if !os.IsNotExist(err) {
 				log.G(ctx).WithError(err).WithField("path", gptPath).Warn("failed to stat erofs gpt vmdk descriptor")

internal/shim/task/service.go

@@ -216,11 +216,49 @@ func (s *service) RegisterTTRPC(server *ttrpc.Server) error {
 	return nil
 }
 
+// Shutdown is bounded by its caller's context — containerd's shutdown service
+// gives it a deadline — so only the best-effort phases carry a timeout of their
+// own. Stopping the VM gets whatever time is left, because it is the phase that
+// releases the bundle's disk files and a VM left running is the one outcome
+// nothing downstream can recover from.
+const (
+	// Flushing guest journals before power-off is worth attempting, not worth
+	// the whole shutdown: an unhealthy guest keeps failing transiently for as
+	// long as it is given, and would leave nothing for the VM stop.
+	guestUnmountTimeout = 5 * time.Second
+
+	// What the VM stop runs under when the caller's context is spent before it
+	// is reached, so the one unrecoverable phase still gets a real attempt
+	// rather than an already-cancelled context.
+	exhaustedStopBudget = 2 * time.Second
+
+	eventSentinelTimeout = 1 * time.Second
+)
+
 func (s *service) shutdown(ctx context.Context) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	var errs []error
 
+	// Deferred so that a phase below hanging cannot skip them: without the
+	// rootfs removal containerd's bundle cleanup attempts a bind filter unmount
+	// that fails on Windows, and without the sentinel the event forwarder never
+	// returns.
+	defer func() {
+		removeRootfsDir(ctx)
+
+		// Bounded rather than an unconditional send: the forwarder may be
+		// blocked in Publish or already gone, and waiting on it forever here
+		// would defeat the point of the deferral.
+		timer := time.NewTimer(eventSentinelTimeout)
+		defer timer.Stop()
+		select {
+		case s.events <- nil:
+		case <-timer.C:
+			log.G(ctx).Warn("timed out sending shutdown sentinel; event forwarder may still be running")
+		}
+	}()
+
 	for id, c := range s.containers {
 		if err := c.shutdown(ctx); err != nil {
 			errs = append(errs, fmt.Errorf("container %q shutdown: %w", id, err))
@@ -230,28 +268,62 @@ func (s *service) shutdown(ctx context.Context) error {
 	if s.sb != nil {
 		// Unmount all block volumes inside the guest before stopping the VM,
 		// to flush ext4 journals and dirty pages to the virtio-blk devices.
-		// Best-effort with a short retry for transient EBUSY.
+		// Best-effort with a short retry for transient EBUSY — bounded, because
+		// an unhealthy guest keeps failing transiently for as long as it is
+		// given and would starve the VM stop below.
 		if vmc, err := s.sb.Client(); err != nil {
 			log.G(ctx).WithError(err).Warn("failed to get VM client; skipping unmount of block volumes before VM shutdown")
 		} else {
-			if err := unmountAllWithRetry(ctx, mountAPI.NewTTRPCMountClient(vmc)); err != nil {
-				log.G(ctx).WithError(err).Warn("failed to unmount all block volumes before VM shutdown")
+			unmountCtx, cancel := context.WithTimeout(ctx, guestUnmountTimeout)
+			err := unmountAllWithRetry(unmountCtx, mountAPI.NewTTRPCMountClient(vmc))
+			cancel()
+			if err != nil {
+				log.G(ctx).WithError(err).
+					Warn("failed to unmount all block volumes before VM shutdown")
 			}
 		}
 
-		if err := s.sb.Stop(ctx); err != nil {
-			errs = append(errs, fmt.Errorf("sandbox shutdown: %w", err))
+		if err := s.stopSandbox(ctx); err != nil {
+			errs = append(errs, err)
 		}
 	}
 
-	// Remove the rootfs directory on Windows so containerd's bundle cleanup
-	// doesn't attempt a bind filter unmount (no-op on other platforms).
-	removeRootfsDir(ctx)
+	return errors.Join(errs...)
+}
 
-	// Signal last event and stop forwarding
-	s.events <- nil
+// stopSandbox stops the VM, abandoning the wait if it outlasts its share of the
+// shutdown budget.
+//
+// Stop cannot be relied on to honour its context — it delegates to a vm.Instance
+// implementation, and one blocked on an internal handoff is not cancellable from
+// here. Abandoning the wait is what keeps the deferred bundle cleanup in
+// [service.shutdown] reachable.
+func (s *service) stopSandbox(ctx context.Context) error {
+	stopCtx := ctx
+	if ctx.Err() != nil {
+		// An earlier phase spent the caller's budget. Running Stop on the dead
+		// context would abandon it before the goroutine below even started, so
+		// give it one of its own — see [exhaustedStopBudget].
+		var cancel context.CancelFunc
+		stopCtx, cancel = context.WithTimeout(context.WithoutCancel(ctx), exhaustedStopBudget)
+		defer cancel()
+		log.G(ctx).WithField("stop_budget", exhaustedStopBudget).
+			Warn("shutdown budget spent before stopping the VM; stopping it anyway")
+	}
 
-	return errors.Join(errs...)
+	done := make(chan error, 1)
+	go func() { done <- s.sb.Stop(stopCtx) }()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			return fmt.Errorf("sandbox shutdown: %w", err)
+		}
+		return nil
+	case <-stopCtx.Done():
+		log.G(ctx).Error("gave up waiting for the VM to stop; it may still hold files in the bundle, which will prevent containerd from deleting the bundle and block subsequent starts of this container")
+		return fmt.Errorf("sandbox shutdown: %w", stopCtx.Err())
+	}
 }
 
 // unmountAllWithRetry asks the guest to unmount all tracked mounts, retrying