Deep Review: 20260423-182700-pr-344

		WatchFunc: func(opts metav1.ListOptions) (watch.Interface, error) {
			opts.FieldSelector = fieldSelector
			return resource.Watch(ctx, opts)
		},
	}
	_, err = watchtools.UntilWithSync(ctx, lw, &unstructured.Unstructured{}, nil,
		func(event watch.Event) (bool, error) {
			if event.Type == watch.Deleted {
				return false, errors.New("resource was deleted while waiting")
			}
			obj, ok := event.Object.(*unstructured.Unstructured)
			if !ok {
				return false, nil
			}
			return checker.check(obj), nil
		},
	)
	return err
}

// parseConditionArg parses "TYPE[=STATUS]" into condition type and status.
// STATUS defaults to "True" when omitted.
func parseConditionArg(arg string) (condType, condStatus string, err error) {

ctl wait-condition carries its own --timeout flag (default 30s) and creates a context with that deadline at line 97. When the deadline expires, watchtools.UntilWithSync returns a wrapped context.DeadlineExceeded, which exits the CLI with code 1 instead of the intended cliexit.CodeTimeout (4). The PR does not touch this file, but the stated goal of a uniform CLI contract leaves this sibling path on the wrong exit code. This matches the warning at repo-context line 48 about uniform-contract refactors that wire only the outermost entry points.

	if err := flags.Parse(rawArgs); err != nil {
		return err
	}
	positional := flags.Args()
	if len(positional) != 2 {
		return fmt.Errorf("expected TYPE/NAME and CONDITION[=STATUS] arguments, got %d arguments", len(positional))
	}

	resourceType, resourceName, err := parseResourceArg(positional[0])
	if err != nil {

	dynClient, err := dynamic.NewForConfig(restConfig)
	if err != nil {
		return fmt.Errorf("failed to create dynamic client: %w", err)
	}

	ctx, cancel := context.WithTimeout(cmd.Context(), *timeout)
	defer cancel()

	checker := conditionChecker{
		condType:   condType,
		condStatus: condStatus,

Fix:

-    return err
+    return cliexit.Classify(err)

// 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())
	return os.RemoveAll(instance.Dir())
}

// preserveAllInstanceLogs moves .log files from each Lima instance directory
// to the service log directory before the instance directories are deleted.
// This is a no-op unless RDD_KEEP_LOGS is set.
//

StopWithWait can return three non-deadline errors: "control plane is not running" (line 350; benign race), the "failed to stop ... pid" wrapper when both Interrupt and Kill fail (line 367; rare on Unix, possible on Windows when no shared console), and "wait for ... cancelled" when the parent context is canceled (line 400; today unreachable per the in-code comment). When any of these fire and the daemon happens to have exited between the error and the re-check, the predicate proceeds with deletion and the diagnostic is lost. The behavior is still strictly better than the pre-PR _ = Stop() (which dropped every error unconditionally), so the outcome is correct; what is lost is the user's only signal that something unexpected happened — most relevantly a Windows console-fallback that bypassed graceful hostagent shutdown.

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

	pid := PID()
	// Try graceful shutdown first. On Unix, Kill already sends SIGTERM which
	// triggers the Go signal handler. On Windows, Kill uses TerminateProcess
	// which bypasses all handlers, so we send Interrupt (CTRL_BREAK_EVENT)
	// first to let the service run its shutdown path (shutdownAllHostagents).
	//
	// On Unix, Interrupt (SIGINT) always succeeds for a valid PID, so the
	// 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)

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

Reconciliation: Claude labeled this "regression"; the pre-existing _ = Stop() discarded errors more aggressively, so the post-PR predicate is an improvement, not a regression. Reclassified as a design-quality concern in newly-introduced code.

Fix: log the dropped error at warn level so the daemon-state surprise is preserved without changing the outcome:

 if Running() {
     if err := StopWithWait(ctx, true, timeout); err != nil {
         if errors.Is(err, context.DeadlineExceeded) || Running() {
             return err
         }
+        logrus.WithError(err).Warn("service exited despite shutdown error; proceeding with delete")
     }
 }

	if err != nil {
		return err
	}
	defer watcher.Stop()

	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
		}
	}
	// 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 {
		if apierrors.IsNotFound(err) {
			return fmt.Errorf("LimaVM %q was deleted while waiting", key.Name)
		}
		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)

The fall-through correctly fixes the "dropped watch → spurious success" trap. But when the predicate still isn't satisfied after the re-read, the code returns a hard error rather than reopening the watch from vm.ResourceVersion. For rdd limavm restart --wait --timeout=5m, a benign Kubernetes server-side watch expiry (~5–10 min) would surface as a hard failure even though the caller's ctx is still valid. The repo already has a retry-aware pattern: cmd/rdd/set.go:490-535 uses watchtools.UntilWithSync, which transparently re-lists on watch-channel closure.

