bun test: exit 1 cleanly on failing runs under LeakSanitizer#32187
bun test: exit 1 cleanly on failing runs under LeakSanitizer#32187robobun wants to merge 1 commit into
Conversation
|
Updated 9:58 PM PT - Jun 20th, 2026
❌ @robobun, your commit 6f1f108 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32187That installs a local version of the PR into your bun-32187 --bun |
There was a problem hiding this comment.
I didn't find any bugs, but this restructures --bail from a hard Global::exit(1) into a flag-and-unwind across the test runner's run loops, plus a new SavedSourceMap::clear() on the exit path — worth a human pass to confirm the unwind fully preserves bail semantics in all the paths it now threads through.
Extended reasoning...
Overview
This PR changes how bun test --bail exits so that failing runs are leak-clean under LeakSanitizer. It touches 7 Rust source files plus tests:
src/jsc/SavedSourceMap.rs: newclear()that releases all cached tagged-pointer entries in place.src/jsc/VirtualMachine.rs:collect_for_leak_check_at_exit()now callssource_mappings.clear().src/runtime/test_runner/jest.rs: newTestRunner.bailedflag.src/runtime/cli/test_command.rs: both bail sites (handle_test_completedand the module-eval-failure branch inrun) now setbailed = trueinstead of callingGlobal::exit(1)/ inline teardown;execgains a centralized bail-exit block; the drive loop andrun_all_testscheck the flag to stop and unwind.src/runtime/test_runner/Execution.rs:step_sequence_oneearly-returnsDoneoncebailedis set.src/runtime/test_runner/bun_test.rs:BunTest::rundiscards queued results oncebailedis set.src/runtime/cli/test/parallel/runner.rs: comment-only update.test/cli/test/isolation.test.ts: hoistsleakCheckEnvand adds 4 new LSan-gated tests for failing/bail exits.
Security risks
None. This is internal test-runner exit-path plumbing and a source-map cache teardown helper; no auth, network, parsing of untrusted input, or permission surfaces are touched.
Level of scrutiny
Moderate-to-high. The core change replaces an immediate exit(1) mid-execution with a cooperative unwind that must be observed at every layer (step_sequence_one, BunTest::run's result drain, the tick()/drive loop in TestCommand::run, run_all_tests, and finally TestCommand::exec). Any layer that misses the flag could run extra tests/hooks, print after the bail message, or hang on a still-ticking event loop. The new bail-exit block in exec also re-implements the teardown sequence with raw-pointer reborrows of vm inside run_with_api_lock. The PR description is thorough and lists the existing bail test files that were re-run, but this is exactly the kind of control-flow refactor where a second pair of eyes on the unwind ordering and the SavedSourceMap::clear() lock/iteration is worthwhile.
Other factors
- Stacked on #32180 (base branch
farm/1b909a84/test-exit-lsan-leak); needs to land after that and be rebased. - The new tests are gated on
isASAN && isLinux, so most CI lanes won't exercise them; the bail-semantics assertions (Bailed out after 1 failure, noMARKER_AFTER_BAIL,Ran 1 test across 1 file.) are good but only run on the ASAN lane. - Bug-hunter found nothing; CI build was just kicked off when I reviewed.
57027b7 to
9170952
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this replaces --bail's hard Global::exit(1) with a flag-based unwind across several layers of the test runner's run loop — worth a human pass to confirm the unwind plumbing covers all the edge cases (concurrent groups, in-flight hooks, the stacked-PR ordering with #32180).
Extended reasoning...
Overview
This PR makes failing bun test runs exit 1 cleanly under LeakSanitizer instead of SIGABRT'ing. It touches 7 source files and 1 test file:
src/runtime/test_runner/jest.rs: adds abailed: boolflag toTestRunner.src/runtime/cli/test_command.rs: replaces both--bailhard-exit sites withbailed = true, adds a centralized bail-exit teardown block inexec(), and threadsbailedchecks through the drive loop andrun_all_tests.src/runtime/test_runner/Execution.rs:step_sequence_oneearly-returnsDoneoncebailedis set so no further callbacks/hooks start.src/runtime/test_runner/bun_test.rs:BunTest::rundiscards queued results oncebailedis set so nothing reports after the bail message.src/jsc/SavedSourceMap.rs: newclear()that releases all cached tagged-pointer entries in place.src/jsc/VirtualMachine.rs:collect_for_leak_check_at_exit()now also callssource_mappings.clear().src/runtime/cli/test/parallel/runner.rs: comment-only update.test/cli/test/isolation.test.ts: hoistsleakCheckEnvand adds 4 new ASAN-gated tests for failing exits.
Security risks
None. This is internal test-runner control flow and exit-path teardown; no auth, network, parsing of untrusted input, or permission surfaces are touched.
Level of scrutiny
This warrants careful human review. The core change — converting --bail from a synchronous Global::exit(1) (which guaranteed nothing else ran) into a cooperative flag-based unwind across step_sequence_one → BunTest::run → TestCommand::run drive loop → run_all_tests → TestCommand::exec — is a non-trivial control-flow refactor of the test runner's hot path. The hard exit was a blunt but airtight guarantee; the unwind must now correctly stop every execution path (concurrent groups mid-flight, hooks, queued result-queue items, event-loop ticks, remaining files/reruns) without hanging or printing stragglers. The PR description shows thorough manual verification (concurrent tests with 30s timers pending, --bail --isolate, --bail --rerun-each, BUN_DESTRUCT_VM_ON_EXIT=1), and the new tests assert the bail contract, but a human should sanity-check the unwind coverage and the new SavedSourceMap::clear() lock/release sequence.
Other factors
- Stacked PR: this is stacked on #32180 and depends on its
collect_for_leak_check_at_exit(); merge ordering matters. - Test coverage: the 4 new tests are gated on
isASAN && isLinux, so they only run in one CI lane. The bail-contract assertions (Bailed out after 1 failure, noMARKER_AFTER_BAIL,Ran 1 test across 1 file.) are good, and the author reports existing bail regression tests (bun-test.test.ts, #12250, #26851, parallel.test.ts) pass. SavedSourceMap::clear(): mirrors the existingDropimpl's release loop but leaves the table valid; correctly invalidatesfind_cache/last_ismfirst and is called only on ASAN exit paths after the final GC, so the risk surface is narrow.- No prior reviews on this PR; CI build was triggered but status not yet reflected in the timeline.
|
CI status: every lane is green except one debian 13 x64-baseline test job, which failed on test/js/web/streams/streams-leak.test.ts ("native ReadableStream reuses the pull buffer across small reads", expected >20% reuse, got 9-18% across 4 in-job retries). That test is known flaky under CI load (wire coalescing changes the measured ratio) and has two open de-flake PRs: #32192 and #31355. Nothing in this diff touches streams, and the ASAN-only exit code here is compiled out in that release lane. The darwin jobs are still queued behind the runner backlog. Ready for review; a retry of that one job should clear the red. |
3347d3f to
57e361b
Compare
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
9170952 to
6f1f108
Compare
|
Rebased onto the current Re-verified on a debug ASAN build after the rebase: the 4 failing-exit leak tests and the 3 green-exit leak tests pass (7/7), and the |
There was a problem hiding this comment.
No bugs found, but this refactors --bail from a hard Global::exit(1) into a flag-based unwind across several run-loop layers — worth a human eye on the unwind path (especially with concurrent tests in flight) and the stacked merge order with #32180.
Extended reasoning...
Overview
This PR makes failing bun test runs exit cleanly (exit 1, not SIGABRT) under LeakSanitizer. It touches 8 files: a new SavedSourceMap::clear() called from collect_for_leak_check_at_exit(), a new TestRunner.bailed flag, and the replacement of two Global::exit(1) bail sites with a flag-based unwind threaded through step_sequence_one, BunTest::run, the TestCommand::run drive loop, run_all_tests, and a new consolidated bail-exit block in TestCommand::exec. New ASAN+Linux-gated tests cover serial failing, --parallel failing, --bail mid-file, and --bail on module-eval failure.
Security risks
None. No auth, network, or untrusted-input handling. The change is internal test-runner control flow and exit-time memory cleanup that only affects ASAN builds (collect_for_leak_check_at_exit is a no-op otherwise).
Level of scrutiny
Moderate-to-high. The SavedSourceMap::clear() addition is straightforward (mirrors the existing Drop impl), and the test additions are well-scoped. But replacing a hard exit(1) with a cooperative unwind across five layers (handle_test_completed → step_sequence_one → BunTest::run → drive loop → run_all_tests → exec) is a meaningful control-flow refactor of user-visible --bail semantics. The old behavior was trivially correct (nothing runs after exit); the new behavior depends on every layer correctly observing the flag and short-circuiting without hanging, double-reporting, or running stragglers. The PR description reports manual verification of these cases (concurrent tests in flight, --isolate, --rerun-each, BUN_DESTRUCT_VM_ON_EXIT=1), and existing bail regression tests pass, but this is exactly the kind of change where a second pair of eyes on the unwind invariants is valuable.
Other factors
- Stacked PR: depends on #32180 for
collect_for_leak_check_at_exit(); merge order matters and the base branch is notmain. - The
step_sequence_oneearly-return (Doneon bail) means in-flight concurrent sequences are abandoned without decrementingremaining_incomplete_entries; the unwind relies on the outer drive loop's!bailedguard rather than the phase reachingDone. This looks correct given the newreturn Ok(())after the drive loop, but it's a subtle dependency between layers. - No prior reviews from
claude[bot]on this PR; CI is green except a known-flaky streams test unrelated to this change.
|
Thanks for the pass. On the concurrent-sequence point: that's correct and intentional. Once CI status for this build: the new failing-exit leak tests pass on the x64-asan lane (19 of 20 asan shards green). The two red lanes are unrelated flakes:
Still stacked on #32180; merge that first, then this. |
Fixes #32183. Stacked on #32180 (base branch is
farm/1b909a84/test-exit-lsan-leak); only the last commit is this PR. It needs that PR'scollect_for_leak_check_at_exit(), so it should land after #32180 and be rebased onto main at that point.Problem
A failing
bun testrun aborts (SIGABRT, exit 134) instead of exiting 1 whenASAN_OPTIONS=detect_leaks=1:abort_on_error=1is set andBUN_DESTRUCT_VM_ON_EXITis not, which is exactly how the ASAN CI lane runs subprocesses. #32180 fixed this for green exits; failing exits abort for three more reasons:--bailon an in-test failure callsGlobal::exit(1)fromCommandLineReporter::handle_test_completedwhile the active file is mid-execution, skipping all teardown. No collection runs, and the run loop's strong ref plus liveBunTest::runframes mean a collection at that depth could not free the test-runner boxes anyway.TestCommand::runtears down inline (deinit_for_exit(),RUNNERclear) but exits without the final collection, leavingExpect/RefData/BunTestCellboxes malloc-live.ParsedSourceMapArc invm.saved_source_map_tableviaSavedSourceMap::get_with_content. The table stores entries as tagged pointers (Value), which LSan does not recognize as pointers, so the live cache entry is reported as a leak:Fix
TestRunner.bailedflag. Both bail sites print the summary, bail message, and junit report exactly as before, then set the flag instead of exiting.step_sequence_onestarts nothing further once the flag is set (no test callbacks, no remaining hooks, matching the old hard-exit behavior),BunTest::rundiscards queued results so nothing reports after the bail message, the drive loop inTestCommand::runstops ticking the event loop and returns, andrun_all_testsskips remaining files.TestCommand::execthen exits 1 through the same teardown sequence as the normal tail (deinit_for_exit,RUNNERclear,collect_for_leak_check_at_exit,global_exit), skipping the tail's snapshot/coverage/summary work the old hard exit also skipped.SavedSourceMap::clear()releases every cached entry in place (samerelease_valuetheDropimpl uses, table stays valid and empty), called fromcollect_for_leak_check_at_exit(). This covers serial runs, both bail paths, and--parallelworkers, whose exit path already calls that method. No-op in non-ASAN builds, which keep the fast exit.The load-bearing lines are the
this.jest.bailed = trueassignments, thestep_sequence_oneearly return, and thesource_mappings.clear()call; the rest is the unwind plumbing.Bail behavior is unchanged: the message prints at the same point, nothing runs after it (including
afterAll, whose absence under bail is pre-existing and tracked in #12250), remaining files, repeats, and reruns are skipped, junit is still written, and exit code is 1.--bailis still not forwarded to parallel workers, and the coordinator's own bail path is untouched.Verification
New
failing exits are leak-clean under LeakSanitizertests intest/cli/test/isolation.test.ts(serial failing run,--parallelfailing run,--bailmid-file,--bailon module evaluation failure), gated onisASAN && isLinuxlike the green-run cases from #32180. All four fail on the unfixed build (exit 134 with theSavedSourceMap::get_with_contentleak above, plusExpect::call/BunTest::ref_frames for the bail cases) and pass with the fix. The bail cases also assert the bail contract:Bailed out after 1 failure, nothing runs after the bail,Ran 1 test across 1 file..Also verified on a debug ASAN build:
bun-test.test.ts(79 tests),regression/issue/12250.test.ts,regression/issue/26851.test.ts(junit outfile on bail),parallel.test.tsbail testtest/cli/test/isolation.test.tspasses (26 tests)--bailfires with concurrent tests still in flight (exits 1 in ~0.6s with 30s timers pending)BUN_DESTRUCT_VM_ON_EXIT=1(a path the old hard exit never reached) and without--bail --isolate,--bail --rerun-each=3behave as beforeThe
test/no-validate-leaksan.txtexemptions for bail-spawning tests are left in place; dropping them is follow-up once the lane proves stable, since those files also exercise unrelated subprocess failure modes.