From fd29ff1073de96f6c50ee2bdf92ae23edac2b567 Mon Sep 17 00:00:00 2001 From: Austin Vazquez Date: Wed, 12 Aug 2026 19:38:34 -0500 Subject: [PATCH] fix(runtime): bound shim loading with the load timeout loadShim runs during runtime plugin initialization and issued its task RPCs on the caller's context, which carries no deadline. A shim that still owns its socket but has stopped serving, for example one leaked by a previous containerd whose shutdown timed out, accepts the connection and then never answers. containerd then never finishes starting and its API socket is never created. Give loadShim a single loadTimeout budget covering the whole load. Nested timeouts can only shorten a deadline, so the load stays bounded however many calls it makes. Reaping a shim we give up on runs on its own cleanup budget, detached from the caller so that a cancelled load cannot leave the shim behind. A failed delete returns before shutting the shim down and closing its client, so both are done here, and the failure is now propagated: delete removes the bundle only when it succeeds, and returning an error lets loadShims remove it, so a shim that cannot be reaped is no longer reloaded on every subsequent start. Co-authored-by: Harshal Patel <106813066+HarshalPatel1972@users.noreply.github.com> Signed-off-by: Austin Vazquez --- core/runtime/v2/shim.go | 35 +++++++++++++++++++++++++++++++ core/runtime/v2/shim_load.go | 18 +++++++++++----- core/runtime/v2/shim_load_test.go | 9 ++++++++ core/runtime/v2/task_manager.go | 15 +------------ 4 files changed, 58 insertions(+), 19 deletions(-) diff --git a/core/runtime/v2/shim.go b/core/runtime/v2/shim.go index 3909de66c0f29..b88c9222aae53 100644 --- a/core/runtime/v2/shim.go +++ b/core/runtime/v2/shim.go @@ -186,6 +186,41 @@ func cleanupAfterDeadShim(ctx context.Context, id string, rt *runtime.NSMap[Shim }) } +// cleanupShimTask reaps a shim task we have given up on, after a failed start or +// when loading a bundle left behind by a previous containerd. An unresponsive +// shim must not block the caller — on the load path that would stall containerd +// startup — so each call is bounded, and detached from the caller's context, +// which by then may be cancelled or out of budget. +// +// A failed delete returns before shutting the shim down and closing its client, +// so both are done here. It also leaves the bundle in place (only a successful +// delete removes it), so callers that own one must remove it on error. The shim +// map is untouched: callers reach this having already removed the task, or never +// added it. +func cleanupShimTask(ctx context.Context, st *shimTask, sandboxed bool) error { + dctx, cancel := timeout.WithContext(context.WithoutCancel(ctx), cleanupTimeout) + defer cancel() + + _, err := st.delete(dctx, sandboxed, func(context.Context, string) {}) + if err == nil { + return nil + } + + // Shutting down needs a context with time left on it. Check the deadline + // rather than the error: a timeout only survives as context.DeadlineExceeded + // over GRPC. Over TTRPC it arrives as the raw context error, which carries no + // GRPC status, so errgrpc.ToNative flattens it into errdefs.ErrUnknown. + if dctx.Err() != nil { + dctx, cancel = timeout.WithContext(context.WithoutCancel(ctx), cleanupTimeout) + defer cancel() + } + + st.Shutdown(dctx) + st.Close() + + return err +} + // CurrentShimVersion is the latest shim version supported by containerd (e.g. TaskService v3). const CurrentShimVersion = 3 diff --git a/core/runtime/v2/shim_load.go b/core/runtime/v2/shim_load.go index 3401596c81f49..80bdea8ae5d45 100644 --- a/core/runtime/v2/shim_load.go +++ b/core/runtime/v2/shim_load.go @@ -131,6 +131,13 @@ func (m *ShimManager) loadShim(ctx context.Context, bundle *Bundle) error { id = bundle.ID ) + // One budget for the whole load: shims are loaded during plugin + // initialization, so a shim that never answers would otherwise stall + // containerd startup. Nested timeouts can only shorten a deadline, so this + // bounds the load however many calls it makes. + ctx, cancel := timeout.WithContext(ctx, loadTimeout) + defer cancel() + // If we're on 1.6+ and specified custom path to the runtime binary, path will be saved in 'shim-binary-path' file. if data, err := os.ReadFile(filepath.Join(bundle.Path, "shim-binary-path")); err == nil { runtime = string(data) @@ -175,7 +182,7 @@ func (m *ShimManager) loadShim(ctx context.Context, bundle *Bundle) error { m.shims.Delete(ctx, id) }) if err != nil { - cleanupAfterDeadShim(ctx, id, m.shims, m.events, binaryCall) + cleanupAfterDeadShim(context.WithoutCancel(ctx), id, m.shims, m.events, binaryCall) return fmt.Errorf("unable to load shim %q: %w", id, err) } @@ -195,7 +202,11 @@ func (m *ShimManager) loadShim(ctx context.Context, bundle *Bundle) error { logEntry = logEntry.WithError(pidErr) } logEntry.Info("cleaning leaked shim process") - shim.delete(ctx, false, func(ctx context.Context, id string) {}) + if err := cleanupShimTask(ctx, shim, false); err != nil && !errdefs.IsNotFound(err) { + // Returning an error makes loadShims remove the bundle; a shim we + // cannot reap would otherwise be reloaded on every start. + return fmt.Errorf("failed to clean up leaked shim %q: %w", id, err) + } } else { if pidErr != nil { log.G(ctx).WithField("id", id).WithError(pidErr).Warn("failed to query shim pids, keeping shim registered") @@ -225,9 +236,6 @@ func loadShimTask(ctx context.Context, bundle *Bundle, onClose func()) (_ *shimT return nil, err } - ctx, cancel := timeout.WithContext(ctx, loadTimeout) - defer cancel() - if _, err := s.PID(ctx); err != nil { if !errdefs.IsNotImplemented(err) { return nil, err diff --git a/core/runtime/v2/shim_load_test.go b/core/runtime/v2/shim_load_test.go index 7be1b0b4f6eb5..0a8e02292e21d 100644 --- a/core/runtime/v2/shim_load_test.go +++ b/core/runtime/v2/shim_load_test.go @@ -17,6 +17,7 @@ package v2 import ( + "context" "errors" "testing" @@ -78,6 +79,14 @@ func TestShouldCleanupShim(t *testing.T) { PInfo: nil, Expected: false, }, + { + // Not answering in time is not proof of a dead shim. + Name: "not a sandbox, pids lookup times out", + SgetErr: errdefs.ErrNotFound, + PidErr: context.DeadlineExceeded, + PInfo: nil, + Expected: false, + }, } for _, tc := range testCases { diff --git a/core/runtime/v2/task_manager.go b/core/runtime/v2/task_manager.go index e5cf9f7d40710..03e1f65ce906b 100644 --- a/core/runtime/v2/task_manager.go +++ b/core/runtime/v2/task_manager.go @@ -253,20 +253,7 @@ func (m *TaskManager) Create(ctx context.Context, taskID string, opts runtime.Cr // NOTE: ctx contains required namespace information. m.manager.shims.Delete(ctx, taskID) - dctx, cancel := timeout.WithContext(context.WithoutCancel(ctx), cleanupTimeout) - defer cancel() - - sandboxed := opts.SandboxID != "" - _, errShim := shimTask.delete(dctx, sandboxed, func(context.Context, string) {}) - if errShim != nil { - if errdefs.IsDeadlineExceeded(errShim) { - dctx, cancel = timeout.WithContext(context.WithoutCancel(ctx), cleanupTimeout) - defer cancel() - } - - shimTask.Shutdown(dctx) - shimTask.Close() - } + _ = cleanupShimTask(ctx, shimTask, opts.SandboxID != "") return nil, fmt.Errorf("failed to create shim task: %w", err) }