bun test: exit through the VM teardown path when --bail stops a run

Closed
RO
19d ago
+245-43

Fixes #32183

Problem

  • bun test --bail on a file whose test fails exits 134 under the environment the sanitizer lane gives every child (BUN_DESTRUCT_VM_ON_EXIT=1, ASAN_OPTIONS=...:detect_leaks=1:abort_on_error=1, LSAN_OPTIONS=suppressions=test/leaksan.supp), instead of 1. stderr ends with ERROR: LeakSanitizer: detected memory leaks / SUMMARY: AddressSanitizer: 796 byte(s) leaked in 8 allocation(s): the Expect box from Expect::to_js, the file's ScopeFunctions boxes, a RefData from Expect::call, and a SavedSourceMap::get_with_content entry.
  • Cause: the bail taken from CommandLineReporter::handle_test_completed (src/runtime/cli/test_command.rs:1519) was a bare Global::exit(1). Under ASAN that is libc exit(), so LSan's atexit scan runs, but VirtualMachine::on_exit() / global_exit() never do, and the BUN_DESTRUCT_VM_ON_EXIT teardown that frees those objects (destructOnExit -> ~VM -> lastChanceToFinalize, plus the VM's source map table) is skipped. Every other exit bun test takes once tests have run (end of run in exec, --parallel workers, and the other bail site, the failed module evaluation in TestCommand::run) already goes through global_exit() and is clean under the same environment.
  • Without --bail the same failing file exits 1 with no report. Existing bail tests did not notice because they assert not.toBe(0) or only look at stderr, or are listed in test/no-validate-leaksan.txt. Under the lane's environment the leak report also takes several seconds to symbolize, which is what put test/regression/issue/12250.test.ts on that list (its bail child hits the 5s test timeout).

