Deep Review: 20260414-232915-pr-309

		}
		targetGen = gen
	case err != nil:
		return fmt.Errorf("failed to get App: %w", err)
	default:
		gen, err := patchApp(ctx, c, &app, specMap, dryRun)
		if err != nil {
			return err
		}
		targetGen = gen
	}

	if dryRun || timeout == 0 {
		return nil

// cmd/rdd/set.go:474-479
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
    })
}

rdd set now calls waitForDesiredState() after every successful write unless --timeout=0, and the running=true branch accepts only ContainerEngineReady=True.

On Windows, pkg/controllers/app/engine/controller.go:23-29 returns from init() before registering the engine controller at all. AppReconciler only mirrors LimaVM conditions and never synthesizes ContainerEngineReady (see pkg/controllers/app/app/controllers/app_controller.go:302-329), so on Windows the condition has no writer and rdd set running=true hangs for the full 5-minute default timeout before erroring out.

	containersv1alpha1 "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/apis/containers/v1alpha1"
	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/app/engine/controllers"
	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/base"
)

func init() {
	// Windows lacks a Docker socket transport, so the reconciler would
	// hot-loop on connect errors. Skip registration until WSL2 support lands.
	if runtime.GOOS == "windows" {
		return
	}
	base.RegisterController(newController())
}

const (
	controllerName = "engine"
	// apiGroup is "containers" because the reconciler watches

			return ctrl.Result{}, fmt.Errorf("failed to propagate running state to LimaVM: %w", err)
		}
		return ctrl.Result{}, nil
	}

	// Mirror LimaVM status conditions onto the App status. The engine
	// reconciler writes ContainerEngineReady on the same object, so
	// app's resourceVersion from the initial Get can be stale.
	// retry.RetryOnConflict + re-Get matches
	// EngineReconciler.setEngineCondition; without it, concurrent
	// writers 409-loop through controller-runtime requeues. Truncate
	// messages defensively against a future bypass of the LimaVM
	// controller's source-side truncation.
	if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
		latest := &v1alpha1.App{}
		if err := r.Get(ctx, client.ObjectKeyFromObject(&app), latest); err != nil {
			return err
		}
		changed := false
		for _, cond := range limaVM.Status.Conditions {
			changed = apimeta.SetStatusCondition(&latest.Status.Conditions, metav1.Condition{
				Type:               cond.Type,
				Status:             cond.Status,
				Reason:             cond.Reason,
				Message:            base.TruncateConditionMessage(cond.Message),
				ObservedGeneration: latest.Generation,
				LastTransitionTime: cond.LastTransitionTime,
			}) || changed
		}
		if !changed {
			return nil
		}
		return r.Status().Update(ctx, latest)
	}); err != nil {
		log.Error(err, "Unable to update App status")
		return ctrl.Result{}, err
	}

Windows support is still a TODO (the engine controller comment calls out that "WSL2 support" is pending), but a 5-minute UX regression on any Windows build path is worse than a clean error. Codex flagged this as Critical; we downgrade to Important because Windows is not yet a shipping target, but it still wants addressing before Windows work lands.

Fix: either (a) register a Windows stub engine reconciler that stamps a terminal ContainerEngineReady=True, Reason=NotApplicable once the App is Running, matching the non-moby path at engine_reconciler.go:136-147, or (b) have waitForDesiredState detect that the engine controller is not enabled (for example, by consulting the discovery ConfigMap's EnabledControllers) and skip the condition wait. Waiting on a condition with no writer is not safe.

// SetupWithManager sets up the controller with the Manager.
func (r *EngineReconciler) SetupWithManager(mgr ctrl.Manager) error {
	r.reconcileChan = make(chan event.GenericEvent, 1)

	// Map any Container/Image/Volume event to a reconcile of the App singleton.
	enqueueApp := handler.EnqueueRequestsFromMapFunc(
		func(_ context.Context, _ client.Object) []reconcile.Request {
			return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: appName}}}
		},
	)

	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).

The engine reconciler watches App, Container, Image, and Volume only. corev1.Namespace and containersv1alpha1.ContainerNamespace are absent from the watch set.

ensureNamespace (sync_volumes.go:40-54) and syncContainerNamespace (sync_volumes.go:61-66) are only reached from fullSync (docker_watcher.go:426-451), which runs at watcher startup. Once the watcher is running, a kubectl delete namespace rancher-desktop or kubectl delete containernamespace moby produces no event that the reconciler cares about. The namespace stays gone — which cascade-deletes every mirror resource in it — and the ContainerNamespace stays gone until the next VM stop/start or Docker reconnect.

	return fmt.Sprintf("vol-%x", sum)
}

