diff --git a/src/jsc/hot_reloader.rs b/src/jsc/hot_reloader.rs index a95eba4b929e..92f44aa9298b 100644 --- a/src/jsc/hot_reloader.rs +++ b/src/jsc/hot_reloader.rs @@ -405,8 +405,10 @@ pub struct NewHotReloader { pub(crate) main: MainFile, + /// Last cached listing per watched directory, kept past `bust_dir_cache` + /// to invalidate per-file stat caches; see [`Self::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, @@ -792,15 +794,39 @@ 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 returns + /// the cached `DirEntry`'s stable address (`None` for a cached read + /// error). + /// + /// Resolver and bundler threads rewrite the cache in place under + /// `entries_mutex` (`read_directory`, `entries_at`), so this watcher + /// thread takes that lock for every probe or `data`-map walk. The pointee + /// is a cache-owned leaked `Box` (process lifetime), valid + /// after the guard drops. Lock order `Watcher.mutex` then `entries_mutex` + /// matches this thread's `bust_dir_cache`; nothing takes the 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::(*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 +995,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 +1006,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 +1097,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 +1158,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 +1190,23 @@ 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) - { + // Locked walk (see `probe_entries_cache`); + // the `*mut Entry` is EntryStore-owned and + // may outlive the guard. + let file_ent: Option> = { + 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. + 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. diff --git a/test/cli/hot/hot.test.ts b/test/cli/hot/hot.test.ts index 8ab6f31dd9e6..80f45026f836 100644 --- a/test/cli/hot/hot.test.ts +++ b/test/cli/hot/hot.test.ts @@ -1,7 +1,17 @@ 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 { + chmodSync, + chownSync, + copyFileSync, + cpSync, + readFileSync, + renameSync, + rmSync, + unlinkSync, + writeFileSync, +} from "fs"; +import { bunEnv, bunExe, isASAN, isDebug, isLinux, isWindows, tempDir, tmpdirSync, waitForFileToExist } from "harness"; import { join } from "path"; const timeout = isDebug ? Infinity : 10_000; @@ -776,3 +786,282 @@ ${Buffer.alloc(counter * 2, " ").toString()}throw new Error(${counter});`, }, 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 +// does not touch the listing there) and on non-ASAN builds: the reload/build +// mix also drives the resolver-side lookup races that #37274 and #34411 fix, +// which segfault this fixture on weakly ordered release lanes (seen on +// ubuntu 25.04 aarch64). ASAN builds are where the watcher-side +// use-after-free this guards against is detectable. +it.skipIf(isWindows || !isASAN)( + "directory events race reload() and Bun.build() rewriting the same cached listing", + async () => { + const files: Record = { + "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(); + 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(); + } + })(); + // Drain stderr from the start so a spewing child (crash report, ASAN + // output) cannot fill the pipe and block instead of exiting. + const stderrDone = proc.stderr.text(); + // 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([stderrDone, 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 ? 300_000 : 60_000, +); + +// A directory event for a directory whose entries cache holds a readdir error +// (EntriesOption::Err, cached by any non-ENOENT failure such as EACCES) used +// to feed the error slot to EntriesOption::entries(), which panics and aborts +// the whole --hot process. Skipped on Windows (the watcher does not touch the +// listing there). +{ + // Root bypasses DAC, so chmod 0 won't yield EACCES. When running as root on + // Linux we drop to `nobody` via runuser (and chown the temp dir so the + // fixture can chmod it back). Otherwise we run the fixture directly. + const isRoot = !isWindows && process.getuid?.() === 0; + const nobody = (() => { + try { + // /etc/passwd format: name:x:uid:gid:gecos:home:shell + const line = readFileSync("/etc/passwd", "utf8") + .split("\n") + .find(l => l.startsWith("nobody:")); + if (!line) return null; + const [, , uid, gid] = line.split(":"); + if (!Number.isInteger(+uid) || !Number.isInteger(+gid)) return null; + return { uid: +uid, gid: +gid }; + } catch { + return null; + } + })(); + const canUseRunuser = isLinux && isRoot && !!Bun.which("runuser") && nobody !== null; + const canTriggerEACCES = !isWindows && (!isRoot || canUseRunuser); + + it.skipIf(!canTriggerEACCES)( + "a directory event for a dir whose cached listing is a read error does not kill the process", + async () => { + using dir = tempDir("hot-direntry-err", { + "main.ts": /* ts */ ` + import "./pages/p1.tsx"; + import { chmodSync } from "fs"; + const g = globalThis as any; + // --hot re-runs this module on reload; keep the first run's stdin hook. + if (!g.__hooked) { + g.__hooked = true; + let buf = ""; + process.stdin.on("data", d => { + buf += d; + let i; + while ((i = buf.indexOf("\\n")) !== -1) { + const line = buf.slice(0, i); + buf = buf.slice(i + 1); + if (line === "err") { + // Make pages/ unreadable and fail a resolve through it: + // readDirectory(pages) hits EACCES, which caches the error + // under the watched directory's cache key. Then restore. + chmodSync(import.meta.dir + "/pages", 0o000); + let threw = false; + try { + Bun.resolveSync("./pages/nope.js", import.meta.dir); + } catch { + threw = true; + } + chmodSync(import.meta.dir + "/pages", 0o755); + console.log("err-cached:" + threw); + } else if (line === "exit") { + process.exit(0); + } + } + }); + } + console.log("ready"); + `, + "pages/p1.tsx": "export default 1;\n", + }); + const root = String(dir); + + let cmd = [bunExe(), "--hot", "main.ts"]; + if (canUseRunuser) { + // Give `nobody` ownership so the fixture's chmodSync calls succeed, and + // open up perms so `nobody` can traverse/read everything it needs. + for (const p of [root, join(root, "main.ts"), join(root, "pages"), join(root, "pages", "p1.tsx")]) { + chmodSync(p, 0o777); + chownSync(p, nobody!.uid, nobody!.gid); + } + cmd = ["runuser", "-u", "nobody", "--", bunExe(), "--hot", "main.ts"]; + } + + try { + await using proc = spawn({ + cmd, + env: bunEnv, + cwd: root, + stdout: "pipe", + stderr: "pipe", + stdin: "pipe", + }); + + let stdout = ""; + const waiters: Array<{ test: (s: string) => boolean; resolve: () => void }> = []; + const poke = () => { + for (let i = waiters.length - 1; i >= 0; i--) { + if (waiters[i].test(stdout)) { + waiters[i].resolve(); + waiters.splice(i, 1); + } + } + }; + const stdoutDone = (async () => { + const decoder = new TextDecoder(); + for await (const chunk of proc.stdout) { + stdout += decoder.decode(chunk, { stream: true }); + poke(); + } + })(); + const stderrDone = proc.stderr.text(); + // A crash resolves every pending wait so the assertions below report + // the death instead of hanging. + const exited = proc.exited.then(code => { + for (const w of waiters.splice(0)) w.resolve(); + return code; + }); + const waitFor = (test: (s: string) => boolean) => + new Promise(resolve => { + if (test(stdout)) return resolve(); + waiters.push({ test, resolve }); + }); + const countReady = (s: string) => s.split("ready\n").length - 1; + + await waitFor(s => s.includes("ready\n")); + proc.stdin.write("err\n"); + await proc.stdin.flush(); + await waitFor(s => s.includes("err-cached:true\n")); + + // The only directory event of the whole test: touching the watched + // page makes the watcher probe the error slot (directory event) and + // reload the fixture (file event). The second "ready" proves the + // watcher thread survived the probe. + writeFileSync(join(root, "pages", "p1.tsx"), "export default 1;\n"); + await waitFor(s => countReady(s) >= 2); + + proc.stdin.write("exit\n"); + await proc.stdin.flush(); + const [stderr, exitCode] = await Promise.all([stderrDone, exited]); + await stdoutDone; + expect({ + stdout, + // Debug builds print a reload notice; release builds print nothing. + stderr: stderr.replaceAll("DEBUG: Reloading...\n", ""), + exitCode, + signalCode: proc.signalCode, + }).toEqual({ + stdout: "ready\nerr-cached:true\nready\n", + stderr: "", + exitCode: 0, + signalCode: null, + }); + } finally { + // Ensure tempDir cleanup can remove the directory even if the fixture + // crashed between the two chmod calls. + try { + chmodSync(join(root, "pages"), 0o755); + } catch {} + } + }, + isDebug ? 120_000 : 30_000, + ); +}