Deep Review: 20260320-225736-pr-236

The default case requeues without removing the owned finalizer. If the LimaVM reaches Terminating through any path other than the App controller's deletion flow (manual delete, CLI tool), the rdd.rancherdesktop.io/owned finalizer blocks deletion indefinitely. The LimaVM controller's handleDeletion (at limavm_lifecycle.go:32–78) removes only the cleanup finalizer and owned finalizers on children — it never touches the owned finalizer on the LimaVM itself. The result: the LimaVM stays Terminating, the App stays Terminating waiting for it, and both resources deadlock.

The same gap exists during normal operation (App not being deleted): if a LimaVM is deleted externally, the reconcile at lines 116–210 proceeds through spec.running propagation and condition mirroring without ever removing the owned finalizer or acknowledging the terminating state.

Verified via git blame: lines 89–101 are introduced in commit 6a94f5d.

			}
		}

		return ctrl.Result{}, base.RemoveCleanupFinalizer(ctx, r.Client, &app)
	case err != nil:
		return ctrl.Result{}, err
	case !base.IsBeingDeleted(limaVM):
		if err := base.RemoveOwnedFinalizer(ctx, r.Client, limaVM); err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to remove owned finalizer from LimaVM: %w", err)
		}
		if err := r.Delete(ctx, limaVM); err != nil && !apierrors.IsNotFound(err) {
			return ctrl.Result{}, fmt.Errorf("failed to delete LimaVM: %w", err)
		}
		log.Info("Requested LimaVM deletion, waiting for teardown")
		return ctrl.Result{RequeueAfter: requeueAfterDeletion}, nil
	default:
		// LimaVM deletion is in progress; keep requeueing until it is gone.
		log.Info("Waiting for LimaVM deletion to complete")
		return ctrl.Result{RequeueAfter: requeueAfterDeletion}, nil
	}

Fix: Always check for the owned finalizer when the LimaVM is terminating. In the deletion switch, consolidate the err == nil cases:

case err == nil:
    if base.HasOwnedFinalizer(limaVM) {
        if err := base.RemoveOwnedFinalizer(ctx, r.Client, limaVM); err != nil {
            return ctrl.Result{}, fmt.Errorf("failed to remove owned finalizer: %w", err)
        }
    }
    if !base.IsBeingDeleted(limaVM) {
        if err := r.Delete(ctx, limaVM); err != nil && !apierrors.IsNotFound(err) {
            return ctrl.Result{}, fmt.Errorf("failed to delete LimaVM: %w", err)
        }
    }
    return ctrl.Result{RequeueAfter: requeueAfterDeletion}, nil

During normal reconcile, add an early check after the Get at line 116:

if base.IsBeingDeleted(limaVM) {
    if err := base.RemoveOwnedFinalizer(ctx, r.Client, limaVM); err != nil {
        return ctrl.Result{}, fmt.Errorf("failed to remove owned finalizer from terminating LimaVM: %w", err)
    }
    return ctrl.Result{RequeueAfter: requeueAfterDeletion}, nil
}

Returning ctrl.Result{}, nil after deleting the ConfigMap relies on the LimaVM controller producing subsequent status updates to re-trigger the App controller. The App controller does not watch or own ConfigMaps, so the deletion itself generates no watch event. In practice the LimaVM always has more lifecycle steps, so this works today. But the coupling is implicit: if any future change reduces LimaVM controller chattiness, the App controller stalls until an external event arrives.

Gemini rated this CRITICAL; after tracing the LimaVM controller lifecycle (limavm_controller.go:176–268), a newly created LimaVM always produces multiple status updates (initial condition at line 176, template ConfigMap at line 248, Created condition). The stall scenario requires all those updates to complete before the App controller processes the ConfigMap deletion — unlikely in practice.

	}

	// LimaVM exists — clean up the input ConfigMap if it still lingers from the
	// creation phase and return to requeueing on LimaVM updates.
	inputConfigMap := &corev1.ConfigMap{}
	if err := r.Get(ctx, client.ObjectKey{Name: inputConfigMapName, Namespace: namespace}, inputConfigMap); err == nil {
		if err := r.Delete(ctx, inputConfigMap); err != nil && !apierrors.IsNotFound(err) {
			return ctrl.Result{}, fmt.Errorf("failed to delete input ConfigMap: %w", err)
		}
		log.Info("Deleted input ConfigMap after LimaVM created its own copy")
		return ctrl.Result{}, nil
	} else if !apierrors.IsNotFound(err) {
		log.Error(err, "Failed to fetch input ConfigMap")
	}

