From 8f46541c9db88ea9a071699304731a67fc36acbd Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 01:11:23 -0700 Subject: [PATCH] fix(pty): retire the write stream's fd copy too, and keep the resize re-probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps in the fd-retirement change. `CustomWriteStream` is the third fd-addressed surface and takes its own plain-number copy of the master at spawn, so `Terminal._fd = -1` never reaches it. Its `dispose()` only cleared a pending immediate, and it was called from `destroy()` alone — the EIO/EOF read-error path and the socket `'close'` path both reach `_close()` without it. A residual `_writeQueue` and an in-flight `fs.write` therefore kept addressing the number after libuv gave it up. Retire the copy and drop the queue in `dispose()`, and call it from `_close()`. Restore the relay's catch-block re-probe. It was never redundant with the patch: libuv closes the fd synchronously inside `uv_close`, before the JS `'close'` that runs `_close()`, so the reuse window opens ahead of any JS guard; and a relay host installs node-pty from npm, where the patch is not applied at all. The re-probe is a `process.kill(pid, 0)` on a path already inside an exception handler. Both existing resize tests pinned liveness with a constant mock, which is why the deletion passed CI invisibly; the new case is alive at the pre-probe and gone by the time the ioctl throws. Also adds the patch's first upstream reference: microsoft/node-pty#220 named this mechanism in 2018 and was closed 2025-12-19 as completed after only improving the error message; #827 is still open. Tracked as #18109. --- config/patches/node-pty@1.1.0.patch | 42 ++++++++++- pnpm-lock.yaml | 6 +- .../pty/node-pty-master-fd-retirement.test.ts | 75 +++++++++++++++++-- .../pty-handler-resize-stale-pty.test.ts | 29 ++++++- src/relay/pty-handler.ts | 22 ++++-- 5 files changed, 150 insertions(+), 24 deletions(-) diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index f008b7c2adf..ff474f7d95e 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -139,10 +139,10 @@ index 8c4fca9022a6d6f015bca87f61625cde2278f428..0a01730616488119aa21ef441cf3c441 //# sourceMappingURL=conpty_console_list_agent.js.map \ No newline at end of file diff --git a/lib/terminal.js b/lib/terminal.js -index e2f9bc9131077b53ebc32d207207ad82804ff185..a6af9300900eafa1c695c4207ba550f64937e542 100644 +index e2f9bc9131077b53ebc32d207207ad82804ff185..6c63bfaaf75128d88f9a2efece13476348780cfd 100644 --- a/lib/terminal.js +++ b/lib/terminal.js -@@ -172,6 +172,11 @@ var Terminal = /** @class */ (function () { +@@ -172,6 +172,21 @@ var Terminal = /** @class */ (function () { this.end = function () { }; this._writable = false; this._readable = false; @@ -150,12 +150,22 @@ index e2f9bc9131077b53ebc32d207207ad82804ff185..a6af9300900eafa1c695c4207ba550f6 + // that number straight to the next open(2). Retire it in the same block + // that gives up the handle so no later ioctl can address a reused fd. + // Inert on Windows, where `_fd` is written once and never read back. ++ // Upstream named this mechanism in microsoft/node-pty#220 ("fd number got ++ // reattached to something else"), closed 2025-12-19 as completed after ++ // only improving the error message; #827 is still open. Windows guards in ++ // windowsPtyAgent.ts, Unix does not. Orca tracking: #18109. + this._fd = -1; ++ // Orca: the write stream holds its own copy of that number, so retiring ++ // `_fd` alone leaves the queued and in-flight writes addressing it. ++ // Undefined on Windows and on `UnixTerminal.open()` handles. ++ if (this._writeStream) { ++ this._writeStream.dispose(); ++ } }; Terminal.prototype._parseEnv = function (env) { var keys = Object.keys(env || {}); diff --git a/lib/unixTerminal.js b/lib/unixTerminal.js -index 1ec12f796a822c78fba9ad7f6448c3987e325c23..09176dcccaa3a040acfbb8ee9534f020f034befd 100644 +index 1ec12f796a822c78fba9ad7f6448c3987e325c23..d838d795ecb9ea72e3bcc31113344947c006af7e 100644 --- a/lib/unixTerminal.js +++ b/lib/unixTerminal.js @@ -28,8 +28,12 @@ var native = utils_1.loadNativeModule('pty'); @@ -197,6 +207,32 @@ index 1ec12f796a822c78fba9ad7f6448c3987e325c23..09176dcccaa3a040acfbb8ee9534f020 pty.resize(this._fd, cols, rows); this._cols = cols; this._rows = rows; +@@ -287,8 +301,15 @@ var CustomWriteStream = /** @class */ (function () { + CustomWriteStream.prototype.dispose = function () { + clearImmediate(this._writeImmediate); + this._writeImmediate = undefined; ++ // Orca: retire this stream's own copy of the master fd and drop what has ++ // not shipped, so nothing queued here reaches a reused descriptor. ++ this._fd = -1; ++ this._writeQueue.length = 0; + }; + CustomWriteStream.prototype.write = function (data) { ++ if (this._fd < 0) { ++ return; ++ } + // Writes are put in a queue and processed asynchronously in order to handle + // backpressure from the kernel buffer. + var buffer = typeof data === 'string' +@@ -304,7 +325,8 @@ var CustomWriteStream = /** @class */ (function () { + CustomWriteStream.prototype._processWriteQueue = function () { + var _this = this; + this._writeImmediate = undefined; +- if (this._writeQueue.length === 0) { ++ // Orca: an in-flight fs.write can re-enter here after dispose(). ++ if (this._fd < 0 || this._writeQueue.length === 0) { + return; + } + var task = this._writeQueue[0]; diff --git a/src/conpty_console_list_agent.ts b/src/conpty_console_list_agent.ts index 181ccabbbe9c4948a9725fb1db907a68e9de01fc..67f31facf85562b67adbfbd04ce28ddd8eeb4a79 100644 --- a/src/conpty_console_list_agent.ts diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 33cd3c2c1ba..eac56401344 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -115,7 +115,7 @@ patchedDependencies: '@xterm/addon-webgl@0.20.0-beta.299': 94687e89a0115e6e6aa102837f986debdc029c091527ee5eb4a4e17ceaf9473e '@xterm/xterm@6.1.0-beta.303': 98756bcedc402bcdb7c6ab7b015d2e59cd18e97b03a2c06a27e95bb3ba429d9d lint-staged@16.4.0: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 - node-pty@1.1.0: 929c703f3743efdd25929a6f7fdca716e3b54fb4a931bc32e1481633ea17defd + node-pty@1.1.0: e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa importers: @@ -156,7 +156,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=929c703f3743efdd25929a6f7fdca716e3b54fb4a931bc32e1481633ea17defd) + version: 1.1.0(patch_hash=e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12194,7 +12194,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=929c703f3743efdd25929a6f7fdca716e3b54fb4a931bc32e1481633ea17defd): + node-pty@1.1.0(patch_hash=e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/pty/node-pty-master-fd-retirement.test.ts b/src/main/pty/node-pty-master-fd-retirement.test.ts index 66571c78a99..6f208dd16de 100644 --- a/src/main/pty/node-pty-master-fd-retirement.test.ts +++ b/src/main/pty/node-pty-master-fd-retirement.test.ts @@ -3,10 +3,16 @@ import { describe, expect, it } from 'vitest' /** * node-pty hands the master fd to libuv, which closes it on EIO/EOF, but upstream - * never invalidated `_fd`, and neither `resize()` nor the `process` getter consulted - * anything. Orca's patch retires `_fd` in the same block that gives up the handle - * (config/patches/node-pty@1.1.0.patch), which is what makes a stale handle - * unreachable instead of merely unlucky. + * never invalidated `_fd`, and none of the three fd-addressed surfaces consulted + * anything: `resize()`, the `process` getter, and `CustomWriteStream`, which holds + * its own plain-number copy of the fd taken at spawn. Orca's patch retires all + * three in the same block that gives up the handle + * (config/patches/node-pty@1.1.0.patch). + * + * Scope: this narrows the window, it does not close it. libuv closes the fd + * synchronously inside `uv_close`, before the JS `'close'` that runs `_close()`, + * so callers still need their own liveness verdict for that tick — and a relay + * host installs node-pty from npm, where this patch is not applied at all. */ const POSIX_SHELL = '/bin/sh' @@ -25,14 +31,36 @@ function masterFd(term: pty.IPty): number { return (term as unknown as { fd: number }).fd } -/** Run `command` to completion and let node-pty finish giving up the master. */ -async function retiredPty(command = 'exit 0'): Promise<{ term: pty.IPty; spawnFd: number }> { +type CustomWriteStream = { _fd: number; _writeQueue: unknown[]; write(data: string): void } + +/** + * `Terminal._close()` already shadows `terminal.write` with a no-op, so the stream + * itself is the surface that still reached the fd: a residual `_writeQueue` and an + * in-flight `fs.write` both re-enter it after the close. + */ +function writeStream(term: pty.IPty): CustomWriteStream { + return (term as unknown as { _writeStream: CustomWriteStream })._writeStream +} + +/** + * Run `command` to completion and let node-pty finish giving up the master. + * + * `destroy: false` exercises only the EIO/EOF read-error path, which reaches + * `_close()` without ever calling `destroy()` — the path the exit of a shell + * actually takes, and the one the write stream was previously never told about. + */ +async function retiredPty( + command = 'exit 0', + { destroy = true }: { destroy?: boolean } = {} +): Promise<{ term: pty.IPty; spawnFd: number }> { const term = spawnPty(command) const spawnFd = masterFd(term) await new Promise((resolve) => { term.onExit(() => resolve()) }) - ;(term as unknown as { destroy?: () => void }).destroy?.() + if (destroy) { + ;(term as unknown as { destroy?: () => void }).destroy?.() + } await new Promise((resolve) => setTimeout(resolve, 400)) return { term, spawnFd } } @@ -59,6 +87,17 @@ describeOnPosix('node-pty master fd retirement', () => { expect([term.cols, term.rows]).toEqual([80, 24]) }, 15000) + it('retires the write stream fd on _close(), not only on destroy()', async () => { + const { term } = await retiredPty('exit 0', { destroy: false }) + + // The stream copied the fd number at spawn, so `Terminal._fd = -1` alone + // leaves it addressing a descriptor the kernel may already have reissued. + expect(writeStream(term)._fd).toBe(-1) + + writeStream(term).write('x') + expect(writeStream(term)._writeQueue).toHaveLength(0) + }, 15000) + it('names the spawn file rather than tcgetpgrp on a retired descriptor', async () => { const { term } = await retiredPty() @@ -99,6 +138,28 @@ describeOnLinux('node-pty master fd reuse', () => { } }, 15000) + it('cannot write into a live pty handed the retired descriptor number', async () => { + const { term: retired, spawnFd } = await retiredPty('exit 0', { destroy: false }) + + const live = spawnPty('read -r line; printf "got:%s" "$line"') + let output = '' + live.onData((data) => { + output += data + }) + try { + expect(masterFd(live)).toBe(spawnFd) + + // Pre-patch this fs.write reached `live`'s master and the tty echoed it + // back: a retired pane's bytes landing in an unrelated terminal. + writeStream(retired).write('leak\r') + await new Promise((resolve) => setTimeout(resolve, 300)) + + expect(output).not.toContain('leak') + } finally { + live.kill() + } + }, 15000) + it('does not name a live pty foreground process off the retired descriptor', async () => { const { term: retired, spawnFd } = await retiredPty() diff --git a/src/relay/pty-handler-resize-stale-pty.test.ts b/src/relay/pty-handler-resize-stale-pty.test.ts index c93e2e3c16b..02039600bb0 100644 --- a/src/relay/pty-handler-resize-stale-pty.test.ts +++ b/src/relay/pty-handler-resize-stale-pty.test.ts @@ -39,10 +39,11 @@ const STALE_PID = 424_242 /** * node-pty's native `pty.resize` error when the ioctl reaches a closed master. * - * Orca's node-pty patch retires `_fd` when it gives up the master, so a real - * handle answers a late resize with a no-op instead. What is left for this - * handler to contain is the tick between libuv closing the fd on a read error - * and node-pty's own handler observing it. + * Orca's node-pty patch retires `_fd` when it gives up the master, so a patched + * handle answers a late resize with a no-op. That leaves this handler two cases + * it still has to contain: the tick between libuv closing the fd and node-pty's + * own handler observing it, and a relay host, which installs node-pty from npm + * and has no such guard. */ function ebadfResize(): never { throw new Error('ioctl(2) failed, EBADF') @@ -109,6 +110,26 @@ describe('PtyHandler.resize against a stale PTY handle', () => { ) }) + it('retires the entry when the pid goes absent between the pre-probe and the ioctl', () => { + // The race the catch-block re-probe exists for, and the one a constant + // liveness mock cannot express: alive when the pre-probe asks, gone by the + // time the ioctl fails. libuv closes the master synchronously inside + // `uv_close`, so this window opens before any JS guard can be set — and on + // a relay host, where node-pty comes from npm, there is no JS guard at all. + vi.spyOn(ptyShellUtils, 'isProcessAlive').mockReturnValueOnce(true).mockReturnValueOnce(false) + resize.mockImplementation(ebadfResize) + const stderr = vi.spyOn(process.stderr, 'write').mockReturnValue(true) + + expect(() => + dispatcher.callNotification('pty.resize', { id: PTY_1, cols: 120, rows: 40 }) + ).not.toThrow() + + expect(resize).toHaveBeenCalledTimes(1) + expect(handler.activePtyCount).toBe(0) + // Proven `exited` retires silently; only live-or-unverifiable is reported. + expect(stderr).not.toHaveBeenCalled() + }) + it('still resizes a live PTY, with the clamped geometry', () => { vi.spyOn(ptyShellUtils, 'isProcessAlive').mockReturnValue(true) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index fa908ae350b..87853bf4ce8 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -2070,17 +2070,25 @@ export class PtyHandler { if (this.reapPtyProvenExited(managed)) { return } - // Safety is the handle's own, not this probe's: node-pty retires `_fd` in - // the same block that gives up the master (config/patches/node-pty@1.1.0.patch), - // so a resize past that point is a no-op instead of a TIOCSWINSZ aimed at a - // closed — or kernel-reused — descriptor. What survives is the tick between - // libuv closing the fd on a read error and node-pty's handler seeing it, and - // an EBADF there is `unverifiable`: it observed the handle, not the host. + // The patched node-pty retires `_fd` in the same block that gives up the + // master (config/patches/node-pty@1.1.0.patch), which makes a resize past + // that point a no-op rather than a TIOCSWINSZ aimed at a reused descriptor. + // That covers only part of the window and does not cover this process at + // all: libuv closes the fd synchronously inside `uv_close`, before the JS + // `'close'` that runs `_close()`, and a relay host installs node-pty from + // npm, where the patch is not applied. So the catch below stays. try { managed.pty.resize(cols, rows) } catch (err) { + // A failed ioctl observed the handle, not the host's process table, so on + // its own it is `unverifiable`. Re-probe: a now-absent pid retires the + // entry, anything else keeps it and is contained here rather than + // escaping as a parse error on every later resize. + if (this.reapPtyProvenExited(managed)) { + return + } process.stderr.write( - `[pty-handler] resize failed for PTY ${id}: ${err instanceof Error ? err.message : String(err)}\n` + `[pty-handler] resize failed for PTY ${id} whose process is still live or unverifiable: ${err instanceof Error ? err.message : String(err)}\n` ) } }