diff --git a/src/main/persistence-async-write-syscalls.test.ts b/src/main/persistence-async-write-syscalls.test.ts index 3a4c1f11e87..282dc1e2988 100644 --- a/src/main/persistence-async-write-syscalls.test.ts +++ b/src/main/persistence-async-write-syscalls.test.ts @@ -821,7 +821,9 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { expect(persisted.sshRemotePtyLeases).toEqual( expect.arrayContaining([ expect.objectContaining({ ptyId: 'pty-1', state: 'attached' }), - expect.objectContaining({ ptyId: 'pty-2', state: 'expired' }), + // An id-qualified reattach named this pty and succeeded, which is the one thing that can + // settle what `expired` meant: the client had lost its route, not that the shell died. + expect.objectContaining({ ptyId: 'pty-2', state: 'attached' }), expect.objectContaining({ ptyId: 'pty-3', state: 'detached' }), expect.objectContaining({ ptyId: 'pty-4', state: 'terminated' }) ]) diff --git a/src/main/persistence-ssh-lease-reattach-reclaim.test.ts b/src/main/persistence-ssh-lease-reattach-reclaim.test.ts new file mode 100644 index 00000000000..9d6c74f710e --- /dev/null +++ b/src/main/persistence-ssh-lease-reattach-reclaim.test.ts @@ -0,0 +1,83 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createStore, testState } from './persistence-test-harness' +import { sshRemotePtyLeaseAllowsReattach } from '../shared/ssh-types' + +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { isEncryptionAvailable: () => false } +})) + +vi.mock('./telemetry/client', () => ({ track: vi.fn() })) +vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: () => ({}) })) + +/** + * `expired` records that the CLIENT lost its route, so a reattach that named the pty and succeeded + * is the only evidence that can settle which of "orphan" or "corpse" it was. Without the edge back + * to `attached`, a lease that proved itself alive stayed `expired` for good and every sweep keyed + * on that state — `ssh:reset`, `ssh:terminateSessions`, the quit-time `detached` mark, supersession + * — silently skipped a running remote shell. + */ +describe('ssh remote pty lease reclaim after a proven reattach', () => { + beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-')) + }) + afterEach(() => { + rmSync(testState.dir, { recursive: true, force: true }) + }) + + it('reclaims an expired lease that the relay reattached, clearing its route-retirement marks', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) + store.markSshRemotePtyLease('ssh-1', 'pty-1', 'expired') + // A predecessor mark left over from a supersession this lease has now outlived. + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'pty-1', + state: 'expired', + supersededBy: 'pty-2' + }) + + await store.markSshRemotePtyLeasesAttachedAsync('ssh-1', ['pty-1']) + + const [lease] = store.getSshRemotePtyLeases('ssh-1') + expect(lease).toMatchObject({ ptyId: 'pty-1', state: 'attached' }) + expect(lease).not.toHaveProperty('supersededBy') + expect(lease).not.toHaveProperty('relayIdRecycled') + expect(sshRemotePtyLeaseAllowsReattach(lease)).toBe(true) + }) + + it('leaves a terminated lease absorbing even when the id appears in a reattach batch', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) + store.markSshRemotePtyLease('ssh-1', 'pty-1', 'terminated') + + await store.markSshRemotePtyLeasesAttachedAsync('ssh-1', ['pty-1']) + + expect(store.getSshRemotePtyLeases('ssh-1')[0]).toMatchObject({ state: 'terminated' }) + }) + + it('does not revive an expired lease from an unqualified bulk attach', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) + store.markSshRemotePtyLease('ssh-1', 'pty-1', 'expired') + + store.markSshRemotePtyLeases('ssh-1', 'attached') + + // Only the id-qualified caller carries per-pty proof; a target-wide mark does not. + expect(store.getSshRemotePtyLeases('ssh-1')[0]).toMatchObject({ state: 'expired' }) + }) + + it('lets a reclaimed lease be swept as detached at quit', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) + store.markSshRemotePtyLease('ssh-1', 'pty-1', 'expired') + await store.markSshRemotePtyLeasesAttachedAsync('ssh-1', ['pty-1']) + + store.markSshRemotePtyLeases('ssh-1', 'detached') + + expect(store.getSshRemotePtyLeases('ssh-1')[0]).toMatchObject({ state: 'detached' }) + }) +}) diff --git a/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts b/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts index 70760df71f4..46db8c4f0c7 100644 --- a/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts +++ b/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts @@ -189,13 +189,25 @@ function updateSshRemotePtyLeaseStates( if (lease.targetId !== targetId || (ptyIds && !ptyIds.has(lease.ptyId))) { continue } - if (state === 'attached' && (lease.state === 'terminated' || lease.state === 'expired')) { + if (state === 'attached' && lease.state === 'terminated') { + continue + } + // `expired` says the CLIENT lost its route, never that the shell died - and a reattach that + // named this exact pty and succeeded is the one thing that can settle which it was. Without + // this edge a lease that proved itself alive stayed `expired` for good, which silently exempted + // a running remote shell from `ssh:reset`, from the SSH_TERMINATE_RECONNECT_REQUIRED fence in + // `ssh:terminateSessions`, and from the quit-time `detached` sweep, and left it unable to win + // supersession so its own successors never retired their predecessors. + // Only the id-qualified caller (`markSshRemotePtyLeasesAttachedAsync`, fed by the relay's + // `attachedLeaseIds`) carries that proof; a bulk mark over a whole target does not. + if (state === 'attached' && lease.state === 'expired' && !ptyIds) { continue } if (state === 'detached' && lease.state !== 'attached') { continue } if (lease.state !== state) { + const reclaimed = state === 'attached' && lease.state === 'expired' lease.state = state lease.updatedAt = now if (state === 'attached') { @@ -203,6 +215,13 @@ function updateSshRemotePtyLeaseStates( } else if (state === 'detached') { lease.lastDetachedAt = now } + if (reclaimed) { + // Route retirement belongs to the shell that lost the pane. This lease just proved it is + // that shell, so `attached` may never carry a supersession mark - the same invariant + // `upsertSshRemotePtyLease` enforces when an id is claimed live again. + delete lease.supersededBy + delete lease.relayIdRecycled + } changed = true } if (shouldClearBindings) { diff --git a/src/main/runtime/orca-runtime-mark-pty-liveness-unverifiable.ts b/src/main/runtime/orca-runtime-mark-pty-liveness-unverifiable.ts index 50fae68e6da..548c2e4a0bc 100644 --- a/src/main/runtime/orca-runtime-mark-pty-liveness-unverifiable.ts +++ b/src/main/runtime/orca-runtime-mark-pty-liveness-unverifiable.ts @@ -39,7 +39,13 @@ export class OrcaRuntimeWithMarkPtyLivenessUnverifiable extends OrcaRuntimeWithO return this.stopRequestedPtyIds.has(ptyId) } - /** Null when nothing has been observed either way, so callers keep their own default. */ + /** + * Null when this register holds no defensible claim — a never-asked host, a fresh app start, or + * an absence observation too weak to name (a relay that answered but does not know the id: see + * the inventory sweep and handlePtyReattachFailure). It is NOT a death certificate, so a caller + * authorizing a kill must fail closed on it; a caller that only has this evidence to work with, + * like terminal.recoverPane, refuses on the positive verdicts instead. + */ getPtyLivenessVerdict(ptyId: string): PtyLivenessVerdict | null { return this.ptyLivenessVerdictByPtyId.get(ptyId)?.verdict ?? null } @@ -92,11 +98,9 @@ export class OrcaRuntimeWithMarkPtyLivenessUnverifiable extends OrcaRuntimeWithO } protected rememberPtyLivenessVerdict(ptyId: string, verdict: PtyLivenessVerdict): void { - if (verdict.status === 'exited') { - // An earned death certificate ends the question; nothing left to remember. - this.ptyLivenessVerdictByPtyId.delete(ptyId) - return - } + // An earned death certificate is KEPT, not dropped, so the register is three-valued on disk as + // well as in the type. Its only writer is a host-delivered exit frame; nothing weaker may + // reach it (docs/reference/ssh-execution-boundary.md). this.ptyLivenessVerdictByPtyId.delete(ptyId) this.ptyLivenessObservationSequence += 1 this.ptyLivenessVerdictByPtyId.set(ptyId, { diff --git a/src/main/runtime/orca-runtime-on-pty-exit.ts b/src/main/runtime/orca-runtime-on-pty-exit.ts index 07f8d4fbbd4..58fc9d6a1e7 100644 --- a/src/main/runtime/orca-runtime-on-pty-exit.ts +++ b/src/main/runtime/orca-runtime-on-pty-exit.ts @@ -202,7 +202,10 @@ export class OrcaRuntimeWithOnPtyExit extends OrcaRuntimeWithOnClientDisconnecte pty.lastExitCode = exitCode pty.lastExitCause = exitCause if (exitCode >= 0 || options.hostExitConfirmed === true) { - this.forgetPtyLivenessVerdict(ptyId) + // Record the certificate rather than merely dropping the doubt: a reader that has to + // authorize a respawn cannot distinguish "the host reported this process gone" from "this + // runtime has never asked" if both are absence. + this.rememberPtyLivenessVerdict(ptyId, { status: 'exited' }) } // Why: the exited process's live frames say nothing about a replacement. // A same-id respawn makes the leaf writable again before any new title, diff --git a/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts b/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts index f9b16f7d7ed..041cfd53257 100644 --- a/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts +++ b/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts @@ -9,9 +9,12 @@ import type { import { headlessBrowserTabsUnchanged } from './mobile-session-browser-equality' import { appendBrowserTabOrder } from './mobile-session-browser-group-projection' import { parseAppSshPtyId, toComparableRelaySshPtyId } from '../../shared/ssh-pty-id' +import { toSshExecutionHostId } from '../../shared/execution-host' +import { parsePaneKey } from '../../shared/stable-pane-id' import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' import type { RuntimeStore } from './runtime-store-contract' import { SSH_PANE_RECOVERY_GRACE_MS } from './orca-runtime-core' +import { findTerminalTabIdForLeaf } from './workspace-session-terminal-membership-authority' export class OrcaRuntimeWithReconcileHeadlessMobileSessionBrowserTabs extends OrcaRuntimeWithHydrateHeadlessMobileSessionTabsFromWorkspaceSession { // Why: keep an existing snapshot's browser tabs in sync with the live bridge @@ -92,6 +95,38 @@ export class OrcaRuntimeWithReconcileHeadlessMobileSessionBrowserTabs extends Or }) } + /** + * The tab this leaf sits in NOW. Only the leaf half of a pane key is remint-stable: a lease + * freezes its tabId at write time and `detachTerminalPaneToTab` moves a live pane, so the stored + * tabId names the tab the pane LEFT. Matching a lease on it is wrong in both directions - it + * accepts the coordinates the pane abandoned (recovering a pane that already moved on, which + * binds one leaf in two tabs and orphans the PTY under the new one) and refuses the correct ones. + * Same resolution `restoreReattachedPtyRuntime` already does for its own reattach fence. + * + * Both workspace partitions are read because SSH spawns bind panes into `ssh:` while + * reattach binds into `local`; consulting one would report "nowhere" for a pane the other holds. + */ + protected findCurrentTerminalTabIdForLeaf(targetId: string, leafId: string): string | undefined { + for (const leaf of this.leaves.values()) { + if (leaf.leafId === leafId) { + return leaf.tabId + } + } + for (const pty of this.ptysById.values()) { + const parsed = parsePaneKey(pty.paneKey ?? '') + if (parsed?.leafId === leafId) { + return parsed.tabId + } + } + return ( + findTerminalTabIdForLeaf(this.store?.getWorkspaceSession?.(), leafId) ?? + findTerminalTabIdForLeaf( + this.store?.getWorkspaceSession?.(toSshExecutionHostId(targetId)), + leafId + ) + ) + } + protected getRecentExpiredSshLease( worktreeId: string, tabId: string, @@ -100,11 +135,17 @@ export class OrcaRuntimeWithReconcileHeadlessMobileSessionBrowserTabs extends Or ): ReturnType>[number] | null { const now = Date.now() return ( - this.store?.getSshRemotePtyLeases?.().find( - (lease) => - lease.state === 'expired' && - lease.worktreeId === worktreeId && - lease.tabId === tabId && + this.store?.getSshRemotePtyLeases?.().find((lease) => { + if (lease.state !== 'expired' || lease.worktreeId !== worktreeId) { + return false + } + // Leaf is the pane's identity; the frozen tabId is only trustworthy while nothing else can + // say where the leaf actually lives. + const currentTabId = lease.leafId + ? this.findCurrentTerminalTabIdForLeaf(lease.targetId, lease.leafId) + : undefined + return ( + (currentTabId ?? lease.tabId) === tabId && // Leases store RELAY form (`toStoredPtyId` -> `toRelaySshPtyId`); the runtime hands us // the APP form (`ssh:@@pty-3`). A raw `===` therefore never held for an SSH // pane, which is what kept this reader's only ptyId-qualified caller inert. @@ -113,7 +154,8 @@ export class OrcaRuntimeWithReconcileHeadlessMobileSessionBrowserTabs extends Or (leafId === undefined || lease.leafId === undefined || lease.leafId === leafId) && lease.updatedAt <= now && now - lease.updatedAt <= SSH_PANE_RECOVERY_GRACE_MS - ) ?? null + ) + }) ?? null ) } diff --git a/src/main/runtime/orca-runtime-refresh-pty-worktree-records-with-controller-inventory.ts b/src/main/runtime/orca-runtime-refresh-pty-worktree-records-with-controller-inventory.ts index 3e3114d4b7a..5b2dcd71f19 100644 --- a/src/main/runtime/orca-runtime-refresh-pty-worktree-records-with-controller-inventory.ts +++ b/src/main/runtime/orca-runtime-refresh-pty-worktree-records-with-controller-inventory.ts @@ -287,6 +287,10 @@ export class OrcaRuntimeWithRefreshPtyWorktreeRecordsWithControllerInventory ext // clears `connected` for every one of its PTYs at once. Only `false` here // is an observed absence; `null` means no provider could be asked. if (observed === false) { + // Drops the doubt without asserting a death: `pty.listProcesses` returns the relay's + // CURRENT session map, so a restarted relay omits every id the previous one minted + // whether or not those shells died. That is the same union as pty.attach's not-found, + // and neither earns `exited` (docs/reference/ssh-execution-boundary.md). this.forgetPtyLivenessVerdict(pty.ptyId) } else if (observed === null && this.isSshOwnedPtyId(pty.ptyId)) { this.markPtyLivenessUnverifiable(pty.ptyId, NO_OBSERVING_PROVIDER_REASON) diff --git a/src/main/runtime/orca-runtime-resolve-terminal-pane.ts b/src/main/runtime/orca-runtime-resolve-terminal-pane.ts index 6ac8fb54975..6d0dd931b77 100644 --- a/src/main/runtime/orca-runtime-resolve-terminal-pane.ts +++ b/src/main/runtime/orca-runtime-resolve-terminal-pane.ts @@ -101,8 +101,15 @@ export class OrcaRuntimeWithResolveTerminalPane extends OrcaRuntimeWithGetTermin // above). `!pty.connected` is the same inference: one dropped relay clears it for every PTY it // owned. So the pair can hold over a remote shell that is still running, and createTerminal // would rebind the pane away from it, leaving the original orphaned and its agent duplicated. - // The runtime already grades this: a host-confirmed exit leaves no verdict, while lost contact - // is recorded `unverifiable` (docs/reference/ssh-execution-boundary.md, shared/pty-liveness-verdict.ts). + // The runtime grades that: `live` is the host proving the shell survived, `unverifiable` is the + // client losing contact, and both refuse. What this gate must NOT require is a positive + // `exited`: the only answer that ever reaches it is a reachable relay reporting it has no such + // id, and that is a union — pty.attach throws not-found for an unknown id with no liveness + // check, and a relay restart makes every previously minted id unknown. No writer of + // `exited` co-occurs with a reattachable `expired` lease either, since a host-delivered exit + // frame tombstones the lease `terminated`. Demanding one would close this gate permanently, and + // an unrecoverable pane is its own failure (docs/reference/ssh-execution-boundary.md, + // shared/pty-liveness-verdict.ts). const liveness = this.getPtyLivenessVerdict(pty.ptyId) if (liveness?.status === 'unverifiable' || liveness?.status === 'live') { throw new Error('terminal_not_recoverable') diff --git a/src/main/runtime/orca-runtime-tests/terminal-handles-part-02.spec.ts b/src/main/runtime/orca-runtime-tests/terminal-handles-part-02.spec.ts index 1edcf81c414..438ec34118e 100644 --- a/src/main/runtime/orca-runtime-tests/terminal-handles-part-02.spec.ts +++ b/src/main/runtime/orca-runtime-tests/terminal-handles-part-02.spec.ts @@ -1,6 +1,12 @@ -import { describe, expect, it } from 'vitest' -import { TEST_WINDOW_ID, TEST_WORKTREE_ID, createRuntime } from '../orca-runtime-test-fixtures.spec' -import '../orca-runtime-test-mocks.spec' +import { describe, expect, it, vi } from 'vitest' +import { + HEADLESS_LEAF_ID, + TEST_WINDOW_ID, + TEST_WORKTREE_ID, + createRuntime, + createRuntimeWithSshLease +} from '../orca-runtime-test-fixtures.spec' +import { makePaneKey } from '../orca-runtime-test-mocks.spec' describe('OrcaRuntimeService', () => { it('invalidates a re-keyed leaf-unique handle so in-flight waiters fail fast', async () => { @@ -143,4 +149,76 @@ describe('OrcaRuntimeService', () => { await waiting expect(settled).toBe('rejected') }) + it('recovers a moved SSH pane through the tab it now sits in, not the one its lease froze', async () => { + // `detachTerminalPaneToTab` moves a live pane and the lease keeps naming the tab it LEFT. + // Matching on that frozen tabId refused the pane's real coordinates outright, so a moved pane + // could only ever be "recovered" through coordinates it had already abandoned. + const leaseTabId = 'tab-before-move' + const currentTabId = 'tab-after-move' + const appPtyId = 'ssh:ssh-target@@pty-8' + const runtime = createRuntimeWithSshLease(appPtyId, leaseTabId) + const paneKey = makePaneKey(currentTabId, HEADLESS_LEAF_ID) + runtime.registerPty(appPtyId, TEST_WORKTREE_ID, 'ssh-target', { + tabId: currentTabId, + leafId: HEADLESS_LEAF_ID + }) + const handle = runtime.resolveTerminalPane(paneKey, TEST_WORKTREE_ID).handle + runtime.onPtyExit(appPtyId, -1, undefined, { hostExitConfirmed: true }) + const createTerminal = vi.spyOn(runtime, 'createTerminal').mockResolvedValue({ + handle: 'term-replacement', + tabId: currentTabId, + paneKey, + ptyId: 'pty-replacement', + worktreeId: TEST_WORKTREE_ID, + title: null, + surface: 'background' + }) + + await expect( + runtime.recoverTerminalPane(paneKey, TEST_WORKTREE_ID, handle) + ).resolves.toMatchObject({ handle: 'term-replacement' }) + expect(createTerminal).toHaveBeenCalledWith(`id:${TEST_WORKTREE_ID}`, { + tabId: currentTabId, + leafId: HEADLESS_LEAF_ID, + focus: false + }) + }) + + it('refuses a moved SSH pane addressed through the tab it left', async () => { + // The other half: a viewer on a stale mirror asks under the OLD tab, whose layout no longer + // holds this leaf. Accepting it binds one leaf in two tabs and leaves the PTY under the current + // tab orphaned with its agent still running. The handle CAS happens to refuse first here, so + // the lease resolver is asserted directly - it is the independent second refusal. + const leaseTabId = 'tab-stale-mirror' + const currentTabId = 'tab-moved-to' + const appPtyId = 'ssh:ssh-target@@pty-9' + const runtime = createRuntimeWithSshLease(appPtyId, leaseTabId) + const stalePaneKey = makePaneKey(leaseTabId, HEADLESS_LEAF_ID) + runtime.registerPty(appPtyId, TEST_WORKTREE_ID, 'ssh-target', { + tabId: leaseTabId, + leafId: HEADLESS_LEAF_ID + }) + const staleHandle = runtime.resolveTerminalPane(stalePaneKey, TEST_WORKTREE_ID).handle + // The move: same leaf, same PTY, new tab. + runtime.registerPty(appPtyId, TEST_WORKTREE_ID, 'ssh-target', { + tabId: currentTabId, + leafId: HEADLESS_LEAF_ID + }) + runtime.onPtyExit(appPtyId, -1, undefined, { hostExitConfirmed: true }) + const createTerminal = vi.spyOn(runtime, 'createTerminal') + + await expect( + runtime.recoverTerminalPane(stalePaneKey, TEST_WORKTREE_ID, staleHandle) + ).rejects.toThrow(/terminal_not_recoverable|terminal_not_found/) + const leases = runtime as unknown as { + getRecentExpiredSshLease: (worktreeId: string, tabId: string, leafId?: string) => unknown + } + expect( + leases.getRecentExpiredSshLease(TEST_WORKTREE_ID, leaseTabId, HEADLESS_LEAF_ID) + ).toBeNull() + expect( + leases.getRecentExpiredSshLease(TEST_WORKTREE_ID, currentTabId, HEADLESS_LEAF_ID) + ).not.toBeNull() + expect(createTerminal).not.toHaveBeenCalled() + }) }) diff --git a/src/main/runtime/orca-runtime-tests/terminal-handles.spec.ts b/src/main/runtime/orca-runtime-tests/terminal-handles.spec.ts index 359abb07f63..4ae933afef4 100644 --- a/src/main/runtime/orca-runtime-tests/terminal-handles.spec.ts +++ b/src/main/runtime/orca-runtime-tests/terminal-handles.spec.ts @@ -624,11 +624,14 @@ describe('OrcaRuntimeService', () => { }) it('does not recreate a shell for an expired lease whose PTY liveness is unverifiable', async () => { - // The production sequence this guards: a relay reattach fails, so ssh-relay-session marks the - // lease 'expired' AND sends a synthetic pty:exit code -1. Neither observed the process — every - // writer of 'expired' documents it as "the client lost its route", and code -1 with no host - // confirmation is recorded 'unverifiable'. Spawning a replacement there rebinds the pane away - // from a remote shell still running on the host and duplicates its agent. + // The production sequence this guards: the relay delivers an exit frame carrying code -1 for an + // SSH pane with no host confirmation (`preservesAbnormalSshSurface`), while the pane's lease is + // already 'expired'. Neither observed the process — every writer of 'expired' documents it as + // "the client lost its route", and code -1 with no host confirmation is recorded + // 'unverifiable'. Spawning a replacement there rebinds the pane away from a remote shell still + // running on the host and duplicates its agent. This is the `unverifiable` arm only; the + // relay's own absence branch (`handlePtyReattachFailure`) reaches the runtime with no verdict + // at all, and is covered by the relay-disowned case in terminal-handles-part-02.spec.ts. const tabId = 'tab-unverifiable' const ptyId = 'ssh:ssh-target@@pty-3' const runtime = createRuntimeWithSshLease(ptyId, tabId) diff --git a/src/main/runtime/pty-inventory-liveness-verdict.test.ts b/src/main/runtime/pty-inventory-liveness-verdict.test.ts index 38e3dda20eb..e5c31f66afa 100644 --- a/src/main/runtime/pty-inventory-liveness-verdict.test.ts +++ b/src/main/runtime/pty-inventory-liveness-verdict.test.ts @@ -89,7 +89,9 @@ describe('inventory sweep liveness verdicts', () => { runtime.onPtyExit(REMOTE_PTY_ID, -1, undefined, { hostExitConfirmed: true }) - expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toBeNull() + // A host-delivered exit frame is the one signal that observes the process, so it both clears + // the lost-contact doubt and is retained as the certificate itself. + expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toEqual({ status: 'exited' }) }) it('records lost contact when no provider can answer for the PTY', async () => { @@ -112,6 +114,23 @@ describe('inventory sweep liveness verdicts', () => { expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toBeNull() }) + it('records no death certificate when a listing of the owning host omits the PTY', async () => { + // The host answered and named a sibling on the same relay, so this is the strongest absence the + // inventory can report — and it is still not a certificate. `pty.listProcesses` returns the + // relay's CURRENT session map, so a relay that restarted omits every id the previous one minted + // (ids are `pty2::` with a fresh epoch per relay start) whether or not those + // shells ever died. Recording `exited` here would only relocate the fabrication that + // handlePtyReattachFailure was corrected for (docs/reference/ssh-execution-boundary.md). + const runtime = makeRuntimeMissingFromInventory( + () => false, + vi.fn(async () => [{ id: 'ssh:conn-1@@relay-sibling', worktreeId: WORKTREE_ID }]) + ) + + await runtime.listTerminals(`id:${WORKTREE_ID}`) + + expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toBeNull() + }) + it('clears lost-contact doubt when reconnect inventory observes the PTY live', async () => { let reconnected = false const runtime = makeRuntimeMissingFromInventory( diff --git a/src/main/runtime/terminal-pane-recovery-liveness-gate.test.ts b/src/main/runtime/terminal-pane-recovery-liveness-gate.test.ts new file mode 100644 index 00000000000..441d04a960b --- /dev/null +++ b/src/main/runtime/terminal-pane-recovery-liveness-gate.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it, vi } from 'vitest' +import { + HEADLESS_LEAF_ID, + TEST_WORKTREE_ID, + createRuntimeWithSshLease +} from './orca-runtime-test-fixtures.spec' +import { makePaneKey } from './orca-runtime-test-mocks.spec' + +// What `terminal.recoverPane` may and may not treat as authority to spawn a replacement shell over +// a remote pane. Lives in a `.test.ts` rather than beside the other recoverPane cases in +// orca-runtime-tests/*.spec.ts because config/vitest.config.ts — the config CI runs — includes only +// `*.test.ts`, so a ratchet placed there would never execute. + +describe('terminal.recoverPane liveness gate', () => { + it('recreates a shell for an SSH pane the relay disowned, the only evidence this gate ever gets', async () => { + // The whole production route into recoverPane, end to end. A reachable relay answered for this + // exact id and did not name it — via pty.attach in handlePtyReattachFailure, or the identical + // answer in the inventory listing below — and that answer is a union: pty.attach throws + // not-found for an unknown id with no liveness check, and pty.listProcesses returns only + // the CURRENT session map, which after a relay restart omits every previously minted id. So no + // `exited` certificate exists to demand here, and no writer of one co-occurs with a + // reattachable `expired` lease: a host-delivered exit frame tombstones the lease `terminated` + // instead. Requiring `exited` therefore closes this gate permanently + // (docs/reference/ssh-execution-boundary.md). + const tabId = 'tab-relay-disowned' + const ptyId = 'ssh:ssh-target@@pty-10' + const runtime = createRuntimeWithSshLease(ptyId, tabId) + const paneKey = makePaneKey(tabId, HEADLESS_LEAF_ID) + runtime.setPtyController({ + write: () => true, + kill: () => true, + hasPty: (id: string) => id !== ptyId, + // A sibling on the same relay still reports, so the host itself answered this listing. + listProcesses: async () => [ + { id: 'ssh:ssh-target@@pty-sibling', worktreeId: TEST_WORKTREE_ID } + ], + getForegroundProcess: async () => null + } as never) + runtime.registerPty(ptyId, TEST_WORKTREE_ID, 'ssh-target', { tabId, leafId: HEADLESS_LEAF_ID }) + const handle = runtime.resolveTerminalPane(paneKey, TEST_WORKTREE_ID).handle + // The sweep is what disconnects the pane: handlePtyReattachFailure tells only the renderer and + // the reattach spawn path only expires the lease, so nothing else reaches the runtime record. + await runtime.listTerminals(`id:${TEST_WORKTREE_ID}`) + expect(runtime.getPtyLivenessVerdict(ptyId)).toBeNull() + const createTerminal = vi.spyOn(runtime, 'createTerminal').mockResolvedValue({ + handle: 'term-replacement', + tabId, + paneKey, + ptyId: 'pty-replacement', + worktreeId: TEST_WORKTREE_ID, + title: null, + surface: 'background' + }) + + await expect( + runtime.recoverTerminalPane(paneKey, TEST_WORKTREE_ID, handle) + ).resolves.toMatchObject({ handle: 'term-replacement' }) + expect(createTerminal).toHaveBeenCalledOnce() + }) + + it('refuses to recreate a shell for an expired lease the host proved is still live', async () => { + // The production writer: reattach SUCCEEDED and then persistPtyBinding refused the surface, so + // ssh-relay-session records `live` before writing `expired`. `expired` alone would read as + // "reattach gave up", and spawning here would put a second agent on a transcript the host just + // proved is still running. + const tabId = 'tab-proved-live' + const ptyId = 'ssh:ssh-target@@pty-11' + const runtime = createRuntimeWithSshLease(ptyId, tabId) + const paneKey = makePaneKey(tabId, HEADLESS_LEAF_ID) + runtime.registerPty(ptyId, TEST_WORKTREE_ID, 'ssh-target', { tabId, leafId: HEADLESS_LEAF_ID }) + const handle = runtime.resolveTerminalPane(paneKey, TEST_WORKTREE_ID).handle + runtime.onPtyExit(ptyId, -1) + runtime.markPtyLivenessLive(ptyId) + const createTerminal = vi.spyOn(runtime, 'createTerminal') + + await expect(runtime.recoverTerminalPane(paneKey, TEST_WORKTREE_ID, handle)).rejects.toThrow( + 'terminal_not_recoverable' + ) + expect(createTerminal).not.toHaveBeenCalled() + }) + + it('refuses to recreate a shell for an expired lease whose PTY liveness is unverifiable', async () => { + // The relay delivers an exit frame carrying code -1 for an SSH pane with no host confirmation + // (`preservesAbnormalSshSurface`) while the lease is already 'expired'. Neither observed the + // process, and spawning here rebinds the pane away from a shell still running on the host. + const tabId = 'tab-unverifiable-gate' + const ptyId = 'ssh:ssh-target@@pty-12' + const runtime = createRuntimeWithSshLease(ptyId, tabId) + const paneKey = makePaneKey(tabId, HEADLESS_LEAF_ID) + runtime.registerPty(ptyId, TEST_WORKTREE_ID, 'ssh-target', { tabId, leafId: HEADLESS_LEAF_ID }) + const handle = runtime.resolveTerminalPane(paneKey, TEST_WORKTREE_ID).handle + runtime.onPtyExit(ptyId, -1) + expect(runtime.getPtyLivenessVerdict(ptyId)?.status).toBe('unverifiable') + const createTerminal = vi.spyOn(runtime, 'createTerminal') + + await expect(runtime.recoverTerminalPane(paneKey, TEST_WORKTREE_ID, handle)).rejects.toThrow( + 'terminal_not_recoverable' + ) + expect(createTerminal).not.toHaveBeenCalled() + }) + + it('still recreates a shell for an SSH pane whose host attested the exit', async () => { + // The negative control on the other side: a host-delivered exit frame is a real certificate, so + // the pane a paired client asks about must still get a replacement. + const tabId = 'tab-attested-gate' + const ptyId = 'ssh:ssh-target@@pty-13' + const runtime = createRuntimeWithSshLease(ptyId, tabId) + const paneKey = makePaneKey(tabId, HEADLESS_LEAF_ID) + runtime.registerPty(ptyId, TEST_WORKTREE_ID, 'ssh-target', { tabId, leafId: HEADLESS_LEAF_ID }) + const handle = runtime.resolveTerminalPane(paneKey, TEST_WORKTREE_ID).handle + runtime.onPtyExit(ptyId, -1, undefined, { hostExitConfirmed: true }) + expect(runtime.getPtyLivenessVerdict(ptyId)?.status).toBe('exited') + const createTerminal = vi.spyOn(runtime, 'createTerminal').mockResolvedValue({ + handle: 'term-replacement', + tabId, + paneKey, + ptyId: 'pty-replacement', + worktreeId: TEST_WORKTREE_ID, + title: null, + surface: 'background' + }) + + await expect( + runtime.recoverTerminalPane(paneKey, TEST_WORKTREE_ID, handle) + ).resolves.toMatchObject({ handle: 'term-replacement' }) + expect(createTerminal).toHaveBeenCalledOnce() + }) +}) diff --git a/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts b/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts index 139e92364b0..f0a7093c61e 100644 --- a/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts +++ b/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts @@ -128,6 +128,7 @@ describe('SshRelaySession abandoned remote PTYs', () => { deps: ReturnType shutdown: ReturnType attachForReconnect: ReturnType + runtime: { onPtyExit: ReturnType; registerPty: ReturnType } }> { const deps = createMockDeps() const shutdown = vi.fn().mockResolvedValue(undefined) @@ -140,27 +141,30 @@ describe('SshRelaySession abandoned remote PTYs', () => { vi.mocked(deps.mockStore.getSshRemotePtyLeases).mockReturnValue([detachedLease()] as ReturnType< typeof deps.mockStore.getSshRemotePtyLeases >) + const runtime = { onPtyExit: vi.fn(), registerPty: vi.fn() } const session = new SshRelaySession( 'target-1', deps.getMainWindow, deps.mockStore, - deps.mockPortForward + deps.mockPortForward, + runtime as never ) await session.establish(deps.mockConn) - return { deps, shutdown, attachForReconnect } + return { deps, shutdown, attachForReconnect, runtime } } 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( + const { deps, shutdown, attachForReconnect, runtime } = await establishWithFailingReattach( new Error('PTY reattach attempt timed out after 15000ms') ) expect(attachForReconnect).toHaveBeenCalled() expect(shutdown).not.toHaveBeenCalled() + expect(runtime.onPtyExit).not.toHaveBeenCalled() expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( 'target-1', 'pty-live', @@ -173,12 +177,13 @@ describe('SshRelaySession abandoned remote PTYs', () => { 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( + const { deps, shutdown, attachForReconnect, runtime } = await establishWithFailingReattach( new Error('PTY "pty-live" not found (identity mismatch)') ) expect(attachForReconnect).toHaveBeenCalled() expect(shutdown).not.toHaveBeenCalled() + expect(runtime.onPtyExit).not.toHaveBeenCalled() expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( 'target-1', 'pty-live', @@ -192,11 +197,14 @@ describe('SshRelaySession abandoned remote PTYs', () => { // pane — respawning leaks the old process rather than killing it — but a restarted relay // answers the same way for ids it never minted, so this is not an `exited` verdict // (docs/reference/ssh-execution-boundary.md). - const { deps, shutdown } = await establishWithFailingReattach( + const { deps, shutdown, runtime } = await establishWithFailingReattach( new Error('PTY "pty-live" not found') ) expect(shutdown).not.toHaveBeenCalled() + // The runtime is where a death certificate would land (`hostExitConfirmed`), and this branch + // holds no evidence to write one from. + expect(runtime.onPtyExit).not.toHaveBeenCalled() // 'expired' records that reattach gave up on the id, not that the shell died; ssh:terminateSessions still reaches it. expect(deps.mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( 'target-1', diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 46a8152e0c8..bca3632b83b 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -2900,6 +2900,12 @@ export class SshRelaySession { ) clearProviderPtyState(appPtyId) deletePtyOwnership(appPtyId) + // Deliberately does NOT call runtime.onPtyExit: pty.attach answers not-found both when it + // verified the pid is dead and when its session map simply has no such id (no liveness check on + // that path at all) — which is every id after a relay restart, since ids carry a per-start + // `ptyIdMintEpoch`. This branch may release the id, but certifying a death from that union + // would orphan a live remote shell (docs/reference/ssh-execution-boundary.md). The renderer + // gets code -1, which every reader treats as unverified loss. this.store.markSshRemotePtyLease(this.targetId, ptyId, 'expired') const win = this.getMainWindow() if (win && !win.isDestroyed()) { diff --git a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-expired-pane-recovery.test.ts b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-expired-pane-recovery.test.ts index a7494317a24..b6f3b9802e7 100644 --- a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-expired-pane-recovery.test.ts +++ b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-expired-pane-recovery.test.ts @@ -132,6 +132,45 @@ describe('createRemoteRuntimePtyTransport', () => { expect(onError).not.toHaveBeenCalled() }) + it('does not recover a pane whose relay reply was an identity mismatch', async () => { + // The mismatch suffix means the relay found a LIVE PTY under that id owned by ANOTHER pane, so + // it is evidence of presence, not absence. This transport is the only caller of + // terminal.recoverPane, so a bare SSH_SESSION_EXPIRED substring test here put a second agent on + // one transcript even though main already refuses the respawn on the same reply. + const onError = vi.fn() + resolvedPaneHandle = 'terminal-mismatch' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const transport = createRemoteRuntimePtyTransport('hub-env', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1' + }) + transport.attach({ + existingPtyId: 'remote:hub-env@@terminal-mismatch', + callbacks: { onError } + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + runtimeCall.mockClear() + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { + type: 'error', + streamId: latestSubscribePayload().streamId, + message: 'SSH_SESSION_EXPIRED: pty-1 SSH_PTY_IDENTITY_MISMATCH' + } + }) + + await vi.waitFor(() => expect(onError).toHaveBeenCalled()) + expect(runtimeCall).not.toHaveBeenCalledWith( + expect.objectContaining({ method: 'terminal.recoverPane' }) + ) + expect(runtimeCall).not.toHaveBeenCalledWith( + expect.objectContaining({ method: 'terminal.create' }) + ) + expect(transport.getPtyId()).toBe('remote:hub-env@@terminal-mismatch') + }) + it('fails closed when an older HUB cannot recover an expired SSH pane', async () => { const onError = vi.fn() resolvedPaneHandle = 'terminal-expired' diff --git a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts index b1bd7336c86..83aca477308 100644 --- a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts @@ -32,6 +32,7 @@ import type { PtyTransportRecoveryState } from './pty-transport-types' import { createPtyOutputProcessor } from './pty-transport' +import { isSshSessionGoneError } from './pty-connection/pty-connect-limits' import { RuntimeRpcCallError, unwrapRuntimeRpcResult } from '../../runtime/runtime-rpc-client' import { getRemoteRuntimePtyEnvironmentId, @@ -120,7 +121,6 @@ type RemoteAgentSessionLaunchResult = | RuntimeEnsureAgentSessionResult | RuntimeCreateAgentSessionResult | { terminal: RuntimeTerminalCreate; disposition?: undefined } -const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED' function isRemoteTerminalStaleMessage(message: string): boolean { return message.includes('terminal_handle_stale') @@ -1652,8 +1652,12 @@ export function createRemoteRuntimePtyTransport( retireRemoteTerminalId() return } - if (message.includes(SSH_SESSION_EXPIRED_ERROR)) { - // Why: only the HUB may replace its expired SSH pane; a paired viewer must never fall back to client-local SSH. + if (isSshSessionGoneError(message)) { + // Why: only the HUB may replace its expired SSH pane; a paired viewer must never fall back to + // client-local SSH. The identity-mismatch suffix is excluded because it means the opposite — + // the relay found a LIVE PTY under that id owned by another pane — and this is the one + // transport that actually calls terminal.recoverPane, so a bare substring test here spawned + // a second agent onto one transcript. recoverExpiredHostPane() return }