diff --git a/src/main/ipc/ssh.test.ts b/src/main/ipc/ssh.test.ts index 6609899dce1..2a95134e712 100644 --- a/src/main/ipc/ssh.test.ts +++ b/src/main/ipc/ssh.test.ts @@ -2467,7 +2467,7 @@ describe('SSH IPC handlers', () => { await expect(reconnect).resolves.toMatchObject({ targetId: 'ssh-1', status: 'connected' }) }) - it('ssh:terminateSessions ignores expired leases when disconnected', async () => { + it('ssh:terminateSessions cannot reach expired leases without a relay', async () => { mockStore.getSshRemotePtyLeases.mockReturnValue([ { targetId: 'ssh-1', ptyId: 'pty-expired', state: 'expired' } ]) @@ -2482,6 +2482,140 @@ describe('SSH IPC handlers', () => { expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') }) + it('ssh:terminateSessions kills expired leases whose remote PTY may still be alive', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockResolvedValue({}) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + mockStore.getSshRemotePtyLeases.mockReturnValue([ + { targetId: 'ssh-1', ptyId: 'pty-abandoned', state: 'expired' } + ]) + vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + mockPtyProvider.shutdown.mockResolvedValue(undefined) + + await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' }) + await handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' }) + + expect(mockPtyProvider.shutdown).toHaveBeenCalledWith('ssh:ssh-1@@pty-abandoned', { + immediate: true, + keepHistory: false + }) + expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + 'ssh-1', + 'pty-abandoned', + 'terminated' + ) + }) + + it('ssh:terminateSessions tombstones an expired lease the relay reports gone', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockResolvedValue({}) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + mockStore.getSshRemotePtyLeases.mockReturnValue([ + { targetId: 'ssh-1', ptyId: 'pty-abandoned', state: 'expired' } + ]) + vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + mockPtyProvider.shutdown.mockRejectedValue(new Error('PTY "pty-abandoned" not found')) + + await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' }) + await handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' }) + + expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + 'ssh-1', + 'pty-abandoned', + 'terminated' + ) + }) + + it('ssh:terminateSessions leaves leases it already proved terminated alone', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockResolvedValue({}) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + mockStore.getSshRemotePtyLeases.mockReturnValue([ + { targetId: 'ssh-1', ptyId: 'pty-tombstoned', state: 'terminated' } + ]) + vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + mockPtyProvider.shutdown.mockResolvedValue(undefined) + + await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' }) + await handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' }) + + expect(mockPtyProvider.shutdown).not.toHaveBeenCalled() + }) + + it('ssh:terminateSessions keeps an expired lease when its shutdown fails', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockResolvedValue({}) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + mockStore.getSshRemotePtyLeases.mockReturnValue([ + { targetId: 'ssh-1', ptyId: 'pty-abandoned', state: 'expired' } + ]) + vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + mockPtyProvider.shutdown.mockRejectedValue(new Error('mux down')) + + await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' }) + + await expect( + handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' }) + ).rejects.toThrow('Failed to terminate SSH host sessions') + expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( + 'ssh-1', + 'pty-abandoned', + 'terminated' + ) + }) + it('ssh:resetRelay force-stops the remote relay and expires tracked leases', async () => { const target: SshTarget = { id: 'ssh-1', diff --git a/src/main/ipc/ssh.ts b/src/main/ipc/ssh.ts index 3b362aa7ef4..ba180620119 100644 --- a/src/main/ipc/ssh.ts +++ b/src/main/ipc/ssh.ts @@ -1279,28 +1279,37 @@ export function registerSshHandlers( invalidateConnectAttempt(args.targetId) await runTargetLifecycle(args.targetId, async () => { const provider = getSshPtyProvider(args.targetId) - const leasedIds = persistedStore! - .getSshRemotePtyLeases(args.targetId) - .filter((lease) => lease.state !== 'terminated' && lease.state !== 'expired') - .map((lease) => lease.ptyId) + const leases = persistedStore!.getSshRemotePtyLeases(args.targetId) const ptyIdsByRelayId = new Map() - for (const ptyId of getPtyIdsForConnection(args.targetId)) { + // Why: only leases the app still believes it owns may force a reconnect; 'expired' ones are + // swept opportunistically because they can name a host that is gone for good (issue #2626). + const ownedRelayIds = new Set() + const trackPtyId = (ptyId: string, owned: boolean): void => { const relayPtyId = toRelaySshPtyId(args.targetId, ptyId) - ptyIdsByRelayId.set(relayPtyId, toAppSshPtyId(args.targetId, ptyId)) + if (!ptyIdsByRelayId.has(relayPtyId)) { + ptyIdsByRelayId.set(relayPtyId, toAppSshPtyId(args.targetId, ptyId)) + } + if (owned) { + ownedRelayIds.add(relayPtyId) + } } - for (const ptyId of leasedIds) { - const relayPtyId = toRelaySshPtyId(args.targetId, ptyId) - ptyIdsByRelayId.set( - relayPtyId, - ptyIdsByRelayId.get(relayPtyId) ?? toAppSshPtyId(args.targetId, ptyId) - ) + for (const ptyId of getPtyIdsForConnection(args.targetId)) { + trackPtyId(ptyId, true) + } + for (const lease of leases) { + if (lease.state === 'terminated') { + continue + } + // Why: 'expired' records that reattach gave up, never that the remote shell died — those are + // precisely the orphans, so the user's terminate action has to be able to reach them. + trackPtyId(lease.ptyId, lease.state !== 'expired') } const ptyIds = Array.from(ptyIdsByRelayId, ([relayPtyId, appPtyId]) => ({ relayPtyId, appPtyId })) - if (ptyIds.length > 0 && !provider) { + if (ownedRelayIds.size > 0 && !provider) { throw new Error( `${SSH_TERMINATE_RECONNECT_REQUIRED}: SSH relay is not connected; reconnect before terminating remote sessions.` ) diff --git a/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts b/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts new file mode 100644 index 00000000000..9d08ac7da0f --- /dev/null +++ b/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts @@ -0,0 +1,243 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { SshRelaySession } from './ssh-relay-session' +import { createMockDeps, mockDeploySuccess } from './ssh-relay-session-test-fixtures' + +const { muxRequestMock, openConsumerSessionMock } = vi.hoisted(() => ({ + muxRequestMock: vi.fn(), + openConsumerSessionMock: vi.fn(async (_mux: unknown, options: { clientInstanceId: string }) => ({ + clientInstanceId: options.clientInstanceId, + clientGeneration: 1, + ownerGeneration: 1, + ownerLease: 'test-owner-lease' + })) +})) + +vi.mock('./ssh-relay-deploy', () => ({ deployAndLaunchRelay: vi.fn() })) +vi.mock('./ssh-pty-consumer-session', () => ({ + openSshPtyConsumerSession: openConsumerSessionMock +})) +vi.mock('../ipc/ssh-pty-output-intake-registry', () => ({ + acceptSshPtyOutputData: vi.fn().mockResolvedValue(undefined), + acceptSshPtyOutputExit: vi.fn().mockResolvedValue(undefined), + allocateSshPtyProviderGeneration: vi.fn(() => 17), + beginSshPtyOutputGenerationMigration: vi.fn(() => ({ + byPty: new Map(), + completion: Promise.resolve() + })), + closeSshPtyOutputGeneration: vi.fn(), + getSshPtyAcceptedSourceCheckpoints: vi.fn(() => []), + applySshPtySourceRecoveryCancellationProof: vi.fn(() => true), + installSshPtySourceAckPublisher: vi.fn(() => () => {}), + installSshPtySourceCancellationPublisher: vi.fn(() => () => {}) +})) +vi.mock('./ssh-relay-deploy-helpers', () => ({ execCommand: vi.fn().mockResolvedValue('') })) +vi.mock('./ssh-remote-orca-cli', () => ({ + runRemoteOrcaCli: vi.fn().mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }) +})) +vi.mock('./ssh-channel-multiplexer', () => ({ + SshChannelMultiplexer: class MockSshChannelMultiplexer { + notify = vi.fn() + notifyWithSettlement = vi.fn() + request = muxRequestMock + onNotification = vi.fn().mockReturnValue(() => {}) + onNotificationByMethod = vi.fn().mockReturnValue(() => {}) + onRequest = vi.fn().mockReturnValue(() => {}) + onDispose = vi.fn().mockReturnValue(() => {}) + dispose = vi.fn() + isDisposed = vi.fn().mockReturnValue(false) + } +})) +vi.mock('../agent-hooks/remote-managed-hook-installers', () => ({ + installRemoteManagedAgentHooks: vi.fn() +})) +vi.mock('../providers/ssh-pty-provider', () => ({ + SshPtyProvider: class MockSshPtyProvider { + onData = vi.fn().mockReturnValue(() => {}) + onReplay = vi.fn().mockReturnValue(() => {}) + onExit = vi.fn().mockReturnValue(() => {}) + attach = vi.fn().mockResolvedValue(undefined) + attachForReconnect = vi.fn().mockResolvedValue({}) + dispose = vi.fn() + } +})) +vi.mock('../providers/ssh-filesystem-provider', () => ({ + SshFilesystemProvider: class MockSshFilesystemProvider { + dispose = vi.fn() + } +})) +vi.mock('../providers/ssh-git-provider', () => ({ + SshGitProvider: class MockSshGitProvider {} +})) +vi.mock('../ipc/pty', () => ({ + registerSshPtyProvider: vi.fn(), + unregisterSshPtyProvider: vi.fn(), + getSshPtyProvider: vi.fn(), + getPtyIdsForConnection: vi.fn().mockReturnValue([]), + clearPtyOwnershipForConnection: vi.fn(), + clearProviderPtyState: vi.fn(), + deletePtyOwnership: vi.fn(), + setPtyOwnership: vi.fn(), + restorePtyIncarnation: vi.fn(), + isCurrentPtyExit: vi.fn(() => true), + answerStartupTerminalColorQueriesForPty: vi.fn((_id: string, data: string) => data) +})) +vi.mock('../providers/ssh-filesystem-dispatch', () => ({ + registerSshFilesystemProvider: vi.fn(), + unregisterSshFilesystemProvider: vi.fn(), + getSshFilesystemProvider: vi.fn().mockReturnValue({ dispose: vi.fn() }) +})) +vi.mock('../providers/ssh-git-dispatch', () => ({ + registerSshGitProvider: vi.fn(), + unregisterSshGitProvider: vi.fn() +})) + +const { getSshPtyProvider, getPtyIdsForConnection, clearProviderPtyState, deletePtyOwnership } = + await import('../ipc/pty') + +const APP_PTY_ID = 'ssh:target-1@@pty-live' +const LEAF_ID = '11111111-1111-4111-8111-111111111111' + +function detachedLease() { + return { + targetId: 'target-1', + ptyId: 'pty-live', + state: 'detached' as const, + worktreeId: 'worktree-1', + tabId: 'tab-1', + leafId: LEAF_ID + } +} + +/** + * STA-3376: every reattach failure walks away from a remote PTY that may still be running a user's + * work. Orca's rule is that unprovable liveness never authorizes destroying a session, so these + * assert the opposite of a leak test: the shell must survive, and the lease must stay in a state + * the user-facing terminate action can still reach. + */ +describe('SshRelaySession abandoned remote PTYs', () => { + beforeEach(() => { + vi.clearAllMocks() + muxRequestMock.mockReset() + muxRequestMock.mockResolvedValue([]) + mockDeploySuccess() + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + }) + + async function establishWithFailingReattach(error: Error): Promise<{ + deps: ReturnType + shutdown: ReturnType + attachForReconnect: ReturnType + }> { + const deps = createMockDeps() + const shutdown = vi.fn().mockResolvedValue(undefined) + const attachForReconnect = vi.fn().mockRejectedValue(error) + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect, + shutdown, + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(deps.mockStore.getSshRemotePtyLeases).mockReturnValue([detachedLease()] as ReturnType< + typeof deps.mockStore.getSshRemotePtyLeases + >) + const session = new SshRelaySession( + 'target-1', + deps.getMainWindow, + deps.mockStore, + deps.mockPortForward + ) + + await session.establish(deps.mockConn) + + return { deps, shutdown, attachForReconnect } + } + + it('leaves a live shell running and reachable when reattach attempts are exhausted', async () => { + // A transport stall proves nothing about the remote shell; the user's long-running process + // may be untouched, so the lease must stay terminable rather than be tombstoned as expired. + const { deps, shutdown, attachForReconnect } = await establishWithFailingReattach( + new Error('PTY reattach attempt timed out after 15000ms') + ) + + expect(attachForReconnect).toHaveBeenCalled() + expect(shutdown).not.toHaveBeenCalled() + expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( + 'target-1', + 'pty-live', + 'expired' + ) + expect(deletePtyOwnership).not.toHaveBeenCalledWith(APP_PTY_ID) + expect(deps.mockWindow.webContents.send).not.toHaveBeenCalledWith('pty:exit', expect.anything()) + }) + + it('leaves another pane live shell running when the relay reports an identity mismatch', async () => { + // The relay answered that a *live* PTY holds this id under a different pane identity. Killing + // it would destroy an unrelated terminal, so this path may only stop claiming the id. + const { deps, shutdown, attachForReconnect } = await establishWithFailingReattach( + new Error('PTY "pty-live" not found (identity mismatch)') + ) + + expect(attachForReconnect).toHaveBeenCalled() + expect(shutdown).not.toHaveBeenCalled() + expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( + 'target-1', + 'pty-live', + 'expired' + ) + expect(clearProviderPtyState).not.toHaveBeenCalledWith(APP_PTY_ID) + }) + + it('retires the lease without a kill when the relay proves the PTY is gone', async () => { + // pty.attach verifies process liveness before answering not-found, so this is the one branch + // with positive proof of death — and a dead process needs no shutdown request. + const { deps, shutdown } = await establishWithFailingReattach( + new Error('PTY "pty-live" not found') + ) + + expect(shutdown).not.toHaveBeenCalled() + expect(deps.mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + 'target-1', + 'pty-live', + 'expired' + ) + expect(clearProviderPtyState).toHaveBeenCalledWith(APP_PTY_ID) + expect(deps.mockWindow.webContents.send).toHaveBeenCalledWith('pty:exit', { + id: APP_PTY_ID, + code: -1 + }) + }) + + it('keeps a recovered session attached when reattach succeeds after an earlier drop', async () => { + const deps = createMockDeps() + const shutdown = vi.fn().mockResolvedValue(undefined) + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect: vi.fn().mockResolvedValue({ incarnationId: 'incarnation-1' }), + shutdown, + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(deps.mockStore.getSshRemotePtyLeases).mockReturnValue([detachedLease()] as ReturnType< + typeof deps.mockStore.getSshRemotePtyLeases + >) + const runtime = { onPtySpawned: vi.fn(), registerPty: vi.fn() } + const session = new SshRelaySession( + 'target-1', + deps.getMainWindow, + deps.mockStore, + deps.mockPortForward, + runtime as never + ) + + await session.establish(deps.mockConn) + + expect(shutdown).not.toHaveBeenCalled() + expect(runtime.registerPty).toHaveBeenCalledWith(APP_PTY_ID, 'worktree-1', 'target-1', { + tabId: 'tab-1', + leafId: LEAF_ID, + incarnationId: 'incarnation-1' + }) + expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( + 'target-1', + 'pty-live', + 'expired' + ) + }) +})