// ensureNamespace creates the Kubernetes namespace that holds the
// mirror resources if it doesn't exist.
func (w *dockerWatcher) ensureNamespace(ctx context.Context) error {
	var ns corev1.Namespace
	if err := w.k8s.Get(ctx, client.ObjectKey{Name: apiNamespace}, &ns); err != nil {
		if !apierrors.IsNotFound(err) {
			return err
		}
		ns = corev1.Namespace{
			ObjectMeta: metav1.ObjectMeta{Name: apiNamespace},
		}
		if err := w.k8s.Create(ctx, &ns); err != nil && !apierrors.IsAlreadyExists(err) {
			return fmt.Errorf("failed to create namespace %s: %w", apiNamespace, err)
		}
	}
	return nil
}

// syncContainerNamespace creates the "moby" ContainerNamespace mirror.
// This resource has no mirror finalizer: Docker has no corresponding
// engine object for the reverse delete, and cleanupMirrorResources
// sweeps it unconditionally on VM stop, so a finalizer with no
// handler would only trap user deletes in Terminating.
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, creates or updates their
// Volume mirrors, and prunes stale ones.
func (w *dockerWatcher) syncAllVolumes(ctx context.Context) error {
	log := logf.FromContext(ctx).WithName("docker-watcher")

	return err
}

// fullSync lists all containers, images, and volumes from Docker and
// creates corresponding mirror resources. It also removes stale mirrors.
func (w *dockerWatcher) fullSync(ctx context.Context) error {
	log := logf.FromContext(ctx).WithName("docker-watcher")
	log.Info("Starting full sync")

	if err := w.ensureNamespace(ctx); err != nil {
		return fmt.Errorf("failed to ensure namespace: %w", err)
	}

	var errs []error

	if err := w.syncContainerNamespace(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync container namespace: %w", err))
	}
	if err := w.syncAllContainers(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync containers: %w", err))
	}
	if err := w.syncAllImages(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync images: %w", err))
	}
	if err := w.syncAllVolumes(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync volumes: %w", err))
	}

	log.Info("Full sync complete")
	return errors.Join(errs...)
}

Deleting the namespace also destroys every mirror resource without giving the mirror finalizer a chance to forward deletes to Docker, which is a correctness issue the design otherwise takes care to preserve.

Fix: add Watches(&corev1.Namespace{}, ...) filtered to rancher-desktop and Watches(&containersv1alpha1.ContainerNamespace{}, enqueueApp), then in the steady-state reconcile path (after startWatcher) unconditionally call w.ensureNamespace(ctx) and w.syncContainerNamespace(ctx). Both are idempotent SSA applies with Get-guards, so no-op cost is negligible.

		return
	}
	base.RegisterController(newController())
}

const (
	controllerName = "engine"
	// apiGroup is "containers" because the reconciler watches
	// Container, Image, and Volume from that group and needs
	// their CRDs at startup. Grouping engine with its dependencies
	// keeps --controllers selections from splitting the two apart.
	apiGroup = "containers"
)

type controller struct{}

func newController() base.Controller {
	return &controller{}
}

var _ base.Controller = &controller{}

func (c *controller) GetName() string {
	return controllerName
}

func (c *controller) GetAPIGroup() string {
	return apiGroup
}

func (c *controller) GetCRDData() string {
	return ""
}

