Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
27 changes: 17 additions & 10 deletions src/bundler/transpiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -766,16 +766,23 @@ impl<'a> Transpiler<'a> {
merge_tsconfig_jsx_into(tsconfig, &mut self.options.jsx);
}

let Some(dir) = dir_info.get_entries(self.resolver.generation) else {
return Ok(());
// Copy the listing's basenames out under `entries_mutex`,
// refreshing it at our generation in the same critical
// section: concurrent resolvers rewrite the `DirEntry` map in
// place under that lock, and `dot_env::Loader::load` does
// file I/O between probes.
Comment thread
robobun marked this conversation as resolved.
let dir = {
let _entries_lock = bun_resolver::fs::FileSystem::instance()
.fs
.entries_mutex
.lock_guard();
match dir_info.get_entries_ref_locked(self.resolver.generation) {
Some(entries) => dot_env::DirEntryKeys(
entries.data.iter().map(|(k, _)| Box::from(&**k)).collect(),
),
None => return Ok(()),
}
};
// `get_entries` returns `*mut bun_resolver::fs::DirEntry`
// (BSSMap-owned). `dot_env::Loader::load` takes
// `impl DirEntryProbe` (bun_dotenv sits below `bun_resolver`
// in the crate graph); `bun_resolver::fs::DirEntry` impls it.
// SAFETY: BSSMap singleton owns `*dir`; single-threaded path —
// sole `&mut` for the call.
let dir: &mut bun_resolver::fs::DirEntry = unsafe { &mut *dir };

// `Env.files: Box<[Box<[u8]>]>` but `Loader::load`
// wants `&[&[u8]]`. Re-borrow into a small Vec; the explicit
Expand All @@ -789,7 +796,7 @@ impl<'a> Transpiler<'a> {
} else {
dot_env::DotEnvFileSuffix::Development
};
env.load(dir, &env_files, suffix, skip_default_env)?;
env.load(&dir, &env_files, suffix, skip_default_env)?;
}
DotEnvBehavior::disable => {
env.load_process()?;
Expand Down
23 changes: 14 additions & 9 deletions src/dotenv/env_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,26 @@
Test,
}

/// Directory-entry probe used by `Loader::load`. `bun_dotenv` sits below
/// `bun_resolver` in the crate graph, so the concrete
/// `bun_resolver::fs::DirEntry` is taken generically; the only operation
/// `load_default_files` performs is a fast O(1) lookup of a
/// known-at-compile-time filename in the directory's entry map. Implemented
/// for `bun_resolver::fs::DirEntry`.
/// `bun_resolver` in the crate graph, so the directory listing is taken
/// generically; the only operation `load_default_files` performs is a lookup
/// of a known-at-compile-time filename. Callers snapshot the resolver's
/// listing into a [`DirEntryKeys`] (the live `DirEntry` map may be rewritten
/// in place by a concurrent resolver, so `load` must not probe it directly).

Check warning on line 29 in src/dotenv/env_loader.rs

View check run for this annotation

Claude / Claude Code Review

Stale comment in load_default_files claims removed DirEntryProbe impl still exists

