Deep Review: 20260416-120302-pr-316

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

// set up the container controller with a webhook which prevents all modification.
func (c *controller) setupWebhookWithRuntimeConfig(mgr ctrl.Manager) error {
	mgr.GetLogger().Info("Setting up container webhook")
	mutatingConfig := base.WebhookConfig[*v1alpha1.Container]{
		Name:        "container-mutating",
		WebhookName: "container-mutating.containers.rancherdesktop.io",
		WebhookPort: c.webhookPort,
		Operations: []admissionregistrationv1.OperationType{
			admissionregistrationv1.Create,
			admissionregistrationv1.Update,
		},
		Validator: &immutableValidator{},
	}

	managers, err := base.SetupWebhookForResource(mgr, &v1alpha1.Container{}, mutatingConfig)
	if err != nil {
		return err

// container_webhooks.go
func (c *immutableValidator) ValidateCreate(context.Context, *v1alpha1.Container) (warnings ctrlwebhookadmission.Warnings, err error) {
    return nil, errors.New("webhook does not implement create")
}
// ValidateUpdate
if raw, ok := newContainer.Annotations[v1alpha1.AnnotationAction]; ok {
    if !v1alpha1.ContainerAction(raw).IsValid() {
        return nil, fmt.Errorf("invalid %s annotation %q", v1alpha1.AnnotationAction, raw)
    }
}

The webhook registers only for Update, so ValidateUpdate is the only enforcer of the action enum. A client that POSTs a hand-written Container with metadata.annotations["containers.rancherdesktop.io/action"]=bogus passes admission because ValidateCreate is never invoked. .spec is now optional (container_types.go:233-238), and the CRD no longer requires spec at the top level (crd.yaml:270-275, crd.yaml:493-500), so the barrier to a hand-written Container is lower than before the PR. The reconciler's !action.IsValid() fallback at docker_watcher.go:350 catches invalid values, but a valid action on a hand-written mirror would be dispatched against whatever container ID was put in metadata.name, bypassing the documented ContainerCreateRequest flow. The new BATS coverage at engine-docker.bats:366-387 only exercises the Update-reject path.

	// Metadata is a standard object metadata
	//
	// +optional
	metav1.ObjectMeta `json:"metadata,omitempty,omitzero"`

	// Spec is reserved for future use. The Container API has no
	// desired-state fields today: actions are requested via the
	// AnnotationAction annotation on metadata instead.
	//
	// +optional
	Spec ContainerSpec `json:"spec,omitempty,omitzero"`

	// Status defines the observed state of Container
	//
	// +optional

// processContainerAction handles a container carrying the AnnotationAction
// annotation. It dispatches the Docker call, records the outcome in
// status.lastAction, and then removes the annotation.
//
// The Docker call runs before the status and metadata patches so that a
// crash mid-flight leaves the annotation in place and the next reconcile
// replays the action. Start, stop, pause, and unpause are idempotent
// against a container already in the target state, so replay is safe.
// Restart has no target state to match: a replay sends SIGTERM and waits
// the grace period a second time, which the controller cannot distinguish
// from a deliberate re-request.

}

@test "invalid action annotation is rejected by the webhook" {
    # An unknown action value must fail admission so the reconciler never
    # sees a value it cannot process. Without this gate, a bad value would
    # fail to persist in status.lastAction (the CRD enum rejects it) and
    # the annotation would stay in place, wedging the retry loop.
    run_e -0 docker run -d --name test-invalid-action busybox sleep inf
    cid=${output}

    rdd ctl wait --for=jsonpath='{.status.status}'=running \
        --namespace="${RDD_NAMESPACE}" container/"${cid}" --timeout=30s

    run -1 rdd ctl annotate container "${cid}" --namespace="${RDD_NAMESPACE}" --overwrite \
        "containers.rancherdesktop.io/action=bogus"
    assert_output --partial "invalid"

    # The rejected request leaves status.lastAction untouched.
    run -0 rdd ctl get container "${cid}" --namespace="${RDD_NAMESPACE}" \
        -o jsonpath='{.status.lastAction}'
    refute_output

    docker rm --force test-invalid-action
}

@test "action annotation on create is rejected by the webhook" {
    # The engine watcher creates Container mirrors and never sets the
    # action annotation. Reject any hand-written create that carries one,
    # so it cannot drive a Docker action against its metadata.name.
    run -1 rdd ctl apply -f - <<EOF
apiVersion: containers.rancherdesktop.io/v1alpha1
kind: Container

Fix: register the validator for Create as well and implement ValidateCreate symmetrically with ValidateUpdate. If direct Container creates should be rejected outright, keep the current errors.New("webhook does not implement create") body and gate the engine reconciler's own apply path (it uses client.ForceOwnership with engine-controller as field owner, which bypasses admission for controller-initiated applies only if configured that way). Add a BATS test that rdd ctl apply -f of a hand-written Container with a bogus action annotation fails admission on create.

			return fmt.Errorf("container %s is not running", id)
		}
		if !paused {
			return nil
		}
		log.Info("Unpausing container", "id", id)
		_, err = w.cli.ContainerUnpause(ctx, id, dockerclient.ContainerUnpauseOptions{})
		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
// failure instead of a silent success.
func (w *dockerWatcher) containerRunState(ctx context.Context, id string) (running, paused bool, err error) {
	inspect, err := w.cli.ContainerInspect(ctx, id, dockerclient.ContainerInspectOptions{})

If the Container mirror is deleted (by cleanupMirrorResources, a concurrent docker rm destroy event, or a user delete) between dispatchContainerAction and these helpers, the Get returns NotFound. retry.RetryOnConflict does not retry on NotFound, so the error propagates up through processContainerAction (line 371) into the joined error at reconcileContainerActions:374. The controller then requeues and logs "failed to patch lastAction: NotFound" for a container that no longer exists. The same log message fires for genuine conflict-exhaustion failures, so the noise misdirects a debugger.

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

Fix: treat NotFound as success on both the Get and the Patch, in both patchContainerLastAction and removeActionAnnotation.

 func (w *dockerWatcher) patchContainerLastAction(ctx context.Context, id string, lastAction *containersv1alpha1.ContainerLastAction) error {
     key := client.ObjectKey{Name: id, Namespace: w.apiNamespace}
     return retry.RetryOnConflict(retry.DefaultRetry, func() error {
         var latest containersv1alpha1.Container
         if err := w.k8s.Get(ctx, key, &latest); err != nil {
-            return err
+            return client.IgnoreNotFound(err)
         }
         patch := client.MergeFromWithOptions(latest.DeepCopy(), client.MergeFromWithOptimisticLock{})
         latest.Status.LastAction = lastAction
-        return w.k8s.Status().Patch(ctx, &latest, patch)
+        return client.IgnoreNotFound(w.k8s.Status().Patch(ctx, &latest, patch))
     })
 }

	// 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,
		ObservedAt:  observedAt,
		CompletedAt: metav1.Now(),

patchContainerLastAction writes the status subresource, which bumps the object's resourceVersion. removeActionAnnotation then re-Gets via the cached client and patches with client.MergeFromWithOptimisticLock{}, which attaches the Got resourceVersion to the payload. Between the two writes, the informer has not yet received the watch event for the status bump, so the Get returns the stale pre-status version; the subsequent patch then 409s. retry.RetryOnConflict re-runs the closure but re-reads the same cache, so successive attempts keep seeing stale data until the informer catches up. If the cache does not sync within retry.DefaultRetry's ~310ms budget, removeActionAnnotation returns the error, processContainerAction propagates it, and the next reconcile replays the whole Docker action because the annotation is still present. For start/stop/pause/unpause the replay is a no-op (Docker returns 304 or the pre-check skips); for restart the replay is a real double-restart.

Fix: thread the fresh Container object through from patchContainerLastAction into removeActionAnnotation. The status Patch updates the passed-in object in place with the new resourceVersion, so removeActionAnnotation can skip its initial Get on the happy path and patch against the known-fresh version.

-func (w *dockerWatcher) patchContainerLastAction(ctx context.Context, id string, lastAction *containersv1alpha1.ContainerLastAction) error {
+func (w *dockerWatcher) patchContainerLastAction(ctx context.Context, id string, lastAction *containersv1alpha1.ContainerLastAction) (*containersv1alpha1.Container, error) {
     key := client.ObjectKey{Name: id, Namespace: w.apiNamespace}
+    var latest containersv1alpha1.Container
-    return retry.RetryOnConflict(retry.DefaultRetry, func() error {
-        var latest containersv1alpha1.Container
+    err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
         if err := w.k8s.Get(ctx, key, &latest); err != nil {
             return err
         }
         patch := client.MergeFromWithOptions(latest.DeepCopy(), client.MergeFromWithOptimisticLock{})
         latest.Status.LastAction = lastAction
         return w.k8s.Status().Patch(ctx, &latest, patch)
     })
+    return &latest, err
 }

Note: preserving the current observed != current check in removeActionAnnotation is important for the concurrent-rewrite path (I1 design strength in Claude's and Gemini's observations). Threading the fresh object through removes the cache-race on the common path without losing that guard; if the threaded object shows the annotation still equal to observed, the Patch uses its resourceVersion.

	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
// failure instead of a silent success.
func (w *dockerWatcher) containerRunState(ctx context.Context, id string) (running, paused bool, err error) {
	inspect, err := w.cli.ContainerInspect(ctx, id, dockerclient.ContainerInspectOptions{})
	if err != nil {
		return false, false, err
	}
	return inspect.Container.State.Running, inspect.Container.State.Paused, nil
}

// patchContainerLastAction writes status.lastAction with retry-on-conflict
// and returns the updated Container. The main engine sync writes the
// status subresource on every reconcile, so this write races against it.
// If the mirror is deleted concurrently, any step may return NotFound;
// the caller treats a nil Container as "nothing left to do".
func (w *dockerWatcher) patchContainerLastAction(ctx context.Context, id string, lastAction *containersv1alpha1.ContainerLastAction) (*containersv1alpha1.Container, error) {
	key := client.ObjectKey{Name: id, Namespace: w.apiNamespace}
	var result *containersv1alpha1.Container
	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
		var latest containersv1alpha1.Container
		if err := w.k8s.Get(ctx, key, &latest); err != nil {
			if apierrors.IsNotFound(err) {
				return nil
			}

| --------- | ------------------- |
| `start`   | `ContainerStart`    |
| `stop`    | `ContainerStop`     |
| `pause`   | `ContainerPause`    |
| `unpause` | `ContainerUnpause`  |
| `restart` | `ContainerRestart`  |

A single annotation holds at most one pending action. Writing a new
value replaces the old, so a user who requests `pause` and then
`unpause` before the reconciler has run will see only the unpause. No
queue, no accumulating history.

The engine calls Docker before patching `status.lastAction` and
removing the annotation, so a crash mid-flight leaves the annotation

	State ContainerActionState `json:"state"`
	// Error is the error message if the action failed.
	//
	// +optional
	Error string `json:"error,omitempty"`
	// ObservedAt is when the reconciler observed the action annotation.
	// Backlog may delay this relative to the user's write time.
	//
	// +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"`

requestedAt := metav1.Now()

	}
	return client.IgnoreNotFound(w.k8s.Delete(ctx, latest))
}

// processContainerAction handles a container carrying the AnnotationAction
// annotation. It dispatches the Docker call, records the outcome in
// status.lastAction, and then removes the annotation.
//
// The Docker call runs before the status and metadata patches so that a
// crash mid-flight leaves the annotation in place and the next reconcile
// replays the action. Start, stop, pause, and unpause are idempotent
// against a container already in the target state, so replay is safe.
// Restart has no target state to match: a replay sends SIGTERM and waits
// the grace period a second time, which the controller cannot distinguish
// from a deliberate re-request.
func (w *dockerWatcher) processContainerAction(ctx context.Context, c *containersv1alpha1.Container) error {
	raw, ok := c.Annotations[containersv1alpha1.AnnotationAction]
	if !ok {

| `restart` | `ContainerRestart`  |

A single annotation holds at most one pending action. Writing a new
value replaces the old, so a user who requests `pause` and then
`unpause` before the reconciler has run will see only the unpause. No
queue, no accumulating history.

The engine calls Docker before patching `status.lastAction` and
removing the annotation, so a crash mid-flight leaves the annotation
in place and the next reconcile replays the action. Start, stop,
pause, and unpause are idempotent against a container already in the
target state, so replay is safe. Restart has no target state to match:
a replay sends SIGTERM and waits the grace period a second time, which
the controller cannot distinguish from a deliberate re-request.

If the Docker call fails (for example, `pause` on a container that is
not running), the reconciler still removes the annotation and records

# api_containers.md:246-248
# Restart has no target state to match,
# so a replay after a crash runs a second restart; the double restart
# is harmless.

// AnnotationAction requests a one-shot action on a container. The reconciler
// performs the action, records the outcome in status.lastAction, and removes
// the annotation. Setting a new value replaces any pending action — there is
// no queue.
const AnnotationAction = "containers.rancherdesktop.io/action"

// +kubebuilder:validation:Enum=start;stop;pause;unpause;restart

// ContainerAction is the name of an action that can be requested on a
// container via the AnnotationAction annotation.

-var AnnotationAction = SchemeGroupVersion.Group + "/action"
+const AnnotationAction = "containers.rancherdesktop.io/action"

    rdd ctl annotate container "${cid}" --namespace="${RDD_NAMESPACE}" --overwrite \
        "containers.rancherdesktop.io/action=${action}"
    try --max 30 --delay 1 -- assert_action_consumed "${cid}"
}

assert_last_action() {
    local cid=$1 action=$2 state=$3
    run -0 rdd ctl get container "${cid}" --namespace="${RDD_NAMESPACE}" \
        -o jsonpath='{.status.lastAction.action}={.status.lastAction.state}'
    assert_output "${action}=${state}"
}

@test "stop action stops a running container" {
    run_e -0 docker run -d --name test-state busybox sleep inf
    cid=${output}

    rdd ctl wait --for=jsonpath='{.status.status}'=running \
        --namespace="${RDD_NAMESPACE}" container/"${cid}" --timeout=30s

 assert_last_action() {
     local cid=$1 action=$2 state=$3
     run -0 rdd ctl get container "${cid}" --namespace="${RDD_NAMESPACE}" \
-        -o jsonpath='{.status.lastAction.action}'
-    assert_output "${action}"
-    run -0 rdd ctl get container "${cid}" --namespace="${RDD_NAMESPACE}" \
-        -o jsonpath='{.status.lastAction.state}'
-    assert_output "${state}"
+        -o jsonpath='{.status.lastAction.action}={.status.lastAction.state}'
+    assert_output "${action}=${state}"
 }