shouldEnableController selects controllers by either name or API group. Starting with --controllers=app therefore enables the engine controller without enabling the containers API group. But the engine reconciler's SetupWithManager calls Watches(&containersv1alpha1.Container{}, ...), Image, and Volume — if those CRDs aren't installed, the manager start fails (the watch cache can't establish) or the reconciler endlessly re-attempts.

The controller also has no CRDs of its own (GetCRDData() returns ""), so the dependency is purely on another controller group's CRDs. That relationship is invisible to shouldEnableController and to the --controllers help text.

cmd/app-controller/main.go:12-20 imports the engine package into the standalone app-controller binary too, which means the standalone binary now also needs the containers API to be installed elsewhere, or it will fail at manager start.

package main

import (
	"os"

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

func main() {
	os.Exit(external.RunControllers("app"))
}

Fix options, in order of preference:

1. Move the engine controller into the containers group (rename apiGroup = "containers"). It mirrors Docker state into the containers API, so this is where it conceptually belongs. This also makes the dependency implicit.
2. Teach shouldEnableController to auto-enable cross-group dependencies, or refuse startup with a clear error if engine is selected without containers.
3. At minimum, document the dependency in the --controllers help text.

	// the same way.
	var labels map[string]string
	if inspect.Config != nil {
		labels = inspect.Config.Labels
	}

	// Build status.
	statusApply := containersv1alpha1apply.ContainerStatus().
		WithName(name).
		WithNamespace(namespace).
		WithPath(inspect.Path).

// sync_images.go:157-163
statusApply := containersv1alpha1apply.ImageStatus().
    WithID(inspect.ID).
    WithRepoDigests(digests...).
    WithArchitecture(inspect.Architecture).
    WithOS(inspect.Os).
    WithSize(inspect.Size).
    WithLabels(inspect.Config.Labels)

mobycontainer.InspectResponse.Config is *container.Config and mobyimage.InspectResponse.Config is *dockerspec.DockerOCIImageConfig (verified in moby api@v1.54.1). Both are pointers that can be nil — the container path also explicitly guards inspect.NetworkSettings a few lines down (sync_containers.go:182), showing the authors already know these fields are nilable.

	}
	if t, err := time.Parse(time.RFC3339Nano, inspect.State.StartedAt); err == nil && !t.IsZero() {
		statusApply.WithStartedAt(metav1.NewTime(t))
	}
	if t, err := time.Parse(time.RFC3339Nano, inspect.State.FinishedAt); err == nil && !t.IsZero() {
		statusApply.WithFinishedAt(metav1.NewTime(t))
	}

	// Sort port bindings by name before applying. Go map iteration is
	// randomized, and even with +listType=map the entries land in the
	// stored object in the order we send them — a stable order avoids

Nil Config is unlikely for a freshly inspected container but possible for legacy/multi-platform images or when a Docker-side bug corrupts an image record. A panic in the Docker watcher goroutine is partially caught by the recover at docker_watcher.go:144-149, but that recover only logs and returns — the watcher goroutine exits, the reconciler detects a dead watcher and reconnects, and the panic rechecks the same input on the next sync, reproducing the crash. That's an infinite reconnect loop on a single bad image, not a clean failure.

	// the reconciler could wake, see alive()==true on the about-to-exit
	// goroutine, and skip the restart.
	defer w.enqueueReconcile()
	defer close(w.done)
	// Keep a bad event shape from crashing the whole app-controller.
	defer func() {
		if r := recover(); r != nil {
			log.Error(nil, "panic in Docker watcher goroutine",
				"recovered", r, "stack", string(debug.Stack()))
		}
	}()

	eventFilter := dockerclient.Filters{}.
		Add("type", string(events.ContainerEventType)).
		Add("type", string(events.ImageEventType)).
		Add("type", string(events.VolumeEventType))

Fix: guard both accesses. The pattern used elsewhere in this file is a local nil-check before building the apply config:

var labels map[string]string
if inspect.Config != nil {
    labels = inspect.Config.Labels
}
// ...WithLabels(labels)

Apply the same pattern in imageStatusFromInspect.

// ValidateUpdate implements [ctrlwebhookadmission.Validator].
// Only spec.state changes are allowed; all other spec fields are immutable.
// The CRD schema already constrains spec.state to the valid enum
// ({created, running, unknown}), so this webhook only enforces that
// nothing else in the spec changes.
func (c *immutableValidator) ValidateUpdate(_ context.Context, oldContainer, newContainer *v1alpha1.Container) (warnings ctrlwebhookadmission.Warnings, err error) {
	oldCopy := oldContainer.Spec
	oldCopy.State = newContainer.Spec.State
	if !equality.Semantic.DeepEqual(oldCopy, newContainer.Spec) {
		return nil, fmt.Errorf("only spec.state may be changed: old: %v, new: %v", oldContainer.Spec, newContainer.Spec)
	}
	return nil, nil
}

	// "created" is satisfied by any non-running state (created,
	// exited, dead).
	//
	// +required
	// +kubebuilder:default:=unknown
	// +kubebuilder:validation:Enum=created;running;unknown
	State ContainerStatusValue `json:"state"`
}

// ContainerStatus defines the observed state of the container.
type ContainerStatus struct {

# Engine controller tests — verify that the engine controller mirrors Docker
# containers, images, and volumes into Container, Image, and Volume
# resources, and that deletions and spec.state changes are forwarded
# to Docker.

NAMESPACE="rancher-desktop"
DOCKER_HOST="unix://$(rdd svc paths docker_socket)"
export DOCKER_HOST

local_setup_file() {
    # The Docker socket access pattern used by these tests is not yet wired
    # up for Windows/WSL2.
    skip_on_windows

local_setup_file() {
    skip_on_windows
    run -0 rdd svc paths docker_socket
    DOCKER_HOST="unix://${output}"
    export DOCKER_HOST
    rdd svc delete
    rdd set running=true
}

			}
		}
		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")
			if condErr := r.setEngineCondition(ctx, &app, metav1.ConditionFalse, "ConnectFailed", err.Error()); condErr != nil {
				log.Error(condErr, "Failed to update ContainerEngineReady to ConnectFailed")
			}
			return ctrl.Result{}, err
		}
	}

	// Re-assert Connected on every steady-state reconcile so its
	// observedGeneration tracks the current App generation. When the
	// spec is patched without cycling the watcher (e.g. `rdd set
	// running=true kubernetes.enabled=true`), this is the only place

w, err := newDockerWatcher(context.WithoutCancel(ctx), r.Client, r.reconcileChan)