Deep Review: 20260319-173741-msys2-ci

Every error is discarded: process kill (line 542), WSL2 terminate (line 553), and file removal (line 560). A failed wsl.exe --terminate or process kill produces no log entry. The function is a standalone helper with no logger. (suggestion, gap)

func stopInstanceForcibly(ctx context.Context, inst *limatype.Instance) {
	for _, pid := range []int{inst.DriverPID, inst.HostAgentPID} {
		if pid > 0 {
			if p, err := os.FindProcess(pid); err == nil {
				_ = p.Kill()
			}
		}
	}
	// On WSL2, terminate the distro so store.Inspect reports StatusStopped.
	if inst.VMType == limatype.WSL2 {
		distroName := "lima-" + inst.Name
		// Best-effort with a timeout: wsl.exe can hang if the WSL
		// subsystem is degraded; don't block the reconciler.
		wslCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
		defer cancel()
		_ = exec.CommandContext(wslCtx, "wsl.exe", "--terminate", distroName).Run()
	}
	// Clean up PID/socket/tmp files so the next hostagent can start cleanly.
	// This matches what limainstance.StopForcibly does.
	for _, suffix := range filenames.TmpFileSuffixes {
		matches, _ := filepath.Glob(filepath.Join(inst.Dir, "*"+suffix))
		for _, m := range matches {
			_ = os.Remove(m)
		}
	}
}

Fix: Accept a logr.Logger parameter and log failed cleanup steps at V(1). Alternatively, make it a method on LimaVMReconciler to access the logger from context.

Both the Windows and Unix implementations overwrite cmd.SysProcAttr entirely. If a future caller sets other process attributes before calling SetGroup, those settings are silently lost. Currently, the only call site (startInstance at line 423) creates a fresh exec.Cmd, so the overwrite is safe. (suggestion, gap)

// SetGroup configures the command to run in its own process group.
// On Windows, CREATE_NEW_PROCESS_GROUP allows GenerateConsoleCtrlEvent to
// target only the child process (using its PID as the group ID) without
// affecting the parent process.
func SetGroup(cmd *exec.Cmd) {
	cmd.SysProcAttr = &windows.SysProcAttr{CreationFlags: windows.CREATE_NEW_PROCESS_GROUP}
}

Fix: Initialize SysProcAttr only if nil, then merge flags:

if cmd.SysProcAttr == nil {
    cmd.SysProcAttr = &windows.SysProcAttr{}
}
cmd.SysProcAttr.CreationFlags |= windows.CREATE_NEW_PROCESS_GROUP

Both functions implement the same signal-wait-forcestop pattern. The two implementations differ slightly (handleRestartNeeded re-inspects via store.Inspect before stopInstanceForcibly; stopInstance reuses its inst parameter). As both grow, they risk diverging. (suggestion, gap)

		if r.signalHostagent(limaVM.Name) {
			stopCtx, cancel := context.WithTimeout(ctx, gracefulShutdownTimeout)
			defer cancel()
			if !r.waitForProcessExit(stopCtx, limaVM.Name) {
				logger.Info("Hostagent did not exit gracefully, forcing stop")
				inst, err := store.Inspect(ctx, limaVM.Name)
				if err != nil {
					logger.Error(err, "Failed to inspect Lima instance for forceful stop")
				} else if inst != nil {
					stopInstanceForcibly(ctx, inst)
				}
				r.waitForProcessExit(ctx, limaVM.Name)
			}
		} else {
			// Signal delivery failed (e.g. process already exited or no console).
			logger.Info("Could not signal hostagent for restart, killing process directly")
			r.killHostagent(limaVM.Name)
			r.waitForProcessExit(ctx, limaVM.Name)
			inst, err := store.Inspect(ctx, limaVM.Name)
			if err != nil {
				logger.Error(err, "Failed to inspect Lima instance for forceful stop")
			} else if inst != nil {
				stopInstanceForcibly(ctx, inst)
			}
		}

	if r.signalHostagent(limaVM.Name) {
		// Signal delivered; wait for graceful exit before falling back to force-stop.
		stopCtx, cancel := context.WithTimeout(ctx, gracefulShutdownTimeout)
		defer cancel()
		if !r.waitForProcessExit(stopCtx, limaVM.Name) {
			logger.Info("Hostagent did not exit gracefully, forcing stop")
			stopInstanceForcibly(ctx, inst)
			r.waitForProcessExit(ctx, limaVM.Name)
		}
	} else {
		// Signal delivery failed (e.g. process already exited or no console).
		// Kill the hostagent and force-stop the instance to clean up the
		// VM driver and tmp files.
		logger.Info("Could not signal hostagent, killing process directly")
		r.killHostagent(limaVM.Name)
		r.waitForProcessExit(ctx, limaVM.Name)
		stopInstanceForcibly(ctx, inst)
	}

Fix: Extract a shutdownHostagent(ctx, name, inst) helper that encapsulates the signal-or-kill-then-cleanup pattern.

The previous implementation tried StopGracefully (SIGTERM + 30s wait) before falling back to StopForcibly. The new code goes straight to hard kill. On Windows, the old graceful path was broken (os.Process.Signal(os.Interrupt) always fails), so this is a net improvement there. On Unix, orphaned hostagents lose their chance to shut down the VM driver gracefully. For post-crash recovery, speed matters more than cleanliness, so the trade-off is reasonable. (suggestion, regression)

// killOrphanedHostagent terminates an orphaned hostagent (one running without a
// watcher, typically after a controller restart).
func (r *LimaVMReconciler) killOrphanedHostagent(ctx context.Context, inst *limatype.Instance) error {
	stopInstanceForcibly(ctx, inst)
	return nil
}

ensureSSHKeys has a check-then-act race: two controllers starting simultaneously against the same Lima config dir can both observe "key missing" and race ssh-keygen. Lima upstream uses lockutil.WithDirLock to serialize this. In practice, the window is narrow and ssh-keygen prompts to overwrite (failing without a tty), so the second instance would return an error and prevent startup. (suggestion, regression)

func ensureSSHKeys() error {
	configDir, err := dirnames.LimaConfigDir()
	if err != nil {
		return err
	}
	privPath := filepath.Join(configDir, filenames.UserPrivateKey)
	if _, err := os.Stat(privPath); err == nil {
		return nil
	}
	if err := os.MkdirAll(configDir, 0o700); err != nil {
		return fmt.Errorf("could not create %q: %w", configDir, err)
	}
	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	defer cancel()
	cmd := exec.CommandContext(ctx, "ssh-keygen", "-t", "ed25519", "-q", "-N", "",
		"-C", "lima", "-f", privPath)
	if out, err := cmd.CombinedOutput(); err != nil {
		return fmt.Errorf("failed to generate SSH key: %s: %w", out, err)
	}
	return nil
}

The call site in SetupWithManager:

// SetupWithManager sets up the controller with the Manager.
func (r *LimaVMReconciler) SetupWithManager(mgr ctrl.Manager) error {
	if err := ensureSSHKeys(); err != nil {
		return fmt.Errorf("failed to generate Lima SSH keys: %w", err)
	}

Fix: Wrap the ssh-keygen call in Lima's lockutil.WithDirLock(configDir, ...).