Deep Review: 20260422-160047-pr-336

# SIGTERM (or TerminateProcess on Windows) and returns
# context.DeadlineExceeded wrapped in cliexit.Timeout. svc delete also
# preserves the instance directory on timeout (see docs/design/cmd_service.md)
# so the user can retry.

local_setup_file() {
    rdd svc delete || :
    rdd svc create
    rdd svc start
}

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

The other deletes in the same file pass --timeout=10s (line 26) or rely on a known-clean state (line 37). The file-level rdd svc delete || : on line 12 has no explicit timeout, so it inherits the new 5-minute default. If a prior test file left a half-dead daemon (the comment on line 23 acknowledges that tests exit while the daemon is still draining), this setup can hang the entire test file for 5 minutes before BATS reports anything.

    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 || :
    rdd svc create
    run -4 rdd svc start --wait --timeout=1ms
    assert_output --partial 'context deadline exceeded'
}

The bats/helpers/controller.bash:81 change explicitly bounds its delete at 120s for exactly this reason. The same reasoning applies here.

    local controllers=${1:-"*"}

    # Bound delete and tolerate failure so a stuck prior daemon cannot
    # hang the helper for the full 5m stopWaitTimeout. `create` then
    # proceeds against whatever state survives.
    rdd svc delete --timeout=120s || :
    rdd svc create --controllers="${controllers}"
    rdd svc start
}

Fix:

 local_setup_file() {
-    rdd svc delete || :
+    rdd svc delete --timeout=120s || :
     rdd svc create
     rdd svc start
 }

		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 !waitFlag {
			return nil
		}
		startTime, err := service.StartTime()
		if err != nil {
			return err
		}
		logrus.Infof("waiting for %q control plane to be ready", instance.Name())
		ctx, cancel := watchtools.ContextWithOptionalTimeout(cmd.Context(), timeout)
		defer cancel()
		if err := service.Wait(ctx); err != nil {
			return cliexit.Classify(err)
		}
		if err := waitForFreshDiscoveryConfigMap(ctx, startTime); err != nil {
			return cliexit.Classify(err)
		}
		logrus.Infof("%q control plane is ready", instance.Name())
		return nil
	}

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

Running() (line 262) reads the PID file, validates the process is alive, and removes the PID file if the process is gone (pkg/service/cmd/service.go:158). StartTime() (line 267) calls os.Stat(instance.PIDFile()). If the process exits between line 262 and line 267 (e.g., a concurrent rdd svc stop) and the PID file is removed, StartTime() returns a raw os.Stat: ENOENT error. The window is microseconds and harmless in practice, but the surfaced error is opaque and not Classify-able.

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

The same race exists in ensureServiceRunning (line 99). Round 6 raised this; this round has not addressed it.

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

Fix: when StartTime() returns os.IsNotExist, retry the Running() check or return a clearer "control plane exited" message:

startTime, err := service.StartTime()
if err != nil {
    if os.IsNotExist(err) {
        return fmt.Errorf("%q control plane exited while waiting", instance.Name())
    }
    return err
}

// stopWaitTimeout bounds the CLI wait for the control plane to shut down.
// Run caps the internal drain at 45s (see pkg/service/cmd/service.go).
// The CLI deadline exceeds that cap to absorb slow disks, misconfigured
// hosts, and sequential multi-VM drains. Aliasing to limaLongWaitTimeout
// makes the per-VM ceiling the single tuning knob for long CLI waits.
const stopWaitTimeout = limaLongWaitTimeout

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

limaLongWaitTimeout is declared in cmd/rdd/limavm.go:42. The two files share the main package, so this compiles, but a reader looking at service.go only has the comment to discover where limaLongWaitTimeout lives. The "single tuning knob" rationale is fine; the implementation makes the file dependency invisible.

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

// limaLongWaitTimeout is the default --timeout for VM boot and shutdown waits,
// which routinely take minutes.
const limaLongWaitTimeout = 5 * time.Minute

