Skip to content

resolver: take Entry.mutex when filling Entry.abs_path in load_as_file and siblings #34411

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

Open
robobun wants to merge 4 commits into
base: main
Choose a base branch
from
Open

resolver: take Entry.mutex when filling Entry.abs_path in load_as_file and siblings #34411

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3da13a6
resolver: take Entry.mutex when filling Entry.abs_path in load_as_fil…
robobun Jul 16, 2026
05e17c8
router: re-check abs_path under Entry.mutex before recomputing in Rou…
robobun Jul 16, 2026
6e4546a
hot_reloader: read Entry.abs_path inside the existing Entry.mutex guard
robobun Jul 16, 2026
dbbfcff
resolver: centralize Entry.abs_path lazy fill in abs_path_or_fill()
robobun Jul 16, 2026
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
78 changes: 48 additions & 30 deletions src/resolver/resolver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3896,17 +3896,21 @@ impl<'a> Resolver<'a> {
}

let absolute_out_path: &[u8] = {
if entry_query.entry().abs_path.is_empty() {
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS fully
// evaluated before LHS `&mut Entry` is materialized.
unsafe { &mut *entry_query.entry }.abs_path = Interned::from_static(
let _entry_guard = entry_query.entry().mutex.lock_guard();
let cached = entry_query.entry().abs_path;
if !cached.is_empty() {
cached.as_bytes()
} else {
let interned = Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(abs_esm_path)
.expect("unreachable"),
);
// SAFETY: EntryStore-owned slot; `Entry.mutex` held.
unsafe { &mut *entry_query.entry }.abs_path = interned;
interned.as_bytes()
}
entry_query.entry().abs_path.as_bytes()
};
let module_type = if let Some(pkg) = resolved_dir_info.package_json() {
pkg.module_type
Expand Down Expand Up @@ -5271,19 +5275,23 @@ impl<'a> Resolver<'a> {
== Fs::file_system::EntryKind::File
{
let out_buf: &[u8] = {
if lookup.entry().abs_path.is_empty() {
let _entry_guard = lookup.entry().mutex.lock_guard();
let cached = lookup.entry().abs_path;
if !cached.is_empty() {
cached.as_bytes()
} else {
let parts = [dir_info.abs_path, &base[..]];
let out_buf_ = self.fs_ref().abs_buf(&parts, bufs!(index));
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS fully
// evaluated before LHS `&mut Entry` is materialized.
unsafe { &mut *lookup.entry }.abs_path = Interned::from_static(
let interned = Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(out_buf_)
.expect("unreachable"),
);
// SAFETY: EntryStore-owned slot; `Entry.mutex` held.
unsafe { &mut *lookup.entry }.abs_path = interned;
interned.as_bytes()
}
lookup.entry().abs_path.as_bytes()
};

if let Some(debug) = self.debug_logs.as_mut() {
Expand Down Expand Up @@ -5768,19 +5776,26 @@ impl<'a> Resolver<'a> {
}

let abs_path: &'static [u8] = {
if query.entry().abs_path.is_empty() {
// `abs_path` is a two-word slice; serialize on the per-entry
// mutex so the check/write/read is atomic relative to the
// router's `Route::parse` doing the same fill.
let _entry_guard = query.entry().mutex.lock_guard();
let cached = query.entry().abs_path;
if !cached.is_empty() {
cached.as_bytes()
} else {
let abs_path_parts = [query.entry().dir, query.entry().base()];
let joined = self.fs_ref().abs_buf(&abs_path_parts, bufs!(load_as_file));
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS fully
// evaluated before LHS `&mut Entry` is materialized.
unsafe { &mut *query.entry }.abs_path = Interned::from_static(
let interned = Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(joined)
.expect("unreachable"),
);
// SAFETY: EntryStore-owned slot; `Entry.mutex` held.
unsafe { &mut *query.entry }.abs_path = interned;
interned.as_bytes()
}
query.entry().abs_path.as_bytes()
};

dec_ret!(Some(LoadResult {
Expand Down Expand Up @@ -5874,7 +5889,11 @@ impl<'a> Resolver<'a> {

dec_ret!(Some(LoadResult {
path: {
if query.entry().abs_path.is_empty() {
let _entry_guard = query.entry().mutex.lock_guard();
let cached = query.entry().abs_path;
if !cached.is_empty() {
cached.as_bytes()
} else {
// SAFETY: `dir` is `&'static [u8]` (DirnameStore-interned),
// copied out so no `&Entry` borrow survives into the
// `&mut Entry` write below.
Expand All @@ -5900,11 +5919,10 @@ impl<'a> Resolver<'a> {
.expect("unreachable"),
)
};
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS
// fully evaluated above — sole `&mut Entry` for this write.
// SAFETY: EntryStore-owned slot; `Entry.mutex` held.
unsafe { &mut *query.entry }.abs_path = new_abs;
new_abs.as_bytes()
}
query.entry().abs_path.as_bytes()
},
diff_case: query.diff_case,
dirname_fd: entries!().fd,
Expand Down Expand Up @@ -5981,21 +5999,21 @@ impl<'a> Resolver<'a> {
// now that we've found it, we allocate it.
return Some(LoadResult {
path: {
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS is fully
// evaluated (shared reads) before the LHS `&mut Entry` is
// materialized for the write — no overlapping unique borrow.
unsafe { &mut *query.entry }.abs_path = if query.entry().abs_path.is_empty()
{
Interned::from_static(
let _entry_guard = query.entry().mutex.lock_guard();
let cached = query.entry().abs_path;
if !cached.is_empty() {
cached.as_bytes()
} else {
let interned = Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(&buffer[..])
.expect("unreachable"),
)
} else {
query.entry().abs_path
};
query.entry().abs_path.as_bytes()
);
// SAFETY: EntryStore-owned slot; `Entry.mutex` held.
unsafe { &mut *query.entry }.abs_path = interned;
interned.as_bytes()
}
},
diff_case: query.diff_case,
dirname_fd: entries.fd,
Expand Down
10 changes: 8 additions & 2 deletions src/router/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,11 +1032,17 @@
// `base_`/`extname` are no longer used.
// SAFETY: caller passes an EntryStore-owned pointer valid for the
// process lifetime; no other live `&mut` to it during this call.
let entry_abs_path = unsafe { &*entry }.abs_path().as_bytes();
// `abs_path` is a two-word slice that other threads (the bundler's
// resolver) fill under `Entry.mutex`; read it under the same lock so
// this thread can never observe a torn (ptr, len).
let entry_abs_path = {
let _entry_guard = unsafe { &*entry }.mutex.lock_guard();

Check failure on line 1039 in src/router/lib.rs

View workflow job for this annotation

GitHub Actions / cargo clippy 

unsafe block missing a safety comment
unsafe { &*entry }.abs_path()

Check failure on line 1040 in src/router/lib.rs

View workflow job for this annotation

GitHub Actions / cargo clippy 

unsafe block missing a safety comment
};

Check warning on line 1041 in src/router/lib.rs

View check run for this annotation

Claude / Claude Code Review

Sibling torn-read site missed: hot_reloader.rs reads Entry.abs_path after releasing Entry.mutex 

Same-class sibling: `src/jsc/hot_reloader.rs:1205` reads `ent.abs_path` right *after* the `_entry_guard` block closes at line 1204, so the two-word `Interned` load is unsynchronized against the resolver fill sites this PR just locked — the identical torn-`(ptr, len)` hazard you're fixing here for `Route::parse`. Moving `path_string = ent.abs_path;` inside the existing guard block closes it. (A lower-risk sibling also exists at `src/runtime/cli/test/Scanner.rs:420/453`, though `bun test`'s scanne
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut abs_path_str: &[u8] = if entry_abs_path.is_empty() {
b""
} else {
entry_abs_path
entry_abs_path.as_bytes()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
};

let base = &base_[0..base_.len() - extname.len()];
Expand Down
12 changes: 10 additions & 2 deletions test/js/bun/util/filesystem_router.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@
await Bun.build({ entrypoints, target: "bun", throw: false });
let matches = 0;
let buildsOk = true;
let pathsOk = true;
for (let round = 0; round < 40; round++) {
const builds = Array.from({ length: 4 }, () =>
Bun.build({ entrypoints, target: "bun", throw: false }),
Expand All @@ -852,10 +853,17 @@
const m = router.match("/p7");
if (m && m.filePath.endsWith("p7.tsx")) matches++;
}
// Each route's abs-path is filled by both the router (under the
// per-entry mutex) and the bundler's resolver for the same fresh
// Entry after every bust+reread; a torn value surfaces as a
// filePath that isn't the absolute .tsx path.
for (const fp of Object.values(router.routes)) {
pathsOk &&= typeof fp === "string" && fp.startsWith(pagesDir) && fp.endsWith(".tsx");
}

Check failure on line 862 in test/js/bun/util/filesystem_router.test.ts

View check run for this annotation

Claude / Claude Code Review

pathsOk assertion fails on Windows: router.routes values are forward-slash but pagesDir uses backslashes 

On Windows this new `pathsOk` check will always fail: `pagesDir = path.join(import.meta.dir, "pages")` yields a backslash path, but `router.routes` values are normalized to forward slashes (see `platform_to_posix_buf` in `Route::parse`, and the `make()` helper at the top of this file which does `.replaceAll("\\", "/")` for exactly this reason). Normalize `pagesDir` to forward slashes before the `startsWith` check so the assertion passes on Windows CI.
Comment thread
robobun marked this conversation as resolved.
const results = await Promise.all(builds);
buildsOk &&= results.every(r => r.success);
}
console.log("matches", matches, "builds-ok", buildsOk);
console.log("matches", matches, "builds-ok", buildsOk, "paths-ok", pathsOk);
`,
};
for (let i = 1; i <= 40; i++) {
Expand All @@ -877,7 +885,7 @@
stderr: normalizeBunSnapshot(stderr, String(dir)),
exitCode,
signalCode: proc.signalCode,
}).toEqual({ stdout: "matches 2000 builds-ok true", stderr: "", exitCode: 0, signalCode: null });
}).toEqual({ stdout: "matches 2000 builds-ok true paths-ok true", stderr: "", exitCode: 0, signalCode: null });
}, 60_000);

it("loads routes from a directory already cached by Bun.build()", async () => {
Expand Down
Loading