Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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
24 changes: 23 additions & 1 deletion src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,22 @@
|| !el.next_immediate_tasks.is_empty()
}

/// Count of ref'd handles and outstanding tasks keeping the loop alive.
/// Snapshotted by the test runner as a per-file drain baseline so prior
/// leaks (or a --parallel worker's IPC pipe) are not waited on.

Check warning on line 1064 in src/jsc/VirtualMachine.rs

View check run for this annotation

Claude / Claude Code Review

Stale doc comment on active_keepalive_count() describes removed baseline mechanism

The doc comment says "Snapshotted by the test runner as a per-file drain baseline", but that describes the `keepalive_baseline` mechanism from 80cdf80 that 39c86cc removed — the sole caller now uses this only as an idle predicate (`== 0` → `idle_after_preloads`) and a drain-until-zero condition; there is no baseline snapshot anywhere. Suggest rewording to describe the idle-gate use, e.g. "Used by the test runner's idle-after-preloads gate: the post-file drain runs only when this was zero before
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn active_keepalive_count(&self) -> usize {
let el = self.event_loop_shared();
let active = self
.platform_loop_opt()
.map(|h| h.active_count() as usize)
.unwrap_or(0);
let concurrent = el
.concurrent_ref
.load(core::sync::atomic::Ordering::SeqCst)
.max(0) as usize;
active + self.active_tasks + concurrent
}

pub fn wakeup(&mut self) {
self.event_loop_mut().wakeup();
}
Expand Down Expand Up @@ -4592,6 +4608,7 @@
pub fn reload_entry_point_for_test_runner(
&mut self,
entry_path: &[u8],
after_preloads: impl FnOnce(&Self),
) -> crate::CrateResult<*mut JSInternalPromise> {
self.has_loaded = false;
self.set_main(entry_path);
Expand All @@ -4618,6 +4635,8 @@
}
}

after_preloads(self);

// Note: reshaped for borrowck.
let global = self.global;
let main_str = bun_core::String::from_bytes(self.main());
Expand Down Expand Up @@ -4648,11 +4667,14 @@
}

/// Loads a test-file entry point and waits for the load promise to settle.
/// `after_preloads` runs between preload completion and entry-point
/// evaluation so the caller can observe preload-created handles.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn load_entry_point_for_test_runner(
&mut self,
entry_path: &[u8],
after_preloads: impl FnOnce(&Self),
) -> crate::CrateResult<*mut JSInternalPromise> {
let promise = self.reload_entry_point_for_test_runner(entry_path)?;
let promise = self.reload_entry_point_for_test_runner(entry_path, after_preloads)?;

// pending_internal_promise can change if hot module reloading is enabled
if self.is_watcher_enabled() {
Expand Down
4 changes: 4 additions & 0 deletions src/libuv_sys/libuv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,10 @@ impl Loop {
unsafe { uv_loop_alive(self) != 0 }
}
#[inline]
pub fn active_count(&self) -> u32 {
self.active_handles
}
#[inline]
pub fn tick(&mut self) {
// SAFETY: self is a live loop.
let _ = unsafe { uv_run(self, RunMode::Default) };
Expand Down
34 changes: 33 additions & 1 deletion src/runtime/cli/test_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3142,7 +3142,12 @@

// need to wake up so autoTick() doesn't wait for 16-100ms after loading the entrypoint
vm.wakeup();
let promise = vm.load_entry_point_for_test_runner(file_path)?;
// Record whether the loop was idle after preloads so the post-test
// drain (below) only runs when every ref'd handle is this file's.
let mut idle_after_preloads = false;
let promise = vm.load_entry_point_for_test_runner(file_path, |vm| {
idle_after_preloads = script_keepalive_count(vm) == 0;
})?;
// Only count the file once, not once per repeat
if repeat_index == 0 {
reporter.summary().files += 1;
Expand Down Expand Up @@ -3233,6 +3238,18 @@
}
}

// A file with no test()/describe() is a plain script: drain the
// loop so late errors surface (like `bun <file>`). Gated on an
// idle loop after preloads so prior-file handles can't hang it.
if buntest.collection.root_scope.entries.is_empty() && idle_after_preloads {
let drain_base = vm.unhandled_error_counter;
while drain_base == vm.unhandled_error_counter && script_keepalive_count(vm) > 0
{
vm.event_loop_ref().tick();
vm.event_loop_ref().auto_tick();
}
}

Check failure on line 3251 in src/runtime/cli/test_command.rs

View check run for this annotation

Claude / Claude Code Review

idle_after_preloads gate bypassed by beforeAll hooks and unref'd timers, causing hang regression

The `idle_after_preloads` gate is bypassed by two mechanisms that leave `script_keepalive_count(vm) == 0` at snapshot time but create ref'd handles afterward, causing the drain loop to **hang forever** (regression from pre-PR exit 0): (1) a preload's `beforeAll()` only *registers* the hook — its body (`Bun.serve`, etc.) executes later inside the phase loop via `generate_all_order(&root.hook_scope.before_all)`, after the idle snapshot; (2) a prior file's `.unref()`'d timer contributes 0 to `activ
Comment thread
robobun marked this conversation as resolved.
Outdated

let el = vm.event_loop();
// SAFETY: el is the VM-owned event loop; vm is passed back as *mut.
unsafe { (*el).tick_immediate_tasks(vm) };
Expand Down Expand Up @@ -3260,6 +3277,21 @@
}
}

/// Count of ref'd handles plus individual JS timers (timers share one loop
/// ref, so `active_count()` alone misses additional timers). Zero means the
/// file's own work is the only thing keeping the loop alive.
Comment thread
robobun marked this conversation as resolved.
Outdated
fn script_keepalive_count(vm: &VirtualMachine) -> usize {
let state = crate::jsc_hooks::runtime_state();
let timers = if state.is_null() {
0
} else {
// SAFETY: `runtime_state()` returns the live per-thread `RuntimeState`;
// `active_timer_count` is plain data, read on the owning JS thread.
unsafe { (*state).timer.active_timer_count.max(0) as usize }
};
vm.active_keepalive_count() + timers
}

pub(crate) fn handle_top_level_test_error_before_javascript_start(err: &crate::Error) -> ! {
if cfg!(debug_assertions) {
if !matches!(err, crate::Error::ModuleNotFound) {
Expand Down
5 changes: 5 additions & 0 deletions src/uws_sys/Loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ impl PosixLoop {
self.active > 0
}

#[inline]
pub fn active_count(&self) -> u32 {
self.active
}

// This exists as a method so that we can stick a debugger in here
pub fn add_active(&mut self, value: u32) {
bun_core::scoped_log!(
Expand Down
161 changes: 161 additions & 0 deletions test/cli/test/bun-test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1458,6 +1458,167 @@ describe("bun test", () => {
});
});

// https://github.com/oven-sh/bun/issues/34859
describe.concurrent("script files with no test() registrations", () => {
test("fails on a delayed unhandled rejection from an async IIFE", async () => {
using dir = tempDir("script-delayed-rejection", {
"delayed.test.js": `
'use strict';
(async () => {
await new Promise(r => setTimeout(r, 20));
throw new Error("delayed rejection should fail the test run");
})();
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "./delayed.test.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("delayed rejection should fail the test run");
expect(stderr).toContain("Unhandled error between tests");
expect(stderr).toContain("1 error");
expect(exitCode).toBe(1);
});

