Repository navigation
fix(pty-daemon): release macOS PTY master descriptors - #5795
Conversation
|
Ready to review this PR? Stage has broken it down into 7 individual chapters for you: 
Chapters generated by Stage for commit 2d022b6 on Jul 20, 2026 5:38am UTC. |
📝 WalkthroughWalkthroughPTY adapters now support idempotent master-descriptor disposal. Open, close, exit, snapshot adoption, shutdown, and handoff paths perform explicit ownership cleanup, with tests covering native, adopted, and server-managed file descriptors. ChangesPTY lifecycle cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Pty
participant SessionStore
Client->>Server: open session
Server->>Pty: spawn PTY
Server->>SessionStore: store session
Client->>Server: close session
Server->>Pty: kill and dispose
Pty-->>Server: exit callback
Server->>SessionStore: remove matching session
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pty-daemon/src/Pty/Pty.ts (1)
638-659: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
adoptFromFdleaks the inherited fd when pid/dims validation fails.The
fd-format check is fine to leave unprotected (nothing to close), but thepidvalidity check andvalidateDims(meta.cols, meta.rows)both run before thetryblock that closesfdon failure. If either throws, this inherited descriptor (already owned by this process via stdio) is never closed — exactly the kind of "failed/partial adoption" leak this PR is meant to close.🐛 Proposed fix
export function adoptFromFd({ fd, pid, meta }: AdoptOptions): Pty { if (!Number.isInteger(fd) || fd < 0) { throw new Error(`invalid fd: ${fd}`); } - if (!Number.isInteger(pid) || pid <= 0) { - throw new Error(`invalid pid: ${pid}`); - } - validateDims(meta.cols, meta.rows); try { + if (!Number.isInteger(pid) || pid <= 0) { + throw new Error(`invalid pid: ${pid}`); + } + validateDims(meta.cols, meta.rows); return new AdoptedPty(fd, pid, meta); } catch (err) { try { fs.closeSync(fd); } catch { // Preserve the adoption error. } throw err; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pty-daemon/src/Pty/Pty.ts` around lines 638 - 659, Update adoptFromFd so the pid validation and validateDims(meta.cols, meta.rows) execution are covered by the existing cleanup try/catch, ensuring fd is closed when either validation throws. Keep the fd-format check outside cleanup, and preserve the original validation or construction error after attempting fs.closeSync(fd).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pty-daemon/src/Server/Server.ts`:
- Around line 127-166: Make rollback disposal in adoptSnapshot best-effort: wrap
both the inner pty.dispose() and each outer session.pty.dispose() in independent
try/catch blocks so disposal failures cannot replace the original adoption error
or stop cleanup of subsequent sessions. Preserve store deletion and rethrow the
original error after attempting all rollback operations.
---
Outside diff comments:
In `@packages/pty-daemon/src/Pty/Pty.ts`:
- Around line 638-659: Update adoptFromFd so the pid validation and
validateDims(meta.cols, meta.rows) execution are covered by the existing cleanup
try/catch, ensuring fd is closed when either validation throws. Keep the
fd-format check outside cleanup, and preserve the original validation or
construction error after attempting fs.closeSync(fd).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc3e6c25-f402-432b-a0c3-f177a4e57fcf
📒 Files selected for processing (11)
packages/pty-daemon/README.mdpackages/pty-daemon/package.jsonpackages/pty-daemon/src/Pty/Pty.tspackages/pty-daemon/src/Server/Server.tspackages/pty-daemon/src/SessionStore/SessionStore.test.tspackages/pty-daemon/src/SessionStore/snapshot.test.tspackages/pty-daemon/src/handlers/handlers.test.tspackages/pty-daemon/src/handlers/handlers.tspackages/pty-daemon/test/byte-fidelity.test.tspackages/pty-daemon/test/fd-lifecycle.test.tspackages/pty-daemon/test/server-fd-lifecycle.test.ts
Greptile SummaryThis PR adds an explicit, idempotent
Confidence Score: 3/5The fd-leak fix is sound and the new test coverage is thorough, but Server.ts has a store-corruption window introduced by this PR that needs a one-line identity guard before merging. The packages/pty-daemon/src/Server/Server.ts — the wireSession exit callback needs an identity guard before deleting from the store.
|
| Filename | Overview |
|---|---|
| packages/pty-daemon/src/Server/Server.ts | Adds dispose calls in close() and wireSession exit handler, and rollback logic in adoptSnapshot. The wireSession exit closure deletes by session ID string without an identity guard; combined with handleClose eagerly setting session.exited = true, this opens a window where the old exit callback can delete a recycled session from the store. |
| packages/pty-daemon/src/Pty/Pty.ts | Adds dispose() to NodePtyAdapter and AdoptedPty with an idempotency guard; wires dispose into the natural-exit path and partial-construction cleanup. AdoptedPty.dispose() deliberately omits clearing livenessTimer so the timer can still drive store cleanup via onExit. |
| packages/pty-daemon/src/handlers/handlers.ts | Restructures handleOpen to kill+dispose a partially-constructed PTY on failure; restructures handleClose to always dispose the fd and eagerly mark session.exited = true before returning, even if kill raised an error. |
| packages/pty-daemon/test/server-fd-lifecycle.test.ts | New Bun integration suite verifying ownership across natural-exit churn, explicit close, shutdown, and successful handoff. fdIsOpen swallows non-EBADF errors and returns true, which could mask unexpected fd states. |
| packages/pty-daemon/test/fd-lifecycle.test.ts | New Node integration suite exercising real node-pty master-fd closure across 128-cycle churn and idempotency of both native and adopted dispose; fdIsOpen correctly rethrows unexpected errors. |
| packages/pty-daemon/src/handlers/handlers.test.ts | Unit tests updated to track disposed state on fake PTYs; adds coverage for partial-open cleanup when wireSession throws. |
| packages/pty-daemon/src/SessionStore/SessionStore.test.ts | Trivial addition of dispose: () => {} to the fake Pty stub to satisfy the updated interface. |
| packages/pty-daemon/src/SessionStore/snapshot.test.ts | Same trivial interface stub update as SessionStore.test.ts. |
| packages/pty-daemon/package.json | Adds the two new test files to the test and test:integration scripts. |
| packages/pty-daemon/README.md | Documents the new PTY master ownership rule and lists the two new integration test files in the test directory table. |
Comments Outside Diff (1)
-
packages/pty-daemon/src/Server/Server.ts, line 525-558 (link)Stale-key store delete can wipe a recycled session
wireSession's exit closure capturessession.id(a string) and callsthis.store.delete(session.id)keyed only on the string. This PR'shandleClosenow eagerly setssession.exited = truebefore the shell actually dies, which meanshandleOpencan legally recycle the same ID (lines 53-54 ofhandlers.ts) and insert a newSessionobject under the same key while the old PTY is still alive. When the old PTY finally exits, the closure fires and silently removes the new session from the store.Before this PR,
session.exitedwas set only insidewireSessionitself (afterstore.deletehad already run), so the recycling window did not exist. Fix: guard the delete with an identity check so it only removes the entry it originally installed:if (this.store.get(session.id) === session) { this.store.delete(session.id); }
Prompt To Fix With AI
This is a comment left during a code review. Path: packages/pty-daemon/src/Server/Server.ts Line: 525-558 Comment: **Stale-key store delete can wipe a recycled session** `wireSession`'s exit closure captures `session.id` (a string) and calls `this.store.delete(session.id)` keyed only on the string. This PR's `handleClose` now eagerly sets `session.exited = true` before the shell actually dies, which means `handleOpen` can legally recycle the same ID (lines 53-54 of `handlers.ts`) and insert a new `Session` object under the same key while the old PTY is still alive. When the old PTY finally exits, the closure fires and silently removes the new session from the store. Before this PR, `session.exited` was set only inside `wireSession` itself (after `store.delete` had already run), so the recycling window did not exist. Fix: guard the delete with an identity check so it only removes the entry it originally installed: ```ts if (this.store.get(session.id) === session) { this.store.delete(session.id); } ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/pty-daemon/src/Server/Server.ts:525-558
**Stale-key store delete can wipe a recycled session**
`wireSession`'s exit closure captures `session.id` (a string) and calls `this.store.delete(session.id)` keyed only on the string. This PR's `handleClose` now eagerly sets `session.exited = true` before the shell actually dies, which means `handleOpen` can legally recycle the same ID (lines 53-54 of `handlers.ts`) and insert a new `Session` object under the same key while the old PTY is still alive. When the old PTY finally exits, the closure fires and silently removes the new session from the store.
Before this PR, `session.exited` was set only inside `wireSession` itself (after `store.delete` had already run), so the recycling window did not exist. Fix: guard the delete with an identity check so it only removes the entry it originally installed:
```ts
if (this.store.get(session.id) === session) {
this.store.delete(session.id);
}
```
### Issue 2 of 3
packages/pty-daemon/test/server-fd-lifecycle.test.ts:53-60
`fdIsOpen` swallows unexpected `fstatSync` errors and returns `true`, so a spurious EPERM or other OS error would be misread as "fd is open" and silently pass the assertion. The version in `fd-lifecycle.test.ts` is stricter — it rethrows anything that isn't EBADF. This suite should match.
```suggestion
function fdIsOpen(fd: number): boolean {
try {
fs.fstatSync(fd);
return true;
} catch (err) {
if ((err as NodeJS.ErrnoException).code === "EBADF") return false;
throw err;
}
}
```
### Issue 3 of 3
packages/pty-daemon/src/Pty/Pty.ts:524-535
**`livenessTimer` left running after explicit `dispose()` — intent needs a comment**
When `dispose()` is called outside `onExit` (from `handleClose`, server shutdown, or the `adoptSnapshot` rollback), the 1-second liveness poll keeps firing until `isPidAlive(pid)` returns false. At that point `onExit` fires, clears the timer, and drives the `wireSession` store-cleanup callback. This is intentional — the timer is the only path that delivers the cleanup event to `wireSession` for adopted PTYs — but the omission is not documented. A short note mirroring the "Do not touch TreeKiller" comment above would make the design explicit for future readers.
Reviews (1): Last reviewed commit: "fix(pty-daemon): dispose master fds on t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function fdIsOpen(fd: number): boolean { | ||
| try { | ||
| fs.fstatSync(fd); | ||
| return true; | ||
| } catch (err) { | ||
| return (err as NodeJS.ErrnoException).code !== "EBADF"; | ||
| } | ||
| } |
There was a problem hiding this comment.
fdIsOpen swallows unexpected fstatSync errors and returns true, so a spurious EPERM or other OS error would be misread as "fd is open" and silently pass the assertion. The version in fd-lifecycle.test.ts is stricter — it rethrows anything that isn't EBADF. This suite should match.
| function fdIsOpen(fd: number): boolean { | |
| try { | |
| fs.fstatSync(fd); | |
| return true; | |
| } catch (err) { | |
| return (err as NodeJS.ErrnoException).code !== "EBADF"; | |
| } | |
| } | |
| function fdIsOpen(fd: number): boolean { | |
| try { | |
| fs.fstatSync(fd); | |
| return true; | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === "EBADF") return false; | |
| throw err; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pty-daemon/test/server-fd-lifecycle.test.ts
Line: 53-60
Comment:
`fdIsOpen` swallows unexpected `fstatSync` errors and returns `true`, so a spurious EPERM or other OS error would be misread as "fd is open" and silently pass the assertion. The version in `fd-lifecycle.test.ts` is stricter — it rethrows anything that isn't EBADF. This suite should match.
```suggestion
function fdIsOpen(fd: number): boolean {
try {
fs.fstatSync(fd);
return true;
} catch (err) {
if ((err as NodeJS.ErrnoException).code === "EBADF") return false;
throw err;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| dispose(): void { | ||
| if (this.disposed) return; | ||
| this.disposed = true; | ||
| // Do not touch TreeKiller or its pending escalation. Closing the adopted | ||
| // stream releases only this daemon's inherited descriptor; kill() owns | ||
| // process-tree signaling and may still be finishing in the background. | ||
| try { | ||
| this.reader.destroy(); | ||
| } catch { | ||
| // The stream may already have closed after EOF/EIO. | ||
| } | ||
| } |
There was a problem hiding this comment.
livenessTimer left running after explicit dispose() — intent needs a comment
When dispose() is called outside onExit (from handleClose, server shutdown, or the adoptSnapshot rollback), the 1-second liveness poll keeps firing until isPidAlive(pid) returns false. At that point onExit fires, clears the timer, and drives the wireSession store-cleanup callback. This is intentional — the timer is the only path that delivers the cleanup event to wireSession for adopted PTYs — but the omission is not documented. A short note mirroring the "Do not touch TreeKiller" comment above would make the design explicit for future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pty-daemon/src/Pty/Pty.ts
Line: 524-535
Comment:
**`livenessTimer` left running after explicit `dispose()` — intent needs a comment**
When `dispose()` is called outside `onExit` (from `handleClose`, server shutdown, or the `adoptSnapshot` rollback), the 1-second liveness poll keeps firing until `isPidAlive(pid)` returns false. At that point `onExit` fires, clears the timer, and drives the `wireSession` store-cleanup callback. This is intentional — the timer is the only path that delivers the cleanup event to `wireSession` for adopted PTYs — but the omission is not documented. A short note mirroring the "Do not touch TreeKiller" comment above would make the design explicit for future readers.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pty-daemon/src/Pty/Pty.ts (1)
524-539: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStop the adopted PTY poll once the session is already untracked
dispose()is also used in the adoption-rollback path after the session has been deleted from the store. In that case,reader.destroy()can skip theonExitcleanup, solivenessTimerkeeps pollingisPidAlive()and retains theAdoptedPtyuntil the old shell exits. The interval is unref'd, but it still does work for an untracked session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pty-daemon/src/Pty/Pty.ts` around lines 524 - 539, The dispose() logic in AdoptedPty must stop livenessTimer when the session has already been removed from tracking during adoption rollback. Update dispose() to clear the timer before or alongside reader.destroy(), while preserving onExit cleanup and existing TreeKiller behavior for normally tracked sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/pty-daemon/src/Pty/Pty.ts`:
- Around line 524-539: The dispose() logic in AdoptedPty must stop livenessTimer
when the session has already been removed from tracking during adoption
rollback. Update dispose() to clear the timer before or alongside
reader.destroy(), while preserving onExit cleanup and existing TreeKiller
behavior for normally tracked sessions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e02d197-8d1b-42af-aca7-7f14abf6e932
📒 Files selected for processing (4)
packages/pty-daemon/src/Pty/Pty.tspackages/pty-daemon/src/Server/Server.tspackages/pty-daemon/test/integration.test.tspackages/pty-daemon/test/server-fd-lifecycle.test.ts
Kitenite
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pty-daemon/package.json (1)
32-33: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRun
server-fd-lifecycle.test.tsunder Node, not Bun.This suite exercises real PTYs, but the README explicitly states that node-pty does not work under Bun. Move it from
testtotest:integration, where it will run through Node with the other real-PTY suites.Proposed script correction
- "test": "bun test src/protocol src/SessionStore src/handlers src/Pty/Pty.test.ts src/process-tree.test.ts test/server-fd-lifecycle.test.ts test/no-encoding-hops.test.ts", - "test:integration": "node --experimental-strip-types --test test/integration.test.ts test/control-plane.test.ts test/signal-recovery.test.ts test/byte-fidelity.test.ts test/handoff.test.ts test/foreground-process.test.ts test/flow-control.test.ts test/kill-tree.test.ts test/fd-lifecycle.test.ts", + "test": "bun test src/protocol src/SessionStore src/handlers src/Pty/Pty.test.ts src/process-tree.test.ts test/no-encoding-hops.test.ts", + "test:integration": "node --experimental-strip-types --test test/integration.test.ts test/control-plane.test.ts test/signal-recovery.test.ts test/byte-fidelity.test.ts test/handoff.test.ts test/foreground-process.test.ts test/flow-control.test.ts test/kill-tree.test.ts test/fd-lifecycle.test.ts test/server-fd-lifecycle.test.ts",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pty-daemon/package.json` around lines 32 - 33, Move test/server-fd-lifecycle.test.ts from the Bun-based "test" script to the Node-based "test:integration" script in package.json, preserving the existing test path and ensuring it runs with the real-PTY integration suites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/pty-daemon/package.json`:
- Around line 32-33: Move test/server-fd-lifecycle.test.ts from the Bun-based
"test" script to the Node-based "test:integration" script in package.json,
preserving the existing test path and ensuring it runs with the real-PTY
integration suites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbed0db3-50da-4bb3-8823-73fbbaa4a07f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/desktop/package.jsonpackages/host-service/package.jsonpackages/pty-daemon/README.mdpackages/pty-daemon/package.jsonpackages/pty-daemon/src/Pty/Pty.tspackages/pty-daemon/src/Server/Server.tspackages/pty-daemon/test/fd-lifecycle.test.tspackages/pty-daemon/test/handoff.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/pty-daemon/test/fd-lifecycle.test.ts
Summary
Root cause
The standalone pty-daemon removed exited sessions from its store but did not have an explicit descriptor-ownership teardown contract. The host-service reaper cannot close fds held by the detached daemon process, so master descriptors could accumulate across long-lived, high-churn workloads until macOS exhausted
kern.tty.ptmx_max.Implementation
Pty.dispose()now releases the process-owned master descriptor exactly once. Native PTYs destroy node-pty's read/write streams, while adopted PTYs destroy their inheritedtty.ReadStream. Close paths signal through the existing TreeKiller before disposal, and its escalation chain continues independently so detached/background descendants are still reaped.Failed native construction and later partial-open setup kill and dispose the newly created PTY. Partial successor adoption closes only the successor's inherited copies without signaling shells that remain owned by the predecessor. Explicitly closed sessions are immediately excluded from fd handoff while their eventual exit notification still completes normal store cleanup.
Successful handoff still calls
close({ killSessions: false }), which neither kills nor disposes predecessor adapters.The package README now documents the ownership rule and lifecycle coverage.
Validation
bun run test— 66 passedbun run build:daemonbun run test:integration— 56 passed, including control-plane, handoff, flow-control, TreeKiller, signal recovery, and new real-fd lifecycle suitesbun run typecheck— repo-wide, 35 tasks passedbun run lint:fixbun run lintCloses #5305
Summary by cubic
Fixes PTY master-fd leaks by disposing descriptors on teardown across
pty-daemon. Also prevents hidden macOS/dev/ptmxleaks by pinningnode-ptyto a fixed version.Bug Fixes
dispose()toPty(idempotent) for nativenode-ptyand adopted PTYs.Dependencies
node-ptyto1.2.0-beta.14to fix hidden macOS/dev/ptmxdescriptor leaks; applied inapps/desktop,packages/host-service, andpackages/pty-daemon.Written for commit f22fb4a. Summary will update on new commits.
Summary by CodeRabbit
dispose()API for PTY instances (optionally preserving exit polling).