# Deep Review: 20260412-003842-pr-295

| | |
|---|---|
| **Date** | 2026-04-12 00:38 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 1 |
| **Author** | [@jandubois](https://github.com/jandubois) |
| **PR** | [#295](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/295) — Container Engine watcher |
| **Branch** | `docker` |
| **Commits** | `cc5875e` ci: capture support bundle when BATS targets time out<br>`02d4d91` Make rdd set wait for the desired state by default<br>`4b062fb` Add BATS integration tests for the engine controller<br>`86969f8` Document engine mirroring and container state transitions<br>`6dfa5ad` Allow container spec.state transitions via webhook<br>`174fc06` 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** | **Rework** — one critical data-path bug breaks `fullSync` for any Docker volume with `_` or uppercase, plus eight important regressions around finalizer/cleanup error handling, containerd backend support, the new `rdd set` wait semantics, and per-reconcile list sweeps. |
| **Wall-clock time** | `1 h 26 min 58 s` |

## Executive Summary

This PR introduces the engine controller that mirrors Docker state into Kubernetes resources, adds a "wait for desired state" default to `rdd set`, ships BATS coverage for the new flow, and adds a CI support-bundle wrapper that captures diagnostics when BATS targets hang. The design cleanly separates user intent (`spec.state`) from engine reality (`status.status`), and the finalizer/mirror contract is sensibly documented. However, the implementation ships one critical regression (volume name sanitization allows characters that K8s rejects, crashing the entire full sync on common volume names), plus eight important issues clustered around finalizer error handling (dropped K8s resources on Docker errors, silently-swallowed Conflict errors, edge-triggered cleanup that can't retry), containerd backend support, the semantics of the new wait logic, and per-reconcile O(N) list sweeps that multiply under event load.

Structure: 1 critical, 8 important, 8 suggestions.

---

## Critical Issues

C1. **`sanitizeKubernetesObjectName` leaves volume names that fail RFC 1123 validation, breaking the entire fullSync** — `pkg/controllers/app/engine/controllers/sync_images.go:27-29`, `pkg/controllers/app/engine/controllers/sync_volumes.go:66-72` [Gemini 3.1 Pro]

```go
func sanitizeKubernetesObjectName(input string) string {
	return strings.NewReplacer("/", "-", ":", ".").Replace(input)
}
```

Docker permits underscores and uppercase letters in volume names (e.g., `My_Volume`, `project_db_data`, any Compose-created volume), but `sanitizeKubernetesObjectName` only replaces `/` and `:`. When `applyVolume` calls `w.k8s.Apply(...)` with a name containing `_` or `A-Z`, the Kubernetes API server rejects it due to RFC 1123 subdomain validation on `metadata.name`.

The failure is not localized. `syncAllVolumes` collects errors from each `applyVolume` and returns `errors.Join(errs...)` at `pkg/controllers/app/engine/controllers/sync_volumes.go:89`; `fullSync` at `pkg/controllers/app/engine/controllers/docker_watcher.go:316-329` joins and returns that to `newDockerWatcher`, which aborts the watcher entirely. One volume with an uppercase letter or underscore means the engine never connects, `ContainerEngineReady` stays `False`, and `rdd set running=true` hangs for its full 5-minute timeout.

Fix: produce a deterministic encoding that always yields a valid RFC 1123 subdomain — e.g. `fmt.Sprintf("vol-%x", sha256.Sum256([]byte(vol.Name)))` — and keep the original Docker name in `status.name` (which `deleteVolume` already uses for the reverse path via `v.Status.Name`). The same treatment is needed for image tags since they can contain uppercase characters as well.

---

## Important Issues

I1. **Engine controller unconditionally targets Docker, so containerd backends never satisfy `ContainerEngineReady`** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:76-107`, `pkg/controllers/app/engine/controllers/docker_watcher.go:42-55` [Codex GPT 5.4, Gemini 3.1 Pro]

```go
running := meta.IsStatusConditionTrue(app.Status.Conditions, "Running")
...
if running && !watcherRunning {
	log.Info("App is running, starting Docker watcher")
	if err := r.startWatcher(ctx); err != nil {
		log.Error(err, "Failed to start Docker watcher")
		_ = r.setEngineCondition(ctx, &app, metav1.ConditionFalse, "ConnectFailed", err.Error())
		return ctrl.Result{}, err
	}
```

The engine reconciler decides the watcher lifecycle purely from the App's `Running` condition; it never consults `app.Spec.ContainerEngine.Name`. Meanwhile `lima-template.yaml:47-52` explicitly disables `docker.service` when `PARAM_CONTAINER_ENGINE=containerd`, and `newDockerWatcher` unconditionally pings the Docker socket. So for a containerd-backed VM, the reconciler enters an endless `ConnectFailed` loop.

This intersects directly with the new `rdd set` wait path: the help text at `cmd/rdd/set.go:70-72` advertises `rdd set running=true containerEngine.name=containerd` as a supported example, and `waitForDesiredState` at `cmd/rdd/set.go:450-454` waits for `ContainerEngineReady=True`. In containerd mode that condition can never become True, so the command always hangs until the 300-second timeout.

Fix: gate the watcher on `app.Spec.ContainerEngine.Name == "moby"` (or any other Docker-compatible engine you add later). For containerd, either short-circuit `ContainerEngineReady` to a distinct terminal reason (e.g. `Unsupported` / `NotApplicable`) so the CLI can detect "nothing to wait for," or skip the condition entirely and drive the CLI off the App's `Running` condition instead. Either way, the "waiting for container engine" branch in `waitForDesiredState` needs a matching adjustment.

I2. **`ContainerNamespace` mirror finalizer is never processed on user delete — objects stick in `Terminating` until the next VM shutdown** — `pkg/controllers/app/engine/controllers/sync_volumes.go:44-50`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:266-281`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:354-361` [Codex GPT 5.4]

```go
func (w *dockerWatcher) syncContainerNamespace(ctx context.Context) error {
	applyConfig := containersv1alpha1apply.ContainerNamespace(containerNamespace, apiNamespace).
		WithFinalizers(mirrorFinalizer)

	return w.k8s.Apply(ctx, applyConfig,
		client.ForceOwnership, client.FieldOwner(controllerName))
}
```

`syncContainerNamespace` now adds the `engine.rancherdesktop.io/docker-mirror` finalizer to the `moby` ContainerNamespace.

`processFinalizers` at `engine_reconciler.go:266-281` only covers Containers, Images, and Volumes, and `SetupWithManager` at lines 354-361 does not watch ContainerNamespaces.

The existing `ValidateDelete` webhook at `pkg/controllers/containers/containernamespace/container_namespace_webhooks.go:24-29` returns only a warning, so users can still issue `kubectl delete containernamespace/moby`. The object gets a deletion timestamp, retains the mirror finalizer, and is never unstuck until the next VM stop (when `cleanupMirrorResources` strips finalizers wholesale). The pre-existing "namespace delete TODO" just graduated into a visible dead end.

Fix: either stop adding the mirror finalizer to ContainerNamespace at all (it isn't reverse-managed and `cleanupMirrorResources` can still sweep it at stop time), or add a dedicated ContainerNamespace watch + finalizer handler and tighten the delete webhook to reject user-initiated deletes of system-managed namespaces up front.

I3. **Finalizer is removed even when the Docker-side delete fails, silently orphaning the real Docker object** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:283-340`, `pkg/controllers/app/engine/controllers/docker_watcher.go:273-304` [Codex GPT 5.4]

```go
func (r *EngineReconciler) processContainerFinalizers(ctx context.Context, w *dockerWatcher) error {
	...
	for i := range containers.Items {
		c := &containers.Items[i]
		if c.DeletionTimestamp == nil || !hasFinalizer(c, mirrorFinalizer) {
			continue
		}
		w.deleteContainer(ctx, c.Name)
		if removeFinalizer(c, mirrorFinalizer) {
			if err := r.Update(ctx, c); err != nil {
				return fmt.Errorf("failed to remove finalizer from container %s: %w", c.Name, err)
			}
		}
	}
	return nil
}
```

`deleteContainer` / `deleteImage` / `deleteVolume` at `docker_watcher.go:273-304` swallow all Docker errors (`V(1)` log only). The finalizer callers (`processContainerFinalizers`, `processImageFinalizers`, `processVolumeFinalizers`) then unconditionally strip the finalizer and delete the K8s mirror.

This directly violates the contract in `docs/design/api_containers.md:56-59`: "the finalizer handler deletes the corresponding object in the container engine, then removes the finalizer to allow K8s garbage collection." A transient Docker daemon error, permission error, or conflict (container still running, image in use, volume mounted) leaves the real Docker object in place while the Kubernetes mirror is gone for good. Eventual divergence is guaranteed unless the next `fullSync` rebuilds the mirror — and a user who tried to delete it via the API will be surprised that it has been silently recreated.

Fix: make the delete helpers return errors. Ignore only the "not found" / already-gone cases (e.g. `errdefs.IsNotFound`), treat all other errors as a failure, and leave the finalizer in place so `processFinalizers` retries on the next reconcile. If the error is persistently terminal (e.g. "container is in use"), surface it on the Container's conditions so the user can see why deletion is blocked.

I4. **`rdd set running=false` wait returns on `ContainerEngineReady` change, not on the VM actually stopping** — `cmd/rdd/set.go:436-462` [Codex GPT 5.4]

```go
const cond = "ContainerEngineReady"
if runningVal == "true" {
	logrus.Info("Waiting for container engine to be ready")
	return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
		return conditionStatus(obj, cond) == metav1.ConditionTrue
	})
}
logrus.Info("Waiting for container engine to disconnect")
return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
	return conditionStatus(obj, cond) != metav1.ConditionTrue
})
```

The help text at `cmd/rdd/set.go:65-67` promises that `running=false` "waits for the VM to stop", but `waitForDesiredState` returns as soon as `ContainerEngineReady` is absent or `False`. That condition is already `False` on the engine controller's `ConnectFailed` path, can be `False` while the App controller is still propagating `running=false` to the LimaVM, and can flip to `False` long before the LimaVM actually finishes shutting down. In those windows the command reports success while the VM is still running.

Fix: for `running=false`, wait on the App's `Running` condition going to `False` (or the LimaVM's equivalent stopped condition). Reserve `ContainerEngineReady` for the `running=true` / Docker branch. This also sidesteps the I1 containerd hang on the `running=true` side once the gating there is fixed.

I5. **Cleanup on VM stop is fragile end-to-end: no Conflict retry, first-error short-circuit, and an edge-triggered outer guard that makes retries impossible** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:78-126`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:210-234` [Claude Opus 4.6, Gemini 3.1 Pro]

```go
for _, item := range items {
	obj := item.(client.Object)
	if removeFinalizer(obj, mirrorFinalizer) {
		if err := r.Update(ctx, obj); err != nil {          // no retry on Conflict
			return fmt.Errorf("failed to remove finalizer from %s/%s: %w", ...)
		}
	}
	if err := client.IgnoreNotFound(r.Delete(ctx, obj)); err != nil {
		return fmt.Errorf("failed to delete %s/%s: %w", ...)
	}
}
```

Three stacked weaknesses turn any transient problem during cleanup into lasting damage:

1. **Inner loop has no Conflict retry** [Claude Opus 4.6]. `deleteAllOfType` calls `r.Update(ctx, obj)` to strip finalizers with no `retry.RetryOnConflict` wrapper. A stale cache or concurrent modification during shutdown returns `Conflict`, which propagates up as a generic error.
2. **First error short-circuits the remaining types** [Claude Opus 4.6]. `cleanupMirrorResources` at `engine_reconciler.go:190-207` calls `deleteAllOfType` four times in sequence for Container / Volume / Image / ContainerNamespace. Any error in the first call returns before the remaining three run, leaving a half-cleaned state across kinds.
3. **Outer retry is edge-triggered on in-memory watcher state** [Gemini 3.1 Pro]. `stopWatcher()` sets `r.watcher = nil` *before* `cleanupMirrorResources` runs at `engine_reconciler.go:115-124`. When cleanup returns an error the reconcile requeues — but on the retry, `watcherRunning` is now `false`, the `!running && watcherRunning` branch is skipped entirely, and no further cleanup attempt happens until the user stops the VM again.

A successful next start partially self-heals via the `syncAllContainers` / `syncAllImages` / `syncAllVolumes` stale-item pruning, so this is not permanent orphaning — but between stop and that next start, the resources are visible to `rdd ctl get` and violate the BATS assertion at `bats/tests/32-app-controllers/engine.bats:160-168`. Objects mid-deletion (finalizer already stripped, Delete not yet issued) can also land in a broken state.

Fix: (1) wrap the inner `Update` in `retry.RetryOnConflict` with the default backoff; (2) collect errors across the four types with `errors.Join` instead of returning on the first; (3) decouple "cleanup needed" from the in-memory watcher pointer — run cleanup whenever `!running` regardless of `watcherRunning`, or persist a short-lived "cleanup pending" condition on the App.

I6. **Initial `fullSync` issues N sequential `Inspect` calls per container/image, amplifying cold-start latency** — `pkg/controllers/app/engine/controllers/sync_containers.go:39-45`, `pkg/controllers/app/engine/controllers/sync_images.go:45-55` [Gemini 3.1 Pro]

```go
for _, dc := range listResult.Items {
	dockerIDs[dc.ID] = true
	if err := w.syncContainer(ctx, dc.ID); err != nil {
		errs = append(errs, err)
	}
}
```

`syncAllContainers` calls `ContainerList`, then loops and calls `ContainerInspect` once per container. `syncAllImages` does the same with `ImageList` + `ImageInspect`. A developer laptop with several hundred images (not uncommon on a long-lived machine) hits the Docker socket hundreds of times sequentially before the watcher goroutine enters its event loop, and every hit goes through the Lima port-forwarded Unix socket. Observed startup latency for `rdd set running=true` will grow linearly with the image count, and the operation is entirely serial — `newDockerWatcher` holds `watcherMu` the whole time.

This also intersects with I1: even in the correct Docker case, any developer with a bloated local cache pays this cost on every VM restart.

Fix: seed the initial mirror from `List` responses where the shape is sufficient (image list has `RepoTags`, `Size`, `Labels`, etc.), and defer `Inspect` to event-driven updates or lazy per-resource fetches when a user actually queries a field that only `Inspect` exposes. If an Inspect is still needed for full status, do it concurrently with a small worker pool so startup isn't bounded by serial round-trips.

I7. **Every App reconcile re-lists Containers, Images, and Volumes; per-event work scales O(N) with mirror size** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:128-140`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:266-341`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:354-361` [Claude Opus 4.6, Codex GPT 5.4]

```go
// Handle container spec changes (state transitions).
if running {
	if err := r.reconcileContainerSpecs(ctx); err != nil {
		return ctrl.Result{}, fmt.Errorf("failed to reconcile container specs: %w", err)
	}
}

// Handle finalizer processing for resources being deleted.
if running {
	if err := r.processFinalizers(ctx); err != nil {
		return ctrl.Result{}, fmt.Errorf("failed to process finalizers: %w", err)
	}
}
```

Every reconcile with `running=true` executes four unfiltered list operations:

- `List(ContainerList)` in `reconcileContainerSpecs` at line 247
- `List(ContainerList)` in `processContainerFinalizers` at line 285
- `List(ImageList)` in `processImageFinalizers` at line 305
- `List(VolumeList)` in `processVolumeFinalizers` at line 325

The reconcile fires on *every* Container / Image / Volume watch event via `Watches(&containersv1alpha1.Container{}, enqueueApp)` at `engine_reconciler.go:358-360`, and each status update the Docker watcher writes back generates one such event. So every container status change on a host with N mirrored containers performs 3·N K8s reads and a nested `reconcileContainerState` sweep that calls `ContainerStart` / `ContainerStop` against Docker for every container with a non-`unknown` spec. On a busy system with dozens of containers cycling the multiplier is immediate and user-visible.

The same architecture also made it easy to miss `ContainerNamespace` coverage entirely (see I2) — the App-level sweep approach has no compile-time guarantee that every mirrored kind has a finalizer handler.

Fix: split the engine controller into (a) an App-level session manager that only owns the Docker watcher lifecycle and (b) per-resource reconcilers (`Container`, `Image`, `Volume`, `ContainerNamespace`) that handle their own finalizer and spec updates with `For()` predicates filtering to "deletion timestamp set" or "spec generation changed." That removes the O(N) sweeps and makes coverage explicit per kind.

I8. **`removeMirrorResource` uses `IgnoreNotFound` on `Update`, silently swallowing `Conflict` / `Forbidden` / every other error** — `pkg/controllers/app/engine/controllers/docker_watcher.go:225-236` [Claude Opus 4.6]

```go
if removeFinalizer(obj, mirrorFinalizer) {
	if err := w.k8s.Update(ctx, obj); err != nil {
		return client.IgnoreNotFound(err)     // swallows Conflict/Forbidden/etc.
	}
}
return client.IgnoreNotFound(w.k8s.Delete(ctx, obj))
```

`client.IgnoreNotFound` returns `nil` only for `IsNotFound` errors and passes everything else through — but here it is being used as a generic "swallow if it fails" on `Update`, which is wrong. `Conflict` on the finalizer-strip `Update` (the common optimistic-concurrency outcome when two goroutines race on the same mirror object) is silently treated as success, and the subsequent `Delete` then runs against a stale snapshot. The only expected benign error on `Update` here is `NotFound` (object deleted out from under us); everything else should surface.

This path is reachable for every Docker `Destroy` event through `handleContainerEvent` / `handleImageEvent` / `handleVolumeEvent`, which makes it a frequent trigger in normal use. Combined with I5 above, a Conflict here leads to a finalizer that was never actually removed plus a Delete that appears to succeed locally — exactly the kind of divergence the finalizer contract is meant to prevent.

Fix: drop `IgnoreNotFound` on the `Update` and either (a) propagate the error so the event loop retries, or (b) wrap in `retry.RetryOnConflict` with a bounded backoff and only propagate on terminal errors. Keep `IgnoreNotFound` on the final `Delete` — that one is correct.

---

## Suggestions

S1. **Design doc still documents `ContainerCreateRequest.spec.state` as defaulting to `running`** — `docs/design/api_containers.md:156-160` [Codex GPT 5.4]

```yaml
spec:
  name: magical_gates
  namespace: k8s.io
  state: running # Default to `running`
```

The API default in `pkg/apis/containers/v1alpha1/container_types.go:217-220` and the generated CRD at `pkg/controllers/containers/container/crd.yaml:133` both say `default: unknown`. Consumers reading the design doc will get the wrong startup semantics for create requests.

Fix: update the example to `state: unknown` (and drop the `Default to running` comment), or, if create-requests really should auto-start, change the API default back to `running` and document why the Container default is different.

S2. **`applyContainer` only sets the finalizer on first create; a manually stripped finalizer never gets restored** — `pkg/controllers/app/engine/controllers/sync_containers.go:86-97` [Gemini 3.1 Pro]

```go
var existing containersv1alpha1.Container
err := w.k8s.Get(ctx, client.ObjectKey{Name: inspect.ID, Namespace: apiNamespace}, &existing)
if apierrors.IsNotFound(err) {
	applyConfig := containersv1alpha1apply.Container(inspect.ID, apiNamespace).
		WithFinalizers(mirrorFinalizer).
		WithSpec(containersv1alpha1apply.ContainerSpec().WithState(containersv1alpha1.ContainerStatusUnknown))
	if err := w.k8s.Apply(ctx, applyConfig, client.ForceOwnership, client.FieldOwner(controllerName)); err != nil {
		return fmt.Errorf("failed to create container %s: %w", inspect.ID, err)
	}
}
```

`applyContainer` deliberately only writes `spec` on first create so that later syncs do not clobber a user-initiated `spec.state`. That's correct for the spec, but as a side effect it also means the mirror finalizer is only written once — if it is ever missing on a subsequent sync (manually stripped, or a new API server behavior), it never gets restored. Since Apply supports setting only specific fields, the structural metadata (finalizers, labels) can be applied unconditionally on every sync without touching `spec.state`.

Fix: split the apply into two field-owner scopes: one that always writes finalizers and any owner-managed metadata, and one that only writes `spec.state=unknown` on the not-found path.

S3. **`rdd set` watch loop aborts on any watch-channel closure instead of reconnecting** — `cmd/rdd/set.go:496-499` [Gemini 3.1 Pro]

```go
case event, ok := <-watcher.ResultChan():
	if !ok {
		return errors.New("watch channel closed while waiting for App state")
	}
```

Kubernetes watches routinely close for benign reasons: API server timeouts, proxies cutting idle connections, resource-version compaction. Returning a hard error on closure means any sufficiently long-running `rdd set` may fail even though the desired state would eventually be reached. The `rdd set` timeout is 300s by default, which is already in range for a typical watch disconnect.

Fix: wrap the watch in a retry loop, ideally via `k8s.io/client-go/tools/watch` (`watchtools.UntilWithSync` or similar), so a benign channel close resumes from the last observed resource version.

S4. **`reconcileContainerState` ignores intermediate container states (`paused`, `restarting`), so `spec.state=created` silently no-ops on them** — `pkg/controllers/app/engine/controllers/docker_watcher.go:254-268` [Claude Opus 4.6]

```go
case containersv1alpha1.ContainerStatusCreated:
	if actual == containersv1alpha1.ContainerStatusRunning {
		log.Info("Stopping container", "id", c.Name)
		_, err := w.cli.ContainerStop(ctx, c.Name, dockerclient.ContainerStopOptions{})
		return err
	}
```

The `desired=created` branch only stops the container when `actual == ContainerStatusRunning`. If a user patches `spec.state=created` on a container that happens to be in `paused` or `restarting` state, the reconciler returns `nil` and waits for a `running` status it may never see — the user's intent is dropped silently. Docker's `ContainerStop` handles paused containers natively.

Fix: drop the narrow `actual == running` guard and call `ContainerStop` for any non-`created`, non-`exited` / non-`dead` state (or just unconditionally and let Docker short-circuit).

S5. **`bats-with-timeout.sh` hand-rolls `instance.LogDir()` logic, with a comment acknowledging the duplication** — `scripts/bats-with-timeout.sh:22-28` vs `pkg/instance/instance.go:67-82` [Claude Opus 4.6]

```bash
instance_name="rancher-desktop-${RDD_INSTANCE:-2}"
case "$(uname -s)" in
    Linux)   log_dir="${HOME}/.local/state/${instance_name}" ;;
    Darwin)  log_dir="${HOME}/Library/Logs/${instance_name}" ;;
    MINGW*|MSYS*|CYGWIN*) log_dir="${LOCALAPPDATA:-${HOME}/AppData/Local}/${instance_name}-logs" ;;
    *)       log_dir="${HOME}/${instance_name}-logs" ;;
