Repository navigation
Conversation
After `process.stdin.on('readable', fn)` then `removeListener('readable', fn)`,
Bun kept polling fd 0, so a `stdio: 'inherit'` child raced the parent for
input bytes. This breaks the common TUI pattern of unmounting before handing
the terminal to vim/less/tmux.
Match Node's mechanism exactly:
- `tty.ReadStream` sets `highWaterMark: 0` (Node lib/tty.js does the same)
- `internalRead`: `if (!stream.push(value)) disown()` — Node's `onStreamRead`
pattern. With hwm=0 on TTY, push() returns false on every chunk so fd 0 is
released after each one.
- `triggerRead` (`_read`): `own()` unless explicitly paused — Node's
`Socket.prototype._read → tryReadStart()` pattern. Re-acquires fd 0 only
when a consumer pulls.
Once the last 'readable' listener is removed, Readable stops calling `_read()`,
the in-flight read delivers one chunk, push() returns false, fd 0 is released,
and the child reads exclusively. No listener-removal tracking, no readable.ts
changes — own/disown is driven purely by stream demand.
Also fixes two prerequisites:
- `tty.ReadStream(fd)` was hardcoding `{fd}` and dropping `highWaterMark`
- stdin's `_readableState.constructed` was false for two ticks (fs.ReadStream
has an async `_construct`; Node's net.Socket doesn't), which made `read(0)`
skip `_read()` when hwm=0
Pipe stdin keeps the default hwm (65536) and only releases at backpressure —
identical to Node.
|
Updated 11:41 AM PT - Apr 10th, 2026
❌ @alii, your commit 3d9ba0b has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29121That installs a local version of the PR into your bun-29121 --bun |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughThis pull request modifies stdin stream handling in ProcessObjectInternals.ts and tty.ts to properly manage fd ownership and backpressure behavior when readable listeners are removed, and adds test files to validate the behavior under PTY scenarios. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/tty.ts`:
- Around line 19-22: The ctor currently replaces caller options by passing { fd,
highWaterMark: 0 } to fs.ReadStream.$apply; change it to merge the
caller-supplied options with defaults so caller values win: build a
mergedOptions object that includes fd and sets highWaterMark: 0 only when
options.highWaterMark is undefined (and preserves other keys like autoClose),
then call fs.ReadStream.$apply(this, ["", mergedOptions]); update references in
the tty.ReadStream constructor to use this mergedOptions approach so external
callers and internal callers (e.g., passing autoClose: false) are honored.
In `@test/js/node/process/stdin/readable-removed-pty.test.ts`:
- Around line 32-37: The combined assertion bundles childLines and exitCode into
one expect(...).toEqual which makes failures hard to read; split it into two
assertions: first assert the parsed stdout-related values (assert stderr is ""
and childLines contains the expected entries and length via childLines /
expect.arrayContaining / expect(childLines.length).toBeGreaterThanOrEqual(4)),
then separately assert exitCode === 0 using expect(exitCode).toBe(0) so stdout
checks come before the exit-code check (references: childLines, stderr,
exitCode).
In `@test/js/node/process/stdin/run-with-pty-readable.py`:
- Around line 8-28: Rename the helper functions waitForReady and waitAndWrite to
snake_case (e.g., wait_for_ready and wait_and_write), add type hints
(wait_and_write(b: bytes) -> None and wait_for_ready() -> None), and add concise
docstrings to both; also update timeout_handler to include the proper signature
with type hints (timeout_handler(signum: int, frame: Any) -> None) and a short
docstring — ensure any internal references to master_fd/pid remain unchanged and
adjust callers if needed to use the new function names.
- Around line 23-28: The timeout_handler currently calls os.kill(pid, 9) which
only kills the session leader; change it to kill the whole process group using
os.killpg(pid, signal.SIGKILL) (or os.kill(-pid, signal.SIGKILL)) so the entire
PTY session is terminated, and narrow the except clause to catch
ProcessLookupError (or OSError with errno ESRCH) instead of a bare except to
only ignore the expected "no such process" lookup failure; ensure signal is
referenced (signal.SIGKILL) when updating timeout_handler and continue to call
sys.exit(1) after the kill.
🪄 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: af95bd37-9f3c-41f7-8b8e-68f3892f20c6
📒 Files selected for processing (5)
src/js/builtins/ProcessObjectInternals.tssrc/js/node/tty.tstest/js/node/process/stdin/readable-removed-pty.test.tstest/js/node/process/stdin/readable-removed-releases-tty.mjstest/js/node/process/stdin/run-with-pty-readable.py
| // Node's tty.ReadStream sets readableHighWaterMark: 0 so push() returns | ||
| // false on every chunk and onStreamRead's backpressure path can readStop() | ||
| // between reads. | ||
| fs.ReadStream.$apply(this, ["", { fd, highWaterMark: 0 }]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the Node.js constructor signature for tty.ReadStream, and should a caller-specified highWaterMark in the options object override the default TTY high-water mark?
💡 Result:
The constructor signature for tty.ReadStream is new tty.ReadStream(fd[, options]), where fd is a numeric file descriptor associated with a TTY, and options (optional, added in v0.9.4) are passed to the parent net.Socket constructor (type net.SocketConstructorOpts). A caller-specified highWaterMark in the options object will override the default high-water mark. There is no TTY-specific default or override documented; tty.ReadStream extends net.Socket (which extends stream.Duplex), so it inherits the standard stream behavior where the highWaterMark option sets the buffer threshold (default 64KB/65536 bytes for byte-mode streams in recent Node.js versions). The official docs confirm options are forwarded to net.Socket, and no special TTY handling for highWaterMark is mentioned in Node.js documentation, TypeScript definitions, or source references.
Citations:
- 1: https://nodejs.org/api/tty.html
- 2: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/8c9533835001e60135d5ede3d75934e7b26aa4b3/types/node/tty.d.ts
- 3: https://nodejs.org/api/net.html
- 4: https://nodejs.org/api/stream.html
Merge caller-supplied options instead of replacing them.
Line 22 hardcodes { fd, highWaterMark: 0 } and discards any options passed by the caller. This violates the documented tty.ReadStream(fd[, options]) API — Node.js forwards options to the parent net.Socket constructor, and caller-specified highWaterMark should override the default. The current approach breaks both external callers (new tty.ReadStream(fd, { highWaterMark: X })) and the internal path in src/js/builtins/ProcessObjectInternals.ts that needs to pass autoClose: false.
Preserve the intentional highWaterMark: 0 as a default only when the caller does not provide one, then merge with any caller-supplied options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/tty.ts` around lines 19 - 22, The ctor currently replaces caller
options by passing { fd, highWaterMark: 0 } to fs.ReadStream.$apply; change it
to merge the caller-supplied options with defaults so caller values win: build a
mergedOptions object that includes fd and sets highWaterMark: 0 only when
options.highWaterMark is undefined (and preserves other keys like autoClose),
then call fs.ReadStream.$apply(this, ["", mergedOptions]); update references in
the tty.ReadStream constructor to use this mergedOptions approach so external
callers and internal callers (e.g., passing autoClose: false) are honored.
| expect({ stderr, childLines, exitCode }).toEqual({ | ||
| stderr: "", | ||
| childLines: expect.arrayContaining(['CHILD:"B"', 'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"']), | ||
| exitCode: 0, | ||
| }); | ||
| expect(childLines.length).toBeGreaterThanOrEqual(4); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split the parsed-stdout assertion from the exit-code assertion.
Bundling childLines and exitCode into one toEqual makes failures much harder to read than the repo-standard pattern of checking output first and exitCode last.
🧪 Suggested assertion ordering
- expect({ stderr, childLines, exitCode }).toEqual({
- stderr: "",
- childLines: expect.arrayContaining(['CHILD:"B"', 'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"']),
- exitCode: 0,
- });
- expect(childLines.length).toBeGreaterThanOrEqual(4);
+ expect(childLines).toEqual(expect.arrayContaining(['CHILD:"B"', 'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"']));
+ expect(childLines.length).toBeGreaterThanOrEqual(4);
+ expect(stderr).toBe("");
+ expect(exitCode).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/process/stdin/readable-removed-pty.test.ts` around lines 32 -
37, The combined assertion bundles childLines and exitCode into one
expect(...).toEqual which makes failures hard to read; split it into two
assertions: first assert the parsed stdout-related values (assert stderr is ""
and childLines contains the expected entries and length via childLines /
expect.arrayContaining / expect(childLines.length).toBeGreaterThanOrEqual(4)),
then separately assert exitCode === 0 using expect(exitCode).toBe(0) so stdout
checks come before the exit-code check (references: childLines, stderr,
exitCode).
| def waitForReady(): | ||
| buffer = b"" | ||
| while b"%ready%" not in buffer: | ||
| ready = select.select([master_fd], [], [], 0.1)[0] | ||
| if ready: | ||
| data = os.read(master_fd, 1024) | ||
| if data: | ||
| buffer += data | ||
| sys.stdout.buffer.write(data) | ||
| sys.stdout.buffer.flush() | ||
|
|
||
| def waitAndWrite(b): | ||
| waitForReady() | ||
| os.write(master_fd, b) | ||
|
|
||
| def timeout_handler(signum, frame): | ||
| try: | ||
| os.kill(pid, 9) | ||
| except: | ||
| pass | ||
| sys.exit(1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Bring the new Python helpers in line with repo conventions.
waitForReady and waitAndWrite use camelCase, and all three helper defs still omit annotations/docstrings. This file is new, so it's a good time to rename them to snake_case and add the bytes / -> None signatures.
As per coding guidelines, "Follow PEP 8 style guide for Python code", "Use type hints for function arguments and return values", and "Write docstrings for all public functions and classes".
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 23-23: Unused function argument: signum
(ARG001)
[warning] 23-23: Unused function argument: frame
(ARG001)
[warning] 24-27: Use contextlib.suppress(BaseException) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(BaseException): ...
(SIM105)
[error] 26-26: Do not use bare except
(E722)
[error] 26-27: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/process/stdin/run-with-pty-readable.py` around lines 8 - 28,
Rename the helper functions waitForReady and waitAndWrite to snake_case (e.g.,
wait_for_ready and wait_and_write), add type hints (wait_and_write(b: bytes) ->
None and wait_for_ready() -> None), and add concise docstrings to both; also
update timeout_handler to include the proper signature with type hints
(timeout_handler(signum: int, frame: Any) -> None) and a short docstring —
ensure any internal references to master_fd/pid remain unchanged and adjust
callers if needed to use the new function names.
| def timeout_handler(signum, frame): | ||
| try: | ||
| os.kill(pid, 9) | ||
| except: | ||
| pass | ||
| sys.exit(1) |
There was a problem hiding this comment.
Kill the whole PTY session on timeout.
Because the child calls os.setsid() at Line 36 and the fixture then spawns another inherited-stdio child, os.kill(pid, 9) only terminates the session leader. If the watchdog fires with that grandchild still alive, it can keep the PTY open and leak until the test runner cleans it up. Use os.killpg(pid, signal.SIGKILL) (or os.kill(-pid, ...)) and narrow the exception to the expected lookup failure.
🔧 Suggested watchdog cleanup
def timeout_handler(signum, frame):
- try:
- os.kill(pid, 9)
- except:
- pass
+ try:
+ os.killpg(pid, signal.SIGKILL)
+ except ProcessLookupError:
+ pass
sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def timeout_handler(signum, frame): | |
| try: | |
| os.kill(pid, 9) | |
| except: | |
| pass | |
| sys.exit(1) | |
| def timeout_handler(signum, frame): | |
| try: | |
| os.killpg(pid, signal.SIGKILL) | |
| except ProcessLookupError: | |
| pass | |
| sys.exit(1) |
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 23-23: Unused function argument: signum
(ARG001)
[warning] 23-23: Unused function argument: frame
(ARG001)
[warning] 24-27: Use contextlib.suppress(BaseException) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(BaseException): ...
(SIM105)
[error] 26-26: Do not use bare except
(E722)
[error] 26-27: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/process/stdin/run-with-pty-readable.py` around lines 23 - 28,
The timeout_handler currently calls os.kill(pid, 9) which only kills the session
leader; change it to kill the whole process group using os.killpg(pid,
signal.SIGKILL) (or os.kill(-pid, signal.SIGKILL)) so the entire PTY session is
terminated, and narrow the except clause to catch ProcessLookupError (or OSError
with errno ESRCH) instead of a bare except to only ignore the expected "no such
process" lookup failure; ensure signal is referenced (signal.SIGKILL) when
updating timeout_handler and continue to call sys.exit(1) after the kill.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR touches subtle async I/O timing and internal readable state, warranting a human review — the logic looks well-reasoned and mirrors Node's internals, but the interplay between needsInternalReadRefresh, shouldDisown, own()/disown() and the direct _readableState.constructed mutation is complex enough that human eyes should verify.
Extended reasoning...
Overview
The PR modifies stdin fd 0 handling in ProcessObjectInternals.ts and tty.ts to match Node's backpressure-based fd release mechanism. Key changes: (1) tty.ReadStream now passes highWaterMark: 0 matching Node's lib/tty.js:64; (2) internalRead calls disown() when push() returns false; (3) the ERR_STREAM_RELEASE_LOCK path no longer re-enters triggerRead; (4) triggerRead calls own() unless the stream is paused; (5) stream._readableState.constructed is set to true to bypass fs.ReadStream's async open.
Security Risks
No security concerns — this is an I/O backpressure fix with no auth, crypto, or permissions involvement.
Level of Scrutiny
High. This touches fd 0 (stdin) handling, which is a fundamental I/O primitive. The state machine governing own()/disown()/needsInternalReadRefresh/shouldDisown is subtle and timing-dependent (PTY, async reader, Readable internals). Directly mutating _readableState.constructed bypasses Node's internal lifecycle; the argument that constructNT will re-set it idempotently is plausible but needs verification.
Other Factors
The author provides a detailed comparison table against Node's source (lib/tty.js, net.js), comprehensive adversarial test coverage, and a new PTY-based integration test. No prior reviews exist on this PR. The fix correctly identifies the root cause (hwm=0 means push() always returns false → readStop equivalent), but the correct behaviour depends on subtle event loop ordering between maybeReadMore_, pause handlers, and own() calls.
|
Superseded by #29134. |
2 participants
What
After
process.stdin.on('readable', fn)→removeListener('readable', fn), Bun kept polling fd 0. A child spawned withstdio: 'inherit'then raced the parent for input — the parent silently buffered keystrokes meant for the child. This breaks the common TUI pattern (Ink, etc.) of unmounting before handing the terminal to vim/less/tmux.How
Match Node's mechanism exactly — no listener tracking, no
readable.tschanges:highWaterMark0(lib/tty.js:64)tty.ReadStreamnow setshighWaterMark: 0onStreamRead:if (!push(buf)) handle.readStop()internalRead:if (!push(value)) disown()Socket.prototype._read→tryReadStart()triggerRead:own()unless pausedWith hwm=0,
push()returns false on every TTY chunk →disown()after each →_read()re-owns only when a consumer pulls. Once the last'readable'listener is gone, Readable stops calling_read()→ fd 0 stays released → child reads exclusively. Pipe stdin keeps hwm=65536 and only releases at backpressure (same as Node).Also fixes two prerequisites this surfaced:
tty.ReadStream(fd)was dropping thehighWaterMarkoption (hardcoded{fd})constructedflag was unset for two ticks (fs.ReadStreamhas async_construct, Node'snet.Socketdoesn't), makingread(0)skip_read()when hwm=0Test plan
bun bd test test/js/node/process/stdin/ test/js/node/process/process-stdin.test.ts test/js/node/process/process-stdio.test.ts test/js/node/readline/ test/js/node/stream/ test/js/node/tty/— 163 passprocess.stdin.readableHighWaterMark: 0 under TTY, 65536 under pipe — matches NoderemoveAllListeners()→ re-subscribe → remove under PTY — matches NodeSupersedes #29061.