diff --git a/src/renderer/src/components/terminal-pane/TerminalPaneSurface.tsx b/src/renderer/src/components/terminal-pane/TerminalPaneSurface.tsx index 4df42a74de2..8e301366c14 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPaneSurface.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPaneSurface.tsx @@ -176,7 +176,10 @@ export function TerminalPaneSurface({ return requestTerminalPaneRecovery({ tabId, ptyId, - reason: 'reattach-unverifiable' + reason: 'reattach-unverifiable', + // The user asking again is the new trigger that reopens + // a reason an observed failure has closed. + trigger: 'user' }).then((recovered) => { if (recovered) { dismissTerminalError() diff --git a/src/renderer/src/components/terminal-pane/pty-connection-hidden-delivery-gate.test.ts b/src/renderer/src/components/terminal-pane/pty-connection-hidden-delivery-gate.test.ts index 645c522fcfc..844db35baee 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection-hidden-delivery-gate.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection-hidden-delivery-gate.test.ts @@ -17,6 +17,11 @@ import { restoreTerminalTestGlobals } from './pty-connection-test-environment' +/** What remountTerminalTabForRecovery answers now that it reports admission. */ +const REMOUNTED = { remounted: true as const, generation: 1 } +const TAB_MISSING = { remounted: false as const, declinedBy: 'tab-missing' as const } +const AUTOMATIC_REQUEST = expect.objectContaining({ trigger: 'automatic' }) + const { resetAndRefreshAllTerminalWebglAtlases, scheduleTerminalWebglAtlasRecovery, @@ -300,7 +305,7 @@ describe('connectPanePty', () => { it('kicks pane recovery when reveal finds the write pipeline certified dead', async () => { // 2026-07-13 fossil-pane incident: bytes drop while hidden, pipeline certified dead, cert recovery empty — reveal must re-kick it. enableMainAuthority() - const remountTerminalTabForRecovery = vi.fn<(tabId: string) => boolean>(() => true) + const remountTerminalTabForRecovery = vi.fn(() => REMOUNTED) mockStoreState = { ...mockStoreState, remountTerminalTabForRecovery } as StoreState const { _resetTerminalPaneRecoveryForTests } = await import('./terminal-pane-recovery') _resetTerminalPaneRecoveryForTests() @@ -319,7 +324,7 @@ describe('connectPanePty', () => { dataCallback('hidden output\r\n', { seq: 16, rawLength: 16 }) // Pipeline dies while hidden; certification-time recovery finds no remountable tab (budget unconsumed, no retry timer). - remountTerminalTabForRecovery.mockReturnValueOnce(false) + remountTerminalTabForRecovery.mockReturnValueOnce(TAB_MISSING as never) const ackCredit = vi.fn() const { writeTerminalOutput } = await import('@/lib/pane-manager/pane-terminal-output-scheduler') @@ -345,7 +350,7 @@ describe('connectPanePty', () => { // Restore stays skipped (a dead pipeline can't parse the snapshot), but recovery got exactly one re-kick. expect(getMainBufferSnapshot).not.toHaveBeenCalled() expect(remountTerminalTabForRecovery).toHaveBeenCalledTimes(2) - expect(remountTerminalTabForRecovery).toHaveBeenLastCalledWith('tab-1') + expect(remountTerminalTabForRecovery).toHaveBeenLastCalledWith('tab-1', AUTOMATIC_REQUEST) // Latched per xterm instance: repeat restore attempts do not spam. _dispatchPtyModelRestoreNeededForTest({ id: 'pty-id', reason: 'hidden-drop', markerSeq: 96 }) diff --git a/src/renderer/src/components/terminal-pane/pty-connection-terminal-input-gating.test.ts b/src/renderer/src/components/terminal-pane/pty-connection-terminal-input-gating.test.ts index 7853d305145..4659fccb5c5 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection-terminal-input-gating.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection-terminal-input-gating.test.ts @@ -23,6 +23,10 @@ import { restoreTerminalTestGlobals } from './pty-connection-test-environment' +/** What remountTerminalTabForRecovery answers now that it reports admission. */ +const REMOUNTED = { remounted: true as const, generation: 1 } +const AUTOMATIC_REQUEST = expect.objectContaining({ trigger: 'automatic' }) + const { resetAndRefreshAllTerminalWebglAtlases, scheduleTerminalWebglAtlasRecovery, @@ -787,7 +791,7 @@ describe('connectPanePty', () => { const { connectPanePty } = await import('./pty-connection') const { _resetTerminalPaneRecoveryForTests } = await import('./terminal-pane-recovery') _resetTerminalPaneRecoveryForTests() - const remountTerminalTabForRecovery = vi.fn<(tabId: string) => boolean>(() => true) + const remountTerminalTabForRecovery = vi.fn(() => REMOUNTED) mockStoreState = { ...mockStoreState, remountTerminalTabForRecovery } as StoreState const transport = createMockTransport('daemon-pty') let writeUnavailable: (() => void) | undefined @@ -803,7 +807,7 @@ describe('connectPanePty', () => { await flushAsyncTicks(6) expect(window.api.pty.hasPty).toHaveBeenCalledWith('daemon-pty') - expect(remountTerminalTabForRecovery).toHaveBeenCalledWith('tab-1') + expect(remountTerminalTabForRecovery).toHaveBeenCalledWith('tab-1', AUTOMATIC_REQUEST) _resetTerminalPaneRecoveryForTests() }) @@ -811,7 +815,7 @@ describe('connectPanePty', () => { const { connectPanePty } = await import('./pty-connection') const { _resetTerminalPaneRecoveryForTests } = await import('./terminal-pane-recovery') _resetTerminalPaneRecoveryForTests() - const remountTerminalTabForRecovery = vi.fn<(tabId: string) => boolean>(() => true) + const remountTerminalTabForRecovery = vi.fn(() => REMOUNTED) mockStoreState = { ...mockStoreState, remountTerminalTabForRecovery } as StoreState const transport = createMockTransport('daemon-pty') let writeUnavailable: (() => void) | undefined @@ -826,7 +830,7 @@ describe('connectPanePty', () => { await flushAsyncTicks(6) writeUnavailable?.() await flushAsyncTicks(6) - expect(remountTerminalTabForRecovery).toHaveBeenCalledWith('tab-1') + expect(remountTerminalTabForRecovery).toHaveBeenCalledWith('tab-1', AUTOMATIC_REQUEST) // The surviving tail of `echo hi; rm -rf x`: reaching the fresh shell would // let the user's own Enter run `rm -rf x` (#10065 follow-up). @@ -848,7 +852,7 @@ describe('connectPanePty', () => { const { connectPanePty } = await import('./pty-connection') const { settleTerminalWriteStallWatch, WRITE_PIPELINE_STALL_CHECK_MS } = await import('@/lib/pane-manager/terminal-write-pipeline-health') - const remountTerminalTabForRecovery = vi.fn<(tabId: string) => boolean>(() => true) + const remountTerminalTabForRecovery = vi.fn(() => REMOUNTED) mockStoreState = { ...mockStoreState, remountTerminalTabForRecovery } as StoreState const transport = createMockTransport('pty-wedged') transportFactoryQueue.push(transport) @@ -865,7 +869,7 @@ describe('connectPanePty', () => { expect(transport.sendInput).toHaveBeenCalledWith('x') expect(pane.terminal.write).toHaveBeenCalledWith('', expect.any(Function)) - expect(remountTerminalTabForRecovery).toHaveBeenCalledWith('tab-1') + expect(remountTerminalTabForRecovery).toHaveBeenCalledWith('tab-1', AUTOMATIC_REQUEST) binding.dispose() }) diff --git a/src/renderer/src/components/terminal-pane/pty-connection/agent-idle-working-handlers.ts b/src/renderer/src/components/terminal-pane/pty-connection/agent-idle-working-handlers.ts index 8516eab7f19..3e379b2d4f8 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/agent-idle-working-handlers.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/agent-idle-working-handlers.ts @@ -12,6 +12,7 @@ import { isWebTerminalSurfaceTabId } from '@/runtime/web-terminal-surface-id' import type { DirectSshPaneRetryAttempt } from '@/store/slices/direct-ssh-terminal-recovery' import { directSshAuthoritiesEqual } from '@/store/slices/direct-ssh-terminal-authority-ledger' +import { settleTerminalPaneRecovery } from '../terminal-pane-recovery' import type { ConnectPanePtySession } from './connect-pane-pty-session' export function installAgentIdleWorkingHandlers(session: ConnectPanePtySession): void { @@ -235,11 +236,16 @@ export function installAgentIdleWorkingHandlers(session: ConnectPanePtySession): } return canAdopt } - session.settleDirectSshPaneRetryAttempt = ( + // One settle for this pane's attach attempt, reporting to both ledgers that + // track it: the direct-SSH pane retry (when a lease owns this attempt) and + // the tab's recovery ledger. Keeping them on one call is what stops a second + // settle path drifting out of step with the first. + session.settlePaneAttachAttempt = ( attempt: DirectSshRetryLease | undefined, - status: 'failed' | 'timed-out' + status: 'success' | 'failed' | 'timed-out' ): void => { - if (!attempt) { + settleTerminalPaneRecovery(session.deps.tabId, session.terminalRecoveryGeneration, status) + if (!attempt || status === 'success') { return } useAppStore.getState().settleDirectSshPaneRetry?.({ diff --git a/src/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty.ts b/src/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty.ts index 2b55854c676..a354833c05c 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty.ts @@ -2,10 +2,8 @@ import type { PaneManager, ManagedPane } from '@/lib/pane-manager/pane-manager' import { useAppStore } from '@/store' import { TerminalKittyKeyboardModeTracker } from '../../../../../shared/terminal-kitty-keyboard-mode-tracker' import type { PtyConnectionDeps } from '../pty-connection-types' -import { - captureTerminalPaneRecoveryGeneration, - registerTerminalPaneRecoveryInstance -} from '../terminal-pane-recovery' +import { registerTerminalPaneRecoveryInstance } from '../terminal-pane-recovery' +import { captureTabRecoveryGeneration } from '@/store/terminals/terminal-tab-recovery-ledger' import { RESET_TERMINAL_CURSOR_STYLE } from '../../../../../shared/terminal-mode-reset-profiles' import { writeTerminalOutput } from '@/lib/pane-manager/pane-terminal-output-scheduler' import { createTerminalStructuralReplayCoordinator } from '@/lib/pane-manager/terminal-structural-replay-coordinator' @@ -89,7 +87,9 @@ export function connectPanePty( session.tabGeneration = tab?.generation ?? 0 // Why: recovery ownership belongs to this xterm instance. A request that // settles after remount must not remount its already-replaced successor. - session.terminalRecoveryGeneration = captureTerminalPaneRecoveryGeneration(session.deps.tabId) + // Read off the row already resolved above — the epoch lives on it, so this + // costs no extra scan of tabsByWorktree on the connect path. + session.terminalRecoveryGeneration = captureTabRecoveryGeneration(terminalTab) session.terminalRecoveryInstance = registerTerminalPaneRecoveryInstance(session.deps.tabId) session.mountFollowsTerminalPark = session.deps.mountFollowsTerminalPark session.authoritativeReattachGeneration = 0 diff --git a/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.ts b/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.ts index 66cfc4f526b..3548d3f7b9f 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.ts @@ -90,7 +90,7 @@ export function startDeferredSessionReattach( if (typeof gen === 'number') { void window.api.pty.clearPendingPaneSerializer(session.cacheKey, gen).catch(() => {}) } - session.settleDirectSshPaneRetryAttempt(session.directSshRetryAttempt, 'failed') + session.settlePaneAttachAttempt(session.directSshRetryAttempt, 'failed') return } if (!result && expiredReattachError) { @@ -153,7 +153,7 @@ export function startDeferredSessionReattach( } if (message.includes(PANE_OWNER_UNVERIFIED_ERROR)) { session.reportError(message) - session.settleDirectSshPaneRetryAttempt(session.directSshRetryAttempt, 'failed') + session.settlePaneAttachAttempt(session.directSshRetryAttempt, 'failed') return } warnTerminalLifecycleAnomaly('restored PTY reattach threw', { diff --git a/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.test.ts b/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.test.ts index 03f3da2f158..0e8af7bfd8d 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.test.ts @@ -13,21 +13,23 @@ describe('recoverUnverifiableDirectSshReattach', () => { it('retries through the exact direct SSH lease when one exists', () => { const attempt = { attemptId: 'attempt-1' } - const settleDirectSshPaneRetryAttempt = vi.fn() + const settlePaneAttachAttempt = vi.fn() recoverUnverifiableDirectSshReattach( - { directSshRetryAttempt: attempt, settleDirectSshPaneRetryAttempt } as never, + { directSshRetryAttempt: attempt, settlePaneAttachAttempt } as never, 'ssh:target@@pty-1' ) - expect(settleDirectSshPaneRetryAttempt).toHaveBeenCalledExactlyOnceWith(attempt, 'failed') + expect(settlePaneAttachAttempt).toHaveBeenCalledExactlyOnceWith(attempt, 'failed') expect(requestTerminalPaneRecovery).not.toHaveBeenCalled() }) it('remounts over the preserved PTY when no retry lease exists', () => { + const settlePaneAttachAttempt = vi.fn() recoverUnverifiableDirectSshReattach( { directSshRetryAttempt: undefined, + settlePaneAttachAttempt, deps: { tabId: 'tab-1' }, terminalRecoveryGeneration: 2, terminalRecoveryInstance: { id: 3 } @@ -35,6 +37,13 @@ describe('recoverUnverifiableDirectSshReattach', () => { 'ssh:target@@pty-1' ) + // Settled before the re-request: this failure is the outcome of the + // remount that mounted this pane, and the ledger must read it that way + // before the pane asks for the same action again (crash b5cfc6ca). + expect(settlePaneAttachAttempt).toHaveBeenCalledExactlyOnceWith(undefined, 'failed') + expect(settlePaneAttachAttempt.mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(requestTerminalPaneRecovery).mock.invocationCallOrder[0] + ) expect(requestTerminalPaneRecovery).toHaveBeenCalledExactlyOnceWith({ tabId: 'tab-1', ptyId: 'ssh:target@@pty-1', diff --git a/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.ts b/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.ts index f32896fc254..ed19e677a97 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.ts @@ -5,8 +5,13 @@ export function recoverUnverifiableDirectSshReattach( session: ConnectPanePtySession, ptyId: string | null | undefined ): void { - if (session.directSshRetryAttempt) { - session.settleDirectSshPaneRetryAttempt(session.directSshRetryAttempt, 'failed') + // Read before settling: the settle clears the lease this branch tests. + const directSshRetryOwnsRecovery = Boolean(session.directSshRetryAttempt) + // Settle BEFORE requesting: this failure is the outcome of the remount that + // mounted this pane. Requesting first would ask for a repeat of the action + // that just failed while its ledger still read 'pending' — the storm. + session.settlePaneAttachAttempt(session.directSshRetryAttempt, 'failed') + if (directSshRetryOwnsRecovery) { return } void requestTerminalPaneRecovery({ diff --git a/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-retry-status.ts b/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-retry-status.ts index a0f72936c5d..bcc34b05070 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-retry-status.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-retry-status.ts @@ -55,7 +55,7 @@ export function installDirectSshRetryStatus(session: ConnectPanePtySession): voi if (session.directSshPaneRetrySettlementCancelled) { return } - session.settleDirectSshPaneRetryAttempt(attempt, 'timed-out') + session.settlePaneAttachAttempt(attempt, 'timed-out') }, DIRECT_SSH_PANE_RETRY_SETTLEMENT_TIMEOUT_MS) session.directSshPaneRetrySettlementTimers.add(timer) void promise diff --git a/src/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler.ts b/src/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler.ts index 6450a71567c..1d81b92a064 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler.ts @@ -45,6 +45,7 @@ type ReattachResultSession = ReattachPayloadSession & | 'sampleVisiblePaneForegroundAgent' | 'scheduleReattachIdleAgentCursorReset' | 'serializeHiddenOutputSnapshot' + | 'settlePaneAttachAttempt' | 'setPanePtyFitBinding' | 'startFreshColdRestoreAgentResume' | 'structuralReplayCoordinator' @@ -163,6 +164,10 @@ export function bindHandleReattachResult(sessionBag: ConnectPanePtySession): voi if (!isCurrentReattachPayload()) { return false } + // The first authoritative attach of the pane a recovery remount produced — + // past the no-PTY-id and session-expired branches, which are failures and + // settle themselves. This is the observation the ledger was waiting for. + session.settlePaneAttachAttempt?.(undefined, 'success') // Strict precedence snapshot > replay > coldRestore: paint exactly one, else overlapping tails duplicate TUI output on worktree switch. const hasStructuralReplay = Boolean( connectResult?.snapshot || connectResult?.replay || connectResult?.coldRestore diff --git a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts index 25bcd4fe1ed..749d8c73492 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts @@ -13,7 +13,7 @@ function buildSession(overrides: Record = {}): never { terminalRecoveryGeneration: 2, terminalRecoveryInstance: { id: 3 }, directSshRetryAttempt: undefined, - settleDirectSshPaneRetryAttempt: vi.fn(), + settlePaneAttachAttempt: vi.fn(), ...overrides } as never } @@ -37,24 +37,24 @@ describe('settleSpawnThatLeftPaneUnbound', () => { it('leaves recovery to the direct SSH retry ledger when it holds a lease', () => { const attempt = { attemptId: 'attempt-1' } - const settleDirectSshPaneRetryAttempt = vi.fn() + const settlePaneAttachAttempt = vi.fn() settleSpawnThatLeftPaneUnbound( - buildSession({ directSshRetryAttempt: attempt, settleDirectSshPaneRetryAttempt }) + buildSession({ directSshRetryAttempt: attempt, settlePaneAttachAttempt }) ) - expect(settleDirectSshPaneRetryAttempt).toHaveBeenCalledExactlyOnceWith(attempt, 'failed') + expect(settlePaneAttachAttempt).toHaveBeenCalledExactlyOnceWith(attempt, 'failed') expect(requestTerminalPaneRecovery).not.toHaveBeenCalled() }) it('settles the spawn as failed before remounting', () => { - const settleDirectSshPaneRetryAttempt = vi.fn() + const settlePaneAttachAttempt = vi.fn() settleSpawnThatLeftPaneUnbound( - buildSession({ deps: { tabId: 'tab-settle' }, settleDirectSshPaneRetryAttempt }) + buildSession({ deps: { tabId: 'tab-settle' }, settlePaneAttachAttempt }) ) - expect(settleDirectSshPaneRetryAttempt).toHaveBeenCalledExactlyOnceWith(undefined, 'failed') + expect(settlePaneAttachAttempt).toHaveBeenCalledExactlyOnceWith(undefined, 'failed') expect(requestTerminalPaneRecovery).toHaveBeenCalledOnce() }) diff --git a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts index 698df12651c..a6dfd3c5a96 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts @@ -19,7 +19,7 @@ import type { ConnectPanePtySession } from './connect-pane-pty-session' export function settleSpawnThatLeftPaneUnbound(session: ConnectPanePtySession): void { // Read before settling: the settle clears the lease this branch tests. const directSshRetryOwnsRecovery = Boolean(session.directSshRetryAttempt) - session.settleDirectSshPaneRetryAttempt(session.directSshRetryAttempt, 'failed') + session.settlePaneAttachAttempt(session.directSshRetryAttempt, 'failed') if (directSshRetryOwnsRecovery) { return } diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts index 9e4289f06cb..d63d54ae85d 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts @@ -1,30 +1,92 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createRemountTerminalTabForRecovery, + createSettleTerminalTabRecovery +} from '@/store/slices/worktrees/session/worktree-slice-lookups' +import type { TerminalTab } from '../../../../shared/terminal-tab-types' import { _resetTerminalPaneRecoveryForTests, captureTerminalPaneRecoveryGeneration, registerTerminalPaneRecoveryInstance, - requestTerminalPaneRecovery + requestTerminalPaneRecovery, + settleTerminalPaneRecovery } from './terminal-pane-recovery' import { isTerminalInputQuarantined } from './terminal-input-quarantine' -type StoredTerminalTab = { id: string; viewMode?: 'terminal' | 'chat' } +type StoredTerminalTab = Pick +const WORKTREE_ID = 'repo1::/path/wt1' + +// The store actions here are the REAL ones, driven over a minimal state bag. +// The recovery budget is a field on the tab row now, so a fake that only +// answered booleans could no longer express what this module reads. const mocks = vi.hoisted(() => ({ - remountTerminalTabForRecovery: vi.fn<(tabId: string) => boolean>(() => true), + state: { + tabsByWorktree: {} as Record, + terminalLayoutsByTabId: {} as Record, + pendingStartupByTabId: {} as Record + }, + // Records tabIds the store ACTUALLY remounted, so every assertion below keeps + // meaning "a remount happened" rather than "a remount was asked for". + remountTerminalTabForRecovery: vi.fn<(tabId: string) => void>(), getTab: vi.fn<() => { viewMode?: 'terminal' | 'chat' } | null>(() => ({})), - // The remount index. Kept separate from getTab so a test can stage the - // drift between the two that crash b5cfc6ca rode in on. - terminalTabs: [] as StoredTerminalTab[], recordRendererCrashBreadcrumb: vi.fn(), hasPty: vi.fn<(id: string) => Promise>(async () => true) })) +const storeSet = (updater: unknown): void => { + const patch = + typeof updater === 'function' + ? (updater as (state: unknown) => object)(mocks.state) + : (updater as object) + Object.assign(mocks.state, patch) +} +const storeGet = (): unknown => mocks.state +const realRemount = createRemountTerminalTabForRecovery(storeSet as never, storeGet as never) +const realSettle = createSettleTerminalTabRecovery(storeSet as never, storeGet as never) + +const remountTerminalTabForRecovery: typeof realRemount = (tabId, request) => { + const result = realRemount(tabId, request) + if (result.remounted) { + mocks.remountTerminalTabForRecovery(tabId) + } + return result +} + +function terminalTabs(): StoredTerminalTab[] { + return (mocks.state.tabsByWorktree[WORKTREE_ID] ?? []) as StoredTerminalTab[] +} + +function setTerminalTabs(tabs: StoredTerminalTab[]): void { + mocks.state.tabsByWorktree = { [WORKTREE_ID]: tabs } +} + +/** What a mounted pane reports for the tab's current recovery attempt. */ +function settleCurrentRecovery(tabId: string, outcome: 'success' | 'failed' | 'timed-out'): void { + settleTerminalPaneRecovery(tabId, captureTerminalPaneRecoveryGeneration(tabId), outcome) +} + +/** A full cycle: request, then the pane the remount mounted reports back. + * Recovery gates on an observed outcome, so a caller that never reports is + * refused — these are the callers that DO report. */ +async function requestAndSettle( + request: Parameters[0], + outcome: 'success' | 'failed' | 'timed-out' = 'success' +): Promise { + const recovered = await requestTerminalPaneRecovery(request) + if (recovered) { + settleCurrentRecovery(request.tabId, outcome) + } + return recovered +} + vi.mock('@/store', () => ({ useAppStore: { getState: () => ({ - remountTerminalTabForRecovery: mocks.remountTerminalTabForRecovery, - getTab: mocks.getTab, - tabsByWorktree: { 'repo1::/path/wt1': mocks.terminalTabs } + ...mocks.state, + remountTerminalTabForRecovery, + settleTerminalTabRecovery: realSettle, + getTab: mocks.getTab }) } })) @@ -35,11 +97,12 @@ vi.mock('@/lib/crash-breadcrumb-recorder', () => ({ beforeEach(() => { _resetTerminalPaneRecoveryForTests() - mocks.remountTerminalTabForRecovery.mockClear() - mocks.remountTerminalTabForRecovery.mockReturnValue(true) + mocks.remountTerminalTabForRecovery.mockReset() mocks.getTab.mockClear() mocks.getTab.mockReturnValue({}) - mocks.terminalTabs = [{ id: 'tab-1' }, { id: 'tab-ssh' }] + mocks.state.terminalLayoutsByTabId = {} + mocks.state.pendingStartupByTabId = {} + setTerminalTabs([{ id: 'tab-1' }, { id: 'tab-ssh' }]) mocks.recordRendererCrashBreadcrumb.mockClear() mocks.hasPty.mockClear() mocks.hasPty.mockResolvedValue(true) @@ -57,7 +120,7 @@ afterEach(() => { describe('requestTerminalPaneRecovery', () => { it('does not remount a terminal surface hidden behind native chat', async () => { - mocks.getTab.mockReturnValue({ viewMode: 'chat' }) + setTerminalTabs([{ id: 'tab-1', viewMode: 'chat' }]) await expect( requestTerminalPaneRecovery({ @@ -72,9 +135,9 @@ describe('requestTerminalPaneRecovery', () => { it('does not remount a chat-owned tab the unified tab index has dropped', async () => { // The drift crash b5cfc6ca documents: present in tabsByWorktree, gone from - // unifiedTabsByWorktree. getTab answers null, so the guard used to pass. + // unifiedTabsByWorktree. getTab answers null, and the guard reads the row. mocks.getTab.mockReturnValue(null) - mocks.terminalTabs = [{ id: 'tab-1', viewMode: 'chat' }] + setTerminalTabs([{ id: 'tab-1', viewMode: 'chat' }]) await expect( requestTerminalPaneRecovery({ @@ -87,6 +150,21 @@ describe('requestTerminalPaneRecovery', () => { expect(mocks.hasPty).not.toHaveBeenCalled() }) + it('reads chat ownership from the row alone, not from the unified tab', async () => { + // The local viewMode toggles now patch the row in the same set(), so the + // unified tab is never the tie-breaker — a stale one cannot veto a heal. + mocks.getTab.mockReturnValue({ viewMode: 'chat' }) + setTerminalTabs([{ id: 'tab-1', viewMode: 'terminal' }]) + + await expect( + requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'write-stalled' + }) + ).resolves.toBe(true) + }) + it('remounts the tab and records a breadcrumb for a certified-dead pipeline', async () => { const result = await requestTerminalPaneRecovery({ tabId: 'tab-1', @@ -118,8 +196,6 @@ describe('requestTerminalPaneRecovery', () => { }) it('records a breadcrumb when the tab cannot be remounted, without consuming budget', async () => { - mocks.remountTerminalTabForRecovery.mockReturnValue(false) - const result = await requestTerminalPaneRecovery({ tabId: 'tab-gone', ptyId: 'pty-1', @@ -132,7 +208,7 @@ describe('requestTerminalPaneRecovery', () => { { tabId: 'tab-gone', reason: 'restore-blocked' } ) // Budget untouched: a later request for the same tab may still remount. - mocks.remountTerminalTabForRecovery.mockReturnValue(true) + setTerminalTabs([...terminalTabs(), { id: 'tab-gone' }]) expect( await requestTerminalPaneRecovery({ tabId: 'tab-gone', @@ -147,7 +223,7 @@ describe('requestTerminalPaneRecovery', () => { vi.setSystemTime(0) expect( - await requestTerminalPaneRecovery({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' }) + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' }) ).toBe(true) expect( await requestTerminalPaneRecovery({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'replay-wedged' }) @@ -161,25 +237,140 @@ describe('requestTerminalPaneRecovery', () => { expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(2) }) + it('refuses a second request while the last remount has reported nothing', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + + expect( + await requestTerminalPaneRecovery({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' }) + ).toBe(true) + // Past the cooldown, inside the cap — only the unsettled attempt refuses it. + vi.setSystemTime(16_000) + expect( + await requestTerminalPaneRecovery({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'replay-wedged' }) + ).toBe(false) + expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(1) + + settleCurrentRecovery('tab-1', 'success') + expect( + await requestTerminalPaneRecovery({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'replay-wedged' }) + ).toBe(true) + }) + + it('refuses the same reason again once a pane reported it failed', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + + expect( + await requestAndSettle( + { tabId: 'tab-ssh', ptyId: 'ssh:target@@pty-1', reason: 'reattach-unverifiable' }, + 'failed' + ) + ).toBe(true) + + // Far past every window: counting would have healed, evidence has not. + for (const now of [16_000, 60_000, 600_000]) { + vi.setSystemTime(now) + expect( + await requestTerminalPaneRecovery({ + tabId: 'tab-ssh', + ptyId: 'ssh:target@@pty-1', + reason: 'reattach-unverifiable' + }) + ).toBe(false) + } + expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(1) + + // The user pressing Retry is the new trigger the refusal waits for. + expect( + await requestTerminalPaneRecovery({ + tabId: 'tab-ssh', + ptyId: 'ssh:target@@pty-1', + reason: 'reattach-unverifiable', + trigger: 'user' + }) + ).toBe(true) + }) + + it('reopens a settled failure when the row moves to a new generation', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + await requestAndSettle( + { tabId: 'tab-ssh', ptyId: 'ssh:target@@pty-1', reason: 'reattach-unverifiable' }, + 'failed' + ) + vi.setSystemTime(60_000) + expect( + await requestTerminalPaneRecovery({ + tabId: 'tab-ssh', + ptyId: 'ssh:target@@pty-1', + reason: 'reattach-unverifiable' + }) + ).toBe(false) + + // An SSH authority rotation / activation respawn bumps tab.generation. + setTerminalTabs( + terminalTabs().map((tab) => + tab.id === 'tab-ssh' ? { ...tab, generation: (tab.generation ?? 0) + 1 } : tab + ) + ) + expect( + await requestTerminalPaneRecovery({ + tabId: 'tab-ssh', + ptyId: 'ssh:target@@pty-1', + reason: 'reattach-unverifiable' + }) + ).toBe(true) + }) + + it('does not reopen a settled failure when a host rebuild drops generation', async () => { + // A remote-runtime snapshot rebuilds the row without `generation`. That is a + // field going missing, not a new trigger — reading it as one would restore + // the tab's allowance on every republication. + vi.useFakeTimers() + vi.setSystemTime(0) + await requestAndSettle( + { tabId: 'tab-ssh', ptyId: 'ssh:target@@pty-1', reason: 'reattach-unverifiable' }, + 'failed' + ) + const rebuilt = terminalTabs().map((tab) => + tab.id === 'tab-ssh' ? { id: tab.id, recovery: tab.recovery } : tab + ) + setTerminalTabs(rebuilt) + + vi.setSystemTime(60_000) + expect( + await requestTerminalPaneRecovery({ + tabId: 'tab-ssh', + ptyId: 'ssh:target@@pty-1', + reason: 'reattach-unverifiable' + }) + ).toBe(false) + }) + it('caps recoveries per window to prevent remount storms', async () => { vi.useFakeTimers() for (let attempt = 0; attempt < 5; attempt += 1) { vi.setSystemTime(attempt * 20_000) - await requestTerminalPaneRecovery({ + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' }) } expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(3) + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_pane_recovery_window_cap', + { tabId: 'tab-1', reason: 'write-stalled' } + ) }) - it('releases recovery budget and retries when the tab closes', async () => { + it('drops the budget with the row the tab closure removes', async () => { vi.useFakeTimers() const instance = registerTerminalPaneRecoveryInstance('tab-1') for (let attempt = 0; attempt < 4; attempt += 1) { vi.setSystemTime(attempt * 20_000) - await requestTerminalPaneRecovery({ + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' @@ -188,14 +379,21 @@ describe('requestTerminalPaneRecovery', () => { expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(3) expect(vi.getTimerCount()).toBe(1) - mocks.terminalTabs = [] - mocks.getTab.mockReturnValue(null) + // Disposing the pane must NOT release anything: that release is what erased + // every consumed remount and let the cap lapse (crash b5cfc6ca). instance.unregister() + expect(captureTerminalPaneRecoveryGeneration('tab-1')).toBe(3) + expect( + await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'write-stalled' + }) + ).toBe(false) + // Closing the tab drops the row, and the budget with it — same object. + setTerminalTabs([{ id: 'tab-1' }]) expect(captureTerminalPaneRecoveryGeneration('tab-1')).toBe(0) - expect(vi.getTimerCount()).toBe(0) - mocks.terminalTabs = [{ id: 'tab-1' }] - mocks.getTab.mockReturnValue({}) expect( await requestTerminalPaneRecovery({ tabId: 'tab-1', @@ -210,7 +408,7 @@ describe('requestTerminalPaneRecovery', () => { vi.setSystemTime(0) for (let attempt = 0; attempt < 3; attempt += 1) { vi.setSystemTime(attempt * 20_000) - await requestTerminalPaneRecovery({ + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' @@ -241,7 +439,9 @@ describe('requestTerminalPaneRecovery', () => { vi.useFakeTimers() for (let attempt = 0; attempt < 4; attempt += 1) { vi.setSystemTime(attempt * 20_000) - await requestTerminalPaneRecovery({ + // Each remounted pane attaches, then wedges again minutes later: the + // window cap, not the outcome gate, is what this test is about. + await requestAndSettle({ tabId: 'tab-ssh', ptyId: 'ssh:target@@pty-1', reason: 'reattach-unverifiable', @@ -280,7 +480,7 @@ describe('requestTerminalPaneRecovery', () => { it('retries a fresh replacement xterm that wedges during the cooldown', async () => { vi.useFakeTimers() vi.setSystemTime(0) - await requestTerminalPaneRecovery({ + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled', @@ -305,7 +505,7 @@ describe('requestTerminalPaneRecovery', () => { it('does not let an awaited scheduled retry remount a newer generation', async () => { vi.useFakeTimers() vi.setSystemTime(0) - await requestTerminalPaneRecovery({ + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled', @@ -337,7 +537,8 @@ describe('requestTerminalPaneRecovery', () => { }) ).toBe(true) resolveLiveness?.(true) - await Promise.resolve() + // Drain the resumed probe here, or its remount lands in the next test. + await vi.advanceTimersByTimeAsync(0) expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(2) }) @@ -379,7 +580,7 @@ describe('requestTerminalPaneRecovery', () => { it('keeps a sibling pane retry when the first requesting split is disposed', async () => { vi.useFakeTimers() vi.setSystemTime(0) - await requestTerminalPaneRecovery({ + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' @@ -408,7 +609,7 @@ describe('requestTerminalPaneRecovery', () => { it('does not abandon a certified sibling behind a failed liveness retry', async () => { vi.useFakeTimers() vi.setSystemTime(0) - await requestTerminalPaneRecovery({ + await requestAndSettle({ tabId: 'tab-1', ptyId: 'pty-initial', reason: 'write-stalled' @@ -447,6 +648,7 @@ describe('requestTerminalPaneRecovery', () => { }) it('budgets tabs independently', async () => { + setTerminalTabs([...terminalTabs(), { id: 'tab-2' }]) expect( await requestTerminalPaneRecovery({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' }) ).toBe(true) @@ -616,8 +818,6 @@ describe('requestTerminalPaneRecovery', () => { }) it('does not consume budget when the tab no longer exists', async () => { - mocks.remountTerminalTabForRecovery.mockReturnValue(false) - const result = await requestTerminalPaneRecovery({ tabId: 'tab-gone', ptyId: 'pty-1', @@ -670,8 +870,6 @@ describe('requestTerminalPaneRecovery', () => { }) it('does not arm when the remount never happened', async () => { - mocks.remountTerminalTabForRecovery.mockReturnValue(false) - const result = await requestTerminalPaneRecovery({ tabId: 'tab-gone', ptyId: 'pty-1', @@ -687,23 +885,28 @@ describe('requestTerminalPaneRecovery', () => { // Crash b5cfc6ca (1.4.198, Windows): 8878 'terminal_pane_recovery_remount' // breadcrumbs, every one reason='reattach-unverifiable', across 8 tabs in // 122.4s (median gap 10ms) — ~1110 per tab against a cap of 3 per 5min. The - // renderer then died allocating a 512x512 SkBitmap. Only one line outside the - // test reset clears recoveryTimestampsByTabId: the unregister() branch that - // fires when getTab() cannot see the tab. getTab reads unifiedTabsByWorktree - // while remountTerminalTabForRecovery reads and bumps tabsByWorktree, so a tab - // present in one index and absent from the other remounts and then erases the - // budget that remount just consumed. + // renderer then died allocating a 512x512 SkBitmap. + // + // The trigger was two tab indices: the budget lived in a module Map keyed by + // tabId, and the pane-disposal release erased it whenever getTab (which reads + // unifiedTabsByWorktree) could not see a tab remountTerminalTabForRecovery + // (which reads tabsByWorktree) still held. The mechanism is what mattered: + // each remount mounted a pane that captured a FRESH epoch, so the epoch check + // could never refuse its request, and a counting budget was the only thing + // between the failure and its own repetition. describe('unverifiable reattach remount storm (crash b5cfc6ca)', () => { const STORM_CYCLES = 200 const OBSERVED_MEDIAN_GAP_MS = 10 // One production reattach cycle: connect-pane-pty captures the epoch and - // registers the xterm, the reattach answers unverifiable - // (recoverUnverifiableDirectSshReattach), and the remount disposes that - // xterm — session-reconcile-dispose unregisters the instance. + // registers the xterm (connect-pane-pty.ts), the reattach answers + // unverifiable, recoverUnverifiableDirectSshReattach settles this pane's + // attempt 'failed' and re-requests, and the remount disposes that xterm — + // session-reconcile-dispose unregisters the instance. async function driveUnverifiableReattachCycle(tabId: string): Promise { const terminalRecoveryGeneration = captureTerminalPaneRecoveryGeneration(tabId) const instance = registerTerminalPaneRecoveryInstance(tabId) + settleTerminalPaneRecovery(tabId, terminalRecoveryGeneration, 'failed') await requestTerminalPaneRecovery({ tabId, ptyId: 'ssh:target@@pty-1', @@ -726,16 +929,47 @@ describe('requestTerminalPaneRecovery', () => { vi.setSystemTime(0) }) - it('caps remounts when the remounted tab is invisible to getTab', async () => { - // remountTerminalTabForRecovery still succeeds — the tab is in - // tabsByWorktree, which is what the 8878 remount breadcrumbs prove. + it('collapses the reported storm to a single remount', async () => { + // The reported run produced ~1110 remounts on this tab. One remount is + // admitted; after its pane reports the same reason failed, every later + // request is refused on evidence — not on a count, and not on a timer. + await driveStorm('tab-ssh') + + expect(mocks.remountTerminalTabForRecovery.mock.calls.length).toBe(1) + }) + + it('stops a slow failure chain the cooldown would have waved through', async () => { + // Gaps wider than the cooldown: counting would allow the cap's worth of + // remounts before noticing. Evidence stops it at the first observed + // failure — the chain never gets a second identical attempt. + for (let cycle = 0; cycle < 10; cycle += 1) { + vi.setSystemTime(cycle * 20_000) + await driveUnverifiableReattachCycle('tab-ssh') + } + + expect(mocks.remountTerminalTabForRecovery.mock.calls.length).toBe(1) + }) + + it('stays capped for a tab the unified index cannot see', async () => { + // The pre-#19745 trigger: present in tabsByWorktree, absent from + // unifiedTabsByWorktree. Nothing reads the unified index for budget or + // existence any more, so the drift has no expression at all. mocks.getTab.mockReturnValue(null) await driveStorm('tab-ssh') - // Pre-fix this ran one remount per cycle. The 15s cooldown — not the - // window cap — coalesces the whole 10ms-gap storm into the first. expect(mocks.remountTerminalTabForRecovery.mock.calls.length).toBe(1) }) + + it('still refuses when every cycle also disposes and re-registers its xterm', async () => { + // The disposal path is the one that used to release the budget. It now + // releases nothing that a remount wrote, so a 200-cycle dispose storm + // cannot restore the tab's allowance. + await driveStorm('tab-ssh') + const ledger = terminalTabs().find((tab) => tab.id === 'tab-ssh')?.recovery + + expect(ledger?.attemptedAt).toHaveLength(1) + expect(ledger?.outcome).toBe('failed') + }) }) }) diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts index 59182ca7b63..02070b3411b 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts @@ -1,7 +1,17 @@ import { useAppStore } from '@/store' import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' -import { isTerminalTabPresent } from '@/store/slices/terminal-tab-retirement' import { locateTerminalTab } from '@/store/terminals/terminal-tab-location' +import { + admitTerminalRecoveryRemount, + captureTabRecoveryGeneration +} from '@/store/terminals/terminal-tab-recovery-ledger' +import type { + TerminalRecoveryDecline, + TerminalRecoveryRemountRequest, + TerminalRecoveryRemountResult, + TerminalRecoveryTrigger +} from '@/store/terminals/terminal-tab-recovery-ledger' +import type { TerminalPaneRecoveryReason } from '../../../../shared/terminal-tab-types' import { _resetTerminalInputQuarantineForTests, armTerminalInputQuarantine @@ -17,25 +27,13 @@ import { // proven remount seam — bumping the tab's generation unmounts TerminalPane, // detach() preserves the live PTY, and the remounted pane builds a fresh // xterm that reattaches and replays the daemon snapshot. No shell restart. +// +// The budget and the epoch live on the tab row (terminal-tab-recovery-ledger), +// not in maps keyed by tabId here. Only the mounted-xterm registry below is +// still module-level: an xterm instance genuinely outlives no store row, so it +// has nothing to shadow. -export type TerminalPaneRecoveryReason = - | 'write-stalled' - | 'replay-wedged' - | 'input-undeliverable' - // The paired runtime that owns the PTY refused this write and said so on the - // wire. Distinct from 'input-undeliverable' because it skips the liveness - // probe: main's registry holds no entry for a `remote:` id, so `pty:hasPty` - // routes it to the local provider and answers a fabricated "dead". The - // rejection frame is the evidence instead — it came from the process that - // owns the PTY, over a connection that is by construction still up. - | 'input-rejected-by-host' - | 'reattach-unverifiable' - // A restore was requested for a certified-dead pipeline (reveal path). - | 'restore-blocked' - // A spawn resolved without a PTY id, so the pane is mounted with no transport - // binding. pty:data for the old id then lands in the pre-handler buffer, which - // ACKs it — main's delivery health stays green while the pane shows nothing. - | 'spawn-left-pane-unbound' +export type { TerminalPaneRecoveryReason } type RecoveryRequest = { tabId: string @@ -47,6 +45,9 @@ type RecoveryRequest = { /** Identifies the concrete mounted xterm making the request. Disposal * invalidates delayed work even when the tab's recovery epoch is unchanged. */ terminalRecoveryInstanceId?: number + /** Defaults to 'automatic'. 'user' marks the explicit Retry in the error + * toast, which is itself the new trigger a settled failure waits for. */ + trigger?: TerminalRecoveryTrigger /** Remote panes (runtime mirrors, app-SSH) must prove the PTY alive before * an input-undeliverable remount: pty:hasPty answers null for ids the local * registry doesn't own, and treating null as "proceed" would let a @@ -62,19 +63,6 @@ type RecoveryRequest = { endpointReplaced?: boolean } -// Why a cap exists: recovery must never loop. If the remounted pane wedges -// again (e.g. a deterministic parser throw in restored content), repeated -// bumps would remount-storm. The window is generous because a legitimate -// second recovery (new wedge minutes later) should still work. -const MAX_RECOVERIES_PER_WINDOW = 3 -const RECOVERY_WINDOW_MS = 5 * 60_000 -// Why a cooldown exists: one incident can trip several detectors (stall watch, -// replay guard, input path) within seconds; the first remount fixes all of -// them, the rest must coalesce instead of re-remounting mid-reattach. -const RECOVERY_COOLDOWN_MS = 15_000 - -const recoveryTimestampsByTabId = new Map() -const recoveryGenerationByTabId = new Map() const activeTerminalRecoveryInstanceIds = new Set() const pendingRetryByTabId = new Map< string, @@ -85,46 +73,40 @@ const pendingRetryByTabId = new Map< >() let nextTerminalRecoveryInstanceId = 0 -type RecoveryBudget = - | { allowed: true } - | { allowed: false; declinedBy: 'window-cap'; retryInMs: number } - | { allowed: false; declinedBy: 'cooldown'; retryInMs: number } - -function shouldScheduleRecoveryRetry(request: RecoveryRequest, budget: RecoveryBudget): boolean { - return ( - !budget.allowed && - (budget.declinedBy === 'cooldown' - ? request.terminalRecoveryGeneration !== undefined - : request.reason !== 'reattach-unverifiable') - ) +function toRemountRequest(request: RecoveryRequest, now: number): TerminalRecoveryRemountRequest { + return { + reason: request.reason, + trigger: request.trigger ?? 'automatic', + ...(request.terminalRecoveryGeneration === undefined + ? {} + : { generation: request.terminalRecoveryGeneration }), + now + } } -function recoveryBudget(tabId: string, now: number): RecoveryBudget { - const timestamps = recoveryTimestampsByTabId.get(tabId) ?? [] - const recent = timestamps.filter((t) => now - t < RECOVERY_WINDOW_MS) - if (recent.length !== timestamps.length) { - recoveryTimestampsByTabId.set(tabId, recent) +function shouldScheduleRecoveryRetry( + request: RecoveryRequest, + decline: TerminalRecoveryDecline +): decline is Extract { + if (decline.declinedBy === 'cooldown') { + return request.terminalRecoveryGeneration !== undefined } - if (recent.length >= MAX_RECOVERIES_PER_WINDOW) { - return { - allowed: false, - declinedBy: 'window-cap', - retryInMs: recent[0] + RECOVERY_WINDOW_MS - now - } + if (decline.declinedBy === 'unsettled') { + // A pane that never reports leaves 'pending' standing; re-asking once the + // settlement bound elapses is how that tab gets a second chance at all. + return request.terminalRecoveryGeneration !== undefined } - const last = recent.at(-1) - if (last !== undefined && now - last < RECOVERY_COOLDOWN_MS) { - return { - allowed: false, - declinedBy: 'cooldown', - retryInMs: last + RECOVERY_COOLDOWN_MS - now - } + if (decline.declinedBy === 'window-cap') { + return request.reason !== 'reattach-unverifiable' } - return { allowed: true } + // 'settled-failure' deliberately schedules nothing: a retry timer would be + // the counting loop again. Only a new trigger reopens that reason. + return false } export function captureTerminalPaneRecoveryGeneration(tabId: string): number { - return recoveryGenerationByTabId.get(tabId) ?? 0 + const state = useAppStore.getState() + return captureTabRecoveryGeneration(locateTerminalTab(state.tabsByWorktree, tabId)?.tab) } export function registerTerminalPaneRecoveryInstance(tabId: string): { @@ -142,15 +124,10 @@ export function registerTerminalPaneRecoveryInstance(tabId: string): { if (pendingRetry?.requestsByInstanceId.size === 0) { cancelPendingRecoveryRetry(tabId) } - // Read the SAME index remountTerminalTabForRecovery mutates. getTab answers - // from unifiedTabsByWorktree, which several slices let drift out of sync with - // tabsByWorktree; on the direct-SSH path that drift made every remount erase - // the budget it had just consumed, so the cap never held (crash b5cfc6ca). - if (!isTerminalTabPresent(useAppStore.getState(), tabId)) { - recoveryTimestampsByTabId.delete(tabId) - recoveryGenerationByTabId.delete(tabId) - cancelPendingRecoveryRetry(tabId) - } + // No budget release here, by construction: the ledger is a field on the + // tab row, so closing the tab drops it and nothing else can. Releasing it + // from a pane disposal is what erased every consumed remount and let the + // cap lapse (crash b5cfc6ca). } } } @@ -211,6 +188,21 @@ function cancelPendingRecoveryRetry(tabId: string): void { } } +function handleDeclinedRecovery(request: RecoveryRequest, decline: TerminalRecoveryDecline): false { + if (decline.declinedBy === 'window-cap') { + // The backstop firing means the outcome gate let a loop through. That is a + // bug in the gate, so leave a trace rather than only declining quietly. + recordRendererCrashBreadcrumb('terminal_pane_recovery_window_cap', { + tabId: request.tabId, + reason: request.reason + }) + } + if (shouldScheduleRecoveryRetry(request, decline)) { + scheduleRecoveryRetry(request, decline.retryInMs) + } + return false +} + /** * Remount the pane's tab to rebuild its renderer over the live PTY. Returns * true when a remount was actually requested. @@ -222,29 +214,33 @@ function cancelPendingRecoveryRetry(tabId: string): void { * either way: a remount rebuilds the renderer over the PTY it already had. */ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Promise { - if (!isCurrentTerminalRecoveryRequest(request)) { - return false - } - // A terminal-backed tab is intentionally hidden while native chat owns the - // provider. Late xterm callbacks from that hidden surface must not remount - // the tab and race the handoff's owner transition. Ask both indices: local - // toggles only patch the unified tab, but that index can transiently drop a - // row the remount index still holds (crash b5cfc6ca) and a hole there must - // not read as "not chat-owned". - const state = useAppStore.getState() if ( - state.getTab?.(request.tabId)?.viewMode === 'chat' || - locateTerminalTab(state.tabsByWorktree, request.tabId)?.tab.viewMode === 'chat' + request.terminalRecoveryInstanceId !== undefined && + !activeTerminalRecoveryInstanceIds.has(request.terminalRecoveryInstanceId) ) { return false } - const budget = recoveryBudget(request.tabId, Date.now()) - if (!budget.allowed) { - if (shouldScheduleRecoveryRetry(request, budget)) { - scheduleRecoveryRetry(request, budget.retryInMs) - } + const state = useAppStore.getState() + const tab = locateTerminalTab(state.tabsByWorktree, request.tabId)?.tab + // A terminal-backed tab is intentionally hidden while native chat owns the + // provider. Late xterm callbacks from that hidden surface must not remount + // the tab and race the handoff's owner transition. + if (tab?.viewMode === 'chat') { return false } + // Fail fast before the liveness probe. The authoritative admission runs + // again inside remountTerminalTabForRecovery's write. + const admission = admitTerminalRecoveryRemount(tab, toRemountRequest(request, Date.now())) + if (!admission.admitted) { + if (admission.declinedBy === 'stale-generation') { + return false + } + // 'tab-missing' deliberately falls through: the store call below is what + // records the remount-unavailable breadcrumb for a vanished tab. + if (admission.declinedBy !== 'tab-missing') { + return handleDeclinedRecovery(request, admission) + } + } // 'input-rejected-by-host' is deliberately absent: no local probe can speak // for the id it carries, and its evidence already came from the PTY's owner. if (request.reason === 'input-undeliverable') { @@ -267,22 +263,12 @@ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Pro // over a dead PTY degrades to the existing dead-pane rendering, not a // broken state. } - // Re-check the budget across the await: a concurrent detector may have - // already consumed it for this tab. - if (!isCurrentTerminalRecoveryRequest(request)) { - return false - } - const recheck = recoveryBudget(request.tabId, Date.now()) - if (!recheck.allowed) { - if (shouldScheduleRecoveryRetry(request, recheck)) { - scheduleRecoveryRetry(request, recheck.retryInMs) - } - return false - } } - let remounted = false + let result: TerminalRecoveryRemountResult try { - remounted = useAppStore.getState().remountTerminalTabForRecovery(request.tabId) + result = useAppStore + .getState() + .remountTerminalTabForRecovery(request.tabId, toRemountRequest(request, Date.now())) } catch { // Why: recovery fires from timer and write-callback contexts (stall watch, // replay guard, onData) — it is best-effort by contract and must never @@ -296,23 +282,21 @@ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Pro }) return false } - if (!remounted) { - // Why: this was the one silent outcome — the tab is gone from the store - // (closed/orphaned), so retrying is pointless, but the trace must show - // that a certified-dead pane asked for recovery and none happened. - recordRendererCrashBreadcrumb('terminal_pane_recovery_remount_unavailable', { - tabId: request.tabId, - reason: request.reason - }) - return false + if (!result.remounted) { + if (result.declinedBy === 'tab-missing') { + // Why: this was the one silent outcome — the tab is gone from the store + // (closed/orphaned), so retrying is pointless, but the trace must show + // that a certified-dead pane asked for recovery and none happened. + recordRendererCrashBreadcrumb('terminal_pane_recovery_remount_unavailable', { + tabId: request.tabId, + reason: request.reason + }) + return false + } + return result.declinedBy === 'stale-generation' + ? false + : handleDeclinedRecovery(request, result) } - const timestamps = recoveryTimestampsByTabId.get(request.tabId) ?? [] - timestamps.push(Date.now()) - recoveryTimestampsByTabId.set(request.tabId, timestamps) - recoveryGenerationByTabId.set( - request.tabId, - captureTerminalPaneRecoveryGeneration(request.tabId) + 1 - ) // A remount replaces every pane xterm in the tab; a previously scheduled // retry would only re-remount the fresh, healthy panes. cancelPendingRecoveryRetry(request.tabId) @@ -334,9 +318,21 @@ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Pro return true } +/** Report what this mounted pane observed for the recovery epoch it captured. + * Reuses the direct-SSH pane retry vocabulary so a pane settles both ledgers + * from the same call sites. Ignored unless the epoch is still current. */ +export function settleTerminalPaneRecovery( + tabId: string, + generation: number | undefined, + outcome: 'success' | 'failed' | 'timed-out' | 'superseded' +): void { + if (generation === undefined) { + return + } + useAppStore.getState().settleTerminalTabRecovery?.(tabId, generation, outcome) +} + export function _resetTerminalPaneRecoveryForTests(): void { - recoveryTimestampsByTabId.clear() - recoveryGenerationByTabId.clear() activeTerminalRecoveryInstanceIds.clear() nextTerminalRecoveryInstanceId = 0 for (const pendingRetry of pendingRetryByTabId.values()) { diff --git a/src/renderer/src/hooks/remote-workspace-session-merge.ts b/src/renderer/src/hooks/remote-workspace-session-merge.ts index fa247d8bbbd..f8c2c9dabf2 100644 --- a/src/renderer/src/hooks/remote-workspace-session-merge.ts +++ b/src/renderer/src/hooks/remote-workspace-session-merge.ts @@ -14,7 +14,11 @@ function preserveNewerLocalTerminalFields(remote: TerminalTab, local: TerminalTa const preserved = { ...remote, generation: local.generation, - ptyId: local.ptyId + ptyId: local.ptyId, + // Why: the recovery ledger is client-local and travels with generation — + // a remote snapshot that dropped it would hand the tab a fresh remount + // allowance on every republication, which is the storm again (b5cfc6ca). + ...(local.recovery ? { recovery: local.recovery } : {}) } return local.pendingActivationSpawn ? { ...preserved, pendingActivationSpawn: local.pendingActivationSpawn } diff --git a/src/renderer/src/lib/session-write-subscriber.test.ts b/src/renderer/src/lib/session-write-subscriber.test.ts index 80111781534..9753d8303b7 100644 --- a/src/renderer/src/lib/session-write-subscriber.test.ts +++ b/src/renderer/src/lib/session-write-subscriber.test.ts @@ -389,6 +389,43 @@ describe('createSessionWriteSubscriber', () => { cleanup() }) + it('ignores recovery-ledger-only changes', () => { + // Why: the ledger is stripped from the persisted session, so churning it + // must not rebuild and rewrite the durable payload on every remount. + const persist = vi.fn<(payload: WorkspaceSessionWrite) => void>() + const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) + + useAppStore.setState({ + workspaceSessionReady: true, + hydrationSucceeded: true, + ...makeTerminalSessionState('bash') + }) + vi.advanceTimersByTime(200) + persist.mockClear() + + useAppStore.setState({ + tabsByWorktree: { + 'wt-1': [ + { + ...useAppStore.getState().tabsByWorktree['wt-1'][0], + recovery: { + attemptedAt: [1], + generation: 1, + outcome: 'pending', + startedAt: 1, + reason: 'reattach-unverifiable', + tabGeneration: 0 + } + } + ] + } + }) + vi.advanceTimersByTime(200) + + expect(persist).not.toHaveBeenCalled() + cleanup() + }) + it('ignores decorative unified terminal label churn', () => { const persist = vi.fn<(payload: WorkspaceSessionWrite) => void>() const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) diff --git a/src/renderer/src/lib/session-write-subscriber.ts b/src/renderer/src/lib/session-write-subscriber.ts index d54675e15ab..685a8e56e3f 100644 --- a/src/renderer/src/lib/session-write-subscriber.ts +++ b/src/renderer/src/lib/session-write-subscriber.ts @@ -14,7 +14,10 @@ type UnifiedTab = UnifiedTabsByWorktree[string][number] const TERMINAL_TAB_LIVE_TITLE_KEYS = new Set(['title']) // Why: this handoff flag is stripped from workspace sessions, so toggling it // alone should not rebuild and rewrite the durable session payload. -const TERMINAL_TAB_TRANSIENT_SESSION_KEYS = new Set(['pendingActivationSpawn']) +const TERMINAL_TAB_TRANSIENT_SESSION_KEYS = new Set([ + 'pendingActivationSpawn', + 'recovery' +]) function terminalTabChangedForSession(prev: TerminalTab, next: TerminalTab): boolean { if (prev === next) { diff --git a/src/renderer/src/lib/workspace-session-patch.test.ts b/src/renderer/src/lib/workspace-session-patch.test.ts index 2f604fb67bb..d2084d6fd03 100644 --- a/src/renderer/src/lib/workspace-session-patch.test.ts +++ b/src/renderer/src/lib/workspace-session-patch.test.ts @@ -182,7 +182,15 @@ describe('buildWorkspaceSessionPatch', () => { title: 'shell', ptyId: 'pty-1', worktreeId: localWorktreeId, - pendingActivationSpawn: true + pendingActivationSpawn: true, + recovery: { + attemptedAt: [1], + generation: 1, + outcome: 'pending', + startedAt: 1, + reason: 'reattach-unverifiable', + tabGeneration: 1 + } } as never ] }, @@ -217,6 +225,9 @@ describe('buildWorkspaceSessionPatch', () => { ].sort() ) expect('pendingActivationSpawn' in patch.tabsByWorktree![localWorktreeId][0]).toBe(false) + // Why: the recovery ledger describes a mounted pane's in-flight heal; a + // persisted one would refuse the first legitimate recovery after restart. + expect('recovery' in patch.tabsByWorktree![localWorktreeId][0]).toBe(false) expect(patch.terminalLayoutsByTabId?.['tab-local'].buffersByLeafId).toBeUndefined() expect(patch.terminalLayoutsByTabId?.['tab-local'].scrollbackRefsByLeafId).toBeUndefined() }) diff --git a/src/renderer/src/lib/workspace-session.ts b/src/renderer/src/lib/workspace-session.ts index 330a150b636..affa2b36ca4 100644 --- a/src/renderer/src/lib/workspace-session.ts +++ b/src/renderer/src/lib/workspace-session.ts @@ -204,12 +204,14 @@ export function buildSanitizedTabsByWorktree( tabsByWorktree: WorkspaceSessionSnapshot['tabsByWorktree'] ): WorkspaceSessionState['tabsByWorktree'] { // Why: strip transient pendingActivationSpawn — session:set persists without Zod re-parse, so a stale flag would drop the first PTY spawn on restart. + // Same for the recovery ledger: it describes a mounted pane's in-flight heal, so a persisted one would refuse the first recovery after restart. return Object.fromEntries( Object.entries(tabsByWorktree).map(([worktreeId, tabs]) => [ worktreeId, tabs.map((tab) => { - const { pendingActivationSpawn: _unused, ...rest } = tab + const { pendingActivationSpawn: _unused, recovery: _recovery, ...rest } = tab void _unused + void _recovery return rest }) ]) diff --git a/src/renderer/src/runtime/web-session-tabs-sync/terminal-build.ts b/src/renderer/src/runtime/web-session-tabs-sync/terminal-build.ts index dfc3759009d..aacb445bbe3 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/terminal-build.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/terminal-build.ts @@ -171,6 +171,10 @@ export function buildMirroredTerminalTabs( // without this dropped the client's agent-prompt label on every snapshot. ...(existing?.generatedTitle ? { generatedTitle: existing.generatedTitle } : {}), ...(existing?.aiVaultTitle ? { aiVaultTitle: existing.aiVaultTitle } : {}), + // Why: the recovery ledger is client-local and the host carries none, so + // rebuilding without it would restore this tab's remount allowance on + // every snapshot — the counting loop recovery is meant to end (b5cfc6ca). + ...(existing?.recovery ? { recovery: existing.recovery } : {}), ...(quickCommandLabel ? { quickCommandLabel } : {}), ...(startupCwd ? { startupCwd } : {}), customTitle: existing?.customTitle ?? null, diff --git a/src/renderer/src/store/slices/tab-view-mode.test.ts b/src/renderer/src/store/slices/tab-view-mode.test.ts index aa892756b23..56b99c13ba4 100644 --- a/src/renderer/src/store/slices/tab-view-mode.test.ts +++ b/src/renderer/src/store/slices/tab-view-mode.test.ts @@ -81,4 +81,41 @@ describe('tab view mode', () => { store.getState().toggleTabViewMode('missing-tab') expect(store.getState().unifiedTabsByWorktree[WT]).toBe(before) }) + + // Why: terminal-pane recovery asks the terminal row who owns the surface. + // Host sync already writes viewMode there; only these local toggles skipped + // it, which is why the guard had to OR two indices to get a safe answer. + describe('mirrors onto the terminal row', () => { + function terminalRow(tabId: string) { + return store.getState().tabsByWorktree[WT]?.find((tab) => tab.id === tabId) + } + + beforeEach(() => { + const tabId = store.getState().createTab(WT).id + store.setState({ + unifiedTabsByWorktree: { + [WT]: [ + ...store.getState().unifiedTabsByWorktree[WT].filter((tab) => tab.id !== tabId), + makeUnifiedTab({ id: tabId, entityId: tabId, worktreeId: WT, groupId: 'g-left' }) + ] + } + } as Partial) + rowTabId = tabId + }) + + let rowTabId = '' + + it('toggleTabViewMode patches the row in the same write', () => { + store.getState().toggleTabViewMode(rowTabId) + expect(terminalRow(rowTabId)?.viewMode).toBe('chat') + + store.getState().toggleTabViewMode(rowTabId) + expect(terminalRow(rowTabId)?.viewMode).toBe('terminal') + }) + + it('setTabViewMode patches the row in the same write', () => { + store.getState().setTabViewMode(rowTabId, 'chat') + expect(terminalRow(rowTabId)?.viewMode).toBe('chat') + }) + }) }) diff --git a/src/renderer/src/store/slices/tabs/tabs-host-mirroring.ts b/src/renderer/src/store/slices/tabs/tabs-host-mirroring.ts index e87a34f81c2..66f09743e37 100644 --- a/src/renderer/src/store/slices/tabs/tabs-host-mirroring.ts +++ b/src/renderer/src/store/slices/tabs/tabs-host-mirroring.ts @@ -2,23 +2,27 @@ import type { AppState } from '../../types' import type { TerminalTab } from '../../../../../shared/terminal-tab-types' import { findTabAndWorktree } from '../tab-group-state' import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' +import { locateTerminalTab } from '../../terminals/terminal-tab-location' -export function patchTerminalTabPinned( +/** + * Mirror a host-tracked unified-tab field onto its terminal row, in whichever + * bucket actually holds the row. Reconcile derives these fields from the + * TerminalTab, so a local toggle that only patched the unified tab would be + * recomputed away by the next host snapshot — and recovery's chat-ownership + * guard reads the row, so a lagging row lets a hidden chat surface remount. + */ +export function patchTerminalTabRow( tabsByWorktree: Record, - worktreeId: string, tabId: string, - isPinned: boolean + patch: Partial> ): Partial> { - const tabs = tabsByWorktree[worktreeId] - if (!tabs?.some((tab) => tab.id === tabId)) { + const location = locateTerminalTab(tabsByWorktree, tabId) + if (!location) { return {} } - return { - tabsByWorktree: { - ...tabsByWorktree, - [worktreeId]: tabs.map((tab) => (tab.id === tabId ? { ...tab, isPinned } : tab)) - } - } + const nextTabs = tabsByWorktree[location.worktreeId].slice() + nextTabs[location.index] = { ...location.tab, ...patch } + return { tabsByWorktree: { ...tabsByWorktree, [location.worktreeId]: nextTabs } } } // Why: pin is host-authoritative for remote-server tabs, so mirror it (like setTabColor) or it's lost on reconnect/other clients. diff --git a/src/renderer/src/store/slices/tabs/tabs-label-actions.ts b/src/renderer/src/store/slices/tabs/tabs-label-actions.ts index 8acb925bfae..42b8e3d7163 100644 --- a/src/renderer/src/store/slices/tabs/tabs-label-actions.ts +++ b/src/renderer/src/store/slices/tabs/tabs-label-actions.ts @@ -6,7 +6,7 @@ import { applyTabOrderSortValues, partitionPinnedTabOrder } from './tabs-tab-ord import { mirrorTabPinnedToHost, mirrorTabViewModeToHost, - patchTerminalTabPinned + patchTerminalTabRow } from './tabs-host-mirroring' export function createTabsLabelActions( @@ -62,7 +62,13 @@ export function createTabsLabelActions( }, setTabViewMode: (tabId, mode) => { - set((state) => patchTab(state.unifiedTabsByWorktree, tabId, { viewMode: mode }) ?? {}) + set((state) => ({ + ...patchTab(state.unifiedTabsByWorktree, tabId, { viewMode: mode }), + // Why the row too: viewMode is declared on both types and host-sync + // already writes it to the row. Only these local toggles skipped it, so + // readers had to OR the two indices to find out who owns the surface. + ...patchTerminalTabRow(state.tabsByWorktree, tabId, { viewMode: mode }) + })) mirrorTabViewModeToHost(get(), tabId, mode) }, @@ -86,7 +92,10 @@ export function createTabsLabelActions( (terminal) => terminal.id === found.tab.entityId )?.launchAgent ?? null toggled = { from: fromMode, to: nextMode, agent } - return patchTab(state.unifiedTabsByWorktree, tabId, { viewMode: nextMode }) ?? {} + return { + ...patchTab(state.unifiedTabsByWorktree, tabId, { viewMode: nextMode }), + ...patchTerminalTabRow(state.tabsByWorktree, tabId, { viewMode: nextMode }) + } }) // Why: emit after the state write so the event reflects the committed mode. const committed = toggled as { @@ -141,7 +150,7 @@ export function createTabsLabelActions( [worktreeId]: applyTabOrderSortValues(tabs, tabOrder) }, // Why: reconcile derives pin from the TerminalTab, so mirror it there too or a host snapshot recomputes isPinned:false and un-pins during the echo window. - ...patchTerminalTabPinned(state.tabsByWorktree, worktreeId, tabId, true), + ...patchTerminalTabRow(state.tabsByWorktree, tabId, { isPinned: true }), groupsByWorktree: { ...state.groupsByWorktree, [worktreeId]: updateGroup(groups, { ...group, tabOrder }) @@ -178,7 +187,7 @@ export function createTabsLabelActions( ...state.unifiedTabsByWorktree, [worktreeId]: applyTabOrderSortValues(tabs, tabOrder) }, - ...patchTerminalTabPinned(state.tabsByWorktree, worktreeId, tabId, false), + ...patchTerminalTabRow(state.tabsByWorktree, tabId, { isPinned: false }), groupsByWorktree: { ...state.groupsByWorktree, [worktreeId]: updateGroup(groups, { ...group, tabOrder }) diff --git a/src/renderer/src/store/slices/terminal-tab-recovery-remount.test.ts b/src/renderer/src/store/slices/terminal-tab-recovery-remount.test.ts index d3b96433e75..ca69e4512c8 100644 --- a/src/renderer/src/store/slices/terminal-tab-recovery-remount.test.ts +++ b/src/renderer/src/store/slices/terminal-tab-recovery-remount.test.ts @@ -21,7 +21,7 @@ describe('remountTerminalTabForRecovery', () => { const remounted = store.getState().remountTerminalTabForRecovery(tabId) - expect(remounted).toBe(true) + expect(remounted.remounted).toBe(true) const after = store.getState().tabsByWorktree[WORKTREE_ID].find((tab) => tab.id === tabId) expect(after?.generation ?? 0).toBe((before?.generation ?? 0) + 1) // Recovery is not user interaction — the remount's PTY updates must not @@ -36,7 +36,7 @@ describe('remountTerminalTabForRecovery', () => { store.getState().queueTabStartupCommand(tabId, startup) const before = store.getState().pendingStartupByTabId[tabId] - expect(store.getState().remountTerminalTabForRecovery(tabId)).toBe(true) + expect(store.getState().remountTerminalTabForRecovery(tabId).remounted).toBe(true) const after = store.getState().pendingStartupByTabId[tabId] expect(after).toEqual(before) @@ -59,7 +59,10 @@ describe('remountTerminalTabForRecovery', () => { const store = createTestStore() seedWorktreeWithTab(store) - expect(store.getState().remountTerminalTabForRecovery('missing-tab')).toBe(false) + expect(store.getState().remountTerminalTabForRecovery('missing-tab')).toEqual({ + remounted: false, + declinedBy: 'tab-missing' + }) }) }) @@ -72,7 +75,7 @@ describe('isTerminalTabPresent as the recovery existence check', () => { const tabId = seedWorktreeWithTab(store) expect(isTerminalTabPresent(store.getState(), tabId)).toBe(true) - expect(store.getState().remountTerminalTabForRecovery(tabId)).toBe(true) + expect(store.getState().remountTerminalTabForRecovery(tabId).remounted).toBe(true) }) it('stays true when the tab is missing from the unified tab index', () => { @@ -90,7 +93,7 @@ describe('isTerminalTabPresent as the recovery existence check', () => { store.setState({ tabsByWorktree: { [WORKTREE_ID]: [] } }) expect(isTerminalTabPresent(store.getState(), tabId)).toBe(false) - expect(store.getState().remountTerminalTabForRecovery(tabId)).toBe(false) + expect(store.getState().remountTerminalTabForRecovery(tabId).remounted).toBe(false) }) // The budget release still has to fire for a real close, or a closed tab's diff --git a/src/renderer/src/store/slices/worktree-helpers.ts b/src/renderer/src/store/slices/worktree-helpers.ts index 3fa3219fa32..d6d9e9de74f 100644 --- a/src/renderer/src/store/slices/worktree-helpers.ts +++ b/src/renderer/src/store/slices/worktree-helpers.ts @@ -25,6 +25,11 @@ import type { import type { WorktreeRemovalTarget } from '../../../../shared/worktree/removal' import type { TerminalGitHubPRLink } from '../../../../shared/terminal-github-pr-link-detector' import type { ExecutionHostId } from '../../../../shared/execution-host' +import type { TerminalPaneRecoveryOutcome } from '../../../../shared/terminal-tab-types' +import type { + TerminalRecoveryRemountRequest, + TerminalRecoveryRemountResult +} from '../terminals/terminal-tab-recovery-ledger' import type { RemoveWorktreeOptions } from './worktree-removal-options' import type { HostQualifiedDetectedWorktreeResult, @@ -310,9 +315,24 @@ export type WorktreeSlice = { * TerminalPane unmounts, detaches (preserving a live PTY), and remounts with * a fresh xterm that reattaches and replays. Used by terminal-pane-recovery * when a pane's write pipeline is certified dead or its input is - * undeliverable while the PTY is alive. Returns false when the tab is gone. + * undeliverable while the PTY is alive. + * + * The generation bump and the tab's recovery ledger are written together, so + * the budget cannot outlive — or be released independently of — the row it + * belongs to. Omitting the request marks an external lifecycle remount: it + * skips admission and writes no ledger. */ - remountTerminalTabForRecovery: (tabId: string) => boolean + remountTerminalTabForRecovery: ( + tabId: string, + request?: TerminalRecoveryRemountRequest + ) => TerminalRecoveryRemountResult + /** Record what a mounted pane observed for its recovery attempt. Ignored + * unless `generation` is the row's current, still-pending ledger epoch. */ + settleTerminalTabRecovery: ( + tabId: string, + generation: number, + outcome: Exclude + ) => void setActiveFolderWorkspace: (folderWorkspaceId: string, executionHostId?: ExecutionHostId) => void setRenamingWorktreeId: (request: string | WorktreeRenameRequest | null) => void allWorktrees: () => Worktree[] diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 606a5a26857..330d27e7bb2 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -52,6 +52,7 @@ import { createGetKnownWorktreeById, createPurgeWorktreeTerminalState, createRemountTerminalTabForRecovery, + createSettleTerminalTabRecovery, createSetRenamingWorktreeId } from './worktrees/session/worktree-slice-lookups' import { createPurgeStaleRuntimeHostState } from './worktrees/teardown/purge-stale-runtime-host-state' @@ -108,6 +109,7 @@ export const createWorktreeSlice: StateCreator seedActiveWorktreeLastVisitedIfMissing: createSeedActiveWorktreeLastVisitedIfMissing(set, get), setRenamingWorktreeId: createSetRenamingWorktreeId(set, get), remountTerminalTabForRecovery: createRemountTerminalTabForRecovery(set, get), + settleTerminalTabRecovery: createSettleTerminalTabRecovery(set, get), setActiveWorktree: createSetActiveWorktree(set, get), setActiveFolderWorkspace: createSetActiveFolderWorkspace(set, get), allWorktrees: createAllWorktrees(set, get), diff --git a/src/renderer/src/store/slices/worktrees/session/worktree-slice-lookups.ts b/src/renderer/src/store/slices/worktrees/session/worktree-slice-lookups.ts index 055171c7cfd..78e0e397bca 100644 --- a/src/renderer/src/store/slices/worktrees/session/worktree-slice-lookups.ts +++ b/src/renderer/src/store/slices/worktrees/session/worktree-slice-lookups.ts @@ -5,6 +5,15 @@ import { getTerminalActivationSpawnSuppression } from '../../terminal-activation import { findKnownWorktreeById } from '../listing/detected-worktree-meta' import { buildWorktreePurgeState } from '../teardown/worktree-purge-state' import { locateTerminalTab } from '../../../terminals/terminal-tab-location' +import { + admitTerminalRecoveryRemount, + nextTerminalRecoveryLedger, + settledTerminalRecoveryLedger +} from '../../../terminals/terminal-tab-recovery-ledger' +import type { + TerminalRecoveryRemountRequest, + TerminalRecoveryRemountResult +} from '../../../terminals/terminal-tab-recovery-ledger' export function createSetRenamingWorktreeId( set: WorktreeSliceSet, @@ -21,26 +30,57 @@ export function createRemountTerminalTabForRecovery( set: WorktreeSliceSet, _get: WorktreeSliceGet ): WorktreeSlice['remountTerminalTabForRecovery'] { - return (tabId) => { - let remounted = false + return (tabId, request) => { + const remountRequest: TerminalRecoveryRemountRequest = request ?? { + // The lifetime bridge's host-hydration remount is an external trigger: it + // is not a heal attempt, so it neither consumes nor consults the ledger. + reason: 'reattach-unverifiable', + trigger: 'external', + now: Date.now() + } + let result: TerminalRecoveryRemountResult = { + remounted: false, + declinedBy: 'tab-missing' + } set((s) => { const location = locateTerminalTab(s.tabsByWorktree, tabId) - if (!location) { + // Why re-admit inside the write: the caller's read happened before an + // async liveness probe, and a concurrent detector may have consumed the + // budget across it. Locating the row and spending its budget is one step. + const admission = admitTerminalRecoveryRemount(location?.tab, remountRequest) + if (!location || !admission.admitted) { + if (admission.admitted) { + result = { remounted: false, declinedBy: 'tab-missing' } + } else { + const { admitted: _admitted, ...decline } = admission + result = { remounted: false, ...decline } + } return {} } const { worktreeId, index, tab } = location const nextTabs = s.tabsByWorktree[worktreeId].slice() const pendingStartup = s.pendingStartupByTabId[tabId] + // Why: bump generation to remount a pane whose renderer died while its PTY stayed alive, so it reattaches, not spawns. + const nextTabGeneration = (tab.generation ?? 0) + 1 + // An external remount is not a heal attempt, so it writes no ledger. The + // generation bump alone supersedes any ledger already on the row, which + // is exactly right: an external remount IS a new trigger. + const recovery = + remountRequest.trigger === 'external' + ? tab.recovery + : nextTerminalRecoveryLedger(tab, remountRequest, nextTabGeneration) nextTabs[index] = { ...tab, - // Why: bump generation to remount a pane whose renderer died while its PTY stayed alive, so it reattaches, not spawns. - generation: (tab.generation ?? 0) + 1, + generation: nextTabGeneration, // Why: recovery isn't a user interaction — suppress its PTY updates from reshuffling Recent, like activation remounts. pendingActivationSpawn: getTerminalActivationSpawnSuppression( s.terminalLayoutsByTabId[tab.id] - ) + ), + // The remount and the budget it spends are one write, so no disposal, + // release path or index drift can undo half of it (crash b5cfc6ca). + ...(recovery ? { recovery } : {}) } - remounted = true + result = { remounted: true, generation: recovery?.generation ?? 0 } return { tabsByWorktree: { ...s.tabsByWorktree, @@ -58,7 +98,29 @@ export function createRemountTerminalTabForRecovery( : {}) } }) - return remounted + return result + } +} + +export function createSettleTerminalTabRecovery( + set: WorktreeSliceSet, + _get: WorktreeSliceGet +): WorktreeSlice['settleTerminalTabRecovery'] { + return (tabId, generation, outcome) => { + set((s) => { + const location = locateTerminalTab(s.tabsByWorktree, tabId) + if (!location) { + return {} + } + const { worktreeId, index, tab } = location + const recovery = settledTerminalRecoveryLedger(tab, generation, outcome) + if (!recovery) { + return {} + } + const nextTabs = s.tabsByWorktree[worktreeId].slice() + nextTabs[index] = { ...tab, recovery } + return { tabsByWorktree: { ...s.tabsByWorktree, [worktreeId]: nextTabs } } + }) } } diff --git a/src/renderer/src/store/terminals/terminal-tab-recovery-ledger.ts b/src/renderer/src/store/terminals/terminal-tab-recovery-ledger.ts new file mode 100644 index 00000000000..6d09d6369c6 --- /dev/null +++ b/src/renderer/src/store/terminals/terminal-tab-recovery-ledger.ts @@ -0,0 +1,214 @@ +import { DIRECT_SSH_PANE_RETRY_SETTLEMENT_TIMEOUT_MS } from '@/components/terminal-pane/pty-connection/pty-connect-limits' +import type { + TerminalPaneRecoveryOutcome, + TerminalPaneRecoveryReason, + TerminalTab, + TerminalTabRecoveryLedger +} from '../../../../shared/terminal-tab-types' + +// Why this module exists: recovery's budget used to live in module-level Maps +// keyed by tabId. Anything keyed outside the row needs a release path, and the +// release fired on every remount-driven pane disposal — so each remount erased +// the budget it had just consumed and the cap never held (crash b5cfc6ca). +// The ledger now lives on the row, so "the budget released itself" has no +// expression: reading the budget IS reading the tab. +// +// The control is not the count. A remount that mounts a pane which fails the +// same way is not evidence that anything changed, so recovery gates on an +// OBSERVED outcome, borrowing the direct-SSH pane retry vocabulary +// (DirectSshPaneRetryResult): an attempt that has not settled blocks the next +// one, and a settled failure refuses the same reason until a new trigger. + +// Backstop only — a breadcrumb-emitting ceiling for a loop the outcome gate +// somehow failed to catch. The outcome gate is what stops a storm. +export const MAX_RECOVERIES_PER_WINDOW = 3 +export const RECOVERY_WINDOW_MS = 5 * 60_000 +// Why a cooldown exists: one incident can trip several detectors (stall watch, +// replay guard, input path) within seconds; the first remount fixes all of +// them, the rest must coalesce instead of re-remounting mid-reattach. +export const RECOVERY_COOLDOWN_MS = 15_000 +// Why reuse the direct-SSH settlement timeout: the same 31s bound already +// decides when a pane's attach attempt has stopped being in flight. A 'pending' +// ledger older than that describes a pane that never reported, not one still +// working, so it must stop blocking rather than wedge recovery forever. +export const RECOVERY_SETTLEMENT_TIMEOUT_MS = DIRECT_SSH_PANE_RETRY_SETTLEMENT_TIMEOUT_MS + +/** Why a request exists at all. Only 'automatic' is subject to the + * settled-failure refusal: a user pressing Retry, or an external lifecycle + * remount, IS the new trigger the refusal is waiting for. */ +export type TerminalRecoveryTrigger = 'automatic' | 'user' | 'external' + +export type TerminalRecoveryRemountRequest = { + reason: TerminalPaneRecoveryReason + trigger: TerminalRecoveryTrigger + /** The recovery epoch the requesting pane captured, when it has one. */ + generation?: number + now: number +} + +export type TerminalRecoveryDecline = + | { declinedBy: 'tab-missing' } + | { declinedBy: 'stale-generation' } + | { declinedBy: 'settled-failure' } + | { declinedBy: 'window-cap'; retryInMs: number } + | { declinedBy: 'unsettled'; retryInMs: number } + | { declinedBy: 'cooldown'; retryInMs: number } + +export type TerminalRecoveryAdmission = + | { admitted: true } + | ({ admitted: false } & TerminalRecoveryDecline) + +export type TerminalRecoveryRemountResult = + /** `generation` is the ledger epoch the remounted pane will capture. */ + { remounted: true; generation: number } | ({ remounted: false } & TerminalRecoveryDecline) + +const ADMITTED: TerminalRecoveryAdmission = { admitted: true } + +function recentAttempts(ledger: TerminalTabRecoveryLedger, now: number): number[] { + return ledger.attemptedAt.filter((at) => now - at < RECOVERY_WINDOW_MS) +} + +/** True once the ledger describes an attempt nothing can still settle: the row + * moved to a generation this ledger never saw (authority change, SSH pane + * retry, activation respawn, external remount). Derived, so no writer can + * forget to mark it — and none can mark it wrongly either. */ +function isSupersededLedger(tab: TerminalTab, ledger: TerminalTabRecoveryLedger): boolean { + // Strictly forward: generation only ever increments, so a row that reads + // LOWER is a host-snapshot rebuild that dropped the field, not a new trigger. + // Treating that as one would hand the tab a fresh allowance per snapshot. + return (tab.generation ?? 0) > ledger.tabGeneration +} + +export function readTerminalRecoveryOutcome( + tab: TerminalTab, + now: number +): TerminalPaneRecoveryOutcome | null { + const ledger = tab.recovery + if (!ledger) { + return null + } + if (isSupersededLedger(tab, ledger)) { + return 'superseded' + } + if (ledger.outcome === 'pending' && now - ledger.startedAt >= RECOVERY_SETTLEMENT_TIMEOUT_MS) { + return 'timed-out' + } + return ledger.outcome +} + +export function captureTabRecoveryGeneration(tab: TerminalTab | null | undefined): number { + return tab?.recovery?.generation ?? 0 +} + +/** + * The single admission decision. Runs read-only to fail a request fast, and + * again inside the store write so a probe's await cannot open a window for two + * panes to both consume the budget. + */ +export function admitTerminalRecoveryRemount( + tab: TerminalTab | null | undefined, + request: TerminalRecoveryRemountRequest +): TerminalRecoveryAdmission { + if (!tab) { + return { admitted: false, declinedBy: 'tab-missing' } + } + const ledger = tab.recovery + if ( + request.generation !== undefined && + request.generation !== captureTabRecoveryGeneration(tab) + ) { + return { admitted: false, declinedBy: 'stale-generation' } + } + if (request.trigger === 'external' || !ledger) { + return ADMITTED + } + const recent = recentAttempts(ledger, request.now) + if (recent.length >= MAX_RECOVERIES_PER_WINDOW) { + // Unconditional: the backstop must survive supersession, or anything that + // bumps tab.generation each cycle would lift the ceiling along with it. + return { + admitted: false, + declinedBy: 'window-cap', + retryInMs: recent[0] + RECOVERY_WINDOW_MS - request.now + } + } + if (request.trigger === 'user') { + // The user asking again IS the new evidence. Only the window cap — the + // backstop against a loop neither side can see — survives it. + return ADMITTED + } + const outcome = readTerminalRecoveryOutcome(tab, request.now) + if (outcome !== 'superseded') { + if (ledger.outcome === 'pending') { + if (outcome === 'pending') { + // Re-requesting under an unsettled attempt is the storm: the remounted + // pane fails the same way and asks again with a freshly captured epoch, + // so an epoch check can never refuse it. Nothing has been observed yet. + return { + admitted: false, + declinedBy: 'unsettled', + retryInMs: ledger.startedAt + RECOVERY_SETTLEMENT_TIMEOUT_MS - request.now + } + } + // Aged past the settlement bound with nobody reporting. Deliberately NOT + // read as an observed failure: a pane kind with no settle path would + // otherwise wedge its tab's recovery forever. The cooldown and the window + // cap bound it instead. + } else if ( + (ledger.outcome === 'failed' || ledger.outcome === 'timed-out') && + ledger.reason === request.reason + ) { + // A pane OBSERVED this reason fail after the last remount. Repeating it + // re-requests exactly the action that just failed with no evidence + // anything changed — wait for a real trigger (generation move, or user). + return { admitted: false, declinedBy: 'settled-failure' } + } + } + const last = recent.at(-1) + if (last !== undefined && request.now - last < RECOVERY_COOLDOWN_MS) { + return { + admitted: false, + declinedBy: 'cooldown', + retryInMs: last + RECOVERY_COOLDOWN_MS - request.now + } + } + return ADMITTED +} + +/** The ledger a remount writes, in the same object as the generation bump. */ +export function nextTerminalRecoveryLedger( + tab: TerminalTab, + request: TerminalRecoveryRemountRequest, + nextTabGeneration: number +): TerminalTabRecoveryLedger { + const previous = tab.recovery + // Carried across supersession on purpose — see the window-cap note above. + const carriedAttempts = previous ? recentAttempts(previous, request.now) : [] + return { + attemptedAt: [...carriedAttempts, request.now], + generation: captureTabRecoveryGeneration(tab) + 1, + outcome: 'pending', + startedAt: request.now, + reason: request.reason, + tabGeneration: nextTabGeneration + } +} + +/** Record what the mounted pane observed. Returns null when this settlement is + * not the current attempt's, so the caller can leave the store untouched. */ +export function settledTerminalRecoveryLedger( + tab: TerminalTab, + generation: number, + outcome: Exclude +): TerminalTabRecoveryLedger | null { + const ledger = tab.recovery + if ( + !ledger || + ledger.generation !== generation || + ledger.outcome !== 'pending' || + isSupersededLedger(tab, ledger) + ) { + return null + } + return { ...ledger, outcome } +} diff --git a/src/shared/terminal-tab-types.ts b/src/shared/terminal-tab-types.ts index 1c455333ea9..d99472e2fde 100644 --- a/src/shared/terminal-tab-types.ts +++ b/src/shared/terminal-tab-types.ts @@ -1,6 +1,58 @@ import type { AiVaultSessionTitle } from './ai-vault-session-title' import type { TuiAgent } from './tui-agent' +/** Why recovery reasons live in the shared row type: the tab row carries the + * recovery ledger, and the ledger records which reason it last acted on. */ +export type TerminalPaneRecoveryReason = + | 'write-stalled' + | 'replay-wedged' + | 'input-undeliverable' + // The paired runtime that owns the PTY refused this write and said so on the + // wire. Distinct from 'input-undeliverable' because it skips the liveness + // probe: main's registry holds no entry for a `remote:` id, so `pty:hasPty` + // routes it to the local provider and answers a fabricated "dead". The + // rejection frame is the evidence instead — it came from the process that + // owns the PTY, over a connection that is by construction still up. + | 'input-rejected-by-host' + | 'reattach-unverifiable' + // A restore was requested for a certified-dead pipeline (reveal path). + | 'restore-blocked' + // A spawn resolved without a PTY id, so the pane is mounted with no transport + // binding. pty:data for the old id then lands in the pre-handler buffer, which + // ACKs it — main's delivery health stays green while the pane shows nothing. + | 'spawn-left-pane-unbound' + +/** Same vocabulary the direct-SSH pane retry ledger settles with + * (DirectSshPaneRetryResult), so a pane reports both through one call. */ +export type TerminalPaneRecoveryOutcome = + | 'pending' + | 'success' + | 'failed' + | 'timed-out' + | 'superseded' + +/** The tab's recovery ledger. Lives on the row — not in a module- or + * store-level map keyed by tabId — so a tab's existence and its recovery + * budget are the same object: nothing can release the budget while keeping + * the row, and closing the tab drops both together (crash b5cfc6ca). */ +export type TerminalTabRecoveryLedger = { + /** Remount timestamps inside the rolling window. Backstop, not the control. */ + attemptedAt: number[] + /** Recovery epoch. A mounted pane captures it and stale requests are refused. */ + generation: number + /** What the mounted pane observed for the attempt this ledger describes. */ + outcome: TerminalPaneRecoveryOutcome + /** When that attempt was requested. Bounds how long 'pending' may block. */ + startedAt: number + /** The reason this attempt acted on. A settled failure refuses the SAME + * reason again until a new trigger arrives. */ + reason: TerminalPaneRecoveryReason + /** `tab.generation` right after the remount. Any later bump — authority + * change, SSH pane retry, activation respawn — is a new trigger, so the + * mismatch alone supersedes this ledger. No writer required. */ + tabGeneration: number +} + // ─── Terminal Tab (legacy — used by persistence and TerminalContentSlice) ─ export type TerminalTab = { id: string @@ -53,6 +105,11 @@ export type TerminalTab = { * `sortEpoch` increments. Split layouts use a numeric count because one tab * can remount several panes. Never persisted — it is a transient handoff. */ pendingActivationSpawn?: boolean | number + /** Transient recovery ledger for this tab. Never persisted — it describes a + * mounted pane's in-flight heal, and a stale one would refuse the first + * legitimate recovery after restart. Stripped exactly like + * `pendingActivationSpawn` (buildSanitizedTabsByWorktree). */ + recovery?: TerminalTabRecoveryLedger } export type TerminalPaneSplitDirection = 'vertical' | 'horizontal'