Skip to content

Build node-pty from patched source on macOS so terminals stop leaking descriptors - #402

Closed
qingshanfeihu wants to merge 1 commit into
mainfrom
fix/node-pty-macos-descriptor-leaks
Closed

qingshanfeihu wants to merge 1 commit into
mainfrom
fix/node-pty-macos-descriptor-leaks

Conversation

@qingshanfeihu

Copy link
Copy Markdown
Collaborator

Why

While investigating the stalled runtime (#401), the stuck worker held a dangling /dev/ptmx and a (revoked) tty with no child process. node-pty 1.1.0's macOS spawn leaks three descriptors every time it starts a terminal:

  • pty_posix_spawn opens /dev/ptmx descriptors to keep 0–2 taken, then cleans up with for (; count > 0; count--) close(low_fds[count]). In a normal process count is 0 there, so low_fds[0] is never closed.
  • The parent never closes the terminal's slave end; posix_spawn_file_actions_addclose closes it only in the child. Early-return paths also leak the master.
  • SetupExitCallback creates a kqueue() per terminal for its exit watcher and never closes it.

Upstream fixed these after 1.1.0 (microsoft/node-pty#882, #931), but only in 1.2.0-beta prereleases so far. Moving to a beta would also change Windows, which src/workspace/pty-exit-cleanup.mjs holds to 1.1.0's lifecycle.

What

  • apps/desktop/scripts/node-pty-macos.mjs, run by the existing desktop postinstall (developer-toolchain.mjs --native-permissions), on macOS only:
    • Checks that node-pty is exactly 1.1.0 and that src/unix/pty.cc has the published hash.
    • Applies the two upstream fixes: close(kq) after the exit watcher; the spawn closes the slave, the master on failure, and every low descriptor. The cleanup loop is also safe when posix_openpt fails, where upstream's i <= count could read low_fds[3].
    • Checks that the result has the reviewed hash.
    • Builds pty.node and spawn-helper for arm64 and x64 with npm's node-gyp, checks each with lipo -archs, and puts them in prebuilds/darwin-* with a receipt. A build that matches its receipt is kept.
    • Removes build/, so nothing shadows the prebuilds.
    • Any other node-pty version or source is refused, with a message saying the script must be dropped or updated.
  • Windows and Linux are untouched: the leaks are in macOS-only code.
  • Docs: docs/development/native-development-loop.md. A macOS desktop install now needs the Xcode Command Line Tools, and node-gyp may download Node's headers once.

Tests

  • apps/desktop/test/node-pty-descriptors.test.ts (macOS): six terminals started through Circle's spawnPty, with lsof counting this process's /dev/ptmx, tty/revoked and kqueue descriptors.
    • Against stock 1.1.0 it failed with { ptmx: 7, tty: 7, kqueue: 12 } vs { 1, 1, 6 }, one of each per terminal.
    • With the fix, the counts are unchanged.
  • apps/desktop/test/node-pty-macos.test.ts (all systems): the fix gives the reviewed source and a second run changes nothing; any other source is refused. With a stubbed node-gyp and lipo, both archs are built into prebuilds with receipts and build/ removed, nothing is rebuilt the second time, a wrong-arch build is refused before replacing anything, and another version is refused while Linux and Windows are left alone.
  • A fresh npm --prefix apps/desktop ci ran the fix by itself: reviewed source hash, pty.node/spawn-helper arm64 and x86_64, no build/, Windows prebuilds intact.
  • Packaged app, checked with a one-off script that is not committed: six terminals opened and closed through the runtime's terminal service, with the runtime worker's descriptors counted by lsof.
    • This branch's package: ptmx 0→0, tty 0→0, kqueue 6→6.
    • Package with stock 1.1.0: ptmx 1→7, tty 1→7, kqueue 7→13.
    • The packaged pty.node is byte-identical to the local fixed build.

Ran on macOS 27.0.1 (arm64), with scratch homes and data folders and the background-only test browser:

  • npm run check: 1563 pass, 0 fail, 10 skipped.
  • Desktop npm test: 163 pass. test:runtime: 826 tests, 809 pass, 0 fail, 17 skipped; this includes all the real terminal tests on the patched spawn.
  • Desktop typecheck and format:check: clean. npm run package and test:launch on it: 4/4.

Not verified: the x64 binaries were built and arch-checked here but not run; the Intel release runner builds and tests its own. Not run on the Intel runner or in CI yet.

🤖 Generated with Claude Code

… descriptors

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@yawiofjangia
yawiofjangia added this pull request to the merge queue Nov 3, 2026
@yawiofjangia
yawiofjangia removed this pull request from the merge queue due to a manual request Nov 3, 2026
@qingshanfeihu

Copy link
Copy Markdown
Collaborator Author

Replaced by #409. It keeps this PR's commit unchanged and adds one commit: the new test's execute-bit check on spawn-helper (mode & 0o111) now runs only off Windows, where there are no execute bits and it failed with 0 !== 73 (runtime-other-platforms (windows-latest), desktop (windows-x64)). The helper is a macOS binary. Closing this one so the work continues in #409.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant