Repository navigation
Conversation
|
Updated 10:39 PM PT - Oct 24th, 2026
✅ @robobun, your commit 1164ab606a216f1f568fc9c2166a8ede8b67fae6 passed in 🧪 To try this PR locally: bunx bun-pr 34718That installs a local version of the PR into your bun-34718 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesAsync watcher behavior
Possibly related PRs
Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely states the main change: implementing fs.promises.watch as a real async generator. |
| Description check | |
The description provides a detailed explanation of the problem, the fix, behavior changes, trade-offs, test coverage, and verification results. It does not use the template headings exactly, but it co… |
Full details: Description check
-
Explanation
The description provides a detailed explanation of the problem, the fix, behavior changes, trade-offs, test coverage, and verification results. It does not use the template headings exactly, but it contains the required information under equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/js/node/fs.promises.ts`:
- Around line 117-124: Remove the unreachable "close" and "error" branches from
the event-processing loop consuming watcher events, leaving it to yield each
filesystem change event. Apply the root-cause listener wiring fix on the watcher
separately as requested by the related comment, rather than preserving dead
checks in this loop.
In `@test/js/node/watch/fs.watch.test.ts`:
- Around line 908-918: Extend the async iterator protocol test around it.throw
to invoke it.throw with a representative error and assert the resulting Promise
follows the expected rejection or closed-generator semantics, while preserving
the existing return() and next() assertions.
🪄 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: 3aa195cf-2f0a-4ebc-950c-b4f0b9c4aa23
📒 Files selected for processing (2)
src/js/node/fs.promises.tstest/js/node/watch/fs.watch.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/node/watch/fs.watch.test.ts (1)
947-949: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not discard iterator cleanup failures.
Both cleanup paths catch every
return()rejection, so tests can pass while iterator shutdown orfinallycleanup is broken. Assert the expected closed result, or the specific expected abort error, instead of usingcatch(() => {}).As per coding guidelines, tests must not silently weaken a safety net, and assertions should verify the strongest invariant.
Also applies to: 965-967
🤖 Prompt for 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. In `@test/js/node/watch/fs.watch.test.ts` around lines 947 - 949, Update the iterator cleanup in the finally blocks around ac.abort() and it.return() so return() rejections are not swallowed. Assert the expected closed iterator result or the specific expected abort error, preserving validation of shutdown and finally cleanup behavior in both affected cleanup paths.Source: Coding guidelines
src/js/node/fs.promises.ts (1)
81-87: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake
watch()shutdown wake a blocked idle iterator.return()/throw()can stay pending while the generator sits atawait promise, sofinallynever runs and the watcher can keep the native handle alive. Add a regression that callsreturn()/throw()after priming the iterator withnext()and before any event arrives.🤖 Prompt for 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. In `@src/js/node/fs.promises.ts` around lines 81 - 87, The watch iterator’s shutdown path must wake the promise awaited by the idle generator so return() or throw() can complete and run cleanup. Update the pendingResolve/wake mechanism in watch() to settle the blocked await during shutdown, while preserving normal event delivery, and add regression coverage that primes next() before invoking both return() and throw() without emitting an event.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/js/node/fs.promises.ts`:
- Around line 81-87: The watch iterator’s shutdown path must wake the promise
awaited by the idle generator so return() or throw() can complete and run
cleanup. Update the pendingResolve/wake mechanism in watch() to settle the
blocked await during shutdown, while preserving normal event delivery, and add
regression coverage that primes next() before invoking both return() and throw()
without emitting an event.
In `@test/js/node/watch/fs.watch.test.ts`:
- Around line 947-949: Update the iterator cleanup in the finally blocks around
ac.abort() and it.return() so return() rejections are not swallowed. Assert the
expected closed iterator result or the specific expected abort error, preserving
validation of shutdown and finally cleanup behavior in both affected cleanup
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4ddd6c1-e39e-4f22-955b-9834b09358e2
📒 Files selected for processing (2)
src/js/node/fs.promises.tstest/js/node/watch/fs.watch.test.ts
|
Re: the two out-of-diff review findings:
const it = fsp.watch(dir)[Symbol.asyncIterator]();
it.next(); // parks at await (no events)
await it.return(); // hangs on Node tooNode's own |
There was a problem hiding this comment.
The abort-between-yields regression I flagged earlier is fixed (23e3ec7) and covered by a new test; the current diff looks correct to me. Deferring to a human because this is a full rewrite of the fs.promises.watch iterator with observable Node-compat behavior shifts (native handle now opens on first next(), validation/ENOENT surface async instead of sync, return() no longer wakes a parked next()) that a maintainer should ratify.
What was reviewed:
- Traced the flattened loop:
signal.abortedis now re-checked after everyyieldand everyawait, so abort-during-yield throwsAbortErrorinstead of resolving{done:true}via the close sentinel. finallyruns on return/throw/error and idempotently closes the watcher + detaches the abort listener; concurrentnext()is serialized by the async-generator queue so a singlependingResolveslot is sufficient.- Confirmed
fs.promises.watchhas no other test-suite consumers that depend on the old synchronous-throw timing.
Extended reasoning...
Overview
Rewrites watch() in src/js/node/fs.promises.ts from a hand-rolled {[Symbol.asyncIterator]() { return {next, return} }} object into a real async function*, matching Node's implementation. Adds four tests to test/js/node/watch/fs.watch.test.ts covering iterator shape (return()/throw() return Promises), concurrent next(), abort-while-suspended-at-yield, and never-iterated process exit.
Security risks
None. Pure JS iterator-protocol reshaping over the existing native watcher binding; no new I/O, parsing, or trust boundaries.
Level of scrutiny
Medium. Built-in node:fs/promises module code with subtle async-generator control flow. The first revision had a real regression (my earlier inline finding: aborting between yields resolved {done:true} instead of throwing AbortError because the close sentinel was drained before the abort re-check). That was fixed by flattening the loop to one queue item per iteration so the abort check sits between every yield/await, with a regression test added. I re-traced the fixed flow and it holds.
Other factors
- All CodeRabbit threads and my prior thread are resolved; the bug-hunting pass on the latest commits found nothing.
- Grep confirms
test/js/node/watch/fs.watch.test.tsis the only test file exercisingfs.promises.watch, so the sync→async error-timing shift shouldn't silently break other suites. - The intentional behavior changes (lazy native-handle open, ENOENT/validation on first
next(),return()queuing behind an in-flight internalawaitper spec/Node) are defensible Node-compat moves but are user-visible enough that a maintainer should sign off rather than auto-approving. - Test cleanup paths look sound: after
Promise.all([p1,p2])the generator is at suspendedYield, soac.abort(); await it.return()completes without swallowed rejections.
|
Status. Head 1164ab6, on main 179664e. The PR waits for a maintainer's yes or no (see the question in the Notes of the body). CI runs on this head. How I reproduced the problem (linux x64, Node v26.3.0 as the reference):
How I checked the fix.
I did not run anything on macOS or Windows. |
c688c43 to
7e37e67
Compare
There was a problem hiding this comment.
No issues found — the abort-between-yields regression I flagged earlier is fixed (abort re-checked at the top of every loop iteration) and covered by a test. Deferring to a maintainer since this shifts observable Node-compat timing (validation/ENOENT now surface on first next() instead of synchronously).
What was reviewed:
- Generator control flow:
finallycloses the watcher and detaches the abort listener on every exit path (return/throw/return()/throw());closeWatcher()is idempotent. - Single
pendingResolveslot is safe because async-generator machinery serializes body execution — only oneawaitcan be pending. return()-while-parked-at-await hanging matches Node/spec (author verified);close/errorsentinel branches are reachable via the internal binding's multiplexed callback.- New tests: concurrent
next(),return()/throw()shape, abort-at-yield, never-iterated exit, ENOENT-on-first-iteration — all wire failures to reject and clean up infinally.
Extended reasoning...
Overview
Rewrites fs.promises.watch in src/js/node/fs.promises.ts from a hand-rolled {next, return} iterator into a real async function*, matching Node's implementation shape. The generator body opens the native watcher lazily on the first next(), queues events via $createFIFO, re-checks signal?.aborted between every yield/await, and closes the watcher + detaches the abort listener in finally. Adds five tests to test/js/node/watch/fs.watch.test.ts.
Security risks
None. This is iterator-protocol plumbing over an existing native watcher binding; no new input parsing, no auth/crypto, no untrusted data handling beyond what already existed.
Level of scrutiny
Moderate. It's a self-contained rewrite of one function, but it's user-facing Node-compat surface with intentional observable-behavior changes: argument validation and ENOENT now reject the first iteration instead of throwing synchronously from watch(), and an un-iterated watch() no longer pins the event loop. Both match Node, but a maintainer should confirm they want that timing shift (it could break code that relied on the old synchronous throw, even though that reliance was itself a Node-incompat).
Other factors
- My prior inline finding (abort while suspended at
yieldcompleting{done:true}instead of throwingAbortError) was fixed by flattening to one queue item per outer iteration and adding a regression test. - CodeRabbit's two comments (dead-branch false positive,
it.throw()coverage) were both resolved; robobun's rebuttal on the internal-binding callback contract is correct — I checkedsrc/js/internal/fs/watch.tsuses the same"close"/"error"eventType dispatch. - The author verified
return()pending behind an in-flightawaitmatches Node and the async-generator spec, so that's not a regression. - Test coverage is thorough for the specific protocol fixes; existing
fs.promises.watchtests in the file continue to exercise the for-await path. Prior CI run had this test file green on all lanes.
|
@robobun rebase |
7e37e67 to
bafe978
Compare
|
Rebased onto main (83c7055). One conflict in |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — both earlier findings (abort-between-yields regression, comment-cop flags) are addressed, and the current revision has no new issues.
What was reviewed
- Generator control flow: abort re-checked between every yield/await;
finallycloses the watcher and detaches the abort listener on all exits (return/throw/close/error/ENOENT). - Pre-aborted signal and ENOENT paths throw before the native handle exists, so nothing leaks; never-iterated watch never opens a handle.
- Test cleanup paths:
it.return()after abort on a suspended generator, and the abort-between-yields test's watcher lifetime on assertion failure — ruled out as a leak concern. - CI:
fs.watch.test.tsgreen on all lanes; the one failure (worker-transfer-terminate-stress) is a documented unrelated flake.
Extended reasoning...
Overview
This PR rewrites fs.promises.watch() in src/js/node/fs.promises.ts from a hand-rolled {next, return} iterator into a real async function*, matching Node's implementation shape. Net ~50 lines removed. Five new tests in test/js/node/watch/fs.watch.test.ts cover concurrent next(), return()/throw() shape, abort-while-suspended-at-yield, never-iterated process exit, and ENOENT surfacing on first iteration.
Security risks
None. This is a JS-side async iterator wrapper around the existing native watcher binding. No new syscalls, no path handling changes, no untrusted input parsing beyond what the old code already did (validateAbortSignal, filename coercion). The __proto__: null on yielded events is preserved.
Level of scrutiny
Medium. It's a Node-compat behavioral change (errors now surface on first next() instead of synchronously), but that's the documented intent and matches Node exactly. The function is self-contained; no other callers in the tree depend on the old synchronous-throw shape. The generator's finally block correctly handles every exit path I traced: normal return on 'close', throw on 'error', it.return()/it.throw() from the consumer, ENOENT before the try, and pre-aborted signal before the watcher exists.
Other factors
- I reviewed this PR twice previously. The first pass found a real regression (abort while suspended at
yieldreturned{done:true}instead of throwingAbortError) — fixed with a flat one-item-per-iteration loop and a dedicated regression test. The second pass flagged comment-cop violations — trimmed. Both threads resolved. - CodeRabbit's two findings (dead-code branches,
throw()not exercised) were both addressed/refuted; the'close'/'error'branches are reachable via the internal binding's multiplexed callback contract. - The bug-hunting system found nothing this run; one candidate (test cleanup leaking a watcher on assertion failure) was verified and ruled out.
Promise.withResolvers()(non-primordial) matches the pre-existing pattern in this file and the removed code.- CI build 88755:
fs.watch.test.tspasses on all lanes; the sole failure is the known-intermittentworker-transfer-terminate-stressSIGABRT on x64-asan, unrelated to fs.
There was a problem hiding this comment.
No issues found — the implementation looks correct and all prior review threads are resolved. Deferring only because this is a public-API rewrite with an observable timing change (ENOENT / arg-validation errors now surface on the first next() instead of synchronously from watch()), which a maintainer should sign off on.
What was reviewed:
- Generator control flow: abort re-checked between every yield/await;
finallycloses the watcher and detaches the abort listener on all exits (return, throw, error, close sentinel). - Single
pendingResolveslot is safe because the async-generator queue serializes body execution — only oneawait promiseis ever live. eventType === "close"/"error"branches are reachable via the internal binding's multiplexed callback (same contract asinternal/fs/watch.ts #onEvent).- Test cleanup paths:
ac.abort()beforeawait it.return()wakes any parked await soreturn()can't hang behind a queuednext().
Extended reasoning...
Overview
Rewrites fs.promises.watch (src/js/node/fs.promises.ts) from a hand-rolled {next, return} iterator to a real async function*, matching Node's implementation shape. The generator body opens the native watcher on first next(), drains a FIFO of {eventType, filename} events, re-checks signal.aborted between every yield, and closes the watcher / detaches the abort listener in a finally. Five new tests in test/js/node/watch/fs.watch.test.ts cover concurrent next(), return()/throw() shape, abort-while-suspended-at-yield, never-iterated process exit, and ENOENT-on-first-iteration.
Security risks
None. This is a JS-side control-flow refactor of an existing Node-compat API; no new attack surface, no untrusted-input parsing, no privilege boundaries touched.
Level of scrutiny
Medium-high. The function itself is small (~50 lines), but async-generator + abort + native-close-sentinel interaction is subtle — my earlier review of this PR caught a real regression (abort between yields resolving {done:true} instead of throwing AbortError), which was fixed with a regression test. The current revision has been through CodeRabbit, comment-cop, and two rounds of my inline findings; all threads are resolved and the bug hunter found nothing this run.
Other factors
- Observable behavior change: because the generator body runs on first
next(),ENOENT,ERR_INVALID_ARG_TYPE, and pre-aborted-signal errors now reject the first iteration rather than throwing synchronously fromwatch(). This matches Node exactly and is documented in the PR description, but it's a user-visible semantics shift for anyone wrappingfs.promises.watch()in a synchronous try/catch. That's the one thing I'd want a maintainer to explicitly acknowledge before merge. - Node parity on
return()while parked atawait: the author verified (and documented in a PR comment) thatreturn()queuing behind an in-flight await matches both the async-generator spec and Node's ownfsPromises.watch; the escape hatch isAbortSignal, which does wake the await viaonAbort → wake(). - CI: build 88755 (pre-rebase head) passed
fs.watch.test.tson all lanes; the post-rebase build 88857 was retriggered at the current head. - Test quality: new tests follow harness conventions (bunEnv/bunExe,
await usingfor spawns, drain stdout/stderr/exited concurrently,repeat()polling instead of sleeps, abort-signal cleanup infinally).
|
This change also fixes On main, Repro: import fsp from "node:fs/promises";
const w = fsp.watch(".");
console.log("asyncDispose:", typeof w[Symbol.asyncDispose]);
try { { await using x = w; } console.log("ok"); } catch (e) { console.log("THREW", e.message); }
const t = setTimeout(() => { console.log("loop still alive after 1s (watcher leaked)"); process.exit(0); }, 1000); t.unref();Bun 1.4.0 and main print: Node v26.3.0 prints A test for this shape would lock it in. For example: |
|
I ran the consumer that the original report names, and the result changes the merge decision for this PR. The report says that a With this PR, pending calls settle oldest first, as in Node. Each event goes to an abandoned call, and the live call starves.
const it = fsp.watch(dir)[Symbol.asyncIterator]();
while (true) {
const r = await Promise.race([it.next(), timeout(100)]);
if (r === TIMEOUT) continue; // abandons the pending next(), then calls next() again
seen.push(r.value.filename);
}
// meanwhile: 5 x fs.mkdirSync(dir + "/dN"), 300 ms apart (one event each)This is the other side of the fix, not of the generator. Every design that settles the first waiter first has it, a resolver queue in the hand-written iterator too. A loop that keeps its pending promise across timeouts ( The PR still fixes |
|
#46218 changes the abort path of the What #46218 does:
The order of the two PRs:
A maintainer decides which lands first. |
The hand-rolled {next, return} iterator had a single nextEventResolve
slot, so a second pending next() overwrote the first's resolver and the
first waiter hung forever. return() returned a plain object instead of a
Promise, and throw() was missing entirely.
Replace it with an async function* like Node does. The generator
machinery queues concurrent next() calls by construction and provides
spec-compliant return()/throw() from %AsyncGeneratorPrototype%.
The native handle is now opened on the first next() (inside the
generator body, like Node), so an fsp.watch() that is never iterated no
longer leaks a watcher that keeps the process alive, and ENOENT is
surfaced on the first next() rather than synchronously.
Resuming from yield landed inside the inner drain loop, skipping the signal?.aborted check. An abort that fired while suspended at yield would drain buffered events (including the close sentinel pushed by the native close) and resolve done:true instead of throwing AbortError. Flatten to one queue item per outer iteration so the abort check runs between every yield/await.
…e abort order - return(), throw(), Symbol.asyncDispose and break each end the iterator, remove its abort listener and close the watcher. - return() waits behind the pending next() calls. - Argument errors reject the first next(), not the call. - A for-await body that aborts on an event is not run again. This replaces the one-write form of the abort test. - The watcher-error fixture moves out of the root-only test, so that it also runs when the tests do not run as root.
2b543e8 to
2ebaef6
Compare
|
This PR needs a yes or no from a maintainer before a merge. The head is now 2ebaef6, on main 179664e. What the PR is. What this push adds.
What I found and corrected.
What is not in this PR. A report said that after an abort the iterator drops the events that are still in its queue, and that Node yields them first. That is true, on main too. I built Node's order and took it out again: with it, the next The question. Is Node's behaviour worth the breaks in the table? No user reported the problem that this PR fixes.
|
There was a problem hiding this comment.
The new commits address the earlier abort-at-yield thread: the loop now re-checks signal?.aborted at the top of every iteration after continue, so an abort that fires while the generator is suspended at yield throws AbortError instead of completing on the close sentinel. Beyond the inline findings, I also checked that the finally's closeWatcher() after a native close event is safe (native close() in src/runtime/node/node_fs_watcher.rs is guarded by a closed flag), that the single pendingResolve slot is sufficient now that only one generator body runs at a time, and that queue is a $createFIFO, so shift() is not the Array#shift pattern.
Extended reasoning...
The change rewrites fs.promises.watch in src/js/node/fs.promises.ts into a native async generator with lazy validation and lazy watcher creation, a single finally for all exit paths, and adds a suite of iterator-protocol and exit-path tests in test/js/node/watch/fs.watch.test.ts. No security-sensitive surface is touched. The prior-run inline finding (abort while suspended at yield) was fixed by commit 4adb1a2, and the comment-cop flags were addressed. The inline findings posted this run concern macOS timing in the new spawned fixture and the queued-return() ordering test, and are the reason a human should still look before merging.
…on events per write - The fixture for break, return(), throw() and await using writes until each exit is done. It waited for a second event from one write before. - The return() test compares the order of the calls, not the file names. - The rounds of the abort test run at the same time.
|
#46937 is stacked on this branch. It adds One more row for the question above. Node.js also bounds the queue when neither option is passed: it keeps 2,048 unread events and drops each later one with a warning. #46937 leaves that case unbounded, as main does, until a maintainer decides. |
Problem
fs.promises.watch()opens its native watcher inside the call (src/js/node/fs.promises.ts:100on main). A watcher that nothing iterates keeps the process alive.fs.promises.ts:153). A second pendingnext()replaces it, so the first never settles.throw()andSymbol.asyncDisposeare missing.Fix
watch()is anasync function*, as in Node. The body runs on the firstnext(), pending calls queue, and afinallycloses the watcher.test/js/node/watch/fs.watch.test.ts(15 new tests, the released bun fails 13),test/js/node/watch/, nine Node tests.Background
next(). It serves one request at a time, oldest first. Areturn()is a request too.Downsides
next()on a timeout and calls it again sees 0 of 5 events (main: 5 of 5).next(), so atryaround only thewatch()call missesENOENT.return()waits behind a pendingnext()with the watcher open. A signal ends it.Notes
Breaking changes, as on Node v26.3.0
Linux x64. The Bun columns are one debug build of main 179664e with only the bundled
node/fs.promises.jsexchanged.next()calls are pending, two events comenext()on a timeout and callsnext()again, 5 eventspending ??= it.next()try { w = watch(missing) } catch {}, with the loop outside thetryENOENTfrom the callnext()rejectsnext()rejectstrywatch()and the firstnext()next()firstreturn()while anext()is pendingsignaland abortnext(),return())next()keeps (100,000 calls)Readable.from(watch(dir)).destroy()while the stream waits:'close'comessignaltowatch()return()behind a pendingnext(), remove and re-create the path, watch it again: the new watcher gets eventssrc/runtime/node/path_watcher.rs), and here the inotify descriptor of that watcher is gone. Plainfs.watchhas the fault on main: with a firstfs.watchleft open, the second watcher got events in 3 of 8 rounds (Node: 8 of 8). fs.watch: watch a macOS file with kqueue, join only a watcher of the same inode #44067 changes the join. This PR moves the fault: it adds thereturn()case, and it removes the case of awatch()that nothing iterates (main 3 of 8, this PR 8 of 8). I did not run these on macOS or Windows.tryaround the call only.@stacksjs/bun-router@0.1.24(dist/cli.js) wrapswatch(dir, { recursive: true })intry/catchto skip a missing directory, and runs the loop in an async function that nothing awaits. On main the missing directory is skipped. With this PR, and on Node, the loop rejects withENOENTand nothing handles it. I read the published file. I did not run the package.Question for a maintainer
What this PR does not change
next()rejects at once and drops the events that are still in the queue, as on main. Node yields them first, up to itsmaxQueueof 2,048.next()after an abort gave one more event in 11 of 20 rounds of onewriteFileSync(main and Node: 0 of 20). The native watcher gives a whole batch of events to the listener in one task, so the queue holds events that Node does not have yet. fs.watch: run nextTicks and microtasks between the events of one batch #43715 changes that delivery.for await: a body that aborts on an event is not run againkeeps the order of main.maxQueueandoverfloware not implemented (fs.promises.watch: implement maxQueue and overflow options #35878 is closed).Related open PRs (checked with
git merge-treeagainst this head)src/js/node/fs.promises.ts: fs.promises.watch: settle an aborted next() on Node's tick and in listener order #46218, node:fs: apply node'spersistent/recursiverules in the JS entry points of the watch APIs #46057, node:util: make aborted() an async function, like Node #46130, domain: route fs callback throws to uncaughtException, unblocking 4 tests (domain 88%→96%) #34661.test/js/node/watch/fs.watch.test.ts: bun test: Vitest compatibility #44833, fs.watch: run nextTicks and microtasks between the events of one batch #43715.Repros
try { for await (const e of watch("/missing")) {} } catch {}catchesENOENTon main, with this PR, and on Node. An earlier text of this body said that main misses it. That was wrong.next()replaces the resolver slot, so the live call gets each event. The abandoned calls never settle.Other differences from main that follow from the generator
return()returns a promise. On main it returns a plain{ value, done }object.it[Symbol.asyncIterator]() === it), and it hasnext(). On mainwatch(dir).nextisundefined.await using w = fsp.watch(dir)works. On main it throwsTypeError: @@asyncDispose and @@dispose must not be undefined or nulland leaks the watcher.next(). On main it is added in the call.Tests
fs.promises.watchblock. The released bun (1.4.3-canary.1) fails 13 of them.for await: a body that aborts on an event is not run againkeeps the abort order of main. It fails when the order is changed (20 rounds, one rename per step).a watcher error rejects next(), and then the process can exitis a fixture from the root-onlyfs.watchtest. It now also runs when the tests do not run as root.removeEventListenerin thefinally, argument errors thrown from the call, noSymbol.asyncDispose, the outer{ [Symbol.asyncIterator]() }object of main, no close in thefinally.if (signal?.aborted) throwbefore the watcher opens. With the line deleted, a pre-aborted signal and a missing path still give theAbortError, so that test cannot fail.Numbers
next()+ abort: 37,820 to 39,925 (+5.6%). One delivered event: +660 (+4.9%). Open + one event +return(): +670 (+2.4%). Cycles and wall time are equal. The generator code did not change after that measurement.node:fs/promisesis 1,330 to 1,331 cells.signal.abortedper delivered event: 2 before and after (Node: 1).Keeping the wake-up from
return()A plain async generator cannot wake a pending
next()fromreturn(). An ownreturnproperty on the generator object can: it closes the watcher, which wakes the body, and then it calls the prototypereturn. The cost is one closure and one property per watcher. This PR does not do it, because it departs from Node.no test proof · iteration 10 · 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