From d85b2667d6d7621ef68b073ee4b668b6966fb582 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Wed, 12 Aug 2026 16:15:43 -0400 Subject: [PATCH] fix(pty): retry rejected SSH cleanup safely --- config/reliability-gates.jsonc | 8 + src/main/ipc/pty.test.ts | 32 +++- src/main/ipc/pty.ts | 70 +++++++- src/main/persistence.test.ts | 77 ++++++++ src/main/persistence.ts | 63 +++++++ ...elay-session-reconnect-incarnation.test.ts | 169 ++++++++++++++++++ .../ssh/ssh-relay-session-test-fixtures.ts | 1 + src/main/ssh/ssh-relay-session.test.ts | 40 ----- src/main/ssh/ssh-relay-session.ts | 99 +++++++++- src/shared/ssh-types.ts | 4 + 10 files changed, 504 insertions(+), 59 deletions(-) diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index fb2328d43c2..f7e7797ba72 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -6638,6 +6638,7 @@ "oracle": "Deterministic tests cover bounded stale-handle replacement, suspended heartbeat clocks, cold and established subscription failure, ten partition/recovery cycles, automatic-recovery cutoff, manual reconnect, and exact direct SSH binding recovery. They assert one atomic store publication clears only exact-target PTY indexes, null-PTY activation remains unchanged, relay identity survives, Git and folder panes retry symmetrically, another target/local/WSL/runtime panes remain byte-identical through target snapshot hydration and reconnect, only an accepted exact failure or timeout starts the second attempt, two 31-second timeouts cannot start a third settlement-triggered attempt, rejected stale/mismatched acknowledgements preserve every store map, and concurrent split-pane spawn and reattach callbacks both commit under the same attempt ID after the first success replaces pending state with live shared authority. A sibling failure revokes that shared authority and starts exactly one second attempt; duplicate failures and late first-attempt PTY callbacks preserve the second attempt and every state map. Attempt-two failure retains continuation authority for later siblings, primary exit promotes a bound survivor or preserves the lease until a late sibling binds, and primary plus non-primary detach retain exact authority and history on both resulting tabs. Both remount callbacks consume split-count activity suppression, intentional dispose emits no failure/timeout, and a same-attempt StrictMode remount still owns one timeout. Hydration clears an untrusted PTY hint without clearing its current pending owner, healthy current-authority bindings suppress correction, hydration finalizes once, and reconnect emits no paired close lifecycle. A provider-level session-ID reattach returns an incarnation, then a legacy exit without an incarnation must resolve to that returned identity rather than minting a fallback identity. The shared-control oracle withholds the first encrypted ready frame, starts one RPC, triggers the successful-probe refresh, then requires exactly two client connections, one host request, a successful response, zero pending calls, and zero retained request bytes. Tests also assert one unsubscribe per remote-runtime epoch, observable recovery phases, stable PTY identity, resumed snapshot/output/input, no healthy state before replay, no retry or input after cutoff, a new manual epoch against the same PTY, quiet recovery UI with an explicit Reconnect action, pane-close state cleanup, one stable create mutation id, old-runtime no-retry behavior, cross-process PTY adoption, and bounded in-flight coordination.", "commands": [ "pnpm exec vitest run --config config/vitest.config.ts src/main/ipc/pty.test.ts src/main/ssh/ssh-relay-session.test.ts", + "pnpm exec vitest run --config config/vitest.config.ts src/main/ipc/pty.test.ts src/main/persistence.test.ts src/main/ssh/ssh-relay-session.test.ts", "pnpm exec vitest run --config config/vitest.config.ts src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts --reporter=dot", "pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/startup/ssh-startup-reconnect.test.ts src/renderer/src/lib/resolved-worktree-execution-host.test.ts src/renderer/src/components/terminal/background-terminal-worktree-mount.test.ts src/renderer/src/runtime/sync-runtime-graph-scheduling.test.ts src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts src/renderer/src/components/terminal-pane/pty-connection.test.ts src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts src/renderer/src/runtime/remote-runtime-session-tabs-inflight.test.ts src/renderer/src/runtime/web-session-terminal-handle-events.test.ts src/renderer/src/store/slices/terminal-pty-identity-replacement.test.ts", "pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts", @@ -6653,6 +6654,7 @@ ], "testFiles": [ "src/main/ipc/pty.test.ts", + "src/main/persistence.test.ts", "src/main/ssh/ssh-relay-session.test.ts", "src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts", "src/renderer/src/startup/ssh-startup-reconnect.test.ts", @@ -6717,6 +6719,12 @@ "reconnect may reclaim a cleanup-only PTY lease but never registers or persists it as a rejected terminal surface" ] }, + { + "file": "src/main/persistence.test.ts", + "assertions": [ + "a cleanup-purpose lease replaces stale surface and cleanup identity when a relay PTY id is reused" + ] + }, { "file": "src/renderer/src/startup/ssh-startup-reconnect.test.ts", "assertions": [ diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 30fbcc57bad..11378020b74 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -8706,10 +8706,10 @@ describe('registerPtyHandlers', () => { } liveRemotePtys.delete(id) }) - registerSshPtyProvider(connectionId, { + const provider = { spawn: vi.fn(async () => { liveRemotePtys.add(appPtyId) - return { id: appPtyId } + return { id: appPtyId, incarnationId: 'rejected-split-incarnation' } }), write: vi.fn(), resize: vi.fn(), @@ -8731,11 +8731,20 @@ describe('registerPtyHandlers', () => { revive: vi.fn(), getDefaultShell: vi.fn(), getProfiles: vi.fn() - } as never) + } + registerSshPtyProvider(connectionId, provider as never) + let cleanupLease: Record | undefined const store = { persistPtyBinding: vi.fn(() => false), - upsertSshRemotePtyLease: vi.fn(), - markSshRemotePtyLease: vi.fn() + upsertSshRemotePtyLease: vi.fn((lease) => { + cleanupLease = { ...lease } + }), + getSshRemotePtyLeases: vi.fn(() => (cleanupLease ? [cleanupLease] : [])), + markSshRemotePtyLease: vi.fn((_targetId, _ptyId, state) => { + if (cleanupLease) { + cleanupLease.state = state + } + }) } let controller: RuntimeSpawnController | null = null const runtime = { @@ -8790,6 +8799,10 @@ describe('registerPtyHandlers', () => { targetId: connectionId, ptyId: 'rejected-split-pty', worktreeId: 'repo-1::/remote/repo', + cleanupPending: true, + cleanupExpectedPaneKey: `tab-rejected-split:${leafId}`, + cleanupExpectedTabId: 'tab-rejected-split', + cleanupExpectedIncarnationId: 'rejected-split-incarnation', state: 'attached' }) ) @@ -8800,6 +8813,15 @@ describe('registerPtyHandlers', () => { expect.objectContaining({ message: 'injected remote shutdown failure' }) ) + unregisterSshPtyProvider(connectionId) + await expect(handlers.get('pty:kill')!(null, { id: appPtyId })).rejects.toThrow( + 'requires reconnect' + ) + expect(liveRemotePtys.has(appPtyId)).toBe(true) + expect(getPtyIdsForConnection(connectionId)).toEqual([appPtyId]) + expect(store.markSshRemotePtyLease).not.toHaveBeenCalled() + + registerSshPtyProvider(connectionId, provider as never) shutdownFails = false await handlers.get('pty:kill')!(null, { id: appPtyId }) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 0dbfb5e06fa..4f6181dedd2 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -1127,6 +1127,21 @@ async function shutdownFreshPtyAfterBindingFailure( finishPtyShutdown(id, connectionId, store) } +function isPendingSshPtyCleanup( + id: string, + connectionId: string, + store: Store | undefined +): boolean { + const relayPtyId = getRelayPtyId(connectionId, id) + const leases = store?.getSshRemotePtyLeases?.(connectionId) + return Boolean( + leases?.some( + (lease) => + lease.ptyId === relayPtyId && lease.cleanupPending === true && lease.state !== 'terminated' + ) + ) +} + // ─── Host PTY env assembly ────────────────────────────────────────── // Why: centralize host-local env injections so both spawn paths (local + daemon) get them; implemented twice they drifted, silently breaking daemon PTYs. @@ -5185,7 +5200,7 @@ export function registerPtyHandlers( markNativeWindowsConptyPty(result.id) } const relayResultId = getRelayPtyId(args.connectionId, result.id) - const persistSshLease = (includeSurfaceIdentity = true): void => { + const persistSshLease = (cleanupPending = false): void => { if (!store || !args.connectionId) { return } @@ -5194,14 +5209,24 @@ export function registerPtyHandlers( targetId: args.connectionId, ptyId: relayResultId, ...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}), - ...(includeSurfaceIdentity && typeof args.tabId === 'string' - ? { tabId: args.tabId } - : {}), - ...(includeSurfaceIdentity && - typeof args.leafId === 'string' && - isTerminalLeafId(args.leafId) + ...(!cleanupPending && typeof args.tabId === 'string' ? { tabId: args.tabId } : {}), + ...(!cleanupPending && typeof args.leafId === 'string' && isTerminalLeafId(args.leafId) ? { leafId: args.leafId } : {}), + cleanupPending, + ...(cleanupPending && + typeof args.tabId === 'string' && + typeof args.leafId === 'string' && + isValidTerminalTabId(args.tabId) && + isTerminalLeafId(args.leafId) + ? { cleanupExpectedPaneKey: makePaneKey(args.tabId, args.leafId) } + : {}), + ...(cleanupPending && typeof args.tabId === 'string' + ? { cleanupExpectedTabId: args.tabId } + : {}), + ...(cleanupPending && result.incarnationId + ? { cleanupExpectedIncarnationId: result.incarnationId } + : {}), state: 'attached', lastAttachedAt: Date.now() }) @@ -5249,7 +5274,7 @@ export function registerPtyHandlers( console.error('[pty] failed to persist runtime PTY binding after spawn:', err) if (!result.isReattach) { // Why: retain durable cleanup authority without reconnect resurrecting the rejected pane. - persistSshLease(false) + persistSshLease(true) await shutdownFreshPtyAfterBindingFailure( provider, result.id, @@ -5477,6 +5502,9 @@ export function registerPtyHandlers( provider = connectionId ? getProvider(connectionId) : getProviderForPty(ptyId) } catch { if (connectionId) { + if (isPendingSshPtyCleanup(ptyId, connectionId, store)) { + return false + } // Why: runtime/CLI close can target a detached SSH PTY after its // provider was unregistered. Tombstone the lease so reconnect does // not revive a terminal the user explicitly closed. @@ -5584,6 +5612,9 @@ export function registerPtyHandlers( provider = connectionId ? getProvider(connectionId) : getProviderForPty(ptyId) } catch { if (connectionId) { + if (isPendingSshPtyCleanup(ptyId, connectionId, store)) { + return false + } // Why: an absent SSH provider means there is no live target left to // await, but the relay lease must still be tombstoned. const incarnationId = finishPtyShutdown(ptyId, connectionId, store) @@ -6723,6 +6754,7 @@ export function registerPtyHandlers( ...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}), ...(typeof args.tabId === 'string' ? { tabId: args.tabId } : {}), ...(validatedLeafId ? { leafId: validatedLeafId } : {}), + cleanupPending: false, state: 'attached', lastAttachedAt: Date.now() }) @@ -6759,6 +6791,25 @@ export function registerPtyHandlers( } catch (err) { console.error('[pty] failed to persist PTY binding after spawn:', err) if (!result.isReattach) { + if (store && args.connectionId) { + store.upsertSshRemotePtyLease({ + targetId: args.connectionId, + ptyId: relayResultId, + ...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}), + cleanupPending: true, + ...(typeof args.tabId === 'string' && + validatedLeafId && + isValidTerminalTabId(args.tabId) + ? { cleanupExpectedPaneKey: makePaneKey(args.tabId, validatedLeafId) } + : {}), + ...(typeof args.tabId === 'string' ? { cleanupExpectedTabId: args.tabId } : {}), + ...(result.incarnationId + ? { cleanupExpectedIncarnationId: result.incarnationId } + : {}), + state: 'attached', + lastAttachedAt: Date.now() + }) + } await shutdownFreshPtyAfterBindingFailure( provider, result.id, @@ -7509,6 +7560,9 @@ export function registerPtyHandlers( } const provider = connectionId ? sshProviders.get(connectionId) : tryGetProviderForPty(args.id) if (!provider && connectionId) { + if (isPendingSshPtyCleanup(args.id, connectionId, store)) { + throw new Error('SSH PTY cleanup requires reconnect before shutdown can be confirmed') + } // Why: detached SSH PTYs intentionally keep ownership after their // provider is unregistered; hydrated app-scoped ids can also arrive // before ownership is rebuilt. Tombstone instead of falling back local. diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 06ee8a9d6a0..0d1bbe899af 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -10822,6 +10822,83 @@ describe('Store', () => { ]) }) + it('replaces stale surface identity with cleanup identity for a reused SSH PTY id', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'remote-pty', + worktreeId: 'wt-old', + tabId: 'tab-old', + leafId: TEST_LEAF_1, + state: 'attached' + }) + store.setWorkspaceSession({ + activeRepoId: 'r1', + activeWorktreeId: 'wt-old', + activeTabId: 'tab-old', + tabsByWorktree: { + 'wt-old': [ + { + id: 'tab-old', + worktreeId: 'wt-old', + title: 'Terminal', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1, + ptyId: 'ssh:ssh-1@@remote-pty' + } + ] + }, + terminalLayoutsByTabId: { + 'tab-old': { + root: { type: 'leaf', leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'ssh:ssh-1@@remote-pty' } + } + } + }) + + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'remote-pty', + cleanupPending: true, + cleanupExpectedPaneKey: `tab-new:${TEST_LEAF_2}`, + cleanupExpectedTabId: 'tab-new', + cleanupExpectedIncarnationId: 'incarnation-new', + state: 'attached' + }) + + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ + expect.objectContaining({ + ptyId: 'remote-pty', + cleanupPending: true, + cleanupExpectedPaneKey: `tab-new:${TEST_LEAF_2}`, + cleanupExpectedTabId: 'tab-new', + cleanupExpectedIncarnationId: 'incarnation-new' + }) + ]) + expect(store.getSshRemotePtyLeases('ssh-1')[0]).not.toHaveProperty('tabId') + expect(store.getSshRemotePtyLeases('ssh-1')[0]).not.toHaveProperty('leafId') + expect(store.getWorkspaceSession().tabsByWorktree['wt-old'][0].ptyId).toBeNull() + expect(store.getWorkspaceSession().terminalLayoutsByTabId['tab-old'].ptyIdsByLeafId).toEqual({}) + + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'remote-pty', + cleanupPending: true, + cleanupExpectedTabId: 'tab-newer', + state: 'attached' + }) + expect(store.getSshRemotePtyLeases('ssh-1')[0]).toEqual( + expect.objectContaining({ cleanupPending: true, cleanupExpectedTabId: 'tab-newer' }) + ) + expect(store.getSshRemotePtyLeases('ssh-1')[0]).not.toHaveProperty( + 'cleanupExpectedIncarnationId' + ) + }) + it('rejects mismatched scoped SSH remote PTY lease ids on write paths', async () => { const store = await createStore() diff --git a/src/main/persistence.ts b/src/main/persistence.ts index b62b25f7977..9201d348aaa 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -1615,6 +1615,16 @@ function normalizeSshRemotePtyLease(value: unknown): SshRemotePtyLease | null { ...(typeof raw.worktreeId === 'string' ? { worktreeId: raw.worktreeId } : {}), ...(typeof raw.tabId === 'string' ? { tabId: raw.tabId } : {}), ...(typeof raw.leafId === 'string' && raw.leafId.length <= 256 ? { leafId: raw.leafId } : {}), + ...(raw.cleanupPending === true ? { cleanupPending: true } : {}), + ...(typeof raw.cleanupExpectedPaneKey === 'string' + ? { cleanupExpectedPaneKey: raw.cleanupExpectedPaneKey } + : {}), + ...(typeof raw.cleanupExpectedTabId === 'string' + ? { cleanupExpectedTabId: raw.cleanupExpectedTabId } + : {}), + ...(typeof raw.cleanupExpectedIncarnationId === 'string' + ? { cleanupExpectedIncarnationId: raw.cleanupExpectedIncarnationId } + : {}), state, createdAt: typeof raw.createdAt === 'number' ? raw.createdAt : now, updatedAt: typeof raw.updatedAt === 'number' ? raw.updatedAt : now, @@ -7264,12 +7274,43 @@ export class Store { entry.targetId === normalizedLease.targetId && entry.ptyId === normalizedLease.ptyId ) const existing = existingIndex !== -1 ? this.state.sshRemotePtyLeases[existingIndex] : undefined + if (normalizedLease.cleanupPending === true && existing && !existing.cleanupPending) { + this.clearSshRemotePtyBindingsForLeases(normalizedLease.targetId, [existing]) + } const next: SshRemotePtyLease = { ...existing, ...normalizedLease, createdAt: existing?.createdAt ?? normalizedLease.createdAt ?? now, updatedAt: normalizedLease.updatedAt ?? now } + if (normalizedLease.cleanupPending === true) { + next.cleanupPending = true + delete next.tabId + delete next.leafId + delete next.cleanupExpectedPaneKey + delete next.cleanupExpectedTabId + delete next.cleanupExpectedIncarnationId + Object.assign(next, { + ...(normalizedLease.cleanupExpectedPaneKey + ? { cleanupExpectedPaneKey: normalizedLease.cleanupExpectedPaneKey } + : {}), + ...(normalizedLease.cleanupExpectedTabId + ? { cleanupExpectedTabId: normalizedLease.cleanupExpectedTabId } + : {}), + ...(normalizedLease.cleanupExpectedIncarnationId + ? { cleanupExpectedIncarnationId: normalizedLease.cleanupExpectedIncarnationId } + : {}) + }) + } else if ( + normalizedLease.cleanupPending === false || + normalizedLease.tabId !== undefined || + normalizedLease.leafId !== undefined + ) { + delete next.cleanupPending + delete next.cleanupExpectedPaneKey + delete next.cleanupExpectedTabId + delete next.cleanupExpectedIncarnationId + } if (existingIndex !== -1) { this.state.sshRemotePtyLeases[existingIndex] = next } else { @@ -7381,6 +7422,28 @@ export class Store { this.flush() } + removeMatchingSshPtyCleanupLease( + targetId: string, + ptyId: string, + cleanupExpectedIncarnationId: string | undefined + ): boolean { + const relayPtyId = this.getRelayPtyIdForSshLeaseStorage(targetId, ptyId) + const leases = this.state.sshRemotePtyLeases ?? [] + const next = leases.filter( + (lease) => + lease.targetId !== targetId || + lease.ptyId !== relayPtyId || + lease.cleanupPending !== true || + lease.cleanupExpectedIncarnationId !== cleanupExpectedIncarnationId + ) + if (next.length === leases.length) { + return false + } + this.state.sshRemotePtyLeases = next + this.flush() + return true + } + removeSshRemotePtyLease(targetId: string, ptyId: string): void { const relayPtyId = this.getRelayPtyIdForSshLeaseStorage(targetId, ptyId) const leases = (this.state.sshRemotePtyLeases ?? []).filter( diff --git a/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts b/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts index 3eebb3574b3..90c109dfa51 100644 --- a/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts +++ b/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts @@ -596,4 +596,173 @@ describe('SshRelaySession reconnect incarnation ordering', () => { ) consoleError.mockRestore() }) + + it('retries a cleanup lease without restoring a terminal surface', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const { getSshPtyProvider } = await import('../ipc/pty') + const mockAttach = vi.fn().mockResolvedValue({ incarnationId: 'cleanup-incarnation' }) + const mockShutdown = vi.fn().mockResolvedValue(undefined) + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect: mockAttach, + listProcesses: vi.fn().mockResolvedValue([ + { + id: 'ssh:target-1@@cleanup-pty', + incarnationId: 'cleanup-incarnation', + cwd: '/remote/repo', + title: 'shell' + } + ]), + shutdown: mockShutdown, + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ + { + targetId: 'target-1', + ptyId: 'cleanup-pty', + worktreeId: 'repo-1::/remote/repo', + cleanupPending: true, + cleanupExpectedPaneKey: 'tab-cleanup:33333333-3333-4333-8333-333333333333', + cleanupExpectedTabId: 'tab-cleanup', + cleanupExpectedIncarnationId: 'cleanup-incarnation', + state: 'detached' + } + ] as ReturnType) + const runtime = { + onPtySpawned: vi.fn(), + registerPty: vi.fn() + } + const session = new SshRelaySession( + 'target-1', + getMainWindow, + mockStore, + mockPortForward, + runtime as never + ) + + await session.establish(mockConn) + + expect(mockAttach).not.toHaveBeenCalled() + expect(mockShutdown).toHaveBeenCalledWith( + 'ssh:target-1@@cleanup-pty', + expect.objectContaining({ immediate: true, deadlineMs: expect.any(Number) }) + ) + expect(mockStore.removeMatchingSshPtyCleanupLease).toHaveBeenCalledWith( + 'target-1', + 'cleanup-pty', + 'cleanup-incarnation' + ) + expect(runtime.onPtySpawned).not.toHaveBeenCalled() + expect(runtime.registerPty).not.toHaveBeenCalled() + expect(mockStore.persistPtyBinding).not.toHaveBeenCalled() + }) + + it('retains a cleanup lease when reconnect shutdown fails', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const { getSshPtyProvider } = await import('../ipc/pty') + const shutdown = vi.fn().mockRejectedValue(new Error('injected cleanup retry failure')) + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect: vi.fn().mockResolvedValue({ incarnationId: 'cleanup-incarnation' }), + listProcesses: vi.fn().mockResolvedValue([ + { + id: 'ssh:target-1@@cleanup-pty', + incarnationId: 'cleanup-incarnation', + cwd: '/remote/repo', + title: 'shell' + } + ]), + shutdown, + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ + { + targetId: 'target-1', + ptyId: 'cleanup-pty', + cleanupPending: true, + cleanupExpectedTabId: 'tab-cleanup', + cleanupExpectedIncarnationId: 'cleanup-incarnation', + state: 'detached' + } + ] as ReturnType) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const runtime = { onPtySpawned: vi.fn(), registerPty: vi.fn() } + const session = new SshRelaySession( + 'target-1', + getMainWindow, + mockStore, + mockPortForward, + runtime as never + ) + + try { + await session.establish(mockConn) + expect(shutdown).toHaveBeenCalledOnce() + expect(mockStore.removeMatchingSshPtyCleanupLease).not.toHaveBeenCalledWith( + 'target-1', + 'cleanup-pty', + 'cleanup-incarnation' + ) + expect(runtime.onPtySpawned).not.toHaveBeenCalled() + expect(runtime.registerPty).not.toHaveBeenCalled() + } finally { + warn.mockRestore() + } + }) + + it.each([ + ['reused', 'different-incarnation', true], + ['unproven', undefined, false] + ])('does not auto-clean a %s relay PTY identity', async (_case, incarnationId, terminates) => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const { getSshPtyProvider } = await import('../ipc/pty') + const shutdown = vi.fn() + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect: vi.fn().mockResolvedValue({ incarnationId }), + listProcesses: vi.fn().mockResolvedValue([ + { + id: 'ssh:target-1@@cleanup-pty', + incarnationId, + cwd: '/remote/repo', + title: 'shell' + } + ]), + shutdown, + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ + { + targetId: 'target-1', + ptyId: 'cleanup-pty', + cleanupPending: true, + cleanupExpectedPaneKey: 'tab-cleanup:33333333-3333-4333-8333-333333333333', + cleanupExpectedTabId: 'tab-cleanup', + cleanupExpectedIncarnationId: 'expected-incarnation', + state: 'detached' + } + ] as ReturnType) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward, { + onPtySpawned: vi.fn(), + registerPty: vi.fn() + } as never) + + try { + await session.establish(mockConn) + expect(shutdown).not.toHaveBeenCalled() + if (terminates) { + expect(mockStore.removeMatchingSshPtyCleanupLease).toHaveBeenCalledWith( + 'target-1', + 'cleanup-pty', + 'expected-incarnation' + ) + } else { + expect(mockStore.removeMatchingSshPtyCleanupLease).not.toHaveBeenCalledWith( + 'target-1', + 'cleanup-pty', + 'expected-incarnation' + ) + } + } finally { + warn.mockRestore() + } + }) }) diff --git a/src/main/ssh/ssh-relay-session-test-fixtures.ts b/src/main/ssh/ssh-relay-session-test-fixtures.ts index c5f084a8785..fcab4bb5fd9 100644 --- a/src/main/ssh/ssh-relay-session-test-fixtures.ts +++ b/src/main/ssh/ssh-relay-session-test-fixtures.ts @@ -26,6 +26,7 @@ export function createMockDeps(): SshRelaySessionTestDeps { markSshRemotePtyLeasesAsync: vi.fn(), markSshRemotePtyLeasesForShutdown: vi.fn(), markSshRemotePtyLeasesAttachedAsync: vi.fn(), + removeMatchingSshPtyCleanupLease: vi.fn().mockReturnValue(true), persistPtyBinding: vi.fn() } as unknown as Store const mockPortForward = { diff --git a/src/main/ssh/ssh-relay-session.test.ts b/src/main/ssh/ssh-relay-session.test.ts index 9cdd34e5877..570fca21fd4 100644 --- a/src/main/ssh/ssh-relay-session.test.ts +++ b/src/main/ssh/ssh-relay-session.test.ts @@ -531,46 +531,6 @@ describe('SshRelaySession', () => { ) }) - it('does not restore a cleanup-only lease as a terminal surface', async () => { - const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() - const { getSshPtyProvider } = await import('../ipc/pty') - const mockAttach = vi.fn().mockResolvedValue({ incarnationId: 'cleanup-incarnation' }) - vi.mocked(getSshPtyProvider).mockReturnValue({ - attachForReconnect: mockAttach, - dispose: vi.fn() - } as unknown as ReturnType) - vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ - { - targetId: 'target-1', - ptyId: 'cleanup-pty', - worktreeId: 'repo-1::/remote/repo', - state: 'detached' - } - ] as ReturnType) - const runtime = { - onPtySpawned: vi.fn(), - registerPty: vi.fn() - } - const session = new SshRelaySession( - 'target-1', - getMainWindow, - mockStore, - mockPortForward, - runtime as never - ) - - await session.establish(mockConn) - - expect(mockAttach).toHaveBeenCalledWith('cleanup-pty') - expect(runtime.onPtySpawned).toHaveBeenCalledWith( - 'ssh:target-1@@cleanup-pty', - 'cleanup-incarnation', - { awaitsRegistration: false } - ) - expect(runtime.registerPty).not.toHaveBeenCalled() - expect(mockStore.persistPtyBinding).not.toHaveBeenCalled() - }) - it('forwards a lease tab identity to reattach so a reset relay cannot cross-wire it', async () => { const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() const { getSshPtyProvider } = await import('../ipc/pty') diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 820eb7e1360..7b23711a55f 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -2248,11 +2248,14 @@ export class SshRelaySession { const activeLeases = this.store .getSshRemotePtyLeases(this.targetId) .filter((lease) => lease.state !== 'terminated' && lease.state !== 'expired') - const activeLeaseByPtyId = new Map(activeLeases.map((lease) => [lease.ptyId, lease])) - const leasedPtyIds = activeLeases.map((lease) => lease.ptyId) + const cleanupLeases = activeLeases.filter((lease) => lease.cleanupPending === true) + const surfaceLeases = activeLeases.filter((lease) => lease.cleanupPending !== true) + const cleanupPtyIds = new Set(cleanupLeases.map((lease) => lease.ptyId)) + const activeLeaseByPtyId = new Map(surfaceLeases.map((lease) => [lease.ptyId, lease])) + const leasedPtyIds = surfaceLeases.map((lease) => lease.ptyId) // Why: pass pane identity so the relay can reject cross-generation id collisions; tabId falls back for pre-leafId leases. const expectedIdentityByPtyId = new Map( - activeLeases + surfaceLeases .map((lease): [string, ExpectedPtyIdentity] | null => { const expected = expectedIdentityForLease(lease) return expected ? [lease.ptyId, expected] : null @@ -2263,9 +2266,9 @@ export class SshRelaySession { // Why: after app restart ptyOwnership is empty, but durable SSH leases still describe grace-window survivors. const ptyIds = Array.from( new Set([ - ...getPtyIdsForConnection(this.targetId).map((ptyId) => - toRelaySshPtyId(this.targetId, ptyId) - ), + ...getPtyIdsForConnection(this.targetId) + .map((ptyId) => toRelaySshPtyId(this.targetId, ptyId)) + .filter((ptyId) => !cleanupPtyIds.has(ptyId)), ...leasedPtyIds ]) ) @@ -2274,6 +2277,13 @@ export class SshRelaySession { if (!ptyProvider || providerGeneration === null || this.mux !== mux) { return } + void this.retryPendingPtyCleanup(ptyProvider, cleanupLeases, shouldContinue).catch((error) => { + console.warn( + `[ssh-relay-session] Cleanup retries failed for ${this.targetId}: ${ + error instanceof Error ? error.message : String(error) + }` + ) + }) let nextPtyIndex = 0 const worker = async (): Promise => { while (shouldContinue()) { @@ -2315,6 +2325,83 @@ export class SshRelaySession { } } + private async retryPendingPtyCleanup( + ptyProvider: SshPtyProvider, + leases: SshPtyLease[], + shouldContinue: () => boolean + ): Promise { + if (leases.length === 0) { + return + } + const inventory = await ptyProvider.listProcesses({ + deadlineMs: Date.now() + SSH_PTY_REATTACH_ATTEMPT_TIMEOUT_MS + }) + const processById = new Map(inventory.map((process) => [process.id, process])) + const retireCleanupLease = (lease: SshPtyLease, clearRouting: boolean): void => { + if (!shouldContinue()) { + return + } + const retired = this.store.removeMatchingSshPtyCleanupLease( + this.targetId, + lease.ptyId, + lease.cleanupExpectedIncarnationId + ) + if (retired && clearRouting) { + const appPtyId = toAppSshPtyId(this.targetId, lease.ptyId) + clearProviderPtyState(appPtyId) + deletePtyOwnership(appPtyId) + } + } + let nextLeaseIndex = 0 + const worker = async (): Promise => { + while (shouldContinue()) { + const lease = leases[nextLeaseIndex++] + if (!lease) { + return + } + const appPtyId = toAppSshPtyId(this.targetId, lease.ptyId) + const process = processById.get(appPtyId) + if (!process) { + retireCleanupLease(lease, true) + continue + } + if ( + !lease.cleanupExpectedIncarnationId || + !process.incarnationId || + process.incarnationId !== lease.cleanupExpectedIncarnationId + ) { + if (process.incarnationId && lease.cleanupExpectedIncarnationId) { + retireCleanupLease(lease, false) + } else { + console.warn( + `[ssh-relay-session] PTY ${lease.ptyId} cleanup retry lacks incarnation proof for ${this.targetId}` + ) + } + continue + } + try { + await ptyProvider.shutdown(appPtyId, { + immediate: true, + deadlineMs: Date.now() + SSH_PTY_REATTACH_ATTEMPT_TIMEOUT_MS + }) + } catch (error) { + if (!isSshPtyNotFoundError(error) && !isSshPtyIdentityMismatchError(error)) { + console.warn( + `[ssh-relay-session] PTY ${lease.ptyId} cleanup retry failed for ${this.targetId}: ${ + error instanceof Error ? error.message : String(error) + }` + ) + continue + } + } + retireCleanupLease(lease, true) + } + } + await Promise.all( + Array.from({ length: Math.min(SSH_PTY_REATTACH_MAX_CONCURRENCY, leases.length) }, worker) + ) + } + private async reattachKnownPty(args: { ptyProvider: SshPtyProvider ptyId: string diff --git a/src/shared/ssh-types.ts b/src/shared/ssh-types.ts index fd6a9a3bcb0..28f9ff989db 100644 --- a/src/shared/ssh-types.ts +++ b/src/shared/ssh-types.ts @@ -198,6 +198,10 @@ export type SshRemotePtyLease = { worktreeId?: string tabId?: string leafId?: string + cleanupPending?: boolean + cleanupExpectedPaneKey?: string + cleanupExpectedTabId?: string + cleanupExpectedIncarnationId?: string state: SshRemotePtyLeaseState createdAt: number updatedAt: number