Deep Review: 20260325-180207-pr-234

Both preservation paths — preserveInstanceLogs in the controller and preserveAllInstanceLogs in the service — log failures as warnings, then allow the caller to delete the source directory. In the controller, preserveInstanceLogs returns void at line 714, and handleDeletion calls limainstance.Delete at line 82 regardless. In the service, preserveAllInstanceLogs continues to the next instance at line 454, and Delete() calls os.RemoveAll(instance.ShortDir()) at line 428.

// preserveInstanceLogs moves log files from the Lima instance directory to
// a subdirectory of the service log directory before the instance is deleted.
// This is a no-op unless RDD_KEEP_LOGS is set.
func preserveInstanceLogs(ctx context.Context, inst *limatype.Instance) {
	if os.Getenv("RDD_KEEP_LOGS") == "" {
		return
	}

	logger := log.FromContext(ctx)
	count, err := instance.PreserveLogs(inst.Dir, inst.Name)
	if err != nil {
		logger.Error(err, "Failed to preserve instance logs")
		return
	}
	if count > 0 {
		logger.Info("Preserved instance logs", "instance", inst.Name, "count", count)
	}
}

The handleDeletion flow calls preserveInstanceLogs at line 77, then unconditionally proceeds to limainstance.Delete at line 82:

	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.
			//
			// This disables Lima's internal kill retry even if stopInstanceForcibly
			// failed above. That is intentional: a failed kill means KillTree
			// could not reach the process (access denied, already reaped), and
			// the PID may already be recycled. Retrying with a stale PID is
			// worse than letting Delete proceed without a kill.
			existingInst.DriverPID = 0
			existingInst.HostAgentPID = 0
		}
		preserveInstanceLogs(ctx, existingInst)
		logger.Info("Deleting Lima instance", "instance", limaVM.Name)
		// Use a timeout because Lima's WSL2 driver calls wsl.exe --unregister
		// which can hang indefinitely if the WSL subsystem is degraded.
		deleteCtx, deleteCancel := context.WithTimeout(ctx, time.Minute)
		err = limainstance.Delete(deleteCtx, existingInst, true)
		deleteCancel()
		if err != nil {
			logger.Error(err, "Failed to delete Lima instance")
			return ctrl.Result{}, err
		}
		logger.Info("Deleted Lima instance", "instance", limaVM.Name)

In service.go, Delete() calls Stop(), then preserveAllInstanceLogs(), then removes the directories unconditionally:

func Delete() error {
	if !Exists() {
		return fmt.Errorf("%q control plane does not exist", instance.Name())
	}
	_ = Stop()
	preserveAllInstanceLogs()
	if os.Getenv("RDD_KEEP_LOGS") == "" {
		_ = os.RemoveAll(instance.LogDir())
	}
	_ = os.RemoveAll(instance.ShortDir())
	return os.RemoveAll(instance.Dir())
}

// preserveAllInstanceLogs moves .log files from each Lima instance directory
// to the service log directory before the instance directories are deleted.
// This is a no-op unless RDD_KEEP_LOGS is set.
func preserveAllInstanceLogs() {
	if os.Getenv("RDD_KEEP_LOGS") == "" {
		return
	}
	entries, err := os.ReadDir(instance.LimaHome())
	if err != nil {
		if !os.IsNotExist(err) {
			logrus.WithError(err).Warn("Failed to read Lima instance directory")
		}
		return
	}
	for _, entry := range entries {
		if !entry.IsDir() {
			continue
		}
		instDir := filepath.Join(instance.LimaHome(), entry.Name())
		count, err := instance.PreserveLogs(instDir, entry.Name())
		if err != nil {
			logrus.WithError(err).WithField("instance", entry.Name()).Warn("Failed to preserve instance logs")
			continue
		}
		if count > 0 {
			logrus.WithFields(logrus.Fields{"instance": entry.Name(), "count": count}).Info("Preserved instance logs")
		}
	}
}

On Windows, stopInstanceForcibly (line 55) and Stop() (line 423) terminate processes, but Windows does not release file locks until the process handle is fully closed. The subsequent os.Rename calls in PreserveLogs (logs.go:54) fail with ERROR_SHARING_VIOLATION, the error is logged, and the source logs are then deleted — the exact outcome this PR aims to prevent.

On Unix this race is unlikely because killed processes release file descriptors immediately, and os.Rename does not require exclusive access. The practical impact is limited to Windows CI, which is the PR author’s primary concern (issue #247).

Fix: In preserveInstanceLogs, return an error and let handleDeletion decide whether to retry or proceed. In Delete(), add a brief time.Sleep or poll for file accessibility after Stop() on Windows before calling preserveAllInstanceLogs. Alternatively, accept the best-effort design but document the Windows limitation with a TODO.

-func preserveInstanceLogs(ctx context.Context, inst *limatype.Instance) {
+func preserveInstanceLogs(ctx context.Context, inst *limatype.Instance) error {
     if os.Getenv("RDD_KEEP_LOGS") == "" {
-        return
+        return nil
     }
     logger := log.FromContext(ctx)
     count, err := instance.PreserveLogs(inst.Dir, inst.Name)
     if err != nil {
-        logger.Error(err, "Failed to preserve instance logs")
-        return
+        return fmt.Errorf("preserve instance logs: %w", err)
     }
     if count > 0 {
         logger.Info("Preserved instance logs", "instance", inst.Name, "count", count)
     }
+    return nil
 }