node:fs: mark the per-VM Binding box as LSan-ignored (fixes worker-terminate-lifetime.test.ts on main)#35159
node:fs: mark the per-VM Binding box as LSan-ignored (fixes worker-terminate-lifetime.test.ts on main)#35159robobun wants to merge 6 commits into
Conversation
The Box<Binding> that create_binding() hands to the JSNodeJSFS wrapper is a per-VM singleton: workers free it via ~VM -> lastChanceToFinalize, and the main thread keeps it for process lifetime. After to_js_boxed the only pointer to it lives in the GC wrapper's m_ctx slot inside the JSC heap, and LSan does not scan bmalloc/libpas pages as roots. This was masked by leak:Bun::generateModule in test/leaksan.supp until the nightly-2026-07-20 Rust bump (#34782) added allocator-internal frames and pushed that entry to frame 31, past the default 30-frame malloc_context_size; worker-terminate-lifetime.test.ts then started reporting a 4104-byte "leak" for require("node:fs") on the main thread. Call __lsan_ignore_object on the box so the main-thread singleton is not reported, regardless of stack depth. Workers still free it normally.
WalkthroughChangesThe ASAN module adds an LSAN object-ignore helper. NodeFS marks its per-VM binding singleton as ignored, and an ASAN-only subprocess test verifies clean NodeFS LSAN leak handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:25 PM PT - Jul 22nd, 2026
❌ @robobun, your commit cb30474 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35159That installs a local version of the PR into your bun-35159 --bun |
|
Reproduced by running Self-review surfaced a real CI (builds 77980/77996/78010/78031) is green on everything this diff touches, including |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM modulo the inline nit — the new test was inserted one line into the c-ares regression comment, splitting it mid-sentence. Worth fixing before merge but not blocking.
Extended reasoning...
Overview
Fixes a red test on main caused by the nightly-2026-07-20 Rust toolchain bump: extra allocator-internal frames pushed Bun::generateModule past LSan's default malloc_context_size, so the existing suppression stopped matching and the main-thread node:fs Box<Binding> singleton (whose only anchor lives in the JSC/bmalloc heap, which LSan doesn't scan) started reporting as a leak. The fix adds bun_core::asan::ignore_object<T> (a thin wrapper over __lsan_ignore_object, mirroring the existing register_root_region helpers) and calls it on the box in create_binding() before ownership transfers to the GC wrapper. A new ASAN-only minimal-repro test is added.
Security risks
None. The only production-code change is an ASAN-only annotation that compiles to let _ = ptr; under #[cfg(not(bun_asan))]. No behavioral change to node:fs, no memory-management change (workers still free the box via ~VM → lastChanceToFinalize → Binding::finalize; the ignore is per-allocation and doesn't prevent freeing).
Level of scrutiny
Low-to-medium. This is a CI-greenness fix with a well-documented root cause (the PR description traces the exact frame depth and explains why the suppression stopped matching). The new ignore_object helper follows the exact pattern of the neighboring register_root_region/unregister_root_region helpers in src/bun_core/lib.rs. The alternative (broadening the suppression file) was considered and rejected in favor of the narrower per-allocation ignore, which is the right call per REVIEW.md ("Never disable sanitizers or weaken CI verification to get green" — this is targeted, not a blanket suppression).
Other factors
- Fail-before/pass-after verified with
git stash -- src/per the robobun comment. - The one finding is a nit: the new test block was inserted between lines 1 and 2 of the pre-existing c-ares regression comment, so the c-ares comment now starts above the wrong test and the dns.lookup test's comment begins mid-sentence. Real but cosmetic — no runtime effect.
- The extern declaration for
__lsan_ignore_objectis inside the#[cfg(bun_asan)]-gated extern block (same block as the other__lsan_*symbols), so non-ASAN builds don't reference an undefined symbol.
![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
🤖 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 `@src/runtime/node/node_fs_binding.rs`:
- Around line 419-427: Shorten the inline comment near the per-VM singleton in
src/runtime/node/node_fs_binding.rs:419-427 to three lines or fewer, retaining
only the ownership and LSan-rooting invariant. Also shorten the comment in
test/js/web/workers/worker-terminate-lifetime.test.ts:124-130 to three lines or
fewer, retaining only the regression intent and removing stack-depth history.
🪄 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: 0164a475-d0b2-417e-ae90-93ab5f21daf3
📒 Files selected for processing (3)
src/bun_core/lib.rssrc/runtime/node/node_fs_binding.rstest/js/web/workers/worker-terminate-lifetime.test.ts
There was a problem hiding this comment.
LGTM — the earlier comment-splitting nit is fixed in ed71d53, and the rest is a narrow, well-justified LSan false-positive suppression.
What was reviewed:
ignore_object<T>follows the exact pattern ofregister_root_regionimmediately above it (samesafe fnextern block, same#[cfg(bun_asan)]gating, same no-op fallback).&*module as *const Bindingyields the boxed data address before ownership transfers viato_js_boxed;__lsan_ignore_objectis per-allocation so worker-VM finalize still frees it normally.- New test mirrors the neighboring dns.lookup test's ASAN/LSAN env setup, drains pipes concurrently, asserts the combined
{stdout, stderr, exitCode}object, and isskipIf(!isASAN). - Checked that this narrows (not widens) suppression scope vs. the pre-existing
leak:Bun::generateModuleentry it effectively replaces for this allocation.
Extended reasoning...
Overview
Three files: (1) src/bun_core/lib.rs adds __lsan_ignore_object to the existing ASAN extern block plus a thin ignore_object<T> wrapper; (2) src/runtime/node/node_fs_binding.rs calls it once on the freshly-boxed Binding in create_binding() before to_js_boxed hands the box to the GC wrapper; (3) test/js/web/workers/worker-terminate-lifetime.test.ts gains an ASAN-only minimal repro (require("node:fs") under detect_leaks=1).
Security risks
None. This is diagnostic tooling only — the new code is entirely #[cfg(bun_asan)]-gated and compiles to let _ = ptr in release builds. No user-facing behavior, no auth/crypto/permissions surface.
Level of scrutiny
Moderate, because REVIEW.md flags "never disable sanitizers or weaken CI verification to get green." The PR clears that bar: the root-cause analysis shows this is a genuine LSan false positive (the box's only anchor is m_ctx in the JSC heap; bmalloc/libpas pages aren't LSan roots), demonstrates it's per-VM-singleton (8 workers → still exactly one report; workers free theirs via ~VM → lastChanceToFinalize → Binding::finalize), and explains the trigger (Rust nightly-2026-07-20 added allocator frames pushing Bun::generateModule past malloc_context_size, so the existing suppression stopped matching). The fix is narrower than the suppression it replaces — one specific allocation vs. every module-init frame — and __lsan_ignore_object is the canonical API for exactly this case.
Other factors
- The new helper matches neighboring code byte-for-byte in structure (
safe fnextern,.cast(), cfg-gated no-op), so there's no novel FFI risk. - The author considered and documented the alternative (suppress
Bun::JS2Native::jsDollarLazy) and chose the more targeted fix. - Other
to_js_boxedcall sites exist (SecureContext, filesystem_router, output_file_jsc) but aren't touched — reasonable, since only this one is a confirmed process-lifetime singleton currently tripping LSan; widening would be speculative. - Verification listed in the PR body covers the failing test, the new repro (fail-before/pass-after via
git stash -- src/), the worker-teardown-still-frees case, andfs.test.ts. - My earlier inline nit (new test inserted mid-sentence into the c-ares regression comment) was fixed in ed71d53 — the current diff shows the c-ares comment block contiguous above its own test, and the thread is resolved.
…t to node/fs - ignore_object<T>(&T) absorbs the ptr::from_ref at the definition so callers don't each have to work around the workspace-denied borrow_as_ptr lint. - The LSan test exercises node_fs_binding.rs with no workers involved, so it belongs under test/js/node/fs/ per the test-organization convention. worker-terminate-lifetime.test.ts is restored to main; the dns.lookup test there still covers the worker path.
There was a problem hiding this comment.
LGTM — the inline timeout nit is non-blocking. Prior feedback (split c-ares comment, comment length) is addressed; the test now lives in fs-oom.test.ts and worker-terminate-lifetime.test.ts is out of the diff.
Extended reasoning...
Overview
Adds bun_core::asan::ignore_object<T>(&T) wrapping __lsan_ignore_object, calls it once in node_fs_binding.rs::create_binding on the Box<Binding> before ownership transfers to the GC wrapper, and adds an ASAN-only subprocess regression test in test/js/node/fs/fs-oom.test.ts. ~13 lines of Rust + ~22 lines of test.
Security risks
None. The helper is #[cfg(bun_asan)]-gated and compiles to let _ = r; in release builds; it only affects LeakSanitizer's leak-report bookkeeping in debug/ASAN, with no effect on allocation, freeing, or runtime behavior.
Level of scrutiny
Low. The new helper is a byte-for-byte sibling of the existing register_root_region / unregister_root_region wrappers directly above it (same extern block, same #[cfg] shape, same #[inline]). At the call site, module: Box<Binding> deref-coerces to &Binding, so ptr::from_ref yields the heap address of the boxed Binding — the correct argument for __lsan_ignore_object. The call is placed before to_js_boxed, while the box is still Rust-owned. The root-cause analysis (JSC-heap-anchored singleton invisible to LSan; suppression stopped matching after allocator frames pushed Bun::generateModule past malloc_context_size) is coherent and the fix is narrower than a blanket suppression.
Other factors
- All prior inline threads are resolved: my earlier nit about the split c-ares comment was fixed in ed71d53, and the test was subsequently moved to
fs-oom.test.ts(7d3fc82); CodeRabbit's comment-length nit was addressed in 62c5a14. - The new test follows harness conventions:
bunEnvspread,.skipIf(!isASAN), concurrent pipe drain, combined{stdout, stderr, exitCode}assertion, points at the checked-intest/leaksan.supp. - CI failures (
test-net-connect-memleak.js,test-gc-http-client-connaborted.js) are release-build GC-timing flakes unrelated to an ASAN-cfg-only change; the author already cross-referenced #33044. - The one remaining inline finding (60s per-test timeout) is a style nit with no runtime impact and doesn't warrant blocking.
Without it the test times out at the 5s default: detect_leaks=1 runs LSan's reachability scan at child exit, which alone takes ~5-6s under debug+ASAN. The sibling ASAN subprocess tests in this file set detect_leaks=0, so they don't pay that cost. Reduced from 60s to 30s.
There was a problem hiding this comment.
LGTM — my two earlier nits are resolved (test moved to fs-oom.test.ts so the c-ares comment is untouched; the 30s timeout is justified with measured evidence and a comment).
What was reviewed:
ignore_object::<Binding>(&module)— turbofish forces deref-coercion so__lsan_ignore_objectgets the heap pointer, not the stackBoxslot.- Non-ASAN builds: the new call compiles to
let _ = r;, so no release-build behavior change. - New extern + wrapper match the sibling
__lsan_register_root_regionpattern in the same module. - Test follows harness conventions:
bunEnvspread, both pipes drained concurrently, combined{stdout, stderr, exitCode}assertion,skipIf(!isASAN).
Extended reasoning...
Overview
Three-file change: adds __lsan_ignore_object to the existing bun_core::asan extern block plus a thin ignore_object<T>(&T) wrapper (src/bun_core/lib.rs), calls it once on the freshly-boxed Binding in create_binding before ownership transfers to the GC wrapper (src/runtime/node/node_fs_binding.rs), and adds an ASAN-only subprocess regression test (test/js/node/fs/fs-oom.test.ts). The root cause — a per-VM singleton whose only anchor lives in JSC-heap pages that LSan doesn't scan, previously masked by a suppression that stopped matching after a Rust bump added allocator frames — is well-argued in the PR body and matches the code.
Security risks
None. The only runtime effect is a call into the LSan interface under #[cfg(bun_asan)]; release builds see let _ = r;. No user-facing surface, no data handling, no auth/crypto.
Level of scrutiny
Low. This is sanitizer-tooling plumbing gated behind cfg(bun_asan), following the exact pattern of the four sibling helpers in the same module (poison, unpoison, register_root_region, unregister_root_region). The one subtle point — that &module on a Box<Binding> must deref-coerce to &Binding so the heap address (not the stack-local Box) reaches LSan — is handled by the explicit ::<Binding> turbofish, and the PR body explains the &T signature was chosen deliberately to keep ptr::from_ref at the definition and avoid the workspace-denied clippy::borrow_as_ptr at call sites.
Other factors
I reviewed this PR twice previously. Both nits are resolved: (1) the split c-ares comment — the test was subsequently moved out of worker-terminate-lifetime.test.ts entirely into fs-oom.test.ts per module-organization convention, so that file is no longer in the diff; (2) the per-test timeout — the author demonstrated the 5s default fails (LSan's exit-time reachability scan alone takes ~6s under debug+ASAN), reduced 60s→30s, and added a comment naming why no shorter bound works, satisfying REVIEW.md's "a literal sleep/timeout … needs a comment naming why". The CodeRabbit comment-length nit was also addressed. CI is green on the touched paths per the author's summary; the one unrelated flake is a known release-build GC-timing race and this change is cfg-gated out of release builds.
1 participant
What
test/js/web/workers/worker-terminate-lifetime.test.ts"terminate() while dns.lookup() is in flight" is red on main:The report is the
Box<Binding>thatcreate_binding()hands to theJSNodeJSFSGC wrapper wheninternal/fs/binding.tsfirst evaluates.Cause
This is not a leak and not worker-related. The box is a per-VM singleton: workers free it via
~VM->lastChanceToFinalize->Binding::finalize(8 workers thatrequire("node:fs")still report exactly one "leak"), and the main thread keeps it for process lifetime because it does not destroy its JSC VM. Afterto_js_boxedthe only pointer to the box lives in the wrapper'sm_ctxslot, inside the JSC heap; LSan does not scan bmalloc/libpas pages as roots, so it cannot find the pointer.This was always masked by
leak:Bun::generateModuleintest/leaksan.supp. Atmalloc_context_size=60that frame sits at depth 31. Thenightly-2026-07-20Rust bump in #34782 added allocator-internal frames (frames 1-10 of the report are all libstd alloc plumbing) and pushedBun::generateModuleone past the default 30-framemalloc_context_size, so the suppression stopped matching.Fix
Call
__lsan_ignore_objecton the box before transferring it to the GC wrapper. The ignore is per-allocation: workers still free it normally, and the main-thread singleton is no longer reported regardless of allocator stack depth. Addsbun_core::asan::ignore_object<T>(&T)alongside the existingregister_root_regionhelpers; taking&T(rather than*const T) keeps theptr::from_refat the definition so callers do not each trip the workspace-deniedclippy::borrow_as_ptrlint.A suppression on
Bun::JS2Native::jsDollarLazy(frame 23) would also work and covers the whole$rust()lazy-init class, but is broader than needed; this keeps it to the one allocation that is known to be VM-lifetime.Verification
test/js/node/fs/fs-oom.test.tsis the minimal repro; fails on main, passes with the fix.bun bd test test/js/web/workers/worker-terminate-lifetime.test.ts: 4 pass (was 3 pass / 1 fail).bun bd test test/js/node/fs/fs-oom.test.ts: 14 pass.node:fsthen terminated: 0 leaks (worker teardown still frees the box).cargo clippy -p bun_runtime -p bun_core --no-deps: clean.test/js/node/fs/fs.test.ts: 427 pass / 6 skip.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs-oom.test.ts