Fix: Replace return ctrl.Result{}, nil with return ctrl.Result{Requeue: true}, nil to guarantee the reconciler continues to spec.running propagation and condition mirroring without depending on external events.

Test 123–126 waits for the LimaVM's Created condition, then test 128 reads the App status without waiting for the App controller to mirror it. The Owns watch triggers a reconcile on LimaVM status changes, but the reconcile may not have completed by the time the BATS test reads the App. The same race applies to test 139–149 ("verify App conditions preserve LimaVM LastTransitionTime").

@test "wait for LimaVM Created condition to be set" {
    rdd ctl wait --for=condition=Created \
        limavm/"${VM_NAME}" --namespace "${RDD_NAMESPACE}" --timeout=60s
}

@test "verify App status mirrors LimaVM Created condition" {
    run -0 rdd ctl get app "${APP_NAME}" -o json
    local json="${output}"

    run -0 jq -r '.status | has("conditions")' <<<"${json}"
    assert_output "true"

    run -0 jq -r '.status.conditions[] | select(.type == "Created") | .status' <<<"${json}"
    assert_output "True"
}

@test "verify App conditions preserve LimaVM LastTransitionTime" {
    run -0 rdd ctl get limavm "${VM_NAME}" --namespace "${RDD_NAMESPACE}" -o json
    local limavm_ts
    limavm_ts=$(jq -r '.status.conditions[] | select(.type == "Created") | .lastTransitionTime' <<<"${output}")

    run -0 rdd ctl get app "${APP_NAME}" -o json
    local app_ts
    app_ts=$(jq -r '.status.conditions[] | select(.type == "Created") | .lastTransitionTime' <<<"${output}")

    [[ "${limavm_ts}" == "${app_ts}" ]]
}

Fix: Add rdd ctl wait --for=condition=Created app/"${APP_NAME}" --timeout=30s before the assertion at line 129.

During App deletion, if fetching either ConfigMap fails with a transient error (connection timeout, rate limit), the code logs the error and falls through to RemoveCleanupFinalizer at line 86. Without a garbage collector, the orphaned ConfigMaps persist indefinitely.

	case apierrors.IsNotFound(err):
		// LimaVM is gone. Clean up any ConfigMaps that may have been left behind.
		inputCM := &corev1.ConfigMap{}
		if cmErr := r.Get(ctx, client.ObjectKey{Name: inputConfigMapName, Namespace: namespace}, inputCM); cmErr == nil {
			if cmErr := r.Delete(ctx, inputCM); cmErr != nil && !apierrors.IsNotFound(cmErr) {
				return ctrl.Result{}, fmt.Errorf("failed to delete input ConfigMap during cleanup: %w", cmErr)
			}
		} else if !apierrors.IsNotFound(cmErr) {
			log.Error(cmErr, "Failed to fetch input ConfigMap during cleanup")
		}

		templateCM := &corev1.ConfigMap{}
		templateCMName := limaVMName + "-template"
		if cmErr := r.Get(ctx, client.ObjectKey{Name: templateCMName, Namespace: namespace}, templateCM); cmErr == nil {
			if cmErr := base.RemoveOwnedFinalizer(ctx, r.Client, templateCM); cmErr != nil {
				return ctrl.Result{}, fmt.Errorf("failed to remove finalizer from template ConfigMap: %w", cmErr)
			}
			if cmErr := r.Delete(ctx, templateCM); cmErr != nil && !apierrors.IsNotFound(cmErr) {
				return ctrl.Result{}, fmt.Errorf("failed to delete template ConfigMap during cleanup: %w", cmErr)
			}
		} else if !apierrors.IsNotFound(cmErr) {
			log.Error(cmErr, "Failed to fetch template ConfigMap during cleanup")
		}

		return ctrl.Result{}, base.RemoveCleanupFinalizer(ctx, r.Client, &app)

