Deep Review: 20260417-113401-pr-315

		log.Info("Deleted input ConfigMap after LimaVM created its own copy")
		// ConfigMaps are not watched (no Owns(&corev1.ConfigMap{})), so deleting
		// one produces no watch event. Requeue explicitly to guarantee the next
		// reconcile runs rather than relying on implicit LimaVM status activity.
		return ctrl.Result{Requeue: true}, nil
	} else if !apierrors.IsNotFound(err) {
		return ctrl.Result{}, fmt.Errorf("failed to fetch input ConfigMap: %w", err)
	}

	if limaVM.Spec.Running && !app.Spec.Running {
		limaVM.Spec.Running = false
		if err := r.Update(ctx, limaVM); err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to propagate running state to LimaVM: %w", err)
		}
		return ctrl.Result{}, nil
	}

	// Propagate app spec.containerEngine and spec.kubernetes into the LimaVM's
	// template ConfigMap.
	if limaVM.Status.TemplateConfigMap != "" {
		templateCM := &corev1.ConfigMap{}
		if err := r.Get(ctx, client.ObjectKey{Name: limaVM.Status.TemplateConfigMap, Namespace: namespace}, templateCM); err != nil {
			if !apierrors.IsNotFound(err) {
				return ctrl.Result{}, fmt.Errorf("failed to fetch LimaVM template ConfigMap: %w", err)
			}
		} else {
			desired, err := applySpecToTemplate(r.LimaTemplateData, app.Spec)
			if err != nil {
				return ctrl.Result{}, fmt.Errorf("failed to apply spec to template: %w", err)
			}
			if templateCM.Data[limav1alpha1.TemplateConfigMapKey] != desired {

The PR's stated motivation is rdd set containerEngine.name=containerd waiting for the VM restart. After the App reconciler patches the template ConfigMap and requeues, the next pass copies LimaVM's still-Running=Started condition into the App and re-stamps it with the new App.Generation at line 334. computeSettledCondition then sees a current-generation Running=Started and, on engine-disabled paths (Windows, --controllers=-engine) immediately returns Settled=True. On engine-enabled paths the engine condition's ObservedGeneration is also stale, which holds Settled at False via the EngineStale branch — but that masking depends on the engine controller running in the same process, not on a real Lima convergence signal.

	}

	if !limaVM.Spec.Running && app.Spec.Running {
		limaVM.Spec.Running = true
		if err := r.Update(ctx, limaVM); err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to propagate running state to LimaVM: %w", err)
		}
		return ctrl.Result{}, nil
	}

	// Mirror LimaVM status conditions and compute Settled. The engine

The new comment at lines 297-300 acknowledges the race as a brief blip, but rdd set's waitForDesiredState returns the moment it sees Settled=True at the new generation. There is no minimum dwell, no integration test exercising the running-VM backend swap (engine-docker.bats:534-559 starts from a stopped VM), and no Lima-side condition the App can synchronise on. The result is exactly the early-return behaviour the PR removes for running=true/false but reintroduces for backend / kubernetes spec changes on a running VM.

    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
    # satisfy the Settled wait below before the engine reconciler has
    # processed the containerd switch.
    rdd set running=false

    # Start with containerd. rdd set waits for Settled=True, which
    # requires ContainerEngineReady=True. The engine reconciler
    # satisfies that immediately with reason NotApplicable because
    # engine mirroring only supports the moby backend.
    rdd set containerEngine.name=containerd running=true

    run -0 rdd ctl get app app \
        -o jsonpath='{.status.conditions[?(@.type=="ContainerEngineReady")].reason}'
    assert_output "NotApplicable"

    # No mirror resources should exist in containerd mode.
    run -0 rdd ctl get containers --namespace="${RDD_NAMESPACE}" --output=name
    refute_output
    run -0 rdd ctl get images --namespace="${RDD_NAMESPACE}" --output=name
    refute_output
    run -0 rdd ctl get volumes --namespace="${RDD_NAMESPACE}" --output=name
    refute_output
    run -0 rdd ctl get ContainerNamespaces --namespace="${RDD_NAMESPACE}" --output=name
    refute_output
}

Fix: gate Settled on a Lima-owned convergence signal rather than the restamped Running mirror. Either (a) add LimaVMStatus.ObservedTemplateResourceVersion and require limaVM.Status.ObservedTemplateResourceVersion == templateCM.ResourceVersion before computing Settled=True, or (b) have LimaVM publish a dedicated "reconciled with current template" condition that the App controller aggregates. As an interim, set Settled to False with reason RestartPending on the same reconcile that patches the template CM, and persist a marker so the next pass does not flip it back to True until LimaVM has reacted.

	if err := v1alpha1.AddToScheme(mgr.GetScheme()); err != nil {
		return err
	}
	if err := limav1alpha1.AddToScheme(mgr.GetScheme()); err != nil {
		return err
	}

	discovery, err := servicecontrollers.NewControllerManagerDiscovery(mgr.GetConfig())
	if err != nil {
		return fmt.Errorf("failed to create controller-manager discovery: %w", err)
	}

	if err := (&controllers.AppReconciler{
		Client:           mgr.GetClient(),
		Scheme:           mgr.GetScheme(),
		LimaTemplateData: limaTemplateData(),

base.IsControllerEnabled is documented as "started in this OS process" (controller.go:122-157), populated only from each manager's local set in service.go:761-772 and external/main.go:86-92. The repo ships separate cmd/app-controller and cmd/containers-controller standalone binaries; docs/design/discovery.md:56-64 allows external controllers to attach later. Whenever the engine controller runs in a different manager from the App controller — including the future where engine ships in cmd/containers-controller — EngineEnabled stays false, computeSettledCondition skips the ContainerEngineReady gate at lines 393-405, and cmd/rdd/set.go:477-486 returns before the engine has connected. This regresses the prior CLI-side behaviour, which queried the discovery ConfigMap to decide whether to wait on the engine condition.

	validator, err := controllers.NewAppValidator(k3sVersionsData)
	if err != nil {
		return err
	}
	validatingConfig := base.WebhookConfig[*v1alpha1.App]{
		Name:        appValidatorConfigName,
		WebhookName: appValidatorWebhookName,
		WebhookPort: c.webhookPort,
		Operations: []admissionregistrationv1.OperationType{
			admissionregistrationv1.Create,
			admissionregistrationv1.Update,
		},
		Validator: validator,
	}

	managers, err := base.SetupWebhookForResource(mgr, &v1alpha1.App{}, validatingConfig)
	if err != nil {
		return err
	}
	c.webhookManagers = append(c.webhookManagers, managers...)
	return nil
}

// RegisterWithManager implements the complete controller registration for both embedded and external modes.
// It registers the CRD types with the scheme and sets up the controller with the manager.
// It also registers Lima types, which allows App controller to create and watch LimaVM resources.
func (c *controller) RegisterWithManager(mgr ctrl.Manager) error {
	if err := v1alpha1.AddToScheme(mgr.GetScheme()); err != nil {
		return err
	}
	if err := limav1alpha1.AddToScheme(mgr.GetScheme()); err != nil {
		return err
	}

	discovery, err := servicecontrollers.NewControllerManagerDiscovery(mgr.GetConfig())
	if err != nil {
		return fmt.Errorf("failed to create controller-manager discovery: %w", err)
	}

	if err := (&controllers.AppReconciler{
		Client:           mgr.GetClient(),
		Scheme:           mgr.GetScheme(),
		LimaTemplateData: limaTemplateData(),
		Discovery:        discovery,
	}).SetupWithManager(mgr); err != nil {
		return err

	if len(controllersToStart) == 0 {
		setupLog.Info("All controllers are already running in RDD controller manager, exiting")
		return 0
	}

	// Expose the enabled set to reconcilers that need to know which
	// siblings share this process (see pkg/controllers/base docs).
	enabledNames := make([]string, 0, len(controllersToStart))
	for _, controller := range controllersToStart {
		enabledNames = append(enabledNames, controller.GetName())
	}
	base.SetInProcessControllers(enabledNames)

	// Create a cancellable context for control plane monitoring
	monitorCtx, cancelMonitor := context.WithCancel(ctx)
	defer cancelMonitor()

Ordering both steps before the annotation keeps the "ready = clients may proceed" contract consistent: any client that waits for the annotation sees both CRDs and the enabled-controller list, not just one of the two.

The ConfigMap is recreated on every control plane startup, so a stale `ready` from a previous crash is always cleared before CRD installation begins.

The annotation only covers controllers known at control plane startup. External controllers (for example, the mock controller, or a controller manager started later via `rdd svc create --controllers=...`) can attach at any time, so a client waiting for the ready annotation must not assume that every controller it cares about is already registered. Discover external controllers by reading the ConfigMap directly.

## Consumers

**Service readiness** -- The control plane queries the ConfigMap to discover which controllers are running across all managers. It uses this to decide when external controllers have finished registering.

**Passthrough proxy** -- The control plane exposes a `/passthrough/<controller>/<endpoint>/...` HTTP route that looks up the target manager via the ConfigMap and reverse-proxies the request.

**External controller startup** -- An external controller checks the ConfigMap to see whether its controllers are already running (to avoid conflicts) and monitors it to detect when the control plane has stopped.

**Health gating** -- `IsControllerRunning` performs an HTTP health check against the registered `healthEndpoint` before reporting a controller as running.

// current generation.
//
// Filtering on ObservedGeneration >= minGen blocks a stale
// Settled=True from a prior reconcile from satisfying the wait
// before the App controller observes our write.
func waitForDesiredState(ctx context.Context, config *rest.Config, timeout time.Duration, minGen int64) error {
	// timeout == 0 means "no deadline", matching kubectl wait.
	ctx, cancel := watchtools.ContextWithOptionalTimeout(ctx, timeout)
	defer cancel()

	logrus.Info("Waiting for App to settle")
	return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
		status, gen, present := conditionInfo(obj, appv1alpha1.AppConditionSettled)
		return present && gen >= minGen && status == metav1.ConditionTrue
	})
}

// watchCondition watches the App singleton until the predicate
// returns true or the context expires. watchtools.UntilWithSync
// re-lists transparently on benign watch-channel closures (API

The split-manager scenario is not exploitable today because engine is not blank-imported by any standalone binary (only pkg/service/cmd/service.go's embedded path imports it). That makes this latent rather than active — but the architectural assumption "engine is enabled iff it's in this process" is built into the wait contract just as engine is being prepared for external execution.

Fix: derive engine availability from the shared discovery ConfigMap (controllers.NewControllerManagerDiscovery(mgr.GetConfig())) and check it during reconciliation, so late-attaching managers can take effect without a controller restart. Cache the result with a short TTL if the lookup cost is a concern.

				if templateCM.Data == nil {
					templateCM.Data = make(map[string]string)
				}
				templateCM.Data[limav1alpha1.TemplateConfigMapKey] = desired
				if err := r.Patch(ctx, templateCM, patch); err != nil {
					return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap: %w", err)
				}
				log.Info("Updated template ConfigMap",
					"containerEngine", app.Spec.ContainerEngine.Name,
					"kubernetesEnabled", app.Spec.Kubernetes.Enabled,
					"kubernetesVersion", app.Spec.Kubernetes.Version)

The PR title is "app: aggregate Ready and Settled conditions, wait for Settled in rdd set" and the body has a two-paragraph "Ready" section explaining the contract. But app_types.go defines only AppConditionSettled (line 38); there is no AppConditionReady, nothing in computeSettledCondition produces a Ready entry, and the unit tests only assert the Settled path. The stale comment on app_controller.go:314 was introduced by this commit and will mislead readers into looking for a Ready aggregator.

				if templateCM.Data == nil {
					templateCM.Data = make(map[string]string)
				}
				templateCM.Data[limav1alpha1.TemplateConfigMapKey] = desired
				if err := r.Patch(ctx, templateCM, patch); err != nil {
					return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap: %w", err)
				}
				log.Info("Updated template ConfigMap",
					"containerEngine", app.Spec.ContainerEngine.Name,
					"kubernetesEnabled", app.Spec.Kubernetes.Enabled,
					"kubernetesVersion", app.Spec.Kubernetes.Version)

)

// ControllerDiscovery enumerates controllers enabled across all controller
// managers in this control plane. AppReconciler queries it during
// reconciliation to decide whether Settled must wait for
// ContainerEngineReady. The interface mirrors a small subset of
// pkg/service/controllers.ControllerManagerDiscovery so tests can substitute
// a fake without taking on the full dependency.
type ControllerDiscovery interface {
	GetEnabledControllers(ctx context.Context) ([]string, error)
}

This matters for the review itself: a reviewer cannot tell from the PR whether Ready was an intentional scope reduction or an implementation oversight — Gemini's executive summary in this round explicitly described the PR as "introduces the Ready and Settled conditions", taking the PR description at face value.

Fix: decide whether Ready is in scope. If not, drop "Ready" from the PR title and body, and correct the comment:

-	// Mirror LimaVM status conditions and aggregate Ready/Settled. The
-	// engine reconciler writes ContainerEngineReady on the same object,
+	// Mirror LimaVM status conditions and compute Settled. The
+	// engine reconciler writes ContainerEngineReady on the same object,

If Ready is still intended, add the constant in app_types.go, compute it alongside Settled, and extend the test matrix.