Deep Review: 20260421-225153-pr-336

}

// StopWithWait sends a shutdown signal to the service. When wait is true, it
// blocks until the process exits or timeout elapses; pass timeout 0 to wait
// indefinitely.
func StopWithWait(wait bool, timeout time.Duration) error {
	if !Running() {
		return fmt.Errorf("%q control plane is not running", instance.Name())
	}

	// Clean up discovery configmap while cluster is still accessible
	_ = cleanupDiscoveryConfigMap() // Clean up discovery configmap to prevent stale controller info

Pre-PR, Delete() called _ = Stop() and discarded the error, so a process that exited between any two checks did not block deletion. Post-PR, Delete(timeout) propagates the StopWithWait error verbatim. A narrow window exists between Delete's outer Running() and StopWithWait's inner one in which the serve subprocess can exit (crash, OOM-kill, external kill, concurrent rdd svc stop). When that happens, StopWithWait returns "… control plane is not running", Delete propagates it, and os.RemoveAll(instance.Dir()) never runs. The user sees exit code 1 and a confusing message, even though the desired end state is one os.RemoveAll away. The condition self-heals on retry — the next invocation sees !Running(), skips the stop branch, and proceeds to delete.

Fix: treat "already not running" as success in Delete by re-checking after the stop failure:

     if Running() {
         if err := StopWithWait(true, timeout); err != nil {
-            return err
+            // The process may have exited between our Running check and
+            // StopWithWait's; in that case its work is already done.
+            if Running() {
+                return err
+            }
         }
     }

(suggestion, regression)

    # 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}"
    run_e -0 rdd limavm create "timeout-vm" "timeout-vm" --namespace "${LIMA_TEST_NS}"
    assert_created "timeout-vm" "${LIMA_TEST_NS}" "timeout-vm"

    # Non-functional template cannot boot, so --wait --timeout should fail
    # with exit code 4 (cliexit.CodeTimeout) to match `rdd set`.
    run -4 rdd limavm start --wait --timeout=3s "timeout-vm"
    assert_output --partial 'context deadline exceeded'
}

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

    # Create with --start: the VM never reaches Running=True because the
    # template is non-functional, so --wait --timeout exits with code 4.
    run -4 rdd limavm create "create-timeout-vm" "create-timeout-vm" \
        --namespace "${LIMA_TEST_NS}" --start --wait --timeout=3s
    assert_output --partial 'context deadline exceeded'
}

@test "lima restart --timeout fails when VM cannot restart" {
    rdd ctl create configmap "restart-timeout-vm" --namespace "${LIMA_TEST_NS}" \
        --from-literal="template=${TEMPLATE}"
    run_e -0 rdd limavm create "restart-timeout-vm" "restart-timeout-vm" --namespace "${LIMA_TEST_NS}"
    assert_created "restart-timeout-vm" "${LIMA_TEST_NS}" "restart-timeout-vm"

    # Restart waits for status.restartCount to increment, which requires the
    # VM to come back up. The non-functional template never boots, so the
    # counter stays at zero and --wait --timeout exits with code 4.
    run -4 rdd limavm restart --wait --timeout=3s "restart-timeout-vm"
    assert_output --partial 'context deadline exceeded'
}

