Fix retained closed window contexts and PTY respawn - #8567
Merged
Merged
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:
📝 WalkthroughWalkthroughThe change coordinates main-window close cleanup, recoverable route ownership, workspace and Dock retirement, terminal-surface registry ownership, guarded worktree rollback, and Sidebar Git reset behavior. Tests cover routing, teardown, weak registrations, rollback, and idempotency. ChangesLifecycle retirement
Sidebar Git tracking reset
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AppKit
participant AppDelegate
participant MainWindowVisibilityController
participant TabManager
participant Workspace
participant TerminalSurfaceRegistry
AppKit->>AppDelegate: close window
AppDelegate->>MainWindowVisibilityController: commitClose(window)
AppDelegate->>TabManager: commitMainWindowClose(window)
TabManager->>Workspace: finalizeAllWorkspacesForWindowClose()
Workspace->>TerminalSurfaceRegistry: retire surface registrations
AppDelegate->>AppDelegate: remove context and recoverable route
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
Greptile SummaryCentralizes exact-window close handling and permanently retires closed workspace, Dock, terminal, and remote-session ownership while preserving recoverable windowless routes.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported transient-window, stale-duplicate, finalized-manager revival, duplicate unregister, and windowless remote-teardown issues are addressed at the current head. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Close[Exact native-window close or explicit windowless-owner close] --> Remove[Remove authoritative route]
Remove --> Finalize[Finalize TabManager and workspaces]
Finalize --> Surfaces[Teardown terminal and Dock surfaces]
Surfaces --> Registry[Unregister runtime ownership]
Registry --> Remote[Detach remote sessions]
Registry --> Sweep[Retire inactive recoverable routes]
WindowLoss[Transient weak-window loss] --> Recoverable[Retain recoverable owner]
Recoverable --> Persist[Include in persistence and quit safety]
Recoverable --> Hidden[Exclude from visible and scriptable routing]
Recoverable --> Reattach[Register exact replacement window]
Reviews (68): Last reviewed commit: "fix: allow windowless noninteractive clo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 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/Sources/CmuxTerminal/Surface/TerminalSurface.swift`:
- Around line 293-297: Restrict the ownsSurfaceRegistryRegistration property in
TerminalSurface to private visibility, preserving its existing one-shot
lifecycle behavior and internal reads or mutations within TerminalSurface.swift.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79a13355-7dae-4500-8141-d2132a231881
📒 Files selected for processing (8)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeLifecycle.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceTeardownCallbackLifetimeTests.swiftSources/App/MainWindowVisibilityController.swiftSources/AppDelegate+NonInteractiveWindowClose.swiftSources/AppDelegate.swiftSources/TabManager+NonInteractiveClose.swiftcmuxTests/ClosedMainWindowRoutingTests.swift
💤 Files with no reviewable changes (1)
- Sources/TabManager+NonInteractiveClose.swift
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 `@cmuxTests/ClosedMainWindowRoutingTests.swift`:
- Around line 267-268: Remove the unsupported manager.tabs.isEmpty assertion
from the commitMainWindowClose test, and retain the workspace.owningTabManager
== nil assertion to verify teardown state without requiring TabManager.tabs to
be emptied.
- Around line 187-191: Update the closed-window test around
toggleSidebarInActiveMainWindow() to assert the missing-window close contract:
after the call, verify the context, recoverableMainWindowRoute, and
terminalSurfaceRegistry no longer retain the manager, route, or terminal
surface. Do not preserve the existing expectations unless the production close
behavior is intentionally changed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1de92757-66cc-4dfd-a8e2-fc840ae9da29
📒 Files selected for processing (1)
cmuxTests/ClosedMainWindowRoutingTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/AppDelegate.swift (1)
16163-16172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail closed when no exact window owner matches.
tabManagerFor(windowId:)can return the manager from the same recoverable route subsequently checked by the private overload, making that manager comparison tautological. A duplicateNSWindowwith the owner’s identifier can therefore finalize the live/windowless owner and retire its terminal surfaces.Require
recoverableMainWindowRoute(windowId:)?.window === windowbefore committing this fallback; when the route has no live window, returnfalse.Proposed fix
- guard let windowId = mainWindowId(from: window), - let manager = tabManagerFor(windowId: windowId) else { + guard let windowId = mainWindowId(from: window), + let route = recoverableMainWindowRoute(windowId: windowId), + let routedWindow = route.window, + routedWindow === window, + let manager = route.tabManager else { return false }As per path instructions, close ownership must use one authoritative identity and fail closed when it is absent. Based on learnings, a missing weak window is recoverable rather than authoritative.
🤖 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 `@Sources/AppDelegate.swift` around lines 16163 - 16172, Update the fallback close path around mainWindowId, tabManagerFor(windowId:), and commitMainWindowClose so it only proceeds when recoverableMainWindowRoute(windowId:) exists and its live window is identical to window (using object identity). Return false when the route is missing or has no live window, preventing a manager lookup from authorizing a duplicate window.Sources: Path instructions, Learnings
Sources/TabManager.swift (1)
2012-2044: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
finalizeWorkspaceForRemovaltears down panels/remote connection unconditionally, before confirmingworkspaceis actually a member oftabs.
closeWorkspaceonly guardstabs.count > 1; it callsfinalizeWorkspaceForRemoval(workspace)(line 2042) before the membership check at line 2044, which is only used for array removal. If a caller ever passed a workspace not owned by thisTabManager(e.g. one belonging to another window), this path would still tear down its panels, remote connection, and null outowningTabManager— the exact class of cross-window lifecycle corruption this PR is fixing in the other direction. No current call site violates this today, but the shared helper now has a wider blast radius (used by two call sites), so it's worth failing closed here.🛡️ Suggested guard
func closeWorkspace(_ workspace: Workspace, recordHistory: Bool = true) { guard tabs.count > 1 else { return } + guard tabs.contains(where: { $0.id == workspace.id }) else { return } panelTitleUpdateCoalescer.flushNow()As per path instructions ("
**/*.{swift,ts,tsx,js,jsx,mjs,cjs}: Apply.github/review-bot-rules/reliability-single-source-of-truth.md"), correctness-critical lifecycle/teardown decisions should not rely on an implicit assumption that always happens to hold at every call site.🤖 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 `@Sources/TabManager.swift` around lines 2012 - 2044, Update closeWorkspace to verify the supplied workspace is a member of tabs before performing any history handling or calling finalizeWorkspaceForRemoval. Return immediately when no matching tab exists, then reuse the validated tab index for removal and history logic so finalizeWorkspaceForRemoval only runs on workspaces owned by this TabManager.Source: Path instructions
🤖 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 `@cmuxTests/ClosedMainWindowRoutingTests.swift`:
- Around line 262-264: Update recoverEmptyWorkspaceAfterStartupIfNeeded() to
detect that finalizeAllWorkspacesForWindowClose() has finalized the manager and
return false without calling addWorkspace(). Preserve recovery for genuinely
empty, non-finalized managers, and ensure finalization remains authoritative
after tabs are cleared.
In `@Sources/TabManager.swift`:
- Around line 1308-1315: Update detachWorkspace to call
recoverEmptyWorkspaceAfterStartupIfNeeded() instead of performing its own
tabs.isEmpty check and addWorkspace recovery, preserving the existing return
behavior while centralizing the empty-workspace invariant in the helper.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 16163-16172: Update the fallback close path around mainWindowId,
tabManagerFor(windowId:), and commitMainWindowClose so it only proceeds when
recoverableMainWindowRoute(windowId:) exists and its live window is identical to
window (using object identity). Return false when the route is missing or has no
live window, preventing a manager lookup from authorizing a duplicate window.
In `@Sources/TabManager.swift`:
- Around line 2012-2044: Update closeWorkspace to verify the supplied workspace
is a member of tabs before performing any history handling or calling
finalizeWorkspaceForRemoval. Return immediately when no matching tab exists,
then reuse the validated tab index for removal and history logic so
finalizeWorkspaceForRemoval only runs on workspaces owned by this TabManager.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 556407af-b743-4b29-8819-f639e7b50aa5
📒 Files selected for processing (7)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftSources/AppDelegate+RecoverableMainWindowRoutes.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/ClosedMainWindowRoutingTests.swift
💤 Files with no reviewable changes (1)
- Sources/Workspace.swift
…ow-contexts # Conflicts: # cmuxTests/ClosedMainWindowRoutingTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Comment on lines
+547
to
+556
| private func recoverableRouteWorkspaceIdsForRemoteTeardown( | ||
| _ route: RecoverableMainWindowRoute | ||
| ) -> [UUID] { | ||
| return route.workspaceIds.filter { workspaceId in | ||
| guard let currentOwner = tabManagerFor(tabId: workspaceId) else { | ||
| return route.tabManager == nil | ||
| } | ||
| return currentOwner === route.tabManager | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
When a windowless recoverable route is retired while its manager and remote-tmux mirror remain retained, tabManagerFor(tabId:) excludes that route because it has no live native window. The ownership filter consequently omits the route's own workspace IDs from handleWindowWorkspacesClosed, leaving the remote session or ControlMaster connection active after retirement.
Knowledge Base Used: Desktop workspace experience
austinywang
added a commit
that referenced
this pull request
Aug 26, 2026
Two constructs in today's #8567 never satisfy the Swift compiler (seen on Swift 6.3.3 / Xcode 26.5): the worktree identity fields were declared 'let ... = nil', which excludes them from the memberwise initializer the creation path calls with real values (extra arguments at #8/#9), and the rollback guard compared an optional filesystem-identity tuple against a non-optional one, which tuples do not support. Make the fields plain memberwise 'let's (the only production call site provides them; the spawn-args test now passes nil explicitly) and unwrap the current identity before comparing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
pushed a commit
that referenced
this pull request
Aug 26, 2026
… fields (#10777) * fix: restore app target compilation broken by worktree identity fields Commit 6cf5630 (#8567) declared worktreeDeviceID/worktreeFileID as 'let ... = nil', which removes them from the synthesized memberwise initializer, so the createWorktree call passing both labels failed to compile. Drop the defaults so the fields enter the memberwise init and the captured identity keeps flowing to rollback. Also unwrap the optional identity tuple in bestEffortCleanupFailedWorktree before comparing, since == is not lifted over optional tuples. * fix: add explicit return in task-group closure in worktree rollback test A multi-statement closure gets no implicit return, so the group.addTask closure returning Int32? failed to compile. Found while running the touched test class on the remote builder. --------- Co-authored-by: cmux reload-cloud <cmux-reload-cloud@users.noreply.github.com>
1 of 6 tasks
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Sep 14, 2026
`mobilePerformFailureReleasesAcceptedOperationID` uses a `TabManager` subclass whose `addWorkspaceIfActive` always returns nil, to simulate a failed workspace creation. It built that manager with the default initializer, which creates an initial workspace and, since manaflow-ai#8567, treats a nil result as a programmer error with `preconditionFailure("Initial workspace creation failed for an active window manager")`. So the test host died before the test body ran. xcodebuild relaunched the host and reran the test, which killed it again. Build the rejecting manager with `createInitialWorkspace: false`.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Sep 20, 2026
…e routing tests Since manaflow-ai#8567 a registered main-window context resolves its window from the context itself, and the lookup by window identifier is gone. These tests registered a windowless context and then named an NSWindow by identifier, so performNewWorkspaceAction found no window, dropped the context, and the assertions that follow saw nothing happen. Set the window on the context, as registerMainWindow does.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Sep 20, 2026
… tests Since manaflow-ai#8567 a registered main-window context resolves its window from the context itself, and the lookup by window identifier is gone. registerWindowedContext registered a windowless context and then named an NSWindow by identifier, so performNewWorkspaceAction found no window, dropped the context, and the routed request returned before it could report its failure. Set the window on the context, as registerMainWindow does.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Sep 20, 2026
… tests Since manaflow-ai#8567 a registered main-window context resolves its window from the context itself, and the lookup by window identifier is gone. registerWindowedContext registered a windowless context and then named an NSWindow by identifier, so performNewWorkspaceAction found no window, dropped the context, and the routed request returned before it could report its failure. Set the window on the context, as registerMainWindow does.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Sep 20, 2026
… tests Since manaflow-ai#8567 a registered main-window context resolves its window from the context itself, and the lookup by window identifier is gone. registerWindowedContext registered a windowless context and then named an NSWindow by identifier, so performNewWorkspaceAction found no window, dropped the context, and the routed request returned before it could report its failure. Set the window on the context, as registerMainWindow does.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Sep 26, 2026
…e routing tests Since manaflow-ai#8567 a registered main-window context resolves its window from the context itself, and the lookup by window identifier is gone. These tests registered a windowless context and then named an NSWindow by identifier, so performNewWorkspaceAction found no window, dropped the context, and the assertions that follow saw nothing happen. Set the window on the context, as registerMainWindow does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8349
Summary
NSWindowtransaction shared by AppKit observation, controller delegation, socket close, tab close, and orphan cleanup.TabManager,Workspace, Dock, terminal-registry, observer, remote-session, and background-work graphs so retained SwiftUI/AppKit models cannot respawn PTYs.AppDelegate.origin/mainand preserve the current last-terminal cancellation/recovery behavior.The root cause was split authority: context removal, native-window visibility, terminal registration, and close callbacks could disagree about whether an ordered-out or retained window still owned runtime work. This change makes route ownership explicit and makes successful route removal the authority boundary before irreversible teardown.
Testing
git diff --check./scripts/check-pbxproj.sh./scripts/lint-pbxproj-test-wiring.sh— passed for 655 test filespython3 scripts/check-package-resolved-policy.pypython3 scripts/check-workspace-package-groups.pypython3 scripts/check-test-determinism.py— 0 findingsxcodebuild, or XCUITest was run. Required GitHub checks are the build/test authority for this pushed HEAD.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit
Note
High Risk
Touches core window routing, session persistence, terminal registry lifetime, and irreversible teardown on close—bugs could leak PTYs, drop session state, or close the wrong window.
Overview
Centralizes main-window close in one idempotent path keyed to the exact
NSWindow, with committed-close tombstones so SwiftUI/AppKit cannot re-register or focus a window already tearing down.Recoverable routes now keep sidebar snapshots and optional Dock transfer through transient context replacement; session autosave and persistence use registered plus recoverable owners, while visible routing still requires a live exact window. Route retirement no longer depends on “any terminal surface still registered”—inactive/finalized
TabManagers are pruned instead.Closed owners are permanently finalized:
TabManager/Workspacegraphs retire viafinalizeAllWorkspacesForWindowClose, workspace creation goes throughaddWorkspaceIfActive(and related guards), and Dock stores honorisRetiredon mutations. Terminal surface registry is reworked with weak registration ledgers, dead-entry sweeps, and coalescedretireInactiveRecoverableMainWindowRoutescallbacks; surfaces unregister on explicit teardown beforedeinit.Smaller fixes: sidebar Git probe reset consolidation, workspace-group creation when host creation fails, Iroh host lifecycle test clock alignment, and extension worktree rollback when workspace admission fails after async creation.
Reviewed by Cursor Bugbot for commit b7ada49. Bugbot is set up for automated code reviews on this repo. Configure here.