node:stream: leave Readable.toWeb source non-flowing after EOF - #34031
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesReadable resume state handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:53 AM PT - Jul 12th, 2026
❌ @robobun, your commit ba3d3e2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34031That installs a local version of the PR into your bun-34031 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The bitmask guard is correct and both sides (fd-slicer drain-resume before 'end', toWeb pull-resume after 'end') are covered by tests. Deferring to a human because this narrows — rather than restores — Node 26's resume() destroyed-guard on a core Readable.prototype method, and that design tradeoff (vs. e.g. fixing the toWeb adapter's post-EOF pull() instead) is worth a maintainer sign-off.
Checked: the guard (kDestroyed | kEndEmitted) only fires when both bits are set, so the fd-slicer path (destroyed set before push(null), 'end' not yet emitted) still reaches resume_.
Checked: all internal resume() callers (pipeOnDrain, on('data'), updateReadableListening, initial pipe()) have nothing left to flow once both destroyed and end-emitted, so the early return is inert there.
Checked: the existing "Readable events in the right order" snapshot test — the skipped kHasPaused/kPaused mutation doesn't change its emitted-event sequence.
Extended reasoning...
Overview
Two-file change: src/js/internal/streams/readable.ts adds a 3-line early-return guard to Readable.prototype.resume() (no-op when kDestroyed && kEndEmitted), replacing a comment-only deliberate-divergence note. test/js/node/stream/node-stream.test.js adds one test asserting readableFlowing === false / isPaused() === true on the source after Readable.toWeb drains to EOF.
Security risks
None. This is post-EOF state-flag bookkeeping in the Node streams compat layer; no untrusted input parsing, allocation sizing, or privilege boundaries are involved.
Level of scrutiny
Medium-high. The diff is tiny and mechanically correct, but Readable.prototype.resume() is on the hot path of every pipe(), 'data' listener attach, and drain cycle. More importantly, the fix embeds a design decision: it narrows Node 26's guard (which checks only kDestroyed) to kDestroyed && kEndEmitted to keep the fd-slicer/yauzl workaround alive. That's a deliberate divergence from upstream in both directions — stricter than Bun's current no-guard, looser than Node's — and an alternative fix (guarding the adapter's pull() at webstreams_adapters.ts:564 instead of the prototype method) exists. A maintainer familiar with the history of 6abf57e and #33467 should confirm this is the preferred layering.
Other factors
- The bitmask expression is correct (
(state & (A|B)) === (A|B)tests both bits set);return thispreserves the chainable contract. - The new test awaits
'close'rather than a timeout, and asserts a full state object — good hygiene. - The existing fd-slicer regression test in the same file exercises the destroyed-but-not-end-emitted branch, so both sides of the narrowed guard have coverage.
- I traced every internal
.resume()caller insrc/js/internal/; none has meaningful work to do oncekEndEmittedis set, so the early return is safe for them. - PR description reports the vendored
test-stream-*.jsparallel suite has no new failures, but CI is still building.
|
On the layer question: Node's The narrowed guard carves out exactly the fd-slicer window ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/stream/node-stream.test.js`:
- Line 804: Update the teardown wait in the stream test to wait for the source
stream’s close completion rather than relying on src.destroyed, which changes
before the close event. Use src.closed or await the "close" event directly while
preserving the existing immediate-resolution behavior when closure has already
completed.
🪄 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: 35ec167c-d351-42ba-af31-9c999c6fb757
📒 Files selected for processing (2)
src/js/internal/streams/readable.tstest/js/node/stream/node-stream.test.js
Readable.toWeb's pull() hook calls source.resume(). After the source emits 'end' and autoDestroys, Node 26's resume() is a no-op on destroyed streams (nodejs/node#62557), so the source is left with readableFlowing === false / isPaused() === true. Bun dropped that guard in 6abf57e so fd-slicer-style readables (which set destroyed = true before push(null)) can still flush their buffered tail on drain, and as a result the post-EOF resume() flips the source back to flowing. Narrow the resume() guard to kDestroyed && kEndEmitted: once 'end' has fired there is nothing left to flush, so resume() can become a no-op again without affecting the fd-slicer case (which has destroyed set but endEmitted clear when drain resumes it).
f3af6fc to
ba3d3e2
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this narrows the destroyed-stream guard in Readable.prototype.resume() — a deliberate divergence from Node 26's exact check on a very hot compat path — and the author explicitly offered an alternative layering (guarding in the adapter instead). Worth a maintainer sign-off on the layering choice.
Checked: the bitmask test (kDestroyed | kEndEmitted) === (kDestroyed | kEndEmitted) correctly requires both bits and returns this for chaining; the fd-slicer regression test's drain-resume window (destroyed && !endEmitted) is not caught by the narrowed guard; pause()'s full kDestroyed guard is intentionally untouched per #33467; the existing "Readable events in the right order" test's single resume event fires before destroy so is unaffected.
Extended reasoning...
Overview
Two files: src/js/internal/streams/readable.ts adds a 3-line early-return to Readable.prototype.resume() when both kDestroyed and kEndEmitted bits are set, replacing a comment-only "no guard" with a narrowed version of Node 26's kDestroyed-only guard. test/js/node/stream/node-stream.test.js adds one regression test asserting the source's post-EOF readableFlowing/isPaused() state matches Node after Readable.toWeb drains it.
Security risks
None. Pure state-flag behavior in the Node stream compat layer; no I/O, parsing, auth, or user-controlled input paths touched.
Level of scrutiny
Medium-high. The diff itself is tiny and mechanically correct (bitmask check is right, return value preserved), but Readable.prototype.resume() is exercised by every flowing-mode consumer and every pipe. This PR reintroduces a guard that was deliberately removed in 6abf57e, in a narrowed form, and codifies a permanent divergence from upstream Node's implementation. The author's own follow-up comment ("Happy to move the check into the adapter instead if that layering is preferred") signals this is a layering decision a maintainer should ratify, not a mechanical fix.
Other factors
The reasoning is sound and well-documented: the fd-slicer window is exactly destroyed && !endEmitted (buffered tail still to deliver), and once 'end' has fired there is provably nothing to flush, so the no-op is safe. Both the fd-slicer regression test and the new toWeb test are present and cover the two sides of the guard. The CodeRabbit nit about waiting on src.closed was applied and resolved. No bugs found by the bug-hunting system. I'm deferring only because CLAUDE.md flags deliberate Node-divergence layering decisions as needing maintainer agreement, and the author explicitly left the layering open.
|
CI on build 72187 failed on lanes unrelated to this diff. The only code touched here is Failing tests, none of which exercise
Ready for review. |
2 participants
Repro
readableEnded/destroyedagree; onlyreadableFlowing/isPaused()diverge, so code that branches on those after the adapter finishes (pooling, reuse logic, diagnostics) observes the wrong state.Cause
The toWeb adapter's
pull()callssource.resume(). After the source emits'end'and autoDestroys, Node 26'sresume()is a no-op on destroyed streams (nodejs/node#62557), so the latepull()s leave the source non-flowing.Bun dropped that guard in 6abf57e so that fd-slicer-style readables (yauzl / extract-zip / puppeteer), which assign
this.destroyed = trueright beforepush(null), can still be resumed by a piped destination's'drain'to flush their buffered tail. With no guard at all, the adapter's post-EOFresume()flipsreadableFlowingback totrue.Fix
Narrow the
resume()guard tokDestroyed && kEndEmittedinstead of removing it entirely. In the fd-slicer case'end'has not yet been emitted when drain resumes the source, so the narrowed guard does not fire and the buffered tail still flushes. Once'end'has fired there is nothing left to flush, soresume()can become a no-op and keep state parity with Node.pause()is left unchanged; #33467 addresses the mirrored post-pipe divergence on thepause()side.Verification
Readable.toWeb leaves the source paused / non-flowing after EOFfails on main (readableFlowing: true, isPaused: false) and passes with this change; the same test body passes on Node v26.3.0.drain still resumes a source that flagged itself destroyed before EOF) still passes.test/js/node/stream/and the vendoredtest-stream-*.jsparallel suite: no new failures relative to main.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/stream/node-stream.test.js