Every concern I raised in my previous review has been addressed — and addressed well.
The victoria ordering fix landed as designed. newVictoriaLogsBoot and newVictoriaMetricsBoot are now Provide-based components that call waitForVictoriaHealth before publishing their victoriaLogsBootOutput / victoriaMetricsBootOutput. newObservabilityBoot takes those typed graph inputs, so the graph enforces the ordering that the old manual WaitForVictoriaLogs call provided.
The etcd readiness regression is gone. The old WaitForReady calls scattered through restartExistingContainer all silently swallowed the error. They've been replaced by waitForHealthy calls that propagate failure, and every failure path now calls ClearRuntimeState() so the component isn't stuck in a phantom-running state. Defaults are applied before the existing-container branch, fixing the port-zero probe that could silently reject a healthy container.
The netdb lifecycle is handled correctly. networkBoot stores the *netdb.NetDB handle, closes it on error via defer, and closes it during stop.
The boot package itself is well-structured. The topological layer sort gives deterministic ordering (sorted by name within each layer), Output.Value() panics on uninitialized or unpublished reads, and the Stop path correctly runs in reverse layer order. The test suite covers the critical invariants: dangling producers caught at validation, consumers not started after producer failure, reverse-order shutdown, per-component stop timeouts, and the ResolvedOutput edge case.
The one remaining pre-existing note from CodeRabbit (the behind-proxy-https / HTTP-01 config rejection) is also fully implemented — line 71-72 of boot_ingress.go now returns a clear error requiring DNS-01 or self-signed in that mode.
This is ready to merge.
Posted to the PR's comment threads when you submit.
newObservabilityBoot now takes boot.Output[victoriaLogsBootOutput] and boot.Output[victoriaMetricsBootOutput] as typed graph inputs, and both victoria components call waitForVictoriaHealth before publishing. The graph enforces the ordering the old manual wait provided. Satisfied, resolving.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1100,
"verdict": "ready",
"event": "approve",
"summary": "Every concern I raised in my previous review has been addressed — and addressed well.\n\n**The victoria ordering fix landed as designed.** `newVictoriaLogsBoot` and `newVictoriaMetricsBoot` are now `Provide`-based components that call `waitForVictoriaHealth` before publishing their `victoriaLogsBootOutput` / `victoriaMetricsBootOutput`. `newObservabilityBoot` takes those typed graph inputs, so the graph enforces the ordering that the old manual `WaitForVictoriaLogs` call provided.\n\n**The etcd readiness regression is gone.** The old `WaitForReady` calls scattered through `restartExistingContainer` all silently swallowed the error. They've been replaced by `waitForHealthy` calls that propagate failure, and every failure path now calls `ClearRuntimeState()` so the component isn't stuck in a phantom-running state. Defaults are applied before the existing-container branch, fixing the port-zero probe that could silently reject a healthy container.\n\n**The netdb lifecycle is handled correctly.** `networkBoot` stores the `*netdb.NetDB` handle, closes it on error via `defer`, and closes it during `stop`. \n\n**The `boot` package itself is well-structured.** The topological layer sort gives deterministic ordering (sorted by name within each layer), `Output.Value()` panics on uninitialized or unpublished reads, and the `Stop` path correctly runs in reverse layer order. The test suite covers the critical invariants: dangling producers caught at validation, consumers not started after producer failure, reverse-order shutdown, per-component stop timeouts, and the `ResolvedOutput` edge case.\n\nThe one remaining pre-existing note from CodeRabbit (the `behind-proxy-https` / HTTP-01 config rejection) is also fully implemented — line 71-72 of `boot_ingress.go` now returns a clear error requiring DNS-01 or self-signed in that mode.\n\nThis is ready to merge.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6dUkX8",
"body": "The fix landed exactly as the suggestion described — `newObservabilityBoot` now takes `boot.Output[victoriaLogsBootOutput]` and `boot.Output[victoriaMetricsBootOutput]` as typed graph inputs, and both victoria components call `waitForVictoriaHealth` before publishing. The graph enforces the ordering the old manual wait provided. Satisfied, resolving.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true
}I checked the current state, including every resolved thread and the new commit (128af8fa) that landed since my last review.
What changed in the latest commit. Two more real bugs were fixed:
etcd.go defaulting order — defaults are now resolved at the top of Start, before the existing-container branch calls restartExistingContainer. The restart path's waitForHealthy call therefore uses the resolved port, not a zero. The commit also upgrades WaitForReady-then-ignore-error to a stricter waitForHealthy that calls the etcd Status API and propagates errors, so a readiness failure now stops startup rather than silently continuing.quotaBackendBytes data race — the field is now atomic.Int64 with .Store() in Start and .Load() in the maintenance goroutine everywhere it's accessed. The comment even calls out the concurrent Start/maintenance-loop race that motivated this. Clean fix.buildkit.waitForReady — now bounded by a single 60-second readyCtx and exercises ListWorkers (not just a dial) so it proves API reachability, not just socket existence. The loop is now for {} that breaks out on readyCtx.Done(), which closes the old fixed-iteration gap.All six threads I opened or that were flagged by CodeRabbit have been resolved and addressed in the code. The core concerns from my initial review — observability graph ordering, network/netdb lifecycle, ingress DNS validation — remain intact and correct. The pkg/boot core is unchanged and sound.
This PR is ready to merge.
Verdict: ready
The concrete issue I raised in my previous review — that observabilityBoot pre-computed victoria{logs,metrics} addresses before those embedded servers had started, without a graph-level ordering dependency — has been fully resolved. newObservabilityBoot now takes boot.Output[victoriaLogsBootOutput] and boot.Output[victoriaMetricsBootOutput] as typed graph inputs (via boot.Provide3), and the addresses are consumed inside the start function, which only executes after those outputs are produced. Both victoria components call waitForVictoriaHealth before publishing their output, so by the time observability starts, the services are reachable. The ordering constraint that was implicit in the old WaitForVictoriaLogs call is now explicit in the graph — exactly the right fix.
The other issues surfaced since my last review have also been addressed in the same commit: the behind-proxy-https mode now rejects an empty AcmeDNSProvider with a clear error (boot_ingress.go:71-73), and networkBoot properly stores and closes the netdb handle on error and in stop.
Everything I looked at in my original review still holds: the pkg/boot core is sound, the test coverage is appropriate, and the structural improvement over the original monolithic startup function is genuine. This draft is ready to graduate to human review.
Posted to the PR's comment threads when you submit.
newObservabilityBoot now takes boot.Output[victoriaLogsBootOutput] and boot.Output[victoriaMetricsBootOutput] as typed graph inputs, and both victoria components are Provide-based and call waitForVictoriaHealth before publishing their output. The graph now enforces the ordering that the old WaitForVictoriaLogs call provided manually. I'm satisfied with this.Verdict: ready
This is a draft PR, so I'm assessing whether the change is ready to graduate to human review. The answer is almost, but there's one concrete issue worth addressing first.
The PR replaces a ~1,360-line imperative startup function in cli/commands/server.go with a typed dataflow graph (pkg/boot) and a set of boot_*.go component files in components/server/. Each component declares its inputs as typed boot.Output[T] values, and the graph derives startup and shutdown ordering from those edges at validation time. This is a meaningful structural improvement: ordering constraints are now explicit and statically checkable rather than implicit in the sequential order of code.
pkg/boot packageThe core is sound. The Kahn's-algorithm topological sort, the completion channel pattern, the layer-based concurrent startup and reverse-layer shutdown — all correct. The node.attempted / node.completed distinction for deciding which components to stop is clean. The per-component timeout isolation in Stop is a nice detail. The test suite covers the key contracts: ordering, isolation of producer failures from consumers, ResolvedOutput as a graph-escape hatch, stop running on a partially-started component.
One subtle thing worth noting: Graph.Stop walks the pre-computed layers slice even when Start never completed (it checks g.validated, not g.running). That's intentional per the code comment and works because n.attempted.Load() || n.completed.current() gates which nodes actually get stopped.
In observabilityInputs (boot_observability.go), the victoria{logs,metrics} addresses are computed eagerly at wiring time:
if options.Config.Victorialogs.GetStartEmbedded() {
logsAddress = localAddress(options.Config.Victorialogs.GetHTTPPort())
}
This works fine for embedded components, because the embedded server will bind that port. But the address is committed before victorialogs/victoriametrics start — so the observability component (which runs after tracing, which is before victoria*) constructs its writers pointing at an address nothing is listening to yet. The old code had a WaitForVictoriaLogs call precisely because of this; the new code removes that wait without restoring the readiness guarantee. In practice the batch writer will retry, but it's a latent ordering issue that the graph doesn't enforce.
This is the same class of problem the graph was introduced to prevent, so it stands out. The graph expresses that observability depends on tracing but not that it depends on the victoria components. If the VictoriaLogs address is taken from the embedded component's output rather than pre-computed, the graph could enforce it. That would be a cleaner fix, though it requires changing what newVictoriaLogsBoot returns (Run → Provide exposing the endpoint).
server_signals.go is clean.server_prepare.go (releaseDownloadDestination) is a readable extraction of the old inline logic.runnerBoot.started / b.containerd fields written by start() and read by stop(): not a race — the graph's Stop only calls a node's stop after start() has returned, by construction.TestStartupGraphValidates wires the whole graph without starting it and checks that Validate() passes — this is the right canary test.The draft is in good shape. The observability ordering gap is the one thing I'd want resolved before merging, but it's scoped and fixable.
observabilityInputs before those embedded servers have started. The old code called WaitForVictoriaLogs to gate on readiness; that wait is gone here. The graph doesn't express a dependency from observability → victoria*, so the writer and reader are created pointing at addresses nothing may be listening on yet. This is the kind of ordering guarantee the boot graph exists to enforce. Consider changing newVictoriaLogsBoot/newVictoriaMetricsBoot from Run* to Provide* returning the bound endpoint string, so observabilityBoot can take those outputs as typed inputs — that makes the ordering explicit and the retry logic optional.Verdict: caveats