@test "lima help text is displayed" {
    # lima is an alias of the limavm command
    run -0 rdd lima --help
    assert_output --partial "LimaVM virtual machines"

8aa55e4 adds two new exit-code-4 tests on top of the one 99aa4e3 updated, all inside lima subcommands. None of the four new svc * timeout paths the PR introduces carry coverage:

• rdd svc start --wait --timeout=… (wraps startAndWaitForReady at cmd/rdd/service.go:302)
• rdd svc start against an already-running service (wraps service.Wait at cmd/rdd/service.go:278)
• rdd svc stop --wait --timeout=… (wraps service.StopWithWait at cmd/rdd/service.go:334)
• rdd svc delete --timeout=… (wraps service.Delete at cmd/rdd/service.go:375)
• --timeout=0 (indefinite) on any of these paths

			return nil
		}
		logrus.Infof("waiting for %q control plane to be ready", instance.Name())
		ctx, cancel := watchtools.ContextWithOptionalTimeout(cmd.Context(), timeout)
		defer cancel()
		return timeoutError(service.Wait(ctx))
	}

	// Collect all provided flags as arguments for serve subprocess
	var serveArgs []string
	if cmd.Flags().Changed("controllers") {

	serveArgs = append(serveArgs, "-v", logrusLevelToKlog())
	serveArgs = append(serveArgs, args...)

	if wait {
		if err := startAndWaitForReady(cmd.Context(), serveArgs, timeout); err != nil {
			return timeoutError(err)
		}
		logrus.Infof("%q control plane is ready", instance.Name())
	} else {
		if err := service.Start(cmd.Context(), serveArgs); err != nil {
			return err

	if err != nil {
		return err
	}

	if err := service.StopWithWait(wait, timeout); err != nil {
		return timeoutError(err)
	}
	if wait {
		logrus.Infof("%q control plane has stopped", instance.Name())
	} else {
		logrus.Infof("%q control plane is stopping", instance.Name())

	timeout, err := cmd.Flags().GetDuration("timeout")
	if err != nil {
		return err
	}
	if err := service.Delete(timeout); err != nil {
		return timeoutError(err)
	}
	logrus.Infof("%q control plane has been deleted", instance.Name())
	return nil
}

A regression that returns the wrapped error directly without going through timeoutError will ship green. This restates Round 1 I3; the fixup commit partially addressed it (lima paths now covered) but left the svc side unasserted. The simplest provoke for svc start is creating the control plane with a broken controller manifest; the simplest for svc stop is the setup from bats/tests/33-lima-controllers/limavm-running.bats:327 ("graceful service stop terminates hostagent"). At minimum, add one svc * timeout assertion so the contract is not lima-only.

# Graceful shutdown test
# Verifies that stopping the service terminates all running hostagents.
# The controller's shutdown runnable calls shutdownAllHostagents() on exit.

@test "graceful service stop terminates hostagent" {
    local ha_pid
    ha_pid=$(get_hostagent_pid "${VM_NAME}")
    assert [ -n "${ha_pid}" ]
    assert_process_alive "${ha_pid}"

(suggestion, gap — restates Round 1 I3 partially addressed.)

		if err := service.Create(args); err != nil {
			return err
		}
		logrus.Infof("successfully created %q control plane", instance.Name())
	}
	if service.Running() {
		logrus.Infof("%q control plane is already running", instance.Name())
		if !wait {
			return nil
		}
		logrus.Infof("waiting for %q control plane to be ready", instance.Name())
		ctx, cancel := watchtools.ContextWithOptionalTimeout(cmd.Context(), timeout)
		defer cancel()
		return timeoutError(service.Wait(ctx))
	}

	// Collect all provided flags as arguments for serve subprocess
	var serveArgs []string
	if cmd.Flags().Changed("controllers") {

docs/design/cmd_service.md:93-95 explicitly advertises the flow "--wait=false … then run rdd service start again with no options to wait." The cold-start path (startAndWaitForReady) waits for both service.Wait(ctx) and waitForFreshDiscoveryConfigMap(ctx, beforeStart) — the latter requires ReadyAnnotation == "true", which the controller manager sets only after every expected controller has registered. The already-running path calls only service.Wait(ctx), which via checkReadiness returns success as soon as the discovery ConfigMap lists registered controllers — before the ready annotation. A concurrent --wait=false followed by rdd svc start during a still-initializing daemon therefore returns success under a narrower contract than the cold-start path guarantees.

Runs `rdd service serve` to actually start the control plane in the background

Additional options:

*   `--wait=false`
    Return immediately, without waiting for all controllers to be ready. Run `rdd service start`
    again with no options to wait.

*   `--timeout=90s`
    How long to wait for the control plane to become ready. Pass `0` to wait indefinitely.
    If the deadline expires, `rdd` exits with code 4.

Codex raised this at Important because the PR's new documentation names the workflow explicitly; Claude raised it at Suggestion because the race window is narrow (controllers finish registering quickly, and a steady-state instance is always fine). We consolidate at Suggestion: the practical exposure is a narrow mid-startup race, but the documented workflow deserves a one-line fix to deliver the advertised guarantee.

Fix: mirror ensureServiceRunning's pattern and wait for the discovery ConfigMap on the already-running branch too:

-    return timeoutError(service.Wait(ctx))
+    if err := service.Wait(ctx); err != nil {
+        return timeoutError(err)
+    }
+    return timeoutError(waitForDiscoveryConfigMap(ctx))

(suggestion, gap — pre-existing; the PR's new docs make it newly relevant.)

    run -0 rdd limavm stop --wait=false "test-vm"
    assert_output --partial 'stopped'
    assert_running "false"

    # Delete the VM
    run -0 rdd limavm delete --wait "test-vm"
    assert_output --partial 'deleted'
    run -1 rdd ctl get limavm "test-vm" --namespace "${LIMA_TEST_NS}"
    assert_output --partial "not found"
}

limavm delete now defaults to --wait=true (cmd/rdd/limavm.go:253). The explicit flag is redundant and signals to a future reader that the flag is load-bearing — when the opposite is now true. Drop the flag:

		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 (0 means wait indefinitely)")
	return command
}

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

-run -0 rdd limavm delete --wait "test-vm"
+run -0 rdd limavm delete "test-vm"

(suggestion, gap)

The PR description lists:

> Default timeouts: > - 30s: svc start, svc stop, svc delete, limavm create > - 5m: limavm start, limavm stop, limavm restart, limavm delete

After 8aa55e4:

• svc start → startWaitTimeout = 90s
• svc stop, svc delete → stopWaitTimeout = limaLongWaitTimeout = 5m
• All limavm paths → limaLongWaitTimeout = 5m

The "30s bucket" no longer exists. Update the PR description, and either amend 99aa4e3 or extend 8aa55e4's body with the final contract so a future git log reader sees the shipped defaults. This also picks up Round 1 S7 (the service.Stop() / service.WaitWithTimeout() removals are still unnamed in either commit body) and Round 1 S8 (the stale limavm create 30s line).

(suggestion, gap — re-raises Round 1 S7 and S8 unaddressed.)

`--wait=false` to return immediately, or `--timeout=0` to wait indefinitely. If the
deadline expires, `rdd` exits with code 4. The default matches the per-VM ceiling
because graceful shutdown will eventually stop every running LimaVM before the
service exits.

### `rdd service delete`

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.

### `rdd service reset`

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

The 99aa4e3 commit body says the change folds the shutdown-wait semantics of svc delete into a reference to svc stop, but the actual doc carries no cross-reference. A reader must know that svc delete uses svc stop's mechanism to understand which signal is sent (SIGINT / CTRLBREAKEVENT, with Kill fallback). A one-line "See svc stop for the signal/fallback details" closes the gap without duplicating content.

(suggestion, gap)

* It starts any additional controllers configured via the `config` ConfigMap.

## Service shutdown

The control plane will be notified to shut down, either by receiving a SIGTERM signal from the user (via `rdd service stop`), or by the OS preparing to shut down the host.

Some controllers need to be notified of pending shutdown so they can gracefully terminate, e.g. shut down virtual machines.

Any controller can request shutdown notification by creating a ConfigMap in the `rdd-system` namespace:

cmd_service.md was updated to say SIGINT (with the Windows CTRL_BREAK_EVENT mapping) and the implementation calls process.Interrupt(pid) first at pkg/service/cmd/service.go:358-362, but the architecture doc api_service.md still names SIGTERM. The two docs now disagree on the shutdown contract — exactly what this PR is standardizing.

	// Kill fallback never fires. On Windows, Interrupt uses
	// GenerateConsoleCtrlEvent, which fails when caller and target lack a
	// shared console; Kill (TerminateProcess) then bypasses graceful shutdown.
	// Hostagents survive in their own process groups and self-heal on the
	// next service start via killOrphanedHostagent.
	if err := process.Interrupt(pid); err != nil {
		if err := process.Kill(pid); err != nil {
			return fmt.Errorf("failed to stop %q control plane with pid %d: %w", instance.Name(), pid, err)
		}
	}

	if wait {
		ctx := context.Background()
		if timeout > 0 {
			var cancel context.CancelFunc

Fix: update docs/design/api_service.md:21 to match: rdd service stop sends SIGINT on Unix or CTRL_BREAK_EVENT on Windows, then falls back to forced termination if graceful shutdown cannot be initiated or does not complete in time.

(suggestion, gap — pre-existing; the PR's companion doc update made the divergence worse.)

cmd/rdd/service.go imports k8s.io/client-go/tools/watch as watchtools, while cmd/rdd/limavm.go imports the same package as toolswatch. Standardize on one alias across the CLI to keep a quick text search from missing call sites.

(suggestion, gap)