runtime: invoke shim.Delete when connection is closed

Project: containerd/containerd | Language: Go | Concepts: error-handling, observability, testing-strategy | Difficulty: mid
Link: https://github.com/containerd/containerd/pull/13309

The problem

When a containerd shim process is killed (e.g., via SIGKILL), containerd was not reliably cleaning up its internal state for the associated container. This could leave behind orphaned state (such as leaked task entries and runc state directories), which in turn could cause resource leaks and prevent containers from being managed correctly.

Why it's non-trivial

A naive fix might only clean up state when a shim exits cleanly or when a specific error is returned. However, if the shim is killed abruptly (such as with SIGKILL), the normal cleanup paths (including RPC-based deletion) cannot run, and the task entry may remain in containerd's runtime state. The fix must ensure cleanup occurs even on unexpected disconnects, and must do so in a way that is robust to context cancellation and avoids race conditions.

The approach

1. Add on-disconnect cleanup callback to shim loading

The PR updates the code that loads a shim to provide a callback function that runs when the shim disconnects (e.g., due to being killed):

-		shim, err := loadShim(ctx, bundle, func() {})
+		runtimePath, err := m.resolveRuntimePath(opts.Runtime)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve runtime path: %w", err)
+		}
+		b := shimBinary(bundle, shimBinaryConfig{
+			runtime:      runtimePath,
+			address:      m.containerdAddress,
+			ttrpcAddress: m.containerdTTRPCAddress,
+			socketDir:    m.socketDir,
+			env:          m.env,
+		})
+		shim, err := loadShim(ctx, bundle, func() {
+			log.G(ctx).WithField("id", id).Info("shim disconnected")
+
+			cleanupAfterDeadShim(context.WithoutCancel(ctx), id, m.shims, m.events, b)
+			// Remove self from the runtime task list. Even though the cleanupAfterDeadShim()
+			// would publish taskExit event, the shim.Delete() will always fail with ttrpc
+			// disconnect and there is no chance to remove this dead task from runtime task lists.
+			// Thus it's better to delete it here.
+			m.shims.Delete(ctx, id)
+		})

What this does and why:

• When the shim disconnects, the callback:
  • Logs the disconnect.
  • Calls cleanupAfterDeadShim to handle event publishing and other cleanup.
  • Deletes the task entry from the runtime's internal shims map, ensuring no orphaned state remains.
• Uses context.WithoutCancel(ctx) to avoid issues if the original context is already canceled, ensuring cleanup runs reliably.

2. Add a regression test for orphaned shim state

A new integration test is added to verify that killing the shim causes containerd to publish task exit and delete events, and that the container is fully cleaned up:

+// Regression test for https://github.com/containerd/containerd/issues/13293 (orphaned shim state after shim SIGKILL).
+// This test asserts that containerd publishes task exit and delete events for a container when the shim disconnects.
+func TestKillShimPublishesTaskExitAndDeleteEventsForContainer(t *testing.T) {
+	ctx := namespaces.WithNamespace(t.Context(), "k8s.io")
+	...
+	shimCli := connectToShim(ctx, t, containerdEndpoint, 3, sbID)
+	shimPID := shimPid(ctx, t, shimCli)
+
+	eventCh, eventErrCh := containerdClient.EventService().Subscribe(ctx,
+		fmt.Sprintf(`topic=="%s",event.container_id=="%s"`, ctrdruntime.TaskExitEventTopic, cnID),
+		fmt.Sprintf(`topic=="%s",event.container_id=="%s"`, ctrdruntime.TaskDeleteEventTopic, cnID),
+	)
+
+	require.NoError(t, syscall.Kill(int(shimPID), syscall.SIGKILL))
+	waitForTaskExitDeleteEvents(t, cnID, eventCh, eventErrCh, 10*time.Second)
+
+	// Wait for the container to exit and ensure it is fully cleaned up.
+	require.NoError(t, Eventually(func() (bool, error) {
+		s, err := runtimeService.ContainerStatus(cnID)
+		if err != nil {
+			return false, err
+		}
+		if s.State == runtime.ContainerState_CONTAINER_EXITED {
+			return true, nil
+		}
+		return false, nil
+	}, 1000*time.Millisecond, 30*time.Second))
+}

What this does and why:

• Simulates a real-world scenario where the shim is killed.
• Subscribes to task exit and delete events to ensure containerd observes and handles the disconnect.
• Waits for the container to be reported as exited, verifying that cleanup logic works as intended.

From the review thread

cpuguy83: Can't we just call the closer on error? Do we even need to?

ningmingxiao: it should called after shim is killed to clean the leak container If I understand correctly @cpuguy83

ningmingxiao: we should call closefunc after ttrpc server is closed (which means shim is exited)

func (c *Client) Close() error {
	c.closeOnce.Do(func() {
		c.closed()

		c.conn.Close()
	})
	return nil
}

I can't find a good way to call closer directly @cpuguy83

Takeaways

• Always handle cleanup for abnormal process termination: Don't rely solely on clean shutdown paths; ensure that disconnects or abrupt exits (e.g., SIGKILL) also trigger resource cleanup.
• Use non-cancelable contexts for critical cleanup: When cleaning up global or shared state, avoid using contexts that may have been canceled by the time cleanup runs.
• Regression tests should simulate real failure scenarios: Integration tests that kill processes and verify event emission help catch leaks that only occur on abnormal shutdowns, not just graceful exits.