Deep Review: 20260415-015141-pr-309

	log.Info("Installing CRDs for all controllers in parallel", "controllers", len(scm.registrations))
	if err := scm.installControllerCRDs(ctx); err != nil {
		return fmt.Errorf("failed to install controller CRDs: %w", err)
	}

	// Configure controller-runtime to use klog
	ctrllog.SetLogger(log.WithName(fmt.Sprintf("controller-runtime-%s", scm.name)))

	// Create and register scheme with required types
	managerScheme := runtime.NewScheme()
	if err := scheme.AddToScheme(managerScheme); err != nil {
		return fmt.Errorf("failed to add core scheme: %w", err)
	}
	// Add CRD types to support dynamic resource discovery
	if err := apiextensionsv1.AddToScheme(managerScheme); err != nil {
		return fmt.Errorf("failed to add apiextensions scheme: %w", err)
	}

SharedControllerManager.Start installs CRDs, then sets ReadyAnnotation, and only later — at manager.go:202 — calls registerDiscovery, which is what writes the EnabledControllers list into configMap.Data.

	// Mark the control plane ready after registerDiscovery so clients
	// waiting on the ready annotation see both CRDs installed and
	// controller registration written, not just CRDs.
	if err := MarkControlPlaneReady(ctx, scm.discovery.client); err != nil {
		return fmt.Errorf("failed to mark control plane as ready: %w", err)
	}

	// Ensure cleanup on shutdown with a timeout to avoid blocking if apiserver is gone
	defer func() {
		cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 5*time.Second)

// set.go:490-505
engineEnabled, err := isEngineControllerEnabled(ctx, config)
if err != nil {
    logrus.WithError(err).Debug("Falling back to ContainerEngineReady wait after discovery lookup failed")
    engineEnabled = true
}
if engineEnabled {
    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 start (engine controller not enabled)")
return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
    status, gen, present := conditionInfo(obj, "Running")
    return present && gen >= minGen && status == metav1.ConditionTrue
})

rdd set running=true calls waitForDesiredState → isEngineControllerEnabled, which reads configMap.Data via GetEnabledControllers (discovery.go:270-289) and returns (false, nil) for a ConfigMap with no data entries. In the window between MarkControlPlaneReady and registerDiscovery, the predicate is: ready annotation set, but EnabledControllers empty. rdd svc start --wait and cmd/rdd/service.go already wait on the ready annotation, so rdd set proceeds into the wait, sees engineEnabled=false, and falls through to the weaker Running=True wait.

	return &info, nil
}

// GetEnabledControllers returns the list of all enabled controllers, across all
// controller managers.  Note that the returned controllers may not be running.
func (d *ControllerManagerDiscovery) GetEnabledControllers(ctx context.Context) ([]string, error) {
	configMap, err := d.client.CoreV1().ConfigMaps(d.namespace).Get(ctx, ControllerManagerConfigMapName, metav1.GetOptions{})
	if errors.IsNotFound(err) {
		return nil, nil // No controller manager running
	}
	if err != nil {
		return nil, fmt.Errorf("failed to discover controller manager: %w", err)
	}

	enabledControllers := make([]string, 0) // non-nil: distinguishes "0 controllers" from "ConfigMap not found"
	for _, serializedInfo := range configMap.Data {
		var info ControllerManagerInfo
		if err := json.Unmarshal([]byte(serializedInfo), &info); err != nil {
			return nil, fmt.Errorf("failed to parse controller manager info: %w", err)
		}

		enabledControllers = append(enabledControllers, info.EnabledControllers...)
	}

	return enabledControllers, nil
}

