Deep Review: 20260421-173630-pr-336

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

// 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.
func Delete(timeout time.Duration) error {
	if !Exists() {
		return fmt.Errorf("%q control plane does not exist", instance.Name())
	}
	if Running() {
		if err := StopWithWait(true, timeout); err != nil {
			return err
		}
	}
	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

Before the PR, Delete() always called Stop(), which always waited. This PR exposes --wait=false on svc delete and threads it through to StopWithWait. When the user passes --wait=false, StopWithWait signals the process and returns immediately (pkg/service/cmd/service.go:358-395); Delete then calls os.RemoveAll(instance.LogDir()), os.RemoveAll(instance.ShortDir()), and os.RemoveAll(instance.Dir()) while the serve subprocess still holds open handles to rdd.pid, rdd.sqlite3, rdd.stderr.log, rdd.stdout.log, and has its working directory pinned to instance.Dir() (set in service.go:607).

	// 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
			ctx, cancel = context.WithTimeout(ctx, timeout)
			defer cancel()
		}
		ticker := time.NewTicker(100 * time.Millisecond)
		defer ticker.Stop()

		for {
			select {
			case <-ctx.Done():
				// Graceful shutdown timed out; terminate so we don't leave
				// a hung service process. Kill targets only the service; on
				// 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
				}
			}
		}
	}

	_ = os.Remove(instance.Config()) // Ignore error as file might not exist
	return nil
}

