From c51be8072ba50abdb50555d565649c3b46b4302c Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 9 Aug 2026 00:43:00 -0700 Subject: [PATCH] fix(ssh): stop sending pane identity on reattach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The relay froze pane identity at spawn, so moving a pane to another tab made it refuse a live shell — and refuse by saying 'not found'. The comparison is presence-guarded, so not sending the fields disarms it on every relay version including ones already installed on hosts: no wire change, no redeploy. Nothing is lost. It existed to catch a relay restart recycling pty-N for a new shell, and in exactly that case pane and tab both still match, so it accepted the wrong shell anyway. The incarnation the attach returns is what distinguishes those, and it already crosses the wire. Removes the whole client-side apparatus: the expected-identity type, its per-lease derivation, its map, and the parameter threaded through four layers. Co-authored-by: Orca --- ...pty-identity-mismatch-is-not-death.test.ts | 40 +++++++++++++ src/main/providers/ssh-pty-provider.test.ts | 30 ++-------- src/main/providers/ssh-pty-provider.ts | 11 ++-- .../providers/ssh-pty-session-reattach.ts | 12 ++-- .../ssh-relay-session-data-delivery.test.ts | 2 - .../ssh-relay-session-model-migration.test.ts | 2 - .../ssh-relay-session-recovery-races.test.ts | 4 +- src/main/ssh/ssh-relay-session.test.ts | 33 ++--------- src/main/ssh/ssh-relay-session.ts | 58 ++----------------- 9 files changed, 65 insertions(+), 127 deletions(-) diff --git a/src/main/providers/ssh-pty-identity-mismatch-is-not-death.test.ts b/src/main/providers/ssh-pty-identity-mismatch-is-not-death.test.ts index 83e40cdb742..6b9f64ba3e3 100644 --- a/src/main/providers/ssh-pty-identity-mismatch-is-not-death.test.ts +++ b/src/main/providers/ssh-pty-identity-mismatch-is-not-death.test.ts @@ -99,3 +99,43 @@ describe('a genuine absence is still a death', () => { expect(error.message).not.toContain(SSH_PTY_IDENTITY_MISMATCH_ERROR) }) }) + +// The relay only compares identity fields present on BOTH sides ("absent +// identity stays permissive", relay/pty-handler.ts). So not sending them stops +// every relay version from rejecting a moved pane — including relays already +// deployed on people's hosts, with no wire change and no redeploy. +// +// Nothing is lost: the check existed to catch a relay restart recycling pty-N +// for a new shell, and in exactly that case the pane and tab both still match, +// so it accepted the wrong shell anyway. +describe('reattach does not ask the relay to police pane identity', () => { + async function attachParams(): Promise> { + const request = vi.fn().mockRejectedValue(new Error('boom')) + try { + await reattachSshPtySession({ + mux: { request, notify: vi.fn() } as never, + connectionId: CONNECTION_ID, + sessionId: RELAY_PTY_ID, + options: { + cols: 80, + rows: 24, + paneKey: 'tab-new:leaf-1', + tabId: 'tab-new', + env: { ORCA_PANE_KEY: 'tab-old:leaf-1', ORCA_TAB_ID: 'tab-old' } + } as never + }) + } catch { + // the attach failure is not what this clause is about + } + const call = request.mock.calls.at(0) + return (call?.[1] ?? call?.[0] ?? {}) as Record + } + + it('sends no expected pane key', async () => { + expect(await attachParams()).not.toHaveProperty('expectedPaneKey') + }) + + it('sends no expected tab id', async () => { + expect(await attachParams()).not.toHaveProperty('expectedTabId') + }) +}) diff --git a/src/main/providers/ssh-pty-provider.test.ts b/src/main/providers/ssh-pty-provider.test.ts index d299e65b716..46dc3ff2e6f 100644 --- a/src/main/providers/ssh-pty-provider.test.ts +++ b/src/main/providers/ssh-pty-provider.test.ts @@ -639,7 +639,9 @@ describe('SshPtyProvider', () => { }) }) - it('reattaches with explicit pane identity when hook env was stripped', async () => { + // Pane identity is deliberately no longer sent: the relay's copy is frozen + // at spawn, so moving a pane to another tab made it refuse a live shell. + it('reattaches without asking the relay to police pane identity', async () => { mux.request.mockResolvedValue({ replay: 'buffered-output' }) await provider.spawn({ @@ -654,9 +656,7 @@ describe('SshPtyProvider', () => { id: 'pty-old', cols: 80, rows: 24, - suppressReplayNotification: true, - expectedPaneKey: 'tab-a:leaf-a', - expectedTabId: 'tab-a' + suppressReplayNotification: true }) }) @@ -739,28 +739,6 @@ describe('SshPtyProvider', () => { ) }) - it('attachForReconnect forwards expected identity when provided', async () => { - await provider.attachForReconnect(scopedPty1, { - paneKey: 'tab-a:leaf-a', - tabId: 'tab-a' - }) - - expectRequest( - mux.request, - 'pty.attach', - { - id: 'pty-1', - suppressReplayNotification: true, - expectedPaneKey: 'tab-a:leaf-a', - expectedTabId: 'tab-a' - }, - expect.objectContaining({ - timeoutMs: 10_000, - beforeResolve: expect.any(Function) - }) - ) - }) - it('write sends pty.data notification', () => { provider.write(scopedPty1, 'hello') expect(mux.notify).toHaveBeenCalledWith('pty.data', { id: 'pty-1', data: 'hello' }) diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index f516a3b3f24..a3d8a7109d1 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -182,19 +182,16 @@ export class SshPtyProvider implements IPtyProvider { async attachForReconnect( id: string, - expected?: { paneKey?: string; tabId?: string }, sourceRecovery?: PtySourceRecoveryRequest ): Promise { // Why: reconnect owns replay delivery so stale/duplicate attach results can - // be filtered before they reach the renderer. The expected identity lets the - // relay reject a cross-generation id collision instead of reattaching this - // lease to a different pane's freshly spawned PTY. + // be filtered before they reach the renderer. Pane identity is deliberately + // not sent — see reattachSshPtySession; the relay's copy is frozen at spawn, + // so it rejected panes that had merely moved tabs. const params = { id: this.toRelayPtyId(id), suppressReplayNotification: true, - ...(sourceRecovery ? { sourceRecovery } : {}), - ...(expected?.paneKey ? { expectedPaneKey: expected.paneKey } : {}), - ...(expected?.tabId ? { expectedTabId: expected.tabId } : {}) + ...(sourceRecovery ? { sourceRecovery } : {}) } const relayPtyId = this.toRelayPtyId(id) return await requestSshPtyAttach({ diff --git a/src/main/providers/ssh-pty-session-reattach.ts b/src/main/providers/ssh-pty-session-reattach.ts index 25ff2bc4933..acd7c2db7b8 100644 --- a/src/main/providers/ssh-pty-session-reattach.ts +++ b/src/main/providers/ssh-pty-session-reattach.ts @@ -185,9 +185,11 @@ export async function reattachSshPtySession(args: { const relaySessionId = toRelaySshPtyId(args.connectionId, args.sessionId) console.warn(`[ssh-pty] spawn() called with sessionId=${args.sessionId}, attempting pty.attach`) try { - // Why: expected pane identity prevents a reused relay id from attaching the wrong shell. - const expectedPaneKey = args.options.paneKey ?? args.options.env?.ORCA_PANE_KEY - const expectedTabId = args.options.tabId ?? args.options.env?.ORCA_TAB_ID + // Why no expected pane identity: the relay froze it at spawn, so moving a + // pane to another tab made it refuse a live shell — and refuse by saying + // "not found", which read as death. It never caught what it was for either: + // a relay restart recycling this id for a new shell leaves pane and tab + // matching. Recycling is caught by the incarnation the attach returns. const attachResult = await requestSshPtyAttach({ mux: args.mux, relayPtyId: relaySessionId, @@ -195,9 +197,7 @@ export async function reattachSshPtySession(args: { id: relaySessionId, cols: args.options.cols, rows: args.options.rows, - suppressReplayNotification: true, - ...(expectedPaneKey ? { expectedPaneKey } : {}), - ...(expectedTabId ? { expectedTabId } : {}) + suppressReplayNotification: true }, installSourceActivation: args.installSourceActivation, rememberPtyIncarnation: args.rememberPtyIncarnation diff --git a/src/main/ssh/ssh-relay-session-data-delivery.test.ts b/src/main/ssh/ssh-relay-session-data-delivery.test.ts index 2eca6802f28..2a6edf1840e 100644 --- a/src/main/ssh/ssh-relay-session-data-delivery.test.ts +++ b/src/main/ssh/ssh-relay-session-data-delivery.test.ts @@ -403,7 +403,6 @@ describe('SshRelaySession data delivery', () => { expect(retryCalls[0]).toHaveProperty('resume') expect(attachForReconnectMock).toHaveBeenCalledWith( 'pty-1', - undefined, Object.freeze({ status: 'checkpointUnavailable' }) ) second.dispose() @@ -762,7 +761,6 @@ describe('SshRelaySession data delivery', () => { expect(attachForReconnectMock).toHaveBeenCalledWith( 'pty-1', - undefined, expect.objectContaining({ status: 'checkpoint', deliveryToken: 'old-token', diff --git a/src/main/ssh/ssh-relay-session-model-migration.test.ts b/src/main/ssh/ssh-relay-session-model-migration.test.ts index ae5db55e0cc..ab1512d82dc 100644 --- a/src/main/ssh/ssh-relay-session-model-migration.test.ts +++ b/src/main/ssh/ssh-relay-session-model-migration.test.ts @@ -199,7 +199,6 @@ describe('SshRelaySession model migration', () => { expect(attachForReconnectMock).toHaveBeenCalledWith( 'pty-1', - undefined, Object.freeze({ status: 'checkpoint', clientGeneration: 1, @@ -259,7 +258,6 @@ describe('SshRelaySession model migration', () => { expect(attachForReconnectMock).toHaveBeenCalledWith( 'pty-1', - undefined, Object.freeze({ status: 'checkpointUnavailable' }) ) }) diff --git a/src/main/ssh/ssh-relay-session-recovery-races.test.ts b/src/main/ssh/ssh-relay-session-recovery-races.test.ts index 26dd798ac5a..f8853832ed1 100644 --- a/src/main/ssh/ssh-relay-session-recovery-races.test.ts +++ b/src/main/ssh/ssh-relay-session-recovery-races.test.ts @@ -705,7 +705,7 @@ describe('SshRelaySession recovery race fencing', () => { await session.reconnect(deps.mockConn) expect(attachForReconnectMock).toHaveBeenCalledTimes(2) - expect(attachForReconnectMock.mock.calls.at(-1)?.[2]).toMatchObject({ + expect(attachForReconnectMock.mock.calls.at(-1)?.[1]).toMatchObject({ status: 'checkpoint', deliveryToken: 'new-token', acceptedSourceEndSu: 8 @@ -763,7 +763,7 @@ describe('SshRelaySession recovery race fencing', () => { const replacementReconnect = session.reconnect(deps.mockConn) await Promise.all([staleReconnect, replacementReconnect]) - const recoveryRequests = attachForReconnectMock.mock.calls.map((call) => call[2]) + const recoveryRequests = attachForReconnectMock.mock.calls.map((call) => call[1]) expect(recoveryRequests).toHaveLength(2) expect(recoveryRequests[1]).toMatchObject({ status: 'checkpoint', diff --git a/src/main/ssh/ssh-relay-session.test.ts b/src/main/ssh/ssh-relay-session.test.ts index 570fca21fd4..7478caa8c48 100644 --- a/src/main/ssh/ssh-relay-session.test.ts +++ b/src/main/ssh/ssh-relay-session.test.ts @@ -531,26 +531,10 @@ describe('SshRelaySession', () => { ) }) - 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') - const mockAttach = vi.fn().mockResolvedValue(undefined) - vi.mocked(getSshPtyProvider).mockReturnValue({ - attachForReconnect: mockAttach, - dispose: vi.fn() - } as unknown as ReturnType) - vi.mocked(getPtyIdsForConnection).mockReturnValue([]) - vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ - { targetId: 'target-1', ptyId: 'pty-1', state: 'detached', tabId: 'tab-a' } - ] as ReturnType) - - const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) - await session.establish(mockConn) - - expect(mockAttach).toHaveBeenCalledWith('pty-1', { tabId: 'tab-a' }) - }) - - it('forwards a lease pane identity when leaf identity is available', async () => { + // The relay froze pane identity at spawn, so sending it made a moved pane + // unreachable — and it never caught the reused-id case it was written for, + // because after a relay restart the pane and tab both still match. + it('does not forward pane identity to reattach', async () => { const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() const { getSshPtyProvider } = await import('../ipc/pty') const mockAttach = vi.fn().mockResolvedValue(undefined) @@ -567,10 +551,7 @@ describe('SshRelaySession', () => { const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) await session.establish(mockConn) - expect(mockAttach).toHaveBeenCalledWith('pty-1', { - paneKey: `tab-a:${leafId}`, - tabId: 'tab-a' - }) + expect(mockAttach).toHaveBeenCalledWith('pty-1') }) it('does not expire a live reused relay id when attach rejects identity mismatch', async () => { @@ -602,10 +583,6 @@ describe('SshRelaySession', () => { await session.reconnect(mockConn) - expect(mockAttach).toHaveBeenCalledWith('pty-1', { - paneKey: `tab-old:${staleLeafId}`, - tabId: 'tab-old' - }) expect(clearProviderPtyState).not.toHaveBeenCalledWith('ssh:target-1@@pty-1') expect(deletePtyOwnership).not.toHaveBeenCalledWith('ssh:target-1@@pty-1') expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith('target-1', 'pty-1', 'expired') diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 7f26fe5f4d6..ed072b9189b 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -100,8 +100,6 @@ import { SSH_AI_VAULT_LIST_SESSIONS_TIMEOUT_MS, type SshAiVaultRelayListParams } from '../../shared/ssh-ai-vault-relay' -import { isTerminalLeafId, makePaneKey } from '../../shared/stable-pane-id' -import { isValidTerminalTabId } from '../../shared/terminal-tab-id' import { openSshPtyConsumerSession, type OpenSshPtyConsumerSessionOptions, @@ -187,28 +185,8 @@ type RemoteCliBridgeEnv = { pathDelimiter?: ':' | ';' } -type ExpectedPtyIdentity = { paneKey?: string; tabId?: string } type TargetedDeliveryRecovery = 'confirm-existing' | 'fresh-activation' -function expectedIdentityForLease(lease: { - tabId?: string - leafId?: string -}): ExpectedPtyIdentity | null { - if (typeof lease.tabId !== 'string' || lease.tabId.length === 0) { - return null - } - const paneKey = - isValidTerminalTabId(lease.tabId) && - typeof lease.leafId === 'string' && - isTerminalLeafId(lease.leafId) - ? makePaneKey(lease.tabId, lease.leafId) - : undefined - return { - ...(paneKey ? { paneKey } : {}), - tabId: lease.tabId - } -} - function parseRecoveryComplete(params: Record): PtySourceRecoveryComplete | null { if ( typeof params.id !== 'string' || @@ -1919,15 +1897,11 @@ export class SshRelaySession { const activeLeaseByPtyId = activeLease ? new Map([[relayPtyId, activeLease]]) : new Map() - const expectedIdentity = activeLease ? expectedIdentityForLease(activeLease) : undefined const attachedLeaseIds = new Set() await this.reattachKnownPty({ ptyProvider, ptyId: relayPtyId, activeLeaseByPtyId, - expectedIdentityByPtyId: expectedIdentity - ? new Map([[relayPtyId, expectedIdentity]]) - : new Map(), attachedLeaseIds, mux, providerGeneration, @@ -2198,15 +2172,6 @@ export class SshRelaySession { .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) - // 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 - .map((lease): [string, ExpectedPtyIdentity] | null => { - const expected = expectedIdentityForLease(lease) - return expected ? [lease.ptyId, expected] : null - }) - .filter((entry): entry is [string, ExpectedPtyIdentity] => entry !== null) - ) const attachedLeaseIds = new Set() // Why: after app restart ptyOwnership is empty, but durable SSH leases still describe grace-window survivors. const ptyIds = Array.from( @@ -2234,7 +2199,6 @@ export class SshRelaySession { ptyProvider, ptyId, activeLeaseByPtyId, - expectedIdentityByPtyId, attachedLeaseIds, mux, providerGeneration, @@ -2267,7 +2231,6 @@ export class SshRelaySession { ptyProvider: SshPtyProvider ptyId: string activeLeaseByPtyId: Map - expectedIdentityByPtyId: Map attachedLeaseIds: Set mux: SshChannelMultiplexer providerGeneration: number @@ -2278,7 +2241,6 @@ export class SshRelaySession { ptyProvider, ptyId, activeLeaseByPtyId, - expectedIdentityByPtyId, attachedLeaseIds, mux, providerGeneration, @@ -2309,7 +2271,6 @@ export class SshRelaySession { const attachResult = await this.attachPtyWithRetry( ptyProvider, ptyId, - expectedIdentityByPtyId.get(ptyId), recoveryRequest, shouldContinue ) @@ -2533,7 +2494,6 @@ export class SshRelaySession { private async attachPtyWithRetry( ptyProvider: SshPtyProvider, ptyId: string, - expectedIdentity: ExpectedPtyIdentity | undefined, recoveryRequest: PtySourceRecoveryRequest | undefined, shouldContinue: () => boolean ): Promise { @@ -2543,12 +2503,7 @@ export class SshRelaySession { throw lastError ?? new Error('PTY reattach attempt is no longer current') } try { - return await this.attachPtyWithDeadline( - ptyProvider, - ptyId, - expectedIdentity, - recoveryRequest - ) + return await this.attachPtyWithDeadline(ptyProvider, ptyId, recoveryRequest) } catch (error) { lastError = error if (!shouldContinue() || isSshPtyNotFoundError(error) || attempt === 1) { @@ -2563,7 +2518,6 @@ export class SshRelaySession { private async attachPtyWithDeadline( ptyProvider: SshPtyProvider, ptyId: string, - expectedIdentity: ExpectedPtyIdentity | undefined, recoveryRequest: PtySourceRecoveryRequest | undefined ): Promise { let timer: ReturnType | undefined @@ -2578,13 +2532,9 @@ export class SshRelaySession { timer.unref?.() }) try { - const attach = expectedIdentity - ? recoveryRequest - ? ptyProvider.attachForReconnect(ptyId, expectedIdentity, recoveryRequest) - : ptyProvider.attachForReconnect(ptyId, expectedIdentity) - : recoveryRequest - ? ptyProvider.attachForReconnect(ptyId, undefined, recoveryRequest) - : ptyProvider.attachForReconnect(ptyId) + const attach = recoveryRequest + ? ptyProvider.attachForReconnect(ptyId, recoveryRequest) + : ptyProvider.attachForReconnect(ptyId) const guardedAttach = attach.then((result) => { if (timedOut) { result.sourceActivationLease?.rollback()