Fix

  • Both bail sites now call one CommandLineReporter::bail_out(vm) -> !. It prints the summary and bail message and writes the junit/timings files exactly as before, then exits the same way exec's tail does: exit_code = 1, skip_exit_listeners from the same gate (#38442), on_exit(), bun_test_root.deinit_for_exit(), RUNNER cleared, global_exit(). The module-evaluation bail site loses its hand-rolled copy of that sequence.
  • Why exiting from inside the test-completion callback is fine: process.exit() called inside a test reaches on_exit() + global_exit() from exactly this state (runner published, file mid-execution, JS frames possibly on the stack), and that is exercised on the sanitizer lane today. destroyVM clears vm.entryScope and drops the outstanding JSLockHolder refs for that reason. The file's BunTest stays allocated because TestCommand::run still holds its strong ref; deinit_for_exit() only drops the root's ref, and the test-runner finalizers that run during teardown (Expect, DoneCallback, ScopeFunctions) release a RefData/drop a box and do not touch RUNNER. So no unwinding of the step loops is needed; the bail still stops synchronously at the same point, and the output is byte-identical to before (checked both sites against an unfixed build).
  • Visible differences outside leak checking: the bail exit now runs the end-of-run on_exit() (exit listeners stay skipped for bun:test files, and run once a node:test API was used, as at the end of a normal run), and global_exit() checkpoints and closes the test's sqlite databases like a normal exit does.
  • Verified with a debug (ASAN) build:
    • test/cli/test/bun-test.test.ts: new --bail > exits cleanly under LeakSanitizer cases (sync failure, failure delivered from a promise reaction with a timer pending, a later file failing with preload hooks registered, a file failing to load) spawn bun test --bail with the lane's environment and assert exit 1, no signal, no LeakSanitizer output, and that nothing after the failure ran; the two existing bail tests now assert exit code 1; new process.on('exit') cases check listeners stay skipped for a bun:test file on bail and run for a node:test file on bail. Without the src change the three in-test leak cases and the node:test listener case fail; the whole file passes with it (95 pass, 6 todo).
    • Also in bun-test.test.ts: a failing run exits 1 cleanly under LeakSanitizer (serial and --parallel=2, carried over from #32187) pins down the non-bail exit paths, which are already clean on main; both cases report the leak if the teardown is skipped (checked by running them with BUN_DESTRUCT_VM_ON_EXIT removed), and the --parallel case relies on the inherited worker stderr since a worker abort does not change the coordinator's exit code.
    • test/regression/issue/12250.test.ts removed from test/no-validate-leaksan.txt; it passes under the lane's environment with this change and times out without it.
    • test/js/bun/test/test-test.test.ts, test/regression/issue/12250.test.ts, test/regression/issue/26851.test.ts, the --parallel --bail tests in test/cli/test/parallel.test.ts pass.
    • Under the lane's environment, bails reached from a test timeout, a throwing beforeEach, done(err), concurrent tests with a 30s sibling and a 60s timer pending, an open bun:sqlite database, --isolate, --rerun-each=3, --bail=2 across files and --watch all exit 1 promptly with no report.
    • This commit cherry-picked onto current main (5448c1e603): the #32183 repro and the leak-check cases from #32180 and #32187, run with the lane's environment, all pass (green serial/--isolate/--parallel, failing serial/--parallel, --bail mid-file, --bail on a module evaluation failure); on main itself only the --bail mid-file case fails.
  • Superseded PRs: #32187 fixed the same bail exit with a bailed flag that unwinds the run loops, plus a SavedSourceMap::clear() for the source map entry a failing run caches; it was stacked on #32180. Both were written against a June main where bun test's exit did not tear the VM down even with BUN_DESTRUCT_VM_ON_EXIT=1 (destructOnExit released only two VM refs, so ~VM never ran under exec's API lock; fixed by #34346) and did not run on_exit() (added by #34444, extended to --parallel workers by #38442). On current main every bun test exit that goes through global_exit() frees those objects and the source map table, so the only remaining case under the lane's environment is the bare exit this PR replaces. #32187 and #32180 are closed in favor of this PR; their extra scenario, leak checking without BUN_DESTRUCT_VM_ON_EXIT, is not one the lane uses (a passing run is not clean under it either).
  • Other open bail PRs: #35504, #35094, #38265 and #38974 change other things about bail and keep the hard exit; #38974 works around this bug by passing detect_leaks=0 to its bail children, which becomes unnecessary with this change. They are independent of this PR apart from textual conflicts at the same lines.

Background

  • BUN_DESTRUCT_VM_ON_EXIT: opt-in (set by scripts/runner.node.mjs on the ASAN lane) that makes VirtualMachine::global_exit() tear the JSC VM down before exiting instead of just calling exit(). Destroying the VM runs lastChanceToFinalize, which runs every live cell's finalizer; that is how the native boxes behind bun:test wrapper objects (Expect, ScopeFunctions, ...) get freed. Without it a bare exit is the normal, fast path, and leak checking is not meaningful in any bun test exit (a passing run also reports these objects).
  • LeakSanitizer reports a malloc'd block as leaked when no pointer to it is found in its roots (stacks, globals, other malloc'd blocks). Pointers held only inside JSC heap cells are invisible to it, so anything owned solely by a JS wrapper is reported unless its finalizer ran. The test runner's own state (CommandLineReporter, BunTest) is reachable from exec's stack frame and is not reported either way.
  • on_exit() is the end-of-process sequence (profilers, process.on('exit') dispatch gated by exit_handler.skip_exit_listeners, napi cleanup hooks); it also sets is_shutting_down, which global_exit() asserts. bun test runs it at the end of every run and in --parallel workers since #38442.
  • BunTestRoot::deinit_for_exit() drops the active file's root reference, the preload hook scope and the orphaned pending_then_refs; exec runs it and clears Jest::RUNNER right before global_exit() so nothing that runs during teardown can see a half-torn-down runner. Its doc comment already describes the bail path as a caller.

no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts