inspector: drain queued frontend messages to the debugger thread before exit

[hughescr]

Fixes #34218

What does this PR do?

Fixes a real (reproducible) loss of bun test's TestReporter inspector
events: when a file's last tests are synchronous, the main thread can queue
their final start/end events, print the run summary, and fall straight
through to exit() -- tearing down the detached debugger thread -- before
that thread's already-posted delivery task runs. The events were genuinely
produced (the tests really ran and passed) but never reach the frontend,
with no error and no way to detect the loss from the frontend side.

This is a Rust port of #29307 ("inspector: drain queued frontend messages
before exit; fix TestReporter ID collision"), which fixed the same bug
against the old Zig debugger implementation and was closed unmerged only
because the Rust migration deleted the files it touched -- a maintainer
explicitly invited a refile against the new Rust code. This PR ports only
the drain-before-exit fix from that PR. Two other things bundled into the
original PR are not included here and are being submitted as separate,
independent PRs instead: the TestReporter ID-collision fix from the same
original PR, and a WebSocket-backpressure correctness bug in
debugger.ts found while investigating this (a !! coercion that treats
sendText()'s "still buffered, will still be delivered" return value as
success, so it's never paced or retried) -- neither shares a root cause or
a call path with the exit-time drain fixed here.

Root cause: Bun__ensureDebugger's connection hands inspector messages
from the main (inspected) thread to a separate, detached debugger thread via
sendMessageToDebuggerThread, which posts a task to that thread to actually
call onMessageFn (and, transitively, write the message to the frontend
socket). If the main thread reaches process exit before that posted task
runs, the task -- and the messages it was going to deliver -- never run at
all; nothing waited for it.

Fix: a new totalPendingDebuggerMessages atomic counts messages handed
off but not yet passed to onMessageFn. Bun__debugger__drain(), called
from VirtualMachine::global_exit immediately before the debugger
connections are torn down, checks that counter only as an entry gate ("is
any handoff still pending?"); if so, it posts a sentinel task to the same
debugger-thread context and waits (capped at 250ms) for the sentinel to run.
Tasks posted to that context run FIFO, so once the sentinel has run, every
message queued before Bun__debugger__drain() was called has already had
write() invoked for it -- the sentinel's ordering is the guarantee, not
the counter reaching zero.

A second, shorter bounded wait (150ms) then gives the debugger thread's own
(still-running) event loop extra wall-clock time to flush anything already
handed to the socket layer -- e.g. still sitting in a WebSocket's own
internal send buffer -- out onto the wire before the process tears down.
This helps independent of the separate backpressure-tracking bug mentioned
above: uWS's send buffer keeps draining on the debugger thread's event loop
regardless of whether debugger.ts is pacing/retrying writes correctly. It
is gated on a new hasQueuedAnyDebuggerMessage flag (set once, never
cleared, the first time anything is ever queued for the debugger thread)
rather than run unconditionally on every debugger-attached exit: if nothing
was ever queued, there's provably nothing to flush, so the wait is skipped.
This flag is deliberately distinct from totalPendingDebuggerMessages,
which decrements back to zero as soon as write() has been called for a
message even though that message's bytes can still be sitting unflushed
below write() -- gating the flush wait on that counter instead (an
earlier revision of this change did exactly that) would give an
already-buffered message zero grace whenever there happened to be no
pending handoff at the moment of the exit-time check.

Both counters are process-global static state (matching the file's existing
debuggerScriptExecutionContext), not per-VM or per-connection -- disclosed
in a comment at their declarations.

Both waits are capped, so a wedged/starved debugger thread, or a frontend
that has stopped reading entirely, can never block process exit
indefinitely; on timeout this simply degrades to the pre-fix behavior for
whatever hadn't been delivered yet.

Scope: verified only for graceful main-process exits -- every
global_exit() call site in this tree is a normal CLI/runtime exit path
(run/test/repl commands, process.exit(), bake production builds).
Behavior on a process torn down by a signal or abort(), neither of which
runs global_exit(), is unverified by this change and no claim is made
about those paths.

How did you verify your code works?

Added a regression test in test/cli/inspect/test-reporter.test.ts
("flushes pending inspector messages to the frontend before process exit")
that spawns several bun test --inspect-wait=unix:... subprocesses in
parallel -- each running a handful of trivial synchronous tests -- with a
normally-reading (not paused) inspector client attached to each. Parallel
spawns supply the CPU contention needed to actually starve the debugger
thread and make the race bite reliably; a lone run on an idle machine can
pass even pre-fix. The test asserts every subprocess's client received
found, start, and end events for every one of its tests, all pass,
with exit code 0 -- zero trailing loss at any of the three reporting
stages.

Synchronization is an observable handshake, not a fixed sleep: each
subprocess's inspector socket is only inspected after both the subprocess's
exited promise and the socket's own FIN (close) have fired. On a Unix
stream socket, FIN is ordered after all data the peer wrote, so this
guarantees the snapshot reflects everything the subprocess actually sent --
process exit alone would not, since the kernel can still be delivering
already-queued bytes after the process is gone. (This preemptively
addresses the same fixed-sleep review nit robobun raised against
#29307's original test.)

Coverage scope: this test uses --inspect-wait=unix:..., which routes
through debugger.ts's #connectOverSocket()/SocketFramer path, not the
ws:/ws+unix: server path that uses webSocketWriter/bufferedWriter.
It therefore exercises only the main-thread-to-debugger-thread drain fixed
in this PR, and provides no coverage of the separate WebSocket-backpressure
bug mentioned above (which will ship with, and be verified by, its own PR
and test).

Fails before, passes after (per this repo's root CLAUDE.md, line 106:
"Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes
with bun bd test <file>. Your test is NOT VALID if it passes with
USE_SYSTEM_BUN=1."):

$ USE_SYSTEM_BUN=1 bun test test/cli/inspect/test-reporter.test.ts -t "flushes pending inspector messages"
# 3/3 runs: FAIL -- started.size observed at 1/5, 4/5, 2/5 across the three runs
# (each run's actual numbers vary run to run, as expected of a scheduler race)

$ bun bd test test/cli/inspect/test-reporter.test.ts -t "flushes pending inspector messages"
# 3/3 runs: PASS

Also ran the full test/cli/inspect/ sweep (bun bd test test/cli/inspect/)
against this branch: 38 pass / 4 pre-existing todo / 0 fail across all 4
files in that directory.

Typechecked src/js (cd src/js && bun x tsc --noEmit) and diffed the
output against an unmodified-main baseline: identical error set (only a
worktree-path string differs in one pre-existing, unrelated error) --
expected, since this PR makes no changes to src/js/internal/debugger.ts
or any other TypeScript source.

https://claude.ai/code/session_013DexKQwDASzoRVDFS8Jtyn

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6620b214-8659-4d93-8f15-57e669713b24

📥 Commits

Reviewing files that changed from the base of the PR and between 91f8dcc and edee9fe.

📒 Files selected for processing (2)

• src/jsc/bindings/BunDebugger.cpp
• test/cli/inspect/test-reporter.test.ts

---

Walkthrough

Adds native debugger-message draining before virtual-machine exit, tracks pending debugger-thread delivery with capped waits, and adds regression coverage for complete TestReporter event delivery across concurrent inspector subprocesses.

Changes

Inspector message draining

Layer / File(s)	Summary
Debugger-thread drain signaling src/jsc/bindings/BunDebugger.cpp	Tracks queued messages, handles successful and exceptional batches, and applies bounded handoff and socket-flush waits during exit.
Rust exit wiring src/jsc/Debugger.rs, src/jsc/VirtualMachine.rs	Exposes Bun__debugger__drain() through Debugger::drain() and invokes it during global_exit when a debugger is present.
Exit-delivery regression coverage test/cli/inspect/test-reporter.test.ts	Runs concurrent inspector subprocesses and verifies complete found, started, and passing ended event sets after process exit.

Possibly related PRs

• oven-sh/bun#34220: Updates WebSocket buffered-writer backpressure handling in the inspector message pipeline, which affects socket delivery during draining.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title matches the main change: draining queued inspector messages before exit.
Description check	✅ Passed	The PR description includes the required sections and provides substantial implementation and verification detail.
Linked Issues check	✅ Passed	The code implements the drain-before-exit fix and regression test needed for #34218's queued-message loss on graceful exit.
Out of Scope Changes check	✅ Passed	The changes stay within the drain-before-exit fix and its regression test, with no unrelated code paths introduced.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/jsc/bindings/BunDebugger.cpp`:
- Line 7: Update DebuggerDrainSignal to inherit from WTF::ThreadSafeRefCounted
and replace std::shared_ptr/std::make_shared usage with WTF Ref and adopt/create
helpers. Capture a Ref<DebuggerDrainSignal> in the cross-thread posted task,
then remove the now-unneeded <memory> dependency while preserving the existing
handoff behavior.
- Around line 415-419: Update the post-JSC::call logic in Bun__debugger__drain()
to check for a pending exception before decrementing
totalPendingDebuggerMessages. Only subtract messageCount when the entire batch
was successfully delivered; preserve the pending count on the exception path so
draining continues to wait for undelivered messages.

In `@test/cli/inspect/test-reporter.test.ts`:
- Line 2: Remove the setDefaultTimeout import and the corresponding
setDefaultTimeout(60_000) call from test-reporter.test.ts. Leave the test
definitions and runner-managed timeout behavior unchanged.
- Around line 443-452: In the results loop, move the exitCode assertion after
all reporter-event assertions. Keep the stderr, found, started, and ended checks
unchanged so captured output and delivery state are reported before validating
process success.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 42d54a99-76c6-4cd2-b908-0cbf25fd339f

📥 Commits

Reviewing files that changed from the base of the PR and between be77b65 and 91f8dcc.

📒 Files selected for processing (4)

• src/jsc/Debugger.rs
• src/jsc/VirtualMachine.rs
• src/jsc/bindings/BunDebugger.cpp
• test/cli/inspect/test-reporter.test.ts

[hughescr]

Housekeeping for reviewers: I've just merged latest main (bbe3f6a262) into this branch — a normal merge commit (ae6fa9d143), no force-push, so existing review comments and their anchors are intact. There was one conflict, in receiveMessagesOnDebuggerThread (src/jsc/bindings/BunDebugger.cpp), against main's new !jsBunDebuggerOnMessageFunction early return (from the disconnect path): resolved by decrementing totalPendingDebuggerMessages for the dropped batch before taking that return — a disconnect racing a just-dequeued batch would otherwise leave Bun__debugger__drain()'s entry gate counting that batch as pending forever. bun bd test test/cli/inspect/test-reporter.test.ts is green after the merge.

Since I have four related-but-independent PRs open (#34219, #34220, #34221, #34222), a quick map so nobody has to reconstruct it: no two of them touch the same source file. The only overlap anywhere is test coverage — this PR, #34220, and #34221 each extend test/cli/inspect/test-reporter.test.ts, so whichever lands first leaves a test-file-only conflict in the other(s); I'll rebase and resolve promptly whenever anything merges. #34220 and #34221 don't overlap even in tests, and #34222 shares nothing with the other three. Any merge order works — though if a preference helps, #34219 → #34221 → #34220 → #34222 is an order I've verified locally composes cleanly.

[robobun]

Thanks for this, and for the clear write-up of the race in #34218. As with #34221 / #36522, CI does not run on fork branches here, so the drain was carried onto a repo branch as #36524 with your commit credited via a Co-authored-by trailer, and that is the PR that will land. Closing this one so the two are not reviewed in parallel.

#36524 is this fix (pending-message counter, FIFO sentinel, capped wait, disconnect and exception accounting) with one change: the layer (b) 150 ms grace wait is dropped. hasQueuedAnyDebuggerMessage is set by the first response and never clears, so in practice it added a fixed 150 ms to every inspected process exit with nothing to wait on, and the sentinel on its own is enough for #34218 (the parallel-spawn test passes reliably without it). The socket-level flushing it was aiming at is the subject of your #34220, which handles it in the writer. #36524 has also been brought up to date with main, which resolves the same test-reporter.test.ts conflict with #36522 that this branch currently has.