diff --git a/src/jsc/hot_reloader.rs b/src/jsc/hot_reloader.rs index c4385b0b82c..204970b8673 100644 --- a/src/jsc/hot_reloader.rs +++ b/src/jsc/hot_reloader.rs @@ -1103,10 +1103,6 @@ where } } - let _ = self.ctx_mut().bust_dir_cache( - strings::paths::without_trailing_slash_windows_path(file_path), - ); - // The watched entrypoint has a per-file inotify watch on its inode. // An atomic rename (`rename(tmp, entrypoint)`) or a rm+recreate over // the entrypoint replaces that inode, so the kernel drops the @@ -1155,10 +1151,24 @@ where } } - if let Some(dir_ent) = entries_option { + 'locked: { + let Some(dir_ent) = entries_option else { + break 'locked; + }; + // `bust_entries_cache` now marks the `DirEntry` stale in place + // instead of orphaning the slot. That means a concurrent resolve + // (under `entries_mutex`) can rewrite this exact `DirEntry`/ + // `EntryMap` via the `in_place` path — including one triggered by + // a *previous* directory event's bust that is still in flight. + // Serialize with those writers so the `dir_ent.entries().get(...)` + // reads below see a consistent map. + let _entries_g = rfs.entries_mutex.lock_guard(); // SAFETY: dir_ent points into rfs.entries (or a tombstoned copy); // both outlive this loop iteration. let dir_ent = unsafe { &mut *dir_ent }; + if !matches!(dir_ent, Fs::EntriesOption::Entries(_)) { + break 'locked; + } let mut last_file_hash: bun_watcher::HashType = bun_watcher::HashType::MAX; @@ -1279,6 +1289,12 @@ where } } + // Bust after releasing `entries_mutex` — `bust_entries_cache` + // takes it internally and the lock is non-recursive. + let _ = self.ctx_mut().bust_dir_cache( + strings::paths::without_trailing_slash_windows_path(file_path), + ); + if self.verbose { Self::debug(format_args!( "Dir change: {} (affecting {})", diff --git a/src/resolver/fs.rs b/src/resolver/fs.rs index c72ebe8c1b7..790534a7c72 100644 --- a/src/resolver/fs.rs +++ b/src/resolver/fs.rs @@ -670,6 +670,11 @@ pub struct DirEntry { pub dir: &'static [u8], pub fd: Fd, pub generation: Generation, + /// Set by `RealFS::bust_entries_cache`. Forces the next read to go through + /// the `in_place` re-scan path regardless of generation, so the existing + /// `DirEntry` allocation and its `Entry`/`FilenameStore` slots are reused + /// instead of being orphaned. + pub stale: bool, pub data: dir_entry::EntryMap, } @@ -682,6 +687,7 @@ impl DirEntry { dir, data: dir_entry::EntryMap::default(), generation, + stale: false, fd: Fd::INVALID, } } diff --git a/src/resolver/lib.rs b/src/resolver/lib.rs index cb2c145c37a..81c3be02608 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -1273,7 +1273,7 @@ pub mod fs { // scrutinee directly so no second `&mut *cached_ptr` is materialized // while the first is on the borrow stack (Stacked Borrows hygiene). match unsafe { &mut *cached_ptr } { - EntriesOption::Entries(e) if e.generation < generation => { + EntriesOption::Entries(e) if e.stale || e.generation < generation => { in_place = Some(std::ptr::from_mut::(*e)); } cached => return Ok(cached), @@ -1378,16 +1378,35 @@ pub mod fs { }))) } - /// Evicts `file_path` from the directory-entry cache; returns whether - /// an entry was removed. + /// Invalidates `file_path` in the directory-entry cache; returns + /// whether an entry was invalidated. pub fn bust_entries_cache(&mut self, file_path: &[u8]) -> bool { - // `entries` is the process-global - // BSSMap singleton and `remove` mutates it; callers (transpiler / - // hot-reloader / VM) reach this without `RESOLVER_MUTEX`, so take - // `entries_mutex` to satisfy `EntriesMap::inner`'s aliasing - // invariant. No caller already holds it (no re-entry from + // `entries` is the process-global BSSMap singleton; callers + // (transpiler / hot-reloader / VM) reach this without + // `RESOLVER_MUTEX`. `read_directory_with_iterator` and + // `dir_info_cached_maybe_log` hold `entries_mutex` while + // reading/overwriting slot contents, so taking it here keeps the + // tag check, the `.stale` write, and the `remove` fallback from + // racing an in-place overwrite (`EntriesMap::inner`'s aliasing + // invariant). No caller already holds it (no re-entry from // `read_directory`/`dir_info_cached_maybe_log`). let _g = self.entries_mutex.lock_guard(); + + // `BSSMap::remove()` only drops the hash→index mapping; the backing + // slot (and the heap `DirEntry` it points at) are orphaned, and the + // next lookup allocates a fresh slot + fresh `DirEntry`. That skips + // the `in_place` re-scan, so every bust leaked the `DirEntry`, its + // `EntryMap`, and grew `EntryStore`/`FilenameStore` by one entry per + // file in the directory. + // + // Instead, keep the slot and flag the `DirEntry` so the next read + // takes the `in_place` path and reuses all of those allocations. + if let Some(EntriesOption::Entries(entries)) = self.entries.get(file_path) { + entries.stale = true; + return true; + } + // `Err` slots and `NotFound` sentinels hold no `DirEntry`; fall back + // to dropping the key so the next read re-checks disk. self.entries.remove(file_path) } @@ -1626,7 +1645,7 @@ pub mod fs { let result_ptr = std::ptr::from_mut::(self.entries.at_index(index)?); // SAFETY: BSSMap-owned slot; uniquely held under `entries_mutex`. if let EntriesOption::Entries(existing) = unsafe { &mut *result_ptr } { - if existing.generation < generation { + if existing.stale || existing.generation < generation { let e_ptr: *mut DirEntry = std::ptr::from_mut::(*existing); // SAFETY: BSSMap-owned `DirEntry` (boxed/leaked into `EntriesOption`); `entries_mutex` held. let dir = unsafe { (*e_ptr).dir }; @@ -1650,9 +1669,15 @@ pub mod fs { // SAFETY: see above — exclusive `&mut` on the prev map for the duration of `readdir`. let prev = Some(unsafe { &mut (*e_ptr).data }); match self.readdir(false, prev, dir, generation, handle, ()) { - Ok(new_entry) => { + Ok(mut new_entry) => { // SAFETY: see above. unsafe { (*e_ptr).data.clear() }; + // `readdir(store_fd=false, …)` leaves `new_entry.fd = INVALID`; + // carry over any previously-stored descriptor so callers that + // cached it (e.g. `DirInfo::get_file_descriptor`) keep working + // and the old handle isn't silently leaked. + // SAFETY: see above. + new_entry.fd = unsafe { (*e_ptr).fd }; // SAFETY: see above — slot is exclusively owned here. unsafe { *e_ptr = new_entry }; } diff --git a/src/resolver/resolver.rs b/src/resolver/resolver.rs index bc10728c0c5..91638d1b25c 100644 --- a/src/resolver/resolver.rs +++ b/src/resolver/resolver.rs @@ -3463,7 +3463,7 @@ impl<'a> Resolver<'a> { if let Some(cached_entry) = rfs!().entries.at_index(cached_dir_entry_result.index) { if let Fs::file_system::real_fs::EntriesOption::Entries(entries) = cached_entry { - if entries.generation >= self.generation { + if !entries.stale && entries.generation >= self.generation { dir_entries_option = cached_entry; needs_iter = false; } else { @@ -4535,7 +4535,7 @@ impl<'a> Resolver<'a> { if let Some(cached_entry) = rfs!().entries.at_index(cached_dir_entry_result.index) { if let Fs::file_system::real_fs::EntriesOption::Entries(entries) = cached_entry { - if entries.generation >= self.generation { + if !entries.stale && entries.generation >= self.generation { dir_entries_option = cached_entry; needs_iter = false; } else { diff --git a/src/router/lib.rs b/src/router/lib.rs index 743513c9ff9..a04f6930232 100644 --- a/src/router/lib.rs +++ b/src/router/lib.rs @@ -796,6 +796,16 @@ impl<'a> RouteLoader<'a> { ) { let fs = self.fs; + // Subdirectory recursion is deferred until after the `entries.data` + // iteration completes. `bust_entries_cache` now marks the `DirEntry` + // stale in place (instead of orphaning the slot), so if the watcher + // thread busts this directory mid-scan, the `read_dir_info_ignore_error` + // call below — on this same thread — can rewrite `entries.data` in place + // via the resolver's `in_place` path while we're still iterating it. + // `*Entry` values live in the append-only `EntryStore`, so snapshotting + // the pointers is sufficient. + let mut subdirs: Vec<*mut Fs::Entry> = Vec::new(); + if let Some(entries) = root_dir_info.get_entries_const() { let iter = entries.iter(); 'outer: for entry_ptr in iter { @@ -826,13 +836,7 @@ impl<'a> RouteLoader<'a> { continue 'outer; } } - - let abs_parts = [entry.dir(), entry.base()]; - if let Some(dir_info) = - resolver.read_dir_info_ignore_error(fs.abs(&abs_parts)) - { - self.load(resolver, &dir_info, base_dir); - } + subdirs.push(entry_ptr); } Fs::EntryKind::File => { @@ -878,6 +882,17 @@ impl<'a> RouteLoader<'a> { } } } + + for entry_ptr in subdirs { + // SAFETY: `entry_ptr` points into the process-static `EntryStore` + // (append-only), so it remains valid across any `in_place` rewrite + // of the parent `DirEntry` triggered by `read_dir_info_ignore_error`. + let entry: &Fs::Entry = unsafe { &*entry_ptr }; + let abs_parts = [entry.dir(), entry.base()]; + if let Some(dir_info) = resolver.read_dir_info_ignore_error(fs.abs(&abs_parts)) { + self.load(resolver, &dir_info, base_dir); + } + } } } diff --git a/src/runtime/api/filesystem_router.rs b/src/runtime/api/filesystem_router.rs index c8be0bf8b38..28df67b2258 100644 --- a/src/runtime/api/filesystem_router.rs +++ b/src/runtime/api/filesystem_router.rs @@ -385,6 +385,14 @@ impl FileSystemRouter { Err(_) => return, }; + // Snapshot the subdirectory `*Entry` pointers before recursing. With + // the `DirEntry.stale` mechanism a concurrent watcher-thread bust of + // `path` can cause the `read_dir_info(child)` call below (on this same + // thread) to rewrite `entries.data` in place while we're mid-iteration. + // `*Entry` values live in the append-only `EntryStore`, so the pointers + // (and their `dir`/`base` strings) stay valid across that. + let mut subdirs: Vec<*mut Fs::Entry> = Vec::new(); + if let Some(dir_ref) = root_dir_info { if let Some(entries) = dir_ref.get_entries_const() { 'outer: for &entry_ptr in entries.data.values() { @@ -415,19 +423,27 @@ impl FileSystemRouter { continue 'outer; } } - let abs_parts: [&[u8]; 2] = [entry.dir, entry.base()]; - // `abs()` writes into a thread-local buffer; copy out - // before recursing (recursion overwrites it). - let full_path = vm.fs().abs(&abs_parts).to_vec(); - let _ = vm.as_mut().transpiler.resolver.bust_dir_cache( - strings::paths::without_trailing_slash_windows_path(&full_path), - ); - self.bust_dir_cache_recursive(global_this, &full_path); + subdirs.push(entry_ptr); } } } } + for entry_ptr in subdirs { + // SAFETY: `entry_ptr` points into the process-static `EntryStore` + // (append-only), so it remains valid regardless of any `DirEntry` + // rewrite above. + let entry = unsafe { &*entry_ptr }; + let abs_parts: [&[u8]; 2] = [entry.dir, entry.base()]; + // `abs()` writes into a thread-local buffer; copy out before + // recursing (recursion overwrites it). + let full_path = vm.fs().abs(&abs_parts).to_vec(); + let _ = vm.as_mut().transpiler.resolver.bust_dir_cache( + strings::paths::without_trailing_slash_windows_path(&full_path), + ); + self.bust_dir_cache_recursive(global_this, &full_path); + } + let _ = vm.as_mut().transpiler.resolver.bust_dir_cache(path); } diff --git a/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index 4a5fa7b9678..dc31a9cdb5e 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -692,3 +692,69 @@ it("does not match a dynamic route whose static segment merely collides on lengt // A different segment that only collides on (length, 32-bit hash) must not. expect(router.match(`/${attackSegment}/42`)).toBeNull(); }); + +// bust_entries_cache used to drop the BSSMap key, orphaning the backing slot +// and the heap DirEntry it pointed at. The next lookup then allocated a fresh +// slot + DirEntry, skipping the in_place reuse — so every reload() leaked one +// DirEntry + EntryMap per directory and appended N Entry/FilenameStore slots +// per directory entry, unbounded. +it("reload() should not leak directory entry caches", async () => { + // The DirEntry/EntryStore/FilenameStore leak scales with the number of + // directory entries seen by readdir — not with the number of routes — so + // most files deliberately do NOT match `fileExtensions`. That keeps the + // route count (and any unrelated per-route/reload overhead) small while the + // directory-entry signal stays large. Long names push past the inline + // small-string limit so the old code had to hit FilenameStore on every + // re-read. + const files: Record = {}; + for (let d = 0; d < 4; d++) { + files[`directory_with_a_long_name_${d}/index.tsx`] = "export default 0;\n"; + for (let f = 0; f < 100; f++) { + files[`directory_with_a_long_name_${d}/asset_with_a_long_file_name_number_${f}.txt`] = "x\n"; + } + } + using dir = tempDir("fsrouter-reload-leak", files); + + const script = /* js */ ` + const router = new Bun.FileSystemRouter({ + dir: ${JSON.stringify(String(dir))}, + style: "nextjs", + fileExtensions: [".tsx"], + }); + + // Settle any one-time growth from the first few scans. + for (let i = 0; i < 20; i++) router.reload(); + Bun.gc(true); + const before = process.memoryUsage.rss(); + + for (let i = 0; i < 400; i++) router.reload(); + Bun.gc(true); + const after = process.memoryUsage.rss(); + + console.log(JSON.stringify({ + before, + after, + deltaKB: Math.round((after - before) / 1024), + routes: Object.keys(router.routes).length, + })); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "--smol", "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + const { deltaKB, routes } = JSON.parse(stdout.trim()); + expect(routes).toBe(4); + // Before the fix this grew by ~65+ MB over 400 reloads (4 DirEntries + + // ~400 Entry structs + ~800 FilenameStore strings orphaned per reload) and + // eventually aborted once the BSSMap/EntryStore overflow lists filled up. + // After, the DirEntry/Entry/FilenameStore allocations are reused in place + // and growth is dominated by unrelated small per-reload overhead. + expect(deltaKB).toBeLessThan(32 * 1024); + expect(exitCode).toBe(0); +}, 30_000);