Skip to content

Fix browser supervisor child reaping race - #3369

Open
ymichael wants to merge 2 commits into
mainfrom
bb/ci-flake-fix-browser-supervisor-child-reap-timeo-thr_mak3w3m48b
Open

Fix browser supervisor child reaping race#3369
ymichael wants to merge 2 commits into
mainfrom
bb/ci-flake-fix-browser-supervisor-child-reap-timeo-thr_mak3w3m48b

Conversation

@ymichael

@ymichael ymichael commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

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 submitted SIGKILL to its child group but exited on an unrelated 100 ms timer instead of the child's close event, 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 child exhausted the existing 5,000 ms assertion at 6,036 ms. The first event-driven revision correctly waited for reaping, but made the app-facing close() wait unbounded if the operating system never delivered that boundary. The unrelated PR #2902 did not change either process file.

What changed

  • Keep the supervisor alive after forced termination until the direct child emits close, replacing the 100 ms scheduling guess with the lifecycle event that follows exit and reaping.
  • Wait for the killed worker's exit event before asserting that pipe closure reaped the supervised child.
  • Bound the app-facing close() operation at 5,000 ms. Normal completion cancels its unref'd timer. If the deadline wins, close() rejects with ProcessReapingUnconfirmedError, 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.
  • Leave the supervisor running after an app-side deadline. The deadline does not signal or terminate it, so it remains the child's parent and can still observe 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.
  • Make the test's finally path 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.
  • Preserve the TERM-resistant child-reaping assertion and all existing test deadlines; no timeout, retry, or polling budget was increased.
  • No HOST_DAEMON_PROTOCOL_VERSION bump 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

  • Red: the linked Linux packages job failed on unchanged process code with the exact child-still-observable assertion after its full 5,000 ms wait.
  • Green CI: the final bounded-shutdown head passed every applicable check in PR run 34509139160, including Tests (packages, ubuntu-latest, Node 22.x) in 3m48s, Linux package smoke in 3m59s, and macOS package smoke in 2m25s.
  • Green: 24 exact worker-death lifecycle cases and 16 deterministic app-deadline/late-reap cases passed under 12 CPU-contention workers with up to eight concurrent test processes; each externally bounded harness reported zero survivors.
  • 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.ts and git diff --check — passed.
  • All stress and validation commands were externally bounded and ended with zero browser-process survivors on host_nwqfteeqz4 (x86_64, Intel Core i5-1038NG7).
  • Work began from and retains merge-base 2e5f34b8c6a9ac617c92123388b0f49bb3bc11d5; origin/main advanced afterward, and this branch was deliberately not rebased off the frozen required SHA.

AGENT GENERATED: by GPT-5.6-Sol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant