# Deep Review: 20260412-221509-pr-295

| | |
|---|---|
| **Date** | 2026-04-12 22:15 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 5 (of PR #295) |
| **Author** | [@jandubois](https://github.com/jandubois) |
| **PR** | [#295](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/295) — Container Engine watcher |
| **Branch** | `docker` |
| **Commits** | `e2df2f9` Address review round 4 findings for PR #295<br>`33db27d` Address review round 3 findings for PR #295<br>`cf4b90e` ci: disable Spotlight indexing on macOS BATS runners<br>`c484dab` ci: capture memory pressure and API state in support bundle<br>`3196b24` bats: park spec.state=unknown before pausing test-state<br>`84c0736` engine: apply Container/Image/Volume capitalization convention<br>`c8fa377` generate: refresh containerspec.go and crd.yaml for spec.state doc<br>`62d222a` scripts: resolve rdd binary relative to bats-with-timeout.sh<br>`66eb50e` Address review round 2 findings for PR #295<br>`7c8f356` ci: always capture support bundle and log hostagent pgid<br>`9669b4b` bats: close fds 3 and 4 when invoking rdd<br>`50bdc15` ci: capture support bundle when BATS targets time out<br>`762c1e8` Make rdd set wait for the desired state by default<br>`65943da` Add BATS integration tests for the engine controller<br>`ba6caf7` Document engine mirroring and container state transitions<br>`4db3f20` Allow container spec.state transitions via webhook<br>`93359f7` Add engine controller that mirrors Docker state into K8s<br>`87f1df1` Add DockerSocket() helper and Docker client dependency<br>`32a0109` Work around distro proxy bug; update docker socket path<br>`536ccb2` Rosetta is now a vz driver option, not a global option |
| **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro |
| **Verdict** | **Merge with fixes** — one critical regression in the new `rdd set` default wait and six important engine-watcher resilience gaps, none of which require redesign. |
| **Wall-clock time** | `37 min 44 s` |

## Executive Summary

Round 5 of PR #295 lands the engine controller that mirrors Docker `Container`/`Image`/`Volume` state into the existing `containers.rancherdesktop.io` API group, plus a new default-wait contract for `rdd set`, CI support-bundle improvements, and related Lima template fixes. The watcher/reconciler split is well-structured, the SSA field-manager workaround is carefully documented, and the BATS coverage for the happy paths is solid.

The review found **one critical regression** and **six important issues**, all correctability without architectural change:

- **Critical (C1):** `rdd set` hangs for the full timeout when the App is already running, because the engine reconciler skips `setEngineCondition` on the steady-state path and `ContainerEngineReady.observedGeneration` stays behind the post-patch generation.
- **Important (I1):** `ContainerNamespace/moby` is not watched, so a user delete stays gone until the next VM restart — and the new BATS test cements that broken behaviour.
- **Important (I2):** `processImageFinalizers`/`processVolumeFinalizers` skip the `retry.RetryOnConflict` pattern that `processContainerFinalizers` uses, so a single 409 wastes a Docker delete on the next reconcile.
- **Important (I3):** The new `rdd set` wait predicates treat `StartFailed`/`ConnectFailed` as "keep waiting" and `StopFailed` as "success," so the default wait is misleading on both failure paths.
- **Important (I4):** Docker `ActionRename` is not in the event switch, so `docker rename` leaves `status.name` stale until another state event refreshes the container.
- **Important (I5):** A transient Docker `Inspect` failure during `syncAllImages` causes the valid K8s `Image` mirror to be classified stale and deleted.
- **Important (I6):** The watcher run loop logs and drops errors from `handleEvent`, so a transient apiserver 503 can permanently desynchronise an image or volume mirror until the next VM restart.

Nine suggestions cover SSA list-ordering churn, lock-while-closing asymmetry in `Reconcile`, `DockerSocket()` side effects, error-message specificity, and a few dead/unreachable branches.

Structure: **1 critical, 6 important, 9 suggestions.**

---

## Critical Issues

C1. **`rdd set` hangs for the full timeout when the App is already running** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:82-192` [Gemini 3.1 Pro]

```go
if !watcherRunning {
    log.Info("App is running, starting Docker watcher")
    if err := r.startWatcher(ctx); err != nil {
        // ...
        return ctrl.Result{}, err
    }
    if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
        return ctrl.Result{}, err
    }
    // Fall through so pending spec.state patches and finalizers ...
}

// reconcileContainerSpecs + processFinalizers ... (no setEngineCondition call)
```

`rdd set` now waits by default (commit `762c1e8`). The wait predicate at `cmd/rdd/set.go:478-481` looks for `ContainerEngineReady.status == True && observedGeneration >= minGen`, where `minGen` is `app.Generation` right after `patchApp` (`cmd/rdd/set.go:425-447`). That generation filter is the exact fix for stale-snapshot races and is correct by itself.

The engine reconciler, however, only calls `setEngineCondition` in two branches: the `!wantWatcher` terminal-state branch (lines 124-155) and the `!watcherRunning` fresh-start branch (lines 157-175). In the steady-state path — App running, watcher already running — the reconciler falls through directly to `reconcileContainerSpecs`/`processFinalizers` and returns without touching `App.Status.Conditions`. Because `meta.SetStatusCondition` is also only called in those two branches, `ContainerEngineReady.observedGeneration` keeps its previous value and never advances past whatever generation was in effect when the watcher last started.

Concretely, any command that bumps `app.Generation` *without* also cycling the watcher hangs for the full 5 minute default:

```
$ rdd set running=true kubernetes.enabled=true   # when already running=true
# patchApp → spec changes → generation N → N+1
# waitForDesiredState → minGen=N+1
# Reconciler reruns but takes the steady-state path → observedGen stays N
# watchCondition loops until timeout → "timed out waiting for App state"
```

The hang is not limited to that one permutation — any property change that leaves `running=true` and doesn't restart the watcher hits it (e.g., toggling `kubernetes.*`, `containerEngine.mirrors`, etc., along with an idempotent `running=true`). The simple start/stop BATS flow in `engine.bats` doesn't exercise it because it only toggles `running` through terminal states, which is why the regression slipped through.

Fix: move the `setEngineCondition(Connected)` call out of the `!watcherRunning` block so every `wantWatcher=true` reconcile re-asserts the condition. `setEngineCondition` already no-ops when nothing changed (`meta.SetStatusCondition` compares all fields) and wraps the write in `retry.RetryOnConflict`, so an unconditional call is cheap on stable reconciles but stamps the current `latest.Generation` into `observedGeneration` whenever the App spec advances.

```diff
 	if !watcherRunning {
 		log.Info("App is running, starting Docker watcher")
 		if err := r.startWatcher(ctx); err != nil {
 			log.Error(err, "Failed to start Docker watcher")
 			if condErr := r.setEngineCondition(ctx, &app, metav1.ConditionFalse, "ConnectFailed", err.Error()); condErr != nil {
 				log.Error(condErr, "Failed to update ContainerEngineReady to ConnectFailed")
 			}
 			return ctrl.Result{}, err
 		}
-		if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
-			return ctrl.Result{}, err
-		}
 		// Fall through so pending spec.state patches and finalizers ...
 	}
 
+	if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
+		return ctrl.Result{}, err
+	}
+
 	if err := r.reconcileContainerSpecs(ctx); err != nil {
```

A matching BATS test should run `rdd set running=true <some-unrelated-prop>=<value>` from the running state and assert that it returns well before the 5-minute timeout. Classification: regression, critical.

---

## Important Issues

I1. **`ContainerNamespace/moby` is not re-mirrored after user delete** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:495-513` [Codex GPT 5.4] [Claude Opus 4.6]

```go
return ctrl.NewControllerManagedBy(mgr).
    For(&appv1alpha1.App{}).
    Named("engine-reconciler").
    WatchesRawSource(source.Channel(r.reconcileChan, enqueueApp)).
    Watches(&containersv1alpha1.Container{}, enqueueApp).
    Watches(&containersv1alpha1.Image{}, enqueueApp).
    Watches(&containersv1alpha1.Volume{}, enqueueApp).
    Complete(r)
```

`syncContainerNamespace()` at `pkg/controllers/app/engine/controllers/sync_volumes.go:62-66` creates `containernamespace/moby` exactly once from `fullSync` on watcher startup. The reconciler watches `Container`, `Image`, and `Volume` but not `ContainerNamespace`, and `ContainerNamespace` has no mirror finalizer, so a user who runs `rdd ctl delete containernamespace/moby` succeeds and produces no event that wakes the engine reconciler back up. Subsequent Container/Image/Volume Docker events trigger reconciles, but `reconcileContainerSpecs`/`processFinalizers` never touch the namespace — it stays gone until the VM is restarted (or the watcher dies and fullSyncs).

The design doc at `docs/design/api_containers.md:88-96` says the dockerd namespace "cannot be modified in any way," but nothing currently enforces that. Worse, the new BATS test at `bats/tests/32-app-controllers/engine.bats:364-370` asserts that deleting `containernamespace/moby` leaves it absent. That locks in the broken invariant instead of protecting it, and any future fix will also need to update this test.

Fix: either add `Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp)` to `SetupWithManager` and idempotently re-run `ensureNamespace + syncContainerNamespace` on every reconcile (both are single Applies and already idempotent), or reject deletion of the `moby` namespace via an admission webhook while the engine controller is active. The watch-and-restore variant is the smaller change and consistent with the existing mirror pattern. Either way, update the BATS test to assert the namespace returns — not that it stays gone. Classification: regression, important.

I2. **`processImageFinalizers`/`processVolumeFinalizers` skip `retry.RetryOnConflict`** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:431-492` [Gemini 3.1 Pro] [Claude Opus 4.6]

```go
// processImageFinalizers
if removeFinalizer(img, mirrorFinalizer) {
    if err := client.IgnoreNotFound(r.Update(ctx, img)); err != nil {
        errs = append(errs, fmt.Errorf("failed to remove finalizer from Image %s: %w", img.Name, err))
    }
}
```

`processContainerFinalizers` (lines 410-423) correctly re-Gets `latest` under `retry.RetryOnConflict` before stripping the finalizer. The Image and Volume helpers mutate the stale `img`/`v` from the outer List and call `Update` directly. On a 409 — which can fire any time a concurrent watcher sync just re-applied status — the Update fails and the finalizer isn't stripped. The Docker-side delete has already succeeded by that point, so the reconciler requeues, re-Lists, re-inspects, and re-issues the engine-side delete against an already-gone object before finally stripping the finalizer. It's self-healing but burns a Docker round-trip per conflict and breaks the invariant "once we tell Docker to delete, we strip the finalizer on the first attempt," which the adjacent Container path upholds.

Fix: mirror the `processContainerFinalizers` pattern in both helpers. Put the finalizer strip inside a `retry.RetryOnConflict` loop that re-Gets `latest` and treats `NotFound` as benign:

```diff
-        if removeFinalizer(img, mirrorFinalizer) {
-            if err := client.IgnoreNotFound(r.Update(ctx, img)); err != nil {
-                errs = append(errs, fmt.Errorf("failed to remove finalizer from Image %s: %w", img.Name, err))
-            }
-        }
+        key := client.ObjectKeyFromObject(img)
+        retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+            latest := &containersv1alpha1.Image{}
+            if err := r.Get(ctx, key, latest); err != nil {
+                if apierrors.IsNotFound(err) {
+                    return nil
+                }
+                return err
+            }
+            if !removeFinalizer(latest, mirrorFinalizer) {
+                return nil
+            }
+            return r.Update(ctx, latest)
+        })
+        if retryErr != nil {
+            errs = append(errs, fmt.Errorf("failed to remove finalizer from Image %s: %w", img.Name, retryErr))
+        }
```

Apply the same change to `processVolumeFinalizers` (lines 485-489). Classification: regression, important.

I3. **`rdd set` wait misinterprets `StartFailed`/`ConnectFailed`/`StopFailed` conditions** — `cmd/rdd/set.go:476-494` [Codex GPT 5.4]

```go
if runningVal == "true" {
    return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
        status, gen, present := conditionInfo(obj, "ContainerEngineReady")
        return present && gen >= minGen && status == metav1.ConditionTrue
    })
}
return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
    status, gen, present := conditionInfo(obj, "Running")
    if !present {
        return true
    }
    return gen >= minGen && status != metav1.ConditionTrue
})
```

The `running=true` branch only accepts `ContainerEngineReady=True`. If startup fails — LimaVM publishes `Running=False/StartFailed` at `pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go:382-385,427-430,463-476`, or the engine reconciler publishes `ContainerEngineReady=False/ConnectFailed` at `engine_reconciler.go:161-163` — the wait keeps looping until the 5-minute timeout and returns the generic "timed out waiting for App state" error instead of the actual failure reason. Users see a timeout when they should have seen "start failed" immediately.

The `running=false` branch has the opposite problem: it treats *any* refreshed non-`True` `Running` condition as success. When Lima publishes `Running=False/StopFailed` at `limavm_lifecycle.go:527-539`, the wait returns success even though the VM didn't stop.

Fix: inspect `reason` as well as `status`. The predicates should look like:

```
running=true success   : ContainerEngineReady.status==True
running=true failure   : Running.reason in {StartFailed} OR ContainerEngineReady.reason in {ConnectFailed}
running=false success  : Running absent OR Running.reason in {Stopped}
running=false failure  : Running.reason in {StopFailed}
```

On failure the wait should return an error whose message carries the condition's `message` field so the user sees the underlying cause. Extending `conditionInfo` to also return `reason`/`message` is a small change that keeps `watchCondition` generic. Classification: regression, important.

I4. **`docker rename` events are dropped** — `pkg/controllers/app/engine/controllers/docker_watcher.go:223-245` [Codex GPT 5.4]

```go
switch msg.Action {
case events.ActionCreate,
    events.ActionStart,
    events.ActionStop,
    events.ActionDie,
    events.ActionPause,
    events.ActionUnPause,
    events.ActionRestart:
    log.V(1).Info("Container event", "action", msg.Action, "id", msg.Actor.ID)
    return w.syncContainer(ctx, msg.Actor.ID)
case events.ActionDestroy:
    // ...
default:
    return nil
}
```

`applyContainer` derives the mirrored name from `inspect.Name` at `sync_containers.go:111` and writes it into `status.name`/`status.namespace`. Docker emits `events.ActionRename` when `docker rename` is used, but the switch lets it fall through to `default: return nil`. The mirror's `status.name` therefore stays stale indefinitely — until some unrelated start/stop/destroy event happens to trigger another `syncContainer`, which re-inspects and rewrites the name. Consumers of `Container.status.name` (including the `rdd ctl` UX and any future label selectors) see the old name until that happens.

Fix: add `events.ActionRename` to the same case block as the other state-refreshing events. Also add a BATS test that renames a running container and waits for `.status.name` to reflect the new name:

```diff
 	case events.ActionCreate,
 		events.ActionStart,
 		events.ActionStop,
 		events.ActionDie,
 		events.ActionPause,
 		events.ActionUnPause,
