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

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.

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() {

src/jsc/bindings/BunDebugger.cpp

@@ -4,6 +4,8 @@
 #include "ZigGlobalObject.h"
 
 #include <JavaScriptCore/InspectorFrontendChannel.h>
+#include <JavaScriptCore/TopExceptionScope.h>
+#include <wtf/threads/BinarySemaphore.h>
 #include <JavaScriptCore/JSGlobalObjectDebuggable.h>
 #include <JavaScriptCore/JSGlobalObjectDebugger.h>
 #include <JavaScriptCore/Debugger.h>
@@ -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<uint64_t> 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<bool> 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 ThreadSafeRefCounted<BunInspectorConnectio
             this->debuggerThreadMessages.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<JSFunction>(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,13 +499,48 @@ class BunInspectorConnection : public ThreadSafeRefCounted<BunInspectorConnectio
         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 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);
     }
 
     void sendMessageToDebuggerThread(WTF::String&& inputMessage)
     {
         {
             Locker<Lock> 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<DebuggerDrainSignal> {
+public:
+    static Ref<DebuggerDrainSignal> 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) {