node:stream: restore the destroyed-stream guard in Readable.prototype.pause - #33467
Conversation
|
Updated 8:14 PM PT - Jul 6th, 2026
❌ @robobun, your commit e25520e has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33467That installs a local version of the PR into your bun-33467 --bun |
|
Warning Review limit reached
Next review available in: 25 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)
WalkthroughAdds an early-return guard to ChangesPause No-Op on Destroyed Streams
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the change is small and well-verified, but since it partially reverts a deliberate Node-26 divergence in core Readable.prototype.pause semantics, it's worth a maintainer confirming the fd-slicer reasoning before merge.
Extended reasoning...
Overview
This PR restores the kDestroyed early-return guard in Readable.prototype.pause() (src/js/internal/streams/readable.ts:1180-1182), which had been removed alongside the same guard in resume() by commit 6abf57e to keep fd-slicer/yauzl/extract-zip working. The resume() divergence stays; only pause() is brought back to upstream Node 26 behavior. Two new tests are added to node-stream.test.js and the vendored test-stream-destroy.js un-gates its pause() assertion.
Security risks
None. This is purely observable stream-state semantics (readableFlowing / isPaused() after a completed pipe). No auth, crypto, filesystem, or network trust boundaries are involved.
Level of scrutiny
Medium-high. The runtime change is a single 3-line early return that exactly matches upstream Node, and the verification is unusually thorough (truth table on Node v26.3.0, both new tests run verbatim on real Node, the existing fd-slicer regression test still passes, 700+ vendored stream/http/net tests show no new failures). However, Readable.prototype.pause() runs on every unpipe() of every completed pipe, and the original guard removal was a deliberate ecosystem-compat divergence. The argument that the pause() half of that removal was not load-bearing is convincing but subtle enough that whoever owns the streams-compat story should sign off.
Other factors
- Bug hunter found nothing.
- No CODEOWNERS coverage for this path.
- Tests follow harness conventions (await observable conditions via
'unpipe'/'close', no sleeps, cover autoDestroy on and off plus the resume-only control). - The fd-slicer regression test in the same describe block is the load-bearing safety net for the original divergence and is untouched.
Agreed that's the thing to check, so here is the evidence rather than the argument. The guard combination is not held together by reasoning, it is pinned by tests that fail in opposite directions: Restoring the Removing the So a future change cannot quietly move either half without a red test. That is what makes the split safe to carry. Why the
|
| variant | bytes delivered |
|---|---|
| both guards (stock Node 26) | 131072 / 171072 (truncated) |
pause() guard only |
131072 / 171072 (truncated) |
resume() guard removed |
171072 / 171072 |
| both guards removed (Bun before this PR) | 171072 / 171072 |
Rows 1 and 2 are identical, which is the whole point: adding back the pause() guard does not move the fd-slicer outcome.
Additional verification since opening
- 278 vendored
test-whatwg-webstreams*/test-webstream*/test-http2-*tests, run on this branch and on a baseline debug build ofmain. Failing sets are identical excepttest-http2-pipe.js, which is flaky onmain(5/8 pass without this change) and passed 20/20 with it. Bun.Cookie'sExpiresserialization is also reachable frompause()-free code paths, so to be explicit: the redtest/js/bun/cookie/cookie-map.test.tslane is unrelated.028f210723switched the output to an IMF-fixdate (Thu, 01 Jan 1970 00:00:00 GMT) while the test still expectsFri, 1 Jan 1970 00:00:00 -0000, which is not even the correct weekday for the epoch. It reproduces on a clean checkout and on unrelated branches' builds.- The
binary-sizeannotation reports +0.0 KB on every target, as expected for a JS-only change.
5991fb8 to
7daed39
Compare
|
Correction to my previous note on the red Rebased onto current main. The only commit that pulls in is 48ff9eb, which touches |
![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/test/parallel/test-stream-destroy.js`:
- Around line 122-137: This test file section diverges from the upstream Node.js
version which reverted the destroyed-stream pause()/resume() no-op changes and
removed the associated tests. Remove the first pause() test block entirely (the
one without the Bun condition check). For the resume() test block that is
wrapped in the if (!('Bun' in globalThis)) condition, move this Bun-specific
test coverage to a Bun-owned test or expectations file instead of keeping it in
this Node mirror file, as it represents deliberate Bun divergence from Node
behavior.
🪄 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: 6abf0401-da35-4153-a4bf-3bd6e3e9fc48
📒 Files selected for processing (3)
src/js/internal/streams/readable.tstest/js/node/stream/node-stream.test.jstest/js/node/test/parallel/test-stream-destroy.js
….pause
A completed pipe unpipes its source, and Readable.prototype.unpipe calls
source.pause(). Node's pause() early-returns on a destroyed stream, so a
source that autoDestroy'd itself at 'end' keeps readableFlowing === true
and isPaused() === false. Bun paused it instead, so the post-pipe readable
state diverged on every completed pipe:
const b = new PassThrough();
b.pipe(new Writable({ write(c, e, cb) { cb(); } }));
b.end("x");
setTimeout(() => console.log(b.readableFlowing, b.isPaused()), 150);
// node: true false
// bun: false true
6abf57e dropped the guard from both pause() and resume() so legacy
fd-slicer-style readables (yauzl, extract-zip, puppeteer), which assign
`this.destroyed = true` right before push(null), can still flush their
buffer when a piped destination drains. Only the resume() guard causes
that truncation: with the guard in place, pause() leaves flowing set, so
the later resume() sees flowing already true and never reschedules flow().
Restoring the guard in pause() alone keeps fd-slicer working and brings
the state flags back in line with Node.
Verified against Node v26.3.0: pipe-to-completion, pipe with
autoDestroy: false, manual unpipe of a live source, and pause()/resume()
on a destroyed stream now all report identical flags.
0d4c18a to
e25520e
Compare
|
Status: the diff is green on every lane it touches. Build #69338 finished with 284 passed / 2 failed, both macOS-only and both unrelated to
This PR's own surface is confirmed green: Leaving the two macOS lanes for a maintainer rather than pushing another retrigger. Happy to rebase again if a fresh main would re-roll them. |
## Repro
```js
import { Readable } from "node:stream";
const src = Readable.from([Buffer.from("a"), Buffer.from("b")], { objectMode: false });
const rd = Readable.toWeb(src).getReader();
for (;;) { const { done } = await rd.read(); if (done) break; }
await new Promise(r => setTimeout(r, 30));
console.log(`flowing=${src.readableFlowing} isPaused=${src.isPaused()}`);
```
```
node v26.3.0: flowing=false isPaused=true
bun 1.4.0: flowing=true isPaused=false
```
`readableEnded` / `destroyed` agree; only `readableFlowing` /
`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()` 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 late `pull()`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 = true`
right before `push(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-EOF `resume()` flips `readableFlowing` back to `true`.
## Fix
Narrow the `resume()` guard to `kDestroyed && kEndEmitted` instead 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, so `resume()` can become a no-op and keep
state parity with Node.
`pause()` is left unchanged; #33467 addresses the mirrored post-pipe
divergence on the `pause()` side.
## Verification
- New test `Readable.toWeb leaves the source paused / non-flowing after
EOF` fails on main (`readableFlowing: true, isPaused: false`) and passes
with this change; the same test body passes on Node v26.3.0.
- The existing fd-slicer regression test (`drain still resumes a source
that flagged itself destroyed before EOF`) still passes.
- `test/js/node/stream/` and the vendored `test-stream-*.js` parallel
suite: no new failures relative to main.
<!-- robobun:evidence:begin -->
---
**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
<!-- robobun:evidence:end -->
2 participants
Repro
The post-pipe readable-state flags diverge on every completed pipe. Code that inspects
isPaused()/readableFlowingto decide whether to re-resume()a reusable source concludes the wrong thing on Bun.Cause
A completed pipe unpipes its source, and
Readable.prototype.unpipecallssource.pause(). Node'spause()early-returns when the stream is destroyed, so a source that autoDestroy'd itself at'end'keepsreadableFlowing === true. Bun'spause()has no such guard, so it pauses the already-destroyed source.The guard was removed from both
pause()andresume()in 6abf57e, so that legacy fd-slicer-style readables (yauzl → extract-zip → puppeteer) keep working. Those assignthis.destroyed = trueright beforepush(null), which on modern streams hits the prototype setter; with the upstream guard, a piped destination's'drain'can no longer resume the source and the last buffered chunk is dropped.Only the
resume()guard causes that truncation. Withpause()guarded,pause()leaveskFlowingset, soresume()'sif (flowing === 0)check is skipped andflow()is never rescheduled. Emulating each variant on Node v26.3.0 with the fd-slicer pattern:pause()guard onlyresume()guard only removedSo
pause()was over-reverted: its guard is not load-bearing for fd-slicer.Fix
Restore
if ((state[kState] & kDestroyed) !== 0) return this;inReadable.prototype.pauseonly.resume()keeps the documented divergence.Verification
Every flag now matches Node v26.3.0:
flowing=true paused=falseflowing=false paused=trueflowing=true paused=falseautoDestroy: falseflowing=false paused=trueflowing=false paused=trueflowing=false paused=trueunpipe()of a live sourceflowing=false paused=trueflowing=false paused=trueflowing=false paused=truepause()afterdestroy()flowing=true paused=falseflowing=false paused=trueflowing=true paused=falseBoth new test bodies in
test/js/node/stream/node-stream.test.jswere run verbatim against real Node v26.3.0 and pass there unchanged, so the expectations are not hand-derived.test/js/node/stream/node-stream.test.js: two new tests (pause()no-op on destroyed; post-pipe state for autoDestroy on/off). Both fail onmain, pass with this change. The existing fd-slicer regression test still passes.test/js/node/test/parallel/test-stream-destroy.js: the vendored upstreampause()assertion no longer needs the Bun gate, so it now runs.test-stream-*.js, 493 vendoredtest-http-*/test-net-*/test-readline*/test-pipe*: no new failures (the handful that fail do so identically on a baseline debug build ofmain).