Deep Review: 20260422-170229-pr-336

// assumes the instance exists. If the service is running, Delete waits up to
// timeout for it to exit before removing files; removing instance.Dir() while
// the serve process holds rdd.pid, rdd.sqlite3, and log files corrupts the
// directory on Windows and breaks PID-file mutual exclusion on Unix.
// Pass timeout 0 to wait indefinitely (bounded only by ctx).
func Delete(ctx context.Context, timeout time.Duration) error {
	if Running() {
		if err := StopWithWait(ctx, true, timeout); err != nil {
			// Return the error whenever the process is alive or the
			// deadline expired, so deletion never races a live daemon
			// and the CLI exits 4 on timeout per docs/design/cmd_service.md.
			// The predicate also absorbs signal-delivery failures and
			// context cancellation when the process has already exited;
			// the invariant is "the directory survives unless the daemon
			// is confirmed gone." context.Canceled is unreachable today
			// because the CLI runs on context.Background(); wiring SIGINT
			// into cmd.Context() would let Ctrl-C fall through to deletion
			// during a live stop, and needs an explicit decision here.
			if errors.Is(err, context.DeadlineExceeded) || Running() {
				return err
			}
		}
	}
	preserveAllInstanceLogs()
	if os.Getenv("RDD_KEEP_LOGS") == "" {
		_ = os.RemoveAll(instance.LogDir())
	}
	_ = os.RemoveAll(instance.ShortDir())

When the deadline fires inside StopWithWait, the wait loop's ctx.Done branch calls process.Kill(pid) and returns DeadlineExceeded. The Kill often takes effect immediately (Unix apiserver's SetupSignalContext exits on the second signal; Windows TerminateProcess blocks until the process is gone). Delete's outer check then short-circuits on errors.Is(DeadlineExceeded) before Running() has a chance to reflect the post-Kill state, so a user running rdd svc delete --timeout=30s against a slow-to-drain daemon sees exit 4 and an intact directory even though the daemon is gone — they must re-run to finish.

Trade-off: Claude's proposed swap (check Running() only, drop the DeadlineExceeded short-circuit) silently succeeds on a timed-out-but-Kill-succeeded delete, contradicting the documented "deadline expires → exit 4" contract. The cleaner fix is inside StopWithWait's wait loop (see S2) — if the wait loop returns nil when the process is already gone by the time ctx.Done fires, Delete's outer predicate never sees DeadlineExceeded in this scenario and the contract stays intact.

		ctx, cancel := watchtools.ContextWithOptionalTimeout(ctx, timeout)
		defer cancel()
		ticker := time.NewTicker(100 * time.Millisecond)
		defer ticker.Stop()

		for {
			select {
			case <-ctx.Done():
				// Deadline fired or caller cancelled; terminate either way
				// to ensure the service process exits. Kill targets the
				// service alone: on Windows (TerminateProcess) it spares
				// hostagents, which live in their own process groups. On
				// Unix, Kill delivers SIGTERM, which is the second
				// shutdown signal after the earlier SIGINT; the apiserver's
				// SetupSignalContext handler responds to a second signal
				// with os.Exit(1), forcing immediate exit. The daemon also
				// caps its own drain at 45s before returning from Run, so
				// timeout values shorter than stopWaitTimeout (5m) routinely
				// race a still-draining daemon.
				_ = process.Kill(pid)
				err := ctx.Err()
				if errors.Is(err, context.DeadlineExceeded) {
					return fmt.Errorf("timed out waiting for %q control plane with pid %d to stop: %w", instance.Name(), pid, err)
				}
				return fmt.Errorf("wait for %q control plane with pid %d to stop cancelled: %w", instance.Name(), pid, err)
			case <-ticker.C:
				if !Running() {
					_ = os.Remove(instance.Config()) // Ignore error as file might not exist
					return nil
				}
			}
		}
	}

	// --wait=false: the daemon may still be serving for tens of seconds.
	// Leave instance.Config() alone so concurrent `rdd ctl`/`rdd kubectl`
	// calls keep working; the next `Start` rewrites it.

Go selects randomly when both ctx.Done() and ticker.C are ready in the same tick. A graceful shutdown that lands right on the deadline can take the ctx.Done branch even though !Running() is already true, returning DeadlineExceeded (exit 4). Re-running the command succeeds, so the symptom is minor user friction — but the race also feeds S1.

Fix: double-check Running() in the ctx.Done branch before declaring a timeout:

     case <-ctx.Done():
+        if !Running() {
+            _ = os.Remove(instance.Config())
+            return nil
+        }
         _ = process.Kill(pid)
         err := ctx.Err()

