Fix Claude process leaks when closing tabs - #9782
austinywang wants to merge 51 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTerminal surface teardown now uses per-runtime native-access gates, paired process termination and free operations, serialized screen-tail reads, and bounded close scheduling. Tests cover atomic operations, lifecycle coordination, concurrency, and Ghostty stubs. Claude SessionEnd hooks now use a 10-second timeout. ChangesRuntime teardown
Claude SessionEnd timeout
Ghostty fork records
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalSurface
participant ScreenTailReader
participant NativeAccessGate
participant TeardownCoordinator
participant GhosttyRuntime
TerminalSurface->>ScreenTailReader: submit screen-tail request
ScreenTailReader->>NativeAccessGate: acquire native borrow
TeardownCoordinator->>NativeAccessGate: begin surface teardown
NativeAccessGate-->>TeardownCoordinator: defer while borrow is active
ScreenTailReader->>GhosttyRuntime: read screen tail
ScreenTailReader->>NativeAccessGate: release native borrow
NativeAccessGate->>TeardownCoordinator: admit teardown
TeardownCoordinator->>GhosttyRuntime: request process termination
TeardownCoordinator->>GhosttyRuntime: free surface
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ghostty`:
- Line 1: Update the ghostty submodule pointer from unreachable commit
5b20c62297aca8289b400cb7f5583f65a5c759bc to a reachable commit in
manaflow-ai/ghostty, and update the associated fork and changelog records to
reference the same commit; alternatively, ensure the missing commit is available
from the configured submodule remote before merging.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e5163d4-f1bf-4175-8d1c-c8f7e1ee0d90
📒 Files selected for processing (8)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownCoordinator.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceRuntimeTeardownCoordinatorTests.swiftPackages/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/GhosttyRuntimeTestStubs.cPackages/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/include/GhosttyRuntimeTestStubs.hdocs/ghostty-fork.mdghosttyscripts/check-test-determinism.pyscripts/ghosttykit-checksums.txt
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c3d4e12. Configure here.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/GhosttyRuntimeTestStubs.c`:
- Around line 474-513: Add a configurable success mode to
ghostty_surface_read_screen_tail_vt that populates text with valid sample
content and returns true, while preserving the existing false path. Update the
relevant runtime tests to enable this mode, assert
TerminalSurfaceRuntimeScreenTailRequest.read() returns the expected string, and
verify ghostty_surface_free_text is called exactly once for each successful
read.
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 617-622: Update the SessionEnd assertion using the hooks data in
this test: flatten all SessionEnd groups, select the hook whose command is
"hooks claude session-end", and assert that this command’s timeout is exactly
10. Do not allow an unrelated hook or only the first group to satisfy the check.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4eb9614c-7458-4685-af27-59e856caa4bb
📒 Files selected for processing (28)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/Concurrency/AtomicRawPointerValue.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/Concurrency/AtomicUInt64Value.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundationAtomicsC/CmuxFoundationAtomicsC.cPackages/macOS/CmuxFoundation/Sources/CmuxFoundationAtomicsC/include/CmuxFoundationAtomicsC.hPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/Concurrency/AtomicRawPointerValueTests.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/Concurrency/AtomicUInt64ValueTests.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeNativeAccessBorrow.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeNativeAccessGate.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeNativeTeardown.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeScreenTailReader.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeScreenTailRequest.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownAction.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownCoordinator.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownRequest.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownRequestQueue.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeLifecycle.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+ScreenSnapshot.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceRuntimeNativeAccessTests.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceRuntimeTeardownCoordinatorTests.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceTeardownCallbackLifetimeTests.swiftPackages/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/GhosttyRuntimeTestStubs.cPackages/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/include/GhosttyRuntimeTestStubs.hResources/bin/cmux-claude-wrappercmuxTests/TabManagerUnitTests.swiftdocs/ghostty-fork.mdscripts/ghosttykit-checksums.txttests/test_claude_wrapper_hooks.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/GhosttyRuntimeTestStubs.c`:
- Around line 459-467: Update ghostty_surface_free_text to increment
cmux_test_surface_free_text_call_count immediately at function entry, before
checking surface or text validity, while preserving the existing cleanup
behavior for non-null text->text values.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fea90a57-c190-46ef-944d-e3079e8e973c
📒 Files selected for processing (4)
Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceRuntimeNativeAccessTests.swiftPackages/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/GhosttyRuntimeTestStubs.cPackages/macOS/CmuxTerminal/Tests/GhosttyRuntimeTestStubs/include/GhosttyRuntimeTestStubs.htests/test_claude_wrapper_hooks.py
|
@coderabbitai review |
✅ Action performedReview finished.
|

Summary
Closes #9573.
Root cause
The one-second SessionEnd hook timeout was real, but increasing it alone could not make pane process lifetime reliable.
A UI close removes the Swift-owned native surface and schedules Ghostty's final free away from the main actor. Process termination previously began only when a bounded worker reached ghostty_surface_free. If both native-free workers were occupied by stuck joins, a later closed pane could remain queued without its process group ever being told to stop.
Starting termination unconditionally before that queue exposed a second ownership race: bounded screen-tail reads capture the native pointer before their first actor suspension. A close could otherwise retire the native surface while one of those reads still held a valid Swift request but had not yet called Ghostty.
The structural defect was therefore missing ownership and ordering between three events: native-read admission, process termination, and final surface destruction.
Architectural invariant
Each installed TerminalSurface runtime pointer now gets a fresh TerminalSurfaceRuntimeNativeAccessGate. The gate is the sole owner of admission and the terminal teardown transition for that runtime generation.
This preserves the normal fast path—termination still begins synchronously before suspension when no read is active—without making a borrowed pointer invalid.
Why atomics, not an actor
Both sides need a decision before suspension: boundedScreenTailVT must acquire its borrow before awaiting the teardown actor, while close/deinit must close admission from a synchronous nonisolated path. An actor would require Task/await admission and reintroduce the ordering gap this change removes.
The gate uses the existing macOS 14-compatible C11 atomic layer: one UInt64 packs the permanent close bit and borrow count, and one atomic raw pointer publishes the retained one-shot action. There are no production locks or global registries.
AtomicRawPointerValue and AtomicUInt64Value use narrowly scoped @unchecked Sendable wrappers because Swift cannot infer the safety of C11 atomic storage. Each wrapper allocates one stable storage address, every pointee access goes through the C11 atomic API, and deallocation occurs only when the wrapper itself is no longer referenced. No broader runtime or domain object is declared @unchecked Sendable.
Ghostty process ownership
The final parent-repo pin is
f76c132e526f124fe4aaebd39f516751656844bc, the current fork-mainmerge from manaflow-ai/ghostty#191. It contains this PR's complete process-teardown lineage through90ba327fc6e0ea614e59a25f3ee5133b91d459da(#184, #187, #188, #192, and #193). The later pin only integrates already-merged fork work from current cmuxmain; it does not replace or bypass the process ownership fix.Ghostty now:
That keeps Claude's ten-second SessionEnd callback inside a larger native grace budget and still prevents a pathological child or foreground job from holding a teardown worker indefinitely.
Shared close-path audit
The UI close button, Cmd-W/configured shortcuts, context menus, and command palette all converge through TabManager/Workspace panel removal. CLI/socket close commands converge on the same TabManager/Workspace operations. TerminalPanel teardown and TerminalSurface deinit both end at the injected TerminalSurfaceRuntimeTeardownCoordinator.
No entrypoint gained its own PID lookup, TTY heuristic, signal path, or optimistic process state. The shared native ownership boundary handles every surface close.
Scope reconciliation after latest main merge
The paths previously flagged as out of scope—
Sources/NotificationsPage.swift,Sources/Panels/FilePreviewPDFSharingPresenter.swift, andscripts/ci/run-app-host-xcodebuild.sh—are now part oforigin/mainand are absent from the current PR diff. The remainingorigin/main...HEADdiff is limited to the terminal lifecycle/atomic ownership fix, its behavioral tests and stubs, the Claude hook budget, and the matching Ghostty pin/docs/checksum records.Regression proof
The branch preserves test-first evidence.
b8a643561reproduced both late repeated-SIGHUP behavior and descendants surviving after the group leader was reaped: 75/77 passed. Fix SHA81b4de4f5made the subprocess stop filter pass 77/77; the laterbabe4266c/47e9bd4c9and53239618f/bc7e9f746pairs cover foreground job-control groups and stale process-group reuse, integrated through90ba327fcand retained by the finalf76c132e5pin.GhosttyKit artifact
f76c132e526f124fe4aaebd39f516751656844bcaf9f8f12e6f41ffe00b5b65f150bb887b19dc752e47d20d3c351696c803509afThe downloaded archive passed
scripts/validate-xcframework-archive.py, matched the checksum pinned inscripts/ghosttykit-checksums.txt, and matched GitHub's release-asset digest. The submodule is onmain, the exact pin is reachable fromorigin/main, and90ba327fc…is an ancestor of it.Verification completed before final CI
TabManagerCloseCurrentTabSpamTests/testCloseWorkspaceEnqueuesTerminalRuntimeTeardownOffMainThread: 1/1 passed with a real Ghostty runtime surfacepython3 tests/test_claude_wrapper_hooks.py: passed./scripts/lint-pbxproj-test-wiring.sh: passed (662 files checked)git diff --check: passed./scripts/reload.sh --tag issue-9573-claude-leak: succeeded without launchingcmux-unitscheme build against the same tagged DerivedData: succeededorigin/main: cleanThe mechanical cmux policy lane flags
cmuxTests/TabManagerUnitTests.swiftbecause the touched legacy file imports XCTest. This is the documented existing-suite exception: the regression already exists onmainin Aziz's XCTest behavior suite (commit2275df619a), and this PR only replaces its unsafe fake native pointer with a real runtime. Moving one assertion would split the suite while the necessary edit to the XCTest file would still trigger the same path-level grep.Warning and localization audits
Note
High Risk
Changes concurrency ordering and child-process teardown on every terminal close path; regressions could cause use-after-free, hung teardown workers, or lingering processes.
Overview
Fixes Claude and other child processes leaking when closing tabs by ordering native API use, process termination, and final
ghostty_surface_freeper runtime generation instead of tying shutdown to bounded free-worker slots.Each installed Ghostty surface gets a
TerminalSurfaceRuntimeNativeAccessGate(C11 atomics via newAtomicRawPointerValueand extendedAtomicUInt64Value). Screen-tail reads acquire a borrow before touching the pointer; close/deinit closes admission and runs a one-shot teardown action when borrows drain.requestTeardownon the coordinator now callsghostty_surface_request_process_terminationbefore the async enqueue to the free queue, so stuck native joins cannot defer telling the process group to stop. Teardown uses pairedTerminalSurfaceRuntimeNativeTeardown(begin + free) instead of a lonefreeSurfaceclosure; screen-tail work goes through aTerminalSurfaceRuntimeScreenTailReaderactor with global serialization.Claude injected
SessionEndhook timeout rises from 1s to 10s; Ghostty fork docs/checksums add the bounded embedded-surface teardown lineage through90ba327fc. Tests and Ghostty runtime stubs cover gate races, overlapping reads, and close paths with real runtime surfaces.Reviewed by Cursor Bugbot for commit f1335e6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores