bun test: free test-runner finalizer-owned allocations before exit so LSan lanes don't abort green runs#32180
bun test: free test-runner finalizer-owned allocations before exit so LSan lanes don't abort green runs#32180robobun wants to merge 5 commits into
Conversation
|
Updated 5:47 PM PT - Jun 20th, 2026
❌ @robobun, your commit 57e361b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32180That installs a local version of the PR into your bun-32180 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughAdds an ASAN-only final garbage-collection step at ChangesLeak-check collection at test exit
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/cli/test_command.rs (1)
2943-2955:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute the
--bailshutdown through this same leak-check path.This fixes the normal
exec()exit, butTestCommand::run()still has a direct bail-outglobal_exit()path at Lines 3179-3205 that skipscollect_for_leak_check_at_exit(). Abun test --bailrun can therefore still bypass the final ASAN GC and hit the same LSan abort class on exit.Based on learnings: "Rust code: fix the whole bug class in the same PR - grep for every sibling site sharing the pattern."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/test_command.rs` around lines 2943 - 2955, The direct bailout path in TestCommand::run() calls global_exit() and skips the leak-check teardown (reporter.jest.bun_test_root.deinit_for_exit(), unsafe { jest::Jest::RUNNER.write(None); }, drop(reporter); vm.collect_for_leak_check_at_exit()), so add routing from the --bail branch to perform the same shutdown sequence instead of calling global_exit() directly: locate the early-exit branch in TestCommand::run() that currently calls global_exit(), and replace or augment it to call the existing exit teardown sequence (deinit_for_exit(), clear RUNNER, drop reporter, then vm.collect_for_leak_check_at_exit()) before returning/exiting so the ASAN/LSan leak check runs on bail as well.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/test/isolation.test.ts`:
- Around line 889-922: Replace the manual for-loop parameterization with Bun's
test.each by turning the cases array into a test.each(cases) call so each tuple
[label, args] becomes a separate test; inside the test callback keep the
existing test.skipIf(!isASAN) guard and reuse the same body (calls to tempDir,
FILE_COUNT, bunExe(), bunEnv, the Bun.spawn invocation assigned to proc, and
assertions on stderr and exitCode) so behavior is identical but the matrix is
declarative and idiomatic.
---
Outside diff comments:
In `@src/runtime/cli/test_command.rs`:
- Around line 2943-2955: The direct bailout path in TestCommand::run() calls
global_exit() and skips the leak-check teardown
(reporter.jest.bun_test_root.deinit_for_exit(), unsafe {
jest::Jest::RUNNER.write(None); }, drop(reporter);
vm.collect_for_leak_check_at_exit()), so add routing from the --bail branch to
perform the same shutdown sequence instead of calling global_exit() directly:
locate the early-exit branch in TestCommand::run() that currently calls
global_exit(), and replace or augment it to call the existing exit teardown
sequence (deinit_for_exit(), clear RUNNER, drop reporter, then
vm.collect_for_leak_check_at_exit()) before returning/exiting so the ASAN/LSan
leak check runs on bail as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 719ec80f-9fd9-4195-8b0c-a30439f181f5
📒 Files selected for processing (5)
src/jsc/VirtualMachine.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rstest/cli/test/isolation.test.tstest/leaksan.supp
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches VM shutdown ordering (GC, has_run_cleanup_hooks, napi finalizer scheduling) across three exit paths and has a noted interaction with #32146, so it's worth a human look.
Extended reasoning...
Overview
This PR adds VirtualMachine::collect_for_leak_check_at_exit() — an ASAN-only synchronous full GC that first sets has_run_cleanup_hooks = true so napi finalizers deferred by the sweep take NapiFinalizerTask::schedule()'s drop-immediately branch instead of being parked on RareData::cleanup_hooks. It's called from the serial/isolate exit path in test_command.rs and the --parallel worker exit path in runner.rs, both after is_shutting_down = true (so the debug_assert holds). A leaksan.supp entry covers the still-rooted ScopeFunctions boxes, and three new ASAN+Linux-gated tests in isolation.test.ts reproduce the original SIGABRT and assert it's gone.
Security risks
None apparent. The new code path is gated on bun_core::env::ENABLE_ASAN (a compile-time constant), so release builds are byte-for-byte unchanged at the call sites. No new inputs, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. Although the runtime change is a no-op outside ASAN builds, shutdown/teardown ordering is notoriously subtle: this flips has_run_cleanup_hooks to true on a path that never ran on_exit(), which means any registered napi env cleanup hooks on that list are intentionally never drained (the PR description argues this is the documented destructOnExit semantics for bun test, and notes that #32146 changes the same bookkeeping — whichever lands second needs a small rebase to use napi_internal_set_cleanup_hooks_only() + run_cleanup_hooks() instead). That cross-PR coupling and the GC-during-shutdown ordering are exactly the kind of thing a maintainer who owns the shutdown path should sign off on.
Other factors
- CI on the head commit shows musl
build-cppfailures and assorted infra noise; this PR touches no C++, so those are likely unrelated, but the build isn't fully green. - The new tests are well-targeted (they assert on the LSan report text for
--parallelsince a worker's at-exit abort doesn't change the coordinator's exit code), and the suppression entry is narrowly scoped and well-commented. - The bug-hunting system found no issues; CodeRabbit's only note was a trivial
test.eachstyle nit.
|
Regarding the review note about the --bail shutdown path (the module-load-failure bail in TestCommand::run): verified, and it is real but insufficient on its own. I prototyped adding collect_for_leak_check_at_exit() there plus a --bail test case; the child still aborts because printing the failure caches a ParsedSourceMap Arc in vm.saved_source_map_table (SavedSourceMap.rs:365 via remap_zig_exception), which LSan cannot scan, and that fires on any failing run, bail or not. The ordinary-test bail is a third case: handle_test_completed calls Global::exit(1) directly mid-execution, where running the exec tail teardown is not obviously safe (the run loop still holds the BunTestCell strong). None of these have CI exposure today: every test spawning bun test --bail is in test/no-validate-leaksan.txt, and the leak lane sets BUN_DESTRUCT_VM_ON_EXIT=1 whose destroy() frees the source-map table. Making failure exits LSan-clean is a separate change with its own blockers, tracked in #32183; this PR stays scoped to green-run exits (the CI flake from #32176). |
|
The failing-exit half from the scope note is up at #32187, stacked on this branch. Merging this PR first, then rebasing that one onto main, is the intended order. |
… LSan lanes don't abort green runs At exit of a bun test run, the final file's expect() wrapper boxes are freed only by GC finalizers (Expect::finalize derefs the RefData that pins the file's BunTestCell), and no collection runs between the last test and exit(). The ASAN CI lane runs subprocesses with ASAN_OPTIONS=detect_leaks=1:abort_on_error=1, so LeakSanitizer reports those boxes and SIGABRTs (exit 134) an otherwise green run. Add VirtualMachine::collect_for_leak_check_at_exit, called from the test runner's exit path and the --parallel worker's exit path. It is a no-op in non-ASAN builds. Under ASAN it marks the cleanup-hook list as never-drained (these exit paths never call on_exit(), so napi finalizers deferred by the sweep would otherwise be parked there forever) and runs one synchronous full collection so the finalizers free their Rust boxes before LSan scans. The final global's ScopeFunctions boxes stay alive at exit (rooted by the still-protected global's bun:test exports); LSan cannot scan the JSC heap cells holding the only pointers to them, so suppress that allocation site in test/leaksan.supp. Fixes #32176
…here LSan is unsupported
robobun
force-pushed
the
farm/1b909a84/test-exit-lsan-leak
branch
from
3347d3f to
57e361b
Compare
|
Rebased onto main. The only conflict was in test/leaksan.supp: main added suppression entries for run_scripts_with_filter / multi_run::run while this branch added the scope_functions::ScopeFunctions entry. Both are independent additions, kept both. VirtualMachine.rs auto-merged. Re-verified on a debug ASAN build: the three leak-clean cases pass with the fix and the serial case still aborts with the same 1884-byte Expect/RefData/BunTestCell leak when src is reverted to main. |
There was a problem hiding this comment.
LGTM — ASAN-only exit-path GC, no-op in release builds; prior feedback addressed.
Extended reasoning...
Overview
This PR adds VirtualMachine::collect_for_leak_check_at_exit() — a method that sets has_run_cleanup_hooks = true and runs one synchronous full GC — and calls it from the two bun test exit paths (main runner at test_command.rs:2955, parallel worker at runner.rs:713) immediately before global_exit(). It also adds a leaksan.supp entry for scope_functions::ScopeFunctions (a reachable-but-unscannable false positive), and three new isASAN && isLinux-gated test cases in isolation.test.ts that spawn bun test under detect_leaks=1:abort_on_error=1 and assert exit 0 with no LeakSanitizer report.
Security risks
None. The new code path is hard-gated on bun_core::env::ENABLE_ASAN (compile-time constant), so release builds early-return and behavior is unchanged. In ASAN builds it runs a GC at process exit after all test results are collected — no new attack surface, no user-controlled inputs.
Level of scrutiny
Low-to-moderate. While VM shutdown / napi finalizer ordering is subtle territory in general, the entire behavioral change is a no-op outside ASAN debug builds, and within ASAN builds it only fires after is_shutting_down = true on the test runner's exit path. The has_run_cleanup_hooks = true write matches the documented semantics on the field (VirtualMachine.rs:214-217) and mirrors what on_exit() already does before destructOnExit()'s collection. The suppression file addition is well-commented and bounded (per-global). This is CI-infrastructure work to deflake a leak-check lane, not a production behavior change.
Other factors
- Both of my prior inline comments (the
--bailsibling path and the LSAN_OPTIONS→ASAN_OPTIONS doc typo) were resolved: the doc comment was fixed, and the author explained (with a prototype) why the--bailpath was intentionally excluded and tracked it in #32183 / #32187. The PR body's scope note documents this. - CodeRabbit's
test.eachnit was applied; the test is now also gated onisLinuxto avoid the macOS-arm64 startup error. - The PR was rebased onto main today with only a trivial additive conflict in leaksan.supp; the author re-verified the three test cases pass and the serial case still fails on unfixed src.
- No CODEOWNERS coverage for the touched files.
- The bug-hunting system found no issues on this run.
- The noted interaction with #32146 is a small rebase concern for whichever lands second, not a correctness issue with this change.
|
CI (build 63682) is red on one lane, unrelated to this diff:
The diff touches only the |
1 participant
Fixes #32176
Problem
On the
13 x64-asanlane,test/regression/issue/30205.test.tsintermittently fails with the innerbun test --isolatesubprocess exiting 134 after a fully green run. The CI runner propagatesASAN_OPTIONS=...detect_leaks=1:abort_on_error=1andLSAN_OPTIONS=...suppressions=test/leaksan.suppinto spawned processes viabunEnv, and LeakSanitizer's exit scan reports test-runner allocations that are still malloc-live, turning the report into SIGABRT.Deterministic repro on main (debug ASAN build, no napi addon needed; also fails without
--isolateand with--parallel):Cause
Three distinct allocation classes are live when LSan scans at exit:
expect()wrapper boxes are freed only by their GC finalizers (Expect::finalize, which also derefs theRefDatapinning that file'sBunTestCell), and no collection runs between the last test andexit(). Under--isolate, earlier files' wrappers are collected by GCs that happen while later files run; the last file has no later GC. Plain runs and--parallelworkers have the same hole.NapiFinalizerTask.NapiFinalizerTask::schedule()parks them onRareData::cleanup_hooksbecausebun test's exit paths never callon_exit(), sohas_run_cleanup_hooksis false and the list is never drained: every task box leaks (32000 bytes in 1000 allocations for the 30205 fixture). This is also why the lane failure is timing dependent on unpatched main: it needsdestructOnExit's conservativecollectNow()to actually collect the wraps.ScopeFunctionsboxes (the native state of itstest/describe/expectfunction wrappers) are genuinely alive at exit, rooted by the still-gcProtected global'sbun:testexports. The only pointers to them live in JSC heap cells, which LSan cannot scan, so it reports them as leaks. They are freed whenever a global actually dies (isolation swap,BUN_DESTRUCT_VM_ON_EXITteardown).Fix
VirtualMachine::collect_for_leak_check_at_exit(), called from the test runner's exit path (covers plain,--isolate, and the--parallelcoordinator) and from the--parallelworker's exit path. No-op in non-ASAN builds, which keep skipping all teardown for exit speed. Under ASAN it setshas_run_cleanup_hooks = truefirst, so napi finalizers deferred by the sweep takeschedule()'s existing drop-immediately branch (the documented shutdown semantics fordestructOnExit's collection) instead of being parked forever, then runs one synchronous full collection so the test-runner finalizers free their boxes. The load-bearing lines are the flag set and thegarbage_collect(true)call.test/leaksan.suppentry for theScopeFunctionsallocation site (class 3 is a reachable-but-unscannable false positive, the exact thing the suppression file exists for; the file already suppresses the globals themselves).Verification
New
exit is leak-clean under LeakSanitizercases intest/cli/test/isolation.test.ts(serial,--isolate,--parallel=2), gated onisASAN. They spawnbun testwithdetect_leaks=1:abort_on_error=1andBUN_DESTRUCT_VM_ON_EXITstripped, and assert no leak report,4 pass, exit 0. All three fail on the unfixed build (the serial and isolate children exit 134; a parallel worker's abort does not change the coordinator's exit code, so that case asserts on the report text in inherited stderr).Verified on a debug ASAN build:
addon.wrap(...),BUN_JSC_collectContinuously=1,BUN_DESTRUCT_VM_ON_EXIT=1, leak checking on) exits 0 with no report, 5/5 runs; on the unfixed build it aborts with theNapiFinalizerTaskleaktest/regression/issue/30205.test.tspasses (4/4) under the CI lane's envtest/cli/test/isolation.test.tspasses (22/22)Note on #32146
#32146 fixes the napi half of the same lane flake (issue #32144) by draining the cleanup hooks inside
global_exit()with a hooks-only restriction. The two changes are complementary but touch the same shutdown bookkeeping; whichever lands second needs a small rebase: with #32146 in, this PR's method should callnapi_internal_set_cleanup_hooks_only()+run_cleanup_hooks()instead of settinghas_run_cleanup_hooksdirectly, so the hooks-only restriction and the env cleanup hooks both still apply before the collection.Scope note
Exits after failures (
--bailand ordinary failing runs) are intentionally excluded: they abort under the same env for additional reasons (a directGlobal::exit(1)in the bail-on-test-failure path, and aParsedSourceMapcache entry from printing the failure that LSan cannot see through), none of which have CI exposure today. Tracked in #32183.