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

Merged
JA
4mo ago
+295-141

What

The filesystem-router load loops (Bun.FileSystemRouter, bake's FrameworkRouter, bust_dir_cache_recursive) iterate the resolver's cached DirEntry hashmap, and Entry's lazily-populated stat cache (need_stat / cache Cells) is rewritten from several places. Both happened without the locks the rest of the cache uses, so a router.reload() on the JS thread concurrent with Bun.build() (whose entry-point resolution runs on the bundler thread and rewrites the same cached DirEntry in place under entries_mutex) reliably crashes:

Calling reload() on a Bun.FileSystemRouter while a concurrent Bun.build() re-reads the same
directory makes the router iterate a cached directory-entry map that the bundler thread is
rewriting in place under its own lock; the route-load loop then reads hashmap buckets that were
just freed and crashes. The new test reproduces this deterministically.

Changes, using only the two locks that already exist:

  • Snapshot under entries_mutex. Each load/bust/scan loop now copies the DirEntry's *mut Entry list while holding the existing RealFS.entries_mutex (the same lock the in-place rewrite holds), then drops the guard before the loop body — the read_dir_info recursion re-acquires it, so the guard is never held across it. Entry slots live in the process-lifetime EntryStore, so the snapshotted pointers stay valid.
  • Per-entry Entry.mutex on every rewrite. Entry::kind / Entry::symlink take Entry.mutex (double-checked; the cached fast path is untouched) inside the lazy-stat branch, and every other Entry rewrite site (Route::parse's set_abs_path, the resolver's set_cache_fd / set_cache_symlink callers, the hot reloader's fd reset) takes the same lock — matching what the directory re-read path (add_entry_with_store) already did. Lock order is always entries_mutex → Entry.mutex; no critical section under Entry.mutex acquires entries_mutex.
  • Stable directory names. An in-place DirEntry refresh now reuses the slot's existing interned dir instead of re-interning the caller's spelling: the resolver and the router spell the same directory with and without a trailing slash, so the old code rewrote Entry.dir (a 2-word slice) to a different value under unlocked readers. The route loader also now accepts both spellings — Bun.build() followed by new Bun.FileSystemRouter() on the same directory tripped an out-of-bounds index in a debug assertion 100% deterministically (debug builds only; release behavior was unaffected because both spellings trim to the same public_dir).
  • Corrected the SAFETY / doc comments that claimed entries_mutex serialized all Entry access.

Intentionally unchanged, called out for review:

  • Reads (cache(), base(), dir(), abs_path()) stay lock-free per the existing design; the rewrite sites now always write back the same or a freshly-published value, but this is not full read-side synchronization.
  • dir_entry_accessor::DirEntryDirIter also iterates a DirEntry map without the lock; its only caller is the single-threaded CLI --filter glob, so it was left alone.

Tests

Two new subprocess tests in test/js/bun/util/filesystem_router.test.ts:

  • reload() while Bun.build() resolves the same directory — fails (segfault, empty stdout) with USE_SYSTEM_BUN=1, passes with the fix (30/30 repeated runs of the fixture on a debug build).
  • loads routes from a directory already cached by Bun.build() — the deterministic sequential repro of the trailing-slash assertion; it only crashes on debug builds of main (the broken assertion is debug_assert!-gated), so it passes under USE_SYSTEM_BUN=1 but fails on an unfixed bun bd test run.

Also ran the full filesystem_router.test.ts, test/bake/framework-router.test.ts, test/bundler/bun-build-api.test.ts, test/cli/hot/hot.test.ts, test/cli/watch/watch.test.ts, and bun run rust:check-all (10/10 targets).