Fix browser supervisor child reaping race - #3369
Open
ymichael wants to merge 2 commits into
Open
Conversation
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.
Human comments
What was wrong
The process-ownership test and the supervisor both treated signal submission as process termination. The test began its five-second child-reaping assertion immediately after
worker.kill("SIGKILL"), although that API only submits the signal and the worker could still own the supervisor pipe. After the pipe closed, the supervisor submittedSIGKILLto its child group but exited on an unrelated 100 ms timer instead of the child'scloseevent, so under contention the child could still be observable, including as an unreaped zombie. This produced the unchanged-main failure in CI run 34404843507:worker death closes the supervisor pipe and kills its childexhausted the existing 5,000 ms assertion at 6,036 ms. The first event-driven revision correctly waited for reaping, but made the app-facingclose()wait unbounded if the operating system never delivered that boundary. The unrelated PR #2902 did not change either process file.What changed
close, replacing the 100 ms scheduling guess with the lifecycle event that follows exit and reaping.exitevent before asserting that pipe closure reaped the supervised child.close()operation at 5,000 ms. Normal completion cancels its unref'd timer. If the deadline wins,close()rejects withProcessReapingUnconfirmedError, explicitly stating that child reaping was not confirmed; it closes the already-ended parent pipe and unreferences the supervisor handle so neither can pin the app.close, reap the child, and exit independently. A single settled-state gate contains completion immediately before, during, or after deadline delivery without changing the already-returned result or creating an unhandled continuation.finallypath wait for worker exit and force-kill the detached child group if it is still observable, so a failed or interrupted assertion cannot leak the tested process tree.HOST_DAEMON_PROTOCOL_VERSIONbump is needed because this changes only an internal plugin-local subprocess and its test. No server/daemon command, session payload, WebSocket field, RPC field, default, or meaning changes.The 5,000 ms production ceiling contains the existing 1,500 ms graceful TERM stage plus 3,500 ms for forced-kill exit and reap notification. Under 12 CPU burners with eight concurrent test processes, the complete worker-death ownership case measured 2.18–2.33 s; the ceiling is more than twice that measured worst case and does not alter any assertion clock.
How you verified
Tests (packages, ubuntu-latest, Node 22.x)in 3m48s, Linux package smoke in 3m59s, and macOS package smoke in 2m25s.pnpm exec vitest run process.test.ts --config vitest.config.ts --reporter=verbose— 3/3 passed: normal close/reap, bounded truthful deadline with independent late reap, and worker-death ownership.pnpm exec turbo run test --filter=bb-plugin-browser-automation— 37/37 passed.pnpm exec turbo run typecheck --filter=bb-plugin-browser-automation— passed.pnpm exec turbo run build --filter=bb-plugin-browser-automation— passed.pnpm exec prettier --check plugins/browser-automation/process.ts plugins/browser-automation/process.test.tsandgit diff --check— passed.host_nwqfteeqz4(x86_64, Intel Core i5-1038NG7).2e5f34b8c6a9ac617c92123388b0f49bb3bc11d5;origin/mainadvanced afterward, and this branch was deliberately not rebased off the frozen required SHA.