esac
```

The shell script duplicates the OS → log-dir mapping because `rdd` itself may be hung at timeout time. That reason is legitimate — but the two implementations will drift.

Fix: expose the log-dir path as a lightweight helper that doesn't touch the control plane (e.g., `rdd debug log-dir` or a standalone `rdd-log-dir` printing binary) so the shell script can call it even when the service is hung. As a cheaper alternative, add a CI test that asserts the shell mapping matches `instance.LogDir()` across all three platforms.

S6. **Support-bundle output is capped at 30–50 lines via `head`, which silently truncates the exact diagnostic detail we'd want after a hang** — `scripts/bats-with-timeout.sh:66-67`, `scripts/bats-with-timeout.sh:86-87`, `scripts/bats-with-timeout.sh:98-107` [Claude Opus 4.6]

```bash
lsof -p "$pid" 2>/dev/null | head -30 | sed 's/^/  /' || true
...
ss -tupn 2>&1 | head -50 || true
```

The bundle already exists as a post-mortem artifact rather than a live log stream, so the reason to cap output is unclear — and the cap hides the specific file descriptors or sockets that would identify the cause of a hang. `head -30` on `lsof -p` of a busy process is almost certain to drop the interesting fd.

Fix: remove the `head` caps, or hoist them to a configurable `BATS_SUPPORT_BUNDLE_MAX_LINES` that defaults high (or to 0 for unlimited) and stays low only on explicit opt-in.

S7. **Add a comment explaining why `applyContainer` uses a Get-then-Apply pattern instead of pure server-side Apply** — `pkg/controllers/app/engine/controllers/sync_containers.go:86-97` [Claude Opus 4.6]

The Get-then-Apply pattern is correct: server-side Apply with field ownership would *still* reassert `spec.state=unknown` on every sync (a field the engine controller does not own after creation), which would clobber user-initiated `spec.state=running` / `spec.state=created`. The Get check ensures `WithSpec` is only called on the not-found path.

Fix: a two-line comment above the `Get` call explaining the invariant — "spec.state must be written exactly once, on creation; subsequent syncs must not reassert it to avoid clobbering user intent." Saves the next reader from puzzling out why the Apply omits `WithSpec` unconditionally.

S8. **Dead `scheme` field in `dockerWatcher`** — `pkg/controllers/app/engine/controllers/docker_watcher.go:30`, `pkg/controllers/app/engine/controllers/docker_watcher.go:41`, `pkg/controllers/app/engine/controllers/docker_watcher.go:63` [Claude Opus 4.6]

```go
type dockerWatcher struct {
	cli    *dockerclient.Client
	k8s    client.Client
	scheme *runtime.Scheme // never read
	...
}
```

`scheme *runtime.Scheme` is plumbed through `newDockerWatcher`, stored on the struct, and never read anywhere in the package (verified with `grep -n 'w\.scheme'`). Remove the field and propagate the removal up through `EngineReconciler.startWatcher` at `engine_reconciler.go:154` and the controller wiring at `pkg/controllers/app/engine/controller.go:59`.

---

## Design Observations

**Concerns**

- **Docker-only coupling in shared APIs** *(in-scope)* [Gemini 3.1 Pro] — The `Container`, `Image`, `Volume`, and `ContainerNamespace` types live under `pkg/apis/containers`, implying general container-engine support, but the engine controller is deeply coupled to Docker (hardcoded `moby` namespace, direct moby SDK). If containerd support is on the near roadmap, consider introducing an engine interface and an engine-specific implementation now — before Docker assumptions leak further into the API surface.

- **Webhook registration is load-bearing but fragile** *(future)* [Claude Opus 4.6] — The engine controller's creates and deletes intentionally bypass the Container webhook's "create not implemented" rejection because the Container webhook only registers `Update` at `pkg/controllers/containers/container/container_controller.go:88-90` and the ContainerNamespace webhook only registers `Delete`. The arrangement works but is invisible from the engine controller's perspective — any future change that registers the Container webhook for `Create` would break the engine silently with a generic error. A comment at the registration sites (or a small assertion that the engine controller's `apiNamespace` is exempt) would make the dependency explicit.

**Strengths**

- *(in-scope)* [Codex GPT 5.4] — The explicit `cleanupMirrorResources` on VM stop respects the project's "no garbage collector" constraint and puts the engine controller in charge of its own mirror ownership, which is the right direction for this architecture.

- *(in-scope)* [Codex GPT 5.4] — The new `bats-with-timeout.sh` diagnostic wrapper collects operator-useful state (process tree, wchan, Go goroutine dumps via SIGQUIT, lsof, `sample`) when a BATS target hangs instead of just killing it. This should make future CI hangs far easier to root-cause.

- *(in-scope)* [Gemini 3.1 Pro] — The design successfully isolates user intent (`spec.state = unknown | running | created`) from mirrored engine reality (`status.status`), so users can direct intent via the API without fighting the controller's continuous mirroring. The webhook at `container_webhooks.go:34-61` enforces exactly this invariant.

- *(in-scope)* [Claude Opus 4.6] — The `ValidateUpdate` spec-normalization trick at `pkg/controllers/containers/container/container_webhooks.go:49-53` (copy old spec, overwrite `State` with the new value, deep-equal against new) is a clean, future-proof way to enforce "only `spec.state` may change" — any field added to `ContainerSpec` later is automatically immutable without touching the webhook. Nice pattern.

---

## Testing Assessment

The new `bats/tests/32-app-controllers/engine.bats` covers the happy-path end-to-end flow well: full sync of the `moby` namespace, image/container/volume create & delete events, `spec.state` transitions in both directions, and VM-stop cleanup with a post-stop "every list returns empty" assertion. That is a strong foundation.

Gaps the findings above would have caught:

1. **Volume / image names outside `[a-z0-9.-]`** (C1): no test creates a Docker volume with an uppercase letter or underscore (e.g., `docker volume create My_Volume`). Adding one would have immediately surfaced the K8s naming crash.
2. **`containerd` backend startup** (I1): no test runs `rdd set running=true containerEngine.name=containerd` to assert either success or a clear "unsupported" error. The unconditional Docker-watcher path is currently unexercised for non-Docker engines.
3. **Docker-side delete failures** (I3): no test covers the case where `ContainerRemove` / `ImageRemove` / `VolumeRemove` returns a transient error. A lightweight way to force this is to delete a container that's mid-stop, or an image referenced by a running container.
4. **`ContainerNamespace` user delete** (I2): a test that does `rdd ctl delete containernamespace/moby` would have immediately shown the finalizer hang.
5. **`running=false` wait with degraded engine** (I4): no test exercises `rdd set running=false` from a state where `ContainerEngineReady` is already `False`, which is exactly the false-positive return case.
6. **Cleanup failure retry** (I5): no test exercises what happens when `cleanupMirrorResources` fails partway through (e.g., deleting `rancher-desktop` namespace concurrently) and the VM is then left in `running=false`.
7. **Watcher-died mid-session recovery** [Claude Opus 4.6]: no test exercises the `watcherDied` path — e.g., restart the Docker daemon while the App stays running and confirm the reconciler cleans up mirror resources, sets `ContainerEngineReady=Disconnected`, and recovers on the next reconcile. Docker-engine restarts are common enough to matter.
8. **Concurrent user patch vs. controller status update** [Claude Opus 4.6]: no test locks in the Server-Side Apply field-ownership guarantee — i.e., that a user `kubectl patch spec.state=running` concurrent with an engine status update does not clobber either write.
9. **`parseContainerName` edge cases** [Claude Opus 4.6] — the helper at `pkg/controllers/app/engine/controllers/sync_containers.go:157-164` has no direct test; names with extra slashes (`/ns/name/with/slashes`) are not exercised.
10. **`reconcileContainerState` against intermediate states** (S4): no test patches `spec.state=created` while the container is `paused` or `restarting`.
11. **Non-default `RDD_INSTANCE`** [Codex GPT 5.4] — The engine BATS suite hardcodes `DOCKER_HOST=unix://${HOME}/.rd2/docker.sock` at `engine.bats:14`, which matches the hardcoded instance in `pkg/instance/instance.go:128-136` and `lima-template.yaml:30`. Once those TODOs are resolved, the BATS setup will also need to derive the socket path from the instance suffix.

