Deep Review: 20260416-144208-pr-316

	// no-ops when nothing changed, so stable reconciles pay nothing.
	if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
		return ctrl.Result{}, err
	}

	// Two passes run per Reconcile: reconcileContainerActions drives
	// Docker actions from the action annotation, and
	// processFinalizers forwards K8s-side deletes to Docker for any
	// mirror still carrying the mirror finalizer.
	//
	// Both passes issue List calls per reconcile, and every
	// Container/Image/Volume watch event triggers a reconcile — so
	// cost is O(N) per child event. The long-term fix is to split
	// these into per-resource reconcilers with watch predicates

The function was renamed to reconcileContainerActions in this PR (called at line 199) and Container.spec.state no longer exists. The rest of the block (lines 188-197) was updated to reference the new "action annotation present" watch predicate, but lines 183-184 were missed. A reader following this comment is pointed at code that was deleted in the same commit. Both Claude and Codex flagged this; Codex also included container_controller.go:81 (see S3) under the same theme.

		Scheme:   mgr.GetScheme(),
		Recorder: mgr.GetEventRecorder(ControllerName + "-controller"),
	}).SetupWithManager(mgr)
}

// setupWebhookWithRuntimeConfig registers a webhook that enforces an immutable
// spec, rejects action annotations on create, and validates action annotations
// on update.
func (c *controller) setupWebhookWithRuntimeConfig(mgr ctrl.Manager) error {
	mgr.GetLogger().Info("Setting up container webhook")
	mutatingConfig := base.WebhookConfig[*v1alpha1.Container]{

	// Two passes run per Reconcile: reconcileContainerActions drives
	// Docker actions from the action annotation, and
	// processFinalizers forwards K8s-side deletes to Docker for any
	// mirror still carrying the mirror finalizer.
	//
	// Both passes issue List calls per reconcile, and every
	// Container/Image/Volume watch event triggers a reconcile — so
	// cost is O(N) per child event. The long-term fix is to split
	// these into per-resource reconcilers with watch predicates
	// (deletionTimestamp set, action annotation present).
	//
	// Run both passes unconditionally and join their errors. Early
	// return on actionsErr would stall every mirror's finalizer behind
	// a single stuck container action: the next reconcile would hit
	// the same broken container first and skip finalizers again.
	var actionsErr, finErr error
	if err := r.reconcileContainerActions(ctx); err != nil {
		actionsErr = fmt.Errorf("failed to reconcile container actions: %w", err)
	}
	if err := r.processFinalizers(ctx); err != nil {
		finErr = fmt.Errorf("failed to process finalizers: %w", err)
	}

Fix:

-	// Two passes run per Reconcile: reconcileContainerSpecs drives
-	// Docker start/stop from Container.spec.state, and
+	// Two passes run per Reconcile: reconcileContainerActions drives
+	// Docker actions from the action annotation, and
 	// processFinalizers forwards K8s-side deletes to Docker for any
 	// mirror still carrying the mirror finalizer.

	// IPv4 and IPv6 bindings.
	//
	// +required
	Bindings []ContainerPortBinding `json:"bindings"`
}

// ContainerSpec is currently empty. Actions are requested via the AnnotationAction
// annotation, not a level-triggered desired-state field. Docker's restart
// policy and out-of-band `docker start/stop` are competing writers, so a
// level-triggered desired state would fight them.
type ContainerSpec struct{}

// ContainerStatus defines the observed state of the container.
type ContainerStatus struct {
	// Name of the container; this is distinct from the container ID.
	//

Line 124 predates the PR but is now directly contradicted by lines 125-128 (added by the PR): the struct is not "the configuration the container was created with," it is intentionally empty. A reader hitting the first line is told to expect created-time configuration. The PR extended the doc but did not rewrite the opener.

Fix: delete or rewrite the first line so the opener matches the rest.

-// ContainerSpec defines the configuration the container was created with.
-// Container spec is empty today: actions are requested via the AnnotationAction
+// ContainerSpec is currently empty. Actions are requested via the AnnotationAction
 // annotation, not a level-triggered desired-state field. Docker's restart
 // policy and out-of-band `docker start/stop` are competing writers, so a
 // level-triggered desired state would fight them.
 type ContainerSpec struct{}

		Scheme:   mgr.GetScheme(),
		Recorder: mgr.GetEventRecorder(ControllerName + "-controller"),
	}).SetupWithManager(mgr)
}

