fs.watch: report watcher queue overflow as ('change', null)

[dylan-conway]

Summary

fs.watch on Linux silently dropped the kernel's queue-overflow signal: once more than fs.inotify.max_queued_events (default 16384) events pile up, the kernel discards events and queues a single IN_Q_OVERFLOW record with wd == -1. The reader thread's wd_map lookup found no watch for wd == -1, so consumers got no signal that events were lost. Windows had the same gap: libuv reports a ReadDirectoryChangesW overflow by invoking the callback with a NULL filename (uv__fs_event_process, src/win/fs-event.c:562), which uv_event_callback discarded with an early return.

Both backends now deliver the loss signal the way node does on Windows: a 'change' event with a null filename, sent to every live watcher (the inotify queue is shared by all watchers on the fd). Node on Linux drops the overflow (libuv cannot map wd == -1 to a handle), so on Linux this is deliberately better than node rather than node parity.

• New Event::NoFilename(EventType) variant flows through both task pipelines and reaches JS as (eventType, null) for every encoding, including 'buffer'.
• On Windows a NULL filename is not exclusively overflow: libuv also passes NULL with UV_RENAME when a filename fails UTF-16 to UTF-8 conversion (the conversion result is unchecked in uv__fs_event_process). The branch keys the event type off events & UV_RENAME, so that case now surfaces as ('rename', null) exactly like node, instead of being dropped.
• Overflow delivery bypasses per-handler duplicate suppression, since a loss signal must always be delivered.

Supersedes #33109, which is the Windows-only half of the same fix; its Windows overflow test is folded in here.

Deliberately excluded:

• src/watcher/INotifyWatcher.rs (the bundler / --watch / --hot watcher) still drops IN_Q_OVERFLOW; the right response there is a rescan or reload decision in the hot reloader, not a JS event, so it needs its own change.
• macOS FSEvents kFSEventStreamEventFlagKernelDropped / UserDropped are not surfaced. libuv and node do not surface them either.

Test plan