There are also no unit tests for the reconciler or watcher internals; all coverage rides on the BATS end-to-end flow. [Claude Opus 4.6] specifically called out envtest-level coverage of the sync/apply/finalizer logic as the biggest testing gap.

---

## Documentation Assessment

`docs/design/api_containers.md` gained useful new sections on engine mirroring, finalizer lifecycle, and `spec.state` transitions, and they match the implementation for the `moby` / Docker case. Two accuracy problems remain:

1. The `ContainerCreateRequest` example at `docs/design/api_containers.md:156-160` still shows `state: running # Default to running`, but the code now defaults to `unknown` (S1).
2. The "engine-specific namespaces for containerd" phrasing at `docs/design/api_containers.md:37-38` overstates current implementation — the controller hardcodes the `moby` namespace and, as called out in I1, cannot run against containerd at all today. Either add a "Docker only in the current release" qualifier, or qualify the sentence so readers don't expect containerd support.

The new CLI help in `cmd/rdd/set.go:65-72` similarly promises behavior that the code doesn't yet deliver for `running=false` (I4) and for `containerEngine.name=containerd` (I1). Updating it along with the fix is worth doing as part of the same change.

---

## Commit Structure

The branch is nine commits, each focused and individually reviewable (Rosetta option cleanup, socket path + distro workaround, Docker client dep, engine controller, webhook, design doc, BATS tests, `rdd set` wait, CI timeout wrapper). Commit order is logical — dependencies first, feature on top, tests and CI last — and most commits build on each other cleanly. No commit mixes unrelated concerns. Nothing to fix here.

