# Deep Review: 20260414-192417-pr-309 | | | |---|---| | **Date** | 2026-04-14 19:24 | | **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) | | **Round** | 1 | | **Author** | [@jandubois](https://github.com/jandubois) | | **PR** | [#309](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/309) — WIP: squash of docker-v2 engine commits for rebase | | **Branch** | `engine-v3` | | **Commits** | `a154249` WIP: squash of docker-v2 engine commits for rebase | | **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro | | **Verdict** | **Merge with fixes** — C1 and C2 are cheap to fix and close real stalls; the three Important items on bootstrap re-watch, the `rdd set` wait, and volume sync error handling all deserve to go in before rebase lands on main | | **Wall-clock time** | `16 min 32 s` | ## Executive Summary PR #309 adds a new engine controller under `pkg/controllers/app/engine/` that connects to the embedded Docker daemon, mirrors Docker state into `Container`, `Image`, `Volume`, and `ContainerNamespace` resources, and forwards user-initiated deletions and `Container.spec.state` transitions back to Docker. It also introduces a `--timeout` flag and `waitForDesiredState` wait path in `rdd set`, extends the Container CRD with a user-settable `spec.state`, adds a new `engine.bats` integration suite, and updates the design docs for App and Container. The implementation is careful about the hard parts: SSA field ownership is split across two field managers so a finalizer-only apply does not drop spec ownership; atomic-list ordering is stabilized to avoid resourceVersion churn; the Docker event stream is anchored to the daemon's own clock via `Info().SystemTime`; and multi-writer status updates consistently use `retry.RetryOnConflict`. The two critical findings are both defer/control-flow bugs in the same file: a LIFO-order race in `dockerWatcher.run()` can leave `w.done` still open when the reconciler checks `alive()` after a stream error, permanently stalling the watcher; and the main Reconcile loop early-returns on any `reconcileContainerSpecs` error, which means a single stuck `Container.spec.state` actuation blocks all finalizer processing across every mirror type. Two sharp Important-severity design gaps round out the picture: the engine creates `rancher-desktop` namespace and `containernamespace/moby` in one-shot `fullSync()` but never watches them for recreation, so a live delete wedges the controller; and the new `rdd set running=true` wait can return success before Lima has actually applied a template/backend change, because `ContainerEngineReady` gets re-stamped with the new generation the moment the engine reconciler runs, long before LimaVM has finished its restart cycle. --- ## Critical Issues C1. **Watcher event-stream error races with `alive()` via defer LIFO order** — `pkg/controllers/app/engine/controllers/docker_watcher.go:145-192` [Gemini 3.1 Pro] ```go func (w *dockerWatcher) run(ctx context.Context, since string) { log := logf.FromContext(ctx).WithName("docker-watcher") defer close(w.done) defer func() { if r := recover(); r != nil { ... w.enqueueReconcile() } }() ... case err := <-result.Err: if ctx.Err() != nil { return } log.Error(err, "Docker event stream error") w.enqueueReconcile() return case msg, ok := <-result.Messages: if !ok { log.Info("Docker event stream closed") w.enqueueReconcile() return } ``` When the Docker event stream errors out, the watcher calls `w.enqueueReconcile()` inline and then `return`s. Defers fire in LIFO order on exit: the panic-recovery defer fires first (no-op), then `close(w.done)` — both *after* the enqueue. The reconciler is woken by the enqueue and, at `engine_reconciler.go:100`, checks `r.watcher.alive()`, which is `select { case <-w.done: return false; default: return true }`. In the window between the inline `enqueueReconcile()` and the deferred `close(w.done)`, `alive()` still reports true, so the reconciler takes the "watcher is already running" branch at `engine_reconciler.go:165-174` and skips the restart. The goroutine then exits and no further event ever wakes the engine reconciler — the watcher is dead but the reconciler believes it is alive. In wall-clock time the race window is short (a few instructions between `enqueueReconcile()` returning and `close(w.done)` executing), but the reconciler runs on a separate goroutine scheduled off a work queue, so it absolutely can observe the intermediate state. The symptom is permanent — there is no further path that wakes the engine reconciler once the child-resource events drain — until an unrelated event like a user `rdd set` finally triggers a reconcile minutes or hours later. Fix: invert the defer order so `close(w.done)` is deferred first (fires last) and `enqueueReconcile` is deferred after it (fires first, when `w.done` is already closed), and drop the inline `enqueueReconcile()` calls in the error paths. That also removes the need for the recover handler to re-call `enqueueReconcile`. ```diff func (w *dockerWatcher) run(ctx context.Context, since string) { log := logf.FromContext(ctx).WithName("docker-watcher") defer close(w.done) + defer w.enqueueReconcile() defer func() { if r := recover(); r != nil { log.Error(nil, "panic in Docker watcher goroutine", "recovered", r, "stack", string(debug.Stack())) - w.enqueueReconcile() } }() ... case err := <-result.Err: if ctx.Err() != nil { return } log.Error(err, "Docker event stream error") - w.enqueueReconcile() return case msg, ok := <-result.Messages: if !ok { log.Info("Docker event stream closed") - w.enqueueReconcile() return } ``` --- C2. **`reconcileContainerSpecs` errors block all finalizer processing** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:195-202` [Gemini 3.1 Pro] ```go if err := r.reconcileContainerSpecs(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile container specs: %w", err) } if err := r.processFinalizers(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("failed to process finalizers: %w", err) } ``` `reconcileContainerSpecs` already uses `errors.Join` internally, so a single failing container does not abort the inner loop — but it still returns a non-nil aggregate error, which causes the main `Reconcile` to early-return and skip `processFinalizers` entirely. If any Container mirror is stuck in a state Docker keeps rejecting (for example `spec.state=running` on a container whose image was removed, so every `ContainerStart` returns an error), that single broken container blocks every `Container`, `Image`, and `Volume` finalizer in the namespace. Deleted mirrors sit in `Terminating` indefinitely because the processFinalizers pass never runs — and because the next reconcile enters the same branch and fails again. This is a persistent stall of the finalizer pipeline, not a transient hiccup: every reconcile until the broken container is manually fixed hits the same path. The fix mirrors the per-item error collection the rest of the file already uses: run both passes unconditionally and join their errors, so the controller still requeues but finalizers are no longer gated on spec actuation. ```diff - if err := r.reconcileContainerSpecs(ctx); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to reconcile container specs: %w", err) - } - if err := r.processFinalizers(ctx); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to process finalizers: %w", err) - } - return ctrl.Result{}, nil + specsErr := r.reconcileContainerSpecs(ctx) + if specsErr != nil { + specsErr = fmt.Errorf("failed to reconcile container specs: %w", specsErr) + } + finErr := r.processFinalizers(ctx) + if finErr != nil { + finErr = fmt.Errorf("failed to process finalizers: %w", finErr) + } + return ctrl.Result{}, errors.Join(specsErr, finErr) ``` --- ## Important Issues I1. **Bootstrap resources are created once in `fullSync` but never re-watched** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:544-551`, `pkg/controllers/app/engine/controllers/sync_volumes.go:40-67`, `pkg/controllers/app/engine/controllers/docker_watcher.go:466-491` [Codex GPT 5.4] ```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 engine creates both the `rancher-desktop` Kubernetes namespace (`sync_volumes.go:40-53` — `ensureNamespace`) and the `containernamespace/moby` mirror (`sync_volumes.go:62-67` — `syncContainerNamespace`) exactly once, inside `fullSync()` at watcher startup. `SetupWithManager` watches App, Container, Image, and Volume — nothing on `corev1.Namespace` or `containersv1alpha1.ContainerNamespace`. While the App is running, deleting either of those bootstrap resources produces no watch event that will re-enter `fullSync()`. The engine.bats test at line 362 (`deleting containernamespace/moby completes without a finalizer hang`) only works because the *next* test does `rdd set running=true` which cycles the watcher; once the file runs, the intervening state is "moby namespace is gone and nobody notices". The namespace case is worse: once `namespace/rancher-desktop` is deleted, every subsequent mirror apply inside `handleEvent` fails with a NotFound, and `docker_watcher.go:203-206` explicitly logs-and-drops those errors until a watcher restart happens. The controller is wedged in a broken steady state with no self-heal path. Fix needs both pieces: watch the bootstrap resources so a delete enqueues a reconcile, AND move `ensureNamespace()` / `syncContainerNamespace()` out of startup-only and into the steady-state reconcile path so the wake actually rebuilds them. ```diff return ctrl.NewControllerManagedBy(mgr). For(&appv1alpha1.App{}). Named("engine-reconciler"). WatchesRawSource(source.Channel(r.reconcileChan, enqueueApp)). + Watches(&corev1.Namespace{}, enqueueApp /* filter name == apiNamespace */). + Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp). Watches(&containersv1alpha1.Container{}, enqueueApp). Watches(&containersv1alpha1.Image{}, enqueueApp). Watches(&containersv1alpha1.Volume{}, enqueueApp). Complete(r) ``` Combined with calling `ensureNamespace` + `syncContainerNamespace` near the top of the steady-state reconcile (before any sub-resource apply) so the reconciler that the watch enqueues can actually recover. --- I2. **`rdd set running=true` wait returns success before Lima applies template/backend changes** — `cmd/rdd/set.go:467-494`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:175-186`, `pkg/controllers/app/app/controllers/app_controller.go:262-292` [Codex GPT 5.4] ```go if runningVal == "true" { logrus.Info("Waiting for container engine to be ready") return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool { status, gen, present := conditionInfo(obj, "ContainerEngineReady") return present && gen >= minGen && status == metav1.ConditionTrue }) } ``` The new wait path treats `ContainerEngineReady` as the completion signal for every `running=true` command. But `engine_reconciler.go:184-186` re-asserts `ContainerEngineReady=True/Connected` on every steady-state reconcile solely to advance `observedGeneration`, with a comment explicitly stating that this "is the only place that stamps the new generation into ContainerEngineReady, and rdd set's wait predicate depends on it". That self-stamping is correct for *this* code path's purpose, but it decouples the signal from actual convergence. When the user runs `rdd set running=true kubernetes.enabled=true` or `rdd set running=true containerEngine.name=containerd` against an already-running App: 1. The patch lands; App generation becomes N+1. 2. `AppReconciler.Reconcile` fires, sees the template ConfigMap differs, patches the template ConfigMap at `app_controller.go:274-290`, and returns with `Requeue: true`. It does *not* wait for Lima to pick up the change. 3. `EngineReconciler.Reconcile` fires in parallel on the same App change. `running=true`, `engineIsDocker=true`, watcher is still alive → it re-stamps `ContainerEngineReady=Connected/True` with `ObservedGeneration=N+1`. 4. `rdd set`'s `watchCondition` observes `status=True && observedGeneration >= minGen` → returns success. 5. Lima still has to pick up the template-ConfigMap change, mark `restartNeeded`, stop the VM, start it with the new template. The engine controller will eventually sweep mirrors and set `ContainerEngineReady=Stopped`, but the user has already seen the command succeed and moved on — `docker ps` shows a different engine's state than the one the user asked for. The existing TODO at `set.go:463-466` acknowledges the gap for "`running` not changed" cases; this finding extends the same logic to `running=true` combined with a template-affecting field. Fix: condition the wait target on what actually changed. Three feasible shapes: - For any patch that touches a template input (`containerEngine.*`, `kubernetes.*`), wait on `LimaVM.status.observedTemplateResourceVersion` (already maintained for this purpose, per `pkg/apis/lima/v1alpha1` and `docs/design/api_lima.md`) rather than `ContainerEngineReady`. - Or introduce the "Reconciled" condition on App mentioned in the TODO comment and key the wait on that. - Or, as a short-term fix that does not need a new condition, stop re-stamping `ContainerEngineReady` purely to advance `observedGeneration` in the steady-state path — but that breaks the current wait semantics for the cold-start-from-stopped path. The first option is cheapest: `rdd set` knows which fields it patched, so it can pick the right wait predicate per patch. --- I3. **`syncAllVolumes` fails the whole `fullSync` on a transient per-item error** — `pkg/controllers/app/engine/controllers/sync_volumes.go:82-89` [Claude Opus 4.6, Gemini 3.1 Pro] ```go var errs []error for _, v := range volumeList.Items { mirrorName := volumeMirrorName(v.Name) activeNames[mirrorName] = true if err := w.applyVolume(ctx, v); err != nil { errs = append(errs, err) } } ``` `syncAllContainers` (`sync_containers.go:57-63`) and `syncAllImages` (`sync_images.go:86-93`) both log a per-item apply failure and continue, with an explicit comment calling out that transient failures must not "prevent every other healthy container from being mirrored, nor pin the ContainerEngineReady condition at ConnectFailed". `syncAllVolumes` does not follow that contract — it accumulates per-item errors into `errs` and returns them. The downstream chain amplifies this: `fullSync` returns `errors.Join(...)`, `newDockerWatcher` forwards that error to `startWatcher`, which sets `ContainerEngineReady=False/ConnectFailed` and returns. A single transient API-server 503 on one Volume apply fails the entire watcher startup until the condition clears. Fix: match the containers/images pattern exactly — log the error and continue. ```diff var errs []error for _, v := range volumeList.Items { mirrorName := volumeMirrorName(v.Name) activeNames[mirrorName] = true if err := w.applyVolume(ctx, v); err != nil { - errs = append(errs, err) + log.Error(err, "Skipping volume during full sync", "name", v.Name) } } ``` The stale-mirror sweep below still uses `errs` — leave that branch untouched. If the mirror is missing, the next event will pick it up; if the volume is permanently broken, nothing in either pattern catches it anyway. --- I4. **`engine.bats` computes `DOCKER_HOST` via command substitution at file scope** — `bats/tests/32-app-controllers/engine.bats:13-14` [Gemini 3.1 Pro, Codex GPT 5.4] ```bash NAMESPACE="rancher-desktop" DOCKER_HOST="unix://$(rdd svc paths short_dir)/docker.sock" export DOCKER_HOST ``` The `$(rdd svc paths short_dir)` substitution runs when BATS loads the file, before `local_setup_file` has a chance to run `skip_on_windows` or establish any test context. If `rdd` is missing, unbuildable, or returns an error, the failure surfaces as a file-load-time error rather than a clean per-test failure, and the value silently becomes `unix:///docker.sock` (no error, no indication which test file broke). The repo's BATS style rules (per `~/.claude/memory/bats-style.md`) explicitly prefer `run -0` for commands whose output is captured, so failures are both visible and attributable. Fix: move the assignment into `local_setup_file`, using `run -0` so a failure is reported against the file rather than silently eaten. ```diff NAMESPACE="rancher-desktop" -DOCKER_HOST="unix://$(rdd svc paths short_dir)/docker.sock" -export DOCKER_HOST local_setup_file() { skip_on_windows rdd svc delete rdd set running=true + run -0 rdd svc paths short_dir + DOCKER_HOST="unix://${output}/docker.sock" + export DOCKER_HOST } ``` Reconciliation note: Gemini and Codex both raised this; I kept it as Important because the failure mode (silent substitution during file load) is the exact class the BATS style rule exists to prevent, even though Codex filed it as a Suggestion. --- I5. **`removeImagesByID` can miss Image mirrors whose `status.id` has not yet been populated** — `pkg/controllers/app/engine/controllers/sync_images.go:259-273` [Claude Opus 4.6] ```go func (w *dockerWatcher) removeImagesByID(ctx context.Context, id string) error { var images containersv1alpha1.ImageList if err := w.k8s.List(ctx, &images, client.InNamespace(apiNamespace)); err != nil { return err } var errs []error for i := range images.Items { if images.Items[i].Status.ID == id { if err := w.removeMirrorResource(ctx, &images.Items[i], images.Items[i].Name); err != nil { errs = append(errs, err) } } } return errors.Join(errs...) } ``` `applyImage` (`sync_images.go:191-212`) issues two sequential calls — the metadata Apply and the status SubResource Apply. If the first succeeds and the second fails transiently (for example a 503 or a SSA conflict past its internal retry), the Image mirror exists with an empty `status.id`. A subsequent Docker `ActionDelete` event for the underlying image ID routes through `removeImagesByID`, which iterates by `status.id == id` — the empty-status mirror never matches and is left as an orphan. The next full sync at watcher restart prunes it via `syncAllImages`' stale-mirror sweep, but during normal operation the orphan persists. The symmetric `applyImageFromInspect` path for dangling images is also affected: a dangling image created and deleted faster than the status apply completes produces an orphan mirror. Fix: as a fallback when no mirror matches by `status.id`, resolve the expected mirror name(s) with `imageMirrorNames(id, nil)` — the same helper used to create the mirror name for a dangling image — and try to remove by computed name too. The dangling-ID name is deterministic from the Docker ID alone, so this cleanly handles the empty-status case without needing to know the tags. ```go matched := make(map[string]bool) for i := range images.Items { if images.Items[i].Status.ID == id { matched[images.Items[i].Name] = true if err := w.removeMirrorResource(ctx, &images.Items[i], images.Items[i].Name); err != nil { errs = append(errs, err) } } } for _, name := range imageMirrorNames(id, nil) { if matched[name] { continue } if err := w.removeMirrorResource(ctx, &containersv1alpha1.Image{}, name); err != nil && !apierrors.IsNotFound(err) { errs = append(errs, err) } } ``` --- ## Suggestions S1. **`isStopped` omits `ContainerStatusUnknown`, triggering a `ContainerStop` dispatch on fresh mirrors** — `pkg/controllers/app/engine/controllers/docker_watcher.go:372-418` [Claude Opus 4.6] ```go func isStopped(s containersv1alpha1.ContainerStatusValue) bool { switch s { case containersv1alpha1.ContainerStatusCreated, containersv1alpha1.ContainerStatusExited, containersv1alpha1.ContainerStatusDead: return true } return false } ``` If a Container mirror has `status.status` still defaulted to `unknown` (the CRD default per `container_types.go:108`, before the first `applyContainer` status Apply lands), and the user patches `spec.state=created` at that moment, `reconcileContainerState` sees `desired=created`, `actual=unknown`, and the `isStopped` guard says "not stopped" → dispatches `ContainerStop`. This matches the state matrix comment at `docker_watcher.go:363` (the author's intentional choice: "created: anything-not-stopped → stop"), but it wastes a Docker round trip per reconcile until the status catches up, and it makes the state matrix disagree with itself (the `running` row treats `unknown` as "start", which is equally speculative). The change is a design judgment call rather than a clear bug — the author's matrix is internally consistent — so this is surfaced as a suggestion rather than an issue. If the intent is "do nothing until the status is known", add `ContainerStatusUnknown` to `isStopped` *and* add an `actual == ContainerStatusUnknown` no-op guard at the top of the `running` branch. Reconciliation note: Claude originally flagged this as Important. Downgraded because the author's documented matrix explicitly covers the `unknown` column — the question is whether the matrix itself is right, not whether the code matches the matrix. --- S2. **Controller multiplexes too many event domains through one App-scoped loop** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:82-203` [Claude Opus 4.6, Codex GPT 5.4] The current `EngineReconciler.Reconcile` drives watcher lifecycle, bootstrap resource creation (when it happens at all — see I1), per-Container `spec.state` actuation, and per-resource finalizer processing, all gated on a single App-scoped reconcile triggered by broad `Container` / `Image` / `Volume` watches. The code already acknowledges the cost ("reconcileContainerSpecs + processFinalizers each issue one or more List() calls across every mirrored object on every reconcile"), but the architectural problem is bigger than List cost: I1 is a "created but not watched" gap that is a direct consequence of collapsing bootstrap into `fullSync()`, and C2 is a "one reconcile path blocks sibling responsibilities" gap that is a direct consequence of serializing unrelated concerns in one reconcile function. Splitting into narrower reconcilers (watcher lifecycle + bootstrap on App; a finalizer reconciler per resource type with a `deletionTimestamp != nil` predicate; a `spec.state` reconciler with a `spec.state changed` predicate) would make each reconcile's wake-up set explicit and eliminate an entire class of stalls. This is a future-work design observation rather than a change request for this PR. --- S3. **`syncAllImages`/`syncAllContainers` inspect each resource sequentially at watcher startup** — `pkg/controllers/app/engine/controllers/sync_containers.go:30-81`, `pkg/controllers/app/engine/controllers/sync_images.go:56-111` [Claude Opus 4.6] Both full-sync functions explicitly call out the sequential Inspect as a conscious trade-off ("typical dev machines have tens of containers, not hundreds"). For a single-user local dev machine the choice is defensible; the note is worth keeping because the O(N) wall-clock cost is user-visible at watcher startup and the engine.bats suite already creates several images/containers serially. If users ever report `ContainerEngineReady` taking a long time after `rdd set running=true`, this is the first thing to parallelize. --- S4. **`engine.bats` bypasses `setup_rdd_control_plane` and relies on `rdd set`'s implicit service start** — `bats/tests/32-app-controllers/engine.bats:16-22` [Claude Opus 4.6] ```bash local_setup_file() { skip_on_windows rdd svc delete rdd set running=true } ``` Every other BATS file calls `setup_rdd_control_plane "..."` which explicitly starts the service with a chosen set of controllers. `engine.bats` skips that helper and relies on `rdd set`'s implicit `ensureServiceRunning` (via `getAppKubeClient` at `cmd/rdd/set.go:574`) to start the service with all controllers. This works, but it: 1. Couples the test to an implicit CLI path. 2. Cannot specify a subset of controllers (always gets the full set). 3. Diverges from the house style so future readers have to trace through `ensureServiceRunning` to understand the setup contract. Fix is either to switch to `setup_rdd_control_plane '*'` + `rdd set running=true`, or to document the deliberate choice with a comment explaining why the implicit path is preferred here. Reconciliation note: Claude originally flagged this as Important. Downgraded — this is a style/consistency issue, not a correctness bug. --- S5. **`containerd backend` test switches the engine to containerd but never switches back** — `bats/tests/32-app-controllers/engine.bats:372-396` [Claude Opus 4.6] The final test sets `containerEngine.name=containerd` and never restores `moby`. `teardown_file` stops the service, so later test files are unaffected as long as each one owns its own instance. But if the file is ever extended with more tests after that one, they would unexpectedly run against containerd. --- S6. **`reconcileContainerState` reads `c.Status.Status` from a potentially stale informer cache** — `pkg/controllers/app/engine/controllers/docker_watcher.go:372-403` [Claude Opus 4.6] The `Container` object passed to `reconcileContainerState` comes from `r.List()` in `engine_reconciler.go:362`, which reads the controller-runtime informer cache. Between the List and the Docker API call, the container's actual state may have moved on. `ContainerStart` / `ContainerStop` are idempotent for their normal targets, but `ContainerUnpause` on an already-unpaused container returns a Docker error, which becomes a reconcile error, which causes a backoff-requeue loop until the cache catches up. In practice the cache converges within milliseconds and the transient error self-heals. Not worth fixing unless the CI starts flaking on pause/unpause races; if it does, the fix is a `cerrdefs.IsNotPaused` (or equivalent) check analogous to the `IsNotFound` pattern already used for the delete helpers. Reconciliation note: Claude originally flagged this as Important. Downgraded — the race is transient and the controller requeues, so it self-heals within the next reconcile tick. --- S7. **`conditionInfo` silently returns `observedGeneration=0` for missing fields** — `cmd/rdd/set.go:551-569` [Claude Opus 4.6] ```go gen, _, _ := unstructured.NestedInt64(cond, "observedGeneration") ``` `unstructured.NestedInt64` returns `(0, false, nil)` when the field is missing. The wait predicate at `set.go:480` and `:492` checks `gen >= minGen`, so an absent `observedGeneration` correctly makes the predicate false (`0 < minGen`) and the wait keeps going. The behavior is right; the error-discarding line is easy to misread as dropping a real error. A one-line comment at the `unstructured.NestedInt64` call ("missing observedGeneration → 0 is the correct default; the predicate already filters on `>= minGen`") would remove the readability papercut. --- S8. **Image mirror naming scheme differs from volume naming scheme for no functional reason** — `pkg/controllers/app/engine/controllers/sync_images.go:43-54`, `pkg/controllers/app/engine/controllers/sync_volumes.go:33-36` [Claude Opus 4.6] Volume mirrors use `vol-` (68 chars, opaque, deterministic from the Docker name). Image mirrors use `-` (~136 chars, encodes the Docker ID into the K8s name). Both are deterministic, both are valid RFC 1123 subdomain names — the asymmetry is purely cosmetic. The image scheme makes K8s-side debugging easier (you can eyeball the image ID from the mirror name), the volume scheme is shorter. No action needed; noting for consistency if either scheme is ever revisited. --- ## Design Observations ### Concerns - **O(N) reconcile cost per child event** (in-scope) [Claude Opus 4.6, Codex GPT 5.4]. Every Container / Image / Volume watch event triggers a full App reconcile that Lists all three types and sweeps for finalizers and spec.state changes. The comment at `engine_reconciler.go:188-194` documents the cost and defers the split into per-resource reconcilers. This PR does not make it worse, but S2 makes the case that the design gap also produces structural stalls (C2, I1), so the split is worth sequencing earlier than "long-term". - **`setEngineCondition` is re-called on every steady-state reconcile purely to advance `observedGeneration`** (in-scope) [Codex GPT 5.4]. This is the mechanism the `rdd set` wait predicate relies on, but it also makes the condition a low-fidelity proxy for "the engine controller has observed the App generation" rather than "the container engine is actually ready for the requested state" — see I2 for the consequence. Worth revisiting once the wait predicate has a better target. ### Strengths - **Docker daemon clock synchronization** (in-scope) [Claude Opus 4.6, Gemini 3.1 Pro]. `dockerEventsSince` (`docker_watcher.go:107-122`) reads the daemon's own `SystemTime` before `fullSync()` and uses it as the `Since` filter for the event stream, closing the blind spot between the List-based snapshot and the event subscription. The host-clock fallback is explicitly documented as biased because `fullSync` is idempotent. This is the kind of race other embedded-VM controllers routinely get wrong. - **Dual SSA field owners for finalizer-only applies** (in-scope) [Claude Opus 4.6, Codex GPT 5.4]. The `finalizerFieldOwner` pattern (`engine_reconciler.go:45-50`, `sync_containers.go:146-158`) is a clean solution to the SSA ownership problem where a finalizer-only apply would drop `spec` ownership from `controllerName` and fail the Container CRD's required-field validation. The comment explaining *why* the second field manager exists is exactly the kind of non-obvious design rationale that belongs inline. - **Generation-threaded wait predicate** (in-scope) [Claude Opus 4.6]. `rdd set`'s `minGen` is captured post-write and threaded through to `waitForDesiredState` / `watchCondition`, so a stale condition from a previous reconcile cannot satisfy the wait. This is the subtle correctness problem I2 picks at — but the predicate itself is right, the issue is which condition to key it on. - **Consistent retry-on-conflict for multi-writer status updates** (in-scope) [Claude Opus 4.6, Codex GPT 5.4]. Both `AppReconciler` (`app_controller.go:311-334`) and `EngineReconciler` (`engine_reconciler.go:241-267`) use `retry.RetryOnConflict` + re-Get when writing to `App.Status.Conditions`. This replaces the older 409-loop behavior that the repo context document calls out as the right pattern for this controller. - **Atomic-list SSA stabilization** (in-scope) [Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro]. Sorting `RepoDigests`, `Container` port bindings, and port names before applying (`sync_containers.go:187-225`, `sync_images.go:171-176`) avoids the Go-map-iteration-order churn that has bitten other controllers. `+listType=map +listMapKey=name` on `Status.Ports` further pins the canonical order. ## Testing Assessment The new `engine.bats` suite is comprehensive for its scope: it covers image pull/tag/untag/dangling (including the tricky direction of tagging a dangling image), container lifecycle with `status=running → exited`, volume create/rm, volume-name hashing, container delete forwarding, finalizer-held in-use image delete, `spec.state=created/running/unknown` transitions including the paused branches, cleanup on VM stop, `rdd set --timeout=0`, VM restart recreating the namespace, and the containerd-backend NotApplicable path. The unit test `sync_containers_test.go` exhaustively covers `parseContainerName`. Coverage gaps the findings expose: 1. **Bootstrap-resource self-heal** (I1). No test deletes `namespace/rancher-desktop` or `containernamespace/moby` while the engine is running and asserts that the controller rebuilds them — only the "delete containernamespace/moby does not hang" case, which succeeds for the wrong reason (the next test restarts the VM). 2. **Watcher self-heal after event-stream failure** (C1). No test simulates a Docker event-stream drop and verifies the reconciler detects the dead watcher and reconnects. This would require some Docker fault injection; worth doing even as a unit test against a fake Docker client. 3. **Finalizer pipeline under persistent per-container error** (C2). No test creates a container with a `spec.state` Docker keeps rejecting and then deletes a sibling `Image` / `Volume`, to verify the sibling finalizer still runs. This is the exact shape C2 would fail at. 4. **`rdd set` wait with template-affecting changes** (I2). No test does `rdd set running=true kubernetes.enabled=true` on an already-running VM and asserts that the wait holds until LimaVM has actually applied the template. The existing tests only exercise `running=true` on a stopped VM, where `ContainerEngineReady=True` is a correct signal. 5. **Status-empty mirror delete race** (I5). No test covers an `ActionDelete` for an Image whose status Apply hasn't landed — the orphan-mirror path. Codex ran `go test ./pkg/controllers/app/engine/controllers` and `go test ./pkg/controllers/containers/container` during its review; both passed. ## Documentation Assessment `docs/design/api_app.md`, `docs/design/api_containers.md`, and `docs/design/cmd_app.md` are all updated consistently with the implementation. The terminology block added to `api_containers.md` (K8s resource `Container` vs Docker engine object `container`) is a useful distinction that is easy to forget in reviews. If I2 is not fixed, both `cmd_app.md` and the new `ContainerEngineReady` text in `api_app.md` should be tightened: as written they read like a "desired state reached" guarantee, but the condition only proves "the engine controller has observed the new App generation". The honest phrasing is "the engine controller is connected to Docker at or after App generation N", which is precisely what the signal means today. ## Commit Structure The PR is a single commit, `a154249`, with message `WIP: squash of docker-v2 engine commits for rebase` — the original rebase-prep placeholder. Before this lands on main the commit message should summarize the actual user-visible change: engine mirroring of Docker containers/images/volumes into new K8s resources, finalizer forwarding in both directions, new `Container.spec.state` actuation, and the new `rdd set --timeout` wait semantics. Attempting to infer the scope from the diff alone is needlessly expensive for future `git blame` readers. ## Acknowledged Limitations - **O(N) List cost on every reconcile** (`engine_reconciler.go:188-194`) — the code calls out the cost and defers the split into per-resource reconcilers. This change does not make it worse, but S2 argues the split is load-bearing for structural reasons beyond performance. - **Dropped events for one-shot image-pull / volume-create actions** (`docker_watcher.go:193-202`) — a transient `handleEvent` error leaves the mirror missing until the next full resync (watcher restart or reconnect). A periodic `fullSync` tick is deferred. - **`rdd set` does not wait for non-`running` template changes** (`set.go:463-466`) — the TODO acknowledges that `containerEngine.name=containerd` alone is not waited on. I2 extends this gap to the `running=true` combined case, which is not captured by the existing TODO. - **Container `spec.state` is level-triggered and known broken** (MEMORY.md) — the level-trigger design fights Docker restart policy and out-of-band state changes. Jan plans to redesign with edge-triggered action annotations after containerd mirroring lands; this PR intentionally does not address it. --- ## Agent Performance Retro ### [Claude] Claude produced the widest review by volume — 5 Important and 5 Suggestion candidates — but leaned heavy on severity at the top of the scale. Its strongest unique catches were I5 (`removeImagesByID` missing mirrors with empty `status.id`) and the `isStopped`/unknown observation (now S1), both of which required tracing the apply path carefully rather than just reading the diff. Its original I2 (mirror-name length exceeding 253 chars) was a speculative finding that its own text admitted "the current Docker ID format stays within bounds" — downgraded out of the findings entirely because there is no code path that can produce a name longer than ~136 chars. Several of its Important calls turned out to be style (I4 → S4) or self-healing transients (I5 → S6); the initial severity floor was too high. Coverage was complete and its file-by-file table made reconciliation straightforward. ### [Codex] Codex found two Important issues and both were the sharpest architectural catches in the review: I1 (bootstrap resources not watched) and I2 (`rdd set` wait premature for template-affecting changes). Both required cross-file reasoning — I1 only makes sense once you pair the Watch list in `SetupWithManager` against the one-shot `fullSync()` creations in `sync_volumes.go`, and I2 required tracing the interaction between `engine_reconciler.go:184-186`, `app_controller.go:274-290`, and the LimaVM restart path across a file not changed by this PR. Codex ran `go test` against the two relevant packages, which neither other agent did. It missed the two critical items — C1 and C2 both require reading for control-flow hazards rather than design hazards — and used "suggestion" severity more conservatively than the other two (its BATS substitution finding became Gemini's Important I4). ### [Gemini] Gemini found both Critical items — the defer-ordering race on `w.done` (C1) and the reconcile-order stall on finalizer processing (C2) — which are the two findings with the highest symptom severity in the review. Both required the kind of "read the control flow, not the comments" attention that the other two agents did not apply to the same code paths. Its Important list was narrow (volume sync inconsistency, BATS substitution) but both items were also independently raised by other agents, so they held up under consolidation. Its Suggestions section was empty and its Design Observations were thin; the investment went almost entirely into the critical findings. Gemini also did not run `git blame` (its daily quota is a known constraint per the review skill notes), which would not have changed the findings here since this is a single new-feature commit with no regressions against prior work to disambiguate. ### Summary | | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro | |---|---|---|---| | Duration | 4m 56s | 7m 27s | 5m 04s | | Findings | 1I 7S | 2I | 2C 2I | | Tool calls | 31 (Read 12, Bash 10, Grep 6) | 57 (shell 55, plan 1, stdin 1) | 6 (run_shell_command 3, grep_search 2, read_file 1) | | Design observations | 5 | 4 | 2 | | False positives | 0 | 0 | 0 | | Unique insights | 5 | 2 | 2 | | Files reviewed | 27 | 27 | 27 | | Coverage misses | 0 | 0 | 0 | | **Totals** | **1I 7S** | **2I** | **2C 2I** | | Downgraded | 3 (I→S) | 0 | 0 | | Dropped | 1 | 0 | 0 | All three reviews complemented each other cleanly: Gemini owned the critical control-flow bugs, Codex owned the architectural wait-semantic and watch-gap findings, and Claude owned the breadth of file-level nits. No agent's findings substantially overlapped the others' — consolidation had to unify only the volume-sync and BATS-substitution items. Codex had the highest hit rate on Important findings after severity reconciliation; Gemini had the highest impact-per-finding. --- ## Review Process Notes ### Skill improvements - **Require a severity-reconciliation paragraph in the retro** (standing rule): When the consolidator downgrades a finding from Important to Suggestion, the retro should capture why in one sentence per downgrade. This makes it clear the downgrade was a judgment call, not an omission, and prevents the report from looking like "Claude was wrong" when the real story is "Claude set a conservative severity floor and the reviewer picked a different one". The consolidation process already does this in the reconciliation notes on each finding; make the retro paragraph also reference the reconciliation so readers do not have to scroll back to see how many downgrades happened. ### Repo context updates - **[repo] Engine-controller state matrices are load-bearing and should be re-read, not skimmed** (standing rule to add to `deep-review-context.md`): The engine reconciler and the Container `reconcileContainerState` function both have tabular comments that document "for every desired×actual pair, dispatch X". Reviewers should read the matrix and pair each cell against `isStopped`-like guards, because the matrix can be internally consistent while still encoding a questionable design choice (as in S1). The reviewer should not take the matrix on faith — they should verify that every helper used in the switch is aligned with the matrix's claims. --- ## Appendix: Original Reviews ### [Claude] I'll start by reading the key changed files and tracing through the code systematically. Let me first scan memory and then examine the diff in detail. Let me read the memory index and then start examining the changed files. Good, I have a comprehensive understanding of the codebase. Now let me trace specific code paths and verify key findings. Now let me check a few more critical files for correctness verification. Now let me check the remaining files - `cmd/rdd/set.go` and the app controller, and verify some specific code paths. Now let me run git blame on a few key areas to verify what's introduced by this change. Let me now check a few more items for the review - the engine.bats test file and a key concern about `removeImagesByID` matching on status.ID which could be stale. Let me check a couple more things to verify findings. I see a potential issue - `engine.bats` calls `rdd svc delete` followed by `rdd set running=true` but never calls `rdd svc create` or `rdd svc start`. Let me check what `rdd set` does when there's no service. Now I see the issue — `engine.bats` calls `rdd svc delete` + `rdd set running=true` but skips `rdd svc create` and `rdd svc start`. `rdd set` creates the App resource but doesn't start the service. Let me check if `rdd set` implicitly starts the service. Good — `getAppKubeClient` calls `ensureServiceRunning`, so `rdd set` implicitly starts the service if not running. That explains why `engine.bats` doesn't call `rdd svc create` + `rdd svc start`. Now let me check for the `Labels` map sorting concern — Go maps have random iteration order, and the review context specifically calls this out: Let me check whether Labels/Options are `+listType=map` or `+listType=atomic` or just regular map fields: Labels and Options are Go `map[string]string` fields, not Kubernetes list types. JSON map serialization is key-order-independent in the API server, so map iteration order doesn't cause resourceVersion churn — this is only a concern for slice/list fields. Good. Now I have enough context to write the comprehensive review. Let me check one more thing — the `controllerName` const name shadow between the two packages: Two different `controllerName` consts in different packages — `"engine"` in the registration package and `"engine-controller"` in the controllers package. These serve different purposes (one for controller registration, one for SSA field owner), so no issue. Now I have everything I need for a thorough review. Let me write it up. --- ## Executive Summary This PR adds a Docker engine mirroring controller that watches the Docker event stream and mirrors containers, images, and volumes into Kubernetes resources, with bidirectional support for deletions and container state transitions. The implementation is well-structured with careful attention to SSA ownership, clock synchronization, and concurrent access patterns. Key findings center on a missing `rdd svc start` in the test setup (relying on implicit service start), `isStopped` not covering `ContainerStatusUnknown` (causing wasted `ContainerStop` calls for unknown-state containers), and an image mirror name scheme that can exceed the Kubernetes 253-character name limit. ## Findings ### Critical Issues None. ### Important Issues I1. **`isStopped` omits `ContainerStatusUnknown`, causing unnecessary `ContainerStop` dispatches** — `docker_watcher.go:410-418` ```go func isStopped(s containersv1alpha1.ContainerStatusValue) bool { switch s { case containersv1alpha1.ContainerStatusCreated, containersv1alpha1.ContainerStatusExited, containersv1alpha1.ContainerStatusDead: return true } return false } ``` When `spec.state=created` and `status.status=unknown` (the status field has not yet been populated by any sync), `isStopped` returns false and `reconcileContainerState` at line 398-400 dispatches `ContainerStop`. This happens on every reconcile until the status catches up, sending repeated unnecessary stop calls to Docker. The state matrix comment at lines 360-363 does not cover the `unknown` column for the `created` row. Since `unknown` means "status not yet known" (not "container is in an unknown Docker state"), dispatching `ContainerStop` against it is premature and wasteful. (important, regression) Fix: Add `ContainerStatusUnknown` to `isStopped`, or guard the `created` branch with `actual != ContainerStatusUnknown`: ```diff func isStopped(s containersv1alpha1.ContainerStatusValue) bool { switch s { case containersv1alpha1.ContainerStatusCreated, containersv1alpha1.ContainerStatusExited, - containersv1alpha1.ContainerStatusDead: + containersv1alpha1.ContainerStatusDead, + containersv1alpha1.ContainerStatusUnknown: return true } return false } ``` --- I2. **Image mirror names can exceed the 253-character Kubernetes name limit** — `sync_images.go:43-53` ```go func imageMirrorNames(id string, repoTags []string) []string { // ... names = append(names, fmt.Sprintf("%s-%x", sanitizeKubernetesObjectName(id), sha256.Sum256([]byte(tag)))) // ... } ``` A Docker image ID is `sha256:64hexchars` (71 chars). After `sanitizeKubernetesObjectName`, the `:` becomes `.`, yielding 71 chars. The SHA-256 of the tag is 64 hex chars. With the `-` separator, the mirror name is `71 + 1 + 64 = 136` characters — this is fine for typical images. However, `sanitizeKubernetesObjectName` only replaces `/` and `:`. Docker image IDs are always `sha256:`, but image summaries from `docker images --all` can include intermediate IDs in non-standard formats on older Docker versions, or the function could receive a registry-prefixed reference via a future code path. More immediately, dangling image names go through `sanitizeKubernetesObjectName(id)` alone and the function does not validate the result length. While the current Docker ID format stays within bounds, there is no runtime guard. (important, gap) Fix: Add a length check or truncation with a trailing hash to guarantee the name stays within 253 characters. Alternatively, hash the entire ID the same way `volumeMirrorName` does, making image names consistently short: ```go if len(name) > 253 { name = fmt.Sprintf("img-%x", sha256.Sum256([]byte(name))) } ``` --- I3. **`removeImagesByID` matches on `status.ID` which may not be populated yet** — `sync_images.go:259-273` ```go func (w *dockerWatcher) removeImagesByID(ctx context.Context, id string) error { // ... for i := range images.Items { if images.Items[i].Status.ID == id { if err := w.removeMirrorResource(ctx, &images.Items[i], images.Items[i].Name); err != nil { ``` When a Docker image is created and immediately deleted before the status SSA apply completes (e.g., during a `docker build` that creates ephemeral intermediate layers), the Image mirror exists (the metadata apply at `applyImage` line 197 succeeded) but `status.ID` is still empty. The `ActionDelete` handler calls `removeImagesByID` with the image ID, which iterates all Image mirrors matching `status.ID == id`. The empty-status mirror will not match and remains as an orphan until the next full sync. This is a narrow race window; the mirror will be cleaned up on watcher restart. (important, regression) Fix: The `imageMirrorNames` function can derive the expected mirror name from the ID alone (dangling case). Use it in `removeImagesByID` as a fallback when no status.ID match is found: ```go func (w *dockerWatcher) removeImagesByID(ctx context.Context, id string) error { var images containersv1alpha1.ImageList if err := w.k8s.List(ctx, &images, client.InNamespace(apiNamespace)); err != nil { return err } matched := make(map[string]bool) var errs []error for i := range images.Items { if images.Items[i].Status.ID == id { matched[images.Items[i].Name] = true if err := w.removeMirrorResource(ctx, &images.Items[i], images.Items[i].Name); err != nil { errs = append(errs, err) } } } // Fallback: remove by computed name for mirrors with empty status. for _, name := range imageMirrorNames(id, nil) { if !matched[name] { _ = w.removeMirrorResource(ctx, &containersv1alpha1.Image{}, name) } } return errors.Join(errs...) } ``` --- I4. **Engine.bats `local_setup_file` skips `rdd svc create`/`rdd svc start`, relying on `rdd set` to implicitly start the service** — `engine.bats:16-22` ```bash local_setup_file() { skip_on_windows rdd svc delete rdd set running=true } ``` The test calls `rdd svc delete` then `rdd set running=true`. `rdd set` calls `ensureServiceRunning` internally (via `getAppKubeClient` at `set.go:574`), which starts the service with default controllers. Every other BATS test file uses `setup_rdd_control_plane` which explicitly calls `rdd svc create --controllers=...` + `rdd svc start`. By bypassing `setup_rdd_control_plane`, the engine tests: 1. Cannot specify which controllers to load (always gets all of them) 2. Don't use the standard test setup pattern, making it harder to understand the setup contract 3. Depend on the implicit behavior of `ensureServiceRunning` which may change The test works because `ensureServiceRunning` starts with all controllers, but this is an implicit coupling. (important, gap) Fix: Use the standard helper or document why it's not used: ```bash local_setup_file() { skip_on_windows setup_rdd_control_plane "*" rdd set running=true } ``` --- I5. **`reconcileContainerState` dispatches Docker actions using stale `c.Status.Status` from the controller-runtime cache** — `docker_watcher.go:372-403` ```go func (w *dockerWatcher) reconcileContainerState(ctx context.Context, c *containersv1alpha1.Container) error { desired := c.Spec.State // ... actual := c.Status.Status ``` The `Container` object `c` comes from `r.List()` at `engine_reconciler.go:362`, which reads from the controller-runtime informer cache. Between the List and the Docker API call, the container's actual state may have changed (e.g., it exited naturally). The Docker client call may then be against an already-stopped container (`ContainerStop` on an exited container) or fail because the container is in a different state. Docker's `ContainerStop` is idempotent for stopped containers and `ContainerStart` is safe for running ones, so the individual calls are safe. However, `ContainerUnpause` at line 388 on a container that has already been unpaused (or transitioned out of paused state) will return an error from Docker, which propagates as a reconcile error and triggers exponential backoff requeues. (important, regression) Fix: Wrap the Docker API calls with a check for the specific "not paused" error from Docker, similar to the `cerrdefs.IsNotFound` pattern used elsewhere: ```go if actual == containersv1alpha1.ContainerStatusPaused { log.Info("Unpausing container", "id", c.Name) _, err := w.cli.ContainerUnpause(ctx, c.Name, dockerclient.ContainerUnpauseOptions{}) if err != nil && !strings.Contains(err.Error(), "is not paused") { return err } return nil } ``` ### Suggestions S1. **`imageMirrorNames` naming scheme differs from `volumeMirrorName`** — `sync_images.go:43-54` vs `sync_volumes.go:33-36` Volumes use a clean `vol-` scheme. Images use `sanitized-id-` which is long and encodes Docker internals into the K8s name. The asymmetry means different debugging approaches for different resource types. Consider whether the image scheme should follow the volume pattern (`img-`) for consistency. The existing scheme works but produces 136-character names vs 68 for volumes. (suggestion, enhancement) --- S2. **`reconcileContainerSpecs` and `processFinalizers` run a full List on every reconcile** — `engine_reconciler.go:188-200` ```go // reconcileContainerSpecs + processFinalizers each issue one or more // List() calls across every mirrored object on every reconcile, and // every Container/Image/Volume watch event triggers a reconcile via // SetupWithManager below. Cost is therefore O(N) per child event. ``` This is already acknowledged in the code comment. Every Container/Image/Volume watch event triggers a reconcile that Lists all three resource types. With N mirrored objects, this is O(N) API calls per event. On a machine with many containers/images, this could become a significant load on the embedded API server. The comment mentions splitting into per-resource reconcilers as the long-term fix. (suggestion, enhancement) --- S3. **Volume `syncAllVolumes` does not log-and-skip per-item failures like containers and images do** — `sync_volumes.go:82-89` ```go var errs []error for _, v := range volumeList.Items { mirrorName := volumeMirrorName(v.Name) activeNames[mirrorName] = true if err := w.applyVolume(ctx, v); err != nil { errs = append(errs, err) } } ``` Containers and images log per-item failures and continue (`log.Error(err, "Skipping container during full sync")`), but volumes collect errors into `errs` without logging. The difference is minor since `fullSync` at `docker_watcher.go:485` uses `errors.Join` to propagate all errors, but the inconsistency means volume errors are less observable at the point of occurrence. (suggestion, regression) Fix: Add a log line matching the container/image pattern: ```diff if err := w.applyVolume(ctx, v); err != nil { + log.Error(err, "Skipping volume during full sync", "name", v.Name) errs = append(errs, err) } ``` --- S4. **`engine.bats` test "containerd backend" switches to containerd but never switches back to moby** — `engine.bats:372-396` The last test switches to `containerEngine.name=containerd` and verifies the `NotApplicable` condition. The test file doesn't restore the engine to `moby` afterward. The `teardown_file()` in `load.bash` stops the service, which cleans up, but if a later test file depends on the engine being `moby` (or if the file is extended), this could cause confusion. (suggestion, gap) --- S5. **`conditionInfo` in `set.go` uses `unstructured.NestedInt64` which returns 0 for absent fields** — `cmd/rdd/set.go:565` ```go gen, _, _ := unstructured.NestedInt64(cond, "observedGeneration") ``` If `observedGeneration` is absent from the condition (e.g., an older controller hasn't been updated to stamp it), this returns 0. The predicate at line 480 checks `gen >= minGen`, so an absent `observedGeneration` would fail the check (0 < minGen) and correctly wait. This is the right behavior but the intent isn't obvious without reading the predicate. A brief comment would help future readers. (suggestion, enhancement) ## Design Observations ### Concerns **O(N) reconcile cost per child event** (in-scope) — Every Container, Image, and Volume event triggers a full reconcile of the App singleton, which then Lists all three resource types and sweeps for finalizers and spec.state changes. This is documented in the code (lines 188-194 of `engine_reconciler.go`) and deferred as a known TODO. For now, the embedded single-user API server has low traffic, but this will scale poorly as users accumulate more containers and images. The long-term fix (per-resource reconcilers with watch predicates) is the right direction. ### Strengths - **Docker daemon clock synchronization**: Using `Info().SystemTime` for the event stream `Since` filter (docker_watcher.go:71-90) correctly handles host/guest clock skew. The fallback to a biased host clock is a practical safety net. - **Dual SSA field owners**: The `finalizerFieldOwner` pattern (engine_reconciler.go:45-50, sync_containers.go:153-158) is a correct and well-documented solution to the SSA ownership problem where a finalizer-only apply would drop spec ownership. - **Generation-based wait predicate**: The `minGen` threading from `patchApp` through `waitForDesiredState` (set.go:446→467→480) correctly prevents stale conditions from satisfying the wait, which is a subtle correctness concern that was handled properly. - **Error collection with `errors.Join`**: The consistent pattern of collecting per-item errors and continuing (rather than failing on the first error) is well-applied throughout the codebase. This prevents one stuck resource from blocking cleanup of all others. - **Retry-on-conflict throughout**: Both the engine reconciler and the app controller use `retry.RetryOnConflict` for status updates (engine_reconciler.go:242-267, app_controller.go:311-334), correctly handling the concurrent-writer pattern. ## Testing Assessment The BATS integration tests in `engine.bats` are comprehensive, covering: 1. Container, Image, and Volume mirroring (create, update, delete) 2. Container state transitions (start/stop/pause/unpause) 3. Image untag/retag and dangling image handling 4. Finalizer lifecycle (in-use image keeps finalizer) 5. Cleanup on VM stop 6. containerd backend behavior Untested scenarios, ranked by risk: 1. **Watcher reconnection after event stream failure** — The watcher `run()` goroutine exits on stream errors, but no test verifies that the reconciler detects the dead watcher and restarts it. 2. **Concurrent `rdd set` + engine reconciler status writes** — The retry-on-conflict logic is not stress-tested. 3. **Image with many tags** — No test verifies that an image with many RepoTags produces the correct number of Image mirrors and that cleanup works correctly. 4. **Container create race** — The Get-then-Apply pattern in `applyContainer` has a documented race window that is not exercised. The unit test `sync_containers_test.go` covers `parseContainerName` thoroughly with edge cases including empty input and slashes. ## Documentation Assessment The design docs (`api_app.md`, `api_containers.md`, `cmd_app.md`) are updated consistently with the implementation. The terminology section in `api_containers.md` (lines 23-38) is a valuable addition that clarifies the capitalization convention between K8s resources and Docker objects. ## Acknowledged Limitations - **O(N) List cost on every reconcile** (engine_reconciler.go:188-194): The code acknowledges the cost and defers optimization to a future per-resource reconciler split. The change does not make this worse; it is inherent to the single-reconciler design. - **Dropped events for one-shot resources** (docker_watcher.go:193-202): The code acknowledges that a dropped `image pull` or `volume create` event leaves the mirror missing until the next full resync. A periodic fullSync tick is deferred. - **`rdd set` does not wait for non-running property changes** (set.go:463-466): The TODO acknowledges that `containerEngine.name` alone does not trigger a wait. A future `Reconciled` condition is proposed. - **Container spec.state is level-triggered and broken** (from MEMORY.md): The level-trigger design fights Docker restart policy and out-of-band state changes. Jan plans to redesign with edge-triggered action annotations after containerd mirroring lands. ## Coverage Summary | File | Finding(s) | |------|-----------| | `.github/workflows/bats.yaml` | Reviewed, no issues | | `bats/tests/32-app-controllers/app.bats` | Reviewed, no issues | | `bats/tests/32-app-controllers/engine.bats` | I4, S4 | | `cmd/app-controller/main.go` | Trivial | | `cmd/rdd/set.go` | S5 | | `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/.../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, I5 | | `pkg/controllers/app/engine/controllers/engine_reconciler.go` | S2 | | `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, I3, S1 | | `pkg/controllers/app/engine/controllers/sync_volumes.go` | S3 | | `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/instance/instance.go` | Reviewed, no issues | | `pkg/service/cmd/service.go` | Trivial | ### [Codex] ### Executive Summary This PR adds a new engine controller that mirrors Docker state into `Container` / `Image` / `Volume` resources, adds `rdd set` waiting semantics, and extends BATS coverage around the new mirror/finalizer behavior. The two substantive regressions are that the engine never recreates its bootstrap namespace resources after a live delete, and that the new `rdd set running=true` wait path can report success before Lima has actually applied backend/template changes to the VM. ### Findings #### Critical Issues None. #### Important Issues I1. **Engine bootstrap resources are created once but never watched for re-creation** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:544-550` (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 engine creates both the Kubernetes namespace at `sync_volumes.go:40-53` and the `containernamespace/moby` mirror at `sync_volumes.go:62-66`, but `fullSync()` only does that work at watcher startup in `docker_watcher.go:470-477`. After that, `SetupWithManager()` at `engine_reconciler.go:544-550` watches only `App`, `Container`, `Image`, and `Volume`; it does not watch either the backing `Namespace` or `ContainerNamespace`. That means deleting `namespace/rancher-desktop` or `containernamespace/moby` while the App is still running produces no watch event that will re-enter `fullSync()`, so the controller stalls indefinitely in a broken steady state. The namespace case is worse: once `rancher-desktop` is gone, subsequent `apply*` calls fail, and `dockerWatcher.run()` explicitly just logs and drops those event-handling failures at `docker_watcher.go:193-205` until a watcher restart/full resync happens. Fix: either watch `corev1.Namespace` and `containersv1alpha1.ContainerNamespace` and enqueue the App singleton on delete, or move `ensureNamespace()` / `syncContainerNamespace()` out of one-time startup and into the steady-state reconcile before any child apply/delete work runs. The fix needs both pieces: watching alone is not enough if reconcile still assumes the scaffolding exists, and re-ensuring alone is not enough if no event ever wakes reconcile after a delete. ```diff return ctrl.NewControllerManagedBy(mgr). For(&appv1alpha1.App{}). Named("engine-reconciler"). WatchesRawSource(source.Channel(r.reconcileChan, enqueueApp)). + Watches(&corev1.Namespace{}, enqueueApp /* filter to apiNamespace */). + Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp). Watches(&containersv1alpha1.Container{}, enqueueApp). Watches(&containersv1alpha1.Image{}, enqueueApp). Watches(&containersv1alpha1.Volume{}, enqueueApp). Complete(r) ``` I2. **`rdd set running=true` can return before Lima has applied a backend/template change** — `cmd/rdd/set.go:476-480` (important, regression) ```go if runningVal == "true" { logrus.Info("Waiting for container engine to be ready") return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool { status, gen, present := conditionInfo(obj, "ContainerEngineReady") return present && gen >= minGen && status == metav1.ConditionTrue }) } ``` The new wait path in `cmd/rdd/set.go:476-480` treats `App.status.conditions[ContainerEngineReady]` as the completion signal for every `running=true` update, but the engine controller updates that condition too early for template-affecting changes. `engine_reconciler.go:176-185` re-stamps `ContainerEngineReady=True/Connected` on every steady-state App reconcile just to advance `observedGeneration`, while the App controller applies template changes by patching the Lima template ConfigMap and returning immediately at `app_controller.go:274-289`. The actual Lima convergence happens later: `handleTemplateUpdate()` in `pkg/controllers/lima/limavm/controllers/limavm_controller.go:396-423` marks `restartNeeded`, writes the template, and explicitly defers completion until the restart cycle finishes. As a result, commands like `rdd set running=true kubernetes.enabled=true` or `rdd set running=true containerEngine.name=containerd` can satisfy the new wait as soon as the App generation is observed, even though the VM is still on the old template/backend and Lima has not finished its restart/apply path. Fix: keep the `ContainerEngineReady` wait for cold-start-from-stopped, but for any `running=true` patch that also changes a template-affecting field (`containerEngine.*`, `kubernetes.*`, and any future Lima-template inputs), wait on the owned `LimaVM`’s real convergence signal instead. The codebase already has one: `api_lima.md:70` and `limavm_controller.go:364-368` define `status.observedTemplateResourceVersion` specifically so callers can wait until a template change is fully applied. If you cannot wire that immediately, the CLI/docs should be narrowed to “waits until the engine controller has observed the App generation”, not “desired state”. Fix: split the wait target by change type. Use `ContainerEngineReady` only when the command is actually starting an already-correct template, and otherwise wait for the LimaVM’s `observedTemplateResourceVersion` (or a `restartCount` increment) after the App controller updates the template ConfigMap. #### Suggestions S1. **Top-level command substitution in the new BATS file will hide setup failures** — `bats/tests/32-app-controllers/engine.bats:13` (suggestion, regression) ```bash NAMESPACE="rancher-desktop" DOCKER_HOST="unix://$(rdd svc paths short_dir)/docker.sock" export DOCKER_HOST ``` The new test file shells out via `$(rdd svc paths short_dir)` during file load at line 13, before `local_setup_file()` has a chance to do `skip_on_windows` or reset the instance. This repo’s BATS guidance explicitly avoids `$(rdd ...)`-style substitutions because failures during command substitution produce opaque load-time errors instead of normal `run` output. Here the problem is worse because the command runs at top level, not inside a test body. Fix: move the `DOCKER_HOST` setup into `local_setup_file()` and populate it from a `run -0` result so failures are reported normally. ```diff NAMESPACE="rancher-desktop" -DOCKER_HOST="unix://$(rdd svc paths short_dir)/docker.sock" -export DOCKER_HOST local_setup_file() { skip_on_windows rdd svc delete rdd set running=true + run -0 rdd svc paths short_dir + DOCKER_HOST="unix://${output}/docker.sock" + export DOCKER_HOST } ``` ### Design Observations **Concerns** 1. **The engine controller is carrying too many event domains through a single App reconcile loop** `(in-scope)` The current design multiplexes watcher lifecycle, namespace/bootstrap creation, child finalizer processing, and `Container.spec.state` actuation through one App-scoped reconciler that is woken by broad child watches. I1 is a direct symptom: the controller creates bootstrap resources in one code path but never attaches watches for them. Splitting bootstrap/self-healing, per-resource finalizer handling, and `Container.spec.state` actuation into narrower reconcilers would make each reconcile’s wake-up set explicit and eliminate an entire class of “created but not watched” stalls. **Strengths** 1. The concurrent App status writers are handled correctly now. `AppReconciler` at `app_controller.go:311-330` and `EngineReconciler` at `engine_reconciler.go:241-266` both use retry-with-reget loops, which is the right fix for the `App.Status.Conditions` multi-writer pattern called out in the repo guidance. 2. The SSA churn mitigation is thoughtful. Sorting `Container` port bindings in `sync_containers.go:187-225` and `Image` digests in `sync_images.go:171-176`, plus adding `+listType=map` for ports, avoids the random-order watch loops that have bitten other controllers in this repo. ### Testing Assessment 1. No test exercises live deletion of `namespace/rancher-desktop` or `containernamespace/moby` while the engine is running and verifies that the controller self-heals. The new BATS file only checks that deleting `containernamespace/moby` does not hang in `Terminating`, which is why I1 slips through. 2. No test covers `rdd set running=true` combined with a template-affecting change on an already-running VM and waits for actual Lima convergence (for example `LimaVM.status.observedTemplateResourceVersion` or `restartCount`). That gap leaves I2 untested. 3. I ran `go test ./pkg/controllers/app/engine/controllers` and `go test ./pkg/controllers/containers/container`; both passed. I did not run the BATS suite. ### Documentation Assessment `docs/design/cmd_app.md:13-19` and the new `ContainerEngineReady` text in `docs/design/api_app.md:92-110` should be tightened if I2 is not fixed. As written, they read like a “desired state reached” guarantee, but the implementation currently only proves that the engine controller has observed the new App generation, not that Lima has finished applying template/backend changes. ### Commit Structure Commit `a154249` still has a placeholder rebase message: `WIP: squash of docker-v2 engine commits for rebase`. That does not describe the user-visible behavior under review, and there is no later commit on the branch that preserves that intent. Rewrite the final commit message to summarize the actual feature set: engine mirroring, finalizer forwarding, and the new `rdd set` wait behavior. ### 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-466`. This change makes the limitation more visible because `rdd set` now has an explicit wait path, but the comment correctly acknowledges the remaining hole for no-`running` backend/template changes. - **Code comment**: `"The bounded-window fix is a periodic fullSync tick, deferred until the rest of the mirroring work lands."` — `pkg/controllers/app/engine/controllers/docker_watcher.go:199-202`. This review does not promote that known dropped-event window to a finding because the comment already captures it; I1 is separate because namespace/bootstrap deletion can leave the controller wedged until a manual restart, not just until the next periodic resync. - **Code comment**: `"TODO: We should validate that: ... when using moby backend: we do not delete the 'moby' namespace."` — `pkg/controllers/containers/containernamespace/container_namespace_webhooks.go:25-27`. The new engine tests make this limitation more relevant, but it is already acknowledged in code. ### Coverage Summary - `.github/workflows/bats.yaml` — Reviewed, no issues - `bats/tests/32-app-controllers/app.bats` — Reviewed, no issues - `bats/tests/32-app-controllers/engine.bats` — S1 - `cmd/app-controller/main.go` — Trivial - `cmd/rdd/set.go` — I2 - `docs/design/api_app.md` — Reviewed, no issues - `docs/design/api_containers.md` — Reviewed, no issues - `docs/design/cmd_app.md` — I2 - `go.mod` — Reviewed, no issues - `go.sum` — Reviewed, no issues - `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` — I2 - `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` — I1, I2 - `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` — 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/instance/instance.go` — Reviewed, no issues - `pkg/service/cmd/service.go` — Trivial ### [Gemini] ### Executive Summary This PR introduces the engine controller, which connects to the embedded Docker daemon and mirrors containers, images, and volumes into Kubernetes resources. The implementation effectively handles Docker-Kubernetes schema translation and uses server-side apply carefully to merge user intent with observed state. However, two critical lifecycle bugs threaten the system's resilience: a race condition in the watcher teardown that leaves the control loop permanently stalled after a transient stream failure, and an error-handling bottleneck in the main reconciler that blocks the cleanup of all mirrored resources when a single container fails to start. ### Findings #### Critical Issues **C1. Watcher restart race condition permanently stalls syncing** — `pkg/controllers/app/engine/controllers/docker_watcher.go:153-157` `(critical, regression)` ```go case err := <-result.Err: if ctx.Err() != nil { return } log.Error(err, "Docker event stream error") w.enqueueReconcile() return ``` When the Docker event stream encounters an error, the watcher goroutine calls `w.enqueueReconcile()` directly and then returns. Because of the LIFO execution order of `defer` statements at the top of the function, `w.done` is not closed until *after* the function returns. This causes a race condition: `w.enqueueReconcile()` wakes up `EngineReconciler.Reconcile()`. The reconciler checks `r.watcher.alive()`, which checks if `w.done` is closed. Because `w.done` is still open while the `return` statement is executing, `alive()` evaluates to `true`. The reconciler skips restarting the watcher, goes back to sleep, and the watcher goroutine finishes and dies silently. No future Docker events will ever be synced until the user manually triggers a reconcile (e.g. via `rdd set`). **Fix:** Ensure the event is always enqueued *after* `w.done` is closed by deferring `enqueueReconcile` at the top of the `run` method, executing it last in the LIFO stack. ```diff func (w *dockerWatcher) run(ctx context.Context, since string) { log := logf.FromContext(ctx).WithName("docker-watcher") + defer w.enqueueReconcile() defer close(w.done) defer func() { if r := recover(); r != nil { log.Error(nil, "panic in Docker watcher goroutine", "recovered", r, "stack", string(debug.Stack())) - w.enqueueReconcile() } }() // ... case err := <-result.Err: if ctx.Err() != nil { return } log.Error(err, "Docker event stream error") - w.enqueueReconcile() return case msg, ok := <-result.Messages: if !ok { log.Info("Docker event stream closed") - w.enqueueReconcile() return } ``` **C2. Container reconciliation errors block all finalizer processing** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:197-203` `(critical, regression)` ```go if err := r.reconcileContainerSpecs(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile container specs: %w", err) } if err := r.processFinalizers(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("failed to process finalizers: %w", err) } return ctrl.Result{}, nil ``` Because `Reconcile` returns early on an error from `reconcileContainerSpecs`, a persistent failure to start or stop a single container (e.g., the user requests `spec.state: running` but the container configuration is invalid and Docker rejects the start) permanently wedges the reconciler. The `processFinalizers` step is never reached, meaning *no* deleted containers, images, or volumes anywhere in the system will ever be cleaned up or have their finalizers stripped until the broken container is removed. **Fix:** Accumulate the errors and process finalizers independently of the spec reconciliation. ```diff - if err := r.reconcileContainerSpecs(ctx); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to reconcile container specs: %w", err) + err1 := r.reconcileContainerSpecs(ctx) + if err1 != nil { + err1 = fmt.Errorf("failed to reconcile container specs: %w", err1) } - if err := r.processFinalizers(ctx); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to process finalizers: %w", err) + + err2 := r.processFinalizers(ctx) + if err2 != nil { + err2 = fmt.Errorf("failed to process finalizers: %w", err2) } - return ctrl.Result{}, nil + return ctrl.Result{}, errors.Join(err1, err2) ``` #### Important Issues **I1. Transient volume sync errors fail the entire watcher startup** — `pkg/controllers/app/engine/controllers/sync_volumes.go:85-87` `(important, inconsistency)` ```go if err := w.applyVolume(ctx, v); err != nil { errs = append(errs, err) } ``` In `syncAllContainers` and `syncAllImages`, transient API server errors (like 409 Conflicts or 503s) on a single item are logged and skipped specifically "rather than failing the whole startup" (as noted in the `sync_containers.go` comment). `syncAllVolumes` fails to follow this fault-tolerance pattern. Instead, it accumulates per-volume apply errors and returns them, which causes `fullSync` to fail, preventing the event stream from starting and permanently pinning `ContainerEngineReady=False` until the user resolves the transient error condition. **Fix:** Mimic the resilience of containers and images by logging the error and continuing. ```diff if err := w.applyVolume(ctx, v); err != nil { - errs = append(errs, err) + log.Error(err, "Skipping volume during full sync", "name", v.Name) } ``` **I2. BATS test uses forbidden command substitution at file scope** — `bats/tests/32-app-controllers/engine.bats:13-14` `(important, gap)` ```bash NAMESPACE="rancher-desktop" DOCKER_HOST="unix://$(rdd svc paths short_dir)/docker.sock" export DOCKER_HOST ``` The test assigns `DOCKER_HOST` using a `$(...)` command substitution at the top level of the file. As noted in the project guidelines, substitutions like `$(rdd ...)` mask process exits and produce opaque failures instead of failing the test outright. **Fix:** Move the dynamic assignment into the `local_setup_file` block, using `run -0` to capture output securely. ```diff -DOCKER_HOST="unix://$(rdd svc paths short_dir)/docker.sock" -export DOCKER_HOST local_setup_file() { # The Docker socket access pattern used by these tests is not yet wired # up for Windows/WSL2. skip_on_windows + + run -0 rdd svc paths short_dir + export DOCKER_HOST="unix://${output}/docker.sock" + rdd svc delete ``` #### Suggestions None. ### Design Observations #### Concerns None. #### Strengths - **SSA Predictability:** The implementation handles Server-Side Apply carefully and accurately by sorting atomic lists (`RepoDigests`, `ContainerPortBindings`) to prevent map-iteration randomness from churning the resource version (`pkg/controllers/app/engine/controllers/sync_containers.go:193`). - **Clock Skew Compensation:** The implementation actively avoids silent event drops during initial sync by deriving the Docker event stream start time (`since`) directly from the daemon's internal clock (`Info().SystemTime`) rather than assuming host-guest clock parity (`pkg/controllers/app/engine/controllers/docker_watcher.go:84`). ### Testing Assessment The BATS tests cover container state mirroring, finalizer deletion delays, volume hashing validation, and backend transitions comprehensively. 1. The only notable missing test scenario is validating the behavior when a `Container` resource is applied with a non-existent Docker ID or un-parseable desired state. ### Documentation Assessment All engine-controller specific functionality is documented neatly in `docs/design/api_containers.md`, explaining the finalizer lifecycle, default fallback actions, and mirror naming logic precisely. The comment-to-code correlation inside the reconcilers is high. ### Commit Structure The single squash commit contains a cohesive, self-contained functional block representing the engine controller logic. ### Acknowledged Limitations - **Periodic fullSync tick requirement for transient API drops:** - **Code comment:** `pkg/controllers/app/engine/controllers/docker_watcher.go:191-196` ("A transient handleEvent error... is logged and the event is dropped... The bounded-window fix is a periodic fullSync tick, deferred until the rest of the mirroring work lands.") - **Note:** The author acknowledges that a transient K8s API failure (like a 409 conflict during the startup sequence) skips that resource's sync until the next explicit Docker event triggers it. ### Coverage Summary - `.github/workflows/bats.yaml`: Trivial - `bats/tests/32-app-controllers/app.bats`: Trivial - `bats/tests/32-app-controllers/engine.bats`: I2 - `cmd/app-controller/main.go`: Trivial - `cmd/rdd/set.go`: Reviewed, no issues - `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`, `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`: Trivial - `pkg/controllers/app/engine/controller.go`: Reviewed, no issues - `pkg/controllers/app/engine/controllers/docker_watcher.go`: C1 - `pkg/controllers/app/engine/controllers/engine_reconciler.go`: C2 - `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`: 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/instance/instance.go`: Reviewed, no issues - `pkg/service/cmd/service.go`: Trivial --- ## Resolution | | | |---|---| | **Addressed** | 2026-04-14 | | **Commit** | `89e9427 WIP: squash of docker-v2 engine commits for rebase` (fixup autosquashed into the original `a154249`) | | # | Finding | Action | |---|---------|--------| | 1 | Critical #1: Watcher event-stream error races with `alive()` via defer LIFO order | Fixed | | 2 | Critical #2: `reconcileContainerSpecs` errors block all finalizer processing | Fixed | | 3 | Important #1: Bootstrap resources are created once in `fullSync` but never re-watched | Skipped | | 4 | Important #2: `rdd set running=true` wait returns success before Lima applies template/backend changes | Skipped | | 5 | Important #3: `syncAllVolumes` fails the whole `fullSync` on a transient per-item error | Fixed | | 6 | Important #4: `engine.bats` computes `DOCKER_HOST` via command substitution at file scope | Fixed (added `docker_socket` key to `rdd svc paths` and switched the test to use it) | | 7 | Important #5: `removeImagesByID` can miss Image mirrors whose `status.id` has not yet been populated | Commented | | 8 | Suggestion #1: `isStopped` omits `ContainerStatusUnknown` | Skipped | | 9 | Suggestion #2: Controller multiplexes too many event domains through one App-scoped loop | Skipped | | 10 | Suggestion #3: `syncAllImages`/`syncAllContainers` inspect sequentially at startup | Skipped | | 11 | Suggestion #4: `engine.bats` bypasses `setup_rdd_control_plane` | Commented | | 12 | Suggestion #5: `containerd backend` test never switches back to moby | Skipped | | 13 | Suggestion #6: `reconcileContainerState` reads from potentially stale informer cache | Skipped | | 14 | Suggestion #7: `conditionInfo` silent-zero on missing `observedGeneration` | Skipped | | 15 | Suggestion #8: Image mirror naming scheme differs from volume naming scheme | Fixed (migrated to `img-` to match `vol-`; updated engine.bats to query by `status.id` instead of derived name) | | 16 | Testing gaps (5 scenarios) | Skipped (batch) | | 17 | Documentation gap (tighten api_app.md / cmd_app.md wording) | Skipped (batch) |