From 1d50151aa3b1800b20223dd5d55970c742d654ce Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Wed, 14 Oct 2026 09:02:01 +0800 Subject: [PATCH 1/3] test(node): own HTTP cleanup and make connect abort tests hermetic Wait for request and server cleanup, assert timeout notification and socket error ordering, and replace WAN abort races with held lookup and local pending-connection fixtures. Keep pending-abort parity with Node separate from Bun pre-aborted immediate-connect behavior. --- test/js/node/http/node-http.test.ts | 164 +++++++++++------- test/js/node/net/net-abort-lookup-fixture.cjs | 79 +++++++++ test/js/node/net/node-net.test.ts | 113 ++++++++---- 3 files changed, 257 insertions(+), 99 deletions(-) create mode 100644 test/js/node/net/net-abort-lookup-fixture.cjs diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index 9296bd11e22f..d190cf0f0b3a 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -619,7 +619,6 @@ describe("node:http", () => { describe("request", () => { function runTest(done: Function, callback: (server: Server, port: number, done: (err?: Error) => void) => void) { - let timer; const server = createServer((req, res) => { if (Object.getPrototypeOf(req.headers) !== Object.prototype) { // Like Node.js, req.headers is a plain object. @@ -630,7 +629,7 @@ describe("node:http", () => { if (reqUrl.pathname === "/redirect") { // Temporary redirect res.writeHead(301, { - Location: `http://localhost:${server.port}/redirected`, + Location: `http://localhost:${(server.address() as AddressInfo).port}/redirected`, }); res.end("Got redirect!\n"); return; @@ -660,22 +659,8 @@ describe("node:http", () => { res.end("Hello World"); return; } - if (reqUrl.pathname.includes("timeout")) { - if (timer) clearTimeout(timer); - req.on("timeout", () => { - console.log("req timeout"); - }); - res.on("timeout", () => { - console.log("res timeout"); - }); - timer = setTimeout(() => { - if (res.closed) { - return; - } - - res.end("Hello World"); - timer = null; - }, 3000).unref(); + if (reqUrl.pathname === "/timeout") { + // The client owns timeout cleanup; this route must never send a response. return; } if (reqUrl.pathname === "/pathTest") { @@ -752,12 +737,11 @@ describe("node:http", () => { res.end("Hello World"); } }); - server.listen({ port: 0 }, (_, __, port) => { - var _done = (...args) => { - server.close(); - done(...args); + server.listen({ port: 0 }, () => { + const _done = (err?: Error) => { + server.close(closeError => done(err ?? closeError)); }; - callback(server, port, _done); + callback(server, (server.address() as AddressInfo).port, _done); }); } @@ -913,40 +897,76 @@ describe("node:http", () => { }); it("should allow us to set timeout with request.setTimeout or `timeout` in options", done => { - runTest(done, (server, serverPort, done) => { - const createDone = createDoneDotAll(done); - const req1Done = createDone(); - const req2Done = createDone(); - - const req1 = request( - { - host: "localhost", - port: serverPort, - path: "/timeout", - timeout: 500, - }, - res => { - req1Done(new Error("Should not have received response")); - }, - ); - req1.on("timeout", () => req1Done()); - - const req2 = request( - { - host: "localhost", - port: serverPort, - path: "/timeout", - }, - res => { - req2Done(new Error("Should not have received response")); - }, - ); - - req2.setTimeout(500, () => { - req2Done(); - }); - req1.end(); - req2.end(); + runTest(done, async (server, serverPort, done) => { + const requests: { + req: http.ClientRequest; + reason: Error; + events: string[]; + errors: Error[]; + destroyedAtTimeout: boolean[]; + closed: Promise; + }[] = []; + let failure; + try { + for (const useOptions of [true, false]) { + const reason = new Error(useOptions ? "options timeout cleanup" : "setTimeout cleanup"); + const req = request({ + host: "localhost", + port: serverPort, + path: "/timeout", + ...(useOptions ? { timeout: 500 } : {}), + }); + const { promise: closed, resolve: onClose } = Promise.withResolvers(); + const state = { + req, + reason, + events: [] as string[], + errors: [] as Error[], + destroyedAtTimeout: [] as boolean[], + closed, + }; + requests.push(state); + req.on("socket", () => state.events.push("socket")); + req.on("response", res => { + state.events.push("response"); + res.resume(); + req.destroy(reason); + }); + req.on("error", error => { + state.events.push("error"); + state.errors.push(error); + }); + req.once("close", () => { + state.events.push("close"); + onClose(); + }); + const onTimeout = () => { + state.events.push("timeout"); + state.destroyedAtTimeout.push(req.destroyed); + req.destroy(reason); + }; + if (useOptions) req.on("timeout", onTimeout); + else req.setTimeout(500, onTimeout); + req.end(); + } + // Timeout only notifies. Settle both explicit destroys before assertions or server teardown. + await Promise.all(requests.map(({ closed }) => closed)); + for (const { req, reason, events, errors, destroyedAtTimeout } of requests) { + expect(events).toEqual(["socket", "timeout", "error", "close"]); + expect(destroyedAtTimeout).toEqual([false]); + expect(errors).toHaveLength(1); + expect(errors[0]).toBe(reason); + expect(req.destroyed).toBe(true); + } + } catch (error) { + failure = error; + } finally { + for (const { req, reason } of requests) { + if (!req.destroyed) req.destroy(reason); + } + await Promise.all(requests.map(({ closed }) => closed)); + done(failure); + } }); }); @@ -1351,16 +1371,30 @@ describe("node:http", () => { }); }); - it("should emit a socket event when connecting", async done => { - runTest(done, async (server, serverPort, done) => { + it("should emit a socket event when connecting", done => { + runTest(done, (server, serverPort, done) => { const req = request(`http://localhost:${serverPort}`, {}); - // Destroying an in-flight request surfaces a "socket hang up" error, - // exactly like Node.js; swallow it so it does not leak into the next - // test as an unhandled error. - req.on("error", () => {}); - req.on("socket", function onRequestSocket(socket) { + const events: string[] = []; + const errors: Error[] = []; + req.on("error", error => { + events.push("error"); + errors.push(error); + }); + req.on("socket", () => { + events.push("socket"); req.destroy(); - done(); + }); + req.once("close", () => { + events.push("close"); + try { + expect(events).toEqual(["socket", "error", "close"]); + expect(errors).toHaveLength(1); + expect(errors[0]).toHaveProperty("code", "ECONNRESET"); + expect(req.destroyed).toBe(true); + done(); + } catch (error) { + done(error); + } }); req.end(); }); diff --git a/test/js/node/net/net-abort-lookup-fixture.cjs b/test/js/node/net/net-abort-lookup-fixture.cjs new file mode 100644 index 000000000000..013a6380541d --- /dev/null +++ b/test/js/node/net/net-abort-lookup-fixture.cjs @@ -0,0 +1,79 @@ +const assert = require("node:assert/strict"); +const { once } = require("node:events"); +const { createServer, Socket } = require("node:net"); + +async function main() { + const autoSelectFamily = process.argv[2] === "true"; + const alreadyAborted = process.argv[3] === "true"; + const controller = new AbortController(); + const reason = new Error("lookup aborted"); + if (alreadyAborted) controller.abort(reason); + + const accepted = []; + const server = createServer(connection => { + accepted.push(connection); + connection.destroy(); + }); + const events = []; + const errors = []; + let socket; + let closed; + + try { + await once(server.listen(0, "127.0.0.1"), "listening"); + socket = new Socket({ signal: controller.signal }); + closed = new Promise(resolve => + socket.once("close", () => { + events.push("close"); + resolve(); + }), + ); + socket.on("error", error => { + events.push("error"); + errors.push(error); + }); + socket.on("connectionAttempt", () => events.push("connectionAttempt")); + socket.on("connect", () => events.push("connect")); + const { promise: lookupStarted, resolve: onLookup } = Promise.withResolvers(); + let completeLookup; + socket.connect({ + host: "held-lookup.test", + port: server.address().port, + autoSelectFamily, + lookup(_hostname, options, callback) { + completeLookup = () => + options.all ? callback(null, [{ address: "127.0.0.1", family: 4 }]) : callback(null, "127.0.0.1", 4); + onLookup(); + }, + }); + if (!alreadyAborted) { + await lookupStarted; + controller.abort(reason); + } + await closed; + + // A late successful lookup may emit "lookup", but must not resurrect a closed socket. + completeLookup?.(); + await new Promise(resolve => setImmediate(resolve)); + assert.deepEqual(events, ["error", "close"]); + assert.equal(errors.length, 1); + assert.equal(errors[0].name, "AbortError"); + assert.equal(errors[0].code, "ABORT_ERR"); + assert.equal(errors[0].cause, reason); + assert.equal(socket.destroyed, true); + assert.equal(accepted.length, 0); + } finally { + if (socket) { + socket.destroy(); + await closed; + } + for (const connection of accepted) connection.destroy(); + if (server.listening) await new Promise(resolve => server.close(resolve)); + } + console.log("abort assertions passed"); +} + +main().catch(error => { + console.error(error); + process.exitCode = 1; +}); diff --git a/test/js/node/net/node-net.test.ts b/test/js/node/net/node-net.test.ts index 89622e5814ff..c0a3e2099715 100644 --- a/test/js/node/net/node-net.test.ts +++ b/test/js/node/net/node-net.test.ts @@ -954,43 +954,40 @@ it("should not hang after destroy", async () => { } }, 120_000); -it("should trigger error when aborted even if connection failed #13126", async () => { - const signal = AbortSignal.timeout(100); - const socket = createConnection({ - host: "example.com", - port: 999, - signal: signal, - }); - const { promise, resolve, reject } = Promise.withResolvers(); - - socket.on("connect", reject); - socket.on("error", resolve); - - // Node destroys the socket with an AbortError carrying the signal's reason as `cause`. - const err = (await promise) as Error & { code?: string; cause?: Error }; - expect(err.name).toBe("AbortError"); - expect(err.code).toBe("ABORT_ERR"); - expect(err.cause?.name).toBe("TimeoutError"); -}); +describe("abort during lookup #13126", () => { + async function runFixture(exe: string, autoSelectFamily: boolean, alreadyAborted: boolean) { + await using proc = Bun.spawn({ + cmd: [ + exe, + join(import.meta.dir, "net-abort-lookup-fixture.cjs"), + String(autoSelectFamily), + String(alreadyAborted), + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: "abort assertions passed\n", + stderr: "", + exitCode: 0, + }); + } -it("should trigger error when aborted even if connection failed, and the signal is already aborted #13126", async () => { - const signal = AbortSignal.timeout(1); - await Bun.sleep(10); - const socket = createConnection({ - host: "example.com", - port: 999, - signal: signal, + describe.each(runtimesWithNode(24))("%s pending abort", (_, exe) => { + for (const autoSelectFamily of [false, true]) { + it(`autoSelectFamily=${autoSelectFamily}`, () => runFixture(exe, autoSelectFamily, false)); + } }); - const { promise, resolve, reject } = Promise.withResolvers(); - socket.on("connect", reject); - socket.on("error", resolve); - - // Node destroys the socket with an AbortError carrying the signal's reason as `cause`. - const err = (await promise) as Error & { code?: string; cause?: Error }; - expect(err.name).toBe("AbortError"); - expect(err.code).toBe("ABORT_ERR"); - expect(err.cause?.name).toBe("TimeoutError"); + // Node destroys a pre-aborted Socket during construction and only guarantees + // reconnecting after close. Bun's immediate-connect behavior is a separate contract. + describe("bun already-aborted immediate connect", () => { + for (const autoSelectFamily of [false, true]) { + it(`autoSelectFamily=${autoSelectFamily}`, () => runFixture(bunExe(), autoSelectFamily, true)); + } + }); }); // Regression test for #30697: net.connect({ localPort, lookup }) on the @@ -3649,6 +3646,54 @@ function closeWithReplacedTimers(mode, replaceBeforeImport) { }); }); } +// The raw libc listener used by blackholePortSource is unavailable on Windows and musl. +it.skipIf(isWindows || isMusl)("aborting a pending native connection emits one AbortError and closes", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + ${blackholePortSource} + const controller = new AbortController(); + const reason = new Error("pending connection aborted"); + const events = []; + const errors = []; + let attempts = 0; + const socket = new net.Socket({ signal: controller.signal }); + socket.on("connectionAttempt", () => attempts++); + socket.on("connect", () => events.push("connect")); + socket.on("error", error => { + events.push("error"); + errors.push({ name: error.name, code: error.code, causeMatches: error.cause === reason }); + }); + socket.on("close", () => { + events.push("close"); + filler.destroy(); + console.log(JSON.stringify({ attempts, events, errors, destroyed: socket.destroyed })); + }); + // Let connect() create its native pending handle before aborting it. + socket.once("connectionAttempt", () => setImmediate(() => controller.abort(reason))); + socket.connect({ host: "127.0.0.1", port }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: + JSON.stringify({ + attempts: 1, + events: ["error", "close"], + errors: [{ name: "AbortError", code: "ABORT_ERR", causeMatches: true }], + destroyed: true, + }) + "\n", + stderr: "", + exitCode: 0, + }); +}); + // There is nothing to half-close yet. shutdown() used to mark the native socket as one that had // opened, so closing it raised a connection's events ('end') and the attempt never failed. it.skipIf(isWindows || isMusl)("closing a handle that was shut down while connecting fails its attempt", async () => { From cd175f6191d04fa5438925d69a3be557b8743576 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Wed, 14 Oct 2026 09:02:06 +0800 Subject: [PATCH 2/3] fix(test): attribute isolated file teardown errors to the outgoing file Finalize file-owned handles and globals before releasing the active test owner and JUnit reporter. Mark teardown terminal without discarding callback metadata, and report uncaught cleanup errors through the existing file failure and worker IPC paths. Adapt the structured unhandled-error reporting pattern from https://github.com/oven-sh/bun/pull/44435 while preserving the existing cleanup callback policy. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com> --- CHANGELOG.md | 2 + src/runtime/cli/test/parallel/runner.rs | 15 +- src/runtime/cli/test_command.rs | 103 ++++++++++--- src/runtime/test_runner/bun_test.rs | 31 +++- test/cli/test/isolation.test.ts | 191 ++++++++++++++++++++++++ 5 files changed, 308 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af246284cf7c..218e86960f14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Count uncaught errors during isolated test-file cleanup against the outgoing file and include them in JUnit; handled cleanup callbacks continue running. Adapts the file-error reporting pattern from [oven-sh/bun#44435](https://github.com/oven-sh/bun/pull/44435); thanks @robobun! + - Drain queued Worker stdout and stderr before emitting `exit`, preserving output when a synchronous worker exit races the parent's stdio setup. - Avoid an extra tracing collection when destroying Workers while preserving native-resource finalizers and the mandatory final heap sweep. diff --git a/src/runtime/cli/test/parallel/runner.rs b/src/runtime/cli/test/parallel/runner.rs index d591d588b4e9..6e0435839307 100644 --- a/src/runtime/cli/test/parallel/runner.rs +++ b/src/runtime/cli/test/parallel/runner.rs @@ -578,6 +578,11 @@ impl<'a> WorkerLoop<'a> { let before_unhandled = self.reporter.jest.unhandled_errors_between_tests; let started_ns = bun_core::Timespec::now(bun_core::TimespecMockMode::ForceRealTime).ns(); + let isolation = if vm.test_isolation_enabled { + test_command::FileIsolation::SwapGlobal + } else { + test_command::FileIsolation::KeepGlobal + }; // A worker never knows which file is its last, so preload-level hooks wrap every file (with or without --isolate). if let Err(err) = TestCommand::run( @@ -588,17 +593,11 @@ impl<'a> WorkerLoop<'a> { first: true, last: true, }, + isolation, ) { test_command::handle_top_level_test_error_before_javascript_start(&err); } - if vm.test_isolation_enabled { - crate::jsc_hooks::stop_active_handles_for_test_isolation(vm); - vm.swap_global_for_test_isolation(); - self.reporter - .jest - .bun_test_root - .reset_hook_scope_for_test_isolation(); - } else { + if !vm.test_isolation_enabled { Global::mimalloc_cleanup(false); } self.reporter.jest.default_timeout_override = u32::MAX; diff --git a/src/runtime/cli/test_command.rs b/src/runtime/cli/test_command.rs index 05ccab10dd03..e54dc4f86404 100644 --- a/src/runtime/cli/test_command.rs +++ b/src/runtime/cli/test_command.rs @@ -374,9 +374,9 @@ impl TestFailure { /// VirtualMachine::on_print_error_zig_exception thunk. pub(crate) fn record_cb(ctx: *mut core::ffi::c_void, exception: &jsc::ZigException) { - // SAFETY: `ctx` was set to `&mut CommandLineReporter.test_failure` by - // `on_uncaught_exception` for the duration of a single - // `run_error_handler` call; single-threaded, no other borrow live. + // SAFETY: `on_uncaught_exception` points `ctx` at an Option + // for one synchronous `run_error_handler` call, then clears the callback. + // The slot is either the reporter's test failure or a local file failure. let slot = unsafe { &mut *ctx.cast::>() }; TestFailure::record(slot, exception); } @@ -1261,6 +1261,30 @@ impl CommandLineReporter { &mut self.jest.summary } + /// The uncaught hook already counted this error. Attach it to its file + /// without turning the last completed test into a failure. + pub(crate) fn handle_unhandled_file_error( + &mut self, + file_id: jest::FileId, + failure: Option, + ) { + let report = TestCaseReport { + file: junit_file_name(self.jest.files.items_source()[file_id as usize].path.text), + scopes: Vec::new(), + name: b"(unhandled error)", + status: bun_test::Execution::Result::Fail, + assertions: 0, + elapsed_ns: 0, + line_number: 0, + failure, + }; + if let Some(idx) = self.worker_ipc_file_idx { + ParallelRunner::worker_emit_test_done(idx, b"", Some(&report)); + } else if let Some(junit) = self.reporters.junit.as_mut() { + junit.record_test_case(&report).expect("oom"); + } + } + pub(crate) fn handle_test_completed( buntest: &mut bun_test::BunTest, sequence: &mut bun_test::Execution::ExecutionSequence, @@ -1730,6 +1754,12 @@ extern "C" fn BunTest__shouldGenerateCodeCoverage(test_name_str: &bun_core::Stri true } +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum FileIsolation { + KeepGlobal, + SwapGlobal, +} + pub(crate) struct TestCommand; impl TestCommand { @@ -2759,6 +2789,11 @@ impl TestCommand { first: isolate || i == 0, last: isolate, }, + if isolate { + FileIsolation::SwapGlobal + } else { + FileIsolation::KeepGlobal + }, ) { handle_top_level_test_error_before_javascript_start(&err); } @@ -2767,14 +2802,6 @@ impl TestCommand { } reporter.jest.default_timeout_override = u32::MAX; Global::mimalloc_cleanup(false); - if isolate { - crate::jsc_hooks::stop_active_handles_for_test_isolation(vm); - vm.swap_global_for_test_isolation(); - reporter - .jest - .bun_test_root - .reset_hook_scope_for_test_isolation(); - } } } @@ -2788,6 +2815,7 @@ impl TestCommand { first: isolate || files.len() == 1, last: true, }, + FileIsolation::KeepGlobal, ) { handle_top_level_test_error_before_javascript_start(&err); } @@ -2819,6 +2847,7 @@ impl TestCommand { vm: &mut VirtualMachine, file_name: &[u8], first_last: bun_test::FirstLast, + isolation: FileIsolation, ) -> crate::Result<()> { // Capture the raw log pointer (Copy) so the guard does not borrow `vm`. let vm_log = vm.log; @@ -2873,6 +2902,13 @@ impl TestCommand { let repeat_count = reporter.repeat_count; let mut repeat_index: u32 = 0; + scopeguard::defer! { + // SAFETY: the caller-owned reporter outlives the run and every + // per-repeat cleanup guard below. Teardown errors are recorded first. + if let Some(junit) = unsafe { &mut *reporter_ptr }.reporters.junit.as_mut() { + let _ = junit.end_file(None); + } + } vm.on_unhandled_rejection_ctx = None; vm.on_unhandled_rejection = jest::on_unhandled_rejection::on_unhandled_rejection; @@ -2893,13 +2929,45 @@ impl TestCommand { bun_test_root.enter_file(file_id, reporter, should_run_concurrent, first_last); let bun_test_root_ptr: *mut bun_test::BunTestRoot = bun_test_root; let global = vm.global(); + let vm_ptr: *mut VirtualMachine = vm; + // An early return also finalizes the file; successful intermediate + // reruns only release their BunTest, retaining the current global. + let finalize_file = core::cell::Cell::new(true); scopeguard::defer! { + let swap = finalize_file.get() && isolation == FileIsolation::SwapGlobal; + if swap { + // SAFETY: the root and active file remain live through this guard. + // End collection/execution before callbacks can re-enter the scheduler. + if let Some(active) = unsafe { (*bun_test_root_ptr).clone_active_file() } { + active.get().begin_file_teardown(); + } + } + // A pending patch must be forgotten before its global is retired. + bun_jsc::cpp::JSMock__forgetPendingModulePatches(global); + if swap { + // SAFETY: the caller's VM remains live on this JS thread. + // Keep the active file and reporter through the existing close + // callbacks so uncaught teardown errors belong to this file. + let vm = unsafe { &mut *vm_ptr }; + crate::jsc_hooks::stop_active_handles_for_test_isolation(vm); + vm.swap_global_for_test_isolation(); + } // SAFETY: `bun_test_root` is `&'static mut` from `Jest::runner()`; // raw-ptr escape so the closure does not hold a borrowck lock on // it for the loop body. unsafe { (*bun_test_root_ptr).exit_file(); } - // A mock.module() patch still pending must not hold up the next file. - bun_jsc::cpp::JSMock__forgetPendingModulePatches(global); + if swap { + // SAFETY: exit_file and the loop's local Rc releases precede + // dropping the old preload scopes those BunTests referenced. + unsafe { (*bun_test_root_ptr).reset_hook_scope_for_test_isolation(); } + } + // SAFETY: reporter is caller-owned and remains live through this guard. + if Output::is_github_action() + && unsafe { (*reporter_ptr).worker_ipc_file_idx.is_none() } + { + pretty_errorln!("\n::endgroup::\n"); + Output::flush(); + } } // SAFETY: `set()` reads only `reporter.{worker_ipc_file_idx, reporters}` @@ -3043,11 +3111,6 @@ impl TestCommand { let _ = vm.global().handle_rejected_promises(); - if Output::is_github_action() && reporter.worker_ipc_file_idx.is_none() { - pretty_errorln!("\n::endgroup::\n"); - Output::flush(); - } - if !vm.test_isolation_enabled { // Ensure these never linger across files. Under --isolate this // is done by swapGlobalForTestIsolation() (kill+clear) and we @@ -3057,9 +3120,7 @@ impl TestCommand { } repeat_index += 1; - } - if let Some(junit) = reporter.reporters.junit.as_mut() { - let _ = junit.end_file(None); + finalize_file.set(repeat_index == repeat_count); } Ok(()) } diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index b67a04b69cfc..e81afbc53643 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -903,6 +903,17 @@ impl BunTest { let _ = self.result_queue.write_item(result); // OOM/capacity: fire-and-forget } + /// Retain error attribution while closing a file, including one whose load + /// failed before execution. Close callbacks must not resume its tests or hooks. + pub(crate) fn begin_file_teardown(&mut self) { + debug_assert!(!self.in_run_loop); + self.phase = Phase::Done; + self.wants_wakeup = false; + if self.timer.state == EventLoopTimerState::ACTIVE { + vm_timer().remove(&raw mut self.timer); + } + } + pub(crate) fn run(this_strong: &BunTestPtr, global_this: &JSGlobalObject) -> JsResult<()> { let _g = group_begin!(); // `Collection::step` / `Execution::step` re-enter and call `.get()` on @@ -1287,17 +1298,20 @@ impl BunTest { return; // the exception should not be visible (eg m_terminationException) }; + let mut file_failure = None; + let is_file_error = self.phase == Phase::Done; let failure_ctx: *mut core::ffi::c_void = 'ctx: { - if handle_status != HandleUncaughtExceptionResult::ShowHandledError { - break 'ctx core::ptr::null_mut(); - } let Some(reporter) = self.reporter else { break 'ctx core::ptr::null_mut(); }; // SAFETY: `BunTest.reporter` carries write provenance from `enter_file`'s // `&mut`; single-threaded test runner, no other borrow live here. let reporter = unsafe { &mut *reporter.as_ptr() }; - if reporter.jest.test_options.reporters.junit { + if !reporter.jest.test_options.reporters.junit { + core::ptr::null_mut() + } else if is_file_error { + core::ptr::from_mut(&mut file_failure).cast() + } else if handle_status == HandleUncaughtExceptionResult::ShowHandledError { core::ptr::from_mut(&mut reporter.test_failure).cast() } else { core::ptr::null_mut() @@ -1343,6 +1357,14 @@ impl BunTest { } Output::flush(); + if is_file_error && !failure_ctx.is_null() { + // SAFETY: the active file retains the caller-owned reporter through + // teardown. The formatter callback was cleared before moving its slot. + unsafe { + (*self.reporter.unwrap().as_ptr()) + .handle_unhandled_file_error(self.file_id, file_failure); + } + } } } @@ -1974,4 +1996,3 @@ pub(crate) use super::execution as Execution; pub(crate) use super::debug; pub(crate) use super::scope_functions as ScopeFunctions; pub(crate) use super::order as Order; - diff --git a/test/cli/test/isolation.test.ts b/test/cli/test/isolation.test.ts index 38999d3e92bc..64da6e27dd8b 100644 --- a/test/cli/test/isolation.test.ts +++ b/test/cli/test/isolation.test.ts @@ -73,6 +73,197 @@ describe.concurrent("bun test --isolate", () => { expect(exitCode).toBe(0); }); + describe.each([ + { + label: "serial non-last file", + mode: "--isolate", + owner: "a-owner.test.ts", + observer: "b-observer.test.ts", + scale: "0", + }, + { + label: "parallel file", + mode: "--parallel=2", + owner: "a-owner.test.ts", + observer: "b-observer.test.ts", + scale: "0", + }, + { + label: "last file on a worker", + mode: "--parallel=2", + owner: "z-owner.test.ts", + observer: "a-observer.test.ts", + scale: "60000", + }, + ])("file teardown errors: $label", ({ label, mode, owner, observer, scale }) => { + test.each(["throw", "handled"] as const)("%s during cleanup retains file ownership", async outcome => { + using dir = tempDir("isolate-teardown-error", { + [owner]: ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + import { Socket } from "node:net"; + const record = message => appendFileSync("events.txt", message + "\\n"); + const worker = process.env.BUN_TEST_WORKER_ID ?? "serial"; + test("owner passes", async () => { + record("start owner " + worker); + const { promise: accepted, resolve } = Promise.withResolvers(); + const listener = Bun.listen({ + hostname: "127.0.0.1", port: 0, + socket: { + open() { resolve(); }, + data() {}, + close() { + record("close owner " + worker); + const reason = new Error("file teardown sentinel"); + ${ + outcome === "throw" + ? "throw reason;" + : ` + const socket = new Socket(); + socket.on("error", error => record("handled socket " + (error === reason))); + socket.destroy(reason); + Promise.reject(reason).catch(error => record("caught rejection " + (error === reason))); + ` + } + }, + }, + }); + await Bun.connect({ hostname: "127.0.0.1", port: listener.port, socket: { data() {} } }); + await accepted; + }); + `, + [observer]: ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + test("observer passes", () => { + appendFileSync("events.txt", "start observer " + (process.env.BUN_TEST_WORKER_ID ?? "serial") + "\\n"); + }); + `, + }); + const { stdout, stderr, exitCode } = await runTests( + String(dir), + [mode, "--reporter=junit", "--reporter-outfile=out.xml"], + [`./${owner}`, `./${observer}`], + { ...bunEnv, BUN_TEST_PARALLEL_SCALE_MS: scale }, + ); + const events = fs + .readFileSync(join(String(dir), "events.txt"), "utf8") + .trim() + .split("\n"); + expect(events.filter(event => event.startsWith("close owner "))).toHaveLength(1); + expect(stderr).toContain("2 pass"); + expect(stderr).toContain("0 fail"); + if (mode.startsWith("--parallel")) { + expect(stdout).toContain("PARALLEL"); + expect(events.find(event => event.startsWith("start owner "))).toMatch(/^start owner \d+$/); + } + if (label === "last file on a worker") { + const starts = events.filter(event => event.startsWith("start ")).map(event => event.split(" ")); + expect(starts.map(([, name]) => name)).toEqual(["observer", "owner"]); + expect(starts[0][2]).toBe(starts[1][2]); + } + const report = await require("xml2js").parseStringPromise(fs.readFileSync(join(String(dir), "out.xml"), "utf8"), { + strict: true, + }); + const ownerSuite = report.testsuites.testsuite.find(suite => suite.$.file === owner); + const observerSuite = report.testsuites.testsuite.find(suite => suite.$.file === observer); + const failures = ownerSuite.testcase.filter(testcase => testcase.failure); + expect(observerSuite.$.failures).toBe("0"); + expect(ownerSuite.testcase.find(testcase => testcase.$.name === "owner passes").failure).toBeUndefined(); + if (outcome === "throw") { + expect(exitCode).toBe(1); + expect(stderr).toContain("1 error"); + expect(ownerSuite.$.failures).toBe("1"); + expect(failures).toHaveLength(1); + expect(failures[0].$).toMatchObject({ name: "(unhandled error)", file: owner, assertions: "0" }); + expect(failures[0].failure[0].$).toEqual({ type: "Error", message: "file teardown sentinel" }); + expect(failures[0].failure[0]._).toContain(owner); + } else { + expect(exitCode).toBe(0); + expect(stderr).not.toContain("Unhandled error"); + expect(ownerSuite.$.failures).toBe("0"); + expect(failures).toEqual([]); + expect(events.filter(event => event === "handled socket true")).toHaveLength(1); + expect(events.filter(event => event === "caught rejection true")).toHaveLength(1); + } + }); + }); + + test.each(["--isolate", "--parallel=2"])("%s does not resume a failed file during teardown", async mode => { + using dir = tempDir("isolate-failed-load-teardown", { + "preload.ts": ` + import { beforeAll, afterAll } from "bun:test"; + import { appendFileSync } from "node:fs"; + beforeAll(() => { + if (globalThis.pendingOwnerHook) { + appendFileSync("events.txt", "beforeAll owner\\n"); + return globalThis.pendingOwnerHook.promise; + } + }); + afterAll(() => appendFileSync("events.txt", "afterAll " + (globalThis.pendingOwnerHook ? "owner" : "observer") + "\\n")); + `, + "a-owner.test.ts": ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + const record = message => appendFileSync("events.txt", message + "\\n"); + globalThis.pendingOwnerHook = Promise.withResolvers(); + test("must not run", () => record("unexpected test")); + const { promise: accepted, resolve } = Promise.withResolvers(); + const listener = Bun.listen({ + hostname: "127.0.0.1", port: 0, + socket: { + open() { resolve(); }, + data() {}, + close() { + record("close owner"); + globalThis.pendingOwnerHook.resolve(); + throw new Error("failed-load teardown sentinel"); + }, + }, + }); + await Bun.connect({ hostname: "127.0.0.1", port: listener.port, socket: { data() {} } }); + await accepted; + throw new Error("module load sentinel"); + `, + "b-observer.test.ts": ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + test("observer passes", () => appendFileSync("events.txt", "observer\\n")); + `, + }); + const { stdout, stderr, exitCode } = await runTests( + String(dir), + [mode, "--preload=./preload.ts", "--reporter=junit", "--reporter-outfile=out.xml"], + ["./a-owner.test.ts", "./b-observer.test.ts"], + { ...bunEnv, BUN_TEST_PARALLEL_SCALE_MS: "0" }, + ); + expect(exitCode).toBe(1); + if (mode.startsWith("--parallel")) expect(stdout).toContain("PARALLEL"); + expect(stderr).toContain("module load sentinel"); + expect(stderr).toContain("failed-load teardown sentinel"); + const events = fs + .readFileSync(join(String(dir), "events.txt"), "utf8") + .trim() + .split("\n"); + expect(events.filter(event => event === "beforeAll owner")).toHaveLength(1); + expect(events.filter(event => event === "close owner")).toHaveLength(1); + expect(events.indexOf("beforeAll owner")).toBeLessThan(events.indexOf("close owner")); + expect(events.filter(event => event === "observer")).toHaveLength(1); + expect(events.filter(event => event === "afterAll observer")).toHaveLength(1); + expect(events).not.toContain("afterAll owner"); + expect(events).not.toContain("unexpected test"); + const report = await require("xml2js").parseStringPromise(fs.readFileSync(join(String(dir), "out.xml"), "utf8"), { + strict: true, + }); + const ownerSuite = report.testsuites.testsuite.find(suite => suite.$.file === "a-owner.test.ts"); + const observerSuite = report.testsuites.testsuite.find(suite => suite.$.file === "b-observer.test.ts"); + expect(observerSuite.$.failures).toBe("0"); + expect(ownerSuite.$.failures).toBe("1"); + expect(ownerSuite.testcase).toHaveLength(1); + expect(ownerSuite.testcase[0].$).toMatchObject({ name: "(unhandled error)", file: "a-owner.test.ts" }); + expect(ownerSuite.testcase[0].failure[0].$.message).toBe("failed-load teardown sentinel"); + }); + test.each(["--isolate", "--parallel=2"])("%s releases each file's stdio polls and preserves output", async mode => { const names = ["a", "b", "c", "d"].map(c => `${c}-stdio.test.ts`); using dir = tempDir( From 42c73e9090d6a0c6f511904d360c43df4c9f1909 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Wed, 14 Oct 2026 10:04:57 +0800 Subject: [PATCH 3/3] fix(test): place reporter safety comment next to unsafe access --- src/runtime/cli/test_command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/cli/test_command.rs b/src/runtime/cli/test_command.rs index e54dc4f86404..64fc2519f210 100644 --- a/src/runtime/cli/test_command.rs +++ b/src/runtime/cli/test_command.rs @@ -2961,8 +2961,8 @@ impl TestCommand { // dropping the old preload scopes those BunTests referenced. unsafe { (*bun_test_root_ptr).reset_hook_scope_for_test_isolation(); } } - // SAFETY: reporter is caller-owned and remains live through this guard. if Output::is_github_action() + // SAFETY: reporter is caller-owned and remains live through this guard. && unsafe { (*reporter_ptr).worker_ipc_file_idx.is_none() } { pretty_errorln!("\n::endgroup::\n");