Skip to content

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

Merged
qingshanfeihu merged 2 commits into
mainfrom
fix/node-pty-macos-windows-test
Nov 3, 2026
Merged

qingshanfeihu merged 2 commits into
mainfrom
fix/node-pty-macos-windows-test

Conversation

@qingshanfeihu

Copy link
Copy Markdown
Collaborator

Replaces #402. It keeps #402's commit unchanged, by its author, and adds one commit that fixes the new test on Windows.

Why a replacement

#402's new apps/desktop/test/node-pty-macos.test.ts checked the spawn helper's execute bits (statSync(...).mode & 0o111) on every system. Windows has no execute bits, so the check failed there (0 !== 73) in runtime-other-platforms (windows-latest) and desktop (windows-x64), in run 41451572250.

The fix

  • Check the spawn helper's execute bits only off Windows: the assertion now runs only when process.platform !== 'win32'. spawn-helper is a macOS binary, and the build it checks only runs on macOS. The rest of that test is unchanged: the prebuilds for both architectures, their receipts, build/ being removed and nothing being rebuilt a second time are still checked on every system.

Two other jobs failed on #402's run, and the fix does not cover them:

  • native (ubuntu-24.04, linux-x64) and package-other-platforms (windows-latest): apps/desktop/test/compaction.mjs:274 timed out in page.screenshot after 45 s. That script is not changed here, and the node-pty script does nothing on Linux or Windows.
  • build (windows-11-arm, windows-arm64) in the release workflow: tests/file-rewind.test.ts expected the short RUNNER~1 spelling of TEMP and got the long one, along with a memory test. Neither file is changed here.

Checks run here (macOS 27, arm64, scratch CIRCLE_HOME and CIRCLE_WORKBENCH_DATA_DIR)

  • node --import tsx --test test/node-pty-macos.test.ts test/node-pty-descriptors.test.ts (apps/desktop): 5 pass, 0 fail.
  • Desktop test:runtime: 826 tests, 809 pass, 0 fail, 17 skipped.
  • Desktop typecheck: clean. Prettier on the changed file: clean.

Not run here: the Windows jobs. The fix's Windows effect is verified only by this PR's CI.


#402's description

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

Jiang Yongze and others added 2 commits November 2, 2026 22:16
… descriptors

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jiang Yongze <jiangyongze@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@qingshanfeihu
qingshanfeihu force-pushed the fix/node-pty-macos-windows-test branch from 13102c4 to 0ba8586 Compare November 3, 2026 08:38
@qingshanfeihu
qingshanfeihu added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit 8b698b6 Nov 3, 2026
19 of 23 checks passed
@qingshanfeihu
qingshanfeihu deleted the fix/node-pty-macos-windows-test branch November 6, 2026 09:03
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