Coordinate provider starts with bridge retirement - #2777
Conversation
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request for security, code quality, performance, architecture, and duplication. I will also run the relevant tests. I will test the full user path when this internal lifecycle change has one. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| async ensureProvider(args: EnsureRuntimeProviderArgs): Promise<void> { | ||
| const retirement = this.providerRetiring.get(args.processKey); | ||
| if (retirement !== undefined) { | ||
| await retirement; |
There was a problem hiding this comment.
🚨 slopcop/review — A provider can start after full runtime shutdown.
ensureProvider() can wait here while shutdown() sets shuttingDown and clears the current process map. When retirement finishes, this method continues to spawnProvider() without another shutdown check. The new child misses the shutdown scan. Its output handlers then ignore all output because shuttingDown remains true.
I reproduced this on the exact head with a focused test. The test starts retirement, calls ensureProvider(), and then calls shutdown(). It fails because requireProviderProcess() still returns a live replacement after shutdown.
Please reject starts after shutdown, check the state again after this wait, and add the overlap test. shutdown() should also drain active starts and retirements.
There was a problem hiding this comment.
Addressed in d2c2dd7 without adding a shutdown rejection path. Full shutdown now waits for active retirement and the same-key start it releases, then performs its existing final process sweep. The regression asserts that retirement, ensure, and shutdown all fulfill while no provider survives. Full @bb/agent-runtime test/typecheck passes (22 files, 318 tests). @bb-slop-cop please re-check.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change makes a new provider process wait for an older same-key process to stop.
That wait prevents a thread from reusing a provider process during retirement.
Review result: I found one blocking lifecycle race.
Main finding
- A provider can start after full runtime shutdown. I reproduced the issue in this inline finding.
- The new wait can finish after
shutdown()clears its process map. ensureProvider()then starts a child that missed the shutdown scan.- Please reject starts after shutdown and recheck the state after lifecycle waits.
- Please add a test that overlaps retirement,
ensureProvider(), and full shutdown.
Other review areas
- Security: I found no direct privilege increase or input validation issue.
- Performance: The hot path adds one keyed map lookup. The map cleans each promise in a
finallyblock. - Architecture: The keyed promise pattern exists elsewhere, but a shared helper would cross current package boundaries.
- Refactor: A shared lifecycle state could replace the two promise maps later. This refactor is not required for this fix.
- Tests: The new test covers replacement after retirement. It does not cover two concurrent shutdown callers.
Verification
git diff --checkpassed.- The new process lifecycle test passed.
- The package typecheck passed.
- The full package run passed 315 of 317 tests.
- The Codex timeout passed when I ran it alone.
- The ACP timeout also failed against the base implementation. It does not result from this change.
- My focused shutdown-overlap test failed because a live replacement remained after shutdown.
- GitHub package, integration, server, and app test jobs passed.
- The Linux package smoke job failed outside these files.
- A browser test did not apply. This internal process change has no server route or user interface path.
Please fix the shutdown overlap before merge. I found no other blocking issue.
3153dfa to
d2c2dd7
Compare
|
@bb-slop-cop please re-review the current head d2c2dd7. The blocking shutdown-overlap finding is addressed without rejecting starts: full shutdown drains the retirement-released same-key start and then performs its final sweep. The regression now proves retirement, start, and shutdown all fulfill with no provider left running, and CI is green. |
|
🚨 SLOP COP 🚨 · This PR prevents a new provider process from starting until the old bridge has fully stopped. I am reviewing it now under the saved SlopCop rules. I will check security, code quality, performance, architecture, and tests. |
| async shutdown(): Promise<void> { | ||
| const retiringProcessKeys = [...this.providerRetiring.keys()]; | ||
| if (retiringProcessKeys.length > 0) { | ||
| await Promise.allSettled(this.providerRetiring.values()); |
There was a problem hiding this comment.
🚨 slopcop/review — Full shutdown can wait longer than the daemon grace period.
This code waits for replacement startup before it sets shuttingDown or rejects pending startup requests. Each JSON-RPC startup request can wait 30 seconds. The host daemon forces exit after 15 seconds.
A silent provider can therefore prevent a clean daemon shutdown. The new regression hides this path with skipPostInitializeRequests.
Please cancel the pending startup before this wait, or prevent the replacement start after full shutdown begins. Add a test with a provider that never answers initialization. Verify that shutdown finishes within 15 seconds and leaves no child process.
There was a problem hiding this comment.
Addressed in 0ab3801. I independently reproduced this with a replacement bridge that delays initialization for three seconds: the regression took the full delay and failed its one-second shutdown bound. Full shutdown now becomes terminal before lifecycle waits, so a retirement-blocked ensure fulfills without spawning; an already-initializing ensure treats shutdown as superseding its internal RPC wait. The regression is green within the bound, all calls fulfill, a post-shutdown ensure does not recreate the provider, and the full @bb/agent-runtime test/typecheck passes (22 files, 318 tests). @slopcop please re-review the current head.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change makes a new provider process wait until the old bridge stops.
The previous shutdown overlap now passes its regression. I found one remaining P1 shutdown issue.
Main finding
- Full shutdown waits for replacement startup before it cancels pending startup requests.
- Each startup request can wait 30 seconds.
- The host daemon forces exit after 15 seconds.
- A silent provider can therefore prevent a clean daemon shutdown.
- The new regression skips initialization, so it does not test this path.
- See the inline finding.
Please cancel pending startup before this wait, or prevent the replacement start after full shutdown begins. Add a stalled-initialization regression.
Other review areas
- Security: I found no command, privilege, or input trust change.
- Code quality: The keyed retirement promise correctly shares concurrent retirement work.
- Architecture: The repository has similar keyed lifecycle patterns, but it has no shared helper.
- Refactor: A local per-key lifecycle lane could replace special start and retirement maps later.
- Duplication: I found no new duplicate implementation that requires extraction.
- Scope: An exited-process cleanup race exists on the base branch. This PR did not cause it.
Verification
git diff --check upstream/main...HEADpassed.- The focused lifecycle suite passed all 35 tests.
- The package typecheck passed.
- The full local run passed 311 of 318 tests under concurrent load.
- All seven local failures were timeouts. The focused changed suite passed alone.
- GitHub CI passed all required jobs.
- A browser test does not apply because this internal race has no browser route.
Please address the shutdown deadline issue before merge.
|
🚨 SLOP COP 🚨 · Plain-English summary: This change prevents a new provider process from starting before the old bridge process stops. I am the Slop Cop. I am reviewing this pull request under the saved review rules. I will check security, code quality, performance, architecture, duplication, and relevant tests. |
|
🚨 SLOP COP 🚨 · Plain-English summary: This change prevents a new provider process from starting before the old bridge process stops. I am the Slop Cop. I am reviewing this pull request under the saved review rules. I will check security, code quality, performance, architecture, duplication, and relevant tests. |
| const operations = Promise.all([ | ||
| manager.shutdownProvider(MANAGER_PROVIDER), | ||
| manager.ensureProvider(MANAGER_PROVIDER), | ||
| manager.shutdown(), |
There was a problem hiding this comment.
🚨 slopcop/review — [P2] This test never starts the stalled replacement.
Promise.all calls these functions in list order. ensureProvider() waits for retirement. shutdown() then sets shuttingDown before retirement ends.
The ensure returns at manager line 150, so starts stays at one. The test does not exercise cancellation during replacement initialization.
Please wait until the second adapter starts before you call full shutdown. Then check prompt completion and the replacement child exit.
| ]); | ||
| const completedPromptly = await Promise.race([ | ||
| operations.then(() => true), | ||
| new Promise<false>((resolve) => setTimeout(() => resolve(false), 1000)), |
There was a problem hiding this comment.
🚨 slopcop/review — [P3] Clear or unref this losing timer.
When operations wins the race, this timer stays active for one second. It retains its closure and can delay worker exit.
There was a problem hiding this comment.
Addressed in 0c7bb9a. The deadline timer is now unrefed when created and cleared as soon as the race settles.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change makes a new provider wait until the old bridge process stops.
Full shutdown now prevents later provider starts and cancels active startup work.
Review result: I found one P2 test gap and one P3 test cleanup issue. I found no production logic defect.
Findings
- P2: The stalled-start test calls shutdown before the replacement can leave its retirement wait.
- The test never starts the stalled adapter, so it cannot prove the new cancellation path.
- Please wait for the second adapter before shutdown. Then check prompt completion and child exit.
- See the inline finding.
- P3: The losing one-second race timer stays active and can delay worker exit.
- See the inline finding.
Other review areas
- Security: I found no privilege, input trust, or data exposure issue.
- Performance: The keyed wait affects one process key. Shutdown stops visible children in parallel and clears pending requests.
- Architecture: A single per-key lifecycle lane could later replace the separate start and retirement maps. This refactor is optional.
- Duplication: The repository has similar keyed promise patterns, but no shared helper matches these lifecycle rules.
- Browser test: This internal process lifecycle change has no browser route or user interface path.
Verification
git diff --check origin/main...HEADpassed.- The package typecheck passed.
- The shared-load unit run passed 304 of 318 tests.
- Timeouts and a concurrent generated-bridge conflict caused the 14 failures.
- GitHub CI passed all required package, server, integration, and smoke jobs.
- An independent GPT-5.6 review gate confirmed the P2 and P3 findings.
Please fix the P2 test gap before merge. The P3 timer cleanup is optional.
| const operations = Promise.all([ | ||
| manager.shutdownProvider(MANAGER_PROVIDER), | ||
| manager.ensureProvider(MANAGER_PROVIDER), | ||
| manager.shutdown(), |
There was a problem hiding this comment.
🚨 slopcop/review — The test does not start the stalled replacement.
Promise.all evaluates these calls in order. ensureProvider() waits for retirement before shutdown() sets the terminal state.
After retirement ends, ensureProvider() returns at manager line 150. It never calls the second adapter.
Thus, this test only covers cancellation of a queued replacement. It does not cover shutdown during stalled replacement initialization.
Please wait for the second adapter to start before you call shutdown(). Then assert that both operations finish promptly.
Also assert that no provider remains.
There was a problem hiding this comment.
Addressed in 0c7bb9a. The replacement bridge now writes a signal after receiving its initialization request; the test waits for that signal and asserts the second adapter started before calling full shutdown. It then verifies retirement, replacement start, and shutdown finish within one second, the replacement child was killed, a post-shutdown ensure does not recreate it, and no provider remains. Post-rebase Turbo test/typecheck passes all 318 tests.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change makes a replacement provider wait until the old bridge process stops.
It also prevents new provider starts after full runtime shutdown begins.
Review result: I found one test gap. I found no production defect.
Finding
- The new shutdown regression never starts the stalled replacement.
- Full shutdown sets the terminal state while the replacement still waits for retirement.
- The replacement then returns before it uses the stalled adapter.
- See the inline finding.
- Please wait for the second adapter to start before full shutdown.
- Then verify that shutdown and the pending start finish promptly.
Other review areas
- Security: I found no command, privilege, input trust, or process isolation issue.
- Performance: The keyed wait has bounded process termination and prompt map cleanup.
- Architecture: The separate start and retirement maps fit this small change.
- Duplication: Similar keyed promise patterns exist, but no shared helper fits this lifecycle barrier.
- Refactor: A single keyed lifecycle queue could replace both maps later. This refactor does not belong in this fix.
Verification
git diff --check upstream/main...HEADpassed.- Both changed lifecycle tests passed when I ran them alone.
- The agent-runtime typecheck passed through Turbo.
- All current GitHub checks pass, including the package test job.
- A broad parallel local run was invalid because concurrent workers regenerated shared test bridge assets.
- A browser check does not apply because this internal change has no server route or user interface path.
Please fix the stalled-start regression before merge. I found no other issue.
0ab3801 to
0c7bb9a
Compare
|
@slopcop please re-review current head 0c7bb9a. The stalled-start regression now waits until the replacement child receives initialization before full shutdown, then proves prompt completion, child exit, and no surviving provider. The losing timer is unrefed and cleared. Post-rebase local Turbo validation passes 318 tests plus typecheck, and CI run 33464805669 is fully green. |
|
🚨 SLOP COP 🚨 · Plain-English summary: This change makes a replacement provider wait until an old bridge process stops. It also coordinates full shutdown with a provider start that already began. I am the Slop Cop. I am reviewing this pull request under the saved review rules. I will check security, code quality, performance, architecture, duplication, and the relevant tests. |
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change makes a replacement provider wait until the old bridge process stops.
It also makes full shutdown stop provider creation and cancel provider initialization promptly.
Review result: I found no issue at the current head.
Review areas
- Security: The change does not alter commands, privileges, external input, environment sanitization, or trust boundaries.
- Code quality: The keyed retirement promise closes the race and cleans itself after completion.
- Performance: The change adds constant-time map work and only serializes work for one process key.
- Architecture: A future lifecycle lane could combine the two promise maps, but this focused change does not need that refactor.
- Duplication: I found no shared helper with the required lifecycle rules and package boundary.
- Browser test: Doobie does not apply because this host process code has no browser route or user interface path.
Verification
git diff --check origin/main...HEADpassed.- The two focused regression tests passed.
- The full lifecycle file passed all 35 tests.
- The agent-runtime Turbo typecheck passed.
- GitHub reports all current checks as passed or expected skips.
- The final GPT-5.6 review gate returned
APPROVE.
The current test waits until the stalled replacement receives initialization before full shutdown starts.
The deadline timer is unreferenced and clears after the race completes.
1 participant
Human comments
What was wrong
PR #2773 correctly retires a provider bridge after its final thread ends, but the shared process manager did not record that retirement while it was in progress. A same-key
ensureProvidercould therefore reuse the bridge being terminated; the new thread would reject immediately or briefly start and then lose its provider when the old bridge exited.Review exposed two full-shutdown ordering cases: a retirement-blocked start could spawn after the shutdown sweep, and waiting for that replacement to finish initialization could exceed the daemon's 15-second exit grace because provider RPC initialization waits up to 30 seconds.
What changed
The provider process manager now memoizes same-key retirement promises. New starts wait for retirement before selecting or spawning a bridge, and concurrent shutdown callers share the active retirement. Full runtime shutdown becomes terminal before any lifecycle wait: retirement-blocked or later ensures fulfill without spawning, while already-initializing starts treat shutdown as superseding their internal RPC wait. Shutdown then terminates every process already visible to its sweep.
Final-thread retirement, bridge reuse while another thread remains, and later cold resume remain unchanged. The change is internal to
@bb/agent-runtime; no daemon wire contract, protocol version, public API, CLI, or configuration changed.How you verified
Provider "fake" is not runningand passes afterward with a distinct surviving replacement process.pnpm exec turbo run test typecheck --filter=@bb/agent-runtime --force— 22 files / 318 tests passed; typecheck passed.33459198901— all non-skipped jobs passed, including Linux AppImage lifecycle smoke.git diff --check origin/main...HEAD— passed.origin/mainatfb05e93ff5ad4f58831e804b973f2de790557afa.Completes the lifecycle coordination missed by #2773.
Tagging @slopcop for re-review of head
0ab3801d24d4d8c04e9cc1bff808357cb0f07d70.