Conversation
…es a buffered EOF
When a piped producer writes all its data and closes in one go, the native
reader delivers the chunk and closes the web stream controller together. A
'data' listener that calls process.stdin.pause() (typically via
readline.Interface#close()) schedules disown() on nextTick, which releaseLock()s
the reader before maybeReadMore() can issue the next pull. The already-queued
{done:true} is dropped, so push(null) never runs and 'end'/readableEnded never
fire.
Fix in two places:
- 'pause' handler: issue one more reader.read() before releaseLock(). If the
controller is already closed that read is resolved {done:true} and survives
the release; if it is still open the release rejects it and the existing
catch re-arms as before.
- EOF path: call stream.read(0) after push(null) so endReadable() runs even
when flow() is stopped. This matches Node's onStreamRead.
Node's net.Socket does not readStop on pause(), only on buffer-full, so the
EOF read always completes; Bun's pull-based stdin needs the extra pull here
to reach parity.
|
Reproduced with the self-spawning script from the report (both the readline variant and a reduced Behavior change: CI: gate passed (fail-before + pass-after on ASAN and release). All Linux/Windows/Alpine/Debian/Ubuntu test lanes that ran are green on the files this PR touches. Remaining red is unrelated: |
WalkthroughChangesThe stdin stream now tracks received data, advances EOF termination after paused reads, and can reacquire native stdin ownership before subsequent reads. Tests cover paused streams, closed readline interfaces, timeout cleanup, and Node/Bun parity. stdin lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:28 AM PT - Jul 18th, 2026
❌ @robobun, your commit e904115 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34613That installs a local version of the PR into your bun-34613 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the comment-length nit, I traced the new internalRead(stream) call in the 'pause' handler against disown()'s releaseLock(): the catch keys on readerForThisRead !== reader, so a non-EOF pending read that gets rejected by the release lands in the re-arm path (not stream.destroy(err)). Also checked that stream.read(0) after push(null) doesn't re-own() (the size !== 0 guard on the read override skips it), and that the existing "'end' not emitted when buffer never drained" test is unaffected (read(0) only calls endReadable() when state.length === 0). Deferring because getStdinStream has subtle nextTick/microtask ordering and a history of regressions — worth a maintainer's eyes.
Extended reasoning...
Traced the two logic additions through the pause→nextTick→disown ordering, the releaseLock rejection path in internalRead's catch, the stream.read override's size!==0 guard, and the stream_reachedEof guard on own(). Found no double-push(null), no spurious destroy(), and no re-own() after EOF. The existing pause/resume churn test covers the case where resume() runs before the pause nextTick fires (readableFlowing guard short-circuits the new internalRead). The only reported finding is a 4-line comment vs the 3-line CLAUDE.md rule, which is cosmetic. Still deferring: process.stdin's own/disown machinery is race-prone and this file has accumulated several regression fixes; a maintainer familiar with its history should confirm the microtask-vs-nextTick reasoning in the PR description.
There was a problem hiding this comment.
Thanks for tightening the comment. I traced the fix and it looks correct to me, but I'd like a human to confirm one assumption it relies on: in the 'pause' handler, internalRead(stream) is called and then disown() runs synchronously right after. The fix depends on a closed controller's reader.read() returning an already-resolved {done:true} promise that survives the immediately-following releaseLock() — that holds per the WHATWG spec, but it's worth a maintainer's eye given how much of getStdinStream hinges on exact nextTick/microtask/releaseLock ordering.
Checked: the size !== 0 guard in the overridden stream.read means the new read(0) never re-own()s; the "buffer never drained" and "exit when paused" tests still pass; and the non-EOF case (extra read pulls a data chunk into the paused buffer, or pends and is rejected by releaseLock()) re-arms the same way as before.
Extended reasoning...
Overview
Two additions to getStdinStream in src/js/builtins/ProcessObjectInternals.ts (9 lines) plus two new test cases in process-stdin.test.ts (56 lines). The fix addresses a race where pause() inside a 'data' handler causes the 'pause' listener's nextTick'd releaseLock() to run before maybeReadMore() can issue the read that would observe the already-queued EOF, so push(null) never happens and 'end' never fires.
The two changes: (1) in the 'pause' nextTick, issue one more internalRead() before disown() so an already-closed controller's synchronously-resolved {done:true} reaches push(null); (2) after push(null), call stream.read(0) (mirroring Node's onStreamRead) so endReadable() runs even when flow is stopped.
Security risks
None. This is Node-compat stream plumbing on process.stdin; no parsing of untrusted input, no auth/crypto/permissions.
Level of scrutiny
Medium-high. src/js/builtins/ is hot-path builtin code, and getStdinStream is already a carefully-ordered dance between the Node Readable state machine, the WHATWG reader, and nextTick scheduling. The fix is small and well-argued (with a Node upstream citation for read(0)), but its correctness rests on a subtle spec detail: reader.read() on a closed-and-drained stream returns an already-fulfilled promise with no pending read-request entry, so the immediately-following releaseLock() cannot reject it. I verified this holds per the Streams spec, but a maintainer who knows Bun's ReadableStream internals should confirm it holds in practice.
Other factors
- I traced the non-EOF branches: if the extra read pends,
releaseLock()rejects it into the existingreaderForThisRead !== readercatch →needsInternalReadRefresh, identical to pre-PR behavior. If it resolves with a data chunk, the chunk is pushed into the paused buffer and delivered on the nextresume()viaflow()→_read()— no data loss, no hang. - The overridden
stream.readskipsown()whensize === 0, andown()itself early-returns oncestream_reachedEofis set, so the newread(0)cannot re-acquire the reader. - Test evidence is solid: both new tests fail on main (debug + release) and pass with the fix; all pre-existing
process-stdin.test.tscases (including "exit when paused", "buffer never drained", and "pause/resume churn") still pass. - My earlier nit (4-line comment) was addressed in fa67f5f.
Deferring rather than approving because the correctness argument depends on precise promise-resolution-vs-releaseLock ordering in builtin code — the kind of thing REVIEW.md flags for maintainer sign-off.
|
On the assumption flagged above: verified empirically against Bun's C++ ReadableStream, not just the spec. const rs = new ReadableStream({ start(c) { c.enqueue(new Uint8Array([1,2,3])); c.close(); } });
const reader = rs.getReader();
await reader.read(); // {value: <3 bytes>, done: false}
const p = reader.read(); // controller closed + queue empty
reader.releaseLock();
console.log(await p); // {value: undefined, done: true} (not rejected)Same result in Node v26.3.0. A read issued against an open controller with no queued chunk does get rejected by |
…OF survives pause() The earlier approach (pull once in the pause handler before releaseLock()) worked on POSIX where the reader sees data+EOF in one synchronous read, but not on Windows where libuv delivers data and EOF as separate IOCP completions. Match Node's onpause/Socket._read contract instead: pause() disowns (readStop), and _read() re-owns (readStart). addChunk()'s maybeReadMore() calls _read() after every push(), so the pull that observes the queued EOF runs with an owned reader on both platforms. Gate the re-own on hasReceivedData && !isTTY so the construct-time maybeReadMore from fs.ReadStream (stdin exits when paused before any data) and TTY pause (child inheriting stdin) keep their existing behaviour.
The open-pipe variant asserted a Bun-specific divergence from Node (exit after pause() mid-stream with the pipe still open). _read now re-arms the native read after pause() so a queued EOF is delivered (Node's onpause/Socket._read contract); with an open pipe that keeps the process alive on Linux as Node does. Repoint the case at the closed-pipe scenario, which is the behaviour this PR fixes and is identical in Bun and Node on every platform.
The 1s autokill is shorter than a debug-build child's startup, so every case in this file was autoKilled under bun bd. Scale to 10s when isDebug and clear the timer on close so release runs stay fast.
![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/stdin/stdin-fixtures.test.ts`:
- Around line 27-33: Update the rejection paths in the stdin fixture test to
clear killTimer before rejecting, including both the error and exhausted-input
paths. Preserve the existing rejection behavior while ensuring failed setup
cancels the pending timer immediately rather than waiting for close.
🪄 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: addea058-9ab6-44b5-b4c9-4cdc6e65a239
📒 Files selected for processing (3)
src/js/builtins/ProcessObjectInternals.tstest/js/node/process/process-stdin.test.tstest/js/node/process/stdin/stdin-fixtures.test.ts
There was a problem hiding this comment.
No open findings from this pass — the earlier three threads are all addressed. Deferring to a maintainer rather than auto-approving because this changes process-lifetime semantics: pause() inside a 'data' handler with the pipe still open now keeps the process alive on Linux (Node parity), and the stdin-fixtures "pause allows process to exit" case was reworked from open-pipe (end: false) to closed-pipe (end: true) with the open-pipe variant now acknowledged as platform-divergent (Linux vs Windows). That trade-off looks right to me but is a behavior/default change worth a human sign-off.
What was reviewed:
triggerReadOwninggating (hasReceivedData && !isTTY) against the pre-datamaybeReadMorekick and TTY inherit (#23341) — both still exit/disown as before.stream.read(0)afterpush(null)vs thesize !== 0/stream_reachedEofguards — no re-own after EOF; matches Node'sonStreamRead.releaseLock()racing the extra pull in the'pause'handler — verified against Bun's ReadableStream that a read on a closed controller resolves{done:true}rather than rejecting.
Extended reasoning...
Overview
The PR touches getStdinStream in src/js/builtins/ProcessObjectInternals.ts (the process.stdin bridge over Bun.stdin.stream()), plus two test files. The runtime change: (1) _read becomes triggerReadOwning, which re-own()s the native reader when !reader && hasReceivedData && !isTTY && !stream_reachedEof && !stream_destroyed, mirroring Node's Socket._read → readStart(); (2) internalRead sets hasReceivedData on the first chunk and issues stream.read(0) after push(null) so endReadable() runs even when flow() is stopped; (3) the why-comment above the stream.read override is refreshed. Tests add two new process-stdin.test.ts cases (direct pause() and readline.close()), and stdin-fixtures.test.ts replaces the Bun-only open-pipe pause case with a runBoth closed-pipe case, plus harness hardening (isDebug-scaled kill timeout, clearTimeout on reject paths).
Security risks
None identified. This is Node-compat stream lifecycle logic in a builtin module; no untrusted-input parsing, auth, crypto, or filesystem paths are involved. The builtin uses $call and closure-captured locals, so tamper-resistance is unchanged.
Level of scrutiny
High. getStdinStream is a small but very stateful piece of code — own/disown, ref/unref, TTY vs pipe vs file, the #23341 TTY-inherit constraint, and multiple nextTick orderings all interact. Three prior review rounds on this PR each surfaced a real issue (comment length, a genuine test regression from the re-own, and a stale why-comment), all now fixed. The mechanism is well-argued against Node's lib/net.js / stream_base_commons.js, and the new tests fail on main and pass with the fix.
Other factors
The reason I'm not auto-approving is the intentional behavior change and the accompanying test replacement. Previously Bun let a process exit after pause() inside a 'data' handler even with the pipe still open; now (on Linux) it stays alive until the pipe closes or unref() is called, matching Node. The old stdin-fixtures test that asserted the Bun-specific exit was converted to a closed-pipe Node-parity test, and the author notes the open-pipe case is now platform-dependent (Linux matches Node, Windows still exits) and out of scope. REVIEW.md flags both "never silently weaken an existing test" (this one is stated, not silent, but it does drop coverage of the open-pipe scenario) and behavior/default changes as things a maintainer should weigh. That call — accepting the Node-parity lifetime change plus a known Windows divergence in exchange for correct 'end' delivery — is reasonable but should be made by a human.
There was a problem hiding this comment.
I didn't find any further issues — all three earlier threads are resolved — but this changes user-visible process.stdin lifetime semantics (pause() after data on an open pipe now keeps the process alive on Linux), introduces an acknowledged Linux/Windows divergence in that same case, and the macOS CI lanes didn't pick up the build, so a maintainer should sign off on the behavior change.
What was reviewed:
triggerReadOwninggating (hasReceivedData && !isTTY) against the pre-data pause-exits case and TTY child-inherit (#23341) — both preserved.stream.read(0)afterpush(null):stream_reachedEofis set first, so neither the wrapper'ssize !== 0guard nortriggerReadOwningre-owns on that call.- The rewritten
stdin-fixturespause case: input changed fromend: falsetoend: truewith a stated rationale; the dropped open-pipe assertion is the behavior change called out in the description.
Extended reasoning...
Overview
The PR fixes a Node-compat gap where process.stdin never emits 'end' if pause() (or rl.close()) is called from inside a 'data'/'line' handler while an EOF is already queued in the underlying web stream. The fix is in src/js/builtins/ProcessObjectInternals.ts getStdinStream: a new triggerReadOwning _read re-acquires the native reader (mirroring Node's Socket._read → readStart()), gated on hasReceivedData && !isTTY; and stream.read(0) is issued after push(null) so endReadable() runs even when flow() is stopped. Two new tests in process-stdin.test.ts cover the direct-pause() and readline.close() variants; stdin-fixtures.test.ts converts the pause case to runBoth({end: true}) and adds killTimer cleanup + a debug-sized timeout.
Security risks
None. This is stream lifecycle/event-ordering logic in a built-in JS module; no parsing of untrusted input, no auth/crypto, no new API surface.
Level of scrutiny
High. getStdinStream is hot-path built-in code with a history of subtle races (nextTick ordering between disown() and maybeReadMore_, releaseLock() rejecting in-flight reads, TTY child-inherit from #23341). More importantly, the PR ships a documented behavior change: on Linux, pause() from inside a 'data' handler with the pipe still open no longer lets the process exit — it stays alive until the pipe closes or unref() is called. That matches Node, but it inverts prior Bun behavior that an existing test asserted. The author also notes this introduces a Linux/Windows divergence (Windows still exits) that "would need a separate look at updateRef on Windows". A maintainer should confirm that trading the old divergence-from-Node for a new divergence-between-platforms is the right call here, and that no downstream Bun users depend on the old exit-on-pause behavior.
Other factors
- All three of my earlier inline threads (4-line comment, the
stdin-fixturesregression, and the stalestream.readwhy-comment) were addressed and resolved; the current diff reflects those fixes. - The
stdin-fixtures.test.ts"pause allows process to exit" case had its input changed (["abc","pause","def"], end:false→["abc","pause"], end:true) rather than being kept alongside a new case. REVIEW.md flags input-mutating an existing test; the author's rationale (the open-pipe variant is now platform-dependent and asserts a Bun-only divergence this PR removes) is stated in the thread and in the test comment, but it's still a weakening a human should ack. - CI: Linux/Windows/Alpine/Debian/Ubuntu green per the gate; the three darwin test lanes expired without an agent, so macOS is untested for this change.
- The mechanism itself checks out:
hasReceivedDatakeeps the construct-timemaybeReadMorefrom re-owning (so "stdin should allow process to exit when paused" still passes),!isTTYpreserves the child-inherit path, andstream_reachedEofset beforedisown()/push(null)/read(0)prevents that finalread(0)from re-owning via either the wrapper ortriggerReadOwning.
This branch has not been deployed
1 participant
What does this PR do?
producer | toolwhere the tool callsrl.close()(orprocess.stdin.pause()) from inside the first'line'/'data'handler never seesprocess.stdinemit'end', andreadableEndedstaysfalse. Node delivers'end'.Repro (deterministic, self-spawning):
Cause
process.stdinwrapsBun.stdin.stream()with a pull-based reader. The'pause'listenernextTicksdisown(), which callsreleaseLock()andsetFlowing(false)(uv_read_stop on Windows). That runs beforemaybeReadMore()'snextTickcan issue the next pull, so the EOF that arrived with the chunk is never observed andpush(null)never runs.Node's
process.stdinuses a different contract:onpausecallsreadStop(), butSocket._read()callsreadStart().addChunk()schedulesmaybeReadMore()after every push, which calls_read(), so the native read is re-armed after the pause and the queued EOF is delivered (lib/internal/bootstrap/switches/is_main_thread.js+lib/net.js).Fix
getStdinStream:_read(triggerReadOwning) re-own()s when the reader was released, mirroring Node'sSocket._read->readStart(). Gated onhasReceivedData && !isTTYso:pause()before any data still lets the process exit (the construct-timemaybeReadMorefromfs.ReadStreamdoes not re-own),stdio:"inherit"is not raced (the behaviour from Fix: after pausing stdin, a subprocess should be able to read from stdin #23341).push(null), callstream.read(0)soendReadable()runs even whenflow()is stopped. This mirrors Node'sonStreamRead(lib/internal/stream_base_commons.js).Behavior change
pause()from inside a'data'handler with the pipe still open no longer exits the process on Linux; it stays alive until the pipe closes orunref()is called. This matches Node (Socket._readre-armsreadStart()afteronpause'sreadStop()). The previous Bun behaviour was a divergence that made EOF delivery impossible.stdin-fixtures.test.tsis updated to assert the closed-pipe Node-parity case instead.How did you verify your code works?
Two new cases in
test/js/node/process/process-stdin.test.ts(directpause()andreadline.close()), both fail on main and pass with this change on Linux and Windows.stdin-fixtures.test.tsswitched torunBothfor the pause case and passes on both platforms. The rest ofprocess-stdin.test.ts, thetest-stdin-*/test-readline-*Node parallel tests,readline.node.test.ts, andstdin-pause-resume.test.tsare unchanged.[review] gate passed · iteration 5 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 5
evidence per changed file