From 07decfb49686f2911484344c35a283bbc2f6ed72 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:52:37 +0000 Subject: [PATCH 1/3] resolver: read every cached directory listing through one reader, cache nothing for a failed read The two listing loops in resolver.rs took an error from the directory iterator as the end of the directory and cached the names read so far as the whole directory. RealFS::readdir returned the error, and its callers cached it with no generation. RealFS::read_listing is now the only reader. A read that fails is tried once more on a handle opened again by path. If it fails again, nothing is stored: a fresh slot stays unknown and a stale listing keeps its names. commit_listing is the only writer of EntriesOption::Entries. A resolution that crossed a failed read fails with 'Cannot read directory "": while resolving ""'. --- src/resolver/lib.rs | 462 ++++++++++++++++++++++++++++++--------- src/resolver/resolver.rs | 337 ++++++++++++++-------------- 2 files changed, 526 insertions(+), 273 deletions(-) diff --git a/src/resolver/lib.rs b/src/resolver/lib.rs index 25986a502f8d..2de470cdd695 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -967,7 +967,8 @@ pub mod fs { bun_alloc::bss_map_inner! { pub entries_option_map : EntriesOption, 2048, true } /// Resolver-side wrapper over `EntriesOptionMap` exposing the BSSMap surface - /// (`get`, `get_or_put`, `at_index`, `put`, `mark_not_found`). ZST handle — + /// (`get`, `get_or_put`, `at_index`; `put` and `mark_not_found` stay inside + /// this module). ZST handle — /// every call resolves to the `entries_option_map()` singleton; this keeps /// `RealFS.entries` field-shaped without inlining the (large) backing array. /// @@ -1019,7 +1020,9 @@ pub mod fs { ) -> Option<&mut EntriesOption> { self.inner().at_index(index) } - pub(crate) fn put( + /// Private to this module: a listing reaches the cache through + /// `RealFS::commit_listing` only. + fn put( &mut self, result: &mut bun_alloc::Result, value: EntriesOption, @@ -1032,7 +1035,7 @@ pub mod fs { .map(std::ptr::from_mut::) .map_err(|_| crate::Error::Alloc(bun_alloc::AllocError)) } - pub(crate) fn mark_not_found(&mut self, result: bun_alloc::Result) { + fn mark_not_found(&mut self, result: bun_alloc::Result) { self.inner().mark_not_found(result) } pub(crate) fn remove(&mut self, key: &[u8]) -> bool { @@ -1040,6 +1043,23 @@ pub mod fs { } } + /// A directory whose read failed, with the error. A failed read is cached + /// nowhere, so whoever needed the listing reports it. + pub struct DirReadFailure { + pub dir: Box<[u8]>, + pub err: crate::Error, + } + + impl DirReadFailure { + #[cold] + pub(crate) fn new(dir: &[u8], err: crate::Error) -> Box { + Box::new(DirReadFailure { + dir: Box::from(strings::paths::without_trailing_slash_windows_path(dir)), + err, + }) + } + } + /// The active filesystem backend (always the real filesystem). pub type Implementation = RealFS; @@ -1053,6 +1073,10 @@ pub mod fs { /// this directly (`rfs.entries.get_or_put(..)`); modeled as the wrapper /// `EntriesMap` (bun_alloc has no BSSMap equivalent). pub entries: EntriesMap, + /// The entries of the last directory whose read failed, which + /// `read_listing` uses again when it reads that directory. Guarded by + /// `entries_mutex`. + failed_listing: Option, pub(crate) cwd: &'static [u8], #[cfg(not(windows))] pub(crate) file_limit: usize, @@ -1069,6 +1093,7 @@ pub mod fs { RealFS { entries_mutex: Mutex::default(), entries: EntriesMap::new(), + failed_listing: None, cwd, #[cfg(not(windows))] file_limit, @@ -1143,39 +1168,234 @@ pub mod fs { } } - /// Iterate `handle` and populate a - /// fresh `DirEntry` (re-using `prev_map` Entry slots where the name matches). + /// Enumerate `handle` into `listing`, re-using `prev_map` Entry slots + /// where the name matches. This is the only loop that reads a directory + /// for the cache. An `Err` leaves `listing` with the names that were + /// read before it, which are not the directory. fn readdir( - &mut self, - store_fd: bool, + listing: &mut DirEntry, mut prev_map: Option<&mut dir_entry::EntryMap>, - dir_: &'static [u8], - generation: Generation, handle: Fd, - iterator: I, - ) -> crate::CrateResult { + iterator: &I, + ) -> crate::CrateResult<()> { let mut iter = bun_sys::iterate_dir(handle); - let mut dir = DirEntry::init(dir_, generation); - - if store_fd { - FileSystem::set_max_fd(bun_sys::Fd::native(handle)); - dir.fd = handle; - } - let mut filename_store = FilenameStoreAppender::new(); while let Some(entry_) = iter.next()? { - // debug("readdir entry {}", BStr::new(entry_.name.slice())); - dir.add_entry_with_store( + listing.add_entry_with_store( prev_map.as_deref_mut(), &entry_, &mut filename_store, - &iterator, + iterator, )?; } + Ok(()) + } + + /// Read `handle` to the end and return the listing of `dir_path`. + /// Every listing that `commit_listing` publishes comes from here, so + /// the cache never holds the names of a read that stopped early. + /// + /// A read that fails is tried once more, on a handle opened again by + /// path, so a transient error does not reach the caller. When + /// `owns_handle`, that handle replaces `*handle` and the old one is + /// closed; otherwise `*handle` is left as it is. A non-void iterator + /// has already been handed entries, so its read is not tried again. + /// + /// On `Err` nothing is stored and the caller still owns `*handle`. + /// The caller holds `entries_mutex`. + pub(crate) fn read_listing( + &mut self, + in_place: Option<*mut DirEntry>, + dir_path: &[u8], + generation: Generation, + handle: &mut Fd, + owns_handle: bool, + store_fd: bool, + reserve: usize, + iterator: I, + ) -> crate::CrateResult { + let (dir, mut interned) = self.listing_dir(in_place, dir_path)?; + let mut listing = DirEntry::init(dir, generation); + listing.data.reserve(reserve); + if store_fd { + FileSystem::set_max_fd(bun_sys::Fd::native(*handle)); + listing.fd = *handle; + } + let prev_map = match interned.as_mut() { + Some(failed) => Some(&mut failed.data), + // SAFETY: BSSMap-owned `DirEntry`; no aliasing here (`entries_mutex` held). + None => in_place.map(|p| unsafe { &mut (*p).data }), + }; + let Err(err) = Self::readdir(&mut listing, prev_map, *handle, &iterator) else { + bun_core::scoped_log!( + crate::fs_full::Fs, + "readdir({}, {}) = {}", + *handle, + bstr::BStr::new(dir), + listing.data.count(), + ); + return Ok(listing); + }; + self.read_listing_again( + listing, + interned, + in_place, + err, + handle, + owns_handle, + store_fd, + I::IS_VOID, + ) + } + + /// The interned path for a new listing of `dir_path`, and the entries + /// that an earlier failed read of the same directory already interned. + fn listing_dir( + &mut self, + in_place: Option<*mut DirEntry>, + dir_path: &[u8], + ) -> crate::CrateResult<(&'static [u8], Option)> { + // SAFETY: `in_place` points to a `DirEntry` inside the BSSMap + // singleton; its `dir` field is DirnameStore-interned (&'static). + let stale_dir = in_place.map(|p| unsafe { (*p).dir }); + let interned = match &self.failed_listing { + Some(failed) + if strings::paths::without_trailing_slash_windows_path(failed.dir) + == strings::paths::without_trailing_slash_windows_path( + stale_dir.unwrap_or(dir_path), + ) => + { + self.failed_listing.take() + } + _ => None, + }; + let dir = match (stale_dir, &interned) { + (Some(dir), _) => dir, + (None, Some(failed)) => failed.dir, + (None, None) => DirnameStore::instance().append_slice(dir_path)?, + }; + Ok((dir, interned)) + } + + /// The cold half of `read_listing`: the read of `failed.dir` stopped + /// at `err` after `failed.data` was filled. + #[cold] + #[inline(never)] + fn read_listing_again( + &mut self, + mut failed: DirEntry, + interned: Option, + in_place: Option<*mut DirEntry>, + err: crate::Error, + handle: &mut Fd, + owns_handle: bool, + store_fd: bool, + try_again: bool, + ) -> crate::CrateResult { + // `EntryStore` and `FilenameStore` only grow. `failed.data` collects + // every entry interned for this directory, so that the next read of + // it, here or in a later lookup, interns only names it has not seen. + match &interned { + Some(earlier) => Self::keep_entries(&mut failed.data, &earlier.data)?, + None => { + if let Some(stale) = in_place { + // SAFETY: BSSMap-owned `DirEntry`; read-only here (`entries_mutex` held). + Self::keep_entries(&mut failed.data, unsafe { &(*stale).data })?; + } + } + } + drop(interned); + + // The failed handle is not read again: its position is unknown. + if try_again { + if let Ok(fresh) = bun_sys::open_dir_for_iteration(Fd::cwd(), failed.dir) { + let mut listing = DirEntry::init(failed.dir, failed.generation); + listing.data.reserve(failed.data.count()); + if Self::readdir(&mut listing, Some(&mut failed.data), fresh, &()).is_ok() { + if owns_handle { + let _ = bun_sys::close(*handle); + *handle = fresh; + FileSystem::set_max_fd(bun_sys::Fd::native(fresh)); + } else { + let _ = bun_sys::close(fresh); + } + if store_fd { + listing.fd = *handle; + } + return Ok(listing); + } + let _ = bun_sys::close(fresh); + Self::keep_entries(&mut failed.data, &listing.data)?; + } + } + + self.failed_listing = Some(failed); + Err(err) + } + + /// Add to `into` the entries of `from` whose names it does not have. + fn keep_entries( + into: &mut dir_entry::EntryMap, + from: &dir_entry::EntryMap, + ) -> crate::CrateResult<()> { + for &entry in from.values() { + // SAFETY: `entry` is an `EntryStore` slot (never freed, never + // moved) and `base_lowercase_` is never mutated after construction. + let key: &'static [u8] = + unsafe { &*core::ptr::from_ref::<[u8]>((*entry).base_lowercase()) }; + let hash = into.hash_key(key); + if into.get_hashed(hash, key).is_none() { + into.put_static_key_hashed(hash, key, entry)?; + } + } + Ok(()) + } + + /// Publish a listing that `read_listing` read to the end, or the empty + /// one of `opaque_listing`. This is the only writer of + /// `EntriesOption::Entries`. The caller holds `entries_mutex`. + pub(crate) fn commit_listing( + &mut self, + slot: &mut bun_alloc::Result, + in_place: Option<*mut DirEntry>, + listing: DirEntry, + ) -> crate::CrateResult<*mut EntriesOption> { + // `EntriesOption::Entries` holds an unbounded `&mut DirEntry` (raw, + // BSSMap-stored pointer), so a fresh slot is a leaked `Box` + // whose lifetime is the `entries_option_map()` singleton (process-static). + let entries_ptr: *mut DirEntry = match in_place { + Some(p) => { + // SAFETY: `p` is a live BSSMap slot, exclusively owned here + // under `entries_mutex`. The assignment drops the stale map. + unsafe { *p = listing }; + p + } + None => bun_core::heap::into_raw(Box::new(listing)), + }; + self.entries.put( + slot, + // SAFETY: see above; re-borrowed as 'static for the BSSMap slot. + EntriesOption::Entries(unsafe { &mut *entries_ptr }), + ) + } - // debug("readdir({}, {}) = {}", handle, dir_, dir.data.count()); + /// The listing of a directory that this process may pass through but + /// not list: it has no entries. + pub(crate) fn opaque_listing( + &mut self, + in_place: Option<*mut DirEntry>, + dir_path: &[u8], + generation: Generation, + ) -> crate::CrateResult { + let (dir, _) = self.listing_dir(in_place, dir_path)?; + Ok(DirEntry::init(dir, generation)) + } - Ok(dir) + /// Cache that `dir` does not exist. + pub(crate) fn mark_dir_not_found(&mut self, dir: &[u8]) -> crate::CrateResult<()> { + let result = self.entries.get_or_put(dir)?; + self.entries.mark_not_found(result); + Ok(()) } /// Cache (or threadlocal- @@ -1206,6 +1426,41 @@ pub mod fs { Ok(unsafe { &mut *opt }) } + /// What a read of `dir` that failed leaves behind. ENOENT and ENOTDIR + /// say that the directory went away while it was open, which ends like + /// a failed open. Any other error stores nothing: a fresh slot stays + /// unknown and a stale listing keeps its names and its generation, so + /// the next lookup reads the directory again. The caller gets the + /// error in the threadlocal slot, and in `read_failure` when it has + /// a place to report it. + fn read_failed( + &mut self, + dir: &[u8], + in_place: Option<*mut DirEntry>, + err: crate::Error, + read_failure: Option<&mut Option>>, + ) -> crate::CrateResult<&'static mut EntriesOption> { + if matches!( + err, + crate::Error::Sys(bun_errno::SystemErrno::ENOENT | bun_errno::SystemErrno::ENOTDIR) + ) { + if let Some(existing) = in_place { + // SAFETY: BSSMap-owned `DirEntry`; `entries_mutex` held. + unsafe { (*existing).data.clear() }; + } + return self.read_directory_error(dir, err); + } + if let Some(read_failure) = read_failure { + *read_failure = Some(DirReadFailure::new(dir, err)); + } + Ok(temp_entries_option_write(EntriesOption::Err( + dir_entry::Err { + original_err: err, + canonical_error: err, + }, + ))) + } + pub fn read_directory( &mut self, dir_: &[u8], @@ -1216,6 +1471,19 @@ pub mod fs { self.read_directory_with_iterator(dir_, handle_, generation, store_fd, ()) } + /// `read_directory` for a resolution. A read that fails is cached + /// nowhere, so the resolution reports it: it gets the directory and + /// the error in `read_failure`. + pub(crate) fn read_directory_for_resolution( + &mut self, + dir_: &[u8], + generation: Generation, + store_fd: bool, + read_failure: &mut Option>, + ) -> crate::CrateResult<&'static mut EntriesOption> { + self.read_directory_impl(dir_, None, generation, store_fd, (), Some(read_failure)) + } + // One of the learnings here // // Closing file descriptors yields significant performance benefits on Linux @@ -1231,6 +1499,25 @@ pub mod fs { generation: Generation, store_fd: bool, iterator: I, + ) -> crate::CrateResult<&'static mut EntriesOption> { + self.read_directory_impl( + dir_maybe_trail_slash, + maybe_handle, + generation, + store_fd, + iterator, + None, + ) + } + + fn read_directory_impl( + &mut self, + dir_maybe_trail_slash: &[u8], + maybe_handle: Option, + generation: Generation, + store_fd: bool, + iterator: I, + read_failure: Option<&mut Option>>, ) -> crate::CrateResult<&'static mut EntriesOption> { let dir = strings::paths::without_trailing_slash_windows_path(dir_maybe_trail_slash); @@ -1267,82 +1554,51 @@ pub mod fs { } let had_handle = maybe_handle.is_some(); - let handle: Fd = match maybe_handle { + let mut handle: Fd = match maybe_handle { Some(h) => h, None => match self.open_dir(dir) { Ok(h) => h, Err(err) => return self.read_directory_error(dir, err), }, }; - - // Close the handle on every exit path. Use - // scopeguard so close happens even if `readdir`/`put` early-return with `?`. let should_close_handle = !had_handle && (!store_fd || self.need_to_close_files()); - let _close_guard = scopeguard::guard(handle, move |h| { - if should_close_handle { - let _ = bun_sys::close(h); - } - }); - - // if we get this far, it's a real directory, so we can just store the dir name. - // An in-place refresh always keeps the slot's existing interned name: callers - // spell the same directory with and without a trailing slash, and rewriting - // `dir` to the other spelling races every unlocked `Entry::dir()` reader. - let dir: &'static [u8] = if let Some(existing) = in_place { - // SAFETY: `in_place` points to a `DirEntry` inside the BSSMap singleton; - // its `dir` field is DirnameStore-interned (&'static). - unsafe { (*existing).dir } - } else if !had_handle { - DirnameStore::instance().append_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)? - }; - // Cache miss: read the directory entries - let prev = in_place.map(|p| { - // SAFETY: BSSMap-owned, no aliasing here (entries_mutex held). - unsafe { &mut (*p).data } - }); - let mut entries = match self.readdir(store_fd, prev, dir, generation, handle, iterator) - { - Ok(e) => e, + // if we get this far, it's a real directory, so `read_listing` can + // store the dir name. An in-place refresh always keeps the slot's + // existing interned name: callers spell the same directory with and + // without a trailing slash, and rewriting `dir` to the other spelling + // races every unlocked `Entry::dir()` reader. + let listing = match self.read_listing( + in_place, + if had_handle { + dir + } else { + dir_maybe_trail_slash + }, + generation, + &mut handle, + !had_handle, + store_fd, + 0, + iterator, + ) { + Ok(listing) => listing, Err(err) => { - if let Some(existing) = in_place { - // SAFETY: see above. - unsafe { (*existing).data.clear() }; + // No listing holds the handle. A non-void iterator was + // handed it with each entry when `store_fd`, so it stays open. + if !had_handle && (I::IS_VOID || !store_fd) { + let _ = bun_sys::close(handle); } - return self.read_directory_error(dir, err); + return self.read_failed(dir, in_place, err, read_failure); } }; - // `EntriesOption::Entries` here holds an unbounded `&mut DirEntry` (raw, BSSMap-stored - // pointer), so a fresh slot is a leaked `Box` whose lifetime is the - // `entries_option_map()` singleton (process-static). - let entries_ptr: *mut DirEntry = match in_place { - Some(p) => p, - None => bun_core::heap::into_raw(Box::new(DirEntry::init(dir, generation))), - }; - if let Some(original) = in_place { - // SAFETY: BSSMap-owned; entries_mutex held. - unsafe { (*original).data.clear() }; - } - if store_fd && !entries.fd.is_valid() { - entries.fd = handle; + let out = self.commit_listing(&mut cache_result, in_place, listing); + if should_close_handle { + let _ = bun_sys::close(handle); } - - // SAFETY: `entries_ptr` is either a live BSSMap slot (`in_place`) or a fresh - // leaked Box; exclusively owned here under `entries_mutex`. - unsafe { *entries_ptr = entries }; - let result = EntriesOption::Entries( - // SAFETY: see above — re-borrow as 'static for the BSSMap slot. - unsafe { &mut *entries_ptr }, - ); - - let out = self.entries.put(&mut cache_result, result)?; // SAFETY: BSSMap-owned slot; outlives caller (process-static singleton). - Ok(unsafe { &mut *out }) + Ok(unsafe { &mut *out? }) } /// Evicts `file_path` from the directory-entry cache; returns whether @@ -1592,7 +1848,8 @@ pub mod fs { /// stale. The generation-stale branch drops the existing `DirEntry` /// (and the bucket allocation behind its `data` map) in place, and /// every `.data` reader holds `entries_mutex` for the probe, so the - /// caller must already hold it (the mutex is non-recursive). + /// caller must already hold it (the mutex is non-recursive). A re-read + /// that fails returns an `Err` and leaves the stale listing in place. pub(crate) fn entries_at_locked( &mut self, index: bun_alloc::IndexType, @@ -1614,7 +1871,7 @@ pub mod fs { let dir = unsafe { (*e_ptr).dir }; // `open_dir_for_iteration`, NOT `RealFS.openDir`. On POSIX // the two diverge: `O_DIRECTORY` only vs `O_RDONLY|O_DIRECTORY`. - let handle = match bun_sys::open_dir_for_iteration(Fd::cwd(), dir) { + let mut handle = match bun_sys::open_dir_for_iteration(Fd::cwd(), dir) { Ok(h) => h, Err(err) => { // SAFETY: see above. @@ -1622,23 +1879,22 @@ pub mod fs { return self.read_directory_error(dir, err.into()).ok(); } }; - let _close_guard = scopeguard::guard(handle, |h| { - let _ = bun_sys::close(h); - }); - // 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) => { - // SAFETY: see above. - unsafe { (*e_ptr).data.clear() }; - // SAFETY: see above — slot is exclusively owned here. - unsafe { *e_ptr = new_entry }; - } - Err(err) => { - // SAFETY: see above. - unsafe { (*e_ptr).data.clear() }; - return self.read_directory_error(dir, err).ok(); - } + let read = self.read_listing( + Some(e_ptr), + dir, + generation, + &mut handle, + true, + false, + 0, + (), + ); + let _ = bun_sys::close(handle); + match read { + // SAFETY: see above — slot is exclusively owned here. + // The assignment drops the stale map. + Ok(listing) => unsafe { *e_ptr = listing }, + Err(err) => return self.read_failed(dir, Some(e_ptr), err, None).ok(), } } } diff --git a/src/resolver/resolver.rs b/src/resolver/resolver.rs index 3615a6efe7fe..1cdf0e9f52f2 100644 --- a/src/resolver/resolver.rs +++ b/src/resolver/resolver.rs @@ -255,7 +255,6 @@ use bun_sys::Fd as FD; use bun_threading::Mutex; use crate::fs as Fs; -use crate::fs::FilenameStoreAppender; use crate::node_fallbacks as NodeFallbackModules; use crate::package_json::{BrowserMap, ESModule, PackageJSON}; use crate::tsconfig_json::TSConfigJSON; @@ -492,6 +491,11 @@ pub struct Resolver<'a> { pub caches: CacheSet, pub generation: Generation, + /// A directory read that failed since `resolve_and_auto_install` last + /// looked. Nothing is cached for that directory, so the resolution that + /// needed its listing has to report the error. + pub(crate) dir_read_failure: Option>, + /// Auto-install backend. `bun_install::PackageManager` implements /// [`AutoInstaller`]; the resolver only sees the trait object so it stays /// below `bun_install` in the dep graph. `None` until the auto-install @@ -627,6 +631,7 @@ impl<'a> Resolver<'a> { watcher: from.watcher, caches: CacheSet::init(), generation: from.generation, + dir_read_failure: None, package_manager: from.package_manager, on_wake_package_manager: from.on_wake_package_manager, env_loader: from.env_loader, @@ -926,6 +931,7 @@ impl<'a> Resolver<'a> { elapsed: 0, watcher: None, generation: 0, + dir_read_failure: None, package_manager: None, on_wake_package_manager: Default::default(), env_loader: None, @@ -1050,12 +1056,78 @@ impl<'a> Resolver<'a> { // var tracing_start: i128 — unused; dropped. + #[inline] pub fn resolve_and_auto_install( &mut self, source_dir: &[u8], import_path: &[u8], kind: ast::ImportKind, global_cache: GlobalCache, + ) -> ResultUnion { + let result = + self.resolve_and_auto_install_once(source_dir, import_path, kind, global_cache); + if self.dir_read_failure.is_some() { + return self.resolve_after_dir_read_failure( + result, + source_dir, + import_path, + kind, + global_cache, + ); + } + result + } + + /// A directory read failed, and nothing is cached for that directory, so + /// `first` can rest on a directory that was not listed: a lookup that + /// needs the listing takes the error as "not there" and tries the next + /// place. The failure can also be one that a lookup outside a resolution + /// left behind. Resolve once more. If the read fails again, this + /// resolution did cross it, and the error is the result. + #[cold] + #[inline(never)] + fn resolve_after_dir_read_failure( + &mut self, + first: ResultUnion, + source_dir: &[u8], + import_path: &[u8], + kind: ast::ImportKind, + global_cache: GlobalCache, + ) -> ResultUnion { + drop(first); + self.dir_read_failure = None; + let result = + self.resolve_and_auto_install_once(source_dir, import_path, kind, global_cache); + let Some(failure) = self.dir_read_failure.take() else { + return result; + }; + drop(result); + self.log_mut().add_resolve_error( + None, + bun_ast::Range::NONE, + format_args!( + "Cannot read directory \"{}\": {} while resolving \"{}\"", + bstr::BStr::new(&failure.dir), + bstr::BStr::new(failure.err.name()), + bstr::BStr::new(import_path) + ), + import_path, + kind, + bun_ast::Error::ModuleNotFound, + ); + ResultUnion::Failure(failure.err) + } + + /// One pass of `resolve_and_auto_install`, without its check for a + /// directory read that failed. The check and its second pass both call + /// this one copy. + #[inline(never)] + fn resolve_and_auto_install_once( + &mut self, + source_dir: &[u8], + import_path: &[u8], + kind: ast::ImportKind, + global_cache: GlobalCache, ) -> ResultUnion { // SAFETY: `import_path` is caller-interned (source text / DirnameStore) // and outlives the returned Result. @@ -3339,7 +3411,7 @@ impl<'a> Resolver<'a> { core::ptr::null_mut(); let mut needs_iter = true; let mut in_place: Option<*mut Fs::file_system::DirEntry> = None; - let open_dir = match bun_sys::open_dir_for_iteration(FD::cwd(), dir_path) { + let mut open_dir = match bun_sys::open_dir_for_iteration(FD::cwd(), dir_path) { Ok(d) => d, Err(err) => { // TODO: handle this error better @@ -3364,83 +3436,29 @@ impl<'a> Resolver<'a> { } if needs_iter { - // SAFETY: (block-wide) `in_place`/`dir_entries_ptr`/`dir_entries_option` point to slots - // in `rfs.entries` (BSSMap singleton) or a fresh leaked Box; both outlive this fn and - // are accessed under `rfs.entries_mutex` (see LIFETIMES.tsv). - let mut new_entry = Fs::file_system::DirEntry::init( - if let Some(existing) = in_place { - // SAFETY: see block-wide note above. - unsafe { &*existing }.dir - } else { - Fs::file_system::DirnameStore::instance() - .append_slice(dir_path) - .expect("unreachable") - }, - self.generation, - ); - - // Pre-size `data` so the per-entry inserts below skip the + // Pre-size the listing so the per-entry inserts skip the // 1→2→4→…→N hashbrown rehash cascade from an empty table. 64 // covers a typical node_modules package dir; larger dirs still // rehash from there (cheap relative to starting at 0). - new_entry.data.reserve(64); - - let mut dir_iterator = bun_sys::iterate_dir(open_dir); - // Hoist the `FilenameStore` singleton resolve out of the per-entry loop - // (see `DirEntry::add_entry` doc-comment) and reuse the appender state. - let mut filename_store = FilenameStoreAppender::new(); - while let Ok(Some(_value)) = dir_iterator.next() { - new_entry - .add_entry_with_store( - // SAFETY: see block-wide note above. - in_place.map(|existing| unsafe { &mut (*existing).data }), - &_value, - &mut filename_store, - (), - ) - .expect("unreachable"); - } - if let Some(existing) = in_place { - // SAFETY: see block-wide note above. - // NOTE: `StringHashMap` (std::HashMap newtype) - // has no separate `clear_and_free`; `clear()` drops all entries. - unsafe { &mut *existing }.data.clear(); - } - - if self.store_fd { - new_entry.fd = open_dir; - } - // NOTE: see `dir_info_cached_maybe_log` — `DirEntry.data` holds a `NonNull`, - // so a zeroed slot is UB; box `new_entry` directly for the fresh case. - let dir_entries_ptr = match in_place { - Some(p) => { - // SAFETY: dir_entries_ptr is a live BSSMap slot (`in_place`). - unsafe { *p = new_entry }; - p + let listing = match rfs!().read_listing( + in_place, + dir_path, + self.generation, + &mut open_dir, + true, + self.store_fd, + 64, + (), + ) { + Ok(listing) => listing, + Err(err) => { + open_dir.close(); + self.dir_read_failure = Some(Fs::DirReadFailure::new(dir_path, err)); + return Err(err); } - None => bun_core::heap::into_raw(Box::new(new_entry)), }; - - bun_core::scoped_log!( - crate::fs_full::Fs, - "readdir({}, {}) = {}", - open_dir, - bstr::BStr::new(dir_path), - // SAFETY: `dir_entries_ptr` is a live BSSMap slot (`in_place`) or a freshly - // boxed entry (see block-wide note above). - unsafe { (*dir_entries_ptr).data.count() }, - ); - - dir_entries_option = rfs!() - .entries - .put( - &mut cached_dir_entry_result, - Fs::file_system::real_fs::EntriesOption::Entries( - // SAFETY: `dir_entries_ptr` is a live BSSMap slot (`in_place`) or a freshly boxed entry. - unsafe { &mut *dir_entries_ptr }, - ), - ) - .expect("unreachable"); + dir_entries_option = + rfs!().commit_listing(&mut cached_dir_entry_result, in_place, listing)?; } // We must initialize it as empty so that the result index is correct. @@ -4429,7 +4447,7 @@ impl<'a> Resolver<'a> { let queue_top_safe_path: &[u8] = qt_safe_path.slice(); queue_slice_len -= 1; - let open_dir: FD = if queue_top.fd.is_valid() { + let mut open_dir: FD = if queue_top.fd.is_valid() { queue_top.fd } else { 'open_dir: { @@ -4508,17 +4526,15 @@ impl<'a> Resolver<'a> { ); break 'open_dir FD::INVALID; } - let cached_dir_entry_result = rfs!() - .entries - .get_or_put(queue_top_unsafe_path) - .expect("unreachable"); // If we don't properly cache not found, then we repeatedly attempt to open the same directories, // which causes a perf trace that looks like this stupidity; // // openat(dfd: CWD, filename: "node_modules/react", flags: RDONLY|DIRECTORY) = -1 ENOENT (No such file or directory) // ... self.dir_cache_mut().mark_not_found(queue_top.result); - rfs!().entries.mark_not_found(cached_dir_entry_result); + rfs!() + .mark_dir_not_found(queue_top_unsafe_path) + .expect("unreachable"); if err != crate::Error::Sys(bun_errno::SystemErrno::ENOENT) { if enable_logging { let pretty = queue_top_unsafe_path; @@ -4615,90 +4631,78 @@ impl<'a> Resolver<'a> { } if needs_iter { - // SAFETY: (block-wide) `in_place`/`dir_entries_ptr`/`dir_entries_option` point to - // slots in `rfs.entries` (BSSMap singleton) or a fresh leaked Box; both outlive this - // fn and are accessed under `rfs.entries_mutex` (see LIFETIMES.tsv). - let mut new_entry = Fs::file_system::DirEntry::init( - if let Some(existing) = in_place { - // SAFETY: see block-wide note above. - unsafe { &*existing }.dir - } else { - Fs::file_system::DirnameStore::instance() - .append_slice(dir_path) - .expect("unreachable") - }, - self.generation, - ); - - // Pre-size `data` so the per-entry inserts below skip the - // 1→2→4→…→N hashbrown rehash cascade from an empty table. 64 - // covers a typical node_modules package dir; larger dirs - // still rehash from there (cheap relative to starting at 0). - new_entry.data.reserve(64); - // A permission-denied ancestor has no fd to enumerate; its // entry set stays empty. + let mut listing = None; if open_dir.is_valid() { - let mut dir_iterator = bun_sys::iterate_dir(open_dir); - // NOTE: `WrappedIterator::next` returns - // `Result>`, so use `?`-style break-on-error. - // Hoist the `FilenameStore` singleton resolve out of the per-entry loop - // (see `DirEntry::add_entry` doc-comment) and reuse the appender state. - let mut filename_store = FilenameStoreAppender::new(); - loop { - let _value = match dir_iterator.next() { - Ok(Some(v)) => v, - Ok(None) => break, - Err(_) => break, - }; - new_entry - .add_entry_with_store( - // SAFETY: see block-wide note above. - in_place.map(|existing| unsafe { &mut (*existing).data }), - &_value, - &mut filename_store, - (), - ) - .expect("unreachable"); + let opened_here = !queue_top.fd.is_valid(); + // Pre-size the listing so the per-entry inserts skip the + // 1→2→4→…→N hashbrown rehash cascade from an empty table. 64 + // covers a typical node_modules package dir; larger dirs + // still rehash from there (cheap relative to starting at 0). + let read = rfs!().read_listing( + in_place, + dir_path, + self.generation, + &mut open_dir, + opened_here, + self.store_fd, + 64, + (), + ); + if opened_here { + // `read_listing` replaces a handle whose read it tried again. + bufs!(open_dirs)[open_dir_count.get() - 1] = open_dir; } - } - if let Some(existing) = in_place { - // SAFETY: see block-wide note above. - // NOTE: bun_collections::StringHashMap exposes `clear`, which drops all entries. - unsafe { &mut *existing }.data.clear(); - } - new_entry.fd = if self.store_fd { open_dir } else { FD::INVALID }; - // NOTE: `DirEntry.data` is a `HashMap` - // (`NonNull` inside), so a zeroed slot is UB and `*ptr = new_entry` would drop it. - // Box `new_entry` directly for the fresh case; assign-into only for `in_place`. - let dir_entries_ptr = match in_place { - Some(p) => { - // SAFETY: dir_entries_ptr is a live BSSMap slot (`in_place`). - unsafe { *p = new_entry }; - p + match read { + Ok(read) => listing = Some(read), + Err(err) => { + // No listing holds the handle, so it is closed here, also when `store_fd`. + if opened_here { + open_dir.close(); + open_dir_count.set(open_dir_count.get() - 1); + } + open_dir = FD::INVALID; + match err { + // The directory went away while it was open: the outcome of a failed open. + crate::Error::Sys(bun_errno::SystemErrno::ENOTDIR) => { + return Ok(None); + } + crate::Error::Sys(bun_errno::SystemErrno::ENOENT) => { + self.dir_cache_mut().mark_not_found(queue_top.result); + rfs!() + .mark_dir_not_found(queue_top_unsafe_path) + .expect("unreachable"); + return Ok(None); + } + // An ancestor that opens but may not be listed is + // opaque and empty, like one that does not open. + crate::Error::Sys( + bun_errno::SystemErrno::EPERM | bun_errno::SystemErrno::EACCES, + ) if queue_slice_len > 0 => { + debuglog!( + "treating permission-denied ancestor \"{}\" as empty: {}", + bstr::BStr::new(queue_top_unsafe_path), + bstr::BStr::new(err.name()) + ); + } + // Nothing is cached for this directory or the ones + // below it, so the next lookup reads it again. + _ => { + self.dir_read_failure = + Some(Fs::DirReadFailure::new(dir_path, err)); + return Err(err); + } + } + } } - None => bun_core::heap::into_raw(Box::new(new_entry)), + } + let listing = match listing { + Some(listing) => listing, + None => rfs!().opaque_listing(in_place, dir_path, self.generation)?, }; - // NOTE (Stacked Borrows): log BEFORE `entries.put` stores the - // `&'static mut DirEntry` — a later read through the parent raw - // pointer would pop that reference's Unique tag (the ordering - // is unobservable). - bun_core::scoped_log!( - crate::fs_full::Fs, - "readdir({}, {}) = {}", - open_dir, - bstr::BStr::new(dir_path), - // SAFETY: `dir_entries_ptr` is a live BSSMap slot (`in_place`) or a - // freshly boxed entry (see block-wide note above). - unsafe { (*dir_entries_ptr).data.count() }, - ); - dir_entries_option = rfs!().entries.put( - &mut cached_dir_entry_result, - Fs::file_system::real_fs::EntriesOption::Entries( - // SAFETY: `dir_entries_ptr` is a live BSSMap slot (`in_place`) or a freshly boxed entry. - unsafe { &mut *dir_entries_ptr }, - ), - )?; + dir_entries_option = + rfs!().commit_listing(&mut cached_dir_entry_result, in_place, listing)?; } // We must initialize it as empty so that the result index is correct. @@ -5581,16 +5585,7 @@ impl<'a> Resolver<'a> { // back to this same BSSMap slot — holding a `&mut` here would alias. let dir_info: DirInfoRef = match self.dir_info_cached(path) { Ok(Some(d)) => d, - Ok(None) => dec_ret!(MatchStatus::NotFound), - Err(_err) => { - #[cfg(debug_assertions)] - bun_core::pretty_errorln!( - "err: {} reading {}", - bstr::BStr::new(_err.name()), - bstr::BStr::new(path) - ); - dec_ret!(MatchStatus::NotFound); - } + Ok(None) | Err(_) => dec_ret!(MatchStatus::NotFound), }; let mut package_json: Option<*const PackageJSON> = None; @@ -5798,11 +5793,11 @@ impl<'a> Resolver<'a> { // holder by ARENA invariant). // SAFETY: `rfs` points at the process-global RealFS singleton (see note at fn top). let dir_entry: bun_ptr::BackRef = - match unsafe { &mut *rfs }.read_directory( + match unsafe { &mut *rfs }.read_directory_for_resolution( dir_path, - None, self.generation, self.store_fd, + &mut self.dir_read_failure, ) { Ok(e) => bun_ptr::BackRef::new(&*e), Err(_) => dec_ret!(None), @@ -5812,6 +5807,8 @@ impl<'a> Resolver<'a> { match err.original_err { crate::Error::Sys(bun_errno::SystemErrno::ENOENT) | crate::Error::Sys(bun_errno::SystemErrno::ENOTDIR) => {} + // `resolve_and_auto_install` reports a read that failed. + _ if self.dir_read_failure.is_some() => {} _ => { let _ = self.log_mut().add_error_fmt( None, From 1c5ee483153b38fa591ca121c311d4a145ff0c86 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 3 Oct 2026 06:01:08 +0000 Subject: [PATCH 2/3] test: a directory read that fails part-way in the resolver --- test/js/bun/resolve/resolve.test.ts | 353 +++++++++++++++++++++++++++- 1 file changed, 350 insertions(+), 3 deletions(-) diff --git a/test/js/bun/resolve/resolve.test.ts b/test/js/bun/resolve/resolve.test.ts index 1d4f61098111..a6fa71c8b056 100644 --- a/test/js/bun/resolve/resolve.test.ts +++ b/test/js/bun/resolve/resolve.test.ts @@ -1,7 +1,28 @@ import { pathToFileURL } from "bun"; -import { describe, expect, it, test } from "bun:test"; -import { chmodSync, chownSync, mkdirSync, readFileSync, realpathSync, symlinkSync, writeFileSync } from "fs"; -import { bunEnv, bunExe, bunRun, isLinux, isMacOS, isWindows, joinP, tempDir, tempDirWithFiles } from "harness"; +import { afterAll, beforeAll, describe, expect, it, test } from "bun:test"; +import { + chmodSync, + chownSync, + existsSync, + mkdirSync, + readFileSync, + realpathSync, + symlinkSync, + writeFileSync, +} from "fs"; +import { + bunEnv, + bunExe, + bunRun, + isGlibc, + isLinux, + isMacOS, + isWindows, + joinP, + tempDir, + tempDirWithFiles, +} from "harness"; +import { constants } from "os"; import { join, resolve, sep } from "path"; const fixture = (...segs: string[]) => resolve(import.meta.dir, "fixtures", ...segs); @@ -1961,3 +1982,329 @@ describe.concurrent("dot specifiers resolve to the directory index, not a siblin expect(exitCode).toBe(1); }); }); + +// A read of a directory can fail after the directory opened: an I/O error, a +// network mount that went away. The resolver took that error as the end of the +// directory and cached the names it had read as the whole directory. +// +// The LD_PRELOAD shim below plays it for one directory. The first getdents64 +// of a handle returns the real records without one name, and each later +// getdents64 of that handle fails. bun issues getdents64 through libc's +// syscall(), which is the symbol the shim interposes, so this needs glibc. +// +// READDIR_FAULT_DIR the directory, as /proc/self/fd names it +// READDIR_FAULT_HIDE the name that the first read leaves out +// READDIR_FAULT_COUNT how many reads fail in total (default: every one) +// READDIR_FAULT_ERRNO the errno of a failed read (default: EIO) +// READDIR_FAULT_WHILE a path: reads fail only while it exists +const cc = Bun.which("cc") || Bun.which("gcc") || Bun.which("clang"); +describe.concurrent.skipIf(!isGlibc || !cc)("a directory read that fails", () => { + const shimSource = /* c */ ` +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct linux_dirent64 { + uint64_t d_ino; + int64_t d_off; + unsigned short d_reclen; + unsigned char d_type; + char d_name[]; +}; + +static long (*next_syscall)(long, ...); +static int reads_left = -2; /* -2: not read from the environment yet, -1: no limit */ + +static int fault_is_on(void) { + if (reads_left == -2) { + const char *count = getenv("READDIR_FAULT_COUNT"); + reads_left = count ? atoi(count) : -1; + } + if (reads_left == 0) return 0; + const char *while_exists = getenv("READDIR_FAULT_WHILE"); + return !while_exists || access(while_exists, F_OK) == 0; +} + +long syscall(long nr, ...) { + va_list ap; + va_start(ap, nr); + long a1 = va_arg(ap, long), a2 = va_arg(ap, long), a3 = va_arg(ap, long); + long a4 = va_arg(ap, long), a5 = va_arg(ap, long), a6 = va_arg(ap, long); + va_end(ap); + if (!next_syscall) next_syscall = dlsym(RTLD_NEXT, "syscall"); + const char *dir = nr == SYS_getdents64 ? getenv("READDIR_FAULT_DIR") : NULL; + if (dir && fault_is_on()) { + int fd = (int)a1; + char link[64], target[PATH_MAX]; + snprintf(link, sizeof link, "/proc/self/fd/%d", fd); + ssize_t n = readlink(link, target, sizeof target - 1); + if (n > 0 && (target[n] = 0, strcmp(target, dir) == 0)) { + if (lseek(fd, 0, SEEK_CUR) != 0) { + if (reads_left > 0) reads_left--; + const char *err = getenv("READDIR_FAULT_ERRNO"); + errno = err ? atoi(err) : EIO; + return -1; + } + long rc = next_syscall(nr, a1, a2, a3, a4, a5, a6); + const char *hide = getenv("READDIR_FAULT_HIDE"); + char *buf = (char *)a2; + for (long pos = 0; hide && pos < rc;) { + struct linux_dirent64 *d = (struct linux_dirent64 *)(buf + pos); + unsigned short len = d->d_reclen; + if (strcmp(d->d_name, hide) == 0) { + memmove(buf + pos, buf + pos + len, rc - pos - len); + rc -= len; + } else { + pos += len; + } + } + return rc; + } + } + return next_syscall(nr, a1, a2, a3, a4, a5, a6); +} +`; + + let shimDir: ReturnType | undefined; + let shim: string; + + beforeAll(() => { + shimDir = tempDir("resolver-readdir-fault-shim", { "shim.c": shimSource }); + shim = join(String(shimDir), "shim.so"); + const compile = Bun.spawnSync({ + cmd: [cc!, "-shared", "-fPIC", "-o", shim, join(String(shimDir), "shim.c"), "-ldl"], + env: bunEnv, + }); + if (compile.exitCode !== 0) { + throw new Error(`Failed to build the readdir fault shim:\n${compile.stderr.toString()}`); + } + }); + + afterAll(() => { + shimDir?.[Symbol.dispose](); + }); + + type Fault = { dir: string; hide?: string; count?: number; errno?: number; while?: string }; + + async function runWithFault(args: string[], cwd: string, fault: Fault) { + const existing = bunEnv.LD_PRELOAD; + const env: Record = { + ...bunEnv, + LD_PRELOAD: existing ? `${shim}:${existing}` : shim, + READDIR_FAULT_DIR: fault.dir, + }; + if (fault.hide !== undefined) env.READDIR_FAULT_HIDE = fault.hide; + if (fault.count !== undefined) env.READDIR_FAULT_COUNT = String(fault.count); + if (fault.errno !== undefined) env.READDIR_FAULT_ERRNO = String(fault.errno); + if (fault.while !== undefined) env.READDIR_FAULT_WHILE = fault.while; + await using proc = Bun.spawn({ cmd: [bunExe(), ...args], env, cwd, stdout: "pipe", stderr: "pipe" }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + // Resolves "dep" four ways. With a READDIR_FAULT_WHILE path it then removes + // that path and resolves once more. + const resolveDep = ` + const { unlinkSync } = require("fs"); + const attempt = fn => { + try { + return fn(); + } catch (e) { + return { error: e.message }; + } + }; + const out = { + import: await import("dep").then(m => m.default, e => ({ error: e.message })), + require: attempt(() => require("dep")), + resolveSync: attempt(() => Bun.resolveSync("dep", import.meta.dir)), + requireResolve: attempt(() => require.resolve("dep")), + }; + if (process.env.READDIR_FAULT_WHILE) { + unlinkSync(process.env.READDIR_FAULT_WHILE); + out.afterwards = attempt(() => require("dep")); + } + console.log(JSON.stringify(out)); + `; + + describe.each([ + // The directory walk reads the package directory. + { main: "real.js", faultDir: "node_modules/dep", hide: "package.json" }, + // The lookup of the "main" file reads the directory that holds it. + { main: "lib/real.js", faultDir: "node_modules/dep/lib", hide: "real.js" }, + ])('in a package with "main": "$main"', ({ main, faultDir, hide }) => { + const files = { + "entry.js": resolveDep, + "node_modules/dep/package.json": JSON.stringify({ name: "dep", version: "1.0.0", main }), + [`node_modules/dep/${main}`]: `module.exports = "the main of package.json";`, + "node_modules/dep/index.js": `module.exports = "index.js, which is not the main";`, + }; + + it("one failed read does not change what resolves", async () => { + using dir = tempDir("resolver-readdir-fault-once", files); + const root = realpathSync(String(dir)); + const result = await runWithFault(["entry.js"], root, { dir: join(root, faultDir), hide, count: 1 }); + expect({ ...result, stdout: JSON.parse(result.stdout || "null") }).toEqual({ + stdout: { + import: "the main of package.json", + require: "the main of package.json", + resolveSync: join(root, "node_modules/dep", main), + requireResolve: join(root, "node_modules/dep", main), + }, + stderr: "", + exitCode: 0, + }); + }); + + it("a read that keeps failing is an error that names the directory, and nothing is cached", async () => { + using dir = tempDir("resolver-readdir-fault-persistent", { ...files, "fault-is-on": "" }); + const root = realpathSync(String(dir)); + const result = await runWithFault(["entry.js"], root, { + dir: join(root, faultDir), + hide, + while: join(root, "fault-is-on"), + }); + const error = { error: `Cannot read directory "${join(root, faultDir)}": EIO while resolving "dep"` }; + expect({ ...result, stdout: JSON.parse(result.stdout || "null") }).toEqual({ + stdout: { + import: error, + require: error, + resolveSync: error, + requireResolve: error, + afterwards: "the main of package.json", + }, + stderr: "", + exitCode: 0, + }); + }); + }); + + // src/x.js is what "./src/x" resolves to when the listing of src has no x.ts. + const staleSibling = { + "entry.ts": `import x from "./src/x";\nconsole.log(x);\n`, + "src/x.ts": `export default "x.ts, the current source";\n`, + "src/x.js": `export default "x.js, a stale build output";\n`, + }; + + it.each(["browser", "bun"])("bun build --target=%s fails when the read keeps failing", async target => { + using dir = tempDir("resolver-readdir-fault-build", staleSibling); + const root = realpathSync(String(dir)); + const { stderr, exitCode } = await runWithFault( + ["build", "entry.ts", `--target=${target}`, "--outfile=out.js"], + root, + { dir: join(root, "src"), hide: "x.ts" }, + ); + expect(stderr).toContain(`error: Cannot read directory "${join(root, "src")}": EIO while resolving "./src/x"`); + expect(existsSync(join(root, "out.js"))).toBe(false); + expect(exitCode).toBe(1); + }); + + it("bun build bundles the same file after one failed read", async () => { + using dir = tempDir("resolver-readdir-fault-build-once", staleSibling); + const root = realpathSync(String(dir)); + const { stderr, exitCode } = await runWithFault(["build", "entry.ts", "--outfile=out.js"], root, { + dir: join(root, "src"), + hide: "x.ts", + count: 1, + }); + expect(stderr).not.toContain("error"); + expect(readFileSync(join(root, "out.js"), "utf8")).toContain("x.ts, the current source"); + expect(exitCode).toBe(0); + }); + + it("Bun.build keeps the listing of the last build when a later read fails", async () => { + using dir = tempDir("resolver-readdir-fault-rebuild", { + ...staleSibling, + "build.js": ` + const { unlinkSync, writeFileSync } = require("fs"); + const faultIsOn = process.env.READDIR_FAULT_WHILE; + async function build() { + try { + const result = await Bun.build({ entrypoints: ["./entry.ts"] }); + const text = await result.outputs[0].text(); + return text.includes("x.ts, the current source") ? "x.ts" : text; + } catch (e) { + return (e.errors ?? [e]).map(e => e.message); + } + } + const out = { healthy: await build() }; + writeFileSync(faultIsOn, ""); + out.failing = await build(); + unlinkSync(faultIsOn); + out.healthyAgain = await build(); + console.log(JSON.stringify(out)); + `, + }); + const root = realpathSync(String(dir)); + const result = await runWithFault(["build.js"], root, { + dir: join(root, "src"), + hide: "x.ts", + while: join(root, "fault-is-on"), + }); + expect({ ...result, stdout: JSON.parse(result.stdout || "null") }).toEqual({ + stdout: { + healthy: "x.ts", + failing: [`Cannot read directory "${join(root, "src")}": EIO while resolving "./src/x"`], + healthyAgain: "x.ts", + }, + stderr: "", + exitCode: 0, + }); + }); + + it("bun run --filter finds every workspace after one failed read of the root", async () => { + const script = (name: string) => JSON.stringify({ name, scripts: { hello: `echo hello from ${name}` } }); + using dir = tempDir("resolver-readdir-fault-filter", { + "package.json": JSON.stringify({ name: "root", workspaces: ["packages/*"] }), + "packages/a/package.json": script("a"), + "packages/b/package.json": script("b"), + "packages/c/package.json": script("c"), + }); + const root = realpathSync(String(dir)); + const { stdout, stderr, exitCode } = await runWithFault(["run", "--filter", "*", "hello"], root, { + dir: root, + hide: "packages", + count: 1, + }); + const ran = (stdout + stderr) + .split("\n") + .filter(line => line.includes("hello from")) + .sort(); + expect(ran).toEqual(["a hello: hello from a", "b hello: hello from b", "c hello: hello from c"]); + expect(exitCode).toBe(0); + }); + + const belowAncestor = { + "outer/project/index.js": `console.log(require("./dep.js"));`, + "outer/project/dep.js": `module.exports = "loaded";`, + }; + + // The same outcome as an ancestor that may not be opened. + it("an ancestor that may not be listed is an empty directory", async () => { + using dir = tempDir("resolver-readdir-fault-eacces-ancestor", belowAncestor); + const root = realpathSync(String(dir)); + const result = await runWithFault(["--no-install", "index.js"], join(root, "outer/project"), { + dir: join(root, "outer"), + errno: constants.errno.EACCES, + }); + expect(result).toEqual({ stdout: "loaded\n", stderr: "", exitCode: 0 }); + }); + + it("an ancestor whose read keeps failing fails the resolutions below it", async () => { + using dir = tempDir("resolver-readdir-fault-eio-ancestor", belowAncestor); + const root = realpathSync(String(dir)); + const { stdout, stderr, exitCode } = await runWithFault(["--no-install", "index.js"], join(root, "outer/project"), { + dir: join(root, "outer"), + }); + expect(stderr).toContain(`Cannot read directory "${join(root, "outer")}": EIO`); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); + }); +}); From 5dc16c4becd5c8a3f76b58dbbe7c42210ec2bf07 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 3 Oct 2026 09:00:32 +0000 Subject: [PATCH 3/3] resolver: a failed directory read leaves nothing in the append-only stores The directory walk interned the path of its input once the first directory opened. A read that failed after that left the path in DirnameStore, once for each attempt. The walk now interns the path after the directory is listed. RealFS keeps what failed reads interned for each directory, and not only for the last one, until a read of that directory succeeds. --- src/resolver/lib.rs | 57 +++++++++++-------- src/resolver/resolver.rs | 120 +++++++++++++++++++++++---------------- 2 files changed, 104 insertions(+), 73 deletions(-) diff --git a/src/resolver/lib.rs b/src/resolver/lib.rs index 2de470cdd695..c739b8c2fd31 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -1073,10 +1073,11 @@ pub mod fs { /// this directly (`rfs.entries.get_or_put(..)`); modeled as the wrapper /// `EntriesMap` (bun_alloc has no BSSMap equivalent). pub entries: EntriesMap, - /// The entries of the last directory whose read failed, which - /// `read_listing` uses again when it reads that directory. Guarded by - /// `entries_mutex`. - failed_listing: Option, + /// What the failed reads of a directory interned, until a read of it + /// succeeds. `EntryStore`, `FilenameStore` and `DirnameStore` only + /// grow, so `read_listing` uses these entries and this path again + /// when it reads the directory. Guarded by `entries_mutex`. + failed_listings: bun_collections::StringHashMap, pub(crate) cwd: &'static [u8], #[cfg(not(windows))] pub(crate) file_limit: usize, @@ -1093,7 +1094,7 @@ pub mod fs { RealFS { entries_mutex: Mutex::default(), entries: EntriesMap::new(), - failed_listing: None, + failed_listings: Default::default(), cwd, #[cfg(not(windows))] file_limit, @@ -1195,6 +1196,9 @@ pub mod fs { /// Every listing that `commit_listing` publishes comes from here, so /// the cache never holds the names of a read that stopped early. /// + /// A new listing is named `dir_path`, followed by a separator when + /// `dir_needs_sep`. A stale one (`in_place`) keeps its name. + /// /// A read that fails is tried once more, on a handle opened again by /// path, so a transient error does not reach the caller. When /// `owns_handle`, that handle replaces `*handle` and the old one is @@ -1207,6 +1211,7 @@ pub mod fs { &mut self, in_place: Option<*mut DirEntry>, dir_path: &[u8], + dir_needs_sep: bool, generation: Generation, handle: &mut Fd, owns_handle: bool, @@ -1214,7 +1219,7 @@ pub mod fs { reserve: usize, iterator: I, ) -> crate::CrateResult { - let (dir, mut interned) = self.listing_dir(in_place, dir_path)?; + let (dir, mut interned) = self.listing_dir(in_place, dir_path, dir_needs_sep)?; let mut listing = DirEntry::init(dir, generation); listing.data.reserve(reserve); if store_fd { @@ -1248,30 +1253,30 @@ pub mod fs { ) } - /// The interned path for a new listing of `dir_path`, and the entries - /// that an earlier failed read of the same directory already interned. + /// The interned name for a listing of `dir_path`, and what the failed + /// reads of that directory left in `failed_listings`. fn listing_dir( &mut self, in_place: Option<*mut DirEntry>, dir_path: &[u8], + dir_needs_sep: bool, ) -> crate::CrateResult<(&'static [u8], Option)> { // SAFETY: `in_place` points to a `DirEntry` inside the BSSMap // singleton; its `dir` field is DirnameStore-interned (&'static). let stale_dir = in_place.map(|p| unsafe { (*p).dir }); - let interned = match &self.failed_listing { - Some(failed) - if strings::paths::without_trailing_slash_windows_path(failed.dir) - == strings::paths::without_trailing_slash_windows_path( - stale_dir.unwrap_or(dir_path), - ) => - { - self.failed_listing.take() - } - _ => None, + let interned = if self.failed_listings.is_empty() { + None + } else { + self.failed_listings + .remove(strings::paths::without_trailing_slash_windows_path( + stale_dir.unwrap_or(dir_path), + )) }; let dir = match (stale_dir, &interned) { (Some(dir), _) => dir, (None, Some(failed)) => failed.dir, + (None, None) if dir_needs_sep => DirnameStore::instance() + .append_parts(&[dir_path, bun_paths::SEP_STR.as_bytes()])?, (None, None) => DirnameStore::instance().append_slice(dir_path)?, }; Ok((dir, interned)) @@ -1292,9 +1297,9 @@ pub mod fs { store_fd: bool, try_again: bool, ) -> crate::CrateResult { - // `EntryStore` and `FilenameStore` only grow. `failed.data` collects - // every entry interned for this directory, so that the next read of - // it, here or in a later lookup, interns only names it has not seen. + // `failed.data` collects every entry interned for this directory, so + // that the next read of it, here or in a later lookup, interns only + // the names it has not seen. match &interned { Some(earlier) => Self::keep_entries(&mut failed.data, &earlier.data)?, None => { @@ -1329,7 +1334,10 @@ pub mod fs { } } - self.failed_listing = Some(failed); + self.failed_listings.put( + strings::paths::without_trailing_slash_windows_path(failed.dir), + failed, + )?; Err(err) } @@ -1385,9 +1393,10 @@ pub mod fs { &mut self, in_place: Option<*mut DirEntry>, dir_path: &[u8], + dir_needs_sep: bool, generation: Generation, ) -> crate::CrateResult { - let (dir, _) = self.listing_dir(in_place, dir_path)?; + let (dir, _) = self.listing_dir(in_place, dir_path, dir_needs_sep)?; Ok(DirEntry::init(dir, generation)) } @@ -1575,6 +1584,7 @@ pub mod fs { } else { dir_maybe_trail_slash }, + false, generation, &mut handle, !had_handle, @@ -1882,6 +1892,7 @@ pub mod fs { let read = self.read_listing( Some(e_ptr), dir, + false, generation, &mut handle, true, diff --git a/src/resolver/resolver.rs b/src/resolver/resolver.rs index 1cdf0e9f52f2..8b6f7a09c939 100644 --- a/src/resolver/resolver.rs +++ b/src/resolver/resolver.rs @@ -3443,6 +3443,7 @@ impl<'a> Resolver<'a> { let listing = match rfs!().read_listing( in_place, dir_path, + false, self.generation, &mut open_dir, true, @@ -4565,54 +4566,17 @@ impl<'a> Resolver<'a> { open_dir_count.set(open_dir_count.get() + 1); } - let dir_path: &'static [u8] = if !queue_top_safe_path.is_empty() { - // SAFETY: non-empty `safe_path` is always a dirname_store-backed - // `&'static [u8]` (set from `entries.dir` above); widen the - // `RawSlice`-tied borrow back to its true `'static` lifetime. - unsafe { bun_ptr::detach_lifetime(queue_top_safe_path) } - } else { - // ensure trailing slash - if _safe_path.is_none() { - // Now that we've opened the topmost directory successfully, it's reasonable to store the slice. - // `path` spans `input_path_len + 1` for the NUL-splice above; the - // logical input is `path[..input_path_len]`. - 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)?); - } else { - _safe_path = Some(self.fs_ref().dirname_store.append_slice(input)?); - } - } - - let safe_path = _safe_path.unwrap(); - - // An empty needle must yield index 0, not None. On Windows - // `queue_top_unsafe_path` is empty when - // `windows_filesystem_root` cannot classify the input — e.g. - // `import(":://x")` is "absolute" per std but has no drive root, - // so `root_path` is `path[0..0]`. Treat that as 0 so the - // resolver caches a not-found instead of panicking. - let dir_path_i = if queue_top_unsafe_path.is_empty() { - 0 + // The key only. The path itself is interned below, once the + // directory is listed: a walk that stops at a failed read leaves + // nothing in `DirnameStore`. + let mut cached_dir_entry_result = rfs!() + .entries + .get_or_put(if !queue_top_safe_path.is_empty() { + queue_top_safe_path } else { - strings::index_of(safe_path, queue_top_unsafe_path).expect("unreachable") - }; - let mut end = dir_path_i + queue_top_unsafe_path.len(); - - // Directories must always end in a trailing slash or else various bugs can occur. - // This covers "what happens when the trailing" - end += usize::from( - safe_path.len() > end - && end > 0 - && safe_path[end - 1] != SEP - && safe_path[end] == SEP, - ); - &safe_path[dir_path_i..end] - }; - - let mut cached_dir_entry_result = - rfs!().entries.get_or_put(dir_path).expect("unreachable"); + queue_top_unsafe_path + }) + .expect("unreachable"); let mut dir_entries_option: *mut Fs::file_system::real_fs::EntriesOption = core::ptr::null_mut(); @@ -4631,6 +4595,10 @@ impl<'a> Resolver<'a> { } if needs_iter { + // Directories must always end in a trailing slash or else various bugs can occur. + let dir_needs_sep = queue_top_unsafe_path + .last() + .is_some_and(|&last| last != SEP); // A permission-denied ancestor has no fd to enumerate; its // entry set stays empty. let mut listing = None; @@ -4642,7 +4610,8 @@ impl<'a> Resolver<'a> { // still rehash from there (cheap relative to starting at 0). let read = rfs!().read_listing( in_place, - dir_path, + queue_top_unsafe_path, + dir_needs_sep, self.generation, &mut open_dir, opened_here, @@ -4690,7 +4659,7 @@ impl<'a> Resolver<'a> { // below it, so the next lookup reads it again. _ => { self.dir_read_failure = - Some(Fs::DirReadFailure::new(dir_path, err)); + Some(Fs::DirReadFailure::new(queue_top_unsafe_path, err)); return Err(err); } } @@ -4699,12 +4668,63 @@ impl<'a> Resolver<'a> { } let listing = match listing { Some(listing) => listing, - None => rfs!().opaque_listing(in_place, dir_path, self.generation)?, + None => rfs!().opaque_listing( + in_place, + queue_top_unsafe_path, + dir_needs_sep, + self.generation, + )?, }; dir_entries_option = rfs!().commit_listing(&mut cached_dir_entry_result, in_place, listing)?; } + let dir_path: &'static [u8] = if !queue_top_safe_path.is_empty() { + // SAFETY: non-empty `safe_path` is always a dirname_store-backed + // `&'static [u8]` (set from `entries.dir` above); widen the + // `RawSlice`-tied borrow back to its true `'static` lifetime. + unsafe { bun_ptr::detach_lifetime(queue_top_safe_path) } + } else { + // ensure trailing slash + if _safe_path.is_none() { + // Now that we've listed the topmost directory successfully, it's reasonable to store the slice. + // `path` spans `input_path_len + 1` for the NUL-splice above; the + // logical input is `path[..input_path_len]`. + 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)?); + } else { + _safe_path = Some(self.fs_ref().dirname_store.append_slice(input)?); + } + } + + let safe_path = _safe_path.unwrap(); + + // An empty needle must yield index 0, not None. On Windows + // `queue_top_unsafe_path` is empty when + // `windows_filesystem_root` cannot classify the input — e.g. + // `import(":://x")` is "absolute" per std but has no drive root, + // so `root_path` is `path[0..0]`. Treat that as 0 so the + // resolver caches a not-found instead of panicking. + let dir_path_i = if queue_top_unsafe_path.is_empty() { + 0 + } else { + strings::index_of(safe_path, queue_top_unsafe_path).expect("unreachable") + }; + let mut end = dir_path_i + queue_top_unsafe_path.len(); + + // Directories must always end in a trailing slash or else various bugs can occur. + // This covers "what happens when the trailing" + end += usize::from( + safe_path.len() > end + && end > 0 + && safe_path[end - 1] != SEP + && safe_path[end] == SEP, + ); + &safe_path[dir_path_i..end] + }; + // We must initialize it as empty so that the result index is correct. // This is important so that browser_scope has a valid index. // SAFETY: `dir_cache()` is the live singleton; resolver mutex held.