This is a draft PR. The change is structurally sound and I'm comfortable recommending it graduate to human review.
What it does. The PR lifts the distributed-runner's flat startup sequence in runner_start.go into a proper boot graph modelled after the existing components/server pattern. ~300 lines of imperative init code give way to a set of typed boot nodes (clusterAccess → nodeStorage + telemetry → sandboxHost → storageAgent + sandboxAgent → nodePresence) wired through the pkg/boot graph. components/containerd gains its own Boot node reusable by both the server and the distributed runner, eliminating the components/server/boot_containerd.go file entirely.
What I verified.
startup.go matches what the old imperative code did. The graph's failure cleanup path in runtime.Start correctly calls runtime.Stop on any partial start, which propagates to each boot node's stop().b.client.HTTP dereferences in boot_telemetry.go lines 93 and 107 are both reachable only when b.client != nil (the outer guard at line 70 sets it when either address is non-empty, and the inner checks are strict subsets of that condition). No nil dereference.stop() in containerd/boot.go is called by the graph's cleanup path even on startup failure (via runtime.Stop in the Start error branch). b.result.Client is set before the readiness check, but stop() closes it, so there's no leak.stopTimeout=0 in the distributed-runner's storage/sandbox/presence boot calls defers to the graph's outer timeout rather than cutting immediately — the pkg/boot/lifecycle.go code only adds a timeout when stopTimeout > 0.resolveRunnerContainerd helper correctly returns ("", "", nil) for an external socket, and containerdBootConfig in boot_containerd.go routes to ExternalBootConfig in that case. The redundant path reconstruction in EmbeddedBootConfig when binDir != "" is harmless.stubIssuer in the test correctly exercises the token-source arm path.One minor note. boot_containerd.go passes a socketPath into EmbeddedBootConfig that is computed by the caller as filepath.Join(options.DataPath, "containerd", "containerd.sock"), but EmbeddedBootConfig also defaults to the same path when its socketPath argument is empty. The explicit argument is harmless — just slightly redundant. Not worth blocking on.
The refactoring is clean and the server startup path is an established reference point for this pattern. The change is ready for human review.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1161,
"verdict": "ready",
"event": "comment",
"summary": "This is a draft PR. The change is structurally sound and I'm comfortable recommending it graduate to human review.\n\n**What it does.** The PR lifts the distributed-runner's flat startup sequence in `runner_start.go` into a proper boot graph modelled after the existing `components/server` pattern. ~300 lines of imperative init code give way to a set of typed boot nodes (`clusterAccess → nodeStorage + telemetry → sandboxHost → storageAgent + sandboxAgent → nodePresence`) wired through the `pkg/boot` graph. `components/containerd` gains its own `Boot` node reusable by both the server and the distributed runner, eliminating the `components/server/boot_containerd.go` file entirely.\n\n**What I verified.**\n- The dependency ordering in `startup.go` matches what the old imperative code did. The graph's failure cleanup path in `runtime.Start` correctly calls `runtime.Stop` on any partial start, which propagates to each boot node's `stop()`.\n- The `b.client.HTTP` dereferences in `boot_telemetry.go` lines 93 and 107 are both reachable only when `b.client != nil` (the outer guard at line 70 sets it when either address is non-empty, and the inner checks are strict subsets of that condition). No nil dereference.\n- `stop()` in `containerd/boot.go` is called by the graph's cleanup path even on startup failure (via `runtime.Stop` in the `Start` error branch). `b.result.Client` is set before the readiness check, but `stop()` closes it, so there's no leak.\n- `stopTimeout=0` in the distributed-runner's storage/sandbox/presence boot calls defers to the graph's outer timeout rather than cutting immediately — the `pkg/boot/lifecycle.go` code only adds a timeout when `stopTimeout \u003e 0`.\n- The `resolveRunnerContainerd` helper correctly returns `(\"\", \"\", nil)` for an external socket, and `containerdBootConfig` in `boot_containerd.go` routes to `ExternalBootConfig` in that case. The redundant path reconstruction in `EmbeddedBootConfig` when `binDir != \"\"` is harmless.\n- Tests cover the graph-validates path, the cascade-failure path (containerd failure prevents sandbox host from starting), and all four telemetry configuration combinations. The `stubIssuer` in the test correctly exercises the token-source arm path.\n\n**One minor note.** `boot_containerd.go` passes a `socketPath` into `EmbeddedBootConfig` that is computed by the caller as `filepath.Join(options.DataPath, \"containerd\", \"containerd.sock\")`, but `EmbeddedBootConfig` also defaults to the same path when its `socketPath` argument is empty. The explicit argument is harmless — just slightly redundant. Not worth blocking on.\n\nThe refactoring is clean and the server startup path is an established reference point for this pattern. The change is ready for human review.",
"posted_to_pr": true,
"draft": true
}