test("fails on a delayed uncaught exception from a timer", async () => {
using dir = tempDir("script-delayed-throw", {
"delayed.test.js": `setTimeout(() => { throw new Error("boom from timer"); }, 20);`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "./delayed.test.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("boom from timer");
expect(stderr).toContain("Unhandled error between tests");
expect(exitCode).toBe(1);
});

test("waits for ref'd async work to complete", async () => {
using dir = tempDir("script-drain", {
"drain.test.js": `
const assert = require("assert");
(async () => {
await new Promise(r => setTimeout(r, 20));
assert.strictEqual(1, 1);
console.log("reached end of async iife");
})();
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "./drain.test.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toContain("reached end of async iife");
expect(stderr).toContain("0 fail");
expect(exitCode).toBe(0);
});

test("drains per file so a later script file's delayed rejection is caught", async () => {
using dir = tempDir("script-two-files", {
"a.test.js": `
(async () => {
await Promise.resolve();
throw new Error("file A microtask error");
})();
`,
"b.test.js": `
(async () => {
await new Promise(r => setTimeout(r, 20));
throw new Error("file B delayed error");
})();
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "./a.test.js", "./b.test.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("file A microtask error");
expect(stderr).toContain("file B delayed error");
expect(exitCode).toBe(1);
});

test("does not drain when the file registered a test()", async () => {
// Files that use bun:test keep today's behavior: the runner does not
// block on leaked handles after the last test finishes.
using dir = tempDir("registered-test-no-drain", {
"with.test.js": `
const { test } = require("bun:test");
setInterval(() => {}, 60_000);
test("noop", () => {});
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "./with.test.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("1 pass");
expect(exitCode).toBe(0);
});

test("does not drain when a prior file left a ref'd handle", async () => {
// a leaks an interval. b's drain is skipped because the loop was not
// idle after preloads, so the run must not hang on a's interval.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
using dir = tempDir("prior-file-leak", {
"a.test.js": `
const { test } = require("bun:test");
setInterval(() => {}, 60_000);
test("noop", () => {});
`,
"b.test.js": `require("assert").strictEqual(1, 1);`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "./a.test.js", "./b.test.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("1 pass");
expect(stderr).toContain("0 fail");
expect(exitCode).toBe(0);
});

test("does not drain when a preload left a ref'd handle", async () => {
using dir = tempDir("preload-leak", {
"setup.js": `setInterval(() => {}, 60_000);`,
"a.test.js": `require("assert").strictEqual(1, 1);`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "--preload", "./setup.js", "./a.test.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
// If the drain waited on the preload's interval this would hang; the
// test framework's own timeout is the guard.
expect(stderr).toContain("0 fail");
expect(exitCode).toBe(0);
});
});

function createTest(input?: string | (string | { filename: string; contents: string })[], filename?: string): string {
const cwd = tmpdirSync();
const inputs = Array.isArray(input) ? input : [input ?? ""];
Expand Down
Loading