Repository navigation
Conversation
|
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)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughThe runtime now stores text-format arenas per VM, supports safe arena reuse across reentrant calls, documents process-lifetime arena retention, and adds worker-thread tests that verify zero leaked mimalloc heaps. ChangesText format arena recycling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. Rebased onto main at Reproduced on canary The new test ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches per-VM allocator lifecycle and adds an unsafe accessor into RuntimeState, a human look would still be worthwhile.
What was reviewed:
text_format_arena_slot()follows the same raw-pointer pattern asglobal_dns_data();deinit_runtime_statenullsRUNTIME_STATEbefore dropping the box, so the SAFETY comment holds.- Drop order in
with_text_format_source_encoded:_ast_scope→ast_memory_allocator→recycle, so the arena is unborrowed whenRecycle::dropparks it. Recycle::dropre-fetches the slot rather than holding the entry-time reference; if VM state is gone the arena is dropped (mi_heap_destroy).- Test asserts an exact live-heap count via
mimallocDump.heaps.lengthafter a warm-up worker, not an RSS threshold.
Extended reasoning...
Overview
This PR fixes a per-Worker leak of one mi_heap_t (plus up to 2 MiB of retained pages) introduced by #37146. The parked arena for Bun.{TOML,YAML,JSON5,JSONC,XML}.parse moves from a #[thread_local] static in src/runtime/api.rs into a new RuntimeState::text_format_arena field in src/runtime/jsc_hooks.rs, so it is dropped by deinit_runtime_state on Worker teardown. A new accessor text_format_arena_slot() exposes the Cell slot; the Recycle guard in api.rs takes/parks through it. The comment in ast_memory_allocator.rs is corrected to describe mimalloc's actual behavior (heaps are not destroyed on thread exit) and why TLS parking is still acceptable there. A new fixture and test assert that 3 Workers exercising all five parsers plus Bun.Transpiler and a data: import leave the process-wide live-heap count unchanged.
Security risks
None. This is internal allocator-lifecycle plumbing with no user-facing input handling, auth, or crypto.
Level of scrutiny
Moderate-to-high. The change is small (~45 net LoC in src/) and mirrors the established transpiler_arena pattern in the same struct, but it lives in the "most-blocked category" per REVIEW.md: native memory safety, per-VM state ownership, and an unsafe block returning &'static to a field of a heap box. I traced the invariants: deinit_runtime_state (jsc_hooks.rs:693) nulls RUNTIME_STATE before heap::take drops the box, so text_format_arena_slot()'s null-check guarantees the pointer is live — matching the SAFETY comment and the neighboring global_dns_data() accessor. Drop order in the caller is correct (the ASTMemoryAllocator::borrowing and its Scope drop before recycle, releasing the borrow of arena before it is reset/parked). The re-entrancy story (argument toString() calling parse again) is unchanged from the previous Cell::take behavior and is documented. Re-fetching the slot in Recycle::drop rather than capturing it means a dangling reference is impossible even if VM teardown somehow ran between entry and exit.
Other factors
The test is deterministic (exact heap count via heapStats({ dump: true }).mimallocDump.heaps.length, not an RSS delta), runs a warm-up Worker to establish a baseline, wires error to reject, and follows the harness conventions (bunEnv, bunExe, concurrent pipe drain, exitCode asserted last). The PR description states it fails on the unfixed binary with leaked: 3. The fixture also exercises Bun.Transpiler and data: import to guard the sibling per-VM heaps in the same test. The corrected ast_memory_allocator.rs comment is accurate and points future readers at the right pattern for short-lived threads.
I found nothing wrong. Deferring only because per-VM allocator lifecycle with new unsafe is exactly the category REVIEW.md flags for careful eyes, not because of any specific concern.
|
Updated 11:48 PM PT - Aug 22nd, 2026
✅ @autofix-ci[bot], your commit 6700e941f59ee5478bee19061ef97738f5333281 passed in 🧪 To try this PR locally: bunx bun-pr 38999That installs a local version of the PR into your bun-38999 --bun |
…er exits
with_text_format_source_encoded kept the recycled mi_heap in a
mimalloc fork's heaps are not thread-bound: thread exit only abandons the
theap pages, so every Worker that parsed a document leaked one mi_heap_t
plus up to 2 MiB of retained pages.
Move the slot onto RuntimeState, which is per JS thread and dropped by
deinit_runtime_state during VM teardown, so the heap dies with the Worker.
Main-thread behaviour is unchanged: one heap is parked and reused across
calls, and a re-entrant call still gets its own arena.
Add a worker_destruction test that counts live mimalloc heaps via
heapStats({ dump: true }) across Worker lifecycles, and correct the
ast_memory_allocator comment the thread-local design was copied from.
0c25551 to
222200a
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
worker_destruction.test.ts runs its tests concurrently and the existing ones start 50 Workers each, so on debug and ASAN builds every test in the file needs far more than the default per-test timeout. Give the new heap-count test its own file next to its fixture, with the per-test ceiling a four-Worker debug run needs.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix follows the existing RuntimeState per-VM ownership pattern (same shape as transpiler_arena / global_dns_data()), and the re-entrancy and drop-order behavior in with_text_format_source_encoded is preserved. Since it changes native allocator lifecycle and adds an unsafe &'static accessor into the per-VM state box, a human look at the memory-safety reasoning would still be worthwhile.
Checked: Recycle::drop re-fetches the slot (so a torn-down VM just drops the arena rather than dereferencing a freed box); slot.set() on outer drop after a re-entrant inner call correctly drops the inner's parked arena instead of leaking it; ASTMemoryAllocator::borrowing never resets the borrowed arena, so Recycle is the sole owner of reset/park; the test's heap-count assertion is exact and deterministic (not RSS-based) with error/exit wired to reject.
Extended reasoning...
Overview
This PR fixes a per-Worker leak of a mimalloc heap introduced by #37146. The recycled arena for Bun.{TOML,YAML,JSON5,JSONC,XML}.parse was parked in a #[thread_local] under the assumption that mimalloc destroys heaps at thread exit; it does not, so every Worker that touched one of these parsers stranded a mi_heap_t plus up to 2 MiB of retained pages. The fix moves the parked arena onto RuntimeState (the per-VM box already used for transpiler_arena and other per-thread allocator state), which is dropped by deinit_runtime_state during VirtualMachine::destroy on the Worker's own thread. api.rs now takes the arena from the slot at call start and re-fetches the slot in Recycle::drop to park it back; ast_memory_allocator.rs gets a comment-only correction. A new subprocess test asserts an exact live-heap count via heapStats({dump: true}).mimallocDump.heaps.length across four Workers.
Security risks
None. This is internal allocator lifecycle management with no user-facing input handling, auth, or crypto surface. The one new unsafe block dereferences the per-thread RuntimeState pointer to hand out &'static Cell<Option<Arena>> — the same pattern the immediately-preceding global_dns_data() uses, with the same SAFETY justification (address stable until deinit_runtime_state nulls RUNTIME_STATE before freeing the box).
Level of scrutiny
High. This is native memory-management code in the most-blocked review category: it changes where an allocator heap is owned, adds an unsafe accessor with a 'static lifetime that is only true until VM teardown, and interacts with Worker shutdown ordering. The change is small and follows an established file-local pattern exactly, but allocator-lifecycle mistakes here manifest as UAF or cross-heap frees rather than compile errors, so a maintainer should confirm the ownership reasoning — particularly that deinit_runtime_state always runs on the Worker's own thread after all JS host calls have unwound (so no Recycle guard can be live when the box is freed), and that dropping the Cell<Option<Arena>> on that thread is the correct place for mi_heap_destroy to run.
Other factors
I verified the behavior-preserving details: local drop order in with_text_format_source_encoded still resets/parks the arena only after _ast_scope and the borrowing ASTMemoryAllocator have dropped; ASTMemoryAllocator::Drop returns early for borrowing instances so it never touches the caller's arena; re-entrant calls (via argument toString()) get a fresh arena because the slot is emptied for the duration, and the outer slot.set() on unwind drops any inner-parked arena rather than leaking it — matching the old Cell::set semantics. The test drains stdout/stderr/exited concurrently, wires error/exit to reject, uses a warm-up Worker for the baseline, and asserts an exact JSON payload rather than a threshold. The comment-cop bot's inline notes were all addressed (trimmed in d62e2d2) and resolved. No prior human review comments are outstanding.
This branch has not been deployed
2 participants
Problem
Bun.TOML.parse,Bun.YAML.parse,Bun.JSON5.parse,Bun.JSONC.parseorBun.XML.parseleaks one mimalloc heap (mi_heap_t) plus whatever pages it still holds (up to 2 MiB of retained blocks) when the Worker exits. 20 Workers each parsing a 2000-table TOML document: 20 heaps left behind, RSS +55 MB vs +14 MB for 20 Workers that parse nothing (debug build; release canaryeabb96de7shows the same 20 heaps, RSS +40 MB vs +2 MB).with_text_format_source_encoded(src/runtime/api.rs:272) parks the recycled arena in a#[thread_local]and relies on "a parked heap is reclaimed with the thread". That is not how our mimalloc fork behaves: heaps are first-class and not bound to a thread, so thread exit (mi_thread_theaps_donein mimalloc'sinit.c) only abandons the thread's theap pages and never destroys heaps created on it. Themi_heap_t, its TLS key slot, and every page still holding a retained block outlive the Worker.Arena::new()with the per-thread cache.ast_memory_allocator.rs, which that comment cites as precedent, makes the same claim; it is harmless there only because the threads parking into it (bundler/install pool threads, the bundle thread) live as long as the process.Fix
RuntimeState::text_format_arena(src/runtime/jsc_hooks.rs).RuntimeStateis the per-JS-thread box that already owns the other per-VM allocator state for exactly this reason (transpiler_arena, the transpile printer, the AST stores) and is dropped bydeinit_runtime_statefromVirtualMachine::destroy, so the heap is destroyed on the Worker's thread before it exits. The main thread's state lives for the process, as before.api.rstakes the arena out of the slot for the duration of a call and parks it back on exit, re-fetching the slot at park time. Behaviour on a live VM is identical to before (verified with a probe counting live heaps: exactly one parked heap after 150 mixed parse calls, and a re-entrant call made from the argument'stoString()still ends with one parked heap); with no VM state on the thread the arena is simply dropped.ast_memory_allocator.rsis replaced with what mimalloc actually does and when a TLS-parked heap is acceptable.test/js/node/worker_threads/worker-heap-leak.test.ts, drivingworker-heap-leak-fixture.js: runs a warm-up Worker, recordsheapStats({ dump: true }).mimallocDump.heaps.length, runs 3 more Workers that exercise all five parsers plusBun.Transpilerand adata:import, and expects the live heap count to be unchanged. Fails on the unfixed binary withleaked: 3, passes with this change.bun bd test test/js/node/worker_threads/worker-heap-leak.test.tspasses;worker_destruction.test.ts(the file the fixture pattern comes from) still passes, 6 of 6.Background
bun_alloc::Arena(MimallocArena) wraps onemi_heap_t; itsDrop/reset()callmi_heap_destroy, which bulk-frees everything allocated from it. The text-format parsers build their AST in one of these and throw the whole thing away per call.reset_retain_with_limit(2 MiB)keeps the heap, including its dead blocks, while it is small, to skipmi_heap_new/mi_heap_destroyon the next call; that is why a parked heap can hold up to 2 MiB of pages.heapStats({ dump: true })frombun:jsccallsmi_heap_dump_json, which lists every livemi_heap_tin the process regardless of whether it has pages, so the length ofmimallocDump.heapsis an exact live-heap count and a cheap way to detect this class of leak for any API.RuntimeState(src/runtime/jsc_hooks.rs) is the high-tier per-VM state thatbun_jsccannot name directly;bun_jscstores it as an opaque pointer, creates it inVirtualMachine::initand reclaims it inVirtualMachine::destroy, which runs on the Worker's own thread during shutdown (web_worker.rs). Worker threads are the only JS threads that exit while the process keeps running, so anything per-thread that must not outlive the thread belongs there.[review] gate passed · iteration 5 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 5
evidence per changed file