Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/jsc/Debugger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -354,6 +355,11 @@ impl Debugger {
}
}

/// 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();
}

/// `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.
Expand Down
5 changes: 5 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1516,6 +1516,11 @@ impl VirtualMachine {
// doing it causes like 50+ tests to break
// self.event_loop().tick();

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();
}

if self.should_destruct_main_thread_on_exit() {
#[cfg(windows)]
if let Some(t) = self.event_loop_mut().forever_timer.take() {
Expand Down
57 changes: 55 additions & 2 deletions src/jsc/bindings/BunDebugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -51,6 +53,9 @@ static PausedWait& pausedWait()
return instance;
}

// Entry gate for Bun__debugger__drain (process-global, not per-connection).
static std::atomic<uint64_t> totalPendingDebuggerMessages { 0 };

static bool waitingForConnection = false;
static bool bunControllerInstalled = false;
// Tracks whether connectFrontend has ever been called on the Bun-installed
Expand Down Expand Up @@ -436,13 +441,20 @@ class BunInspectorConnection : public ThreadSafeRefCounted<BunInspectorConnectio
this->debuggerThreadMessages.swap(messages);
}

if (!jsBunDebuggerOnMessageFunction)
size_t messageCount = messages.size();

if (!jsBunDebuggerOnMessageFunction) {
// Disconnected: batch dropped, account for it so Bun__debugger__drain's gate doesn't count it 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));
Expand All @@ -451,13 +463,25 @@ class BunInspectorConnection : public ThreadSafeRefCounted<BunInspectorConnectio
messages.clear();

JSC::call(debuggerGlobalObject, onMessageFn, arguments, "BunInspectorConnection::receiveMessagesOnDebuggerThread - onMessageFn"_s);

// On throw, leave the batch's count pending (can't know how many reached write(); costs at most one capped wait at exit).
bool delivered = true;
if (auto* exception = scope.exception()) [[unlikely]] {
delivered = false;
if (scope.clearExceptionExceptTermination())
debuggerGlobalObject->reportUncaughtExceptionAtEventLoop(debuggerGlobalObject, exception);
}
if (messageCount > 0 && delivered)
totalPendingDebuggerMessages.fetch_sub(messageCount, std::memory_order_release);
}

void sendMessageToDebuggerThread(WTF::String&& inputMessage)
{
{
Locker<Lock> locker(debuggerThreadMessagesLock);
debuggerThreadMessages.append(inputMessage);
// Inside the lock so receiveMessagesOnDebuggerThread's swap+decrement can't undercount.
totalPendingDebuggerMessages.fetch_add(1, std::memory_order_release);
}

if (this->debuggerThreadMessageScheduledCount++ == 0) {
Expand Down Expand Up @@ -668,6 +692,35 @@ 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.
struct DebuggerDrainSignal : public ThreadSafeRefCounted<DebuggerDrainSignal> {
public:
static Ref<DebuggerDrainSignal> create() { return adoptRef(*new DebuggerDrainSignal()); }
WTF::BinarySemaphore semaphore;

private:
DebuggerDrainSignal() = default;
};

static constexpr double kDrainHandoffTimeoutMs = 250;

// 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;

if (totalPendingDebuggerMessages.load(std::memory_order_acquire) == 0)
return;

// 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));
Comment thread
robobun marked this conversation as resolved.
}

extern "C" void BunDebugger__willHotReload()
{
if (debuggerScriptExecutionContext == nullptr) {
Expand Down
118 changes: 117 additions & 1 deletion test/cli/inspect/test-reporter.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -168,13 +168,18 @@ class TestReporterSession extends InspectorSession {
describe.if(isPosix)("TestReporter inspector protocol", () => {
let proc: Subprocess | undefined;
let socket: ReturnType<typeof connect> extends Promise<infer T> ? 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<Subprocess>();

afterEach(() => {
proc?.kill();
proc = undefined;
// @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 () => {
Expand Down Expand Up @@ -310,4 +315,115 @@ afterAll(async () => {
const exitCode = await proc.exited;
expect(exitCode).toBe(0);
});

test(
"flushes pending inspector messages to the frontend before process exit",
async () => {
// 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.
//
// 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.
//
// 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 },
(_, 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<ReturnType<typeof connect>>;
const socketClosed = Promise.withResolvers<void>();
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 live-collection path, not retroactively.
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: 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;

return {
stderr,
exitCode,
found: session.getFoundTests(),
started: session.getStartedTests(),
ended: session.getEndedTests(),
};
}

// 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;

const results: Awaited<ReturnType<typeof once>>[] = [];
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`);
// 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);
}
},
// Spawns a batch of --inspect-wait subprocesses; ASAN/debug startup is slow.
(isASAN || isDebug ? 120 : 60) * 1000,
);
});
Loading