The divergence pinned by #18601 is real and worth keeping, but its stated reason
was wrong. It claimed the desktop patch carries the early conin placement "and
therefore the +2 File / +1 Process regression, measured against its exact
installed tree" -- i.e. that the shipped desktop app leaks because of its own
leak fix.
It does not, for any terminal a user opens. node-pty defaults `_useConptyDll` to
false. Every desktop site that opens a pane sets it true (`local-pty-utils.ts`
twice, `native-pty-spawn.ts`), as does the `windows-conpty-warmup.ts` warm-up, so
they take the `else` branch, where upstream already destroys the input socket. The
relay passes no such option (`src/relay/pty-handler.ts`) and takes the
`!useConptyDll` branch -- the one both this asset and the desktop patch edit.
The desktop is not entirely off that branch, though: the hidden rate-limit probes
in `src/main/rate-limits/claude-pty.ts` and `codex-pty-rate-limit-probe.ts` omit
the option, recur, and tear down through `kill()`, so the hunk is live there --
just never for a visible pane. Whether the early placement costs the same +2 File
/ +1 Process across a probe's lifecycle is unmeasured; the numbers in this comment
were taken on relay-style spawn/kill cycles, and the comment now says so. What is
settled is the replaced claim: not every Windows user, and not every terminal.
Those two probes were missed three enumerations running because they use
`await import('node-pty')`, which no static-import grep finds. The comment now
tells the next reader to grep for `node-pty` instead.
The measurement that produced the wrong claim was taken by a standalone harness
that passed no `useConptyDll` and so defaulted into the branch it was not trying
to measure -- the same standalone-is-not-the-real-host trap #18601's own body
warns about, one level down.
Also refreshes the self-exit paragraph, which #18635 made stale. That leak is now
fixed for the desktop, and the note records why the fix cannot reach a Windows
relay. The fix is mostly native (`src/win/conpty.cc`) and this asset only rewrites
`lib/*.js`, and all three delivery paths stop short of Windows: pnpm patches do
not cross the SSH boundary; `MATRIX_SLOTS` in `build-orcad-prebuilds.mjs` has no
win32 entry; and the one relay asset that does patch native source and rebuild on
the host (`node-pty-1.1.0-master-cloexec-patch.cjs`) returns
`skipped:unsupported-platform` for anything but linux/darwin. #18635's flat self-exit relay numbers were measured
against a locally rebuilt binary, so they describe the relay code path on a
patched tree, not the tree a relay host installs -- the note says so explicitly
rather than leaving the next reader to conflate them.
Assertion and hashes unchanged: the relay must still release conin after the
console-list fork, and a patch sync must still not copy the early placement onto
the relay's branch, where it does cost +2 File and +1 Process per terminal. Only
the justification changes, plus the test name, which said "like the desktop
patch" where it meant "unlike the desktop patch placement".
A Windows SSH relay leaked one Windows File handle per terminal, for the life of
the relay process, across reconnects. node-pty's `kill()` flips `readable` on the
conin and conout sockets and destroys neither; `_cleanUpProcess` destroys
`_outSocket`, so only conin is stranded, and it wraps a real named-pipe handle
from `fs.openSync(term.conin, 'w')`.
The obvious fix -- and the one config/patches/node-pty@1.1.0.patch ships for the
desktop -- releases it at the top of the branch, before `_getConsoleProcessList()`
forks and before the native kill. Measured against a real Windows SSH host, that
is three times worse than leaving the leak alone: teardown aborts partway, the
forked console-list agent is never reaped, and both pipe handles stay alive.
Releasing it at the end of the branch instead is flat.
20 spawn/kill cycles, handles bucketed by NT object type, identical numbers
standalone and through a real relay:
published node-pty File +1/terminal, Process flat
desktop patch placement File +2/terminal, Process +1/terminal
released last (this) File flat, Process flat
`windowsTerminal.js` takes the desktop's error-listener hunks verbatim. The conin
listener is not what fixes the leak -- adding it alone changed nothing -- but it
is what keeps a pipe error retiring one terminal instead of the host.
The desktop patch has the early placement and therefore the regression, measured
against its exact installed tree. Correcting it there needs its own verification
on a Windows desktop build, so the trees diverge on this one hunk deliberately and
a test pins that so a future patch sync cannot copy the bug back.
The relay asset from #17920 only rewrote the forkpty `default:` call site, which
sits in the `#else` arm of PtyFork's `#if defined(__APPLE__)`. macOS takes
`pty_posix_spawn`, so the asset had never patched anything a Mac executes -- and
`applyNodePtyMasterCloexecPatch` returned 'fixed' for any non-Linux host without
running the script at all, which is what publishes a tree to the shared
native-deps cache.
Stock `pty_posix_spawn` opens up to three throwaway ptys to push the real master
off fds 0-2 and never closes them: the cleanup loop is `for (; count > 0;
count--)`, but the first `posix_openpt()` in a running process already returns
>= 2, so it breaks with `count == 0` and the body never runs -- and where it does
run it closes `low_fds[count]`, never `low_fds[0]`. One orphaned /dev/ptmx fd per
terminal, for the life of the relay.
Ports the `low_fds` fix and the Apple-branch `pty_cloexec(master)` call from the
app's `config/patches/node-pty@1.1.0.patch`, byte-identical, and runs the gate on
darwin. macOS needs a different build layout than Linux: it has no `build/` at
all, so the fallback moved aside is `prebuilds/darwin-<arch>` -- which is also
what makes node-pty's install script fall through from "prebuild found" to
node-gyp -- and the compile writes a `build/Release` the loader checks first.
Verification is per-platform too: Linux's leak is inheritance (/proc), macOS's is
self-held (lsof).
Also corrects the asset's claim that "macOS re-opens the tty through uv_tty_init's
cloexec dup". Measured false: FD_CLOEXEC is not set on the master. What protects
it is POSIX_SPAWN_CLOEXEC_DEFAULT, one option away from gone since uid/gid drops
libuv back to fork()/exec() -- so the master is now marked there too.
Measured on darwin-arm64, one PTY per open/close cycle in a relay-shaped dir
running the relay's own commands:
before cycle:ptmx 1:1 2:2 3:3 ... 10:10 (10 after a settle)
after cycle:ptmx 1:0 2:0 3:0 ... 10:0 (0 after a settle)
Linux re-verified in docker node:22: inherited before, isolated after,
`already-patched` on the second run.
Refs #17915
Refs #8362
* fix(ssh): close the pty master fd leak on Linux relay hosts
The app gets the FD_CLOEXEC patch through pnpm patchedDependencies (#17914);
the relay installs stock node-pty from npm, where no pnpm patch reaches. Linux
is where that matters -- it is the only relay platform that takes forkpty()'s
no-atomic-O_CLOEXEC path, and it is also the only one that already compiles
node-pty at install time, so the fix costs a second compile rather than a first.
Ships the patch as a relay asset applied like the existing Windows console-list
one, and rebuilds only after the probe has proven node-pty loadable. The rebuild
is non-fatal by construction: the working build is moved aside first and moved
back on any failure, a failed attempt drops a skip marker so the compile is
attempted at most once per relay directory, and the caller swallows the whole
step. macOS and Windows relays never run it.
Measured on node:22 with a relay-style npm install: before, the master is
cloexec=false and shows up as `26 -> /dev/pts/ptmx` in both a later pty child
and a later child_process child; after, cloexec=true and neither child sees it.
Closes#17915.
* test(ssh): feed the cloexec patch exec to the hand-rolled namespace fixtures
These sequences are positional, so the new Linux-only patch exec swallowed the
READY slot and every install/repair case timed out waiting for the relay.
* fix(ssh): patch the pty master before publishing the shared native-deps tree
* fix(ssh): refuse to publish a native-deps tree whose cloexec patch did not take
Neil
OrcaWinandOrcaWin