-		events.ActionRestart:
+		events.ActionRestart,
+		events.ActionRename:
 		log.V(1).Info("Container event", "action", msg.Action, "id", msg.Actor.ID)
 		return w.syncContainer(ctx, msg.Actor.ID)
```

Classification: gap, important.

I5. **Transient Docker `Inspect` failures during `syncAllImages` delete valid `Image` mirrors** — `pkg/controllers/app/engine/controllers/sync_images.go:41-85` [Gemini 3.1 Pro]

```go
var errs []error
for _, summary := range listResult.Items {
    names, err := w.syncImageFromSummary(ctx, summary)
    if err != nil {
        log.Error(err, "Skipping image during full sync", "id", summary.ID)
        continue
    }
    for _, n := range names {
        activeNames[n] = true
    }
}

// Remove stale Image mirrors.
// ...
for i := range imageMirrors.Items {
    img := &imageMirrors.Items[i]
    if !activeNames[img.Name] {
        log.V(1).Info("Removing stale Image", "name", img.Name)
        if err := w.removeMirrorResource(ctx, img, img.Name); err != nil {
            errs = append(errs, err)
        }
    }
}
```

`syncImageFromSummary` returns the mirror names *after* a successful `ImageInspect`. If Docker returns a transient 500/503 for a single image — which happens on loaded or recently-restarted daemons — the names are never added to `activeNames`, and the stale-mirror sweep at the bottom then deletes the valid K8s `Image` mirror that already existed for that image. The user sees their `Image` blink out and reappear on the next event or fullSync.

The `applyImageFromInspect` helper already computes the deterministic mirror name from `inspect.ID + RepoTags` (or the ID alone for dangling images). The same name can be computed from the `ImageListItem` summary — it has both `ID` and `RepoTags`.

Fix: pre-compute the expected names from the summary and seed `activeNames` *before* calling `syncImageFromSummary`, so transient inspect failures can't trigger mirror deletion:

```diff
 	var errs []error
 	for _, summary := range listResult.Items {
+		// Seed active names before Inspect so a transient Inspect failure
+		// does not classify the existing mirror as stale.
+		if len(summary.RepoTags) > 0 {
+			for _, tag := range summary.RepoTags {
+				name := fmt.Sprintf("%s-%x",
+					sanitizeKubernetesObjectName(summary.ID),
+					sha256.Sum256([]byte(tag)))
+				activeNames[name] = true
+			}
+		} else {
+			activeNames[sanitizeKubernetesObjectName(summary.ID)] = true
+		}
 		names, err := w.syncImageFromSummary(ctx, summary)
 		if err != nil {
 			log.Error(err, "Skipping image during full sync", "id", summary.ID)
 			continue
 		}
-		for _, n := range names {
-			activeNames[n] = true
-		}
 	}
```

Refactor the name derivation into a helper so `applyImageFromInspect` and `syncAllImages` share it and can't drift. `syncAllContainers` already uses `dc.ID` from the list response for the same purpose, so it doesn't need this fix. Classification: bug, important.

I6. **Docker watcher drops events on transient `handleEvent` errors** — `pkg/controllers/app/engine/controllers/docker_watcher.go:171-199` [Gemini 3.1 Pro]

```go
case msg, ok := <-result.Messages:
    if !ok {
        // closed stream handling ...
    }
    if err := w.handleEvent(ctx, msg); err != nil {
        log.Error(err, "Failed to handle Docker event",
            "type", msg.Type, "action", msg.Action, "actor", msg.Actor.ID)
    }
```

A transient failure inside `handleEvent` — apiserver 503, SSA conflict that survives its internal retry, temporary network partition — is logged and dropped. The watcher never periodically resyncs (there's no time-based `fullSync` tick), so whether the mirror eventually catches up depends on whether Docker emits *another* event for the same object:

- **Container:** a subsequent Start/Stop/Die usually arrives, so drift tends to self-heal.
- **Image:** a pull event fires once. If its apply fails, the `Image` mirror never shows up until the next watcher restart or manual pull of the same tag.
- **Volume:** a create event also fires once. Same permanent-drift risk.

Gemini's proposed fix — return from `run` on any error and let the dead watcher be detected by the reconciler — is correct but aggressive: every transient 503 would tear down the event stream, trigger a fresh `fullSync`, and churn every mirror. A gentler fix is enough in practice:

1. On `handleEvent` error, enqueue an event-specific retry (e.g., store `msg.Actor.ID` in a small retry set and call `syncContainer`/`reconcileImageByID`/`syncVolume` on a short backoff), **or**
2. Run `fullSync` on a periodic tick (e.g., every 5 minutes) as a belt-and-suspenders resync, so any missed event is reconciled within a bounded window.

Option 2 is the smaller change, matches how controller-runtime's `SyncPeriod` works, and is how `kubelet`'s image manager handles the same class of bug. Classification: gap, important.

---

## Suggestions

S1. **`Reconcile` holds `watcherMu` across the blocking `w.stop()` call** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:93-101` [Claude Opus 4.6]

```go
r.watcherMu.Lock()
watcherRunning := r.watcher != nil
watcherDied := watcherRunning && !r.watcher.alive()
if watcherDied {
    r.watcher.stop()      // blocks on <-w.done and cli.Close() under the lock
    r.watcher = nil
    watcherRunning = false
}
r.watcherMu.Unlock()
```

The standalone `stopWatcher()` helper at lines 212-221 correctly captures the watcher, releases `watcherMu`, *then* calls `w.stop()`. The inline version above holds the lock across a blocking `cli.Close()`. It's only a latent bug at `MaxConcurrentReconciles=1` (the current default), but the asymmetry is a trap: anyone who later enables concurrent reconciles will hit a lock held across a blocking Docker close. Match the pattern:

```diff
 	r.watcherMu.Lock()
 	watcherRunning := r.watcher != nil
-	watcherDied := watcherRunning && !r.watcher.alive()
+	var diedWatcher *dockerWatcher
+	if watcherRunning && !r.watcher.alive() {
+		diedWatcher = r.watcher
+		r.watcher = nil
+		watcherRunning = false
+	}
+	r.watcherMu.Unlock()
+	watcherDied := diedWatcher != nil
 	if watcherDied {
-		r.watcher.stop()
-		r.watcher = nil
-		watcherRunning = false
+		diedWatcher.stop()
 	}
-	r.watcherMu.Unlock()
```

Classification: gap, suggestion.

S2. **`ImageStatus.RepoDigests` is sent to SSA in Docker's order** — `pkg/controllers/app/engine/controllers/sync_images.go:151-164` [Claude Opus 4.6]

```go
statusApply := containersv1alpha1apply.ImageStatus().
    WithID(inspect.ID).
    WithRepoDigests(inspect.RepoDigests...).
```

`ImageStatus.RepoDigests` in `pkg/apis/containers/v1alpha1/image_types.go:28-31` is a plain `[]string` with no `+listType` — atomic under SSA. Docker's `RepoDigests` is sourced from the reference store and is not documented to have a stable order, so an image with multiple references can reorder between inspects. Any reorder mints a new `resourceVersion` on every status apply, which fires watch events and re-triggers the engine reconciler — exactly the churn the port-sort already guards against.

Fix: sort a copy before passing it in.

```diff
+    digests := slices.Clone(inspect.RepoDigests)
+    sort.Strings(digests)
     statusApply := containersv1alpha1apply.ImageStatus().
         WithID(inspect.ID).
-        WithRepoDigests(inspect.RepoDigests...).
+        WithRepoDigests(digests...).
```

Classification: gap, suggestion.

S3. **`ContainerPort.Bindings` is sent to SSA in Docker's order** — `pkg/controllers/app/engine/controllers/sync_containers.go:192-214` [Claude Opus 4.6]

```go
for _, port := range ports {
    bindings = append(bindings, containersv1alpha1apply.ContainerPortBinding().
        WithHostIP(port.HostIP.String()).
        WithHostPort(port.HostPort))
}
applyPorts = append(applyPorts, containersv1alpha1apply.ContainerPort().
    WithName(portName.String()).
    WithBindings(bindings...))
```

Same pattern as S2, one nesting level deeper: the outer `portNames` slice is correctly sorted at line 198, but the inner `bindings` slice follows whatever order Docker returned (Dual-stack `docker run -p 80:80` can emit either order depending on the kernel). `ContainerPort.Bindings` in `container_types.go:48-52` has no `+listType`, so it's atomic. Sort by `(HostIP, HostPort)` before building the apply config. Classification: gap, suggestion.

S4. **`rdd set` timeout error doesn't name the condition** — `cmd/rdd/set.go:535-543` [Claude Opus 4.6]

```go
if _, err := watchtools.UntilWithSync(...); err != nil {
    if errors.Is(err, context.DeadlineExceeded) {
        return errors.New("timed out waiting for App state")
    }
```

When `rdd set running=true` times out, the user sees `timed out waiting for App state` with no hint that the wait was specifically on `ContainerEngineReady=True`. When this fix ships with I3, the error will also need to carry the failure reason. Plumb a descriptive label into `watchCondition` and return something like `timed out waiting for ContainerEngineReady=True (5m)` or `timed out waiting for App to stop (5m)`. Classification: enhancement, suggestion.

