hot_reloader: take entries_mutex for watcher-thread directory cache probes

src/jsc/hot_reloader.rs

@@ -405,8 +405,14 @@ pub struct NewHotReloader<Ctx, EventLoopType, const RELOAD_IMMEDIATELY: bool> {
 
     pub(crate) main: MainFile,
 
+    /// Last cached listing seen for each watched directory, kept after
+    /// `bust_dir_cache` evicts the directory so later events can still
+    /// invalidate the per-file stat caches of its entries. The pointees are
+    /// cache-owned leaked `Box<DirEntry>`s (process lifetime), but their
+    /// `data` maps are rewritten in place under `entries_mutex`, so every
+    /// walk takes that lock (see `probe_entries_cache`).
     #[cfg(not(windows))]
-    pub(crate) tombstones: StringHashMap<*mut Fs::EntriesOption>,
+    pub(crate) tombstones: StringHashMap<*mut Fs::DirEntry>,
 
     /// See [`HotReloaderCtx::reload_handle`].
     pub(crate) reload_handle: Option<crate::VmHandle>,
@@ -792,15 +798,42 @@ where
     }
 
     #[cfg(not(windows))]
-    fn put_tombstone(&mut self, key: &[u8], value: *mut Fs::EntriesOption) {
+    fn put_tombstone(&mut self, key: &[u8], value: *mut Fs::DirEntry) {
         self.tombstones.put(key, value).expect("unreachable");
     }
 
     #[cfg(not(windows))]
-    fn get_tombstone(&mut self, key: &[u8]) -> Option<*mut Fs::EntriesOption> {
+    fn get_tombstone(&mut self, key: &[u8]) -> Option<*mut Fs::DirEntry> {
         self.tombstones.get(key).copied()
     }
 
+    /// Looks up `key` in the process-global directory-entry cache and, when a
+    /// listing is cached, returns the `DirEntry`'s stable address (`None` for
+    /// a cached read error).
+    ///
+    /// This runs on the watcher thread while resolver/bundler threads rewrite
+    /// the cache in place under `entries_mutex` (`read_directory`,
+    /// `entries_at`), so the probe takes that lock. The returned pointer is a
+    /// cache-owned leaked `Box<DirEntry>` and stays valid after the guard
+    /// drops; only the lookup itself is a critical section. Lock order: the
+    /// platform watcher holds `Watcher.mutex` around `on_file_update`, and
+    /// `Watcher.mutex` → `entries_mutex` is the established order
+    /// (`bust_dir_cache` takes it the same way); nothing acquires them in
+    /// reverse.
+    #[cfg(not(windows))]
+    fn probe_entries_cache(
+        rfs: &mut Fs::file_system::RealFS,
+        key: &[u8],
+    ) -> Option<*mut Fs::DirEntry> {
+        let _entries_lock = rfs.entries_mutex.lock_guard();
+        match rfs.entries.get(key) {
+            Some(Fs::EntriesOption::Entries(existing)) => {
+                Some(std::ptr::from_mut::<Fs::DirEntry>(*existing))
+            }
+            _ => None,
+        }
+    }
+
     pub(crate) fn on_error(_: &mut Self, err: &bun_sys::Error) {
         // `bun_sys::Error::name()` does the errno→tag-name lookup.
         Output::err(err.name(), "Watcher crashed", ());
@@ -969,7 +1002,7 @@ where
                     #[cfg(not(windows))]
                     {
                         let mut affected_buf: [&[u8]; 128] = [b"".as_slice(); 128];
-                        let mut entries_option: Option<*mut Fs::EntriesOption> = None;
+                        let mut entries_option: Option<*mut Fs::DirEntry> = None;
 
                         // Note: the affected-name element type differs by
                         // platform (kqueue vs inotify). Split into two locals;
@@ -980,9 +1013,7 @@ where
 
                         let affected_len: usize = 'brk: {
                             if IS_KQUEUE {
-                                // SAFETY: hot-reload runs single-threaded on the JS thread;
-                                // no other live `&mut EntriesOption` for this key here.
-                                if let Some(existing) = rfs.entries.get(file_path) {
+                                if let Some(existing) = Self::probe_entries_cache(rfs, file_path) {
                                     self.put_tombstone(file_path, existing);
                                     entries_option = Some(existing);
                                 } else if let Some(existing) = self.get_tombstone(file_path) {
@@ -1073,7 +1104,7 @@ where
                         };
 
                         if affected_len > 0 && !IS_KQUEUE {
-                            if let Some(existing) = rfs.entries.get(file_path) {
+                            if let Some(existing) = Self::probe_entries_cache(rfs, file_path) {
                                 self.put_tombstone(file_path, existing);
                                 entries_option = Some(existing);
                             } else if let Some(existing) = self.get_tombstone(file_path) {
@@ -1134,11 +1165,6 @@ where
                         }
 
                         if let Some(dir_ent) = entries_option {
-                            // SAFETY: dir_ent points into rfs.entries (or a tombstoned copy);
-                            // both outlive this loop iteration. Shared access only —
-                            // `entries()` takes `&self` and per-entry mutation below goes
-                            // through the entry's own mutex + cells.
-                            let dir_ent = unsafe { &*dir_ent };
                             let mut last_file_hash: bun_watcher::HashType =
                                 bun_watcher::HashType::MAX;
 
@@ -1171,10 +1197,33 @@ where
                                     let path_string: bun_ptr::Interned;
                                     let file_hash: bun_watcher::HashType;
                                     let abs_path: &[u8] = 'brk: {
-                                        if let Some(file_ent) = dir_ent.entries().get(changed_name)
-                                        {
+                                        // Walk the cached listing's `data`
+                                        // map under `entries_mutex`: resolver
+                                        // and bundler threads rewrite it in
+                                        // place under that lock
+                                        // (`entries_at_locked`), dropping the
+                                        // old bucket allocation, so an
+                                        // unlocked walk can read freed
+                                        // memory. The `*mut Entry` the lookup
+                                        // yields is EntryStore-owned (process
+                                        // lifetime) and stays valid after the
+                                        // guard drops.
+                                        let file_ent: Option<core::ptr::NonNull<Fs::Entry>> = {
+                                            let _entries_lock = rfs.entries_mutex.lock_guard();
+                                            // SAFETY: `dir_ent` is cache-owned
+                                            // (see `probe_entries_cache`); the
+                                            // shared borrow is confined to
+                                            // this statement, under the lock
+                                            // that serializes in-place
+                                            // rewrites.
+                                            let lookup = unsafe { (*dir_ent).get(changed_name) };
+                                            lookup.map(|l| core::ptr::NonNull::from(l.entry()))
+                                        };
+                                        if let Some(file_ent) = file_ent {
                                             // reset the file descriptor
-                                            let ent = file_ent.entry();
+                                            // SAFETY: EntryStore-owned slot;
+                                            // never freed.
+                                            let ent = unsafe { file_ent.as_ref() };
                                             {
                                                 // Every cached-`Entry` rewrite takes
                                                 // the per-entry mutex.

test/cli/hot/hot.test.ts

@@ -1,7 +1,7 @@
 import { spawn } from "bun";
 import { beforeEach, expect, it } from "bun:test";
 import { copyFileSync, cpSync, readFileSync, renameSync, rmSync, unlinkSync, writeFileSync } from "fs";
-import { bunEnv, bunExe, isDebug, isWindows, tmpdirSync, waitForFileToExist } from "harness";
+import { bunEnv, bunExe, isDebug, isWindows, tempDir, tmpdirSync, waitForFileToExist } from "harness";
 import { join } from "path";
 
 const timeout = isDebug ? Infinity : 10_000;
@@ -776,3 +776,113 @@
   },
   longTimeout,
 );
+
+// The watcher thread walks the cached directory listing of a changed watched
+// directory, while FileSystemRouter.reload() and Bun.build() rewrite the same
+// listing in place on the JS/bundler threads. Skipped on Windows: the watcher
+// handles directory events there without touching the listing.
+it.skipIf(isWindows)(
+  "directory events race reload() and Bun.build() rewriting the same cached listing",
+  async () => {
+    const files: Record<string, string> = {
+      "main.ts": /* ts */ `
+        import "./pages/p1.tsx";
+        import "./pages/p2.tsx";
+        import path from "path";
+        const pagesDir = path.join(import.meta.dir, "pages");
+        const entrypoints: string[] = [];
+        for (let i = 1; i <= 20; i++) entrypoints.push(path.join(pagesDir, "p" + i + ".tsx"));
+        const router = new Bun.FileSystemRouter({
+          dir: pagesDir,
+          style: "nextjs",
+          fileExtensions: [".tsx"],
+        });
+        // The first build completes with generation 0 and the bundle thread
+        // then bumps its generation, so every later build's resolver re-reads
+        // the directory listing in place.
+        await Bun.build({ entrypoints, target: "bun", throw: false });
+        console.log("ready");
+        let matches = 0;
+        let buildsOk = true;
+        for (let round = 0; round < 30; round++) {
+          const builds = Array.from({ length: 4 }, () =>
+            Bun.build({ entrypoints, target: "bun", throw: false }),
+          );
+          for (let i = 0; i < 50; i++) {
+            router.reload();
+            const m = router.match("/p7");
+            if (m && m.filePath.endsWith("p7.tsx")) matches++;
+          }
+          const results = await Promise.all(builds);
+          buildsOk &&= results.every(r => r.success);
+        }
+        console.log("matches", matches, "builds-ok", buildsOk);
+        process.exit(0);
+      `,
+    };
+    for (let i = 1; i <= 20; i++) {
+      files[`pages/p${i}.tsx`] = `export default ${i};\n`;
+    }
+    using dir = tempDir("hot-direntry-race", files);
+
+    // --hot watches main.ts, the imported pages, and (through them) pages/
+    // itself as a directory. Run in a subprocess so a crash is observable as
+    // a signal instead of taking down the test runner.
+    await using proc = spawn({
+      cmd: [bunExe(), "--hot", "main.ts"],
+      env: bunEnv,
+      cwd: String(dir),
+      stdout: "pipe",
+      stderr: "pipe",
+      stdin: "ignore",
+    });
+
+    let stdout = "";
+    const ready = Promise.withResolvers<void>();
+    const stdoutDone = (async () => {
+      const decoder = new TextDecoder();
+      for await (const chunk of proc.stdout) {
+        stdout += decoder.decode(chunk, { stream: true });
+        if (stdout.includes("ready\n")) ready.resolve();
+      }
+    })();
+    // If the fixture dies before printing "ready", unblock the wait so the
+    // assertions below report the crash instead of hanging.
+    const exited = proc.exited.then(code => {
+      ready.resolve();
+      return code;
+    });
+    await ready.promise;
+
+    // Churn pages/ from outside while the fixture's reload()/build() loops
+    // rewrite its cached listing: every write lands a directory event on the
+    // watcher thread, which then walks that listing. The churned names use a
+    // transpilable extension (so the walk visits them) but are never imported
+    // and are excluded by fileExtensions, so no reload fires and the
+    // fixture's counters stay deterministic.
+    let running = true;
+    void exited.finally(() => {
+      running = false;
+    });
+    let i = 0;
+    while (running) {
+      for (let k = 0; k < 4; k++, i++) {
+        writeFileSync(join(String(dir), "pages", `churn-${i % 32}.ts`), `export const v = ${i};\n`);
+      }
+      if (i % 64 === 0) {
+        rmSync(join(String(dir), "pages", `churn-${(i + 8) % 32}.ts`), { force: true });
+      }
+      await Bun.sleep(4);
+    }
+
+    const [stderr, exitCode] = await Promise.all([proc.stderr.text(), exited]);
+    await stdoutDone;
+    expect({ stdout, stderr, exitCode, signalCode: proc.signalCode }).toEqual({
+      stdout: "ready\nmatches 1500 builds-ok true\n",
+      stderr: "",
+      exitCode: 0,
+      signalCode: null,
+    });
+  },
+  isDebug ? Infinity : 60_000,
+);