fix(pty): invalidate the descriptor when node-pty gives up the handle (#17930)

Carried forward from PR #17930, which merged into this branch. Rebased onto
current main; main's newer node-pty-fd-leak test is kept as-is.

config/patches/node-pty@1.1.0.patch

@@ -138,8 +138,34 @@ index 8c4fca9022a6d6f015bca87f61625cde2278f428..0a01730616488119aa21ef441cf3c441
  process.exit(0);
  //# 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..6c63bfaaf75128d88f9a2efece13476348780cfd 100644
+--- a/lib/terminal.js
++++ b/lib/terminal.js
+@@ -172,6 +172,21 @@ var Terminal = /** @class */ (function () {
+         this.end = function () { };
+         this._writable = false;
+         this._readable = false;
++        // Orca: libuv closes the master fd on EIO/EOF, and the kernel may hand
++        // 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..cec8b67aef02f8199e5606a0d257088bf1865877 100644
+index 1ec12f796a822c78fba9ad7f6448c3987e325c23..d838d795ecb9ea72e3bcc31113344947c006af7e 100644
 --- a/lib/unixTerminal.js
 +++ b/lib/unixTerminal.js
 @@ -28,8 +28,12 @@ var native = utils_1.loadNativeModule('pty');
@@ -157,6 +183,56 @@ index 1ec12f796a822c78fba9ad7f6448c3987e325c23..cec8b67aef02f8199e5606a0d257088b
  var DEFAULT_FILE = 'sh';
  var DEFAULT_NAME = 'xterm';
  var DESTROY_SOCKET_TIMEOUT_MS = 200;
+@@ -234,6 +238,11 @@ var UnixTerminal = /** @class */ (function (_super) {
+          * Gets the name of the process.
+          */
+         get: function () {
++            // Orca: tcgetpgrp on a retired fd would name whatever process now
++            // owns that descriptor, so a closed master reports the spawn file.
++            if (this._fd < 0) {
++                return this._file;
++            }
+             if (process.platform === 'darwin') {
+                 var title = pty.process(this._fd);
+                 return (title !== 'kernel_task') ? title : this._file;
+@@ -250,6 +259,11 @@ var UnixTerminal = /** @class */ (function (_super) {
+         if (cols <= 0 || rows <= 0 || isNaN(cols) || isNaN(rows) || cols === Infinity || rows === Infinity) {
+             throw new Error('resizing must be done using positive cols and rows');
+         }
++        // Orca: a retired master is unreachable rather than EBADF-or-worse; cols
++        // and rows stay at the last size actually applied instead of a claim.
++        if (this._fd < 0) {
++            return;
++        }
+         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

src/main/pty/node-pty-master-fd-retirement.test.ts

@@ -0,0 +1,194 @@
+import * as pty from 'node-pty'
+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 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'
+
+function spawnPty(command: string, cols = 80, rows = 24): pty.IPty {
+  return pty.spawn(POSIX_SHELL, ['-c', command], {
+    name: 'xterm-256color',
+    cols,
+    rows,
+    cwd: process.cwd(),
+    env: { ...process.env }
+  })
+}
+
+function masterFd(term: pty.IPty): number {
+  return (term as unknown as { fd: number }).fd
+}
+
+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<void>((resolve) => {
+    term.onExit(() => resolve())
+  })
+  if (destroy) {
+    ;(term as unknown as { destroy?: () => void }).destroy?.()
+  }
+  await new Promise<void>((resolve) => setTimeout(resolve, 400))
+  return { term, spawnFd }
+}
+
+// Windows never reaches this code: WindowsTerminal.resize goes through the conpty
+// agent and reads no fd, so the sentinel is written and never consulted there.
+const describeOnPosix = process.platform === 'win32' ? describe.skip : describe
+
+describeOnPosix('node-pty master fd retirement', () => {
+  it('invalidates the descriptor once it gives up the handle', async () => {
+    const { term, spawnFd } = await retiredPty()
+
+    expect(spawnFd).toBeGreaterThanOrEqual(0)
+    expect(masterFd(term)).toBe(-1)
+  }, 15000)
+
+  it('answers a resize past retirement without issuing the ioctl', async () => {
+    const { term } = await retiredPty()
+
+    // Pre-patch this threw `ioctl(2) failed, EBADF` out of whatever called it.
+    expect(() => term.resize(200, 50)).not.toThrow()
+    // Geometry stays at the last size actually applied rather than claiming one
+    // that no descriptor ever received.
+    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()
+
+    expect(term.process).toBe(POSIX_SHELL)
+  }, 15000)
+})
+
+// Linux frees the master synchronously enough that the very next pty is handed the
+// same descriptor number every time, which makes the reuse hazard directly
+// observable rather than a race to reproduce.
+//
+// Which is also the constraint on writing a case here: the kernel hands out the
+// lowest free number, so a case that returns while its live pty is still open
+// leaks that descriptor into the next case's premise as an off-by-one. Await the
+// exit, never a fixed sleep.
+const describeOnLinux = process.platform === 'linux' ? describe : describe.skip
+
+describeOnLinux('node-pty master fd reuse', () => {
+  it('cannot resize a live pty handed the retired descriptor number', async () => {
+    const { term: retired, spawnFd } = await retiredPty()
+
+    const live = spawnPty('sleep 1; stty size')
+    let output = ''
+    live.onData((data) => {
+      output += data
+    })
+    try {
+      // The premise of this test: the kernel really did reissue the number. If it
+      // stops holding, the assertion below would pass for the wrong reason.
+      expect(masterFd(live)).toBe(spawnFd)
+
+      // Pre-patch this reached TIOCSWINSZ on `live`'s master and silently resized
+      // a terminal it has no relationship to — no error, nothing for a liveness
+      // probe of the retired pid to observe.
+      retired.resize(200, 50)
+
+      await new Promise<void>((resolve) => {
+        live.onExit(() => resolve())
+      })
+      expect(output.trim()).toBe('24 80')
+    } finally {
+      live.kill()
+    }
+  }, 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('sleep 1')
+    let output = ''
+    live.onData((data) => {
+      output += data
+    })
+    try {
+      expect(masterFd(live)).toBe(spawnFd)
+
+      // Pre-patch this fs.write reached `live`'s master, and the line discipline
+      // echoed it straight back: a retired pane's bytes landing in an unrelated
+      // terminal. Nothing has to read them for the leak to be observable.
+      writeStream(retired).write('leak\r')
+
+      // Await the exit rather than sleeping: a fixed wait leaves this descriptor
+      // open into the next case, whose `expect(masterFd(live)).toBe(spawnFd)`
+      // premise then sees the kernel hand out the lower number this pty was still
+      // holding. That is what broke `node 24 1/8` on Linux, where alone among the
+      // platforms these cases actually run.
+      await new Promise<void>((resolve) => {
+        live.onExit(() => resolve())
+      })
+      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()
+
+    // `exec` replaces the shell, so the foreground pgrp's cmdline is distinct
+    // from the file this pty was spawned with.
+    const live = spawnPty('exec sleep 5')
+    try {
+      expect(masterFd(live)).toBe(spawnFd)
+      // Let the shell finish exec'ing, or its own cmdline is still the fallback.
+      await new Promise<void>((resolve) => setTimeout(resolve, 300))
+
+      // Pre-patch this read tcgetpgrp off `live`'s master and reported `sleep`,
+      // attributing an unrelated pane's process to a pty that had already exited.
+      expect(retired.process).toBe(POSIX_SHELL)
+    } finally {
+      live.kill()
+    }
+  }, 15000)
+})

src/relay/pty-handler-resize-stale-pty.test.ts

@@ -36,7 +36,15 @@ import type { MockDispatcher } from './pty-handler-test-harness'
 const PTY_1 = testPtyId(1)
 const STALE_PID = 424_242
 
-/** node-pty's native `pty.resize` error when the master fd is already closed. */
+/**
+ * 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 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')
 }
@@ -55,7 +63,7 @@ describe('PtyHandler.resize against a stale PTY handle', () => {
     }))
     resize = vi.fn()
     // A shell that exited without node-pty producing `onExit`: the record is
-    // still in the pool and undisposed, but the master fd behind it is closed.
+    // still in the pool and undisposed, but the master behind it is gone.
     mockPtySpawn.mockReturnValue({ ...mockPtyInstance, pid: STALE_PID, resize })
     await dispatcher.callRequest('pty.spawn', {})
     expect(handler.activePtyCount).toBe(1)
@@ -80,7 +88,7 @@ describe('PtyHandler.resize against a stale PTY handle', () => {
     expect(handler.activePtyCount).toBe(0)
   })
 
-  it('contains an ioctl failure whose process is still live, and keeps the record', () => {
+  it('contains an ioctl failure without re-classifying liveness, and keeps the record', () => {
     vi.spyOn(ptyShellUtils, 'isProcessAlive').mockReturnValue(true)
     resize.mockImplementation(ebadfResize)
     const stderr = vi.spyOn(process.stderr, 'write').mockReturnValue(true)
@@ -94,13 +102,34 @@ describe('PtyHandler.resize against a stale PTY handle', () => {
       dispatcher.callNotification('pty.resize', { id: PTY_1, cols: 90, rows: 30 })
     ).not.toThrow()
 
-    // Loss of an fd is not evidence the shell exited, so the claim is retained.
+    // Loss of an fd observed the handle, not the host that owns the pid, so it is
+    // `unverifiable` and the claim is retained. Only the probe above retires.
     expect(handler.activePtyCount).toBe(1)
     expect(stderr.mock.calls.map(([line]) => String(line)).join('')).toContain(
       'ioctl(2) failed, EBADF'
     )
   })
 
+  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)
 

src/relay/pty-handler.ts

@@ -2084,18 +2084,23 @@ export class PtyHandler {
     if (!managed || managed.disposed) {
       return
     }
-    // Why probe first (same probe attach() and listProcesses() run): a shell
-    // that exited without node-pty's `onExit` leaves this entry holding a closed
-    // master fd, and `UnixTerminal.resize` has no fd guard — the ioctl throws
-    // `ioctl(2) failed, EBADF` straight out of this notification handler into the
-    // dispatcher's generic parse-error catch, where it is logged and nothing
-    // else. Nothing retired the entry, so it kept being advertised as live and
-    // kept holding `activePtyCount` above zero, which is what stops a relay with
+    // Why probe (same probe attach() and listProcesses() run): a shell that
+    // exited without node-pty's `onExit` leaves an undisposed entry behind, and
+    // while it stays the relay keeps advertising a dead shell and keeps holding
+    // `activePtyCount` above zero, which is what stops a relay with
     // `relayGracePeriodSeconds: 0` from ever reaching its idle-no-ptys exit
-    // (#12423).
+    // (#12423). This is retirement, not ioctl safety: only ESRCH from the host
+    // that owns the pid is evidence of `exited`.
     if (this.reapPtyProvenExited(managed)) {
       return
     }
+    // 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) {