fix(runtime): make terminal tab closure durable#8774
Conversation
NON_DEPLOYABLE
📝 WalkthroughWalkthroughTerminal closure now supports separate terminal and renderer-tab modes. Runtime results include close-mode metadata, tab identity is validated, and tab requests notify the renderer without killing the PTY. Workspace persistence adds durable tab removal and replay fencing. A new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab4663ce-dbb7-4c48-8a25-63d89e0b494d
📒 Files selected for processing (2)
src/main/runtime/orca-runtime-terminal-close.test.tssrc/main/runtime/orca-runtime.ts
chsong1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/runtime/orca-runtime.ts (1)
4909-4936: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire the dormant-close state into this helper
closeHeadlessMobileTerminalTabonly ever gets the defaultalreadyKilledPtyIds/durableRemovalCompletedvalues from its current call sites, so the duplicate-kill and durable-removal branches never run. Either pass that state through from the dormant close path or drop the extra parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7a3b0a2-b1ba-4ac7-8ae5-ef45691fe156
📒 Files selected for processing (6)
src/main/persistence.test.tssrc/main/persistence.tssrc/main/runtime/orca-runtime-terminal-close.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/errors.test.tssrc/main/runtime/rpc/errors.ts
| durableRemoval: true | ||
| } | ||
|
|
||
| const TERMINAL_TAB_REMOVAL_REPLAY_FENCE_MAX = 32 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not evict live replay fences after 32 closes.
The test in src/main/persistence.test.ts at Line 9667 confirms that the 33rd removal lets a stale snapshot resurrect bounded-tab-0. This breaks deterministic cleanup and can persist a tab that was already closed. Retain fences until renderer/session-generation acknowledgement rather than evicting solely by count.
Also applies to: 6314-6320
| const activeGroup = groups.find((group) => group.id === activeGroupId) ?? groups[0] ?? null | ||
| const activeUnifiedId = activeGroup?.activeTabId ?? activeGroup?.tabOrder[0] | ||
| const activeUnifiedTab = next.unifiedTabs?.[identity.worktreeId]?.find( | ||
| (tab) => tab.id === activeUnifiedId | ||
| ) | ||
| const browserSurvivors = next.browserTabsByWorktree?.[identity.worktreeId] ?? [] | ||
| const fileSurvivors = next.openFilesByWorktree?.[identity.worktreeId] ?? [] | ||
| const hasSurfaceSurvivor = | ||
| nextTabs.length > 0 || | ||
| (next.unifiedTabs?.[identity.worktreeId]?.length ?? 0) > 0 || | ||
| browserSurvivors.length > 0 || | ||
| fileSurvivors.length > 0 | ||
| const restoredBrowserId = next.activeBrowserTabIdByWorktree?.[identity.worktreeId] ?? null | ||
| const restoredFileId = next.activeFileIdByWorktree?.[identity.worktreeId] ?? null | ||
| const restoredTerminalId = next.activeTabIdByWorktree?.[identity.worktreeId] ?? null | ||
| const browserStillOpen = browserSurvivors.some((browser) => browser.id === restoredBrowserId) | ||
| const fileStillOpen = fileSurvivors.some((file) => file.filePath === restoredFileId) | ||
| const terminalStillExists = nextTabs.some((tab) => tab.id === restoredTerminalId) | ||
| const hasGroupOwnedSurface = groups.length > 0 || groupLayout !== undefined | ||
| let projectedBrowserId: string | null | ||
| let projectedFileId: string | null | ||
| let projectedType: 'terminal' | 'browser' | 'simulator' | 'editor' | ||
| if (activeUnifiedTab) { | ||
| projectedFileId = | ||
| terminalTabRemovalVisibleType(activeUnifiedTab.contentType) === 'editor' | ||
| ? activeUnifiedTab.entityId | ||
| : fileStillOpen | ||
| ? restoredFileId | ||
| : null | ||
| projectedBrowserId = | ||
| activeUnifiedTab.contentType === 'browser' | ||
| ? activeUnifiedTab.entityId | ||
| : browserStillOpen | ||
| ? restoredBrowserId | ||
| : (browserSurvivors[0]?.id ?? null) | ||
| projectedType = terminalTabRemovalVisibleType(activeUnifiedTab.contentType) | ||
| } else if (hasGroupOwnedSurface) { | ||
| projectedFileId = fileStillOpen ? restoredFileId : null | ||
| projectedBrowserId = browserStillOpen ? restoredBrowserId : (browserSurvivors[0]?.id ?? null) | ||
| projectedType = 'terminal' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prioritize real browser/file survivors over empty group chrome.
A retained empty tab group makes hasGroupOwnedSurface true, so Line 3050 selects terminal before checking surviving legacy browser or file surfaces. Closing the last terminal can therefore leave the workspace in terminal mode with no terminal.
Proposed branch ordering
- } else if (hasGroupOwnedSurface) {
- projectedFileId = fileStillOpen ? restoredFileId : null
- projectedBrowserId = browserStillOpen ? restoredBrowserId : (browserSurvivors[0]?.id ?? null)
- projectedType = 'terminal'
- } else if (browserStillOpen) {
+ } else if (browserStillOpen) {
projectedFileId = fileStillOpen ? restoredFileId : null
projectedBrowserId = restoredBrowserId
projectedType = 'browser'
} else if (fileStillOpen) {
projectedFileId = restoredFileId
projectedBrowserId = browserSurvivors[0]?.id ?? null
projectedType = 'editor'
+ } else if (hasGroupOwnedSurface) {
+ projectedFileId = null
+ projectedBrowserId = browserSurvivors[0]?.id ?? null
+ projectedType = 'terminal'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const activeGroup = groups.find((group) => group.id === activeGroupId) ?? groups[0] ?? null | |
| const activeUnifiedId = activeGroup?.activeTabId ?? activeGroup?.tabOrder[0] | |
| const activeUnifiedTab = next.unifiedTabs?.[identity.worktreeId]?.find( | |
| (tab) => tab.id === activeUnifiedId | |
| ) | |
| const browserSurvivors = next.browserTabsByWorktree?.[identity.worktreeId] ?? [] | |
| const fileSurvivors = next.openFilesByWorktree?.[identity.worktreeId] ?? [] | |
| const hasSurfaceSurvivor = | |
| nextTabs.length > 0 || | |
| (next.unifiedTabs?.[identity.worktreeId]?.length ?? 0) > 0 || | |
| browserSurvivors.length > 0 || | |
| fileSurvivors.length > 0 | |
| const restoredBrowserId = next.activeBrowserTabIdByWorktree?.[identity.worktreeId] ?? null | |
| const restoredFileId = next.activeFileIdByWorktree?.[identity.worktreeId] ?? null | |
| const restoredTerminalId = next.activeTabIdByWorktree?.[identity.worktreeId] ?? null | |
| const browserStillOpen = browserSurvivors.some((browser) => browser.id === restoredBrowserId) | |
| const fileStillOpen = fileSurvivors.some((file) => file.filePath === restoredFileId) | |
| const terminalStillExists = nextTabs.some((tab) => tab.id === restoredTerminalId) | |
| const hasGroupOwnedSurface = groups.length > 0 || groupLayout !== undefined | |
| let projectedBrowserId: string | null | |
| let projectedFileId: string | null | |
| let projectedType: 'terminal' | 'browser' | 'simulator' | 'editor' | |
| if (activeUnifiedTab) { | |
| projectedFileId = | |
| terminalTabRemovalVisibleType(activeUnifiedTab.contentType) === 'editor' | |
| ? activeUnifiedTab.entityId | |
| : fileStillOpen | |
| ? restoredFileId | |
| : null | |
| projectedBrowserId = | |
| activeUnifiedTab.contentType === 'browser' | |
| ? activeUnifiedTab.entityId | |
| : browserStillOpen | |
| ? restoredBrowserId | |
| : (browserSurvivors[0]?.id ?? null) | |
| projectedType = terminalTabRemovalVisibleType(activeUnifiedTab.contentType) | |
| } else if (hasGroupOwnedSurface) { | |
| projectedFileId = fileStillOpen ? restoredFileId : null | |
| projectedBrowserId = browserStillOpen ? restoredBrowserId : (browserSurvivors[0]?.id ?? null) | |
| projectedType = 'terminal' | |
| const activeGroup = groups.find((group) => group.id === activeGroupId) ?? groups[0] ?? null | |
| const activeUnifiedId = activeGroup?.activeTabId ?? activeGroup?.tabOrder[0] | |
| const activeUnifiedTab = next.unifiedTabs?.[identity.worktreeId]?.find( | |
| (tab) => tab.id === activeUnifiedId | |
| ) | |
| const browserSurvivors = next.browserTabsByWorktree?.[identity.worktreeId] ?? [] | |
| const fileSurvivors = next.openFilesByWorktree?.[identity.worktreeId] ?? [] | |
| const hasSurfaceSurvivor = | |
| nextTabs.length > 0 || | |
| (next.unifiedTabs?.[identity.worktreeId]?.length ?? 0) > 0 || | |
| browserSurvivors.length > 0 || | |
| fileSurvivors.length > 0 | |
| const restoredBrowserId = next.activeBrowserTabIdByWorktree?.[identity.worktreeId] ?? null | |
| const restoredFileId = next.activeFileIdByWorktree?.[identity.worktreeId] ?? null | |
| const restoredTerminalId = next.activeTabIdByWorktree?.[identity.worktreeId] ?? null | |
| const browserStillOpen = browserSurvivors.some((browser) => browser.id === restoredBrowserId) | |
| const fileStillOpen = fileSurvivors.some((file) => file.filePath === restoredFileId) | |
| const terminalStillExists = nextTabs.some((tab) => tab.id === restoredTerminalId) | |
| const hasGroupOwnedSurface = groups.length > 0 || groupLayout !== undefined | |
| let projectedBrowserId: string | null | |
| let projectedFileId: string | null | |
| let projectedType: 'terminal' | 'browser' | 'simulator' | 'editor' | |
| if (activeUnifiedTab) { | |
| projectedFileId = | |
| terminalTabRemovalVisibleType(activeUnifiedTab.contentType) === 'editor' | |
| ? activeUnifiedTab.entityId | |
| : fileStillOpen | |
| ? restoredFileId | |
| : null | |
| projectedBrowserId = | |
| activeUnifiedTab.contentType === 'browser' | |
| ? activeUnifiedTab.entityId | |
| : browserStillOpen | |
| ? restoredBrowserId | |
| : (browserSurvivors[0]?.id ?? null) | |
| projectedType = terminalTabRemovalVisibleType(activeUnifiedTab.contentType) | |
| } else if (browserStillOpen) { | |
| projectedFileId = fileStillOpen ? restoredFileId : null | |
| projectedBrowserId = restoredBrowserId | |
| projectedType = 'browser' | |
| } else if (fileStillOpen) { | |
| projectedFileId = restoredFileId | |
| projectedBrowserId = browserSurvivors[0]?.id ?? null | |
| projectedType = 'editor' | |
| } else if (hasGroupOwnedSurface) { | |
| projectedFileId = null | |
| projectedBrowserId = browserSurvivors[0]?.id ?? null | |
| projectedType = 'terminal' |
|
Thanks for identifying and driving this cleanup gap. Independent validation found that this implementation could acknowledge before renderer persistence, did not cover headless or dormant-tab closure, and could resurrect closes after the bounded replay fence rolled over. The CLI contract and product direction are preserved in #8958, which uses canonical terminal retirement plus an awaited disk flush and a headless persisted-session projection. Closing this version in favor of #8958 so there is one canonical implementation to review. |
2 participants
Summary
orca terminal close --tablifecycle primitive while making final tab closure durable across persistence and runtime reconstructionWhy
Completed automation runs were closing the PTY but leaving or reconstructing the renderer tab. Repeated schedules therefore accumulated completed worktrees and tabs in Orca. The durable close path now validates the exact terminal identity, commits a replay-safe projection, and prevents stale selectors from recreating the closed container.
Verification
git diff --checkpassedOperational follow-up
After CI, the patched macOS app will be installed through a rollback-safe local transaction. mctl will then reconcile only receipt-owned completed automation tabs, followed by an idempotent second cleanup and scheduled no-growth canary cycles.