nit: the inline comment inside `load_default_files` (src/dotenv/env_loader.rs:690-692) still says "`bun_resolver::fs::DirEntry` impls `DirEntryProbe`", but this PR deletes that impl from src/resolver/fs.rs. The trait-level doc at :24-29 was correctly updated to point at `DirEntryKeys`; this one was missed and should be updated or removed in the same PR.
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
pub trait DirEntryProbe {
/// The argument MUST already be ASCII-lowercase.
fn has_comptime_query(&self, query_lower: &'static [u8]) -> bool;
}

// LAYERING: the concrete `DirEntry` lives in `bun_resolver::fs` (higher tier,
// depends on this crate). `impl DirEntryProbe for bun_resolver::fs::DirEntry`
// is provided there — see src/resolver/lib.rs. No impl here; that would be a
// dep-cycle.
/// Directory-listing basenames copied out under the resolver's
/// `entries_mutex`, probed between the `.env` file reads in `Loader::load`.
Comment thread
robobun marked this conversation as resolved.
pub struct DirEntryKeys(pub Vec<Box<[u8]>>);

impl DirEntryProbe for DirEntryKeys {
fn has_comptime_query(&self, query_lower: &'static [u8]) -> bool {
self.0.iter().any(|k| **k == *query_lower)
}
}

/// Canonical definition; re-exported as
/// `bun_options_types::schema::api::DotEnvBehavior` for higher tiers.
Expand Down
6 changes: 3 additions & 3 deletions src/dotenv/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ pub mod error;
pub use error::{Error, Result};

pub use env_loader::{
DirEntryProbe, DotEnvBehavior, DotEnvFileSuffix, HAS_NO_CLEAR_SCREEN_CLI_FLAG, HashTable,
HashTableValue, INSTANCE, Loader, Map, NullDelimitedEnvMap, S3Credentials, StdEnvMapWrapper,
instance, set_instance,
DirEntryKeys, DirEntryProbe, DotEnvBehavior, DotEnvFileSuffix, HAS_NO_CLEAR_SCREEN_CLI_FLAG,
HashTable, HashTableValue, INSTANCE, Loader, Map, NullDelimitedEnvMap, S3Credentials,
StdEnvMapWrapper, instance, set_instance,
};

/// `dotenv::map::{HashTable, Entry}` namespace expected by `install_jsc::ini_jsc` et al.
Expand Down
24 changes: 19 additions & 5 deletions src/install/PackageManager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1736,11 +1736,20 @@ pub fn init(
};

env.load_process()?;
// Reborrow the BSSMap-owned `*DirEntry` for the
// call; `env.load` only reads it (`hasComptimeQuery` lookups for `.env*`).
// Copy the listing's basenames out under `entries_mutex`; `.data` must
// only be probed while the lock is held.
Comment thread
robobun marked this conversation as resolved.
let env_probe_keys = {
let _entries_lock = FileSystem::instance().fs.entries_mutex.lock_guard();
dot_env::DirEntryKeys(
entries_option
.data
.iter()
.map(|(k, _)| Box::from(&**k))
.collect(),
)
};
env.load(
// SAFETY: see `entries_option` above — single-threaded init, BSSMap-owned.
unsafe { &mut *std::ptr::from_mut::<fs::DirEntry>(entries_option) },
&env_probe_keys,
&[],
dot_env::DotEnvFileSuffix::Production,
false,
Expand Down Expand Up @@ -2454,7 +2463,12 @@ fn init_with_runtime_once(
// `root_dir` was moved into `*manager` above (the field is
// an unbounded `&mut DirEntry`, so the local reborrow is for `'static` and the
// original binding is dead). Read it back through `manager.root_dir`.
if manager.root_dir.has_comptime_query(b"bun.lockb") {
// `.data` probes must hold `entries_mutex`.
let has_lockb = {
let _entries_lock = FileSystem::instance().fs.entries_mutex.lock_guard();
manager.root_dir.has_comptime_query(b"bun.lockb")
};
if has_lockb {
let mut lockfile = core::mem::replace(&mut manager.lockfile, Box::new(Lockfile::default()));
match lockfile.load_from_cwd::<true>(Some(&mut *manager), log) {
lockfile::LoadResult::Ok(_) => {}
Expand Down
23 changes: 16 additions & 7 deletions src/install/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1736,20 +1736,29 @@ impl<'a> Printer<'a> {
// Capture the `'static` cwd slice
// before borrowing `fs.fs` mutably.
let top_level_dir = fs.top_level_dir;
let entries_option = fs.fs.read_directory(top_level_dir, None, 0, true)?;
let entries: &mut Fs::DirEntry = match entries_option {
Fs::EntriesOption::Entries(e) => &mut **e,
Fs::EntriesOption::Err(e) => return Err(e.canonical_error.into()),
// Erase to raw so the `entries_mutex` reborrow below doesn't conflict
// with the `&mut self` borrow `read_directory` took.
Comment thread
robobun marked this conversation as resolved.
let entries_option: *const Fs::EntriesOption =
fs.fs.read_directory(top_level_dir, None, 0, true)?;
// Copy the listing's basenames out under `entries_mutex`; `.data` must
// only be probed while the lock is held.
Comment thread
robobun marked this conversation as resolved.
let entries = {
let _entries_lock = fs.fs.entries_mutex.lock_guard();
// SAFETY: BSSMap-owned slot; shared read under `entries_mutex`.
match unsafe { &*entries_option } {
Fs::EntriesOption::Entries(e) => {
DotEnv::DirEntryKeys(e.data.iter().map(|(k, _)| Box::from(&**k)).collect())
}
Fs::EntriesOption::Err(e) => return Err(e.canonical_error.into()),
}
};

let mut env_loader = DotEnv::Loader::init();
env_loader.quiet = true;

env_loader.load_process()?;
// `DotEnv::Loader::load` takes `impl DirEntryProbe` (bun_dotenv sits
// below `bun_resolver` in the crate graph); `Fs::DirEntry` impls it.
env_loader.load(
&*entries,
&entries,
&[] as &[&[u8]],
DotEnv::DotEnvFileSuffix::Production,
false,
Expand Down
17 changes: 14 additions & 3 deletions src/jsc/hot_reloader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1171,16 +1171,27 @@ where
let path_string: bun_ptr::Interned;
let file_hash: bun_watcher::HashType;
let abs_path: &[u8] = 'brk: {
if let Some(file_ent) = dir_ent.entries().get(changed_name)
{
// Probe `.data` under `entries_mutex`; a
// resolver at a newer generation rewrites
// the map in place under that lock. The
// entry pointer stays valid after unlock
// (EntryStore-owned).
Comment thread
robobun marked this conversation as resolved.
let looked_up = {
let _entries_lock = rfs.entries_mutex.lock_guard();
dir_ent.entries().get(changed_name)
};
if let Some(file_ent) = looked_up {
// reset the file descriptor
let ent = file_ent.entry();
{
// Every cached-`Entry` rewrite takes
// the per-entry mutex.
let _entry_guard = ent.mutex.lock_guard();
ent.set_cache_fd(Fd::INVALID);
ent.need_stat.set(true);
ent.need_stat.store(
true,
core::sync::atomic::Ordering::Release,
);
}
path_string = ent.abs_path;
file_hash = Watcher::get_hash(path_string.as_bytes());
Expand Down
70 changes: 35 additions & 35 deletions src/resolver/dir_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,63 +216,63 @@

pub fn get_file_descriptor(&self) -> Fd {
if FeatureFlags::STORE_FILE_DESCRIPTORS {
// `entries_at(_, 0)` never re-reads (`u16 < 0` is always false), so the
// lock it would take covers no mutation; go through the same plain
// `at_index` lookup `get_entries_const` uses.
// Scalar field read; see `get_entries_const` for the contract.
return self.get_entries_const().map_or(Fd::INVALID, |e| e.fd);
}
Fd::INVALID
}

/// Returns a
/// raw pointer (not `&'static mut`) because the BSSMap singleton is
/// shared-mutable and Rust forbids manufacturing aliased `&mut`. Callers
/// dereference at the use site where exclusivity is locally provable.
pub fn get_entries(&self, generation: Generation) -> Option<*mut fs::DirEntry> {
/// Generation-checked listing accessor for call sites that already hold
/// `entries_mutex` (the mutex is non-recursive); see
/// [`RealFS::entries_at_locked`](fs::RealFS::entries_at_locked). The
/// `DirEntry` is a slot in the BSSMap-backed `EntriesOptionMap` singleton
/// (ARENA — process lifetime), so a `&'static` reborrow of the
/// `&'static mut` returned by `entries_at_locked` is sound and needs no
/// `unsafe` here. The `.data` map must only be probed/iterated while the
/// lock is held; use [`get_entry`](Self::get_entry) for one-shot lookups.
Comment thread
robobun marked this conversation as resolved.
pub fn get_entries_ref_locked(&self, generation: Generation) -> Option<&'static fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs
.entries_at(self.entries, generation)?;
match entries_ptr {
fs::EntriesOption::Entries(entries) => Some(std::ptr::from_mut(*entries)),
fs::EntriesOption::Err(_) => None,
}
}

/// Shared-borrow variant of [`get_entries`](Self::get_entries) for the
/// read-only call sites (`.get()`, `.fd`, iteration). The `DirEntry` is a
/// slot in the BSSMap-backed `EntriesOptionMap` singleton (ARENA — process
/// lifetime), so a `&'static` reborrow of the `&'static mut` returned by
/// `entries_at` is sound and needs no `unsafe` here. Prefer this over
/// `get_entries` + per-site raw deref whenever the caller only reads.
pub(crate) fn get_entries_ref(&self, generation: Generation) -> Option<&'static fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs
.entries_at(self.entries, generation)?;
.entries_at_locked(self.entries, generation)?;
match entries_ptr {
fs::EntriesOption::Entries(entries) => Some(&**entries),
fs::EntriesOption::Err(_) => None,
}
}

/// [`get_entries_ref`](Self::get_entries_ref) for call sites that already
/// hold `entries_mutex` (the mutex is non-recursive); see
/// [`RealFS::entries_at_locked`](fs::RealFS::entries_at_locked).
pub(crate) fn get_entries_ref_locked(
/// Generation-checked lookup of one basename in this directory's cached
/// listing, performed in a single `entries_mutex` critical section. A
/// concurrent resolver with a newer generation rewrites the `DirEntry` in
/// place under that lock ([`RealFS::entries_at_locked`](fs::RealFS::entries_at_locked)
/// drops the old `data` map's buckets), so probing the map after the lock
/// is released can walk freed buckets. The returned lookup wraps a raw
/// pointer into the process-lifetime `EntryStore`, which stays valid after
/// the lock is dropped.
Comment thread
robobun marked this conversation as resolved.
pub(crate) fn get_entry(
&self,
generation: Generation,
) -> Option<&'static fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs
.entries_at_locked(self.entries, generation)?;
match entries_ptr {
fs::EntriesOption::Entries(entries) => Some(&**entries),
query: &[u8],
) -> Option<fs::EntryLookup<'static>> {
let rfs = &mut fs::FileSystem::instance().fs;
// `MutexGuard` stores the mutex by raw pointer, so holding it does not
// keep `rfs` borrowed.
Comment thread
robobun marked this conversation as resolved.
let _lock = rfs.entries_mutex.lock_guard();
match rfs.entries_at_locked(self.entries, generation)? {
fs::EntriesOption::Entries(entries) => {
let entries: &'static fs::DirEntry = entries;
entries.get(query)
}
fs::EntriesOption::Err(_) => None,
}
}

/// As-cached listing access with no generation check and no locking.
/// Scalar fields (`fd`, `dir`) may be read through this unlocked; the
/// `.data` map must only be probed/iterated while `entries_mutex` is held
/// (a concurrent stale-generation re-read rewrites it in place).
Comment thread
robobun marked this conversation as resolved.
pub fn get_entries_const(&self) -> Option<&fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs

Check warning on line 275 in src/resolver/dir_info.rs

View check run for this annotation

Claude / Claude Code Review

Audit missed unlocked .data.iter() sites carrying the same false 'entries_mutex held' SAFETY comments

The audit missed a few sibling `.data` iteration sites that still carry the same false `// SAFETY: entries_mutex held` claim this PR removed elsewhere: `src/runtime/cli/run_command.rs:3665-3707` / `:3720-3740` (`get_entries_const()` → `entries.data.iter()`, SAFETY at :3674 and :3737) and `src/resolver/lib.rs:1937-1943` (`DirEntryDirIter::iterate`, SAFETY at :1910). The new debug assert only guards `get`/`get_comptime_query`/`has_comptime_query`, so raw `.data.iter()` bypasses it — which is why t
Comment thread
robobun marked this conversation as resolved.
.entries
.at_index(self.entries)?;
match entries_ptr {
Expand Down
Loading