fix(host-daemon): bump node-pty to 1.2.0-beta.15 to stop macOS pty fd leaks - #2675
Conversation
node-pty 1.1.0's macOS posix_spawn path leaks one /dev/ptmx master fd per spawn (cleanup loop never closes low_fds[0]) plus the parent slave fd and a kqueue fd. The host daemon exhausted the 511-device pty pool. Upstream fixed all three in get-bb#882 and get-bb#931, shipped in 1.2.0-beta.15.
6cf041c to
3352353
Compare
|
@slopcop review this |
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I started a security, code quality, architecture, and performance review. I will also run relevant tests. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| }); | ||
| }); | ||
|
|
||
| describe.runIf(process.platform === "darwin")("macOS fd cleanup", () => { |
There was a problem hiding this comment.
🚨 slopcop/review — Run this macOS assertion in pull request CI.
The package test jobs use Ubuntu, so this describe.runIf always skips the leak assertion.
The macOS job runs only the bb-app tarball smoke task. It does not run @bb/host-daemon#test.
CI can therefore pass without testing the defect that this pull request fixes.
Add a Turbo-backed host-daemon test step to the macOS job. A targeted run for this file is enough.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Simple summary
This PR updates the terminal library to stop macOS from keeping file descriptors after a terminal closes.
It also adds a test that opens and closes terminals, then checks that master descriptors return to the initial count.
Finding
Medium — The macOS leak assertion never runs in pull request CI.
The package test jobs run on Ubuntu. The macOS condition at line 123 therefore skips the leak assertion there.
The macOS job runs only the bb-app tarball smoke task. It does not run the host-daemon tests.
CI can pass without checking the defect this PR fixes.
Add a Turbo host-daemon test step to the macOS job. A targeted run for this file is sufficient.
Other review results
- Security: I found no security or supply-chain defect. The npm integrity matches the lockfile and the verified upstream tag.
- Code quality: I found no duplicate leak test or useful shared helper. The two package pins have separate development and distribution roles.
- Architecture: The dependency change does not alter the host-daemon wire contract. It does not need a protocol version change.
- Performance: The update removes persistent descriptor growth. I found no new production hot-path cost.
- Optional coverage: The test does not check the separate slave and KQUEUE leaks. The master descriptor check still covers the reported exhaustion cause.
Verification
All current GitHub checks pass. Ubuntu ran this file with one skipped test, and the macOS job did not run it.
My isolated macOS run passed both lifecycle tests. The tests took 2.63 seconds, and the full targeted Turbo task took 39.9 seconds.
I did not use Doobie because a browser cannot count host-daemon file descriptors. The direct process test gives the relevant result.
I left a comment-only review. I did not approve the PR or request changes.
2 participants
Human comments
What was wrong
node-pty 1.1.0 leaks three file descriptors per spawned terminal on macOS:
pty_posix_spawn(for (; count > 0; count--)) never closeslow_fds[0]. One/dev/ptmxmaster leaks per spawn.SetupExitCallbackis never closed.The host daemon spawns a pty per terminal session, so a long-lived daemon drains the system-wide pool of 511 pty devices. On the machine that hit this, the daemon held 505 leaked masters after 22 hours. No app on the host, Ghostty included, could open a new terminal.
Upstream fixed all three leaks in microsoft/node-pty#882 and microsoft/node-pty#931.
What changed
node-pty1.1.0 → 1.2.0-beta.15 inapps/host-daemonandpackages/bb-app, plus the lockfile.apps/host-daemon/src/terminals/node-pty-fd-leak.test.tsspawns five ptys and asserts the/dev/ptmxfd count returns to baseline. CI's macOS 15 runners execute it.Why a beta: npm
latestis still 1.1.0, and the fixes exist only on the 1.2.0 line. VS Code ships this exact version in production (^1.2.0-beta.15inpackage.jsonandremote/package.json).The only typings change since 1.1.0 is an optional
pixelSizeargument onresize. Nothing on the wire changes, so noHOST_DAEMON_PROTOCOL_VERSIONbump.How you verified
expected 5 to be less than or equal to 0) and passes on 1.2.0-beta.15. The failing test is committed before the fix.pnpm exec turbo run test --filter=@bb/host-daemon: 561 passed. One pre-existing flake inhost-branches-dispatch.test.ts, passes standalone, unrelated.pnpm exec turbo run typecheck --filter=@bb/host-daemon --filter=bb-app: passes.