stdin: apply highwater backpressure to the pipe FileReader source - #35977
Conversation
FileReader.on_read_chunk exempted pollable fds from the highwater
backstop ("for pipes we have to keep pulling"), so with no JS read
pending the posix read loop kept draining the fd into self.buffered
until EAGAIN. A single Bun.stdin.stream() read followed by an idle
consumer ingested the entire pipe; process.stdin.pause() stopped
delivery to JS but not the fd ingestion (RSS grew to match the feed).
Drop the pollable exemption and fold !flowing into the continue
decision. Make the blocking-pipe read path honor that return value
(the nonblocking path already did), and have register_poll / on_poll
respect IS_PAUSED so a pause issued from inside the read loop's JS
re-entry is not immediately undone. on_pull re-arms the poll when it
returns Pending, since the one-shot poll may be kernel-disarmed while
has_pending_read() still reports the sticky registration flag.
|
Warning Review limit reached
Next review available in: 15 seconds 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 (3)
Comment |
|
Updated 3:12 PM PT - Jul 26th, 2026
❌ @robobun, your commit a772e2f has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35977That installs a local version of the PR into your bun-35977 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Checked #30189: that reproduction ( |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Overlaps with #35975 (opened a few minutes earlier from the same analysis). The #35975 fixes the explicit- This PR additionally drops the pollable exemption in the no-pending path, which is what fixes the zero-demand case ( const rd = Bun.stdin.stream().getReader();
await rd.read();
await new Promise(r => setTimeout(r, 1500));
// RSS grows to match the feed under #35975; stays flat hereThe other differences here ( Happy to rebase either direction. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
on_pull's watch() re-arm and the read loop's own register_poll both land on the same one-shot poll per chunk in the streaming path, so register_with_fd was issuing an idempotent epoll_ctl(CTL_MOD) / kevent64 twice. Skip it when is_watching() already holds.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… watch register_poll is called from the read loop's EAGAIN path, whose epoll_ctl count the shell-pipe-read-fault tests fault-inject against. watch() is the setup/restart path (FileReader.on_pull, subprocess bring-up); skipping the redundant epoll_ctl there covers the per-chunk on_pull re-arm without shifting the read loop's call pattern.
|
CI on a772e2f (build #82683): all test lanes green. The only red is Ready for review. |
…5977) ## Reproduction ```js const rd = Bun.stdin.stream().getReader(); await rd.read(); // one read const base = process.memoryUsage().rss; await new Promise(r => setTimeout(r, 1500)); // idle, zero demand console.log(Math.round((process.memoryUsage().rss - base) / 1048576)); ``` With 40 MB piped in: RSS grows by ~100 MB while the consumer is idle. The `process.stdin.pause()` variant is the same: `'data'` delivery stops but the fd keeps being ingested into an internal buffer. Node stays flat at the kernel pipe buffer. ## Cause `FileReader.on_read_chunk` tells the read loop whether to keep reading. When no JS read is pending the chunk is appended to `self.buffered` and the continue decision was ```rust !(buffered_len >= highwater_mark && !reader_is_pollable()) ``` so for a pollable fd the highwater backstop was exempted and the loop always continued. The posix pipe read loop drains the fd until EAGAIN, and the blocking-pipe path (`read_blocking_pipe`, which stdin-from-a-pipe uses because fd 0 is not O_NONBLOCK) additionally discarded the return value of `on_read_chunk` and re-armed the poll unconditionally. `pause()` set `IS_PAUSED` and unregistered the poll, but the in-flight read loop's own `register_poll()` did not check the flag (so a pause issued from inside the loop's JS re-entry was immediately undone), and `on_poll` did not check it either. ## Fix - `FileReader.on_read_chunk`: drop the pollable exemption and fold `!flowing` into the continue decision. A full kernel pipe buffer blocking the writer is backpressure. - `PosixBufferedReader::register_poll` / `on_poll`: early-return when `IS_PAUSED`. - `read_blocking_pipe` stack-buffer streaming arm: honor the `on_read_chunk` return value the way the with-buffer arm and `read_with_fn` already do. - `FileReader.on_pull`: when returning `Pending`, re-arm the poll. After the backstop returns without re-arming, the one-shot poll is disarmed kernel-side while `has_pending_read()` (which checks the sticky `PollReadable` flag, not the arm state) still reports true, so the next pull would otherwise wait on a poll that will never fire. `onPull` and `setFlowing(true)` already restart the reader on demand. Only `FileReader` ever returned `false` with `received_hup == false` from `on_read_chunk`, so the `read_blocking_pipe` change does not affect shell/subprocess/Terminal readers. ## Verification `test/js/node/process/process-stdin.test.ts` adds three cases: a single `Bun.stdin.stream()` read with a 40 MB feed stays within a few MB of the baseline, `process.stdin.pause()` after 1 MB stops fd ingestion, and reading to EOF after the backstop engaged delivers every byte. Without the source change the first two fail with ~100 MB RSS growth under `bun bd`. <!-- robobun:evidence:begin --> --- **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/process/process-stdin.test.ts <!-- robobun:evidence:end -->
2 participants
Reproduction
With 40 MB piped in: RSS grows by ~100 MB while the consumer is idle. The
process.stdin.pause()variant is the same:'data'delivery stops but the fd keeps being ingested into an internal buffer. Node stays flat at the kernel pipe buffer.Cause
FileReader.on_read_chunktells the read loop whether to keep reading. When no JS read is pending the chunk is appended toself.bufferedand the continue decision wasso for a pollable fd the highwater backstop was exempted and the loop always continued. The posix pipe read loop drains the fd until EAGAIN, and the blocking-pipe path (
read_blocking_pipe, which stdin-from-a-pipe uses because fd 0 is not O_NONBLOCK) additionally discarded the return value ofon_read_chunkand re-armed the poll unconditionally.pause()setIS_PAUSEDand unregistered the poll, but the in-flight read loop's ownregister_poll()did not check the flag (so a pause issued from inside the loop's JS re-entry was immediately undone), andon_polldid not check it either.Fix
FileReader.on_read_chunk: drop the pollable exemption and fold!flowinginto the continue decision. A full kernel pipe buffer blocking the writer is backpressure.PosixBufferedReader::register_poll/on_poll: early-return whenIS_PAUSED.read_blocking_pipestack-buffer streaming arm: honor theon_read_chunkreturn value the way the with-buffer arm andread_with_fnalready do.FileReader.on_pull: when returningPending, re-arm the poll. After the backstop returns without re-arming, the one-shot poll is disarmed kernel-side whilehas_pending_read()(which checks the stickyPollReadableflag, not the arm state) still reports true, so the next pull would otherwise wait on a poll that will never fire.onPullandsetFlowing(true)already restart the reader on demand.Only
FileReaderever returnedfalsewithreceived_hup == falsefromon_read_chunk, so theread_blocking_pipechange does not affect shell/subprocess/Terminal readers.Verification
test/js/node/process/process-stdin.test.tsadds three cases: a singleBun.stdin.stream()read with a 40 MB feed stays within a few MB of the baseline,process.stdin.pause()after 1 MB stops fd ingestion, and reading to EOF after the backstop engaged delivers every byte. Without the source change the first two fail with ~100 MB RSS growth underbun bd.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/process/process-stdin.test.ts