fix(browser): kill the whole process group during pre-identity cleanup#58
Merged
Conversation
stopOwnedBrowserDaemon only signaled the Chrome supervisor process and returned cleanup-complete unconditionally, including when the supervisor had already exited on its own. A pathological /proc identity-read failure combined with a supervisor crash could leave a same-process-group Chrome alive while its session record was marked fully cleaned up. Signal the whole owned process group by pgid instead (safe without a captured ProcessIdentity, since the daemon is our own just-spawned, not-yet-reaped child), and confirm via a new portable isProcessGroupAlive kill(2) probe that no member survives before reporting the group gone. Refs #29
The exited-branch doc comment on stopOwnedBrowserDaemon overstated its pid-reuse safety: Node/libuv can reap a child (making its pid OS-reusable) before exitCode/signalCode observably go non-null, so that branch is not provably safe the way the not-yet-exited branch is. Split the doc comment per branch to state the real guarantee each one relies on, and add an isProcessGroupAlive pre-check before signaling in the exited branch so a fully vacated group is never blindly signaled (this narrows, but does not close, the reuse gap). Add the missing-supervisor integration regression the issue checklist asks for: a stand-in supervisor that exits immediately after spawning Chrome, exercising stopOwnedBrowserDaemon's already-exited branch end-to-end through createBrowserSession. Captures Chrome's pid synchronously from its own spawn() return rather than waiting on Chrome's script to self-report, after the wait-based version proved flaky under a fully parallel `bun run test`. Refs #29
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
1 participant
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.
Summary
Implements #29 — closes the pre-identity cleanup window for the browser supervisor:
stopOwnedBrowserDaemonpreviously did an individual-process kill (or nothing, if the supervisor had already exited) and reported success — leaving a live orphaned Chrome in the supervisor's process group while the session record claimed cleanisProcessGroupAliveprimitive (kill-0 group probe, Darwin-safe), and reports gone only once confirmed; failure stays fail-open to the reaper via the existing pending-cleanup meta keyisProcessGroupAlivepre-check so a vacated group is never blindly signaled)Closes #29. Follow-up filed: #57 (same discipline for the Xvfb daemon, lower risk profile). No feature flag: defensive hardening, no user-facing behavior.
Validation
Local review: standard risk class — one independent kill-safety/correctness reviewer; P1 (pid-reuse overclaim + missing pre-check) and P3 (missing exited-supervisor test) both fixed in
1bed6bfper the reviewer's own smallest-fix prescription. KISS keep.