From bd48403886c664a297de93d82a6b00cda7b4622b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 09:26:20 +0000 Subject: [PATCH] fs: reuse DirEntry in bust_entries_cache instead of orphaning it --- src/jsc/hot_reloader.rs | 31 +++++++--- src/resolver/fs.rs | 6 ++ src/resolver/lib.rs | 48 ++++++++++++--- src/resolver/resolver.rs | 4 +- test/js/bun/util/filesystem_router.test.ts | 68 ++++++++++++++++++++++ 5 files changed, 139 insertions(+), 18 deletions(-) diff --git a/src/jsc/hot_reloader.rs b/src/jsc/hot_reloader.rs index 6e66a67d0395..72cd624961d5 100644 --- a/src/jsc/hot_reloader.rs +++ b/src/jsc/hot_reloader.rs @@ -1002,8 +1002,9 @@ where let affected_len: usize = 'brk: { if IS_KQUEUE { - // SAFETY: hot-reload runs single-threaded on the JS thread; - // no other live `&mut EntriesOption` for this key here. + // Index lookup only (`BSSMap::get` locks internally). The slot's + // contents can be rewritten in place by a JS-thread resolve, so + // they are read only under `entries_mutex` ('locked block below). if let Some(existing) = rfs.entries.get(file_path) { self.put_tombstone(file_path, existing); entries_option = Some(existing); @@ -1103,10 +1104,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 +1152,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; @@ -1284,6 +1295,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 5bce1207b0ca..488c4224da26 100644 --- a/src/resolver/fs.rs +++ b/src/resolver/fs.rs @@ -563,6 +563,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, } @@ -575,6 +580,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 7658a68ba4e8..58237d0e2a27 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -1254,7 +1254,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), @@ -1360,16 +1360,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) } @@ -1631,6 +1650,11 @@ 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 } { + // `DirEntry.stale` is deliberately not honored here: this serves the + // `DirInfo::get_entries*` accessors, and after a bust they keep their + // `DirInfo`'s listing (as they kept the orphaned one) until the next + // directory-level read (`read_directory_with_iterator`, + // `dir_info_cached_miss`, `dir_info_for_resolution`) refreshes it. if existing.generation < generation { let e_ptr: *mut DirEntry = std::ptr::from_mut::(*existing); // SAFETY: BSSMap-owned `DirEntry` (boxed/leaked into `EntriesOption`); `entries_mutex` held. @@ -1655,9 +1679,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 24b6988048f0..fd139652708d 100644 --- a/src/resolver/resolver.rs +++ b/src/resolver/resolver.rs @@ -3493,7 +3493,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 { @@ -4562,7 +4562,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/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index a815be1d9c7d..f6969beaf312 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -917,3 +917,71 @@ it("loads routes from a directory already cached by Bun.build()", async () => { signalCode: proc.signalCode, }).toEqual({ stdout: "/a /b /sub/c /b", stderr: "", exitCode: 0, signalCode: null }); }); + +// 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], + // ASAN's quarantine retains freed allocations (default 256 MB) so the + // per-reload freed EntryMap/snapshot buffers inflate RSS under bun-asan. + // Disable it in the child so the RSS delta reflects retained allocations. + env: { ...bunEnv, ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "quarantine_size_mb=0"].filter(Boolean).join(":") }, + 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 ~100 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, those allocations are reused in place and RSS is flat. + expect(deltaKB).toBeLessThan(32 * 1024); + expect(exitCode).toBe(0); +}, 30_000);