I re-checked the PR in its current state (598a2ba), reading all threads, the top-level comments, and the relevant files directly. Nothing has changed from the analysis I documented in my previous review — all the concerns that were open are addressed in the code.
Every item from my prior review is confirmed resolved:
CloudControl owns cancel and wg; Stop calls c.cancel() then c.wg.Wait(). ControlPlane.Stop calls c.cloud.Stop() first.ControlPlane stores all six components (including runnerEndpoints); Stop guards each with a nil check and calls them all.ControlPlane.Start uses a named retErr return and a deferred 30-second c.Stop() rollback for partial-startup failures.ClusterAccess.Start uses a named retErr return and defers rs.Close() (plus r.state = nil) on any failure after the RPC state is created.NodePresence has a closed bool field guarded by sessMu, checked at the top of establishSession and again atomically before publishing the session — correctly handling the race between session publication and shutdown.Two CodeRabbit threads still show as unresolved in the UI (the ControlPlane.Start partial-startup rollback and the ClusterAccess.Start RPC cleanup). Both are fully addressed in the current code; they just haven't been closed in the thread list. Since they belong to CodeRabbit, I'm noting this here rather than closing them.
This is still a draft, which is fine — the substance is solid and it's ready to graduate to human review.
Posted to the PR's comment threads when you submit.
ControlPlane.Start in the current head. Lines 98–111 show the (retErr error) named return and the deferred cleanup that calls c.Stop(cleanupCtx) on any failure — exactly what was asked for. This is fully addressed.ClusterAccess.Start in the current head (lines 432–479). It now uses a named retErr return and defers a cleanup that calls rs.Close() and clears r.state whenever retErr != nil. The concern is fully addressed.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1158,
"verdict": "ready",
"event": "comment",
"summary": "I re-checked the PR in its current state (`598a2ba`), reading all threads, the top-level comments, and the relevant files directly. Nothing has changed from the analysis I documented in my previous review — all the concerns that were open are addressed in the code.\n\n**Every item from my prior review is confirmed resolved:**\n\n- `CloudControl` owns `cancel` and `wg`; `Stop` calls `c.cancel()` then `c.wg.Wait()`. `ControlPlane.Stop` calls `c.cloud.Stop()` first. \n- `ControlPlane` stores all six components (including `runnerEndpoints`); `Stop` guards each with a nil check and calls them all. \n- `ControlPlane.Start` uses a named `retErr` return and a deferred 30-second `c.Stop()` rollback for partial-startup failures. \n- `ClusterAccess.Start` uses a named `retErr` return and defers `rs.Close()` (plus `r.state = nil`) on any failure after the RPC state is created. \n- `NodePresence` has a `closed bool` field guarded by `sessMu`, checked at the top of `establishSession` and again atomically before publishing the session — correctly handling the race between session publication and shutdown.\n\n**Two CodeRabbit threads still show as unresolved in the UI** (the `ControlPlane.Start` partial-startup rollback and the `ClusterAccess.Start` RPC cleanup). Both are fully addressed in the current code; they just haven't been closed in the thread list. Since they belong to CodeRabbit, I'm noting this here rather than closing them.\n\nThis is still a draft, which is fine — the substance is solid and it's ready to graduate to human review.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6fYFzp",
"body": "I've re-read `ControlPlane.Start` in the current head. Lines 98–111 show the `(retErr error)` named return and the deferred cleanup that calls `c.Stop(cleanupCtx)` on any failure — exactly what was asked for. This is fully addressed."
},
{
"thread_id": "PRRT_kwDONZ__ss6fYFz_",
"body": "I've re-read `ClusterAccess.Start` in the current head (lines 432–479). It now uses a named `retErr` return and defers a cleanup that calls `rs.Close()` and clears `r.state` whenever `retErr != nil`. The concern is fully addressed."
}
],
"posted_to_pr": true,
"draft": true
}I reviewed this PR again in its current state, checking all prior threads and reading the changed files directly.
CloudControl goroutine cleanup — CloudControl now owns a cancel context.CancelFunc and a sync.WaitGroup. Start creates a derived context, stores the cancel, and launches reportStatusPeriodically through c.wg.Go. Stop calls c.cancel() then c.wg.Wait(), so shutdown blocks until the goroutine exits. ControlPlane.Stop now calls c.cloud.Stop() first, before tearing down maintenance and the rest. The compatibility path is fully symmetric.
RunnerEndpoints retained on ControlPlane — NewControlPlane now allocates (or accepts) a *RunnerEndpoints and stores it as c.runnerEndpoints. ControlPlane.Stop calls c.runnerEndpoints.Stop(). If RunnerEndpoints ever grows a meaningful Stop, it will be called correctly.
ControlPlane.Start now has a defer-based rollback on partial startup: if any later step fails, it calls c.Stop with a fresh 30-second context, so previously started components aren't orphaned. That was the CodeRabbit concern about partial startup, and it's handled cleanly.
ClusterAccess.Start now uses a retErr-named return and a deferred cleanup to close the RPC state if a later step in the same function fails — addressing the CodeRabbit concern about the etcd client leaking on mid-startup errors.
NodePresence now has a closed bool field guarded by sessMu. establishSession checks it at the top and again atomically before publishing the session, returning a sentinel errNodePresenceClosed if it races with Close. superviseSession's retry loop also checks r.closed before each attempt and exits cleanly on the sentinel. The race CodeRabbit flagged is covered.
The decomposition is coherent, the lifecycle symmetry is now correct across all paths, and the failure-cleanup work is solid. I'm comfortable marking this ready to graduate to human review.
Posted to the PR's comment threads when you submit.
ControlPlane now has a runnerEndpoints *RunnerEndpoints field populated in NewControlPlane, and ControlPlane.Stop calls c.runnerEndpoints.Stop(). Exactly what I asked for.CloudControl now owns cancel and wg fields; Stop calls c.cancel() then c.wg.Wait(), and ControlPlane.Stop invokes it first. The lifecycle is fully symmetric and no longer needs a comment to explain the omission — because it's no longer omitted.Verdict: ready
This is a draft PR, so the question is whether it's ready to hand to human reviewers — I think it nearly is, with one real concern and one design question worth resolving first.
The monolithic Coordinator struct (1800+ lines in coordinate.go) is split into six focused types — Foundation, SecretStore, ApplicationManagement, WorkloadControl, EntityMaintenance, CloudControl — stitched together into a ControlPlane. On the runner side, Runner is split into ClusterAccess, NodeStorage, SandboxHost, SandboxAgent, NodePresence, and StorageAgent, each independently startable. The server boot graph gets a matching set of fine-grained boot_*.go components. A new boot.DependsOn option adds order-only edges to the graph without publishing a value. The result is a meaningful staged-startup model: foundation is ready before management, management before workload control, and so on.
The decomposition is conceptually clean. Foundation.Start uses context.WithoutCancel precisely at the right moment — it keeps RPC alive across graph cancellation so dependents can still make their final coordinator calls before the stop hook drains it. CloudControl.Stop intentionally delegating to its context-bound goroutines (rather than calling Foundation.Stop, which is correct since Foundation owns the actual shutdown) is carefully documented. The DependsOn tests cover ordering and reverse-shutdown order explicitly and correctly. The TestWorkAdmissionWaitsForExecutionCapabilities test now validates the right boundary (workload control, not just runner/buildkit).
ControlPlane.Stop does not stop CloudControl. cloud.Stop() is a deliberate no-op — the comment says its goroutines are context-bound — but ControlPlane.Stop is the path taken by test/embedded callers that call ControlPlane.Start directly (the "compatibility path" per the comment). In that path CloudControl.Start launches reportStatusPeriodically as an untracked goroutine with a copy of the caller's ctx. When Stop is called and the caller's context is cancelled, those goroutines do exit; but Stop returns before they finish, so the caller has no way to know they're done. For production the server boot graph cancels the context before calling stop hooks, so it works correctly. For tests and embedded callers it leaves goroutines running until GC. This is low-blast-radius today (status-reporting goroutines, not anything stateful), but the asymmetry between Start and Stop in the compatibility path is worth at least a comment documenting the design decision, if not a wait-group.
ControlPlane.Start creates a RunnerEndpoints instance on the fly but doesn't retain it for Stop. NewRunnerEndpoints(c.Foundation).Start(ctx) is called, the returned pointer is thrown away. If RunnerEndpoints ever acquires lifecycle (Stop), this will silently fail to clean up. Worth retaining the instance on ControlPlane even if it's currently just a pass-through.
NewRunnerEndpoints(c.Foundation).Start(ctx) — the constructed RunnerEndpoints is not stored anywhere. If it ever gains a Stop method, the ControlPlane.Start compatibility path will silently skip cleanup. Consider retaining the instance as a field, analogous to how the server boot graph owns each component separately.ControlPlane.Stop does not stop the cloud component. CloudControl.Stop is an intentional no-op, but the goroutines launched in CloudControl.Start (reportStatusPeriodically) are context-bound and exit only when the caller's context is cancelled, which may happen after Stop returns. For the server boot graph this is fine (context is cancelled before stop hooks run), but the compatibility path (ControlPlane.Start + Stop) leaves those goroutines running until the context the caller passed to Start is cancelled. Please add a comment to ControlPlane.Stop explaining why cloud is intentionally omitted here, so future readers don't add a no-op call thinking it's missing.Verdict: caveats