Deep Review: 20260321-135724-pr-242

If the controller crashes during ssh-keygen execution, the .tmp file persists. On next start, ssh-keygen detects the existing file and prompts Overwrite (y/n)?. Running without a TTY, it hangs until the 30-second timeout kills it. The function returns an error, SetupWithManager fails at line 452, and the lima-controller cannot start. Every subsequent restart hits the same hang until someone manually deletes the .tmp file.

	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"
	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	defer cancel()
	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)
	}

Claude rated this Suggestion; Gemini rated it Critical. The trigger requires a crash during the ~1-second keygen window on first run (before keys exist). The impact is indefinite: the controller blocks on every startup until manual intervention. Important balances the narrow trigger against the persistent impact.

Fix: Clean up stale temp files before running ssh-keygen, and use defer for the post-failure cleanup.

 	tmpPath := privPath + ".tmp"
+	_ = os.Remove(tmpPath)
+	_ = os.Remove(tmpPath + ".pub")
+	defer os.Remove(tmpPath)
+	defer os.Remove(tmpPath + ".pub")
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()
 	cmd := exec.CommandContext(ctx, "ssh-keygen", "-t", "ed25519", "-q", "-N", "",
 	    "-C", "lima", "-f", tmpPath)
 	if out, err := cmd.CombinedOutput(); err != nil {
-	    _ = os.Remove(tmpPath)
-	    _ = os.Remove(tmpPath + ".pub")
 	    return fmt.Errorf("failed to generate SSH key: %s: %w", out, err)
 	}

When only the public key is missing (external deletion, filesystem error, antivirus quarantine), the code deletes the valid private key at line 38 and regenerates both. The guest VM's authorized_keys still trusts the old public key, so the new keypair breaks SSH access to existing VMs. A missing public key is recoverable: ssh-keygen -y -f <privPath> derives the public key from the private key.

func ensureSSHKeys() error {
	configDir, err := dirnames.LimaConfigDir()
	if err != nil {
		return err
	}
	privPath := filepath.Join(configDir, filenames.UserPrivateKey)
	pubPath := privPath + ".pub"
	if _, err := os.Stat(privPath); err == nil {
		if _, err := os.Stat(pubPath); err == nil {
			return nil
		}
		// Private key exists without public key — corrupt keypair.
		// Remove and regenerate.
		_ = os.Remove(privPath)
	}
	// Remove orphaned public key (e.g. from a previous partial rename
	// failure). On Windows, os.Rename fails if the destination exists.
	_ = os.Remove(pubPath)

ensureSSHKeys runs during SetupWithManager at line 452, so this executes on every controller startup. In practice, the trigger (pub key missing while priv key remains) is unusual, but the impact (VM unreachable over SSH) is severe.

Fix: Regenerate the public key from the existing private key instead of deleting both.

 if _, err := os.Stat(privPath); err == nil {
     if _, err := os.Stat(pubPath); err == nil {
         return nil
     }
-    // Private key exists without public key — corrupt keypair.
-    // Remove and regenerate.
-    _ = os.Remove(privPath)
+    // Derive the public key from the existing private key.
+    tmpPub := pubPath + ".tmp"
+    _ = os.Remove(tmpPub)
+    out, err := exec.Command("ssh-keygen", "-y", "-f", privPath).Output()
+    if err != nil {
+        return fmt.Errorf("failed to derive public key: %w", err)
+    }
+    if err := os.WriteFile(tmpPub, out, 0o644); err != nil {
+        return fmt.Errorf("failed to write public key: %w", err)
+    }
+    return os.Rename(tmpPub, pubPath)
 }

The code already documents this with a TODO at line 503. With N hung hostagents, the worst case is N × 30 seconds. Windows SCM and systemd enforce shutdown timeouts (typically 30–90 seconds), so with two or more struggling instances, the OS kills the service before it finishes iterating. Trailing VMs miss the stopInstanceForcibly sweep, leaving unmanaged WSL2 distros running.

	}

	// Wait for each hostagent process to exit. The watcher goroutine may finish
	// before the process (events.Watch returns on context cancel), so we wait on
	// procExited (closed by haCmd.Wait) rather than done (closed by the watcher).
	// TODO: Wait on all hostagents in parallel instead of sequentially; with the
	// current loop, the total wait is N × gracefulShutdownTimeout in the worst case.
	for name, state := range states {
		graceful := true
		select {
		case <-state.procExited:
		case <-time.After(gracefulShutdownTimeout):
			graceful = false
			if state.cmd != nil && state.cmd.Process != nil {
				_ = process.KillTree(context.Background(), state.cmd.Process.Pid)
			}
			<-state.procExited
		}
		if !graceful {
			// The manager context is cancelled, so use a fresh context for
			// store.Inspect and WSL2 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.cancel()
		r.instanceStatesMu.Lock()
		delete(r.instanceStates, name)
		r.instanceStatesMu.Unlock()
	}

Fix: Wait on all hostagents in parallel with a shared timeout, as the TODO suggests.

On Unix, process.Kill sends SIGTERM (line 32 of process_unix.go), which triggers Go's signal handler and runs waitForShutdown → shutdownAllHostagents. On Windows, process.Kill calls TerminateProcess (line 48 of process_windows.go), which kills the service immediately without running any Go shutdown code. Hostagents, now in their own process groups, survive as orphans.

// Kill terminates the process with the given PID.
func Kill(pid int) error {
	hProcess, err := windows.OpenProcess(
		windows.PROCESS_TERMINATE|windows.SYNCHRONIZE,
		false,
		uint32(pid))
	if err != nil {
		return fmt.Errorf("failed to open process %d: %w", pid, err)
	}
	defer func() {
		_ = windows.CloseHandle(hProcess)
	}()
	if err := windows.TerminateProcess(hProcess, 1); err != nil {
		return fmt.Errorf("failed to terminate process %d: %w", pid, err)
	}
	_, err = windows.WaitForSingleObject(hProcess, uint32(10*time.Second/time.Millisecond))
	if err != nil {
		return fmt.Errorf("timed out waiting for process %d to terminate: %w", pid, err)
	}

	return nil
}

The author documented this as known behavior in the BATS test at lines 342–345, with recovery via orphan detection on next startup. This is a gap in the new Windows support, not a regression in existing behavior — TerminateProcess always killed only the target process, and before this PR no hostagents ran on Windows.

@test "restart service after graceful shutdown" {
    rdd svc start

    # On Unix, shutdownAllHostagents ran during graceful shutdown, so the
    # hostagent is already dead. On Windows, TerminateProcess bypasses Go's
    # signal handlers, so the hostagent survives as an orphan and is cleaned
    # up by orphan recovery on the next start.
    rdd ctl await "limavm/${VM_NAME}" --namespace "${NAMESPACE}" \
        --for=condition=Running --reason=Started --since=startup --timeout=300s
}

Fix: Make rdd svc stop on Windows try process.Interrupt first (which sends CTRL_BREAK_EVENT, triggering the graceful shutdown path), then fall back to TerminateProcess only if the service does not exit within a timeout.