Skip to content

resolver: take the per-entry lock when rewriting cached directory entries #33056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 1 commit into from
Jun 30, 2026
Merged

resolver: take the per-entry lock when rewriting cached directory entries #33056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions src/jsc/hot_reloader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,8 +1195,13 @@ where
{
// reset the file descriptor
let ent = file_ent.entry();
ent.set_cache_fd(Fd::INVALID);
ent.need_stat.set(true);
{
// 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);
}
path_string = ent.abs_path;
file_hash = Watcher::get_hash(path_string.as_bytes());
for (entry_id, hash) in hashes.iter().enumerate() {
Expand Down
98 changes: 55 additions & 43 deletions src/resolver/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,9 +382,9 @@ impl Default for EntryCache {
// `cache` / `need_stat` are lazily populated by `Entry::kind` /
// `Entry::symlink` while callers hold a shared
// `&Entry`. `EntryCache` is `Copy`, so `Cell` gives us safe
// `.get()/.set()` through `&self` — `RealFS.entries_mutex` serializes access
// across threads (the `unsafe impl Sync for Entry` below opts back in under
// that external-locking discipline).
// `.get()/.set()` through `&self` — the per-entry `mutex` serializes every
// rewrite of these `Cell`s across threads (the `unsafe impl Sync for Entry`
// below opts back in under that external-locking discipline).
pub struct Entry {
pub cache: core::cell::Cell<EntryCache>,
pub dir: &'static [u8],
Expand Down Expand Up @@ -416,7 +416,7 @@ impl Entry {
}

/// Update a single cache field. Read-modify-write is fine: callers hold
/// `RealFS.entries_mutex` so no torn writes; `EntryCache` is `Copy`.
/// the per-entry `mutex` so no torn writes; `EntryCache` is `Copy`.
#[inline(always)]
pub fn set_cache_fd(&self, fd: Fd) {
let mut c = self.cache.get();
Expand Down Expand Up @@ -469,26 +469,32 @@ impl Entry {
///
/// # Safety
/// `fs` must point to a live `EntryKindResolver` (the process-global
/// `RealFS` singleton in practice) and the caller must hold
/// `RealFS.entries_mutex` so the `&mut *fs` reborrow is exclusive for the
/// duration of the call.
// `Entry` lives in the EntryStore BSSMap singleton; all access is
// serialized through `RealFS.entries_mutex`. `fs` is `*mut` so the
// call site does not require a second exclusive `&mut RealFS` borrow while a
// `&mut Entry` (borrowed out of `RealFS.entries`) is live. Mutation of the
// lazily-populated `need_stat` / `cache` goes through `Cell`. Generic over
// `R: EntryKindResolver` so this block is independent of which `RealFS`
// copy `fs` points at (see file-top comment).
/// `RealFS` singleton in practice). `resolve_kind` must not re-enter
/// this entry's `mutex` (it only performs syscalls and string interning).
// `Entry` lives in the EntryStore BSSMap singleton. The lazy-stat rewrite
// of `need_stat` / `cache` is serialized on the per-entry `mutex` here
// (double-checked: the cached fast path stays lock-free). `fs` is `*mut`
// so the call site does not require a second exclusive `&mut RealFS`
// borrow while a `&mut Entry` (borrowed out of `RealFS.entries`) is live.
// Generic over `R: EntryKindResolver` so this block is independent of
// which `RealFS` copy `fs` points at (see file-top comment).
pub unsafe fn kind<R: EntryKindResolver>(&self, fs: *mut R, store_fd: bool) -> EntryKind {
if self.need_stat.get() {
self.need_stat.set(false);
// This is technically incorrect, but we are choosing not to handle errors here
// SAFETY: `fs` points at the process-global RealFS singleton; caller holds
// `entries_mutex` so the `&mut` is exclusive for the duration of this call.
match unsafe { &mut *fs }.resolve_kind(self.dir, self.base(), self.cache().fd, store_fd)
{
Ok(c) => self.cache.set(c),
Err(_) => return self.cache().kind,
let _guard = self.mutex.lock_guard();
if self.need_stat.get() {
self.need_stat.set(false);
// This is technically incorrect, but we are choosing not to handle errors here
// SAFETY: `fs` points at the process-global RealFS singleton; `resolve_kind`
// only does syscalls + string interning, so the short `&mut` cannot alias.
match unsafe { &mut *fs }.resolve_kind(
self.dir,
self.base(),
self.cache().fd,
store_fd,
) {
Ok(c) => self.cache.set(c),
Err(_) => return self.cache().kind,
}
Comment on lines +472 to +497

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# EntryCache layout
ast-grep --lang rust --pattern 'struct EntryCache { $$$ }' src/resolver/fs.rs
ast-grep --lang rust --pattern 'pub struct EntryCache { $$$ }' src/resolver/fs.rs
# Interned representation + as_bytes
fd -e rs . src/ptr 2>/dev/null | head
rg -nP 'struct\s+Interned' --type rust -C4
rg -nP 'impl\b.*\bInterned\b' --type rust -C2 | rg -n 'as_bytes' -C3
# Mutex semantics (does lock_guard imply a full barrier readers observe?)
rg -nP 'fn\s+lock_guard' --type rust -C4

Repository: oven-sh/bun

Length of output: 668

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant definitions and access patterns.
sed -n '340,560p' src/resolver/fs.rs

printf '\n--- Interned ---\n'
rg -n --context 4 'pub struct Interned|type Interned|enum Interned|impl .*Interned' src -g '!**/target/**'

printf '\n--- lock_guard / mutex impls ---\n'
rg -n --context 4 'fn lock_guard|struct .*Mutex|impl .*lock_guard|lock_guard\(' src -g '!**/target/**'

Repository: oven-sh/bun

Length of output: 50367

🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '340,560p' src/resolver/fs.rs
printf '\n--- Interned ---\n'
rg -n --context 4 'pub struct Interned|type Interned|enum Interned|impl .*Interned' src -g '!**/target/**'
printf '\n--- lock_guard / mutex impls ---\n'
rg -n --context 4 'fn lock_guard|struct .*Mutex|impl .*lock_guard|lock_guard\(' src -g '!**/target/**'

Repository: oven-sh/bun

Length of output: 50367

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Narrow, read-only inspection of kind()/symlink() callers and cache access patterns.
rg -n --context 2 '\.kind\(\s*[^)]*\)|\.symlink\(\s*[^)]*\)|cache\(\)\.(kind|symlink)|set_cache(_kind|_fd|_symlink)?\(' src/resolver src/runtime -g '!**/target/**' | head -n 200

printf '\n--- files mentioning Entry::kind / Entry::symlink ---\n'
rg -n --context 2 'Entry::kind|Entry::symlink|\.kind\(' src -g '!**/target/**' | head -n 200

Repository: oven-sh/bun

Length of output: 31387

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1120,1185p' src/router/lib.rs
printf '\n---\n'
sed -n '360,430p' src/runtime/api/filesystem_router.rs
printf '\n---\n'
sed -n '500,560p' src/runtime/node/node_fs.rs

Repository: oven-sh/bun

Length of output: 11712

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n --context 3 '\.kind\(' src/resolver/fs.rs src/router/lib.rs src/runtime/api/filesystem_router.rs src/runtime/node/node_fs.rs src/runtime/bake/FrameworkRouter.rs src -g '!**/target/**' | head -n 120
printf '\n--- symlink ---\n'
rg -n --context 3 '\.symlink\(' src/resolver/fs.rs src/router/lib.rs src/runtime/api/filesystem_router.rs src/runtime/node/node_fs.rs src/runtime/bake/FrameworkRouter.rs src -g '!**/target/**' | head -n 120

Repository: oven-sh/bun

Length of output: 17081

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1788,1825p' src/resolver/resolver.rs
printf '\n---\n'
sed -n '1728,1742p' src/resolver/resolver.rs
printf '\n--- unsafe impl Sync for Entry ---\n'
rg -n --context 3 'unsafe impl Sync for Entry|impl Sync for Entry' src/resolver/fs.rs src/resolver/resolver.rs src -g '!**/target/**'

Repository: oven-sh/bun

Length of output: 4761

Guard the cache reads on the same mutex as the rewrites. kind() and symlink() still read self.cache() lock-free, but other paths rewrite the same EntryCache under Entry.mutex (set_cache_fd, set_cache_symlink, etc.). Since EntryCache includes Interned = &'static [u8], a concurrent Cell::set() can race the fast path and tear the slice metadata. Keep these reads under the mutex or split the mutable state into atomic/independently guarded fields.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/resolver/fs.rs` around lines 472 - 497, The lock-free cache reads in
Entry::kind and symlink are racing with cache rewrites protected by Entry.mutex,
which can corrupt the shared EntryCache state. Update these fast paths to read
the cache only while holding the same mutex used by set_cache_fd and
set_cache_symlink, or split the mutable cache fields so each part is
independently synchronized. Use the existing Entry::kind, symlink, and cache
accessors as the main touchpoints when applying the fix.

}
}
self.cache().kind
Expand All @@ -497,23 +503,28 @@ impl Entry {
///
/// # Safety
/// `fs` must point to a live `EntryKindResolver` (the process-global
/// `RealFS` singleton in practice) and the caller must hold
/// `RealFS.entries_mutex` so the `&mut *fs` reborrow is exclusive for the
/// duration of the call.
/// `RealFS` singleton in practice). See [`Entry::kind`].
pub unsafe fn symlink<R: EntryKindResolver>(
&self,
fs: *mut R,
store_fd: bool,
) -> &'static [u8] {
if self.need_stat.get() {
self.need_stat.set(false);
// This error can happen if the file was deleted between the time the directory
// was scanned and the time it was read
// SAFETY: see the note on `Entry::kind`.
match unsafe { &mut *fs }.resolve_kind(self.dir, self.base(), self.cache().fd, store_fd)
{
Ok(c) => self.cache.set(c),
Err(_) => return b"",
let _guard = self.mutex.lock_guard();
if self.need_stat.get() {
self.need_stat.set(false);
// This error can happen if the file was deleted between the time the directory
// was scanned and the time it was read
// SAFETY: see the note on `Entry::kind`.
match unsafe { &mut *fs }.resolve_kind(
self.dir,
self.base(),
self.cache().fd,
store_fd,
) {
Ok(c) => self.cache.set(c),
Err(_) => return b"",
}
}
}
self.cache().symlink.as_bytes()
Expand Down Expand Up @@ -565,7 +576,7 @@ pub struct DifferentCase<'a> {
// `entry` is a RAW `*mut Entry`. A safe
// `&self → &mut Entry` accessor would let two `get()` calls produce coexisting
// aliased `&mut Entry` (PORTING.md §Forbidden). Callers `unsafe { &mut *entry }`
// at each write site under `entries_mutex`.
// at each write site under the per-entry `Entry.mutex`.
pub struct EntryLookup<'a> {
pub entry: *mut Entry,
pub diff_case: Option<DifferentCase<'static>>,
Expand Down Expand Up @@ -593,8 +604,8 @@ impl<'a> EntryLookup<'a> {
// (zero callers). `Entry`'s only mutable state (`cache`) is `Cell`-backed,
// so all mutation goes through `entry().set_cache*()` on a shared borrow;
// no `&mut Entry` escape hatch is needed. Write sites that bypass the
// accessor go through the raw `self.entry` field directly under
// `entries_mutex` (see struct doc above).
// accessor go through the raw `self.entry` field directly under the
// per-entry `Entry.mutex` (see struct doc above).
}

/// `DirEntry` companion items: the entry map, the global entry store, and the
Expand Down Expand Up @@ -1775,13 +1786,14 @@ impl RealFS {
};

// if we get this far, it's a real directory, so we can just store the dir name.
let dir: &'static [u8] = if !had_handle {
if let Some(existing) = in_place {
// SAFETY: in_place points to BSSMap-owned DirEntry
unsafe { (*existing).dir }
} else {
DirnameStore::instance().append(dir_maybe_trail_slash)?
}
// 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 BSSMap-owned DirEntry
unsafe { (*existing).dir }
} else if !had_handle {
DirnameStore::instance().append(dir_maybe_trail_slash)?
} else {
// Intern into DirnameStore so the cache entry never dangles — `append` is a
// bump-pointer copy and dedups against the singleton, so cost is bounded.
Expand Down
17 changes: 9 additions & 8 deletions src/resolver/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,14 +1305,15 @@ pub mod fs {
});

// if we get this far, it's a real directory, so we can just store the dir name.
let dir: &'static [u8] = if !had_handle {
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 {
DirnameStore::instance().append_slice(dir_maybe_trail_slash)?
}
// 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.
Expand Down
35 changes: 26 additions & 9 deletions src/resolver/resolver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1766,7 +1766,11 @@ impl<'a> Resolver<'a> {
// panic on EACCES/EMFILE/ELOOP here.
let file = bun_sys::open(span, bun_sys::O::RDONLY, 0)
.map_err(Into::<bun_core::Error>::into)?;
query.entry().set_cache_fd(file);
{
// Every cached-`Entry` rewrite takes the per-entry mutex.
let _entry_guard = query.entry().mutex.lock_guard();
query.entry().set_cache_fd(file);
}
Fs::FileSystem::set_max_fd(file.native());
}

Expand All @@ -1784,6 +1788,8 @@ impl<'a> Resolver<'a> {
scopeguard::defer! {
if need_close {
let e = entry_ref.get();
// Every cached-`Entry` rewrite takes the per-entry mutex.
let _entry_guard = e.mutex.lock_guard();
let fd = e.cache().fd;
if fd.is_valid() {
fd.close();
Expand All @@ -1801,9 +1807,13 @@ impl<'a> Resolver<'a> {
bstr::BStr::new(path.text())
));
}
query
.entry()
.set_cache_symlink(Interned::from_static(symlink));
{
// Every cached-`Entry` rewrite takes the per-entry mutex.
let _entry_guard = query.entry().mutex.lock_guard();
query
.entry()
.set_cache_symlink(Interned::from_static(symlink));
}
if !result.file_fd.is_valid() && store_fd {
result.file_fd = query.entry().cache().fd;
}
Expand Down Expand Up @@ -6224,13 +6234,16 @@ impl<'a> Resolver<'a> {
&& !lookup.entry().cache().fd.is_valid()
&& self.store_fd
{
// Every cached-`Entry` rewrite takes the per-entry mutex.
let _entry_guard = lookup.entry().mutex.lock_guard();
lookup.entry().set_cache_fd(entries_fd);
}
// SAFETY: EntryStore-owned slot; `entries_mutex` held — read-only borrow,
// SAFETY: EntryStore-owned slot — read-only borrow,
// dies (NLL) before any later `&mut` to this slot.
let entry = lookup.entry();

// SAFETY: entries_mutex held; `rfs_ptr` points at the process-global RealFS.
// SAFETY: `rfs_ptr` points at the process-global RealFS; the lazy-stat
// rewrite inside `symlink()` is serialized on `Entry.mutex`.
let mut symlink = unsafe { entry.symlink(rfs_ptr, self.store_fd) };
if !symlink.is_empty() {
if let Some(logs) = self.debug_logs.as_mut() {
Expand Down Expand Up @@ -6270,9 +6283,13 @@ impl<'a> Resolver<'a> {
.ok();
logs.add_note(buf);
}
lookup
.entry()
.set_cache_symlink(Interned::from_static(symlink));
{
// Every cached-`Entry` rewrite takes the per-entry mutex.
let _entry_guard = lookup.entry().mutex.lock_guard();
lookup
.entry()
.set_cache_symlink(Interned::from_static(symlink));
}
info.abs_real_path = symlink;
}
}
Expand Down
Loading
Loading