// setupWebhookWithRuntimeConfig registers a webhook that enforces an immutable
// spec, rejects action annotations on create, and validates action annotations
// on update.
func (c *controller) setupWebhookWithRuntimeConfig(mgr ctrl.Manager) error {
	mgr.GetLogger().Info("Setting up container webhook")
	mutatingConfig := base.WebhookConfig[*v1alpha1.Container]{

After this PR, ValidateCreate allows creates without the action annotation, and ValidateUpdate allows updates that add or change a valid action annotation. The webhook no longer "prevents all modification" — it enforces a spec-immutability rule and a valid-action rule. Rewrite the comment to describe what the webhook actually does.

	// The webhook rejects invalid action values, but one written while the
	// webhook is offline can still reach storage. Drop such values here;
	// otherwise the CRD enum rejects the status.lastAction write, the
	// annotation stays in place, and every reconcile retries forever.
	if !action.IsValid() {
		log.Info("Dropping invalid container action annotation", "id", c.Name, "action", raw)
		return w.removeActionAnnotation(ctx, c, raw)
	}

	dockerErr := w.dispatchContainerAction(ctx, log, c.Name, action)

	lastAction := &containersv1alpha1.ContainerLastAction{
		Action:      action,

The webhook normally rejects invalid values at admission, so this branch only fires when the webhook is bypassed (failurePolicy: Ignore, or the startup window before registration). When it does fire, the caller gets no feedback: lastAction is untouched and the annotation disappears. An Info log is invisible to a GUI client. Two options: raise the log to Error with a clear message, or emit a Kubernetes Event on the Container so a UI can surface it. Low priority — the webhook catches the normal case — but worth doing because the dropped-annotation branch is specifically the "webhook was bypassed" path, which is exactly when observability matters.

	State ContainerActionState `json:"state"`
	// Error is the error message if the action failed.
	//
	// +optional
	Error string `json:"error,omitempty"`
	// ObservedAt is when the reconciler began processing the action annotation.
	// Backlog may delay this relative to the user's write time, and dispatch
	// (especially restart's grace period) extends the gap to CompletedAt.
	//
	// +optional
	ObservedAt metav1.Time `json:"observedAt,omitempty,omitzero"`
	// CompletedAt is when the action completed, regardless of outcome.
	//
	// +optional
	CompletedAt metav1.Time `json:"completedAt,omitempty,omitzero"`

In processContainerAction, observedAt := metav1.Now() is captured after the annotation has already been seen via List, just before the IsValid check and dispatch. CompletedAt is captured after dispatchContainerAction returns. If a caller computes CompletedAt - ObservedAt to measure user-visible latency, it will get the dispatch duration (which includes a multi-second grace period on restart or stop), not the observe-to-process backlog. Either rename the field to StartedAt/ProcessingStartedAt, or clarify the Godoc to "when the reconciler began processing the annotation." Keep the design doc text (docs/design/api_containers.md:160-161) in sync.

  # direct `docker stop`) in between. The `error` field is set only
  # when `state` is `Failed`.
  lastAction:
    action: pause
    state: Succeeded
    observedAt: "2026-04-15T10:30:00Z"
    completedAt: "2026-04-15T10:30:00Z"
```

### Container state

`status.status` always reflects the actual Docker state. The engine

		return nil
	})
	return result, err
}

// removeActionAnnotation clears the AnnotationAction annotation only if
// its value still matches the action just processed. A concurrent writer
// may replace the annotation with a different action between dispatch
// and cleanup; that new value must survive so the next reconcile picks
// it up. Callers pass either a fresh Container from patchContainerLastAction
// (which bypasses the informer cache, not yet showing the preceding status
// write) or a cached one (which may 409 on the first Patch). On conflict,
// the retry re-reads from the cache; by then it has usually caught up.
func (w *dockerWatcher) removeActionAnnotation(ctx context.Context, latest *containersv1alpha1.Container, observed string) error {
	firstAttempt := true
	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
		if !firstAttempt {
			if err := w.k8s.Get(ctx, client.ObjectKeyFromObject(latest), latest); err != nil {

The !action.IsValid() branch in processContainerAction passes c directly from the cache-backed List in reconcileContainerActions, not a fresh Container from patchContainerLastAction. Functionally the RetryOnConflict loop still works (its second attempt re-reads on conflict), but the comment's claim that "the caller passes a fresh Container" is not universally true. Either do a direct (non-cache) Get in the invalid-action branch before calling removeActionAnnotation, or soften the comment to "callers may pass either a fresh Container or a cached one; the retry loop tolerates cache lag."

		return err
	case containersv1alpha1.ContainerActionRestart:
		log.Info("Restarting container", "id", id)
		_, err := w.cli.ContainerRestart(ctx, id, dockerclient.ContainerRestartOptions{})
		return err
	}
	return fmt.Errorf("unknown container action %q", action)
}

// containerRunState reports whether Docker currently shows the container as
// running and as paused. Pause uses paused to skip a no-op dispatch; unpause
// also checks running so that unpause on a stopped container reports a

IsValid and the webhook both gate input, so this path is unreachable under normal operation. The comment at lines 406-408 correctly notes that the default branch only triggers when a new ContainerAction constant is added without a matching switch case. A tiny unit test that constructs a value not in the switch and asserts the error message would catch future drift between the enum, IsValid, and this switch. Low priority.

	}
	return nil
}

// dispatchContainerAction executes the Docker call for a single action. The
// caller pre-validates the action name; the default branch triggers only
// when a new ContainerAction value is added to the type but not to the switch.
//
// Pause and unpause pre-check the current Docker state and return nil when
// the container is already in the target state. Two reconcile ticks can read
// the same action annotation through the informer cache before the cache
// sees the removal that follows the first dispatch — the pre-check keeps
// the second dispatch from flipping lastAction to Failed. Start, stop, and