router: dedupe DirnameStore interns so FileSystemRouter.reload() does not exhaust the store - #34276
router: dedupe DirnameStore interns so FileSystemRouter.reload() does not exhaust the store#34276robobun wants to merge 7 commits into
Conversation
…not exhaust the store
|
Status: ready for review; diff is green. Reproduced with 800 .tsx files in a tight CI (build #73518):
All other annotations are known flakes that passed on retry. The |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRoute parsing now deduplicates route path strings through thread-local interning helpers, including lowercase variants. Test-only append-count instrumentation and a reload regression test verify bounded storage growth and route matching. ChangesRoute path interning
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:50 PM PT - Jul 15th, 2026
❌ @robobun, your commit 66949bd has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34276That installs a local version of the PR into your bun-34276 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding on the test setup, I checked the Rust side: intern_route_path is a faithful copy of the established intern_transpile_path pattern in src/runtime/jsc_hooks.rs:4061 (same exists() pointer-range fast path, same thread-local content-dedup map, handle_oom on append), and the exists() SAFETY comment matches src/resolver/fs.rs:103. The thread-local dedup map is per-worker-thread, so each worker appends once per path — bounded, same as the reference pattern.
Extended reasoning...
The inline comment covers the test's macOS PATH_MAX / Windows path.join backslash-key issue, which will break CI on those platforms. The Rust change itself is a straightforward transplant of an existing in-tree pattern and I found no issues with it — noting that here so a follow-up pass doesn't re-derive it.
The fixture needs ~1500-byte absolute paths so the leaked abs_path interns dominate RSS over the unrelated per-reload DirEntry churn; macOS PATH_MAX=1024 cannot fit it, and 200*400 file opens through Windows is too slow for CI. Also build the tempDir keys with forward slashes so the harness creates parent dirs on any platform. The leak and fix are platform-independent (Route::parse).
![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/router/lib.rs`:
- Around line 1368-1403: Update intern_route_path and
intern_route_path_lower_case to reuse a shared route/path interning helper with
intern_transpile_path from jsc_hooks.rs, rather than maintaining duplicate
interning logic in the router. Preserve content deduplication, process-lifetime
storage, and lowercase handling while keeping the existing callers’ behavior
unchanged.
🪄 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: fe4e911b-a34f-48dd-a006-c6aa821284a5
📒 Files selected for processing (2)
src/router/lib.rstest/js/bun/util/filesystem_router.test.ts
There was a problem hiding this comment.
My earlier macOS/Windows test-setup concern is addressed (test is now it.skipIf(!isLinux) with forward-slash tempDir keys and a documented rationale), and no new issues were found this pass. Deferring to a human because the fix adds unsafe lifetime-widening around the process-global DirnameStore — it mirrors the existing intern_transpile_path pattern and I verified BSSStringList::exists is the pointer-range check the SAFETY comment claims (src/bun_alloc/lib.rs:2767), but memory-lifetime changes to a shared intern store warrant a maintainer look.
Also checked and ruled out: intern_route_path_lower_case's scratch buffer cannot escape (the exists() fast-path only fires on store-resident input, and the caller is gated on has_uppercase); .expect("unreachable") → bun_core::handle_oom matches repo policy; the thread-local dedup map matches TRANSPILE_PATH_INTERN in jsc_hooks.rs.
Extended reasoning...
Overview
Replaces five direct DirnameStore::append / append_lower_case calls in Route::parse (src/router/lib.rs) with two new helpers, intern_route_path and intern_route_path_lower_case, that dedupe by content via a thread-local HashMap<&'static [u8], ()> before appending. Adds a Linux-gated RSS-growth regression test to test/js/bun/util/filesystem_router.test.ts.
Security risks
None identified. No user-controlled input reaches new parsing logic; the change only alters where already-computed route path bytes are stored. The unsafe block widens a slice to 'static only after BSSStringList::exists (a pointer-range check against the process-lifetime backing_buf, verified at src/bun_alloc/lib.rs:2767-2775) confirms the bytes already live there — the same widen intern_transpile_path and FilenameStore::append_slice themselves perform.
Level of scrutiny
Medium-high. This is a targeted leak/panic fix, not a mechanical change: it introduces unsafe lifetime code and a new thread-local around a process-global store. The pattern is a near-verbatim copy of intern_transpile_path (src/runtime/jsc_hooks.rs:4061-4082), which reduces risk, but per repo review norms memory-lifetime changes to shared intern stores are the most-blocked category and deserve a human signoff.
Other factors
- Prior review addressed: my earlier finding (test would fail on macOS
PATH_MAX=1024and Windows backslash keys) was fixed in 5a97db1 by gating to Linux and switching to.join("/")keys; the author explained the 14-segment depth is needed for a robust 4× RSS margin over the unrelated #29919 DirEntry churn. ThetempDirfixture setup now runs entirely inside theskipIf(!isLinux)body, so macOS/Windows never construct the long paths. - CodeRabbit's "share the helper" nit was reasonably declined on crate-layering grounds (
bun_routersits belowbun_runtime) and marked resolved. copy_lowercasescratch: verified the stack/heap scratch cannot leak — it never satisfiesexists()(pointer-range), so it always routes through the append-and-record path which copies into the store.- RSS-threshold test: 110 MB threshold vs. 57 MB observed under debug/ASAN gives ~2× headroom, matching repo guidance for RSS thresholds. Still inherently coarser than a counter-based assertion, so worth a maintainer glance.
Replace the RSS-threshold test with a direct assertion on the DirnameStore append count exposed through bun:internal-for-testing (same pattern as sslCtxLiveCount). The RSS proxy was Linux-only, coupled to an unrelated open DirEntry leak (#29919), and carried a provably-unfailable AllocError check. Also drop the exists() fast path from intern_route_path: every call site passes bytes from a thread-local buffer, the filename store, or Entry inline storage, so the pointer-range check against DirnameStore's backing buffer is never true.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/router/lib.rs`:
- Around line 1384-1385: Update intern_route_path to replace
FileSystem::instance().dirname_store() with the shared
FileSystem::get().dirname_store() accessor, preserving the existing handle_oom
and append flow.
In `@test/js/bun/util/filesystem_router.test.ts`:
- Around line 438-443: Update the append-count assertion for the Route::parse
reload test so its upper bound scales only with reloads, using a small fixed
allowance of approximately six appends per reload rather than routes * reloads.
Preserve the existing delta measurement and ensure the expectation remains
independent of the route count.
- Around line 420-422: Replace the local require of dirnameStoreAppendCount with
a module-scope import from bun:internal-for-testing, keeping the test’s existing
binding usage unchanged.
🪄 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: fda66018-f928-4214-b9dc-42d37c1e3436
📒 Files selected for processing (6)
src/codegen/generate-js2native.tssrc/js/internal-for-testing.tssrc/resolver/lib.rssrc/router/lib.rssrc/runtime/api/filesystem_router.rstest/js/bun/util/filesystem_router.test.ts
…-scope import for the counter
There was a problem hiding this comment.
No issues found in the latest revision — both earlier test-file concerns are addressed and the counter-based test is now portable. Deferring to a maintainer since this is a memory-management change to a process-global intern store spanning three crates plus codegen.
Reviewed: the intern_route_path / intern_route_path_lower_case dedup wrappers (thread-local map keyed by DirnameStore-backed &'static [u8], so scratch buffers don't leak into keys); all five Route::parse call sites now route through them; .expect("unreachable") → handle_oom; append_count() reads under the store mutex; the new test uses forward-slash tempDir keys and no deep nesting, so the earlier macOS/Windows setup failures no longer apply.
Extended reasoning...
Overview
Fixes an unbounded-growth leak where FileSystemRouter.reload() re-appends every route's public path, absolute path, basename, and lowercased match name into the process-global DirnameStore on each reload, eventually exhausting the store's slot capacity and panicking with AllocError. The fix wraps all five DirnameStore::append sites in Route::parse with a content-deduplicating helper backed by a thread-local HashMap<&'static [u8], ()>, mirroring the existing intern_transpile_path pattern in jsc_hooks.rs. Supporting changes: an append_count() accessor on BSSStringList (resolver macro), a jsDirnameStoreAppendCount host fn wired through generate-js2native.ts / internal-for-testing.ts, and a regression test in filesystem_router.test.ts.
Security risks
None identified. No user-controlled input reaches new parsing or allocation logic; the dedup map is keyed by DirnameStore-backed &'static slices (not the caller's scratch buffer), so there's no dangling-key hazard. The new append_count binding is test-only and read-only under the store's existing mutex.
Level of scrutiny
This warrants maintainer review. It's a correct-looking, pattern-following fix, but it introduces new process-lifetime state (a thread-local HashMap that grows with unique route paths) and changes error handling on a hot path in the router. The change spans bun_router, bun_resolver (macro edit), bun_runtime, and codegen. The author already noted a follow-up refactor (lifting the dedup helper into bun_resolver where both FilenameStore/DirnameStore live) that a maintainer may want to weigh in on now vs. later.
Other factors
Two earlier rounds of feedback on the test fixture (macOS PATH_MAX / Windows tempDir backslash keys, and dynamic require) were addressed; the test was subsequently reworked from an RSS-threshold check to a direct append-count assertion (commit 3293348), which drops the deep-nesting fixture and the Linux gate — I confirmed the current test uses forward-slash keys and short paths, so it should run on all platforms. The FileSystem::instance() → FileSystem::get() change and module-scope import were applied in ea9f1e4. rust:check-all and the full filesystem_router.test.ts suite are reported passing; CI build #73467 is in progress.
There was a problem hiding this comment.
LGTM — prior feedback (test platform portability, module-scope import, FileSystem::get() shared accessor) is addressed and CI is green.
Reviewed: intern_route_path mirrors the established intern_transpile_path pattern (thread-local content-keyed dedup over a process-lifetime store); intern_route_path_lower_case copies into stack/heap scratch before interning so no borrow escapes. append_count() reads under the store's mutex. The $newRustFunction / codegen wiring follows sslCtxLiveCount exactly. Confirmed copy_lowercase returns a slice into the caller-provided buffer of the input's length, so the 256-byte stack scratch and vec[0u8; value.len()] fallback are both correctly sized.
Extended reasoning...
Overview
Fixes an unbounded-growth leak in Bun.FileSystemRouter.reload(): Route::parse interns 4–5 path strings per route into the process-global DirnameStore on every reload, and BSSStringList::append does not dedupe by content, so the store's slot capacity (~8.4M appends) is eventually exhausted and the process panics with unreachable: AllocError. The fix wraps all DirnameStore::append sites in Route::parse with a thread-local content-keyed HashMap<&'static [u8], ()> so each distinct path is appended once. Touches: src/router/lib.rs (the fix + lower-case variant), src/resolver/lib.rs (test-only append_count() accessor on the bss_store! macro), src/runtime/api/filesystem_router.rs + src/js/internal-for-testing.ts + src/codegen/generate-js2native.ts (expose the counter to JS via the standard $newRustFunction path), and a regression test in filesystem_router.test.ts.
Security risks
None. No user-controlled input reaches new parsing/allocation logic; the change only routes existing append calls through a dedup map keyed by content. The new append_count() accessor is read-only, mutex-guarded, and reachable only via bun:internal-for-testing.
Level of scrutiny
Moderate — native Rust touching process-global storage and &'static [u8] lifetime widening. However, the pattern is a near-verbatim copy of intern_transpile_path in src/runtime/jsc_hooks.rs (same thread-local HashMap<&'static [u8], ()>, same get_key_value/insert shape, same store-backed 'static widen), which has been in production for the same bug class on FilenameStore. The lower-case helper's scratch-buffer lifetime was checked: copy_lowercase returns a borrow into the caller's buffer, and intern_route_path copies it into the store before the scratch drops. The thread-local map is per-thread, but FileSystemRouter construction/reload() runs only on the JS thread, and the underlying DirnameStore::append is already mutex-protected — so worst case across threads is per-thread dedup, not a correctness issue (same trade-off as intern_transpile_path).
Other factors
This PR has been through two prior review rounds. My earlier findings (macOS PATH_MAX / Windows tempDir key-separator issues in the first test iteration; dynamic require() vs module-scope import) were addressed in 5a97db1 / 3293348 / ea9f1e4. CodeRabbit's FileSystem::instance() → FileSystem::get() shared-ref suggestion was applied; its "share the helper with intern_transpile_path" and "tighten the test bound" suggestions were declined with sound reasoning (crate-layering constraint; avoiding coupling the test threshold to an unrelated resolver-side residual). All threads are resolved. robobun reports filesystem_router.test.ts green on every CI lane; the one hard failure is a pre-existing unrelated break on main. The test asserts the append-count delta over 50 reloads stays below routes * reloads (2000), which catches a single-site regression (41 × 50 = 2050) while staying decoupled from the ~6/reload resolver-side residual. The bug-hunting system found no issues on the current revision.
1 participant
Repro
Panics after ~3400 reloads (~13s, RSS ~1 GB):
Cause
Route::parseinterns each route's public path, absolute path, basename, and (when the name has uppercase) lowercased match name into the process-globalDirnameStore.BSSStringList::appenddoes not deduplicate by content, so everyreload()re-appends identical strings for every route: ~4 appends per file per reload on POSIX, ~5 on Windows. The store'sslice_buf(4096 slots) plus overflow list (4096 blocks x 2048) cap out at ~8.4M appends, after whichappendreturnsAllocErrorand the.expect("unreachable")panics. Before that, every append past the first ~528KB of bytes heap-allocates a never-freed buffer, so RSS climbs unboundedly.This is the same bug class as the one
intern_transpile_path(src/runtime/jsc_hooks.rs) already fixed forFilenameStore.Fix
Route all
DirnameStoreappends inRoute::parsethrough a content-deduplicating wrapperintern_route_path: a thread-localHashMap<&'static [u8], ()>keyed by content returns the previously interned slice on hit, and appends once on miss.intern_route_path_lower_caselowercases into a stack scratch (heap only for >256 bytes) and delegates to the same dedup, replacing the oneappend_lower_casesite. After the first load, every subsequentreload()over an unchanged directory performs zero newDirnameStoreappends from the router.The
.expect("unreachable")calls becomebun_core::handle_oomper repo convention.Verification
dirnameStoreAppendCount()helper exposed viabun:internal-for-testing(same pattern assslCtxLiveCount): readsslice_buf_used + overflow_list.countfrom theDirnameStoresingleton under its mutex.reload() does not leak route paths into the process-global intern storeintest/js/bun/util/filesystem_router.test.ts: 40 routes plus one nested directory, 50reload()s, asserts the append-count delta stays belowroutes * reloadsand thatmatch()still resolves after the loop. Without the fix the delta is ~8000 (4 appends per route per reload); with it, ~300 (O(dirs), a resolver-side residual handed off separately). Runs in ~180 ms on all platforms.for(;;) router.reload()) panics on the canary after ~3400 iterations and runs indefinitely with the fix applied.test/js/bun/util/filesystem_router.test.tssuite: 30/30 pass.bun run rust:check-all(all targets, including the Windowscfgarm) andcargo clippy -p bun_router -p bun_resolverare clean.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/filesystem_router.test.ts