metadata: bound snapshotter Remove during garbage collection

Project: containerd/containerd | Language: Go | Concepts: garbage-collection, error-handling, observability | Difficulty: mid
Link: https://github.com/containerd/containerd/pull/13799

The problem

During snapshot garbage collection (GC), the system calls the snapshotter's Remove method while holding a write lock on the metadata store. This call had no timeout and stripped any cancellation from the context. If the snapshotter hangs (for example, due to a bug or resource exhaustion), the lock is held indefinitely. This blocks all snapshot operations (such as starting new containers), but does not affect readiness checks, so the node appears healthy while being unable to start new pods. This situation was observed in production, causing nodes to be stuck for hours.

Why it's non-trivial

A naive fix might simply add a timeout to the Remove call, but:

• The timeout must be configurable and consistent with other timeouts in the codebase.
• The system must handle what happens after a timeout: it should abandon the rest of the GC pass to avoid multiple stuck calls, and avoid making further unbounded calls (like Cleanup) to the same unresponsive snapshotter.
• The fix must not break existing error-handling or GC logic, and should be tested for both hanging and failed-precondition cases.

The approach

1. Introduce a new configurable timeout for GC snapshotter Remove:

+const (
+	gcSnapshotterRemoveTimeoutKey     = "io.containerd.timeout.gc.snapshotter.remove"
+	defaultGCSnapshotterRemoveTimeout = 30 * time.Minute
+)
+
+func init() {
+	timeout.Set(gcSnapshotterRemoveTimeoutKey, defaultGCSnapshotterRemoveTimeout)
+}

This defines a new timeout key and default (30 minutes) for the GC Remove operation, and registers it at initialization. This makes the timeout configurable via the standard mechanism used elsewhere in containerd.

2. Bound the Remove call with the new timeout:

-		if err := s.Snapshotter.Remove(ctx, node.info.Name); err != nil {
+		removeCtx, cancel := timeout.WithContext(ctx, gcSnapshotterRemoveTimeoutKey)
+		err := s.Snapshotter.Remove(removeCtx, node.info.Name)
+		cancel()
+		if err != nil {

This wraps the Remove call in a context with the configured timeout, ensuring that the lock cannot be held indefinitely. The explicit cancel() ensures timers are released promptly, especially important since pruneBranch is recursive.

3. Update documentation to include the new timeout key:

+  "io.containerd.timeout.gc.snapshotter.remove" = "30m"

The new timeout is documented in the example configuration, making it discoverable for operators.

4. Add targeted tests for hanging and failed-precondition Remove:

A test snapshotter is implemented that hangs on Remove, and the test ensures that:

• The Remove call is bounded and fails with DeadlineExceeded.
• Only one Remove attempt occurs before the pass is abandoned.
• Cleanup is not called after a timed-out Remove.
• Orphaned snapshots remain for the next GC pass.

Example from the test:

func (s *hangingRemoveSnapshotter) Remove(ctx context.Context, key string) error {
	s.removes.Add(1)
	<-ctx.Done()
	return ctx.Err()
}
...
if n := sn.removes.Load(); n != 1 {
	t.Fatalf("expected 1 remove attempt before abandoning the pass, got %d", n)
}
if n := sn.cleanups.Load(); n != 0 {
	t.Fatalf("expected no Cleanup call after abandoned pass, got %d", n)
}

This ensures the new logic works as intended and is robust to regression.

From the review thread

Copilot: This test mutates the global timeout registry and restores it to a hard-coded 5m. Because pkg/timeout is process-global and go test runs packages in parallel, this can leak state across tests and also becomes brittle if the default is ever changed. Capture the previous value with timeout.Get and restore that via t.Cleanup.

NahumLitvin: Kept cancel() non-deferred deliberately: pruneBranch is recursive, and deferring would keep every ancestor frame's timer alive until the whole branch unwinds; the explicit cancel releases each timer as soon as its Remove returns.

Takeaways

• Always bound long-running or potentially blocking operations with a timeout, especially when holding critical locks.
• Make operational timeouts configurable and document them clearly, so operators can tune behavior for their environments.
• When adding timeouts, consider the error-handling flow: ensure that timeouts abort further dependent actions and leave the system in a recoverable state.
• Write targeted tests for both the normal and failure paths (including timeouts), especially when dealing with concurrency or resource cleanup.
• Release resources (like timers or contexts) as soon as possible, especially in recursive or iterative code, to avoid leaks or excessive resource usage.