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 6b9f64ba3e3..1daa30aa730 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 @@ -109,7 +109,9 @@ describe('a genuine absence is still a death', () => { // 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> { + async function attachParams( + extraOptions: Record = {} + ): Promise> { const request = vi.fn().mockRejectedValue(new Error('boom')) try { await reattachSshPtySession({ @@ -121,7 +123,8 @@ describe('reattach does not ask the relay to police pane identity', () => { rows: 24, paneKey: 'tab-new:leaf-1', tabId: 'tab-new', - env: { ORCA_PANE_KEY: 'tab-old:leaf-1', ORCA_TAB_ID: 'tab-old' } + env: { ORCA_PANE_KEY: 'tab-old:leaf-1', ORCA_TAB_ID: 'tab-old' }, + ...extraOptions } as never }) } catch { @@ -138,4 +141,26 @@ describe('reattach does not ask the relay to police pane identity', () => { it('sends no expected tab id', async () => { expect(await attachParams()).not.toHaveProperty('expectedTabId') }) + + // The producer pin for the relay's incarnation guard. The guard is only worth having if the + // expectation actually leaves the client, and nothing else here would notice if it stopped: + // the relay stays permissive on an absent field, so a silent regression reads as "all green". + it('sends the expected incarnation when the host attested one', async () => { + expect(await attachParams({ expectedIncarnationId: 'inc-host-7' })).toMatchObject({ + expectedIncarnationId: 'inc-host-7' + }) + }) + + // A stand-in minted locally when the host reported none. It is first-write-wins and is dropped + // when provider state resets, so the same live shell can present a different one after a + // reconnect — sending it would make the relay refuse the pane its own shell. + it('sends no expected incarnation when the value was synthesized locally', async () => { + expect(await attachParams({ expectedIncarnationId: 'legacy:23:0:pty-7' })).not.toHaveProperty( + 'expectedIncarnationId' + ) + }) + + it('sends no expected incarnation when there is none to send', async () => { + expect(await attachParams()).not.toHaveProperty('expectedIncarnationId') + }) }) diff --git a/src/main/providers/ssh-pty-session-reattach.ts b/src/main/providers/ssh-pty-session-reattach.ts index acd7c2db7b8..e9dbcd8253b 100644 --- a/src/main/providers/ssh-pty-session-reattach.ts +++ b/src/main/providers/ssh-pty-session-reattach.ts @@ -1,5 +1,9 @@ import type { SshChannelMultiplexer } from '../ssh/ssh-channel-multiplexer' -import { isPtyIncarnationId, type PtyIncarnationId } from '../../shared/pty-incarnation' +import { + isPtyIncarnationId, + isRelayAttestedPtyIncarnationId, + type PtyIncarnationId +} from '../../shared/pty-incarnation' import { SSH_PTY_IDENTITY_MISMATCH_ERROR, SSH_SESSION_EXPIRED_ERROR, @@ -197,7 +201,14 @@ export async function reattachSshPtySession(args: { id: relaySessionId, cols: args.options.cols, rows: args.options.rows, - suppressReplayNotification: true + suppressReplayNotification: true, + // The shell's own identity, so it survives a pane moving between tabs — unlike the pane + // identity this replaced. Sent only when the host attested it: a locally synthesized + // stand-in is not stable across reconnects and would refuse the pane its own shell. An + // older relay ignores the field, which leaves today's permissive behaviour. + ...(isRelayAttestedPtyIncarnationId(args.options.expectedIncarnationId) + ? { expectedIncarnationId: args.options.expectedIncarnationId } + : {}) }, installSourceActivation: args.installSourceActivation, rememberPtyIncarnation: args.rememberPtyIncarnation diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 35d11461c53..1673781b3ed 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -42,7 +42,6 @@ import { IMMEDIATE_PTY_EXIT_TIMEOUT_MS, MAX_RELAY_PTY_SESSIONS, PtyHandler, - attachIdentityMismatches, formatNodePtyUnavailableMessage } from './pty-handler' import { RelayDispatcher } from './dispatcher' @@ -1168,6 +1167,68 @@ describe('PtyHandler', () => { expect(mockKill).toHaveBeenCalledWith('SIGKILL') expect(handler.activePtyCount).toBe(0) + }) + + // INVERTED from the deleted `attachIdentityMismatches` suite. That guard compared the paneKey and + // tabId frozen at spawn; it caught a recycled id, but it also refused a pane that had merely moved + // to another tab — and refused as "not found", which the client read as death and answered by + // resuming the agent a second time. The property worth keeping is "a recycled id must not attach"; + // the property that had to go is "identity is where the pane lives". Both are pinned below, now + // against the shell's own incarnation. + it('refuses an attach whose expected incarnation names a different shell', async () => { + const first = await spawnPty({}) + + await expect( + dispatcher.callRequest('pty.attach', { + id: first.id, + expectedIncarnationId: 'incarnation-from-a-previous-relay' + }) + ).rejects.toThrow(/identity mismatch/i) + }) + + // The wording matters as much as the refusal: "not found" is what the client maps to an expired + // session, and expiry authorizes a respawn onto the very shell this branch proved is alive. + it('does not word an incarnation refusal as not-found', async () => { + const first = await spawnPty({}) + + await expect( + dispatcher.callRequest('pty.attach', { + id: first.id, + expectedIncarnationId: 'incarnation-from-a-previous-relay' + }) + ).rejects.toThrow(expect.not.stringMatching(/not found/i)) + }) + + it('attaches when the expected incarnation is the shell it names', async () => { + const first = await spawnPty({}) + + await expect( + attachPty({ id: first.id, expectedIncarnationId: first.incarnationId }) + ).resolves.toMatchObject({ incarnationId: first.incarnationId }) + }) + + // An older client sends no expectation. Refusing there would strand every pane it owns. + it('stays permissive when the caller supplies no expected incarnation', async () => { + const first = await spawnPty({}) + + await expect(attachPty({ id: first.id })).resolves.toMatchObject({ + incarnationId: first.incarnationId + }) + }) + + // The regression the old tab-keyed guard caused. A pane carries its shell to a new tab; the + // relay still holds the paneKey/tabId frozen at spawn. That must not refuse the attach. + it('attaches a pane that has moved to another tab', async () => { + const first = await spawnPty({ paneKey: 'tab-a:0', tabId: 'tab-a' }) + + await expect( + attachPty({ + id: first.id, + expectedIncarnationId: first.incarnationId, + expectedPaneKey: 'tab-b:0', + expectedTabId: 'tab-b' + }) + ).resolves.toMatchObject({ incarnationId: first.incarnationId }) expect(vi.getTimerCount()).toBe(0) }) @@ -1876,13 +1937,19 @@ describe('PtyHandler', () => { expect(spawnOptions.env.ORCA_PANE_KEY).toBeUndefined() expect(spawnOptions.env.ORCA_TAB_ID).toBeUndefined() + // INVERTED: this used to require a differing pane identity to be REFUSED. That refusal is the + // defect — the paneKey and tabId are frozen at spawn, so it also refused a pane that had merely + // moved tabs, and it refused as "not found", which read as death and resumed the agent twice. + // The metadata is still recorded and still kept out of the shell env (asserted above); it just + // no longer gates an attach. A recycled id is rejected on the incarnation instead, which is + // pinned separately. await expect( attachPty({ id: 'pty-1', expectedPaneKey: 'tab-b:leaf-b', expectedTabId: 'tab-b' }) - ).rejects.toThrow('PTY "pty-1" not found') + ).resolves.toEqual({ incarnationId: spawn.incarnationId }) await expect( attachPty({ @@ -3116,13 +3183,17 @@ describe('PtyHandler', () => { expect(callArgs.env.ORCA_PANE_KEY).toBeUndefined() expect(callArgs.env.ORCA_TAB_ID).toBeUndefined() + // INVERTED, same reason as the spawn-side clause: the revived PTY still carries the pane + // identity it was serialized with, and that identity no longer gates an attach. What the + // clause still proves is that revive PRESERVES the metadata and keeps it out of the shell env + // (asserted above) — it just may not refuse a pane for living somewhere else now. await expect( dispatcher.callRequest('pty.attach', { id: 'pty-1', expectedPaneKey: 'tab-other:leaf', expectedTabId: 'tab-other' }) - ).rejects.toThrow('PTY "pty-1" not found') + ).resolves.toMatchObject({ incarnationId: expect.any(String) }) }) it('invokes the exit listener with the spawn-time paneKey', async () => { @@ -3593,39 +3664,3 @@ describe('PtyHandler', () => { expect(handler.pendingPtyCreationCount).toBe(0) }) }) - -describe('attachIdentityMismatches', () => { - it('rejects a paneKey collision across relay generations', () => { - // Old lease expects tab-a's pane; the reset relay's pty-1 belongs to tab-b. - expect( - attachIdentityMismatches({ paneKey: 'tab-a:0' }, { paneKey: 'tab-b:0', tabId: 'tab-b' }) - ).toBe(true) - }) - - it('rejects a tabId collision when only tab identity is known', () => { - expect(attachIdentityMismatches({ tabId: 'tab-a' }, { tabId: 'tab-b' })).toBe(true) - }) - - it('accepts a matching identity', () => { - expect( - attachIdentityMismatches( - { paneKey: 'tab-a:0', tabId: 'tab-a' }, - { paneKey: 'tab-a:0', tabId: 'tab-a' } - ) - ).toBe(false) - }) - - it('stays permissive when the caller supplies no identity', () => { - expect(attachIdentityMismatches({}, { paneKey: 'tab-a:0', tabId: 'tab-a' })).toBe(false) - }) - - it('stays permissive when the managed PTY predates identity capture', () => { - expect(attachIdentityMismatches({ paneKey: 'tab-a:0', tabId: 'tab-a' }, {})).toBe(false) - }) - - it('does not reject on tabId when paneKey matches (split panes share a tab)', () => { - expect( - attachIdentityMismatches({ paneKey: 'tab-a:1' }, { paneKey: 'tab-a:1', tabId: 'tab-a' }) - ).toBe(false) - }) -}) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index c8b39c0f285..10c2126b670 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -329,17 +329,13 @@ export type PtyExitListener = (event: { id: string; paneKey?: string }) => void type PtyIdentity = { paneKey?: string; tabId?: string } -/** - * True when a reattach's expected pane identity contradicts the target PTY's own. - * Rejects cross-relay-generation id collisions (a reset relay reuses `pty-N`). - * Only compares fields present on both sides; absent identity stays permissive. - */ -export function attachIdentityMismatches(expected: PtyIdentity, managed: PtyIdentity): boolean { - return Boolean( - (expected.paneKey && managed.paneKey && expected.paneKey !== managed.paneKey) || - (expected.tabId && managed.tabId && expected.tabId !== managed.tabId) - ) -} +/* Why there is no attach-time pane-identity comparison here any more: it keyed on the paneKey and + * tabId frozen at spawn, so moving a pane to another tab made the relay refuse its own live shell, + * and refuse by saying "not found" — which the client read as death and answered by resuming the + * agent a second time. Cross-generation id collisions, the thing it was actually for, are rejected + * by the incarnation comparison in `attach` instead: that is the shell's own identity, so it + * follows the pane wherever it moves. `attachIdentity` is still recorded and still round-trips + * through serialize/revive; it just no longer gates an attach. */ /** Returns env to merge into the PTY's spawn env. Receives spawn context so augmenters can derive per-PTY identity from paneKey. * `command` is the renderer-chosen agent launch command (`pi`, `omp`, …); undefined for CLI-launched bare shells. */ export type PtyEnvAugmenter = (ctx: { @@ -1611,16 +1607,18 @@ export class PtyHandler { throw new Error(`PTY "${id}" not found`) } - // Why: generation resets can reuse PTY IDs; reject conflicting identities. - const mismatch = attachIdentityMismatches( - { - paneKey: typeof params.expectedPaneKey === 'string' ? params.expectedPaneKey : undefined, - tabId: typeof params.expectedTabId === 'string' ? params.expectedTabId : undefined - }, - managed.attachIdentity ?? { paneKey: managed.paneKey, tabId: managed.tabId } - ) - if (mismatch) { - throw new Error(`PTY "${id}" not found (identity mismatch)`) + // Why: a reset relay restarts its ids at pty-1, so an id alone can name somebody else's shell. + // The incarnation is the shell's OWN identity, so it discriminates a recycled id without + // caring where the pane lives. The pane identity this replaced froze the TAB at spawn, so a + // pane moved to another tab was refused — and refused as "not found", which read as death and + // resumed the agent a second time. Absent expectation stays permissive: an older client sends + // none, and refusing there would strand every pane it owns. + const expectedIncarnationId = + typeof params.expectedIncarnationId === 'string' ? params.expectedIncarnationId : undefined + if (expectedIncarnationId && expectedIncarnationId !== managed.incarnationId) { + // Deliberately NOT worded "not found": that phrasing is what the client maps to an expired + // session, and expiry authorizes a respawn onto a shell this branch just proved is alive. + throw new Error(`PTY "${id}" identity mismatch`) } managed.startupIngress?.snapshotBarrier()