diff --git a/docs/reference/ssh-reconnect-source-recovery.md b/docs/reference/ssh-reconnect-source-recovery.md index 5dffc50e7dd..e8c12cf5189 100644 --- a/docs/reference/ssh-reconnect-source-recovery.md +++ b/docs/reference/ssh-reconnect-source-recovery.md @@ -129,6 +129,18 @@ the renderer retries. The SSH e2e lane must be green and triggering on **source** changes before any of this is attempted. It was skipping for 15 specs; four regressions reached a user during that window. +## Resolved: a disposed pane killed its successor's new shell + +A pane rebuilt during its first spawn uses the same reservation key, so main can return the +same PTY to both transports (#19386, #22578). The disposed transport must keep that shell +while its tab and layout leaf remain and its execution host's workspace is not being deleted. +A live transport refusing the id still retires it (#11003). This applies to local, WSL and SSH +IPC terminals; the remote-runtime transport has no corresponding kill. + +The remount trigger in the Scan-22 user report remains unknown. A retained shell can outlive +its tab if the tab closes before a successor binds it; keeping potentially owned work follows +the SSH execution boundary. + ## Open: the pane behind a preserved tab does not always rebind The merge now keeps a local tab the host has never been told about, so the tab and its title survive diff --git a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts index 52b983c51fd..39a3ce0c5af 100644 --- a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts +++ b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts @@ -39,7 +39,7 @@ export async function connectIpcPty( context: IpcPtyConnectContext ): Promise { const { transportOptions, handlers } = context - const { onPtySpawn } = transportOptions + const { onPtySpawn, retainDisposedSpawn } = transportOptions context.setCallbacks(options.callbacks) ensurePtyDispatcher() @@ -89,36 +89,37 @@ export async function connectIpcPty( // recorded before we asked for a PTY, so it belongs to that earlier owner, not to us. const priorIncarnationFence = currentPreHandlerPtySequence() const spawnResult = await spawnIpcPty(transportOptions, options, admittedSessionId) - const retireFreshSpawn = async (): Promise => { + const retireFreshSpawn = async (path: 'disposed' | 'refused'): Promise => { if (context.handleExplicitlyClosedConnect?.(spawnResult.id)) { return } // A newer generation may already own a recycled id; an id-only kill would retire its PTY. - if ( - !spawnResult.isReattach && - !spawnResult.coldRestore && - !context.ownsPtyId(spawnResult.id) - ) { + if (spawnResult.isReattach || spawnResult.coldRestore || context.ownsPtyId(spawnResult.id)) { + return + } + // Only disposed transports can have a successor; live refusal must still retire the PTY (#11003). + const retained = path === 'disposed' && retainDisposedSpawn?.() === true + if (!retained) { await window.api.pty.kill(spawnResult.id) } } if (context.isDestroyed()) { - await retireFreshSpawn() + await retireFreshSpawn('disposed') return } if (options.admitPtyId && !options.admitPtyId(spawnResult.id)) { - await retireFreshSpawn() + await retireFreshSpawn('refused') return context.isDestroyed() ? undefined : spawnResult } if (context.isDestroyed()) { - await retireFreshSpawn() + await retireFreshSpawn('disposed') return } if (spawnResult.isReattach && !admittedSessionId) { context.getCallbacks().onReattachDetermined?.() if (context.isDestroyed()) { - await retireFreshSpawn() + await retireFreshSpawn('disposed') return } } diff --git a/src/renderer/src/components/terminal-pane/pty-connection-fresh-spawn-guards.test.ts b/src/renderer/src/components/terminal-pane/pty-connection-fresh-spawn-guards.test.ts index d6d7246f017..17511577376 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection-fresh-spawn-guards.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection-fresh-spawn-guards.test.ts @@ -221,6 +221,44 @@ describe('connectPanePty', () => { expect(deps.onPtyErrorRef.current).not.toHaveBeenCalled() }) + // The disposed-spawn kill in ipc-pty-connect asks this callback before retiring a PTY; it must + // answer from the live store, or a remounted pane's shell dies under it. + it('lets a disposed spawn survive while the pane surface still exists in the store', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + transportFactoryQueue.push(transport) + const deps = createDeps({ tabId: 'tab-retain-disposed-spawn' }) + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [{ id: 'tab-retain-disposed-spawn', ptyId: null }] } + } + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks() + + const retain = createdTransportOptions[0]?.retainDisposedSpawn + if (typeof retain !== 'function') { + throw new Error('pane transport was built without retainDisposedSpawn') + } + expect(retain()).toBe(true) + mockStoreState = { + ...mockStoreState, + deleteStateByWorktreeId: { 'wt-1': { isDeleting: true, phase: 'deleting' } } + } + expect(retain()).toBe(false) + mockStoreState = { + ...mockStoreState, + deleteStateByWorktreeId: { 'local|wt-1': { isDeleting: true, phase: 'deleting' } } + } + expect(retain()).toBe(false) + mockStoreState = { + ...mockStoreState, + deleteStateByWorktreeId: {}, + tabsByWorktree: { 'wt-1': [] } + } + expect(retain()).toBe(false) + }) + it('fresh-spawns normally when the pane worktree is not being deleted', async () => { const { connectPanePty } = await import('./pty-connection') const transport = createMockTransport() diff --git a/src/renderer/src/components/terminal-pane/pty-connection/disposed-spawn-retention.test.ts b/src/renderer/src/components/terminal-pane/pty-connection/disposed-spawn-retention.test.ts new file mode 100644 index 00000000000..0aa1422e552 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection/disposed-spawn-retention.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, it } from 'vitest' +import { composeWorktreeHostIdentity } from '../../../../../shared/worktree/host-qualified-identity' +import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../../shared/terminal-tab-types' +import type { WorktreeDeleteState } from '@/store/slices/worktree-delete-state-types' +import { shouldRetainDisposedPaneSpawn } from './disposed-spawn-retention' + +const WT = 'wt' +const TAB = 'tab-a' +const LEAF = '11111111-1111-4111-8111-111111111111' +const OTHER_LEAF = '22222222-2222-4222-8222-222222222222' + +function tab(id: string): TerminalTab { + return { + id, + ptyId: null, + worktreeId: WT, + title: 'Terminal 1', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 0 + } +} + +function layout(leafId: string): TerminalLayoutSnapshot { + return { root: { type: 'leaf', leafId }, activeLeafId: leafId, expandedLeafId: null } +} + +function state(overrides: { + tabs?: Record + layouts?: Record + deleting?: Record +}) { + return { + tabsByWorktree: overrides.tabs ?? { [WT]: [tab(TAB)] }, + terminalLayoutsByTabId: overrides.layouts ?? {}, + deleteStateByWorktreeId: overrides.deleting ?? {} + } +} + +const deleting: WorktreeDeleteState = { + isDeleting: true, + error: null, + canForceDelete: false, + forceDeleteReason: null +} + +describe('shouldRetainDisposedPaneSpawn', () => { + it('keeps the PTY for a tab that still exists and has no layout root yet', () => { + expect(shouldRetainDisposedPaneSpawn(state({}), WT, TAB, LEAF)).toBe(true) + }) + + it('keeps the PTY when the layout still names the leaf', () => { + expect( + shouldRetainDisposedPaneSpawn(state({ layouts: { [TAB]: layout(LEAF) } }), WT, TAB, LEAF) + ).toBe(true) + }) + + it('kills the PTY when the tab is gone from every worktree', () => { + expect( + shouldRetainDisposedPaneSpawn(state({ tabs: { [WT]: [tab('other-tab')] } }), WT, TAB, LEAF) + ).toBe(false) + }) + + it('kills the PTY when the leaf was removed from the layout', () => { + expect( + shouldRetainDisposedPaneSpawn( + state({ layouts: { [TAB]: layout(OTHER_LEAF) } }), + WT, + TAB, + LEAF + ) + ).toBe(false) + }) + + it('kills the PTY when its worktree is being deleted even though the tab is still listed', () => { + expect( + shouldRetainDisposedPaneSpawn(state({ deleting: { [WT]: deleting } }), WT, TAB, LEAF) + ).toBe(false) + expect( + shouldRetainDisposedPaneSpawn(state({ deleting: { other: deleting } }), WT, TAB, LEAF) + ).toBe(true) + }) + + it.each(['local', 'ssh:target', 'runtime:paired'] as const)( + 'retires a disposed spawn when its %s workspace is being deleted', + (executionHostId) => { + const deletingState = state({ + deleting: { [composeWorktreeHostIdentity(executionHostId, WT)]: deleting } + }) + + expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, executionHostId)).toBe( + false + ) + expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, 'ssh:other-target')).toBe( + true + ) + } + ) + + it('keeps legacy deletion entries scoped to their recorded execution host', () => { + const deletingState = state({ + deleting: { [WT]: { ...deleting, executionHostId: 'ssh:target' } } + }) + + expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, 'ssh:target')).toBe(false) + expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, 'ssh:other-target')).toBe( + true + ) + expect( + shouldRetainDisposedPaneSpawn(state({ deleting: { [WT]: deleting } }), WT, TAB, LEAF, 'local') + ).toBe(false) + }) + + it('finds the tab under a worktree other than the one it was opened in', () => { + expect( + shouldRetainDisposedPaneSpawn(state({ tabs: { [WT]: [], other: [tab(TAB)] } }), WT, TAB, LEAF) + ).toBe(true) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-connection/disposed-spawn-retention.ts b/src/renderer/src/components/terminal-pane/pty-connection/disposed-spawn-retention.ts new file mode 100644 index 00000000000..98eeec0d0db --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection/disposed-spawn-retention.ts @@ -0,0 +1,33 @@ +import type { AppState } from '@/store/types' +import type { ExecutionHostId } from '../../../../../shared/execution-host' +import { composeWorktreeHostIdentity } from '../../../../../shared/worktree/host-qualified-identity' +import { collectLeafIdsInOrder } from '../terminal-layout-leaf-ids' + +// Main can give a remounted pane the same PTY, so disposal alone does not make it ownerless. +export function shouldRetainDisposedPaneSpawn( + state: Pick, + worktreeId: string, + tabId: string, + leafId: string, + executionHostId?: ExecutionHostId +): boolean { + const deleteState = + (executionHostId + ? state.deleteStateByWorktreeId?.[composeWorktreeHostIdentity(executionHostId, worktreeId)] + : undefined) ?? state.deleteStateByWorktreeId?.[worktreeId] + if ( + deleteState?.isDeleting && + (!deleteState.executionHostId || deleteState.executionHostId === executionHostId) + ) { + return false + } + const tabPresent = Object.values(state.tabsByWorktree).some((tabs) => + tabs.some((tab) => tab.id === tabId) + ) + if (!tabPresent) { + return false + } + // A new single-pane tab has no layout root until its first pane binds. + const root = state.terminalLayoutsByTabId[tabId]?.root + return !root || collectLeafIdsInOrder(root).includes(leafId) +} diff --git a/src/renderer/src/components/terminal-pane/pty-connection/pty-input-recovery.ts b/src/renderer/src/components/terminal-pane/pty-connection/pty-input-recovery.ts index cec1da62369..4533a02cd21 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/pty-input-recovery.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/pty-input-recovery.ts @@ -21,6 +21,7 @@ import { import { isRemoteRuntimePtyId } from './paired-parked-terminal-restore' import { TRANSPORT_CONNECT_SETTLE_GRACE_MS } from './pty-connect-limits' +import { shouldRetainDisposedPaneSpawn } from './disposed-spawn-retention' import type { ConnectPanePtySession } from './connect-pane-pty-session' import { resolveTerminalInlineImagesEnabled } from '../../../../../shared/terminal-inline-images-settings' @@ -112,6 +113,14 @@ export function installPtyInputRecovery(session: ConnectPanePtySession): void { onPtyExit: session.onExit, onPtySpawn: session.onPtySpawn, onPtyRebind: session.onPtyRebind, + retainDisposedSpawn: () => + shouldRetainDisposedPaneSpawn( + useAppStore.getState(), + session.deps.worktreeId, + session.deps.tabId, + session.pane.leafId, + session.executionHostId + ), ...(session.mainSideEffectAuthority ? {} : { diff --git a/src/renderer/src/components/terminal-pane/pty-transport-detach-attach-handoff.test.ts b/src/renderer/src/components/terminal-pane/pty-transport-detach-attach-handoff.test.ts index 7cac9384052..3d1cec9ea77 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-detach-attach-handoff.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-detach-attach-handoff.test.ts @@ -169,6 +169,46 @@ describe('createIpcPtyTransport', () => { expect(transport.getPtyId()).toBeNull() }) + // A pane remounted mid-spawn is handed the same PTY by main's pane-spawn reservation, so a kill + // from the disposed transport lands on the successor's shell. The surface owner decides. + it('keeps a fresh spawn that resolves after destroy when the pane surface still owns it', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + const kill = vi.mocked(window.api.pty.kill) + let resolveSpawn: (result: { id: string }) => void = () => {} + vi.mocked(window.api.pty.spawn).mockImplementation( + () => + new Promise((resolve) => { + resolveSpawn = resolve + }) + ) + const transport = createIpcPtyTransport({ retainDisposedSpawn: () => true }) + const pending = transport.connect({ url: '', callbacks: {} }) + await transport.destroy?.() + resolveSpawn({ id: 'pty-shared-with-successor' }) + + await expect(pending).resolves.toBeUndefined() + expect(kill).not.toHaveBeenCalled() + }) + + it('still kills a fresh spawn that resolves after destroy when nothing owns the surface', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + const kill = vi.mocked(window.api.pty.kill) + let resolveSpawn: (result: { id: string }) => void = () => {} + vi.mocked(window.api.pty.spawn).mockImplementation( + () => + new Promise((resolve) => { + resolveSpawn = resolve + }) + ) + const transport = createIpcPtyTransport({ retainDisposedSpawn: () => false }) + const pending = transport.connect({ url: '', callbacks: {} }) + await transport.destroy?.() + resolveSpawn({ id: 'pty-orphaned' }) + + await expect(pending).resolves.toBeUndefined() + expect(kill).toHaveBeenCalledWith('pty-orphaned') + }) + it('drops the exit observer when abandoning an obsolete reattach without killing it', async () => { const { createIpcPtyTransport } = await import('./pty-transport') const onPtyExit = vi.fn() diff --git a/src/renderer/src/components/terminal-pane/pty-transport-reattach-admission.test.ts b/src/renderer/src/components/terminal-pane/pty-transport-reattach-admission.test.ts index 879db477f40..7eeba23ec8d 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-reattach-admission.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-reattach-admission.test.ts @@ -153,7 +153,10 @@ describe('createIpcPtyTransport', () => { const onDataCallback = vi.fn() const onExitCallback = vi.fn() spawn.mockResolvedValueOnce({ id: 'pty-fresh-fallback', sessionExpired: true }) - const transport = createIpcPtyTransport({ onPtySpawn }) + // Built the way installPtyInputRecovery builds it: every real pane supplies + // retainDisposedSpawn, and for a live pane refusing its own id it answers "retain". The refusal + // must kill anyway — this transport is the pane's only one, so nothing else owns the PTY. + const transport = createIpcPtyTransport({ onPtySpawn, retainDisposedSpawn: () => true }) const result = await transport.connect({ url: '', @@ -184,7 +187,7 @@ describe('createIpcPtyTransport', () => { const retirementError = new Error('provider shutdown refused') spawn.mockResolvedValueOnce({ id: 'pty-fresh-fallback', sessionExpired: true }) kill.mockRejectedValueOnce(retirementError) - const transport = createIpcPtyTransport({ onPtySpawn }) + const transport = createIpcPtyTransport({ onPtySpawn, retainDisposedSpawn: () => true }) await expect( transport.connect({ diff --git a/src/renderer/src/components/terminal-pane/pty-transport-types.ts b/src/renderer/src/components/terminal-pane/pty-transport-types.ts index c7753abbd3e..3b835b9d0a2 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-types.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-types.ts @@ -278,6 +278,9 @@ export type IpcPtyTransportOptions = { onPtyExit?: (ptyId: string, exitCode?: number) => void onTitleChange?: (title: string, rawTitle: string) => void onPtySpawn?: (ptyId: string) => void + /** Asked when a fresh spawn resolves after this transport was destroyed: true keeps the PTY for + * the pane's successor (disposed-spawn-retention.ts); absent or false kills it. */ + retainDisposedSpawn?: () => boolean /** Rebind an existing pane after its provider replaces the PTY identity. */ onPtyRebind?: (ptyId: string, replacedPtyId: string, incarnationId?: string | null) => void onBell?: () => void diff --git a/src/renderer/src/components/terminal-pane/scan22-ssh-new-tab-disposed-spawn-kill.repro.test.ts b/src/renderer/src/components/terminal-pane/scan22-ssh-new-tab-disposed-spawn-kill.repro.test.ts new file mode 100644 index 00000000000..ed142fea393 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/scan22-ssh-new-tab-disposed-spawn-kill.repro.test.ts @@ -0,0 +1,50 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { installIpcPtyWindow, restorePtySpecWindow } from './pty-transport-test-harness' + +// Scan-22 repro: a new SSH tab whose pane remounts while its first pty:spawn is in flight. +// Main's pane-spawn reservation hands the successor the SAME PTY id; the disposed +// predecessor then killed it on resolve (tab closes on pty-exit, or goes input-dead). +describe('scan22: disposed mid-spawn SSH transport vs successor on the same PTY', () => { + const originalWindow = (globalThis as { window?: typeof window }).window + beforeEach(() => { + vi.resetModules() + installIpcPtyWindow(originalWindow, {}) + }) + afterEach(() => restorePtySpecWindow(originalWindow)) + + it('does not kill the PTY the remounted successor is bound to', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + let resolveFirst: (value: { id: string }) => void = () => {} + const spawn = vi.mocked(window.api.pty.spawn) + spawn.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = resolve + }) + ) + // Successor is handed the same id by main's (worktree, connection, paneKey) reservation. + spawn.mockResolvedValueOnce({ id: 'ssh-conn@@pty-7' }) + // As installPtyInputRecovery wires it: the tab and its leaf still exist in the store. + const paneOptions = { + connectionId: 'ssh-conn', + worktreeId: 'wt', + tabId: 'tab-2', + leafId: 'leaf-a', + retainDisposedSpawn: () => true + } + + const first = createIpcPtyTransport(paneOptions) + const firstConnect = first.connect({ url: '', callbacks: {} }) + // Pane remount (generation bump / recovery / park flip) while spawn is in flight. + first.destroy?.() + + const successor = createIpcPtyTransport(paneOptions) + await successor.connect({ url: '', callbacks: {} }) + expect(successor.getPtyId()).toBe('ssh-conn@@pty-7') + + resolveFirst({ id: 'ssh-conn@@pty-7' }) + await firstConnect + + expect(window.api.pty.kill).not.toHaveBeenCalledWith('ssh-conn@@pty-7') + }) +})