Fix #1983: avoid workspace creation crash after restore#1985
Merged
lawrencecchen merged 2 commits intomainfrom Mar 25, 2026
Merged
Fix #1983: avoid workspace creation crash after restore#1985lawrencecchen merged 2 commits intomainfrom
lawrencecchen merged 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughUpdated PortScanner callback to be Changes
Sequence Diagram(s)sequenceDiagram
participant TabMgr as TabManager
participant OldWS as Workspace (old)
participant NewWS as Workspace (new)
participant Remote as RemoteConnection
participant Port as PortScanner
rect rgba(255,127,0,0.5)
Note over TabMgr: restoreSessionSnapshot() begins
TabMgr->>TabMgr: capture previousTabs
TabMgr->>TabMgr: assign new tabs & selectedTabId
TabMgr->>TabMgr: prune pendingBackgroundWorkspaceLoadIds / sidebarSelectedWorkspaceIds
end
rect rgba(255,99,71,0.5)
Note over TabMgr,OldWS: releaseRestoredAwayWorkspace() for each old workspace
TabMgr->>OldWS: clear notifications
TabMgr->>OldWS: tear down panels
TabMgr->>Remote: disconnect remote connection
TabMgr->>OldWS: set owningTabManager = nil
end
rect rgba(70,130,180,0.5)
Note over Port,NewWS: port update delivery on main actor
Port->>Port: Task { `@MainActor` in onPortsUpdated(...) }
Port->>NewWS: update surfaceListeningPorts
Port->>NewWS: recomputeListeningPorts()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR fixes the post-restore new-workspace crash (#1983) via two targeted changes: promoting
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant PS as PortScanner (bg queue)
participant MA as Main Actor
participant TC as TerminalController
participant TM as TabManager
participant WS as Workspace
Note over PS: Burst scan completes
PS->>PS: deliverResults()
PS->>MA: Task { @MainActor in callback(...) }
MA->>TC: onPortsUpdated(workspaceId, panelId, ports)
TC->>TM: tabs.first(where: id == workspaceId)
alt workspace found & panel valid
TC->>WS: surfaceListeningPorts[panelId] = ports
TC->>WS: recomputeListeningPorts()
else workspace not found (released after restore)
TC-->>TC: guard returns early
end
Note over TM: Session restore begins
TM->>TM: previousTabs = tabs
TM->>TM: unwireClosedBrowserTracking (previousTabs)
TM->>TM: build newTabs from snapshot
TM->>TM: tabs = newTabs (atomic swap)
TM->>TM: pruneBackgroundWorkspaceLoads
TM->>TM: sidebarSelectedWorkspaceIds.formIntersection
loop each old workspace
TM->>WS: clearNotifications(forTabId)
TM->>WS: teardownAllPanels() → PortScanner.unregisterPanel(...)
TM->>WS: teardownRemoteConnection()
TM->>WS: owningTabManager = nil
end
Reviews (1): Last reviewed commit: "Fix workspace creation crash after resto..." | Re-trigger Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/PortScanner.swift (1)
174-178:⚠️ Potential issue | 🟡 MinorPreserve burst result ordering in high-frequency port updates.
The burst scanner (
runBurst) callsdeliverResults()multiple times in rapid succession, each wrapping results in separateTask {@mainactorin ... }submissions. Swift does not guarantee FIFO ordering for independent actor task submissions—later scans can complete before earlier ones, causing stale port state to temporarily overwrite fresh results.Add sequence numbers to gate deliveries:
Ordering pattern to preserve scan sequence
+ private var nextDeliverySequence: UInt64 = 0 + `@MainActor` private var lastAppliedDeliverySequence: UInt64 = 0 + private func deliverResults(_ results: [(PanelKey, [Int])]) { guard let callback = onPortsUpdated else { return } + let sequence = nextDeliverySequence + nextDeliverySequence &+= 1 Task { `@MainActor` in + guard sequence >= lastAppliedDeliverySequence else { return } + lastAppliedDeliverySequence = sequence for (key, ports) in results { callback(key.workspaceId, key.panelId, ports) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/PortScanner.swift` around lines 174 - 178, The Burst scanner's deliverResults submissions can be reordered because each Task { `@MainActor` in ... } is independent; modify deliverResults() (called by runBurst) to attach an incrementing sequence number (e.g., scanSeq) to each result batch, stash the latest delivered sequence on the MainActor (or a dedicated `@MainActor` property) and have the MainActor task compare sequence numbers before invoking callback(workspaceId:panelId:ports) so only newer-or-equal sequences apply; update runBurst/deliverResults to increment and pass the sequence and update the stored last-applied sequence when a task actually runs to prevent stale deliveries from overwriting newer state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 1022-1024: The closure assigned to
PortScanner.shared.onPortsUpdated currently resolves the workspace via
self.tabManager (active-window only) which drops updates for workspaces in other
windows; change the resolution to use AppDelegate.shared?.workspaceFor(tabId:
workspaceId) (as used by sendPickedElementToTerminal(workspaceId:summary:)) so
the lookup searches across all mainWindowContexts, and keep the existing weak
self capture and early-return behavior if workspace lookup fails; update any
references to tabManager.tabs.first(where:) to use the AppDelegate lookup to
ensure surfaceListeningPorts/listeningPorts are updated for workspaces in other
windows.
---
Outside diff comments:
In `@Sources/PortScanner.swift`:
- Around line 174-178: The Burst scanner's deliverResults submissions can be
reordered because each Task { `@MainActor` in ... } is independent; modify
deliverResults() (called by runBurst) to attach an incrementing sequence number
(e.g., scanSeq) to each result batch, stash the latest delivered sequence on the
MainActor (or a dedicated `@MainActor` property) and have the MainActor task
compare sequence numbers before invoking callback(workspaceId:panelId:ports) so
only newer-or-equal sequences apply; update runBurst/deliverResults to increment
and pass the sequence and update the stored last-applied sequence when a task
actually runs to prevent stale deliveries from overwriting newer state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc20f0e0-9e8f-4565-95b4-9c5bf4042426
📒 Files selected for processing (3)
Sources/PortScanner.swiftSources/TabManager.swiftSources/TerminalController.swift
Comment on lines
1022
to
+1024
| PortScanner.shared.onPortsUpdated = { [weak self] workspaceId, panelId, ports in | ||
| MainActor.assumeIsolated { | ||
| guard let self, let tabManager = self.tabManager else { return } | ||
| guard let workspace = tabManager.tabs.first(where: { $0.id == workspaceId }) else { return } | ||
| let validSurfaceIds = Set(workspace.panels.keys) | ||
| guard validSurfaceIds.contains(panelId) else { return } | ||
| workspace.surfaceListeningPorts[panelId] = ports.isEmpty ? nil : ports | ||
| workspace.recomputeListeningPorts() | ||
| } | ||
| guard let self, let tabManager = self.tabManager else { return } | ||
| guard let workspace = tabManager.tabs.first(where: { $0.id == workspaceId }) else { return } |
There was a problem hiding this comment.
Resolve port-scan updates by workspaceId, not the active-window tabManager.
Line 1023/Line 1024 still route through self.tabManager, which only tracks the active window. Port updates for workspaces in other windows will be dropped here, so their surfaceListeningPorts/listeningPorts can silently go stale.
Suggested fix
- PortScanner.shared.onPortsUpdated = { [weak self] workspaceId, panelId, ports in
- guard let self, let tabManager = self.tabManager else { return }
- guard let workspace = tabManager.tabs.first(where: { $0.id == workspaceId }) else { return }
+ PortScanner.shared.onPortsUpdated = { workspaceId, panelId, ports in
+ guard let workspace = AppDelegate.shared?.workspaceFor(tabId: workspaceId) else { return }
let validSurfaceIds = Set(workspace.panels.keys)
guard validSurfaceIds.contains(panelId) else { return }
workspace.surfaceListeningPorts[panelId] = ports.isEmpty ? nil : ports
workspace.recomputeListeningPorts()
}Based on learnings: sendPickedElementToTerminal(workspaceId:summary:) resolves the target workspace via AppDelegate.shared?.workspaceFor(tabId: workspaceId), which searches across all mainWindowContexts (not self.tabManager, which is active-window only).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 1022 - 1024, The closure
assigned to PortScanner.shared.onPortsUpdated currently resolves the workspace
via self.tabManager (active-window only) which drops updates for workspaces in
other windows; change the resolution to use
AppDelegate.shared?.workspaceFor(tabId: workspaceId) (as used by
sendPickedElementToTerminal(workspaceId:summary:)) so the lookup searches across
all mainWindowContexts, and keep the existing weak self capture and early-return
behavior if workspace lookup fails; update any references to
tabManager.tabs.first(where:) to use the AppDelegate lookup to ensure
surfaceListeningPorts/listeningPorts are updated for workspaces in other
windows.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TabManager.swift`:
- Around line 5166-5173: In releaseRestoredAwayWorkspace, clear the workspace's
owningTabManager before tearing down panels or the remote connection to prevent
callbacks (e.g., Workspace.closeWorkspaceWithConfirmation called during
teardownAllPanels) from re-entering TabManager against the restored tab list;
specifically, set workspace.owningTabManager = nil prior to calling
workspace.teardownAllPanels() and workspace.teardownRemoteConnection() in the
releaseRestoredAwayWorkspace helper.
Comment on lines
+5166
to
+5173
| private func releaseRestoredAwayWorkspace(_ workspace: Workspace) { | ||
| // Session restore replaces the bootstrap workspace objects with freshly | ||
| // restored ones. Tear the old graph down after the atomic swap so late | ||
| // panel/socket callbacks cannot keep mutating hidden pre-restore state. | ||
| AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: workspace.id) | ||
| workspace.teardownAllPanels() | ||
| workspace.teardownRemoteConnection() | ||
| workspace.owningTabManager = nil |
There was a problem hiding this comment.
Detach the old workspace before tearing its panels down.
Unlike closeWorkspace(_:), this helper runs after the old workspace has already been removed from tabs. Workspace can still call back into owningTabManager?.closeWorkspaceWithConfirmation(self) during panel teardown (see Sources/Workspace.swift, Lines 9747-9750), so leaving owningTabManager set through teardownAllPanels() lets restore cleanup re-enter TabManager against the restored tab list. That can prompt or close the wrong workspace/window. Nil the manager before teardown.
🛠️ Proposed fix
private func releaseRestoredAwayWorkspace(_ workspace: Workspace) {
// Session restore replaces the bootstrap workspace objects with freshly
// restored ones. Tear the old graph down after the atomic swap so late
// panel/socket callbacks cannot keep mutating hidden pre-restore state.
AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: workspace.id)
+ workspace.owningTabManager = nil
workspace.teardownAllPanels()
workspace.teardownRemoteConnection()
- workspace.owningTabManager = nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 5166 - 5173, In
releaseRestoredAwayWorkspace, clear the workspace's owningTabManager before
tearing down panels or the remote connection to prevent callbacks (e.g.,
Workspace.closeWorkspaceWithConfirmation called during teardownAllPanels) from
re-entering TabManager against the restored tab list; specifically, set
workspace.owningTabManager = nil prior to calling workspace.teardownAllPanels()
and workspace.teardownRemoteConnection() in the releaseRestoredAwayWorkspace
helper.
6 tasks
zzdif
pushed a commit
to zzdif/cmux
that referenced
this pull request
Mar 26, 2026
Co-authored-by: Lawrence Chen <[email protected]>
10 tasks
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
2 participants
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 #1983.
Summary
Verification
Summary by cubic
Fixes #1983 by preventing the post-restore crash when creating a new workspace. Port updates now run on the main actor, and replaced workspaces are fully torn down so stale callbacks can’t touch them.
PortScanner.onPortsUpdatedas@MainActorand deliver results via a main-actorTask; simplify theTerminalControllerhandler.Written for commit 61e25a5. Summary will update on new commits.
Summary by CodeRabbit