---

## Acknowledged Limitations

- **Docker socket hardcoded to `.rd2`** — `pkg/instance/instance.go:127-128`, `pkg/controllers/app/app/lima-template.yaml:30`, `bats/tests/32-app-controllers/engine.bats:12-14` [Codex GPT 5.4]. All three locations carry a TODO noting the Lima template still hardcodes `.rd2` until the template derives the socket path from the instance suffix. The new controller and BATS tests both depend on this path, so the hardcode matters more now than before — but it is an explicit, acknowledged limitation.
- **Docker socket access not wired up for Windows/WSL2** — `bats/tests/32-app-controllers/engine.bats:17-20`. The `local_setup_file` helper skips the whole engine BATS suite on Windows; the current skip is consistent with the known limitation.
- **`containerEngine.name` changes still not waited on** — `cmd/rdd/set.go:436-439`. The TODO accurately acknowledges that VM-restarting property changes are not covered by the new default waiting until a `Reconciled` condition with `observedGeneration` exists. This is tracked elsewhere.

---

## Agent Performance Retro

### [Claude]

Claude's review was initially lost to a launcher bug (see Review Process Notes): the `claude --print` mode only captures the last assistant text block, and Claude emitted its findings across many text blocks as it worked. The full structured review was recovered post-hoc from the session JSONL and folded into this report. Claude's unique contributions are the strongest set of the three: the `removeMirrorResource` `IgnoreNotFound`-masking bug (I8), the compound cleanup weakness (Conflict-retry + first-error short-circuit) that strengthened I5, and the O(N) per-reconcile sweep concern that promoted the architectural observation into an actionable Important finding (I7). It also contributed four useful Suggestions (S4–S7) and a small dead-code cleanup (S8) that neither other agent caught. Its execution pattern — read a file, build a theory, verify with a targeted grep, write a finding — is visible in the tool-call mix and delivered zero false positives.

### [Codex]

Codex produced the widest "contract-violation" set: the `ContainerNamespace` finalizer stuckness (I2), the `running=false` wait-semantics regression (I4), and the "finalizer stripped even when Docker delete fails" contract violation (I3) are all unique to it. Its severity calibration was restrained — it intentionally chose zero criticals and classified everything as Important, which in hindsight left C1 slightly underweighted compared to Gemini's read but was otherwise well-tuned.

### [Gemini]

Gemini was fast and aggressive and caught the one true critical in the change — the volume-name sanitization crash (C1). It also raised real important issues: the edge-triggered cleanup bug (I5), the N+1 startup latency (I6), and the watch-channel-closure concern. Its weakness was an obvious false positive: it claimed a bash syntax error `"$ @"` in `scripts/bats-with-timeout.sh:143` that does not exist in the file (the actual code is `"$@"`). That finding carried a **critical** label and an Executive Summary pull quote, so without verification it would have completely dominated the consolidated output. Gemini also upgraded the edge-trigger cleanup bug to critical on a plausible-but-overblown "permanent orphaning" argument — partial self-healing on the next fullSync brings it back to Important. Net: strong unique findings, but the only agent that needed a correction on severity and hallucinated a finding outright.

### Summary

| | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 15m 59s | 6m 54s | 6m 33s |
| Findings | 3I 5S | 4I 1S | 1C 2I 2S |
| Tool calls | 88 (Bash 39, Read 28, Grep 13) | 50 (shell 50) | 6 (grep_search 3, run_shell_command 3) |
| Design observations | 2 | 3 | 2 |
| False positives | 0 | 0 | 1 |
| Unique insights | 5 | 3 | 3 |
| Files reviewed | 21 | 21 | 21 |
| Coverage misses | 0 | 0 | 0 |
| **Totals** | **3I 5S** | **4I 1S** | **1C 2I 2S** |
| Downgraded | 1 (I→S) | 0 | 1 (I→S) |
| Dropped | 0 | 0 | 1 |


**Reconciliation:** Gemini C1 ("bash syntax error in `bats-with-timeout.sh:144`"): rejected as a false positive — verified the file directly, the actual code is `"$@" &` with no space. Gemini C2 ("edge-triggered cleanup permanently orphans resources"): critical → important (merged into I5) — the failure mode is real but `syncAllContainers` / `syncAllImages` / `syncAllVolumes` stale-item pruning partially self-heals on the next successful start, so "permanent" is overstated. Gemini C3 ("volume naming"): kept as critical (C1 in the consolidated report), since one misnamed volume is enough to break the entire engine startup. Claude I4 ("dead `scheme` field"): important → suggestion (S8) — a hygiene cleanup rather than a correctness or performance concern.

Overall assessment: Claude and Codex each contributed the highest-signal set for their respective problem classes — Claude for cleanup / concurrency error handling and perf architecture, Codex for contract violations and CLI semantics. Gemini contributed the data-path critical (C1) and the reconciler state-machine concern (I5) plus the efficiency finding (I6). No two agents alone would have produced the full consolidated set; all three were necessary.

---

## Review Process Notes

### Skill improvements

- **Launcher bug fixed post-consolidation: `claude --print` only captures the last assistant text block, not the full response.** During this review, Claude wrote a complete 18KB structured review — 4 Important findings, 7 Suggestions, full coverage summary — across many `assistant` text blocks as it worked, then emitted a one-paragraph wrap-up as its final message. `launch.go` was piping `claude --print` stdout directly to the review file, so only the wrap-up survived. The full review was recovered from the session JSONL and folded into this consolidated report, and the skill's `launch.go` now invokes Claude with `--print --verbose --output-format stream-json`, post-processes the NDJSON stream to concatenate every `assistant` text block, and writes the result to the review file. Future Claude reviews will capture the complete output. (Claude was also switched to `claude-opus-4-6[1m]` to get the 1M context variant, closing a smaller latent issue that would have eventually bitten larger PRs.)

### Repo context updates

- None for this review. `deep-review-context.md` already describes the controller registration pattern, the finalizer / CRD lifecycle, and the BATS `RDD_INSTANCE` isolation, all of which held up against this PR.

---

## Appendix: Original Reviews

### [Gemini] — Pass 1

### Executive Summary
This PR implements an engine controller that mirrors Docker state into Kubernetes resources and introduces state-waiting logic to `rdd set`. While the design correctly isolates user intent (`spec.state`) from engine reality (`status`), it introduces several regressions. There is a critical bash syntax error that will immediately crash the integration tests, a state machine bug in the reconciler that can permanently orphan K8s resources, and a K8s object naming violation for Docker volumes containing non-RFC1123 characters. Additionally, the initial Docker synchronization triggers severe N+1 query latency, and the new `rdd set` waiting logic fails to account for non-Docker environments like containerd.

### Findings

**Critical Issues**

C1. **Syntax error in background command execution breaks integration tests** — `scripts/bats-with-timeout.sh:144` (critical, regression)

```bash
# Start the command in the background.
"$ @" &
cmd_pid=$!
```
The space in `"$ @"` causes Bash to attempt to execute a string starting with `$` and a space (e.g., `$ @`), rather than expanding the positional arguments. This results in an immediate `command not found` error (exit code 127). Because `BATS_TIMEOUT` routes all integration tests through this script, the entire integration test suite will crash instantly without running any tests.

**Fix**: Remove the space so positional arguments expand correctly.
```bash
# Start the command in the background.
"$@" &
cmd_pid=$!
```

