runtime: single-owner reset for transpile_source_code_arena#31119
runtime: single-owner reset for transpile_source_code_arena#31119Jarred-Sumner wants to merge 2 commits into
Conversation
|
Updated 1:00 AM PT - May 20th, 2026
✅ @Jarred-Sumner, your commit 56a0c2e58aa721a9a8d3d0ea6c745ec39e15a627 passed in 🧪 To try this PR locally: bunx bun-pr 31119That installs a local version of the PR into your bun-31119 --bun |
ModuleLoader::reset_arena is now the only place that resets the per-VM transpiler arena (smol -> reset(), else reset_retain_with_limit(8M), matching ModuleLoader.zig:24-29). The inner give-back in transpile_source_code stops resetting and just clears AST_HEAP + parks the Box; the caller's ArenaResetGuard handles the reset after process_fetch_log. Previously the inner did reset_retain_with_limit(8M) and reset_arena immediately followed with a full reset(), so the retain never stuck and every module paid mi_heap_destroy + mi_heap_new. Two scopes with two inline reset bodies had diverged; consolidating to one function prevents that. Replaces the scopeguard closure with ActiveTranspilerArena (Drop) and drops the now-unused give_back_arena flag.
Jarred-Sumner
force-pushed
the
claude/ast-alloc-bump-arena
branch
from
9e35b3a to
8e66a2b
Compare
|
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)
WalkthroughRefactors transpiler arena ownership into an ActiveTranspilerArena RAII guard, updates ModuleLoader arena reset to branch on the VM smol flag, adapts async/print/error paths to the new guard, and adds a test that validates arena retention across 200 CommonJS modules. ChangesTranspile Arena Lifecycle Refactoring and Memory Optimization
Suggested reviewers:
🚥 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: 2
🤖 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/run/require-cache.test.ts`:
- Line 378: Replace the use of tempDirWithFiles with the harness helper tempDir
for the multi-file fixture: locate the test code that calls tempDirWithFiles
(the line creating const dir = tempDirWithFiles("transpile-arena-retain",
files)) and change it to call tempDir with the fixture tree as the second
argument (i.e., call tempDir("transpile-arena-retain", files)); ensure the
variable name (dir) and the fixture object (files) are passed unchanged so the
test continues to use the disposable temp directory pattern required by the
harness.
- Line 387: The test currently asserts expect(stderr).toBe("") which can fail
due to ASAN startup noise; update the assertion by splitting stderr into lines,
filtering out any lines that start with "WARNING: ASAN interferes" (and trim
whitespace/newlines), then assert the filtered array is empty (or join back and
expect("")). Locate the stderr variable and the expect(stderr).toBe("")
assertion in the test and replace it with the filtered-lines check so ASAN
warning lines are ignored before asserting no other stderr output.
🪄 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: c7abdf8a-4b92-403e-974b-f456b00293da
📒 Files selected for processing (3)
src/jsc/ModuleLoader.rssrc/runtime/jsc_hooks.rstest/cli/run/require-cache.test.ts
| `exports.values = [${Array.from({ length: 8 }, (_, k) => i * k).join(", ")}];\n` + | ||
| `exports.padded = ${JSON.stringify(Buffer.alloc(4096, 46).toString() + i)};\n`; | ||
| } | ||
| const dir = tempDirWithFiles("transpile-arena-retain", files); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use tempDir for this multi-file fixture instead of tempDirWithFiles.
On Line 378, please switch to tempDir(...) (with the fixture tree as the second argument) so this follows test harness conventions and disposable temp-dir patterns.
As per coding guidelines: “for multi-file tests, create a temporary directory using tempDir from harness.”
🤖 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 `@test/cli/run/require-cache.test.ts` at line 378, Replace the use of
tempDirWithFiles with the harness helper tempDir for the multi-file fixture:
locate the test code that calls tempDirWithFiles (the line creating const dir =
tempDirWithFiles("transpile-arena-retain", files)) and change it to call tempDir
with the fixture tree as the second argument (i.e., call
tempDir("transpile-arena-retain", files)); ensure the variable name (dir) and
the fixture object (files) are passed unchanged so the test continues to use the
disposable temp directory pattern required by the harness.
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); |
There was a problem hiding this comment.
Filter known ASAN startup noise before asserting empty stderr.
On Line 387, expect(stderr).toBe("") can fail on ASAN runs when the subprocess boots the VM. Please split/filter stderr lines and ignore lines starting with "WARNING: ASAN interferes" before asserting emptiness.
Suggested patch
- expect(stderr).toBe("");
+ const stderrLines = stderr
+ .split(/\r?\n/)
+ .filter(line => line.length > 0)
+ .filter(line => !line.startsWith("WARNING: ASAN interferes"));
+ expect(stderrLines).toEqual([]);Based on learnings: when spawning subprocesses with bunEnv, use the repo’s standard ASAN warning-line filter before empty-stderr assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(stderr).toBe(""); | |
| const stderrLines = stderr | |
| .split(/\r?\n/) | |
| .filter(line => line.length > 0) | |
| .filter(line => !line.startsWith("WARNING: ASAN interferes")); | |
| expect(stderrLines).toEqual([]); |
🤖 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 `@test/cli/run/require-cache.test.ts` at line 387, The test currently asserts
expect(stderr).toBe("") which can fail due to ASAN startup noise; update the
assertion by splitting stderr into lines, filtering out any lines that start
with "WARNING: ASAN interferes" (and trim whitespace/newlines), then assert the
filtered array is empty (or join back and expect("")). Locate the stderr
variable and the expect(stderr).toBe("") assertion in the test and replace it
with the filtered-lines check so ASAN warning lines are ignored before asserting
no other stderr output.
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/jsc_hooks.rs:2125-2134— nit: thereset_storenote just above (jsc_hooks.rs:2098-2099) still says "both reclaimed at the give-backarena.reset_retain_with_limit" — after this change the give-back no longer resets, so that line now contradicts the new comment here stating "It does not reset". Since the point of the PR is making reset ownership single-sourced, the stale "at the give-back" phrasing should be updated to reference the caller'sArenaResetGuard/ModuleLoader::reset_arena.Extended reasoning...
This is a comment-accuracy nit with no runtime effect, but it directly undermines the PR's stated goal so it's worth a one-line fix.
What is stale. The pre-existing comment at
src/runtime/jsc_hooks.rs:2093-2101justifies skippingstore_ast_alloc_heap::reset()by noting thatAstAllocand the parser scratch share onemi_heap_t*with "identical lifetime — both reclaimed at the give-backarena.reset_retain_with_limit". Before this PR that was accurate: the scopeguard closure (the give-back) calledarena.reset_retain_with_limit(8M)inline. This PR removes that call —ActiveTranspilerArena::dropnow only clearsAST_HEAPand parks theBox— and moves the reset exclusively toModuleLoader::reset_arenavia the caller'sArenaResetGuard. The PR's own new comment at :2125-2134 states this explicitly: "It does not reset: the caller'sArenaResetGuard… is the single owner of reset semantics."The contradiction. A reader scanning
transpile_source_code_innertop-down now hits, in the same function and ~27 lines apart:- :2098-2099 — "both reclaimed at the give-back
arena.reset_retain_with_limit" - :2128-2129 — "It does not reset: the caller's
ArenaResetGuard… is the single owner of reset semantics"
These directly contradict each other on where the reset happens. The diff hunk starts at :2109, so :2093-2101 was not touched — the PR made it stale rather than it being pre-existingly wrong.
Addressing the "not worth flagging" objection. It's true that the semantic core of the :2098 comment —
AstAllocand parser scratch share one heap with identical lifetime, both reclaimed byreset_retain_with_limit— remains correct (the call still happens, just inModuleLoader::reset_arenainstead of inline). And yes, the new bold-faced comment 27 lines below does clarify the actual reset location. But the PR description is explicit that the motivating bug was "two scopes had two inline reset bodies that diverged" and the fix is "makingModuleLoader::reset_arenathe single owner of reset semantics". Leaving a comment that names the old owner is exactly the kind of divergence-in-prose the PR is trying to eliminate in code. A future reader debugging arena lifetime would have to notice and reconcile the two contradicting comments rather than getting one authoritative answer.Step-by-step proof.
- Read
jsc_hooks.rs:2098-2099on this branch:both reclaimed at the give-backarena.reset_retain_with_limit``. - Read
ActiveTranspilerArena::drop(jsc_hooks.rs:1958-1971): it callsset_thread_heap(null)and*slot = Some(arena)— noreset/reset_retain_with_limitcall anywhere. - Grep for
reset_retain_with_limitin the touched files: the only call is now inModuleLoader::reset_arena(ModuleLoader.rs:73), invoked byArenaResetGuard::drop. - Therefore :2098-2099's "at the give-back" attribution is false on this branch.
Fix. Replace "at the give-back
arena.reset_retain_with_limit" with something like "by the caller'sArenaResetGuard(ModuleLoader::reset_arena)". One-line edit; the rest of the comment (the lifetime-unification rationale) stays as-is.Severity: nit — documentation only, no behavioral impact, should not block the PR.
- :2098-2099 — "both reclaimed at the give-back
| impl Drop for ActiveTranspilerArena { | ||
| fn drop(&mut self) { | ||
| let Some(arena) = self.arena.take() else { | ||
| return; | ||
| }; | ||
| bun_alloc::ast_alloc::set_thread_heap(core::ptr::null_mut()); | ||
| // SAFETY: `self.vm` is the live per-thread VM (`transpile_source_code` | ||
| // fn contract; `Drop` runs on the same thread before the hook returns). | ||
| let slot = unsafe { &mut (*self.vm).module_loader.transpile_source_code_arena }; | ||
| if slot.is_none() { | ||
| *slot = Some(arena); | ||
| } | ||
| // else (re-entrant — slot occupied): drop the fresh Box (spec :161-163). | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 When the slot is occupied, this drops the current frame's arena — but in the re-entrant + outer-ParseError case, that arena owns the source bytes the log spans point into, and process_fetch_log hasn't run yet. The old !give_back branch parked the arena unconditionally (overwriting any occupant) precisely to keep those bytes alive; with give_back_arena removed, the outer arena is now mi_heap_destroy'd here and transpile_file reads freed memory at :4290. Fix: when the slot is occupied, swap (park self.arena, drop the previous occupant) so the current frame's arena always survives for the caller's ArenaResetGuard.
Extended reasoning...
What the bug is
ActiveTranspilerArena::drop parks self.arena back into transpile_source_code_arena only when slot.is_none(); otherwise it lets the Box drop, which is MimallocArena::drop → mi_heap_destroy. The new comment at jsc_hooks.rs:2125-2134 claims this is safe because "the caller's ArenaResetGuard … runs after process_fetch_log, so log spans pointing into arena-owned source bytes stay valid on the ParseError path" — but that statement only holds when the arena was actually parked. In the re-entrant case the slot is occupied, so the arena is destroyed instead, and the caller's ArenaResetGuard resets a different arena.
This is a regression introduced by removing the give_back_arena flag. Compare the three versions:
- Zig spec (ModuleLoader.zig:145-165): on ParseError,
give_back_arena = false(set at :293 and :339) makes the entiredeferblock a no-op — the arena is neither parked nordeinit()'d. It is intentionally leaked so its bytes outliveprocessFetchLog. The:161-163destroy branch the new comment cites is only reachable whengive_back_arena == true. - Old Rust port (removed by this PR): on
!give_back, did*slot = Some(arena); return;unconditionally — overwriting any occupant. The deleted comment explicitly documented why: "reads log entries whose spans point into arena-owned source bytes. Freeing here would be a use-after-free." - New Rust port: no
give_backflag; the sameif slot.is_none() { park } else { drop }logic now applies to all paths including ParseError. This matches neither the spec nor the old port on the parse-error path.
Code path that triggers it
transpile_source_code_inner is re-entrant — the spec says so explicitly at ModuleLoader.zig:130 ("This code is potentially re-entrant"). The concrete trigger is macro evaluation during parse: parse_maybe_return_file_only (jsc_hooks.rs:2468) → parser visits a macro call → MacroContext::call → vm.load_macro_entry_point → JSModuleLoader::load_and_evaluate_module → JSC → Bun__transpileFile → transpile_file (creates an ArenaResetGuard at jsc_hooks.rs:4210) → transpile_source_code_inner again on the same VM/thread.
The source bytes that log spans point into do live in this arena: transpile_source_code_inner calls parse_maybe_return_file_only::<false> → parse_maybe_return_file_only_allow_shared_buffer::<_, false> (transpiler.rs:1390), so USE_SHARED_BUFFER=false and the file is read via read_file_with_allocator(..., Some(arena)) (transpiler.rs:1502-1508). The PORT NOTE at transpiler.rs:1528-1533 confirms: "bytes live … in this_parse.arena … bulk-freed by mi_heap_destroy when the per-call arena is recycled".
Why existing code doesn't prevent it
The new comment at the else branch says "drop the fresh Box (spec :161-163)", assuming the dropped Box is always the inner re-entrant frame's freshly-allocated arena. But re-entry is stack-shaped, and the inner frame returns first — it parks its fresh arena (slot was None at that point) and its ArenaResetGuard resets it in place via as_mut() (ModuleLoader.rs:68-75), leaving slot = Some(arena_Y). When the outer frame later returns with a ParseError, its Drop is the one that sees the occupied slot — and the Box it drops is arena_X, the outer frame's source-bearing arena, not a "fresh" one.
Step-by-step proof
- Level 0:
transpile_filecreates_reset_arena_0: ArenaResetGuard(jsc_hooks.rs:4210), callstranspile_source_code_inner. Ittake()sarena_Xfrom the slot (slot →None), buildsarena_guard_0 = ActiveTranspilerArena { arena: Some(arena_X) }, and reads the file intoarena_X. - During parse/visit, a macro call evaluates and
import()s a module →Bun__transpileFile→ level 1transpile_filecreates_reset_arena_1, callstranspile_source_code_inner. Slot isNone, so it allocates fresharena_Yand buildsarena_guard_1. - Level 1 completes successfully.
arena_guard_1::dropseesslot.is_none()→ parksarena_Y(slot →Some(arena_Y))._reset_arena_1::dropcallsreset_arena, which doesslot.as_mut().reset_retain_with_limit(...)— slot staysSome(arena_Y). - Level 0's parse continues and hits an error (e.g. a syntax error after the macro call, or the macro itself logged an error so
log.errors > 0at jsc_hooks.rs:2546) →return Err(err!("ParseError")). - Level 0's locals drop.
arena_guard_0::dropseesslot = Some(arena_Y), so theif slot.is_none()branch is false →arena_Xfalls through and isBox::drop'd →mi_heap_destroy(heap_X). - Back in level 0's
transpile_file, theErrarm callsprocess_fetch_log(..., &mut log, ...)at jsc_hooks.rs:4290. This walkslog.msgsand callsBuildMessage::createper msg (VirtualMachine.rs:2553-2585), readingLocationline-text spans that point intoarena_X's now-destroyed heap. UAF. _reset_arena_0::dropthen resetsarena_Y— the wrong arena.
Impact
Use-after-free on the JS thread when a module that uses a bundle-time macro (or otherwise re-enters the transpiler during parse) subsequently fails to parse. The freed bytes are read to build the user-visible error message, so this would typically manifest as garbage in the diagnostic location/line-text or a crash, depending on whether mimalloc has unmapped the page. The trigger is narrow (macro re-entry + outer parse error) but reachable from user code without any special flags.
Fix
Restore the old !give_back invariant that the current frame's arena always survives for the caller's process_fetch_log + ArenaResetGuard. The minimal change is to swap rather than drop when the slot is occupied:
let slot = unsafe { &mut (*self.vm).module_loader.transpile_source_code_arena };
// Park this frame's arena unconditionally so log spans stay valid for
// process_fetch_log; if a re-entrant frame already parked one, drop *that*
// (it has already been reset by the inner ArenaResetGuard).
let _prev = core::mem::replace(slot, Some(arena));This preserves the PR's single-owner-reset goal (still no reset here) while matching the old port's parse-error semantics.
remap_zig_exception PrintSource-fetches the top frame's source into the per-module transpile arena; with reset_arena now retaining (8 MB cap), 100K Bun.inspect(error) calls accumulated dead AST and tripped inspect-error-leak.test.js (10-11 MB vs 10 MB threshold). Error display is one-off, not the hot transpile loop, so full-reset here.
2 participants
Closes #31113
The inner give-back in
transpile_source_codedidarena.reset_retain_with_limit(8M), then the outerArenaResetGuard(ModuleLoader::reset_arena) immediately did a fullarena.reset()— so the retain never stuck and every synchronously-transpiled module paidmi_heap_destroy + mi_heap_new. Two scopes had two inline reset bodies that diverged.Fix by making
ModuleLoader::reset_arenathe single owner of reset semantics (matchingModuleLoader.zig:24-29:smol → reset(), elsereset_retain_with_limit(8M)). The inner give-back stops resetting — it just clearsAST_HEAPand parks theBox; the caller'sArenaResetGuardhandles the reset afterprocess_fetch_log, so log spans stay valid on the parse-error path.Also replaces the 90-line
scopeguardclosure with a smallActiveTranspilerArenaDrop struct and drops the now-unusedgive_back_arenaflag.The 16 KB small-alloc bump layer in
ast_alloc.rsis untouched and now actually survives across modules (sinceset_thread_heapkeeps it on same-heap re-entry and the heap is no longer destroyed every module).Tests
require-cache.test.tscorrectness test (200 distinct modules, every export verified)runtime-transpiler.test.ts,transpiler-cache.test.tspassawait importof a 120K-node AST module with cache-bust: RSS bounded on both this branch andmain(overlapping ranges, no leak)rust:check-all10/10