Deep Review: 20260323-220902-pr-242

The deletion path at line 47-69 correctly guards against PID recycling: it skips PID-based kills for StatusBroken instances and clears PIDs on Windows before calling Lima's Delete.

	if existingInst != nil {
		// Only use PID-based force-stop for Running instances. Broken
		// instances may have stale PID files pointing to recycled processes
		// on Windows (Lima's ReadPIDFile treats any live PID as valid).
		// Not tested: simulating stale PID files requires Windows-specific
		// PID file manipulation that BATS cannot easily reproduce.
		if existingInst.Status == limatype.StatusRunning {
			stopInstanceForcibly(ctx, logger, existingInst)
		} else if existingInst.VMType == limatype.WSL2 {
			// A "stopped" WSL2 distro can retain kernel state that deadlocks
			// wsl.exe --unregister. Terminate it without PID-based killing,
			// since the PIDs may have been recycled on Windows.
			terminateWSL2Distro(ctx, logger, existingInst.Name)
		}
		if runtime.GOOS == "windows" {
			// Clear PIDs so Lima's Delete -> StopForcibly does not kill
			// unrelated processes if the PIDs were recycled. Windows recycles
			// PIDs aggressively, and Lima's ReadPIDFile treats any live PID
			// as valid. On Unix, PID recycling is rare (wraps around 32768+),
			// so we let Lima's Delete clean up any surviving driver processes.
			existingInst.DriverPID = 0
			existingInst.HostAgentPID = 0
		}

However, handleWatchedState (line 209-212, changed by commit 16c73c0) and handleUnwatchedState (line 269-277, pre-existing) route StatusBroken instances through stopInstanceForcibly and killOrphanedHostagent without similar guards.

		// The VM driver (e.g., QEMU) may outlive the hostagent. Force-stop
		// the instance so the next hostagent can start with a clean slate.
		if inst.Status == limatype.StatusRunning || inst.Status == limatype.StatusBroken {
			logger.Info("Force-stopping orphaned VM driver", "status", inst.Status)
			stopInstanceForcibly(ctx, logger, inst)
		}
		return r.startInstance(ctx, limaVM, inst)

	switch inst.Status {
	case limatype.StatusRunning, limatype.StatusBroken:
		// Orphaned hostagent from before controller restart. Kill it so the
		// next reconcile can start with a watcher.
		logger.Info("Found orphaned hostagent, killing it", "status", inst.Status)
		if err := r.killOrphanedHostagent(ctx, inst); err != nil {
			logger.Error(err, "Failed to kill orphaned hostagent")
			return ctrl.Result{}, err
		}
		return ctrl.Result{RequeueAfter: time.Second}, nil

This PR changed line 211 from limainstance.StopForcibly (which used GenerateConsoleCtrlEvent, scoped to the console group) to stopInstanceForcibly (which uses taskkill /F /T /PID, system-wide). On Windows, if a StatusBroken instance has stale PID files and the PIDs have been recycled, taskkill will terminate unrelated processes and their children.

The practical likelihood is low — PID recycling must happen in the window between hostagent death and controller reconciliation — but the consequence (killing arbitrary processes) is severe enough to warrant the same defensive treatment as the deletion path.

Fix: Apply the same pattern used in handleDeletion — skip PID-based kills for StatusBroken on Windows, falling back to WSL2 distro termination by name:

 // handleWatchedState, line 209-211:
 if inst.Status == limatype.StatusRunning || inst.Status == limatype.StatusBroken {
     logger.Info("Force-stopping orphaned VM driver", "status", inst.Status)
-    stopInstanceForcibly(ctx, logger, inst)
+    if inst.Status == limatype.StatusBroken && runtime.GOOS == "windows" {
+        // Broken instances may have stale PID files pointing to recycled
+        // processes. Skip PID-based kill; terminate WSL2 distro by name.
+        if inst.VMType == limatype.WSL2 {
+            terminateWSL2Distro(ctx, logger, inst.Name)
+        }
+    } else {
+        stopInstanceForcibly(ctx, logger, inst)
+    }
 }

Apply the same guard in killOrphanedHostagent (line 557-571): skip process.Interrupt and stopInstanceForcibly for StatusBroken on Windows.

After KillTree at line 516 kills the hostagent, store.Inspect at line 525 reads PID files that have not been cleaned up yet. Between the kill and the inspect, PIDs could theoretically be recycled on Windows. stopInstanceForcibly at line 527 would then call taskkill on unrelated processes. The KillTree at line 516 is also redundant — stopInstanceForcibly already calls KillTree on both DriverPID and HostAgentPID.

		select {
		case <-state.procExited:
		case <-time.After(gracefulShutdownTimeout):
			graceful = false
			// The manager context is cancelled; use background context for
			// post-shutdown cleanup.
			if state.cmd != nil && state.cmd.Process != nil {
				killCtx, killCancel := context.WithTimeout(context.Background(), 5*time.Second)
				_ = process.KillTree(killCtx, state.cmd.Process.Pid)
				killCancel()
			}
			<-state.procExited
		}
		if !graceful {
			// Manager context is cancelled; use a fresh context for cleanup.
			logger := ctrl.Log.WithName("shutdownAllHostagents")
			cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
			inst, err := store.Inspect(cleanupCtx, name)
			if err == nil && inst != nil {
				stopInstanceForcibly(cleanupCtx, logger, inst)
			}
			cancel()
		}

Fix: Remove the redundant KillTree call and let stopInstanceForcibly handle the full kill-and-cleanup sequence:

 		case <-time.After(gracefulShutdownTimeout):
-			// The manager context is cancelled; use background context for
-			// post-shutdown cleanup.
-			if state.cmd != nil && state.cmd.Process != nil {
-				killCtx, killCancel := context.WithTimeout(context.Background(), 5*time.Second)
-				_ = process.KillTree(killCtx, state.cmd.Process.Pid)
-				killCancel()
-			}
-			<-state.procExited
-		}
-		if !graceful {
-			// Manager context is cancelled; use a fresh context for cleanup.
 			logger := ctrl.Log.WithName("shutdownAllHostagents")
 			cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 			inst, err := store.Inspect(cleanupCtx, name)
 			if err == nil && inst != nil {
 				stopInstanceForcibly(cleanupCtx, logger, inst)
 			}
 			cancel()
+			<-state.procExited
 		}