fix(tty): prevent ReadStream from closing fd on EAGAIN#27286
fix(tty): prevent ReadStream from closing fd on EAGAIN#27286
Conversation
When a non-blocking PTY master fd returns EAGAIN (no data available), fs.ReadStream._read() was calling errorOrDestroy() which auto-destroyed the stream and closed the fd. This caused node-pty (used by Gemini CLI) to crash with "ioctl(2) failed, EBADF" on subsequent operations. Override _read on tty.ReadStream prototype to emit EAGAIN/EWOULDBLOCK errors without destroying the stream, keeping the fd valid for continued use. Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a custom ReadStream implementation for TTY master file descriptors that gracefully handles EAGAIN/EWOULDBLOCK errors without destroying the stream, along with a regression test validating the behavior. 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 47-55: When the fs.read callback sees EAGAIN you currently emit
the error and return, but that leaves the Readable's internal "reading" state
set and the stream will stall; update the EAGAIN branch in the fs.read callback
(the function passed to require("node:fs").read in tty._read) to clear the
reading state so future reads can proceed — either call
this.push(Buffer.alloc(0)) (or push an empty chunk) immediately after
this.emit("error", er) to clear the reading flag without ending the stream, or
schedule a retry (e.g., setImmediate(() => this._read(n))) so _read runs again;
do not call this.push(null) (EOF) unless you intend to end the stream.
- Around line 37-38: The Prototype methods on tty.ReadStream (Prototype._read,
Prototype.ref, Prototype.unref, Prototype.setRawMode) are missing explicit
TypeScript `this` parameter types; update each method signature to add a typed
`this` parameter (e.g., `this: ReadStream` or the appropriate tty.ReadStream
type) so the methods adhere to the builtin guideline and support direct binding
from native code. Locate the methods on the Prototype object and prepend the
`this` parameter to each function declaration, keeping the existing parameter
list and implementation unchanged.
In `@test/regression/issue/27285.test.ts`:
- Around line 49-60: Replace the time-based wait with an event-driven wait:
create and await a Promise that listens for the stream "error" event (use
stream.on or stream.once) and resolves only when err.code === "EAGAIN" or
"EWOULDBLOCK", and rejects (hard-fails) for any other error; keep the call to
stream.resume() to trigger the read, and remove the setTimeout-based Promise so
the test only proceeds once the EAGAIN condition is observed.
- Around line 9-23: The test "tty.ReadStream does not close PTY fd on EAGAIN"
calls POSIX APIs via dlopen (openpty, fcntl, ioctl) and must be skipped on
non-POSIX platforms; add an early guard at the top of that test (before calling
dlopen) that returns immediately when process.platform === "win32" (matching
other tests like tty-readstream-ref-unref.test.ts), so the dlopen and subsequent
openpty/fcntl/ioctl calls are never executed on Windows (and avoid CI failures
on non-POSIX environments).
| Prototype._read = function (n) { | ||
| n = this.pos !== undefined ? $min(this.end - this.pos + 1, n) : $min(this.end - this.bytesRead + 1, n); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file and content at lines 37-38
head -40 src/js/node/tty.ts | tail -15Repository: oven-sh/bun
Length of output: 848
🏁 Script executed:
# Check if the file exists and its size
ls -la src/js/node/tty.ts
wc -l src/js/node/tty.tsRepository: oven-sh/bun
Length of output: 145
🏁 Script executed:
# Search for existing this parameter typing patterns in the codebase
rg -n "this:\s*\w+" src/js/node src/js/builtins src/js/internal -t ts | head -30Repository: oven-sh/bun
Length of output: 3121
🏁 Script executed:
# Search for ReadStream type definitions
rg -n "interface\s+ReadStream|type\s+ReadStream|class ReadStream" src/js/node -t tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check the broader context around lines 37-75 of tty.ts
sed -n '30,80p' src/js/node/tty.tsRepository: oven-sh/bun
Length of output: 2005
🏁 Script executed:
# Check what type Prototype should be - look for ReadStream interface/type
rg -n "export\s+(interface|type|class)\s+ReadStream" src/js/node src/js/builtins src/js/internal -t tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check how fs.ReadStream is used/typed in this file context
rg -n "fs\.ReadStream|ReadStream" src/js/node/tty.ts -B2 -A2Repository: oven-sh/bun
Length of output: 1641
🏁 Script executed:
# Check if there's a declare or import for ReadStream type
head -30 src/js/node/tty.tsRepository: oven-sh/bun
Length of output: 976
🏁 Script executed:
# Check all methods in the Prototype object to see if they all lack this typing
sed -n '37,145p' src/js/node/tty.ts | grep -n "function\s*("Repository: oven-sh/bun
Length of output: 217
🏁 Script executed:
# Also check the ReadStream constructor at line 15
sed -n '15,20p' src/js/node/tty.tsRepository: oven-sh/bun
Length of output: 226
🏁 Script executed:
# Check the FSStream type definition to see what type should be used
rg -n "type\s+FSStream|interface\s+FSStream" src/js/internal/fs/streams.ts -B2 -A5Repository: oven-sh/bun
Length of output: 287
🏁 Script executed:
# Check how fs.ReadStream functions are typed in streams.ts to match the pattern
sed -n '130,140p' src/js/internal/fs/streams.tsRepository: oven-sh/bun
Length of output: 273
🏁 Script executed:
# Check if other methods in the same Prototype (ref, unref, setRawMode) also lack this typing
sed -n '37,90p' src/js/node/tty.ts | grep -A1 "function"Repository: oven-sh/bun
Length of output: 401
Add explicit this parameter typing to all Prototype methods.
Multiple methods on the tty.ReadStream Prototype lack explicit this type annotation, violating the builtin TypeScript guideline. This includes _read, ref, unref, and setRawMode.
- Prototype._read = function (n) {
+ Prototype._read = function (this: any, n) {
- Prototype.ref = function () {
+ Prototype.ref = function (this: any) {
- Prototype.unref = function () {
+ Prototype.unref = function (this: any) {
- Prototype.setRawMode = function (flag) {
+ Prototype.setRawMode = function (this: any, flag) {Per coding guidelines, "src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts: Builtin functions must include this parameter typing in TypeScript to enable direct method binding in C++".
🤖 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 37 - 38, The Prototype methods on
tty.ReadStream (Prototype._read, Prototype.ref, Prototype.unref,
Prototype.setRawMode) are missing explicit TypeScript `this` parameter types;
update each method signature to add a typed `this` parameter (e.g., `this:
ReadStream` or the appropriate tty.ReadStream type) so the methods adhere to the
builtin guideline and support direct binding from native code. Locate the
methods on the Prototype object and prepend the `this` parameter to each
function declaration, keeping the existing parameter list and implementation
unchanged.
| require("node:fs").read(this.fd, buf, 0, n, this.pos, (er, bytesRead, buf) => { | ||
| if (er) { | ||
| if (er.code === "EAGAIN" || er.code === "EWOULDBLOCK") { | ||
| // Non-blocking fd has no data available yet - just emit the | ||
| // error so user-space handlers (e.g. node-pty) can handle it, | ||
| // but do NOT destroy the stream or close the fd. | ||
| this.emit("error", er); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/js/node/tty.ts | head -70Repository: oven-sh/bun
Length of output: 3068
🏁 Script executed:
cat -n src/js/node/tty.ts | tail -20Repository: oven-sh/bun
Length of output: 912
🏁 Script executed:
cat -n src/js/node/tty.ts | sed -n '70,100p'Repository: oven-sh/bun
Length of output: 1113
🏁 Script executed:
fd -t f "streams" src/js/internal/fs/Repository: oven-sh/bun
Length of output: 84
🏁 Script executed:
wc -l src/js/internal/fs/streams.tsRepository: oven-sh/bun
Length of output: 88
🏁 Script executed:
rg -n "class ReadStream|_read\(" src/js/internal/fs/streams.tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
rg -n "ReadStream|_read" src/js/internal/fs/streams.ts | head -30Repository: oven-sh/bun
Length of output: 1036
🏁 Script executed:
sed -n '301,350p' src/js/internal/fs/streams.tsRepository: oven-sh/bun
Length of output: 1603
🏁 Script executed:
rg -n "reading\s*=|_readableState" src/js/internal/stream.ts | head -40Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
fd -t f "stream" src/js/internal/ | grep -v ".d.ts"Repository: oven-sh/bun
Length of output: 225
🏁 Script executed:
wc -l src/js/internal/stream.ts && rg -n "class Readable|reading\s*=" src/js/internal/stream.ts | head -40Repository: oven-sh/bun
Length of output: 84
🏁 Script executed:
cat -n src/js/internal/stream.tsRepository: oven-sh/bun
Length of output: 4581
🏁 Script executed:
wc -l src/js/internal/streams/readable.ts && rg -n "reading\s*=|class Readable|_read" src/js/internal/streams/readable.ts | head -50Repository: oven-sh/bun
Length of output: 2856
🏁 Script executed:
rg -n "\.reading\s*=" src/js/internal/streams/readable.tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
rg -n "reading" src/js/internal/streams/readable.ts | head -60Repository: oven-sh/bun
Length of output: 805
🏁 Script executed:
sed -n '620,680p' src/js/internal/streams/readable.tsRepository: oven-sh/bun
Length of output: 2428
Confirm EAGAIN path leaves stream stuck in reading state - requires fix.
In tty.ts lines 49–54, when EAGAIN occurs, the code emits an error and returns without pushing data. However, the reading flag is set by the parent Readable class before _read() is invoked (see readable.ts line 654) and is only cleared when push() is called or when the implementation explicitly manages state.
On the EAGAIN path, since no push() call occurs and the callback returns early, the reading flag remains set. The next call to read() will see the flag still true and skip invoking _read() again (see readable.ts line 648–650: if ((state[kState] & (kReading | ...)) !== kConstructed) { doRead = false; }), causing the stream to stall.
To fix this, either clear the reading state after emitting the error (e.g., by calling push() with no data, or manually resetting the flag) or reschedule the read attempt so the stream can retry when data becomes available.
🤖 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 47 - 55, When the fs.read callback sees
EAGAIN you currently emit the error and return, but that leaves the Readable's
internal "reading" state set and the stream will stall; update the EAGAIN branch
in the fs.read callback (the function passed to require("node:fs").read in
tty._read) to clear the reading state so future reads can proceed — either call
this.push(Buffer.alloc(0)) (or push an empty chunk) immediately after
this.emit("error", er) to clear the reading flag without ending the stream, or
schedule a retry (e.g., setImmediate(() => this._read(n))) so _read runs again;
do not call this.push(null) (EOF) unless you intend to end the stream.
| test("tty.ReadStream does not close PTY fd on EAGAIN", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const tty = require("tty"); | ||
| const { dlopen } = require("bun:ffi"); | ||
|
|
||
| const suffix = process.platform === "darwin" ? "dylib" : "so.6"; | ||
| const { symbols } = dlopen("libc." + suffix, { | ||
| openpty: { args: ["ptr", "ptr", "ptr", "ptr", "ptr"], returns: "int" }, | ||
| close: { args: ["int"], returns: "int" }, | ||
| ioctl: { args: ["int", "u64", "ptr"], returns: "int" }, | ||
| fcntl: { args: ["int", "int", "int"], returns: "int" }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check CI coverage for Windows and other platforms
echo "=== Checking CI workflows for platform coverage ==="
rg -n "windows|win32|musl|alpine" .github/workflows | head -30
echo ""
echo "=== Checking dlopen(\"libc patterns ==="
rg -n 'dlopen\("libc' test src | head -20
echo ""
echo "=== Checking openpty usage ==="
rg -n 'openpty' test | head -20Repository: oven-sh/bun
Length of output: 1979
🏁 Script executed:
#!/bin/bash
# Check the test file itself and similar platform guards in tests
echo "=== Viewing the full test file ==="
cat -n test/regression/issue/27285.test.ts | head -50
echo ""
echo "=== Checking for platform guards in other regression tests ==="
rg -B2 'process\.platform.*win32|return' test/regression/issue/*.test.ts | head -40Repository: oven-sh/bun
Length of output: 5204
Guard the PTY/libc regression test on Windows and other non-POSIX platforms.
The test directly calls POSIX functions (openpty, fcntl, ioctl) via dlopen("libc.so.6"), which will fail on Windows and musl-based Linux. Similar TTY tests in the same directory (tui-app-tty-pattern.test.ts, tty-readstream-ref-unref.test.ts) already guard against this with if (process.platform === "win32") { return; }. Add the same guard to prevent CI failures on Windows.
Suggested fix
test("tty.ReadStream does not close PTY fd on EAGAIN", async () => {
+ if (process.platform === "win32") return;
await using proc = Bun.spawn({📝 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.
| test("tty.ReadStream does not close PTY fd on EAGAIN", async () => { | |
| await using proc = Bun.spawn({ | |
| cmd: [ | |
| bunExe(), | |
| "-e", | |
| ` | |
| const tty = require("tty"); | |
| const { dlopen } = require("bun:ffi"); | |
| const suffix = process.platform === "darwin" ? "dylib" : "so.6"; | |
| const { symbols } = dlopen("libc." + suffix, { | |
| openpty: { args: ["ptr", "ptr", "ptr", "ptr", "ptr"], returns: "int" }, | |
| close: { args: ["int"], returns: "int" }, | |
| ioctl: { args: ["int", "u64", "ptr"], returns: "int" }, | |
| fcntl: { args: ["int", "int", "int"], returns: "int" }, | |
| test("tty.ReadStream does not close PTY fd on EAGAIN", async () => { | |
| if (process.platform === "win32") return; | |
| await using proc = Bun.spawn({ | |
| cmd: [ | |
| bunExe(), | |
| "-e", | |
| ` | |
| const tty = require("tty"); | |
| const { dlopen } = require("bun:ffi"); | |
| const suffix = process.platform === "darwin" ? "dylib" : "so.6"; | |
| const { symbols } = dlopen("libc." + suffix, { | |
| openpty: { args: ["ptr", "ptr", "ptr", "ptr", "ptr"], returns: "int" }, | |
| close: { args: ["int"], returns: "int" }, | |
| ioctl: { args: ["int", "u64", "ptr"], returns: "int" }, | |
| fcntl: { args: ["int", "int", "int"], returns: "int" }, |
🤖 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 9 - 23, The test
"tty.ReadStream does not close PTY fd on EAGAIN" calls POSIX APIs via dlopen
(openpty, fcntl, ioctl) and must be skipped on non-POSIX platforms; add an early
guard at the top of that test (before calling dlopen) that returns immediately
when process.platform === "win32" (matching other tests like
tty-readstream-ref-unref.test.ts), so the dlopen and subsequent
openpty/fcntl/ioctl calls are never executed on Windows (and avoid CI failures
on non-POSIX environments).
| // Handle EAGAIN errors (node-pty does this too) | ||
| stream.on("error", (err) => { | ||
| if (err.code === "EAGAIN" || err.code === "EWOULDBLOCK") { | ||
| // Expected - no data available on non-blocking fd | ||
| } | ||
| }); | ||
|
|
||
| // Start reading to trigger _read -> fs.read -> EAGAIN | ||
| stream.resume(); | ||
|
|
||
| // Wait for the event loop to process the EAGAIN | ||
| await new Promise((resolve) => setTimeout(resolve, 200)); |
There was a problem hiding this comment.
Avoid time-based waits; await the EAGAIN event and fail on unexpected errors.
setTimeout is disallowed in tests and the current handler doesn’t guarantee EAGAIN occurred or fail on other errors. Await the first EAGAIN event and hard‑fail on unexpected errors so the regression actually exercises the bug.
🛠️ Suggested change (event‑driven EAGAIN wait)
-// Handle EAGAIN errors (node-pty does this too)
-stream.on("error", (err) => {
- if (err.code === "EAGAIN" || err.code === "EWOULDBLOCK") {
- // Expected - no data available on non-blocking fd
- }
-});
-
-// Start reading to trigger _read -> fs.read -> EAGAIN
-stream.resume();
-
-// Wait for the event loop to process the EAGAIN
-await new Promise((resolve) => setTimeout(resolve, 200));
+// Handle EAGAIN errors (node-pty does this too) and await the first one
+let sawEagain = false;
+const eagainReady = new Promise((resolve) => {
+ stream.on("error", (err) => {
+ if (err.code === "EAGAIN" || err.code === "EWOULDBLOCK") {
+ if (!sawEagain) {
+ sawEagain = true;
+ resolve();
+ }
+ return;
+ }
+ console.error("FAIL: unexpected stream error", err);
+ process.exit(1);
+ });
+});
+
+// Start reading to trigger _read -> fs.read -> EAGAIN
+stream.resume();
+
+// Await the EAGAIN instead of a time delay
+await eagainReady;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.
| // Handle EAGAIN errors (node-pty does this too) | |
| stream.on("error", (err) => { | |
| if (err.code === "EAGAIN" || err.code === "EWOULDBLOCK") { | |
| // Expected - no data available on non-blocking fd | |
| } | |
| }); | |
| // Start reading to trigger _read -> fs.read -> EAGAIN | |
| stream.resume(); | |
| // Wait for the event loop to process the EAGAIN | |
| await new Promise((resolve) => setTimeout(resolve, 200)); | |
| // Handle EAGAIN errors (node-pty does this too) and await the first one | |
| let sawEagain = false; | |
| const eagainReady = new Promise((resolve) => { | |
| stream.on("error", (err) => { | |
| if (err.code === "EAGAIN" || err.code === "EWOULDBLOCK") { | |
| if (!sawEagain) { | |
| sawEagain = true; | |
| resolve(); | |
| } | |
| return; | |
| } | |
| console.error("FAIL: unexpected stream error", err); | |
| process.exit(1); | |
| }); | |
| }); | |
| // Start reading to trigger _read -> fs.read -> EAGAIN | |
| stream.resume(); | |
| // Await the EAGAIN instead of a time delay | |
| await eagainReady; |
🤖 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 49 - 60, Replace the
time-based wait with an event-driven wait: create and await a Promise that
listens for the stream "error" event (use stream.on or stream.once) and resolves
only when err.code === "EAGAIN" or "EWOULDBLOCK", and rejects (hard-fails) for
any other error; keep the call to stream.resume() to trigger the read, and
remove the setTimeout-based Promise so the test only proceeds once the EAGAIN
condition is observed.
1 participant
Summary
Fixes #27285
_readontty.ReadStream.prototypeto handle EAGAIN/EWOULDBLOCK gracefully instead of callingerrorOrDestroy()which auto-destroys the stream and closes the file descriptorfs.read()returns EAGAIN. The inheritedfs.ReadStream._read()treats this as a fatal error, destroying the stream and closing the fd. This causes@lydell/node-pty(used by Gemini CLI) to crash withioctl(2) failed, EBADFon subsequent operations likeresize()Root cause
Bun's
tty.ReadStreamextendsfs.ReadStream(Node.js extendsnet.Socketinstead, which uses poll-based I/O and never encounters EAGAIN). When node-pty createsnew tty.ReadStream(masterFd)on a non-blocking PTY master fd:_read()callsfs.read()on the non-blocking fdfs.read()returns EAGAINerrorOrDestroy()is called withautoDestroy: true(default)ioctl()call on the closed fd returns EBADFFix
The
tty.ReadStreamprototype now overrides_readto intercept EAGAIN/EWOULDBLOCK errors. Instead of destroying the stream, it emits the error event so user-space handlers (like node-pty's EAGAIN handler) can handle it without the fd being closed.Test plan
test/regression/issue/27285.test.tsopens a PTY pair, sets master fd to non-blocking, creates atty.ReadStream, and verifies the fd remains valid after EAGAINtty-readstream-ref-unref,tty-reopen-after-stdin-eof,tui-app-tty-pattern,tty.test.ts,nodettywrap.test.ts)🤖 Generated with Claude Code