Conversation
|
Updated 7:23 AM PT - Aug 4th, 2026
❌ @robobun, your commit 2b543e8 has 1 failures 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 | ✅ 4✅ Passed checks (4 passed)
Comment |
![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.
|
CI on the rebased head (build 88857 at 2b543e8, re-rolled once):
The previous run (88755) had Update: |
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 |
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.
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 |
This branch has not been deployed
2 participants
Problem
fs.promises.watch()opens its native watcher inside the call (src/js/node/fs.promises.ts:100on main). An un-iterated watcher keeps the process alive, and nothing can close it.watch("/missing")throwsENOENTfrom the call, so atryaround thefor awaitmisses it.fs.promises.ts:153). A second pendingnext()replaces it, so the firstnext()never settles.throw()andSymbol.asyncDisposeare missing.Fix
watch()is anasync function*, as in Node. The body runs on the firstnext(): the watcher opens there, and each error rejects thatnext().next()calls and suppliesreturn(),throw()andSymbol.asyncDispose. Afinallycloses the watcher on every exit.test/js/node/watch/fs.watch.test.ts(five new tests, the released bun fails four), plustest/js/node/watch/.Background
next(). It serves one request at a time, oldest first. Areturn()is a request too.Downsides
next()calls settle oldest first, as in Node. A loop that abandonsnext()on a timeout and calls it again saw 5 of 5 events on main. It sees 0 of 5 here and on Node v26.3.0, and itsreturn()waits for the next event (anAbortSignalstill ends it).watch()and the firstnext()are lost, as in Node. Argument errors (watch(12)) reject the firstnext(), not the call.next()+ abort 37,820 to 39,925 (+5.6%), one delivered event +660 (+4.9%). Cycles and wall time are equal.Notes
Repro for the resolver slot
Repro for the eager start
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).await using w = fsp.watch(dir)works. On main it throwsTypeError: @@asyncDispose and @@dispose must not be undefined or nulland leaks the watcher. See the comment below on this PR.Tests
fs.promises.watchblock: iterator shape (return()andthrow()), two concurrentnext()calls, abort between two yields, a never-iterated watcher in a child process, andENOENTon the first iteration.yield, the body must check the signal again before it reads the queue, or an abort ends the loop with{ done: true }and noAbortError.Numbers
return()is +670 (+2.4%). Runs with N events, 2N events, and the top JIT tier off give +604 to +911 per event, so it is a cost per operation. The load ofnode:fs/promisesandfs.promises.readFiledo not change. 30,000 events take 2,384 ms against 2,390 ms.The timeout-retry consumer
next()replaces the resolver slot, so the live call gets each event. The abandoned calls never settle.pending ??= it.next(), racepending, clear it when it settles) sees 5 of 5 on main, on this branch and on Node.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 queues the close event and wakes the body, and then it calls the prototypereturn. A userland model of this shape settles the pendingnext()with{ done: true }at once, on bun 1.4.3 and on Node v26.3.0, and the object is still an[object AsyncGenerator]. 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 6 · 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