Deep Review: 20260325-122724-pr-234

If one file fails to rename (e.g., held open by a lingering process on Windows, or due to an EXDEV cross-device link error), the function returns immediately and skips all remaining files. For a best-effort debugging aid gated behind an environment variable, aggregating errors and preserving as many files as possible is more useful.

	count := 0
	for _, entry := range logFiles {
		src := filepath.Join(srcDir, entry.Name())
		dst := filepath.Join(destDir, entry.Name())
		if err := os.Rename(src, dst); err != nil {
			return count, fmt.Errorf("preserve %s: %w", entry.Name(), err)
		}
		count++
	}

Fix: Collect errors and return errors.Join(errs...) at the end.

+	var errs []error
 	count := 0
 	for _, entry := range logFiles {
 		src := filepath.Join(srcDir, entry.Name())
 		dst := filepath.Join(destDir, entry.Name())
 		if err := os.Rename(src, dst); err != nil {
-			return count, fmt.Errorf("preserve %s: %w", entry.Name(), err)
+			errs = append(errs, fmt.Errorf("preserve %s: %w", entry.Name(), err))
+			continue
 		}
 		count++
 	}

-	return count, nil
+	return count, errors.Join(errs...)

Delete() removes ShortDir() (which contains LimaHome) with a raw os.RemoveAll. The normal LimaVM deletion path goes through limainstance.Delete, which lets the Lima driver run platform-specific cleanup (e.g., WSL2 distro unregister on Windows). Stop() at line 402 stops the service, but controllers may not complete full instance cleanup before returning. On macOS (VZ driver) this is harmless — VMs are just files. On Windows/WSL2, it could leave registered distros behind.

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())
}

This is future-facing: the project has no Windows support yet. Worth a comment documenting the limitation. If Windows support lands before this is revisited, add Lima-level instance deletion before the RemoveAll.

New code in logs.go:69 uses the modern errors.Is(err, fs.ErrExist), while service.go:420 uses the legacy os.IsNotExist. Both work identically for *os.PathError, but mixing styles within a PR is a minor inconsistency.

	entries, err := os.ReadDir(instance.LimaHome())
	if err != nil {
		if !os.IsNotExist(err) {
			logrus.WithError(err).Warn("Failed to read Lima instance directory")
		}
		return
	}

Fix: Use errors.Is(err, fs.ErrNotExist) at line 420 to match logs.go. Or leave it — this matches the existing pattern in service.go.

Unlike Create (which calls os.MkdirAll), Rotate assumes the directory exists. Current callers satisfy this, but the exported API does not document the precondition.

// Rotate renames {name}.log to {name}.{N}.log without creating a new file.
// Use this for logs managed by an external process (e.g., VM serial console
// output written by the VM driver) that would be overwritten on next start.
//
// When keepAll is false, old numbered files beyond the retention count
// are removed.
func Rotate(dir, name string, keepAll bool) error {
	pattern := regexp.MustCompile(`^` + regexp.QuoteMeta(name) + `\.(\d+)\.log$`)

	entries, err := os.ReadDir(dir)
	if err != nil {
		return fmt.Errorf("read log directory %s: %w", dir, err)
	}

Fix: Add "The directory must already exist" to the docstring.

 // Rotate renames {name}.log to {name}.{N}.log without creating a new file.
+// The directory must already exist (unlike Create, which creates it).
 // Use this for logs managed by an external process ...