Deep Review: 20260412-203409-pr-295

		return w.k8s.Update(ctx, latest)
	})
	if retryErr != nil {
		return fmt.Errorf("failed to remove finalizer from %s: %w", name, retryErr)
	}
	deleteTarget := obj.DeepCopyObject().(client.Object)
	deleteTarget.SetName(name)
	deleteTarget.SetNamespace(apiNamespace)
	return client.IgnoreNotFound(w.k8s.Delete(ctx, deleteTarget))
}

// reconcileContainerState checks the Container's spec.state against the
// observed status and dispatches the Docker action that bridges them.
// The engine creates Containers with spec.state="unknown", which the
// reconciler ignores.
//
// Full state matrix (rows=desired from the spec.state enum, columns=actual
// from Docker). The webhook restricts desired to {unknown, created,
// running}; actual can be any value in the CRD enum.
//
//	desired \ actual  | running | paused   | created | exited | dead | pausing/restarting | removing | unknown
//	------------------+---------+----------+---------+--------+------+--------------------+----------+--------
//	running           | nil     | unpause  | start   | start  | start| start              | start    | start
//	created           | stop    | stop     | nil     | nil    | nil  | stop               | stop     | stop
//
// ContainerStop handles paused/restarting containers natively, so the
// created branch always dispatches Stop from any non-stopped state. The
// running branch handles paused explicitly because Docker rejects
// ContainerStart on a paused container. isStopped() treats
// created/exited/dead as terminal stopped states: the user's intent
// ("do not run") is already satisfied and redispatching Stop would be
// wasted work on every unrelated reconcile event.
func (w *dockerWatcher) reconcileContainerState(ctx context.Context, c *containersv1alpha1.Container) error {
	desired := c.Spec.State
	if desired == containersv1alpha1.ContainerStatusUnknown {
		return nil
	}

	actual := c.Status.Status
	log := logf.FromContext(ctx).WithName("docker-watcher")

	switch desired {
	case containersv1alpha1.ContainerStatusRunning:
		if actual == containersv1alpha1.ContainerStatusRunning {
			return nil
		}
		if actual == containersv1alpha1.ContainerStatusPaused {

After the user patches spec.state=created on a running container, ContainerStop transitions the container to Docker's exited state (not created), and applyContainer writes status.status=exited at sync_containers.go:157. On the next reconcile — triggered by any Container/Image/Volume watch event via SetupWithManager's unconditional enqueueApp mapping — desired == "created" and actual == "exited", so the desired != actual check at line 341 falls through and dispatches ContainerStop again. Docker's ContainerStop on a non-running container returns NotModified which the client treats as success, so no error propagates — but the reconciler never reaches a quiescent state for this container. Cost scales with the rate of unrelated watch events times the number of created-desired stuck containers. The same issue applies to actual == "dead" or actual == "created" when desired == "created".

		finalizerOnly := containersv1alpha1apply.Container(inspect.ID, apiNamespace).
			WithFinalizers(mirrorFinalizer)
		if err := w.k8s.Apply(ctx, finalizerOnly,
			client.ForceOwnership, client.FieldOwner(finalizerFieldOwner)); err != nil {
			return fmt.Errorf("failed to reassert container finalizer %s: %w", inspect.ID, err)
		}
	}

	// Build status.
	statusApply := containersv1alpha1apply.ContainerStatus().
		WithName(name).

The new BATS test at engine.bats:231-248 only verifies the one-shot transition to exited and does not observe the subsequent churn, so this isn't caught by CI.

        "${image_ref}" --timeout=30s
}

# --- Container state transitions via spec ---

@test "patching spec.state=created stops Docker container" {
    docker run -d --name test-state busybox sleep 3600

    run -0 docker inspect test-state --format '{{.Id}}'
    cid=${output}

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

    rdd ctl patch container "${cid}" --namespace="${NAMESPACE}" \
        --type=merge -p '{"spec":{"state":"created"}}'

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

    run -0 docker inspect test-state --format '{{.State.Status}}'
    assert_output "exited"
}

@test "patching spec.state=running restarts Docker container" {
    run -0 docker inspect test-state --format '{{.Id}}'
    cid=${output}

Fix: treat any stopped state (created, exited, dead) as satisfying desired=created, symmetric with the unpause handling in the Running branch at lines 354-363.

 func (w *dockerWatcher) reconcileContainerState(ctx context.Context, c *containersv1alpha1.Container) error {
     desired := c.Spec.State
     if desired == containersv1alpha1.ContainerStatusUnknown {
         return nil
     }

     actual := c.Status.Status
-    if desired == actual {
-        return nil
-    }
+    // Treat any stopped state as satisfying desired=created.
+    if desired == containersv1alpha1.ContainerStatusCreated {
+        switch actual {
+        case containersv1alpha1.ContainerStatusCreated,
+            containersv1alpha1.ContainerStatusExited,
+            containersv1alpha1.ContainerStatusDead:
+            return nil
+        }
+    }
+    if desired == actual {
+        return nil
+    }

(Gemini flagged this as Critical on the grounds of "Docker API spam loop"; Claude and Codex called it Important. Calibrated as Important here: the loop is bounded by unrelated event frequency, NotModified is a silent success, and no error propagates — the cost is wasted RPCs, not a hang or failure.)

		if err := r.Update(ctx, limaVM); err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to propagate running state to LimaVM: %w", err)
		}
		return ctrl.Result{}, nil
	}

	// Mirror LimaVM status conditions onto the App status. The engine
	// reconciler writes ContainerEngineReady on the same object, so the
	// App's resourceVersion from the initial Get can be stale by the time
	// we write. retry.RetryOnConflict + re-Get matches the pattern in
	// EngineReconciler.setEngineCondition so concurrent writers no longer
	// 409-loop through controller-runtime requeue. Truncate messages
	// defensively: the LimaVM controller already truncates at source, but
	// guarding here ensures a future bypass can't cause the App status
	// update to fail CRD validation.
	if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
		latest := &v1alpha1.App{}
		if err := r.Get(ctx, client.ObjectKeyFromObject(&app), latest); err != nil {
			return err
		}
		changed := false
		for _, cond := range limaVM.Status.Conditions {
			changed = apimeta.SetStatusCondition(&latest.Status.Conditions, metav1.Condition{
				Type:               cond.Type,
				Status:             cond.Status,
				Reason:             cond.Reason,
				Message:            base.TruncateConditionMessage(cond.Message),
				ObservedGeneration: latest.Generation,
				LastTransitionTime: cond.LastTransitionTime,
			}) || changed
		}
		if !changed {
			return nil

This PR adds a second writer to App.Status.Conditions via setEngineCondition (engine_reconciler.go:223-240), which correctly wraps its update in retry.RetryOnConflict with a re-Get. The existing AppReconciler still does not: the comment at line 230-231 claims the resourceVersion from the initial Get is still current, which was true when the App controller was the sole writer but is no longer true once the engine reconciler can land a ContainerEngineReady update in between the Get and the Update. The repo overview explicitly calls this out: *"App.Status.Conditions has multiple concurrent writers (AppReconciler mirrors LimaVM conditions; EngineReconciler writes ContainerEngineReady). Flag any non-SSA, non-retry-on-conflict Status().Update on the App object."*

	if w != nil {
		w.stop()
	}
}

// setEngineCondition updates the ContainerEngineReady condition on the
// App resource. The App controller also writes App.Status.Conditions
// (to mirror LimaVM conditions) and controller-runtime does not
// serialize reconciles across controllers, so a naive Update can race
// and 409. retry.RetryOnConflict plus a re-Get inside the loop is the
// same pattern used elsewhere in this file (see removeMirrorResource,
// deleteAllOfType).
func (r *EngineReconciler) setEngineCondition(ctx context.Context, app *appv1alpha1.App, status metav1.ConditionStatus, reason, message string) error {
	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
		latest := &appv1alpha1.App{}
		if err := r.Get(ctx, client.ObjectKey{Name: app.Name}, latest); err != nil {
			// The App can be deleted concurrently with the reconciler
			// (e.g., rdd svc delete mid-reconcile). Treat NotFound as a
			// no-op so we do not propagate a spurious error that would
			// requeue only to be IgnoreNotFound-dropped on the next
			// reconcile.
			if apierrors.IsNotFound(err) {
				return nil
			}
			return err
		}
		changed := meta.SetStatusCondition(&latest.Status.Conditions, metav1.Condition{
			Type:               conditionContainerEngineReady,

This isn't catastrophic — controller-runtime requeues on the returned conflict error and the next reconcile re-Gets — but it turns routine status churn into avoidable 409s and noisy reconcile-error logs.

Fix: convert the App controller's status write to the same retry.RetryOnConflict + re-Get pattern used in setEngineCondition.

if statusChanged {
    if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
        latest := &v1alpha1.App{}
        if err := r.Get(ctx, client.ObjectKeyFromObject(&app), latest); err != nil {
            return err
        }
        for _, cond := range limaVM.Status.Conditions {
            apimeta.SetStatusCondition(&latest.Status.Conditions, metav1.Condition{
                Type:               cond.Type,
                Status:             cond.Status,
                Reason:             cond.Reason,
                Message:            base.TruncateConditionMessage(cond.Message),
                ObservedGeneration: latest.Generation,
                LastTransitionTime: cond.LastTransitionTime,
            })
        }
        return r.Status().Update(ctx, latest)
    }); err != nil {
        log.Error(err, "Unable to update App status")
        return ctrl.Result{}, err
    }
}

// waitForDesiredState waits for the App to reach the state implied by the
// properties that were set. If running=true was set, it waits for the
// container engine to be ready. If running=false was set, it waits for the
// App's Running condition to go to False — i.e. the VM has actually
// stopped, which is stricter than "container engine disconnected".
// Other property changes do not wait.
//
// minGen is the App's metadata.generation after our write. The predicate
// only accepts condition snapshots whose ObservedGeneration >= minGen,
// so a stale ContainerEngineReady=True from a previous backend (or a
// stale Running=False from a prior stop) cannot satisfy the wait before
// the reconciler has observed the new spec. EngineReconciler and
// AppReconciler both stamp ObservedGeneration when they update conditions.
//
// TODO: changes that trigger a VM restart without changing "running" (e.g.
// containerEngine.name alone) are still not waited on. A dedicated
// "Reconciled" condition on App would let the CLI detect when the full
// reconcile chain has settled for any property change.
func waitForDesiredState(ctx context.Context, config *rest.Config, properties map[string]string, timeout time.Duration, minGen int64) error {
	runningVal, ok := properties["running"]
	if !ok {
		return nil
	}

	ctx, cancel := context.WithTimeout(ctx, timeout)
	defer cancel()

	if runningVal == "true" {
		logrus.Info("Waiting for container engine to be ready")
		return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
			status, gen, present := conditionInfo(obj, "ContainerEngineReady")

The predicate only checks Status == True and has no generation handoff from the preceding patch. The "Known hazard" comment at cmd/rdd/set.go:445-452 acknowledges that rdd set containerEngine.name=<other> running=true on an already-running VM can be satisfied by the stale Connected condition left over from the previous backend — but setEngineCondition already writes ObservedGeneration: latest.Generation at engine_reconciler.go:234, so the fix is cheap: capture the generation from the patch response in setAction and compare it in the predicate. The waitForDesiredState signature needs a minGen int64 parameter, patchApp/createAndPatchApp need to return the post-patch generation, and watchCondition's predicate gains cond.ObservedGeneration >= minGen.

	if dryRun {
		logrus.Info("App validated (dry run)")
	} else {
		logrus.Info("App updated")
	}
	return app.Generation, nil
}

// waitForDesiredState waits for the App to reach the state implied by the
// properties that were set. If running=true was set, it waits for the
// container engine to be ready. If running=false was set, it waits for the
// App's Running condition to go to False — i.e. the VM has actually
// stopped, which is stricter than "container engine disconnected".
// Other property changes do not wait.
//
// minGen is the App's metadata.generation after our write. The predicate
// only accepts condition snapshots whose ObservedGeneration >= minGen,

// deleteAllOfType).
func (r *EngineReconciler) setEngineCondition(ctx context.Context, app *appv1alpha1.App, status metav1.ConditionStatus, reason, message string) error {
	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
		latest := &appv1alpha1.App{}
		if err := r.Get(ctx, client.ObjectKey{Name: app.Name}, latest); err != nil {
			// The App can be deleted concurrently with the reconciler
			// (e.g., rdd svc delete mid-reconcile). Treat NotFound as a
			// no-op so we do not propagate a spurious error that would
			// requeue only to be IgnoreNotFound-dropped on the next
			// reconcile.
			if apierrors.IsNotFound(err) {

Without the fix, rdd set containerEngine.name=containerd (without changing running) is the worst case: no wait triggers at all because no running argument is set, even though a full VM restart follows. With running=true included, the wait happens but can be satisfied by a pre-patch snapshot.

Fix sketch (conceptual, paired with I4 in rdd set.go):

// In setAction, after patchApp/createAndPatchApp:
var app appv1alpha1.App
if err := c.Get(ctx, client.ObjectKey{Name: "app"}, &app); err != nil {
    return fmt.Errorf("failed to get App after patch: %w", err)
}
minGen := app.Generation

// In waitForDesiredState predicate:
return conditionStatusForGeneration(obj, "ContainerEngineReady", metav1.ConditionTrue, minGen)

Claude flagged this as Important because the fix is already available (the stamp exists), not because the hazard itself is new.

	if inspect.State.Error != "" {
		statusApply.WithError(inspect.State.Error)
	}

	if t, err := time.Parse(time.RFC3339Nano, inspect.Created); err == nil {
		statusApply.WithCreatedAt(metav1.NewTime(t))
	}
	if t, err := time.Parse(time.RFC3339Nano, inspect.State.StartedAt); err == nil && !t.IsZero() {
		statusApply.WithStartedAt(metav1.NewTime(t))
	}
	if t, err := time.Parse(time.RFC3339Nano, inspect.State.FinishedAt); err == nil && !t.IsZero() {
		statusApply.WithFinishedAt(metav1.NewTime(t))
	}

	// Port bindings. Go map iteration is randomized, so sort by port
	// name before building the apply slice: even with
	// +listType=map +listMapKey=name on Status.Ports, the entries land
	// in the stored object in the order we provide, and a stable order
	// avoids spurious resourceVersion churn when consumers render the
	// list or diff it.
	var applyPorts []*containersv1alpha1apply.ContainerPortApplyConfiguration
	if inspect.NetworkSettings != nil {
		portNames := make([]mobynetwork.Port, 0, len(inspect.NetworkSettings.Ports))
		for p := range inspect.NetworkSettings.Ports {

inspect.NetworkSettings.Ports is a Go map, so iteration order is randomized per process. The Container CRD declares status.ports as a plain array with no x-kubernetes-list-type annotation (verified against pkg/controllers/containers/container/crd.yaml: only status.conditions carries x-kubernetes-list-type: map). Lists without a list-type default to atomic, which is position-sensitive: the apiserver treats [A, B] and [B, A] as different values and produces a new resourceVersion on every apply that reorders them. That fires a watch event, which the engine reconciler catches via Watches(&containersv1alpha1.Container{}, enqueueApp), which re-enters reconcileContainerSpecs + processFinalizers — a four-list sweep per watch event. For a container with two or more exposed ports, every unrelated Docker event (image pull, volume create, etc.) can amplify into a status re-apply and a full reconcile sweep.

Fix: sort inspect.NetworkSettings.Ports by port name before building the apply slice. The inner bindings loop is Docker-ordered (IPv4 then IPv6) and does not need sorting unless Docker's internal order changes.

 if inspect.NetworkSettings != nil {
+    portNames := make([]mobynat.Port, 0, len(inspect.NetworkSettings.Ports))
+    for p := range inspect.NetworkSettings.Ports {
+        portNames = append(portNames, p)
+    }
+    sort.Slice(portNames, func(i, j int) bool { return portNames[i] < portNames[j] })
-    for portName, ports := range inspect.NetworkSettings.Ports {
+    for _, portName := range portNames {
+        ports := inspect.NetworkSettings.Ports[portName]

Alternatively, annotate Ports with +listType=map + +listMapKey=name in container_types.go so the apiserver merger ignores ordering — this is more robust but requires a CRD regeneration and is a larger change.