Deep Review: 20260428-102700-pr-333

}

@test "moby engine sets currentContext when no healthy context exists" {
    local context_name="rancher-desktop-${RDD_INSTANCE}"

    # Seed a named context pointing at an unreachable socket and make it
    # current. This forces the reconciler into the probeNamedDockerContext
    # branch with a guaranteed-failing endpoint, independent of whether the
    # host has a running Docker daemon at the default socket (e.g.
    # ubuntu-latest runners ship Docker pre-installed). Using a named context
    # also avoids the stale-env issue: the service process's DOCKER_HOST is
    # frozen at first start, so FromEnv-based probing of "" / "default" is
    # host-dependent and unreliable from inside a long-lived daemon.
    probe_target="test-unreachable"
    run_e -0 docker_context_dir "${probe_target}"
    probe_dir="${output}"
    mkdir -p "${probe_dir}"
    cat >"${probe_dir}/meta.json" <<EOF

The "Using a named context also avoids the stale-env issue" claim does not hold once round 3's env-skip lands. The skip at engine_reconciler.go:379-381 runs before the named-context probe, so a frozen DOCKER_HOST in the daemon's env disables both branches equally — the named-context detour buys nothing on that axis. The detour only escapes the FromEnv flake when the daemon inherits no env vars at all. Pair this rewrite with the I1 unset so the comment describes the actual contract.

		// vars: the daemon inherits them frozen from the shell that ran
		// rdd svc start and they may not reflect the user's global
		// preference. Rewriting config.json based on them could clobber a
		// correct currentContext for sessions that don't have those vars
		// set. Skip the probe-and-set entirely when either is present.
		if os.Getenv("DOCKER_HOST") != "" || os.Getenv("DOCKER_CONTEXT") != "" {
			return
		}

		var healthy bool
		if current != "" && current != "default" {
			healthy = probeNamedDockerContext(probeCtx, current)
		} else {

-    # Using a named context
-    # also avoids the stale-env issue: the service process's DOCKER_HOST is
-    # frozen at first start, so FromEnv-based probing of "" / "default" is
-    # host-dependent and unreliable from inside a long-lived daemon.
+    # Both probe branches (named and FromEnv) require the daemon's env to
+    # have neither DOCKER_HOST nor DOCKER_CONTEXT set, otherwise
+    # manageDockerContext skips the probe entirely. local_setup_file
+    # unsets both before spawning the daemon.

(suggestion, gap)

	defer cli.Close()
	_, err = cli.Ping(ctx, dockerclient.PingOptions{})
	return err == nil
}

// probeDockerContext pings the implicit default Docker endpoint
// (DOCKER_HOST or the platform default socket) within dockerContextProbeTimeout.
// Used when currentContext is "" or "default".
func probeDockerContext(ctx context.Context) bool {
	probeCtx, cancel := context.WithTimeout(ctx, dockerContextProbeTimeout)
	defer cancel()
	return pingDocker(probeCtx, dockerclient.FromEnv)
}

// probeNamedDockerContext pings the Docker endpoint for the named context
// within dockerContextProbeTimeout and returns true if the endpoint is healthy.
//
// When we cannot determine whether the context is healthy — because the store

By the time probeDockerContext(probeCtx) runs at engine_reconciler.go:387, the goroutine has already returned if either DOCKER_HOST or DOCKER_CONTEXT was set (lines 379-381). So FromEnv's WithHostFromEnv only ever resolves to the platform default socket — DOCKER_HOST is empty by construction here. The docstring's "DOCKER_HOST or the platform default socket" phrasing is reachable from unit tests but never on the production path. Tighten the comment so a future refactor that removes the env-skip does not silently change probe semantics.

		var healthy bool
		if current != "" && current != "default" {
			healthy = probeNamedDockerContext(probeCtx, current)
		} else {
			healthy = probeDockerContext(probeCtx)
		}
		// Guard against writing currentContext after removeDockerContext has
		// already cancelled probeCtx and deleted the context directory.
		if !healthy && probeCtx.Err() == nil {
			if err := setCurrentDockerContext(contextName); err != nil {

 // probeDockerContext pings the implicit default Docker endpoint
-// (DOCKER_HOST or the platform default socket) within dockerContextProbeTimeout.
-// Used when currentContext is "" or "default".
+// (the platform default socket) within dockerContextProbeTimeout. Used
+// when currentContext is "" or "default". The caller short-circuits when
+// DOCKER_HOST or DOCKER_CONTEXT is set, so FromEnv's host-from-env path
+// is unreachable on the production path and the probe always targets
+// the platform default socket.
 func probeDockerContext(ctx context.Context) bool {

(suggestion, documentation)

The new Test_probeNamedDockerContext covers four common outcomes (missing, SSH, unreachable unix, unreachable tcp), but the four "assume healthy" branches at docker_context.go:172-174, 178-180, 188-191, 193-196 (metadata read error, decode error, TLS load error, ClientOpts error) and the TCP+TLS / mTLS / SkipTLSVerify paths motivating the PR have no unit anchor. The malformed-meta case is the cheapest to lock in: write a meta.json with an unparseable endpoint payload and assert the function returns true.

		_, hasAuths := cfg["auths"]
		assert.Assert(t, hasAuths, "auths key must be preserved across clear")
	})
}

func Test_probeNamedDockerContext(t *testing.T) {
	// seedContext writes a named docker context with the given Host into the
	// store. It uses the same store config as production code so the context
	// is readable by probeNamedDockerContext.
	seedContext := func(t *testing.T, name, host string) {
		t.Helper()
		s, err := newContextStore()
		assert.NilError(t, err)
		assert.NilError(t, s.CreateOrUpdate(store.Metadata{
			Name:     name,
			Metadata: map[string]any{},
			Endpoints: map[string]any{
				docker.DockerEndpoint: docker.EndpointMeta{Host: host},
			},
		}))
	}

	t.Run("missing context returns false", func(t *testing.T) {
		newDockerTestDir(t)
		result := probeNamedDockerContext(context.Background(), "does-not-exist")
		assert.Assert(t, !result, "missing context must be treated as unhealthy")
	})

	t.Run("ssh context returns true (non-probed scheme)", func(t *testing.T) {
		newDockerTestDir(t)
		seedContext(t, "ssh-ctx", "ssh://user@remote-host")
		result := probeNamedDockerContext(context.Background(), "ssh-ctx")
		assert.Assert(t, result, "ssh context must be assumed healthy to avoid clobbering user's choice")
	})

	t.Run("unix context with unreachable socket returns false", func(t *testing.T) {
		newDockerTestDir(t)
		seedContext(t, "local-ctx", "unix:///nonexistent/does-not-exist.sock")
		result := probeNamedDockerContext(context.Background(), "local-ctx")
		assert.Assert(t, !result, "unreachable unix socket must be treated as unhealthy")
	})

	t.Run("tcp context with unreachable endpoint returns false", func(t *testing.T) {
		newDockerTestDir(t)
		// Port 1 is reserved and will be refused on any sane host.
		seedContext(t, "tcp-ctx", "tcp://127.0.0.1:1")
		result := probeNamedDockerContext(context.Background(), "tcp-ctx")
		assert.Assert(t, !result, "unreachable tcp endpoint must be treated as unhealthy")
	})
}

A fake context store seeded with TLS data plus an unreachable TLS endpoint would pin the TLS construction without needing a live TLS daemon and close the largest residual gap from round 2 S2.

(suggestion, gap)

The four-line env-var skip and the two-branch probe selector are entirely behavioral additions to the goroutine in manageDockerContext and have no direct test. The BATS path covers a subset (named-unhealthy promotes ours), but no test asserts:

1. setCurrentDockerContext is never called when DOCKER_HOST is set;
2. setCurrentDockerContext is never called when DOCKER_CONTEXT is set;
3. current == "default" routes through probeDockerContext (FromEnv), not the named branch.

A small refactor that extracts the goroutine body into a testable helper (taking getEnv func(string) string and the probe functions as parameters) would make all three reachable from _test.go. Worth doing because the env-skip is the headline behavior of this PR — silent regression of it would not be caught.

(suggestion, gap)

			}
			r.contextMu.Unlock()
			cancel()
		}()

		current, err := getCurrentDockerContext()
		if err != nil {
			// An error here means the config file exists but could not be
			// read or parsed. Do not proceed — writing currentContext now
			// would overwrite a file we cannot safely round-trip.
			log.Error(err, "Failed to read current Docker context")
			return
		}

		// DOCKER_HOST and DOCKER_CONTEXT take precedence over config.json
		// in the Docker CLI's resolution order. Both are shell-local env
		// vars: the daemon inherits them frozen from the shell that ran
		// rdd svc start and they may not reflect the user's global

