Repository navigation
Broker concurrent teardown fix - #173
Open
Noah-Tervalon-Nvidia wants to merge 9 commits into
Open
Noah-Tervalon-Nvidia wants to merge 9 commits into
Noah-Tervalon-Nvidia wants to merge 9 commits into
Conversation
Serve joined its ten workers through ten sequential `defer sup.Stop()` calls, so teardown cost the sum of every worker's exit rather than the slowest one. The proxy and node-info both drain HTTP servers on their way out, so on a loaded tree that was several drains in a row. Each started worker is now registered with trackWorker, and one deferred stopWorkers joins them all at once. It stops the proxy first on every exit path, including a Serve that returns before shutdownInferenceStack, so no inference arrives while engine-manager stops its engines. Only workers whose first spawn succeeded are joined, since supervisor.Stop waits on a monitor goroutine that only a successful Start launches. graceFor drops the engine-manager reservation, which existed only because engine-manager was joined eighth of eleven. Joined together, every worker reads the same remaining budget at the same moment. Signed-off-by: Terve <ntervalon@nvidia.com>
supervisor.Stop names a single worker that takes longer than workerStopReportAfter, and the escalation warnings name a worker that misses its grace, but nothing accounts for the whole teardown: not the engine sweep that engine:prepare-shutdown waits for, nor phases that are each prompt yet together cross the threshold. When teardown takes longer than 2s from beginTeardown to the last worker join, the broker now logs one line with the total, the proxy and engine-sweep phases, and each worker's join, slowest first. It logs at warn because the desktop runs the broker at --log-level warn, so an info-level report would be missing from exactly the logs collected when a user reports that quitting is slow. Signed-off-by: Terve <ntervalon@nvidia.com>
The broker README, services-backend.md, and services-parity.md said the broker waits for each worker to exit with no timeout and no force-kill. The joins have been bounded for some time: a worker that misses its grace is terminated and then killed, except engine-manager on Windows, whose join is abandoned rather than killed mid-stop. They now say so, and that the joins run concurrently within one shared teardown budget, with a warn-level breakdown when teardown is slow. Signed-off-by: Terve <ntervalon@nvidia.com>
kjlubick
approved these changes
Oct 15, 2026
Signed-off-by: Terve <ntervalon@nvidia.com> # Conflicts: # services/nvpair-ui-broker/stopwait_test.go
Local constants in a const block read oddly; plain short declarations match the rest of the package. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Terve <ntervalon@nvidia.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Terve <ntervalon@nvidia.com>
Makes later diffs of this bullet readable. No wording changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Terve <ntervalon@nvidia.com>
The parity doc said a managed engine's stop waits with no timeout and is never escalated to SIGKILL. managedProc.stop waits out the manifest's stop grace and then force-kills, as it has since 0.1.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Terve <ntervalon@nvidia.com>
Name the new teardown tests Test<Subject>_<ExpectedBehavior> and assert with testify, matching the rest of stopwait_test.go after #160. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Terve <ntervalon@nvidia.com>
kjlubick
approved these changes
Oct 29, 2026
kjlubick
left a comment
kjlubick
left a comment
Collaborator
There was a problem hiding this comment.
Don't the new commits need a Signed Off message? Or is that handled by the squash?
Noah-Tervalon-Nvidia
force-pushed
the
perf/broker-concurrent-teardown
branch
2 times, most recently
from
October 29, 2026 19:31
1dc9777 to
7e65dc6
Compare
kjlubick
approved these changes
Oct 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The broker joined its ten workers through ten sequential
defer sup.Stop()calls, so quitting cost the sum of every worker's exit rather than the slowest one. The proxy and node-info both drain HTTP servers on their way out, so on a loaded machine that was several drains in a row.Each started worker is now registered with
trackWorker, and one deferredstopWorkersjoins them all at once. It stops the proxy first on every exit path, including aServethat returns beforeshutdownInferenceStack, so no inference arrives while engine-manager stops its engines.graceFordrops the engine-manager reservation, which existed only because engine-manager was joined eighth of eleven.When teardown takes longer than 2s, the broker logs one line at warn, the level the desktop runs it at, with the total, the proxy and engine-sweep phases, and each worker's join.
The docs said the broker waits for each worker with no timeout and no force-kill. The joins were already bounded with a terminate-then-kill escalation, so the broker
README.md,services-backend.mdandservices-parity.mdnow describe that, along with the concurrent joins.Release intent
Changelog title
Background services shut down in parallel
Changelog body
Bumps
Scope
In: concurrent worker joins, proxy-first on every exit path, the
graceForchange, the slow-teardown log, and the teardown docs.Out: the engine-manager single sweep (#137) and the startup-gate doc correction, which are separate pull requests. The three touch disjoint files and merge cleanly in any order. There is one proxy process, so closing ingress is still a single join and
shutdownInferenceStackkeeps its order.Validation
On macOS (Apple silicon), on current
develop:TestStopWorkersJoinsConcurrently,TestStopWorkersStopsTheProxyFirst,TestStopWorkersSkipsUnstartedTree,TestGraceForSharesTheRemainingBudget,TestTeardownReportCoversThePhasesAheadOfTheJoins.cd services/nvpair-ui-broker && go test ./... -count=1: pass.-race: pass.cd services/tests && go test ./... -count=1: pass (345s).npm run service-contracts:check: pass.Risk
Workers are no longer joined in a fixed order. That is safe because by then ingress is closed and the engine-manager request has returned, which are teardown's only two dependencies. One property of the old order goes away: a worker that crashes during teardown may no longer reach
nvpair-errors, whose output is being discarded at that point anyway.Checklist
git commit -s), certifying the Developer Certificate of Origin.services/versions.jsonis written by automation — do not edit it by hand.Review order
services/nvpair-ui-broker/stopwait.go:stopWorkers, thegraceForchange, andreportTeardown.services/nvpair-ui-broker/broker.go: mostly mechanical. Eachdefer .Stop()becomestrackWorker, andServedefersstopWorkers. Worth checking that every started worker is tracked exactly once.services/nvpair-ui-broker/stopwait_test.go.README.md,desktop/docs/services-backend.md,desktop/docs/services-parity.md.