• New Linux test in test/js/node/watch/fs.watch.test.ts triggers a real IN_Q_OVERFLOW deterministically without root: closing a recursive watcher over max_queued_events + 6144 directories unregisters one watch per directory inside a single reader-lock critical section, and each inotify_rm_watch queues an IN_IGNORED the blocked reader cannot drain (it gets at most one 64KB read, 4096 events). Asserts ('change', null) on a default-encoding watcher and an encoding: 'buffer' watcher, then that the watcher still delivers normal events afterwards. Self-skips when /proc/sys/fs/inotify limits make the setup impractical.
• The Linux test fails without the fix (the overflow event is never delivered, so it times out) and passes with it.
• New Windows test (from fs.watch: emit null-filename change on Windows watcher overflow #33109): 100 synchronous writes of ~100-char names while the event loop is blocked overflow libuv's 4KB ReadDirectoryChangesW buffer, which libuv reports by calling back with a NULL filename. Asserts a null-filename 'change' reaches both a utf8 and a buffer watcher, or that every write was observed.
• Full test/js/node/watch/ suite (56 pass, 0 fail) and all 33 test/js/node/test/parallel/test-fs-watch*.js pass under a debug ASAN build.
• All four build-rust lanes (linux-x64, windows-x64, windows-x64-baseline, windows-aarch64) pass on this diff, covering the #[cfg(windows)] path.
• The Windows UTF-16 to UTF-8 conversion-failure path (('rename', null)) has no test: a filename libuv cannot convert is not creatable from JS. It compiles for both Windows targets and shares the delivery code the overflow test exercises.

Two test changes beyond the new coverage:

• The Linux overflow test carries an explicit 90s budget. Its directory count is dictated by the kernel constant (overflowing needs more watch removals than max_queued_events plus one 64KB read), so its ~22k syscalls of setup cannot fit the 5s default under a debug ASAN build (6s measured). A cpSync-based setup was measured 6x slower, so the flat mkdirSync loop is already the cheapest form.
• The two pre-existing EACCES permission tests in the same file now skip when running as root: root has CAP_DAC_OVERRIDE, so the chmod 0o200 they rely on never produces the error they assert. They fail identically on an unpatched build, so this is environmental, not a regression.

[robobun]

^{Updated 7:02 PM PT - Jun 29th, 2026}

❌ @robobun, your commit d8e06d1 has 1 failures in Build #67023 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 33110

That installs a local version of the PR into your bun-33110 executable, so you can run:

bun-33110 --bun

[dylan-conway]

@robobun adopt

[robobun]

Adopted and ready. The Linux overflow test fails without the fix (it times out) and passes with it under a debug ASAN build; the full test/js/node/watch/ suite plus all 33 node fs-watch parallel tests are green, and the Windows overflow test from #33109 is folded in, so this supersedes #33109.

CI (build 67023) is red only on lanes this diff cannot affect: two darwin-26-aarch64 jobs where buildkite-agent artifact download timed out after 120s before any test ran, test-net-connect-memleak.js on both alpine lanes (a GC-timing net test that failed identically on the pre-adoption commit, whose net code is byte-for-byte the same as main), and a bun-install.test.ts run that CI itself labeled flaky. 282 lanes passed including every fs.watch lane and all four cross-target rust build lanes. The diff is ready; it needs a maintainer to merge over the unrelated red.

[github-actions]

This PR may be a duplicate of:

1. fs.watch: emit null-filename change on Windows watcher overflow #33109 - Also emits ('change', null) on Windows ReadDirectoryChangesW overflow by adding NoFilename variant and bypassing dedup, with the same structural changes to win_watcher.rs and node_fs_watcher.rs

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6f6edc4a-29a0-4169-baae-95625e1ab976

📥 Commits

Reviewing files that changed from the base of the PR and between e8ed4fe and d8e06d1.

📒 Files selected for processing (1)

• test/js/node/watch/fs.watch.test.ts

---

Walkthrough

Adds Event::NoFilename(EventType) for filesystem events without a filename, routes Linux inotify overflow and Windows null-filename callbacks through it, and updates fs.watch tests for overflow behavior and platform-specific skips.

Changes

Null-filename fs.watch event support

Layer / File(s)	Summary
Event variant and emit helper src/runtime/node/node_fs_watcher.rs	Adds Event::NoFilename(path_watcher::EventType), updates Event::dupe(), and adds FSWatcher::emit_null_filename() to emit JS events with a null filename.
POSIX and Windows dispatch src/runtime/node/node_fs_watcher.rs, src/runtime/node/win_watcher.rs	Extends FSWatchTaskPosix::run and FSWatchTaskWindows::run with NoFilename handling. win_watcher re-exports EventType as pub(crate) and changes uv_event_callback to emit null-filename events instead of dropping them.
Linux inotify overflow handling src/sys/lib.rs, src/runtime/node/path_watcher.rs	Adds linux::IN::Q_OVERFLOW, introduces PathWatcher::emit_overflow(), and updates the inotify reader loop to detect overflow and notify registered watchers.
fs.watch overflow tests test/js/node/watch/fs.watch.test.ts	Tightens Linux permission-denied skips for root, adds a Linux overflow test that expects ("change", null) and recovery, and adds a Windows subprocess overflow test that accepts either full delivery or null-filename overflow notification.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: surfacing fs.watch queue overflow as a null-filename change event.
Description check	✅ Passed	The description is detailed and includes both the change summary and verification steps, covering the template's required information.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/node/win_watcher.rs`:
- Around line 245-256: The Windows-specific null-filename branch in the watcher
logic is currently untested, so add a regression test that exercises the
`filename === null` path in `win_watcher.rs` around the `Event::NoFilename`
handling. Use the existing watcher/update flow symbols such as
`on_path_update_fn`, `on_update_end_fn`, and the `handlers` dispatch to verify
the callback path is triggered on Windows when libuv returns a null filename.
Mirror the Linux overflow test structure, but make it Windows-only and assert
the null-name case is covered.

In `@test/js/node/watch/fs.watch.test.ts`:
- Around line 625-637: The pending promises in this fs.watch test are only
rejected on "error", so a close or other watcher failure can leave overflow,
bufferOverflow, or survived hanging until timeout. Update the watcher setup in
fs.watch.test.ts so the main watcher and bufferWatcher both reject every pending
resolver on all failure paths, and ensure the awaited promises are wired to the
watcher’s failure/close lifecycle rather than only the null-filename or f.txt
callbacks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e98514e7-43e0-4c9e-824f-9abc3e2fbe54

📥 Commits

Reviewing files that changed from the base of the PR and between b2aa0d5 and e8ed4fe.

📒 Files selected for processing (5)

• src/runtime/node/node_fs_watcher.rs
• src/runtime/node/path_watcher.rs
• src/runtime/node/win_watcher.rs
• src/sys/lib.rs
• test/js/node/watch/fs.watch.test.ts

[robobun]

On the duplicate flag: #33109 and this PR are two takes on the same bug, opened a minute apart by the same author, and both rewrite the same block in win_watcher.rs, so only one can merge. This one also handles the Linux inotify IN_Q_OVERFLOW case (the part #33109 does not touch), via a single Event::NoFilename variant shared by both backends. The one thing #33109 has that this PR lacks is a Windows overflow test, which CodeRabbit also asked for here, so I am folding that test into this branch to make it a strict superset. #33109 can then be closed in favor of this one; leaving that call to a maintainer.

[claude]

I didn't find any issues, but this touches unsafe cross-thread watcher dispatch on two platforms, deliberately diverges from Node's Linux behavior, and the Windows null-filename path has no automated coverage — worth a human pass.

Extended reasoning...

Overview

This PR surfaces OS watcher queue-overflow signals to fs.watch consumers as ('change', null) instead of silently dropping them. It adds a new Event::NoFilename(EventType) variant threaded through both the POSIX and Windows task pipelines, an IN_Q_OVERFLOW branch in the Linux inotify reader thread that fans out to every live PathWatcher, and reworks the Windows libuv callback so a NULL filename is forwarded (as change for overflow or rename for UTF-8 conversion failure) rather than discarded. A new IN_Q_OVERFLOW constant is exposed in src/sys/lib.rs, win_watcher.rs re-exports EventType as pub(crate) so the shared path_watcher::EventType path resolves on Windows, and a deterministic Linux test is added.

Security risks

None apparent. There is no new untrusted input parsing — the only new data is a kernel-supplied flag bit and a NULL pointer check. The fan-out iterates manager.watchers under manager.mutex exactly like the existing fatal-error path does, and the Windows branch mirrors the adjacent error-path handler iteration with the same emit_in_progress / maybe_deinit guard pattern.

Level of scrutiny

Medium-high. The change is conceptually small but lives in hand-managed unsafe Rust: raw *mut PathWatcher derefs under a mutex on the inotify reader thread, a re-export visibility change to make the Windows alias compile, and a control-flow restructure of uv_event_callback that hoists the event_type computation and replaces the let-else return with an explicit null branch. All of this looks correct and follows the patterns already in the file (the new overflow fan-out is structurally identical to the existing emit_error fan-out a few lines below), but it is exactly the kind of code where a second pair of eyes on the lock/lifetime invariants and the emit_in_progress ordering on Windows is valuable.

Other factors

• The PR makes an explicit design decision to diverge from Node on Linux (Node/libuv drops IN_Q_OVERFLOW there; this PR delivers it). That's a reasonable product call but is the kind of behavioral choice a maintainer should sign off on.
• The Windows code path (both overflow and the UV_RENAME-with-NULL conversion-failure case) has no automated test, only cargo check across targets — acknowledged in the description.
• The Linux test is clever (overflows the queue by mass-inotify_rm_watch under the reader lock) and self-skips when host sysctls make it impractical, which seems sound but adds a moderately heavy (~22k mkdirs) test that someone familiar with the CI fleet may want to glance at.
• No prior reviewer comments and no bugs surfaced by the bug-hunting pass.

[robobun]

Pushed d8e06d1, which folds in the Windows ReadDirectoryChangesW overflow test from #33109. That was the one gap claude[bot] and CodeRabbit both flagged, so this PR is now a strict superset of #33109. It also fixes CodeRabbit's other finding (a watcher error or unexpected close now rejects every pending promise in the Linux test) and carries two small drive-bys called out in the updated PR body: an explicit 90s budget on the overflow test, whose directory count is fixed by the kernel's max_queued_events, and a root skip on the two pre-existing EACCES permission tests, which were already failing in a root container on an unpatched build. Full watch suite and all 33 node fs-watch parallel tests pass under a debug ASAN build.