resolver: take entries_mutex in entries_at before the in-place DirEntry rewrite - #34271
Conversation
…e DirEntry rewrite entries_at replaces a cached DirEntry in place when the caller's resolver generation is newer than the cached listing's, and that replacement drops the old DirEntry's data hashmap (freeing its bucket allocation). The function's comment claims "entries_mutex held by caller", but of its five callers reachable from Bun.build only dir_info_uncached actually holds the lock; finalize_result, handle_esm_resolution, load_index_with_extension, and Transpiler::run_env_loader all reach entries_at after dir_info_cached_maybe_log has already returned and released both guards. FileSystemRouter::reload() and RouteLoader::load iterate that same DirEntry.data map under entries_mutex (the snapshot pattern #33056 introduced). With the rewrite unsynchronized, a Bun.build on the bundler thread can drop the map while reload() on the JS thread is mid-iteration. ASAN catches it as a heap-use-after-free (free at entries_at lib.rs:1639 on the bundler thread, use at filesystem_router.rs:395 / router/lib.rs:816 on the main thread); on a release build it segfaults in bust_dir_cache_recursive. entries_at now takes entries_mutex itself, matching read_directory_with_iterator which does the same. The one call site that already holds the lock (dir_info_uncached via dir_info_cached_miss) routes through a new entries_at_locked / get_entries_ref_locked pair so the non-recursive mutex is not re-acquired. The reload()/Bun.build() concurrency test now awaits one build first (so the bundle thread's generation is already past zero) and then runs forty reload/build rounds instead of one, which is the shape that reaches the stale-generation rewrite path.
|
Warning Review limit reached
Next review available in: 13 seconds 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 (4)
Comment |
|
Reproduced the heap-use-after-free under ASAN with the fixture shape in the PR body (bundler thread frees the The race is scheduling-dependent, so the updated test widens the window (await one build first so the bundle thread's generation is past zero, then forty reload/build rounds) but is not guaranteed to crash an unpatched build on every machine; the ASAN trace above is the deterministic evidence for the before/after. f25d2d2 additionally takes Build 73420's failures ( |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Re the bot's suggested issues: #9517 and #26075 both report an "Unexpected reading file" resolver error rather than a crash. The underlying machinery overlaps (the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I traced the other direction — re-entry deadlock on the now-self-locking entries_at from inside dir_info_cached_miss's critical section (lock taken at resolver.rs:4269). The only entries_at reachable from dir_info_uncached is the parent_.get_entries_ref at line 6207, which this PR routes through _locked; no other get_entries*/get_file_descriptor call sits inside that lock scope. MutexGuard stores the mutex via BackRef (no lifetime), so let _g = self.entries_mutex.lock_guard(); self.entries_at_locked(...) does not conflict on &mut self.
Extended reasoning...
Checked whether moving the lock into entries_at introduces a self-deadlock on the one path that already holds entries_mutex. dir_info_cached_miss takes the lock at line 4269 and calls dir_info_uncached at line 4671 inside that scope. Grepped every entries_at entry point (get_entries, get_entries_ref, get_file_descriptor) — none are called from within dir_info_uncached except the changed line 6207, and none of the get_file_descriptor callers (2901, 3376, 5301, 5313) or get_entries_ref callers (1714, 3790, 3853, 5264) sit inside the 4269 lock scope. Also confirmed dir_info_uncached's two call sites match the PR body's parent-None/Some split (3603 passes None, 4671 passes the slot pointer). Not approving: unsafe-Rust concurrency fix in the resolver hot path warrants human review, and the inline comment flags a same-class sibling in dir_info_for_resolution.
…ce DirEntry rewrite dir_info_for_resolution (the auto-install global-cache path) open-codes the same in-place DirEntry rewrite at resolver.rs:3543/3554 that entries_at does, and similarly did so without entries_mutex held; the block-wide SAFETY comment claiming the lock was held was not backed by a guard anywhere in the function. Take the lock mirroring dir_info_cached_miss. dir_info_uncached is called with parent=None from here, so the get_entries_ref_locked branch (the only entries_mutex re-entry point inside dir_info_uncached) is not reached.
There was a problem hiding this comment.
f25d2d2 addresses the dir_info_for_resolution sibling from my earlier note. Beyond the inline timeout nit, I also checked that the new entries_mutex guard there doesn't re-enter the (non-recursive) lock via dir_info_uncached: it passes parent: None so the get_entries_ref_locked branch is skipped, and the rest of dir_info_uncached's body was already run under this lock from dir_info_cached_miss before this PR — a transitive entries_at reach would have deadlocked deterministically in the test runs listed. Deferring to a human on the broader call-graph audit given this is concurrency-sensitive resolver hot-path code.
Extended reasoning...
The prior review's dir_info_for_resolution finding is resolved by f25d2d2 (now in the diff at resolver.rs:3472). This run's only new finding is the test-timeout nit. I additionally verified that f25d2d2's new lock guard cannot re-enter entries_mutex: dir_info_for_resolution calls dir_info_uncached with parent: None, and every other call inside dir_info_uncached (parse_package_json, parse_tsconfig, entry.kind/symlink) was already executed under the same lock from dir_info_cached_miss (line 4272) prior to this change, so any newly-introduced deadlock would be 100% deterministic and would have hung the resolve/bundler/hot/watch/autoinstall suites the author ran. Not approving because the fix's soundness rests on a call-graph invariant ("only dir_info_cached_miss reaches the parent_.get_entries_ref branch") that a human should confirm for a memory-safety change in the resolver hot path.
…ock in get_file_descriptor - entries_at_locked now debug_asserts is_held_by_current_thread so the "caller must hold entries_mutex" contract is checked rather than written in a comment again. - get_file_descriptor was calling entries_at with generation 0, which never enters the rewrite branch; route it through the same lock-free at_index lookup get_entries_const uses so it does not take entries_mutex for a dead branch. - The two Bun.build subprocess tests now assert stderr in the combined object so an ASAN report is visible in the failure diff instead of being drained and discarded.
|
Heads up: the intensified fixture here caught a remaining race on the bundler side of the same scenario (aarch64, build 74037). Symbolized to |
…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
2 participants
test/js/bun/util/filesystem_router.test.tswent red on alpine x64 in build 73276: thereload() while Bun.build() resolves the same directorysubprocess segfaulted inbust_dir_cache_recursive, inlined fromNonNull::new.Cause
RealFS::entries_at(src/resolver/lib.rs) replaces a cachedDirEntryin place when the caller's resolver generation is newer than the cached listing's. The replacement at*e_ptr = new_entrydrops the oldDirEntry, which drops itsdata: StringHashMap<*mut Entry>and frees the hashmap's bucket allocation. The function's comment saysentries_mutex held by caller, but that is only true on one of the five paths that reach it:dir_info_uncached, when entered fromdir_info_cached_miss. The other callers (finalize_result,handle_esm_resolution,load_index_with_extension,Transpiler::run_env_loader) all reachentries_atafterdir_info_cached_maybe_loghas already returned and released bothRESOLVER_MUTEXandentries_mutex.FileSystemRouter::reload()andRouteLoader::loaditerate the sameDirEntry.datamap underentries_mutex(the snapshot pattern #33056 introduced for exactly this kind of concurrent rewrite). Withentries_at's rewrite unsynchronized, aBun.build()on the bundler thread can drop the map whilereload()on the JS thread is mid-iteration.The generation mismatch is what makes
entries_atenter its rewrite branch, so the window only opens once the bundle thread has processed at least one batch (it bumps its own generation after every queue drain); every subsequentBun.build()then re-reads any directory thatreload()just refreshed to generation 0.ASAN catches it as a heap-use-after-free with the two sides of the race laid out exactly:
(The use side is sometimes
RouteLoader::loadatsrc/router/lib.rs:816instead; same map, same lock.)This has been the shape of
entries_atsince the Rust port; #33056 narrowed the race by snapshotting under the lock but assumed the rewrite side already held it.Fix
entries_atnow takesentries_mutexitself, matchingread_directory_with_iteratorwhich already does. The one call path that reaches it with the lock already held (dir_info_cached_miss->dir_info_uncached->parent_.get_entries_ref) routes through a newentries_at_locked/get_entries_ref_lockedpair so the non-recursive mutex is not re-entered. That path is the only one that passes a non-Noneparent todir_info_uncached; the other caller (dir_info_for_resolution) passesNone, so the parent branch containing the accessor never runs there.Test
The existing concurrency test now awaits one
Bun.build()first, so the bundle thread's generation is already past zero when the concurrent rounds start, and then runs forty reload/build rounds instead of one. That is the shape that reaches the stale-generation rewrite at all; the original single-round fixture usually completes with every build still on generation 0.The race is scheduling-dependent. Pinning the fixture to a single core reproduces the ASAN use-after-free on roughly 3 in 10 runs against an unpatched debug build and 0 in 15 with this change; with all 16 cores available the unpatched build reproduces at roughly 1 in 30. The assertions are otherwise the same as before, so the test continues to cover the behavior #33056 added.
Also ran the full
filesystem_router.test.ts,test/bundler/bun-build-api.test.ts(including the thousands-of-builds test that exercises the generation path heavily),test/js/bun/resolve/resolve.test.ts,test/cli/hot/hot.test.ts,test/cli/watch/watch.test.ts,test/bake/framework-router.test.ts, andbun run rust:check-all(10/10 targets).no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/filesystem_router.test.ts