C2. **Reconciler edge-triggers cleanup on transient memory state, permanently orphaning resources** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:77-85`, `110-116` (critical, regression)

```go
	if !running && watcherRunning {
		log.Info("App is not running, stopping Docker watcher")
		r.stopWatcher()
		if err := r.cleanupMirrorResources(ctx); err != nil {
			log.Error(err, "Failed to clean up mirror resources")
			return ctrl.Result{}, err
		}
```
The reconciler uses transient memory state (`watcherRunning`, `watcherDied`) to trigger resource cleanup. If `cleanupMirrorResources` returns an API error (e.g., due to a Kubernetes server blip), the reconcile loop requeues, but `r.watcher` is already `nil` (`watcherRunning == false`). On the subsequent retry, the cleanup block is bypassed entirely, leaving stale mirror resources permanently in the cluster. Furthermore, because `processFinalizers` silently returns without stripping finalizers when `w == nil` (lines 352-358), any user attempt to manually delete these orphaned resources will cause them to hang indefinitely in `Terminating`.

**Fix**: Make cleanup level-triggered rather than edge-triggered by removing the `watcherRunning` condition from the check, ensuring it always executes when the engine is not running.

C3. **Invalid Kubernetes object names for Docker volumes break engine synchronization** — `pkg/controllers/app/engine/controllers/sync_volumes.go:66-70`, `pkg/controllers/app/engine/controllers/sync_images.go:26-28` (critical, regression)

```go
	for _, v := range volumeList.Items {
		k8sName := sanitizeKubernetesObjectName(v.Name)
		activeNames[k8sName] = true
		if err := w.applyVolume(ctx, v); err != nil {
			errs = append(errs, err)
		}
	}
```
Docker permits underscores and uppercase letters in volume names (e.g., `My_Volume`, `project_db_data`), but `sanitizeKubernetesObjectName` only replaces `/` and `:`. When `applyVolume` calls `w.k8s.Apply`, the Kubernetes API server will reject any volume containing `_` or `A-Z` due to strict RFC 1123 subdomain validation rules on `metadata.name`. Because `syncAllVolumes` aggregates these errors, a single invalid volume name will cause `fullSync` to fail entirely, preventing the Docker watcher from ever starting.

**Fix**: Produce a safe, deterministic hash or encoding for the K8s Volume object name, while preserving the actual Docker volume name in `Status.Name`.

**Important Issues**

I1. **`rdd set` wait logic hangs indefinitely when containerd is used** — `cmd/rdd/set.go:440-445`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:68-75` (important, gap)

The CLI now waits for `ContainerEngineReady=True`. However, `EngineReconciler` unconditionally attempts to connect to the Docker socket without verifying if the user has selected `containerEngine.name=containerd`. In containerd mode, the socket is absent, causing ping failures. The reconciler continuously sets the condition to `ConnectFailed` (`ConditionFalse`), meaning the `rdd set` command will hang for its full 5-minute timeout.

**Fix**: Check `app.Spec.ContainerEngine.Name` in `EngineReconciler`. If it is `containerd`, update the condition to `True` (or `Unsupported`) immediately and skip the Docker watcher.

I2. **N+1 API queries during initial engine sync** — `pkg/controllers/app/engine/controllers/sync_containers.go:39-44`, `pkg/controllers/app/engine/controllers/sync_images.go:45-50` (important, regression)

During `fullSync`, the controller calls `ContainerList` and `ImageList`, then loops over every returned item to make individual `ContainerInspect` or `ImageInspect` API calls. For a user with a typical development environment containing hundreds of images and containers, this executes hundreds of sequential queries over the socket, introducing significant startup latency.

**Fix**: Use the data already available in the `List` responses to seed the initial K8s resources, and only call `Inspect` lazily when required or in response to specific update events.

I3. **`rdd set` watch loop aborts prematurely on K8s watch channel closure** — `cmd/rdd/set.go:485-489` (important, regression)

```go
		case event, ok := <-watcher.ResultChan():
			if !ok {
				return errors.New("watch channel closed while waiting for App state")
			}
```
K8s watch channels routinely close or disconnect gracefully due to API server timeouts, proxies, or compaction. Returning an error immediately rather than resuming from the last `ResourceVersion` will lead to flaky command timeouts for CLI users.

**Fix**: Wrap the watch in a retry loop using `k8s.io/client-go/tools/watch` utilities (like `watchtools.UntilWithSync`) to automatically reconnect.

**Suggestions**

S1. **`applyContainer` misses restoring finalizers on existing resources** — `pkg/controllers/app/engine/controllers/sync_containers.go:87-95` (suggestion, enhancement)

To avoid overwriting a user-initiated `spec.state`, `applyContainer` deliberately only applies the Spec (and the finalizer) when `IsNotFound` is true. If the K8s Container object exists but has had its finalizer erroneously removed, subsequent syncs will not restore it.

**Fix**: Since `ApplyConfiguration` allows omitting fields, omit `spec.state` for subsequent syncs but unconditionally apply the structural metadata and finalizers every time.

### Design Observations

**Concerns**

- **Docker-only coupling in shared APIs** *(in-scope)*: The `Container` and `Volume` APIs reside in `pkg/apis/containers`, implying general container engine support. However, the engine controller implementation is deeply coupled to Docker constructs (e.g., hardcoded "moby" namespaces, direct Docker SDK consumption). If containerd support is planned, consider splitting this controller into a swappable interface or an engine-specific implementation right away.

**Strengths**

- The design successfully isolates the App Spec state transitions (e.g., `unknown` vs `running`) from the actual mirrored status, allowing users to direct intent without fighting the engine's real-time reflection.

### [Codex] — Pass 1

### Executive Summary

This PR adds a Docker-backed engine mirroring controller, default `rdd set` waiting, and BATS coverage around the new flow. The main problems are that the new controller is started even for `containerd` VMs, `ContainerNamespace` mirrors now get a finalizer with no matching delete path, and the finalizer code drops Kubernetes resources even when the Docker delete fails. The new `running=false` wait path is also weaker than the help text promises: it returns on `ContainerEngineReady=False`, not when the VM has actually stopped.

### Findings

**Critical Issues**

None.

**Important Issues**

I1. **`ContainerNamespace` mirrors now get stuck in `Terminating` forever** — `pkg/controllers/app/engine/controllers/sync_volumes.go:44-49` (important, regression)

```go
func (w *dockerWatcher) syncContainerNamespace(ctx context.Context) error {
	applyConfig := containersv1alpha1apply.ContainerNamespace(containerNamespace, apiNamespace).
		WithFinalizers(mirrorFinalizer)

	return w.k8s.Apply(ctx, applyConfig,
		client.ForceOwnership, client.FieldOwner(controllerName))
}
```

`syncContainerNamespace()` now adds `engine.rancherdesktop.io/docker-mirror` to `ContainerNamespace`, but `processFinalizers()` only handles Containers, Images, and Volumes, and `SetupWithManager()` does not watch `ContainerNamespace` objects. The pre-existing delete webhook still allows DELETE with only a warning, so deleting `containernamespace/moby` now leaves a permanently terminating object until some later VM shutdown calls `cleanupMirrorResources()`. That materially worsens the older namespace-delete TODO into a user-visible dead end.

Fix: either stop adding the engine finalizer to `ContainerNamespace` entirely, or add a dedicated `ContainerNamespace` watch and finalizer handler and tighten the webhook so unsupported deletes are rejected before the object enters `Terminating`.

I2. **The Docker watcher runs for `containerd` VMs and drives them into a permanent `ConnectFailed` loop** — `pkg/controllers/app/engine/controllers/docker_watcher.go:42-55` (important, regression)

```go
socketPath := instance.DockerSocket()
cli, err := dockerclient.New(
	dockerclient.WithHost("unix://" + socketPath),
)
...
if _, err := cli.Ping(pingCtx, dockerclient.PingOptions{}); err != nil {
	cli.Close()
	return nil, fmt.Errorf("failed to ping Docker: %w", err)
}
```

The engine reconciler decides only from the App's `Running` condition; it never checks `app.Spec.ContainerEngine.Name`. At the same time, the Lima template explicitly disables Docker when `PARAM_CONTAINER_ENGINE=containerd`, while `newDockerWatcher()` always opens and pings the Docker socket. That means a containerd-backed VM will keep reconciling into `ConnectFailed` even though the selected backend intentionally does not run `docker.service`. The new CLI help even advertises `rdd set running=true containerEngine.name=containerd`, but this controller can never satisfy `ContainerEngineReady` for that configuration.

Fix: gate the engine controller on a Docker-capable backend, e.g. skip watcher startup unless `app.Spec.ContainerEngine.Name == "moby"`, and make the condition/wait logic report "unsupported backend" or no-op for `containerd` until there is a containerd watcher.

I3. **Mirror finalizers are cleared even when the Docker-side delete fails** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:293-297` (important, regression)

```go
w.deleteContainer(ctx, c.Name)
if removeFinalizer(c, mirrorFinalizer) {
	if err := r.Update(ctx, c); err != nil {
		return fmt.Errorf("failed to remove finalizer from container %s: %w", c.Name, err)
	}
}
```

`processContainerFinalizers()`, `processImageFinalizers()`, and `processVolumeFinalizers()` always remove the Kubernetes finalizer after calling the backend delete helpers. But `deleteContainer()`, `deleteImage()`, and `deleteVolume()` swallow every Docker error and only log it. That breaks the contract documented in `docs/design/api_containers.md:56-59`: a transient daemon error, permission error, or conflict can leave the real Docker object intact while the Kubernetes mirror is deleted permanently.

Fix: make the delete helpers return errors, ignore only backend `not found` cases, and keep the finalizer in place for every other failure so the delete retries on the next reconcile instead of orphaning the Docker object.

I4. **`rdd set running=false` returns on engine disconnect, not on VM stop** — `cmd/rdd/set.go:449-462` (important, regression)

```go
const cond = "ContainerEngineReady"
...
logrus.Info("Waiting for container engine to disconnect")
return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
	return conditionStatus(obj, cond) != metav1.ConditionTrue
})
```

The help text says that `running=false` waits "for the VM to stop", but `waitForDesiredState()` actually returns as soon as `ContainerEngineReady` is absent or False. That condition is already False on the engine controller's `ConnectFailed` path, and it can stay False while the App controller is still propagating `spec.running=false` to the LimaVM and before the LimaVM controller reports `Running=False`. In those states the new default wait reports success even though the VM is still stopping.

Fix: for `running=false`, wait on the App's `Running=False` condition (or the LimaVM's equivalent stopped condition). Reserve `ContainerEngineReady` for the `running=true`/Docker case.

**Suggestions**

S1. **The design doc still documents `ContainerCreateRequest.spec.state` as defaulting to `running`** — `docs/design/api_containers.md:156-160` (suggestion, regression)

```yaml
spec:
  name: magical_gates # If unset, generate one randomly
  namespace: k8s.io # Refers to a `ContainerNamespace` object
  state: running # Default to `running`
