Deep Review: 20260422-125913-pr-336

// Delete removes all instance data. 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 !Exists() {
		return fmt.Errorf("%q control plane does not exist", instance.Name())
	}
	if Running() {
		if err := StopWithWait(ctx, true, timeout); err != nil {
			// Suppress only the outer/inner Running() race, where the
			// process exits between Delete's check and StopWithWait's.
			// Deadline expiry must surface so the CLI exits 4 and leaves
			// the directory in place, per docs/design/cmd_service.md.
			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 expires, StopWithWait at pkg/service/cmd/service.go:381 already calls process.Kill(pid), so the process is almost always dead before the error returns. Delete still short-circuits on the DeadlineExceeded arm and leaves the directory in place; the user runs rdd svc delete a second time, which succeeds because Running() is now false. The two-step UX is awkward given the kill usually took effect.

				// Windows (TerminateProcess) this avoids killing hostagents
				// that are children of the service but run in their own
				// process groups. On Unix, SIGTERM suffices because the
				// service is responsive to signals (it's just slow shutting
				// down hostagents sequentially).
				_ = process.Kill(pid)
				return fmt.Errorf("timed out waiting for %q control plane with pid %d to stop: %w", instance.Name(), pid, ctx.Err())
			case <-ticker.C:
				if !Running() {
					_ = os.Remove(instance.Config()) // Ignore error as file might not exist
					return nil

Consider re-checking Running() after the error and continuing with cleanup if the service has exited. Keep docs/design/cmd_service.md:117-122 in sync if you change the behaviour.

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.

(suggestion, enhancement)

6-timeout.bats covers svc stop --timeout=1ms. svc delete --timeout=1ms has no equivalent, so the new branching in Delete at pkg/service/cmd/service.go:428-438 (the errors.Is(err, context.DeadlineExceeded) || Running() guard and the "directory stays on timeout" contract) is asserted only in docs. A test that runs rdd svc delete --timeout=1ms, asserts exit 4, and confirms the instance directory survives (e.g. via rdd svc status reporting created: true) would pin the contract down.

(suggestion, gap)

		RunE:  serviceConfigAction,
	}
	return command
}

func ensureServiceRunning(ctx context.Context) error {
	if !service.Exists() {
		if err := service.Create(nil); err != nil {
			return err
		}
	}
	if service.Running() {
		ctx, cancel := context.WithTimeout(ctx, startWaitTimeout)
		defer cancel()
		if err := service.Wait(ctx); err != nil {
			return cliexit.Classify(err)
		}
		return cliexit.Classify(waitForDiscoveryConfigMap(ctx))
	}
	return cliexit.Classify(startAndWaitForReady(ctx, nil, startWaitTimeout))
}

// startAndWaitForReady starts the service, waits for the API server and the
// discovery ConfigMap to be ready. After an unclean shutdown the ConfigMap may
// survive with stale data, so the freshness check waits for a ConfigMap whose
// creationTimestamp is at or after the current startup.

ensureServiceRunning is called transitively by rdd set, every rdd limavm *, rdd kubectl, and rdd service config. It always applies the 90s startWaitTimeout, so rdd limavm create --timeout=0 (user asks for indefinite wait) can still fail with exit 4 at 90s if the service is slow coming up. The PR framing that --timeout now flows uniformly through the autostart helper is not quite accurate — the user's timeout does not propagate through the start phase.

This is not a regression (the old code had the same 90s cap) but the function is already being reshaped. At minimum, the docstring at cmd/rdd/service.go:31-33 should note that the 90s cap is independent of the caller's --timeout. Standardising on watchtools.ContextWithOptionalTimeout (Round 4 S1) would also align the style with every sibling wait in this PR.

	service "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/service/cmd"
	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/service/controllers"
	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/util/tail"
)

// startWaitTimeout bounds the wait for the API server, CRDs, and
// 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 wait for the control plane to shut down. The
// service itself drains its controller-manager goroutines for up to 45s
// (see Run in pkg/service/cmd/service.go); when LimaVM stop orchestration

(suggestion, enhancement)

	command := &cobra.Command{
		Use:  "delete",
		Long: "Delete RDD control plane",
		RunE: serviceDeleteAction,
	}
	command.Flags().Duration("timeout", stopWaitTimeout, "Timeout for the stop-and-wait (0 means wait indefinitely)")
	return command
}

func serviceDeleteAction(cmd *cobra.Command, _ []string) error {
	if !service.Exists() {

Every other Duration("timeout", …) in this PR uses "Timeout for --wait; ignored if --wait=false (0 means wait indefinitely)". svc delete drops --wait, which is why the wording diverges, but "stop-and-wait" reads as a noun phrase rather than a knob description. The docs at docs/design/cmd_service.md:117-122 phrase it clearly.

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.

-    command.Flags().Duration("timeout", stopWaitTimeout, "Timeout for the stop-and-wait (0 means wait indefinitely)")
+    command.Flags().Duration("timeout", stopWaitTimeout, "Timeout for waiting for the control plane to exit before removing files (0 means wait indefinitely)")

(suggestion, code clarity)

// startWaitTimeout bounds the wait for the API server, CRDs, and
// 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 wait for the control plane to shut down. The
// service itself drains its controller-manager goroutines for up to 45s
// (see Run in pkg/service/cmd/service.go); when LimaVM stop orchestration
// lands, the drain will sequence each VM's shutdown, so the CLI deadline must
// match the per-VM ceiling in [limaLongWaitTimeout]. A multi-VM instance may
// still exceed this budget while shutdownAllHostagents drains sequentially.
const stopWaitTimeout = limaLongWaitTimeout

// logrusLevelToKlog converts current logrus level to klog level.
func logrusLevelToKlog() string {
	switch logrus.GetLevel() {
	case logrus.DebugLevel:

Today the service drains for 45s at pkg/service/cmd/service.go:832, so 5m is roughly 6× the actual worst case. The rationale is tied to "when LimaVM stop orchestration lands" — unwritten work whose shape may differ when it arrives. Rot risk is real: if that change never lands, or lands with a different budget, this comment points future readers at a policy that was never implemented.

		mgrWg.Wait()
		close(mgrDone)
	}()
	select {
	case <-mgrDone:
	case <-time.After(45 * time.Second):
		klog.Warning("Controller manager did not shut down within 45s, exiting anyway")
	}

	return nil
}

A shorter rationale that states the current policy ("the CLI waits longer than the service drain so slow disks or misconfigured hosts don't trip the default") survives code drift. The coupling-to-limaLongWaitTimeout constant can stay; the comment just needs to justify today's value.

(suggestion, code clarity)

    run -0 rdd ctl get limavm "restart-vm" --namespace "${LIMA_TEST_NS}" \
        --output jsonpath='{.spec.running}'
    assert_output "true"

    # Delete the VM
    run -0 rdd limavm delete "restart-vm"
}

@test "lima start --timeout fails when VM cannot reach desired state" {
    rdd ctl create configmap "timeout-vm" --namespace "${LIMA_TEST_NS}" \
        --from-literal="template=${TEMPLATE}"

The PR removed the trailing rdd ctl wait --for=delete "limavm/restart-vm" … line (diff hunk at -@@ -194,7 +191,6 @@), leaving run -0 followed only by the closing brace. Per the repo BATS style rules in deep-review-context.md:

> Use run -0 only when the next line reads ${output} or ${status}. For commands whose success is the entire assertion (setup, cleanup, fixture calls), invoke them directly.

The symmetric fix at line 94 (run -0 rdd limavm delete --wait "start-vm" → rdd limavm delete "start-vm") was applied; the restart-vm case below was missed.

    assert_created "start-vm" "${LIMA_TEST_NS}" "start-vm"

    run -0 rdd ctl get limavm "start-vm" --namespace "${LIMA_TEST_NS}" -o jsonpath='{.spec.running}'
    assert_output "true"

    rdd limavm delete "start-vm"
}

@test "lima create with file template" {
    # Create a temporary template file
    local template_file="${BATS_TEST_TMPDIR}/test-template.yaml"

     # Delete the VM
-    run -0 rdd limavm delete "restart-vm"
+    rdd limavm delete "restart-vm"
 }

(suggestion, style)

// Timeout wraps err with [CodeTimeout].
func Timeout(err error) *Error {
	return &Error{Code: CodeTimeout, Err: err}
}

// Classify wraps err with [CodeTimeout] when err wraps
// [context.DeadlineExceeded]. Other errors pass through unchanged.
func Classify(err error) error {
	if errors.Is(err, context.DeadlineExceeded) {
		return Timeout(err)
	}
	return err
}

errors.Is unwraps, so Classify(*Error{Code: CodeTimeout, Err: someDeadlineErr}) returns *Error{Code: CodeTimeout, Err: *Error{Code: CodeTimeout, Err: someDeadlineErr}}. The CLI driver still resolves the correct exit code via errors.As, but the nested Error structure is noisy in logs and defeats idempotence.

No current call path triggers the double-wrap (audited ensureServiceRunning → getAppKubeClient → setAction and every svc * / limavm * caller), but the call sites are growing, and the helper is a natural target for future refactors that may chain. A short-circuit on an existing *Error{Code: CodeTimeout} is cheap insurance:

 func Classify(err error) error {
+    var exitErr *Error
+    if errors.As(err, &exitErr) && exitErr.Code == CodeTimeout {
+        return err
+    }
     if errors.Is(err, context.DeadlineExceeded) {
         return Timeout(err)
     }
     return err
 }

(suggestion, enhancement)

The cold-start branch logs "%q control plane is ready" at :297 after the wait succeeds. The already-running branch waits on the same readiness signal (service.Wait + waitForDiscoveryConfigMap) but returns silently on success — a user running rdd svc start against a running-but-still-initialising service sees the "waiting for … to be ready" line and then nothing. Mirror the cold-start log so both paths print the same terminator.

(suggestion, observation)