From cc96aec459747ec95de1eab696b4602b10f46adf Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 2 Jun 2026 21:43:56 -0700 Subject: [PATCH] fix: restore parked terminals from main snapshots (#4539) --- .../terminal-pane/pty-connection.test.ts | 221 ++++++++++++++++++ .../terminal-pane/pty-connection.ts | 203 +++++++++++----- .../terminal-pane/pty-dispatcher.ts | 1 + .../terminal-pane/pty-transport.test.ts | 1 + .../components/terminal-pane/pty-transport.ts | 1 + 5 files changed, 367 insertions(+), 60 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 677310ede..4a2570022 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -3691,6 +3691,227 @@ describe('connectPanePty', () => { expect(deps.syncPanePtyLayoutBinding).toHaveBeenCalledWith(1, 'wt-1@@detached-local') }) + it('prefers the bounded main snapshot when a parked local PTY remounts', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('wt-1@@detached-local') + transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) => { + if (sessionId) { + return { + id: sessionId, + isReattach: true, + snapshot: 'provider-stale-snapshot' + } + } + return null + }) + transportFactoryQueue.push(transport) + const getMainBufferSnapshot = window.api.pty.getMainBufferSnapshot as unknown as ReturnType< + typeof vi.fn + > + getMainBufferSnapshot.mockResolvedValue({ + data: 'main-headless-current\r\n', + cols: 100, + rows: 30, + seq: 42 + }) + + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { + 'wt-1': [{ id: 'tab-1', ptyId: 'wt-1@@detached-local' }] + }, + settings: { + ...mockStoreState.settings + } + } as StoreState + + const pane = createPane(1) + const manager = createManager(1) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks(30) + + expect(getMainBufferSnapshot).toHaveBeenCalledWith('wt-1@@detached-local', { + scrollbackRows: 5000 + }) + expect(pane.terminal.resize).toHaveBeenCalledWith(100, 30) + expect(pane.terminal.write).toHaveBeenCalledWith( + 'main-headless-current\r\n', + expect.any(Function) + ) + expect(pane.terminal.write).not.toHaveBeenCalledWith( + 'provider-stale-snapshot', + expect.any(Function) + ) + }) + + it('replays foreground bytes that arrive while a parked reattach snapshot is in flight', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('wt-1@@detached-local') + const capturedDataCallback: { + current: ((data: string, meta?: { seq?: number; rawLength?: number }) => void) | null + } = { current: null } + transport.connect.mockImplementation( + async ({ sessionId, callbacks }: { sessionId?: string; callbacks: ConnectCallbacks }) => { + capturedDataCallback.current = callbacks.onData ?? null + if (sessionId) { + return { + id: sessionId, + isReattach: true, + snapshot: 'provider-stale-snapshot' + } + } + return null + } + ) + transportFactoryQueue.push(transport) + const getMainBufferSnapshot = window.api.pty.getMainBufferSnapshot as unknown as ReturnType< + typeof vi.fn + > + const snapshot = createDeferred<{ data: string; cols: number; rows: number; seq: number }>() + getMainBufferSnapshot.mockReturnValue(snapshot.promise) + + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { + 'wt-1': [{ id: 'tab-1', ptyId: 'wt-1@@detached-local' }] + }, + settings: { + ...mockStoreState.settings + } + } as StoreState + + const pane = createPane(1) + const manager = createManager(1) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks(8) + expect(capturedDataCallback.current).not.toBeNull() + expect(getMainBufferSnapshot).toHaveBeenCalledTimes(1) + + const live = 'live-after-snapshot-start\r\n' + capturedDataCallback.current?.(live, { + seq: 100 + live.length, + rawLength: live.length + }) + snapshot.resolve({ + data: 'main-before-live\r\n', + cols: 120, + rows: 40, + seq: 100 + }) + await flushAsyncTicks(30) + + expect(pane.terminal.write).toHaveBeenCalledWith('main-before-live\r\n', expect.any(Function)) + expect(pane.terminal.write).toHaveBeenCalledWith(live, expect.any(Function)) + expect(pane.terminal.write).not.toHaveBeenCalledWith( + 'provider-stale-snapshot', + expect.any(Function) + ) + }) + + it('does not paint stale fallback data when disposed during parked snapshot restore', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('wt-1@@detached-local') + transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) => { + if (sessionId) { + return { + id: sessionId, + isReattach: true, + snapshot: 'provider-stale-snapshot' + } + } + return null + }) + transportFactoryQueue.push(transport) + const getMainBufferSnapshot = window.api.pty.getMainBufferSnapshot as unknown as ReturnType< + typeof vi.fn + > + const snapshot = createDeferred<{ data: string; cols: number; rows: number; seq: number }>() + getMainBufferSnapshot.mockReturnValue(snapshot.promise) + + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { + 'wt-1': [{ id: 'tab-1', ptyId: 'wt-1@@detached-local' }] + }, + settings: { + ...mockStoreState.settings + } + } as StoreState + + const pane = createPane(1) + const manager = createManager(1) + const deps = createDeps() + + const binding = connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks(8) + expect(getMainBufferSnapshot).toHaveBeenCalledTimes(1) + + binding.dispose() + snapshot.resolve({ + data: 'main-after-dispose\r\n', + cols: 120, + rows: 40, + seq: 100 + }) + await flushAsyncTicks(30) + + expect(pane.terminal.write).not.toHaveBeenCalledWith( + 'main-after-dispose\r\n', + expect.any(Function) + ) + expect(pane.terminal.write).not.toHaveBeenCalledWith( + 'provider-stale-snapshot', + expect.any(Function) + ) + expect(window.api.pty.settlePaneSerializer).not.toHaveBeenCalled() + }) + + it('keeps fresh cold-restore content when main has no parked reattach snapshot', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) => { + if (sessionId) { + return { + id: sessionId, + coldRestore: { scrollback: 'cold-restore-scrollback', cwd: '/tmp/wt-1' } + } + } + return null + }) + transportFactoryQueue.push(transport) + const getMainBufferSnapshot = window.api.pty.getMainBufferSnapshot as unknown as ReturnType< + typeof vi.fn + > + + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { + 'wt-1': [{ id: 'tab-1', ptyId: 'wt-1@@detached-local' }] + }, + settings: { + ...mockStoreState.settings + } + } as StoreState + + const pane = createPane(1) + const manager = createManager(1) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks(20) + + expect(getMainBufferSnapshot).not.toHaveBeenCalled() + expect(pane.terminal.write).toHaveBeenCalledWith( + 'cold-restore-scrollback', + expect.any(Function) + ) + expect(window.api.pty.ackColdRestore).toHaveBeenCalledWith('wt-1@@detached-local') + }) + it('attaches remote runtime PTY handles instead of creating a replacement terminal', async () => { const { connectPanePty } = await import('./pty-connection') const transport = createMockTransport() diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index acfa54bb6..76779c117 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -354,6 +354,29 @@ function isSessionOwnedByWorktree(sessionId: string, worktreeId: string): boolea return sessionId.slice(0, separatorIdx) === worktreeId } +function shouldPreferMainBufferSnapshotForReattach(args: { + ptyId: string + staleSessionId?: string | null + connectResult: PtyConnectResult | null +}): boolean { + if (isRemoteRuntimePtyId(args.ptyId)) { + return false + } + if ( + args.connectResult?.coldRestore && + !args.connectResult.snapshot && + !args.connectResult.replay && + args.connectResult.isReattach !== true + ) { + return false + } + return ( + args.connectResult?.isReattach === true || + Boolean(args.connectResult?.snapshot) || + args.ptyId === args.staleSessionId + ) +} + function shouldWritePtyOutputForeground(isPaneVisible: boolean): boolean { if (!isPaneVisible) { return false @@ -1618,6 +1641,7 @@ export function connectPanePty( let hiddenOutputRestorePendingChars = 0 let hiddenOutputRestorePendingOverflow = false let hiddenOutputRestoreFreshSnapshotNeeded = false + let mainBufferSnapshotApplyCount = 0 // Why: hidden recovery state belongs to one PTY stream. Reattach/restart // can reuse the pane object for a different session before visibility. let hiddenOutputRestorePtyId: string | null = null @@ -1932,6 +1956,7 @@ export function connectPanePty( writeReplayData('\x1b[2J\x1b[3J\x1b[H') writeReplayData(snapshot.data) writeReplayData(POST_REPLAY_LIVE_SNAPSHOT_RESET) + mainBufferSnapshotApplyCount += 1 recordTerminalOutput(pane.terminal) const currentPtyId = transport.getPtyId() if (currentPtyId && !getFitOverrideForPty(currentPtyId)) { @@ -1945,7 +1970,10 @@ export function connectPanePty( restoreScrollStateAfterSnapshotReplay(scrollState) } - function requestHiddenOutputRestoreIfNeeded(): boolean { + function requestHiddenOutputRestoreIfNeeded( + opts: { showUnavailableWarning?: boolean } = {} + ): boolean { + const showUnavailableWarning = opts.showUnavailableWarning !== false resetHiddenOutputRestoreIfPtyChanged() const ptyId = hiddenOutputRestorePtyId ?? transport.getPtyId() if (!hiddenOutputRestoreNeeded && hiddenOutputRestorePendingChunks.length === 0) { @@ -1970,7 +1998,9 @@ export function connectPanePty( if (hiddenOutputRestorePtyId === currentPtyId) { clearHiddenOutputRestoreState() } - writeRestoreUnavailableWarning() + if (showUnavailableWarning) { + writeRestoreUnavailableWarning() + } return } if (transport.getPtyId() !== currentPtyId) { @@ -2006,7 +2036,9 @@ export function connectPanePty( } if (!snapshot) { clearHiddenOutputRestoreState() - writeRestoreUnavailableWarning() + if (showUnavailableWarning) { + writeRestoreUnavailableWarning() + } return } applyMainBufferSnapshot(snapshot) @@ -2035,12 +2067,34 @@ export function connectPanePty( hiddenOutputRestoreNeeded && shouldWritePtyOutputForeground(deps.isVisibleRef.current) ) { - requestHiddenOutputRestoreIfNeeded() + requestHiddenOutputRestoreIfNeeded({ showUnavailableWarning }) } }) return true } + async function restoreMainBufferSnapshotForReattach(ptyId: string): Promise { + if (!canUseMainBufferSnapshot(ptyId)) { + return false + } + if (hiddenOutputRestorePtyId !== null && hiddenOutputRestorePtyId !== ptyId) { + clearHiddenOutputRestoreState() + } + const applyCountBefore = mainBufferSnapshotApplyCount + hiddenOutputRestorePtyId = ptyId + hiddenOutputRestoreNeeded = true + requestHiddenOutputRestoreIfNeeded({ showUnavailableWarning: false }) + const inFlight = hiddenOutputRestoreInFlight + if (inFlight) { + await inFlight + } + return ( + !disposed && + transport.getPtyId() === ptyId && + mainBufferSnapshotApplyCount > applyCountBefore + ) + } + unregisterBacklogRecovery = registerTerminalBacklogRecovery( pane.terminal, requestHiddenOutputRestoreIfNeeded @@ -2120,10 +2174,10 @@ export function connectPanePty( } } - const handleReattachResult = ( + const handleReattachResult = async ( result: PtyConnectResult | string | void, staleSessionId?: string | null - ): void => { + ): Promise => { if (disposed) { return } @@ -2171,62 +2225,85 @@ export function connectPanePty( // main-process hydration path has full status parity. registerPaneSerializerFor(ptyId) - // Strict precedence: snapshot > replay > coldRestore. Paint exactly - // one source per reattach. Painting snapshot AND replay produced the - // duplicated TUI output users saw on worktree switch (the relay replay - // buffer's tail typically overlaps with the daemon snapshot's tail, so - // both writing into xterm doubles the same lines). Snapshot wins - // because the daemon's authoritative buffer is freshest when present; + // Strict precedence: main snapshot > provider snapshot > replay > coldRestore. + // Paint exactly one source per reattach. Painting snapshot AND replay + // produced the duplicated TUI output users saw on worktree switch (the + // relay replay buffer's tail typically overlaps with the daemon snapshot's + // tail, so both writing into xterm doubles the same lines). Snapshot wins + // because the main/provider authoritative buffer is freshest when present; // replay wins over coldRestore because the relay's last 100 KB is // newer than disk-recorded scrollback. If we ever return all three, // the daemon and relay are by definition tracking the same session // and only the freshest source belongs on screen. - if (connectResult?.snapshot) { - writeReplayData('\x1b[2J\x1b[3J\x1b[H') - writeReplayData(connectResult.snapshot) - // Snapshot reattach keeps a live session, so avoid the broader mode - // reset. We only drop stale cursor/focus state that should not leak - // from replay bytes into the restored renderer terminal. - writeReplayData(POST_REPLAY_REATTACH_RESET) - if (connectResult.coldRestore) { - // Snapshot superseded the cold-restore payload — ack it so the - // daemon does not redeliver it on the next reattach. + const restoredMainBufferSnapshot = shouldPreferMainBufferSnapshotForReattach({ + ptyId, + staleSessionId, + connectResult + }) + ? await restoreMainBufferSnapshotForReattach(ptyId) + : false + const currentPtyIdAfterMainSnapshot = transport.getPtyId() + if ( + disposed || + (currentPtyIdAfterMainSnapshot !== null && currentPtyIdAfterMainSnapshot !== ptyId) + ) { + return + } + if ( + restoredMainBufferSnapshot && + connectResult?.coldRestore && + !isRemoteRuntimePtyId(ptyId) + ) { + window.api.pty.ackColdRestore(ptyId) + } + if (!restoredMainBufferSnapshot) { + if (connectResult?.snapshot) { + writeReplayData('\x1b[2J\x1b[3J\x1b[H') + writeReplayData(connectResult.snapshot) + // Snapshot reattach keeps a live session, so avoid the broader mode + // reset. We only drop stale cursor/focus state that should not leak + // from replay bytes into the restored renderer terminal. + writeReplayData(POST_REPLAY_REATTACH_RESET) + if (connectResult.coldRestore) { + // Snapshot superseded the cold-restore payload — ack it so the + // daemon does not redeliver it on the next reattach. + if (!isRemoteRuntimePtyId(ptyId)) { + window.api.pty.ackColdRestore(ptyId) + } + } + } else if (connectResult?.replay) { + // Relay replay holds the last 100 KB of raw output. The xterm may + // already hold pre-disconnect content; clear first to avoid + // duplication. The reattach reset prevents stale cursor/focus mode + // bits in the replayed data from leaking into the restored terminal. + writeReplayData('\x1b[2J\x1b[3J\x1b[H') + writeReplayData(connectResult.replay) + writeReplayData(POST_REPLAY_REATTACH_RESET) + if (connectResult.coldRestore) { + if (!isRemoteRuntimePtyId(ptyId)) { + window.api.pty.ackColdRestore(ptyId) + } + } + } else if (connectResult?.coldRestore) { + // restoreScrollbackBuffers() already wrote the saved xterm buffer + // before this rAF ran. The cold-restore scrollback overlaps with + // that content; clear first. + // replayIntoTerminal: the recorded scrollback is raw PTY output that + // may contain query sequences the previous agent CLI emitted; + // writing them through xterm.write would trigger auto-replies that + // land in the new shell's stdin. See replay-guard.ts. + writeReplayData('\x1b[2J\x1b[3J\x1b[H') + writeReplayData(connectResult.coldRestore.scrollback) + writeReplayData('\r\n\x1b[2m--- session restored ---\x1b[0m\r\n\r\n') + // Cold-restore means the daemon lost the session and spawned a + // fresh shell — no TUI is consuming the mode-setting bytes that a + // crashed TUI (e.g. Claude's \e[?1004h) left in the scrollback, so + // reset them to match the fresh shell's expectations. + writeReplayData(POST_REPLAY_MODE_RESET) if (!isRemoteRuntimePtyId(ptyId)) { window.api.pty.ackColdRestore(ptyId) } } - } else if (connectResult?.replay) { - // Relay replay holds the last 100 KB of raw output. The xterm may - // already hold pre-disconnect content; clear first to avoid - // duplication. The reattach reset prevents stale cursor/focus mode - // bits in the replayed data from leaking into the restored terminal. - writeReplayData('\x1b[2J\x1b[3J\x1b[H') - writeReplayData(connectResult.replay) - writeReplayData(POST_REPLAY_REATTACH_RESET) - if (connectResult.coldRestore) { - if (!isRemoteRuntimePtyId(ptyId)) { - window.api.pty.ackColdRestore(ptyId) - } - } - } else if (connectResult?.coldRestore) { - // restoreScrollbackBuffers() already wrote the saved xterm buffer - // before this rAF ran. The cold-restore scrollback overlaps with - // that content; clear first. - // replayIntoTerminal: the recorded scrollback is raw PTY output that - // may contain query sequences the previous agent CLI emitted; - // writing them through xterm.write would trigger auto-replies that - // land in the new shell's stdin. See replay-guard.ts. - writeReplayData('\x1b[2J\x1b[3J\x1b[H') - writeReplayData(connectResult.coldRestore.scrollback) - writeReplayData('\r\n\x1b[2m--- session restored ---\x1b[0m\r\n\r\n') - // Cold-restore means the daemon lost the session and spawned a - // fresh shell — no TUI is consuming the mode-setting bytes that a - // crashed TUI (e.g. Claude's \e[?1004h) left in the scrollback, so - // reset them to match the fresh shell's expectations. - writeReplayData(POST_REPLAY_MODE_RESET) - if (!isRemoteRuntimePtyId(ptyId)) { - window.api.pty.ackColdRestore(ptyId) - } } // Why: when a mobile-fit override is active, skip sending desktop dims // to the PTY — the PTY is already at phone dimensions and must stay there. @@ -2439,7 +2516,7 @@ export function connectPanePty( ) if (!result && expiredReattachError) { const gen = await preSignalPromise - if (typeof gen === 'number') { + if (typeof gen === 'number' && typeof window !== 'undefined') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) } if (disposed) { @@ -2450,8 +2527,11 @@ export function connectPanePty( startFreshSpawn() return } - handleReattachResult(result, pendingSessionId) + await handleReattachResult(result, pendingSessionId) const gen = await preSignalPromise + if (disposed || typeof window === 'undefined') { + return + } if (typeof gen === 'number') { if (!isRemoteRuntimePtyId(pendingSessionId)) { void window.api.pty.settlePaneSerializer(cacheKey, gen).catch(() => {}) @@ -2460,7 +2540,7 @@ export function connectPanePty( }) .catch(async (err) => { const gen = await preSignalPromise - if (typeof gen === 'number') { + if (typeof gen === 'number' && typeof window !== 'undefined') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) } console.warn(`[pty-connection] Reattach FAILED for tab=${deps.tabId}:`, err) @@ -2565,7 +2645,7 @@ export function connectPanePty( .then(async (result) => { if (!result && expiredReattachError) { const gen = await preSignalPromise - if (typeof gen === 'number') { + if (typeof gen === 'number' && typeof window !== 'undefined') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) } if (disposed) { @@ -2576,8 +2656,11 @@ export function connectPanePty( startFreshSpawn() return } - handleReattachResult(result, deferredReattachSessionId) + await handleReattachResult(result, deferredReattachSessionId) const gen = await preSignalPromise + if (disposed || typeof window === 'undefined') { + return + } if (typeof gen === 'number') { if (!isRemoteRuntimePtyId(deferredReattachSessionId)) { void window.api.pty.settlePaneSerializer(cacheKey, gen).catch(() => {}) @@ -2586,7 +2669,7 @@ export function connectPanePty( }) .catch(async (err) => { const gen = await preSignalPromise - if (typeof gen === 'number') { + if (typeof gen === 'number' && typeof window !== 'undefined') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) } const message = err instanceof Error ? err.message : String(err) diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts index 03fe661f9..84045bc68 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts @@ -265,6 +265,7 @@ export type PtyConnectResult = { snapshotCols?: number snapshotRows?: number isAlternateScreen?: boolean + isReattach?: boolean sessionExpired?: boolean coldRestore?: { scrollback: string; cwd: string } replay?: string diff --git a/src/renderer/src/components/terminal-pane/pty-transport.test.ts b/src/renderer/src/components/terminal-pane/pty-transport.test.ts index 5d78a73d4..7e439abce 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -651,6 +651,7 @@ describe('createIpcPtyTransport', () => { snapshotCols: 132, snapshotRows: 43, isAlternateScreen: undefined, + isReattach: true, coldRestore: undefined, replay: undefined, sessionExpired: undefined diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index ed840e968..5aa876ce1 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -577,6 +577,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra snapshotCols: spawnResult.snapshotCols, snapshotRows: spawnResult.snapshotRows, isAlternateScreen: spawnResult.isAlternateScreen, + isReattach: spawnResult.isReattach, sessionExpired: spawnResult.sessionExpired, coldRestore: spawnResult.coldRestore, replay: spawnResult.replay