io(PipeReader): re-check HUP before looping in readBlockingPipe stdin drain - #29966
Conversation
readBlockingPipe() snapshots received_hup from the epoll/kqueue event
and then loops while(true) draining the pipe, on the assumption that a
hung-up pipe will read to EOF. But onReadChunk() re-enters JS (resolves
the pending read, drains microtasks, fires the 'data' event), and user
code there can open a new writer on the same FIFO. After that the pipe
is no longer hung up, so the next readNonblocking() can't reach EOF:
- Linux named FIFOs: preadv2(RWF_NOWAIT) returns EOPNOTSUPP (unlike
anonymous pipes), so readNonblocking() falls back to blocking
read() and the event-loop thread blocks forever.
- O_NONBLOCK fd: read() returns EAGAIN, the loop falls through the
'don't re-arm on HUP' check, and spins at 100% CPU.
Re-arm the poll on EAGAIN, and re-poll the fd after returning from JS
so a stale HUP doesn't commit us to another blocking read.
The test reproduces this deterministically: stdin is a FIFO, the
harness writes a chunk and closes its writer (POLLIN|POLLHUP together),
and the fixture's 'data' handler opens a new writer from inside the
drain loop. Without the fix the child never exits.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughDistinguishes valid vs. stale POLLHUP in Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 58 minutes. Comment |
|
Updated 6:28 PM PT - Apr 30th, 2026
❌ @Jarred-Sumner, your commit b374a12 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29966That installs a local version of the PR into your bun-29966 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found and the earlier nit is resolved — but this changes the core POSIX stdin drain loop with subtle epoll/kqueue HUP + JS-reentrancy reasoning, so it's worth a human pass before merging.
Extended reasoning...
Overview
This PR modifies PosixBufferedReader.readBlockingPipe() in src/io/PipeReader.zig — the drain loop used for process.stdin when stdin is a blocking pipe/FIFO on POSIX. The fix makes received_hup mutable, tracks whether the read returned EAGAIN, and on the HUP path either re-arms the poll (on EAGAIN) or re-checks bun.isReadable(fd) before looping back, to avoid wedging when user JS (re-entered via onReadChunk) opens a new writer on the same FIFO. Two new test files add a deterministic regression test using a named FIFO.
Security risks
None identified. This is internal IO control flow; no parsing of untrusted input, no auth/crypto/permissions. The added poll() is bounded (one per chunk, only on the HUP path).
Level of scrutiny
High. readBlockingPipe sits on the hot path for every process whose stdin is a pipe, and the reasoning hinges on platform-specific semantics (level-triggered HUP, preadv2(RWF_NOWAIT) returning EOPNOTSUPP on named FIFOs, O_NONBLOCK inheritance across the spawned child's fd 0) plus JS re-entrancy from inside the native read loop. The logic and comments are coherent and the regression test gates the fix, but this is exactly the class of subtle event-loop change a maintainer should sign off on rather than a bot.
Other factors
My prior inline nit (stale "~200ms" comment in the fixture) was addressed in ee44ba6 and the thread is resolved. The bug-hunting pass found nothing further. CI shows musl build failures and a Windows agent-creation failure on the latest commit, but those appear to be infra/build issues unrelated to this diff (no compile change since the previous green-on-those-targets state). Given the criticality of the code path, deferring rather than auto-approving.
There was a problem hiding this comment.
No issues found and the fix is well-reasoned with a solid regression test, but since this changes the core POSIX stdin/pipe drain loop (event-loop re-entry + HUP semantics) I'd like a human to sign off rather than auto-approving.
Extended reasoning...
Overview
This PR modifies PosixBufferedReader.readBlockingPipe() in src/io/PipeReader.zig — the drain loop used for process.stdin (and other blocking pipes) on POSIX. Previously the loop snapshotted received_hup once and looped while(true) assuming a hung-up pipe will eventually return EOF. The fix makes received_hup mutable, tracks whether the read returned EAGAIN (got_retry), and at the bottom of the HUP path either re-arms the poll (on EAGAIN) or re-checks bun.isReadable(fd) to detect a writer that reappeared while user JS ran inside onReadChunk. Two new test files reproduce the wedge deterministically with a named FIFO.
Security risks
None. This is purely event-loop / fd-readability control flow on already-open file descriptors; no parsing, auth, or untrusted-input handling is involved.
Level of scrutiny
High. readBlockingPipe sits on the hot path for every POSIX process that reads stdin from a pipe/FIFO, and the loop now interleaves an extra poll() and three-way branch with JS re-entry. The reasoning in the PR description and inline comments is thorough (EAGAIN ⇒ HUP provably stale; .hup ⇒ keep draining; .ready ⇒ drop stale flag; .not_ready ⇒ re-arm), and the fast path (HUP still set) is preserved. But subtle mistakes here can hang or spin every Bun process whose parent dies, so this warrants a maintainer's eyes on the state machine rather than bot approval.
Other factors
- The one nit I raised earlier (stale "~200ms" comment in the fixture) was fixed in ee44ba6 and the thread is resolved.
- The PR includes a gated regression test (
process-stdin-stale-hup.test.ts) that fails on the old binary and passes with the fix, plus verification that the existing stdin/stdio tests still pass. - The added
bun.isReadable()call only fires on the HUP path (final-drain), so the per-chunk overhead claim looks correct. - No CODEOWNERS or outstanding human review comments to consider; CI build was triggered.
… drain (oven-sh#29966) ## Problem `PosixBufferedReader.readBlockingPipe()` — the read loop used for `process.stdin` when stdin is a FIFO — snapshots `received_hup` from the epoll/kqueue event and then loops `while(true)` draining the pipe, on the assumption that a hung-up pipe will eventually read 0 (EOF). That assumption breaks because `onReadChunk()` re-enters JS: it resolves the pending read promise, which drains microtasks, which fires the Node `'data'` event **synchronously while `readBlockingPipe` is still on the stack**. If the handler opens a new writer on the same FIFO (or any code running at that point does), the pipe is no longer hung up — but `received_hup` is still `true`, and the loop commits to another read that can never reach EOF: | fd state | `readNonblocking()` result | outcome | |-|-|-| | blocking, Linux named FIFO | `preadv2(RWF_NOWAIT)` → `EOPNOTSUPP`† → fallback to blocking `read()` | **event loop thread blocks in `read()`** | | `O_NONBLOCK` | `read()` → `EAGAIN` → `isRetry()` → fall through → `!received_hup` false → loop | **100% CPU spin** | † Linux supports `RWF_NOWAIT` on anonymous pipes but not named FIFOs (verified on 6.17); the `EOPNOTSUPP` also permanently disables RWF process-wide via `RWFFlagSupport.disable()`. The more general form is just "parent process dies and the stdin pipe's HUP state goes stale before the drain loop finishes" — any writer reappearing in that window hits this. ## Fix At the bottom of the loop, when `received_hup` is set: - If the read returned `EAGAIN`, the HUP is provably stale — re-arm the poll and return. - Otherwise re-check `bun.isReadable(fd)` (one `poll()` syscall, only on the HUP path) before looping back. If HUP has cleared, drop the stale flag / re-arm instead of committing to another blocking read. This keeps the fast-path behaviour for the normal case (HUP still set → keep draining locally without re-arming). ## Reproduction Deterministic in the new test: stdin is a FIFO, the harness writes one chunk and closes its writer (so `POLLIN|POLLHUP` arrive together), and the fixture's `'data'` handler reopens the FIFO for writing from inside the drain loop. ## Verification ``` $ USE_SYSTEM_BUN=1 bun test test/js/node/process/stdin/process-stdin-stale-hup.test.ts (fail) process.stdin drain loop does not wedge on a stale POLLHUP … { exited: "timeout", stdout: "" } # child wedged, never printed OK $ bun bd test test/js/node/process/stdin/process-stdin-stale-hup.test.ts (pass) process.stdin drain loop does not wedge on a stale POLLHUP … [2.2s] ``` Gate: with `src/` stashed the test fails (timeout); with the fix it passes. `process-stdin.test.ts`, `process-stdio.test.ts`, and `stdin-pause-resume.test.ts` still pass. `zig:check-all` passes on all targets. This is the native-level root cause behind the "stdin spinloop after parent dies" report; it supersedes the JS-side band-aid in oven-sh#29610. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
Problem
PosixBufferedReader.readBlockingPipe()— the read loop used forprocess.stdinwhen stdin is a FIFO — snapshotsreceived_hupfrom the epoll/kqueue event and then loopswhile(true)draining the pipe, on the assumption that a hung-up pipe will eventually read 0 (EOF).That assumption breaks because
onReadChunk()re-enters JS: it resolves the pending read promise, which drains microtasks, which fires the Node'data'event synchronously whilereadBlockingPipeis still on the stack. If the handler opens a new writer on the same FIFO (or any code running at that point does), the pipe is no longer hung up — butreceived_hupis stilltrue, and the loop commits to another read that can never reach EOF:readNonblocking()resultpreadv2(RWF_NOWAIT)→EOPNOTSUPP† → fallback to blockingread()read()O_NONBLOCKread()→EAGAIN→isRetry()→ fall through →!received_hupfalse → loop† Linux supports
RWF_NOWAITon anonymous pipes but not named FIFOs (verified on 6.17); theEOPNOTSUPPalso permanently disables RWF process-wide viaRWFFlagSupport.disable().The more general form is just "parent process dies and the stdin pipe's HUP state goes stale before the drain loop finishes" — any writer reappearing in that window hits this.
Fix
At the bottom of the loop, when
received_hupis set:EAGAIN, the HUP is provably stale — re-arm the poll and return.bun.isReadable(fd)(onepoll()syscall, only on the HUP path) before looping back. If HUP has cleared, drop the stale flag / re-arm instead of committing to another blocking read.This keeps the fast-path behaviour for the normal case (HUP still set → keep draining locally without re-arming).
Reproduction
Deterministic in the new test: stdin is a FIFO, the harness writes one chunk and closes its writer (so
POLLIN|POLLHUParrive together), and the fixture's'data'handler reopens the FIFO for writing from inside the drain loop.Verification
Gate: with
src/stashed the test fails (timeout); with the fix it passes.process-stdin.test.ts,process-stdio.test.ts, andstdin-pause-resume.test.tsstill pass.zig:check-allpasses on all targets.This is the native-level root cause behind the "stdin spinloop after parent dies" report; it supersedes the JS-side band-aid in #29610.