resolver: lock DirEntry map probes against the in-place stale-generation rewrite

[robobun]

Fixes #37266
Fixes #23773

Crash

In build 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 resolver: take entries_mutex in entries_at before the in-place DirEntry rewrite #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

[coderabbitai]

Walkthrough

The resolver now uses generation-checked, mutex-protected directory entry lookups. Entry stat caches use atomic publication ordering. Dotenv probing copies directory basenames into owned storage, and hot-reloader invalidation uses an atomic release store.

Changes

Resolver entry safety

Layer / File(s)	Summary
Atomic stat cache synchronization src/resolver/fs.rs, src/jsc/hot_reloader.rs	Entry::need_stat now uses AtomicBool with acquire, relaxed, and release ordering. Hot-reloader invalidation uses a release store.
Generation-checked directory lookup src/resolver/dir_info.rs, src/resolver/lib.rs, src/resolver/resolver.rs, src/runtime/cli/*	Resolver paths use locked, generation-aware entry lookups and captured directory descriptors for symlink, ESM, wildcard, index, and package resolution. CLI entry probes now hold entries_mutex.
Owned dotenv probe keys src/dotenv/*, src/bundler/transpiler.rs, src/install/*	Dotenv loading copies directory basenames while holding entries_mutex and passes them through DirEntryKeys by reference.

Possibly related issues

• oven-sh/bun#37266 — Addresses the entries_at and in-place DirEntry race associated with reload() and Bun.build() crashes.

Possibly related PRs

• oven-sh/bun#36675 — Refactors resolver directory-entry synchronization and refresh behavior.
• oven-sh/bun#36251 — Modifies resolver cache synchronization and hot-reloader directory-entry access.
• oven-sh/bun#37276 — Also synchronizes hot-reloader directory-cache access with entries_mutex.

Suggested reviewers: jarred-sumner, dylan-conway, cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: locking DirEntry map probes during stale-generation rewrites.
Description check	✅ Passed	The description explains the cause, fix, scope, residual risks, linked issues, and verification results, covering the template requirements.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/resolver/fs.rs (1)

498-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Publish the Release store after the cache writes.

The Release store at Line 501 runs before the set_cache_kind / set_cache_symlink writes at Lines 511-512. Today this is safe only because the two conditions are coupled: the stored value can be false only when found_kind.is_some() and Some(existing.cache().kind) == found_kind, in which case the if block does not run. A later change to either condition would clear need_stat before the cache writes and expose the stale cache to lock-free kind() / symlink() readers.

Compute the flag first, apply the cache writes, then publish with the Release store.

♻️ Proposed reordering

-                    // Relaxed load: writes are serialized on the per-entry
-                    // mutex held above; Release store pairs with the Acquire
-                    // fast-path load in `kind()`/`symlink()`.
-                    existing.need_stat.store(
-                        existing.need_stat.load(Ordering::Relaxed)
-                            || found_kind.is_none()
-                            || Some(existing.cache().kind) != found_kind,
-                        Ordering::Release,
-                    );
-                    // TODO: is this right?
-                    if Some(existing.cache().kind) != found_kind {
-                        // if found_kind is null, we have set need_stat above, so we
-                        // store an arbitrary kind
-                        existing.set_cache_kind(found_kind.unwrap_or(EntryKind::File));
-                        existing.set_cache_symlink(Interned::EMPTY);
-                    }
+                    // Relaxed load: writes are serialized on the per-entry
+                    // mutex held above.
+                    let kind_changed = Some(existing.cache().kind) != found_kind;
+                    let needs_stat = existing.need_stat.load(Ordering::Relaxed)
+                        || found_kind.is_none()
+                        || kind_changed;
+                    // TODO: is this right?
+                    if kind_changed {
+                        // if found_kind is null, we have set need_stat above, so we
+                        // store an arbitrary kind
+                        existing.set_cache_kind(found_kind.unwrap_or(EntryKind::File));
+                        existing.set_cache_symlink(Interned::EMPTY);
+                    }
+                    // Release store publishes the `cache` writes above to the
+                    // Acquire fast-path load in `kind()`/`symlink()`.
+                    existing.need_stat.store(needs_stat, Ordering::Release);

🤖 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 498 - 513, In the existing entry-update
flow, compute the need_stat flag before modifying state, perform the conditional
set_cache_kind and set_cache_symlink writes, then publish the computed flag with
the existing Release store on existing.need_stat. Preserve the current
conditions and fallback kind while ensuring the Release store occurs after both
cache writes.

src/resolver/resolver.rs (1)

3948-3960: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve the trailing separator for the node_modules check. path_contains_node_modules_folder searches for <SEP>node_modules<SEP>, so /project/node_modules returns false while /project/node_modules/ returns true. When DISABLE_AUTO_JS_TO_TS_IN_NODE_MODULES is enabled, this allows .mjs → .mts probing in that directory. Handle resolved_dir_info.is_node_modules() or pass a separator-terminated path.

🤖 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/resolver.rs` around lines 3948 - 3960, Update the `.mjs` branch
in the extension selection logic to correctly recognize `node_modules`
directories when `DISABLE_AUTO_JS_TO_TS_IN_NODE_MODULES` is enabled. Use
`resolved_dir_info.is_node_modules()` or pass a separator-terminated path to
`strings::path_contains_node_modules_folder`, preserving the existing `.mts`
probing behavior outside `node_modules`.

🤖 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/resolver/resolver.rs`:
- Around line 5397-5449: Capture the directory file descriptor while the
`entries_mutex` lookup remains protected, using the value associated with
`lookup` rather than calling `dir_info.get_file_descriptor()` after unlock.
Store that captured descriptor and reuse it in both `MatchResult` constructions
in this file-result path, matching the existing ESM path behavior.

---

Outside diff comments:
In `@src/resolver/fs.rs`:
- Around line 498-513: In the existing entry-update flow, compute the need_stat
flag before modifying state, perform the conditional set_cache_kind and
set_cache_symlink writes, then publish the computed flag with the existing
Release store on existing.need_stat. Preserve the current conditions and
fallback kind while ensuring the Release store occurs after both cache writes.

In `@src/resolver/resolver.rs`:
- Around line 3948-3960: Update the `.mjs` branch in the extension selection
logic to correctly recognize `node_modules` directories when
`DISABLE_AUTO_JS_TO_TS_IN_NODE_MODULES` is enabled. Use
`resolved_dir_info.is_node_modules()` or pass a separator-terminated path to
`strings::path_contains_node_modules_folder`, preserving the existing `.mts`
probing behavior outside `node_modules`.

🪄 Autofix

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: a31c4b1a-6305-4b39-9e70-904969657794

📥 Commits

Reviewing files that changed from the base of the PR and between d2f48c6 and 594e54b.

📒 Files selected for processing (5)

• src/bundler/transpiler.rs
• src/jsc/hot_reloader.rs
• src/resolver/dir_info.rs
• src/resolver/fs.rs
• src/resolver/resolver.rs

[robobun]

Addressed the review in 137b425:

• load_index_with_extension captures the listing fd in the same entries_mutex critical section as the basename probe (gated on STORE_FILE_DESCRIPTORS to match get_file_descriptor) instead of re-reading the DirEntry field after unlock.
• The recycle path in add_entry_with_store now performs its cache writes before the Release store of need_stat, so the publication order matches kind()/symlink() instead of relying on the coupled conditions.
• The .mjs to .mts probe gate uses the DirInfo node_modules flags (already used for extension ordering on the same DirInfo) instead of scanning the path for a separator-bounded needle that misses the node_modules directory itself.

[github-actions]

Found 1 issue this PR may fix:

1. Segmentation fault during build #23773 - The crashing frame (fs.zig:397 inlined into Resolver.finalizeResult) is the entry.cache = kind(...) write inside Entry.symlink(), hitting both windows this PR closes: finalize_result's unlocked DirEntry map probe and the need_stat-before-cache publication order — and it reproduces ~1 in 20 builds on linux aarch64 from a Bun.build plugin onResolve → Bun.resolveSync, the same concurrent bundler-thread/JS-thread shape.

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #23773

🤖 Generated with Claude Code

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/resolver/resolver.rs:5394-5401 — load_as_file (resolver.rs:5846→5877→5894/5993) and load_extension (:6098) have the identical unlocked DirEntry.data probe this PR fixes here — read_directory releases entries_mutex on return, then BackRef::new(dir_entry.entries()) is captured and .get() walks the map with no lock, while the SAFETY comments at :5895/:5994/:6099 still claim "entries_mutex held". load_as_file is reached from the same load_as_file_or_directory parent as this fixed site and is the hotter path (every relative-file import), so the same freed-bucket-walk crash vector remains open; the PR description excludes hot_reloader.rs explicitly but does not mention these.

  Extended reasoning...

  What the bug is

  This PR's window #1 fix — probe DirEntry.data inside an entries_mutex critical section instead of after the lock is released — is applied to finalize_result, handle_esm_resolution, load_index_with_extension, probe_wildcard_extensions, and run_env_loader. But load_as_file (resolver.rs:5810) and its helper load_extension (resolver.rs:6071) have the identical pattern and were not touched:

  • resolver.rs:5846 — unsafe { &mut *rfs }.read_directory(dir_path, None, self.generation, ...). read_directory_with_iterator takes entries_mutex via _unlock_guard (lib.rs:1183-1187) and releases it when the function returns.
  • resolver.rs:5877 — let entries = bun_ptr::BackRef::new(dir_entry.entries()): captures a &DirEntry with no lock held.
  • resolver.rs:5894, :5993, and (via load_extension) :6098 — entries!().get(...) / entries.get().get(file_name) walk DirEntry.data buckets with no entries_mutex.
  • The SAFETY comments at :5895/:5994/:6099 read "entries_mutex held", which is factually false — the guard already dropped inside read_directory_with_iterator.

  The code path that triggers it

  Between the read_directory return and the unlocked .get(), a concurrent resolver at a newer generation reaches entries_at_locked (lib.rs:1567) or the in-place branch of read_directory_with_iterator (lib.rs:1282-1290) for the same directory, and under entries_mutex does data.clear() + *e_ptr = new_entry — dropping the old StringHashMap<*mut Entry> and freeing its bucket allocation. The unlocked .get() on thread A then walks freed buckets, loads a garbage *mut Entry, and faults at a small absolute address — the exact mechanism the PR description names for the 5 sites it fixed.

  Why existing code doesn't prevent it

  load_as_file is reached from load_as_file_or_directory (resolver.rs:5563), the same parent that reaches load_as_index → load_index_with_extension (:5551) which this PR did fix. Neither RESOLVER_MUTEX nor entries_mutex is held around the parent call: RESOLVER_MUTEX is taken only inside dir_info_cached_maybe_log and released before it returns, and the concurrent-rewrite paths take entries_mutex only. So the locking context at load_as_file is provably identical to the sibling site the PR just fixed — if load_index_with_extension needed the lock, load_as_file needs it for the same reason. The unlocked get_entries_ref accessor was deleted, but the equivalent unlocked path via read_directory + BackRef remains.

  Step-by-step proof

  1. Thread A (resolver, generation N) enters load_as_file("./foo"), calls rfs.read_directory(dir_path, None, N, ...) at :5846. Inside read_directory_with_iterator, _unlock_guard = entries_mutex.lock_guard() is taken (lib.rs:1183), the cached DirEntry is returned, and the guard drops on return.
  2. Thread A executes :5877: entries = BackRef::new(dir_entry.entries()) — a raw pointer to the cached DirEntry, no lock held.
  3. Thread B (Bun.build() resolver at generation N+1 after reload()) reaches entries_at_locked for the same directory, takes entries_mutex, sees existing.generation < N+1, and rewrites in place: data.clear() + *e_ptr = new_entry (lib.rs:1604-1606 / 1282-1290). The old data map's bucket allocation is freed. Thread B releases the lock.
  4. Thread A executes :5894: entries!().get(base) → self.data.get(query) walks the freed bucket array, reads a garbage *mut Entry, and query.entry().kind(...) dereferences it → SEGV at a small address (matches the 0x25 in the CI crash).

  This is the reload() while Bun.build() resolves the same directory fixture in filesystem_router.test.ts — the PR's own reproduction scenario.

  Impact

  load_as_file is the hotter path than any of the five fixed sites: every relative-file import (./foo, ../bar) goes through it before load_as_index runs. The PR description's Cause section says the fix "closes the remaining windows" of window #1; it does not — the highest-traffic instance of that window is left open, so the same low-probability SEGV-at-small-address the PR is fixing can still fire.

  Per REVIEW.md: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep) … If a site is intentionally excluded, say so in the PR." The PR description explicitly excludes hot_reloader.rs and defers abs_path to #34411, but load_as_file/load_extension are unmentioned. Also REVIEW.md: "SAFETY comments … must be accurate" — the three stale "entries_mutex held" comments now directly contradict the locking discipline this PR establishes at every sibling site it touched.

  How to fix

  Mirror the load_index_with_extension change: replace the read_directory + BackRef<DirEntry> + unlocked entries!().get(...) sequence with per-probe DirInfo::get_entry(self.generation, name) (or take entries_mutex explicitly around the probe and capture entries.fd under the same critical section, as done in handle_esm_resolution). load_extension should take a DirInfoRef + captured dirname_fd instead of &DirEntry, exactly as probe_wildcard_extensions was reworked. Delete or correct the three false SAFETY comments.

[robobun]

Both review findings are addressed in a9f3629:

• load_as_file and load_extension now probe through EntriesOption::lookup, which takes entries_mutex for the map walk and captures the listing fd in the same critical section; the watch-registration read goes through dir_and_fd(). The stale SAFETY comments claiming the lock was held are gone, and load_extension takes the slot handle instead of a bare &DirEntry.
• The recycle path in add_entry_with_store no longer rewrites the cache at all. The reviewer is right that no store order can publish anything to a reader that already loaded the stale need_stat flag, so the overwrite itself was the hazard; it now only publishes need_stat = true and the next kind()/symlink() re-stats under the per-entry mutex. set_cache_kind had no remaining callers and is removed.

Also audited the remaining DirEntry access sites: the dir_info_uncached probes run with entries_mutex held by dir_info_cached_miss (the path #34271 documented), and the global-disk-cache population reads a slot this thread just created. The PR description is updated accordingly.

[robobun]

Follow-up in 1be4e6a, from a deeper audit of the locking contract:

• DirEntry::get / get_comptime_query / has_comptime_query now debug-assert that the caller holds entries_mutex. Since this race has no reproducing test, the assert is the enforcement: the next unlocked probe fails deterministically in debug builds instead of surfacing as a rare segfault.
• The callers the assert would have caught are converted: the dotenv loader now always probes a DirEntryKeys snapshot copied out under the lock (the DirEntryProbe impl for the live DirEntry is deleted, so the type system forces the copy; covers run_env_loader, PackageManager::init, and the lockfile printer), the lockfile-presence and pm view probes take the uncontended guard, and the hot reloader's changed-file probe runs under the lock.
• DirInfo::get_entries (raw pointer return, doc claiming callers can locally prove exclusivity, which this PR showed to be false at its one deref site) is deleted along with the now-uncalled RealFS::entries_at; get_entries_ref_locked is pub for the single-critical-section snapshot pattern; get_entries_const documents which fields are safe to read unlocked.

Verified with the assert-armed debug build: the full filesystem_router, resolve, env, hot, watch, and packagejson suites, plus bun install / run / pm ls / pm view / run --filter / bun build smoke runs and the concurrency fixtures. No assert fires.

[robobun]

Data point for the remaining-windows analysis here: the stress fixture added in #37276 (--hot process running FileSystemRouter.reload() plus four concurrent Bun.build() calls while the watched directory is churned from outside) segfaulted at address 0x1E, 41ms in, on its first CI run on ubuntu 25.04 aarch64 release (build 91033). That sha has #37276's watcher-side locks but not this PR's resolver-side ones, and the watcher-side reads in that fixture cannot tear (its churn entries never get abs_path filled), so the crash is consistent with the lookup-side windows this PR closes (or #34411's fill race). Since local stress runs did not reproduce for you, that fixture on the ubuntu-aarch64 lane may be a faster verification vehicle; #37276 gates it to ASAN lanes for now for exactly this reason.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/resolver/fs.rs (1)

225-287: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift

Synchronize every EntryCache read with every cache write.

kind(), symlink(), and direct cache() reads can access Cell<EntryCache> without mutex. set_cache_fd and set_cache_symlink write the same cell under mutex only. The watcher thread can update an entry while the resolver reads it, causing undefined behavior and a torn Interned value. Moving need_stat.store(true) before the write is not sufficient because a reader can already have observed false. Apply one synchronization scheme to all cache accesses, including src/jsc/hot_reloader.rs:1190 and the setters at src/resolver/resolver.rs:1712, 1736, 1754, 6358, and 6410.

🤖 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 225 - 287, Synchronize all EntryCache reads
and writes through the same mutex to prevent unsynchronized Cell<EntryCache>
access. Update kind() and symlink() in src/resolver/fs.rs (225-287), the cache
read in src/jsc/hot_reloader.rs (1189-1194), and the setter paths in
src/resolver/resolver.rs (1709-1713 and 1749-1754); apply the same
synchronization to the additional setters at 1736, 6358, and 6410. Do not rely
on need_stat ordering alone.

🤖 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.

Outside diff comments:
In `@src/resolver/fs.rs`:
- Around line 225-287: Synchronize all EntryCache reads and writes through the
same mutex to prevent unsynchronized Cell<EntryCache> access. Update kind() and
symlink() in src/resolver/fs.rs (225-287), the cache read in
src/jsc/hot_reloader.rs (1189-1194), and the setter paths in
src/resolver/resolver.rs (1709-1713 and 1749-1754); apply the same
synchronization to the additional setters at 1736, 6358, and 6410. Do not rely
on need_stat ordering alone.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: da8170b7-27aa-445b-9b3d-ac7e9b1b02a4

📥 Commits

Reviewing files that changed from the base of the PR and between 594e54b and ea7d69f.

📒 Files selected for processing (12)

• src/bundler/transpiler.rs
• src/dotenv/env_loader.rs
• src/dotenv/lib.rs
• src/install/PackageManager.rs
• src/install/lockfile.rs
• src/jsc/hot_reloader.rs
• src/resolver/dir_info.rs
• src/resolver/fs.rs
• src/resolver/lib.rs
• src/resolver/resolver.rs
• src/runtime/cli/pm_view_command.rs
• src/runtime/cli/run_command.rs

[robobun]

On the remaining EntryCache synchronization finding: that is the residual the PR description already calls out under known residual races, and it is deferred deliberately rather than missed.

Closing it means either taking the per-entry mutex on every cache() read (the lock-free double-checked fast path in kind()/symlink() is a deliberate perf choice on the resolver's hottest path) or redesigning EntryCache around a seqlock or atomics (the 16-byte Interned cannot be read atomically on our targets). Either is a measurable-perf design change that deserves its own PR and benchmarks, not a rider on this fix.

What this PR did do to narrow that window: the one systematic different-value cache writer (the recycle-path overwrite, firing on every re-read on DT_UNKNOWN filesystems) is gone, the lazy-stat result is now published with Release/Acquire ordering, and every map probe is locked. The writers left are the rare getFdPath symlink-directory fill in finalize_result and dir_info_uncached's one-time fill, both writing under the per-entry mutex against readers that raced the need_stat flag. The watcher-thread side is #37276.