diff --git a/src/codegen/generate-js2native.ts b/src/codegen/generate-js2native.ts index 2143557ef408..68195acc606e 100644 --- a/src/codegen/generate-js2native.ts +++ b/src/codegen/generate-js2native.ts @@ -59,6 +59,7 @@ const rustIdentifierPaths: Record = { "escapeRegExp.rs": "string/escapeRegExp.rs", "event_loop.rs": "jsc/event_loop.rs", "ffi.rs": "runtime/ffi/ffi.rs", + "filesystem_router.rs": "runtime/api/filesystem_router.rs", "h2_frame_parser.rs": "runtime/api/bun/h2_frame_parser.rs", "hosted_git_info.rs": "install/hosted_git_info.rs", "http/H2Client.rs": "http/H2Client.rs", diff --git a/src/js/internal-for-testing.ts b/src/js/internal-for-testing.ts index 292a5c598fb6..0b49d6ea6019 100644 --- a/src/js/internal-for-testing.ts +++ b/src/js/internal-for-testing.ts @@ -52,6 +52,8 @@ const shellParse = $newRustFunction("shell.rs", "TestingAPIs.shellParse", 2); export const sslCtxLiveCount = $newRustFunction("SecureContext.rs", "jsLiveCount", 0); +export const dirnameStoreAppendCount = $newRustFunction("filesystem_router.rs", "jsDirnameStoreAppendCount", 0); + export const napiThreadsafeFunctionLiveCount = $newRustFunction("napi_body.rs", "jsThreadsafeFunctionLiveCount", 0); export const escapeRegExp = $newRustFunction("escapeRegExp.rs", "jsEscapeRegExp", 1); diff --git a/src/resolver/lib.rs b/src/resolver/lib.rs index 7c33992c09f0..c8987c64cdf0 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -122,6 +122,15 @@ pub mod fs { // SAFETY: see `append_slice`. unsafe { &*$backing() }.exists(value) } + /// Total number of `append*` calls (inline + overflow). + /// Exposed via `bun:internal-for-testing` so leak tests can + /// assert "N reloads performed zero new appends". + pub fn append_count(&self) -> u32 { + // SAFETY: see `append_slice`. + let b = unsafe { &*$backing() }; + let _g = b.mutex.lock(); + b.slice_buf_used as u32 + b.overflow_list.count + } } }; } diff --git a/src/router/lib.rs b/src/router/lib.rs index 797365348028..1a7a6fd5261c 100644 --- a/src/router/lib.rs +++ b/src/router/lib.rs @@ -1123,18 +1123,14 @@ impl Route { let name_offset = name.as_ptr() as usize - public_path.as_ptr() as usize; let name_len = name.len(); - // NOTE: DirnameStore::append returns `&'static [u8]` (process- + // NOTE: `intern_route_path` returns `&'static [u8]` (process- // lifetime arena), so rebinding here drops the borrow on // `route_file_buf` and avoids needing lifetime transmutes // below. - let dirname_store = FileSystem::instance().dirname_store(); - let public_path: &'static [u8] = - dirname_store.append(public_path).expect("unreachable"); + let public_path: &'static [u8] = intern_route_path(public_path); let name: &'static [u8] = &public_path[name_offset..][0..name_len]; let match_name: &'static [u8] = if has_uppercase { - dirname_store - .append_lower_case(&name[1..]) - .expect("unreachable") + intern_route_path_lower_case(&name[1..]) } else { &name[1..] }; @@ -1143,9 +1139,7 @@ impl Route { debug_assert!(name[0] == b'/'); (public_path, name, match_name) } else { - let dirname_store = FileSystem::instance().dirname_store(); - let public_path: &'static [u8] = - dirname_store.append(public_path).expect("unreachable"); + let public_path: &'static [u8] = intern_route_path(public_path); ( public_path, Route::INDEX_ROUTE_NAME, @@ -1231,10 +1225,7 @@ impl Route { } }; - abs_path_str = FileSystem::instance() - .dirname_store() - .append(_abs) - .expect("unreachable"); + abs_path_str = intern_route_path(_abs); // SAFETY: sole mutation; `base_`/`extname` (which may borrow // `(*entry).base_.remainder_buf`) are not used after this. @@ -1249,11 +1240,7 @@ impl Route { abs_path_str, &mut bufs.normalized_abs_path_buf, ); - let interned: &'static [u8] = FileSystem::instance() - .dirname_store() - .append(normalized) - .expect("unreachable"); - Interned::from_static(interned) + Interned::from_static(intern_route_path(normalized)) }; #[cfg(not(windows))] let abs_path = Interned::from_static(abs_path_str); @@ -1269,14 +1256,11 @@ impl Route { } // NOTE: name/match_name/public_path are already `&'static` via - // DirnameStore::append above. `entry.base()` borrows the entry (it + // `intern_route_path` above. `entry.base()` borrows the entry (it // may be inline-stored for ≤31-byte names); intern it // explicitly to get `&'static` without a lifetime transmute. // SAFETY: read-only reborrow; the `&mut` write above is dead. - let basename: &'static [u8] = FileSystem::instance() - .dirname_store() - .append(unsafe { &*entry }.base()) - .expect("unreachable"); + let basename: &'static [u8] = intern_route_path(unsafe { &*entry }.base()); Some(Route { name, @@ -1381,6 +1365,37 @@ thread_local! { #[cfg(windows)] normalized_abs_path_buf: bun_sys::windows::PathBuffer::ZEROED, })); + + static ROUTE_PATH_INTERN: RefCell> = + RefCell::new(bun_collections::HashMap::new()); +} + +/// Intern `value` into the process-lifetime `DirnameStore`, deduplicated by +/// content. `BSSStringList::append` does not dedupe, so without this every +/// `FileSystemRouter::reload()` would re-append each route's public path, +/// absolute path and basename, eventually exhausting the store's slot capacity +/// (`AllocError`) and leaking one heap buffer per append. Same pattern as +/// `intern_transpile_path` in `jsc_hooks.rs`. +fn intern_route_path(value: &[u8]) -> &'static [u8] { + ROUTE_PATH_INTERN.with_borrow_mut(|set| { + if let Some((interned, ())) = set.get_key_value(value) { + return *interned; + } + let interned: &'static [u8] = + bun_core::handle_oom(FileSystem::get().dirname_store().append(value)); + set.insert(interned, ()); + interned + }) +} + +fn intern_route_path_lower_case(value: &[u8]) -> &'static [u8] { + if value.len() <= 256 { + let mut scratch = [0u8; 256]; + intern_route_path(strings::copy_lowercase(value, &mut scratch[..value.len()])) + } else { + let mut scratch = vec![0u8; value.len()]; + intern_route_path(strings::copy_lowercase(value, &mut scratch)) + } } pub struct Match<'a> { diff --git a/src/runtime/api/filesystem_router.rs b/src/runtime/api/filesystem_router.rs index b8de69852189..460bed4f7e5e 100644 --- a/src/runtime/api/filesystem_router.rs +++ b/src/runtime/api/filesystem_router.rs @@ -53,6 +53,18 @@ pub use crate::bake::framework_router::JSFrameworkRouter as FrameworkFileSystemR pub(crate) const DEFAULT_EXTENSIONS: &[&[u8]] = &[b"tsx", b"jsx", b"ts", b"mjs", b"cjs", b"js"]; +/// Exposed via `bun:internal-for-testing` so leak tests can assert +/// `reload()` performed zero new `DirnameStore` appends. +#[bun_jsc::host_fn] +pub fn js_dirname_store_append_count( + _global: &JSGlobalObject, + _callframe: &CallFrame, +) -> JsResult { + Ok(JSValue::js_number( + Fs::DirnameStore::instance().append_count() as f64, + )) +} + // ── local shims ─────────────────────────────────────────────────────────── // `to_js` lives on the `bun_jsc::ZigStringJsc` extension trait; `from_bytes` // auto-detects UTF-8. diff --git a/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index 65d3390e3293..e74cd164ce97 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -1,4 +1,5 @@ import { FileSystemRouter } from "bun"; +import { dirnameStoreAppendCount } from "bun:internal-for-testing"; import { expect, it } from "bun:test"; import fs, { mkdirSync, rmSync } from "fs"; import { bunEnv, bunExe, isASAN, isMacOS, isWindows, normalizeBunSnapshot, tempDir, tmpdirSync } from "harness"; @@ -402,6 +403,37 @@ it("reload() works", () => { expect(router.match("/posts")!.name).toBe("/posts"); }); +it("reload() does not leak route paths into the process-global intern store", () => { + // Route::parse interns each route's public path, absolute path, basename and + // lowercased match name into the never-freed DirnameStore. Without content + // dedup every reload() re-appended identical strings, leaking one heap + // buffer per append and eventually panicking with `unreachable: AllocError` + // when the store's slot capacity was exhausted. + const routes = 40; + const reloads = 50; + const files: Record = { "pages/sub/Nested.tsx": "export default 1;\n" }; + for (let i = 0; i < routes; i++) files[`pages/Page${i}.tsx`] = "export default 1;\n"; + using dir = tempDir("fsr-reload-intern", files); + + const router = new Bun.FileSystemRouter({ + dir: path.join(String(dir), "pages"), + style: "nextjs", + fileExtensions: [".tsx"], + }); + router.reload(); + const before = dirnameStoreAppendCount(); + for (let i = 0; i < reloads; i++) router.reload(); + const delta = dirnameStoreAppendCount() - before; + // Route::parse itself must perform zero new appends after the first reload. + // The resolver's bust-then-reread path still interns a couple of directory + // paths per reload (tracked separately), so allow O(dirs) residual but fail + // on any O(routes) growth. Without the fix delta is routes * 4 * reloads + // (~8000 here); with it, ~6 * reloads. + expect(delta).toBeLessThan(routes * reloads); + expect(router.match("/Page7")?.filePath).toEndWith("Page7.tsx"); + expect(router.match("/sub/Nested")?.filePath).toEndWith("Nested.tsx"); +}); + it("reload() works with new dirs/files", () => { const { dir } = make(["posts.tsx"]);