The early return is the safe choice — setCurrentDockerContext would have failed on the same config.Load call in the old flow. A future reader could mistake the early return for "RDD ignores broken config.json forever"; in fact the next reconcile (e.g., another rdd set) re-enters manageDockerContext, so repairing the file and triggering any reconcile is enough to recover.

     // An error here means the config file exists but could not be
     // read or parsed. Do not proceed — writing currentContext now
-    // would overwrite a file we cannot safely round-trip.
+    // would overwrite a file we cannot safely round-trip. The next
+    // reconcile (e.g. another `rdd set`) re-enters this function, so
+    // repairing config.json restores the probe-and-set behavior.

(suggestion, documentation)

		}))
	}

	t.Run("missing context returns false", func(t *testing.T) {
		newDockerTestDir(t)
		result := probeNamedDockerContext(context.Background(), "does-not-exist")
		assert.Assert(t, !result, "missing context must be treated as unhealthy")
	})

	t.Run("ssh context returns true (non-probed scheme)", func(t *testing.T) {
		newDockerTestDir(t)

Go 1.25 (go.mod) gives every *testing.T a per-test Context() that cancels when the test finishes. Switch the four context.Background() callers in Test_probeNamedDockerContext so test timeouts and cancellations propagate into the probe.

-result := probeNamedDockerContext(context.Background(), "does-not-exist")
+result := probeNamedDockerContext(t.Context(), "does-not-exist")

(suggestion, enhancement)

		e.stop()
	}
	r.removeDockerContext()
}

