resolver: take Entry.mutex when filling Entry.abs_path in load_as_file and siblings - #34411
resolver: take Entry.mutex when filling Entry.abs_path in load_as_file and siblings#34411robobun wants to merge 4 commits into
Conversation
…e and siblings The five lazy abs_path fill sites in the resolver (load_as_file plain path, load_as_file js-to-ts rewrite, load_extension, load_index_with_extension, handle_esm_resolution) read/check/write Entry.abs_path without taking the per-entry mutex. Route::parse fills the same field under that mutex. After a reload() bust+reread both sides see the same fresh Entry and race to fill it; abs_path is a two-word (ptr, len) slice, so on weakly-ordered CPUs the bundler thread can read a torn value and pass it as LoadResult.path. The symbolized stack from build 74037 (linux aarch64 release-profile) lands exactly at that read: last_index_of src/bun_core/string/immutable.rs:713 Resolver::load_as_file_or_directory src/resolver/resolver.rs:5435 Resolver::resolve_without_symlinks Resolver::resolve_and_auto_install Transpiler::_resolve_entry_point BundleV2::enqueue_entry_points_normal BundleThread::generate_in_new_thread Serialize each fill on Entry.mutex (the invariant #33056 established for every Entry rewrite) and return the locally-held interned slice instead of re-reading the field. Route::parse's initial abs_path read is also moved under the same mutex so it cannot observe a torn value either. The race requires a weakly-ordered CPU to surface the torn (old-ptr, new-len) combination; x86-TSO makes that ordering unreachable, so the existing concurrency test passes with and without this change on x64 (30/30 release, 15/15 debug+ASAN). On aarch64 the test's subprocess segfaulted in build 74037.
StatusAfter review feedback the five per-site lock blocks are centralized into The torn-read requires the All of CI on build 74159 is red on every lane for |
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesThe resolver now synchronizes Absolute-path synchronization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/router/lib.rs`:
- Around line 1038-1045: Make the abs_path empty check and cache fill atomic in
the Route::parse flow. Re-read entry.abs_path() after acquiring the fill mutex,
immediately before recomputing or writing the path, and skip the fill if another
resolver already populated it. Do not rely on the earlier entry_abs_path
snapshot outside the critical section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 647bb6c8-a23d-4750-b97a-2ee1b49e8a22
📒 Files selected for processing (3)
src/resolver/resolver.rssrc/router/lib.rstest/js/bun/util/filesystem_router.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
…te::parse The initial locked read releases Entry.mutex before Route::parse's fill block re-acquires it; the bundler's resolver may fill abs_path in between. Re-read under the lock and skip the open + get_fd_path + intern when already populated.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Same torn-(ptr, len) hazard as the router and resolver sites; the guard block already exists for set_cache_fd/need_stat, so move the abs_path load inside it. Also normalize pagesDir to forward slashes for the filesystem_router test's path prefix check, since router.routes values are normalized via platform_to_posix_buf on Windows.
There was a problem hiding this comment.
Both earlier findings (Windows path.sep normalization in the test fixture, and the hot_reloader.rs abs_path read outside the guard) are addressed in 6e4546a. I didn't find further issues, but this adds per-entry locking across five resolver hot-path sites, so a human should confirm the lock-ordering claim (Entry.mutex over the dirname_store append lock) and that the added contention is acceptable.
Reviewed: the five abs_path fill sites in resolver.rs — each now returns the locally-held interned.as_bytes() instead of re-reading the field, so no torn read after write. Route::parse re-checks under the lock before recomputing (CodeRabbit's TOCTOU note). The pagesDirPosix fix uses path.sep evaluated in the subprocess, so the backslash is a runtime value, not a template-literal escape.
Extended reasoning...
Overview
Fixes a torn-read data race on Entry.abs_path (a two-word Interned = &'static [u8] fat pointer) that segfaulted on aarch64 CI. Five sibling fill sites in src/resolver/resolver.rs (load_as_file, its js→ts rewrite, load_extension, load_index_with_extension, handle_esm_resolution) now take Entry.mutex around the check-fill-return; Route::parse in src/router/lib.rs reads and re-checks under the same lock; src/jsc/hot_reloader.rs moves its abs_path read inside the existing guard block. The concurrency test in filesystem_router.test.ts gains a per-round assertion that every router.routes value is a valid absolute .tsx path.
Security risks
None. No user-controlled input parsing, auth, or crypto is touched — this is internal cache synchronization.
Level of scrutiny
High. The resolver is on every module-resolution path and is one of Bun's hottest code paths. Adding a mutex acquire/release to five sites there is mechanically simple but has performance and lock-ordering implications a maintainer should sign off on. The PR body asserts Entry.mutex nests over the BSSStringList mutex taken by dirname_store.append_slice and that nothing under that append touches Entry.mutex; that claim looks right from the diff but deserves a human eye given the CLAUDE.md guidance that "a mutex only counts if EVERY accessor takes it" and the #33056 lock-order precedent it cites.
Other factors
- All three review threads (CodeRabbit's TOCTOU on
Route::parse, my WindowsstartsWithseparator mismatch, myhot_reloader.rssibling read) are resolved in 05e17c8 and 6e4546a respectively; I re-checked each fix against the current diff. - The five resolver edits are byte-for-byte parallel (lock → snapshot
cached→ branch → write + return local), and each returns the locally-held interned slice rather than re-reading the shared field, so the write-then-read-torn window is closed even if another writer interleaves after the guard drops. - The test's
pagesDirPosix = pagesDir.replaceAll(path.sep, "/")runs inside the spawned fixture, sopath.sepis a runtime one-char string on Windows — no template-literal escaping issue. rust:check-alland the four named test files are reported green; the fail-before half is only reproducible on weakly-ordered CPUs, which the PR body documents.
Move the double-checked lock+fill into a single Entry method in fs.rs, next to the existing Entry::kind/symlink helpers that use the same pattern for the sibling cache/need_stat fields. Entry.abs_path is now Cell<Interned> (matching cache and need_stat) so the helper writes through &self. The five resolver.rs fill sites and Route::parse's fill block now call the helper (or do the equivalent single-acquire check+fill), dropping the per-site unsafe raw writes and the redundant second lock in Route::parse. Scanner.rs uses set_abs_path()/abs_path() for the Cell accessors.
There was a problem hiding this comment.
I didn't find any bugs, but this is a concurrency/locking change across the resolver, router, hot-reloader, and test scanner, so it's worth a human look.
Checked that Cell<Interned> matches the existing cache/need_stat pattern under the unsafe impl Sync for Entry at fs.rs:1444, and that all remaining Entry.abs_path accessors (Scanner, hot_reloader, router, resolver) now go through the accessor methods.
Verified abs_path_or_fill doesn't nest with Entry::kind()'s mutex acquisition — resolver calls them sequentially.
Confirmed the router's 'fill block preserves the file-open/stat body and only adds the locked re-check; the entry-mutex → filename_store lock order already existed there.
The fill closures in resolver.rs run under entry.mutex and call into dirname_store/abs_buf — I didn't spot a reverse ordering, but someone who knows the resolver's lock hierarchy should confirm.
Extended reasoning...
Overview
This PR fixes a data race on Entry.abs_path in the resolver's shared file-system cache. abs_path is a two-word Interned (ptr + len) that was previously written via unsafe { &mut *entry } under an assumed "resolver mutex held" invariant, but read unlocked from the router (Route::parse) and hot-reloader — allowing torn (ptr, len) reads on weakly-ordered CPUs. The fix converts the field to Cell<Interned>, adds a centralized abs_path_or_fill() helper that does double-checked lazy fill under the per-entry mutex, and rewires 5 resolver sites plus the router, hot-reloader, and test scanner to use it. The existing concurrent reload() + Bun.build() test is extended to assert every route's filePath is a well-formed absolute path (the observable symptom of a torn read).
Security risks
None. This is internal locking discipline; no user-facing surface, parsing, or trust-boundary changes.
Level of scrutiny
High. The resolver's Entry cache sits under every import/require, Bun.build, FileSystemRouter, and --hot. The change alters lock acquisition on that path: the resolver now takes entry.mutex around each abs_path fill (previously it relied on the coarser resolver mutex per the deleted SAFETY comments), and Route::parse now unconditionally takes entry.mutex for the read where it previously had an unlocked fast path. The new fill closures run dirname_store.append_slice / fs_ref().abs_buf while holding entry.mutex — a lock-nesting order that appears consistent with the router's existing entry-mutex → filename_store.append ordering, but a maintainer who knows the resolver's threading model should confirm there's no reverse-order path.
Other factors
- The
Cell<T>+ per-entrymutex+unsafe impl Syncpattern is not new here —cache: Cell<EntryCache>andneed_stat: Cell<bool>on the same struct already work this way (fs.rs:264-269), andabs_path_or_fillmirrorsEntry::kind()/symlink(). So the design is consistent with the file's established discipline. - Net -46/+59 across 5 source files, but the resolver.rs delta is almost entirely mechanical (5 identical unsafe-write-then-read blocks collapsed into the shared helper) and removes 5
unsafe { &mut * }sites whose SAFETY comments were arguably incorrect. - I grepped for remaining
Entry.abs_pathaccesses: the otherabs_pathhits in router/lib.rs, dir_info.rs, cron.rs, bake/, bundle_v2.rs are onRoute/DirInfo/other structs, notEntry. Scanner.rs still reads/writes without the mutex, but it holds&mut Entry(exclusive) during single-threaded test discovery. - Four incremental commits and no PR timeline metadata was available, so I couldn't check for prior review discussion.
Given the criticality of the resolver hot path and the change in lock-acquisition sites, this should get a human sign-off even though the implementation looks correct.
…e stress test to ASAN builds The new test plants an EACCES error in the directory-entry cache for a watched directory (chmod 000 plus a failed resolve, dropping to nobody via runuser when root) and then lands one directory event on it. Before the fix the watcher thread fed the error slot to EntriesOption::entries, which panics (EntriesOption::entries on non-Entries variant) and aborts the process; deterministic on Linux, verified 5/5 red on the unfixed build and 5/5 green on the fixed one. The reload/build stress test also drives the resolver-side lookup races that #37274 and #34411 fix, which segfault the fixture on weakly ordered release lanes (ubuntu 25.04 aarch64, build 91033, sigsegv at 0x1E, 41ms in). Gate it to ASAN builds, where the watcher-side use-after-free it guards against is detectable and those lanes are x64.
…ion rewrite (#37274) Fixes #37266 Fixes #23773 ## Crash In [build 90948](https://buildkite.com/bun/bun/builds/90948) (ubuntu 25.04 aarch64), the `reload() while Bun.build() resolves the same directory` subprocess in `test/js/bun/util/filesystem_router.test.ts` died 22ms in with ``` panic: Segmentation fault at address 0x25 ``` instead of printing `matches 2000 builds-ok true`. The branch did not touch the resolver or router (its diff against main is empty for those modules), and the same test passed in the surrounding ~9 runs of the same code, so this is a low-probability race. It did not reproduce locally: ~3000 stress runs of the fixture (linux x64 release and debug+ASAN, windows aarch64 release, with cpu pinning, oversubscription, and `MIMALLOC_PURGE_DELAY=0` variants) produced 0 failures, nor did ~150 further runs of a `Bun.resolveSync`-from-plugin variant modeled on #23773. The buildkite artifact for that build has expired from the network I can reach, so the trace could not be symbolized; the fix below closes the remaining windows of the already-proven race class rather than a bisected instruction. #23773 is the same crash family observed in the wild on linux aarch64 (about 1 in 20 builds of a large project): `Segmentation fault at address 0x0` with `Entry.symlink`'s cache access (`fs.zig:397`) inlined into `finalizeResult`, reached from `Bun.resolveSync` inside a bundler plugin's `onResolve`, which is the JS thread running `finalize_result`'s unlocked probe concurrently with bundler-thread re-reads. ## Cause `RealFS::entries_at` and `read_directory` rewrite a cached `DirEntry` in place when a resolver with a newer generation re-reads the directory: `data.clear()` plus `*e_ptr = new_entry`, which drops the old `data: StringHashMap<*mut Entry>` and frees its bucket allocation. #34271 made the `entries_at` rewrite take `entries_mutex`, and the route-loader iteration sites already snapshot the map under the same lock. Three windows remained: 1. **Unlocked map probes.** The resolver lookup sites probed `.data` after the lock was released: `finalize_result`, `handle_esm_resolution`, `load_index_with_extension`, `probe_wildcard_extensions`, `Transpiler::run_env_loader` (all via `get_entries_ref`), and `load_as_file` / `load_extension` (via the `EntriesOption` slot captured after `read_directory` returned, with SAFETY comments incorrectly claiming "entries_mutex held"). A concurrent stale-generation rewrite frees the buckets mid-probe; a probe walking freed memory can load a garbage `*mut Entry` and fault at a small absolute address, which matches `0x25`. This is the same mechanism #34271's ASAN trace proved fires in this test, just on the lookup side instead of the iteration side, and `load_as_file` is the hottest instance (every relative-file import). 2. **`need_stat` published before `cache`.** `Entry::kind`/`symlink` cleared `need_stat` first and wrote the resolved `cache` after, with plain `Cell` ops. The fast path reads `need_stat` without the per-entry mutex, so on a weakly ordered CPU a reader could observe `need_stat == false` while the `cache` write was not yet visible, or mid-store: a torn read of the 16-byte `Interned` symlink faults inside `as_bytes`. This window needs entries whose kind is not known from `getdents` (symlinks, or filesystems returning `DT_UNKNOWN`), which is also consistent with firing on CI but not on local ext4/tmpfs/NTFS. 3. **Recycled entries overwrote a live cache.** When a re-read's `getdents` result disagreed with the cached kind, `add_entry_with_store` rewrote `cache` (kind plus symlink) in place under the per-entry mutex. A lock-free reader that had already observed `need_stat == false` reads `cache` without that mutex, and no store ordering can publish anything to a reader that loaded the stale flag, so the overwrite could tear a populated `Interned` under it. On a `DT_UNKNOWN` filesystem this fired on every re-read of every entry. ## Fix - `DirInfo::get_entry(generation, query)`: generation-checked lookup of one basename in a single `entries_mutex` critical section. The returned `EntryLookup` wraps a pointer into the process-lifetime `EntryStore`, so it stays valid after unlock; only the map walk needed the lock. The `get_entries_ref` probe sites now go through it (or through an explicit guard plus `get_entries_ref_locked` where the listing fd is captured in the same critical section). The now-unused unlocked `get_entries_ref` is removed. - `EntriesOption::lookup(query)` does the same for the `read_directory`-based sites (`load_as_file`, `load_extension`), returning the lookup and the listing fd from one critical section; `dir_and_fd()` covers the watch-registration read. `load_extension` takes the slot handle instead of a bare `&DirEntry`. - `run_env_loader` copies the listing's basenames out under `entries_mutex` and lets the dotenv loader probe the copy, instead of probing the live map between `.env` file reads. - `Entry.need_stat` is an `AtomicBool`: Acquire fast-path load, Release store after the `cache` write, so a reader that skips the mutex is guaranteed to see the completed `cache` write. - The recycle path only publishes `need_stat = true` and no longer rewrites `cache`; the next `kind()`/`symlink()` re-stats under the per-entry mutex, and a reader that raced the flag sees the old cache, stale but untorn (`set_cache_kind` had no other callers and is removed). - **Enforcement**: `DirEntry::get` / `get_comptime_query` / `has_comptime_query` debug-assert that the calling thread holds `entries_mutex`, so the next unlocked probe fails deterministically in debug builds instead of surfacing as a rare segfault. The callers that probed without the lock are converted: the dotenv loader always takes a `DirEntryKeys` basename snapshot copied out under the lock (the `DirEntryProbe` impl for the live `DirEntry` is removed so the type system forces the copy, covering `run_env_loader`, `PackageManager::init`, and the lockfile printer), PackageManager's lockfile-presence probe and `bun pm view`'s package-name probe take the uncontended guard, and the hot reloader's changed-file probe runs under the lock. - API cleanup from the same audit: `DirInfo::get_entries` (raw-pointer return documented as "callers prove exclusivity locally", which is the claim the deleted `run_env_loader` deref made and this PR proved false) is removed along with the now-uncalled `RealFS::entries_at`; `get_entries_ref_locked` becomes `pub` for the one-critical-section snapshot pattern; `get_entries_const` documents which fields may be read unlocked. Known residual same-value races are left alone and are not crash vectors: the recycle path's `existing.dir = self.dir` always rewrites identical bytes (the re-read passes the old `DirEntry`'s interned dir through), and `DirEntry.fd` is a word-sized field read where still unlocked. The two remaining different-value `set_cache_symlink` writers (the `getFdPath` path in `finalize_result` and `dir_info_uncached`) still race lock-free `cache` readers in principle; closing those needs reader-side locking or an atomic cache and is left for a follow-up. The lazy `abs_path` fill has its own torn-read window with a symbolized trace from an earlier occurrence of this same test crash (address `0x31`, also ~24ms, Debian 13 aarch64); #34411 already covers that and is complementary to this PR. `src/jsc/hot_reloader.rs` has a similar unlocked probe on the watcher thread, left for a separate change. ## Why there is no new test The fixture that crashed is already the regression test for this race class, and the windows here are sub-microsecond interleavings: ~3000 runs of it against an unfixed build (including 55 debug+ASAN runs, the configuration that catches the freed-bucket read as a heap-use-after-free) produced 0 failures, as did 155 release and 35 debug+ASAN runs of a dedicated `Bun.resolveSync`-from-`onResolve` fixture modeled on #23773's trace, so no honest test can fail without the fix. The fix is verified by analysis plus the class precedent: #34271's ASAN trace demonstrates the identical free-while-probing mechanism on the iteration sites this PR's lookup sites share. ## Verification - `bun bd test test/js/bun/util/filesystem_router.test.ts`: 33 pass (including the 40-round race test) - `bun bd test test/js/bun/resolve/resolve.test.ts`: 73 pass (wildcard exports extension probing) - `bun bd test test/bundler/esbuild/packagejson.test.ts`: 84 pass (exports map resolution) - `bun bd test test/bundler/bundler_edgecase.test.ts`: 134 pass (extension probing, js-to-ts rewrites) - `bun bd test test/bundler/bun-build-api.test.ts`: 52 pass - `bun bd test test/cli/run/env.test.ts`: 96 pass (`.env` discovery through the copied-keys probe) - `bun bd test test/cli/hot/hot.test.ts`: 12 pass, `test/cli/watch/watch.test.ts`: 8 pass - Stress on the fixed debug+ASAN build: 25 long + 150 short fixture runs, 0 failures - `cargo clippy -p bun_resolver -p bun_bundler` clean
…ion rewrite (oven-sh#37274) Fixes oven-sh#37266 Fixes oven-sh#23773 ## Crash In [build 90948](https://buildkite.com/bun/bun/builds/90948) (ubuntu 25.04 aarch64), the `reload() while Bun.build() resolves the same directory` subprocess in `test/js/bun/util/filesystem_router.test.ts` died 22ms in with ``` panic: Segmentation fault at address 0x25 ``` instead of printing `matches 2000 builds-ok true`. The branch did not touch the resolver or router (its diff against main is empty for those modules), and the same test passed in the surrounding ~9 runs of the same code, so this is a low-probability race. It did not reproduce locally: ~3000 stress runs of the fixture (linux x64 release and debug+ASAN, windows aarch64 release, with cpu pinning, oversubscription, and `MIMALLOC_PURGE_DELAY=0` variants) produced 0 failures, nor did ~150 further runs of a `Bun.resolveSync`-from-plugin variant modeled on oven-sh#23773. The buildkite artifact for that build has expired from the network I can reach, so the trace could not be symbolized; the fix below closes the remaining windows of the already-proven race class rather than a bisected instruction. oven-sh#23773 is the same crash family observed in the wild on linux aarch64 (about 1 in 20 builds of a large project): `Segmentation fault at address 0x0` with `Entry.symlink`'s cache access (`fs.zig:397`) inlined into `finalizeResult`, reached from `Bun.resolveSync` inside a bundler plugin's `onResolve`, which is the JS thread running `finalize_result`'s unlocked probe concurrently with bundler-thread re-reads. ## Cause `RealFS::entries_at` and `read_directory` rewrite a cached `DirEntry` in place when a resolver with a newer generation re-reads the directory: `data.clear()` plus `*e_ptr = new_entry`, which drops the old `data: StringHashMap<*mut Entry>` and frees its bucket allocation. oven-sh#34271 made the `entries_at` rewrite take `entries_mutex`, and the route-loader iteration sites already snapshot the map under the same lock. Three windows remained: 1. **Unlocked map probes.** The resolver lookup sites probed `.data` after the lock was released: `finalize_result`, `handle_esm_resolution`, `load_index_with_extension`, `probe_wildcard_extensions`, `Transpiler::run_env_loader` (all via `get_entries_ref`), and `load_as_file` / `load_extension` (via the `EntriesOption` slot captured after `read_directory` returned, with SAFETY comments incorrectly claiming "entries_mutex held"). A concurrent stale-generation rewrite frees the buckets mid-probe; a probe walking freed memory can load a garbage `*mut Entry` and fault at a small absolute address, which matches `0x25`. This is the same mechanism oven-sh#34271's ASAN trace proved fires in this test, just on the lookup side instead of the iteration side, and `load_as_file` is the hottest instance (every relative-file import). 2. **`need_stat` published before `cache`.** `Entry::kind`/`symlink` cleared `need_stat` first and wrote the resolved `cache` after, with plain `Cell` ops. The fast path reads `need_stat` without the per-entry mutex, so on a weakly ordered CPU a reader could observe `need_stat == false` while the `cache` write was not yet visible, or mid-store: a torn read of the 16-byte `Interned` symlink faults inside `as_bytes`. This window needs entries whose kind is not known from `getdents` (symlinks, or filesystems returning `DT_UNKNOWN`), which is also consistent with firing on CI but not on local ext4/tmpfs/NTFS. 3. **Recycled entries overwrote a live cache.** When a re-read's `getdents` result disagreed with the cached kind, `add_entry_with_store` rewrote `cache` (kind plus symlink) in place under the per-entry mutex. A lock-free reader that had already observed `need_stat == false` reads `cache` without that mutex, and no store ordering can publish anything to a reader that loaded the stale flag, so the overwrite could tear a populated `Interned` under it. On a `DT_UNKNOWN` filesystem this fired on every re-read of every entry. ## Fix - `DirInfo::get_entry(generation, query)`: generation-checked lookup of one basename in a single `entries_mutex` critical section. The returned `EntryLookup` wraps a pointer into the process-lifetime `EntryStore`, so it stays valid after unlock; only the map walk needed the lock. The `get_entries_ref` probe sites now go through it (or through an explicit guard plus `get_entries_ref_locked` where the listing fd is captured in the same critical section). The now-unused unlocked `get_entries_ref` is removed. - `EntriesOption::lookup(query)` does the same for the `read_directory`-based sites (`load_as_file`, `load_extension`), returning the lookup and the listing fd from one critical section; `dir_and_fd()` covers the watch-registration read. `load_extension` takes the slot handle instead of a bare `&DirEntry`. - `run_env_loader` copies the listing's basenames out under `entries_mutex` and lets the dotenv loader probe the copy, instead of probing the live map between `.env` file reads. - `Entry.need_stat` is an `AtomicBool`: Acquire fast-path load, Release store after the `cache` write, so a reader that skips the mutex is guaranteed to see the completed `cache` write. - The recycle path only publishes `need_stat = true` and no longer rewrites `cache`; the next `kind()`/`symlink()` re-stats under the per-entry mutex, and a reader that raced the flag sees the old cache, stale but untorn (`set_cache_kind` had no other callers and is removed). - **Enforcement**: `DirEntry::get` / `get_comptime_query` / `has_comptime_query` debug-assert that the calling thread holds `entries_mutex`, so the next unlocked probe fails deterministically in debug builds instead of surfacing as a rare segfault. The callers that probed without the lock are converted: the dotenv loader always takes a `DirEntryKeys` basename snapshot copied out under the lock (the `DirEntryProbe` impl for the live `DirEntry` is removed so the type system forces the copy, covering `run_env_loader`, `PackageManager::init`, and the lockfile printer), PackageManager's lockfile-presence probe and `bun pm view`'s package-name probe take the uncontended guard, and the hot reloader's changed-file probe runs under the lock. - API cleanup from the same audit: `DirInfo::get_entries` (raw-pointer return documented as "callers prove exclusivity locally", which is the claim the deleted `run_env_loader` deref made and this PR proved false) is removed along with the now-uncalled `RealFS::entries_at`; `get_entries_ref_locked` becomes `pub` for the one-critical-section snapshot pattern; `get_entries_const` documents which fields may be read unlocked. Known residual same-value races are left alone and are not crash vectors: the recycle path's `existing.dir = self.dir` always rewrites identical bytes (the re-read passes the old `DirEntry`'s interned dir through), and `DirEntry.fd` is a word-sized field read where still unlocked. The two remaining different-value `set_cache_symlink` writers (the `getFdPath` path in `finalize_result` and `dir_info_uncached`) still race lock-free `cache` readers in principle; closing those needs reader-side locking or an atomic cache and is left for a follow-up. The lazy `abs_path` fill has its own torn-read window with a symbolized trace from an earlier occurrence of this same test crash (address `0x31`, also ~24ms, Debian 13 aarch64); oven-sh#34411 already covers that and is complementary to this PR. `src/jsc/hot_reloader.rs` has a similar unlocked probe on the watcher thread, left for a separate change. ## Why there is no new test The fixture that crashed is already the regression test for this race class, and the windows here are sub-microsecond interleavings: ~3000 runs of it against an unfixed build (including 55 debug+ASAN runs, the configuration that catches the freed-bucket read as a heap-use-after-free) produced 0 failures, as did 155 release and 35 debug+ASAN runs of a dedicated `Bun.resolveSync`-from-`onResolve` fixture modeled on oven-sh#23773's trace, so no honest test can fail without the fix. The fix is verified by analysis plus the class precedent: oven-sh#34271's ASAN trace demonstrates the identical free-while-probing mechanism on the iteration sites this PR's lookup sites share. ## Verification - `bun bd test test/js/bun/util/filesystem_router.test.ts`: 33 pass (including the 40-round race test) - `bun bd test test/js/bun/resolve/resolve.test.ts`: 73 pass (wildcard exports extension probing) - `bun bd test test/bundler/esbuild/packagejson.test.ts`: 84 pass (exports map resolution) - `bun bd test test/bundler/bundler_edgecase.test.ts`: 134 pass (extension probing, js-to-ts rewrites) - `bun bd test test/bundler/bun-build-api.test.ts`: 52 pass - `bun bd test test/cli/run/env.test.ts`: 96 pass (`.env` discovery through the copied-keys probe) - `bun bd test test/cli/hot/hot.test.ts`: 12 pass, `test/cli/watch/watch.test.ts`: 8 pass - Stress on the fixed debug+ASAN build: 25 long + 150 short fixture runs, 0 failures - `cargo clippy -p bun_resolver -p bun_bundler` clean
1 participant
test/js/bun/util/filesystem_router.test.tswent red on Debian 13 aarch64 in build 74037: thereload() while Bun.build() resolves the same directorysubprocess segfaulted at address0x31after ~24ms.Cause
Symbolizing the crash trace against the build's own
bun-linux-aarch64-profilebinary gives the bundle thread at:resolver.rs:5435isstrings::last_index_of(file.path, "node_modules/"), andfile.pathis whatload_as_filereturned fromquery.entry().abs_path.as_bytes(). That field was lazily filled at five sibling sites in the resolver (load_as_fileplain path and js-to-ts rewrite,load_extension,load_index_with_extension,handle_esm_resolution), each with the shapeNone of them took the per-entry
Entry.mutex, whileRoute::parse(src/router/lib.rs) filled the same field under that mutex; the accompanying SAFETY comments said "resolver mutex held", butload_as_file/load_extension/load_as_file_or_directoryare reached fromresolve_without_symlinkswithout ever takingRESOLVER_MUTEX.Every
reload()busts the directory out of both caches anddir_info_cached_missre-reads it within_place = None, so freshEntryobjects are appended withabs_path = Interned::EMPTY. The bundler'sread_directorythen rewrites that freshDirEntryin place withprev = &mut D.dataand reuses those sameEntryobjects. So the router (JS thread, underEntry.mutex) and the bundler's resolver (bundle thread, no lock) race to fillabs_pathon the very sameEntry.Internedis a#[repr(transparent)] &'static [u8], i.e. a two-word(ptr, len). An unsynchronized two-word store/load pair is a data race; under aarch64's weak memory model the reader can observe the torn(old_ptr, new_len)combination that x86-TSO never produces, andlast_index_ofthen indexes into a slice whose pointer and length do not belong together.This race predates #34271; #34271's intensified test fixture (one priming build, then forty rounds of four concurrent
Bun.build()calls against fiftyreload()s) is what widened the window enough for CI to hit it.Fix
Add
Entry::abs_path_or_fill(&self, fill: impl FnOnce() -> Interned) -> &'static [u8]insrc/resolver/fs.rsnext to the existingEntry::kind/Entry::symlinkhelpers, which already implement the same double-checked-lock lazy-fill for the siblingcache/need_statfields.Entry.abs_pathbecomesCell<Interned>(matchingcache: Cell<EntryCache>andneed_stat: Cell<bool>on the same struct) so the helper writes through&self.All five resolver fill sites collapse to
query.entry().abs_path_or_fill(|| Interned::from_static(...)), dropping the per-siteunsafe { &mut *query.entry }writes and the now-incorrect SAFETY comments.Route::parsedoes the equivalent single-acquire check+fill inline (its fill body canreturn Noneon I/O error, which a closure cannot), and the hot reloader'sabs_pathread is moved inside theEntry.mutexguard it already holds forset_cache_fd/need_stat. The per-site drift that let these five sites miss the lock #33056 established for every otherEntryrewrite is what this centralization removes.Lock order is unchanged: the helper nests
Entry.mutexover theBSSStringListmutexdirname_store.append_slicetakes inside the closure, and nothing under that append touchesEntry.mutex, matching theentries_mutex->Entry.mutexorder #33056 documented.Verification
bun bd test test/js/bun/util/filesystem_router.test.ts(29/29),resolve.test.ts(43/43),import-meta.test.js(32/32),framework-router.test.ts(35/35),hot.test.ts(12/12) all green.bun run rust:check-allclean on all 10 targets.router.routesvalue is a valid absolute.tsxpath after each round, so a tornabs_pathon the router side is observable as a wrongfilePathrather than only as a subprocess crash.The torn state requires a weakly-ordered CPU (x86-TSO only surfaces
(new_ptr, old_len), which is the empty slice), so the fail-before half of the gate's probe cannot fire on the x64 gate host: 30 consecutive release runs and 15 debug+ASAN runs of the test pass there without this change. The aarch64 crash trace above is the direct evidence for the fail-before side.