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..2ac43e95a699 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -122,12 +122,87 @@ 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 { + let this = $backing(); + // SAFETY: `this` is the live process-lifetime singleton; `Mutex: Sync` + // so concurrent `&Mutex` formation is sound. With the inner mutex + // held no other thread holds `&mut *this`, so the raw-place reads + // of `slice_buf_used` / `overflow_list.count` are race-free. + unsafe { + let _g = (*this).mutex.lock(); + (*this).slice_buf_used as u32 + (*this).overflow_list.count + } + } } }; } string_store_impl!(DirnameStore, DIRNAME_STORE_ZST, dirname_store_backing); string_store_impl!(FilenameStore, FILENAME_STORE_ZST, filename_store_backing); + // Process-wide (not thread-local): the backing store is process-global, so the + // dedupe index must match its lifetime or resolver worker threads each re-append. + static DIRNAME_INTERN: std::sync::LazyLock< + bun_core::Mutex>, + > = std::sync::LazyLock::new(Default::default); + + impl DirnameStore { + /// Content-deduplicated [`append_slice`]. `BSSStringList::append` does not + /// dedupe, so a bust-then-reread cycle (hot reload, `FileSystemRouter.reload`) + /// would otherwise re-append the same directory path on every miss, exhausting + /// the store's slot capacity and leaking one heap buffer per overflow append. + pub fn intern_slice(&self, value: &[u8]) -> crate::CrateResult<&'static [u8]> { + if self.exists(value) { + // SAFETY: `exists` is a pointer-range check — `value` lies wholly + // within the process-lifetime backing buffer, so widening is sound. + return Ok(unsafe { core::slice::from_raw_parts(value.as_ptr(), value.len()) }); + } + let mut set = DIRNAME_INTERN.lock(); + if let Some((interned, ())) = set.get_key_value(value) { + return Ok(*interned); + } + let interned = self.append_slice(value)?; + set.insert(interned, ()); + Ok(interned) + } + + /// Content-deduplicated [`append_parts`]. See [`intern_slice`]. + pub fn intern_parts(&self, parts: &[&[u8]]) -> crate::CrateResult<&'static [u8]> { + const STACK: usize = 512; + let total: usize = parts.iter().map(|p| p.len()).sum(); + let mut stack = [0u8; STACK]; + let mut heap: Vec; + let scratch: &mut [u8] = if total <= STACK { + &mut stack[..total] + } else { + heap = vec![0u8; total]; + &mut heap[..] + }; + let mut at = 0; + for p in parts { + scratch[at..at + p.len()].copy_from_slice(p); + at += p.len(); + } + self.intern_slice(&scratch[..at]) + } + + /// Content-deduplicated [`append_lower_case`]. See [`intern_slice`]. + pub fn intern_lower_case(&self, value: &[u8]) -> crate::CrateResult<&'static [u8]> { + const STACK: usize = 256; + let mut stack = [0u8; STACK]; + let mut heap: Vec; + let scratch: &mut [u8] = if value.len() <= STACK { + &mut stack[..value.len()] + } else { + heap = vec![0u8; value.len()]; + &mut heap[..] + }; + self.intern_slice(bun_core::strings::copy_lowercase(value, scratch)) + } + } + macro_rules! string_store_append_impl { ($t:ty, $backing:ident) => { impl $t { @@ -1297,11 +1372,10 @@ pub mod fs { // its `dir` field is DirnameStore-interned (&'static). unsafe { (*existing).dir } } else if !had_handle { - DirnameStore::instance().append_slice(dir_maybe_trail_slash)? + DirnameStore::instance().intern_slice(dir_maybe_trail_slash)? } else { - // Intern into DirnameStore so the cache entry never dangles — - // `append_slice` is a bump-pointer copy, cost is bounded. - DirnameStore::instance().append_slice(dir)? + // Intern into DirnameStore so the cache entry never dangles. + DirnameStore::instance().intern_slice(dir)? }; // Cache miss: read the directory entries diff --git a/src/resolver/resolver.rs b/src/resolver/resolver.rs index b0ef596c5453..dd60d0740ecc 100644 --- a/src/resolver/resolver.rs +++ b/src/resolver/resolver.rs @@ -3509,7 +3509,7 @@ impl<'a> Resolver<'a> { unsafe { &*existing }.dir } else { Fs::file_system::DirnameStore::instance() - .append_slice(dir_path) + .intern_slice(dir_path) .expect("unreachable") }, self.generation, @@ -4517,9 +4517,9 @@ impl<'a> Resolver<'a> { let input = &path[..input_path_len]; if input[input.len() - 1] != SEP { let parts: [&[u8]; 2] = [input, SEP_STR.as_bytes()]; - _safe_path = Some(self.fs_ref().dirname_store.append_parts(&parts)?); + _safe_path = Some(self.fs_ref().dirname_store.intern_parts(&parts)?); } else { - _safe_path = Some(self.fs_ref().dirname_store.append_slice(input)?); + _safe_path = Some(self.fs_ref().dirname_store.intern_slice(input)?); } } @@ -4578,7 +4578,7 @@ impl<'a> Resolver<'a> { unsafe { &*existing }.dir } else { Fs::file_system::DirnameStore::instance() - .append_slice(dir_path) + .intern_slice(dir_path) .expect("unreachable") }, self.generation, diff --git a/src/router/lib.rs b/src/router/lib.rs index 797365348028..721d0c9860c8 100644 --- a/src/router/lib.rs +++ b/src/router/lib.rs @@ -1123,17 +1123,18 @@ 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: DirnameStore::intern_* 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] = dirname_store + .intern_slice(public_path) + .expect("unreachable"); 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..]) + .intern_lower_case(&name[1..]) .expect("unreachable") } else { &name[1..] @@ -1144,8 +1145,9 @@ impl Route { (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] = dirname_store + .intern_slice(public_path) + .expect("unreachable"); ( public_path, Route::INDEX_ROUTE_NAME, @@ -1233,7 +1235,7 @@ impl Route { abs_path_str = FileSystem::instance() .dirname_store() - .append(_abs) + .intern_slice(_abs) .expect("unreachable"); // SAFETY: sole mutation; `base_`/`extname` (which may borrow @@ -1251,7 +1253,7 @@ impl Route { ); let interned: &'static [u8] = FileSystem::instance() .dirname_store() - .append(normalized) + .intern_slice(normalized) .expect("unreachable"); Interned::from_static(interned) }; @@ -1269,13 +1271,13 @@ impl Route { } // NOTE: name/match_name/public_path are already `&'static` via - // DirnameStore::append above. `entry.base()` borrows the entry (it - // may be inline-stored for ≤31-byte names); intern it + // DirnameStore::intern_slice 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()) + .intern_slice(unsafe { &*entry }.base()) .expect("unreachable"); Some(Route { 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..afd3dfcd8ef1 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,32 @@ it("reload() works", () => { expect(router.match("/posts")!.name).toBe("/posts"); }); +it("reload() does not re-intern directory paths into DirnameStore on every bust+reread", () => { + const files: Record = { "pages/sub/nested.tsx": "export default 1;\n" }; + for (let i = 0; i < 20; i++) files[`pages/p${i}.tsx`] = "export default 1;\n"; + using dir = tempDir("fsr-reload-dirname-intern", files); + + const router = new Bun.FileSystemRouter({ + dir: path.join(String(dir), "pages"), + style: "nextjs", + fileExtensions: [".tsx"], + }); + // First reload primes the intern cache for the bust+reread paths. + router.reload(); + const before = dirnameStoreAppendCount(); + for (let i = 0; i < 50; i++) router.reload(); + const delta = dirnameStoreAppendCount() - before; + // Without the fix each reload re-appended identical bytes into the never-freed + // store: `dir_info_cached_miss` re-interned safe_path + DirEntry.dir per dir + // (6 per reload here) and `Route::parse` re-interned public_path/abs_path/ + // basename per route (63 per reload here), so 50 reloads grew the store by + // ~3450 slots and eventually overflowed it with `AllocError`. + expect({ delta, match: router.match("/sub/nested")?.filePath.endsWith("nested.tsx") }).toEqual({ + delta: 0, + match: true, + }); +}); + it("reload() works with new dirs/files", () => { const { dir } = make(["posts.tsx"]);