S5. **`deleteAllOfType` passes the stale list item to `r.Delete`** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:299-328` [Claude Opus 4.6]

```go
for _, item := range items {
    obj := item.(client.Object)
    key := client.ObjectKeyFromObject(obj)
    // ...
    retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
        latest := obj.DeepCopyObject().(client.Object)
        // ...
    })
    // ...
    if err := client.IgnoreNotFound(r.Delete(ctx, obj)); err != nil {
```

The final `r.Delete` passes `obj` — the stale list item — not `latest` from the retry closure. That's functionally correct because `Delete` keys off name+namespace and doesn't send `resourceVersion`, but the closure-scoped `latest` isn't visible outside the retry body and future maintainers could reasonably think they need to pass it. Add a one-line comment explaining that `obj` is acceptable because `Delete` doesn't use `resourceVersion`, or hoist `latest` out of the closure. Classification: gap, suggestion.

S6. **`DockerSocket()` performs filesystem side effects at construction** — `pkg/instance/instance.go:136-144` [Claude Opus 4.6]

```go
var DockerSocket = sync.OnceValue(func() string {
    home, err := os.UserHomeDir()
    if err != nil {
        panic(fmt.Errorf("could not get home directory: %w", err))
    }
    dir := filepath.Join(home, ".rd2")
    _ = os.MkdirAll(dir, 0o700)
    return filepath.Join(dir, "docker.sock")
})
```

Unlike the other `instance.*` accessors (`Dir`, `LogDir`, `LimaHome`), which are pure path computations, `DockerSocket()` performs `os.MkdirAll` and swallows the error. Any caller that just wants the path — `rdd svc paths`, CLI help, a test — silently creates the directory as a side effect. Two cleaner options:

1. Move the `MkdirAll` to the watcher (`newDockerWatcher`) where the directory is actually needed, with proper error handling.
2. Keep it in `instance` but rename to `EnsureDockerSocket()` and return the error.

Option 1 matches how `LimaHome()` stays pure and defers mkdir to the Lima controller. This will also need to change when the hardcoded `".rd2"` is replaced by `ShortDir()` per the existing TODO at lines 127-135. Classification: gap, suggestion.

S7. **`watchCondition` allocates a new dynamic client per call** — `cmd/rdd/set.go:501-516` [Claude Opus 4.6]

`watchCondition` constructs a fresh `dynamic.NewForConfig(config)` on every invocation. For `rdd set` today this is fine — one call per command — but if the helper is reused elsewhere it's wasteful. Consider accepting the dynamic client as a parameter and reusing the existing controller-runtime client from `setAction` where possible. Classification: enhancement, suggestion.

S8. **Dead branches in the container state-transition webhook** — `pkg/controllers/containers/container/container_webhooks.go:36-54` [Claude Opus 4.6]

```go
if oldContainer.Spec.State != newContainer.Spec.State {
    switch newContainer.Spec.State {
    case v1alpha1.ContainerStatusCreated, v1alpha1.ContainerStatusRunning, v1alpha1.ContainerStatusUnknown:
    default:
        return nil, fmt.Errorf("invalid target state %q: must be %q, %q, or %q", ...)
    }
    // ... DeepEqual check
}
```

The CRD enum validation in `crd.yaml:285-288` (`enum: [created, running, unknown]`) runs before the webhook, so the `default` branch is unreachable through normal admission. The second `equality.Semantic.DeepEqual` check at line 51 is also dead code today because `ContainerSpec` has only the `State` field — but the moment another field is added (immutable ports, immutable labels, etc.) it becomes load-bearing. Either add a one-line comment noting the DeepEqual guards future fields, or drop both dead branches and trust the CRD enum until more fields land. Classification: enhancement, suggestion.

S9. **`reconcileContainerState` silently ignores unknown desired states** — `pkg/controllers/app/engine/controllers/docker_watcher.go:361-392` [Claude Opus 4.6]

```go
switch desired {
case containersv1alpha1.ContainerStatusRunning:
    // ...
case containersv1alpha1.ContainerStatusCreated:
    // ...
}
return nil
```

Today the webhook restricts `desired` to `{unknown, running, created}`, so the switch covers every reachable value. A future relaxation of the enum — or a direct storage manipulation that bypasses admission — would produce a silent no-op. Add a `default` branch that logs the unexpected state (at `Error` level) so regressions surface quickly instead of silently dropping state transitions. Classification: gap, suggestion.

---

## Design Observations

### Concerns

**D1. `reconcileContainerSpecs` + `processFinalizers` fan out on every mirror event (in-scope).** [Claude Opus 4.6] The existing comment at `engine_reconciler.go:177-183` spells out the cost: every Container/Image/Volume watch event triggers a reconcile that Lists every mirrored object to check for spec.state changes and pending finalizers. The long-term fix — per-resource reconcilers with watch predicates for `deletionTimestamp!=nil` and `spec.state` changes — is correct. On typical dev machines the load is bounded, but this will need attention before the feature supports larger workloads. Classification: future.

**D2. Two field managers to work around SSA spec ownership (in-scope).** [Claude Opus 4.6] The explanation at `sync_containers.go:144-157` is correct but it documents a design wart: server-side apply can't express "update only finalizers" without releasing spec from the original owner, so a second field manager (`engine-controller-finalizer`) was introduced. An alternative — keep SSA only for the creation path and switch to a strategic merge patch for finalizer re-assertion — would avoid the two-manager split and match how most controllers add finalizers. Classification: future.

### Strengths

**S1. Docker-daemon clock for the events `Since` anchor (in-scope).** [Claude Opus 4.6] [Codex GPT 5.4] `dockerEventsSince()` at `docker_watcher.go:107-122` reads `Info.SystemTime` from the daemon and uses that for the events `Since` filter rather than `time.Now()` on the host. The host/guest clock-skew hazard is real for anything running inside Lima; the fallback to a biased host clock is a sensible safety net.

**S2. Cross-controller retry-on-conflict for `App.Status.Conditions` (in-scope).** [Claude Opus 4.6] [Codex GPT 5.4] `setEngineCondition` (`engine_reconciler.go:230-255`) and the matching AppReconciler helper (`app_controller.go:239-259`) both re-Get inside a `retry.RetryOnConflict` loop. That's the right fix for controller-runtime not serializing writes across reconcilers that both touch the App's status.

**S3. `applyContainer` spec-on-create-only with Get-gated Apply (in-scope).** [Claude Opus 4.6] The Get-gated `WithSpec(unknown)` at `sync_containers.go:126-131` preserves user `spec.state` patches across subsequent syncs without needing a second field manager for spec — a cleaner alternative than the finalizer split in D2.

**S4. Explicit state-matrix doc on `reconcileContainerState` (in-scope).** [Claude Opus 4.6] The comment at `docker_watcher.go:340-360` tabulates every desired×actual combination and explains why paused must be handled for `running` but not for `created`. The matching BATS tests at `engine.bats:264-320` verify both directions.

**S5. Panic recovery + `alive()`-triggered restart (in-scope).** [Claude Opus 4.6] [Gemini 3.1 Pro] The `recover()` block in `run()` (`docker_watcher.go:153-159`) falls through to `enqueueReconcile`, so the next reconcile observes the dead watcher and fulls-syncs a fresh one. It's a clean handoff that avoids permanent lockups on unexpected event shapes.

**S6. Port-binding sort to avoid SSA churn (in-scope).** [Claude Opus 4.6] [Gemini 3.1 Pro] `sync_containers.go:197-201` sorts `portNames` before building the apply config, which was the same anti-pattern S2/S3 above flag at the inner level. It's the right instinct — the inner bindings slice just needs the same treatment.

## Testing Assessment

[Claude Opus 4.6] [Codex GPT 5.4] [Gemini 3.1 Pro] The new BATS engine tests are well-designed: they cover tag/untag (tagged, dangling, in-use), volume lifecycle, container lifecycle, `spec.state` transitions including the paused variants, cleanup-on-shutdown, the containerd `NotApplicable` path, and the `rdd set` timeout semantics. The comment at `engine.bats:272-275` ("park spec.state=unknown before pausing") is a good example of documenting test-only gotchas.

Untested scenarios ranked by risk:

1. **`rdd set running=true <extra-prop>=<value>` when already running** (C1). The simple start/stop flow doesn't exercise the steady-state condition path, which is why the critical regression landed unnoticed. Add a test that sets an unrelated property with `running=true` from the running state and asserts the command returns well before the default timeout.
2. **`rdd set` against `StartFailed`/`ConnectFailed`/`StopFailed`** (I3). The new default wait has zero coverage on failure paths — the tests only exercise success paths.
3. **`docker rename` event path** (I4). No test renames a container and asserts `.status.name` refreshes.
4. **`containernamespace/moby` recreation** (I1). The current test at `engine.bats:364-370` asserts the broken "stays gone" invariant. Rewrite it to assert the namespace *returns* after a subsequent `docker run`.
5. **Engine watcher transient disconnect** (related to I6). No test kills dockerd inside the Lima VM (e.g. `limactl shell rdd-bats-lima systemctl restart docker`) to verify the watcher dies, the reconciler restarts it, and mirrors re-converge without data loss.
6. **Transient Docker `Inspect` failures during `syncAllImages`** (I5). A unit test with a fake Docker client that returns 503 on selected IDs would demonstrate the current mirror-deletion bug.
7. **Concurrent `rdd set` races.** No test exercises the `IsAlreadyExists` retry at `set.go:354-362` that the "two rdd sets" comment handles.
8. **In-flight Docker events during `fullSync`.** The `dockerEventsSince` window at `docker_watcher.go:71-90` is critical for correctness but has no test that creates/removes resources *during* the watcher startup window to verify replayed events land.

Codex ran `make` against the worktree during its review; the engine controller package tests were fine once `pkg/guestagent/lima-guestagent.gz` was built.

## Documentation Assessment

[Claude Opus 4.6] [Codex GPT 5.4]

- `docs/design/api_app.md` accurately captures the new `ContainerEngineReady` condition and its writer.
- `docs/design/api_containers.md:88-96` says the dockerd `moby` namespace "cannot be modified in any way." That's no longer true in the shipped behaviour (see I1), and the new BATS test cements the drift. Fix the doc alongside I1.
- `docs/design/cmd_app.md` documents `--timeout` and the wait-by-default semantics, but two things need updating once the fixes land: (1) the wait currently swallows failure reasons (I3), so the doc should describe the failure behaviour once fixed; (2) the "stricter than 'container engine disconnected'" phrasing could use a concrete example of when the distinction matters.
- `pkg/controllers/app/engine/controller.go:46-48` — the engine controller's `GetCRDData() string { return "" }` has no doc comment explaining why it returns empty. A one-liner helps future readers: the engine controller owns no CRDs because it mirrors into the `containers.rancherdesktop.io` types owned by the containers controller.
- The Go doc on `ContainerSpec.State` at `pkg/apis/containers/v1alpha1/container_types.go:56-64` is thorough. Good.

## Commit Structure

[Codex GPT 5.4] [Claude Opus 4.6] This PR is broader than "Container Engine watcher" — the engine watcher itself is bundled with a new CLI wait contract (`762c1e8`), a large CI support-bundle wrapper (`50bdc15`, `7c8f356`, `c484dab`, `cf4b90e`), a BATS fd-inheritance fix (`9669b4b`), and unrelated Lima template/runtime fixes (`32a0109`, `536ccb2`). Splitting the CI infrastructure into its own PR would make the engine controller history easier to bisect, but at round 5 the review history is already anchored here — flag for the next large feature rather than re-slicing this one. The "Address review round N findings" commits are the repo convention and are fine as-is.

## Acknowledged Limitations

[Claude Opus 4.6] [Codex GPT 5.4]

- **`DockerSocket()` hardcodes `.rd2`** — `pkg/instance/instance.go:127-135` and `bats/tests/32-app-controllers/engine.bats:13-14` both carry matching TODOs. Multi-instance BATS runs cannot share the same host until this moves to `ShortDir()`. Pre-existing state that the new engine controller inherits.
- **`rdd set` only waits on `running` property changes** — `cmd/rdd/set.go:463-466`. The TODO proposes a "Reconciled" condition on App with `observedGeneration` semantics; the current implementation only handles `running`. Limitation documented in `docs/design/cmd_app.md` as well.
- **`reconcileContainerSpecs` + `processFinalizers` O(N) cost** — `engine_reconciler.go:177-183`. See D1 above.
- **`containernamespace/moby` has no mirror finalizer** — `sync_volumes.go:56-61` explains the asymmetry (Docker has no engine object to delete on the reverse path). The "no finalizer" decision is sound; the missing piece is the reconciler-side re-create, which is what I1 flags.
- **`shutdownAllHostagents` sequential wait** — pre-existing TODO in the memory file, not introduced by this PR.
- **Windows signal/pgid semantics** — `limavm_lifecycle.go:483-494` adds a forward-looking note explaining that the pgid log field reflects Unix semantics and will need to change for Windows.
- **Docker-socket tests are non-Windows only** — `bats/tests/32-app-controllers/engine.bats:19-20`. Intentional.

## Unresolved Feedback

No unresolved inline review comments from earlier rounds — all of the PR's inline comments are marked resolved or addressed in the `Address review round N findings` commits.

---

## Agent Performance Retro

### [Claude]

Claude returned the most complete coverage — two important findings (I1, I2) and nine suggestions — and was the only agent to produce Design Observations. The two importants were both shared (with Codex on the `ContainerNamespace/moby` watch gap, with Gemini on the missing image/volume finalizer retry), but Claude reasoned through the SSA consequences in each case better than the other two. Its biggest miss was the critical C1 bug — the analysis touched `setEngineCondition` and the generation filter separately but never connected them to the steady-state `wantWatcher=true && watcherRunning=true` path. Claude also missed I3/I4/I5/I6. The per-file coverage table was exhaustive, the suggestions were tightly scoped (no speculative refactors), and every finding carried a concrete diff.

### [Codex]

Codex found three unique important issues: the `ContainerNamespace/moby` watch gap (shared with Claude), the `rdd set` failure-state mishandling (I3), and the dropped `docker rename` event (I4). Both I3 and I4 were missed by the other two agents, and I4 in particular is the kind of event-taxonomy bug that's only visible if you go read the Docker SDK event enum — Codex did. It ran `make` against the worktree and briefly hit a build error (missing `pkg/guestagent/lima-guestagent.gz`) which it recovered from. Zero suggestions and zero design observations, which is on-brand — Codex stays at the important-bug bar and doesn't pad. Missed C1, I2, I5, I6.

### [Gemini]

Gemini was the only agent to catch C1 — the critical `rdd set` hang — and its reasoning on how the generation filter interacts with the `!watcherRunning` branch was tight and correct. It also found I5 (transient `Inspect` failures deleting valid Image mirrors) and I6 (dropped events on `handleEvent` errors), both of which the other two agents missed. Gemini hit a tool error during its shell-call loop — one bash invocation mis-parsed a chunk of the `bats-with-timeout.sh` patch as a filename, producing `ENAMETOOLONG: name too long, stat '/private/var/folders/.../review-gemini-pass-1-.../ &\n+cmd_pid=$!\n...'`. The error didn't block the review; findings still landed. Zero suggestions and zero design observations. Per known behaviour, Gemini skipped `git blame` for regression classification, but the findings were all clearly regression-scoped from the diff alone.

### Summary

| | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 21m 49s | 6m 37s | 4m 28s |
| Findings | 2I 9S | 3I | 1C 3I |
| Tool calls | 72 (Read 36, Bash 19, Grep 9) | 45 (shell 42, stdin 3) | — |
| Design observations | 2 | 0 | 0 |
| False positives | 0 | 0 | 0 |
| Unique insights | 9 | 2 | 3 |
| Files reviewed | 30 | 30 | 30 |
| Coverage misses | 0 | 0 | 0 |
| **Totals** | **2I 9S** | **3I** | **1C 3I** |
| Downgraded | 0 | 0 | 0 |
| Dropped | 0 | 0 | 0 |


**Overall assessment.** Gemini was the highest-value agent this round — it alone caught the critical regression that would hang the new default wait, and it also found two important issues nobody else saw. Codex matched it on unique-important count (I3, I4) and did the deepest event-taxonomy work. Claude delivered the broadest coverage and the most suggestions, but on the critical path it missed what Gemini caught. Without Gemini, the critical C1 bug would have shipped; without Codex, the failure-state wait and the rename event would have shipped. All three were necessary to get the full picture this round.

## Review Process Notes

### Skill improvements

None this round. The critical finding C1 was caught by one agent; the missed-by-two-agents pattern is already handled by the "unique findings" heuristic in the prompt. No new dimension or instruction would have helped Claude or Codex see C1 without also increasing noise.

### Repo context updates

None this round. The existing `deep-review-context.md` adequately framed the cross-controller status-condition mechanics (the part that matters for C1) and the SSA field-manager patterns (the part that matters for I2). Both agents that missed C1 had access to the same context; the miss was an analysis gap, not a context gap.

---

## Appendix: Original Reviews

### [Claude] — Claude Opus 4.6

I'll start by reading the key files in the PR to build context for the review.

Now let me check critical parts—first the Container type definitions and the apply config to verify behavior:

Now let me check the crucial concern around Status.ID matching for image removal, and verify what Docker actually sends for the ActionDelete:

Now let me trace some critical paths. First, I want to confirm the Docker daemon event behavior for `ActionDelete` - what does the event payload look like vs `ActionUnTag`?

Now let me examine a potential issue: how the dockerWatcher handles reconcileImageByID and what happens with `removeImagesByID` vs what Status.ID looks like for a newly-created Image mirror. Are image IDs stored with sha256: prefix or not?

Now let me look at concerning patterns around DryRun and a potential bug:

Now I have a complete picture. Let me verify one more potential issue concerning the rdd set wait semantics with a quick trace.

Now I have a complete picture. Let me write the review.

## Executive Summary

PR #295 introduces an engine controller that mirrors Docker engine state (containers, images, volumes) into `Container`, `Image`, and `Volume` resources in the `containers.rancherdesktop.io` API group, plus a `rdd set --timeout` wait-for-ready flow, CI diagnostics, and a new container `spec.state=unknown` admission path. The implementation is careful and well-commented; findings are modest (two importance-level correctness gaps and several suggestions), and no critical regressions were identified.

## Findings

### Critical Issues

None.

### Important Issues

**I1. `processImageFinalizers`/`processVolumeFinalizers` don't retry-on-conflict and write stale objects** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:431-492`

```go
// processImageFinalizers
if removeFinalizer(img, mirrorFinalizer) {
    if err := client.IgnoreNotFound(r.Update(ctx, img)); err != nil {
        errs = append(errs, fmt.Errorf("failed to remove finalizer from Image %s: %w", img.Name, err))
    }
}
```

Unlike `processContainerFinalizers` (engine_reconciler.go:410-423) which re-Gets under `retry.RetryOnConflict` before stripping the finalizer, both `processImageFinalizers` (line 454-458) and `processVolumeFinalizers` (line 485-489) mutate the stale `img`/`v` from the list and Update directly. On a 409 Conflict — which happens any time another writer (e.g., an engine watcher sync that just re-applied status) touches the object between our List and Update — the Update fails. The Docker-side delete has already succeeded by that point, so the next reconcile will retry `deleteImage`/`deleteVolume` from scratch (wasting an engine round-trip) before attempting the finalizer strip again. This is self-healing but loses the invariant "once we tell Docker to delete, we strip the finalizer" on the first attempt, and it's inconsistent with the adjacent Container path that does use retry.

Fix: mirror `processContainerFinalizers`' retry-on-conflict pattern. Put the finalizer strip inside a retry loop that re-Gets `latest`:

```diff
-        if removeFinalizer(img, mirrorFinalizer) {
-            if err := client.IgnoreNotFound(r.Update(ctx, img)); err != nil {
-                errs = append(errs, fmt.Errorf("failed to remove finalizer from Image %s: %w", img.Name, err))
-            }
-        }
+        key := client.ObjectKeyFromObject(img)
+        retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+            latest := &containersv1alpha1.Image{}
+            if err := r.Get(ctx, key, latest); err != nil {
+                if apierrors.IsNotFound(err) {
+                    return nil
+                }
+                return err
+            }
+            if !removeFinalizer(latest, mirrorFinalizer) {
+                return nil
+            }
+            return r.Update(ctx, latest)
+        })
+        if retryErr != nil {
+            errs = append(errs, fmt.Errorf("failed to remove finalizer from Image %s: %w", img.Name, retryErr))
+        }
```

Apply the same change to `processVolumeFinalizers` (lines 485-489). Classification: regression (new code in this PR), important.

**I2. `ContainerNamespace` mirror is not re-created after user delete until a watcher restart** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:505-512` and `pkg/controllers/app/engine/controllers/sync_volumes.go:62-67`

```go
// engine_reconciler.go:505-512 — SetupWithManager
return ctrl.NewControllerManagedBy(mgr).
    For(&appv1alpha1.App{}).
    Named("engine-reconciler").
    WatchesRawSource(source.Channel(r.reconcileChan, enqueueApp)).
    Watches(&containersv1alpha1.Container{}, enqueueApp).
    Watches(&containersv1alpha1.Image{}, enqueueApp).
    Watches(&containersv1alpha1.Volume{}, enqueueApp).
    Complete(r)
```

The engine reconciler watches `Container`, `Image`, `Volume`, but not `ContainerNamespace`. `syncContainerNamespace` (`sync_volumes.go:62`) is only called from `fullSync` (`docker_watcher.go:455-480`), which runs once when the watcher starts. The test at `bats/tests/32-app-controllers/engine.bats:364-370` explicitly verifies that deleting `containernamespace/moby` succeeds without a finalizer hang — and documents that it stays deleted. A user who runs `rdd ctl delete containernamespace/moby` then expects Docker operations to continue; the next container/image/volume event will enqueue a reconcile, but that reconcile only runs `reconcileContainerSpecs` and `processFinalizers` — neither recreates the namespace. Creating a new container *through Docker* triggers an `ActionCreate` container event, which calls `syncContainer` → `applyContainer`, which never recreates the `ContainerNamespace` either. So the namespace stays gone until the VM is restarted.

This is a gap introduced by this PR because the design doc (`docs/design/api_containers.md:88-96`) says `ContainerNamespace` reflects engine namespaces and (for `dockerd`) `"cannot be modified in any way"` — but the admission/webhook story for `ContainerNamespace` doesn't actually prevent deletion, and the engine reconciler doesn't watch it.

Fix (choice of two): either (a) add `Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp)` in `SetupWithManager` and make the reconciler idempotently re-run `ensureNamespace` + `syncContainerNamespace` on every engine reconcile (it's one Apply, cheap and already idempotent); or (b) if deletion is meant to be rejected, add an admission webhook denying non-internal deletes. Option (a) is the smaller change and matches the existing mirror pattern. Classification: gap, important.

### Suggestions

**S1. `stopWatcher` inline in `Reconcile` holds `watcherMu` during `w.stop()`** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:93-101`

