Repository navigation
Preserve recoverable windows in session snapshots - #9749
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:
📝 WalkthroughWalkthroughMain-window lifecycle state now belongs to ChangesMain-window lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Window
participant AppDelegate
participant MainWindowLifecycleCoordinator
participant RecoverableMainWindowRoute
participant SessionSnapshot
Window->>AppDelegate: register, orphan, or close
AppDelegate->>MainWindowLifecycleCoordinator: transition lifecycle state
MainWindowLifecycleCoordinator->>RecoverableMainWindowRoute: retain live or frozen route state
AppDelegate->>SessionSnapshot: build ordered persistence snapshot
SessionSnapshot-->>AppDelegate: return window, dock, and workspace state
Possibly related issues
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 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: 5
🤖 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 217-226: Extend the recovered-window assertions in the session
snapshot test to inspect the recovered SessionWindowSnapshot, not just its
identifiers. Verify sidebar visibility, sidebar width, sidebar selection, and
dock state match the restored window’s expected persisted values, ensuring the
recovery path preserves the full payload.
In `@Sources/AppDelegate.swift`:
- Around line 4100-4105: Extract the route sorting logic into a shared
orderedSessionRouteSnapshots() helper, preserving the key-window-first ordering
used by buildSessionSnapshotResult. Update both sessionAutosaveFingerprint and
buildSessionSnapshotResult to use this helper before applying
SessionPersistencePolicy.maxWindowsPerSnapshot, ensuring both select the same
window subset.
In `@Sources/AppDelegate`+RecoverableMainWindowRoutes.swift:
- Around line 99-116: Update recoverableMainWindowRouteSnapshot(for:) to
preserve the route’s existing DockSplitStore instead of setting windowDock to
nil. Capture or retain the dock before teardownWindowDock() clears it, so
sessionWindowSnapshot(for route:) continues producing the dock session snapshot
for recoverable routes.
- Around line 145-157: Update mainWindowRouteSnapshots and the
tabManagerFor(windowId:) lookup path to avoid rescanning all recoverable routes
on every lookup. Build and reuse a window-ID snapshot index for the current
operation or actor turn, invalidating it when the relevant context or route
ledger changes, while preserving registered-route precedence.
- Around line 86-88: Update recoverableWindowParticipatesInLiveTopology(_:) to
stop using NSApp.isHidden as evidence that a window participates in live
topology. Track and consult the per-window ordered-out state instead, returning
false when that state is absent, while preserving participation for visible or
miniaturized windows.
🪄 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: 16a7d5ed-a423-4d7a-a634-49f37949a490
📒 Files selected for processing (3)
Sources/AppDelegate+RecoverableMainWindowRoutes.swiftSources/AppDelegate.swiftcmuxTests/ClosedMainWindowRoutingTests.swift
|
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
4103-4128: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe fingerprint prefix and the snapshot loop can still select different windows.
Both functions now share
orderedSessionRouteSnapshots(), so the ordering matches. The selection does not.sessionAutosaveFingerprinthashes only the firstmaxWindowsPerSnapshotroutes.buildSessionSnapshotResultskips routes throughomitsRemoteMirrorOnlyWindowand through crash-diagnostic pruning, then breaks only after it has appendedmaxWindowsPerSnapshotwindows. When any route is skipped, the persisted set reaches past the fingerprint prefix. A change inside such a window does not change the fingerprint, so autosave does not run and the persisted session goes stale.Hash every route. The fingerprint then covers a superset of the persisted windows.
🐛 Proposed fix
- for route in routes.prefix(SessionPersistencePolicy.maxWindowsPerSnapshot) { + // Hash every route. buildSessionSnapshotResult skips remote-mirror-only + // and crash-diagnostic windows, so its persisted set can reach past the + // first maxWindowsPerSnapshot routes. + for route in routes { hasher.combine(route.windowId)🤖 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 4103 - 4128, Update sessionAutosaveFingerprint to combine fingerprint data for every route from orderedSessionRouteSnapshots(), rather than limiting iteration with SessionPersistencePolicy.maxWindowsPerSnapshot. Keep the existing per-route hashing logic unchanged so the fingerprint covers all routes, including those that may be skipped by buildSessionSnapshotResult.
🤖 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-269: Extend the test around the recovered
SessionWindowSnapshot after window.orderOut(nil) to assert the app-hidden
visibility state and per-window restore-target fields captured before hiding.
Keep the existing windowId assertion, but validate the exact app-hidden route
payload so the test fails if recovery retains the window record while losing its
visibility restore target.
- Around line 329-337: Harden the assertions in the session snapshot test by
first asserting that the projected window and workspace source arrays each
contain exactly two entries, then retain the existing Set comparisons against
the registered and recovered IDs. Apply this to the unified
registered-plus-recoverable route projection represented by snapshot.windows and
its tabManager.workspaces.
In `@Sources/AppDelegate.swift`:
- Around line 4643-4656: Widen registeredMainWindowRouteSnapshot(for:) from
private to internal, then replace the duplicated MainWindowRouteSnapshot
construction in the sessionWindowSnapshot call with that helper, passing the
current MainWindowContext and preserving the existing includeScrollback,
restorableAgentIndex, and surfaceResumeBindingIndex arguments.
In `@Sources/AppDelegate`+RecoverableMainWindowRoutes.swift:
- Around line 265-276: The tabManagerFor(windowId:) method must stop falling
back to raw mainWindowRouteLedger routes. Add a separate teardown-only accessor
for bookkeeping that can resolve lingering managers from the ledger, while
keeping tabManagerFor limited to registered mainWindowContexts and validated
recoverableMainWindowRouteSnapshot results.
In `@Sources/MainWindowRouteDockState.swift`:
- Around line 1-21: Annotate the MainWindowRouteDockState enum with `@MainActor`
so its sessionSnapshot method and DockSplitStore interaction are main-actor
isolated, while preserving the existing live and frozen behavior.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 4103-4128: Update sessionAutosaveFingerprint to combine
fingerprint data for every route from orderedSessionRouteSnapshots(), rather
than limiting iteration with SessionPersistencePolicy.maxWindowsPerSnapshot.
Keep the existing per-route hashing logic unchanged so the fingerprint covers
all routes, including those that may be skipped by buildSessionSnapshotResult.
🪄 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: 791bf2c3-04a6-4858-8e7b-353b2e73d74e
📒 Files selected for processing (8)
Sources/AppDelegate+RecoverableMainWindowRoutes.swiftSources/AppDelegate.swiftSources/MainWindowRouteDockState.swiftSources/MainWindowRouteSnapshot.swiftSources/RecoverableMainWindowRoute.swiftSources/RecoverableMainWindowRoutePurpose.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ClosedMainWindowRoutingTests.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 `@Sources/MainWindowRouteAutosaveProjection.swift`:
- Around line 36-39: Refactor the loop over uniqueOrderedWindowIds so the where
clause only checks the selection limit, then use an explicit loop body to insert
each window ID, append it only when insertion succeeds, and break once selected
reaches limit. Preserve the existing selectedWindowIds deduplication and
selection order without mutating state in the loop condition.
In `@Sources/Workspace`+SessionPersistenceSelection.swift:
- Line 60: Update combineSessionPersistenceSelectionMetadata() to avoid hashing
the full restoredTerminalScrollbackByPanelId string; instead combine a presence
indicator and bounded length/size proxy so the fingerprint changes when
scrollback is set, cleared, or replaced while remaining cheap for large restored
content.
🪄 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: 9191d47f-7d02-44d0-9378-5fe9eaa90bb0
📒 Files selected for processing (16)
Sources/App/MainWindowVisibilityController+Lifecycle.swiftSources/AppDelegate+RecoverableMainWindowRoutes.swiftSources/AppDelegate+WindowDock.swiftSources/AppDelegate.swiftSources/CmuxLifecycleEventPublishing.swiftSources/DockSplitStore+SessionSnapshot.swiftSources/MainWindowRouteAutosaveProjection.swiftSources/MainWindowRouteDockState.swiftSources/MainWindowRouteSnapshot.swiftSources/RemoteTmuxController.swiftSources/TabManager+SessionPersistenceSelection.swiftSources/Workspace+SessionPersistenceSelection.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ClosedMainWindowRoutingTests.swiftcmuxTests/MainWindowVisibilityLifecycleTests.swiftcmuxTests/RecoverableMainWindowLifecycleTests.swift
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/RecoverableMainWindowLifecycleTests.swift`:
- Around line 337-342: Update the browser surface setup in
RecoverableMainWindowLifecycleTests to use nil or about:blank instead of the
live example.com URL. Preserve the existing creationPolicy, focus, and
browser-panel assertions because the test only requires the surface, not initial
network navigation.
In `@Sources/AppDelegate`+RecoverableMainWindowRoutes.swift:
- Around line 221-237: Update the documentation for
mainWindowPersistenceRouteSnapshots() to explicitly state that it mutates
lifecycle state by freezing windowless recoverable routes, replacing records,
tearing down panels, and releasing remote connections; keep the existing
projection behavior unchanged.
In `@Sources/MainWindowLifecycleCoordinator.swift`:
- Around line 181-188: Update removeRecoverableRoute so the guard returns
immediately when record.phase is .registered, leaving removal and
bumpPersistenceTopologyRevision on the normal path for all other phases.
Preserve the existing record lookup and removal behavior.
In `@Sources/MainWindowRouteAutosaveProjection.swift`:
- Around line 3-5: Use one shared eligible, ordered route collection for both
fingerprinting and snapshot construction in AppDelegate, filtering
remote-tmux-only windows before applying the maxWindowsPerSnapshot limit. Update
orderedSessionRouteSnapshots consumers so skipped routes cannot consume
fingerprint slots, and add a regression test covering one skipped route, one
persisted route, and a one-window fingerprint limit.
🪄 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: 9b04a885-b0ca-45b3-b139-7002598c28b4
📒 Files selected for processing (16)
Sources/AppDelegate+RecoverableMainWindowRoutes.swiftSources/AppDelegate.swiftSources/MainWindowLifecycleCoordinator.swiftSources/MainWindowLifecyclePhase.swiftSources/MainWindowLifecycleRecord.swiftSources/MainWindowPersistenceRouteSnapshot.swiftSources/MainWindowRouteAutosaveProjection.swiftSources/MainWindowRouteSnapshot.swiftSources/RecoverableMainWindowRoute.swiftSources/RecoverableMainWindowRoutePayload.swiftSources/SessionWindowSnapshot+Scrollback.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateIssue2907RoutingTests.swiftcmuxTests/AppDelegateMainWindowTestingSupport.swiftcmuxTests/RecoverableMainWindowLifecycleTests.swiftcmuxTests/WorkspaceRecoveryTests.swift
💤 Files with no reviewable changes (1)
- Sources/MainWindowRouteSnapshot.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. |
|
All contributors have signed the CLA ✍️ ✅ |
…w-restore-crash # Conflicts: # .github/workflows/cla-policy-guard.yml
…w-restore-crash # Conflicts: # .github/workflows/cmux-skill-contract.yml # Sources/RestorableAgentSession.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. |
…w-restore-crash # Conflicts: # cmuxTests/FileDropOverlayViewTests.swift
PR #9749 review auditVerified against HEAD
Top-level review bodies
Explicit trade-offs
|
|
Merge-policy note for closeout: the current repository ruleset requires one approving review, while this PR has 32/32 review threads explicitly answered and resolved and no CHANGES_REQUESTED. The required checks and final remote lifecycle gates are green, and the PR author has personally verified the restore behavior. Under the explicit closeout instruction, I am using the administrator merge path; this bypasses only the missing independent approval, not any status or thread requirement. |
|
Closeout status: all 32 review threads are explicitly answered and resolved, there are no CHANGES_REQUESTED reviews, all visible/required checks are green, and the final remote lifecycle gates passed at HEAD 91b5a4b. The instructed squash merge was attempted with a head-SHA guard, including the administrator path, but GitHub rejected it because the live ruleset requires an approval from someone other than the last pusher (require_last_push_approval). This PR remains open pending a legitimate independent maintainer approval; no code or check blocker remains. |
…w-restore-crash # Conflicts: # Sources/AppDelegate.swift # cmux.xcodeproj/project.pbxproj
bb05939 Preserve recoverable windows in session snapshots (manaflow-ai#9749)
* test: preserve recoverable windows in session snapshots * fix: preserve recoverable windows in session snapshots * fix: harden recoverable window lifecycle * refactor: isolate recoverable window route models * fix: isolate dock route snapshots to main actor * fix: separate live and teardown window routes * test: cover recoverable window lifecycle gaps * fix: unify recoverable window lifecycle finalization * fix: return live dock snapshots * fix: preserve bounded recoverable snapshots * fix: harden recoverable snapshot safety * test: cover production windowless recovery path * fix: make window recovery lifecycle authoritative * fix: freeze windowless recovery state * test: cover recovery freeze safety gaps * fix: restore persistence route compilation * test: stabilize windowless recovery fixture * test: isolate windowless routing state * test: target active windowless recovery path * fix: harden frozen recovery state * test: migrate issue 2907 routing suite * test: remove stale visibility accessor assertions * test: serialize shared app delegate snapshots * test: cover windowless transient cleanup * fix: retire windowless transient state * test: cover lifecycle freeze finalization * fix: finalize recoverable window lifecycle ownership * test: preserve detected binding during recovery freeze * fix: preserve verified bindings during recovery freeze * test: allow hidden orphan window replacement * fix: coalesce windowless recovery scans * fix: preserve hidden orphan reattachment * fix: consume frozen routes during reopen * fix: bound recovery scan churn * fix: bound frozen route persistence work * fix: preserve lightweight orphan recovery * fix: account for eligible orphan persistence slots * fix: bound orphan cleanup and TTY capture * fix: retain full fidelity for bounded orphans * fix: keep fingerprint projections read-only * fix: deadline fresh orphan detection * fix: own deferred orphan freeze tasks * fix: preserve bounded recovery scan ownership * fix: release completed recovery scan handles * fix: fail closed on unavailable recovery scans * fix: fail closed while recovery scan drains * fix: merge late recovery bindings * fix: align orphan freeze eligibility * fix: count frozen recovery slots * fix: align recovery task result types * fix: retire pruned frozen routes * refactor: name local snapshot values clearly * fix: protect reattached windows and dock bindings * fix: retain recovery fallback and deadline ownership * fix: make recovery deadline race structured * fix: bound and qualify orphan process bindings * fix: restore merged lifecycle compatibility * test: cover lifecycle lookup index repair * fix: close lifecycle review regressions * fix: harden recovery persistence fallbacks * test: cover recoverable route ownership teardown * fix: preserve recoverable route ownership during teardown * test: cover hidden recoverable window scripting * fix: keep explicitly hidden recoverable windows routable * test: cover compatibility route reattachment * fix: close orphan ownership and adoption gaps * fix: avoid retaining orphaned window contexts * fix: compare recovered sidebar selections by value * fix: expose shared TTY bindings to recovery lifecycle * fix: type windowless recovery task result * fix: scope recovered window replacement checks * fix: consolidate exact restored-session lookup * fix: restore hibernation record memberwise initialization * fix: wire main window routing test file * fix: keep Swift Testing notification predicate nonthrowing * fix: handle throwing mouse event test helpers * fix: align browser test helpers with current APIs * fix: align simulator and socket test helpers * fix: complete hibernation index test fixtures * fix: update recovery lifecycle fixture for optional docks * fix: unwrap browser drag translation assertions * fix: annotate window dock lifecycle test result * fix: constrain window dock gate result * fix: make window dock test gate effects explicit * fix: assert the live recovered dock owner * fix: retain recoverable close observer and remote-only fixture * test: align recovery routing with current lifecycle contracts * test: stabilize app-host regression fixtures * fix: avoid weak self capture in recovery worker callback * test: cover delayed frozen orphan retention * fix: harden recoverable route persistence * fix: make recovery fallback Swift 5 compatible * ci: document hosted skill contract runner * test: restore cloud test target compatibility
Addresses the restore-loss path in #9666.
Summary
willClosewindow cannot be resurrected by autosave.NSApp.isHidden.Investigation boundary
Cross-window
workspace selectresolves the destination throughTerminalController+ControlWorkspaceContext.swift, focuses throughAppDelegate.focusMainWindow, and selects on the main actor. That path does not directly mutate WebKit or display-link state. The supplied crash is on the CVDisplayLink thread in WebKit, and the reporter could not reproduce it on 0.64.22 after five trials, so a causal link remains unproven.The restore loss is independently proven: routing included terminal-backed recovery routes while session autosave enumerated only
mainWindowContexts. A live window whose context was orphaned could therefore remain routable but disappear from the saved topology.Regression coverage
The first commit is test-only, followed by the production fix in a separate commit. Focused coverage now verifies:
Validation
scripts/check-pbxproj.shscripts/lint-pbxproj-test-wiring.shscripts/check-package-resolved-policy.pyscripts/check-workspace-package-groups.py --checkf4270ad51bfailed on the newly added verified-binding freeze regression: run 31289054856.aeae1af9aapasses all nineRecoverableMainWindowLifecycleTests: run 31289688004.MainWindowLifecycleCoordinatorTestssuite also passes: run 31289688841.issue-9666-crosswindow-restore-crashtag.OK workspace:N; every health check returnedPONG, with a stable process PID within each launch.SIGKILLonly to the tagged process, relaunched through the tagged opener, and confirmed all five window/workspace UUID pairs restored.Note
High Risk
Changes core window registration, close/orphan transitions, and session snapshot construction—areas where subtle lifecycle bugs can drop windows from restore or resurrect closing UI.
Overview
Fixes session restore loss where orphaned main windows stayed routable but were omitted from autosave because snapshots only walked registered
mainWindowContexts.MainWindowLifecycleCoordinatorreplaces the separate recovery ledger and context map with one phased model (registered→orphaned→closing). Session autosave and fingerprinting now useorderedSessionRouteSnapshots, merging live registered routes with recoverable orphans (including frozen value snapshots after AppKit loses the window).Orphan recovery preserves sidebar state and can freeze windowless routes asynchronously (tear down live panels, retain bounded snapshots). Intentional closes go through
closingso they are excluded from persistence even if AppKit still reports the window visible. Process-detected resume bindings without verification are downgraded to manual during those freezes.Routing, focus, teardown, and closed-window history are updated to use route snapshots and
tabManagerForWindowTeardownwhere teardown-only managers must not receive live work.Reviewed by Cursor Bugbot for commit 77136d6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Final closeout revalidation (2026-09-03)
5abbd2033008f5bbbe312d3c391a19f4517dc92b; it includes the clean three-way synchronization merge with currentorigin/main69cea3077841885e667d9e670400f781a44745d5. No iOS paths are in the PR diff.8342f7e6e8makes only the minimal test-target compatibility corrections; it changes no production behavior.cmux-dev-backend-1did not resolve; I retried on the fleet withCMUX_DEV_BACKEND_MODE=off, built successfully oncmux11s-mac-mini.1in 431s, and launched the isolated tagged build. Four scratch windows were persisted; five cross-window selections before restart and four after restart returnedPONG; killing/relaunching only the tagged process restored all four exact window/workspace UUID pairs. The tagged log contained no restore/save/fatal errors or new diagnostic report.git diff --check. The historical Swift file-length script is not present because it was removed in Remove Swift file length budget #8125.