// IsControllerRunning checks if a specific controller is running in any of the
// shared controller managers.
func (d *ControllerManagerDiscovery) IsControllerRunning(ctx context.Context, controllerName string) (bool, *ControllerManagerInfo, error) {

Running=True is satisfied by LimaVM reaching Running — well before the engine controller has connected to Docker, run fullSync, and stamped ContainerEngineReady=True. That is exactly the race this PR is designed to close. engine.bats never catches it because its tests either start the service with rdd set running=true against an already-ready control plane or run after startup has fully settled.

Codex originally traced this as Important; Claude independently flagged the same ordering (as S1 "isEngineControllerEnabled queries discovery before controller managers register") and proposed defaulting to ContainerEngineReady when the controller list is empty-during-startup. We keep the Important classification because the failure mode reintroduces a ship-blocking behaviour from before the PR.

Fix options, in order of preference:

1. Move registerDiscovery before MarkControlPlaneReady in SharedControllerManager.Start. The enabled-controller list is then visible to every client that waits for the ready annotation. This is the minimal change and preserves the "ready = clients may use the control plane" contract.
2. If keeping readiness independent of controller-manager health is load-bearing, precompute the enabled-controller list into the discovery ConfigMap (or a sibling key) before MarkControlPlaneReady, and have isEngineControllerEnabled read that precomputed record.
3. As a defensive change on top, let isEngineControllerEnabled distinguish "empty list" from "registered with zero engine" and default to the stricter ContainerEngineReady wait when the list is empty. This alone is not sufficient — it papers over the ordering — but it is a belt-and-braces safeguard.

	}
	if err := r.deleteAllOfType(ctx, &containersv1alpha1.ImageList{}); err != nil {
		errs = append(errs, fmt.Errorf("failed to delete Images: %w", err))
	}
	if err := r.deleteAllOfType(ctx, &containersv1alpha1.ContainerNamespaceList{}); err != nil {
		errs = append(errs, fmt.Errorf("failed to delete ContainerNamespaces: %w", err))
	}
	return errors.Join(errs...)
}

// deleteAllOfType lists and removes every resource of the given kind
// in apiNamespace. Engine is authoritative for Container, Image, and
// Volume in `rancher-desktop`: this loop deletes every object, not
// just engine-owned mirrors. Coexistence with another writer to these
// kinds requires an engine-owned label and matching filters here and
// in the sync_*.go full-sync prune paths. Finalizer removal retries
// on conflict; per-item errors are collected so one stuck object does
// not block the rest.
func (r *EngineReconciler) deleteAllOfType(ctx context.Context, list client.ObjectList) error {
	if err := r.List(ctx, list, client.InNamespace(apiNamespace)); err != nil {
		return err
	}

	items, err := meta.ExtractList(list)
	if err != nil {
		return err
	}

	var errs []error
	for _, item := range items {
		obj := item.(client.Object)
		key := client.ObjectKeyFromObject(obj)
		// meta.ExtractList leaves each item's TypeMeta empty (GVK is
		// only on the list), so format the Go type with %T instead of
		// obj.GetObjectKind().
		retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
			latest := obj.DeepCopyObject().(client.Object)
			if err := r.Get(ctx, key, latest); err != nil {
				if apierrors.IsNotFound(err) {
					return nil
				}
				return err
			}
			if !controllerutil.RemoveFinalizer(latest, mirrorFinalizer) {
				return nil
			}
			return r.Update(ctx, latest)
		})
		if retryErr != nil {
			errs = append(errs, fmt.Errorf("failed to remove finalizer from %T %s: %w",
				obj, obj.GetName(), retryErr))
			continue
		}
		// Delete uses the stale list item, not latest from the retry

When cleanupMirrorResources runs (App stopped, backend switched, controller teardown), deleteAllOfType lists every object of a given kind in the rancher-desktop namespace. For each object, it enters a retry closure that removes the engine mirrorFinalizer if present — but the closure exits with nil on "finalizer not present" (line 319). The outer loop then falls through to r.Delete(ctx, obj) unconditionally at line 331. An object created by any other controller in rancher-desktop, lacking the engine finalizer, is deleted anyway.

The full-sync prune paths have the same shape:

// sync_containers.go:62-75
var containerMirrors containersv1alpha1.ContainerList
if err := w.k8s.List(ctx, &containerMirrors, client.InNamespace(apiNamespace)); err != nil {
    return fmt.Errorf("failed to list Containers: %w", err)
}
for i := range containerMirrors.Items {
    c := &containerMirrors.Items[i]
    if !dockerIDs[c.Name] {
        log.V(1).Info("Removing stale Container", "id", c.Name)
        if err := w.removeMirrorResource(ctx, c, c.Name); err != nil {
            errs = append(errs, err)
        }
    }
}

Any Container whose metadata.name does not correspond to a live Docker container ID is pruned, regardless of whether it was created by the engine controller. sync_images.go:220-239 and sync_volumes.go apply the same logic to their kinds. The fallout is already visible in this PR: bats/tests/34-containers-controllers/containers-mock.bats had to switch to --controllers=containers,-engine so that the mock container controller keeps exclusive ownership of the mirror kinds while its tests run. That is a test-only workaround for an architectural problem: two controllers in the same group both write authoritative state into the same namespace and the same kinds, with no ownership marker distinguishing their objects.

	freshNames, applyErr := w.applyImageFromInspect(ctx, result.InspectResponse)
	keep := make(map[string]bool, len(freshNames))
	for _, n := range freshNames {
		keep[n] = true
	}

	var images containersv1alpha1.ImageList
	if err := w.k8s.List(ctx, &images, client.InNamespace(apiNamespace)); err != nil {
		return errors.Join(applyErr, err)
	}
	var errs []error
	if applyErr != nil {
		errs = append(errs, applyErr)
	}
	for i := range images.Items {
		img := &images.Items[i]
		if img.Status.ID != result.InspectResponse.ID {
			continue
		}
		if keep[img.Name] {
			continue
		}
		if err := w.removeMirrorResource(ctx, img, img.Name); err != nil {
			errs = append(errs, err)
		}
	}
	return errors.Join(errs...)
}

Fix: stamp engine-owned objects with a distinguishing label or annotation (e.g. engine.rancherdesktop.io/owned-by=engine-controller) in addition to the finalizer, and filter both deleteAllOfType and every full-sync prune by that marker. Unmarked objects must be skipped entirely so other controllers — the mock used by containers-mock.bats today, any future containerd mirror, or a human-created test fixture — can coexist in the same namespace without losing state on a VM stop or backend switch. Deleting by marker also removes the need for the -engine carve-out in containers-mock.bats.

Two earlier rounds discussed the related "namespace self-healing" question and decided not to re-create rancher-desktop / containernamespace/moby after a user deletes them mid-run. That decision is distinct from this one: self-healing is about re-creating your own objects, while this finding is about not destroying someone else's.

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/external"
)

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

cmd/app-controller/main.go now blank-imports engine to trigger its init(). The intent of round 3 was the opposite: engine declares itself in the containers API group (pkg/controllers/app/engine/controller.go:32-38) precisely so --controllers=app selections cannot pull it in without its CRDs.

		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 {

// pkg/external/main.go:62-72
for _, controller := range base.GetAllControllers() {
    shouldStart, err := shouldStartController(ctx, config, controller.GetName(), setupLog)
    // ...
    } else if shouldStart {
        controllersToStart = append(controllersToStart, controller)
    }
}

RunControllers iterates the global registry and filters only by shouldStartController, which checks whether the controller is already running in another manager. It never compares controller.GetAPIGroup() to the apiGroupName passed in at line 31. If the embedded control plane is running with engine disabled (--controllers=-engine), or if engine has not registered itself in discovery yet, shouldStartController returns true, and cmd/app-controller adds engine to its controllersToStart list. Engine is then registered into the shared manager named "app" at line 99 — not "containers" — which is exactly the scenario round 3 tried to prevent. Once in, engine's SetupWithManager watches Container/Image/Volume from the containers group; if those CRDs are not installed (they are not installed by app-controller itself — demo and app have no containers dependency), the watch caches fail to establish and the manager startup errors out.

Gemini filed a related finding but reversed the effect: it claimed external.RunControllers("app") filters by API group and would therefore silently omit engine. That would be a safer outcome than what the code actually does, but RunControllers does not filter. The engine controller is imported and registered under the wrong shared manager.

Fix options, in order of preference:

1. Drop the _ "...controllers/app/engine" import from cmd/app-controller/main.go. The blank import contributes nothing the standalone binary needs now that engine belongs to a different group, and the other two app-group controllers continue to work. This is a one-line change.
2. In pkg/external/main.go, add an if controller.GetAPIGroup() != apiGroupName { continue } guard to the controllers-to-start loop. This turns the filtering into a systemic guardrail: a future mistaken import in any standalone binary is prevented from cross-registering into the wrong manager. Pair with (1) for belt-and-braces.
3. If the intent is to eventually run engine from a containers-specific standalone binary (cmd/containers-controller), add that binary and move the engine import there instead.