Conversation
tty.ReadStream inherited autoClose:true from fs.ReadStream, causing it to auto-destroy and close the fd on read errors. This broke node-pty (used by Gemini CLI and others) which stores the PTY master fd and calls ioctl(TIOCSWINSZ) for resize - the fd was already closed, causing EBADF. tty.WriteStream already passed autoClose:false; apply the same to ReadStream to match Node.js behavior where tty streams don't own the fd. Closes #27285 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 5:01 AM PT - Mar 18th, 2026
❌ Your commit 🧪 To try this PR locally: bunx bun-pr 28219That installs a local version of the PR into your bun-28219 --bun |
|
Found 8 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughThis PR modifies the tty.ReadStream initialization to set Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/27285.test.ts`:
- Around line 67-75: The test uses a fixed 200ms setTimeout to check fd validity
(setTimeout(..., 200)) which is flaky; replace this with an
event-loop/condition-based wait that repeatedly attempts fs.fstatSync(fd) until
it succeeds or a short test-timeout is reached (use an async loop with await new
Promise(resolve => setImmediate(resolve)) or a Promise.race with a timeout), log
"FD_STILL_VALID:true"/"FD_STILL_VALID:false" based on the condition, and end the
test by resolving/rejecting the test promise instead of calling process.exit(0);
update the code around setTimeout, fs.fstatSync, fd and process.exit to use the
async polling pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26c5bb0a-ddca-4d6e-89b2-99c4e58dfc4f
📒 Files selected for processing (2)
src/js/node/tty.tstest/regression/issue/27285.test.ts
| setTimeout(() => { | ||
| try { | ||
| fs.fstatSync(fd); | ||
| console.log("FD_STILL_VALID:true"); | ||
| } catch { | ||
| console.log("FD_STILL_VALID:false"); | ||
| } | ||
| process.exit(0); | ||
| }, 200); |
There was a problem hiding this comment.
Replace timer-based wait with condition/event-loop based check.
Line 67 to Line 75 uses setTimeout(..., 200), which makes this regression timing-sensitive and can be flaky.
🔧 Proposed change
-// After 200ms, check that the fd is still valid.
-// Before the fix, the fd would be closed within the first event loop tick
-// because autoDestroy:true caused the stream to destroy itself on errors.
-setTimeout(() => {
+// Check that the fd is still valid on the next event loop turn.
+// Before the fix, the fd could be closed within the first event loop tick.
+setImmediate(() => {
try {
fs.fstatSync(fd);
console.log("FD_STILL_VALID:true");
} catch {
console.log("FD_STILL_VALID:false");
}
process.exit(0);
-}, 200);
+});As per coding guidelines: “Do not use setTimeout in tests; instead await the condition to be met - you are testing the CONDITION, not TIME PASSING.”
📝 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.
| setTimeout(() => { | |
| try { | |
| fs.fstatSync(fd); | |
| console.log("FD_STILL_VALID:true"); | |
| } catch { | |
| console.log("FD_STILL_VALID:false"); | |
| } | |
| process.exit(0); | |
| }, 200); | |
| setImmediate(() => { | |
| try { | |
| fs.fstatSync(fd); | |
| console.log("FD_STILL_VALID:true"); | |
| } catch { | |
| console.log("FD_STILL_VALID:false"); | |
| } | |
| process.exit(0); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/27285.test.ts` around lines 67 - 75, The test uses a
fixed 200ms setTimeout to check fd validity (setTimeout(..., 200)) which is
flaky; replace this with an event-loop/condition-based wait that repeatedly
attempts fs.fstatSync(fd) until it succeeds or a short test-timeout is reached
(use an async loop with await new Promise(resolve => setImmediate(resolve)) or a
Promise.race with a timeout), log "FD_STILL_VALID:true"/"FD_STILL_VALID:false"
based on the condition, and end the test by resolving/rejecting the test promise
instead of calling process.exit(0); update the code around setTimeout,
fs.fstatSync, fd and process.exit to use the async polling pattern.
Address review feedback: use setImmediate-based event loop tick polling instead of a fixed 200ms setTimeout to check fd validity. This directly targets the bug (fd closed within first event loop tick) and avoids timing-dependent flakiness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line fix matching the existing autoClose: false pattern already used by tty.WriteStream.
Extended reasoning...
Overview
This PR modifies a single line in src/js/node/tty.ts to pass autoClose: false when constructing ReadStream, and adds a new regression test file. The fix aligns ReadStream with WriteStream, which already passes autoClose: false on line 117 of the same file.
Security risks
None. The change prevents an fd from being prematurely closed, which is the correct behavior matching Node.js. No auth, crypto, or permissions code is involved.
Level of scrutiny
This is a minimal, mechanical fix following an established pattern in the same file. The intent is clear, the change is one line, and it matches Node.js semantics. Low scrutiny is appropriate.
Other factors
The first test directly asserts autoClose:false and autoDestroy:false on the stream, providing solid regression coverage. The bug report filed is a nit about the second test quality — these are minor observations that do not affect the correctness of the actual fix. The PR has no CODEOWNERS concerns, no outstanding reviewer comments, and the CI build was triggered.
There was a problem hiding this comment.
LGTM — single-line fix matching the existing WriteStream pattern, aligns with Node.js behavior.
Extended reasoning...
Overview
This PR adds autoClose: false to tty.ReadStream initialization in src/js/node/tty.ts (one line changed), matching what tty.WriteStream already does. A new regression test file test/regression/issue/27285.test.ts is added with two tests.
Security risks
None. The change prevents premature fd closure, which is a correctness fix. It does not introduce new attack surface or modify any security-sensitive code paths.
Level of scrutiny
This is a minimal, mechanical fix. The production change is a single option added to a constructor call, following an identical pattern already established by WriteStream on line 117 of the same file. The fix aligns Bun with Node.js behavior (where tty.ReadStream extends net.Socket and does not auto-close). This warrants low scrutiny.
Other factors
The setTimeout concern raised by both coderabbit and my previous review has been addressed — the current diff uses setImmediate polling. The /dev/null fallback nit I raised is minor and does not affect the validity of the core regression coverage (the first test directly asserts autoClose:false and autoDestroy:false). Build failures in CI appear to be infrastructure-related (asan/musl build failures), not caused by this change. No CODEOWNERS concerns for these files.
|
Closing this PR because it has been inactive for more than 90 days. |
1 participant
Summary
tty.ReadStreamto passautoClose: falseto the underlyingfs.ReadStream, matching the behavior already used bytty.WriteStreamnode-pty(used by Gemini CLI) to crash withioctl(2) failed, EBADFRoot Cause
tty.ReadStreamextendsfs.ReadStreamwhich defaultsautoClose(and thusautoDestroy) totrue. When the stream encountered a read error on a PTY fd, it would auto-destroy itself, callingfs.close(fd)on the PTY master fd. This closed the PTY master, causing the kernel to send SIGHUP to the child process and making subsequentioctl(TIOCSWINSZ)calls fromnode-pty's native addon fail with EBADF.tty.WriteStreamalready passedautoClose: false— this applies the same fix toReadStream.In Node.js,
tty.ReadStreamextendsnet.Socket(notfs.ReadStream) and doesn't auto-close the fd, so this aligns Bun's behavior with Node.js.Closes #27285
Test plan
test/regression/issue/27285.test.tsverifiesautoClose: falseand fd validity after errorsautoClose:true, fd closed), passes with fixtty.test.ts,tty-readstream-ref-unref.test.ts,tui-app-tty-pattern.test.ts)@lydell/node-ptyreproduction — resize succeeds instead of EBADF crash🤖 Generated with Claude Code