// manageDockerContext creates the instance Docker context and, in a goroutine,
// probes the user's current context; if it is absent or unhealthy, switches
// the default to the new context. At most one probe runs at a time.
func (r *EngineReconciler) manageDockerContext(socketPath string) {
	// TODO(windows): Docker context management uses unix:// sockets; Windows
	// requires npipe:// in the context meta file, an npipe-aware probe client,
	// and platform-specific path handling in getCurrentDockerContext and
	// createReplaceDockerContext. Track in a follow-up issue before shipping

After round 2 the empty / "default" branch routes through probeDockerContext (FromEnv) and treats a foreign default daemon as healthy; round 3 adds a further env-skip for DOCKER_HOST / DOCKER_CONTEXT. Neither the docstring nor docs/design/bootstrap.md:58,86 reflects either change. Round 2 S3 flagged the same drift and was not picked up.

    * The `LimaVM` controller creates the VM. Since the status is set to `running` it also starts the VM. Once the VM has completed startup the controller sets the status to "ready".

  * The `App` controller is notified of the status change of the `LimaVM` object and detects that the VM is running. It now monitors the VM to check when the `docker.sock` is accepting requests.

  * It creates a `rancher-desktop-2` docker context. If there is no current context, or if the context isn't accepting connections, then it makes the new context the default.

  * It sets the status of the `App` object to `ready`.

* `rdd start` is notified that the `App` status has changed. It verifies that the status is "ready" and exits.

docs/design/bootstrap.md:58
   "It creates a `rancher-desktop-2` docker context. If there is no
   current context, or if the context isn't accepting connections, then
   it makes the new context the default."

docs/design/bootstrap.md:86
   "`rdd start` has created the `rancher-desktop-2` docker context, but
   it will only activate it if the current context doesn't exist, or is
   non-functional."

Fix: rewrite the docstring and both bullets to spell out the new contract — RDD probes the current named context, falls back to probing the implicit default endpoint when none is set, and skips the entire promotion when DOCKER_HOST or DOCKER_CONTEXT is set in the daemon's env.

(suggestion, documentation, round 2 S3 unaddressed)

// manageDockerContext creates the instance Docker context and, in a goroutine,
// probes the user's current context; if it is absent or unhealthy, switches
// the default to the new context. At most one probe runs at a time.
func (r *EngineReconciler) manageDockerContext(socketPath string) {
	// TODO(windows): Docker context management uses unix:// sockets; Windows
	// requires npipe:// in the context meta file, an npipe-aware probe client,
	// and platform-specific path handling in getCurrentDockerContext and
	// createReplaceDockerContext. Track in a follow-up issue before shipping
	// Windows support.
	if runtime.GOOS == "windows" {
		return
	}
	contextName := instance.Name()
	log := logf.FromContext(r.watcherCtx).WithName("docker-context")

getCurrentDockerContext delegates to docker/cli's config.Load, and createReplaceDockerContext plus newContextStore delegate to the store's path handling — both resolve %USERPROFILE%\.docker on Windows through os.UserHomeDir() with no extra work. The Windows-specific work concentrates in one line of createReplaceDockerContext (the hard-coded "unix://" prefix) and in the probe client selection. Round 1 S5 and round 2 S6 flagged this and were not picked up.

-    // TODO(windows): Docker context management uses unix:// sockets; Windows
-    // requires npipe:// in the context meta file, an npipe-aware probe client,
-    // and platform-specific path handling in getCurrentDockerContext and
-    // createReplaceDockerContext. Track in a follow-up issue before shipping
-    // Windows support.
+    // TODO(windows): createReplaceDockerContext hard-codes "unix://"; Windows
+    // needs "npipe://" plus an npipe-aware probe client in
+    // probeDockerContext / probeNamedDockerContext. Track in a follow-up
+    // issue before shipping Windows support.

(suggestion, documentation, round 2 S6 / round 1 S5 unaddressed)

    refute_output
}

# --- Docker context management ---

# docker_context_dir returns the ~/.docker/contexts/meta/<hash> directory for
# the named context. Docker derives the sub-directory from sha256(name).
docker_context_dir() {
    local name="$1"
    local hash
    # sha256sum on Linux, shasum on macOS
    if command -v sha256sum &>/dev/null; then

The body returns ${DOCKER_CONFIG}/contexts/meta/${hash}, but the docstring still says ~/.docker/contexts/meta/<hash>. The two diverged when the round-2 fix isolated tests via DOCKER_CONFIG, and the docstring still has not been updated.

-# docker_context_dir returns the ~/.docker/contexts/meta/<hash> directory for
-# the named context. Docker derives the sub-directory from sha256(name).
+# docker_context_dir returns the contexts/meta/<hash> directory under
+# DOCKER_CONFIG for the named context. Docker derives the sub-directory
+# from sha256(name).

(suggestion, documentation, round 2 S8 unaddressed)