Repository navigation
fs.watch: deliver every event, and merge a record only into an event the listener has not received - #44008
Conversation
The watcher thread posts up to 8 events as one task. The task called the listener for each event back to back, so what the listener queued for one event (process.nextTick, promise reactions, the continuation of an await) ran only after the whole batch. Node makes one callback per event. FSWatchTaskPosix::run now runs the microtask checkpoint before every event after the first. The task queue runs it after the last event.
…pin keeps the order The checkpoint between two events can run a continuation that spins the event loop (expect().resolves in bun:test, Bun.build with an async plugin). A later batch of the same watcher then ran inside that spin and its events reached the listener before the rest of the batch that was being delivered. Each batch now moves its events to a queue on the FSWatcher and delivers from the front of that queue, so a batch that runs inside the spin delivers the older events first. close() empties the queue.
The loop that delivered the watcher queue ran the microtask checkpoint itself and held the rest of the queue while it did. A listener or a continuation that spun the event loop in there could wait for an event that only that suspended loop would deliver. Each task now delivers one event and, if more events wait, queues a task for the rest before it calls the listener. The event loop runs the checkpoint between two events, as it does between any two tasks. A spin inside a listener or a continuation runs the queued task, so the events keep coming, in order. Two watchers of one directory now get each event in turn, as in node. Tests: the spin test covers a spin in the listener and in a continuation and observes the checkpoint, so both cases fail without the fix. Add a test for two watchers of one directory.
The watcher thread posts one batch per watcher. Two watchers of one directory get each event in turn only when both batches reach the JS thread in the same drain, so the exact interleaving is not a guarantee and the test could fail under load.
The continuation of the first event of a rename spins the event loop until the listener has seen the second name. Only the task queued for the rest of the batch can deliver it while the continuation spins. With the events held by a suspended delivery loop the spin never returned.
|
Updated 7:04 PM PT - Sep 28th, 2026
✅ @robobun, your commit 1e8fea81624b6bee4edf799b702714093acce245 passed in 🧪 To try this PR locally: bunx bun-pr 44008That installs a local version of the PR into your bun-44008 --bun |
|
Status: open, waits for CI, review and a maintainer's decision (two questions in the last comment). The base is the branch of #44007, and this branch holds the commits of #43715. Both are merged with How to reproduce (Linux, const fs = require("fs"), path = require("path"), os = require("os");
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "w-"));
fs.mkdirSync(path.join(dir, "views"));
const events = [];
const w = fs.watch(dir, (e, f) => {
if (f === "sentinel") { console.log(JSON.stringify(events)); return w.close(); }
events.push(`${e}:${f}`);
});
fs.rmdirSync(path.join(dir, "views"));
fs.mkdirSync(path.join(dir, "views"));
fs.mkdirSync(path.join(dir, "sentinel"));
|
016aa23 to
0f59aea
Compare
…er has not received
The fs.watch backends for Linux, macOS and FreeBSD dropped an event when
the previous event for the same handler had the same path and the same
type ('rename' or 'change') and was at most 1 ms old. A removal and a
creation of one name are both 'rename', so a listener received one
event for the two. A listener that handled the removal and waited for
the entry to come back never heard of the creation. An attribute change
and a write are both 'change' and were dropped the same way, and so was
a write that came at most 1 ms after the listener ran.
ChangeEvent, should_emit and emit_unsuppressed are removed.
The rule also hid that the reader thread reads each record at once. The
kernel merges a record into an identical one only while that one is
unread. Node reads on the loop thread, so the records of one burst stay
unread while JS runs, and merge.
The reader now makes that merge itself. For each handler it keeps the
last record it posted. PathWatcher::emit_record drops a record that is
identical to it while the listener has not received that event.
FSWatcher publishes the number of the newest event of a batch before
the first listener call of the batch, so a listener that runs after a
dropped record sees the change of that record. A record is identical
when the kernel would merge it: same mask, cookie and path for inotify,
same path and kind of event for kqueue.
0f59aea to
9f3bcab
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I checked the new delivered/seq handshake for a lost change: a record is dropped only when the reader's SeqCst load of delivered precedes the JS thread's store in FSWatchTaskPosix::run, and that store precedes the listener call, so every dropped record is followed by a listener run that starts after its change; tasks reach the JS thread in enqueue order, so delivered cannot regress. I also confirmed the remaining emit callers (the Linux IN_IGNORED path and the Darwin FSEvents dispatch) all sit outside the FreeBSD cfg, so the new not(any(windows, freebsd)) gate leaves no dangling call there.
Extended reasoning...
The change replaces the 1 ms per-handler dedup in the POSIX fs.watch backend with a per-handler tail record merged only while the listener has not yet received the previous identical event, threading a seq through the task entries and an AtomicU64 on FSWatcher; it touches no security-sensitive surface. The one inline finding is a stale SAFETY comment on the FreeBSD kqueue path; the concurrency invariant and the cfg gating were examined and hold, but the cross-thread merge logic and the unrun FreeBSD/macOS arms still warrant a human look.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/node/path_watcher.rs— nit: maintainers reading the FreeBSD kqueue reader get a SAFETY comment that names a function this PR removed from that target. path_watcher.rs:1638 still says "Shared access only —emittakes&self", butemitis now cfg'd out on FreeBSD (path_watcher.rs:272) and the call at path_watcher.rs:1664 isemit_record. Fix: re-verify and update the SAFETY text to nameemit_record, matching the sibling comments the PR did update at path_watcher.rs:1001, 1130 and 1207.Why this was flagged
The diff changes the FreeBSD kqueue dispatch at src/runtime/node/path_watcher.rs:1664 from
watcher.emit(...)towatcher.emit_record(record, ...)and gatesemitwith#[cfg(not(any(windows, target_os = "freebsd")))]at src/runtime/node/path_watcher.rs:272, soemitdoes not exist in the FreeBSD build. The SAFETY comment justifying theunsafe { &*entry.watcher }deref at src/runtime/node/path_watcher.rs:1636-1639 still reads "Shared access only —emittakes&self." The PR updated the equivalent inotify-side SAFETY comments (src/runtime/node/path_watcher.rs:1001, 1130, 1207) to nameemit/emit_recordcorrectly but missed this one. No runtime effect; the base branch's comment was accurate becauseemitwas the callee there. A correct fix namesemit_record(which takes&self) so the stated invariant matches the code it covers.Verification: nit. Triggering condition: anyone reading the FreeBSD kqueue reader. Mechanism verified: src/runtime/node/path_watcher.rs:1636-1639 still reads "SAFETY: entry.watcher live under manager.mutex; ... Shared access only —
emittakes&self." while the diff changed the call it guards (path_watcher.rs:1664) fromwatcher.emit(event_type, rel, entry.is_file)to `watcher.emit_record(record,… | nit.…
|
Fixed in a8371c1. The SAFETY comment of the kqueue reader now names |
|
I moved this PR back to draft. A self-review of head a8371c1 found two defects, and a rework is in progress.
The rework replaces the event numbers with one atomic flag, which does not depend on the order of the batches, and it adds a test for each case. |
…ts own record The watcher thread sets `FSWatcher.tail_pending` when it posts an event, and the JS thread clears it before it calls a listener. A record merges into the last event of a listener only while the flag is set. This does not depend on the order in which the batches of one watcher run. The key of a record is the inotify mask and the path, which is what the kernel compares. The cookie is not part of it. The walk of a new directory posts each entry as `Record::Found`, which merges with the entry's own IN_CREATE or IN_MOVED_TO.
|
The rework is pushed (a015f45) and the PR is ready for review again. The description has the new design and the new measurements. @cirospaciari @dylan-conway This PR needs a decision from a maintainer before it can merge. There are three questions.
|
…eliver every event A record merges into an event that the listener has not received. A listener that `once` puts back in a continuation gets the second event of a batch only when the continuation runs between the two events, which #43715 does. `FSWatchTaskPosix::run` clears `tail_pending` where it takes a posted batch over, before `deliver_one` calls a listener.
…iter rejects the round
|
Both findings of the review of a015f45 are addressed in 0c1fcb0. The
I did not take the other way, a merge only into the first event of a batch. The event of the walk of a new directory is never the first of its batch, so the double report of a file in a new directory comes back. The writer (yellow). Done. The test reads the output of the child from the start, and the end of the child rejects the wait of the round with the exit code and stderr in the message. @cirospaciari @dylan-conway This answers question 3 of my earlier comment: the order of #43715 and this PR is free now. Questions 1 and 2 are open. |
|
@robobun wake up!! |
|
@robobun wake up!! |
…ver every event
|
I am here. The state of this PR:
@Jarred-Sumner The PR needs two decisions from a maintainer:
The branch holds the commits of #43715, because the merge is safe only when each event has a task of its own. If #43715 lands first, the diff of this PR gets smaller. |
Five synchronous writes on Darwin currently yield two or three file-watch callbacks because a background kqueue reader consumes events while JavaScript is still writing. Node 24 consumes the vnode event on the owning loop and delivers one callback. Poll Darwin files through the owning JavaScript loop with EVFILT_VNODE/EV_ONESHOT and rearm after delivery, matching libuv. The watcher owns and closes its descriptor; its existing lifetime and ref/unref machinery remain responsible for JavaScript delivery. FSEvents directory watches and the Linux/FreeBSD backends retain their paths. The old Darwin file-reader route is removed. Upstream search considered oven-sh#44008. Its pending-delivery coalescing approach still races with a background reader, so this implementation follows Node 24's deps/uv/src/unix/kqueue.c instead. The two burst regressions cover writes alone and writes followed by unlink, including Node's change-before-rename precedence. Validation: both standalone burst repros and both new tests fail on the verified b368 Darwin release and pass on the AWS-cross-built Darwin candidate. Seven focused watch/GC regressions pass. A standalone lifecycle check passes change, rename-over, unlink, repeated rearm, 500 open/close cycles with no descriptor growth, and unref exit. Scoped-clean P2 Codex branch review. The full Linux watch suite passes (66 passed, 12 skipped), and all 12 cross-target Rust checks pass. Exact-head CI results are linked by the checks below. Local macOS 27's relative-directory watch timeout also reproduces on untouched b368; clean macOS CI supplies directory/recursive coverage. No tests are skipped or weakened here.
Part of #44005
Problem
fs.watch()reports one'rename'when an entry is removed and created again (Node: two). The listener never hears of the creation.ChangeEvent::should_emit(src/runtime/node/path_watcher.rs:226) drops an event when the listener's previous event had the same path and type at most 1 ms before.Fix
ChangeEventandemit_unsuppressedare removed (Linux, macOS, FreeBSD).PathWatcher::emit_recorddrops a record only when it equals the listener's last event and the listener has not received that event. The listener then runs after the change.test/js/node/watch/fs.watch.test.ts, 29 new Linux tests, 20 fail withoutsrc/. macOS and FreeBSD: compiled, not run.Background
FSWatcher.tail_pendingis an atomic flag. The watcher thread sets it with each event. The JS thread clears it before a listener call.oncelistener misses the second event of a batch. This branch holds fs.watch: run nextTicks and microtasks between the events of one batch #43715, so the diff shows its change until it merges.Downsides
main403, Node 17023). Appends andchmodin turn: 200000 and a 4.5 s timer delay (main506, Node 44160).Notes
Stack. #44007 and #43715, then this PR. The base of this PR is the branch of #44007. The branch of #43715 is merged into this branch (bec5bed), so this PR is safe whichever of the two lands first. Both branches are merged with
mainat a4f1429. Until #43715 merges, the diff of this PR showsdeliver_one, the queueundeliveredand five tests that belong to #43715. Without #44007 a recursive watch reports each change to a subdirectory twice once the suppression is gone.Other open PRs.
should_emitin the sharedPathWatcher::emitand calls it from its Windows reader thread. This PR removesshould_emitfrom that function. The two PRs conflict, and a maintainer has to say which rule Windows gets.should_emit. It conflicts with this PR in the same way.cfgofemit_record,Recordand the flag to macOS.Event counts. Linux x64, release builds, 100 runs for each row,
fs.watch(dir), the actions run in one synchronous block. A sentinel entry marks the end, so the rows use no timers.cischange,risrename. Themaincolumn is the base of this PR, which has the rule ofmain.mainrmdir,mkdirmkdir,rmdirunlink, createrename a b,rename b arename a boverbchmod, appendwriteFileSyncchmod, append,unlinkunlinkRows equal to Node in every run:
main3 of 10, this PR 10 of 10.Other measurements. Same builds. The host had a load average of 340 to 420, so each timing has a wide range. The runs of the three runtimes were made in turn.
mainwriteFileSyncin the process: runs with one'change', of 2000setImmediate, append, 200 runs: runs with two'change'onceloop: appendother, appendf, 5 ms, appendf, 300 runs: runs where the loop never heard offchmodof one file in turn: calls, timer delay, RSSmkdir nand a file in it: events, mean of 100, 4 runsmkdir -p a/b/cand a file, 4 runs.textof the release binary, bytessize_ofFSWatcher, posted task,Entry, bytes.textand the 32 bytes ofFSWatchercome from fs.watch: run nextTicks and microtasks between the events of one batch #43715. Without it this PR removes 512 bytes of.textand adds nothing toFSWatcher.maintoo./proc.perf,valgrind,straceandbloatyare not on the machine, so there is no count of instructions.Why the merge cannot lose a change. The watcher thread sets the flag before it posts an event. The JS thread clears the flag where
runtakes a posted batch over, before a listener call. So a set flag means that no batch arrived on the JS thread after the last event was posted, and the listener call for that event starts later than the change of each record that merged into it. The order of the batches does not matter. Both sides useSeqCst.The flag against a number for each event. The first form of this PR gave each event a number and stored the newest delivered number. That is exact: a batch that holds older events does not stop the merge. It is wrong when a later batch runs first, which happens for a watcher that a macro created, because the macro loop and the regular loop both take its batches. The flag allows one more event for each batch that arrives. Measured by the review with the flood where each 10th append goes to a second file: 52000 to 56000 calls with the flag, 40000 with numbers.
What "identical" is. For inotify: the same mask and the same path. The kernel compares the mask, the watch and the name, not the cookie. For kqueue: the same path and kind of event. The kernel merges more there: each note of one file. An entry that the walk of a new directory found merges with the entry's own
IN_CREATEorIN_MOVED_TO. Error, overflow andIN_IGNOREDevents, and all macOS events, are posted with no check.Tests and the clause that each one holds. Each clause was changed in a debug build, and the tests named here failed.
IN_CREATEandIN_MOVED_TO: the four tests of "a new directory reports an entry once"./proc/self/fdinfoof the inotify descriptor to see the watch of a sentinel directory.History. The rule came with the first
fs.watch(#3249), which read throughbun.Watcher. #29952 gavefs.watcha reader of its own. #31830 narrowed the rule, and #32962 added a way around it for one event.Removed with the suppression. It compared wall-clock times. After a backward step of the clock, every event with the path and type of the last one was dropped until the clock passed the old value.
Cases that remain.
win_watcher.rs) has the same rule and is not changed here. On Windows Server 2019 Node reports two'rename'forrmdir,mkdirand Bun one.fs.watch()of a directory that was removed and created again gets no event while the old watcher is open.mainhas the same defect. fs.watch: report the watched directory's removal once on Windows, and start a new watch when the path is watched again #39827 holds the fix for a moved root.fs.promises.watchhas no bound on its queue (fs.promises.watch: implement maxQueue and overflow options #35878).main.Self-review, open.
Suites run with the debug build.
test/js/node/watch/(0 fail),test/js/node/test/parallel/test-fs-watch*andtest-fs-promises-watch*(42 of 42), the new tests 5 times (0 fail).cargo checkpasses for the 12 CI targets.cargo clippypasses forbun_runtimeon Linux and macOS.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/watch/fs.watch.test.ts