From 1e4f6b305a0fba0ee3984d848bb7a6f1c3d7c140 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:55:59 +0000 Subject: [PATCH 1/6] inspector: drain queued frontend messages to the debugger thread before 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 --- src/jsc/Debugger.rs | 23 +++ src/jsc/VirtualMachine.rs | 9 ++ src/jsc/bindings/BunDebugger.cpp | 210 ++++++++++++++++++++++++- test/cli/inspect/test-reporter.test.ts | 141 ++++++++++++++++- 4 files changed, 380 insertions(+), 3 deletions(-) diff --git a/src/jsc/Debugger.rs b/src/jsc/Debugger.rs index 232c04b79a6b..c123fb26413c 100644 --- a/src/jsc/Debugger.rs +++ b/src/jsc/Debugger.rs @@ -169,6 +169,7 @@ unsafe extern "C" { is_connect: bool, is_node_inspector: bool, ); + safe fn Bun__debugger__drain(); } static FUTEX_ATOMIC: AtomicU32 = AtomicU32::new(0); @@ -354,6 +355,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 753a2eaecb08..d1622189339e 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -1516,6 +1516,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 c5a46de3013c..7b3281591fd7 100644 --- a/src/jsc/bindings/BunDebugger.cpp +++ b/src/jsc/bindings/BunDebugger.cpp @@ -4,6 +4,8 @@ #include "ZigGlobalObject.h" #include +#include +#include #include #include #include @@ -51,6 +53,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; static bool bunControllerInstalled = false; // Tracks whether connectFrontend has ever been called on the Bun-installed @@ -436,13 +474,23 @@ class BunInspectorConnection : public ThreadSafeRefCounteddebuggerThreadMessages.swap(messages); } - if (!jsBunDebuggerOnMessageFunction) + size_t messageCount = messages.size(); + + if (!jsBunDebuggerOnMessageFunction) { + // Disconnected (jsFunctionDisconnect cleared the callback): this + // batch is dropped, never reaching write(). Mark its handoff + // complete, or Bun__debugger__drain()'s entry gate would treat it + // as pending forever. + if (messageCount > 0) + totalPendingDebuggerMessages.fetch_sub(messageCount, std::memory_order_release); return; + } JSFunction* onMessageFn = uncheckedDowncast(jsBunDebuggerOnMessageFunction.get()); MarkedArgumentBuffer arguments; - arguments.ensureCapacity(messages.size()); + arguments.ensureCapacity(messageCount); auto& vm = debuggerGlobalObject->vm(); + auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); for (auto& message : messages) { arguments.append(jsString(vm, message)); @@ -451,6 +499,32 @@ class BunInspectorConnection : public ThreadSafeRefCountedreportUncaughtExceptionAtEventLoop(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); } void sendMessageToDebuggerThread(WTF::String&& inputMessage) @@ -458,6 +532,15 @@ class BunInspectorConnection : public ThreadSafeRefCounted 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) { @@ -668,6 +751,129 @@ 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 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 +// 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 = DebuggerDrainSignal::create(); + 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..2ce33be07569 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 { bunEnv, bunExe, isASAN, isDebug, isPosix, tempDir } from "harness"; import { join } from "node:path"; import { InspectorSession, connect } from "./junit-reporter"; import { SocketFramer } from "./socket-framer"; @@ -168,6 +168,9 @@ class TestReporterSession extends InspectorSession { 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 +178,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 +315,138 @@ 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 fixed by Bun__debugger__drain: 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 + // concern tracked independently. + 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`); + // 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")); + expect(exitCode).toBe(0); + } + // Per-test timeout: spawns a batch of `bun test --inspect-wait` subprocesses + // in parallel; ASAN/debug subprocess startup alone is several seconds each. + }, (isASAN || isDebug ? 120 : 60) * 1000); }); From d9215853f61ad5df6bf5bca90b1a416ef5aa9bcd Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:59:15 +0000 Subject: [PATCH 2/6] [autofix.ci] apply automated fixes --- test/cli/inspect/test-reporter.test.ts | 258 +++++++++++++------------ 1 file changed, 131 insertions(+), 127 deletions(-) diff --git a/test/cli/inspect/test-reporter.test.ts b/test/cli/inspect/test-reporter.test.ts index 2ce33be07569..c63483fd60d6 100644 --- a/test/cli/inspect/test-reporter.test.ts +++ b/test/cli/inspect/test-reporter.test.ts @@ -316,137 +316,141 @@ afterAll(async () => { expect(exitCode).toBe(0); }); - test("flushes pending inspector messages to the frontend before process exit", async () => { - // Regression test for the exit-race fixed by Bun__debugger__drain: 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 - // concern tracked independently. - 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": ` + test( + "flushes pending inspector messages to the frontend before process exit", + async () => { + // Regression test for the exit-race fixed by Bun__debugger__drain: 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 + // concern tracked independently. + 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), + 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(), }; - 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`); - // 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")); - expect(exitCode).toBe(0); - } - // Per-test timeout: spawns a batch of `bun test --inspect-wait` subprocesses - // in parallel; ASAN/debug subprocess startup alone is several seconds each. - }, (isASAN || isDebug ? 120 : 60) * 1000); + } + + // 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`); + // 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")); + expect(exitCode).toBe(0); + } + // Per-test timeout: spawns a batch of `bun test --inspect-wait` subprocesses + // in parallel; ASAN/debug subprocess startup alone is several seconds each. + }, + (isASAN || isDebug ? 120 : 60) * 1000, + ); }); From e46d851a3177d6110727cdc28dabe9af0d8fb050 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 31 Jul 2026 05:06:30 +0000 Subject: [PATCH 3/6] trim verbose design-rationale comments to essentials 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. --- src/jsc/Debugger.rs | 23 +--- src/jsc/VirtualMachine.rs | 8 +- src/jsc/bindings/BunDebugger.cpp | 181 ++++--------------------- test/cli/inspect/test-reporter.test.ts | 65 +++------ 4 files changed, 57 insertions(+), 220 deletions(-) diff --git a/src/jsc/Debugger.rs b/src/jsc/Debugger.rs index c123fb26413c..efbfc85bb2ed 100644 --- a/src/jsc/Debugger.rs +++ b/src/jsc/Debugger.rs @@ -355,24 +355,11 @@ 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). + /// Block (briefly, capped) until the debugger thread has delivered queued + /// inspector messages to the frontend socket. Call from the main thread + /// immediately before process exit so `exit()` doesn't kill the detached + /// debugger thread mid-delivery (e.g. `bun test`'s final `TestReporter.end` + /// events). See `Bun__debugger__drain` in `BunDebugger.cpp`. pub fn drain() { Bun__debugger__drain(); } diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index d1622189339e..9943a3e3b603 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -1516,11 +1516,9 @@ 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. + // Flush queued inspector messages so exit() doesn't kill the detached + // debugger thread mid-delivery. The debugger thread runs its own VM, + // so this wait never contends with the main VM's API lock. if self.debugger.is_some() { crate::debugger::Debugger::drain(); } diff --git a/src/jsc/bindings/BunDebugger.cpp b/src/jsc/bindings/BunDebugger.cpp index 7b3281591fd7..d3861ab30a26 100644 --- a/src/jsc/bindings/BunDebugger.cpp +++ b/src/jsc/bindings/BunDebugger.cpp @@ -53,40 +53,16 @@ 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?". +// Messages queued via sendMessageToDebuggerThread() that haven't yet reached +// onMessageFn on the debugger thread. Bun__debugger__drain() reads this only +// as an entry gate; the FIFO sentinel's ordering is the actual guarantee. +// Process-global (like debuggerScriptExecutionContext), not per-connection. 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. +// Set once (never cleared) the first time anything is queued for the debugger +// thread. Gates Bun__debugger__drain()'s socket-flush grace wait: the pending +// counter above is zero once write() returns even if bytes are still buffered +// below write(), so that counter can't gate the flush wait. static std::atomic hasQueuedAnyDebuggerMessage { false }; static bool waitingForConnection = false; @@ -477,10 +453,8 @@ class BunInspectorConnection : public ThreadSafeRefCounted 0) totalPendingDebuggerMessages.fetch_sub(messageCount, std::memory_order_release); return; @@ -500,28 +474,14 @@ class BunInspectorConnection : public ThreadSafeRefCountedreportUncaughtExceptionAtEventLoop(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); @@ -532,14 +492,9 @@ class BunInspectorConnection : public ThreadSafeRefCounted 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. + // Inside the lock: incrementing after unlock could race the swap+ + // decrement in receiveMessagesOnDebuggerThread and undercount. 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); } @@ -751,13 +706,8 @@ 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 WTF::Ref (ThreadSafeRefCounted); -// whichever finishes last frees it. +// Ref-counted so a timed-out wait doesn't leave a dangling pointer for the +// already-posted task: both sides hold a Ref, last one out frees it. struct DebuggerDrainSignal : public ThreadSafeRefCounted { public: static Ref create() { return adoptRef(*new DebuggerDrainSignal()); } @@ -767,80 +717,25 @@ struct DebuggerDrainSignal : public ThreadSafeRefCounted { DebuggerDrainSignal() = default; }; -// 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. +// Caps, not targets: a wedged debugger thread must never block exit forever. 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. +// Called from VirtualMachine::global_exit immediately before process exit so +// exit() doesn't kill the detached debugger thread mid-delivery of queued +// inspector messages (most visibly `bun test`'s final TestReporter events). // -// 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. +// Two layers: (a) the main->debugger-thread handoff itself, drained by posting +// a FIFO sentinel task and waiting for it (tasks on this context are FIFO, so +// once the sentinel runs every earlier receiveMessagesOnDebuggerThread has +// already called write()); and (b) a short further grace for the debugger +// thread's own event loop to flush the socket send buffer. Both waits are +// capped; on timeout we degrade to pre-fix behavior. 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 = DebuggerDrainSignal::create(); debuggerScriptExecutionContext->postTaskConcurrently([signal](ScriptExecutionContext&) { @@ -849,25 +744,9 @@ extern "C" void Bun__debugger__drain() 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). + // Gated on hasQueuedAnyDebuggerMessage, not totalPendingDebuggerMessages: + // the latter can be zero while bytes are still buffered below write(). + // BinarySemaphore is used purely for its timeout (nothing signals it). if (hasQueuedAnyDebuggerMessage.load(std::memory_order_acquire)) { WTF::BinarySemaphore topUp; topUp.waitFor(WTF::Seconds::fromMilliseconds(kDrainFlushGraceMs)); diff --git a/test/cli/inspect/test-reporter.test.ts b/test/cli/inspect/test-reporter.test.ts index c63483fd60d6..ca70e950e442 100644 --- a/test/cli/inspect/test-reporter.test.ts +++ b/test/cli/inspect/test-reporter.test.ts @@ -319,32 +319,18 @@ afterAll(async () => { test( "flushes pending inspector messages to the frontend before process exit", async () => { - // Regression test for the exit-race fixed by Bun__debugger__drain: 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. + // Regression for the exit-race fixed by Bun__debugger__drain: the main + // thread queues the final TestReporter events for the detached debugger + // thread, then falls through to exit() which 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). + // Fast SYNCHRONOUS fixture tests on purpose: they let the runner reach + // exit() in the same tick as the last `end` event. A lone run on an idle + // machine may pass even without the fix; parallel spawns supply the CPU + // contention that makes the race bite reliably. // - // 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 - // concern tracked independently. + // Uses --inspect-wait=unix:... (SocketFramer path), not ws:, so this + // exercises the main->debugger-thread drain, not WebSocket backpressure. const testCount = 5; const body = Array.from( { length: testCount }, @@ -391,10 +377,8 @@ ${body} 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. + // Enable TestReporter before Inspector.initialized so every test is + // reported via the normal live-collection path, not retroactively. session.enableInspector(); session.enableTestReporter(); session.initialize(); @@ -402,13 +386,9 @@ ${body} 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). + // Wait for the socket's FIN, not just process exit: FIN is ordered + // after data, so once `close` fires every byte the subprocess wrote has + // been delivered. Process exit alone wouldn't guarantee this. // @ts-ignore - close the socket if it exists localSocket?.end?.(); await socketClosed.promise; @@ -422,12 +402,8 @@ ${body} }; } - // 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. + // Parallel batch for contention (see above). ASAN/debug subprocess + // startup is slow enough that a smaller batch still fits the timeout. const slow = isASAN || isDebug; const width = slow ? 4 : 8; const rounds = slow ? 1 : 2; @@ -439,18 +415,15 @@ ${body} for (const { stderr, exitCode, found, started, ended } of results) { expect(stderr).toContain(`${testCount} pass`); - // 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. + // Zero trailing loss 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")); expect(exitCode).toBe(0); } - // Per-test timeout: spawns a batch of `bun test --inspect-wait` subprocesses - // in parallel; ASAN/debug subprocess startup alone is several seconds each. }, + // Spawns a batch of --inspect-wait subprocesses; ASAN/debug startup is slow. (isASAN || isDebug ? 120 : 60) * 1000, ); }); From 80870ce4268f03a946d9f2e9b8ab18e9a9628a17 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 31 Jul 2026 05:10:48 +0000 Subject: [PATCH 4/6] reduce added src/ comments to single lines The detailed two-layer design rationale (FIFO sentinel ordering guarantee, separate sticky gate flag, capped waits) lives in the PR body. --- src/jsc/Debugger.rs | 6 +---- src/jsc/VirtualMachine.rs | 4 +--- src/jsc/bindings/BunDebugger.cpp | 40 +++++++------------------------- 3 files changed, 10 insertions(+), 40 deletions(-) diff --git a/src/jsc/Debugger.rs b/src/jsc/Debugger.rs index efbfc85bb2ed..86eef828b8aa 100644 --- a/src/jsc/Debugger.rs +++ b/src/jsc/Debugger.rs @@ -355,11 +355,7 @@ impl Debugger { } } - /// Block (briefly, capped) until the debugger thread has delivered queued - /// inspector messages to the frontend socket. Call from the main thread - /// immediately before process exit so `exit()` doesn't kill the detached - /// debugger thread mid-delivery (e.g. `bun test`'s final `TestReporter.end` - /// events). See `Bun__debugger__drain` in `BunDebugger.cpp`. + /// Bounded wait for the debugger thread to deliver queued inspector messages before `exit()` kills it mid-delivery; see `Bun__debugger__drain` in `BunDebugger.cpp`. pub fn drain() { Bun__debugger__drain(); } diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 9943a3e3b603..c3487618532a 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -1516,10 +1516,8 @@ 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 VM, - // so this wait never contends with the main VM's API lock. if self.debugger.is_some() { + // Flush queued inspector messages so exit() doesn't kill the detached debugger thread mid-delivery; that thread runs its own VM so this never contends with the main VM's API lock. crate::debugger::Debugger::drain(); } diff --git a/src/jsc/bindings/BunDebugger.cpp b/src/jsc/bindings/BunDebugger.cpp index d3861ab30a26..e4f899de7157 100644 --- a/src/jsc/bindings/BunDebugger.cpp +++ b/src/jsc/bindings/BunDebugger.cpp @@ -53,16 +53,8 @@ static PausedWait& pausedWait() return instance; } -// Messages queued via sendMessageToDebuggerThread() that haven't yet reached -// onMessageFn on the debugger thread. Bun__debugger__drain() reads this only -// as an entry gate; the FIFO sentinel's ordering is the actual guarantee. -// Process-global (like debuggerScriptExecutionContext), not per-connection. +// See Bun__debugger__drain: entry-gate counter (process-global, not per-connection), and a sticky "ever queued" flag for its flush-grace wait. static std::atomic totalPendingDebuggerMessages { 0 }; - -// Set once (never cleared) the first time anything is queued for the debugger -// thread. Gates Bun__debugger__drain()'s socket-flush grace wait: the pending -// counter above is zero once write() returns even if bytes are still buffered -// below write(), so that counter can't gate the flush wait. static std::atomic hasQueuedAnyDebuggerMessage { false }; static bool waitingForConnection = false; @@ -453,8 +445,7 @@ class BunInspectorConnection : public ThreadSafeRefCounted 0) totalPendingDebuggerMessages.fetch_sub(messageCount, std::memory_order_release); return; @@ -474,9 +465,7 @@ class BunInspectorConnection : public ThreadSafeRefCounted locker(debuggerThreadMessagesLock); debuggerThreadMessages.append(inputMessage); - // Inside the lock: incrementing after unlock could race the swap+ - // decrement in receiveMessagesOnDebuggerThread and undercount. + // Inside the lock so receiveMessagesOnDebuggerThread's swap+decrement can't undercount. totalPendingDebuggerMessages.fetch_add(1, std::memory_order_release); hasQueuedAnyDebuggerMessage.store(true, std::memory_order_release); } @@ -706,8 +694,7 @@ extern "C" void Bun__ensureDebugger(ScriptExecutionContextIdentifier scriptId, b } } -// Ref-counted so a timed-out wait doesn't leave a dangling pointer for the -// already-posted task: both sides hold a Ref, last one out frees it. +// Ref-counted so a timed-out wait doesn't leave a dangling pointer for the already-posted task. struct DebuggerDrainSignal : public ThreadSafeRefCounted { public: static Ref create() { return adoptRef(*new DebuggerDrainSignal()); } @@ -717,25 +704,16 @@ struct DebuggerDrainSignal : public ThreadSafeRefCounted { DebuggerDrainSignal() = default; }; -// Caps, not targets: a wedged debugger thread must never block exit forever. static constexpr double kDrainHandoffTimeoutMs = 250; static constexpr double kDrainFlushGraceMs = 150; -// Called from VirtualMachine::global_exit immediately before process exit so -// exit() doesn't kill the detached debugger thread mid-delivery of queued -// inspector messages (most visibly `bun test`'s final TestReporter events). -// -// Two layers: (a) the main->debugger-thread handoff itself, drained by posting -// a FIFO sentinel task and waiting for it (tasks on this context are FIFO, so -// once the sentinel runs every earlier receiveMessagesOnDebuggerThread has -// already called write()); and (b) a short further grace for the debugger -// thread's own event loop to flush the socket send buffer. Both waits are -// capped; on timeout we degrade to pre-fix behavior. +// Called from VirtualMachine::global_exit so exit() doesn't kill the detached debugger thread mid-delivery of queued inspector messages (e.g. `bun test`'s final TestReporter events). extern "C" void Bun__debugger__drain() { if (debuggerScriptExecutionContext == nullptr) return; + // Layer (a): FIFO sentinel. Tasks on this context are FIFO, so once it runs every earlier receiveMessagesOnDebuggerThread has already called write(). if (totalPendingDebuggerMessages.load(std::memory_order_acquire) != 0) { auto signal = DebuggerDrainSignal::create(); debuggerScriptExecutionContext->postTaskConcurrently([signal](ScriptExecutionContext&) { @@ -744,9 +722,7 @@ extern "C" void Bun__debugger__drain() signal->semaphore.waitFor(WTF::Seconds::fromMilliseconds(kDrainHandoffTimeoutMs)); } - // Gated on hasQueuedAnyDebuggerMessage, not totalPendingDebuggerMessages: - // the latter can be zero while bytes are still buffered below write(). - // BinarySemaphore is used purely for its timeout (nothing signals it). + // Layer (b): brief grace for the debugger thread's event loop to flush the socket send buffer. Gated on hasQueuedAnyDebuggerMessage (the pending counter can be zero while bytes are still buffered below write()); BinarySemaphore used purely for its timeout. if (hasQueuedAnyDebuggerMessage.load(std::memory_order_acquire)) { WTF::BinarySemaphore topUp; topUp.waitFor(WTF::Seconds::fromMilliseconds(kDrainFlushGraceMs)); From 268d010faacb7653ae47337f034c4baaad07bc60 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 31 Jul 2026 05:44:45 +0000 Subject: [PATCH 5/6] drop the unconditional 150ms flush-grace sleep 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. --- src/jsc/bindings/BunDebugger.cpp | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/jsc/bindings/BunDebugger.cpp b/src/jsc/bindings/BunDebugger.cpp index e4f899de7157..d9781770fe7e 100644 --- a/src/jsc/bindings/BunDebugger.cpp +++ b/src/jsc/bindings/BunDebugger.cpp @@ -53,9 +53,8 @@ static PausedWait& pausedWait() return instance; } -// See Bun__debugger__drain: entry-gate counter (process-global, not per-connection), and a sticky "ever queued" flag for its flush-grace wait. +// Entry gate for Bun__debugger__drain (process-global, not per-connection). static std::atomic totalPendingDebuggerMessages { 0 }; -static std::atomic hasQueuedAnyDebuggerMessage { false }; static bool waitingForConnection = false; static bool bunControllerInstalled = false; @@ -483,7 +482,6 @@ class BunInspectorConnection : public ThreadSafeRefCounteddebuggerThreadMessageScheduledCount++ == 0) { @@ -705,7 +703,6 @@ struct DebuggerDrainSignal : public ThreadSafeRefCounted { }; static constexpr double kDrainHandoffTimeoutMs = 250; -static constexpr double kDrainFlushGraceMs = 150; // Called from VirtualMachine::global_exit so exit() doesn't kill the detached debugger thread mid-delivery of queued inspector messages (e.g. `bun test`'s final TestReporter events). extern "C" void Bun__debugger__drain() @@ -713,20 +710,15 @@ extern "C" void Bun__debugger__drain() if (debuggerScriptExecutionContext == nullptr) return; - // Layer (a): FIFO sentinel. Tasks on this context are FIFO, so once it runs every earlier receiveMessagesOnDebuggerThread has already called write(). - if (totalPendingDebuggerMessages.load(std::memory_order_acquire) != 0) { - auto signal = DebuggerDrainSignal::create(); - debuggerScriptExecutionContext->postTaskConcurrently([signal](ScriptExecutionContext&) { - signal->semaphore.signal(); - }); - signal->semaphore.waitFor(WTF::Seconds::fromMilliseconds(kDrainHandoffTimeoutMs)); - } + if (totalPendingDebuggerMessages.load(std::memory_order_acquire) == 0) + return; - // Layer (b): brief grace for the debugger thread's event loop to flush the socket send buffer. Gated on hasQueuedAnyDebuggerMessage (the pending counter can be zero while bytes are still buffered below write()); BinarySemaphore used purely for its timeout. - if (hasQueuedAnyDebuggerMessage.load(std::memory_order_acquire)) { - WTF::BinarySemaphore topUp; - topUp.waitFor(WTF::Seconds::fromMilliseconds(kDrainFlushGraceMs)); - } + // FIFO sentinel: tasks on this context are FIFO, so once it runs every earlier receiveMessagesOnDebuggerThread has already called write() on the socket. + auto signal = DebuggerDrainSignal::create(); + debuggerScriptExecutionContext->postTaskConcurrently([signal](ScriptExecutionContext&) { + signal->semaphore.signal(); + }); + signal->semaphore.waitFor(WTF::Seconds::fromMilliseconds(kDrainHandoffTimeoutMs)); } extern "C" void BunDebugger__willHotReload() From b1567b6c8418f8e39b8f00d76b769072e483fd28 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 31 Jul 2026 06:16:23 +0000 Subject: [PATCH 6/6] ci: retrigger