Repository navigation
fix: close the PTY master fd when a session is reaped - #23
Open
brianKing88 wants to merge 1 commit into
Open
brianKing88 wants to merge 1 commit into
brianKing88 wants to merge 1 commit into
Conversation
`killSession()` tears a session down with `pty.kill()`, which on POSIX is
`process.kill(pid, SIGHUP)` and nothing else. node-pty closes the master
fd from its own `exit` handler, so that fd is only released if the shell
actually dies from the SIGHUP. A shell that traps the signal, or that
sits in a foreground job outliving the session, leaves the fd open for
the lifetime of the plugin server.
PTYs are a fixed machine-wide resource -- `kern.tty.ptmx_max` on macOS
(511 by default), `/proc/sys/kernel/pty/max` on Linux. Reaped sessions
accumulate until the pool is gone, and then nothing on the host can open
a terminal any more. On the install where this was found, the plugin
server had been up for ~2.5 days and held 508 `/dev/ptmx` handles with
zero live children; Ghostty, Terminal.app and Cursor were all failing to
allocate a PTY.
Teardown now goes through `disposePty()`, which prefers `destroy()` --
it closes the fd first and signals afterwards, so it is safe on a live
and on an already-exited shell -- and falls back to `kill()` for a
node-pty that does not expose it. `destroy()` is declared optional on
`PtyProcess` since node-pty's public typings omit it, though both
UnixTerminal and WindowsTerminal implement it.
The `onExit` path calls it too. node-pty's close chain has only a
timeout fallback there, and `destroy()` is a no-op once the socket is
down, so the insurance is free.
Measured over 20 sessions whose shell traps SIGHUP, fds stranded per
session:
kill() destroy()
1.1.0 3 2
1.2.0-beta.18 1 0
Reaching zero also needs the node-pty bump in cloudcli-ai#15; on the pinned 1.1.0
this change reduces the leak without eliminating it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After ~2.5 days of normal use, nothing on the host could open a terminal any more — not the plugin, not the user's own emulator:
The plugin server was holding 508
/dev/ptmxhandles with zero live children:Zero children, 508 master fds: the shells were long gone, the fds were not.
Cause
killSession()tears a session down withpty.kill(), which on POSIX isprocess.kill(pid, SIGHUP)and nothing else:node-pty closes the master fd from its own
exithandler, so the fd comes back only if the shell actually dies from that SIGHUP. A shell that traps the signal, or that sits in a foreground job outliving the session, keeps the fd for the lifetime of the plugin server. Every detached-and-reaped session strands one.PTYs are a fixed machine-wide resource (
kern.tty.ptmx_maxon macOS, 511 by default;/proc/sys/kernel/pty/maxon Linux), so this is not a slow memory creep — it eventually takes the whole machine's terminal capability down with it.destroy()is the API that actually releases the fd: it closes the socket first and signals afterwards, which is safe on both a live and an already-exited shell.Fix
Teardown goes through a new
disposePty()that prefersdestroy()and falls back tokill()on a node-pty that does not expose it.destroy()is declared optional onPtyProcessbecause node-pty's public typings omit it, although bothUnixTerminalandWindowsTerminalimplement it.The
onExitpath calls it too — node-pty's close chain has only a timeout fallback there, anddestroy()is a no-op once the socket is down, so the insurance is free.Measurements
20 sessions, each a shell that traps SIGHUP (
trap '' HUP; sleep 300), torn down the waykillSession()does it. fds stranded per session:kill()destroy()1.1.0(currently pinned)1.2.0-beta.18Repro, if you want to confirm it independently:
Relationship to the open PRs
1.2.0-betaline) — the table above is independent evidence for that bump:1.1.0leaks regardless of how teardown is done, so this change alone takes it from 3 to 2 fds per session, and only reaches 0 on top of fix: bump node-pty to ^1.2.0-beta.12 so the plugin works on Linux #15. Happy to rebase or drop anything here if fix: bump node-pty to ^1.2.0-beta.12 so the plugin works on Linux #15 lands first.spawn-helperexecute bit) — unrelated to the leak and already covered there; deliberately left out of this PR. For what it is worth, I hit that one too on a fresh install (prebuilds/darwin-arm64/spawn-helperunpacked as0644, every spawn failingposix_spawnp failed, 9 of 24 tests red), so it is still reproducible onmain.Verification
npm run typecheck— cleannpm test— 21/24, the same 3 failures asmainwith this patch stashed, so no regression. They are pre-existing and environmental: one asserts a cwd path that macOS resolves through the/var→/private/varsymlink, two time out matching shell output.🤖 Generated with Claude Code