```go
r.watcherMu.Lock()
watcherRunning := r.watcher != nil
watcherDied := watcherRunning && !r.watcher.alive()
if watcherDied {
    r.watcher.stop()      // <-- blocks on <-w.done and cli.Close() under the lock
    r.watcher = nil
    watcherRunning = false
}
r.watcherMu.Unlock()
```

Compare with the standalone `stopWatcher()` helper at lines 212-221, which correctly releases the lock *before* calling `w.stop()`:

```go
func (r *EngineReconciler) stopWatcher() {
    r.watcherMu.Lock()
    w := r.watcher
    r.watcher = nil
    r.watcherMu.Unlock()

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

The inline version in `Reconcile` is a latent bug only with `MaxConcurrentReconciles > 1`; the engine reconciler currently runs single-threaded, so no concurrent reconciler blocks on the lock today. Still, the asymmetry is a trap — anyone who later enables concurrent reconciles will hit a lock held across a blocking `cli.Close()`.

Fix: match the pattern in `stopWatcher`:

```diff
     r.watcherMu.Lock()
     watcherRunning := r.watcher != nil
-    watcherDied := watcherRunning && !r.watcher.alive()
+    var diedWatcher *dockerWatcher
+    if watcherRunning && !r.watcher.alive() {
+        diedWatcher = r.watcher
+        r.watcher = nil
+        watcherRunning = false
+    }
+    r.watcherMu.Unlock()
+    watcherDied := diedWatcher != nil
     if watcherDied {
-        r.watcher.stop()
-        r.watcher = nil
-        watcherRunning = false
+        diedWatcher.stop()
     }
-    r.watcherMu.Unlock()
```

Classification: gap, suggestion.

**S2. `ImageStatus.RepoDigests` is sent to SSA without sorting** — `pkg/controllers/app/engine/controllers/sync_images.go:151-164`

```go
statusApply := containersv1alpha1apply.ImageStatus().
    WithID(inspect.ID).
    WithRepoDigests(inspect.RepoDigests...).       // not sorted
```

`ImageStatus.RepoDigests` in `pkg/apis/containers/v1alpha1/image_types.go:28-31` is a plain `[]string` with no `+listType` annotation, which makes it atomic under SSA. Docker's `RepoDigests` are sourced from the reference store and are not documented to be in any guaranteed order; an image with two digest references (one from pull, one from push) could reorder between inspect calls. Any reorder produces a new `resourceVersion` on every status apply, which fires watch events and triggers engine-reconciler reconciles — matching the exact anti-pattern the review prompt warns against.

Fix:

```diff
+    digests := slices.Clone(inspect.RepoDigests)
+    sort.Strings(digests)
     statusApply := containersv1alpha1apply.ImageStatus().
         WithID(inspect.ID).
-        WithRepoDigests(inspect.RepoDigests...).
+        WithRepoDigests(digests...).
```

Classification: gap, suggestion.

**S3. `ContainerPort.Bindings` is sent to SSA in Docker's order** — `pkg/controllers/app/engine/controllers/sync_containers.go:192-214`

```go
for _, port := range ports {
    bindings = append(bindings, containersv1alpha1apply.ContainerPortBinding().
        WithHostIP(port.HostIP.String()).
        WithHostPort(port.HostPort))
}
applyPorts = append(applyPorts, containersv1alpha1apply.ContainerPort().
    WithName(portName.String()).
    WithBindings(bindings...))
```

`ContainerPort.Bindings` in `pkg/apis/containers/v1alpha1/container_types.go:48-52` is `[]ContainerPortBinding` with no `+listType`, atomic under SSA. The outer `portNames` slice is correctly sorted at line 198, but the inner bindings follow Docker's iteration order. If Docker ever returns IPv6 before IPv4 (or changes this ordering between calls), consumers see spurious resourceVersion bumps. Same fix as S2 — sort `bindings` by `(HostIP, HostPort)` before constructing the apply config.

Classification: gap, suggestion.

**S4. Ambiguous "timed out waiting for App state" error** — `cmd/rdd/set.go:535-543`

```go
if _, err := watchtools.UntilWithSync(...); err != nil {
    if errors.Is(err, context.DeadlineExceeded) {
        return errors.New("timed out waiting for App state")
    }
```

The error message does not tell the user *which* state was expected. When `rdd set running=true` times out at 5 minutes, the user sees "timed out waiting for App state" without any hint that the wait was specifically for `ContainerEngineReady=True`. Consider plumbing a descriptive label into `watchCondition` and returning something like `"timed out waiting for ContainerEngineReady=True (5m)"` or `"timed out waiting for App to stop (5m)"`. Classification: gap, suggestion.

**S5. `deleteAllOfType` keeps the old `obj` for the final `r.Delete` call** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:299-328`

```go
for _, item := range items {
    obj := item.(client.Object)
    key := client.ObjectKeyFromObject(obj)
    ...
    retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
        latest := obj.DeepCopyObject().(client.Object)
        ...
    })
    ...
    if err := client.IgnoreNotFound(r.Delete(ctx, obj)); err != nil {
```

The final `r.Delete(ctx, obj)` at line 324 passes the stale list item rather than `latest`. That's functionally fine — `Delete` keys off name+namespace by default and doesn't send `resourceVersion` — but it's subtle and the closure-scoped `latest` isn't visible outside the retry body. Future maintainers might reasonably think they need to pass `latest`. Either hoist `latest` out of the closure or add a one-line comment at line 324 explaining that `obj` is acceptable because Delete doesn't use resourceVersion. Classification: gap, suggestion.

**S6. `DockerSocket()` calls `os.MkdirAll` at construction time** — `pkg/instance/instance.go:136-144`

```go
var DockerSocket = sync.OnceValue(func() string {
    home, err := os.UserHomeDir()
    if err != nil {
        panic(fmt.Errorf("could not get home directory: %w", err))
    }
    dir := filepath.Join(home, ".rd2")
    _ = os.MkdirAll(dir, 0o700)
    return filepath.Join(dir, "docker.sock")
})
```

`DockerSocket()` performs filesystem side effects (`os.MkdirAll`) and swallows the error, unlike the other `instance.*` accessors next to it (`Dir`, `LogDir`, `LimaHome`, etc.) which are pure path computations. A callsite like `instance.DockerSocket()` in CLI help paths or `rdd svc paths` would silently create the directory as a side effect. Consider either (a) having the *watcher* (`newDockerWatcher`) call `os.MkdirAll` on demand, with proper error handling, or (b) if `DockerSocket()` must create the directory, rename it to `EnsureDockerSocket()` and return the error. Note also that this will need to change when the hardcoded `".rd2"` moves to `ShortDir()` per the TODO. Classification: gap, suggestion.

**S7. `watchCondition` allocates a new dynamic client per call** — `cmd/rdd/set.go:501-516`

`watchCondition` constructs a fresh `dynamic.NewForConfig(config)` on every invocation. For `rdd set` this is fine (one call per command), but if the helper is reused elsewhere it could become wasteful. Suggestion: pass the dynamic client in rather than constructing it, and reuse the controller-runtime client from `setAction` where possible. Classification: enhancement, suggestion.

**S8. Unreachable `default` branch in the state-transition webhook** — `pkg/controllers/containers/container/container_webhooks.go:36-54`

```go
if oldContainer.Spec.State != newContainer.Spec.State {
    switch newContainer.Spec.State {
    case v1alpha1.ContainerStatusCreated, v1alpha1.ContainerStatusRunning, v1alpha1.ContainerStatusUnknown:
    default:
        return nil, fmt.Errorf("invalid target state %q: must be %q, %q, or %q", ...)
    }
```

CRD-level enum validation (`crd.yaml:285-288` — `enum: [created, running, unknown]`) runs *before* the webhook, so the `default` branch is unreachable through normal admission paths. The defensive check is not wrong, but the second `equality.Semantic.DeepEqual` check at line 51 is also dead code for the current `ContainerSpec` shape (which has only the `State` field). When a new field is added to `ContainerSpec`, the DeepEqual check becomes load-bearing and must be kept; for now it's acceptable. Consider a one-line comment noting the check is there for future fields — or delete both dead branches and trust the CRD enum until more fields arrive. Classification: enhancement, suggestion.

**S9. `reconcileContainerState` silently drops unknown desired states** — `pkg/controllers/app/engine/controllers/docker_watcher.go:361-392`

```go
switch desired {
case containersv1alpha1.ContainerStatusRunning:
    ...
case containersv1alpha1.ContainerStatusCreated:
    ...
}
return nil
```

If `desired` is anything other than `unknown`/`running`/`created`, the switch silently returns nil. Today the webhook prevents this, but a future change that loosens the enum or a direct storage manipulation would produce a silent no-op. Add a default case that logs an unexpected state so regressions surface quickly. Classification: gap, suggestion.

## Design Observations

### Concerns

**D1. `reconcileContainerSpecs` + `processFinalizers` fan out on every mirror event (in-scope, comment-acknowledged).** The existing comment at `engine_reconciler.go:177-183` accurately describes the cost — every Container/Image/Volume watch event triggers a reconcile, which in turn Lists every mirrored object to check for spec.state changes and pending finalizers. For large container counts this is O(N²) across a sync burst. The long-term fix the comment proposes (per-resource reconcilers with watch predicates for `deletionTimestamp!=nil` and `spec.state` changes) is correct. The near-term impact is bounded because typical dev machines carry few containers, but this will need attention before the feature supports larger workloads. Classification: future.

**D2. Two field managers ("engine-controller" and "engine-controller-finalizer") to work around SSA spec ownership (in-scope).** The explanation at `sync_containers.go:144-157` is correct but it points at a design wart: server-side apply couldn't express "update only finalizers" without releasing spec from the original owner, so a second field manager was introduced. An alternative — keep SSA only for the creation path and switch to a strategic merge patch for finalizer re-assertion — would avoid the two-manager split and match how most controllers add finalizers. The current approach works; flag for future simplification. Classification: future.

### Strengths

- The `minGen`/`ObservedGeneration` scheme in `rdd set` correctly solves the stale-condition-snapshot problem. `setEngineCondition`'s `retry.RetryOnConflict` + re-Get pattern is the right fix for the cross-controller write race on `App.Status.Conditions`.
- The event replay window is well-thought-out: `dockerEventsSince` uses the Docker daemon's own clock (`Info.SystemTime`) rather than `time.Now()`, explicitly addressing host/guest time skew. The fallback to a biased host clock on query failure is a sensible safety net.
- `applyContainer`'s "spec on create only, status on every sync" split with a `Get`-gated `WithSpec` call correctly preserves user spec.state patches without needing a second field manager for spec.
- The dockerWatcher's `alive()` / `watcherDied` handling in `Reconcile` cleanly separates transient watcher death (reconnect via fresh fullSync) from terminal shutdown (sweep mirrors). The fall-through after `startWatcher` to run `reconcileContainerSpecs` + `processFinalizers` is a nice detail — it avoids stalling on pending state in the "reconnect" path.
- `reconcileContainerState`'s explicit state matrix in the doc comment, including why paused must be handled for the `running` branch but not for the `created` branch, is exactly the kind of documentation this code needs. The BATS tests at lines 264-320 verify both directions.
- The BATS engine tests cover the non-obvious cases (dangling image, in-use image finalizer retention, containerd NotApplicable, paused→created/running transitions). The "park spec.state=unknown before pausing" comment at `engine.bats:272-275` is a good example of documenting test-only gotchas.

## Testing Assessment

New functionality is well covered by the BATS engine tests:
- image tag/untag (tagged, dangling, in-use), volume lifecycle, container lifecycle, spec.state transitions (including paused), cleanup-on-shutdown, containerd NotApplicable path, rdd-set timeout semantics.

Untested scenarios (ranked by risk):

1. **Engine watcher transient disconnect and reconnect** — no test exercises the "watcher dies while App is running" path (`engine_reconciler.go:95-113`). This matters because the live reconnect path is what users hit when the Docker daemon is restarted inside the VM. Could be tested by killing dockerd inside the Lima VM (e.g. `systemctl restart docker` via limactl shell) and verifying mirrors reconcile without data loss.
2. **Concurrent `rdd set` calls** — no test verifies the race handled at `set.go:354-362` (IsAlreadyExists after Create). The "two rdd sets" comment is plausible but unverified.
3. **In-flight Docker events during fullSync** — the "since" window logic at `docker_watcher.go:71-90` is critical for correctness, but no test creates/removes resources *during* the watcher startup window to verify events aren't dropped.
4. **Finalizer retry on 409 conflict** — no unit or integration test covers the retry path in `processContainerFinalizers` (or would show I1 as a failure). A fake-client test that injects a conflict on Update would demonstrate the retry behavior.
5. **Namespace recreate after user delete** (tied to I2) — the current test verifies deletion completes but not that subsequent container creation recreates the `ContainerNamespace`. Add a test that deletes `containernamespace/moby`, runs `docker run -d …`, and asserts the namespace mirror returns.

## Documentation Assessment

- `docs/design/api_app.md` accurately captures the new `ContainerEngineReady` condition and its writer (engine reconciler); the table additions are complete.
- `docs/design/api_containers.md` adds a well-written Terminology section and Engine Mirroring section, and the Container state-transition semantics are explained clearly.
- `docs/design/cmd_app.md` documents `--timeout` and the wait-by-default semantics, but the description of "*stricter than 'container engine disconnected'*" could use a concrete example of when that distinction matters.
- `pkg/apis/containers/v1alpha1/container_types.go:56-64` — the Go doc comment on `ContainerSpec.State` is thorough. Good.
- No changelog file exists in this repo, so the docs/design updates serve as the public record.

One doc gap: the engine controller's `GetCRDData() string { return "" }` at `pkg/controllers/app/engine/controller.go:46-48` has no doc comment explaining why it returns empty. Since the engine controller owns no CRDs (it only consumes `Container`/`Image`/`Volume` from another package), a one-line comment would help: `// GetCRDData returns empty because the engine controller does not own any CRDs — it mirrors into the containers.rancherdesktop.io types owned by the containers controller.`

## Commit Structure

Commit history is clean:
- The functional commits (`32a0109`, `536ccb2`, `87f1df1`, `93359f7`, `4db3f20`, `65943da`, `762c1e8`, `ba6caf7`, `84c0736`, `c8fa377`) each represent a single concept with clear messages.
- The "Address review round N findings" commits (`66eb50e`, `33db27d`, `e2df2f9`) are legitimate review-response commits kept separate for rebase reviewability — per the repo convention Jan documented.
- The CI / BATS infrastructure commits (`50bdc15`, `9669b4b`, `7c8f356`, `62d222a`, `c484dab`, `cf4b90e`) are arguably a separate concern from the engine controller PR; splitting them would make the engine controller history easier to bisect. Not a blocker for a PR this size, but worth considering for future features.

## Acknowledged Limitations

- **`DockerSocket()` hardcodes `.rd2`** — `pkg/instance/instance.go:127-135` and `bats/tests/32-app-controllers/engine.bats:13-14` both contain matching TODO comments explaining the coupling between the Go side and the Lima template. The comment explicitly flags that "multi-instance BATS runs cannot share the same host" until this is fixed. The PR does not materially worsen this limitation — it's a pre-existing state that the new engine controller inherits.
- **`rdd set` only waits on `running` property changes** — `cmd/rdd/set.go:463-466` has an explicit TODO noting that `containerEngine.name` changes without `running` don't trigger a wait. Addressed in the `docs/design/cmd_app.md` text as well. The limitation is consistent with the implementation.
- **`reconcileContainerSpecs` + `processFinalizers` O(N) cost** — `engine_reconciler.go:177-183` acknowledges that per-resource reconcilers are the long-term fix; current cost is bounded by typical dev machine container counts.
- **`containerNamespace/moby` has no mirror finalizer** — `sync_volumes.go:56-61` explains why: Docker has no corresponding engine object to delete on the reverse path. Note: this is consistent with I2 above — the "no finalizer" decision is sound; the missing piece is the reconciler-side re-create on watch event, which is what I2 flags.
- **`shutdownAllHostagents` sequential wait** — pre-existing TODO documented in the memory file, not introduced by this PR.
- **Windows signal/pgid semantics** — `limavm_lifecycle.go:483-494` adds a note explaining that the pgid log field reflects Unix semantics and will need to change for Windows. This is a forward-looking note, not a current bug.

## Coverage Summary

| File | Status |
|---|---|
| `.github/workflows/bats.yaml` | Reviewed, no issues — Spotlight disable is a sensible CI fix |
| `bats/Makefile` | Reviewed, no issues |
| `bats/helpers/commands.bash` | Reviewed, no issues — fd close is a targeted fix for the known BATS-grandchild-inheritance issue |
| `bats/tests/32-app-controllers/engine.bats` | Reviewed — see Testing Assessment for untested scenarios |
| `cmd/app-controller/main.go` | Trivial |
| `cmd/rdd/set.go` | S4, S7 |
| `docs/design/api_app.md` | Reviewed, no issues |
| `docs/design/api_containers.md` | Reviewed — minor prose note in Documentation Assessment |
| `docs/design/cmd_app.md` | Reviewed, no issues |
| `go.mod`, `go.sum` | Reviewed, no issues — dependencies are appropriate |
| `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerspec.go` | Trivial (generated) |
| `pkg/apis/containers/v1alpha1/container_types.go` | Reviewed, no issues — well-documented comment |
| `pkg/apis/containers/v1alpha1/volume_types.go` | Reviewed, no issues |
| `pkg/controllers/app/app/controllers/app_controller.go` | Reviewed, no issues — retry.RetryOnConflict pattern is correct for the cross-writer race |
| `pkg/controllers/app/app/lima-template.yaml` | Reviewed, no issues |
| `pkg/controllers/app/engine/controller.go` | Documentation gap noted in Documentation Assessment |
| `pkg/controllers/app/engine/controllers/docker_watcher.go` | S3 (bindings sort), no other issues; event handling and since-window logic are correct |
| `pkg/controllers/app/engine/controllers/engine_reconciler.go` | I1, I2, S1, S5 |
| `pkg/controllers/app/engine/controllers/sync_containers.go` | S3 |
| `pkg/controllers/app/engine/controllers/sync_containers_test.go` | Reviewed, no issues — good unit test |
| `pkg/controllers/app/engine/controllers/sync_images.go` | S2 |
| `pkg/controllers/app/engine/controllers/sync_volumes.go` | I2 (nearby, re: syncContainerNamespace) |
| `pkg/controllers/containers/container/container_webhooks.go` | S8 |
| `pkg/controllers/containers/container/crd.yaml` | Trivial (generated) |
| `pkg/controllers/containers/volume/crd.yaml` | Trivial (generated) |
| `pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go` | Reviewed, no issues — pgid log field explanation is good |
| `pkg/instance/instance.go` | S6 |
| `pkg/service/cmd/service.go` | Trivial |
| `scripts/bats-with-timeout.sh` | Reviewed, no issues — careful implementation with sibling-target isolation |

Review complete. The PR adds an engine controller that mirrors Docker state into Kubernetes resources, and is generally in good shape — no critical regressions. I flagged two important issues: (I1) `processImage`/`processVolumeFinalizers` don't use retry-on-conflict despite the adjacent Container code doing so, and (I2) `ContainerNamespace/moby` isn't watched, so a user delete isn't re-mirrored until the next VM restart. Plus nine suggestions covering SSA list-ordering, lock-while-closing patterns, error-message specificity, and the `DockerSocket()` side-effect.
---

### [Codex] — Codex GPT 5.4

### Executive Summary

This PR adds the engine watcher, Docker-to-Kubernetes mirroring, reverse finalizer handling, and a new default wait path in `rdd set`, plus a sizeable BATS/CI support bundle improvement. The main regressions are a missing watch path for `ContainerNamespace/moby`, `rdd set` wait predicates that mis-handle terminal failure states, and an omitted Docker `rename` event that leaves mirrored container names stale.

### Findings

**Critical Issues**

None.

**Important Issues**

I1. **`ContainerNamespace/moby` can be deleted permanently while dockerd is running** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:505-512` `(important, regression)`

```go
return ctrl.NewControllerManagedBy(mgr).
    For(&appv1alpha1.App{}).
    Named("engine-reconciler").
    WatchesRawSource(source.Channel(r.reconcileChan, enqueueApp)).
    Watches(&containersv1alpha1.Container{}, enqueueApp).
    Watches(&containersv1alpha1.Image{}, enqueueApp).
    Watches(&containersv1alpha1.Volume{}, enqueueApp).
    Complete(r)
```

The watcher creates `containernamespace/moby` in `syncContainerNamespace()` at `pkg/controllers/app/engine/controllers/sync_volumes.go:62-66`, and the design doc says the dockerd backend has exactly one valid namespace and that it “cannot be modified in any way” at `docs/design/api_containers.md:90-92`. But the controller never watches `ContainerNamespace` objects at `pkg/controllers/app/engine/controllers/engine_reconciler.go:505-512`, so a user delete produces no follow-up reconcile and no Docker event can wake the controller back up. The new test at `bats/tests/32-app-controllers/engine.bats:364-369` hard-codes that broken behavior by asserting the namespace stays gone. After that delete, mirrored `Container`, `Image`, and `Volume` objects can still advertise `status.namespace=moby`, but the referenced `ContainerNamespace` object is absent indefinitely until some unrelated App event or a full VM restart happens.

Fix: either reject deletion of `containernamespace/moby` while `running && engineIsDocker`, or add a `Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp)` path and re-apply `moby` during reconcile whenever the engine watcher should be active.

I2. **`rdd set` wait treats failure states as success or hangs until timeout** — `cmd/rdd/set.go:476-492` `(important, regression)`

```go
if runningVal == "true" {
    return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
        status, gen, present := conditionInfo(obj, "ContainerEngineReady")
        return present && gen >= minGen && status == metav1.ConditionTrue
    })
}
return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
    status, gen, present := conditionInfo(obj, "Running")
    if !present {
        return true
    }
    return gen >= minGen && status != metav1.ConditionTrue
})
```

The start-side predicate at `cmd/rdd/set.go:479-480` only accepts `ContainerEngineReady=True`. If startup fails and the Lima controller publishes `Running=False/StartFailed` at `pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go:382-385`, `427-430`, or `463-476`, or if the engine reconciler publishes `ContainerEngineReady=False/ConnectFailed|Stopped` at `pkg/controllers/app/engine/controllers/engine_reconciler.go:130-154`, `rdd set` keeps waiting until the outer timeout and then returns a generic timeout instead of the actual failure. The stop-side predicate at `cmd/rdd/set.go:488-492` has the opposite problem: it treats any refreshed non-`True` `Running` condition as success, so `Running=False/StopFailed` from `pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go:527-539` satisfies the wait even when the instance is still running. That makes the new default wait misleading on both failure paths.

Fix: inspect `reason` and `message` as well as `status`. `running=true` should fail immediately on fresh terminal failures such as `StartFailed`, `ConnectFailed`, or a fresh `Stopped`; `running=false` should succeed only on `ReasonStopped` (or an absent `Running` condition for “never started”) and should return an error on `StopFailed`.

I3. **Container renames never reach the mirror** — `pkg/controllers/app/engine/controllers/docker_watcher.go:227-240` `(important, regression)`

```go
switch msg.Action {
case events.ActionCreate,
    events.ActionStart,
    events.ActionStop,
    events.ActionDie,
    events.ActionPause,
    events.ActionUnPause,
    events.ActionRestart:
    return w.syncContainer(ctx, msg.Actor.ID)
case events.ActionDestroy:
    return w.removeMirrorResource(ctx, &containersv1alpha1.Container{}, msg.Actor.ID)
}
```

`applyContainer()` derives the mirrored container name from `inspect.Name` at `pkg/controllers/app/engine/controllers/sync_containers.go:111` and writes it into status at `pkg/controllers/app/engine/controllers/sync_containers.go:161-163`, but the Docker event switch at `pkg/controllers/app/engine/controllers/docker_watcher.go:227-240` ignores `events.ActionRename` entirely. Docker does emit that action, so `docker rename` updates the engine object without triggering `syncContainer()`. Because the reconciler then returns `ctrl.Result{}, nil` and there is no Kubernetes-side watch event for an engine-only rename, `.status.name` can stay stale indefinitely until some unrelated create/start/stop/destroy event happens to resync the container.

Fix: handle `events.ActionRename` the same way as the other state-refreshing container events and add an integration test that renames a running container and waits for `.status.name` to change.

**Suggestions**

None.

### Design Observations

**Strengths**

- Using the daemon’s `Info().SystemTime` in `pkg/controllers/app/engine/controllers/docker_watcher.go:71-90` is the right fix for the host/guest clock-skew hazard around Docker `since` filters.
- The new conflict-retry pattern in `pkg/controllers/app/app/controllers/app_controller.go:239-259` and `pkg/controllers/app/engine/controllers/engine_reconciler.go:230-255` correctly addresses the concurrent `App.Status.Conditions` writers.

### Testing Assessment

1. No test exercises `rdd set` against `StartFailed`, `ConnectFailed`, or `StopFailed`, so the new default wait path is only covered on success paths and misses I2 completely.
2. The suite does not cover `docker rename`, so the stale-name regression in I3 is untested.
3. `bats/tests/32-app-controllers/engine.bats:364-369` asserts that deleting `containernamespace/moby` leaves it absent, which cements I1 instead of protecting the intended invariant.

I ran `make`, which completed successfully. A targeted `go test ./pkg/controllers/app/engine/controllers ./pkg/controllers/containers/container ./cmd/...` initially failed because `pkg/guestagent/lima-guestagent.gz` had not been built yet; after `make`, the engine/controller package tests were fine.

### Documentation Assessment

`docs/design/cmd_app.md` now promises that `rdd set` “waits for the desired state,” but the current predicates in I2 do not distinguish success from terminal failure. `docs/design/api_containers.md:90-92` says the dockerd `moby` namespace “cannot be modified in any way,” which no longer matches the shipped behavior or the new BATS test that expects it to disappear.

### Commit Structure

This PR is broader than “Container Engine watcher”. The engine watcher itself is mixed with a new CLI wait contract, a large CI support-bundle wrapper, and unrelated Lima template/runtime fixes (`32a0109`, `536ccb2`), which makes rollback and review more coupled than necessary.

### Acknowledged Limitations

- **Code comment**: “`TODO: changes that trigger a VM restart without changing "running" (e.g. containerEngine.name alone) are still not waited on.`” — `cmd/rdd/set.go:463-465`. This is still accurate; the PR documents the limitation, but it remains a user-visible gap.
- **Code comment**: “`TODO: use ShortDir() once the Lima template derives the socket path from the instance suffix instead of hardcoding ".rd2".`” — `pkg/instance/instance.go:127-135`. The engine watcher makes this more important because mirroring now depends on that host socket path being correct.
- **Code comment**: “`# TODO: use .rd${RDD_INSTANCE} once the Lima template derives the socket path from the instance suffix instead of hardcoding ".rd2".`” — `bats/tests/32-app-controllers/engine.bats:13-14`. The new engine BATS coverage inherits the same single-instance limitation.
- **Code comment**: “`# The Docker socket access pattern used by these tests is not yet wired up for Windows/WSL2.`” — `bats/tests/32-app-controllers/engine.bats:19-20`. The new integration coverage is intentionally non-Windows for now.

### Coverage Summary

- `.github/workflows/bats.yaml` — Reviewed, no issues
- `bats/Makefile` — Reviewed, no issues
- `bats/helpers/commands.bash` — Reviewed, no issues
- `bats/tests/32-app-controllers/engine.bats` — I1
- `cmd/app-controller/main.go` — Trivial
- `cmd/rdd/set.go` — I2
- `docs/design/api_app.md` — Reviewed, no issues
- `docs/design/api_containers.md` — I1
- `docs/design/cmd_app.md` — I2
- `go.mod` — Reviewed, no issues
- `go.sum` — Trivial
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerspec.go` — Reviewed, no issues
- `pkg/apis/containers/v1alpha1/container_types.go` — Reviewed, no issues
- `pkg/apis/containers/v1alpha1/volume_types.go` — Reviewed, no issues
- `pkg/controllers/app/app/controllers/app_controller.go` — Reviewed, no issues
- `pkg/controllers/app/app/lima-template.yaml` — Reviewed, no issues
- `pkg/controllers/app/engine/controller.go` — Reviewed, no issues
- `pkg/controllers/app/engine/controllers/docker_watcher.go` — I3
- `pkg/controllers/app/engine/controllers/engine_reconciler.go` — I1, I2
- `pkg/controllers/app/engine/controllers/sync_containers.go` — I3
- `pkg/controllers/app/engine/controllers/sync_containers_test.go` — Reviewed, no issues
- `pkg/controllers/app/engine/controllers/sync_images.go` — Reviewed, no issues
- `pkg/controllers/app/engine/controllers/sync_volumes.go` — I1
- `pkg/controllers/containers/container/container_webhooks.go` — Reviewed, no issues
- `pkg/controllers/containers/container/crd.yaml` — Reviewed, no issues
- `pkg/controllers/containers/volume/crd.yaml` — Reviewed, no issues
- `pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go` — I2
- `pkg/instance/instance.go` — Reviewed, no issues
- `pkg/service/cmd/service.go` — Trivial
- `scripts/bats-with-timeout.sh` — Reviewed, no issues

---

### [Gemini] — Gemini 3.1 Pro

### Executive Summary

This PR introduces the engine controller to mirror Docker's `Container`, `Image`, and `Volume` state into Kubernetes resources. The review identifies a critical bug where `rdd set` hangs because condition generation updates are bypassed for already-running instances. It also flags important data integrity issues on transient errors: dropped Docker events can permanently desynchronize state, and failed image inspects erroneously delete valid K8s mirrors.

### Findings

#### Critical Issues

**C1. `rdd set` hangs because `ContainerEngineReady` `ObservedGeneration` is not updated when the watcher is already running** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:121-167`

