bundler: stop re-interning module paths into FilenameStore on every Bun.build() - #31647
Conversation
…un.build() Repeated in-process `Bun.build()` calls panicked with `index out of bounds: the len is 4095 but the index is 4095` (SIGTRAP, exit 133) after ~2000 builds. `Path::dupeAlloc` interns every module path into the process-lifetime `FilenameStore`; the Rust port had dropped two things the Zig original does, so each build re-appended every path and the append-only store grew without bound until its overflow-block pointer array (fixed at 4095 blocks) went out of bounds. The Zig `dupeAlloc`: - Short-circuits via `isSliceInBuffer`: when `text`/`pretty` already point into a process-lifetime store (`FilenameStore` or `DirnameStore`), the slices are already `'static`, so it returns the path unchanged instead of appending a duplicate. - Takes an allocator and routes the disjoint `text`/`pretty` case — a freshly-relativized display path recomputed every build — into the per-build bundle arena, not the permanent store. Restore both in `Path::dupe_alloc`/`dupe_alloc_fix_pretty`, porting the four Zig branches faithfully and threading `BundleV2::arena()` through the call sites. The arena is reset per build and every path that escapes to JS is copied into an owned buffer first, so arena-backing the transient display path is safe. Verified: the 5000-build repro now completes with flat RSS; regression test in bun-build-api.test.ts crashes the pre-fix binary at ~build 270 and passes after.
|
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 (6)
WalkthroughPath interning now accepts an explicit per-build arena allocator. Resolver trait signatures and Path::dupe_alloc/dupe_alloc_fix_pretty were updated to use the arena and avoid permanently appending transient pretty/text buffers to the process FilenameStore. Bundler callsites were updated to pass their arenas, and a regression test ensures repeated Bun.build() calls no longer panic. ChangesFilename store arena allocation fix
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 12:52 AM PT - Jun 1st, 2026
❌ @robobun, your commit af729f6 has 2 failures in
The baseline build contains instructions not available on Static scan violations
|
StatusReproduced the SIGTRAP crash ( Root cause: the Rust port of Fix: restore both behaviors (faithful port of Zig's four Verified:
Waiting on CI. |
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 `@src/bundler/bundle_v2.rs`:
- Line 2471: The code calls path.dupe_alloc(self.arena()).expect("oom") inside
the path_as_static construction, which panics on OOM; replace the expect("oom")
with the repo OOM helper (e.g., call .unwrap_or_oom() on the Result returned by
dupe_alloc or pass the Result into bun_core::handle_oom) so allocation failures
are converted to the controlled OOM path; update both occurrences that use
path.dupe_alloc(self.arena()).expect("oom") (including the call that feeds
path_as_static) to use .unwrap_or_oom() or handle_oom() consistently.
In `@src/resolver/lib.rs`:
- Around line 569-578: Add a debug assertion before each early return that calls
self.into_static() to verify the namespace is also interned/static: check that
self.namespace is empty or "file" or that is_interned(self.namespace) is true,
so callers that produce interned text but a transient namespace will be caught
in debug builds; place this debug_assert immediately above the branches that
return Ok(unsafe { (*self).into_static() }) (the branches that currently check
core::ptr::eq(self.text.as_ptr(), self.pretty.as_ptr()) && self.text.len() ==
self.pretty.len() and the other similar early-return branches) so the invariant
is enforced without changing release behavior.
🪄 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: c68345a7-81cf-428f-b715-9b95689b0fba
📒 Files selected for processing (3)
src/bundler/bundle_v2.rssrc/resolver/lib.rstest/bundler/bun-build-api.test.ts
…nchanged The dupe_alloc short-circuit widens text/pretty/namespace to 'static via into_static(). The caller proves text/pretty are interned; add a debug_assert that namespace is also static (empty/"file"/store-interned) so a transient namespace would be caught in debug builds. Deduplicates the three early-return sites behind one checked closure.
bun:test's default per-test timeout is 5s, so an explicit timeout is required for this hundreds-of-builds test (as with the sibling leak tests); drop it from 300s to 180s to match the CI runner's own per-test ceiling. Extract BUILDS to a const and note the ~550-module recursion cliff the module count deliberately stays under. No behavior change.
Update — review feedback addressed
Also worth noting: while tuning the test I confirmed that going much higher than ~550 modules/build hits a separate, pre-existing stack overflow in the recursive tree-shaker ( All three review threads resolved. Gate fail-before (old binary panics, 3/3 runs) and pass-after (completes) both reconfirmed on the debug build. CI running. |
CI updateThe only red lane was
Pushed one |
CI: the failing lane is unrelated to this diff (concrete evidence)Pulled the
This looks like a pre-existing Windows-CRT I've used my one CI re-roll, so I'm not pushing further retriggers. Flagging for a maintainer: the change is ready and the red lane needs either an allowlist entry for |
CI follow-up — sharper read on the
|
…st-processing
Making generic_path_with_pretty_initialized honor its arena (prior commit)
turned LinkerContext::path_with_pretty_initialized into a cross-thread
allocation hazard: it passed self.arena() — the bundle-thread-owned
MimallocArena — but generate_isolated_hash runs on worker threads (via
generate_chunk -> post_process_{js,css,html}_chunk). MimallocArena asserts
single-thread ownership, so a non-aliased path reaching the arena branch
from a worker would panic (debug) or corrupt the heap (release).
Thread worker.arena() through generate_isolated_hash into
path_with_pretty_initialized, matching how generateCodeForFileInChunkJS
already handles the same 'pretty not computed' edge case. The bundle-thread
scan/enqueue callers in bundle_v2.rs keep self.arena() (correct thread).
Verified: bun-build-api (incl. the 500x400 regression test), splitting,
html and css bundler suites all pass.
Update — fixed a real cross-thread allocation bug the review caughtCommit af729f6: making Fix: thread the worker-local arena ( Verified: All four review threads addressed and resolved. This is a substantive code change (not a retrigger), so CI re-runs legitimately. |
CI status — both red lanes are unrelated flake; diff is green1. 2. Every lane that exercises this diff is green: the full bundler/resolver test coverage, the ASAN lane, and the 500×400 in-process-build regression test. CodeRabbit's latest pass reported no actionable comments, and all review threads are resolved. I've already used my one CI re-roll, so I'm not pushing further — handing off. A maintainer can re-run the flaky |
Repro
Repeated in-process
Bun.build()calls panic with an index-out-of-bounds and the process dies with SIGTRAP (exit 133) after ~2000 builds:RSS stays flat (~560 MB) the whole time — this is not a general heap leak, it's a fixed-capacity pointer array being overrun.
Cause
Path::dupeAllocinterns every module path'stext/prettyinto the process-lifetimeFilenameStore(aBSSStringList) so the returnedPathborrows'staticdata. The store is append-only and never freed. When its inline buffer (8192 slots) fills, entries spill into anOverflowGroupwhose block-pointer array is fixed-size; eventually the array is overrun and the bounds check panics.The Rust port of
dupeAllochad dropped two things the Zig original (src/resolver/fs.zig) does, so each build re-appended every path:isSliceInBuffershort-circuit. Zig checksFilenameStore.exists(text) or DirnameStore.exists(text)— if a slice already points into a process-lifetime store, it's already'static, so Zig returns the path unchanged instead of appending a duplicate. The port always appended (aPORT NOTEflagged it: "TYPE_ONLY shim … this always interns").dupeAlloctakes an allocator and, whentextandprettyare disjoint, allocates one combinedtext\0pretty\0buffer from the per-build arena (not the permanent store).prettyhere is a freshly-relativized display path (../../tmp/.../m0.js) recomputed every build and never a byte-subslice of the absolutetext, so it always hit this branch — and the port interned it intoFilenameStore, leaking one copy perBun.build()call.Fix
Restore both behaviors in
Path::dupe_alloc/dupe_alloc_fix_pretty, porting the four Zig branches faithfully:exists()short-circuit (checking bothFilenameStoreandDirnameStore, matching Zig) — already-interned paths are returned unchanged.BundleV2::arena()throughdupe_alloc/dupe_alloc_fix_prettyand allocate the disjointtext/prettybuffer (and the Windows posix-normalizedpretty) from it, not the store.The arena is reset per build, and every path that escapes to JS is copied into an owned buffer (
OutputFile::init→BuildArtifact.path: Box<[u8]>) before the arena is torn down, so arena-backing the transient display path is safe.Verification
test/bundler/bun-build-api.test.ts(500 modules × 400 in-process builds): the pre-fix binary panics with the exact reported message at ~build 270; the fixed binary printsOK 400and exits 0.test/bundler/bun-build-api.test.ts(46 pass),bundler_edgecase(103 pass),bundler_naming+bundler_splitting(21 pass),bundler_bun(10 pass) all green — including the existing sourcemap-leak test, confirming path/display output is unchanged.Relationship to #31504
#31504 fixes the allocator (
src/bun_alloc/lib.rs): an off-by-one inOverflowGroup::tailand a 32×-too-small overflow block size, which together let the store hold ~8.4M distinct names without panicking — the right fix for thebun --bun/Bun.resolveSynccase that genuinely interns that many distinct filenames.This PR fixes the resolver layer for the
Bun.build()case, where the same ~60 paths are re-interned every build. That's an unbounded-growth leak that #31504's larger ceiling would only delay (≈64k builds), not fix. The two are complementary and touch disjoint files.