Fix: Return the error so the deletion retries:

} else if !apierrors.IsNotFound(cmErr) {
    return ctrl.Result{}, fmt.Errorf("failed to fetch input ConfigMap during cleanup: %w", cmErr)
}

r.Update(ctx, limaVM) sends the entire LimaVM object fetched at line 116. The LimaVM controller frequently patches its status concurrently, incrementing the resourceVersion between the Get and Update. Each collision produces a 409 Conflict and a wasted reconcile cycle. Worse, a full Update can overwrite concurrent metadata changes (annotations, labels) made by the LimaVM controller.

	}

	// Propagate spec.running from App into to the LimaVM.
	if limaVM.Spec.Running != app.Spec.Running {
		limaVM.Spec.Running = app.Spec.Running
		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
	}

Fix: Use a merge patch targeting only spec.running:

patch := client.MergeFrom(limaVM.DeepCopy())
limaVM.Spec.Running = app.Spec.Running
if err := r.Patch(ctx, limaVM, patch); err != nil {

The SSH port at line 14 is fixed at 51422, ignoring instance.Index() which derives per-instance ports. The host socket path at line 37 uses .rd instead of .rd<suffix> (the default suffix is "2", so the runtime path is ~/.rd2). Both break parallel instances and the socket path is wrong even for the default instance.

The controller.go:18 comment acknowledges the template is temporary ("This is temporary and will be removed once the app controller is fully implemented"). The BATS tests never boot the VM to Running, so these paths are untested.

images:
- location: https://github.com/rancher-sandbox/rancher-desktop-opensuse/releases/download/v0.1.2/distro.v0.1.2.amd64.qcow2
  arch: x86_64
  digest: "sha256:45de5c158399d5bc31e93f42d9c7fcfaf8dd3279ee157c702e9bd86bcaf9e088"
- location: https://github.com/rancher-sandbox/rancher-desktop-opensuse/releases/download/v0.1.2/distro.v0.1.2.arm64.qcow2
  arch: aarch64
  digest: "sha256:39598a41ebe5c8da0baf428716a63c50598e8306e85bb85043877407eecc8991"
cpus: 2
memory: "6442450944"
mounts:
- location: "~"
  writable: true
ssh:
  localPort: 51422
  loadDotSSHPubKeys: false
firmware:
  legacyBIOS: false
containerd:
  system: false
  user: false
portForwards:
- guestIPMustBeZero: true
  guestPortRange:
  - 1
  - 65535
  hostIP: 0.0.0.0
  hostPortRange:
  - 0
  - 0
- guestPortRange:
  - 0
  - 0
  guestSocket: /var/run/docker.sock
  hostPortRange:
  - 0
  - 0
  hostSocket: "{{.Home}}/.rd/docker.sock"
hostResolver:
  hosts:
    host.docker.internal: host.lima.internal
    host.rancher-desktop.internal: host.lima.internal
    lima-rancher-desktop: lima-0
rosetta:
  enabled: false
  binfmt: false

Fix: Render dynamic fields at reconcile time from instance.ShortDir() and instance.Index() instead of embedding literal values.

This sentence stops mid-thought.

	} else if added {
		return ctrl.Result{}, nil
	}

	// Check LimaVM first. If it exists and we can skip ConfigMap
	limaVM := &limav1alpha1.LimaVM{}
	limaVMErr := r.Get(ctx, client.ObjectKey{Name: limaVMName, Namespace: namespace}, limaVM)
	if limaVMErr != nil && !apierrors.IsNotFound(limaVMErr) {
		return ctrl.Result{}, limaVMErr
	}

Fix: // Check whether the LimaVM already exists. If so, skip ConfigMap creation.