// watchCondition watches the App singleton until the predicate
// returns true or the context expires. watchtools.UntilWithSync
// re-lists transparently on benign watch-channel closures (API
// timeouts, proxy disconnects, resource-version compaction).
func watchCondition(ctx context.Context, config *rest.Config, satisfied func(*unstructured.Unstructured) bool) error {
	dynClient, err := dynamic.NewForConfig(config)
	if err != nil {
		return fmt.Errorf("failed to create dynamic client: %w", err)
	}

	// Defensive filter; the webhook enforces metadata.name=app on the singleton.
	lw := &cache.ListWatch{
		ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {
			options.FieldSelector = "metadata.name=app"
			return dynClient.Resource(appGVR).List(ctx, options)
		},
		WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
			options.FieldSelector = "metadata.name=app"
			return dynClient.Resource(appGVR).Watch(ctx, options)
		},
	}

	precondition := func(store cache.Store) (bool, error) {
		for _, item := range store.List() {
			if obj, ok := item.(*unstructured.Unstructured); ok && satisfied(obj) {
				return true, nil
			}
		}
		return false, nil
	}

	condition := func(event watch.Event) (bool, error) {
		obj, ok := event.Object.(*unstructured.Unstructured)
		if !ok {
			return false, nil
		}
		return satisfied(obj), nil
	}

	if _, err := watchtools.UntilWithSync(ctx, lw, &unstructured.Unstructured{}, precondition, condition); err != nil {
		if errors.Is(err, context.DeadlineExceeded) {
			return fmt.Errorf("timed out waiting for App state: %w", err)
		}
		if errors.Is(err, context.Canceled) {
			return fmt.Errorf("wait cancelled: %w", err)
		}
		return fmt.Errorf("failed to watch App: %w", err)
	}
	return nil
}

// conditionInfo returns the status, observedGeneration, and presence
// of the named condition on an App. Callers must check present
// before trusting the other return values.
func conditionInfo(obj *unstructured.Unstructured, condType string) (status metav1.ConditionStatus, observedGen int64, present bool) {

Fix: replace the hand-rolled loop with watchtools.UntilWithSync (preferred — matches the set implementation), or wrap the existing loop in a retry that opens a fresh watch from the current ResourceVersion until ctx is done.

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

## Service shutdown

The control plane is notified to shut down, either by `rdd service stop`
(SIGINT on Unix, `CTRL_BREAK_EVENT` on Windows; if the wait deadline expires
the wait force-terminates with SIGTERM on Unix or `TerminateProcess` on
Windows), 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:

The in-code comment at pkg/service/cmd/service.go:356-364 documents that on Windows, GenerateConsoleCtrlEvent "fails when caller and target lack a shared console; Kill (TerminateProcess) then bypasses graceful shutdown." Since rdd.exe typically launches the daemon as a detached background process, this fallback is the common Windows path, not a corner case — graceful hostagent shutdown then relies on killOrphanedHostagent at next start. The doc currently captures only the deadline-expiry fallback.

	}

	pid := PID()
	// Try graceful shutdown first. On Unix, Kill already sends SIGTERM which
	// triggers the Go signal handler. On Windows, Kill uses TerminateProcess
	// which bypasses all handlers, so we send Interrupt (CTRL_BREAK_EVENT)
	// first to let the service run its shutdown path (shutdownAllHostagents).
	//
	// On Unix, Interrupt (SIGINT) always succeeds for a valid PID, so the
	// 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)
		}
	}

Fix: extend the parenthetical, e.g.:

> On Windows, when the CLI and daemon do not share a console, CTRL_BREAK_EVENT cannot be delivered and the stop falls back to TerminateProcess immediately; graceful hostagent shutdown does not run, and orphaned hostagents are reaped at the next service start.

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

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

Every other --timeout flag in this PR uses the shape "Timeout for --wait; ignored if --wait=false (0 means wait indefinitely)". Because svc delete deliberately omits --wait, the wording correctly differs — but it then drops the exit-4 contract that cmd_service.md:121 explicitly documents.

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.

Fix: align the wording to surface the exit-4 contract:

-    command.Flags().Duration("timeout", stopWaitTimeout, "Timeout for the control plane to exit before deletion (0 means wait indefinitely)")
+    command.Flags().Duration("timeout", stopWaitTimeout, "Timeout for the control plane to exit before deletion; on expiry rdd exits with code 4 and preserves the instance directory (0 means wait indefinitely)")

@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' {
    # Hard reset: the prior test sent SIGTERM but does not wait for the
    # process to exit, so a plain `rdd svc start` could still see
    # Running()=true and take the already-running branch.
    rdd svc delete --timeout=10s || :
    rdd svc create
    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 --timeout=120s
    rdd svc create

The hard-reset uses --timeout=10s, while every other reset in the suite (e.g. setup_rdd_control_plane) uses --timeout=120s. If the prior test's SIGTERM ever takes longer than 10s on slow CI, the reset times out, rdd svc create errors with "instance already exists", and run -4 on the subsequent line misreports the failure as "expected exit 4 but got 1". The comment also reads slightly off: the prior test does wait (1ms) before the synchronous Kill fallback fires.

Fix: bump the reset to --timeout=120s for parity with the rest of the suite, or move the reset into a local_setup so each @test starts from a known daemon state independent of the prior test's force-exit timing.