Deep Review: 20260323-003519-pr-242

The old code sent SIGTERM via process.Kill. The new code sends SIGINT via process.Interrupt, falling back to Kill only on error. Both signals are handled identically by the apiserver signal handler (which registers both os.Interrupt and syscall.SIGTERM), so this is functionally correct. On Unix, process.Interrupt (which sends SIGINT via syscall.Kill) always succeeds for a valid PID, so the Kill fallback never fires. The fallback exists for Windows, where GenerateConsoleCtrlEvent can fail cross-console. The asymmetry deserves a comment.

	pid := PID()
	// Try graceful shutdown first. On Unix, Kill already sends SIGTERM which
	// triggers the Go signal handler. On Windows, Kill uses TerminateProcess
	// which bypasses all handlers, so we send Interrupt (CTRL_BREAK_EVENT)
	// first to let the service run its shutdown path (shutdownAllHostagents).
	//
	// On Windows, Interrupt uses GenerateConsoleCtrlEvent, which only works
	// when caller and target share a console. If invoked from a different
	// terminal, the call fails and we fall back to Kill (TerminateProcess),
	// bypassing graceful shutdown. Hostagents survive in their own process
	// groups and self-heal on the next service start via killOrphanedHostagent.
	if err := process.Interrupt(pid); err != nil {
		if err := process.Kill(pid); err != nil {
			return fmt.Errorf("failed to stop %q control plane with pid %d: %w", instance.Name(), pid, err)
		}
	}

Fix:

+	// On Unix, Interrupt (SIGINT) always succeeds for a valid PID; the Kill
+	// fallback fires only on Windows when caller and target lack a shared console.
 	if err := process.Interrupt(pid); err != nil {

When signalHostagent returns false (no watcher), killHostagent is also a no-op (same nil check). The message "killing process directly" is misleading; the actual work happens in forceStop() via stopInstanceForcibly.

		logger.Info("Hostagent did not exit gracefully, forcing stop")
			forceStop()
			waitAfterKill()
		}
	} else {
		logger.Info("Could not signal hostagent, killing process directly")
		r.killHostagent(name)
		forceStop()
		waitAfterKill()
	}
}

Fix:

-		logger.Info("Could not signal hostagent, killing process directly")
-		r.killHostagent(name)
+		logger.Info("No watcher for hostagent, forcing stop via stored PIDs")
+		r.killHostagent(name) // no-op if watcher absent; forceStop uses stored PIDs

vm_template_running passes through RDD_VM_TYPE only on Unix (line 88). On Windows, _wsl2_template always sets vmType: wsl2 and ignores the variable.

# Lima VM template selection for BATS tests.
#
# On Windows (WSL2), set RDD_WSL_DISTRO to choose between distros:
#   - "finch"    (default): Finch rootfs (Fedora-based, stable for testing)
#   - "opensuse":           rancher-desktop-opensuse (the target distro for RD2)
#
# Usage in test files:
#   VM_TEMPLATE=$(vm_template)         # for limavm-instance tests
#   VM_TEMPLATE=$(vm_template_running) # for limavm-running tests (supports RDD_VM_TYPE)
#
: "${RDD_WSL_DISTRO:=finch}"

Fix:

-#   VM_TEMPLATE=$(vm_template_running) # for limavm-running tests (supports RDD_VM_TYPE)
+#   VM_TEMPLATE=$(vm_template_running) # for limavm-running tests (supports RDD_VM_TYPE on Unix)

The helper uses a fixed temp path (user.tmp) without an inter-process lock, unlike Lima's upstream DefaultPubKeys() which wraps generation in a config-dir lock. If two controller processes race through startup, one can delete the other's temp files. In practice, only one lima-controller runs per instance, so this race is unlikely. The code already handles crash recovery (orphaned .tmp files) correctly.

	// Remove orphaned public key (e.g. from a previous partial rename
	// failure). On Windows, os.Rename fails if the destination exists.
	_ = os.Remove(pubPath)
	configDir := filepath.Dir(privPath)
	if err := os.MkdirAll(configDir, 0o700); err != nil {
		return fmt.Errorf("could not create %q: %w", configDir, err)
	}
	// Generate into a temporary path and rename on success, so an
	// interrupted ssh-keygen does not leave a partial key that blocks
	// future attempts.
	tmpPath := privPath + ".tmp"
	// Clean up stale temp files from a prior crash so ssh-keygen does not
	// prompt "Overwrite (y/n)?" and hang waiting for a TTY that doesn't exist.
	_ = os.Remove(tmpPath)
	_ = os.Remove(tmpPath + ".pub")
	cmd := exec.CommandContext(ctx, "ssh-keygen", "-t", "ed25519", "-q", "-N", "",
		"-C", "lima", "-f", tmpPath)
	if out, err := cmd.CombinedOutput(); err != nil {
		// Clean up any partial files left by ssh-keygen.
		_ = os.Remove(tmpPath)
		_ = os.Remove(tmpPath + ".pub")
		return fmt.Errorf("failed to generate SSH key: %s: %w", out, err)
	}
	if err := os.Rename(tmpPath+".pub", pubPath); err != nil {
		_ = os.Remove(tmpPath)
		_ = os.Remove(tmpPath + ".pub")
		return fmt.Errorf("failed to rename public key: %w", err)

Fix: Consider wrapping the generate/rename section in lockutil.WithDirLock(configDir, ...), or document why single-controller-per-instance makes locking unnecessary.

The main RDD service uses logrus, which supports warn, warning, error, and fatal. Setting RDD_LOG_LEVEL=error (valid for the service) triggers four warning messages to stderr — one per spawned external controller. Since klog has no equivalent granularity, mapping these levels to the default info verbosity is correct, but the warning is noise.

func setVerbosityFromEnv() {
	level := strings.TrimSpace(os.Getenv("RDD_LOG_LEVEL"))
	switch strings.ToLower(level) {
	case "trace":
		if err := flag.Set("v", "4"); err != nil {
			fmt.Fprintf(os.Stderr, "failed to set klog verbosity: %v\n", err)
		}
	case "debug":
		if err := flag.Set("v", "2"); err != nil {
			fmt.Fprintf(os.Stderr, "failed to set klog verbosity: %v\n", err)
		}
	case "", "info":
		// info is the default klog verbosity; nothing to change.
	default:
		// klog has no warn/error levels; unsupported values fall through to
		// the default verbosity (v=0), which is equivalent to info-only.
		fmt.Fprintf(os.Stderr, "RDD_LOG_LEVEL=%q is not supported for klog; valid values are trace, debug, info\n", level)
	}
}

Fix: Accept valid logrus levels silently:

-	case "", "info":
+	case "", "info", "warn", "warning", "error", "fatal":
 		// info is the default klog verbosity; nothing to change.
+		// klog cannot suppress info-level messages, so higher logrus levels
+		// fall through to the default verbosity (v=0).
 	default: