From 820f7aa74a63fd35fdf2e8136e34d10973e9bf67 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Mon, 12 Jan 2026 11:37:49 +0000 Subject: [PATCH] fix(tty): prevent tty.ReadStream from closing caller-owned PTY fds When node-pty (or other PTY libraries) passes a PTY master fd to tty.ReadStream, Bun's implementation would: 1. Auto-close the fd when the stream was destroyed, closing the fd that node-pty still owned 2. Treat EAGAIN errors from non-blocking PTY reads as fatal, destroying the stream prematurely This caused the shell process to receive SIGHUP because the PTY master was unexpectedly closed. Changes: - Set autoClose: false in tty.ReadStream when an fd is provided - Handle EAGAIN in fs.ReadStream._read() by retrying instead of destroying the stream Fixes #25822 Co-Authored-By: Claude Opus 4.5 --- src/js/internal/fs/streams.ts | 7 ++ src/js/node/tty.ts | 4 +- test/regression/issue/25822.test.ts | 119 ++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 test/regression/issue/25822.test.ts diff --git a/src/js/internal/fs/streams.ts b/src/js/internal/fs/streams.ts index 6508b0db72..fc54a5cd14 100644 --- a/src/js/internal/fs/streams.ts +++ b/src/js/internal/fs/streams.ts @@ -310,6 +310,13 @@ readStreamPrototype._read = function (n) { this[kFs].read(this.fd, buf, 0, n, this.pos, (er, bytesRead, buf) => { if (er) { + // EAGAIN means no data available on a non-blocking fd (e.g., PTY master). + // This is not a fatal error - we should just wait and try again later. + if (er.code === "EAGAIN") { + // Schedule a retry after a short delay to avoid busy-waiting + setTimeout(() => this._read(n), 10); + return; + } require("internal/streams/destroy").errorOrDestroy(this, er); } else if (bytesRead > 0) { if (this.pos !== undefined) { diff --git a/src/js/node/tty.ts b/src/js/node/tty.ts index 8972f77d27..5fceaf4d43 100644 --- a/src/js/node/tty.ts +++ b/src/js/node/tty.ts @@ -16,7 +16,9 @@ function ReadStream(fd): void { if (!(this instanceof ReadStream)) { return new ReadStream(fd); } - fs.ReadStream.$apply(this, ["", { fd }]); + // When an fd is provided, we must not auto-close it - the caller owns it. + // This is critical for node-pty and other PTY libraries that manage their own fds. + fs.ReadStream.$apply(this, ["", { fd, autoClose: false }]); this.isRaw = false; // Only set isTTY to true if the fd is actually a TTY this.isTTY = isatty(fd); diff --git a/test/regression/issue/25822.test.ts b/test/regression/issue/25822.test.ts new file mode 100644 index 0000000000..f02080f9e8 --- /dev/null +++ b/test/regression/issue/25822.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, isWindows } from "harness"; + +// This test verifies that tty.ReadStream properly handles PTY file descriptors. +// Issue #25822: node-pty's onData callback never fires because: +// 1. tty.ReadStream auto-closed the PTY fd that node-pty owned +// 2. EAGAIN errors from non-blocking PTY reads destroyed the stream prematurely + +describe.skipIf(isWindows)("tty.ReadStream with PTY", () => { + test("should not auto-close fd passed to tty.ReadStream", async () => { + // Verify that tty.ReadStream sets autoClose: false when fd is passed + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const tty = require('tty'); + const fs = require('fs'); + + // Create a pipe to simulate an fd we own + const { createPipe } = require('child_process').ChildProcess.prototype.constructor; + + // Use stdout fd (1) for testing - we don't want to close it + const stream = new tty.ReadStream(1); + + // Check that autoClose is false (stream won't close our fd) + console.log('autoClose:', stream.autoClose); + + // Manually destroy to trigger cleanup + stream.destroy(); + + // Give time for any async cleanup + setTimeout(() => { + // If autoClose was true, writing to stdout would fail + console.log('stdout still works'); + }, 100); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout).toContain("autoClose: false"); + expect(stdout).toContain("stdout still works"); + expect(exitCode).toBe(0); + }); + + test("node-pty should receive data from spawned process", async () => { + // First check if node-pty is available + const checkPty = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + "try { require.resolve('node-pty'); console.log('found'); } catch { console.log('not-found'); }", + ], + env: bunEnv, + stdout: "pipe", + }); + + const checkResult = await checkPty.stdout.text(); + await checkPty.exited; + + if (checkResult.trim() !== "found") { + console.log("Skipping node-pty test - node-pty not installed"); + return; + } + + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const pty = require('node-pty'); + + const shell = pty.spawn('/bin/echo', ['test-output-12345'], { + name: 'xterm-256color', + cols: 80, + rows: 24, + }); + + let dataReceived = ''; + shell.onData((data) => { + dataReceived += data; + }); + + shell.onExit((e) => { + if (dataReceived.includes('test-output-12345')) { + console.log('SUCCESS: received expected data'); + } else { + console.log('FAILURE: data was:', JSON.stringify(dataReceived)); + } + console.log('exit code:', e.exitCode); + process.exit(e.exitCode === 0 && dataReceived.includes('test-output-12345') ? 0 : 1); + }); + + // Timeout in case onData/onExit never fire + setTimeout(() => { + console.log('TIMEOUT: no exit event'); + console.log('data received:', JSON.stringify(dataReceived)); + process.exit(1); + }, 5000); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stdout).toContain("SUCCESS: received expected data"); + expect(stdout).toContain("exit code: 0"); + expect(exitCode).toBe(0); + }); +});