Deep Review: 20260422-150402-pr-336

		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)
		}
		return cliexit.Classify(waitForFreshDiscoveryConfigMap(ctx, startTime))
	}
	return cliexit.Classify(startAndWaitForReady(ctx, nil, startWaitTimeout))
}

// startAndWaitForReady starts the service, waits for the API server and the

Both the cold-start path (beforeStart := time.Now().Truncate(time.Second) at cmd/rdd/service.go:125) and the new already-running path (service.StartTime() at :99, :267) anchor freshness on a second-truncated timestamp. The predicate CreationTimestamp.Before(beforeStart) accepts a stale CM whose CT and the new startup fall in the same wall-clock second. The race requires a prior unclean shutdown that left a ReadyAnnotation=true CM behind, a new service startup whose PID-file mtime lands in the same wall-clock second as that stale CT, and the caller entering the already-running branch before InitDiscovery deletes and recreates the CM. The window is sub-second.

// the apiserver may become ready before the serve command creates the ConfigMap.
func startAndWaitForReady(ctx context.Context, serveArgs []string, timeout time.Duration) error {
	// Truncate to second precision because metav1.Time drops sub-seconds
	// during JSON serialization. Without this, a server that starts in the
	// same second would appear to have a startTime *before* beforeStart.
	beforeStart := time.Now().Truncate(time.Second)
	if err := service.Start(ctx, serveArgs); err != nil {
		return err
	}

	ctx, cancel := watchtools.ContextWithOptionalTimeout(ctx, timeout)

Demoted from Codex's Important: pre-Round-6 the already-running branch skipped the freshness check entirely and would have accepted any surviving CM unconditionally, so Round 6 is strictly better. The cold-start path has carried this second-precision limit since before the PR. A durable fix replaces the timestamp anchor with a startup nonce copied from InitDiscovery into the CM and matched in waitForFreshDiscoveryConfigMap, and would also let the docs at docs/design/cmd_service.md describe readiness identity without hedging on timestamp precision.

(suggestion, gap — pre-existing precision limit; Round 6 extends it to a new path.)

		updated, ok := ev.Object.(*limav1alpha1.LimaVM)
		if ok && check(updated) {
			return nil
		}
	}
	// ResultChan closed. If ctx ended, return its error. Otherwise the
	// watch dropped (server close, proxy timeout, watch expiry); re-read
	// and re-evaluate to distinguish a satisfied predicate from a drop.
	if err := ctx.Err(); err != nil {
		return err
	}
	if err := c.Get(ctx, key, &vm); err != nil {
		return err
	}
	if check(&vm) {
		return nil
	}
	return fmt.Errorf("watch closed before LimaVM %q reached the desired state", key.Name)
}

// watchUntilDeleted watches a LimaVM until it is deleted.
func watchUntilDeleted(ctx context.Context, c client.WithWatch, limaVM *limav1alpha1.LimaVM) error {
	key := client.ObjectKeyFromObject(limaVM)

When a Deleted event arrives on the live channel, line 489-490 returns the descriptive "LimaVM %q was deleted while waiting". When the channel closes before that event lands and the resource is gone, the fall-through c.Get at line 503 returns an unwrapped apierrors.NewNotFound(...); the user sees "limavms.lima.rancherdesktop.io \"foo\" not found" instead of the consistent message. Same asymmetry applies to terminal-state predicates — limavm start --wait against a VM deleted mid-wait reports a stale NotFound that does not tell the user why the wait ended.

	for ev := range watcher.ResultChan() {
		switch ev.Type {
		case k8swatch.Error:
			return apierrors.FromObject(ev.Object)
		case k8swatch.Deleted:
			return fmt.Errorf("LimaVM %q was deleted while waiting", key.Name)
		}
		updated, ok := ev.Object.(*limav1alpha1.LimaVM)
		if ok && check(updated) {
			return nil
		}

Demoted from Claude's Important: the fall-through does surface an error rather than report a silent success, so the Round-5 regression class is closed. The remaining gap is one of message symmetry.

Fix:

     if err := c.Get(ctx, key, &vm); err != nil {
+        if apierrors.IsNotFound(err) {
+            return fmt.Errorf("LimaVM %q was deleted while waiting", key.Name)
+        }
         return err
     }

(suggestion, regression — the fall-through code is new in Round 6.)

	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() walks PID(), which removes the PID file when the process no longer responds to signal 0 (pkg/service/cmd/service.go:158). A concurrent CLI invocation that passes between our Running() and StartTime() can remove the file, so os.Stat(PIDFile) in StartTime() returns ENOENT and the user sees the bare stat error with no context.

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

Demoted from Claude's Important: the only path that removes the file is another PID() call, which requires a second process, and the gap is microseconds. Still worth handling: on os.IsNotExist, fall through to the cold-start path instead of propagating the raw error.

     startTime, err := service.StartTime()
-    if err != nil {
+    if os.IsNotExist(err) {
+        // Service exited between Running() and StartTime(); treat as cold start.
+        return cliexit.Classify(startAndWaitForReady(ctx, nil, startWaitTimeout))
+    } else if err != nil {
         return err
     }

*(suggestion, regression — StartTime is new; prior paths had no such dependency.)*

The new 6-timeout.bats asserts exit 4 for svc stop, svc start, and svc delete, and limavm-cli.bats covers lima create --start, lima start, and lima restart. The same timeout contract now flows through ensureServiceRunning for rdd svc config, rdd set, and rdd ctl (at cmd/rdd/kubectl.go), but no test asserts exit 4 on any of those call sites. A caller-specific regression that unwraps *Error before returning from the autostart path would ship green.

Fix: add one run -4 case per representative shared-helper caller — e.g. rdd svc config and rdd set running=false against a deliberately unready control plane.

(suggestion, gap.)

	"github.com/sirupsen/logrus"
	"github.com/spf13/cobra"

	apierrors "k8s.io/apimachinery/pkg/api/errors"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/apimachinery/pkg/util/wait"
	"k8s.io/client-go/kubernetes"
	watchtools "k8s.io/client-go/tools/watch"

	cliexit "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/cli/exit"
	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/developer"

Neither function references wait.PollUntilContextCancel, so no bug lands today. A future edit that adds a wait.Poll… call in either function would compile-fail on the shadow and the fix would be obscure. Renaming the local to waitFlag (or hoisting the branch selection into a helper) removes the trap. The shadow is pre-existing in serviceStopAction; this PR introduces the same shadow to serviceStartAction.

(suggestion, gap.)

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

The predicate errors.Is(err, context.DeadlineExceeded) || Running() suppresses any StopWithWait error whose post-call Running() returns false — including the process.Interrupt / process.Kill failure branch at lines 370-373 and the context.Canceled branch at lines 393-397. The context.Canceled path is unreachable today because the CLI uses context.Background() for RunE, but a future commit that wires SIGINT into the CLI context would silently proceed to RemoveAll(instance.Dir()) on Ctrl-C. Either tighten the predicate to errors.Is(err, ErrAlreadyStopped) (once StopWithWait returns a typed sentinel for that race), or expand the comment to enumerate every case the predicate absorbs.

	// 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, 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, SIGTERM suffices because the service handles
				// signals promptly; its slowness comes from draining
				// hostagents sequentially.
				_ = 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
				}

(suggestion, regression.)

    local patch=$3

    rdd ctl patch "${resource_type}" "${name}" -n "${RDD_NAMESPACE}" --type='merge' -p="${patch}"
}

setup_rdd_control_plane() {
    local controllers=${1:-"*"}

    rdd svc delete
    rdd svc create --controllers="${controllers}"
    rdd svc start
}

Pre-PR, rdd svc delete waited 60 s and always removed the directory (old Stop() errors were ignored). After this PR, delete waits up to 5 minutes (stopWaitTimeout) and fails with exit 4 if the service does not exit in time, leaving the directory in place. A test file whose prior run left a hostagent in a pathological state would now hang the helper for 5 minutes and then fail, rather than recovering.

Fix: bound the helper with a shorter timeout and tolerate a failed delete so a partial teardown still lets create run.

-    rdd svc delete
+    rdd svc delete --timeout=120s || :

(suggestion, gap — pre-existing helper, behaviour shift introduced by this PR.)

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

@test 'svc delete --timeout=1ms exits with code 4 and preserves the instance directory' {
    # Restart the service; the prior test killed it.
    rdd svc start
    run -4 rdd svc delete --timeout=1ms
    assert_output --partial 'context deadline exceeded'
    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 || :
    rdd svc create

Test 1 sends SIGINT then SIGTERM via process.Kill and returns. process.Kill is async on Unix: the service may still report Running() == true when Test 2's rdd svc start runs, and Test 2 then takes the already-running branch — see I2 for why that can hang 90 s on a missing CM. Test 3 already resets with rdd svc delete || :; rdd svc create; rdd svc start; applying the same reset before Test 2's rdd svc start (or a wait_for_dead polling helper) removes the timing dependency.

(suggestion, regression — new test.)

The PR's new flags say "Timeout for --wait; ignored if --wait=false (0 means wait indefinitely)". The pre-existing rdd set --timeout says "Timeout for waiting (0 to wait indefinitely)". The PR's stated goal is to standardize this surface — aligning set's help string in this PR (or as a targeted follow-up) keeps --help output uniform. The hardcoded 300*time.Second at cmd/rdd/set.go:92 also duplicates limaLongWaitTimeout = 5 * time.Minute; promoting a single named constant prevents drift.

	}
	command.Flags().BoolVar(&dryRun, "dry-run", false,
		"Validate changes against the API server without persisting them")
	command.Flags().BoolVar(&wait, "wait", true,
		"Wait for the desired state after the patch is accepted")
	command.Flags().DurationVar(&timeout, "timeout", 300*time.Second,
		"Timeout for waiting (0 to wait indefinitely)")

	// Override help to append live property descriptions from the CRD schema.
	defaultHelp := command.HelpFunc()
	command.SetHelpFunc(func(cmd *cobra.Command, args []string) {

(suggestion, enhancement.)

		},
	}
	command.Flags().StringVarP(&namespace, "namespace", "n", metav1.NamespaceDefault, "Namespace for the LimaVM resource")
	command.Flags().BoolVar(&dryRun, "dry-run", false, "If set, do not commit any changes to the cluster")
	command.Flags().BoolVar(&start, "start", false, "Start the VM after creation")
	command.Flags().BoolVar(&wait, "wait", true, "Wait for the operation to complete (only applies with --start)")
	command.Flags().DurationVar(&timeout, "timeout", limaLongWaitTimeout, "Timeout for --wait; ignored if --wait=false (0 means wait indefinitely)")
	return command
}

func newLimaVMStartCommand() *cobra.Command {
	var wait bool

--wait mentions "only applies with --start"; --timeout's help mentions only --wait=false. The wait branch at line 409 gates on start && wait && !dryRun, so --timeout is also a no-op without --start. Mention --start in the timeout help on create for parity with --wait.

			// Keep createdConfigMap until limaVM itself is deleted.
			createdConfigMap = nil
		}
	}

	if start && wait && !dryRun {
		waitCtx, cancel := watchtools.ContextWithOptionalTimeout(ctx, timeout)
		defer cancel()
		key := client.ObjectKeyFromObject(limaVM)
		if err := watchUntil(waitCtx, c, key, func(vm *limav1alpha1.LimaVM) bool {
			return apimeta.IsStatusConditionPresentAndEqual(vm.Status.Conditions, "Running", metav1.ConditionTrue)

(suggestion, enhancement.)