From ccb082740fc20d7985e72d1dd2bfffd2f52e1bbd Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 7 Aug 2026 18:30:50 -0700 Subject: [PATCH] fix(ssh): let the durable pane binding outrank recency when retiring leases Choosing the newest lease for a pane is wrong whenever a newer lease exists that no pane is bound to: it retires the lease the pane is actually attached to, detaching a live terminal instead of healing it. Two changes. Arbitration now prefers the lease matching the pane's durable binding, across both the SSH-target and local partitions, falling back to recency only when no binding names either candidate. And supersession at upsert time now defers rather than expiring a bound predecessor. When a lease arrives for a pane that is still bound to a different PTY, the binding has not caught up yet, so both stay live and reattach arbitrates once the binding is available. Co-authored-by: Orca --- ...-docker-reconnect-pane-cardinality-e2e.mjs | 41 +++++ package.json | 1 + src/main/persistence.ts | 54 +++++- .../ssh-reattach-pane-cardinality.test.ts | 66 +++++-- .../helpers/docker-ssh-relay-remote-ptys.ts | 87 +++++++++ .../ssh-reconnect-pane-cardinality.spec.ts | 166 ++++++++++++++++++ 6 files changed, 404 insertions(+), 11 deletions(-) create mode 100644 config/scripts/run-ssh-docker-reconnect-pane-cardinality-e2e.mjs create mode 100644 tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts create mode 100644 tests/e2e/ssh-reconnect-pane-cardinality.spec.ts diff --git a/config/scripts/run-ssh-docker-reconnect-pane-cardinality-e2e.mjs b/config/scripts/run-ssh-docker-reconnect-pane-cardinality-e2e.mjs new file mode 100644 index 00000000000..cde82379c1f --- /dev/null +++ b/config/scripts/run-ssh-docker-reconnect-pane-cardinality-e2e.mjs @@ -0,0 +1,41 @@ +import { spawnSync } from 'node:child_process' + +const rawExtraArgs = process.argv.slice(2) +const extraArgs = rawExtraArgs[0] === '--' ? rawExtraArgs.slice(1) : rawExtraArgs +const pnpm = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm' +const env = { + ...process.env, + ORCA_E2E_SSH_DOCKER: '1' +} + +// Why: Node's CVE-2024-27980 hardening rejects .cmd spawns without shell on Windows. +const spawnOptions = { + stdio: 'inherit', + env, + shell: process.platform === 'win32' +} + +const runtime = spawnSync(pnpm, ['run', 'ensure:electron-runtime'], spawnOptions) + +if (runtime.status !== 0) { + process.exit(runtime.status ?? 1) +} + +const result = spawnSync( + pnpm, + [ + 'exec', + 'playwright', + 'test', + 'tests/e2e/ssh-reconnect-pane-cardinality.spec.ts', + '--config', + 'tests/playwright.config.ts', + '--project', + 'electron-headless', + '--workers=1', + ...extraArgs + ], + spawnOptions +) + +process.exit(result.status ?? 1) diff --git a/package.json b/package.json index 726a5081dc4..2386940877c 100644 --- a/package.json +++ b/package.json @@ -97,6 +97,7 @@ "test:e2e:ssh-docker-perf": "node config/scripts/run-ssh-docker-perf-e2e.mjs", "test:e2e:ssh-docker-watcher-isolation": "node config/scripts/run-ssh-docker-watcher-isolation-e2e.mjs", "test:e2e:ssh-docker-terminal-parking": "node config/scripts/run-ssh-docker-terminal-parking-e2e.mjs", + "test:e2e:ssh-docker-reconnect-pane-cardinality": "node config/scripts/run-ssh-docker-reconnect-pane-cardinality-e2e.mjs", "test:e2e:nested-runtime-ssh": "node config/scripts/run-nested-runtime-ssh-e2e.mjs", "test:e2e:source-control-scale": "pnpm run ensure:electron-runtime && npx playwright test tests/e2e/source-control-large-file-count.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1", "win-update-e2e": "node tests/tools/win-update-e2e/run.mjs", diff --git a/src/main/persistence.ts b/src/main/persistence.ts index a30848b75b4..98c5bda8e28 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -7223,6 +7223,14 @@ export class Store { if (winner.state === 'terminated' || winner.state === 'expired') { return } + // Why consult the binding here: at upsert time the caller's lease may not be + // the one the pane is bound to yet. Expiring the bound predecessor would + // detach a live pane, so leave both live and let reattach arbitrate with the + // binding in hand. + const boundPtyId = this.durablyBoundPtyIdForPane(winner.targetId, winner.tabId, winner.leafId) + if (boundPtyId && boundPtyId !== winner.ptyId) { + return + } const superseded: SshRemotePtyLease[] = [] for (const lease of this.state.sshRemotePtyLeases ?? []) { if ( @@ -7252,6 +7260,50 @@ export class Store { * * Returns the number of leases retired, for logging. */ + /** The PTY a pane is durably bound to, across the target and local partitions. */ + private durablyBoundPtyIdForPane( + targetId: string, + tabId: string, + leafId: string + ): string | undefined { + for (const session of [ + this.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)], + this.state.workspaceSession + ]) { + const boundPtyId = session?.terminalLayoutsByTabId?.[tabId]?.ptyIdsByLeafId?.[leafId] + if (boundPtyId) { + return this.getRelayPtyIdForSshLeaseComparison(targetId, boundPtyId) + } + } + return undefined + } + + /** + * The durable pane binding outranks recency. Picking the newest lease alone + * would retire the one the pane is actually bound to whenever a newer unbound + * lease exists, which detaches a live pane instead of healing it. + */ + private outranksForPane( + candidate: SshRemotePtyLease, + incumbent: SshRemotePtyLease, + targetId: string + ): boolean { + const boundPtyId = this.durablyBoundPtyIdForPane( + targetId, + candidate.tabId ?? '', + candidate.leafId ?? '' + ) + if (boundPtyId) { + if (incumbent.ptyId === boundPtyId) { + return false + } + if (candidate.ptyId === boundPtyId) { + return true + } + } + return isNewerSshRemotePtyLease(candidate, incumbent) + } + supersedeDuplicatePaneLeases(targetId: string): number { const live = (this.state.sshRemotePtyLeases ?? []).filter( (lease) => @@ -7264,7 +7316,7 @@ export class Store { } const paneKey = [lease.worktreeId, lease.tabId, lease.leafId].join('\0') const incumbent = winnerByPane.get(paneKey) - if (!incumbent || isNewerSshRemotePtyLease(lease, incumbent)) { + if (!incumbent || this.outranksForPane(lease, incumbent, targetId)) { winnerByPane.set(paneKey, lease) } } diff --git a/src/main/ssh-reattach-pane-cardinality.test.ts b/src/main/ssh-reattach-pane-cardinality.test.ts index bc7c23c4fd5..2ea3cb10b49 100644 --- a/src/main/ssh-reattach-pane-cardinality.test.ts +++ b/src/main/ssh-reattach-pane-cardinality.test.ts @@ -103,24 +103,50 @@ describe('STA-3077: one pane owns at most one live remote PTY lease', () => { }) }) -describe('STA-3077: superseding a lease scrubs only the predecessor binding', () => { - it('drops the stale pane binding without disturbing the successor', async () => { - const store = await createStore() - - // Spawn-shaped setup: the creating branches are intentionally allowed here. +describe('STA-3077: superseding respects the durable pane binding', () => { + // Spawn-shaped setup: the creating branches are intentionally allowed here. + function bindPaneTo(store: Awaited>, ptyId: string) { store.persistPtyBinding({ worktreeId: WORKTREE, tabId: TAB, leafId: LEAF, - ptyId: 'relay-pty-a', - incarnationId: 'inc-a' + ptyId, + incarnationId: `inc-${ptyId}` }) + } + + it('scrubs the predecessor binding when the arriving lease is the bound one', async () => { + const store = await createStore() + bindPaneTo(store, 'relay-pty-a') + store.upsertSshRemotePtyLease(leaseFor('relay-pty-a', 1)) + bindPaneTo(store, 'relay-pty-b') + store.upsertSshRemotePtyLease(leaseFor('relay-pty-b', 2)) + + expect(liveLeasesForPane(store).map((lease) => lease.ptyId)).toEqual(['relay-pty-b']) + const layout = store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB] + expect(layout?.ptyIdsByLeafId?.[LEAF]).toBe('relay-pty-b') + }) + + // Expiring the bound predecessor here would detach a live pane, so both stay + // live and reattach arbitrates with the binding in hand. + it('defers instead of expiring the lease the pane is bound to', async () => { + const store = await createStore() + bindPaneTo(store, 'relay-pty-a') store.upsertSshRemotePtyLease(leaseFor('relay-pty-a', 1)) store.upsertSshRemotePtyLease(leaseFor('relay-pty-b', 2)) - const layout = store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB] - expect(layout?.ptyIdsByLeafId?.[LEAF]).not.toBe('relay-pty-a') - expect(liveLeasesForPane(store).map((lease) => lease.ptyId)).toEqual(['relay-pty-b']) + expect( + liveLeasesForPane(store) + .map((lease) => lease.ptyId) + .sort() + ).toEqual(['relay-pty-a', 'relay-pty-b']) + expect(store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]).toBe( + 'relay-pty-a' + ) + + // Arbitration keeps the bound lease, not the newer one. + expect(store.supersedeDuplicatePaneLeases(TARGET)).toBe(1) + expect(liveLeasesForPane(store).map((lease) => lease.ptyId)).toEqual(['relay-pty-a']) }) }) @@ -141,6 +167,26 @@ describe('STA-3077: existing duplicate leases are healed, not revived', () => { expect(liveLeasesForPane(store).map((lease) => lease.ptyId)).toEqual(['relay-pty-19']) }) + // Recency alone would retire the lease the pane is actually bound to whenever a + // newer unbound lease exists, detaching a live pane instead of healing it. + it('keeps the durably bound lease even when an unbound one is newer', async () => { + const store = await createStore() + store.persistPtyBinding({ + worktreeId: WORKTREE, + tabId: TAB, + leafId: LEAF, + ptyId: 'relay-pty-bound', + incarnationId: 'inc-bound' + }) + store.upsertSshRemotePtyLease({ ...leaseFor('relay-pty-bound', 1), createdAt: 1 }) + // Arrives later but no pane is bound to it. + store.upsertSshRemotePtyLease({ ...leaseFor('relay-pty-newer', 99), createdAt: 99 }) + + store.supersedeDuplicatePaneLeases(TARGET) + + expect(liveLeasesForPane(store).map((lease) => lease.ptyId)).toEqual(['relay-pty-bound']) + }) + it('leaves distinct panes alone', async () => { const otherLeaf = '8a2b4c6d-1e3f-4a5b-8c7d-9e0f1a2b3c4d' const store = await createStore({ diff --git a/tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts b/tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts new file mode 100644 index 00000000000..6b227185ae6 --- /dev/null +++ b/tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts @@ -0,0 +1,87 @@ +import { execDockerSshRelayTargetCommand, type DockerSshRelayTarget } from './docker-ssh-relay-target' + +/** One live remote shell the relay is hosting on the Docker target. */ +export type DockerSshRelayRemotePty = { + pid: number + relayPid: number + pts: string + paneKey: string | null + tabId: string | null + worktreeId: string | null +} + +// A remote PTY is a direct child of a detached relay whose stdin is a pts — +// node-pty's forkpty child. That excludes relay-watcher.js and every sshd +// session, so the count is exactly "shells the relay is hosting right now". +const LIST_RELAY_REMOTE_PTYS_COMMAND = ` +relay_pids=() +for proc in /proc/[0-9]*; do + [ -r "$proc/cmdline" ] || continue + argv=() + mapfile -d '' -t argv < "$proc/cmdline" 2>/dev/null || continue + [ "\${argv[1]##*/}" = relay.js ] || continue + case " \${argv[*]:2} " in *" --detached "*) relay_pids+=("\${proc##*/}") ;; esac +done +[ \${#relay_pids[@]} -gt 0 ] || exit 0 +for proc in /proc/[0-9]*; do + [ -r "$proc/status" ] || continue + ppid="$(awk '/^PPid:/{print $2}' "$proc/status" 2>/dev/null)" + [ -n "$ppid" ] || continue + matched= + for relay in "\${relay_pids[@]}"; do + if [ "$relay" = "$ppid" ]; then matched=1; break; fi + done + [ -n "$matched" ] || continue + pts="$(readlink "$proc/fd/0" 2>/dev/null)" + case "$pts" in /dev/pts/*) ;; *) continue ;; esac + pane=; tab=; worktree= + while IFS= read -r -d '' entry; do + case "$entry" in + ORCA_PANE_KEY=*) pane="\${entry#ORCA_PANE_KEY=}" ;; + ORCA_TAB_ID=*) tab="\${entry#ORCA_TAB_ID=}" ;; + ORCA_WORKTREE_ID=*) worktree="\${entry#ORCA_WORKTREE_ID=}" ;; + esac + done < "$proc/environ" + printf '%s\\t%s\\t%s\\t%s\\t%s\\t%s\\n' "\${proc##*/}" "$ppid" "$pts" "\${pane:--}" "\${tab:--}" "\${worktree:--}" +done +` + +function optional(value: string | undefined): string | null { + return value === undefined || value === '' || value === '-' ? null : value +} + +export function readDockerSshRelayRemotePtys( + target: DockerSshRelayTarget +): DockerSshRelayRemotePty[] { + const output = execDockerSshRelayTargetCommand(target, LIST_RELAY_REMOTE_PTYS_COMMAND) + if (!output) { + return [] + } + return output + .split('\n') + .map((line) => { + const [rawPid, rawRelayPid, pts, paneKey, tabId, worktreeId] = line.split('\t') + const pid = Number(rawPid) + const relayPid = Number(rawRelayPid) + // Why: a vanished /proc entry must throw so expect.poll retries rather + // than folding a half-read row into the census as pid 0. + if (!Number.isInteger(pid) || !Number.isInteger(relayPid) || !pts?.startsWith('/dev/pts/')) { + throw new Error(`Unexpected Docker SSH relay remote PTY row: ${line}`) + } + return { + pid, + relayPid, + pts, + paneKey: optional(paneKey), + tabId: optional(tabId), + worktreeId: optional(worktreeId) + } + }) + .sort((left, right) => left.pid - right.pid) +} + +export function describeDockerSshRelayRemotePtys(ptys: DockerSshRelayRemotePty[]): string { + return ptys + .map((pty) => `${pty.pid}@${pty.relayPid} ${pty.pts} pane=${pty.paneKey ?? '-'}`) + .join(', ') +} diff --git a/tests/e2e/ssh-reconnect-pane-cardinality.spec.ts b/tests/e2e/ssh-reconnect-pane-cardinality.spec.ts new file mode 100644 index 00000000000..7bed171c890 --- /dev/null +++ b/tests/e2e/ssh-reconnect-pane-cardinality.spec.ts @@ -0,0 +1,166 @@ +import type { Page, TestInfo } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' +import { + splitActiveTerminalPane, + waitForActivePanePtyId, + waitForActiveTerminalManager, + waitForPaneIdentitySnapshot +} from './helpers/terminal' +import { + cleanupDockerSshRelayTarget, + startDockerSshRelayTarget, + type DockerSshRelayTarget +} from './helpers/docker-ssh-relay-target' +import { + connectDockerSshRelayTarget, + reconnectDockerSshRelayTarget +} from './helpers/docker-ssh-relay-connection' +import { + describeDockerSshRelayRemotePtys, + readDockerSshRelayRemotePtys +} from './helpers/docker-ssh-relay-remote-ptys' + +const RUN_DOCKER_SSH = process.env.ORCA_E2E_SSH_DOCKER === '1' +const PANE_COUNT = 2 +const RECONNECT_CYCLES = 3 +// Why: the relay must outlive every disconnect. If it exits with the client the +// remote shells die too, reconnect degrades to a cold spawn, and the reattach +// path this spec exists to bound is never entered. +const RELAY_GRACE_PERIOD_SECONDS = 900 + +test.use({ seedTestRepo: false }) + +/** Every terminal pane the user can see in a workspace, keyed tab/leaf. */ +type RemotePaneCensus = { + tabIds: string[] + paneIds: string[] +} + +async function readRemotePaneCensus(page: Page, worktreeId: string): Promise { + return page.evaluate((worktreeId) => { + const state = window.__store?.getState() + if (!state) { + throw new Error('Store unavailable') + } + type LayoutNode = + | { type: 'leaf'; leafId: string } + | { type: 'split'; first: LayoutNode; second: LayoutNode } + | null + const collectLeafIds = (node: LayoutNode): string[] => { + if (!node) { + return [] + } + return node.type === 'leaf' + ? [node.leafId] + : [...collectLeafIds(node.first), ...collectLeafIds(node.second)] + } + const tabs = state.tabsByWorktree[worktreeId] ?? [] + const paneIds = tabs.flatMap((tab) => { + const leafIds = collectLeafIds( + (state.terminalLayoutsByTabId[tab.id]?.root ?? null) as LayoutNode + ) + // Why: `root: null` is the implicit single-pane layout a fresh tab carries + // until it is first split, so it still counts as one visible pane. + return leafIds.length > 0 + ? leafIds.map((leafId) => `${tab.id}/${leafId}`) + : [`${tab.id}/`] + }) + return { tabIds: tabs.map((tab) => tab.id), paneIds: paneIds.sort() } + }, worktreeId) +} + +// STA-3077: reconnecting an SSH-backed workspace must be cardinality-neutral. +// The report had relay PTYs go 2 -> 19 -> 20 over three reconnects while panes +// the user never opened appeared alongside them, so both counts are asserted +// against the same fixed workspace after every cycle. +test.describe('SSH reconnect pane and remote PTY cardinality', () => { + test.skip(!RUN_DOCKER_SSH, 'Set ORCA_E2E_SSH_DOCKER=1 to run Docker-backed SSH tests.') + test.skip(process.platform === 'win32', 'Docker SSH reconnect uses POSIX SSH tooling.') + + test('adds no panes and no remote PTYs across repeated reconnects', async ({ + orcaPage + }, testInfo: TestInfo) => { + test.setTimeout(480_000) + let target: DockerSshRelayTarget | null = null + try { + target = startDockerSshRelayTarget(testInfo) + const relayTarget = target + await waitForSessionReady(orcaPage) + const remote = await connectDockerSshRelayTarget(orcaPage, relayTarget, { + relayGracePeriodSeconds: RELAY_GRACE_PERIOD_SECONDS + }) + await expect + .poll(() => waitForActiveWorktree(orcaPage), { timeout: 30_000 }) + .toBe(remote.worktreeId) + await ensureTerminalVisible(orcaPage, 45_000) + await waitForActiveTerminalManager(orcaPage, 60_000) + await waitForActivePanePtyId(orcaPage, 60_000) + + await splitActiveTerminalPane(orcaPage, 'vertical') + await waitForPaneIdentitySnapshot(orcaPage, PANE_COUNT) + const baselinePanes = await readRemotePaneCensus(orcaPage, remote.worktreeId) + expect(baselinePanes.paneIds).toHaveLength(PANE_COUNT) + + // The remote census is the reporter's oracle: shells the relay hosts right + // now, counted on the container rather than inferred from app state. + await expect + .poll(() => readDockerSshRelayRemotePtys(relayTarget).length, { + timeout: 60_000, + message: 'remote shells did not settle at one per pane before the first reconnect' + }) + .toBe(PANE_COUNT) + const baselineRemotePtys = readDockerSshRelayRemotePtys(relayTarget) + const baselinePids = baselineRemotePtys.map((pty) => pty.pid) + testInfo.annotations.push({ + type: 'ssh-reconnect-cardinality-baseline', + description: describeDockerSshRelayRemotePtys(baselineRemotePtys) + }) + + for (let cycle = 1; cycle <= RECONNECT_CYCLES; cycle += 1) { + await reconnectDockerSshRelayTarget(orcaPage, remote.targetId) + await ensureTerminalVisible(orcaPage, 45_000) + await waitForActiveTerminalManager(orcaPage, 60_000) + await waitForActivePanePtyId(orcaPage, 60_000) + + // Poll rather than sample once: a leaked pane or a duplicate shell can + // land after reattach reports ready, and a single read would miss it. + await expect + .poll(() => readDockerSshRelayRemotePtys(relayTarget).map((pty) => pty.pid), { + timeout: 90_000, + message: `reconnect ${cycle} changed the live remote shells` + }) + .toEqual(baselinePids) + await expect + .poll(async () => (await readRemotePaneCensus(orcaPage, remote.worktreeId)).paneIds, { + timeout: 30_000, + message: `reconnect ${cycle} changed the visible terminal panes` + }) + .toEqual(baselinePanes.paneIds) + expect( + (await readRemotePaneCensus(orcaPage, remote.worktreeId)).tabIds, + `reconnect ${cycle} changed the workspace tabs` + ).toEqual(baselinePanes.tabIds) + + // Settle before the next cycle so a late graft is attributed to the + // reconnect that caused it instead of leaking into the next assertion. + await expect + .poll(() => readDockerSshRelayRemotePtys(relayTarget).map((pty) => pty.pid), { + intervals: [2_000, 2_000, 2_000], + timeout: 8_000, + message: `reconnect ${cycle} grew the remote shells after settling` + }) + .toEqual(baselinePids) + } + + testInfo.annotations.push({ + type: 'ssh-reconnect-cardinality-final', + description: `${RECONNECT_CYCLES} reconnects: ${describeDockerSshRelayRemotePtys( + readDockerSshRelayRemotePtys(relayTarget) + )}` + }) + } finally { + cleanupDockerSshRelayTarget(target) + } + }) +})