diff --git a/src/jsc/hot_reloader.rs b/src/jsc/hot_reloader.rs index 5650ac5b85ff..9f96c7943c03 100644 --- a/src/jsc/hot_reloader.rs +++ b/src/jsc/hot_reloader.rs @@ -496,6 +496,11 @@ where } pub(crate) fn append(&mut self, id: u32) { + // A replaced file is reported by its directory and by itself. + if self.hashes[..self.count as usize].contains(&id) { + return; + } + if self.count == 8 { self.enqueue(); self.count = 0; diff --git a/src/watcher/INotifyWatcher.rs b/src/watcher/INotifyWatcher.rs index 23637c21ca76..a2f4d2d5360e 100644 --- a/src/watcher/INotifyWatcher.rs +++ b/src/watcher/INotifyWatcher.rs @@ -5,7 +5,7 @@ use core::ffi::c_int; use core::mem::{align_of, size_of}; use core::sync::atomic::{AtomicU32, Ordering}; -use bun_core::{ZStr, env_var, output as Output}; +use bun_core::{ZStr, env_var, output as Output, strings}; use bun_paths::MAX_PATH_BYTES; use bun_sys::{self, Fd}; use bun_threading::Futex; @@ -74,6 +74,12 @@ impl Default for INotifyWatcher { pub(crate) type EventListIndex = c_int; +/// IN_ATTRIB reports the link-count change of an inode that a rename replaces: see `file_was_replaced`. +const WATCH_FILE_MASK: u32 = { + use bun_sys::linux::IN; + IN::EXCL_UNLINK | IN::MOVE_SELF | IN::DELETE_SELF | IN::MOVED_TO | IN::MODIFY | IN::ATTRIB +}; + #[repr(C)] pub struct Event { pub watch_descriptor: EventListIndex, @@ -127,14 +133,11 @@ impl Event { impl INotifyWatcher { pub(crate) fn watch_path(&mut self, pathname: &ZStr) -> bun_sys::Result { - use bun_sys::linux::IN; debug_assert!(self.loaded); let old_count = self.watch_count.fetch_add(1, Ordering::Release); - let watch_file_mask = - IN::EXCL_UNLINK | IN::MOVE_SELF | IN::DELETE_SELF | IN::MOVED_TO | IN::MODIFY; // SAFETY: fd is a valid inotify fd (loaded == true), pathname is NUL-terminated. let rc = unsafe { - bun_sys::linux::inotify_add_watch(self.fd.native(), pathname.as_ptr(), watch_file_mask) + bun_sys::linux::inotify_add_watch(self.fd.native(), pathname.as_ptr(), WATCH_FILE_MASK) }; bun_core::scoped_log!(watcher, "inotify_add_watch({}) = {}", self.fd, rc); let result = if rc < 0 { @@ -451,7 +454,16 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { continue; } }; - this.watch_events[event_id] = watch_event_from_inotify_event(event, idx); + let mut watch_event = watch_event_from_inotify_event(event, idx); + if (event.mask & bun_sys::linux::IN::ATTRIB) != 0 { + if file_was_replaced(this, idx, event.watch_descriptor) { + watch_event.op |= Op::DELETE; + } else if watch_event.op.is_empty() { + events_processed += 1; + continue; + } + } + this.watch_events[event_id] = watch_event; // Safely handle event names with bounds checking if event.name_len > 0 && (temp_name_off as usize) < temp_name_list.len() { @@ -524,6 +536,38 @@ fn process_inotify_event_batch( Ok(()) } +/// inotify returns another watch descriptor for the path of a watched file once that path names another inode. +fn file_was_replaced(this: &Watcher, index: WatchItemIndex, wd: EventListIndex) -> bool { + use crate::watcher_impl::{WatchItemColumns, WatchItemKind}; + let mut buf = bun_paths::path_buffer_pool::get(); + let path = { + let _guard = this.mutex.lock_guard(); + let i = usize::from(index); + // `index` is from this cycle's snapshot; an eviction since then can have moved another item there. + if this.watchlist.items_eventlist_index().get(i) != Some(&wd) + || this.watchlist.items_kind()[i] != WatchItemKind::File + { + return false; + } + let file_path: &[u8] = &this.watchlist.items_file_path()[i]; + if file_path.len() >= buf.len() || strings::contains(file_path, b"node_modules") { + return false; + } + buf[..file_path.len()].copy_from_slice(file_path); + buf[file_path.len()] = 0; + ZStr::from_buf(&buf[..], file_path.len()) + }; + // SAFETY: the inotify fd stays open until this thread stops it; `path` is NUL-terminated. + let now = unsafe { + bun_sys::linux::inotify_add_watch( + this.platform.fd.native(), + path.as_ptr(), + WATCH_FILE_MASK | bun_sys::linux::IN::MASK_ADD, + ) + }; + now >= 0 && now != wd +} + fn watch_event_from_inotify_event(event: &Event, index: WatchItemIndex) -> WatchEvent { use bun_sys::linux::IN; let mut op = Op::empty(); diff --git a/test/bake/dev/html.test.ts b/test/bake/dev/html.test.ts index f76c1c330789..502af990698a 100644 --- a/test/bake/dev/html.test.ts +++ b/test/bake/dev/html.test.ts @@ -1,4 +1,5 @@ // HTML tests are tests relating to HTML files themselves. +import { renameSync, writeFileSync } from "node:fs"; import { devTest, emptyHtmlFile } from "../bake-harness"; devTest("html file is watched", { @@ -444,3 +445,34 @@ devTest("editing a file imported from outside the project root hot-reloads", { await c.expectMessage("three"); }, }); +devTest("replacing a file imported from outside the project root by rename hot-reloads", { + // This pins the inotify watcher. The Windows watcher does not watch files outside the project directory. + skip: ["win32", "darwin"], + files: { + "web/index.html": emptyHtmlFile({ + scripts: ["index.ts"], + }), + "web/index.ts": ` + import { value } from "../outside/dep"; + console.log(value); + import.meta.hot.accept(); + `, + "outside/dep.ts": ` + export const value = "one"; + `, + }, + cwd: "web", + async test(dev) { + await using c = await dev.client("/"); + await c.expectMessage("one"); + const dep = dev.join("outside/dep.ts"); + for (const value of ["two", "three"]) { + { + await using _batch = await dev.batchChanges(); + writeFileSync(dep + ".next", `export const value = "${value}";`); + renameSync(dep + ".next", dep); + } + await c.expectMessage(value); + } + }, +}); diff --git a/test/cli/hot/watch.test.ts b/test/cli/hot/watch.test.ts index 4a6a5c2908b7..485b1841f71f 100644 --- a/test/cli/hot/watch.test.ts +++ b/test/cli/hot/watch.test.ts @@ -1,7 +1,7 @@ import { spawn } from "bun"; import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, forEachLine, isBroken, isWindows, tempDir } from "harness"; -import { writeFile } from "node:fs/promises"; +import { bunEnv, bunExe, forEachLine, isBroken, isLinux, isWindows, tempDir } from "harness"; +import { chmod, link, mkdir, readFile, rename, rm, symlink, utimes, writeFile } from "node:fs/promises"; import { join } from "node:path"; describe.todoIf(isBroken && isWindows)("--watch works", async () => { @@ -48,3 +48,320 @@ describe.todoIf(isBroken && isWindows)("--watch works", async () => { }); } }); + +// What vim and `sed -i` do: write a temporary file, then rename it over the target. +async function renameSave(path: string, content: string) { + await writeFile(path + ".next", content); + await rename(path + ".next", path); +} + +async function nextEval(iter: AsyncIterator): Promise { + while (true) { + const { value, done } = await iter.next(); + if (done) throw new Error("stream ended before an EVAL line"); + if (value.startsWith("EVAL ")) return value; + } +} + +const counted = (rest: string) => + `globalThis.g = (globalThis.g ?? 0) + 1;\n` + `console.log("EVAL g=" + globalThis.g + " " + ${rest});\n`; +const counterEntry = (specifier: string) => + `import { sh } from ${JSON.stringify(specifier)};\n` + counted(`"shared=" + sh`); + +async function watcherTrace(path: string): Promise { + return (await readFile(path, "utf8")) + .split("\n") + .filter(Boolean) + .flatMap(line => Object.keys(JSON.parse(line).files)); +} + +// A rename over a file replaces its inode. The inotify watch of the file stays +// on the old inode, and the kernel reports nothing more for it while bun holds +// it open. Where no directory event leads back to the file, that save and each +// later save of the file were missed. +describe.skipIf(!isLinux)("a watched file that is replaced by rename is reloaded", () => { + for (const flag of ["--watch", "--hot"] as const) { + const g = (n: number) => (flag === "--hot" ? n : 1); + + test.concurrent(`${flag} a module outside cwd`, async () => { + await using dir = tempDir("watch-outside-cwd", { + "app/entry.ts": counterEntry("../shared/lib.ts"), + "shared/lib.ts": `export const sh = "V0";\n`, + }); + const sharedLib = join(String(dir), "shared", "lib.ts"); + + await using proc = spawn({ + cmd: [bunExe(), flag, "--no-clear-screen", "entry.ts"], + cwd: join(String(dir), "app"), + env: bunEnv, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 shared=V0"); + + await renameSave(sharedLib, `export const sh = "V1";\n`); + expect(await nextEval(out)).toBe(`EVAL g=${g(2)} shared=V1`); + + // The watch has to be on the new inode now. + await renameSave(sharedLib, `export const sh = "V2";\n`); + expect(await nextEval(out)).toBe(`EVAL g=${g(3)} shared=V2`); + }); + + // The real-path directory of the package is inside cwd and is watched. + // The resolver caches it under the `node_modules/lib/` spelling, so its + // directory event finds no watched file. + test.concurrent(`${flag} a workspace package imported by bare name through node_modules`, async () => { + await using dir = tempDir("watch-workspace-bare-import", { + "package.json": JSON.stringify({ name: "root", private: true, workspaces: ["packages/*"] }), + "packages/app/package.json": JSON.stringify({ name: "app", dependencies: { lib: "workspace:*" } }), + "packages/app/entry.ts": counterEntry("lib/index.js"), + "packages/lib/package.json": JSON.stringify({ name: "lib", version: "1.0.0", main: "index.js" }), + "packages/lib/index.js": `export const sh = "V0";\n`, + }); + const root = String(dir); + const libIndex = join(root, "packages", "lib", "index.js"); + await mkdir(join(root, "packages", "app", "node_modules"), { recursive: true }); + await symlink(join("..", "..", "lib"), join(root, "packages", "app", "node_modules", "lib"), "dir"); + + await using proc = spawn({ + cmd: [bunExe(), flag, "--no-clear-screen", "packages/app/entry.ts"], + cwd: root, + env: bunEnv, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 shared=V0"); + + await renameSave(libIndex, `export const sh = "V1";\n`); + expect(await nextEval(out)).toBe(`EVAL g=${g(2)} shared=V1`); + + await renameSave(libIndex, `export const sh = "V2";\n`); + expect(await nextEval(out)).toBe(`EVAL g=${g(3)} shared=V2`); + }); + + test.concurrent(`${flag} the entry point, outside cwd`, async () => { + const entry = (value: string) => counted(JSON.stringify("shared=" + value)); + await using dir = tempDir("watch-entry-outside-cwd", { + "app/.keep": "", + "scripts/entry.ts": entry("V0"), + }); + const entryPath = join(String(dir), "scripts", "entry.ts"); + + await using proc = spawn({ + cmd: [bunExe(), flag, "--no-clear-screen", "../scripts/entry.ts"], + cwd: join(String(dir), "app"), + env: bunEnv, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 shared=V0"); + + await renameSave(entryPath, entry("V1")); + expect(await nextEval(out)).toBe(`EVAL g=${g(2)} shared=V1`); + + await renameSave(entryPath, entry("V2")); + expect(await nextEval(out)).toBe(`EVAL g=${g(3)} shared=V2`); + }); + } + + // With a second hard link the replaced inode is neither removed nor + // without a link, so only what its path names now tells that it was replaced. + for (const hardLink of [false, true]) { + const suffix = hardLink ? " and has a second hard link" : ""; + + test.concurrent(`--hot a module that is behind a directory symlink${suffix}`, async () => { + await using dir = tempDir("watch-symlink-outside-cwd", { + "app/entry.ts": counterEntry("./link/dep.ts"), + "realdir/dep.ts": `export const sh = "V0";\n`, + }); + const appDir = join(String(dir), "app"); + const realDep = join(String(dir), "realdir", "dep.ts"); + await symlink(join(String(dir), "realdir"), join(appDir, "link"), "dir"); + if (hardLink) await link(realDep, realDep + ".hardlink"); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", "entry.ts"], + cwd: appDir, + env: bunEnv, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 shared=V0"); + + await renameSave(realDep, `export const sh = "V1";\n`); + expect(await nextEval(out)).toBe("EVAL g=2 shared=V1"); + + await renameSave(realDep, `export const sh = "V2";\n`); + expect(await nextEval(out)).toBe("EVAL g=3 shared=V2"); + }); + } + + test.concurrent("--hot a module outside cwd that has a second hard link", async () => { + await using dir = tempDir("watch-outside-cwd-hardlink", { + "app/entry.ts": counterEntry("../shared/lib.ts"), + "shared/lib.ts": `export const sh = "V0";\n`, + }); + const sharedLib = join(String(dir), "shared", "lib.ts"); + await link(sharedLib, sharedLib + ".hardlink"); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", "entry.ts"], + cwd: join(String(dir), "app"), + env: bunEnv, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 shared=V0"); + + await renameSave(sharedLib, `export const sh = "V1";\n`); + expect(await nextEval(out)).toBe("EVAL g=2 shared=V1"); + }); + + // Inside cwd the directory reports the file too. Both reports must count as one. + test.concurrent("--hot evaluates once for three modules that are replaced in one batch", async () => { + const dep = (name: string, value: string) => `export const ${name} = "${value}";\n`; + await using dir = tempDir("hot-replace-batch", { + "entry.ts": + `import { a } from "./a.ts";\nimport { b } from "./b.ts";\nimport { c } from "./c.ts";\n` + + counted("a + b + c"), + "a.ts": dep("a", "a0"), + "b.ts": dep("b", "b0"), + "c.ts": dep("c", "c0"), + }); + const path = (name: string) => join(String(dir), name + ".ts"); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", "entry.ts"], + cwd: String(dir), + env: bunEnv, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 a0b0c0"); + + // The stopped process finds the three saves waiting and reads them as one batch. + process.kill(proc.pid, "SIGSTOP"); + try { + await renameSave(path("a"), dep("a", "a1")); + await renameSave(path("b"), dep("b", "b1")); + await renameSave(path("c"), dep("c", "c1")); + } finally { + process.kill(proc.pid, "SIGCONT"); + } + expect(await nextEval(out)).toBe("EVAL g=2 a1b1c1"); + + // A second evaluation for that batch would show here as g=3 with c1. + await renameSave(path("c"), dep("c", "c2")); + expect(await nextEval(out)).toBe("EVAL g=3 a1b1c2"); + }); +}); + +// These pass without the check for a replaced file. They pin what it must not change. +describe.skipIf(!isLinux)("inotify file watch", () => { + // The inode only gets another name and then its name back. Its watch must + // survive the first move, or nothing reports the second. + test.concurrent("--hot reloads when a dependency is moved away and then back", async () => { + await using dir = tempDir("hot-move-away-and-back", { + "entry.ts": counterEntry("./dep.ts"), + "dep.ts": `export const sh = "V0";\n`, + }); + const dep = join(String(dir), "dep.ts"); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", "entry.ts"], + cwd: String(dir), + env: bunEnv, + stdio: ["ignore", "pipe", "pipe"], + }); + const out = forEachLine(proc.stdout); + const err = forEachLine(proc.stderr); + + expect(await nextEval(out)).toBe("EVAL g=1 shared=V0"); + + await rename(dep, dep + ".away"); + // The reload for the first move fails. Wait for it, so that the move back + // is handled on its own. + while (true) { + const { value, done } = await err.next(); + if (done) throw new Error("stderr ended before the reload without dep.ts failed"); + if (value.includes("Cannot find module")) break; + } + + await rename(dep + ".away", dep); + expect(await nextEval(out)).toBe("EVAL g=2 shared=V0"); + }); + + // The watcher handles events in order. In both cases the reload of the saved + // entry shows that it has handled the change before it. + test.concurrent("a metadata-only change of a watched file is not reported", async () => { + await using dir = tempDir("watch-metadata-only", { + "app/entry.ts": `import "./dep.ts";\n` + counted(`"entry"`), + "app/dep.ts": `export const x = 1;\n`, + "trace/.keep": "", + }); + const appDir = join(String(dir), "app"); + const dep = join(appDir, "dep.ts"); + const entry = join(appDir, "entry.ts"); + const trace = join(String(dir), "trace", "events.jsonl"); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", "entry.ts"], + cwd: appDir, + env: { ...bunEnv, BUN_WATCHER_TRACE: trace }, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 entry"); + + await chmod(dep, 0o600); + await chmod(dep, 0o644); + await utimes(dep, new Date(), new Date()); + await renameSave(entry, (await readFile(entry, "utf8")) + "// saved\n"); + expect(await nextEval(out)).toBe("EVAL g=2 entry"); + + proc.kill("SIGKILL"); + await proc.exited; + const reported = await watcherTrace(trace); + expect(reported.some(path => path.endsWith("/app/"))).toBe(true); + expect(reported.filter(path => path.endsWith("/dep.ts"))).toEqual([]); + }); + + // A removed module is not a replaced one. A program that removes a fixture + // it imported from a temporary directory must not start again for that. + test.concurrent("a removed file outside cwd is not reported", async () => { + await using dir = tempDir("watch-removed-outside-cwd", { + "app/entry.ts": counterEntry("../shared/lib.ts"), + "shared/lib.ts": `export const sh = "V0";\n`, + "trace/.keep": "", + }); + const appDir = join(String(dir), "app"); + const trace = join(String(dir), "trace", "events.jsonl"); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", "entry.ts"], + cwd: appDir, + env: { ...bunEnv, BUN_WATCHER_TRACE: trace }, + stdio: ["ignore", "pipe", "inherit"], + }); + const out = forEachLine(proc.stdout); + + expect(await nextEval(out)).toBe("EVAL g=1 shared=V0"); + + await rm(join(String(dir), "shared", "lib.ts")); + await renameSave(join(appDir, "entry.ts"), counted(`"without the import"`)); + expect(await nextEval(out)).toBe("EVAL g=2 without the import"); + + proc.kill("SIGKILL"); + await proc.exited; + const reported = await watcherTrace(trace); + expect(reported.some(path => path.endsWith("/app/"))).toBe(true); + expect(reported.filter(path => path.endsWith("/lib.ts"))).toEqual([]); + }); +});