child_process: honor child.stdout.pause() once the stream has flowed - #36035
Conversation
|
Warning Review limit reached
Next review available in: 17 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 (1)
WalkthroughChangesThe update coordinates native stream backpressure with reader lifecycle handling, including re-entrant closure safeguards, revised POSIX and Windows buffering behavior, native flowing toggles, FileReader pull gating, and a child-process stdout regression test. Native stream backpressure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:17 AM PT - Jul 27th, 2026
✅ @robobun, your commit 922288b3a87a42bfc45414de7fbdca536780a786 passed in 🧪 To try this PR locally: bunx bun-pr 36035That installs a local version of the PR into your bun-36035 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
d254877 addresses both prior findings (the Windows stop_reading() leak and the follow-up take_read_ref() UAF) by reverting both and letting the JS-side setFlowing(false) path pause the reader from outside the uv callback instead; the test nit is also picked up. No new issues found this pass. Deferring to a human because this is a three-layer change across both platforms' hot stdio path, the Windows backpressure now hinges on Readable's maybeReadMore→second push()===false rather than the native hwm return, and CI hasn't reported yet.
What was reviewed:
- Unconditional
_buffer.clear()inWindowsBufferedReader::on_readvsFileResponseStream::on_read_chunk's Backpressure/RESPONSE_DONE arms —resp.writecopies and the arm already self-pauses, so no dangling slice / no re-leak. - POSIX
has_pending_read()→is_watching()pairs withon_read_chunkreturningfalseat hwm soon_pullre-arms viareader().read(). setFlowing(true)placement before thekPendingReadearly-return in_read()so a paused reader with an outstanding pull promise still un-pauses.- The new test's
c.on('close', failFirst)firing after resume — rejects an already-resolved promise, harmless.
Extended reasoning...
Overview
Touches src/io/PipeReader.rs (both POSIX and Windows readers), src/runtime/webcore/FileReader.rs (native-side highwater backstop), src/js/internal/streams/native-readable.ts (propagate push()===false → setFlowing(false)), and adds a test in child_process.test.ts. The goal is to make child.stdout.pause() actually stop the native reader once the stream has flowed, so kernel backpressure blocks the child instead of the parent buffering everything.
Prior findings resolved
My first pass flagged that WindowsBufferedReader::on_read calling stop_reading() on should_continue==false would leak FileResponseStream on client abort (its RESPONSE_DONE early-return relied on read-to-EOF). The author's cdbe688 fix added take_read_ref() there, which my second pass showed drops the refcount to 0 inside on_read_chunk — a UAF against the caller. d254877 resolves both by removing the stop_reading() call from on_read entirely and reverting the FileResponseStream change; Windows backpressure now comes from the JS side (pushAndCheck → setFlowing(false) → reader().pause()), which runs from a microtask outside the uv callback so it cannot free the parent under the caller.
What I checked this pass
The remaining Windows change is that _buffer.clear() is no longer gated on should_continue. I checked FileResponseStream::on_read_chunk: on Backpressure it calls resp.write(chunk) (copies into uWS) and self.reader.pause() before returning false, and on RESPONSE_DONE it just returns false and lets the pre-existing read-to-EOF fallthrough fire on_reader_done — clearing _buffer after either return doesn't strand data or change lifetime. On POSIX, the is_watching() change in has_pending_read() is load-bearing: after on_read_chunk returns false at hwm, read_with_fn returns without re-arming, and the next on_pull must see has_pending_read()==false to call reader().read() — with is_registered() it would have deadlocked.
Security / scrutiny
No security surface. Scrutiny is high: this is the shared BufferedReader used by child_process stdio, Bun.serve file responses, and shell IO on both platforms, with hand-rolled intrusive refcounting and an explicit "on_read_chunk never frees" contract that two earlier revisions violated. Windows behavior is untested locally (author is on POSIX; CI build #83071 pending).
Other factors
The Windows path now depends on Node Readable semantics: after the user pauses inside the first 'data' handler, push() returns true (buffer is empty), so setFlowing(false) doesn't fire yet; maybeReadMore schedules a second _read(), whose push() buffers (not flowing) and returns false, and that triggers setFlowing(false) → uv_read_stop. This is one more chunk than POSIX (which stops at the native hwm), which the test tolerates (bytes < SIZE, eventsAfterPause === 0 since chunk 2 is buffered not emitted). It looks correct but is subtle enough that I'd like a maintainer to confirm and see Windows CI green before this lands.
|
CI build #83071 has four failures, all tagged
The Windows |
|
@robobun rebase or close |
FileReader.on_read_chunk exempted pollable fds from the highwater-mark backstop, so once the first _read() started the native reader it re-armed the poll forever and self.buffered grew unbounded. The Node Readable bridge in native-readable.ts also never propagated push()===false back to the source, so even setFlowing(false) (which unregisters the poll) was never sent. Apply the hwm cap to pollable sources; teach has_pending_read() that a fired-not-rearmed one-shot poll is not pending so on_pull re-arms on the next pull; on Windows stop uv_read when the streaming parent says stop; and wire push()->setFlowing(false) / _read()->setFlowing(true) in the NativeReadable bridge, matching net.Socket's readStop/readStart.
…ached Non-lazy Bun.spawn eagerly starts the pipe reader before JS touches the stream, so applying the cap in that window stalls a child that writes to both stdout and stderr while the test only awaits one of them. Also release FileResponseStream's in-flight read ref in the RESPONSE_DONE early return now that returning false stops the Windows reader.
…ref change Calling stop_reading() from inside the uv read callback could free a parent (FileResponseStream on abort) whose caller still dereferences this after return. Parents that want to pause call reader().pause() themselves; for FileReader the JS-side setFlowing(false) path does that from a microtask, outside the callback. Keep the _buffer.clear() unconditional so a parent that says stop does not double-buffer. Also wire error/close to reject firstData in the new test.
d254877 to
d7e12b1
Compare
|
Rebased onto 6c12afd (main now includes #35977, which made the same Verified locally after rebase: |
… watch() has_pending_read() using is_watching() means a re-entrant on_pull reached via the microtask drain inside p.run() can call reader().read() and hit EOF; the outer loop would then re-read the captured fd. Re-check reader().is_done() after p.run(). The trailing watch() in on_pull is now a guaranteed no-op (the state it handled is covered by the !has_pending_read() branch above) and its comment described the old semantics.
There was a problem hiding this comment.
Beyond the inline finding: the Windows _buffer.clear() change (dropping the should_continue gate) is safe for the other streaming parents — they consume slice synchronously and is_streaming_enabled() still protects accumulating parents. The JS-initiated setFlowing(true) → reader().read() re-entrancy path is covered by d204caa because PosixBufferedReader::read() passes received_hup=false; only the poll-callback HUP entry is uncovered.
Extended reasoning...
This is the fourth review pass. The new inline finding (HUP bypass of the d204caa guard) is the same re-entrancy class as the previous round, on the one entry path where read_with_fn's && !received_hup deliberately ignores a false return. I checked the two other things d204caa touched or left in place: the unconditional Windows _buffer.clear() does not regress shell IOReader / FileResponseStream (both consume the slice inside the callback), and the new setFlowing(true) call ahead of the kPendingRead gate cannot hit the HUP variant because the JS-initiated read() hard-codes received_hup=false. Nothing else new to flag.
…the reader The HUP-gated on_read_chunk callsites ignored a false return when received_hup was set (to let the shell PipeReader drain to bytes_read==0), so the is_done() guard in FileReader was insufficient there: a nested on_pull that read to EOF and handed the fd to the async closer left the outer loop to recv on the captured fd. Check parent.is_done() after each of those calls and return unconditionally. Also fix the NativeReadable.push type annotation to => boolean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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 (1)
src/io/PipeReader.rs (1)
1063-1074: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the re-entrancy guard before
continue.on_read_chunkcan close the reader, so re-checkparent.is_done()here before the loop reuses the capturedfdon the next syscall.🤖 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/io/PipeReader.rs` around lines 1063 - 1074, In the streaming branch of the read loop, after `on_read_chunk` returns and before `continue`, re-check `parent.is_done()` and return immediately when the reader was closed. Keep the existing buffer clearing and `keep_going` handling unchanged, ensuring the captured file descriptor is not reused after closure.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/io/PipeReader.rs`:
- Around line 1063-1074: In the streaming branch of the read loop, after
`on_read_chunk` returns and before `continue`, re-check `parent.is_done()` and
return immediately when the reader was closed. Keep the existing buffer clearing
and `keep_going` handling unchanged, ensuring the captured file descriptor is
not reused after closure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24577d7d-5bc3-448b-8d5a-5a278b108535
📒 Files selected for processing (4)
src/io/PipeReader.rssrc/js/internal/streams/native-readable.tssrc/runtime/webcore/FileReader.rstest/js/node/child_process/child_process.test.ts
There was a problem hiding this comment.
No new findings on 922288b; all prior inline comments are addressed. Deferring to a human — this reworks re-entrancy and fd-lifetime handling in the POSIX/Windows read loops and changes has_pending_read() semantics for every BufferedReaderParent, which is more than I'm comfortable auto-approving.
What was reviewed:
has_pending_read()→is_watching(): traced the new re-entranton_pull → reader().read()path; the fiveparent.is_done()guards now cover every read-loop continuation site including the HUP-bypass arms.FileReader::on_read_chunkhwm backstop: the!startedexemption preserves eager buffering for non-lazyBun.spawn(stderr-first-read deadlock repro reported passing).- Windows
on_read: droppingshould_continuefrom the_buffer.clear()gate — checked that non-streaming parents are excluded byis_streaming_enabled()and streaming parents already own/copy the chunk before returning. native-readable.ts:setFlowingis optional-chained so non-FileReader native ptrs are unaffected;pushtype updated.
Extended reasoning...
Overview
Four files: src/io/PipeReader.rs (POSIX has_pending_read semantics + five is_done() re-entrancy guards in read_blocking_pipe/read_with_fn; Windows on_read buffer-clear gating), src/runtime/webcore/FileReader.rs (hwm backstop now gated on started && flowing, post-p.run() guard extended to reader().is_done(), dead watch() block removed), src/js/internal/streams/native-readable.ts (pushAndCheck → setFlowing(false) on push()===false, _read → setFlowing(true)), and a new cross-platform test in child_process.test.ts.
Security risks
None user-facing. The risk class here is memory safety: the has_pending_read() change opens a nested read_with_fn on a captured local fd while the outer frame is still on the stack, and the async Closer::close on the WorkPool means a stale-fd read races a threadpool close. The five parent.is_done() guards were added over three review rounds specifically to close that; I believe they now cover every continuation site, but the fact that it took three rounds to enumerate them is exactly why a human should confirm.
Level of scrutiny
High. This is core I/O plumbing shared by every BufferedReaderParent (shell PipeReader, FileResponseStream, subprocess readers, FileReader), on both platforms, with re-entrant JS in the middle of the read loop. The has_pending_read() contract change is observable to every caller, and the Windows should_continue drop changes behavior for any streaming parent that returns false.
Other factors
All four of my earlier inline findings (nested-read UAF, HUP-bypass variant, dead watch(), fifth sibling site) were addressed in d204caa/8a60853/eb220c52/922288b. CI #83071 was green modulo pre-existing flakes; no CI result posted yet for the last two commits. Jarred asked for rebase-or-close and it was rebased onto #35977; no human sign-off on the merged shape yet.
|
CI build #83264 (922288b, rebased onto main with #35977): two failures, both tagged
Neither touches the files this PR changes. The new |
2 participants
Problem
child.stdout.pause()fromnode:child_processis ignored once the stream has started flowing:'data'keeps firing whileisPaused()===true, the child is never throttled, and the parent buffers the child's entire output.#34971 covered the "paused before any read" case (the lazy reader stays paused until the first
_read()), but once_read()has run once the native side never stops.Cause
Two layers:
FileReader::on_read_chunkcomputed keep-reading as!(buffered >= hwm && !pollable), which is always true for a pollable fd, so after the first pull the posix read loop re-armed the poll forever andself.bufferedgrew without bound.internal/streams/native-readable.tsnever propagatedpush()===falseback to the source. ThesetFlowing(false)hook (which unregisters the FilePoll / callsuv_read_stop) already existed, but onlyprocess.stdincalled it.Fix
FileReader::on_read_chunk: drop the pollable exemption so the highwater mark caps native buffering for pipes too. The cap only engages onceon_starthas run (a consumer has attached); a non-lazyBun.spawnreader that is already delivering before anyone reads keeps its old eager-buffer behavior so it cannot deadlock a child that writes to both stdout and stderr while the caller only awaits one of them.PosixBufferedReader::has_pending_read(): useis_watching()instead ofis_registered(). A one-shot poll that has fired but not been re-armed will not deliver another callback, soon_pullmust not be told a read is in flight (it would wait on a poll that never fires).WindowsBufferedReader::on_read: clear_bufferregardless ofshould_continueso a FileReader that says stop at hwm does not also leave_buffergrowing. Parents that want the reader paused callreader().pause()themselves; for FileReader that is the JS-sidesetFlowing(false)path from a microtask.native-readable.ts:push()===false->ptr.setFlowing(false);_read()->ptr.setFlowing(true). Same readStop/readStart model as Node'snet.Socket.Verification
The new
child.stdout.pause() after flowing stops native reads and blocks the childtest inchild_process.test.tspauses a 20 MB writer after the first'data', asserts the child is still blocked with zero extra events, then resumes and counts every byte. Fails on main (exitCode: 0, child finished), passes here on both POSIX and Windows.Related: #35977 fixes the same
FileReaderroot for theprocess.stdinface (blocking-pipe path) and will conflict on that file; whichever lands first, the other rebases cleanly.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/child_process/child_process.test.ts