If the App is already running and the Docker watcher is active, updating a property (e.g. `rdd set running=true kubernetes.version=1.31.0`) increments the `App`'s `metadata.generation`. `waitForDesiredState` then loops, waiting for `ContainerEngineReady` to reach this new `minGen`. However, because `wantWatcher` and `watcherRunning` are both true, `EngineReconciler` falls through and bypasses the `setEngineCondition` call entirely. The condition's `ObservedGeneration` remains stale, and `rdd set` hangs until its 5-minute timeout.

Fix: Unconditionally assert the `Connected` condition when the watcher is desired and running. `setEngineCondition` already checks for changes, so this is a cheap no-op when stable but ensures `ObservedGeneration` matches the current `App` generation.

```diff
 	if !watcherRunning {
 		log.Info("App is running, starting Docker watcher")
 		if err := r.startWatcher(ctx); err != nil {
 			log.Error(err, "Failed to start Docker watcher")
 			if condErr := r.setEngineCondition(ctx, &app, metav1.ConditionFalse, "ConnectFailed", err.Error()); condErr != nil {
 				log.Error(condErr, "Failed to update ContainerEngineReady to ConnectFailed")
 			}
 			return ctrl.Result{}, err
 		}
-		if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
-			return ctrl.Result{}, err
-		}
 		// Fall through so pending spec.state patches and finalizers
 		// made during the watcher-down window are processed on this
 		// same reconcile, instead of waiting for a later event. The
 		// restart-after-crash path would otherwise stall if the
 		// ContainerEngineReady condition hadn't changed (setEngineCondition
 		// is a no-op) and no mirror watch event follows.
 	}
 
+	if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
+		return ctrl.Result{}, err
+	}
+
 	// reconcileContainerSpecs + processFinalizers each issue one or more
```
*(critical, regression)*

#### Important Issues

**I1. Docker watcher drops events on transient Kubernetes API errors** — `pkg/controllers/app/engine/controllers/docker_watcher.go:189-192`

In the `run` loop, if `w.handleEvent(ctx, msg)` fails (e.g., a transient 503 from the API server or a conflict when applying the status update), the error is merely logged and the event is dropped. Because the controller relies entirely on the event stream and does not periodically resync, the Kubernetes mirror becomes permanently out-of-sync with Docker.

Fix: Return from the loop on error. The deferred `recover()` block can be adjusted or the `w.enqueueReconcile()` call can be executed directly before returning. The reconciler will then observe the dead watcher, restart it, and perform a reliable `fullSync`.

```diff
 			if err := w.handleEvent(ctx, msg); err != nil {
-				log.Error(err, "Failed to handle Docker event",
+				log.Error(err, "Failed to handle Docker event; restarting watcher to resync",
 					"type", msg.Type, "action", msg.Action, "actor", msg.Actor.ID)
+				return
 			}
```
*(important, gap)*

**I2. Transient `Inspect` failures in `syncAllImages` cause valid Image mirrors to be erroneously deleted** — `pkg/controllers/app/engine/controllers/sync_images.go:59-64`

In `syncAllImages`, `syncImageFromSummary` inspects the image. If this Docker API call fails transiently, it logs the error and continues. The expected mirror names are never added to `activeNames`. The subsequent cleanup loop then erroneously considers the valid Kubernetes `Image` mirrors as stale and deletes them. 

Fix: Derive the expected mirror names from the `summary`'s `RepoTags` directly and add them to `activeNames` *before* calling `syncImageFromSummary`, ensuring transient inspect failures do not trigger mirror deletion.

```diff
 	var errs []error
 	for _, summary := range listResult.Items {
+		if len(summary.RepoTags) > 0 {
+			for _, tag := range summary.RepoTags {
+				activeNames[fmt.Sprintf("%s-%x", sanitizeKubernetesObjectName(summary.ID), sha256.Sum256([]byte(tag)))] = true
+			}
+		} else {
+			activeNames[sanitizeKubernetesObjectName(summary.ID)] = true
+		}
 		names, err := w.syncImageFromSummary(ctx, summary)
 		if err != nil {
 			log.Error(err, "Skipping image during full sync", "id", summary.ID)
 			continue
 		}
-		for _, n := range names {
-			activeNames[n] = true
-		}
 	}
```
*(important, bug)*

**I3. Missing `retry.RetryOnConflict` in `processImageFinalizers` and `processVolumeFinalizers`** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:440`

`processContainerFinalizers` rightly wraps finalizer removal in `retry.RetryOnConflict` to survive stale caches and concurrent updates without forcing a whole-reconcile requeue. `processImageFinalizers` and `processVolumeFinalizers` omit this wrapper. A transient conflict on `r.Update` will cause the reconciler to requeue and unnecessarily re-attempt the Docker API `deleteImage`/`deleteVolume` calls.

Fix: Apply the exact same `retry.RetryOnConflict` block used in `processContainerFinalizers` to the Image and Volume finalizer removal logic.
*(important, gap)*

#### Suggestions

None.

### Design Observations

**Strengths**
- The panic recovery mechanism in `dockerWatcher.run` correctly utilizes the `alive()` check during the `App` reconcile to safely restart a dead watcher and run a `fullSync`. This cleanly prevents permanent lockups or orphaned streams.
- The sorting of port mappings before passing them to the Kubernetes list (`sync_containers.go`) effectively circumvents spurious apply churn that usually plagues `+listType=map` fields.

### Testing Assessment

1. **Wait Condition Timeout (`rdd set`)**: The hanging behavior flagged in C1 lacks test coverage because the BATS tests run `rdd set running=true` after `rdd set running=false`. A test ensuring `rdd set running=true kubernetes.version=xxx` returns successfully when the VM is *already* running would prevent regressions.
2. **Transient API Errors**: There is no coverage for the watcher's ability to recover from dropped events or transient Docker socket timeouts.

### Documentation Assessment

Reviewed, no issues.

### Acknowledged Limitations

None.

### Coverage Summary

- `.github/workflows/bats.yaml` - Trivial
- `bats/Makefile` - Trivial
- `bats/helpers/commands.bash` - Trivial
- `bats/tests/32-app-controllers/engine.bats` - Reviewed, no issues
- `cmd/app-controller/main.go` - Trivial
- `cmd/rdd/set.go` - C1
- `docs/design/api_app.md` - Reviewed, no issues
- `docs/design/api_containers.md` - Reviewed, no issues
- `docs/design/cmd_app.md` - Reviewed, no issues
- `go.mod` - Trivial
- `go.sum` - Trivial
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerspec.go` - Reviewed, no issues
- `pkg/apis/containers/v1alpha1/container_types.go` - Reviewed, no issues
- `pkg/apis/containers/v1alpha1/volume_types.go` - Reviewed, no issues
- `pkg/controllers/app/app/controllers/app_controller.go` - Reviewed, no issues
- `pkg/controllers/app/app/lima-template.yaml` - Reviewed, no issues
- `pkg/controllers/app/engine/controller.go` - Reviewed, no issues
- `pkg/controllers/app/engine/controllers/docker_watcher.go` - I1
- `pkg/controllers/app/engine/controllers/engine_reconciler.go` - C1, I3
- `pkg/controllers/app/engine/controllers/sync_containers.go` - Reviewed, no issues
- `pkg/controllers/app/engine/controllers/sync_containers_test.go` - Reviewed, no issues
- `pkg/controllers/app/engine/controllers/sync_images.go` - I2
- `pkg/controllers/app/engine/controllers/sync_volumes.go` - Reviewed, no issues
- `pkg/controllers/containers/container/container_webhooks.go` - Reviewed, no issues
- `pkg/controllers/containers/container/crd.yaml` - Reviewed, no issues
- `pkg/controllers/containers/volume/crd.yaml` - Reviewed, no issues
- `pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go` - Reviewed, no issues
- `pkg/instance/instance.go` - Reviewed, no issues
- `pkg/service/cmd/service.go` - Trivial
- `scripts/bats-with-timeout.sh` - Reviewed, no issues

---

## Resolution

| | |
|---|---|
| **Addressed** | 2026-04-12 |
| **Commit** | `94a5a34` Address review round 5 findings for PR #295 |

| # | Finding | Action |
|---|---------|--------|
| 1 | Critical #1: `rdd set` hangs for the full timeout when the App is already running | Fixed |
| 2 | Important #1: `ContainerNamespace/moby` is not re-mirrored after user delete | Skipped |
| 3 | Important #2: `processImageFinalizers`/`processVolumeFinalizers` skip `retry.RetryOnConflict` | Fixed |
| 4 | Important #3: `rdd set` wait misinterprets `StartFailed`/`ConnectFailed`/`StopFailed` conditions | Skipped |
| 5 | Important #4: `docker rename` events are dropped | Fixed |
| 6 | Important #5: Transient Docker `Inspect` failures during `syncAllImages` delete valid `Image` mirrors | Fixed |
| 7 | Important #6: Docker watcher drops events on transient `handleEvent` errors | Commented |
| 8 | Suggestion #1: `Reconcile` holds `watcherMu` across the blocking `w.stop()` call | Fixed |
| 9 | Suggestion #2: `ImageStatus.RepoDigests` is sent to SSA in Docker's order | Fixed |
| 10 | Suggestion #3: `ContainerPort.Bindings` is sent to SSA in Docker's order | Fixed |
| 11 | Suggestion #4: `rdd set` timeout error doesn't name the condition | Skipped |
| 12 | Suggestion #5: `deleteAllOfType` passes the stale list item to `r.Delete` | Commented |
| 13 | Suggestion #6: `DockerSocket()` performs filesystem side effects at construction | Skipped |
| 14 | Suggestion #7: `watchCondition` allocates a new dynamic client per call | Skipped |
| 15 | Suggestion #8: Dead branches in the container state-transition webhook | Skipped |
| 16 | Suggestion #9: `reconcileContainerState` silently ignores unknown desired states | Skipped |
| 17 | Testing gaps (8 items) | Skipped |
| 18 | Documentation gaps (4 items) | Skipped |