Deep Review: 20260412-221509-pr-295

func (r *EngineReconciler) processVolumeFinalizers(ctx context.Context, w *dockerWatcher) error {
	var volumes containersv1alpha1.VolumeList
	if err := r.List(ctx, &volumes, client.InNamespace(apiNamespace)); err != nil {
		return err
	}
	var errs []error
	for i := range volumes.Items {
		v := &volumes.Items[i]
		if v.DeletionTimestamp == nil || !hasFinalizer(v, mirrorFinalizer) {
			continue
		}
		// A Volume mirror with an empty Status.Name has no
		// engine-populated status yet — either a user created it as a
		// bare skeleton or it landed in the startup race window before
		// applyVolume ran. There is no Docker-side name to forward a
		// delete to, so strip the finalizer and let the Delete proceed.
		if v.Status.Name != "" {
			if err := w.deleteVolume(ctx, v.Status.Name); err != nil {
				errs = append(errs, fmt.Errorf("failed to delete volume %s from Docker: %w", v.Name, err))
				continue
			}
		}
		key := client.ObjectKeyFromObject(v)
		retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
			latest := &containersv1alpha1.Volume{}
			if err := r.Get(ctx, key, latest); err != nil {
				if apierrors.IsNotFound(err) {
					return nil
				}

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

// Unlike `Container` / `Image` / `Volume` mirrors, this resource has no
// mirror finalizer: Docker has no corresponding engine object to delete
// on the reverse path, and cleanupMirrorResources sweeps it
// unconditionally on VM stop, so a finalizer with no handler would only
// trap user-initiated deletes in Terminating until the next bounce.
func (w *dockerWatcher) syncContainerNamespace(ctx context.Context) error {
	applyConfig := containersv1alpha1apply.ContainerNamespace(containerNamespace, apiNamespace)

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

// syncAllVolumes lists all Docker volumes and creates/updates `Volume`
// mirrors, then removes stale ones.
func (w *dockerWatcher) syncAllVolumes(ctx context.Context) error {

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

When the container engine deletes an object (the user ran `docker rm` or
`nerdctl rm`, for example), the engine controller strips the finalizer
and deletes the mirror directly, so no reverse engine call occurs.

## Namespaces

`ContainerNamespace` objects reflect the container engine namespaces.  This is
only useful when using the `containerd` backend; when using `dockerd`, the only
valid instance will have a name of `moby`, and it cannot be modified in any way.

`ContainerNamespace` objects only have the default metadata, since they
currently do not need anything else.

```yaml
apiVersion: containers.rancherdesktop.io/v1alpha1
kind: ContainerNamespace
metadata:
  name: k8s.io # `moby` when using the dockerd engine.

    rdd set running=true
    rdd ctl wait --for=create --namespace="${NAMESPACE}" \
        containernamespace/moby --timeout=10s
}

@test "deleting containernamespace/moby completes without a finalizer hang" {
    # moby ContainerNamespace has no mirror finalizer, so a user delete
    # must return promptly rather than get trapped in Terminating.
    rdd ctl delete containernamespace/moby --namespace="${NAMESPACE}" --timeout=10s
    run -0 rdd ctl get containernamespaces --namespace="${NAMESPACE}" --output=name
    refute_output
}

# --- containerd backend ---

@test "containerd backend reports ContainerEngineReady=NotApplicable and skips mirroring" {
    # Stop first so there is no stale True/Connected from moby to

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

		retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
			latest := &containersv1alpha1.Container{}
			if err := r.Get(ctx, key, latest); err != nil {
				if apierrors.IsNotFound(err) {
					return nil
				}
				return err
			}
			if !removeFinalizer(latest, mirrorFinalizer) {
				return nil
			}
			return r.Update(ctx, latest)
		})
		if retryErr != nil {
			errs = append(errs, fmt.Errorf("failed to remove finalizer from Container %s: %w", c.Name, retryErr))
		}
	}
	return errors.Join(errs...)
}

func (r *EngineReconciler) processImageFinalizers(ctx context.Context, w *dockerWatcher) error {
	var images containersv1alpha1.ImageList
	if err := r.List(ctx, &images, client.InNamespace(apiNamespace)); err != nil {
		return err
	}
	var errs []error
	for i := range images.Items {
		img := &images.Items[i]
		if img.DeletionTimestamp == nil || !hasFinalizer(img, mirrorFinalizer) {
			continue
		}
		// An Image mirror with no engine-populated status has no Docker
		// reference to forward the delete to — either a user created it
		// as a bare skeleton or it landed in the startup race window
		// before applyImage ran. Symmetric with processVolumeFinalizers'
		// empty-Status.Name guard: strip the finalizer and let the
		// Delete proceed.
		if img.Status.ID != "" || img.Status.RepoTag != "" {
			if err := w.deleteImage(ctx, img); err != nil {
				errs = append(errs, fmt.Errorf("failed to delete image %s from Docker: %w", img.Name, err))
				continue
			}
		}
		key := client.ObjectKeyFromObject(img)
		retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
			latest := &containersv1alpha1.Image{}
			if err := r.Get(ctx, key, latest); err != nil {
				if apierrors.IsNotFound(err) {
					return nil
				}
				return err
			}
			if !removeFinalizer(latest, mirrorFinalizer) {
				return nil
			}
			return r.Update(ctx, latest)
		})
		if retryErr != nil {
			errs = append(errs, fmt.Errorf("failed to remove finalizer from Image %s: %w", img.Name, retryErr))
		}
	}
	return errors.Join(errs...)
}

func (r *EngineReconciler) processVolumeFinalizers(ctx context.Context, w *dockerWatcher) error {
	var volumes containersv1alpha1.VolumeList
	if err := r.List(ctx, &volumes, client.InNamespace(apiNamespace)); err != nil {
		return err
	}
	var errs []error
	for i := range volumes.Items {
		v := &volumes.Items[i]

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

func (r *EngineReconciler) processContainerFinalizers(ctx context.Context, w *dockerWatcher) error {
	var containers containersv1alpha1.ContainerList
	if err := r.List(ctx, &containers, client.InNamespace(apiNamespace)); err != nil {
		return err
	}
	var errs []error
	for i := range containers.Items {
		c := &containers.Items[i]
		if c.DeletionTimestamp == nil || !hasFinalizer(c, mirrorFinalizer) {
			continue
		}
		if err := w.deleteContainer(ctx, c.Name); err != nil {
			errs = append(errs, fmt.Errorf("failed to delete container %s from Docker: %w", c.Name, err))
			continue
		}
		// Strip the finalizer with retry-on-conflict so a stale cache
		// does not force a whole-reconcile requeue just to reach the
		// same idempotent deleteContainer call. NotFound is benign: a
		// concurrent Docker destroy event may already have stripped
		// the finalizer and deleted the mirror.
		key := client.ObjectKeyFromObject(c)
		retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
			latest := &containersv1alpha1.Container{}
			if err := r.Get(ctx, key, latest); err != nil {

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

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

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

	}

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

	if runningVal == "true" {
		logrus.Info("Waiting for container engine to be ready")
		return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
			status, gen, present := conditionInfo(obj, "ContainerEngineReady")
			return present && gen >= minGen && status == metav1.ConditionTrue
		})
	}
	logrus.Info("Waiting for the VM to stop")
	return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
		// Running absent means the VM was never started (nothing to
		// wait for). Otherwise wait until the condition is refreshed
		// with our generation and reports a non-True status.
		status, gen, present := conditionInfo(obj, "Running")
		if !present {
			return true
		}
		return gen >= minGen && status != metav1.ConditionTrue
	})
}

// watchCondition watches the App singleton until the supplied predicate
// returns true or the context expires. It uses watchtools.UntilWithSync
// so benign watch-channel closures (API server timeouts, proxy
// disconnects, resource-version compaction) re-list transparently

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

		if !alreadyClean {
			if err := r.cleanupMirrorResources(ctx); err != nil {
				log.Error(err, "Failed to clean up mirror resources")
				return ctrl.Result{}, err
			}
		}
		return ctrl.Result{}, r.setEngineCondition(ctx, &app, terminalStatus, terminalReason, terminalMessage)
	}

	if !watcherRunning {
		log.Info("App is running, starting Docker watcher")
		if err := r.startWatcher(ctx); err != nil {
			log.Error(err, "Failed to start Docker watcher")

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

	// Verify the instance stopped
	inst, err := store.Inspect(ctx, limaVM.Name)
	if err != nil {
		logger.Error(err, "Failed to inspect instance after stop")
		if updateErr := r.updateCondition(ctx, limaVM, ConditionRunning, metav1.ConditionFalse, ReasonStopFailed, err.Error()); updateErr != nil {
			logger.Error(updateErr, "Failed to update status after stop failure")
		}
		return ctrl.Result{}, err
	}

	if inst != nil && inst.Status == limatype.StatusRunning {
		err := errors.New("instance still running after stop attempt")
		logger.Error(err, "Failed to stop Lima instance")
		if updateErr := r.updateCondition(ctx, limaVM, ConditionRunning, metav1.ConditionFalse, ReasonStopFailed, err.Error()); updateErr != nil {
			logger.Error(updateErr, "Failed to update status after stop failure")
		}
		return ctrl.Result{}, err
	}

	logger.Info("Stopped Lima instance", "instance", limaVM.Name)
	if err := r.updateCondition(ctx, limaVM, ConditionRunning, metav1.ConditionFalse, ReasonStopped, "Lima instance stopped successfully"); err != nil {
		logger.Error(err, "Failed to update status after stop")

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

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

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

// handleEvent processes a single Docker event.
func (w *dockerWatcher) handleEvent(ctx context.Context, msg events.Message) error {
	switch msg.Type {
	case events.ContainerEventType:
		return w.handleContainerEvent(ctx, msg)
	case events.ImageEventType:
		return w.handleImageEvent(ctx, msg)
	case events.VolumeEventType:
		return w.handleVolumeEvent(ctx, msg)
	default:
		return nil
	}
}

// handleContainerEvent processes container events.
func (w *dockerWatcher) handleContainerEvent(ctx context.Context, msg events.Message) error {
	log := logf.FromContext(ctx).WithName("docker-watcher")

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

	case events.ActionDestroy:
		log.V(1).Info("Container destroyed", "id", msg.Actor.ID)

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

// WithSpec(unknown), the engine-controller field owner would keep
// reasserting spec.state=unknown, clobbering any user patch to
// running/created. Gating the WithSpec call behind a Get — so spec is
// written exactly once, on creation — keeps the invariant "subsequent
// syncs never touch spec" without needing a second field owner.
func (w *dockerWatcher) applyContainer(ctx context.Context, inspect mobycontainer.InspectResponse) error {
	namespace, name := parseContainerName(inspect.Name)

	// Create the `Container` mirror if it doesn't exist. spec.state is
	// always set to "unknown" on creation — meaning the engine mirrors
	// Docker container state without expressing intent. The user can later

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

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

Classification: gap, important.

// correspond to a Docker image ID and its RepoTags. A tagged image gets
// one mirror per tag (name = sanitized-id + "-" + sha256(tag)); a
// dangling image gets a single mirror named after the sanitized ID.
// Sharing this helper between applyImageFromInspect and syncAllImages
// lets the stale-mirror sweep seed activeNames from the list response
// alone, so a transient Inspect failure cannot classify existing
// mirrors as stale.
func imageMirrorNames(id string, repoTags []string) []string {
	if len(repoTags) == 0 {
		return []string{sanitizeKubernetesObjectName(id)}
	}
	names := make([]string, 0, len(repoTags))
	for _, tag := range repoTags {
		names = append(names, fmt.Sprintf("%s-%x",
			sanitizeKubernetesObjectName(id),
			sha256.Sum256([]byte(tag))))
	}
	return names
}

// syncAllImages lists all Docker images and creates/updates `Image`
// mirrors, then removes stale ones.
//
// Images are Inspected sequentially for the same reason as
// syncAllContainers: one-shot at startup, typical dev machines have
// tens of images, and we need fields that only Inspect exposes
// (RepoDigests, full Labels, detailed metadata). Parallelise here if
// startup latency becomes a concern.
func (w *dockerWatcher) syncAllImages(ctx context.Context) error {
	log := logf.FromContext(ctx).WithName("docker-watcher")

	listResult, err := w.cli.ImageList(ctx, dockerclient.ImageListOptions{All: true})
	if err != nil {
		return fmt.Errorf("failed to list images: %w", err)
	}

	// Track which `Image` mirror names we create so we can prune stale ones.
	activeNames := make(map[string]bool)

	// Per-item inspect failures are logged and skipped rather than
	// failing the whole startup (see the matching note in
	// syncAllContainers): a single broken image must not block the
	// watcher from coming up. Structural errors below (k8s list,
	// stale-mirror cleanup) are still fatal.
	//
	// Seed activeNames from the list response before calling Inspect.
	// If Inspect fails transiently the mirror stays in activeNames and
	// is not pruned below — otherwise a flaky Docker daemon would
	// classify a perfectly good Image mirror as stale and delete it.
	var errs []error
	for _, summary := range listResult.Items {
		for _, n := range imageMirrorNames(summary.ID, summary.RepoTags) {
			activeNames[n] = true
		}
		if err := w.syncImageFromSummary(ctx, summary); err != nil {

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

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

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

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

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

	result := w.cli.Events(ctx, dockerclient.EventsListOptions{
		Since:   since,
		Filters: eventFilter,
	})

	for {
		select {
		case <-ctx.Done():
			log.Info("Docker watcher stopping")
			return
		case err := <-result.Err:
			if ctx.Err() != nil {
				return
			}
			log.Error(err, "Docker event stream error")
			w.enqueueReconcile()
			return
		case msg, ok := <-result.Messages:
			if !ok {
				// The daemon closed the event stream without writing to
				// result.Err. Treat the same way as an error: wake the
				// reconciler so it observes the dead watcher and starts
				// a fresh one.
				log.Info("Docker event stream closed")
				w.enqueueReconcile()
				return
			}
			// A transient handleEvent error (apiserver 503, SSA conflict
			// past its internal retry) is logged and the event is
			// dropped. Container events tend to self-heal because the
			// next state change (start/stop/die) triggers another sync
			// on the same ID; image pull and volume create events fire
			// only once, so a dropped apply leaves the mirror missing
			// until the next full resync (watcher restart or engine
			// reconnect). The bounded-window fix is a periodic
			// fullSync tick, deferred until the rest of the mirroring
			// work lands.
			if err := w.handleEvent(ctx, msg); err != nil {
				log.Error(err, "Failed to handle Docker event",

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

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

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

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

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