// cleanupDiscoveryConfigMap removes the discovery configmap to prevent readiness check confusion.
func cleanupDiscoveryConfigMap() error {

On Windows, os.RemoveAll fails with a sharing violation mid-removal because the handles are still open; the user sees an error and a partially-deleted tree. On Unix, removal succeeds, but then Running() starts returning false (PID file is gone; PID() returns PIDNotFound at pkg/service/cmd/service.go:138-161) even though the serve process remains alive. A subsequent rdd svc start for the same instance now sees !Exists(), recreates the directory, and starts a second serve subprocess that races the first for the secure port (6443 + instance.Index()).

// PIDNotFound indicates no running service process was found.
const PIDNotFound = 0

// PID returns the process ID of the running service, or PIDNotFound if it is not running.
func PID() int {
	pidStr, err := os.ReadFile(instance.PIDFile())
	if err != nil {
		return PIDNotFound
	}
	pid, err := strconv.Atoi(string(pidStr))
	if err == nil {
		var process *os.Process
		process, err = os.FindProcess(pid)
		if err == nil {
			// on non-Windows, FindProcess may return without the process being
			// alive; on Windows, the result encapsulates a valid handle.
			if runtime.GOOS != "windows" {
				err = process.Signal(syscall.Signal(0))
			}
			_ = process.Release()
		}
	}
	if err != nil {
		_ = os.Remove(instance.PIDFile())
		return PIDNotFound
	}
	return pid
}

// Running reports whether the service process is alive.
func Running() bool {
	return PID() != PIDNotFound
}

Gemini flagged this as critical; Codex and Claude as important. We consolidate at important: the data being removed is itself destined for deletion, but the PID-file-based mutual exclusion breaks and the Windows partial-deletion path leaves the user with an error.

Fix: always wait for the process to exit before deleting state. Either drop --wait=false from svc delete entirely (deletion cannot usefully be non-blocking since the writer must be gone) or pass true unconditionally to the internal StopWithWait call while still exposing --timeout so users can tune how long to wait:

     if Running() {
-        if err := StopWithWait(wait, timeout); err != nil {
+        // Data deletion requires the writer to be gone before the directory
+        // is removed; --wait=false on svc delete is not safe, so ignore it.
+        if err := StopWithWait(true, timeout); err != nil {
             return err
         }
     }

If --wait=false is preserved for delete, docs/design/cmd_service.md:112 should document the race explicitly.

	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

The new default wires into three subcommands with different sizing constraints:

Stop / delete path. The serve subprocess allows up to 45s for its controller-manager goroutine to drain (pkg/service/cmd/service.go:826: case <-time.After(45 * time.Second)), and Lima hostagents get 30s of that budget for graceful shutdown. When the CLI's 30s deadline fires first, StopWithWait calls process.Kill(pid) at line 384, aborting the subprocess's own 45s window. The now-removed comment inside StopWithWait captured exactly this: // The service needs up to 30s for graceful hostagent shutdown plus overhead, so allow 60s total. With the new default, a healthy shutdown of a service running a Lima VM will routinely force-kill instead of draining cleanly.

	mgrDone := make(chan struct{})
	go func() {
		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")
	}

}

func newServiceStatusCommand() *cobra.Command {
	command := &cobra.Command{
		Use:  "status",
		Long: "Show control plane status",
		RunE: func(*cobra.Command, []string) error {
			logrus.SetLevel(logrus.InfoLevel)
			logrus.Infof("%q control plane has been created: %v", instance.Name(), service.Exists())
			logrus.Infof("%q control plane has been started: %v", instance.Name(), service.Running())
			logrus.Infof("%q control plane PID is: %v", instance.Name(), service.PID())

Start path. The replaced WaitWithTimeout used 90s with the comment // Increased timeout for CRD establishment. Both ensureServiceRunning call sites (cmd/rdd/service.go:97, 104) still use 90s, which is the correct budget for CRD establishment + controller-manager registration + discovery ConfigMap readiness. The explicit rdd svc start path at line 234 now uses 30s. Either 90s is right in both places or 30s is right in both places — the split is itself evidence that 30s is too tight for the readiness path.

		return err
	}
	logrus.Infof("successfully created %q control plane", instance.Name())
	return nil
}

func newServiceStartCommand() *cobra.Command {
	command := &cobra.Command{
		Use:  "start",
		Long: "Start RDD control plane. When called without parameters, uses default parameters from create. When called with parameters, those override the defaults for this session only.",
		RunE: serviceStartAction,

Test exposure. bats/tests/20-service/2-create.bats:65, 4-start-parameters.bats:45-63, and 5-port-fallback.bats:72, 105, 147 all run run -0 rdd svc stop against services that may have a live Lima VM. Those now risk exit-code-4 flakes when the hostagent drain exceeds 30s.

    # stdout may be empty, just verify the command succeeds
    rdd svc logs --stdout
}

@test 'stop instance' {
    run -0 rdd svc stop
    run -0 extract_msg
    assert_output "\"rancher-desktop-${RDD_INSTANCE}\" control plane has stopped"
    assert_dir_exist "${RDD_DIR}"
}

    assert_line configmapreplicaset
    refute_line demo
}

@test 'start instance with parameter override' {
    run -0 rdd svc stop
    run -0 rdd svc start --controllers="*"
    get_enabled_controllers
    assert_line notary
    assert_line configmapreplicaset
    assert_line demo
}

@test 'start instance with controller parameter override' {
    run -0 rdd svc stop
    run -0 rdd svc start --controllers="app"
    get_enabled_controllers
    assert_line demo
    refute_line notary
    refute_line configmapreplicaset
}

@test 'start instance returns to default parameters after override' {
    run -0 rdd svc stop
    # Start with default parameters (should use saved rdd controllers)
    run -0 rdd svc start
    get_enabled_controllers
    assert_line notary
    assert_line configmapreplicaset

Fix: split the one constant into named constants that each match a concrete invariant. For example:

-// defaultWaitTimeout is the default --timeout for subcommands whose wait
-// normally settles within seconds.
-const defaultWaitTimeout = 30 * time.Second
+// startWaitTimeout covers CRD establishment plus controller-manager registration.
+const startWaitTimeout = 90 * time.Second
+
+// stopWaitTimeout must exceed the service's own 45s controller-manager drain
+// (see Run in pkg/service/cmd/service.go) plus overhead.
+const stopWaitTimeout = 60 * time.Second

Then use startWaitTimeout in both newServiceStartCommand and ensureServiceRunning, and stopWaitTimeout in newServiceStopCommand and newServiceDeleteCommand.

The PR introduces a uniform CLI contract: every wait-capable subcommand exits with code 4 on --timeout expiry. The only assertion of that contract is the mechanically updated limavm start --timeout test (run -1 → run -4). None of the new paths are exercised:

• rdd svc start --wait --timeout=… (uses timeoutError at cmd/rdd/service.go:294)
• rdd svc stop --wait --timeout=… (at cmd/rdd/service.go:326)
• rdd svc delete --wait --timeout=… (at cmd/rdd/service.go:372)
• rdd limavm stop|restart|delete|create --start --timeout=… (at cmd/rdd/limavm.go:235, 415, 455, 556)
• --timeout=0 (indefinite wait) on any command

	}
	if cmd.Flags().Changed("secure-port") {
		securePort, err := cmd.Flags().GetInt("secure-port")
		if err != nil {
			return err
		}
		serveArgs = append(serveArgs, "--secure-port", strconv.Itoa(securePort))
	}
	serveArgs = append(serveArgs, "-v", logrusLevelToKlog())
	serveArgs = append(serveArgs, args...)

		return nil
	}

	wait, err := cmd.Flags().GetBool("wait")
	if err != nil {
		return err
	}
	timeout, err := cmd.Flags().GetDuration("timeout")
	if err != nil {
		return err
	}

		logrus.Infof("%q control plane does not exist", instance.Name())
		return nil
	}
	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())