func newLimaVMCommand() *cobra.Command {
	command := &cobra.Command{
		Use:     "limavm",
		Short:   "Manage LimaVM resources",

Either move both constants to a small shared file, or qualify the comment with the file location:

-// hosts, and sequential multi-VM drains. Aliasing to limaLongWaitTimeout
-// makes the per-VM ceiling the single tuning knob for long CLI waits.
+// hosts, and sequential multi-VM drains. Aliased to limaLongWaitTimeout
+// (defined in limavm.go) so the per-VM ceiling is the single tuning knob
+// for long CLI 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 {
			// 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 only caller is serviceDeleteAction (cmd/rdd/service.go:368-372), which short-circuits on !service.Exists() before calling service.Delete. The library-side guard is dead code, and if it ever fired, it would propagate as a CLI error (exit 1) rather than the action's "info + success".

	}
	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() {
		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(cmd.Context(), timeout); err != nil {

Either remove the inner check (and document Delete's precondition that the instance exists) or have Delete log+return-nil to match the action. Removal is the smallest change.

					_ = 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 {
	// Try to get kubeconfig, but ignore errors since control plane might be stopped

The !wait branch falls through to remove instance.Config() (the kubeconfig the CLI uses to talk to the apiserver) immediately after sending SIGINT. The daemon may still be alive and serving for tens of seconds. Any concurrent rdd ctl get / rdd kubectl call between stop's return and the daemon's actual exit will fail with "could not read config" (pkg/service/cmd/service.go:95), masking the real state ("daemon is still up").

	if !Running() {
		return nil, fmt.Errorf("control plane %q is not running", instance.Name())
	}
	data, err := os.ReadFile(instance.Config())
	if err != nil {
		return nil, fmt.Errorf("could not read config from %s: %w (control plane may still be starting)", instance.Config(), err)
	}
	return data, nil
}

// GetKubeRestConfig generates and returns the kubeconfig as a *rest.Config.

This predates the PR but the new contract makes --wait=false more attractive (per the docs), so the symptom will get more exposure. The fix is to gate the removal on a confirmed-exited check, or to leave the kubeconfig in place when !wait (it will be re-created on next start).

func StopWithWait(ctx context.Context, wait bool, timeout time.Duration) error {
	if !Running() {
		return fmt.Errorf("%q control plane is not running", instance.Name())
	}

	// Delete the discovery configmap before signaling so the kube-API
	// delete still has a live apiserver. Trade-off: a timed-out --wait
	// leaves the configmap deleted while the daemon drains, so a quick
	// `rdd svc start` polls the missing configmap until startWaitTimeout.
	// Moving cleanup into the daemon's own shutdown path is the durable fix.
	_ = cleanupDiscoveryConfigMap()

	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)

Three concerns, all pre-existing but in scope of this PR's "wire context through" theme:

1. The function is not bounded by the user's --timeout (it runs before watchtools.ContextWithOptionalTimeout).
2. CleanupDiscovery is called with context.TODO() (pkg/service/cmd/service.go:433), so it has no deadline at all.
3. The author already documented the trade-off ("Moving cleanup into the daemon's own shutdown path is the durable fix").

	if err != nil {
		logrus.WithError(err).Debug("Could not create kubernetes client for discovery cleanup")
		return nil // Not a critical error during shutdown
	}

	err = controllers.CleanupDiscovery(context.TODO(), client)
	if err != nil {
		logrus.WithError(err).Debug("Failed to delete discovery configmap")
	}

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

Listed because the PR did the bulk of the context plumbing for Stop/Delete and this single TODO is now the loudest holdout.

Fix: thread ctx into cleanupDiscoveryConfigMap, or move the cleanup into the daemon's own shutdown path as the comment proposes.

The deadline-expiry SIGTERM is the second shutdown signal (the first SIGINT was sent earlier in the same call), and the apiserver's SetupSignalContext handler responds to a second signal by calling os.Exit(1). The current comment reads as if SIGTERM lets the daemon "handle signals" gracefully, but the second signal forces an abrupt termination — which is the desired behavior, just not what the comment describes.

-                // On Unix, SIGTERM suffices because the service handles
-                // signals promptly; its slowness comes from draining
-                // hostagents sequentially.
+                // On Unix, SIGTERM is the *second* shutdown signal
+                // (SIGINT was sent at line 374); the apiserver signal
+                // handler responds to a second signal by calling
+                // os.Exit(1), forcing immediate termination — which is
+                // exactly what the deadline path needs.

}

@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_e -0 invokes rdd limavm create, but assert_created reads ${stderr} set by the previous run_e. If the helper relies on ${stderr}, the wrapper is justified; otherwise per the BATS style notes, commands whose success is the entire assertion should be invoked directly. Verify by reading assert_created: if it inspects $stderr, leave the call as-is; if not, drop the wrapper.