[pull] main from containerd:main

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
 

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

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 {

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)
 	}