From a87a19c9969fb3145c8d271e34b3403bf59bb120 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 7 Sep 2026 09:41:29 -0700 Subject: [PATCH] fix(terminal): remount a pane left unbound by a spawn that returned no PTY id (#19223) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): remount a pane left unbound by a spawn that returned no PTY id A restored-PTY reattach that resolves without a PTY id leaves the pane mounted with no transport binding, so registerData never runs. Main keeps pushing pty:data for the id; the dispatcher finds no handler and parks the bytes in the pre-handler buffer, which claims no delivery credit and so ACKs them anyway — main's flow control reads healthy while the pane shows its last frame forever. The visibility reconciler skips unbound panes, so nothing rebinds one until the user remounts the tab. Every startFreshColdRestoreAgentResume call site is floating (no await, no catch) and startFreshSpawn resolves null rather than rejecting, so no caller could see the failure. Settle it at the completion hook they all funnel through, whose guards already mean "no pty, pane alive, still unbound, nobody else spawning" — it settled the direct-SSH lease there and did nothing for local panes. Route those to the existing remount seam instead; the SSH retry ledger keeps ownership so the two never race. Observed in the field on three concurrent panes, each parked just past the 64KB pre-handler warn threshold. * test: drop a mock-only assertion that failed typecheck --------- Co-authored-by: Merge Sim --- ...connection-spawn-left-pane-unbound.test.ts | 226 ++++++++++++++++++ .../pty-connection/fresh-spawn-start.ts | 3 +- .../unbound-pane-spawn-recovery.test.ts | 89 +++++++ .../unbound-pane-spawn-recovery.ts | 40 ++++ .../terminal-pane/terminal-pane-recovery.ts | 4 + 5 files changed, 361 insertions(+), 1 deletion(-) create mode 100644 src/renderer/src/components/terminal-pane/pty-connection-spawn-left-pane-unbound.test.ts create mode 100644 src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts create mode 100644 src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts diff --git a/src/renderer/src/components/terminal-pane/pty-connection-spawn-left-pane-unbound.test.ts b/src/renderer/src/components/terminal-pane/pty-connection-spawn-left-pane-unbound.test.ts new file mode 100644 index 00000000000..63778d251c0 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection-spawn-left-pane-unbound.test.ts @@ -0,0 +1,226 @@ +import type * as React from 'react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { flushAsyncTicks } from './pty-connection-test-async' +import { + createMockTransport, + createPane, + createManager, + type MockTransport +} from './pty-connection-test-pane-fixtures' +import { buildPaneConnectionDeps } from './pty-connection-test-deps' +import { createInitialStoreState } from './pty-connection-test-store-fixtures' +import type { StoreState } from './pty-connection-test-store-state' +import { + installTerminalTestGlobals, + restoreTerminalTestGlobals +} from './pty-connection-test-environment' + +const { + resetAndRefreshAllTerminalWebglAtlases, + scheduleTerminalWebglAtlasRecovery, + scheduleRuntimeGraphSync, + shouldSeedCacheTimerOnInitialTitle, + toastInfo, + notifyCodexPaneBoundForStaleSweep, + requestTerminalPaneRecovery +} = vi.hoisted(() => ({ + resetAndRefreshAllTerminalWebglAtlases: vi.fn(), + scheduleTerminalWebglAtlasRecovery: vi.fn(), + scheduleRuntimeGraphSync: vi.fn(), + shouldSeedCacheTimerOnInitialTitle: vi.fn(() => false), + toastInfo: vi.fn(), + notifyCodexPaneBoundForStaleSweep: vi.fn(), + requestTerminalPaneRecovery: vi.fn(async () => true) +})) + +let mockStoreState: StoreState +let transportFactoryQueue: MockTransport[] = [] +let createdTransportOptions: Record[] = [] +let storeSubscribers: ((state: StoreState) => void)[] = [] + +vi.mock('@/runtime/sync-runtime-graph', () => ({ scheduleRuntimeGraphSync })) + +vi.mock('@/lib/pane-manager/pane-manager-registry', async (importOriginal) => ({ + ...(await importOriginal>()), + resetAndRefreshAllTerminalWebglAtlases +})) + +vi.mock('./terminal-webgl-atlas-recovery', () => ({ + scheduleTerminalWebglAtlasRecovery +})) + +// Only the request is spied: connect still needs the real generation/instance registry. +vi.mock('./terminal-pane-recovery', async (importOriginal) => ({ + ...(await importOriginal>()), + requestTerminalPaneRecovery +})) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => mockStoreState, + subscribe: (listener: (state: StoreState) => void) => { + storeSubscribers.push(listener) + return () => { + storeSubscribers = storeSubscribers.filter((candidate) => candidate !== listener) + } + } + } +})) + +vi.mock('@/lib/agent-status', async (importOriginal) => { + const { buildAgentStatusModuleMock } = await import('./pty-connection-test-environment') + return buildAgentStatusModuleMock(await importOriginal>()) +}) + +vi.mock('./cache-timer-seeding', () => ({ + shouldSeedCacheTimerOnInitialTitle +})) + +vi.mock('sonner', () => ({ toast: { info: toastInfo } })) + +vi.mock('@/lib/codex-stale-pane-sweep', () => ({ + notifyCodexPaneBoundForStaleSweep +})) + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useCallback: unknown>(fn: T): T => fn + } +}) + +vi.mock('./pty-transport', () => ({ + createIpcPtyTransport: vi.fn((options: Record) => { + createdTransportOptions.push(options) + const nextTransport = transportFactoryQueue.shift() + if (!nextTransport) { + throw new Error('No mock transport queued') + } + return nextTransport + }) +})) + +vi.mock('./remote-runtime-pty-transport', () => ({ + createRemoteRuntimePtyTransport: vi.fn( + (_environmentId: string, options: Record) => { + createdTransportOptions.push(options) + const nextTransport = transportFactoryQueue.shift() + if (!nextTransport) { + throw new Error('No mock transport queued') + } + return nextTransport + } + ) +})) + +vi.mock('./pty-dispatcher', async (importOriginal) => { + const actual = await importOriginal>() + return { ...actual, getEagerPtyBufferHandle: vi.fn(() => undefined) } +}) + +describe('fresh spawn leaves a local pane unbound', () => { + beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + transportFactoryQueue = [] + createdTransportOptions = [] + storeSubscribers = [] + mockStoreState = createInitialStoreState(() => mockStoreState) + installTerminalTestGlobals() + }) + + afterEach(async () => { + await restoreTerminalTestGlobals() + }) + + function createDeps(overrides: Record = {}) { + return buildPaneConnectionDeps(() => mockStoreState, overrides) + } + + it('remounts the pane when the spawn resolves without a PTY id', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + // A spawn that produced nothing: no id returned and nothing bound after it. + transport.connect.mockImplementation(async () => null) + transportFactoryQueue.push(transport) + + connectPanePty( + createPane(1) as never, + createManager(1) as never, + createDeps({ tabId: 'tab-unbound-spawn' }) as never + ) + await flushAsyncTicks(40) + + expect(transport.connect).toHaveBeenCalled() + expect(requestTerminalPaneRecovery).toHaveBeenCalledWith( + expect.objectContaining({ + tabId: 'tab-unbound-spawn', + ptyId: null, + reason: 'spawn-left-pane-unbound' + }) + ) + }) + + // The direct-SSH ledger runs its own retry; a second remount would race it. + it('leaves recovery to the direct SSH retry ledger', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + transport.connect.mockResolvedValueOnce(null) + transportFactoryQueue.push(transport) + const settleDirectSshPaneRetry = vi.fn() + const pendingRetry = { + attemptId: 'attempt-1', + authority: { targetId: 'target-a', providerEpoch: 'epoch-1', connectionGeneration: 3 }, + tabGeneration: 7, + startedAt: 1 + } + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: null, generation: 7 }] }, + ptyIdsByTabId: { 'tab-1': [] }, + repos: [{ id: 'repo1', connectionId: 'target-a', displayName: 'orca' }], + sshConnectionStates: new Map([ + [ + 'target-a', + { + targetId: 'target-a', + status: 'connected', + providerEpoch: 'epoch-1', + connectionGeneration: 3 + } + ] + ]), + directSshPaneRetryByTabId: { 'tab-1': pendingRetry }, + settleDirectSshPaneRetry + } as StoreState + + connectPanePty(createPane(1) as never, createManager(1) as never, createDeps() as never) + await flushAsyncTicks(40) + + // The lease settled, so the unbound branch ran and deliberately skipped recovery. + expect(settleDirectSshPaneRetry).toHaveBeenCalledWith( + expect.objectContaining({ status: 'failed', attemptId: 'attempt-1' }) + ) + expect(requestTerminalPaneRecovery).not.toHaveBeenCalledWith( + expect.objectContaining({ reason: 'spawn-left-pane-unbound' }) + ) + }) + + it('does not remount when the spawn bound a PTY', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('pty-bound') + transportFactoryQueue.push(transport) + + connectPanePty( + createPane(1) as never, + createManager(1) as never, + createDeps({ tabId: 'tab-bound-spawn' }) as never + ) + await flushAsyncTicks(40) + + expect(requestTerminalPaneRecovery).not.toHaveBeenCalledWith( + expect.objectContaining({ reason: 'spawn-left-pane-unbound' }) + ) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts b/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts index aefc798bb87..f3dab2d5425 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts @@ -2,6 +2,7 @@ import { useAppStore } from '@/store' import { hasPtySerializer } from '../pty-buffer-serializer' import { writeTerminalOutput } from '@/lib/pane-manager/pane-terminal-output-scheduler' +import { settleSpawnThatLeftPaneUnbound } from './unbound-pane-spawn-recovery' import { STARTUP_CWD_FALLBACK_NOTICE } from './startup-cwd-fallback-notice' import { pendingSpawnByPaneKey, pendingSpawnGenerationByPaneKey } from './pty-connect-limits' import { shouldWritePtyOutputForeground } from './foreground-output-scan' @@ -331,7 +332,7 @@ export function bindStartFreshSpawn(session: ConnectPanePtySession): void { ) { return } - session.settleDirectSshPaneRetryAttempt(session.directSshRetryAttempt, 'failed') + settleSpawnThatLeftPaneUnbound(session) }) }) // Why: split panes in the same tab can spawn concurrently. Key by pane 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 new file mode 100644 index 00000000000..25bcd4fe1ed --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts @@ -0,0 +1,89 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { requestTerminalPaneRecovery } from '../terminal-pane-recovery' +import { settleSpawnThatLeftPaneUnbound } from './unbound-pane-spawn-recovery' + +vi.mock('../terminal-pane-recovery', () => ({ + requestTerminalPaneRecovery: vi.fn() +})) + +function buildSession(overrides: Record = {}): never { + return { + deps: { tabId: 'tab-1', worktreeId: 'wt-1', restoredLeafId: 'leaf-1' }, + pane: { id: 4, leafId: 'pane-leaf' }, + terminalRecoveryGeneration: 2, + terminalRecoveryInstance: { id: 3 }, + directSshRetryAttempt: undefined, + settleDirectSshPaneRetryAttempt: vi.fn(), + ...overrides + } as never +} + +describe('settleSpawnThatLeftPaneUnbound', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('remounts the tab so the pane rebinds over its live PTY', () => { + settleSpawnThatLeftPaneUnbound(buildSession()) + + expect(requestTerminalPaneRecovery).toHaveBeenCalledExactlyOnceWith({ + tabId: 'tab-1', + ptyId: null, + reason: 'spawn-left-pane-unbound', + terminalRecoveryGeneration: 2, + terminalRecoveryInstanceId: 3 + }) + }) + + it('leaves recovery to the direct SSH retry ledger when it holds a lease', () => { + const attempt = { attemptId: 'attempt-1' } + const settleDirectSshPaneRetryAttempt = vi.fn() + + settleSpawnThatLeftPaneUnbound( + buildSession({ directSshRetryAttempt: attempt, settleDirectSshPaneRetryAttempt }) + ) + + expect(settleDirectSshPaneRetryAttempt).toHaveBeenCalledExactlyOnceWith(attempt, 'failed') + expect(requestTerminalPaneRecovery).not.toHaveBeenCalled() + }) + + it('settles the spawn as failed before remounting', () => { + const settleDirectSshPaneRetryAttempt = vi.fn() + + settleSpawnThatLeftPaneUnbound( + buildSession({ deps: { tabId: 'tab-settle' }, settleDirectSshPaneRetryAttempt }) + ) + + expect(settleDirectSshPaneRetryAttempt).toHaveBeenCalledExactlyOnceWith(undefined, 'failed') + expect(requestTerminalPaneRecovery).toHaveBeenCalledOnce() + }) + + // Distinct ids per case: warnTerminalLifecycleAnomaly dedups on a module-global key. + it('prefers the restored leaf id when reporting the anomaly', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + settleSpawnThatLeftPaneUnbound( + buildSession({ deps: { tabId: 'tab-warn', worktreeId: 'wt-1', restoredLeafId: 'leaf-1' } }) + ) + + expect(warn).toHaveBeenCalledWith( + '[terminal-lifecycle] fresh spawn left the pane unbound', + expect.objectContaining({ leafId: 'leaf-1', paneId: 4, worktreeId: 'wt-1' }) + ) + warn.mockRestore() + }) + + it('falls back to the pane leaf id when no restored leaf exists', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + settleSpawnThatLeftPaneUnbound( + buildSession({ deps: { tabId: 'tab-2', worktreeId: 'wt-2', restoredLeafId: null } }) + ) + + expect(warn).toHaveBeenCalledWith( + '[terminal-lifecycle] fresh spawn left the pane unbound', + expect.objectContaining({ leafId: 'pane-leaf' }) + ) + warn.mockRestore() + }) +}) 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 new file mode 100644 index 00000000000..698df12651c --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts @@ -0,0 +1,40 @@ +import { warnTerminalLifecycleAnomaly } from '../terminal-lifecycle-diagnostics' +import { requestTerminalPaneRecovery } from '../terminal-pane-recovery' +import type { ConnectPanePtySession } from './connect-pane-pty-session' + +/** Settle a spawn that resolved without a PTY id, remounting the pane when + * nothing else owns its recovery. + * + * Why this is not self-correcting: the pane stays mounted with no transport + * binding, so `registerData` never runs. Main keeps pushing pty:data for the + * old id, the dispatcher finds no handler and buffers it in the pre-handler + * buffer — which claims no delivery credit, so the bytes are ACKed anyway and + * main's flow control reads healthy while the pane displays its last frame + * forever. The visibility reconciler skips unbound panes, so nothing else + * rebinds one. A remount reattaches over the still-live PTY and drains the + * buffer. + * + * A direct-SSH lease runs its own retry ledger, so it keeps ownership here and + * a second remount never races it. */ +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') + if (directSshRetryOwnsRecovery) { + return + } + warnTerminalLifecycleAnomaly('fresh spawn left the pane unbound', { + tabId: session.deps.tabId, + worktreeId: session.deps.worktreeId, + leafId: session.deps.restoredLeafId ?? session.pane.leafId, + paneId: session.pane.id, + ptyId: null + }) + void requestTerminalPaneRecovery({ + tabId: session.deps.tabId, + ptyId: null, + reason: 'spawn-left-pane-unbound', + terminalRecoveryGeneration: session.terminalRecoveryGeneration, + terminalRecoveryInstanceId: session.terminalRecoveryInstance.id + }) +} 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 9e2aa0f0d99..ff1f9843ede 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts @@ -30,6 +30,10 @@ export type TerminalPaneRecoveryReason = | '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' type RecoveryRequest = { tabId: string