# Deep Review: 20260512-133636-pr-350

| | |
|---|---|
| **Date** | 2026-05-12 13:36 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 1 (of target) |
| **Author** | [@Nino-K](https://github.com/Nino-K) |
| **PR** | [#350](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/350) — Wire up docker socket windows |
| **Commits** | `0322994` Fixes test 104 stopping VM removes all mirror resources<br>`e917657` Wire Docker socket forwarding for Windows<br>`b5ab872` Bump openSUSE to v0.2.3 |
| **Reviewers** | Claude Opus 4.7 (effort: xhigh), Codex GPT 5.5 (effort: xhigh), Gemini 3.1 Pro (effort: default) |
| **Verdict** | **Merge with fixes** — bound `waitMirrorResourcesGone` to prevent reconciler wedge; register engine controller in standalone `containers-controller` binary. |
| **Wall-clock time** | `16 min 51 s` |

## Executive Summary

The PR wires Windows Docker socket forwarding by introducing `instance.DockerEndpoint()` (`npipe:////./pipe/docker_engine` on Windows, `unix://…` on Unix), removes the Windows early-return guards in the engine controller and Docker-context management, accepts the `npipe` scheme in `probeNamedDockerContext`, and re-enables `engine-docker.bats` on Windows with `cygpath`/`is_windows` shims. The third commit fixes test 104 by tightening `computeSettledCondition` to require a terminal engine reason (`Stopped`/`NotApplicable`) at the current generation before declaring `Settled=True`, switching `cleanupMirrorResources` reads to `apiReader`, and polling until the API server reports the mirrors gone. The condition logic is correctly scoped and well-tested. The remaining concerns are: (1) the new `waitMirrorResourcesGone` polls an unbounded loop using a reader that bypasses the very cache it claims to wait for, and (2) the standalone `containers-controller` binary still does not blank-import the engine package — a pre-existing gap now made visible by Windows support.


---

## Critical Issues

None.

## Important Issues

I1. **`waitMirrorResourcesGone` is unbounded and reads etcd, not the watch cache** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:507-549` [Claude Opus 4.7] [Gemini 3.1 Pro]

```go
func (r *EngineReconciler) waitMirrorResourcesGone(ctx context.Context) error {
    log := logf.FromContext(ctx)
    for {
        remaining, err := r.countMirrorResources(ctx)
        if err != nil {
            return fmt.Errorf("error verifying mirror resources gone: %w", err)
        }
        if remaining == 0 {
            return nil
        }
        ...
        select {
        case <-ctx.Done():
            return fmt.Errorf("timed out waiting for %d mirror resources to be deleted: %w",
                remaining, ctx.Err())
        case <-time.After(200 * time.Millisecond):
        }
    }
}

func (r *EngineReconciler) countMirrorResources(ctx context.Context) (int, error) {
    ...
    for _, list := range lists {
        if err := r.apiReader.List(ctx, list, client.InNamespace(r.apiNamespace)); err != nil {
            return 0, err
        }
        ...
    }
}
```

Two problems compound. First, `pkg/service/controllers/manager.go:143-153` sets no `ReconciliationTimeout`, so the `ctx` passed in is the manager's long-lived stop context. The loop exits only when every mirror disappears or the manager shuts down. A `Container`/`Image`/`Volume` created between `stopWatcher()` and the snapshot list in `deleteAllOfType`, a user-added finalizer that is not `mirrorFinalizer` (only that one is stripped at `engine_reconciler.go:593`), or a storage hiccup leaves an object visible forever. Because `App` is a cluster-scoped singleton with at most one in-flight `Reconcile`, a stuck loop blocks every subsequent state transition — `rdd set running=true` afterwards never runs.

Second, the loop's stated purpose (the function comment) is to "Block until the API server's watch cache reflects the deletions". `apiReader` is the manager's `GetAPIReader()` — it bypasses every cache and issues a quorum etcd read (no `resourceVersion=0`). DELETE commits to etcd synchronously, so `apiReader.List` returns `0` immediately. The user-facing `rdd ctl get` reads with `resourceVersion=0` from the API server's watch cache, which lags etcd by the goroutine that processes the watch event. The poll exits before the cache it claims to drain is drained; the test passes today because the other fixes in this commit (the `ObservedGeneration` guard on `alreadyClean`, plus reads via `apiReader` in `deleteAllOfType`) close the actual race.

Fix: bound the wait with `context.WithTimeout`, and switch `countMirrorResources` to the controller-runtime cache (`r.List`) so the loop drains a cache downstream of the API server's watch cache.

```diff
 func (r *EngineReconciler) waitMirrorResourcesGone(ctx context.Context) error {
     log := logf.FromContext(ctx)
+    ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+    defer cancel()
     for {
```

```diff
 func (r *EngineReconciler) countMirrorResources(ctx context.Context) (int, error) {
     ...
     for _, list := range lists {
-        if err := r.apiReader.List(ctx, list, client.InNamespace(r.apiNamespace)); err != nil {
+        if err := r.List(ctx, list, client.InNamespace(r.apiNamespace)); err != nil {
             return 0, err
         }
```

Note: `r.List` reads the controller's informer cache, which is populated only for types the controller `Watches`. `Container`, `Image`, and `Volume` are watched; `ContainerNamespace` is not (see I2 below), so this fix should land alongside watching `ContainerNamespace` or accept the small risk that one `ContainerNamespace` slips by on the rare delete-during-stop window.

I2. **Standalone `containers-controller` binary does not register the engine controller** — `cmd/containers-controller/main.go:11-13`, `pkg/controllers/containers/controller.go:10-16` [Codex GPT 5.5]

```go
// cmd/containers-controller/main.go
import (
    "os"

    // Import app controller packages to trigger init() functions.
    _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers"
    "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/external"
)
```

```go
// pkg/controllers/containers/controller.go
import (
    // Import controllers to register them.
    _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/container"
    _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/containernamespace"
    _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/image"
    _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/volume"
)
```

The engine controller registers itself in `pkg/controllers/app/engine/controller.go:25-26` via `init()` and declares the `containers` API group (`pkg/controllers/app/engine/controller.go:59-60`). The embedded service imports it at `pkg/service/cmd/service.go:59`, but the standalone `containers-controller` aggregator imports only the four container-CRD packages. `--controllers` help text at `pkg/service/controllers/options.go:18` advertises `containers,-engine`, implying engine is part of the containers group, yet `bin/containers-controller` silently omits it. The repo context flags exactly this shape: "a missing import in either is a silent drop with no startup error."

The gap pre-dates this PR (engine has always lived under `pkg/controllers/app/engine` while declaring group `containers`), but Windows support makes engine reachable on every platform now and amplifies the consequence: a user who runs `bin/containers-controller` standalone on Windows gets Container/Image/Volume CRDs but no mirroring.

Fix: blank-import the engine package from the containers aggregator so every consumer of `controllers/containers` gets the full group.

```diff
 import (
     // Import controllers to register them.
+    _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/app/engine"
     _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/container"
     _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/containernamespace"
     _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/image"
     _ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/volume"
 )
```

## Suggestions

S1. **Stale "tcp/unix" wording in `probeNamedDockerContext` after adding `npipe`** — `pkg/controllers/app/engine/controllers/docker_context.go:155-186` [Claude Opus 4.7] [Codex GPT 5.5]

```go
// When we cannot determine whether the context is healthy — because the store
// is unreadable, the endpoint metadata is missing or malformed, the scheme is
// not tcp/unix (e.g. SSH), or the TLS config cannot be loaded — we return true
// (assume healthy). ...
    switch scheme {
    case "unix", "tcp", "npipe":
    default:
        log.Info("Non-tcp/unix endpoint scheme; assuming context healthy", "scheme", scheme)
```

The PR adds `"npipe"` to the supported schemes at line 183 but leaves both the function docstring at line 157 and the structured-log message at line 185 saying "tcp/unix". Future readers will see the disagreement; the log message also misrepresents the actual decision when an `ssh://` context appears.

Fix:

```diff
-// not tcp/unix (e.g. SSH), or the TLS config cannot be loaded — we return true
+// not tcp/unix/npipe (e.g. SSH), or the TLS config cannot be loaded — we return true
 ...
-        log.Info("Non-tcp/unix endpoint scheme; assuming context healthy", "scheme", scheme)
+        log.Info("Unprobed endpoint scheme; assuming context healthy", "scheme", scheme)
```

S2. **BATS test uses `$(cygpath -m …)` command substitution where repo style prefers `run -0`** — `bats/tests/32-app-controllers/engine-docker.bats:22-26` [Claude Opus 4.7]

```bash
if is_windows; then
    DOCKER_CONFIG="$(cygpath -m "${BATS_FILE_TMPDIR}/docker-config")"
else
    DOCKER_CONFIG="${BATS_FILE_TMPDIR}/docker-config"
fi
```

`$(cygpath -m …)` swallows a non-zero exit and silently produces an empty string; `mkdir -p ""` then fails with a less actionable error than `cygpath` itself would. Repo BATS style prefers `run -0 cmd …; value=${output}` so command, status, and output all surface on failure.

Fix:

```diff
 if is_windows; then
-    DOCKER_CONFIG="$(cygpath -m "${BATS_FILE_TMPDIR}/docker-config")"
+    run -0 cygpath -m "${BATS_FILE_TMPDIR}/docker-config"
+    DOCKER_CONFIG=${output}
 else
     DOCKER_CONFIG="${BATS_FILE_TMPDIR}/docker-config"
 fi
```

S3. **Engine reason strings duplicated across the app and engine packages without shared constants** — `pkg/controllers/app/app/controllers/app_controller.go:452-454`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:215-228, 266` [Claude Opus 4.7]

```go
// app_controller.go
engineSettled := engineCond != nil &&
    engineCond.ObservedGeneration >= app.Generation &&
    (engineCond.Reason == "Stopped" || engineCond.Reason == "NotApplicable")
```

```go
// engine_reconciler.go
terminalReason := "Stopped"
...
if running && !engineIsDocker {
    terminalReason = "NotApplicable"
...
if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
```

`computeSettledCondition` now hard-depends on the engine reconciler's choice of reason strings. A typo or rename on one side silently breaks the wait. `pkg/apis/app/v1alpha1/app_types.go` already exports `AppConditionContainerEngineReady` as a constant; the reason values deserve the same treatment.

Fix: declare `EngineReasonStopped`, `EngineReasonNotApplicable`, `EngineReasonConnected`, `EngineReasonConnectFailed` in `pkg/apis/app/v1alpha1/app_types.go` and reference them from both packages.

S4. **`instance.DockerSocket()` docstring claims "for this instance" but the Windows return is process-global** — `pkg/instance/instance.go:123-132` [Claude Opus 4.7] [Gemini 3.1 Pro]

```go
// DockerSocket returns the path to the Docker socket for this instance
// (e.g., ~/.rd2/docker.sock). This is the host-side socket that Lima
// port-forwards from the guest's /var/run/docker.sock.
// On Windows, returns the named pipe path (\\.\pipe\docker_engine).
var DockerSocket = sync.OnceValue(func() string {
    if runtime.GOOS == "windows" {
        return `\\.\pipe\docker_engine`
    }
    return filepath.Join(ShortDir(), "docker.sock")
})
```

On Unix the path embeds `ShortDir()` (which factors in `RDD_INSTANCE`), so two instances coexist. On Windows the pipe is the well-known Docker name and is also hardcoded in `pkg/socketbridge/host_windows.go:23`, so two RDD instances will collide if both run, and an installed Docker Desktop already owns the same pipe. The well-known name is a deliberate choice (Docker CLI defaults work without extra setup), but the docstring should not promise per-instance isolation it does not deliver.

Fix at minimum: update the docstring to reflect the Windows leg's one-instance constraint. A per-instance pipe (e.g., `\\.\pipe\docker_engine_<suffix>`) is out of scope here because it would break the Docker CLI's default endpoint.

```diff
-// DockerSocket returns the path to the Docker socket for this instance
-// (e.g., ~/.rd2/docker.sock). This is the host-side socket that Lima
-// port-forwards from the guest's /var/run/docker.sock.
-// On Windows, returns the named pipe path (\\.\pipe\docker_engine).
+// DockerSocket returns the path to the Docker socket. On Unix this is the
+// per-instance host-side socket that Lima port-forwards from the guest's
+// /var/run/docker.sock (e.g., ~/.rd2/docker.sock). On Windows it is the
+// well-known named pipe \\.\pipe\docker_engine — process-global, so at
+// most one RDD instance can serve Docker on Windows at a time.
```

S5. **`run -0 jq` at line 790 leaves output unused on the non-Windows branch, and `local` appears inside an `@test` block** — `bats/tests/32-app-controllers/engine-docker.bats:790-797` [Codex GPT 5.5] [Gemini 3.1 Pro]

```bash
run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
if is_windows; then
    assert_output "npipe:////./pipe/docker_engine"
else
    run -0 rdd service paths docker_socket
    local socket_path=${output}
    run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
    assert_output "unix://${socket_path}"
fi
```

The `run -0` at line 790 is consumed only by the Windows branch's `assert_output`; on the non-Windows branch its `${output}` is discarded and the same `jq` is re-run at line 796. `local socket_path=…` at line 795 sits inside an `@test` body, which the repo's BATS style flags (each `@test` runs in its own subshell already).

Fix:

```diff
-    run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
     if is_windows; then
+        run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
         assert_output "npipe:////./pipe/docker_engine"
     else
         run -0 rdd service paths docker_socket
-        local socket_path=${output}
+        socket_path=${output}
         run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
         assert_output "unix://${socket_path}"
     fi
```

## Design Observations

**Concerns**

- **`ContainerNamespace/moby` is created only during `fullSync` and never watched or re-applied (in-scope)** [Codex GPT 5.5] — `pkg/controllers/app/engine/controllers/docker_watcher.go:615`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:829-831`. If a user deletes `ContainerNamespace/moby` while the engine watcher is alive, no watch event reaches the reconciler (the steady-state watch list omits the type) and no Docker event recreates it, so the mirror namespace stays missing until the next watcher restart. The repo context names this exact anti-pattern. The "deleting ContainerNamespace/moby completes without a finalizer hang" test today verifies the namespace stays gone after a user delete, so the behaviour is at least partly intentional — but the reconciler-side gap deserves an explicit comment ("startup-only, not re-applied on steady-state") or a `Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp)` plus an `ensureContainerNamespace` step.

**Strengths**

- **`computeSettledCondition` precisely closes the Connected-at-current-generation hole (in-scope)** [Claude Opus 4.7] [Codex GPT 5.5] — `pkg/controllers/app/app/controllers/app_controller.go:434-463`. The engine reconciler can stamp `Connected` at `M+1` between the spec patch and the LimaVM running flip; before this PR the `!desiredRunning` branch returned `Settled=True` unconditionally and let `rdd set running=false` exit before mirror cleanup. Requiring a terminal reason at the current generation closes that window without overreach, and the new unit-test cases (`engine still Connected at current generation waits`, `engine ready at older generation is stale`, `NotApplicable settles`) cover each branch.
- **Switching cleanup reads to `apiReader` (in-scope)** [Claude Opus 4.7] — `pkg/controllers/app/engine/controllers/engine_reconciler.go:539, 565, 587`. The controller-runtime informer cache lags writes by the watcher goroutine; using the direct API reader in `deleteAllOfType` and `countMirrorResources` makes the cleanup see resources the watcher applied seconds before `stopWatcher()` killed it.
- **Centralizing the Docker client address in `instance.DockerEndpoint()` (in-scope)** [Codex GPT 5.5] — `pkg/instance/instance.go:134-142`. Each call site (`engine_reconciler.go:315`, `docker_watcher.go:55`) used to stitch `"unix://" + DockerSocket()`; the helper hides the Windows `npipe` switch behind a single URL.

## Testing Assessment

`Test_computeSettledCondition` is comprehensive for the new branches: every transition added in the third commit has a case. Two gaps remain. First, no unit test exercises the `npipe` scheme branch in `probeNamedDockerContext`; that path is reachable only via Windows BATS today. Seeding a `seedContext(t, "npipe-ctx", "npipe:////./pipe/some-pipe")` case in `Test_probeNamedDockerContext` would lock it down on non-Windows CI. Second, no test covers I1's wedge scenario — a unit test that injects a fake `apiReader` returning a non-empty list and verifies `waitMirrorResourcesGone` honours a context deadline would make the bound regression-safe. Third, neither `cygpath` BATS shim nor the `npipe` `assert_output` is exercised on non-Windows CI; if the Windows runner is intermittent, the Windows-only branches drift.

## Documentation Assessment

The new `instance.DockerEndpoint()` is documented well. The pre-existing `DockerSocket` doc is now misleading on Windows (S4) and the `probeNamedDockerContext` doc and log message drift from the code (S1). The `TODO(windows)` blocks removed from `engine_reconciler.go` (manageDockerContext, removeDockerContext) and `engine/controller.go` (init) were correctly retired — their preconditions (npipe-aware probe client, npipe scheme in the context meta, engine controller registration on Windows) are all satisfied in this PR.

## Commit Structure

Three commits map to natural boundaries: openSUSE bump (`b5ab872`), Windows wiring (`e917657`), drain race fix on slow Windows CI (`0322994`). The third is logically a follow-up to the second since it only became reachable after engine-controller registration on Windows. Bundling them in one PR is reasonable.

## Acknowledged Limitations

- `engine_reconciler.go:206-213` already documents the watcher-died vs. never-started branch and the deliberate dual-source approach (`stopWatcher` vs. direct `removeDockerContext`). Not an issue.
- `engine_reconciler.go:259-266` documents that the steady-state `setEngineCondition("Connected", …)` is the only place that re-stamps the current generation when the spec is patched without cycling the watcher. The new generation guard at line 238 leans on exactly that lifecycle.
- `pkg/controllers/lima/limavm/controllers/hostswitch_windows.go:151-155` documents fire-and-forget host-switch lifecycle: failures are logged but not surfaced to the reconciler. Pre-existing and not made worse by this PR.

---

## Agent Performance Retro

### [Claude]

Strongest pass on the wedge analysis (I1): traced the manager's missing `ReconciliationTimeout` through `pkg/service/controllers/manager.go` and enumerated three realistic ways drain can fail. Also produced the cleanest framing for the multi-instance pipe question — calibrating it as a doc fix (S4) rather than a critical, because the well-known pipe name is intentional. Useful suggestions on shared reason constants (S3) and the stale `npipe`/log wording (S1).

### [Codex]

Found two pre-existing gaps that the other agents missed: the `containers-controller` standalone binary missing the engine import (I2) and the `ContainerNamespace/moby` startup-only application. Both required tracing outside the diff into the import graph and the watch list, which matches Codex's pattern of going deeper into wiring. Suggestion S5 caught the redundant `run -0 jq` plus the `local` BATS-style miss that the other agents partially flagged.

### [Gemini]

Made the cache-bypass observation that gave I1 its second leg — flagged `apiReader.List` reading etcd rather than the watch cache, which the other two agents missed. Inflated the well-known pipe name to Critical (multi-instance and Docker Desktop collision) without weighing the deliberate well-known-name design choice; downgraded to S4 in consolidation. As usual on this repo, did not run `git blame` (daily quota), so could not distinguish PR-introduced from pre-existing concerns. Misread `docker_watcher.go:91` as the primary path (it's the fallback after `dockerEventsSince` fails); the design observation was dropped from consolidation.

### Summary

| | Claude Opus 4.7 | Codex GPT 5.5 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 7m 19s | 9m 51s | — |
| Findings | 1I 4S | 2I 1S | 1I 1S |
| Tool calls | 38 (Read 15, Grep 14, Bash 9) | 89 (shell 88, stdin 1) | — |
| Design observations | 4 | 2 | 2 |
| False positives | 0 | 0 | 0 |
| Unique insights | 3 | 2 | 1 |
| Files reviewed | 11 | 11 | 11 |
| Coverage misses | 0 | 0 | 0 |
| **Totals** | **1I 4S** | **2I 1S** | **1I 1S** |
| Downgraded | 0 | 1 (I→S) | 1 (I→S) |
| Dropped | 0 | 0 | 0 |


Reconciliation:

- Gemini C1 "Global named pipe on Windows breaks multi-instance isolation": critical → suggestion S4. The well-known pipe is a deliberate design choice for Docker CLI compatibility; this is a documentation gap (the docstring still says "for this instance"), not a critical bug.
- Codex I2 "ContainerNamespace not watched or re-applied": important → design observation. The "deleting ContainerNamespace/moby completes without a finalizer hang" BATS test deliberately verifies the namespace stays gone after a user delete, so the behavior is partly intentional; surfaced as a design observation with a recommendation to add an explanatory comment or a `Watches` entry.

Codex contributed the highest pre-existing-gap signal (I1 wiring + the `ContainerNamespace` observation); Claude provided the most defensible severity calibration; Gemini contributed one decisive insight (the cache layer) and one false-positive design observation. The most valuable agent on this review depends on what you want to optimise for — coverage breadth (Codex), severity calibration (Claude), or independent-cache analysis (Gemini).

---

## Review Process Notes

### Repo context updates

- The Suggestions section already contains a rule about reason-string constants for `App.Status.Conditions` writers (S3-style); consider adding a standing rule to `deep-review-context.md`: **When multiple controllers write to the same status condition and a downstream consumer compares the `Reason` field by string literal, the reason values must live as exported constants in the API-types package. Flag string-literal reason comparisons across package boundaries.**
- Add a standing rule for `waitForGone`/polling-loop patterns: **A polling loop that waits for a resource to disappear "from the cache" must (a) carry an explicit `context.WithTimeout` bound and (b) read from a cache downstream of the one it claims to drain. Reading via `mgr.GetAPIReader()` queries etcd directly and bypasses the API server's watch cache; pair that reader with the informer-cache `r.List` for the wait, or document why the etcd read is sufficient.**

---

## Appendix: Original Reviews

### [Claude] — Pass 1

    Now I have enough context to write the review.
    
    # Code Review — PR #350: Wire up docker socket windows
    
    **Verdict:** Approve with one important fix.
    
    ## Executive Summary
    
    This PR wires Docker socket forwarding for Windows by replacing `DockerSocket()` paths with a new `DockerEndpoint()` URL helper across the engine controller chain, removes the `runtime.GOOS == "windows"` guards in the engine controller and Docker-context manager, re-enables the engine-docker BATS suite on Windows, and tightens `computeSettledCondition` plus `cleanupMirrorResources` to make `rdd set running=false` actually wait for mirror-resource cleanup to drain before returning. The core logic changes are correct, but the new `waitMirrorResourcesGone` poll has no upper bound and can stall the singleton App reconciler indefinitely if any condition prevents drain.
    
    ## Findings
    
    ### Critical Issues
    
    None.
    
    ### Important Issues
    
    **I1. `waitMirrorResourcesGone` can wedge the reconciler indefinitely** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:507-526`
    
    ```go
    func (r *EngineReconciler) waitMirrorResourcesGone(ctx context.Context) error {
    	log := logf.FromContext(ctx)
    	for {
    		remaining, err := r.countMirrorResources(ctx)
    		if err != nil {
    			return fmt.Errorf("error verifying mirror resources gone: %w", err)
    		}
    		if remaining == 0 {
    			return nil
    		}
    		...
    		select {
    		case <-ctx.Done():
    			return fmt.Errorf("timed out waiting for %d mirror resources to be deleted: %w",
    				remaining, ctx.Err())
    		case <-time.After(200 * time.Millisecond):
    		}
    	}
    }
    ```
    
    The `ctx` passed in is the reconcile context, which in this codebase is the manager's long-lived stop context — `pkg/service/controllers/manager.go:143-153` sets no `ReconciliationTimeout`. So the loop only exits when either (a) every mirror resource disappears or (b) the manager shuts down.
    
    Real ways drain can fail to complete:
    
    1. A user creates a `Container` / `Image` / `Volume` between `stopWatcher()` (line 208) and `cleanupMirrorResources` (line 240). `deleteAllOfType` already snapshot-listed before the new mirror appeared, so the new mirror is never deleted; `countMirrorResources` keeps seeing it.
    2. A user-added finalizer that isn't `mirrorFinalizer` blocks Delete. `deleteAllOfType` only strips `mirrorFinalizer` (line 593), so a foreign finalizer leaves the object Terminating forever.
    3. An apiserver storage hiccup keeps an object in Terminating between Delete and tombstone GC.
    
    Because the App reconciler is a cluster-scoped singleton, a stuck `Reconcile` blocks every subsequent state transition — `rdd set running=true` afterwards never gets re-reconciled. This is a regression of this PR: the prior code returned immediately after `errors.Join`.
    
    Fix: bound the poll and let the next reconcile retry. Either re-list-and-redelete inside the loop, or apply a deadline:
    
    ```diff
     func (r *EngineReconciler) waitMirrorResourcesGone(ctx context.Context) error {
     	log := logf.FromContext(ctx)
    +	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
    +	defer cancel()
     	for {
    ```
    
    (important, regression)
    
    ### Suggestions
    
    **S1. Stale comment and log message in `probeNamedDockerContext` after adding `npipe`** — `pkg/controllers/app/engine/controllers/docker_context.go:155-186`
    
    ```go
    // When we cannot determine whether the context is healthy — because the store
    // is unreadable, the endpoint metadata is missing or malformed, the scheme is
    // not tcp/unix (e.g. SSH), or the TLS config cannot be loaded — we return true
    // (assume healthy). ...
    ...
    	switch scheme {
    	case "unix", "tcp", "npipe":
    	default:
    		log.Info("Non-tcp/unix endpoint scheme; assuming context healthy", "scheme", scheme)
    ```
    
    The PR added `"npipe"` to the scheme switch at line 183, but the function docstring (line 157) still says "the scheme is not tcp/unix (e.g. SSH)" and the log message at line 185 still says "Non-tcp/unix endpoint scheme". Both are now misleading.
    
    Fix:
    
    ```diff
    -// not tcp/unix (e.g. SSH), or the TLS config cannot be loaded — we return true
    +// not tcp/unix/npipe (e.g. SSH), or the TLS config cannot be loaded — we return true
     ...
    -		log.Info("Non-tcp/unix endpoint scheme; assuming context healthy", "scheme", scheme)
    +		log.Info("Unprobed endpoint scheme; assuming context healthy", "scheme", scheme)
    ```
    
    (suggestion, regression)
    
    **S2. BATS uses command substitution where the project convention is `run -0 ; value=${output}`** — `bats/tests/32-app-controllers/engine-docker.bats:22-26`
    
    ```bash
    if is_windows; then
        DOCKER_CONFIG="$(cygpath -m "${BATS_FILE_TMPDIR}/docker-config")"
    else
        DOCKER_CONFIG="${BATS_FILE_TMPDIR}/docker-config"
    fi
    ```
    
    Repo BATS style (see [BATS style rules](file:///Users/jan/.claude/projects/-Users-jan-git-rancher-desktop-daemon/memory/bats-style.md)) prefers `run -0 cmd ...; value=${output}` over `value=$(cmd ...)`. Command substitution swallows a non-zero exit and silently produces an empty string, which would then fail downstream with a less-actionable error than `cygpath` exiting non-zero on its own.
    
    Fix:
    
    ```diff
     if is_windows; then
    -    DOCKER_CONFIG="$(cygpath -m "${BATS_FILE_TMPDIR}/docker-config")"
    +    run -0 cygpath -m "${BATS_FILE_TMPDIR}/docker-config"
    +    DOCKER_CONFIG=${output}
     else
         DOCKER_CONFIG="${BATS_FILE_TMPDIR}/docker-config"
     fi
    ```
    
    (suggestion, regression)
    
    **S3. Magic strings `"Stopped"` / `"NotApplicable"` / `"Connected"` duplicated across app and engine packages** — `pkg/controllers/app/app/controllers/app_controller.go:452-454`, `pkg/controllers/app/engine/controllers/engine_reconciler.go:215-228, 266`
    
    ```go
    // app_controller.go
    engineSettled := engineCond != nil &&
        engineCond.ObservedGeneration >= app.Generation &&
        (engineCond.Reason == "Stopped" || engineCond.Reason == "NotApplicable")
    ```
    
    ```go
    // engine_reconciler.go
    terminalReason := "Stopped"
    ...
    if running && !engineIsDocker {
        terminalReason = "NotApplicable"
    ...
    if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", ...
    ```
    
    `computeSettledCondition` now hard-depends on the engine reconciler's choice of reason strings. A typo or rename on one side silently breaks the wait. The `app_types.go` file already exports `AppConditionContainerEngineReady` as a constant; the reason strings deserve the same treatment so they can be cross-imported.
    
    Fix: declare these as constants in `pkg/apis/app/v1alpha1/app_types.go` (e.g., `EngineReasonStopped`, `EngineReasonNotApplicable`, `EngineReasonConnected`, `EngineReasonConnectFailed`) and reference them from both packages.
    
    (suggestion, regression)
    
    **S4. `instance.DockerSocket()` docstring says "for this instance" but the Windows return is a process-global named pipe** — `pkg/instance/instance.go:123-132`
    
    ```go
    // DockerSocket returns the path to the Docker socket for this instance
    // (e.g., ~/.rd2/docker.sock). This is the host-side socket that Lima
    // port-forwards from the guest's /var/run/docker.sock.
    // On Windows, returns the named pipe path (\\.\pipe\docker_engine).
    var DockerSocket = sync.OnceValue(func() string {
    	if runtime.GOOS == "windows" {
    		return `\\.\pipe\docker_engine`
    	}
    	return filepath.Join(ShortDir(), "docker.sock")
    })
    ```
    
    On Unix the path embeds `ShortDir()` which incorporates `RDD_INSTANCE`, so two instances coexist. On Windows the pipe name is constant — two RDD instances will collide when both try to listen on `\\.\pipe\docker_engine` (the daemon's `socketbridge.HostBridge` from `pkg/socketbridge/host_windows.go:23` also hardcodes the same name). The docstring claim "for this instance" doesn't reflect that, and an operator running with `RDD_INSTANCE=foo` would reasonably expect an isolated endpoint.
    
    Fix at minimum: drop "for this instance" from the Windows leg of the doc and explicitly note the one-instance constraint. Long-term, derive a per-instance pipe name (e.g., `\\.\pipe\docker_engine_<suffix>`), but that touches the Docker CLI default which expects the well-known name — out of scope here. A short doc clarification is enough:
    
    ```diff
    -// DockerSocket returns the path to the Docker socket for this instance
    -// (e.g., ~/.rd2/docker.sock). This is the host-side socket that Lima
    -// port-forwards from the guest's /var/run/docker.sock.
    -// On Windows, returns the named pipe path (\\.\pipe\docker_engine).
    +// DockerSocket returns the path to the Docker socket. On Unix this is the
    +// per-instance host-side socket that Lima port-forwards from the guest's
    +// /var/run/docker.sock (e.g., ~/.rd2/docker.sock). On Windows it is the
    +// well-known named pipe \\.\pipe\docker_engine — process-global, so only
    +// one RDD instance can serve Docker on Windows at a time.
    ```
    
    (suggestion, regression)
    
    ## Design Observations
    
    ### Strengths
    
    - The `computeSettledCondition` fix nails the exact race the comment names: an engine reconciler can stamp `Connected` at the new generation between the spec patch and the LimaVM status flip, and the prior code (always-True when `!desiredRunning`) would have let `rdd set running=false` exit before mirror cleanup. The new check on terminal reason + generation closes that window precisely.
    - Switching cleanup to `mgr.GetAPIReader()` for `List` and `Get` (`engine_reconciler.go:539,565,587`) is the right move — the controller-runtime cache lags writes by the watcher goroutine, and consulting the direct reader during teardown avoids missing a just-created mirror that the watcher applied seconds before it was killed.
    - Test coverage for `computeSettledCondition` is good: the new cases distinguish "engine condition absent", "engine condition stale", "engine still Connected at current generation", and "engine Stopped/NotApplicable at current generation". The Connected-at-current-generation case is the one the older code would have allowed through.
    
    ### Concerns
    
    - The new wait depends on the watch cache draining cleanly. If it doesn't, I1's fix (a 30s bound) trades a permanent hang for a transient cleanup error followed by a retry, which is the correct degradation mode for a polling watcher. Once that bound is in place, an additional safety net would be to delete-and-re-list per iteration so a mirror created during the cleanup window is swept on the next poll. `(in-scope for the I1 fix)`
    
    ## Testing Assessment
    
    - The unit-test coverage for `computeSettledCondition` is comprehensive for the new gate.
    - No unit test exercises the `npipe` branch of `probeNamedDockerContext`; that path is only covered via the new Windows BATS run. Adding a `seedContext(t, "npipe-ctx", "npipe:////./pipe/some-pipe")` case in `Test_probeNamedDockerContext` (`docker_context_test.go:215-258`) would be cheap insurance on non-Windows CI.
    - No regression test for I1's hang scenario. A unit test that injects a fake `apiReader` returning a non-empty list and verifies the loop terminates on context cancellation would lock the bound in. Worth adding alongside the fix.
    - The `waitMirrorResourcesGone` polling logic is exercised end-to-end only by the BATS test "stopping VM removes all mirror resources", which doesn't cover the never-drains failure mode.
    
    ## Documentation Assessment
    
    - The `instance.DockerSocket()` doc needs the clarification described in S4.
    - The `probeNamedDockerContext` doc and log message need the `npipe` mention described in S1.
    - The `TODO(windows): …` blocks that this PR removes from `engine_reconciler.go` (`manageDockerContext`, `removeDockerContext`) and `engine/controller.go` (`init`) were correctly retired — their preconditions (npipe-aware probe client, npipe scheme in the context meta, controller registration) are all satisfied here.
    
    ## Commit Structure
    
    The three commits are coherent and split along the natural boundaries: openSUSE bump (`b5ab872`), Windows wiring (`e917657`), drain race fix on slow Windows CI (`0322994`). The third is logically a follow-up to the second since it only became reachable after engine controller registration on Windows; bundling them in one PR is reasonable.
    
    ## Acknowledged Limitations
    
    - `engine_reconciler.go:206-213` already documents the watcher-died vs. never-started branch and the deliberate dual-source approach (`stopWatcher` vs. direct `removeDockerContext`). No issue.
    - `engine_reconciler.go:259-266` documents that the steady-state `setEngineCondition("Connected", …)` is the only place that re-stamps the current generation when the spec is patched without cycling the watcher. The same lifecycle is exactly what the new generation guard at line 238 leans on.
    
    ## Coverage Summary
    
    | File | Status |
    | --- | --- |
    | `bats/tests/32-app-controllers/engine-docker.bats` | S2 |
    | `pkg/controllers/app/app/controllers/app_controller.go` | S3 (also touched by the Settled fix; logic is correct, see Strengths) |
    | `pkg/controllers/app/app/controllers/app_controller_test.go` | Reviewed, no issues |
    | `pkg/controllers/app/app/lima-images-unix.yaml` | Trivial |
    | `pkg/controllers/app/app/lima-images-wsl2.yaml` | Trivial |
    | `pkg/controllers/app/engine/controller.go` | Reviewed, no issues |
    | `pkg/controllers/app/engine/controllers/docker_context.go` | S1 |
    | `pkg/controllers/app/engine/controllers/docker_context_test.go` | Reviewed, no issues |
    | `pkg/controllers/app/engine/controllers/docker_watcher.go` | Reviewed, no issues |
    | `pkg/controllers/app/engine/controllers/engine_reconciler.go` | I1, S3 |
    | `pkg/instance/instance.go` | S4 |
### [Codex] — Pass 1

    ### Executive Summary
    
    This PR wires the engine controller to use a platform-specific Docker endpoint, enables Windows Docker socket paths, tightens App settling around engine cleanup, and bumps the embedded openSUSE image. The main issues are existing controller wiring gaps that become more visible with engine-on-Windows support, plus one small BATS regression in the updated Docker context test.
    
    ### Findings
    
    #### Critical Issues
    
    None.
    
    #### Important Issues
    
    I1. **Standalone containers-controller still does not register the engine controller** — `cmd/containers-controller/main.go:11-17` (important, gap)
    
    ```go
    		// Import app controller packages to trigger init() functions.
    		_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers"
    		"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/external"
    	)
    
    func main() {
    	os.Exit(external.RunControllers("containers"))
    ```
    
    The standalone `containers-controller` imports only the containers aggregator at line 12 and then starts the `containers` group at line 17. That aggregator imports the container, containernamespace, image, and volume controllers at `pkg/controllers/containers/controller.go:12-15`, but it does not import `pkg/controllers/app/engine`. The engine controller only registers via `init()` at `pkg/controllers/app/engine/controller.go:25-26`, and the embedded service imports it separately at `pkg/service/cmd/service.go:59`. As a result, embedded mode gets the engine controller, but `bin/containers-controller` silently omits it even though the engine controller declares the `containers` API group at `pkg/controllers/app/engine/controller.go:59-60` and the help text advertises `containers,-engine` at `pkg/service/controllers/options.go:18`.
    
    Fix: add a blank import for the engine controller to the containers aggregator or the standalone `containers-controller` entry point. Prefer the aggregator so the group stays complete wherever it is imported.
    
    ```diff
     import (
     	// Import controllers to register them.
    +	_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/app/engine"
     	_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/container"
     	_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/containernamespace"
     	_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/containers/image"
    ```
    
    I2. **ContainerNamespace is created only during full sync and is not watched or re-applied** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:825-832` (important, gap)
    
    ```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)
    ```
    
    `fullSync` creates `ContainerNamespace/moby` at `pkg/controllers/app/engine/controllers/docker_watcher.go:615`, and `syncContainerNamespace` applies it at `pkg/controllers/app/engine/controllers/sync_namespace.go:21-24`. But the steady-state watch list at lines 829-831 includes only `Container`, `Image`, and `Volume`, not `ContainerNamespace`, and the running steady-state path at `pkg/controllers/app/engine/controllers/engine_reconciler.go:266-292` does not re-apply the namespace. If `ContainerNamespace/moby` is deleted while the watcher remains alive, no watch event reaches the reconciler and no Docker event can recreate it, leaving the mirror namespace missing until a watcher restart.
    
    Fix: watch `ContainerNamespace` and re-apply the singleton on steady-state reconciles while the Docker watcher is running. Adding the watch alone is not enough; the reconcile path must also recreate it.
    
    ```diff
     	if err := r.setEngineCondition(ctx, &app, metav1.ConditionTrue, "Connected", "Container engine synced"); err != nil {
     		return ctrl.Result{}, err
     	}
    +	if err := r.ensureContainerNamespace(ctx); err != nil {
    +		return ctrl.Result{}, err
    +	}
     
     	var actionsErr, finErr error
    ```
    
    ```diff
     		Watches(&containersv1alpha1.Container{}, enqueueApp).
     		Watches(&containersv1alpha1.Image{}, enqueueApp).
     		Watches(&containersv1alpha1.Volume{}, enqueueApp).
    +		Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp).
     		Complete(r)
    ```
    
    #### Suggestions
    
    S1. **Docker context BATS assertion now has an unused `run -0` and a new `local` inside an `@test`** — `bats/tests/32-app-controllers/engine-docker.bats:790-797` (suggestion, regression)
    
    ```bash
        run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
        if is_windows; then
            assert_output "npipe:////./pipe/docker_engine"
        else
            run -0 rdd service paths docker_socket
            local socket_path=${output}
            run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
            assert_output "unix://${socket_path}"
    ```
    
    The `run -0` at line 790 is followed by the branch at line 791, so on non-Windows its captured `${output}` is discarded and the same `jq` read is repeated at line 796. The new `local socket_path=${output}` at line 795 is inside the `@test` block that starts at line 774, which this repo’s BATS style avoids.
    
    Fix: move the `jq` read into each branch and use a normal assignment in the test body.
    
    ```diff
    -    run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
         if is_windows; then
    +        run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
             assert_output "npipe:////./pipe/docker_engine"
         else
             run -0 rdd service paths docker_socket
    -        local socket_path=${output}
    +        socket_path=${output}
             run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
             assert_output "unix://${socket_path}"
         fi
    ```
    
    ### Design Observations
    
    **Strengths** — Centralizing the Docker client address in `instance.DockerEndpoint()` is a good direction; it removes repeated `"unix://"+socket` construction and makes the Windows `npipe` switch explicit.
    
    **Strengths** — The new stopped-state settling logic is well targeted: `App` now waits for a current terminal engine condition before reporting `Settled=True`, and the added unit tests cover the stale `Connected` case.
    
    ### Testing Assessment
    
    1. I1 is not covered: there is no test that starts or enumerates the standalone `containers-controller` binary and verifies the engine controller is registered in that mode.
    
    2. I2 is not covered: the integration suite checks startup creation and stop cleanup, but not deletion of `ContainerNamespace/moby` while the engine watcher remains running.
    
    3. The Docker context unit tests still exercise `unix`, `tcp`, and `ssh` endpoints only; add a `npipe:////./pipe/docker_engine` case for `createReplaceDockerContext` and scheme handling.
    
    Verified: `go test ./pkg/controllers/app/app/controllers ./pkg/controllers/app/engine/controllers ./pkg/instance` passes.
    
    ### Documentation Assessment
    
    `pkg/instance/instance.go:123-126` still describes `DockerSocket` as the Lima-forwarded host socket, but on Windows the new value is a named pipe served by the host-switch/socket bridge. `pkg/controllers/app/engine/controllers/docker_context.go:155-157` and the log at line 185 also still say “tcp/unix” after adding `npipe`.
    
    ### Acknowledged Limitations
    
    - **Code comment**: `pkg/controllers/app/engine/controllers/docker_watcher.go:206` notes a TODO for periodic full sync so dropped image/volume events self-heal. This is pre-existing and not worsened by the Windows endpoint change.
    
    - **Code comment**: `pkg/controllers/lima/limavm/controllers/hostswitch_windows.go:151-155` says unexpected host-switch exits are logged but not surfaced to the reconciler. This remains relevant to Windows socket forwarding, but the PR does not make that limitation materially worse.
    
    ### Coverage Summary
    
    - `bats/tests/32-app-controllers/engine-docker.bats` — S1
    - `pkg/controllers/app/app/controllers/app_controller.go` — Reviewed, no issues
    - `pkg/controllers/app/app/controllers/app_controller_test.go` — Reviewed, no issues
    - `pkg/controllers/app/app/lima-images-unix.yaml` — Reviewed, no issues
    - `pkg/controllers/app/app/lima-images-wsl2.yaml` — Reviewed, no issues
    - `pkg/controllers/app/engine/controller.go` — I1
    - `pkg/controllers/app/engine/controllers/docker_context.go` — Reviewed, no issues
    - `pkg/controllers/app/engine/controllers/docker_context_test.go` — Reviewed, no issues
    - `pkg/controllers/app/engine/controllers/docker_watcher.go` — I2
    - `pkg/controllers/app/engine/controllers/engine_reconciler.go` — I2
    - `pkg/instance/instance.go` — Reviewed, no issues

### [Gemini] — Pass 1

    ### Executive Summary
    
    This PR enables Windows Docker socket forwarding by wiring up the `npipe://` scheme and removing early-return guards in the engine controller, allowing the host-switch and Docker context integrations to operate on Windows. The core logic for engine condition settling correctly handles generation updates. However, the PR introduces a critical flaw by hardcoding a global Windows named pipe (`\\.\pipe\docker_engine`), which breaks Rancher Desktop's multi-instance isolation and creates an immediate conflict with Docker Desktop. Additionally, the cache-polling logic introduced to fix test flakes bypasses the very cache it intends to poll, leaving the race condition open.
    
    ### Findings
    
    #### Critical Issues
    
    **C1. Global named pipe on Windows breaks multi-instance isolation** — `pkg/instance/instance.go:128-135` (critical, regression)
    
    The `DockerSocket` and `DockerEndpoint` functions hardcode `\\.\pipe\docker_engine` on Windows instead of scoping it per-instance like the Unix path does (via `ShortDir()`, which factors in `instance.Suffix()`). This breaks the multi-instance design defined earlier in `pkg/instance/instance.go`. If a user runs multiple control planes (e.g., `RDD_INSTANCE=1` and `RDD_INSTANCE=2`), they will collide on this static pipe. Furthermore, it conflicts directly with the pipe used by Docker Desktop if both are installed on the same host. 
    
    *Note: Implementing this fix will also require updating `NewDockerHostBridge` in the untouched `pkg/socketbridge/host_windows.go` to use `instance.DockerSocket()` instead of its own hardcoded `DockerPipeName` constant.*
    
    Fix: Scope the named pipe to the instance suffix.
    
    ```diff
    -// On Windows, returns the named pipe path (\\.\pipe\docker_engine).
    +// On Windows, returns an instance-specific named pipe path.
     var DockerSocket = sync.OnceValue(func() string {
     	if runtime.GOOS == "windows" {
    -		return `\\.\pipe\docker_engine`
    +		return `\\.\pipe\rancher_desktop_` + Suffix() + `_docker_engine`
     	}
     	return filepath.Join(ShortDir(), "docker.sock")
     })
     
     // DockerEndpoint returns the full Docker endpoint URL for this instance.
    -// On Windows this is a named-pipe URL (npipe:////./pipe/docker_engine);
    +// On Windows this is a named-pipe URL (e.g., npipe:////./pipe/rancher_desktop_2_docker_engine);
     // on Unix it is a unix-socket URL (unix:///path/to/docker.sock).
     var DockerEndpoint = sync.OnceValue(func() string {
     	if runtime.GOOS == "windows" {
    -		return `npipe:////./pipe/docker_engine`
    +		return "npipe://" + strings.ReplaceAll(DockerSocket(), `\`, `/`)
     	}
     	return "unix://" + DockerSocket()
     })
    ```
    
    #### Important Issues
    
    **I1. `waitMirrorResourcesGone` polls the API server instead of the watch cache** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:535-546` (important, regression)
    
    The `waitMirrorResourcesGone` function exists to block until the API server's cache reflects the mirror resource deletions, preventing a race condition where a downstream client (like `kubectl`) briefly sees deleted resources. However, it polls `countMirrorResources` (line 535), which uses `r.apiReader.List`. `apiReader` is explicitly configured to bypass all caches and query etcd directly. Because the DELETE requests commit to etcd synchronously, `apiReader.List` will return `0` immediately, terminating the loop *before* the asynchronous watch cache has processed the event. 
    
    Fix: Use the local controller-runtime cache (`r.List`) in `countMirrorResources`. Once the local cache drains, the API server's upstream cache is guaranteed to have already processed the deletions.
    
    ```diff
     func (r *EngineReconciler) countMirrorResources(ctx context.Context) (int, error) {
     	lists := []client.ObjectList{
     		&containersv1alpha1.ContainerList{},
     		&containersv1alpha1.VolumeList{},
     		&containersv1alpha1.ImageList{},
     		&containersv1alpha1.ContainerNamespaceList{},
     	}
     	var total int
     	for _, list := range lists {
    -		if err := r.apiReader.List(ctx, list, client.InNamespace(r.apiNamespace)); err != nil {
    +		if err := r.List(ctx, list, client.InNamespace(r.apiNamespace)); err != nil {
     			return 0, err
     		}
     		items, err := meta.ExtractList(list)
    ```
    
    #### Suggestions
    
    **S1. Redundant `run -0 jq` leaves output unread on Unix** — `bats/tests/32-app-controllers/engine-docker.bats:787-795` (suggestion, regression)
    
    The `docker_context_dir` helper places `run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"` just before the `if is_windows; then` block (line 787). On Unix systems, the code enters the `else` branch, executes `run -0 rdd service paths docker_socket`, and leaves the output of the first `jq` command completely unread. The exact same `jq` command is then redundantly executed again later in the Unix branch. This violates the BATS style rule: "Use `run -0` only when the next line reads `${output}`".
    
    Fix: Move the first `jq` call strictly into the Windows branch.
    
    ```diff
    -    run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
         if is_windows; then
    +        run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
             assert_output "npipe:////./pipe/docker_engine"
         else
             run -0 rdd service paths docker_socket
             local socket_path=${output}
             run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
             assert_output "unix://${socket_path}"
         fi
    ```
    
    ### Design Observations
    
    **Concerns**
    - **Docker API time skew (gap)**: In `pkg/controllers/app/engine/controllers/docker_watcher.go` (line 91), the watcher passes `time.Now().Add(-2*time.Minute).Unix()` into the Docker API `since` filter. As noted in the repo context, the Docker daemon runs inside the Lima VM and uses its own clock; passing the host's `time.Now()` is vulnerable to VM clock drift/skew, potentially causing dropped events. 
    
    **Strengths**
    - **Robust Condition Sequencing**: The `computeSettledCondition` logic in `pkg/controllers/app/app/controllers/app_controller.go` (line 450) elegantly enforces `engineCond.ObservedGeneration >= app.Generation`. This successfully averts the classic operator trap where a terminal status belonging to a *previous* reconciliation state incorrectly satisfies the `rdd set running=false` block.
    
    ### Testing Assessment
    
    The test suite updates accurately reflect the expected URL and socket paths across both Unix and Windows paradigms. If the `waitMirrorResourcesGone` cache-bypass issue (I1) is resolved, the integration test will be effectively shielded from the targeted race condition. 
    
    ### Documentation Assessment
    
    None.
    
    ### Acknowledged Limitations
    
    None.
    
    ### Coverage Summary
    
    - `bats/tests/32-app-controllers/engine-docker.bats`: S1
    - `pkg/controllers/app/app/controllers/app_controller.go`: Reviewed, no issues
    - `pkg/controllers/app/app/controllers/app_controller_test.go`: Reviewed, no issues
    - `pkg/controllers/app/app/lima-images-unix.yaml`: Trivial
    - `pkg/controllers/app/app/lima-images-wsl2.yaml`: Trivial
    - `pkg/controllers/app/engine/controller.go`: Reviewed, no issues
    - `pkg/controllers/app/engine/controllers/docker_context.go`: Reviewed, no issues
    - `pkg/controllers/app/engine/controllers/docker_context_test.go`: Reviewed, no issues
    - `pkg/controllers/app/engine/controllers/docker_watcher.go`: Reviewed, no issues
    - `pkg/controllers/app/engine/controllers/engine_reconciler.go`: I1
    - `pkg/instance/instance.go`: C1