This also resolves S1 without changing Delete's outer predicate.

	if !service.Exists() {
		if err := service.Create(nil); err != nil {
			return err
		}
	}
	if service.Running() {
		startTime, err := service.StartTime()
		if err != nil {
			return err
		}
		ctx, cancel := watchtools.ContextWithOptionalTimeout(ctx, startWaitTimeout)
		defer cancel()
		if err := service.Wait(ctx); err != nil {
			return cliexit.Classify(err)
		}

Running() calls PID(), which removes a stale PID file when the process check fails (pkg/service/cmd/service.go:158). Between Running() returning true and StartTime() calling os.Stat(instance.PIDFile()), the daemon can exit and a second PID() invocation can remove the file. StartTime() then returns fs.ErrNotExist, which propagates as an opaque error (not Classify-able, not actionable) from an unrelated-looking call like rdd ctl get. The window is microseconds, but once a user hits it the diagnosis is hard.

			}
			_ = process.Release()
		}
	}
	if err != nil {
		_ = os.Remove(instance.PIDFile())
		return PIDNotFound
	}
	return pid
}

Round 7 S2 flagged the race in serviceStartAction only; this round notes the same pattern in ensureServiceRunning too.

Fix: treat a missing PID file as "daemon exited between the two checks" at both call sites. Two options:

 startTime, err := service.StartTime()
