inspector: drain queued frontend messages to the debugger thread before exit - #36524
inspector: drain queued frontend messages to the debugger thread before exit#36524robobun wants to merge 7 commits into
Conversation
…re exit When the last tests in a `bun test` run are synchronous, the main thread can queue their final TestReporter start/end events for the detached debugger thread, print the run summary, and fall straight through to exit() before that thread's already-posted delivery task runs. The events were produced but never reach the frontend, and there is no error and no way to detect the loss from the frontend side. Root cause: `sendMessageToDebuggerThread` posts a task to the debugger thread's ScriptExecutionContext to invoke onMessageFn. Nothing in `VirtualMachine::global_exit` waits for that posted task before exit() tears the thread down. Fix: add `Bun__debugger__drain()`, called from `global_exit` when a debugger is attached. A new `totalPendingDebuggerMessages` atomic counts messages handed off but not yet passed to onMessageFn; if any are pending, drain posts a FIFO sentinel task to the same context and waits (capped at 250ms) for it to run. Tasks posted to that context run FIFO, so once the sentinel has run, every message queued before drain was called has already had write() invoked for it. A second shorter wait (150ms, gated on `hasQueuedAnyDebuggerMessage`) gives the debugger thread's own event loop time to flush anything already handed to the socket layer's send buffer. Both waits are capped so a wedged debugger thread or non-reading frontend cannot block exit indefinitely; on timeout this degrades to pre-fix behavior for whatever hadn't been delivered yet. Adopts #34219 by @hughescr (itself a Rust port of the closed #29307). Fixes #34218. Co-authored-by: Craig R. Hughes <craig.git@rungie.com>
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: diff is ready. Self-review findings addressed (layer (b) 150ms sleep dropped in 268d010; FIFO sentinel alone suffices). Fail-before note: the race only reproduces on non-ASAN builds (Linux Reproduced with: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
The detailed rationale lives in the PR body; in-code comments are reduced to the minimum needed to understand the concurrency design at each site.
The detailed two-layer design rationale (FIFO sentinel ordering guarantee, separate sticky gate flag, capped waits) lives in the PR body.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked: DebuggerDrainSignal is captured by Ref in the posted lambda, so a layer-(a) timeout on the main thread doesn't leave the debugger-thread task with a dangling semaphore; and the test's connect(url, onClose) — junit-reporter.ts does wire the second arg to the socket close handler, so await socketClosed.promise won't hang.
Extended reasoning...
This PR adds cross-thread synchronization (atomics, semaphores, posted tasks between the main VM thread and the detached debugger thread) inside VirtualMachine::global_exit. That is squarely in the highest-scrutiny category per REVIEW.md (thread affinity, exit-path completeness), and the inline finding about layer (b) being an unconditional 150ms sleep on every inspected exit is a real design concern a maintainer should weigh — so this is not auto-approvable. The two items in the message are the concrete side-checks I ran and ruled out so a later pass need not re-derive them.
The sticky hasQueuedAnyDebuggerMessage gate is set on the first protocol response and never clears, so every inspected process exit paid a fixed +150ms with no completion condition. The FIFO sentinel alone (which wakes as soon as the debugger thread has called write() for every queued message) is sufficient for the bug in #34218; WebSocket-level backpressure flushing is a separate concern tracked in #34220. Test passes 5/5 with the sentinel alone.
There was a problem hiding this comment.
My earlier 150ms flush-grace concern was addressed in 268d010 — Bun__debugger__drain is now a single signaled FIFO-sentinel wait that short-circuits when totalPendingDebuggerMessages == 0. Beyond the description nit, I also checked that DebuggerDrainSignal is ref-counted so a timed-out wait can't leave the already-posted task holding a dangling semaphore, and that the pending-count increment sits inside debuggerThreadMessagesLock so the debugger thread's swap+decrement can't undercount it.
Extended reasoning...
The author addressed my previous review by removing the layer-(b) 150ms sleep entirely (commit 268d010). The remaining drain path is a bounded, signaled wait: post a FIFO sentinel to the debugger thread's context, waitFor(250ms) on a ref-counted BinarySemaphore, and only enter at all when the atomic pending-message counter is nonzero. I verified the ref-counted DebuggerDrainSignal avoids the dangling-pointer-on-timeout hazard, that the atomic increment is under the same lock as the message append (so swap+decrement in receiveMessagesOnDebuggerThread cannot observe an unaccounted message), and that the test's connect(url, onClose) close-callback wiring matches junit-reporter.ts's signature. Not approving: this is cross-thread synchronization on the global_exit teardown path and the FIFO ordering of postTaskConcurrently is load-bearing for correctness — worth a human look even though I found no defect beyond the stale description.
There was a problem hiding this comment.
I didn't find any bugs, but this touches cross-thread synchronization on the process-exit path and drops the socket-flush layer from #34219 in favor of deferring to #34220 — worth a maintainer confirming that trade-off.
What was reviewed:
totalPendingDebuggerMessagescounter balance across send / receive / disconnect / exception paths — all covered; exception path leaves count pending which is bounded to one 250ms wait.DebuggerDrainSignallifetime on timeout — ref-counted, lambda holds aRef, so no dangling pointer ifwaitForreturns first.- Test synchronization — awaits both
proc.exitedand the Unix socket FIN (data-ordered),afterEachSIGKILLs stragglers,connect()does accept theonClosecallback used here.
Extended reasoning...
Overview
The PR adds Bun__debugger__drain() (BunDebugger.cpp) and calls it from VirtualMachine::global_exit (VirtualMachine.rs, via a thin wrapper in Debugger.rs) so that inspector protocol messages queued for the detached debugger thread are delivered before exit() tears that thread down. A process-global atomic totalPendingDebuggerMessages counts messages handed off but not yet passed to onMessageFn; when nonzero, drain posts a FIFO sentinel task to the debugger thread's ScriptExecutionContext and waits up to 250ms for it to run. receiveMessagesOnDebuggerThread gains a TopExceptionScope so a throwing onMessageFn is reported rather than swallowed and the batch is conservatively left counted. A new regression test in test-reporter.test.ts spawns 4-16 --inspect-wait=unix: subprocesses in parallel and asserts every found/start/end event arrives.
Security risks
None identified. This is inspector-thread lifecycle plumbing; no user-controlled input reaches the new code beyond what already flowed through sendMessageToDebuggerThread.
Level of scrutiny
High. global_exit runs on every graceful process exit, and the new code sits on a cross-thread path (main → debugger thread) with atomics, a semaphore, and a FIFO-ordering assumption on postTaskConcurrently. The change also encodes a design decision — layer (b) from the adopted #34219 (a 150ms socket-flush grace) was dropped after my earlier review flagged it as an unconditional per-exit sleep, with the rationale that socket-level backpressure is #34220's concern. That is reasonable, but a maintainer should confirm the FIFO-sentinel-only design is acceptable for the intermediate state.
Other factors
Prior review feedback (the unconditional 150ms sleep, and the stale PR description) has been addressed in 268d010 and the description update. The comment-cop bot flags on verbose comments were resolved by trimming to single lines. The counter accounting looks balanced on every path I traced (increment inside debuggerThreadMessagesLock; decrement on the disconnected early-return and on successful JSC::call; deliberately left pending on throw). The DebuggerDrainSignal is ThreadSafeRefCounted and the posted lambda captures a Ref, so a timed-out wait cannot leave a dangling pointer for the already-posted task. The test correctly branches width/rounds/timeout on isASAN || isDebug, uses stdout: "ignore" to avoid pipe deadlock, and cleans up spawned processes in afterEach. Given the concurrency subtlety and the exit-path placement, a human familiar with the debugger threading model should sign off.
…or-drain-on-exit # Conflicts: # test/cli/inspect/test-reporter.test.ts
|
Updated 8:50 PM PT - Aug 12th, 2026
❌ @robobun, your commit d49c779 has some failures in 🧪 To try this PR locally: bunx bun-pr 36524That installs a local version of the PR into your bun-36524 --bun |
There was a problem hiding this comment.
I reviewed this PR and found no bugs in the current revision (prior findings — the unconditional 150ms sleep and the stale description — were both addressed). Because it adds cross-thread synchronization on the process exit path (atomic pending-message accounting, a ref-counted semaphore sentinel posted to the debugger thread's task queue, and a new TopExceptionScope around onMessageFn), a human look at the design would still be worthwhile.
What was reviewed:
- Counter accounting:
fetch_addis insidedebuggerThreadMessagesLockso the swap+decrement inreceiveMessagesOnDebuggerThreadcan't undercount; the disconnected-early-return decrements the dropped batch. DebuggerDrainSignalisThreadSafeRefCountedand the posted lambda captures aRef, so a timed-out main-thread wait can't leave the task holding a dangling semaphore.- Placement in
global_exitis before teardown/Global::exit, gated onself.debugger.is_some(), so the common no-debugger path adds no latency. - The sentinel and delivery tasks both go through
postTaskConcurrently→Bun__VmHandle__queueTaskConcurrently, so the FIFO-ordering claim rests on that queue's ordering.
Extended reasoning...
Overview
This PR fixes #34218: when bun test finishes with synchronous tests, the main thread can queue the final TestReporter events for the detached debugger thread and then call exit() before that thread's delivery task runs, so IDE frontends silently miss the last results. The fix adds Bun__debugger__drain() (BunDebugger.cpp), called from VirtualMachine::global_exit (VirtualMachine.rs) via a thin Rust wrapper (Debugger.rs). A process-global atomic totalPendingDebuggerMessages counts messages handed off but not yet passed to onMessageFn; when non-zero, drain posts a FIFO sentinel task to the debugger thread's ScriptExecutionContext and waits up to 250ms for it. receiveMessagesOnDebuggerThread also gains a TopExceptionScope so a throwing onMessageFn is reported and the batch conservatively left pending. A new regression test in test-reporter.test.ts spawns parallel --inspect-wait=unix: subprocesses to reproduce the race.
Security risks
None identified. The change is confined to the debugger/inspector plumbing and the process-exit path; it does not touch parsing of untrusted input, auth, crypto, or file/network permissions. The 250ms wait is bounded, so a wedged debugger thread cannot indefinitely delay exit.
Level of scrutiny
Medium-high. This is concurrency code on the exit path: an atomic counter shared across two threads with acquire/release ordering, a ref-counted semaphore captured by a cross-thread lambda, and a new exception scope around a JSC::call. The failure modes (undercount → drain skipped, overcount → 250ms extra wait, dangling semaphore on timeout, exception left pending on the debugger VM) are all subtle. The design choices — FIFO-sentinel vs. per-connection drain, 250ms cap, process-global counter rather than per-connection — are reasonable but worth a maintainer's eye. It is not a simple mechanical change.
Other factors
Two prior review rounds by this bot already ran on earlier revisions. The first flagged the original "layer (b)" — a stack-local BinarySemaphore gated on a sticky flag amounting to an unconditional +150ms on every inspected exit — which was removed in 268d010. The second flagged a stale PR description, since updated. All comment-cop inline threads are resolved. The PR description is unusually thorough (including why the race doesn't reproduce under ASAN debug builds due to libc_exit vs quick_exit), and the test has documented fail-before/pass-after evidence. The bug hunting system found no issues on the current revision. Given the concurrency subtlety and exit-path placement, deferring for a human sign-off rather than auto-approving.
1 participant
Adopts #34219 by @hughescr (a Rust port of the closed #29307). Fixes #34218.
Problem
When a
bun testrun's last tests are synchronous, the main thread can queue their finalTestReporter.start/TestReporter.endevents for the detached debugger thread to deliver, print the run summary, and fall straight through toexit()before that thread's already-posted delivery task runs. The events were produced (the tests ran and passed) but never reach the frontend. There is no error and no way to detect the loss from the frontend side, so IDE test UIs silently miss the final results.Cause
sendMessageToDebuggerThreadinBunDebugger.cppposts a task to the debugger thread'sScriptExecutionContextto invokeonMessageFn(which writes to the frontend socket).VirtualMachine::global_exithas no step that waits for that posted task beforeexit()tears the debugger thread down mid-delivery.Fix
Add
Bun__debugger__drain(), called fromglobal_exitimmediately before teardown when a debugger is attached. A newtotalPendingDebuggerMessagesatomic counts messages handed off but not yet passed toonMessageFn. If any are pending, drain posts a FIFO sentinel task to the same debugger-thread context and waits (capped at 250ms) for it to run. Concurrent tasks on that context run FIFO, so once the sentinel has run, everyreceiveMessagesOnDebuggerThreadinvocation posted before drain was called has already invokedwrite()for every message it was holding. The wait is capped so a wedged debugger thread cannot block process exit indefinitely; on timeout this degrades to pre-fix behavior for whatever hadn't been delivered yet. When nothing is pending the drain is a no-op, so there is no added exit latency on the common path.receiveMessagesOnDebuggerThreadnow also declares aTopExceptionScopearound theJSC::callintoonMessageFn: on a thrown exception the batch is conservatively treated as undelivered (count left pending, at worst one extra capped wait at exit), and a non-termination exception is reported viareportUncaughtExceptionAtEventLoopso it isn't swallowed.This covers graceful main-process exits only (every
global_exit()call site: CLI run/test/repl,process.exit(), bake production). A process torn down by a signal orabort()does not runglobal_exit()and is unchanged.Difference from #34219
The original PR additionally included a second "layer (b)" bounded sleep (150ms, gated on a sticky
hasQueuedAnyDebuggerMessageflag) intended to give the debugger thread's event loop time to flush socket-layer send buffers. That sleep is dropped here: the gate is set on the first protocol response and never clears, so it was effectively an unconditional +150ms on every inspected process exit with no completion condition. The FIFO sentinel alone is sufficient for #34218 (test passes 5/5 without it), and WebSocket-level backpressure flushing is a separate concern tracked in #34220 which fixes it at the writer rather than by sleeping at exit.Verification
New test in
test/cli/inspect/test-reporter.test.ts("flushes pending inspector messages to the frontend before process exit") spawns a batch ofbun test --inspect-wait=unix:...subprocesses in parallel, each running trivial synchronous tests with a normally-reading inspector client attached. Parallel spawns supply the CPU contention needed to starve the debugger thread and make the race bite reliably. The test asserts every subprocess's client receivedfound,start, andendfor every test (allpass, exit 0). Synchronization waits on both the subprocessexitedpromise and the Unix socket's own FIN (data-ordered), not a fixed sleep.Fail-before (3/3 runs, varying scheduler outcomes as expected of a race):
Pass-after (5/5 runs):
Why this race does not reproduce under ASAN debug builds
Global::exit()insrc/bun_core/Global.rscallsquick_exit()on Linux, which terminates all threads immediately without running atexit handlers. Under ASAN (cfg(bun_asan), which is the default forbun bdon Linux) it callslibc_exit()instead so ASAN's atexit reporting can run. Duringlibc_exit()'s atexit-handler phase the debugger thread is still schedulable and delivers its queued task, so the loss never manifests. This is a compile-time branch (build_options::ENABLE_ASAN), not a runtime toggle.This means the standard fail-before proof (
bun bd testwithout thesrc/change) cannot observe the failure: an ASAN debug subprocess always reacheslibc_exit()and the debugger thread catches up. The race is provable under any non-ASAN build:A 32-wide probe against an unmodified ASAN debug subprocess showed 0/32 loss; the same probe against release showed 19/32 loss. The test still provides real coverage on release CI lanes, where
quick_exit()is the exit path that users actually hit.Full
test/cli/inspect/test-reporter.test.tsis green with the fix under both ASAN and non-ASAN debug.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file