From 8e3a554631fd9d898c7d5c366bf67f58d1d9bd05 Mon Sep 17 00:00:00 2001 From: Austin Vazquez Date: Wed, 5 Aug 2026 15:24:38 -0500 Subject: [PATCH] fix(shim): bound teardown waits so a leftover bundle can't wedge a container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows a container id could be wedged permanently: every start returned creating containerd task for container : failed to remove stale bundle path "...": unlinkat ...\\merged_fs_gpt_header.bin: Access is denied. The bundle holds VMDK descriptors and auxiliary blobs that the VM maps while it runs, and a mapped file cannot be unlinked on Windows. Teardown had two unbounded waits that let containerd race ahead of the release: - manager.Stop waited on WaitForSingleObject(h, INFINITE) for the shim to exit. containerd bounds the whole `shim delete` call by io.containerd.timeout.shim.cleanup (5s), so it killed us mid-call — "failed to delete dead shim: exit status 1" — and a killed process runs no defers, so the bundle cleanup never ran at all. TerminateProcess is not instantaneous for a VM-backed shim whose threads sit in kernel-mode hypervisor calls. - service.shutdown waited on sb.Stop with no bound. A vm.Instance that does not honour its context cannot be cancelled from there, and a callback that never returns keeps the shutdown service from ever completing. Changes: - Bound the shim-exit wait and make it observe ctx. On expiry, report a valid StopStatus instead of an error: containerd deletes the bundle either way, so the useful thing is to finish under our own control with the cleanup done, and to name the surviving pid. - Widen cleanup from rootfs to every artifact the shim wrote, classified by erofs.IsBundleArtifact and enumerated with ReadDir. Enumeration rather than globbing because a bundle path holding a glob metacharacter would match nothing and report no error, and on Windows cannot be escaped. Retries run under one deadline for the whole set, not per file, so the total stays inside containerd's cleanup budget. - Keep the descriptor and blob names beside the writers that derive them, so teardown cannot drift from what was written. - Bound the in-shim teardown: the best-effort guest unmount gets a flat timeout so it cannot starve the VM stop, the VM stop gets the caller's remaining context, and rootfs removal plus the forwarder sentinel move to a defer so a hang cannot skip them. - Apply the same bounded wait on unix, and reject a non-positive shim pid before it reaches kill, where 0 would signal the whole process group. This bounds the damage and makes the failure diagnosable, but cannot by itself release a handle held elsewhere. Signed-off-by: Austin Vazquez --- internal/erofs/vmdk.go | 64 +++- internal/erofs/vmdk_names_test.go | 134 +++++++ internal/shim/task/mount.go | 4 +- internal/shim/task/service.go | 94 ++++- internal/shim/task/service_shutdown_test.go | 274 ++++++++++++++ pkg/shim/manager/manager_unix.go | 107 ++++-- pkg/shim/manager/manager_windows.go | 148 +++++++- pkg/shim/manager/manager_windows_stop_test.go | 335 ++++++++++++++++++ 8 files changed, 1103 insertions(+), 57 deletions(-) create mode 100644 internal/erofs/vmdk_names_test.go create mode 100644 internal/shim/task/service_shutdown_test.go create mode 100644 pkg/shim/manager/manager_windows_stop_test.go diff --git a/internal/erofs/vmdk.go b/internal/erofs/vmdk.go index 36e85eff..0cc148c0 100644 --- a/internal/erofs/vmdk.go +++ b/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 FLAT "" // 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 diff --git a/internal/erofs/vmdk_names_test.go b/internal/erofs/vmdk_names_test.go new file mode 100644 index 00000000..4fb6f7ec --- /dev/null +++ b/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") +} diff --git a/internal/shim/task/mount.go b/internal/shim/task/mount.go index 4f626eaf..d3491d03 100644 --- a/internal/shim/task/mount.go +++ b/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") diff --git a/internal/shim/task/service.go b/internal/shim/task/service.go index d3c81998..ffd9be27 100644 --- a/internal/shim/task/service.go +++ b/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 diff --git a/internal/shim/task/service_shutdown_test.go b/internal/shim/task/service_shutdown_test.go new file mode 100644 index 00000000..33d8e458 --- /dev/null +++ b/internal/shim/task/service_shutdown_test.go @@ -0,0 +1,274 @@ +/* + 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 task + +import ( + "context" + "errors" + "net" + "testing" + "time" + + "github.com/containerd/ttrpc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/containerd/nerdbox/internal/shim/sandbox" +) + +// fakeSandbox is a sandbox.Sandbox whose Stop behaviour is controlled by the +// test. Client always fails, which makes service.shutdown skip the guest +// unmount phase and go straight to stopping the VM — the phase under test. +type fakeSandbox struct { + // stopBlocks, when true, makes Stop block until released is closed, + // ignoring its context. This models a vm.Instance implementation that + // blocks on an internal handoff and cannot be cancelled. + stopBlocks bool + released chan struct{} + + stopErr error + stopCalled chan struct{} + // stopCtxErr records ctx.Err() as observed on entry to Stop, so a test can + // assert Stop was not handed an already-dead context. + stopCtxErr error +} + +func newFakeSandbox() *fakeSandbox { + return &fakeSandbox{ + released: make(chan struct{}), + stopCalled: make(chan struct{}, 1), + } +} + +func (f *fakeSandbox) Start(context.Context, ...sandbox.Opt) error { return nil } + +func (f *fakeSandbox) Stop(ctx context.Context) error { + f.stopCtxErr = ctx.Err() + select { + case f.stopCalled <- struct{}{}: + default: + } + if f.stopBlocks { + // Deliberately ignores ctx. + <-f.released + } + return f.stopErr +} + +func (f *fakeSandbox) Client() (*ttrpc.Client, error) { + return nil, errors.New("no VM client in test") +} + +func (f *fakeSandbox) StartStream(context.Context, string) (net.Conn, error) { + return nil, errors.New("not implemented") +} + +func (f *fakeSandbox) ReservedDisks() int { return 0 } + +// newTestService builds a service with the given sandbox, wired up the way +// NewTaskService does but without the shim framework. +func newTestService(sb sandbox.Sandbox) *service { + return &service{ + context: context.Background(), + sb: sb, + events: make(chan any, 128), + containers: make(map[string]*container), + } +} + +// TestShutdownReturnsWhenVMStopHangs is the regression test for the wedge: +// a VM that never finishes stopping must not hold shutdown open, because the +// steps after it are what let containerd delete the bundle. Before the fix, +// service.shutdown blocked in sb.Stop forever and never reached them. +func TestShutdownReturnsWhenVMStopHangs(t *testing.T) { + sb := newFakeSandbox() + sb.stopBlocks = true + t.Cleanup(func() { close(sb.released) }) + + s := newTestService(sb) + + // A deadline stands in for the shutdown service's overall budget. + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + done := make(chan error, 1) + go func() { done <- s.shutdown(ctx) }() + + select { + case err := <-done: + // The hang must be reported, not swallowed. + require.Error(t, err) + assert.Contains(t, err.Error(), "sandbox shutdown") + case <-time.After(5 * time.Second): + t.Fatal("service.shutdown did not return while sb.Stop was blocked") + } + + // Stop was actually attempted. + select { + case <-sb.stopCalled: + default: + t.Error("expected sb.Stop to have been called") + } + + // The forwarder sentinel must still have been sent, so the event + // forwarding goroutine can exit. + select { + case ev := <-s.events: + assert.Nil(t, ev, "expected the nil shutdown sentinel") + default: + t.Error("shutdown sentinel was not sent when sb.Stop hung") + } +} + +// TestShutdownStopWaitEndsWithCallerContext pins where the VM stop's patience +// comes from: the caller's deadline, not a timeout of its own. It should wait +// out the context it was given, then stop waiting. +func TestShutdownStopWaitEndsWithCallerContext(t *testing.T) { + sb := newFakeSandbox() + sb.stopBlocks = true + t.Cleanup(func() { close(sb.released) }) + + s := newTestService(sb) + + const budget = time.Second + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + start := time.Now() + err := s.shutdown(ctx) + elapsed := time.Since(start) + + require.Error(t, err) + assert.GreaterOrEqual(t, elapsed, budget, + "the VM stop should wait out the caller's context, not give up early") + // Generous upper bound: the point is that it stops waiting, not that it is + // prompt to the millisecond. + assert.Less(t, elapsed, budget+5*time.Second, + "the VM stop should not outlive the caller's context") +} + +// TestShutdownStopsVMWhenBudgetAlreadyExhausted covers the case where an +// earlier phase (a container IO shutdown waiting out the whole budget on a hung +// guest) leaves no time on the clock. Stopping the VM is what releases the +// bundle's disk files, so it must still be attempted with a live context rather +// than skipped — otherwise the VM survives and wedges the bundle, which is the +// failure the bounded shutdown exists to prevent. +func TestShutdownStopsVMWhenBudgetAlreadyExhausted(t *testing.T) { + sb := newFakeSandbox() + s := newTestService(sb) + + // An already-expired budget. + ctx, cancel := context.WithTimeout(context.Background(), -time.Second) + defer cancel() + + require.NoError(t, s.shutdown(ctx)) + + select { + case <-sb.stopCalled: + default: + t.Fatal("sb.Stop was not called when the shutdown budget was already spent") + } + assert.NoError(t, sb.stopCtxErr, "sb.Stop must not be handed an already-cancelled context") +} + +func TestShutdownSucceedsWhenVMStops(t *testing.T) { + sb := newFakeSandbox() + s := newTestService(sb) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + require.NoError(t, s.shutdown(ctx)) + + select { + case ev := <-s.events: + assert.Nil(t, ev) + default: + t.Error("shutdown sentinel was not sent") + } +} + +func TestShutdownPropagatesVMStopError(t *testing.T) { + sb := newFakeSandbox() + sb.stopErr = errors.New("boom") + s := newTestService(sb) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + err := s.shutdown(ctx) + require.Error(t, err) + assert.ErrorContains(t, err, "boom") +} + +// TestShutdownSentinelDoesNotBlockOnFullChannel guards the other way shutdown +// used to be able to wedge: an unconditional send on a full event channel with +// no forwarder draining it. +func TestShutdownSentinelDoesNotBlockOnFullChannel(t *testing.T) { + sb := newFakeSandbox() + s := newTestService(sb) + + // Fill the event channel so the sentinel send cannot proceed. + for len(s.events) < cap(s.events) { + s.events <- struct{}{} + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + done := make(chan struct{}) + go func() { + defer close(done) + _ = s.shutdown(ctx) + }() + + select { + case <-done: + case <-time.After(eventSentinelTimeout + 3*time.Second): + t.Fatal("service.shutdown blocked sending the sentinel on a full event channel") + } +} + +// TestShutdownWaitsIndefinitelyWithoutDeadline documents the consequence of +// deferring to the caller: given a context with no deadline, the VM stop waits. +// The shim framework always supplies one, so this is the contract, not a hazard. +func TestShutdownWaitsIndefinitelyWithoutDeadline(t *testing.T) { + sb := newFakeSandbox() + sb.stopBlocks = true + + s := newTestService(sb) + + done := make(chan struct{}) + go func() { + defer close(done) + _ = s.shutdown(context.Background()) + }() + + select { + case <-done: + t.Fatal("shutdown returned while sb.Stop was still blocked and no deadline was set") + case <-time.After(500 * time.Millisecond): + } + + // Releasing Stop lets it finish, proving it was waiting on Stop and nothing else. + close(sb.released) + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("shutdown did not return after sb.Stop was released") + } +} diff --git a/pkg/shim/manager/manager_unix.go b/pkg/shim/manager/manager_unix.go index 81b3301d..b785e379 100644 --- a/pkg/shim/manager/manager_unix.go +++ b/pkg/shim/manager/manager_unix.go @@ -37,6 +37,7 @@ import ( "github.com/containerd/containerd/v2/pkg/namespaces" "github.com/containerd/containerd/v2/pkg/shim" "github.com/containerd/errdefs" + "github.com/containerd/log" "golang.org/x/sys/unix" ) @@ -292,11 +293,28 @@ func (manager) Start(ctx context.Context, bparams *bootapi.BootstrapParams) (_ * }, nil } +const ( + // Bounds the wait for the shim to release its shim.pid lock after SIGKILL, + // leaving slack inside containerd's "io.containerd.timeout.shim.cleanup" + // (5s by default), which bounds the whole `shim delete` binary call. Being + // killed by containerd instead would report no exit status at all. + shimExitWaitTimeout = 3 * time.Second + + shimExitPollInterval = 50 * time.Millisecond +) + func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { - pid, err := waitForShimPidLock() + pid, exited, err := waitForShimPidLock(ctx) if err != nil { return shim.StopStatus{}, err } + if !exited { + log.G(ctx).WithFields(log.Fields{ + "pid": pid, + "id": id, + "timeout": shimExitWaitTimeout, + }).Error("shim process did not exit before the wait budget expired; it may still hold bundle files") + } return shim.StopStatus{ ExitedAt: time.Now(), ExitStatus: 128 + int(unix.SIGKILL), @@ -304,46 +322,89 @@ func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { }, nil } -// waitForShimPidLock opens shim.pid, reads the shim PID, and blocks until the -// shim process releases the exclusive flock it holds on that file (i.e. until -// the shim exits). It returns the PID on success. +// waitForShimPidLock SIGKILLs the shim if it is still running and waits for it +// to release the exclusive flock on shim.pid, which the OS does only once it +// exits. It reports the shim's PID and whether that exit was observed. // -// The shim is the only process that acquires the lock, so after SIGKILL it will -// inevitably exit and release it. Blocking unconditionally — rather than racing -// against a timeout — guarantees the caller does not return while the shim is -// still alive. -func waitForShimPidLock() (int, error) { +// The shim is the only process that takes the lock, so after SIGKILL it will +// almost always exit promptly; the wait is bounded anyway, and reports a shim +// that outlives its budget rather than blocking on it. See [shimExitWaitTimeout]. +func waitForShimPidLock(ctx context.Context) (int, bool, error) { f, err := os.Open("shim.pid") if err != nil { - return 0, err + return 0, false, err } defer f.Close() p, err := io.ReadAll(f) if err != nil { - return 0, err + return 0, false, err } pid, err := strconv.Atoi(strings.TrimSpace(string(p))) if err != nil { - return 0, err + return 0, false, err + } + // Reject non-positive pids before they reach Kill: kill(0) signals every + // process in the caller's process group and kill(-n) a whole other group, + // so a truncated or zeroed shim.pid would take down the `shim delete` + // invocation (and whatever else shares its group) instead of the shim. + if pid <= 0 { + return 0, false, fmt.Errorf("invalid shim pid %d in shim.pid", pid) + } + + // tryLock reports whether the lock is now free, i.e. the shim has exited. + tryLock := func() (bool, error) { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return true, nil + } + if errors.Is(err, syscall.EWOULDBLOCK) { + return false, nil + } + return false, fmt.Errorf("flock shim.pid: %w", err) } // Try a non-blocking acquire first. If it succeeds the shim has already // exited and the lock is free. - if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err == nil { - return pid, nil - } else if !errors.Is(err, syscall.EWOULDBLOCK) { - return 0, fmt.Errorf("flock shim.pid: %w", err) + free, err := tryLock() + if err != nil { + return 0, false, err + } + if free { + return pid, true, nil } - // Lock is held — shim is still running. Kill it and block until the OS - // releases the lock on shim exit. There is no timeout: the shim cannot - // hold the lock after it dies, and returning early would leave it alive. + // Lock is held — the shim is still running. Kill it, then poll for the + // lock rather than blocking on LOCK_EX, so the wait stays bounded. if kerr := unix.Kill(pid, unix.SIGKILL); kerr != nil && !errors.Is(kerr, unix.ESRCH) { - return 0, fmt.Errorf("kill shim: %w", kerr) + return 0, false, fmt.Errorf("kill shim: %w", kerr) + } + + deadline := time.Now().Add(shimExitWaitTimeout) + if dl, ok := ctx.Deadline(); ok && dl.Before(deadline) { + deadline = dl } - if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { - return 0, fmt.Errorf("flock shim.pid (wait): %w", err) + for { + free, err := tryLock() + if err != nil { + return 0, false, err + } + if free { + return pid, true, nil + } + + remaining := time.Until(deadline) + if remaining <= 0 { + return pid, false, nil + } + wait := shimExitPollInterval + if remaining < wait { + wait = remaining + } + select { + case <-ctx.Done(): + return pid, false, nil + case <-time.After(wait): + } } - return pid, nil } diff --git a/pkg/shim/manager/manager_windows.go b/pkg/shim/manager/manager_windows.go index 4979f48d..96052976 100644 --- a/pkg/shim/manager/manager_windows.go +++ b/pkg/shim/manager/manager_windows.go @@ -38,6 +38,8 @@ import ( "github.com/containerd/containerd/v2/pkg/shim" "github.com/containerd/log" "golang.org/x/sys/windows" + + "github.com/containerd/nerdbox/internal/erofs" ) func newCommand(ctx context.Context, id, containerdAddress, containerdTTRPCAddress string, debug bool) (*exec.Cmd, error) { @@ -164,6 +166,17 @@ const ( shimPipeReadyTimeout = 10 * time.Second shimPipeDialPerAttempt = 1 * time.Second shimPipeRetryDelay = 10 * time.Millisecond + + // Stop waits for the shim to exit and then clears the bundle. The sum of + // the two budgets must leave slack inside containerd's + // "io.containerd.timeout.shim.cleanup" (5s by default), which bounds the + // whole `shim delete` binary call: if containerd kills us instead, the + // deferred bundle cleanup never runs at all. + shimExitWaitTimeout = 2 * time.Second + bundleRemoveWindow = 1 * time.Second + + shimExitPollInterval = 50 * time.Millisecond + bundleRemoveRetryDelay = 200 * time.Millisecond ) // waitForShimPipe polls a named pipe address with a short per-attempt DialPipe timeout @@ -263,23 +276,111 @@ func bundlePath(ctx context.Context) string { return "" } -// removeRootfs removes the rootfs directory from the bundle so that -// containerd's bundle cleanup doesn't attempt a bind filter unmount. -// On Windows, Unmount calls bindfilter.RemoveFileBinding which fails with -// ERROR_ACCESS_DENIED on directories that were never bind filter mounts -// (nerdbox uses VM-based virtio block devices instead). Removing the -// directory makes UnmountAll a no-op. -func removeRootfs(ctx context.Context) { - if bp := bundlePath(ctx); bp != "" { - os.RemoveAll(filepath.Join(bp, "rootfs")) +// removeBundleArtifacts removes everything the shim itself put in the bundle +// directory, leaving containerd's own bundle cleanup nothing to trip over. Two +// Windows failure modes make it necessary: Unmount calls +// bindfilter.RemoveFileBinding, which fails with ERROR_ACCESS_DENIED on a rootfs +// that was never a bind filter mount (nerdbox uses virtio block devices +// instead), and a VMDK extent still mapped by the VM cannot be unlinked at all — +// see [erofs.IsBundleArtifact]. +func removeBundleArtifacts(ctx context.Context) { + bp := bundlePath(ctx) + if bp == "" { + return + } + + targets := []string{filepath.Join(bp, "rootfs")} + entries, err := os.ReadDir(bp) + if err != nil { + log.G(ctx).WithError(err).WithField("bundle", bp). + Error("failed to list bundle directory; shim-written artifacts may be left behind") + } + for _, entry := range entries { + if erofs.IsBundleArtifact(entry.Name()) { + targets = append(targets, filepath.Join(bp, entry.Name())) + } + } + + // A shim being terminated can hold its mappings for a moment after + // TerminateProcess returns, so retry — but over the whole remaining set + // under one deadline. Retrying per target would multiply by the target + // count and could push this call past containerd's cleanup timeout, which + // is the very failure being fixed. + deadline := time.Now().Add(bundleRemoveWindow) + if dl, ok := ctx.Deadline(); ok && dl.Before(deadline) { + deadline = dl + } + + var failures map[string]error + for { + var remaining []string + failures = make(map[string]error, len(targets)) + for _, target := range targets { + if err := os.RemoveAll(target); err != nil { + failures[target] = err + remaining = append(remaining, target) + } + } + targets = remaining + if len(targets) == 0 || time.Until(deadline) <= bundleRemoveRetryDelay { + break + } + time.Sleep(bundleRemoveRetryDelay) + } + + // Name the survivors: this is the file that will wedge subsequent starts. + for _, target := range targets { + log.G(ctx).WithError(failures[target]).WithField("path", target). + Error("failed to remove bundle artifact; containerd bundle cleanup and subsequent starts of this container will fail until it is released") + } +} + +// waitForProcessExit blocks until the process handle h is signalled, ctx is +// done, or timeout elapses — whichever comes first. It reports whether the +// process exited. +// +// The wait is polled rather than a single WaitForSingleObject(INFINITE) so it +// can honour ctx: containerd bounds the `shim delete` binary call, and being +// killed mid-call would skip our deferred bundle cleanup. +func waitForProcessExit(ctx context.Context, h windows.Handle, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + if dl, ok := ctx.Deadline(); ok && dl.Before(deadline) { + deadline = dl + } + + for { + remaining := time.Until(deadline) + if remaining <= 0 { + return false + } + if err := ctx.Err(); err != nil { + return false + } + + wait := shimExitPollInterval + if remaining < wait { + wait = remaining + } + // WaitForSingleObject returns WAIT_OBJECT_0 (0) when the process has + // exited and WAIT_TIMEOUT when the interval elapsed first. + event, err := windows.WaitForSingleObject(h, uint32(wait.Milliseconds())) + if err != nil { + return false + } + if event == uint32(windows.WAIT_OBJECT_0) { + return true + } } } func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { // must run on all exits (including when the process is already gone) - // to ensure containerd's bundle cleanup is successful. See [removeRootfs] - // for more details. - defer removeRootfs(ctx) + // to ensure containerd's bundle cleanup is successful. See + // [removeBundleArtifacts] for more details. Every wait below is bounded so + // that this defer actually gets to run — containerd kills the `shim delete` + // call once "io.containerd.timeout.shim.cleanup" expires, and a killed + // process runs no defers. + defer removeBundleArtifacts(ctx) p, err := os.ReadFile(filepath.Join(bundlePath(ctx), "shim.pid")) if err != nil { @@ -326,11 +427,24 @@ func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { return shim.StopStatus{}, fmt.Errorf("terminate shim process: %w", err) } - // Block until the process has fully exited. There is no timeout: the - // shim is the only target and TerminateProcess is unconditional, so - // WaitForSingleObject will always complete. - if _, err := windows.WaitForSingleObject(h, windows.INFINITE); err != nil { - return shim.StopStatus{}, fmt.Errorf("wait for shim process: %w", err) + // Wait for the process to fully exit, but only for a bounded period. + // TerminateProcess is not instantaneous: a VM-backed shim can have threads + // parked in kernel-mode hypervisor or memory-mapping calls, and the process + // does not die until those return. containerd bounds this whole binary call + // by "io.containerd.timeout.shim.cleanup" (5s), so waiting forever means + // being killed and skipping the bundle cleanup deferred above — which is + // what leaves a locked VMDK extent behind and wedges the container id. + // + // Report success either way: containerd proceeds to delete the bundle + // regardless of what we return here, so the useful thing is to finish under + // our own control with the cleanup done, and to say loudly when the shim + // outlived us. + if !waitForProcessExit(ctx, h, shimExitWaitTimeout) { + log.G(ctx).WithFields(log.Fields{ + "pid": pid, + "id": id, + "timeout": shimExitWaitTimeout, + }).Error("shim process did not exit before the wait budget expired; it may still hold bundle files") } return shim.StopStatus{ diff --git a/pkg/shim/manager/manager_windows_stop_test.go b/pkg/shim/manager/manager_windows_stop_test.go new file mode 100644 index 00000000..bea95596 --- /dev/null +++ b/pkg/shim/manager/manager_windows_stop_test.go @@ -0,0 +1,335 @@ +//go:build windows + +/* + 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 manager + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/containerd/containerd/v2/pkg/shim" + "golang.org/x/sys/windows" +) + +// bundleCtx returns a context carrying bundle path dir, the way the shim +// framework supplies it from the -bundle flag. +func bundleCtx(dir string) context.Context { + return context.WithValue(context.Background(), shim.OptsKey{}, shim.Opts{BundlePath: dir}) +} + +// seedBundle populates dir with the artifacts the shim writes into a bundle, +// plus a file that must survive cleanup. It returns the paths that must be +// removed and the paths that must remain. +func seedBundle(t *testing.T, dir string) (removed, kept []string) { + t.Helper() + + rootfs := filepath.Join(dir, "rootfs") + if err := os.MkdirAll(filepath.Join(rootfs, "nested"), 0o755); err != nil { + t.Fatalf("mkdir rootfs: %v", err) + } + removed = append(removed, rootfs) + + for _, name := range []string{ + "merged_fs_gpt.vmdk", + "merged_fs_gpt_header.bin", + "merged_fs_gpt_pad.bin", + "merged_fs_b.vmdk", + } { + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + removed = append(removed, p) + } + + // containerd owns these; cleanup must not touch them. + for _, name := range []string{"config.json", "shim.pid", "bootstrap.json"} { + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte("1"), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + kept = append(kept, p) + } + + return removed, kept +} + +func assertBundleCleaned(t *testing.T, removed, kept []string) { + t.Helper() + for _, p := range removed { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("expected %s to be removed, stat err = %v", filepath.Base(p), err) + } + } + for _, p := range kept { + if _, err := os.Stat(p); err != nil { + t.Errorf("expected %s to survive cleanup: %v", filepath.Base(p), err) + } + } +} + +// TestRemoveBundleArtifacts verifies that cleanup removes the rootfs directory +// and every VMDK descriptor and auxiliary blob the shim wrote, and nothing else. +// A single leftover descriptor is enough to make containerd's bundle removal +// fail and wedge the container id, so the coverage here is deliberately exact. +func TestRemoveBundleArtifacts(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + removeBundleArtifacts(bundleCtx(dir)) + + assertBundleCleaned(t, removed, kept) +} + +func TestRemoveBundleArtifactsNoBundlePath(t *testing.T) { + // Must not panic or touch the working directory when the context carries + // no bundle path. + removeBundleArtifacts(context.Background()) +} + +// TestRemoveBundleArtifactsBudgetIsGlobal guards the sum that matters: cleanup +// must stay inside bundleRemoveWindow no matter how many artifacts are locked, +// because Stop's total (shim wait + cleanup) has to fit inside containerd's 5s +// cleanup timeout. Retrying per-target instead would multiply by target count. +func TestRemoveBundleArtifactsBudgetIsGlobal(t *testing.T) { + dir := t.TempDir() + + // Many artifacts, all of them undeletable: a directory that is not empty + // and whose child is held open cannot be removed on Windows. + var held []*os.File + for _, name := range []string{ + "merged_fs_gpt.vmdk", "merged_fs_gpt_header.bin", "merged_fs_gpt_pad.bin", + "merged_fs_a.vmdk", "merged_fs_b.vmdk", "merged_fs_c.vmdk", + } { + p := filepath.Join(dir, name) + f, err := os.OpenFile(p, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + t.Fatalf("create %s: %v", name, err) + } + held = append(held, f) + } + t.Cleanup(func() { + for _, f := range held { + f.Close() + } + }) + + start := time.Now() + removeBundleArtifacts(bundleCtx(dir)) + elapsed := time.Since(start) + + // Generous headroom for slow CI, but far below targets × window. + if limit := bundleRemoveWindow + 2*time.Second; elapsed > limit { + t.Errorf("cleanup took %s, beyond the %s bound", elapsed, limit) + } + if shimExitWaitTimeout+bundleRemoveWindow >= 5*time.Second { + t.Errorf("shimExitWaitTimeout+bundleRemoveWindow = %s, which does not fit inside containerd's 5s cleanup timeout", + shimExitWaitTimeout+bundleRemoveWindow) + } +} + +// startSleeper launches a process that stays alive for the duration of the +// test and returns its pid and an open handle with terminate/synchronize +// rights. Cleanup kills it. +func startSleeper(t *testing.T) (int, windows.Handle) { + t.Helper() + + cmd := exec.Command("ping", "-n", "60", "127.0.0.1") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + pid := cmd.Process.Pid + + t.Cleanup(func() { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + }) + + h, err := windows.OpenProcess(windows.PROCESS_TERMINATE|windows.SYNCHRONIZE, false, uint32(pid)) + if err != nil { + t.Fatalf("OpenProcess(%d): %v", pid, err) + } + t.Cleanup(func() { windows.CloseHandle(h) }) + + return pid, h +} + +// TestWaitForProcessExitTimeout is the core regression test: a process that +// does not exit must not hold the wait open. Before the fix this was +// WaitForSingleObject(INFINITE), so containerd killed the whole `shim delete` +// call at its 5s cleanup timeout and the deferred bundle cleanup never ran. +func TestWaitForProcessExitTimeout(t *testing.T) { + _, h := startSleeper(t) + + const budget = 300 * time.Millisecond + start := time.Now() + exited := waitForProcessExit(context.Background(), h, budget) + elapsed := time.Since(start) + + if exited { + t.Error("expected waitForProcessExit to report the process as still running") + } + if elapsed < budget { + t.Errorf("returned after %s, before the %s budget elapsed", elapsed, budget) + } + if elapsed > budget+2*time.Second { + t.Errorf("returned after %s, far beyond the %s budget", elapsed, budget) + } +} + +func TestWaitForProcessExitProcessExits(t *testing.T) { + cmd := exec.Command("cmd.exe", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + pid := cmd.Process.Pid + + h, err := windows.OpenProcess(windows.PROCESS_TERMINATE|windows.SYNCHRONIZE, false, uint32(pid)) + if err != nil { + // The process may already have exited and been reaped; nothing to wait on. + t.Skipf("OpenProcess(%d): %v", pid, err) + } + defer windows.CloseHandle(h) + defer func() { _, _ = cmd.Process.Wait() }() + + if !waitForProcessExit(context.Background(), h, shimExitWaitTimeout) { + t.Error("expected waitForProcessExit to observe the process exiting") + } +} + +// TestWaitForProcessExitCtxCancelled verifies the wait honours context +// cancellation, which is how containerd signals that the cleanup budget is +// spent. +func TestWaitForProcessExitCtxCancelled(t *testing.T) { + _, h := startSleeper(t) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + if waitForProcessExit(ctx, h, shimExitWaitTimeout) { + t.Error("expected waitForProcessExit to report failure on a cancelled context") + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("took %s on a pre-cancelled context; expected a prompt return", elapsed) + } +} + +// TestStopCleansBundleWhenShimAlreadyGone covers the common path: the shim +// exited and removed its own pid file, so Stop has nothing to terminate but +// must still clear the bundle artifacts. +func TestStopCleansBundleWhenShimAlreadyGone(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + // Drop the pid file so Stop takes the "already exited" branch. + pidFile := filepath.Join(dir, "shim.pid") + if err := os.Remove(pidFile); err != nil { + t.Fatalf("remove shim.pid: %v", err) + } + kept = filterOut(kept, pidFile) + + status, err := manager{}.Stop(bundleCtx(dir), "test-id") + if err != nil { + t.Fatalf("Stop: %v", err) + } + if status.ExitStatus != 128+9 { + t.Errorf("ExitStatus = %d, want %d", status.ExitStatus, 128+9) + } + + assertBundleCleaned(t, removed, kept) +} + +// TestStopCleansBundleWhenPidIsStale covers a stale pid file pointing at a pid +// that is no longer in the process table: Stop must still succeed and clean up +// rather than erroring out and leaving the bundle behind. +func TestStopCleansBundleWhenPidIsStale(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + // Start and reap a process so its pid is almost certainly gone. + cmd := exec.Command("cmd.exe", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + pid := cmd.Process.Pid + _, _ = cmd.Process.Wait() + + if err := os.WriteFile(filepath.Join(dir, "shim.pid"), + []byte(strconv.Itoa(pid)), 0o644); err != nil { + t.Fatalf("write shim.pid: %v", err) + } + + if _, err := (manager{}).Stop(bundleCtx(dir), "test-id"); err != nil { + t.Fatalf("Stop: %v", err) + } + + assertBundleCleaned(t, removed, kept) +} + +// TestStopTerminatesLiveShimAndCleansBundle verifies the full path: a live +// process is terminated, waited for, and the bundle is cleaned. +func TestStopTerminatesLiveShimAndCleansBundle(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + cmd := exec.Command("ping", "-n", "60", "127.0.0.1") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + t.Cleanup(func() { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + }) + + if err := os.WriteFile(filepath.Join(dir, "shim.pid"), + []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil { + t.Fatalf("write shim.pid: %v", err) + } + + start := time.Now() + status, err := manager{}.Stop(bundleCtx(dir), "test-id") + elapsed := time.Since(start) + if err != nil { + t.Fatalf("Stop: %v", err) + } + if status.Pid != cmd.Process.Pid { + t.Errorf("Pid = %d, want %d", status.Pid, cmd.Process.Pid) + } + // The process is killable, so this must not approach the wait budget. + if elapsed > shimExitWaitTimeout { + t.Errorf("Stop took %s, beyond the %s wait budget", elapsed, shimExitWaitTimeout) + } + + assertBundleCleaned(t, removed, kept) +} + +func filterOut(paths []string, drop string) []string { + out := make([]string, 0, len(paths)) + for _, p := range paths { + if p != drop { + out = append(out, p) + } + } + return out +}