```

The example still says `ContainerCreateRequest.spec.state` defaults to `running`, but the actual API default changed to `unknown` in `pkg/apis/containers/v1alpha1/container_types.go:215-220`, and the generated CRD now matches that. That mismatch will send API consumers the wrong startup semantics for new create requests.

Fix: update the design doc and examples to `unknown`, or restore the old default if create requests are still supposed to start containers automatically.

### Design Observations

**Concerns**

1. The engine feature is currently split between a backend session manager and namespace-wide sweeps in the App reconciler. `reconcileContainerSpecs()` and `processFinalizers()` rescan every mirrored object on every relevant event, which made it easy to miss `ContainerNamespace` coverage entirely. A cleaner split would keep App-level logic only for backend session lifecycle and move per-resource finalizer/spec handling into resource-specific reconcilers; that removes the O(n) sweeps and makes watch coverage explicit per kind. `(future)`

**Strengths**

1. The explicit shutdown cleanup in `cleanupMirrorResources()` respects the repo's "no garbage collector" constraint and is the right ownership direction for mirror resources.

2. The BATS timeout wrapper collects operator-useful diagnostics instead of only killing a hung target, which should make future CI failures much easier to root-cause.

### [Claude] — Pass 1

*The full review below was recovered from the Claude CLI session JSONL after `claude --print` only returned the final wrap-up message. See Review Process Notes for the root cause and the fix to `launch.go`. The content is verbatim from the session log; only the headings were left unchanged.*

# Code Review: PR #295 — Container Engine watcher

## Executive Summary

This PR adds an engine controller that mirrors Docker container/image/volume state into K8s resources, relaxes the Container webhook to allow `spec.state` transitions, teaches `rdd set` to wait for the desired state, and wires a new BATS suite plus CI support-bundle capture. The core architecture is sound and consistent with the existing mock-controller mirror pattern. Several correctness concerns in cleanup/finalizer paths are worth addressing, and there are a handful of efficiency and hygiene issues. No blocking defects. Test coverage is solid for the happy path but thin for failure modes.

## Findings

### IMPORTANT

**I1 — `deleteAllOfType` aborts on first error; `client.Update` has no Conflict retry**
`pkg/controllers/app/engine/controllers/engine_reconciler.go:210-234`

```go
for _, item := range items {
    obj := item.(client.Object)
    if removeFinalizer(obj, mirrorFinalizer) {
        if err := r.Update(ctx, obj); err != nil {          // no retry on Conflict
            return fmt.Errorf("failed to remove finalizer from %s/%s: %w", ...)
        }
    }
    ...
}
```

`cleanupMirrorResources` (`engine_reconciler.go:190-207`) calls `deleteAllOfType` four times in sequence; any error in the first type short-circuits the remaining three. Combined with the non-retrying optimistic `Update` on line 223, a stale cache or concurrent-modification `Conflict` during shutdown leaves containers half-cleaned while volumes/images/namespaces remain. Self-heals on next startup via `fullSync` in `docker_watcher.go:309`, but between stop and start the stale resources are observable via `rdd ctl get` and violate the promise of the bats assertion at `bats/tests/32-app-controllers/engine.bats:160-168`. Wrap `Update` in `retry.RetryOnConflict` and collect errors across types instead of returning on the first.

**I2 — `removeMirrorResource` masks non-`NotFound` Update errors**
`pkg/controllers/app/engine/controllers/docker_watcher.go:225-236`

```go
if removeFinalizer(obj, mirrorFinalizer) {
    if err := w.k8s.Update(ctx, obj); err != nil {
        return client.IgnoreNotFound(err)     // swallows Conflict/Forbidden/etc.
    }
}
return client.IgnoreNotFound(w.k8s.Delete(ctx, obj))
```

`IgnoreNotFound` is the wrong filter here; the only expected benign error on `Update` is `NotFound`. A `Conflict` on the finalizer-strip update is silently treated as success, and the subsequent `Delete` runs against a stale snapshot. Either fail loudly (and let the caller retry) or specifically retry on Conflict. This is reachable from `handleContainerEvent`/`handleVolumeEvent` for every Docker `Destroy` event.

**I3 — `processFinalizers` + `reconcileContainerSpecs` run on every reconcile, O(N) each**
`pkg/controllers/app/engine/controllers/engine_reconciler.go:128-140, 266-341`

When `running=true` each reconcile unconditionally performs:
- 1 × `List(ContainerList)` in `reconcileContainerSpecs` (line 247)
- 1 × `List(ContainerList)` in `processContainerFinalizers` (line 285)
- 1 × `List(ImageList)` in `processImageFinalizers` (line 305)
- 1 × `List(VolumeList)` in `processVolumeFinalizers` (line 325)

Every status update from the Docker watcher enqueues a reconcile via `Watches(&containersv1alpha1.Container{}, enqueueApp)` at line 358. So each container status change triggers four List calls (plus a nested per-container `reconcileContainerState` pass that calls `ContainerStart`/`ContainerStop`). On a busy system with dozens of containers cycling, this is effectively O(N) work per status update and multiplies rapidly. Predicates (e.g., filter for `DeletionTimestamp != nil` updates for finalizers; filter for `spec.State` generation changes for state reconciliation) would cut this by an order of magnitude.

**I4 — Dead `scheme` field in `dockerWatcher`**
`pkg/controllers/app/engine/controllers/docker_watcher.go:30, 41, 63`

The `scheme *runtime.Scheme` field is plumbed through `newDockerWatcher`, stored on the struct, and never read. Verified: `grep -n 'w\.scheme'` returns no matches in the package. Remove the field and the parameter from `EngineReconciler.startWatcher` (`engine_reconciler.go:154`) and `controller.go:59`.

### SUGGESTIONS

**S1 — `reconcileContainerState` ignores intermediate states**
`pkg/controllers/app/engine/controllers/docker_watcher.go:254-268`

The `desired=created` branch only stops when `actual == ContainerStatusRunning`. If a user patches `spec.state=created` on a container in `paused`/`restarting` state, nothing happens — the reconciler waits for the container to reach `running` which it may never do. Docker's `ContainerStop` handles paused containers natively; drop the narrow `actual == running` guard.

**S2 — `err2` variable name**
`pkg/controllers/app/engine/controllers/sync_images.go:81-82`

The surrounding function already used `err` at line 76 in a scope that has ended; there's no shadowing concern. Rename to `err` and fold into a single return.

**S3 — `bats-with-timeout.sh` duplicates `instance.LogDir()` logic**
`scripts/bats-with-timeout.sh:22-28` vs `pkg/instance/instance.go:67-82`

The shell script hand-rolls the OS→log_dir mapping to avoid calling `rdd` (which may be hung). The comment at `bats-with-timeout.sh:20-21` already acknowledges the duplication. Consider a Go helper that prints just `LogDir()` without touching any runtime state, invoked as `rdd print log-dir` or similar — callable even when the control plane is stuck. Alternatively, add a test that asserts both implementations produce the same path.

**S4 — Goroutine watcher recovery depends on a non-obvious status-update re-trigger**
`pkg/controllers/app/engine/controllers/engine_reconciler.go:88-100, 345-362`

When the watcher errors, the reconciler cleans up and sets `ContainerEngineReady=Disconnected`, then returns `nil`. Recovery relies on the `Status().Update()` at line 186 triggering another reconcile through `For(&appv1alpha1.App{})`. This works in practice, but the single-buffer `reconcileChan` (line 345) means if a second event is dropped by the non-blocking send in `enqueueReconcile` (line 134-139), the only remaining trigger is the status update. Add a test for "Docker engine restarts while app is running" to lock this in.

**S5 — Stale-detection is O(N*M) per full sync**
`sync_containers.go:47-60`, `sync_images.go:56-69`, `sync_volumes.go:74-87`

Each full sync lists all K8s resources and for each checks membership in the `activeNames`/`dockerIDs` map — fine, already O(N) with the map. Good. But note the sync itself issues one `ContainerInspect`/`ImageInspect` per item in serial (`sync_containers.go:42`, `sync_images.go:46`). For hundreds of images, the initial sync serializes hundreds of HTTP calls — worth a parallelism bump (`errgroup`) if sync time matters.

**S6 — `dump_sockets` and `dump_macos_ps` output lines are capped at 30–50 via `head`, which will silently truncate in noisy environments**
`scripts/bats-with-timeout.sh:66-67, 86-87, 98-106`

Diagnostic bundles benefit from being complete; the cap seems designed to keep the bundle small but will hide the exact file descriptors or processes that caused the hang. Consider removing the `head` caps or making them configurable, since this is a post-mortem artifact, not a log stream.

**S7 — `applyContainer` Get-then-Apply pattern is correct but expensive**
`pkg/controllers/app/engine/controllers/sync_containers.go:86-97`

Fetching the container before applying is the right mechanism to avoid overwriting user-set `spec.state` — Server-Side Apply would merge based on field ownership, but the initial spec needs to be created exactly once with `spec.state=unknown` and never reasserted. The logic is correct. Consider adding a short comment explaining *why* the Get is needed (the apply-config intentionally omits `WithSpec` after creation), since the reason is not obvious from the code alone.

## Design Observations

- **Mirror pattern consistency**: `apiNamespace = "rancher-desktop"` and `containerNamespace = "moby"` hardcoded in `engine_reconciler.go:37-38` match the mock controller at `pkg/controllers/mock/mock_controller.go:27-29`. This is intentional and not a regression — the two mirror-style controllers share the same namespace convention, distinct from the app-controller pattern of `app.GetResourceNamespace()` (which returns `"default"` by design per `pkg/apis/app/v1alpha1/app_types.go:94-99`). Worth a comment or doc note so the next reader doesn't treat it as inconsistency.
- **Webhook interaction**: Verified the Container webhook only intercepts `Update` (`container_controller.go:88-90`) and ContainerNamespace only intercepts `Delete` (`container_namespace_controller.go:79-81`). The engine controller's `Apply` (Create semantics) and `Delete` paths therefore bypass the "create not implemented" error. Good catch on the design, but fragile — any future registration change would break the engine silently.
- **Spec normalization trick in webhook**: `container_webhooks.go:49-50` copies the old spec, overwrites `State` with the new value, then compares. Clean and correct — rejects any change where the diff is not purely `spec.state`.
- **`ContainerStatusUnknown` default**: Changing the default from `running` to `unknown` in `container_types.go:59-60` plus the enum addition is the key enabler: the reconciler ignores `unknown`, so Docker-originated containers don't get state-corrected by accident. Consistent with the doc at `api_containers.md:127-139`.
- **No Windows path**: `engine.bats:20` uses `skip_on_windows`, `DockerSocket()` in `instance.go:129-137` uses unix domain path, and the watcher hardcodes `"unix://" + socketPath` in `docker_watcher.go:44`. WSL2 support is deliberately deferred.

## Testing Assessment

**Covered:** ContainerNamespace moby creation, image mirroring via docker pull, container lifecycle (create / running / stop / exited / rm), volume lifecycle, K8s→Docker deletion, spec.state transitions in both directions, full cleanup on `running=false`, restart restoring `ContainerEngineReady` and `moby` namespace.

**Not covered:** No Go unit tests for the engine controller package; no test for `watcherDied` recovery (engine restart mid-session); no test for concurrent user patch vs. controller status update (Server-Side Apply field-ownership guarantee); no test for finalizer cleanup error branches (what happens when mid-cleanup a Conflict occurs); `parseContainerName` has no direct test (slash edge cases); no test for `reconcileContainerState` against intermediate states (paused / restarting). Unit-test gap is the biggest testing concern — BATS covers the happy path end-to-end but cannot reach the failure modes cheaply.

## Commit Structure

The 9 commits are well-sequenced (Rosetta fix, distro proxy + socket path, Docker client dep, engine controller, webhook relaxation, docs, BATS tests, `rdd set` wait, CI timeout wrapper). The only quibble is commit `32a0109` which bundles two distinct fixes (distro proxy workaround + socket path change) — consider splitting in future PRs.

## Acknowledged Limitations

Explicitly called out in the PR with TODO comments: `.rd2` hardcoding in `pkg/instance/instance.go:126-128` and `engine.bats:12-13` pending Lima template change; `rdd set` wait for non-`running` property changes (`cmd/rdd/set.go:436-439`) pending a `Reconciled` condition with `observedGeneration`; Windows/WSL2 Docker socket deferred; distro proxy bug workaround is temporary pending the upstream `rancher-desktop-opensuse` fix.

---

## Resolution

| | |
|---|---|
| **Addressed** | 2026-04-12 |
| **Commit** | 5 fixup commits autosquashed into their target PR commits (`93359f7`, `4db3f20`, `762c1e8`, `65943da`, `7c8f356`) |

| # | Finding | Action |
|---|---------|--------|
| 1 | Critical #1: `sanitizeKubernetesObjectName` leaves volume names that fail RFC 1123 validation | Fixed (volumeK8sName hash + Volume selectable field + design doc + test) |
| 2 | Important #1: Engine controller unconditionally targets Docker, containerd backends never satisfy ContainerEngineReady | Fixed (engineIsDocker gate, True/NotApplicable) |
| 3 | Important #2: ContainerNamespace mirror finalizer sticks in Terminating | Fixed (finalizer dropped from syncContainerNamespace) |
| 4 | Important #3: Finalizer removed even when Docker-side delete fails | Fixed (delete helpers return errors, processFinalizers checks + errors.Join) |
| 5 | Important #4: `rdd set running=false` wait returns on engine disconnect, not VM stop | Fixed (wait on App Running condition) |
| 6 | Important #5: Cleanup fragility (no Conflict retry, first-error short-circuit, edge-triggered outer guard) | Fixed (retry.RetryOnConflict, errors.Join, level-triggered !wantWatcher branch) |
| 7 | Important #6: N+1 Inspect calls during fullSync | Commented (rationale for sequential Inspect documented in syncAllContainers / syncAllImages) |
| 8 | Important #7: O(N) per-reconcile List sweeps | Commented (cost and future split documented in Reconcile) |
| 9 | Important #8: removeMirrorResource IgnoreNotFound masks Conflict/Forbidden | Fixed (retry.RetryOnConflict + Get gate in closure) |
| 10 | Suggestion #1: Design doc ContainerCreateRequest default | Fixed (CRD reverted to default=running/enum=created,running; doc was already correct) |
| 11 | Suggestion #2: applyContainer misses restoring finalizers on existing resources | Skipped |
| 12 | Suggestion #3: `rdd set` watch loop aborts on watch-channel closure | Fixed (watchtools.UntilWithSync) |
| 13 | Suggestion #4: reconcileContainerState ignores intermediate states | Fixed (drop narrow `actual == running` guard) |
| 14 | Suggestion #5: bats-with-timeout.sh duplicates instance.LogDir() | Fixed (use `rdd svc paths log_dir`) |
| 15 | Suggestion #6: Support-bundle head caps truncate diagnostics | Fixed (caps removed) |
| 16 | Suggestion #7: Comment Get-then-Apply intent | Fixed (comment added explaining why server-side Apply alone is not enough) |
| 17 | Suggestion #8: Dead scheme field in dockerWatcher | Fixed (field + parameter + EngineReconciler.Scheme removed) |
| 18 | Testing Gap #1: Volume/image names outside `[a-z0-9.-]` | Test written (BATS: `My_Vol_Ume` create/delete via field selector) |
| 19 | Testing Gap #2: containerd backend startup | Test written (BATS: containerd reports NotApplicable, no mirror resources) |
| 20 | Testing Gap #3: Docker-side delete failures | Test written (BATS: in-use image delete keeps finalizer until container removed) |
| 21 | Testing Gap #4: ContainerNamespace user delete | Test written (BATS: delete containernamespace/moby completes) |
| 22 | Testing Gap #5: running=false wait with degraded engine | Test written (BATS: `rdd set running=false` from already stopped) |
| 23 | Testing Gap #6: Cleanup failure retry | Skipped (needs fault injection) |
| 24 | Testing Gap #7: Watcher-died mid-session recovery | Skipped (needs in-VM access) |
| 25 | Testing Gap #8: Concurrent user patch vs controller status update | Skipped (race test; architectural) |
| 26 | Testing Gap #9: parseContainerName edge cases | Test written (Go unit: sync_containers_test.go, 6 cases) |
| 27 | Testing Gap #10: reconcileContainerState against intermediate states | Test written (BATS: pause then spec.state=created) |
| 28 | Testing Gap #11: Non-default RDD_INSTANCE | Skipped (acknowledged limitation) |
| 29 | Documentation Gap #1: ContainerCreateRequest default | Skipped (already correct after S1 revert) |
| 30 | Documentation Gap #2: Containerd overstatement | Skipped |