io(posix): do not re-arm the fd poll after a re-entrant process.stdin.pause() - #35975
io(posix): do not re-arm the fd poll after a re-entrant process.stdin.pause()#35975robobun wants to merge 6 commits into
Conversation
process.stdin.pause() called from inside a 'data' handler (as readline's async iterator does via the events.on 1024-line highWaterMark) unregistered the FilePoll and set IS_PAUSED, but control then returned to the PosixBufferedReader read loop, which called register_poll() on EAGAIN and re-armed the fd. The native reader kept draining the pipe into FileReader.buffered with no bound, so a drain-throttled writer could push the whole input through while the consumer was paused: for 100 MB piped stdin with a slow async-iterator consumer, node pauses stdin ~95 times and stays at ~85 MB RSS while bun paused ~3 times and buffered everything (peak RSS ~2.5x input). register_poll() now returns early when IS_PAUSED is set so the loop's own re-arm cannot undo a re-entrant pause; unpause() + read() re-register when JS resumes. on_read_chunk also reports 'stop' after p.run() when re-entrant JS cleared flowing, so the inner read loop exits promptly instead of continuing until EAGAIN.
WalkthroughThe change updates paused-read handling in the POSIX pipe reader and file reader, then adds a concurrent child-process test that verifies stdin writes stop progressing under kernel backpressure after pausing inside a data handler. ChangesStdin pause backpressure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:01 PM PT - Jul 26th, 2026
❌ @robobun, your commit a3373d3 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35975That installs a local version of the PR into your bun-35975 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Status at a3373d3: Fix verified locally on both profiles. Clean release build succeeds and all 15 Gate: three of four checks prove the fix (ASAN without fix: fail, release without fix: fail, ASAN with fix: pass). CI (builds 82627/82680/82695/82719): the new test passed on every lane. Remaining red is unrelated:
Ready for review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found. The two-line native change looks correct and I traced the other register_poll() / IS_PAUSED consumers (lazy subprocess stdio, spawnSync watch loop, FileResponseStream) and none regress — but this is shared POSIX I/O plumbing that every pipe reader flows through, so it's worth a human look, and CI hasn't reported yet.
What was reviewed:
register_poll()early-return: spawnSync's per-tick.watch()never seesIS_PAUSED(start passeslazy = !IS_SYNC && lazy); the lazy async-spawn path clears it viaon_start()'sunpause()before reading;FileResponseStream'spause()calls are#[cfg(not(unix))]so don't intersect.on_read_chunk's new!flowingstop: inread_blocking_pipe's capacity==0 streaming arm the return value is discarded, so the fix there relies on theregister_poll()gate — which it gets. The capacity>0 arm andread_with_fnhonor the return.- Resume path re-registers correctly (
set_flowing(true)→unpause()clears the flag →read()reachesregister_poll()). - Test: threshold (2000 lines ≈ 2 MB) is well above any platform's pipe+HWM buffer;
bunEnvpropagates to the grandchild sostderrshould stay empty in debug.
Extended reasoning...
Overview
Two small native changes plus a regression test:
PosixBufferedReader::register_poll()returns early (as success) whenIS_PAUSEDis set, so the read loop's own re-arm on EAGAIN cannot undo a re-entrantpause().FileReader::on_read_chunknow returnsfalseafterp.run()when re-entrant JS clearedflowing, so the read loop exits promptly instead of continuing to EAGAIN.- New
test.concurrentinprocess-stdin.test.tsthat pumps 10 MB into a child that pauses on the first'data'and asserts the drain-throttled writer stalls (< 2000 lines vs 10000 without the fix).
Security risks
None. This is backpressure/flow-control logic on an already-open fd; no new input parsing, no auth, no bounds arithmetic.
Level of scrutiny
High. PosixBufferedReader is shared plumbing for subprocess stdout/stderr, shell IOReader, Bun.serve file streaming, and stdin. I traced every consumer that touches IS_PAUSED or calls register_poll()/watch():
- spawnSync watch loop (
js_bun_spawn_bindings.rs:1936/1940):start()is called withlazy = !IS_SYNC && lazy, soIS_PAUSEDis never set on the sync path —.watch()still registers. - Lazy async subprocess stdio (
SubprocessPipeReader.rs:194setsIS_PAUSED): the flag is transferred into aFileReaderviafrom_pipe, andFileReader::on_start()explicitly callsunpause()beforeread(), so the new gate is not reached with the flag set. - FileResponseStream
pause()calls are#[cfg(not(unix))], so they don't touch the POSIXregister_poll(). - Resume:
set_flowing(true)→unpause()clearsIS_PAUSED→read()→register_poll()re-arms. Theread()path for blocking pipes handles the NotReady case by callingregister_poll()directly.
I also verified the fix actually covers the bug path: for the streaming blocking-pipe case (_buffer.capacity() == 0), on_read_chunk's return value is discarded (let _), so change (2) alone doesn't help there — but the subsequent register_poll() is now a no-op via change (1), so the loop exits without re-arming. In the capacity>0 arm and in read_with_fn, change (2) short-circuits earlier. The HUP drain paths (&& !received_hup guards) still drain to EOF, which is bounded.
Other factors
- The PR description's mechanism explanation is precise and matches the code.
- CI build #82627 is still running; no test results yet.
- The test uses timing-based stall detection (3×100 ms idle ticks) rather than a hard sleep, and the 2000-line threshold gives ~10× headroom over the expected ~150-300 lines on Linux/macOS/Windows pipe buffers, so it should be robust — but a maintainer may want to eyeball the Windows pipe-buffer margin.
Given this is a subtle re-entrancy fix in shared native I/O code, deferring to a human reviewer.
The stall detector previously started immediately after spawn, so on slow builds it observed the pipe-buffer stall that happens before the child has even attached its 'data' listener, not the pause-backpressure stall, and the test passed without the fix. The child now prints PAUSED after calling pause() and the writer only begins stall detection once that arrives.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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/process/process-stdin.test.ts`:
- Around line 446-470: Update the stdin error handling in the test around the
pump and stall-detection logic: reject or otherwise fail the test for unexpected
stdin errors before the deliberate child.kill() path begins, while allowing
errors caused by that intentional shutdown. Ensure the test’s promise/assertion
flow observes this failure so early child exit or EPIPE cannot be masked, and
retain the existing stall result reporting.
🪄 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: 77f62fa9-d7d6-4670-b817-b98bc74ff36c
📒 Files selected for processing (3)
src/io/PipeReader.rssrc/runtime/webcore/FileReader.rstest/js/node/process/process-stdin.test.ts
There was a problem hiding this comment.
No issues found, but this touches PosixBufferedReader::register_poll() — the shared re-arm path under every POSIX pipe/tty/socket reader — so it's worth a human look from someone who owns the I/O layer.
What was reviewed:
- Traced
IS_PAUSEDon POSIX: onlyFileReader::set_flowing(false)sets it (FileResponseStreampauses only oncfg(not(unix)); subprocess onlyunpause()s), so the new early-return's blast radius is FileReader-backed readers. - Resume path:
set_flowing(true)→unpause()clears the flag →read()reachesregister_poll()/read_with_fnwith the guard inactive, so the poll re-arms. read_blocking_pipe's cap==0 streaming arm ignoreson_read_chunk's return — theregister_poll()guard is what actually stops re-arming there; the!flowingreturn inFileReadercovers the other loops.- Test: prior grandchild-orphan and pre-kill stdin-error concerns are addressed; stall detector converges on both fixed and unfixed builds.
Extended reasoning...
Overview
Two-line native fix plus a subprocess test. PosixBufferedReader::register_poll() gains an IS_PAUSED early-return so a re-entrant process.stdin.pause() (reached via on_read_chunk → p.run() → microtasks → 'data' handler → set_flowing(false)) is not immediately undone by the read loop's own EAGAIN/tail re-arm. FileReader::on_read_chunk additionally reports "stop" when !flowing after p.run(), so the inner read loops that do honour the return value exit promptly instead of spinning to EAGAIN. The new process-stdin.test.ts case pumps 10 MB into a grandchild that pauses on the first chunk and asserts the writer stalls (< 2000 lines vs. 10000 unfixed).
Security risks
None. This is a backpressure/resource-usage fix on a local fd; no parsing of untrusted input, no auth/crypto, no new surface.
Level of scrutiny
Medium-high. The diff is tiny and the mechanism is well-argued, but register_poll() is the single re-arm choke point for every PosixBufferedReader (process.stdin, subprocess stdout/stderr after to_readable_stream, shell PipeReader, Bun.file().stream()). I verified that on POSIX IS_PAUSED is only ever set from FileReader::set_flowing(false) (the FileResponseStream reader.pause() calls are #[cfg(not(unix))], and subprocess only calls unpause()), so in practice the guard is inert for shell/subprocess non-stream readers. The resume path (set_flowing(true) → unpause() → read()) re-registers because IS_PAUSED is cleared before read() reaches register_poll(). Still, this is exactly the kind of shared-helper change REVIEW.md flags for "enumerate every consumer", and a maintainer who owns src/io/ should confirm no lazy-start path relies on register_poll() firing while paused.
Other factors
- All prior review feedback is addressed: comment-cop (comments shortened), CodeRabbit (pre-kill stdin errors now throw), and my earlier nit (grandchild keep-alive bounded to 30 s with
exit(1)). - CI: the new test passed on every lane per the robobun status; remaining red is unrelated flakes/binary-size.
- Windows is untouched by design (
on_file_readalready checksIS_PAUSED; pipe/tty useuv_read_stop). - One thing I did not exercise: whether any consumer relies on a pending-read
Drainedchunk afterregister_poll()inread_with_fn's EAGAIN arm — thereregister_poll()runs before the trailingon_read_chunk(.., Drained), so a pause during that final chunk still unregisters viapause()itself; the new guard only matters whenon_read_chunkruns beforeregister_poll, which is theread_blocking_pipeand mid-buffer-flush paths.
|
@robobun fix conflicts. |
|
Superseded by #35977, which landed the same Closing; nothing left to merge. |
2 participants
Problem
for await (const line of readline.createInterface({input: process.stdin}))applies no backpressure on Bun: the whole piped input is buffered in native memory. With a 100 MB input and any per-line async work, Node pauses stdin ~95 times and stays flat at ~85 MB RSS; Bun pauses ~3 times and buffers everything (peak RSS ~2.5x input, scaling linearly, so big pipes OOM).More directly, a child that calls
process.stdin.pause()from inside its first'data'handler does not block the writer:fs.createReadStream(file)is bounded; this is stdin-specific.Cause
FileReader::on_read_chunkresolves the pending JS read, which drains microtasks and runs the'data'handler synchronously. When that handler callsprocess.stdin.pause(), the nextTickdisown()path reachesFileReader::set_flowing(false)which callsPosixBufferedReader::pause():IS_PAUSEDis set and theFilePollis unregistered.Control then returns to
PosixBufferedReader::read_with_fn/read_blocking_pipe, whose EAGAIN path callsregister_poll()unconditionally. The poll is re-armed, undoing the pause. The next readable event fires,on_read_chunkfinds no pending promise and appends toFileReader.buffered(returning "keep reading" for pollable fds), and the cycle repeats until EOF, buffering the entire input natively.Windows is not affected:
on_file_readalready checksIS_PAUSEDbefore re-arming and the pipe/tty path relies onuv_read_stop().Fix
PosixBufferedReader::register_poll()returns early whenIS_PAUSEDis set, so a read loop's own re-arm cannot undo a re-entrantpause().unpause()followed byread()re-registers when JS resumes.FileReader::on_read_chunkreports "stop" afterp.run()when re-entrant JS clearedflowing, so the inner read loop exits promptly instead of continuing until EAGAIN.Verification
The new test pumps 10 MB via drain-throttled
child.stdininto a child that pauses stdin on the first chunk, then asserts the writer was blocked (writtenWhilePaused < 2000). On main the writer drains all 10000 lines.[review] gate passed · iteration 3 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 3 rejected · iteration 3
evidence per changed file