From 4548d20cc46da6a5faef82f4699c0db79927a2ee Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Fri, 1 May 2026 18:53:34 -0400 Subject: [PATCH] fix(pty): patch node-pty native off-by-one leaking /dev/ptmx per spawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node-pty 1.1.0's pty_posix_spawn on macOS walks low_fds[0..2] in an allocation loop that breaks at the first fd >= STDERR_FILENO, then cleans up via `for (; count > 0; count--) close(low_fds[count])`. In the typical case (break at count=0) the cleanup body never runs and low_fds[0] — a /dev/ptmx handle — leaks per spawn. Fixed upstream in microsoft/node-pty af053f2 (PR #882), not in any 1.1.0 release. Backport the 3-line cleanup-loop fix as a pnpm patch. E2E validated against a dev daemon: 200 spawn/kill cycles kept the daemon's ptmx fd count flat at baseline; prior runs reproduced linear 1-per-spawn growth. Also documents the native root cause as a status addendum in docs/fix-pty-fd-leak.md — the JS-side destroy() discipline previously landed is still load-bearing for the SIGHUP-to-recycled-pid hazard and for synchronous fd release on daemon shutdown. Co-authored-by: Orca --- config/patches/node-pty@1.1.0.patch | 15 +++++++++++ docs/fix-pty-fd-leak.md | 42 +++++++++++++++++++++++++++++ pnpm-lock.yaml | 6 ++--- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index d3b5ebbd521..4ff713f6c2b 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -73,3 +73,18 @@ index 1ec12f796a822c78fba9ad7f6448c3987e325c23..cec8b67aef02f8199e5606a0d257088b var DEFAULT_FILE = 'sh'; var DEFAULT_NAME = 'xterm'; var DESTROY_SOCKET_TIMEOUT_MS = 200; +diff --git a/src/unix/pty.cc b/src/unix/pty.cc +index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..c17decbb7bc06f10d7b5d26317bd68fdc8c4e339 100644 +--- a/src/unix/pty.cc ++++ b/src/unix/pty.cc +@@ -778,8 +778,8 @@ done: + posix_spawn_file_actions_destroy(&acts); + posix_spawnattr_destroy(&attrs); + +- for (; count > 0; count--) { +- close(low_fds[count]); ++ for (size_t i = 0; i <= count && i < 3; i++) { ++ close(low_fds[i]); + } + } + #endif diff --git a/docs/fix-pty-fd-leak.md b/docs/fix-pty-fd-leak.md index acf2fea5447..1205407873f 100644 --- a/docs/fix-pty-fd-leak.md +++ b/docs/fix-pty-fd-leak.md @@ -1,5 +1,47 @@ # Fix per-session PTY fd leak in the current daemon +## Status: resolved (2026-05-01) + +**Native-side root cause confirmed and patched.** E2E validation under both Node and Electron ABIs: 50 spawn/kill cycles, ptmx count stays at baseline (0). Before the patch, the same harness reproduced a 1-fd-per-spawn linear leak (5 → 55 ptmx fds over 50 cycles). + +## Root cause (actual) + +The leak is a native-side off-by-one in `node-pty@1.1.0`'s `pty_posix_spawn` cleanup loop on macOS. In `src/unix/pty.cc`: + +- **Allocation** (lines 697-701) walks `low_fds[0..2]`, allocating via `posix_openpt(O_RDWR)`. The loop `break`s at the first `low_fds[count] >= STDERR_FILENO`, so `count` holds the index of the last allocated slot (0, 1, or 2). These are decoy `/dev/ptmx` handles used as a workaround for a macOS pty race condition that ensures the real master fd lands above `STDERR_FILENO`. +- **Cleanup** (lines 781-783, buggy): + ```c + for (; count > 0; count--) { + close(low_fds[count]); + } + ``` + When the typical case triggers (`break` at `count=0`, i.e. the very first `posix_openpt` returned an fd ≥ 2), the loop body never executes — `low_fds[0]` is never closed. Every spawn leaks one ptmx handle. + +Upstream fixed this in commit [`af053f2`](https://github.com/microsoft/node-pty/commit/af053f2) (PR #882, 2026-01-28), but version 1.1.0 (pinned here) predates that fix. + +**Applied fix:** minimal 3-line backport in `config/patches/node-pty@1.1.0.patch`: + +```c +- for (; count > 0; count--) { +- close(low_fds[count]); ++ for (size_t i = 0; i <= count && i < 3; i++) { ++ close(low_fds[i]); + } +``` + +The `i < 3` bound is an over-cautious defense against the pathological case where all three `posix_openpt` calls returned fds < `STDERR_FILENO` (loop exhausts to `count=3`, which would otherwise read `low_fds[3]` — UB in upstream's fix too, though unreachable in practice). + +## Why the JS-side `destroy()` discipline (below) is still correct + +The native patch alone closes the leak. But the JS-side changes landed in `pty-subprocess.ts`, `session.ts`, `terminal-host.ts`, `local-pty-provider.ts`, and `relay/pty-handler.ts` remain load-bearing for separate reasons: + +1. **SIGHUP-to-recycled-pid hazard.** `UnixTerminal.destroy()` registers `_socket.once('close', () => this.kill('SIGHUP'))`. After the child is reaped, its pid can be recycled to an unrelated user process (Chrome tab, editor, etc.) before the socket close event fires. Neutralizing `proc.kill` on POSIX before `destroy()` prevents delivering SIGHUP to a stranger. +2. **`forceKill`/`signal` guard against recycled pid.** `process.kill(proc.pid, …)` after `onExit` targets a potentially-recycled pid. The internal `dead` guard converts these into no-ops. +3. **Daemon shutdown determinism.** `TerminalHost.dispose()` → `Session.forceKillAndDisposeSubprocess()` reaps stubborn children via SIGKILL and releases the master fd synchronously, bypassing the 5s `KILL_TIMEOUT_MS` fallback. +4. **Belt-and-suspenders fd release.** Calling `destroy()` on every teardown path remains the contract that will keep holding if the pinned node-pty is ever replaced or upgraded to a build without the bug. + +## Original problem statement (for historical context) + ## Problem The current `daemon-v4` is leaking PTY master file descriptors (`ptmx`) at a rate roughly 20× the live session count. In a real reported case, the live v4 daemon was holding **269 `ptmx` fds for only 13 active terminals** — every closed terminal should have returned its `ptmx` to the OS, but the daemon keeps accumulating them until it hits macOS's `kern.tty.ptmx_max=511` cap and every new terminal (Orca or otherwise) fails with "cannot allocate any more pty devices." diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c2335eb9b1f..071808dcd11 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -9,7 +9,7 @@ patchedDependencies: hash: df21db050a4d85552cafbe583ece863d7fa19ed634a1219210a0455b986b6634 path: config/patches/@xterm__addon-ligatures@0.11.0-beta.198.patch node-pty@1.1.0: - hash: 02e16954edbb8e511963557e3b2c80f8a6fb230cd0c6a75ade936022f3347322 + hash: d39825bae630ddf0fc73e83e6001057c70957161772ba078988767d8569cee63 path: config/patches/node-pty@1.1.0.patch importers: @@ -159,7 +159,7 @@ importers: version: 0.55.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=02e16954edbb8e511963557e3b2c80f8a6fb230cd0c6a75ade936022f3347322) + version: 1.1.0(patch_hash=d39825bae630ddf0fc73e83e6001057c70957161772ba078988767d8569cee63) pdfjs-dist: specifier: ^5.6.205 version: 5.6.205 @@ -11426,7 +11426,7 @@ snapshots: transitivePeerDependencies: - supports-color - node-pty@1.1.0(patch_hash=02e16954edbb8e511963557e3b2c80f8a6fb230cd0c6a75ade936022f3347322): + node-pty@1.1.0(patch_hash=d39825bae630ddf0fc73e83e6001057c70957161772ba078988767d8569cee63): dependencies: node-addon-api: 7.1.1