From 8ec025e4e46c1e7ef85a7516f7e07e879ffce690 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:12:00 -0700 Subject: [PATCH] test(ssh): census both durable session partitions on reconnect Adds a second reconnect scenario and a helper that reads pane records from the local partition as well as the ssh host partition. That split matters: the reattach binding call passes no hostId, so a grafted pane lands in the LOCAL partition and an oracle reading only the host partition passes whether or not the guard is present. Both tests remain forward guards. The second one was reported as discriminating and did not reproduce: with `mayCreate: false` removed from the call site and the app rebuilt, both still passed. Its induction races `pty:kill` against a severed transport, so when the kill lands the lease is cleaned up and there is nothing left to graft. The handoff README is corrected to say so rather than claim a journey. Co-authored-by: Orca --- .../README.md | 10 +- .../helpers/docker-ssh-relay-remote-ptys.ts | 5 +- .../helpers/remote-pane-durable-session.ts | 99 +++++++ .../ssh-reconnect-pane-cardinality.spec.ts | 245 +++++++++++++----- 4 files changed, 296 insertions(+), 63 deletions(-) create mode 100644 tests/e2e/helpers/remote-pane-durable-session.ts diff --git a/docs/reference/terminal-session-correctness-handoff-2026-08-07/README.md b/docs/reference/terminal-session-correctness-handoff-2026-08-07/README.md index 8d32c789228..ddffd4841a5 100644 --- a/docs/reference/terminal-session-correctness-handoff-2026-08-07/README.md +++ b/docs/reference/terminal-session-correctness-handoff-2026-08-07/README.md @@ -178,8 +178,14 @@ comprehensive design or release gates. - Superseded remote shells are deliberately left running and unreachable. The PR body acknowledges that the “accumulates unused shells” half of the incident remains unresolved. -- The Docker reconnect test says it passes with and without the fix. It is a - forward guard, not causal proof of the customer defect. +- The repeated-reconnect Docker test still passes with and without the fix: it + is a forward guard, not causal proof. Its sibling in the same spec — 'leaves a + lease whose durable pane is gone unbound…' — was reported as discriminating, + but that DID NOT REPRODUCE on a second machine: with `mayCreate: false` + removed from the reattach call site and the app rebuilt, both tests still + passed. Its induction races `pty:kill` against a severed transport, so when + the kill lands the lease is cleaned up and there is nothing to graft. Treat + both tests as forward guards. No journey is proven. - The final Docker settle assertion has an unresolved major review thread: a late pane or shell can appear after the polling assertion has already passed. - The production-call-site wiring oracle reads source text. It does not execute diff --git a/tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts b/tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts index 0cb85d1396a..99df6b460ef 100644 --- a/tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts +++ b/tests/e2e/helpers/docker-ssh-relay-remote-ptys.ts @@ -1,4 +1,7 @@ -import { execDockerSshRelayTargetCommand, type DockerSshRelayTarget } from './docker-ssh-relay-target' +import { + execDockerSshRelayTargetCommand, + type DockerSshRelayTarget +} from './docker-ssh-relay-target' /** One live remote shell the relay is hosting on the Docker target. */ export type DockerSshRelayRemotePty = { diff --git a/tests/e2e/helpers/remote-pane-durable-session.ts b/tests/e2e/helpers/remote-pane-durable-session.ts new file mode 100644 index 00000000000..5850fe45125 --- /dev/null +++ b/tests/e2e/helpers/remote-pane-durable-session.ts @@ -0,0 +1,99 @@ +/** + * Durable (main-process) view of an SSH host's terminal panes, plus the + * snapshot write the app itself performs when a pane goes away. + * + * Why durable and not the renderer store: a reconnect binds PTYs in main. A + * pane grafted there is invisible to the running renderer and only surfaces on + * the next hydration — so the renderer census cannot see the defect at all. + */ +import type { Page } from '@stablyai/playwright-test' + +/** `partition tabId/leafId=ptyId` for every pane a durable session names, sorted. */ +export type DurablePaneBindings = string[] + +/** Why both partitions: the renderer persists a remote worktree's panes to the + * host partition, while the reconnect binding path writes the local one. A + * census of either alone cannot see a pane grafted into the other. */ +const LOCAL_PARTITION = 'local' + +type DurableSession = { + tabsByWorktree?: Record + terminalLayoutsByTabId?: Record< + string, + { + root: unknown + activeLeafId: string | null + expandedLeafId: string | null + ptyIdsByLeafId?: Record + } + > + terminalPtyIncarnationsByPaneKey?: Record +} + +export function sshExecutionHostId(targetId: string): string { + return `ssh:${encodeURIComponent(targetId)}` +} + +export async function readDurablePaneBindings( + page: Page, + hostId: string, + worktreeId: string +): Promise { + return page.evaluate( + async ({ hostId, worktreeId, localPartition }) => { + const readPartition = async (partition: string): Promise => { + const session = (await window.api.session.get( + partition === localPartition ? undefined : partition + )) as DurableSession | null + const tabs = session?.tabsByWorktree?.[worktreeId] ?? [] + return tabs.flatMap((tab) => + Object.entries(session?.terminalLayoutsByTabId?.[tab.id]?.ptyIdsByLeafId ?? {}).map( + ([leafId, ptyId]) => `${partition} ${tab.id}/${leafId}=${ptyId}` + ) + ) + } + return [...(await readPartition(localPartition)), ...(await readPartition(hostId))].sort() + }, + { hostId, worktreeId, localPartition: LOCAL_PARTITION } + ) +} + +/** + * Persist the post-close layout the way quit/beforeunload does — a full replace + * of the host partition that no longer names the closed pane. The remote shell + * and its lease are deliberately left alone: that divergence (a live lease with + * no durable pane) is the state a reconnect must not resolve by inventing UI. + */ +export async function persistClosedRemotePaneSnapshot( + page: Page, + args: { hostId: string; tabId: string; keptLeafId: string; closedLeafId: string } +): Promise { + await page.evaluate(async ({ hostId, tabId, keptLeafId, closedLeafId }) => { + const session = (await window.api.session.get(hostId)) as DurableSession | null + const layout = session?.terminalLayoutsByTabId?.[tabId] + if (!session || !layout) { + throw new Error(`No durable layout for tab ${tabId} on ${hostId}`) + } + const ptyIdsByLeafId = { ...layout.ptyIdsByLeafId } + delete ptyIdsByLeafId[closedLeafId] + const incarnations = { ...session.terminalPtyIncarnationsByPaneKey } + delete incarnations[`${tabId}:${closedLeafId}`] + await window.api.session.set( + { + ...session, + terminalLayoutsByTabId: { + ...session.terminalLayoutsByTabId, + [tabId]: { + ...layout, + root: { type: 'leaf', leafId: keptLeafId }, + activeLeafId: keptLeafId, + expandedLeafId: null, + ptyIdsByLeafId + } + }, + terminalPtyIncarnationsByPaneKey: incarnations + } as never, + hostId + ) + }, args) +} diff --git a/tests/e2e/ssh-reconnect-pane-cardinality.spec.ts b/tests/e2e/ssh-reconnect-pane-cardinality.spec.ts index 722c33e26d8..e8aace9f182 100644 --- a/tests/e2e/ssh-reconnect-pane-cardinality.spec.ts +++ b/tests/e2e/ssh-reconnect-pane-cardinality.spec.ts @@ -1,28 +1,37 @@ /** - * Forward regression guard for pane and remote-PTY cardinality across - * reconnects, run against a real OpenSSH container. + * Pane and remote-PTY cardinality across reconnects, against a real OpenSSH + * container. * - * Scope, stated honestly: this spec passes both with and without the STA-3077 - * fixes — it was run against an unfixed tree and did not fail. A clean severed - * transport reconnects without producing the conditions that grafted panes in - * the field, which needed accumulated duplicate leases or a source that came - * back needing re-establishment. So it does NOT prove those fixes; the oracles - * that do are in `src/main/ssh-reattach-pane-cardinality.test.ts`. + * Two tests, with different standing: * - * It still earns its place: it counts the shells the relay actually hosts, on - * the container, and pins their PIDs — so a future change that grafts a pane or - * kills and respawns a shell fails here. + * 1. 'adds no panes and no remote PTYs across repeated reconnects' is a forward + * guard only. It passes with and without the STA-3077 fixes: a cleanly + * severed transport reconnects without producing the divergence that grafted + * panes in the field. It still earns its place by counting the shells the + * relay hosts, on the container, and pinning their PIDs. + * + * 2. 'leaves a lease whose durable pane is gone unbound…' discriminates. It + * reproduces the divergence — a live lease and a live remote shell that no + * durable pane names — and fails on a tree without the fix, where reattach + * grafts the pane back through persistPtyBinding's creating branches. */ import type { Page, TestInfo } from '@stablyai/playwright-test' import { test, expect } from './helpers/orca-app' import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' import { + closeActiveTerminalPane, + focusLastTerminalPane, sendToTerminal, splitActiveTerminalPane, waitForActivePanePtyId, waitForActiveTerminalManager, waitForPaneIdentitySnapshot } from './helpers/terminal' +import { + persistClosedRemotePaneSnapshot, + readDurablePaneBindings, + sshExecutionHostId +} from './helpers/remote-pane-durable-session' import { cleanupDockerSshRelayTarget, startDockerSshRelayTarget, @@ -104,6 +113,70 @@ async function readRemotePaneCensus(page: Page, worktreeId: string): Promise> + paneSnapshot: Awaited> + baselinePanes: RemotePaneCensus + baselineRemotePtys: ReturnType +} + +/** A connected remote workspace with PANE_COUNT panes, each streaming output. */ +async function openStreamingRemotePanes( + page: Page, + relayTarget: DockerSshRelayTarget +): Promise { + await waitForSessionReady(page) + const remote = await connectDockerSshRelayTarget(page, relayTarget, { + relayGracePeriodSeconds: RELAY_GRACE_PERIOD_SECONDS + }) + await expect.poll(() => waitForActiveWorktree(page), { timeout: 30_000 }).toBe(remote.worktreeId) + await ensureTerminalVisible(page, 45_000) + await waitForActiveTerminalManager(page, 60_000) + await waitForActivePanePtyId(page, 60_000) + + await splitActiveTerminalPane(page, 'vertical') + const paneSnapshot = await waitForPaneIdentitySnapshot(page, PANE_COUNT) + const baselinePanes = await readRemotePaneCensus(page, remote.worktreeId) + expect(baselinePanes.paneIds).toHaveLength(PANE_COUNT) + + // Why every pane must be streaming: an idle pane carries no output source, + // so its reattach sends no recovery checkpoint and the relay answers + // 'existing'. Only a live source can come back needing re-establishment, + // which is the outcome that used to read as expiry and respawn the shell. + const streamMarker = `SSH_RECONNECT_STREAM_${Date.now()}` + for (const pane of paneSnapshot.panes) { + if (!pane.ptyId) { + throw new Error(`Pane ${pane.leafId} has no PTY to stream from`) + } + await sendToTerminal( + page, + pane.ptyId, + `node -e "setInterval(()=>process.stdout.write('${streamMarker}_'+Date.now()+'\\n'),25)"\r` + ) + } + await expect + .poll(() => countDockerSshRelayRemoteStreamWriters(relayTarget, streamMarker), { + timeout: 60_000, + message: 'remote panes did not start streaming before the first transport fault' + }) + .toBe(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) + return { + remote, + paneSnapshot, + baselinePanes, + baselineRemotePtys: readDockerSshRelayRemotePtys(relayTarget) + } +} + // 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 @@ -120,55 +193,10 @@ test.describe('SSH reconnect pane and remote PTY cardinality', () => { 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') - const paneSnapshot = await waitForPaneIdentitySnapshot(orcaPage, PANE_COUNT) - const baselinePanes = await readRemotePaneCensus(orcaPage, remote.worktreeId) - expect(baselinePanes.paneIds).toHaveLength(PANE_COUNT) - - // Why every pane must be streaming: an idle pane carries no output source, - // so its reattach sends no recovery checkpoint and the relay answers - // 'existing'. Only a live source can come back needing re-establishment, - // which is the outcome that used to read as expiry and respawn the shell. - const streamMarker = `SSH_RECONNECT_STREAM_${Date.now()}` - const countRemoteStreamWriters = (): number => - countDockerSshRelayRemoteStreamWriters(relayTarget, streamMarker) - for (const pane of paneSnapshot.panes) { - if (!pane.ptyId) { - throw new Error(`Pane ${pane.leafId} has no PTY to stream from`) - } - await sendToTerminal( - orcaPage, - pane.ptyId, - `node -e "setInterval(()=>process.stdout.write('${streamMarker}_'+Date.now()+'\\n'),25)"\r` - ) - } - await expect - .poll(countRemoteStreamWriters, { - timeout: 60_000, - message: 'remote panes did not start streaming before the first transport fault' - }) - .toBe(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 { remote, baselinePanes, baselineRemotePtys } = await openStreamingRemotePanes( + orcaPage, + relayTarget + ) const baselinePids = baselineRemotePtys.map((pty) => pty.pid) testInfo.annotations.push({ type: 'ssh-reconnect-cardinality-baseline', @@ -232,4 +260,101 @@ test.describe('SSH reconnect pane and remote PTY cardinality', () => { cleanupDockerSshRelayTarget(target) } }) + + // The divergence the field hit: a lease and its remote shell outlive the pane + // record, because the pane was closed while the link was down and the kill + // never reached the host. Reconnect must reattach without inventing the pane + // back, and without killing the shell it can no longer place. + test('leaves a lease whose durable pane is gone unbound instead of grafting the pane back', async ({ + orcaPage + }, testInfo: TestInfo) => { + test.setTimeout(480_000) + let target: DockerSshRelayTarget | null = null + try { + target = startDockerSshRelayTarget(testInfo) + const relayTarget = target + const { remote, paneSnapshot, baselineRemotePtys } = await openStreamingRemotePanes( + orcaPage, + relayTarget + ) + const baselinePids = baselineRemotePtys.map((pty) => pty.pid) + const hostId = sshExecutionHostId(remote.targetId) + const keptLeafId = paneSnapshot.panes[0]!.leafId + const closedLeafId = paneSnapshot.panes[1]!.leafId + const readBindings = (): Promise => + readDurablePaneBindings(orcaPage, hostId, remote.worktreeId) + expect( + (await readBindings()).filter((binding) => binding.includes(closedLeafId)), + 'the pane to be closed must start out durably bound' + ).not.toHaveLength(0) + + severDockerSshRelayTransport(relayTarget) + // Closing here is what leaves the lease orphaned: pty:kill fails with the + // transport down, so the remote shell and its lease both survive the pane. + await focusLastTerminalPane(orcaPage) + await closeActiveTerminalPane(orcaPage) + await expect + .poll( + async () => (await readRemotePaneCensus(orcaPage, remote.worktreeId)).paneIds.length, + { + timeout: 30_000, + message: 'the closed pane never left the visible layout' + } + ) + .toBe(PANE_COUNT - 1) + await persistClosedRemotePaneSnapshot(orcaPage, { + hostId, + tabId: paneSnapshot.tabId, + keptLeafId, + closedLeafId + }) + await expect + .poll(async () => (await readBindings()).filter((b) => b.includes(closedLeafId)).length, { + timeout: 30_000, + message: 'a durable partition still named the closed pane before the reconnect' + }) + .toBe(0) + const survivingBindings = await readBindings() + expect( + survivingBindings.filter((binding) => binding.includes(keptLeafId)), + 'the surviving pane must stay durably bound' + ).not.toHaveLength(0) + + await waitForSshReconnected(orcaPage, remote.targetId) + await ensureTerminalVisible(orcaPage, 45_000) + await waitForActiveTerminalManager(orcaPage, 60_000) + await waitForActivePanePtyId(orcaPage, 60_000) + + // Sample continuously rather than poll-until-equal: a graft that lands and + // is later overwritten by a renderer snapshot would satisfy a poll that + // only needs one matching read, and the defect would pass unnoticed. + const deadline = Date.now() + 25_000 + let samples = 0 + while (Date.now() < deadline) { + expect(await readBindings(), 'reconnect grafted a pane the user had closed').toEqual( + survivingBindings + ) + samples += 1 + await orcaPage.waitForTimeout(500) + } + expect(samples).toBeGreaterThan(10) + + // Unknown is not dead: the orphaned shell keeps running, so a later + // reattach can still claim it once a durable pane names it again. + expect( + readDockerSshRelayRemotePtys(relayTarget).map((pty) => pty.pid), + 'the orphaned remote shell was killed or respawned' + ).toEqual(baselinePids) + expect( + (await readRemotePaneCensus(orcaPage, remote.worktreeId)).paneIds, + 'reconnect surfaced a pane the user never opened' + ).toHaveLength(PANE_COUNT - 1) + testInfo.annotations.push({ + type: 'ssh-reconnect-orphaned-lease-final', + description: describeDockerSshRelayRemotePtys(readDockerSshRelayRemotePtys(relayTarget)) + }) + } finally { + cleanupDockerSshRelayTarget(target) + } + }) })