fix: fix incorrect restart=always restart logic

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

The problem

When running a container with the --restart=always policy, if the container is manually stopped (e.g., via nerdctl stop), containerd would incorrectly restart the container after a few seconds. This behavior differs from Docker, where a manually stopped container with restart=always should not restart until the daemon itself is restarted.

Why it's non-trivial

A naive fix might only check the restart policy, ignoring whether the container was explicitly stopped by the user. Additionally, handling the transition between daemon restarts and user-initiated stops requires careful tracking and label management to avoid race conditions and ensure correct behavior across restarts.

The approach

1. Short-circuit restart if container was explicitly stopped

The core logic in Reconcile is updated to immediately return false if the container's ExplicitlyStoppedLabel is set:

+	explicitlyStopped, _ := strconv.ParseBool(labels[ExplicitlyStoppedLabel])
+	if explicitlyStopped {
+		return false
+	}

Annotation:
This ensures that for any restart policy, if the container was explicitly stopped by the user, it won't be restarted by the monitor.

2. Simplify and correct restart policy handling

The restart policy switch is changed so that both "always" and "unless-stopped" are handled together, and the explicit stop check for "unless-stopped" is removed (now handled above):

-	case "", "always":
+	case "always", "unless-stopped":
 		return true

Annotation:
This prevents duplicate logic and ensures that the explicit stop check applies consistently.

3. Reset explicitly stopped marker on daemon startup for "always" containers

On monitor startup, the code resets the ExplicitlyStoppedLabel to false for containers with the "always" policy:

+			// This function is executed exactly once.
+			m.resetAlwaysContainerExplicitlyStoppedLabel()

The reset function:

+func (m *monitor) resetAlwaysContainerExplicitlyStoppedLabel() {
+	ns, err := m.client.NamespaceService().List(context.Background())
+	if err != nil {
+		log.G(context.Background()).WithError(err).Errorf("failed to get container namespace")
+		return
+	}
+	for _, name := range ns {
+		ctx := namespaces.WithNamespace(context.Background(), name)
+		containers, err := m.client.Containers(ctx, fmt.Sprintf("labels.%q", restart.StatusLabel))
+		if err != nil {
+			log.G(ctx).WithError(err).Error("failed to get containers")
+			continue
+		}
+		for _, c := range containers {
+			labels, err := c.Labels(ctx)
+			if err != nil {
+				log.G(ctx).WithError(err).Errorf("failed to get container %s label", c.ID())
+				continue
+			}
+			rp, err := restart.NewPolicy(labels[restart.PolicyLabel])
+			if err != nil {
+				log.G(ctx).WithError(err).Error("failed to parse restart policy")
+				continue
+			}
+			if rp.Name() == "always" {
+				explicitlyStopped, _ := strconv.ParseBool(labels[restart.ExplicitlyStoppedLabel])
+				if explicitlyStopped {
+					if err := resetContainerExplicitlyStoppedLabel(ctx, c); err != nil {
+						log.G(ctx).WithError(err).Error("failed to reset explicitly-stopped label")
+					}
+				}
+			}
+		}
+	}
+}

Annotation:
This ensures that containers with "always" policy are eligible for restart after a daemon restart, even if they were previously stopped by the user.

4. Add integration test for "always" policy

A new integration test is added to verify the correct behavior for restart=always:

+func TestContainerRestartPolicyAlways(t *testing.T) {
+	...
+	// Start container, kill it, ensure it restarts
+	// Set explicitly stopped label, kill again, ensure it does not restart
+}

Annotation:
This test covers the scenario where a container should restart after a crash, but not after being explicitly stopped.

From the review thread

Copilot: Containers() / Labels() / NewPolicy() errors are logged but then execution continues, and rp.Name() is called even when NewPolicy returns an error. This can both (a) incorrectly treat a container as always when labels couldn't be read (empty policy defaults to always) and (b) panic on invalid policy strings (nil rp). Also, the result of resetContainerExplicitlyStoppedLabel is ignored.

Copilot: Reconcile now short-circuits on ExplicitlyStoppedLabel for all policies (including always and on-failure), but the unit tests currently only cover the explicitly-stopped behavior for unless-stopped. Adding test cases for always (and ideally on-failure) with ExplicitlyStoppedLabel=true would prevent regressions in this updated behavior.

Takeaways

• Always check for explicit user actions (like manual stops) before applying automated recovery or restart logic.
• When updating restart logic, ensure tests cover all policy variants and user-interaction edge cases to prevent regressions.
• On system or daemon restarts, ensure that any stateful flags (like "explicitly stopped") are reset or reconciled as needed to match the intended policy semantics.