diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index d7b1e5955901..d137343d57a4 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -6843,19 +6843,24 @@ pub mod bv2_impl { .text; let loader = this.graph.input_files.items_loader()[source_index as usize]; if this.should_add_watcher(source_path) { - // const generic `CLONE_FILE_PATH = isWindows` - // matches `cfg!(windows)` at compile time. - let _ = this - .bun_watcher_mut() - .unwrap() - .add_file::<{ cfg!(windows) }>( - parse_result.watcher_data.fd, - source_path, - bun_wyhash::hash(source_path) as u32, - bun_watcher::Loader(loader as u8), - parse_result.watcher_data.dir_fd, - None, - ); + // CLONE_FILE_PATH=true: `source_path` is only interned + // process-lifetime in the common case. `dupe_alloc`'s + // disjoint text/pretty branch (imports outside the + // project root, bake's `ssr:`-prefixed pretty paths) + // allocates `path.text` in the per-build arena + // (`graph.heap`), which DevServer frees at the end of + // every build — while the watcher keeps the path for + // the process lifetime and reads it on the watcher + // thread. Borrowing here (Zig passed + // `Environment.isWindows`) dangled those entries. + let _ = this.bun_watcher_mut().unwrap().add_file::( + parse_result.watcher_data.fd, + source_path, + bun_wyhash::hash(source_path) as u32, + bun_watcher::Loader(loader as u8), + parse_result.watcher_data.dir_fd, + None, + ); } } } diff --git a/src/runtime/node/node_fs_stat_watcher.rs b/src/runtime/node/node_fs_stat_watcher.rs index a2c85d1aaa6b..7f6699198b55 100644 --- a/src/runtime/node/node_fs_stat_watcher.rs +++ b/src/runtime/node/node_fs_stat_watcher.rs @@ -1018,11 +1018,29 @@ impl StatWatcher { // (`FileSystem::init` runs before any JS module loads). let top_level_dir = fs::FileSystem::get().top_level_dir; let parts: [&[u8]; 1] = [slice]; - let file_path = - Path::join_abs_string_buf::(top_level_dir, &mut buf[..], &parts); - + // The cwd-joined result can exceed the pooled buffer even when the raw + // path passed per-platform length validation; the unchecked join + // overflowed the buffer (panic) on such inputs. Fall back to a heap + // scratch sized like `join_abs_string_buf_checked`'s slow path: the + // watcher is still created and its stat polls fail with ENAMETOOLONG, + // so the listener observes zeroed stats — matching Node, which never + // throws for un-stat-able `watchFile` paths. // allocSentinel + memcpy → owned NUL-terminated copy (ZBox) - let alloc_file_path = ZBox::from_bytes(file_path); + let alloc_file_path = match Path::join_abs_string_buf_checked::( + top_level_dir, + &mut buf[..], + &parts, + ) { + Some(file_path) => ZBox::from_bytes(file_path), + None => { + let mut scratch = vec![0u8; top_level_dir.len() + slice.len() + 3]; + ZBox::from_bytes(Path::join_abs_string_buf::( + top_level_dir, + &mut scratch, + &parts, + )) + } + }; // errdefer free → Drop handles it // `args.global_this` is a `BackRef` (JSC_BORROW); safe Deref. diff --git a/src/watcher/KEventWatcher.rs b/src/watcher/KEventWatcher.rs index 719a91d49b4d..d4ca433b66c7 100644 --- a/src/watcher/KEventWatcher.rs +++ b/src/watcher/KEventWatcher.rs @@ -1,5 +1,3 @@ -use core::ffi::c_int; - use bun_core::output as Output; use bun_sys::Fd; @@ -58,7 +56,10 @@ pub(crate) fn watch_event_from_kevent(kevent: &libc::kevent) -> WatchEvent { } pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { - use bun_sys::c; + // Zig: `defer Output.flush()` — flushes on the `?` early returns below + // too (mirrors INotifyWatcher's watch_loop_cycle). + let _flush = Output::flush_guard(); + let fd: Fd = this .platform .fd @@ -69,42 +70,26 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { let mut changelist_array: [libc::kevent; CHANGELIST_COUNT] = bun_core::ffi::zeroed(); let changelist = &mut changelist_array; - // SAFETY: fd is a valid kqueue fd; changelist points to CHANGELIST_COUNT zeroed entries - let mut count: c_int = unsafe { - c::kevent( - fd.native(), - changelist.as_ptr(), - 0, - changelist.as_mut_ptr(), - CHANGELIST_COUNT as c_int, - core::ptr::null(), // timeout - ) - }; + // kevent(2) returns -1 on failure (e.g. EINTR). Zig called the raw syscall + // and let a negative count fall through `@max(0, count)` into an empty + // batch; the direct port's checked `usize::try_from(count)` panicked on + // that -1 instead. Use the EINTR-retrying `bun_sys::kevent` wrapper and + // propagate real errors to the watch loop. + let mut count: usize = bun_sys::kevent(fd, &[], changelist, None)?; // Give the events more time to coalesce - if count < 128 / 2 { - let remain: c_int = 128 - count; - let off = usize::try_from(count).expect("int cast"); + if count < CHANGELIST_COUNT / 2 { let ts = libc::timespec { tv_sec: 0, tv_nsec: 100_000, }; // 0.0001 seconds - // SAFETY: off < CHANGELIST_COUNT (count < 64), remain entries fit in the buffer - let extra: c_int = unsafe { - c::kevent( - fd.native(), - changelist.as_ptr().add(off), - 0, - changelist.as_mut_ptr().add(off), - remain, - &raw const ts, - ) - }; - - count += extra; + // Best-effort coalescing read: deliver the events we already have even + // if this extra poll fails; a persistent kqueue error resurfaces on the + // next cycle's blocking call. + count += bun_sys::kevent(fd, &[], &mut changelist[count..], Some(&ts)).unwrap_or(0); } - let changes_len = usize::try_from(count.max(0)).expect("int cast"); + let changes_len = count; let changes = &changelist[0..changes_len]; // Track out_len and slice once at the end to avoid overlapping &mut // borrows of `this`. @@ -140,8 +125,5 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { (this.on_file_update)(this.ctx, &mut deduped, changed, &this.watchlist); } - // No early returns above, so flush once at the single exit point instead - // of via scopeguard. - Output::flush(); Ok(()) } diff --git a/src/watcher/WindowsWatcher.rs b/src/watcher/WindowsWatcher.rs index 31f965792882..c8986cc75551 100644 --- a/src/watcher/WindowsWatcher.rs +++ b/src/watcher/WindowsWatcher.rs @@ -410,6 +410,19 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { // NOTE: using a 1ms timeout would be ideal, but that actually makes the thread wait for at least 10ms more than it should // Instead we use a 0ms timeout, which may not do as much coalescing but is more responsive. timeout = Timeout::None; + // PORT NOTE: locked — diverges from Zig spec (which scanned the + // `file_path` column unlocked; `WindowsWatcher.zig:217`). The JS + // thread, transpiler workers and the bundle thread append watch items + // under `this.mutex`, which can grow the MultiArrayList — freeing the + // old backing slab mid-scan — so the unlocked `items_file_path()` read + // raced the realloc and `is_parent_or_equal` dereferenced freed + // memory. Mirrors the locked snapshot in INotifyWatcher's + // `watch_loop_cycle`. The guard is dropped and re-acquired around + // `process_watch_event_batch`, which takes the same (non-recursive) + // mutex internally. This never holds the lock across a blocking wait: + // `platform.next()` runs before the guard is taken and `iter.next()` + // only parses the already-filled completion buffer. + let mut guard = this.mutex.lock_guard(); bun_core::scoped_log!( watcher, "number of watched items: {}", @@ -439,8 +452,18 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { // to implement and maintain. // - others that i'm not thinking of - let n_items = this.watchlist.items_file_path().len(); - for item_idx in 0..n_items { + // The length is re-read every iteration: releasing the lock around + // `process_watch_event_batch` lets `on_file_update` evict entries + // and compact the watchlist. Known trade-off: that compaction is + // `swap_remove`-based, so a mid-batch flush can move a not-yet- + // scanned tail entry into a slot behind `item_idx`, skipping it + // for the *current* OS event (requires 128+ matches for one event + // plus a concurrent eviction below the cursor). Rescanning from 0 + // instead would risk duplicate notifications; a missed coalesced + // event is the safer failure mode, and the next event for that + // path re-delivers. + let mut item_idx: usize = 0; + while item_idx < this.watchlist.items_file_path().len() { // reshaped for borrowck — `rel` is computed in a scoped // block so the borrows of `this.watchlist` / `this.platform.buf` // are released before we touch `this.watch_events` or hand the @@ -463,13 +486,16 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { }; // skip unrelated items if rel == ParentEqual::Unrelated { + item_idx += 1; continue; } // if the event is for a parent dir of the item, only emit it if it's a delete or rename // Check if we're about to exceed the watch_events array capacity if event_id >= this.watch_events.len() { - // Process current batch of events + // Process current batch of events; it locks `this.mutex` + // itself, so release our guard first (non-recursive mutex). + drop(guard); process_watch_event_batch(this, event_id)?; // passing `this: &mut Watcher` above materialises a fresh Unique // borrow over the whole `Watcher`, which under Stacked Borrows pops the @@ -481,13 +507,17 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> { iter.watcher = BackRef::new(&this.platform.watcher); // Reset event_id to start a new batch event_id = 0; + guard = this.mutex.lock_guard(); + continue; } this.watch_events[event_id] = create_watch_event(&event, item_idx as WatchItemIndex); event_id += 1; + item_idx += 1; } } + drop(guard); } // Process any remaining events in the final batch diff --git a/test/bake/bake-harness.ts b/test/bake/bake-harness.ts index 21074a328557..448299d616f8 100644 --- a/test/bake/bake-harness.ts +++ b/test/bake/bake-harness.ts @@ -136,6 +136,11 @@ export interface DevServerTest { * Avoid if possible, this is to reproduce specific bugs. */ mainDir?: string; + /** + * Extra environment variables for the dev server process. + * Avoid if possible, this is to reproduce specific bugs. + */ + env?: Record; skip?: ("win32" | "darwin" | "linux" | "ci")[]; /** @@ -1971,6 +1976,7 @@ function testImpl( // BUN_DEBUG_INCREMENTALGRAPH: isDebugBuild && interactive ? "1" : undefined, // BUN_DEBUG_WATCHER: isDebugBuild && interactive ? "1" : undefined, BUN_ASSUME_PERFECT_INCREMENTAL: "0", + ...options.env, }, ]), stdio: ["pipe", "pipe", "pipe"], diff --git a/test/bake/dev/bundle.test.ts b/test/bake/dev/bundle.test.ts index 6c6d3657029c..b1b72a80d4f7 100644 --- a/test/bake/dev/bundle.test.ts +++ b/test/bake/dev/bundle.test.ts @@ -865,3 +865,51 @@ devTest("barrel optimization: namespace re-export cycle through a star-exported await c.expectMessage("result: object Y KEEP DEEP OTHER"); }, }); + +// The watcher entry for a file whose "pretty" path is disjoint from its +// absolute path (here: an import from outside the project root, whose pretty +// path starts with "../") used to borrow path bytes out of the per-build +// bundle arena instead of cloning them. DevServer frees that arena when the +// build finishes, so a later watch event for the file made the watcher thread +// read freed memory instead of hot-reloading. +// +// The rebuild loop frees a few more per-build arenas before the outside-root +// file is touched, and MIMALLOC_PURGE_DELAY=0 makes mimalloc hand those freed +// pages straight back to the OS instead of parking them in its segment cache +// (which is what let the stale read silently return the right bytes). Together +// they turn the dangling read into the SEGV seen in production. +devTest("hot reload of a file imported from outside the project root", { + // The Windows watcher is a single recursive ReadDirectoryChangesW rooted at + // the project root and refuses outside-root paths outright ("will not be + // watched"), so the dev.write below would never produce a watch event there + // (and the dangling-borrow bug was POSIX-only: Windows always cloned). + skip: ["win32"], + env: { MIMALLOC_PURGE_DELAY: "0", MIMALLOC_ABANDONED_PAGE_PURGE: "1" }, + files: { + "../outside-root-dep/db.ts": `export const abc = "123";`, + "routes/index.ts": ` + import { abc } from '../../outside-root-dep/db'; + export default function (req, meta) { + return new Response('Hello, ' + abc + '!'); + } + `, + }, + framework: minimalFramework, + async test(dev) { + await dev.fetch("/").equals("Hello, 123!"); + for (let i = 0; i < 4; i++) { + await dev.write( + "routes/index.ts", + ` + import { abc } from '../../outside-root-dep/db'; + export default function (req, meta) { + return new Response('Hello v${i}, ' + abc + '!'); + } + `, + ); + await dev.fetch("/").equals(`Hello v${i}, 123!`); + } + await dev.write("../outside-root-dep/db.ts", `export const abc = "456";`); + await dev.fetch("/").equals("Hello v3, 456!"); + }, +});