From 91f8dcc02f0eed32befd3e94849bae1ebc013ca1 Mon Sep 17 00:00:00 2001 From: "Craig R. Hughes" Date: Tue, 14 Jul 2026 19:19:25 -0700 Subject: [PATCH 1/2] inspector: drain queued frontend messages to the debugger thread before exit `bun test`'s TestReporter inspector events can be silently, permanently lost when the process exits before they've actually reached the debugger thread: inspector events emitted from the main thread are queued and a task is posted to the detached debugger thread to write them to the frontend socket. When the last tests in a file are synchronous, the main thread can queue their final start/end events, print the summary, and call exit() -- tearing down the debugger thread -- all without yielding, before that thread's posted task runs. This is a Rust port of oven-sh/bun#29307 ("inspector: drain queued frontend messages before exit; fix TestReporter ID collision"), which was closed unmerged only because the Rust migration deleted the Zig files it touched (maintainers explicitly invited a refile against the new Rust code). This PR ports only that PR's drain-before-exit design -- the TestReporter ID-collision fix from the same original PR, and a separate WebSocket-backpressure correctness fix in debugger.ts, are independent bugs and land as their own PRs. Design (BunDebugger.cpp, Debugger.rs, VirtualMachine.rs): A new `totalPendingDebuggerMessages` atomic tracks messages handed off from the main thread to the debugger thread but not yet passed to onMessageFn. `Bun__debugger__drain()`, called from `VirtualMachine::global_exit` before the debugger connections are torn down, reads that counter only as an entry gate ("is any handoff still pending?"); if so it posts a sentinel task to the debugger thread and waits (capped at 250ms) for it to run. Concurrent tasks on that context are FIFO, so once the sentinel runs, every message queued before the call has had write() invoked for it. (It does not spin-wait for the counter to reach zero -- the sentinel's FIFO ordering is the guarantee, not the counter.) The signal is a std::shared_ptr rather than the original PR's heap-allocated-and-intentionally-leaked semaphore: Bun's leak-sanitizer builds (fba43af684) now flag exactly that pattern, so this port avoids it -- whichever side (the waiting thread, or the posted task) finishes last frees it. `Bun__debugger__drain()` also takes a second, shorter bounded wait (150ms) giving 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 internal send buffer -- out onto the wire before the process tears down. This is useful independent of any JS-level backpressure tracking: uWS's own send buffer keeps draining on the debugger thread's event loop regardless of whether debugger.ts is pacing/retrying correctly, so the wait still helps even without the separate backpressure fix mentioned above. It is gated on a new `hasQueuedAnyDebuggerMessage` flag -- set once, never cleared, the first time any message is ever queued for the debugger thread -- rather than run unconditionally on every debugger-attached exit: if nothing was ever queued, there is provably nothing to flush, so the wait is skipped outright. This flag is deliberately distinct from `totalPendingDebuggerMessages`, which decrements back to zero as soon as write() has been called for a message even though its bytes can still be sitting unflushed below write() -- gating the flush wait on that counter (as an earlier revision of this change did) would give an already-buffered message zero grace whenever there was no pending handoff at the moment of the exit-time check. Both `totalPendingDebuggerMessages` and `hasQueuedAnyDebuggerMessage` are process-global static state (matching the existing `debuggerScriptExecutionContext`), not per-VM or per-connection; see the disclosure comment at their declarations. Both waits are capped so a wedged/starved debugger thread, or a consumer that has stopped reading entirely, cannot block process exit indefinitely. Scope: verified only for graceful main-process exits (CLI run/test/repl commands, `process.exit()`, bake production builds -- i.e. every `global_exit()` call site in this tree). Behavior on a process torn down by a signal or `abort()`, which do not run `global_exit()`, is unverified; no claim is made about those paths. Test (test/cli/inspect/test-reporter.test.ts): Adds a regression test where several NORMALLY-READING (not paused) consumers, connected to `bun test` subprocesses running fast synchronous tests, are spawned in parallel -- parallel spawns supply the CPU contention that starves the debugger thread and makes the race bite reliably; a lone run on an idle machine may pass even pre-fix. Asserts every subprocess delivers found/start/end events for every test, all "pass", with exit code 0. Synchronization is a handshake, not a fixed sleep: each subprocess's socket is only inspected after both `exited` and the socket's own FIN (`close`) have been observed, which on a Unix stream socket is ordered after all data the subprocess wrote -- guaranteeing the snapshot reflects everything sent, not whatever happened to arrive within an arbitrary wait window. (This addresses the 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 -- so it exercises the main->debugger-thread drain only and provides ZERO automated coverage of the separate WebSocket-backpressure fix mentioned above (that fix has, and needs, its own test). Verified fails-before/passes-after: 3/3 runs fail under unpatched system Bun 1.3.14 (`USE_SYSTEM_BUN=1`), with observed loss of started.size 1/5, 4/5, and 2/5 across the three runs; 3/3 runs pass against this branch's `bun bd` build. Claude-Session: https://claude.ai/code/session_013DexKQwDASzoRVDFS8Jtyn --- src/jsc/Debugger.rs | 23 ++++ src/jsc/VirtualMachine.rs | 9 ++ src/jsc/bindings/BunDebugger.cpp | 175 ++++++++++++++++++++++++- test/cli/inspect/test-reporter.test.ts | 146 ++++++++++++++++++++- 4 files changed, 350 insertions(+), 3 deletions(-) diff --git a/src/jsc/Debugger.rs b/src/jsc/Debugger.rs index 92eea89bf629..2d5072d205d6 100644 --- a/src/jsc/Debugger.rs +++ b/src/jsc/Debugger.rs @@ -156,6 +156,7 @@ unsafe extern "C" { from_env: c_int, is_connect: bool, ); + safe fn Bun__debugger__drain(); } static FUTEX_ATOMIC: AtomicU32 = AtomicU32::new(0); @@ -341,6 +342,28 @@ impl Debugger { } } + /// Block (briefly, with a cap) until the debugger thread has written any + /// inspector protocol messages queued for it to the frontend socket, and + /// give it a further short grace period to flush anything still sitting + /// in the WebSocket layer's own send buffer to a still-reading consumer. + /// + /// Call this from the main thread immediately before process exit (see + /// [`VirtualMachine::global_exit`](crate::virtual_machine::VirtualMachine::global_exit)) + /// so the detached debugger thread isn't killed mid-delivery. Without + /// this, `exit()` can tear down the debugger thread while the final + /// events of a run (e.g. `bun test`'s last `TestReporter.end` events) + /// are still queued or buffered, and the frontend never sees them. + /// + /// Both waits inside are capped, so a wedged debugger thread -- or a + /// frontend that has stopped reading entirely -- cannot block process + /// exit indefinitely. See `Bun__debugger__drain` in `BunDebugger.cpp` + /// for the full design (it covers two distinct loss layers: the + /// main-to-debugger-thread message handoff, and WebSocket-level + /// backpressure buffering). + pub fn drain() { + Bun__debugger__drain(); + } + /// `Debugger.create(vm, global)` — first-time debugger setup: create the /// JSC inspector context, spawn the debugger VM thread, and arm the /// keep-alive on the parent loop. diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 2dc03fbd4246..4cfd4ea992b2 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -1528,6 +1528,15 @@ impl VirtualMachine { // doing it causes like 50+ tests to break // self.event_loop().tick(); + // Flush queued inspector messages so exit() doesn't kill the + // detached debugger thread mid-delivery. The debugger thread runs + // its own VirtualMachine (see debugger::start_js_debugger_thread), + // so this wait never contends with the main VM's API lock that + // callers of global_exit typically hold. + if self.debugger.is_some() { + crate::debugger::Debugger::drain(); + } + if self.should_destruct_main_thread_on_exit() { #[cfg(windows)] if let Some(t) = self.event_loop_mut().forever_timer.take() { diff --git a/src/jsc/bindings/BunDebugger.cpp b/src/jsc/bindings/BunDebugger.cpp index 07de46c89962..4302e6fcd684 100644 --- a/src/jsc/bindings/BunDebugger.cpp +++ b/src/jsc/bindings/BunDebugger.cpp @@ -3,6 +3,8 @@ #include "ZigGlobalObject.h" #include +#include +#include #include #include #include @@ -46,6 +48,42 @@ static PausedWait& pausedWait() return instance; } +// Count of messages handed off from the inspected (main) thread to the +// debugger thread via sendMessageToDebuggerThread() that haven't yet had +// their corresponding onMessage/write() call invoked on the debugger thread. +// Bun__debugger__drain() reads this only as an entry gate -- "is any handoff +// still pending?" -- to decide whether to post its FIFO sentinel task; it +// does NOT spin-wait for the count to reach zero (the sentinel's FIFO +// ordering is what guarantees the handoff has caught up, not this counter). +// +// DISCLOSURE: this is process-global static state (like +// debuggerScriptExecutionContext above), not per-VM or per-connection. A +// process that opens more than one debugger connection -- e.g. a main VM +// plus a WebWorker, each with their own inspector -- shares a single count +// across all of them. That is fine for Bun__debugger__drain()'s use (any +// pending handoff, from any connection, is reason enough to wait), but it +// does mean this counter cannot answer "is *this specific* connection's +// handoff caught up?". +static std::atomic totalPendingDebuggerMessages { 0 }; + +// Set (never cleared) the first time any message is queued for the debugger +// thread via sendMessageToDebuggerThread(). Also process-global, for the +// same reason as totalPendingDebuggerMessages above. +// +// Bun__debugger__drain()'s layer-(b) flush grace uses this -- not +// totalPendingDebuggerMessages -- as its gate: that counter decrements back +// to zero as soon as write() has been called for a message, even though the +// bytes it produced can still be sitting unflushed in the socket layer below +// write() (exactly the case the flush grace exists to help). This flag, +// by contrast, stays true for the rest of the process's life once anything +// has ever been queued, so it can safely answer the coarser question +// Bun__debugger__drain() actually needs -- "has any message ever been +// queued for delivery, so there is any chance of pending output to flush?" +// -- without needing to introspect the debugger thread's live buffer state. +// If this is still false, nothing was ever queued, so there is provably +// nothing to flush and the grace wait can be skipped outright. +static std::atomic hasQueuedAnyDebuggerMessage { false }; + static bool waitingForConnection = false; extern "C" void Debugger__didConnect(); @@ -359,9 +397,11 @@ class BunInspectorConnection : public Inspector::FrontendChannel { this->debuggerThreadMessages.swap(messages); } + size_t messageCount = messages.size(); + JSFunction* onMessageFn = uncheckedDowncast(jsBunDebuggerOnMessageFunction.get()); MarkedArgumentBuffer arguments; - arguments.ensureCapacity(messages.size()); + arguments.ensureCapacity(messageCount); auto& vm = debuggerGlobalObject->vm(); for (auto& message : messages) { @@ -371,6 +411,12 @@ class BunInspectorConnection : public Inspector::FrontendChannel { messages.clear(); JSC::call(debuggerGlobalObject, onMessageFn, arguments, "BunInspectorConnection::receiveMessagesOnDebuggerThread - onMessageFn"_s); + + // After JSC::call, so onMessageFn (which calls into debugger.ts's + // webSocketWriter/bufferedWriter write()) has been invoked for every + // message in this batch. See Bun__debugger__drain(). + if (messageCount > 0) + totalPendingDebuggerMessages.fetch_sub(messageCount, std::memory_order_release); } void sendMessageToDebuggerThread(WTF::String&& inputMessage) @@ -378,6 +424,15 @@ class BunInspectorConnection : public Inspector::FrontendChannel { { Locker locker(debuggerThreadMessagesLock); debuggerThreadMessages.append(inputMessage); + // Incremented inside the same locked section as the append, + // before the lock is released. Incrementing after unlocking would + // race receiveMessagesOnDebuggerThread(), which could swap the + // vector out (and later decrement by the batch size) before this + // increment ran, undercounting totalPendingDebuggerMessages. + totalPendingDebuggerMessages.fetch_add(1, std::memory_order_release); + // Never cleared -- see the declaration above for why this is a + // separate flag from totalPendingDebuggerMessages. + hasQueuedAnyDebuggerMessage.store(true, std::memory_order_release); } if (this->debuggerThreadMessageScheduledCount++ == 0) { @@ -572,6 +627,124 @@ extern "C" void Bun__ensureDebugger(ScriptExecutionContextIdentifier scriptId, b } } +// Small ref-counted signal so a wait that times out doesn't leave a dangling +// pointer for the debugger thread's already-posted task to dereference, and +// so nothing is deliberately leaked (Bun's leak-sanitizer builds treat that +// as a bug to fix -- see fba43af684, "Fix effectively every native-code +// memory leak in Bun"). Both the waiting thread (Bun__debugger__drain) and +// the posted task hold a reference via std::shared_ptr; whichever finishes +// last frees it. +struct DebuggerDrainSignal { + WTF::BinarySemaphore semaphore; +}; + +// Cap on how long the main thread waits, on exit, for the debugger thread to +// finish the pending main->debugger-thread message handoff (layer a). A cap, +// not a target: a wedged/starved debugger thread must never block process +// exit indefinitely. +static constexpr double kDrainHandoffTimeoutMs = 250; + +// Additional bounded grace period, taken on exit whenever any message was +// ever queued for the debugger thread (see hasQueuedAnyDebuggerMessage +// above), for the debugger thread's own event loop to flush whatever it has +// already handed to the socket layer out of that layer's send buffer and +// onto the wire (layer b). Also a cap, not a target. +static constexpr double kDrainFlushGraceMs = 150; + +// Called from the main (inspected) thread immediately before process exit +// (see VirtualMachine::global_exit in VirtualMachine.rs). Blocks briefly so +// the detached debugger thread isn't killed mid-delivery of queued inspector +// protocol messages -- without this, exit() tears down the debugger thread +// while messages are still queued or buffered, and the frontend misses the +// final events (most visibly the last TestReporter.start/end events from +// `bun test`, whose synchronous tests can queue right up to the same +// event-loop iteration that falls through to exit()). +// +// This addresses two distinct layers of loss: +// +// (a) The main->debugger-thread message handoff itself +// (sendMessageToDebuggerThread / receiveMessagesOnDebuggerThread). +// totalPendingDebuggerMessages tracks messages queued but not yet +// handed to onMessageFn; if any are pending we post a sentinel task and +// wait (capped) for it to run. Concurrent tasks on this context are +// FIFO, so once the sentinel runs, every receiveMessagesOnDebuggerThread +// invocation posted before this call has already invoked write() for +// every message it was holding. +// +// (b) Whatever write() already handed to the socket layer may still be +// sitting in that layer's own send buffer rather than on the wire -- +// e.g. a WebSocket send reporting backpressure means the message was +// accepted but not yet flushed. That buffer only drains as the +// debugger thread's own event loop services socket writability, which +// keeps running independently of this (main) thread. This case is NOT +// captured by totalPendingDebuggerMessages -- the handoff counter can +// already be zero (write() was called, decrementing it) while bytes are +// still buffered below write(). So the layer-(b) grace wait is gated on +// hasQueuedAnyDebuggerMessage instead, independent of the layer-(a) +// counter: gating it on that counter (as an earlier revision did) gave +// a message already sitting in the send buffer zero grace whenever +// there was no pending layer-(a) handoff at the moment of this call. +// hasQueuedAnyDebuggerMessage avoids that specific bug while still +// skipping the wait outright on the common case where nothing was ever +// queued for this debugger connection to begin with (e.g. a debugger +// attached but no messages were ever sent to it) -- in that case there +// is provably no pending output, so the wait would be pure cost. This +// intentionally adds a bounded (~kDrainFlushGraceMs) cost to every +// exit where at least one message was queued for the debugger thread. +// +// Both waits are capped so a wedged/starved debugger thread, or a consumer +// that has stopped reading entirely, cannot block process exit indefinitely. +// On timeout we simply degrade to pre-fix behavior for whatever hasn't been +// delivered yet -- a truly non-reading consumer is inherently undeliverable +// (a zero TCP/socket window can't be waited past), so timing out there is +// correct, not a bug. +// +// SCOPE: this is exercised on graceful main-process exit paths only (see the +// `global_exit()` call sites in VirtualMachine.rs's callers -- CLI run/test/ +// repl commands, `process.exit()`, bake production builds). Its behavior on +// a process torn down by a signal or `abort()` -- paths that do not run +// `global_exit()` -- is unverified by this change; no claim is made about +// those paths. +extern "C" void Bun__debugger__drain() +{ + if (debuggerScriptExecutionContext == nullptr) + return; + + // Layer (a): if a main->debugger-thread handoff is still pending, wait for + // it to catch up via a FIFO sentinel. + if (totalPendingDebuggerMessages.load(std::memory_order_acquire) != 0) { + auto signal = std::make_shared(); + debuggerScriptExecutionContext->postTaskConcurrently([signal](ScriptExecutionContext&) { + signal->semaphore.signal(); + }); + signal->semaphore.waitFor(WTF::Seconds::fromMilliseconds(kDrainHandoffTimeoutMs)); + } + + // Layer (b): give the debugger thread's own event loop a further bounded + // grace period to flush anything still sitting in the socket layer's send + // buffer to a normally-reading consumer. Gated on hasQueuedAnyDebuggerMessage + // (see its declaration above) rather than run unconditionally: if nothing + // was ever queued for this process's debugger thread, there is nothing + // that could possibly still be buffered, so the wait is skipped outright. + // When something WAS queued, we still can't tell from here whether it has + // already fully flushed -- unlike layer (a), there is no main-thread- + // visible counter for "bytes still buffered below write()": the layer-(a) + // counter can already be zero while such bytes remain. + // + // This is a deliberately simple, coarsely-gated wait rather than a poll of + // live buffer state: safely reading the debugger thread's JS-heap-owned + // writer state from this (main) thread would need new cross-thread + // synchronization whose surface area we judged not worth adding for a + // best-effort exit-time extension. Reusing BinarySemaphore purely for its + // timeout (nothing ever signals `topUp`) matches the existing + // wait-with-timeout-as-sleep idiom already used in this file (see + // pausedWait()'s `wait.condition.waitFor(wait.lock, Seconds(1))` above). + if (hasQueuedAnyDebuggerMessage.load(std::memory_order_acquire)) { + WTF::BinarySemaphore topUp; + topUp.waitFor(WTF::Seconds::fromMilliseconds(kDrainFlushGraceMs)); + } +} + extern "C" void BunDebugger__willHotReload() { if (debuggerScriptExecutionContext == nullptr) { diff --git a/test/cli/inspect/test-reporter.test.ts b/test/cli/inspect/test-reporter.test.ts index ccc4744f66f9..0fbe6f96a250 100644 --- a/test/cli/inspect/test-reporter.test.ts +++ b/test/cli/inspect/test-reporter.test.ts @@ -1,6 +1,6 @@ import { Subprocess, spawn, write } from "bun"; -import { afterEach, describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, isPosix, tempDir } from "harness"; +import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; +import { bunEnv, bunExe, isASAN, isDebug, isPosix, tempDir } from "harness"; import { join } from "node:path"; import { InspectorSession, connect } from "./junit-reporter"; import { SocketFramer } from "./socket-framer"; @@ -165,9 +165,17 @@ class TestReporterSession extends InspectorSession { } } +// Every test spawns `bun test` under --inspect-wait; the drain test below +// spawns a batch of them (and ASAN/debug startup alone is already several +// seconds), so give the suite more headroom than the default. +setDefaultTimeout(60_000); + describe.if(isPosix)("TestReporter inspector protocol", () => { let proc: Subprocess | undefined; let socket: ReturnType extends Promise ? T : never; + // The drain test below spawns several subprocesses in parallel; force-kill + // any that are still alive if the test times out or fails partway through. + const spawnedProcs = new Set(); afterEach(() => { proc?.kill(); @@ -175,6 +183,8 @@ describe.if(isPosix)("TestReporter inspector protocol", () => { // @ts-ignore - close the socket if it exists socket?.end?.(); socket = undefined as any; + for (const p of spawnedProcs) p.kill("SIGKILL"); + spawnedProcs.clear(); }); test("retroactively reports tests when TestReporter.enable is called after tests are discovered", async () => { @@ -310,4 +320,136 @@ afterAll(async () => { const exitCode = await proc.exited; expect(exitCode).toBe(0); }); + + test("flushes pending inspector messages to the frontend before process exit", async () => { + // Regression test for the exit-race this PR fixes: the main thread + // queues the final TestReporter events for the detached debugger thread + // to deliver, then calls exit() -- and without draining first, exit() + // can kill that thread mid-delivery. + // + // The fixture uses fast SYNCHRONOUS tests on purpose: synchronous tests + // let the runner fall straight through from the last test's `end` event + // into process exit within the same tick, which maximizes the race + // window (an async test would yield the event loop at least once, + // giving the debugger thread a natural opportunity to catch up). + // + // A single lone run of this on an idle many-core machine may well pass + // even without the fix -- the debugger thread usually gets scheduled + // quickly enough. What makes the race bite reliably is CPU contention: + // running a batch of these subprocesses in parallel starves the + // debugger thread of scheduler time relative to each subprocess's own + // main thread, which is what actually reproduces the loss. This test is + // reliably-failing-under-parallel-load pre-fix, not deterministic on a + // single idle run. + // + // COVERAGE SCOPE: this uses `--inspect-wait=unix:...`, which routes + // through debugger.ts's #connectOverSocket() / SocketFramer path, NOT the + // ws:/ws+unix: server path that uses webSocketWriter/bufferedWriter. So it + // exercises the layer-(a) main->debugger-thread drain only; it does NOT + // cover WebSocket-level backpressure delivery, which is a separate bug + // with its own fix and test in a separate PR. + const testCount = 5; + const body = Array.from( + { length: testCount }, + (_, i) => `test("t${i}", () => { expect(${i}).toBe(${i}); });`, + ).join("\n"); + + using dir = tempDir("test-reporter-drain", { + "drain.test.ts": ` +import { test, expect } from "bun:test"; +${body} +`, + }); + + async function once() { + const socketPath = join(String(dir), `inspector-${Math.random().toString(36).substring(2)}.sock`); + + const session = new TestReporterSession(); + const framer = new SocketFramer((message: string) => { + session.onMessage(message); + }); + + let localSocket: Awaited>; + const socketClosed = Promise.withResolvers(); + const socketPromise = connect(`unix://${socketPath}`, () => socketClosed.resolve()).then(s => { + localSocket = s; + session.socket = s; + session.framer = framer; + s.data = { + onData: framer.onData.bind(framer), + }; + return s; + }); + + const localProc = spawn({ + cmd: [bunExe(), `--inspect-wait=unix:${socketPath}`, "test", "drain.test.ts"], + env: bunEnv, + cwd: String(dir), + // "ignore" (not "pipe") for stdout: we only read stderr below, and an + // undrained "pipe" can deadlock the child once the OS pipe buffer fills. + stdout: "ignore", + stderr: "pipe", + }); + spawnedProcs.add(localProc); + + await socketPromise; + + // Enable TestReporter *before* Inspector.initialized so every test is + // reported via the normal (non-retroactive) live-collection path -- + // this test is isolating the exit-drain bug, not the ID-collision bug + // covered by the previous test. + session.enableInspector(); + session.enableTestReporter(); + session.initialize(); + + const [stderr, exitCode] = await Promise.all([localProc.stderr.text(), localProc.exited]); + spawnedProcs.delete(localProc); + + // Wait for the socket's FIN, not just process exit. On a Unix stream + // socket, FIN is ordered after data: once `close` fires, every byte + // the subprocess wrote has already been delivered through `onData` and + // into `session.onMessage`, so this is what actually guarantees the + // snapshot below reflects everything the subprocess sent (process exit + // alone would not -- the kernel can still be delivering already-queued + // bytes after the process is gone). + // @ts-ignore - close the socket if it exists + localSocket?.end?.(); + await socketClosed.promise; + + return { + stderr, + exitCode, + found: session.getFoundTests(), + started: session.getStartedTests(), + ended: session.getEndedTests(), + }; + } + + // Run a batch of subprocesses in parallel (see comment above for why + // that matters for reproducing the race) and check every one delivered + // everything. Under ASAN/debug builds subprocess startup itself is slow + // enough that this loses its ability to reproduce the pre-fix race + // within a reasonable timeout, so dial down the batch size there -- + // the test still verifies correctness, just with less contention. + const slow = isASAN || isDebug; + const width = slow ? 4 : 8; + const rounds = slow ? 1 : 2; + + const results: Awaited>[] = []; + for (let round = 0; round < rounds; round++) { + results.push(...(await Promise.all(Array.from({ length: width }, () => once())))); + } + + for (const { stderr, exitCode, found, started, ended } of results) { + expect(stderr).toContain(`${testCount} pass`); + expect(exitCode).toBe(0); + // Every test must have a `found`, a `start`, and an `end` event, and + // every `end` event must report a pass -- zero trailing loss across + // exit at any of the three reporting stages. + expect(found.size).toBe(testCount); + expect(started.size).toBe(testCount); + expect(ended.size).toBe(testCount); + expect([...ended.values()].map(e => e.status)).toEqual(Array(testCount).fill("pass")); + } + }); }); From edee9fe89a3e9454089a0ca697ef4cc19336ca25 Mon Sep 17 00:00:00 2001 From: "Craig R. Hughes" Date: Thu, 16 Jul 2026 00:08:59 -0700 Subject: [PATCH 2/2] review feedback: WTF::Ref drain signal, exception-safe drain accounting, scaled default timeout, exit-code-last assertions CodeRabbit review feedback on the exit-drain PR (34219-E/F/G/H): - BunDebugger.cpp: DebuggerDrainSignal now derives from WTF::ThreadSafeRefCounted with a private constructor and static create() (mirrors SharedEnvStore.h in the same directory), replacing the std::shared_ptr the waiting thread and posted task both held a reference through. Drops the now-unused include. - BunDebugger.cpp: receiveMessagesOnDebuggerThread now wraps the onMessageFn call in a DECLARE_TOP_EXCEPTION_SCOPE (matching WebKitBackend.cpp's HostClient::onData) and only decrements totalPendingDebuggerMessages when the batch delivered without throwing. An exception partway through a batch leaves the whole batch's count pending, since there's no way to know how far onMessageFn got -- the cost is bounded to at most one extra capped drain wait at exit (Bun__debugger__drain's waits are capped at 250ms/150ms regardless), never a stall. A termination exception is left uncleared so it keeps propagating. - test-reporter.test.ts: setDefaultTimeout scales to 120s under ASAN/debug builds (was a flat 60s), since setDefaultTimeout overrides the CI runner's own --timeout and a flat value would undercut the runner's ASAN allowance. - test-reporter.test.ts: in the drain test's per-result assertions, exitCode is now checked last, after the found/started/ended event-count and status assertions, so a delivery-loss failure reports the more diagnostic event-based mismatch rather than being masked by the exit-code check. --- src/jsc/bindings/BunDebugger.cpp | 40 +++++++++++++++++++++----- test/cli/inspect/test-reporter.test.ts | 11 ++++--- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/jsc/bindings/BunDebugger.cpp b/src/jsc/bindings/BunDebugger.cpp index 4302e6fcd684..ff81360dddbf 100644 --- a/src/jsc/bindings/BunDebugger.cpp +++ b/src/jsc/bindings/BunDebugger.cpp @@ -3,8 +3,8 @@ #include "ZigGlobalObject.h" #include +#include #include -#include #include #include #include @@ -403,6 +403,7 @@ class BunInspectorConnection : public Inspector::FrontendChannel { MarkedArgumentBuffer arguments; arguments.ensureCapacity(messageCount); auto& vm = debuggerGlobalObject->vm(); + auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); for (auto& message : messages) { arguments.append(jsString(vm, message)); @@ -414,8 +415,28 @@ class BunInspectorConnection : public Inspector::FrontendChannel { // After JSC::call, so onMessageFn (which calls into debugger.ts's // webSocketWriter/bufferedWriter write()) has been invoked for every - // message in this batch. See Bun__debugger__drain(). - if (messageCount > 0) + // message in this batch. See Bun__debugger__drain(). If onMessageFn + // threw partway through the batch, there is no way to know how many + // of the messages it was given actually made it to write() before + // the throw, so the whole batch is conservatively treated as + // undelivered (the count is left pending) rather than risking an + // undercount that would let Bun__debugger__drain() skip a wait for a + // message that never actually got written. The cost of that + // conservatism is bounded: Bun__debugger__drain() only reads this + // counter once as an entry gate for waits already capped at + // kDrainHandoffTimeoutMs (250ms) / kDrainFlushGraceMs (150ms), so an + // over-retained count costs at most one extra capped wait at exit, + // never a stall. + bool delivered = true; + if (auto* exception = scope.exception()) [[unlikely]] { + delivered = false; + if (scope.clearExceptionExceptTermination()) + debuggerGlobalObject->reportUncaughtExceptionAtEventLoop(debuggerGlobalObject, exception); + // else: termination exception -- left uncleared so it keeps + // propagating, and (as with any other exception here) not + // reported, since the process is already on its way down. + } + if (messageCount > 0 && delivered) totalPendingDebuggerMessages.fetch_sub(messageCount, std::memory_order_release); } @@ -632,10 +653,15 @@ extern "C" void Bun__ensureDebugger(ScriptExecutionContextIdentifier scriptId, b // so nothing is deliberately leaked (Bun's leak-sanitizer builds treat that // as a bug to fix -- see fba43af684, "Fix effectively every native-code // memory leak in Bun"). Both the waiting thread (Bun__debugger__drain) and -// the posted task hold a reference via std::shared_ptr; whichever finishes -// last frees it. -struct DebuggerDrainSignal { +// the posted task hold a reference via WTF::Ref (ThreadSafeRefCounted); +// whichever finishes last frees it. +struct DebuggerDrainSignal : public ThreadSafeRefCounted { +public: + static Ref create() { return adoptRef(*new DebuggerDrainSignal()); } WTF::BinarySemaphore semaphore; + +private: + DebuggerDrainSignal() = default; }; // Cap on how long the main thread waits, on exit, for the debugger thread to @@ -713,7 +739,7 @@ extern "C" void Bun__debugger__drain() // Layer (a): if a main->debugger-thread handoff is still pending, wait for // it to catch up via a FIFO sentinel. if (totalPendingDebuggerMessages.load(std::memory_order_acquire) != 0) { - auto signal = std::make_shared(); + auto signal = DebuggerDrainSignal::create(); debuggerScriptExecutionContext->postTaskConcurrently([signal](ScriptExecutionContext&) { signal->semaphore.signal(); }); diff --git a/test/cli/inspect/test-reporter.test.ts b/test/cli/inspect/test-reporter.test.ts index 0fbe6f96a250..ee8844b639eb 100644 --- a/test/cli/inspect/test-reporter.test.ts +++ b/test/cli/inspect/test-reporter.test.ts @@ -166,9 +166,12 @@ class TestReporterSession extends InspectorSession { } // Every test spawns `bun test` under --inspect-wait; the drain test below -// spawns a batch of them (and ASAN/debug startup alone is already several -// seconds), so give the suite more headroom than the default. -setDefaultTimeout(60_000); +// spawns a batch of them, and ASAN/debug startup alone is several seconds. +// Note setDefaultTimeout() overrides the CI runner's own --timeout (per-test +// option > setDefaultTimeout > CLI flag), so scale for slow builds here +// instead of using a flat value that would undercut the runner's ASAN +// allowance. +setDefaultTimeout(isASAN || isDebug ? 120_000 : 60_000); describe.if(isPosix)("TestReporter inspector protocol", () => { let proc: Subprocess | undefined; @@ -442,7 +445,6 @@ ${body} for (const { stderr, exitCode, found, started, ended } of results) { expect(stderr).toContain(`${testCount} pass`); - expect(exitCode).toBe(0); // Every test must have a `found`, a `start`, and an `end` event, and // every `end` event must report a pass -- zero trailing loss across // exit at any of the three reporting stages. @@ -450,6 +452,7 @@ ${body} expect(started.size).toBe(testCount); expect(ended.size).toBe(testCount); expect([...ended.values()].map(e => e.status)).toEqual(Array(testCount).fill("pass")); + expect(exitCode).toBe(0); } }); });