Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions src/jsc/hot_reloader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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 {})",
Expand Down
6 changes: 6 additions & 0 deletions src/resolver/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand All @@ -682,6 +687,7 @@ impl DirEntry {
dir,
data: dir_entry::EntryMap::default(),
generation,
stale: false,
fd: Fd::INVALID,
}
}
Expand Down
45 changes: 35 additions & 10 deletions src/resolver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<DirEntry>(*e));
}
cached => return Ok(cached),
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -1626,7 +1645,7 @@ pub mod fs {
let result_ptr = std::ptr::from_mut::<EntriesOption>(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::<DirEntry>(*existing);
// SAFETY: BSSMap-owned `DirEntry` (boxed/leaked into `EntriesOption`); `entries_mutex` held.
let dir = unsafe { (*e_ptr).dir };
Expand All @@ -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 };
Comment thread
robobun marked this conversation as resolved.
// SAFETY: see above — slot is exclusively owned here.
unsafe { *e_ptr = new_entry };
}
Expand Down
4 changes: 2 additions & 2 deletions src/resolver/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
29 changes: 22 additions & 7 deletions src/router/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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);
}
}
}
}

Expand Down
32 changes: 24 additions & 8 deletions src/runtime/api/filesystem_router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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);
}

Expand Down
66 changes: 66 additions & 0 deletions test/js/bun/util/filesystem_router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string> = {};
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);
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
}, 30_000);
Loading