From 6f1f108f453e92f034c3e69881d251536f4bd5cf Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 16:27:54 +0000 Subject: [PATCH] bun test: exit 1 cleanly on failing runs under LeakSanitizer Failing runs aborted (exit 134) instead of exiting 1 when ASAN_OPTIONS included detect_leaks=1:abort_on_error=1 and BUN_DESTRUCT_VM_ON_EXIT was unset, for three reasons: - --bail called Global::exit(1) from the reporter callback with the active file mid-execution, skipping all teardown. The bail sites now set TestRunner.bailed instead; the execution stepper stops starting new entries, the run loops unwind without ticking the event loop further, and TestCommand::exec exits 1 through the same teardown sequence as the normal tail (deinit_for_exit, RUNNER clear, pre-exit collection, global_exit). - The module-evaluation-failure bail branch tore down inline but exited without the pre-exit collection; it now unwinds through the same path. - Printing a failure remaps the stack trace, which caches a ParsedSourceMap Arc in the saved-source-map table. The table stores entries as tagged pointers LSan cannot recognize, so the cached entry was reported as a leak even though it is live VM state. collect_for_leak_check_at_exit() now clears the table, covering serial runs, both bail paths, and parallel workers. Bail behavior is preserved: the bail message prints at the same point, nothing runs after it, remaining files and repeats are skipped, and the junit report is still written. Fixes #32183 --- src/jsc/SavedSourceMap.rs | 22 +++++ src/jsc/VirtualMachine.rs | 6 ++ src/runtime/cli/test/parallel/runner.rs | 9 +- src/runtime/cli/test_command.rs | 75 +++++++++++----- src/runtime/test_runner/Execution.rs | 14 +++ src/runtime/test_runner/bun_test.rs | 9 ++ src/runtime/test_runner/jest.rs | 7 ++ test/cli/test/isolation.test.ts | 109 +++++++++++++++++++++--- 8 files changed, 212 insertions(+), 39 deletions(-) diff --git a/src/jsc/SavedSourceMap.rs b/src/jsc/SavedSourceMap.rs index 059a22747864..01703be43140 100644 --- a/src/jsc/SavedSourceMap.rs +++ b/src/jsc/SavedSourceMap.rs @@ -145,6 +145,28 @@ impl SavedSourceMap { unsafe { bun_core::heap::destroy(provider) }; } } + + /// Releases every cached entry, leaving the table empty but valid (unlike + /// `Drop`, which tears the table down for good). Used by the pre-exit + /// leak-check collection: the table stores its entries as tagged pointers + /// (`Value`), which LeakSanitizer does not recognize as pointers, so any + /// entry still cached at exit is reported as a leak even though it is + /// live VM state. + pub fn clear(&mut self) { + self.lock(); + // Mirror `put_value`: the caches may point at blobs freed below. + self.find_cache.invalidate_all(); + self.last_ism = None; + let map = self.map_mut(); + for val in map.values() { + let value = Value::from(Some(*val)); + // SAFETY: values were stored by us and are live until released + // here; `clear()` below removes them so they are not released again. + unsafe { Self::release_value(value) }; + } + map.clear(); + self.unlock(); + } } /// Thin forwarder to the leaf-crate state in diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 074e37d7d342..e53a1549f9d5 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -1525,6 +1525,12 @@ impl VirtualMachine { // during `destructOnExit()`'s collection after `on_exit()` ran. self.has_run_cleanup_hooks = true; let _ = self.garbage_collect(true); + // Remapping a stack trace (printing a test failure does this) caches + // a `ParsedSourceMap` Arc in the saved-source-map table, which stores + // entries as tagged pointers LSan cannot see through, so any cached + // entry is reported as a leak. Nothing remaps after this point on + // these exit paths; the table stays valid (empty) either way. + self.source_mappings.clear(); } pub fn global_exit(&mut self) -> ! { diff --git a/src/runtime/cli/test/parallel/runner.rs b/src/runtime/cli/test/parallel/runner.rs index dff17cf24fd7..fb502d023e2a 100644 --- a/src/runtime/cli/test/parallel/runner.rs +++ b/src/runtime/cli/test/parallel/runner.rs @@ -352,10 +352,11 @@ fn build_worker_argv( if let Some(seed) = opts.seed { argv.push(print_z(format_args!("--seed={}", seed))?); } - // --bail is intentionally NOT forwarded: workers Global.exit(1) on bail - // (see test_command.rs handle_test_completed), which the coordinator would - // misread as a crash. Cross-worker bail is handled at file granularity by - // the coordinator instead. + // --bail is intentionally NOT forwarded: an in-worker bail (see + // test_command.rs handle_test_completed) would stop the worker mid + // file-list, which the coordinator would misread as a crash. + // Cross-worker bail is handled at file granularity by the coordinator + // instead, and `jest.bailed` is never set in a worker (bail stays 0). if opts.repeat_count > 0 { argv.push(print_z(format_args!("--rerun-each={}", opts.repeat_count))?); } diff --git a/src/runtime/cli/test_command.rs b/src/runtime/cli/test_command.rs index bdd295b5cbd9..bf7fa215fb81 100644 --- a/src/runtime/cli/test_command.rs +++ b/src/runtime/cli/test_command.rs @@ -1380,7 +1380,14 @@ impl CommandLineReporter { ); Output::flush(); this.write_junit_report_if_needed(); - Global::exit(1); + // Don't `Global::exit(1)` here: the active file is + // mid-execution (`BunTest::run` frames and the run loop's + // strong ref are live), so exiting now would skip all + // teardown. Setting `bailed` stops the stepper from + // starting new entries and unwinds every run loop back to + // `TestCommand::exec`, which exits 1 through the shared + // teardown path. + this.jest.bailed = true; } } } @@ -2085,6 +2092,7 @@ impl TestCommand { run_todo: ctx.test_options.run_todo, only: ctx.test_options.only, bail: ctx.test_options.bail, + bailed: false, max_concurrency: ctx.test_options.max_concurrency, // `test_filter_regex` is an erased `*mut RegularExpression` (see // options_types::context); cast back to a typed `NonNull` — @@ -2597,6 +2605,30 @@ impl TestCommand { } } + // --bail hit its failure threshold: the bail message and junit report + // were already written at the bail site, the run loops unwound without + // running further tests, and everything after this point in the normal + // tail (snapshot writes, coverage, summary) is intentionally skipped. + // Exit 1 through the same teardown sequence as the normal tail below + // so the exit is clean under ASAN leak checking. + if reporter.jest.bailed { + vm.exit_handler.exit_code = 1; + vm.is_shutting_down = true; + reporter.jest.bun_test_root.deinit_for_exit(); + // SAFETY: `RUNNER` is a `RacyCell` touched only from the single JS + // thread; no concurrent reader exists on this shutdown path. + unsafe { + jest::Jest::RUNNER.write(None); + } + drop(reporter); + vm.collect_for_leak_check_at_exit(); + let vm_ptr: *mut VirtualMachine = vm; + // SAFETY: `vm_ptr` reborrows the live `&mut VirtualMachine`; + // `run_with_api_lock` takes `&self` only and `global_exit()` + // diverges, so the closure is the sole mutator. + vm.run_with_api_lock(|| unsafe { (*vm_ptr).global_exit() }); + } + // With --changed, only a subset of test files (possibly none) runs, // so the module loader won't naturally add every source file to the // watcher. Seed it from the module graph so editing any local source @@ -3008,6 +3040,9 @@ impl TestCommand { ) { handle_top_level_test_error_before_javascript_start(err); } + if reporter.jest.bailed { + return; + } reporter.jest.default_timeout_override = u32::MAX; Global::mimalloc_cleanup(false); if isolate { @@ -3184,25 +3219,11 @@ impl TestCommand { if reporter.jest.bail == 1 { "" } else { "s" } ); reporter.write_junit_report_if_needed(); - - vm.exit_handler.exit_code = 1; - vm.is_shutting_down = true; - // `global_exit()` diverges, so the `exit_file()` defer - // above never fires. Release the active file's - // `Strong`s and the preload-hook scope here so - // `destructOnExit()`'s `collectNow()` can reclaim them, - // then clear `RUNNER` so finalizers can't observe a - // partially-torn-down `TestRunner`. - // SAFETY: single-threaded; raw-ptr reborrow mirrors the - // defer's escape. - unsafe { - (*bun_test_root_ptr).deinit_for_exit(); - jest::Jest::RUNNER.write(None); - } - let vm_ptr = std::ptr::from_mut::(vm); - // SAFETY: global_exit diverges; `vm_ptr` is a fresh - // raw-ptr reborrow of the exclusive `vm` borrow. - unsafe { (*vm_ptr).run_with_api_lock(|| (&mut *vm_ptr).global_exit()) }; + // Unwind to `TestCommand::exec`'s bail exit: the + // `exit_file()` defer above releases the active file's + // `Strong`s on the way out, and exec tears down the + // rest (`RUNNER`, pre-exit collection) before exiting 1. + reporter.jest.bailed = true; } return Ok(()); @@ -3230,10 +3251,12 @@ impl TestCommand { bun_test::BunTest::run(&buntest_strong, vm.global())?; // Process event loop while bun_test tests are running - vm.event_loop_ref().tick(); + if !reporter.jest.bailed { + vm.event_loop_ref().tick(); + } let mut prev_unhandled_count = vm.unhandled_error_counter; - while buntest.phase != bun_test::Phase::Done { + while !reporter.jest.bailed && buntest.phase != bun_test::Phase::Done { if buntest.wants_wakeup { buntest.wants_wakeup = false; vm.wakeup(); @@ -3250,6 +3273,14 @@ impl TestCommand { } } + if reporter.jest.bailed { + // Bail message already printed. Stop ticking the event + // loop so nothing else runs or prints, and unwind to + // `TestCommand::exec`'s bail exit (the defers above + // release the active file on the way out). + return Ok(()); + } + 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) }; diff --git a/src/runtime/test_runner/Execution.rs b/src/runtime/test_runner/Execution.rs index a803a5a1fc85..271b2aaf965b 100644 --- a/src/runtime/test_runner/Execution.rs +++ b/src/runtime/test_runner/Execution.rs @@ -957,6 +957,20 @@ fn step_sequence_one( ) -> JsResult> { let _g = group_begin!(); let buntest = buntest_strong.get(); + + // Once --bail has hit its threshold, start nothing further — no test + // callbacks, no remaining hooks. Reporting Done lets the group loops + // drain without executing anything so the run loop can unwind to the + // bail exit in `TestCommand::exec`. + if let Some(reporter) = buntest.reporter { + // SAFETY: the reporter outlives every BunTest (owned by + // `test_command::exec`); `exit_file()` nulls this field before the + // file is dropped. + if unsafe { reporter.as_ref() }.jest.bailed { + return Ok(Some(AdvanceSequenceStatus::Done)); + } + } + let buntest_ptr = NonNull::from(&mut *buntest); let this = &mut buntest.execution; diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index a59d41f484d2..5fa91136bcd9 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -953,6 +953,15 @@ impl BunTest { // SAFETY: see block-SAFETY above. `step()` may call `.get()` internally; // no outer `&mut` overlaps because we only touch `*this` between calls. while let Some(result) = unsafe { (*this).result_queue.read_item() } { + // Once --bail has hit its threshold, discard queued results + // instead of stepping them: stepping a queued completion would + // report another test result after the bail message. The drive + // loop in `TestCommand::run` unwinds to the bail exit. + // SAFETY: short-lived reborrow; the reporter outlives every + // BunTest (owned by `test_command::exec`). + if unsafe { (*this).reporter.is_some_and(|r| r.as_ref().jest.bailed) } { + break; + } global_this.clear_termination_exception(); // SAFETY: `UnsafeCell`-derived `*mut`; short-lived field read between re-entrant calls. let step_result: StepResult = match unsafe { (*this).phase } { diff --git a/src/runtime/test_runner/jest.rs b/src/runtime/test_runner/jest.rs index a2a3f8ffdeb8..31dd8fa1929d 100644 --- a/src/runtime/test_runner/jest.rs +++ b/src/runtime/test_runner/jest.rs @@ -127,6 +127,13 @@ pub struct TestRunner<'a> { pub concurrent_test_glob: Option<&'a [&'a [u8]]>, pub last_file: u64, pub bail: u32, + /// Set when `--bail`'s failure threshold is reached (after the bail + /// message has been printed). Execution stops starting new entries, the + /// run loops unwind without ticking the event loop further, and + /// `TestCommand::exec` exits 1 through the shared teardown path instead + /// of `Global::exit(1)` mid-run (which would skip the pre-exit + /// collection that keeps ASAN leak-check exits clean). + pub bailed: bool, pub max_concurrency: u32, pub drainer: jsc::AnyTask::AnyTask, diff --git a/test/cli/test/isolation.test.ts b/test/cli/test/isolation.test.ts index 1415366a07dc..c3b6c38e543b 100644 --- a/test/cli/test/isolation.test.ts +++ b/test/cli/test/isolation.test.ts @@ -870,6 +870,19 @@ test.concurrent("--isolate: require(esm) caches a BunTranspiledModule SourceProv } }); +// LeakSanitizer only exists in ASAN builds, and `detect_leaks=1` is a +// startup error on platforms without LSan (macOS arm64); the CI lane +// that leak-checks is linux x64-asan. +const leakCheckEnv = { + ...bunEnv, + // The CI runner sets this for outer test processes; the exit path + // must be leak-clean without the full destruct-on-exit teardown too. + BUN_DESTRUCT_VM_ON_EXIT: undefined, + BUN_TEST_PARALLEL_SCALE_MS: "0", + ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=1:abort_on_error=1", + LSAN_OPTIONS: `malloc_context_size=30:print_suppressions=0:suppressions=${join(import.meta.dir, "..", "..", "leaksan.supp")}`, +}; + // At exit, the final file's `expect()` wrapper boxes (and the `RefData` / // `BunTestCell` they pin) are freed only by GC finalizers, and no collection // used to run between the last test and `exit()`. With LeakSanitizer active @@ -886,19 +899,6 @@ describe.concurrent("exit is leak-clean under LeakSanitizer", () => { `; } - // LeakSanitizer only exists in ASAN builds, and `detect_leaks=1` is a - // startup error on platforms without LSan (macOS arm64); the CI lane - // that leak-checks is linux x64-asan. - const leakCheckEnv = { - ...bunEnv, - // The CI runner sets this for outer test processes; the exit path - // must be leak-clean without the full destruct-on-exit teardown too. - BUN_DESTRUCT_VM_ON_EXIT: undefined, - BUN_TEST_PARALLEL_SCALE_MS: "0", - ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=1:abort_on_error=1", - LSAN_OPTIONS: `malloc_context_size=30:print_suppressions=0:suppressions=${join(import.meta.dir, "..", "..", "leaksan.supp")}`, - }; - test.skipIf(!isASAN || !isLinux).each([ ["serial", []], ["--isolate", ["--isolate"]], @@ -922,3 +922,86 @@ describe.concurrent("exit is leak-clean under LeakSanitizer", () => { expect(exitCode).toBe(0); }); }); + +// https://github.com/oven-sh/bun/issues/32183 +// Failing runs must exit 1 (not SIGABRT from LeakSanitizer's exit scan): +// printing a failure caches a ParsedSourceMap in the saved-source-map table, +// and --bail used to Global::exit(1) mid-run with no teardown at all. +describe.concurrent("failing exits are leak-clean under LeakSanitizer", () => { + const runLeakChecked = async (args: string[], files: Record) => { + using dir = tempDir("test-fail-exit-lsan", files); + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", ...args, "."], + env: leakCheckEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + }; + + test.skipIf(!isASAN || !isLinux)("failing test, serial", async () => { + const { stderr, exitCode } = await runLeakChecked([], { + "a.test.js": ` + import { test, expect } from "bun:test"; + test("fails", () => { expect(1 + 1).toBe(3); }); + `, + }); + expect(stderr).not.toContain("LeakSanitizer"); + expect(stderr).toContain("1 fail"); + expect(exitCode).toBe(1); + }); + + test.skipIf(!isASAN || !isLinux)("failing test, --parallel", async () => { + // The failure prints (and caches a source map) inside the worker, whose + // abort would not change the coordinator's exit code, so the stderr + // assertion is the load-bearing one. + const { stderr, exitCode } = await runLeakChecked(["--parallel=2"], { + "a.test.js": ` + import { test, expect } from "bun:test"; + test("fails", () => { expect(1 + 1).toBe(3); }); + `, + "b.test.js": ` + import { test, expect } from "bun:test"; + test("passes", () => { expect(1 + 1).toBe(2); }); + `, + }); + expect(stderr).not.toContain("LeakSanitizer"); + expect(stderr).toContain("1 fail"); + expect(exitCode).toBe(1); + }); + + test.skipIf(!isASAN || !isLinux)("--bail mid-file", async () => { + const { stdout, stderr, exitCode } = await runLeakChecked(["--bail"], { + "a.test.js": ` + import { test, expect } from "bun:test"; + test("fails", () => { expect(1 + 1).toBe(3); }); + test("after bail", () => { console.log("MARKER_AFTER_BAIL"); }); + `, + "b.test.js": ` + import { test, expect } from "bun:test"; + test("also fails", () => { expect(1 + 1).toBe(3); }); + `, + }); + expect(stderr).not.toContain("LeakSanitizer"); + expect(stderr).toContain("Bailed out after 1 failure"); + // Nothing may run after the bail: not the rest of the bailing file... + expect(stdout).not.toContain("MARKER_AFTER_BAIL"); + // ...and not the other file (whichever file ran first, it failed). + expect(stderr).toContain("Ran 1 test across 1 file."); + expect(exitCode).toBe(1); + }); + + test.skipIf(!isASAN || !isLinux)("--bail on module evaluation failure", async () => { + const { stderr, exitCode } = await runLeakChecked(["--bail"], { + "a.test.js": ` + import { expect } from "bun:test"; + expect(1 + 1).toBe(3); + `, + }); + expect(stderr).not.toContain("LeakSanitizer"); + expect(stderr).toContain("Bailed out after 1 failure"); + expect(exitCode).toBe(1); + }); +});