From 25d83886981dd2fe4916d4cdd956a0f29f2e8e7f Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 16 Aug 2026 19:35:44 -0700 Subject: [PATCH] wip(ssh): probePtyLiveness unverifiable work rescued after agent death The reviewing agent died with this uncommitted while implementing the P1 fix for probePtyLiveness returning false for PTYs absent from both liveness sets. Introduces ssh-pty-liveness-state.ts. Not reviewed, not verified, not complete. --- src/main/providers/ssh-pty-errors.ts | 9 +++ src/main/providers/ssh-pty-liveness-state.ts | 58 +++++++++++++++++ src/main/providers/ssh-pty-provider.ts | 45 +++++++------ .../ssh-pty-restore-required-reattach.test.ts | 28 ++++++++ .../providers/ssh-pty-session-reattach.ts | 11 ++-- src/main/ssh/ssh-relay-session.test.ts | 64 +++++++++++++++++++ src/main/ssh/ssh-relay-session.ts | 17 ++++- 7 files changed, 205 insertions(+), 27 deletions(-) create mode 100644 src/main/providers/ssh-pty-liveness-state.ts diff --git a/src/main/providers/ssh-pty-errors.ts b/src/main/providers/ssh-pty-errors.ts index 95ef265836a..fa2f3edffad 100644 --- a/src/main/providers/ssh-pty-errors.ts +++ b/src/main/providers/ssh-pty-errors.ts @@ -20,6 +20,15 @@ export function isSshPtyLivenessUnverifiableError(error: unknown): boolean { return message.includes(SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR) } +export function isSshPtyExitedEvidenceError(error: unknown): boolean { + const message = error instanceof Error ? error.message : String(error) + return ( + !isSshPtyIdentityMismatchError(error) && + (message.includes(SSH_SESSION_EXPIRED_ERROR) || + message === 'agent_session_exited_during_start') + ) +} + export function isSshPtyIdentityMismatchError(error: unknown): boolean { const message = error instanceof Error ? error.message : String(error) return message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) || /identity mismatch/i.test(message) diff --git a/src/main/providers/ssh-pty-liveness-state.ts b/src/main/providers/ssh-pty-liveness-state.ts new file mode 100644 index 00000000000..c36b8fdd575 --- /dev/null +++ b/src/main/providers/ssh-pty-liveness-state.ts @@ -0,0 +1,58 @@ +export const MAX_SSH_PTY_EXIT_TOMBSTONES = 1000 + +export class SshPtyLivenessState { + readonly livePtyIds = new Set() + private readonly unverifiablePtyIds = new Set() + private readonly exitedPtyIds = new Set() + + constructor( + private readonly toAppPtyId: (id: string) => string, + initialUnverifiablePtyIds: Iterable + ) { + for (const id of initialUnverifiablePtyIds) { + this.acceptUnverifiable(id) + } + } + + clear(): void { + this.livePtyIds.clear() + this.unverifiablePtyIds.clear() + this.exitedPtyIds.clear() + } + + hasRoutingOwnership(id: string): boolean { + return this.livePtyIds.has(id) || this.unverifiablePtyIds.has(id) + } + + probe(id: string): boolean | null { + return this.livePtyIds.has(id) ? true : this.exitedPtyIds.has(id) ? false : null + } + + acceptLive(id: string): void { + const appPtyId = this.toAppPtyId(id) + this.unverifiablePtyIds.delete(appPtyId) + this.exitedPtyIds.delete(appPtyId) + this.livePtyIds.add(appPtyId) + } + + acceptUnverifiable(id: string): void { + const appPtyId = this.toAppPtyId(id) + this.livePtyIds.delete(appPtyId) + this.exitedPtyIds.delete(appPtyId) + this.unverifiablePtyIds.add(appPtyId) + } + + acceptExited(id: string): void { + const appPtyId = this.toAppPtyId(id) + this.livePtyIds.delete(appPtyId) + this.unverifiablePtyIds.delete(appPtyId) + this.exitedPtyIds.delete(appPtyId) + this.exitedPtyIds.add(appPtyId) + if (this.exitedPtyIds.size > MAX_SSH_PTY_EXIT_TOMBSTONES) { + const oldest = this.exitedPtyIds.values().next().value + if (oldest !== undefined) { + this.exitedPtyIds.delete(oldest) + } + } + } +} diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index ac0a70117b6..5d90d62e635 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -23,6 +23,7 @@ import { SshPtySpawnExitRaceTracker } from './ssh-pty-spawn-exit-race' import { SshAgentSessionCapabilities } from './ssh-agent-session-capabilities' import type { PtyProcessInspection } from './pty-process-inspection' import { writeToSshPty, writeToSshPtyWithSettlement } from './ssh-pty-write' +import { SshPtyLivenessState } from './ssh-pty-liveness-state' // Why: sequential relay teardown calls share one absolute budget; convert to the mux-relative timeout only at dispatch. function relayTimeoutOptions(deadlineMs: number | undefined): { timeoutMs: number } | undefined { @@ -33,8 +34,7 @@ function relayTimeoutOptions(deadlineMs: number | undefined): { timeoutMs: numbe export class SshPtyProvider implements IPtyProvider { private mux: SshChannelMultiplexer private connectionId: string - private livePtyIds = new Set() - private unverifiablePtyIds = new Set() + private readonly livenessState: SshPtyLivenessState readonly getAppliedSize: NonNullable private readonly agentSessionCapabilities: SshAgentSessionCapabilities private spawnExitRaces = new SshPtySpawnExitRaceTracker() @@ -47,19 +47,21 @@ export class SshPtyProvider implements IPtyProvider { connectionId: string, mux: SshChannelMultiplexer, private readonly remoteCliBridgeEnv?: RemoteCliBridgeEnv, - readonly providerGeneration = 1 + readonly providerGeneration = 1, + initialUnverifiablePtyIds: Iterable = [] ) { this.connectionId = connectionId this.mux = mux this.agentSessionCapabilities = new SshAgentSessionCapabilities(mux) this.getAppliedSize = createSshPtyAppliedSizeReader(mux, connectionId) + this.livenessState = new SshPtyLivenessState(this.toAppPtyId, initialUnverifiablePtyIds) this.outputState = new SshPtyProviderOutputState(providerGeneration, { mux, toAppPtyId: (id) => this.toAppPtyId(id), - livePtyIds: this.livePtyIds, + livePtyIds: this.livenessState.livePtyIds, recordExit: (relayPtyId, incarnationId) => { - this.unverifiablePtyIds.delete(this.toAppPtyId(relayPtyId)) + this.acceptExitedPty(this.toAppPtyId(relayPtyId)) this.spawnExitRaces.recordExit(relayPtyId, incarnationId) } }) @@ -67,8 +69,7 @@ export class SshPtyProvider implements IPtyProvider { dispose(): void { this.outputState.dispose() - this.livePtyIds.clear() - this.unverifiablePtyIds.clear() + this.livenessState.clear() } getConnectionId = (): string => this.connectionId @@ -104,7 +105,8 @@ export class SshPtyProvider implements IPtyProvider { rememberPtyIncarnation: (relayPtyId, incarnationId) => this.outputState.rememberPtyIncarnation(relayPtyId, incarnationId), acceptLivePty: (relayPtyId) => this.acceptLivePty(relayPtyId), - acceptUnverifiablePty: (relayPtyId) => this.acceptUnverifiablePty(relayPtyId) + acceptUnverifiablePty: (relayPtyId) => this.acceptUnverifiablePty(relayPtyId), + acceptExitedPty: (relayPtyId) => this.acceptExitedPty(relayPtyId) }) } @@ -162,6 +164,7 @@ export class SshPtyProvider implements IPtyProvider { rememberPtyIncarnation: (ptyId, incarnationId) => this.outputState.rememberPtyIncarnation(ptyId, incarnationId) }) + this.acceptLivePty(id) } async attachForReconnect( @@ -181,7 +184,7 @@ export class SshPtyProvider implements IPtyProvider { ...(expected?.tabId ? { expectedTabId: expected.tabId } : {}) } const relayPtyId = this.toRelayPtyId(id) - return await requestSshPtyAttach({ + const result = await requestSshPtyAttach({ mux: this.mux, relayPtyId, params, @@ -191,6 +194,8 @@ export class SshPtyProvider implements IPtyProvider { rememberPtyIncarnation: (ptyId, incarnationId) => this.outputState.rememberPtyIncarnation(ptyId, incarnationId) }) + this.acceptLivePty(id) + return result } write(id: string, data: string): boolean { @@ -218,8 +223,7 @@ export class SshPtyProvider implements IPtyProvider { }, relayTimeoutOptions(opts.deadlineMs) ) - this.livePtyIds.delete(id) - this.unverifiablePtyIds.delete(id) + this.acceptExitedPty(id) } async sendSignal(id: string, signal: string): Promise { @@ -294,19 +298,20 @@ export class SshPtyProvider implements IPtyProvider { } // `hasPty` retains routing ownership; liveness callers must use the tri-state probe. - hasPty = (id: string): boolean => this.livePtyIds.has(id) || this.unverifiablePtyIds.has(id) + hasPty = (id: string): boolean => this.livenessState.hasRoutingOwnership(id) - probePtyLiveness = async (id: string): Promise => - this.unverifiablePtyIds.has(id) ? null : this.livePtyIds.has(id) + probePtyLiveness = async (id: string): Promise => this.livenessState.probe(id) - private acceptLivePty(id: string): void { - this.unverifiablePtyIds.delete(id) - this.livePtyIds.add(id) + acceptLivePty(id: string): void { + this.livenessState.acceptLive(id) } - private acceptUnverifiablePty(id: string): void { - this.livePtyIds.delete(id) - this.unverifiablePtyIds.add(id) + acceptUnverifiablePty(id: string): void { + this.livenessState.acceptUnverifiable(id) + } + + acceptExitedPty(id: string): void { + this.livenessState.acceptExited(id) } async getDefaultShell(): Promise { diff --git a/src/main/providers/ssh-pty-restore-required-reattach.test.ts b/src/main/providers/ssh-pty-restore-required-reattach.test.ts index d824be7957f..7bbc390278e 100644 --- a/src/main/providers/ssh-pty-restore-required-reattach.test.ts +++ b/src/main/providers/ssh-pty-restore-required-reattach.test.ts @@ -4,6 +4,7 @@ import { SSH_PTY_RESTORE_REQUIRED_ERROR, SSH_SESSION_EXPIRED_ERROR } from './ssh-pty-errors' +import { MAX_SSH_PTY_EXIT_TOMBSTONES } from './ssh-pty-liveness-state' import { SshPtyProvider } from './ssh-pty-provider' // The relay answers `restoreRequired` from its DELIVERY layer only: `requireRestore` @@ -41,6 +42,33 @@ async function spawnError(provider: SshPtyProvider): Promise { } describe('SSH PTY reattach when the relay requires source restoration', () => { + it('keeps an unprobed PTY unverifiable', async () => { + const provider = new SshPtyProvider('conn-1', createMockMux() as never) + + expect(provider.hasPty('ssh:conn-1@@pty-unprobed')).toBe(false) + await expect(provider.probePtyLiveness('ssh:conn-1@@pty-unprobed')).resolves.toBeNull() + }) + + it('retains routing ownership for a reconnecting PTY without reporting it live', async () => { + const id = 'ssh:conn-1@@pty-reconnecting' + const provider = new SshPtyProvider('conn-1', createMockMux() as never, undefined, 1, [id]) + + expect(provider.hasPty(id)).toBe(true) + await expect(provider.probePtyLiveness(id)).resolves.toBeNull() + }) + + it('bounds exited PTY evidence and evicts to unverifiable', async () => { + const provider = new SshPtyProvider('conn-1', createMockMux() as never) + for (let index = 0; index <= MAX_SSH_PTY_EXIT_TOMBSTONES; index += 1) { + provider.acceptExitedPty(`pty-${index}`) + } + + await expect(provider.probePtyLiveness('ssh:conn-1@@pty-0')).resolves.toBeNull() + await expect( + provider.probePtyLiveness(`ssh:conn-1@@pty-${MAX_SSH_PTY_EXIT_TOMBSTONES}`) + ).resolves.toBe(false) + }) + it('re-attaches over the live PTY instead of reporting exited', async () => { const mux = createMockMux() // Why: the relay retires the stale delivery record as the restoreRequired response settles, diff --git a/src/main/providers/ssh-pty-session-reattach.ts b/src/main/providers/ssh-pty-session-reattach.ts index dda07f09ef3..be1c1baab2f 100644 --- a/src/main/providers/ssh-pty-session-reattach.ts +++ b/src/main/providers/ssh-pty-session-reattach.ts @@ -5,6 +5,7 @@ import { SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR, SSH_PTY_RESTORE_REQUIRED_ERROR, SSH_SESSION_EXPIRED_ERROR, + isSshPtyExitedEvidenceError, isSshPtyIdentityMismatchError, isSshPtyNotFoundError } from './ssh-pty-errors' @@ -262,6 +263,7 @@ export async function reattachSshPtySessionForSpawn( args: Parameters[0] & { acceptLivePty: (relayPtyId: string) => void acceptUnverifiablePty: (relayPtyId: string) => void + acceptExitedPty: (relayPtyId: string) => void } ): Promise { let result: SshPtyReattachResult | undefined @@ -277,11 +279,7 @@ export async function reattachSshPtySessionForSpawn( try { result = await reattachSshPtySessionWithExitFence(args) } catch (error) { - const message = error instanceof Error ? error.message : String(error) - if ( - message.includes(SSH_SESSION_EXPIRED_ERROR) || - message === 'agent_session_exited_during_start' - ) { + if (isSshPtyIdentityMismatchError(error) || isSshPtyExitedEvidenceError(error)) { throw error } // Why: losing contact during verification is unverifiable, not live or exited. @@ -304,6 +302,9 @@ export async function reattachSshPtySessionForSpawn( return spawnResult } catch (error) { await result?.sourceActivationLease?.rollback() + if (isSshPtyExitedEvidenceError(error)) { + args.acceptExitedPty(result?.id ?? toAppSshPtyId(args.connectionId, args.sessionId)) + } throw error } } diff --git a/src/main/ssh/ssh-relay-session.test.ts b/src/main/ssh/ssh-relay-session.test.ts index 2f36c537f08..c7b716c70af 100644 --- a/src/main/ssh/ssh-relay-session.test.ts +++ b/src/main/ssh/ssh-relay-session.test.ts @@ -66,6 +66,9 @@ vi.mock('../providers/ssh-pty-provider', () => ({ isSshPtyIdentityMismatchError: (err: unknown) => (err instanceof Error ? err.message : String(err)).includes('identity mismatch'), SshPtyProvider: class MockSshPtyProvider { + private livePtyIds = new Set() + private unverifiablePtyIds = new Set() + private exitedPtyIds = new Set() onData = vi.fn().mockReturnValue(() => {}) onReplay = vi.fn().mockReturnValue(() => {}) onExit = vi.fn().mockReturnValue(() => {}) @@ -73,6 +76,32 @@ vi.mock('../providers/ssh-pty-provider', () => ({ attachForReconnect = vi.fn().mockResolvedValue({}) setPtyDeliveryPauseAdapter = pauseAdapterMock dispose = vi.fn() + + constructor( + _connectionId: string, + _mux: unknown, + _remoteCliBridgeEnv: unknown, + _providerGeneration: number, + initialUnverifiablePtyIds: Iterable = [] + ) { + this.unverifiablePtyIds = new Set(initialUnverifiablePtyIds) + } + + hasPty = (id: string): boolean => this.livePtyIds.has(id) || this.unverifiablePtyIds.has(id) + probePtyLiveness = vi.fn( + async (id: string): Promise => + this.livePtyIds.has(id) ? true : this.exitedPtyIds.has(id) ? false : null + ) + acceptUnverifiablePty = vi.fn((id: string) => { + this.livePtyIds.delete(id) + this.exitedPtyIds.delete(id) + this.unverifiablePtyIds.add(id) + }) + acceptExitedPty = vi.fn((id: string) => { + this.livePtyIds.delete(id) + this.unverifiablePtyIds.delete(id) + this.exitedPtyIds.add(id) + }) } })) @@ -206,6 +235,41 @@ describe('SshRelaySession', () => { expect(registerSshGitProvider).toHaveBeenCalledWith('target-1', expect.anything()) }) + it('keeps a transiently unreattached PTY unverifiable after the relay becomes ready', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const { getSshPtyProvider } = await import('../ipc/pty') + vi.mocked(getPtyIdsForConnection).mockReturnValue(['pty-unverifiable']) + vi.mocked(getSshPtyProvider).mockImplementationOnce(() => { + const provider = vi.mocked(registerSshPtyProvider).mock.calls.at(-1)?.[1] as { + attachForReconnect: ReturnType + } + provider.attachForReconnect.mockRejectedValue(new Error('transport interrupted')) + return provider as never + }) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + + try { + await session.establish(mockConn) + const provider = vi.mocked(registerSshPtyProvider).mock.calls.at(-1)?.[1] as { + hasPty: (id: string) => boolean + probePtyLiveness: (id: string) => Promise + } + const appPtyId = 'ssh:target-1@@pty-unverifiable' + + expect(session.getState()).toBe('ready') + expect(provider.hasPty(appPtyId)).toBe(true) + await expect(provider.probePtyLiveness(appPtyId)).resolves.toBeNull() + expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( + 'target-1', + 'pty-unverifiable', + 'expired' + ) + } finally { + warn.mockRestore() + } + }) + it('continues provider registration when the relay managed-hook request fails', async () => { process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1' muxRequestMock.mockImplementation(async (method: string) => { diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 820eb7e1360..a8b41b49493 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -1009,11 +1009,21 @@ export class SshRelaySession { this.wireUpRemoteOrcaCli(mux, connectionIncarnation) const providerGeneration = allocateSshPtyProviderGeneration() + const initialUnverifiablePtyIds = new Set( + [ + ...getPtyIdsForConnection(this.targetId), + ...this.store + .getSshRemotePtyLeases(this.targetId) + .filter((lease) => lease.state !== 'terminated' && lease.state !== 'expired') + .map((lease) => lease.ptyId) + ].map((ptyId) => toAppSshPtyId(this.targetId, ptyId)) + ) const ptyProvider = new SshPtyProvider( this.targetId, mux, this.remoteCliBridgeEnv ?? undefined, - providerGeneration + providerGeneration, + initialUnverifiablePtyIds ) const consumerOwnerState = this.activePtyConsumerOwner() if (consumerOwnerState) { @@ -2509,7 +2519,7 @@ export class SshRelaySession { if (!shouldContinue()) { return } - this.handlePtyReattachFailure(ptyId, appPtyId, pendingReattach, error) + this.handlePtyReattachFailure(ptyProvider, ptyId, appPtyId, pendingReattach, error) } finally { recoveryActivationLease?.retire() sourceActivationLease?.rollback() @@ -2649,12 +2659,14 @@ export class SshRelaySession { } private handlePtyReattachFailure( + ptyProvider: SshPtyProvider, ptyId: string, appPtyId: string, pending: PendingPtyReattach, error: unknown ): void { if (!isSshPtyNotFoundError(error)) { + ptyProvider.acceptUnverifiablePty?.(appPtyId) pending.restoreRequired = 'reattachAttemptsExhausted' this.wakeRecovery(pending) console.warn( @@ -2672,6 +2684,7 @@ export class SshRelaySession { ) return } + ptyProvider.acceptExitedPty?.(appPtyId) console.warn( `[ssh-relay-session] Dropping stale PTY ${ptyId} for ${this.targetId} after relay reattach failed: ${ error instanceof Error ? error.message : String(error)