-if err != nil {
+if errors.Is(err, fs.ErrNotExist) {
+    return cliexit.Classify(startAndWaitForReady(ctx, nil, startWaitTimeout))
+}
+if err != nil {
     return err
 }

Or fold the retry inside StartTime() so every caller gets consistent handling.

	if err != nil {
		logrus.WithError(err).Debug("Could not create kubernetes client for discovery cleanup")
		return nil // Not a critical error during shutdown
	}

	err = controllers.CleanupDiscovery(context.TODO(), client)
	if err != nil {
		logrus.WithError(err).Debug("Failed to delete discovery configmap")
	}

	return nil // Don't fail stop operation due to discovery cleanup issues

The --timeout envelope does not bound the pre-shutdown discovery-cleanup RPC. If the apiserver is wedged or a connection stalls, rdd svc stop --timeout=1m / rdd svc delete --timeout=5m can hang indefinitely before the timed wait begins, contradicting docs/design/cmd_service.md:117-121 which frames --timeout as bounding the whole operation.

Stops the control plane and removes the instance directory, the short directory
(which contains the Lima home), and — unless `RDD_KEEP_LOGS` is set — the log
directory.

Delete always waits for the control plane to exit before removing files, because
removing the instance directory under a live process corrupts it on Windows and
breaks PID-file mutual exclusion on Unix. Use `--timeout` to bound that wait
(default `5m`); `0` waits indefinitely. If the deadline expires, `rdd` exits
with code 4 and the directory is left in place. See [`rdd service stop`](#rdd-service-stop)
for the signal and fallback details of the shutdown itself.

### `rdd service reset`

Deletes the datastore, but create a new (empty) one with the same settings.

Round 7 S6 raised the same three concerns (unbounded RPC, context.TODO, deferred durable fix) as a Suggestion, and the author skipped it pending the discovery-CM-cleanup pending-work item. The code comment at lines 353-357 documents moving cleanup into the daemon as the durable fix, so this stays a Suggestion rather than an Important issue — but threading the caller's ctx through cleanupDiscoveryConfigMap(ctx) is a one-line interim fix that makes the --timeout contract honest today:

func StopWithWait(ctx context.Context, wait bool, timeout time.Duration) error {
	if !Running() {
		return fmt.Errorf("%q control plane is not running", instance.Name())
	}

	// Delete the discovery configmap before signaling so the kube-API
	// delete still has a live apiserver. Trade-off: a timed-out --wait
	// leaves the configmap deleted while the daemon drains, so a quick
	// `rdd svc start` polls the missing configmap until startWaitTimeout.
	// Moving cleanup into the daemon's own shutdown path is the durable fix.
	_ = cleanupDiscoveryConfigMap()

	pid := PID()
	// Try graceful shutdown first. On Unix, Kill already sends SIGTERM which
	// triggers the Go signal handler. On Windows, Kill uses TerminateProcess

-func cleanupDiscoveryConfigMap() error {
+func cleanupDiscoveryConfigMap(ctx context.Context) error {
     ...
-    err = controllers.CleanupDiscovery(context.TODO(), client)
+    err = controllers.CleanupDiscovery(ctx, client)

Codex raised this as Important. Per the repo-context rule about deliberate code-comment trade-offs, demoted to Suggestion.

# SIGTERM (or TerminateProcess on Windows) and returns
# context.DeadlineExceeded wrapped in cliexit.Timeout. svc delete also
# preserves the instance directory on timeout (see docs/design/cmd_service.md)
# so the user can retry.

local_setup_file() {
    rdd svc delete --timeout=120s
    rdd svc create
    rdd svc start
}

@test 'svc stop --timeout=1ms exits with code 4' {
    run -4 rdd svc stop --wait --timeout=1ms
    assert_output --partial 'context deadline exceeded'
}

bats/helpers/controller.bash:setup_rdd_control_plane does exactly this (and its 120s bound carries a detailed comment explaining why). Using the helper keeps the bounded-delete rationale in one place and inherits future updates automatically.

 local_setup_file() {
-    rdd svc delete --timeout=120s
-    rdd svc create
-    rdd svc start
+    setup_rdd_control_plane
 }

    run -0 rdd svc status
    assert_output --partial 'has been created: true'
}

@test 'svc start --timeout=1ms exits with code 4' {
    # Reset to a known state; prior tests left the control plane half-dead.
    rdd svc delete --timeout=120s
    rdd svc create
    run -4 rdd svc start --wait --timeout=1ms
    assert_output --partial 'context deadline exceeded'
}

If the prior svc delete --timeout=1ms test left a daemon that refuses to die within 120s, this line exits 4 and BATS's set -e aborts the test with the same message context deadline exceeded — indistinguishable from the failure being tested. Test 2 uses || : on its reset for exactly this reason.

Fix: add || : to match test 2, or skip the teardown entirely by reusing test 2's verified post-state.

		Args:  cobra.ExactArgs(1),
		RunE: func(cmd *cobra.Command, args []string) error {
			return limaVMDeleteAction(cmd.Context(), args[0], wait, timeout)
		},
	}
	command.Flags().BoolVar(&wait, "wait", true, "Wait for the VM to be deleted before returning")
	command.Flags().DurationVar(&timeout, "timeout", limaLongWaitTimeout, "Timeout for --wait; ignored if --wait=false (0 means wait indefinitely)")
	return command
}

// getKubeClient returns a controller-runtime client configured for the RDD control plane.

Every --timeout flag added by this PR carries the "ignored if --wait=false" nudge. The --wait flag itself stays terse across all limavm subcommands. Not a bug; minor wording polish for symmetry.

// controller-manager registration to land. Cold start can take well over 30s
// in CI on slower runners.
const startWaitTimeout = 90 * time.Second

// stopWaitTimeout bounds the CLI wait for the control plane to shut down.
// Run caps the internal drain at 45s (see pkg/service/cmd/service.go).
// The CLI deadline exceeds that cap to absorb slow disks, misconfigured
// hosts, and sequential multi-VM drains. Aliased to limaLongWaitTimeout
// (defined in limavm.go) so the per-VM ceiling stays the single tuning
// knob for long CLI waits.
const stopWaitTimeout = limaLongWaitTimeout

The "see pkg/service/cmd/service.go" pointer is accurate but vague — Run lives in that file at line 692, and the 45s cap is at line 859. Naming the function (Run) in the comment helps a reader find the cross-reference in one step.

The paragraph documents the directory-preservation invariant ("the directory is left in place") but not the parallel effect: a timed-out delete (like a timed-out stop) wipes the discovery ConfigMap while the daemon continues draining, so a follow-up command that auto-starts the service polls the missing ConfigMap until startWaitTimeout (90s). The code comment at pkg/service/cmd/service.go:353-357 calls this out; the user-facing doc does not.

func StopWithWait(ctx context.Context, wait bool, timeout time.Duration) error {
	if !Running() {
		return fmt.Errorf("%q control plane is not running", instance.Name())
	}

	// Delete the discovery configmap before signaling so the kube-API
	// delete still has a live apiserver. Trade-off: a timed-out --wait
	// leaves the configmap deleted while the daemon drains, so a quick
	// `rdd svc start` polls the missing configmap until startWaitTimeout.
	// Moving cleanup into the daemon's own shutdown path is the durable fix.
	_ = cleanupDiscoveryConfigMap()

	pid := PID()
	// Try graceful shutdown first. On Unix, Kill already sends SIGTERM which
	// triggers the Go signal handler. On Windows, Kill uses TerminateProcess

Fix: add one sentence to the delete section noting the discovery-ConfigMap side-effect, or cross-reference the svc stop doc if that gains a matching note.