mirror of
https://github.com/stablyai/orca.git
synced 2026-10-12 08:02:37 +00:00
fix(pty): patch node-pty native off-by-one leaking /dev/ptmx per spawn
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 <help@stably.ai>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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."
|
||||
|
||||
Generated
+3
-3
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user