A regression that returns the underlying error directly instead of the cliexit.Timeout envelope will ship green. The pre-existing TODO at bats/tests/20-service/2-create.bats:3 (# TODO: test rdd svc start --wait=false) also becomes newly relevant: the PR formalizes that flag's semantics but the test is still missing.

load '../../helpers/load'

# TODO: test `rdd svc start --wait=false`

@test 'Make sure instance does not exist' {
    rdd svc delete || :
    assert_dir_not_exist "${RDD_DIR}"
}

Fix: add at least one test per subcommand that forces a timeout and asserts exit code 4. The svc start --timeout case is the simplest to provoke — run against an instance whose controllers cannot reach ready.

`TEMPLATE` will be treated as a local filename or template URL if it contains a `/` or a `:`.  In that case the `create` command will create a ConfigMap in `NAMESPACE` called `NAME`. It will store the "fully embedded" template from the file or URL inside that ConfigMap and use it as the `spec.templateRef`. If a ConfigMap of this name already exists, then the `create` command will fail. If `LimaVM` creation succeeds then ownership of this ConfigMap is transferred to the `LimaVM` resource, and it will be deleted when the `LimaVM` instance is deleted. But when the command fails, then any ConfigMap created from a file or URL is deleted immediately.

- `--start` (default `false`): Set `spec.running=true` to start the VM immediately after creation.
- `--wait` (default `true`): When used with `--start`, wait until the Running condition becomes True before returning.
- `--timeout` (default `5m`): Maximum time to wait. A value of `0` waits indefinitely. Accepts Go duration strings (e.g., `30s`, `5m`). If the deadline expires, `rdd` exits with code 4.

### `rdd limavm start NAME`

Set `spec.running` of the specified instance to `true`. There is no `--namespace` option because `LimaVM` names are globally unique (within the control plane).

	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

    If the deadline expires, `rdd` exits with code 4.


### `rdd service stop`

Sends SIGINT to the control plane process (`rdd.pid`) and waits up to 5 minutes for
it to exit. On Windows the signal is delivered as `CTRL_BREAK_EVENT`. Pass
`--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.

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

	}
	cmd.SysProcAttr.Setpgid = true
}

// Interrupt sends SIGINT to the process with the given PID.
func Interrupt(pid int) error {
	return unix.Kill(pid, unix.SIGINT)
}

// Kill sends SIGTERM to the process with the given PID.
func Kill(pid int) error {
	return unix.Kill(pid, unix.SIGTERM)
}

-Sends SIGTERM to the control plane process (`rdd.pid`) and waits up to 30s for it to exit.
+Sends SIGINT to the control plane process (`rdd.pid`) and waits up to 30s for it to exit.
+On Windows this is delivered as `CTRL_BREAK_EVENT`.

		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.
// The returned client supports Watch for event-driven waiting on resource changes.

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

// 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].
const stopWaitTimeout = limaLongWaitTimeout

// timeoutError wraps a deadline-expiry error with [cliexit.Timeout] so the CLI
// exits with [cliexit.CodeTimeout], matching `rdd set`. Other errors pass
// through unchanged.
func timeoutError(err error) error {
	if errors.Is(err, context.DeadlineExceeded) {
		return cliexit.Timeout(err)

-// timeoutError wraps a deadline-expiry err with [cliexit.Timeout] so the CLI
+// timeoutError wraps a deadline-expiry error with [cliexit.Timeout] so the CLI