diff --git a/src/main/ipc/pty-controller-ownership-routing.test.ts b/src/main/ipc/pty-controller-ownership-routing.test.ts index 0d236e63cb2..2101d17f26b 100644 --- a/src/main/ipc/pty-controller-ownership-routing.test.ts +++ b/src/main/ipc/pty-controller-ownership-routing.test.ts @@ -324,6 +324,7 @@ describe('registerPtyHandlers', () => { } const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(), removeSshRemotePtyLease: vi.fn(), markSshRemotePtyLease: vi.fn(), diff --git a/src/main/ipc/pty-daemon-spawn-session-identity.test.ts b/src/main/ipc/pty-daemon-spawn-session-identity.test.ts index 90ea56049fa..b1ead181530 100644 --- a/src/main/ipc/pty-daemon-spawn-session-identity.test.ts +++ b/src/main/ipc/pty-daemon-spawn-session-identity.test.ts @@ -345,6 +345,7 @@ describe('registerPtyHandlers', () => { ) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn() } registerSshPtyProvider('ssh-1', { diff --git a/src/main/ipc/pty-pane-reservation-settlement.test.ts b/src/main/ipc/pty-pane-reservation-settlement.test.ts index 64a4441f376..06e8679ace4 100644 --- a/src/main/ipc/pty-pane-reservation-settlement.test.ts +++ b/src/main/ipc/pty-pane-reservation-settlement.test.ts @@ -124,6 +124,7 @@ describe('registerPtyHandlers', () => { flushOrThrow: vi.fn(), persistPtyBinding: vi.fn(), upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), removeSshRemotePtyLease: vi.fn(), markSshRemotePtyLease: vi.fn(), clearSshRemotePtyKillIntent: vi.fn() @@ -482,6 +483,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(), removeSshRemotePtyLease: vi.fn(), markSshRemotePtyLease: vi.fn(), diff --git a/src/main/ipc/pty-runtime-ssh-binding-persistence.test.ts b/src/main/ipc/pty-runtime-ssh-binding-persistence.test.ts index 3fd1f89b11d..d1078477d12 100644 --- a/src/main/ipc/pty-runtime-ssh-binding-persistence.test.ts +++ b/src/main/ipc/pty-runtime-ssh-binding-persistence.test.ts @@ -154,6 +154,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(), removeSshRemotePtyLease: vi.fn(), markSshRemotePtyLease: vi.fn(), @@ -260,6 +261,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn() } let controller: RuntimeSpawnController | null = null @@ -370,6 +372,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(() => { throw new Error('disk full') }), @@ -471,6 +474,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(), removeSshRemotePtyLease: vi.fn(), markSshRemotePtyLease: vi.fn(), @@ -577,6 +581,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(), removeSshRemotePtyLease: vi.fn(), markSshRemotePtyLease: vi.fn(), diff --git a/src/main/ipc/pty-serializer-settlement-mapping.test.ts b/src/main/ipc/pty-serializer-settlement-mapping.test.ts index 4af42f1dbd5..53755214238 100644 --- a/src/main/ipc/pty-serializer-settlement-mapping.test.ts +++ b/src/main/ipc/pty-serializer-settlement-mapping.test.ts @@ -108,6 +108,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(), removeSshRemotePtyLease: vi.fn(), markSshRemotePtyLease: vi.fn(), @@ -212,6 +213,7 @@ describe('registerPtyHandlers', () => { } as never) const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(() => { throw new Error('disk full') }), diff --git a/src/main/ipc/pty-session-liveness-and-ownership.test.ts b/src/main/ipc/pty-session-liveness-and-ownership.test.ts index fb88fc4b6cb..574e83c1220 100644 --- a/src/main/ipc/pty-session-liveness-and-ownership.test.ts +++ b/src/main/ipc/pty-session-liveness-and-ownership.test.ts @@ -386,6 +386,7 @@ describe('registerPtyHandlers', () => { it('ignores fire-and-forget IPC for detached SSH PTYs without a provider', async () => { const store = { upsertSshRemotePtyLease: vi.fn(), + supersedeSshRemotePtyLeasesForBoundPane: vi.fn(), persistPtyBinding: vi.fn(), markSshRemotePtyLease: vi.fn(), clearSshRemotePtyKillIntent: vi.fn() diff --git a/src/main/ipc/pty/ipc/spawn-commit-persist.ts b/src/main/ipc/pty/ipc/spawn-commit-persist.ts index be9cb31394a..540ada9397d 100644 --- a/src/main/ipc/pty/ipc/spawn-commit-persist.ts +++ b/src/main/ipc/pty/ipc/spawn-commit-persist.ts @@ -134,6 +134,13 @@ export async function persistPtyIpcSpawnCommit(ctx: PtyIpcSpawnState): Promise<{ }) } } + // Why here and not at the upsert: this path leases before it binds, so supersession fenced on the + // pane's binding still named the predecessor and bailed on every reconnect — one more reattachable + // lease, and one more `pty.attach`, per reconnect forever. Runs after whichever binding write this + // commit made, so the lease/binding order no longer decides. + if (ctx.deps.store && args.connectionId && ctx.validatedLeafId !== null) { + ctx.deps.store.supersedeSshRemotePtyLeasesForBoundPane(args.connectionId, ctx.validatedLeafId) + } // Why: when the renderer has declared it will own the serializer for this paneKey, suppress the daemon-snapshot seed so its hydration path is sole authority (keyed on paneKey since the ptyId isn't known yet). See docs/mobile-prefer-renderer-scrollback.md. const rendererPreSignaled = ctx.validatedPaneKey ? pendingByPaneKey.has(ctx.validatedPaneKey) diff --git a/src/main/ipc/pty/ipc/spawn-commit-ssh-lease-cardinality.test.ts b/src/main/ipc/pty/ipc/spawn-commit-ssh-lease-cardinality.test.ts new file mode 100644 index 00000000000..6266faf0c02 --- /dev/null +++ b/src/main/ipc/pty/ipc/spawn-commit-ssh-lease-cardinality.test.ts @@ -0,0 +1,289 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { rmSync, mkdtempSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { testState, createStore } from '../../../persistence-test-harness' +import { TEST_LEAF_1, TEST_LEAF_2 } from '../../../persistence-session-fixtures' +import { sshRemotePtyLeaseAllowsReattach } from '../../../../shared/ssh-types' +import { toAppSshPtyId } from '../../../providers/ssh-pty-id' +import { toSshExecutionHostId } from '../../../../shared/execution-host' +import type { PtySpawnIpcArgs, PtySpawnIpcDeps } from './spawn-types' +import { createPtyIpcSpawnState } from './spawn-state' +import { persistPtyIpcSpawnCommit } from './spawn-commit-persist' + +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { isEncryptionAvailable: () => false } +})) + +const TARGET = 'ssh-1' +const WORKTREE = 'repo1::/worktree' +const TAB = 'tab-1' + +/** + * Drives the shipped IPC spawn commit rather than the store primitives it calls. + * + * The store-level suite could not catch this: it exercised bind-then-upsert, and this path does the + * opposite — it writes the lease row first so a force-quit in the renderer's debounce window cannot + * strand a running remote shell without one, then binds the pane. Supersession is fenced on the + * pane's binding, so under this real order it bailed on the predecessor every time and never re-ran, + * and each reconnect left one more reattachable lease for `reattachKnownPtys` to `pty.attach`. + */ +async function commitSshSpawn( + store: ReturnType, + args: { relayPtyId: string; leafId: string } +): Promise { + const deps = { store } as unknown as PtySpawnIpcDeps + const spawnArgs = { + cols: 80, + rows: 24, + worktreeId: WORKTREE, + tabId: TAB, + leafId: args.leafId, + connectionId: TARGET + } as unknown as PtySpawnIpcArgs + const ctx = createPtyIpcSpawnState(deps, spawnArgs) + ctx.result = { id: toAppSshPtyId(TARGET, args.relayPtyId) } + ctx.validatedLeafId = args.leafId + await persistPtyIpcSpawnCommit(ctx) +} + +/** One pane's layout, so the two host partitions can be given different bindings for one leaf. */ +function sessionBinding(ptyId: string) { + return { + activeRepoId: 'repo1', + activeWorktreeId: WORKTREE, + activeTabId: TAB, + tabsByWorktree: {}, + terminalLayoutsByTabId: { + [TAB]: { + root: { type: 'leaf' as const, leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: ptyId } + } + } + } +} + +function bulkReattachPtyIds(store: ReturnType): string[] { + return store + .getSshRemotePtyLeases(TARGET) + .filter(sshRemotePtyLeaseAllowsReattach) + .map((lease) => lease.ptyId) + .sort() +} + +describe('the IPC spawn commit keeps one reattachable lease per SSH pane', () => { + beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-')) + }) + afterEach(() => { + rmSync(testState.dir, { recursive: true, force: true }) + }) + + // QA's measurement, driven through the real path: five relay restarts, one pane, N+1 leases. + it('holds the reattach set flat across five reconnects of one pane', async () => { + const store = await createStore() + + for (let reconnect = 0; reconnect < 5; reconnect++) { + // A relay renumbers from `pty-1` on every start; a reconnect therefore re-leases the same + // pane under an id it has never used before. + await commitSshSpawn(store, { relayPtyId: `pty-${reconnect}`, leafId: TEST_LEAF_1 }) + } + + expect(bulkReattachPtyIds(store)).toEqual(['pty-4']) + }) + + it('retires each predecessor as `expired` with the winner recorded, never `terminated`', async () => { + const store = await createStore() + + await commitSshSpawn(store, { relayPtyId: 'pty-0', leafId: TEST_LEAF_1 }) + await commitSshSpawn(store, { relayPtyId: 'pty-1', leafId: TEST_LEAF_1 }) + + const predecessor = store.getSshRemotePtyLeases(TARGET).find((entry) => entry.ptyId === 'pty-0') + // `expired`, not `terminated`: losing the lease is not evidence the remote shell died, and the + // process is deliberately left running (docs/reference/ssh-execution-boundary.md). + expect(predecessor).toMatchObject({ state: 'expired', supersededBy: 'pty-1' }) + }) + + // The failure that would be worse than the fan-out: over-superseding strands a live remote + // process behind a pane that can no longer find it. + it('leaves a genuine orphan reattachable while superseding the pane that re-leased', async () => { + const store = await createStore() + + await commitSshSpawn(store, { relayPtyId: 'orphan-pty', leafId: TEST_LEAF_2 }) + // The orphan's client lost its route; nothing observed the shell, so it stays askable. + store.markSshRemotePtyLease(TARGET, 'orphan-pty', 'expired') + + await commitSshSpawn(store, { relayPtyId: 'pty-0', leafId: TEST_LEAF_1 }) + await commitSshSpawn(store, { relayPtyId: 'pty-1', leafId: TEST_LEAF_1 }) + + const orphan = store.getSshRemotePtyLeases(TARGET).find((entry) => entry.ptyId === 'orphan-pty') + expect(orphan?.supersededBy).toBeUndefined() + expect(bulkReattachPtyIds(store)).toEqual(['orphan-pty', 'pty-1']) + }) + + /** + * The shape the Docker lane exposed, and the reason a spawn-time trigger is not enough on its + * own. When the spawn commit writes no binding, the renderer's debounced layout publish does it + * later — so at commit time the pane still names the predecessor and supersession correctly + * declines. Nothing revisited it afterwards, and the predecessor stayed reattachable forever. + * + * Measured rows agreed on target, worktree, tab and leaf and still carried no `supersededBy`. + */ + it('retires a predecessor whose successor bound the pane after the spawn commit', async () => { + const store = await createStore() + + await commitSshSpawn(store, { relayPtyId: 'pty2:aaa:1', leafId: TEST_LEAF_1 }) + // What `handlePtyReattachFailure` writes when a restarted relay disowns the id. + store.markSshRemotePtyLease(TARGET, 'pty2:aaa:1', 'expired') + + // The successor leases without binding the pane; the binding catches up afterwards, exactly as + // the renderer's debounced publish does. + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: 'pty2:bbb:1', + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + state: 'attached' + }) + expect(bulkReattachPtyIds(store)).toEqual(['pty2:aaa:1', 'pty2:bbb:1']) + store.persistPtyBinding({ + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + ptyId: toAppSshPtyId(TARGET, 'pty2:bbb:1') + }) + + // What the connect path does before reading the set it feeds to `pty.attach`. + store.reconcileSshRemotePtyLeasesForTarget(TARGET) + + expect(bulkReattachPtyIds(store)).toEqual(['pty2:bbb:1']) + }) + + // Reconciliation must not invent evidence: with no binding naming the pane, nothing says which + // shell owns it, so every lease stays askable. + it('leaves leases reattachable when no binding names the pane', async () => { + const store = await createStore() + + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: 'unbound-a', + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + state: 'expired' + }) + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: 'unbound-b', + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + state: 'expired' + }) + + store.reconcileSshRemotePtyLeasesForTarget(TARGET) + + expect(bulkReattachPtyIds(store)).toEqual(['unbound-a', 'unbound-b']) + }) + + /** + * The measured defect, reduced to its cause. + * + * Main writes an SSH pane's binding to the `ssh:` partition, but a stale copy of the same + * leaf survives in `local`. Reading `local` first named the PREDECESSOR as the pane's current + * PTY, so supersession took an already-expired lease as its winner and returned having marked + * nothing — once per relay restart, forever. Both partitions name the same PTY again once the + * renderer republishes, which is why the finished store looks consistent and hides this. + */ + it('supersedes when the local partition still names the predecessor', async () => { + const store = await createStore() + const hostId = toSshExecutionHostId(TARGET) + const predecessor = toAppSshPtyId(TARGET, 'pty2:old:1') + const successor = toAppSshPtyId(TARGET, 'pty2:new:1') + + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: 'pty2:old:1', + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + state: 'expired' + }) + // Both partitions start on the predecessor, as they do before a relay restart. + store.setWorkspaceSession(sessionBinding(predecessor)) + store.setWorkspaceSession(sessionBinding(predecessor), hostId) + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: 'pty2:new:1', + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + state: 'attached' + }) + // Production's writer for an SSH pane binding, and the whole point: it updates ONLY the host + // partition, so `local` is left naming the predecessor until the renderer republishes. + store.persistPtyBinding( + { worktreeId: WORKTREE, tabId: TAB, leafId: TEST_LEAF_1, ptyId: successor }, + hostId + ) + expect( + store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[TEST_LEAF_1] + ).toBe(predecessor) + + store.supersedeSshRemotePtyLeasesForBoundPane(TARGET, TEST_LEAF_1) + + const retired = store.getSshRemotePtyLeases(TARGET).find((l) => l.ptyId === 'pty2:old:1') + expect(retired).toMatchObject({ state: 'expired', supersededBy: 'pty2:new:1' }) + expect(bulkReattachPtyIds(store)).toEqual(['pty2:new:1']) + }) + + // The mirror: a live shell the pane is still bound to must never be retired, whichever partition + // names it. Over-superseding strands a running remote process. + it('never retires a live lease the pane is still bound to', async () => { + const store = await createStore() + const live = toAppSshPtyId(TARGET, 'pty2:live:1') + + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: 'pty2:live:1', + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + state: 'attached' + }) + // Bound BEFORE the stray lease arrives, which is the order that makes the binding meaningful: + // with no binding at all, an arriving lease is the only evidence there is and does win. + store.setWorkspaceSession(sessionBinding(live)) + store.setWorkspaceSession(sessionBinding(live), toSshExecutionHostId(TARGET)) + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: 'pty2:other:1', + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + state: 'attached' + }) + + store.supersedeSshRemotePtyLeasesForBoundPane(TARGET, TEST_LEAF_1) + + const stillLive = store.getSshRemotePtyLeases(TARGET).find((l) => l.ptyId === 'pty2:live:1') + expect(stillLive).toMatchObject({ state: 'attached' }) + expect(stillLive?.supersededBy).toBeUndefined() + }) + + // Panes are independent, and supersession keys on the leaf: a second live pane on the same + // target must survive its neighbour reconnecting. + it('does not touch a sibling pane on the same target', async () => { + const store = await createStore() + + await commitSshSpawn(store, { relayPtyId: 'sibling-pty', leafId: TEST_LEAF_2 }) + await commitSshSpawn(store, { relayPtyId: 'pty-0', leafId: TEST_LEAF_1 }) + await commitSshSpawn(store, { relayPtyId: 'pty-1', leafId: TEST_LEAF_1 }) + + expect(bulkReattachPtyIds(store)).toEqual(['pty-1', 'sibling-pty']) + }) +}) diff --git a/src/main/ipc/pty/pane/ssh-pane-lease-claim.ts b/src/main/ipc/pty/pane/ssh-pane-lease-claim.ts new file mode 100644 index 00000000000..5746814a5f7 --- /dev/null +++ b/src/main/ipc/pty/pane/ssh-pane-lease-claim.ts @@ -0,0 +1,44 @@ +import { isTerminalLeafId } from '../../../../shared/stable-pane-id' +import { getRelayPtyId } from '../provider/registry' +import type { Store } from '../../../persistence' + +/** + * Claim a remote PTY for a pane: record the lease, then retire the pane's predecessors. + * + * The lease keeps the RELAY id, because reconnect calls `pty.attach` with target-local ids, while + * the pane binding keeps the app-facing id used for hydration. + * + * Supersession is a second step rather than something `upsertSshRemotePtyLease` finishes on its own + * because it is fenced on the pane's durable binding — it refuses to retire a predecessor the pane + * is still bound to, which would detach a live pane. A caller that leases BEFORE it binds therefore + * trips that fence on every reconnect and, with the upsert as the only trigger, never re-runs: one + * more reattachable lease, and one more `pty.attach` round trip on every later connect, forever. + * Re-running it here from the binding side is what makes the two writes commute. + */ +export function claimSshPaneLease(args: { + store: Store | undefined + connectionId: string | null | undefined + ptyId: string + worktreeId: string | undefined + tabId: string | undefined + leafId: string | undefined +}): void { + const { store, connectionId } = args + if (!store || !connectionId) { + return + } + const leafId = + typeof args.leafId === 'string' && isTerminalLeafId(args.leafId) ? args.leafId : null + store.upsertSshRemotePtyLease({ + targetId: connectionId, + ptyId: getRelayPtyId(connectionId, args.ptyId), + ...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}), + ...(typeof args.tabId === 'string' ? { tabId: args.tabId } : {}), + ...(leafId ? { leafId } : {}), + state: 'attached', + lastAttachedAt: Date.now() + }) + if (leafId) { + store.supersedeSshRemotePtyLeasesForBoundPane(connectionId, leafId) + } +} diff --git a/src/main/ipc/pty/runtime/spawn-commit.ts b/src/main/ipc/pty/runtime/spawn-commit.ts index 0b4e8804ac0..23592604bea 100644 --- a/src/main/ipc/pty/runtime/spawn-commit.ts +++ b/src/main/ipc/pty/runtime/spawn-commit.ts @@ -1,8 +1,6 @@ import { isValidTerminalTabId } from '../../../../shared/terminal-tab-id' -import { isTerminalLeafId } from '../../../../shared/stable-pane-id' import { ptyOwnership, ptyIncarnationById, deletePtyOwnership } from '../provider/ownership-state' import { ptySizes } from '../delivery/visibility-state' -import { getRelayPtyId } from '../provider/registry' import { shouldSkipCodexHomeEnvForWindowsShell, recordCodexPaneAccountForSpawn, @@ -25,6 +23,7 @@ import { requestKindSchema } from '../../../../shared/telemetry-events' import { persistAdmittedStablePaneBinding } from '../pane/stable-owner' +import { claimSshPaneLease } from '../pane/ssh-pane-lease-claim' import { isNativeWindowsLocalPtySpawn, markNativeWindowsConptyPty @@ -114,23 +113,15 @@ export async function commitRuntimePtySpawn(ctx: RuntimePtySpawnState) { ) { markNativeWindowsConptyPty(ctx.result.id) } - const persistSshLease = (): void => { - if (!ctx.deps.store || !args.connectionId) { - return - } - // Why: SSH leases keep relay ids for remote reconciliation, while session bindings keep app-facing ids for hydration. - ctx.deps.store.upsertSshRemotePtyLease({ - targetId: args.connectionId, - ptyId: getRelayPtyId(args.connectionId, ctx.result.id), - ...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}), - ...(typeof args.tabId === 'string' ? { tabId: args.tabId } : {}), - ...(typeof args.leafId === 'string' && isTerminalLeafId(args.leafId) - ? { leafId: args.leafId } - : {}), - state: 'attached', - lastAttachedAt: Date.now() + const persistSshLease = (): void => + claimSshPaneLease({ + store: ctx.deps.store, + connectionId: args.connectionId, + ptyId: ctx.result.id, + worktreeId: args.worktreeId, + tabId: args.tabId, + leafId: args.leafId }) - } if (!ctx.hostSessionBinding) { persistSshLease() } diff --git a/src/main/ipc/ssh-ipc-test-harness.ts b/src/main/ipc/ssh-ipc-test-harness.ts index 84802fe83a0..581fcc916ca 100644 --- a/src/main/ipc/ssh-ipc-test-harness.ts +++ b/src/main/ipc/ssh-ipc-test-harness.ts @@ -25,6 +25,7 @@ export type SshLeaseStoreMock = { upsertSshPtyConsumerRecovery: Mock removeSshPtyConsumerRecovery: Mock getSshRemotePtyLeases: Mock + reconcileSshRemotePtyLeasesForTarget: Mock markSshRemotePtyLease: Mock markSshRemotePtyLeases: Mock markSshRemotePtyLeasesAsync: Mock @@ -102,6 +103,7 @@ export function createSshIpcHarness(mocks: SshIpcMocks): SshIpcHarness { upsertSshPtyConsumerRecovery: vi.fn(), removeSshPtyConsumerRecovery: vi.fn(), getSshRemotePtyLeases: vi.fn().mockReturnValue([]), + reconcileSshRemotePtyLeasesForTarget: vi.fn(), markSshRemotePtyLease: vi.fn(), markSshRemotePtyLeases: vi.fn(), markSshRemotePtyLeasesAsync: vi.fn(), 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 46db8c4f0c7..f0628cb25e5 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 @@ -1,9 +1,8 @@ -import { toSshExecutionHostId } from '../../../shared/execution-host' import type { PersistedState } from '../../../shared/persisted-state-types' import type { SshRemotePtyLease } from '../../../shared/ssh-types' import { isTerminalLeafId } from '../../../shared/stable-pane-id' -import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { invalidateLocalWorktreeMetadataPruneInputs } from '../../local-worktree-metadata-prune-gate' +import { supersedeSiblingLeasesForPane } from './ssh-pty-pane-supersession' export type SshPtyLeaseOperations = { state: PersistedState @@ -15,91 +14,6 @@ export type SshPtyLeaseOperations = { flushDurableStateOrThrowAsync: () => Promise } -/** - * The PTY a pane is durably bound to, keyed on the leaf alone — the only remint-stable half of a - * pane key, since `detachTerminalPaneToTab` moves a live pane and leaves its lease naming the tab - * it left. - * - * Reads both partitions deliberately. Main writes some SSH pane bindings to `ssh:` and - * some to `local`, so a reader that consulted one would see "unbound" for a live pane and expire - * its lease. Reading both makes this fence correct whichever partition the binding landed in. - */ -function durablyBoundPtyIdForPane( - operations: SshPtyLeaseOperations, - targetId: string, - leafId: string -): string | undefined { - const findLeafBinding = (session: WorkspaceSessionState | undefined): string | undefined => - Object.values(session?.terminalLayoutsByTabId ?? {}).find( - (layout) => layout?.ptyIdsByLeafId?.[leafId] - )?.ptyIdsByLeafId?.[leafId] - const boundPtyId = - findLeafBinding(operations.state.workspaceSession) ?? - findLeafBinding(operations.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)]) - return boundPtyId ? operations.toComparablePtyId(targetId, boundPtyId) : undefined -} - -/** - * One pane owns at most one live remote PTY. Lease identity is `(targetId, ptyId)` alone, so a - * pane re-leasing under a new relay id leaves its predecessor live with nothing to retire it and - * the next reattach fans out over both — the reported 2 -> 19 -> 20 across three reconnects. - * - * Superseded leases are marked `expired`, never `terminated`: losing a lease is not evidence the - * shell died, so the remote process is deliberately left running. They also carry `supersededBy`, - * which is what keeps them out of the bulk reattach set now that plain `expired` no longer does — - * the winner's ptyId is already in hand here, so recording it needs no relay-start identity. - */ -function supersedeSiblingLeasesForPane( - operations: SshPtyLeaseOperations, - winner: SshRemotePtyLease, - now: number -): void { - if (!winner.worktreeId || !winner.leafId) { - return - } - if (winner.state === 'terminated' || winner.state === 'expired') { - return - } - // At upsert time the arriving 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 = durablyBoundPtyIdForPane(operations, winner.targetId, winner.leafId) - if (boundPtyId && boundPtyId !== winner.ptyId) { - return - } - const superseded: SshRemotePtyLease[] = [] - for (const lease of operations.state.sshRemotePtyLeases ?? []) { - if ( - lease.ptyId === winner.ptyId || - lease.targetId !== winner.targetId || - lease.worktreeId !== winner.worktreeId || - // Leaf only: a lease freezes its tabId, so a pane broken out into a new tab would otherwise - // never compete with its own predecessor — which is the reported cardinality growth. - lease.leafId !== winner.leafId || - lease.state === 'terminated' - ) { - continue - } - if (lease.state === 'expired') { - // An already-expired predecessor is superseded by the same evidence, and marking it is what - // bounds the reattach set: without this, every past orphan for this pane stays reattachable - // forever. `updatedAt` stays put — bumping it would make a stale lease look recent to - // `getRecentExpiredSshLease`. - lease.supersededBy = winner.ptyId - continue - } - lease.state = 'expired' - lease.supersededBy = winner.ptyId - lease.updatedAt = now - superseded.push(lease) - } - if (superseded.length > 0) { - // Why: matching on lease ptyId first means this scrubs only the predecessor's stale binding — - // the winner's own binding cannot match and is left intact. - operations.clearBindingsForLeases(winner.targetId, superseded) - } -} - /** * Only `terminated` unbinds a pane. It is the operator-close state and the one written after a * host-acknowledged stop; `expired` records that the CLIENT lost its route and says nothing about diff --git a/src/main/persistence/leasing-ssh-ptys/ssh-pty-pane-supersession.ts b/src/main/persistence/leasing-ssh-ptys/ssh-pty-pane-supersession.ts new file mode 100644 index 00000000000..61d6b934db5 --- /dev/null +++ b/src/main/persistence/leasing-ssh-ptys/ssh-pty-pane-supersession.ts @@ -0,0 +1,201 @@ +import { toSshExecutionHostId } from '../../../shared/execution-host' +import type { SshRemotePtyLease } from '../../../shared/ssh-types' +import { isTerminalLeafId } from '../../../shared/stable-pane-id' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import type { SshPtyLeaseOperations } from './ssh-pty-lease-operations' + +/** + * Every PTY id any partition binds to this pane, most authoritative first. + * + * Keyed on the leaf alone — the only remint-stable half of a pane key, since + * `detachTerminalPaneToTab` moves a live pane and leaves its lease naming the tab it left. + * + * Returns a LIST, and reads the target's own partition first, because the two partitions disagree + * for the length of a reconnect and this resolved that disagreement backwards. Main writes an SSH + * pane's binding to `ssh:`, while a stale copy of the same leaf survives in `local`; + * consulting `local` first therefore named the PREDECESSOR as the pane's current PTY on every relay + * restart. Supersession then took that expired predecessor as its winner and returned without + * marking anything — the per-reconnect lease growth. Both partitions are still read, because a + * reader that consulted only one would see "unbound" for a live pane and expire its lease. + */ +function durablyBoundPtyIdsForPane( + operations: SshPtyLeaseOperations, + targetId: string, + leafId: string +): string[] { + const findLeafBindings = (session: WorkspaceSessionState | undefined): string[] => + Object.values(session?.terminalLayoutsByTabId ?? {}) + .map((layout) => layout?.ptyIdsByLeafId?.[leafId]) + .filter((ptyId): ptyId is string => Boolean(ptyId)) + const ordered = [ + ...findLeafBindings( + operations.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)] + ), + ...findLeafBindings(operations.state.workspaceSession) + ] + return [...new Set(ordered.map((ptyId) => operations.toComparablePtyId(targetId, ptyId)))] +} + +/** A lease this client still holds a route to, as opposed to one it has already lost. */ +function isLiveLeaseState(state: SshRemotePtyLease['state']): boolean { + return state === 'attached' || state === 'detached' +} + +/** + * One pane owns at most one live remote PTY. Lease identity is `(targetId, ptyId)` alone, so a + * pane re-leasing under a new relay id leaves its predecessor live with nothing to retire it and + * the next reattach fans out over both — the reported 2 -> 19 -> 20 across three reconnects. + * + * Superseded leases are marked `expired`, never `terminated`: losing a lease is not evidence the + * shell died, so the remote process is deliberately left running. They also carry `supersededBy`, + * which is what keeps them out of the bulk reattach set now that plain `expired` no longer does — + * the winner's ptyId is already in hand here, so recording it needs no relay-start identity. + */ +export function supersedeSiblingLeasesForPane( + operations: SshPtyLeaseOperations, + winner: SshRemotePtyLease, + now: number +): boolean { + if (!winner.worktreeId || !winner.leafId) { + return false + } + if (winner.state === 'terminated' || winner.state === 'expired') { + return false + } + // At upsert time the arriving 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. `supersedeSshRemotePtyLeasesForBoundPane` re-runs this once the + // binding write lands, so a caller that upserts before it binds is not left bailed forever. + // Membership rather than equality: during a reconnect the two partitions name different PTYs for + // the same leaf, and requiring the winner to match the FIRST one read is what made this bail. + const boundPtyIds = durablyBoundPtyIdsForPane(operations, winner.targetId, winner.leafId) + if (boundPtyIds.length > 0 && !boundPtyIds.includes(winner.ptyId)) { + return false + } + let marked = false + const superseded: SshRemotePtyLease[] = [] + for (const lease of operations.state.sshRemotePtyLeases ?? []) { + if ( + lease.ptyId === winner.ptyId || + lease.targetId !== winner.targetId || + lease.worktreeId !== winner.worktreeId || + // Leaf only: a lease freezes its tabId, so a pane broken out into a new tab would otherwise + // never compete with its own predecessor — which is the reported cardinality growth. + lease.leafId !== winner.leafId || + lease.state === 'terminated' || + // Never retire a shell the pane is BOTH still bound to and still routable to. The stale + // partition can name a predecessor, and retiring that is the point; retiring a live one + // would strand a running remote process behind a pane that can no longer reach it. + (boundPtyIds.includes(lease.ptyId) && isLiveLeaseState(lease.state)) + ) { + continue + } + if (lease.state === 'expired') { + // An already-expired predecessor is superseded by the same evidence, and marking it is what + // bounds the reattach set: without this, every past orphan for this pane stays reattachable + // forever. `updatedAt` stays put — bumping it would make a stale lease look recent to + // `getRecentExpiredSshLease`. + marked ||= lease.supersededBy !== winner.ptyId + lease.supersededBy = winner.ptyId + continue + } + lease.state = 'expired' + lease.supersededBy = winner.ptyId + lease.updatedAt = now + marked = true + superseded.push(lease) + } + if (superseded.length > 0) { + // Why: matching on lease ptyId first means this scrubs only the predecessor's stale binding — + // the winner's own binding cannot match and is left intact. + operations.clearBindingsForLeases(winner.targetId, superseded) + } + return marked +} + +/** + * Supersede from the lease the pane's binding names — preferring a LIVE one when the partitions + * disagree, since a reconnect leaves the stale partition naming an already-expired predecessor and + * an expired winner supersedes nothing. + */ +function supersedeFromBoundPane( + operations: SshPtyLeaseOperations, + targetId: string, + leafId: string, + now: number +): boolean { + if (!isTerminalLeafId(leafId)) { + return false + } + const boundPtyIds = durablyBoundPtyIdsForPane(operations, targetId, leafId) + if (boundPtyIds.length === 0) { + // No binding names this pane, so nothing here is evidence about which shell owns it. Leaving + // every lease reattachable is the deliberate direction: an orphan must stay askable. + return false + } + const candidates = (operations.state.sshRemotePtyLeases ?? []).filter( + (lease) => + lease.targetId === targetId && lease.leafId === leafId && boundPtyIds.includes(lease.ptyId) + ) + const winner = candidates.find((lease) => isLiveLeaseState(lease.state)) + const marked = winner ? supersedeSiblingLeasesForPane(operations, winner, now) : false + return marked +} + +/** + * The binding-side trigger for supersession, and the reason the two writes that together claim a + * pane are commutative. + * + * `upsertSshRemotePtyLease` is the only other trigger, and it bails whenever the pane's durable + * binding still names the predecessor. A spawn path that upserts its lease BEFORE it writes the + * binding therefore bails and never re-runs on its own. Re-resolving the winner from the binding + * is safe in the other direction too: it supersedes only from the lease the pane is actually bound + * to, so it can never strand a live orphan. + */ +export function supersedeSshRemotePtyLeasesForBoundPane( + operations: SshPtyLeaseOperations, + targetId: string, + leafId: string +): void { + if (supersedeFromBoundPane(operations, targetId, leafId, Date.now())) { + operations.flush() + } +} + +/** + * Bound the reattach set to one lease per pane, re-derived from each pane's CURRENT binding. + * + * The spawn-side trigger cannot be sufficient alone, and measuring the shipped path is what showed + * it: a pane's binding has several writers — the spawn commit, the relay's reattach bind, and the + * renderer's debounced layout publish — and the last of those lands well after the spawn commit + * that leased the pty. A predecessor that was still bound when its successor was claimed therefore + * keeps its reattachability forever, because nothing revisits it once the binding catches up. The + * observed rows agreed on target, worktree, tab and leaf and still carried no mark. + * + * Running this immediately before the reattach set is read makes the answer independent of which + * writer bound the pane and when. It also repairs stores written by earlier builds, where these + * rows have already accumulated and no spawn-time trigger would ever revisit them. + * + * Panes with no binding are skipped rather than pruned: absence of a binding is not evidence about + * which shell owns the pane, and a genuine orphan has to stay askable + * (docs/reference/ssh-execution-boundary.md). + */ +export function reconcileSshRemotePtyLeasesForTarget( + operations: SshPtyLeaseOperations, + targetId: string +): void { + const leafIds = new Set() + for (const lease of operations.state.sshRemotePtyLeases ?? []) { + if (lease.targetId === targetId && lease.leafId) { + leafIds.add(lease.leafId) + } + } + const now = Date.now() + let changed = false + for (const leafId of leafIds) { + changed = supersedeFromBoundPane(operations, targetId, leafId, now) || changed + } + if (changed) { + operations.flush() + } +} diff --git a/src/main/persistence/loading-store/ssh-lease-recovery-operations.ts b/src/main/persistence/loading-store/ssh-lease-recovery-operations.ts index 8768a47e50a..75aa19ad24b 100644 --- a/src/main/persistence/loading-store/ssh-lease-recovery-operations.ts +++ b/src/main/persistence/loading-store/ssh-lease-recovery-operations.ts @@ -23,6 +23,10 @@ import { type SshPtyLeaseOperations, upsertSshRemotePtyLease as upsertSshRemotePtyLeaseOperation } from '../leasing-ssh-ptys/ssh-pty-lease-operations' +import { + reconcileSshRemotePtyLeasesForTarget as reconcileSshRemotePtyLeasesForTargetOperation, + supersedeSshRemotePtyLeasesForBoundPane as supersedeSshRemotePtyLeasesForBoundPaneOperation +} from '../leasing-ssh-ptys/ssh-pty-pane-supersession' import { getSshPtyConsumerRecovery as getSshPtyConsumerRecoveryOperation, removeSshPtyConsumerRecovery as removeSshPtyConsumerRecoveryOperation, @@ -95,6 +99,28 @@ export class SshLeaseRecoveryOperations { upsertSshRemotePtyLeaseOperation(getSshPtyLeaseOperations(this), lease) } + /** + * Re-run pane supersession from the binding rather than from an arriving lease. Spawn commits + * call this after their binding write so it does not matter whether the lease or the binding + * landed first; see `supersedeSshRemotePtyLeasesForBoundPane`. + */ + supersedeSshRemotePtyLeasesForBoundPane(targetId: string, leafId: string): void { + supersedeSshRemotePtyLeasesForBoundPaneOperation( + getSshPtyLeaseOperations(this), + targetId, + leafId + ) + } + + /** + * Re-derive one reattachable lease per pane from each pane's current binding. Called on the + * connect path immediately before the reattach set is read; see + * `reconcileSshRemotePtyLeasesForTarget`. + */ + reconcileSshRemotePtyLeasesForTarget(targetId: string): void { + reconcileSshRemotePtyLeasesForTargetOperation(getSshPtyLeaseOperations(this), targetId) + } + markSshRemotePtyLeases(targetId: string, state: SshRemotePtyLease['state']): void { markSshRemotePtyLeasesOperation(getSshPtyLeaseOperations(this), targetId, state) } diff --git a/src/main/ssh-reattach-pane-cardinality.test.ts b/src/main/ssh-reattach-pane-cardinality.test.ts index 5827ec1ee68..d7179703360 100644 --- a/src/main/ssh-reattach-pane-cardinality.test.ts +++ b/src/main/ssh-reattach-pane-cardinality.test.ts @@ -82,24 +82,38 @@ function relayReattachBinds( } /** - * Both binding writers land before the lease upsert — spawn asserts that ordering directly, and - * the relay's reattach binds the pane before `markSshRemotePtyLeasesAttachedAsync`. Supersession - * therefore sees a session already naming the arriving shell. + * One reconnect's worth of writes, in the order the spawn commits actually issue them: the lease + * row first — so a force-quit in the renderer's debounce window cannot leave a running remote shell + * with no lease to reattach it — then the binding, then the binding-side supersession trigger. + * + * This suite used to bind BEFORE upserting, an order no caller uses. Under that order supersession + * always saw a session already naming the arriving shell and passed; under production's order it + * bailed on the predecessor's binding every time and never re-ran, so the guard could not catch the + * per-reconnect lease growth it exists to pin. * * Goes through `persistPtyBinding` rather than `setWorkspaceSession` because that is the writer * production uses; a raw session write is reconciled back to the attached lease's PTY by binding * recovery, which would make the fixture disagree with the real flow. */ -function paneBindsTo( +function paneSpawnCommits( store: ReturnType, - args: { tabId: string; leafId: string; ptyId: string } + args: { tabId: string; leafId: string; ptyId: string; leaseTabId?: string } ): void { + store.upsertSshRemotePtyLease({ + targetId: TARGET, + ptyId: args.ptyId, + worktreeId: WORKTREE, + tabId: args.leaseTabId ?? args.tabId, + leafId: args.leafId, + state: 'attached' + }) store.persistPtyBinding({ worktreeId: WORKTREE, tabId: args.tabId, leafId: args.leafId, ptyId: args.ptyId }) + store.supersedeSshRemotePtyLeasesForBoundPane(TARGET, args.leafId) } function liveLeasePtyIds(store: ReturnType): string[] { @@ -300,8 +314,7 @@ describe('STA-3077: one pane keeps at most one live remote lease', () => { const lease = { targetId: TARGET, worktreeId: WORKTREE, tabId: TAB, leafId: TEST_LEAF_1 } store.upsertSshRemotePtyLease({ ...lease, ptyId: 'pty-1', state: 'attached' }) - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) - store.upsertSshRemotePtyLease({ ...lease, ptyId: 'pty-2', state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) expect(liveLeasePtyIds(store)).toEqual(['pty-2']) }) @@ -314,8 +327,7 @@ describe('STA-3077: one pane keeps at most one live remote lease', () => { const lease = { targetId: TARGET, worktreeId: WORKTREE, tabId: TAB, leafId: TEST_LEAF_1 } store.upsertSshRemotePtyLease({ ...lease, ptyId: 'pty-1', state: 'attached' }) - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) - store.upsertSshRemotePtyLease({ ...lease, ptyId: 'pty-2', state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) const predecessor = store.getSshRemotePtyLeases(TARGET).find((entry) => entry.ptyId === 'pty-1') expect(predecessor?.state).toBe('expired') @@ -325,11 +337,9 @@ describe('STA-3077: one pane keeps at most one live remote lease', () => { it('holds the live lease count flat across ten reconnects of one pane', async () => { const store = await createStore() store.setWorkspaceSession(sessionWithPane({ tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-0' })) - const lease = { targetId: TARGET, worktreeId: WORKTREE, tabId: TAB, leafId: TEST_LEAF_1 } for (let reconnect = 0; reconnect < 10; reconnect++) { - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: `pty-${reconnect}` }) - store.upsertSshRemotePtyLease({ ...lease, ptyId: `pty-${reconnect}`, state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: `pty-${reconnect}` }) } expect(liveLeasePtyIds(store)).toEqual(['pty-9']) @@ -349,17 +359,14 @@ describe('STA-3077: one pane keeps at most one live remote lease', () => { state: 'attached' }) - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) // The successor's lease names the tab the pane sits in NOW; the predecessor's still names the // one it was written in. Only the leaf is common, so keying on the tab would stop the two // competing and leave both live — the cardinality growth. - store.upsertSshRemotePtyLease({ - targetId: TARGET, - ptyId: 'pty-2', - worktreeId: WORKTREE, - tabId: OTHER_TAB, + paneSpawnCommits(store, { + tabId: TAB, leafId: TEST_LEAF_1, - state: 'attached' + ptyId: 'pty-2', + leaseTabId: OTHER_TAB }) expect(liveLeasePtyIds(store)).toEqual(['pty-2']) @@ -394,8 +401,7 @@ describe('STA-3077: one pane keeps at most one live remote lease', () => { const lease = { targetId: TARGET, worktreeId: WORKTREE, tabId: TAB, leafId: TEST_LEAF_1 } store.upsertSshRemotePtyLease({ ...lease, ptyId: 'pty-1', state: 'attached' }) - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) - store.upsertSshRemotePtyLease({ ...lease, ptyId: 'pty-2', state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) expect(liveLeasePtyIds(store).sort()).toEqual(['pty-2', 'sibling-pty']) }) @@ -455,8 +461,7 @@ describe('STA-3077: `expired` separates a superseded sibling from an orphan', () it('never bulk-reattaches a superseded sibling', async () => { const store = await storeWithPane('pty-1') - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) - store.upsertSshRemotePtyLease({ ...paneLease, ptyId: 'pty-2', state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) const predecessor = store.getSshRemotePtyLeases(TARGET).find((entry) => entry.ptyId === 'pty-1') expect(predecessor).toMatchObject({ state: 'expired', supersededBy: 'pty-2' }) @@ -469,8 +474,7 @@ describe('STA-3077: `expired` separates a superseded sibling from an orphan', () store.setWorkspaceSession(sessionWithPane({ tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-0' })) for (let reconnect = 0; reconnect < 10; reconnect++) { - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: `pty-${reconnect}` }) - store.upsertSshRemotePtyLease({ ...paneLease, ptyId: `pty-${reconnect}`, state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: `pty-${reconnect}` }) } expect(bulkReattachPtyIds(store)).toEqual(['pty-9']) @@ -496,8 +500,7 @@ describe('STA-3077: `expired` separates a superseded sibling from an orphan', () store.markSshRemotePtyLease(TARGET, 'pty-1', 'expired') const orphanUpdatedAt = store.getSshRemotePtyLeases(TARGET)[0].updatedAt - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) - store.upsertSshRemotePtyLease({ ...paneLease, ptyId: 'pty-2', state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) const predecessor = store.getSshRemotePtyLeases(TARGET).find((entry) => entry.ptyId === 'pty-1') expect(predecessor).toMatchObject({ state: 'expired', supersededBy: 'pty-2' }) @@ -510,8 +513,7 @@ describe('STA-3077: `expired` separates a superseded sibling from an orphan', () // belongs to the lease that lost, never to whatever claims the id next. it('clears the supersession mark when the id is re-upserted as a live lease', async () => { const store = await storeWithPane('pty-1') - paneBindsTo(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) - store.upsertSshRemotePtyLease({ ...paneLease, ptyId: 'pty-2', state: 'attached' }) + paneSpawnCommits(store, { tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-2' }) // A restarted relay hands `pty-1` to a new shell for a different pane. store.upsertSshRemotePtyLease({ diff --git a/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts index 3201f8709fe..afb365c075c 100644 --- a/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts +++ b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts @@ -53,7 +53,8 @@ function createHarness( shutdown } as unknown as IPtyProvider const store = { - getSshRemotePtyLeases: vi.fn().mockReturnValue(leases) + getSshRemotePtyLeases: vi.fn().mockReturnValue(leases), + reconcileSshRemotePtyLeasesForTarget: vi.fn() } as unknown as Store return { provider, store, shutdown } } diff --git a/src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts b/src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts index 49fc1e98ccd..76d92e77e9a 100644 --- a/src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts +++ b/src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts @@ -157,6 +157,7 @@ function createSession(targetId: string): InstanceType { upsertSshPtyConsumerRecovery: vi.fn(), removeSshPtyConsumerRecovery: vi.fn(), getSshRemotePtyLeases: vi.fn().mockReturnValue([]), + reconcileSshRemotePtyLeasesForTarget: vi.fn(), markSshRemotePtyLease: vi.fn(), markSshRemotePtyLeases: vi.fn(), markSshRemotePtyLeasesAsync: vi.fn(), diff --git a/src/main/ssh/ssh-relay-session-terminal-error.test.ts b/src/main/ssh/ssh-relay-session-terminal-error.test.ts index 9bb14618319..4cfa657e401 100644 --- a/src/main/ssh/ssh-relay-session-terminal-error.test.ts +++ b/src/main/ssh/ssh-relay-session-terminal-error.test.ts @@ -104,6 +104,7 @@ function createMockDeps(): { upsertSshPtyConsumerRecovery: vi.fn(), removeSshPtyConsumerRecovery: vi.fn(), getSshRemotePtyLeases: vi.fn().mockReturnValue([]), + reconcileSshRemotePtyLeasesForTarget: vi.fn(), markSshRemotePtyLease: vi.fn(), markSshRemotePtyLeases: vi.fn(), markSshRemotePtyLeasesAsync: vi.fn(), diff --git a/src/main/ssh/ssh-relay-session-test-fixtures.ts b/src/main/ssh/ssh-relay-session-test-fixtures.ts index efdee31c406..ba0782b3a20 100644 --- a/src/main/ssh/ssh-relay-session-test-fixtures.ts +++ b/src/main/ssh/ssh-relay-session-test-fixtures.ts @@ -21,6 +21,7 @@ export function createMockDeps(): SshRelaySessionTestDeps { upsertSshPtyConsumerRecovery: vi.fn(), removeSshPtyConsumerRecovery: vi.fn(), getSshRemotePtyLeases: vi.fn().mockReturnValue([]), + reconcileSshRemotePtyLeasesForTarget: vi.fn(), getWorkspaceSession: vi.fn(), markSshRemotePtyLease: vi.fn(), markSshRemotePtyLeases: vi.fn(), diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index bca3632b83b..99a2ce9fbf9 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -2322,6 +2322,12 @@ export class SshRelaySession { if (!shouldContinue()) { return } + // Why immediately before the read: a pane's binding is written by several writers, and the + // renderer's debounced layout publish lands long after the spawn commit that leased the pty — + // so a predecessor that was still bound at spawn time never gets marked by a spawn-side + // trigger. Re-deriving from each pane's CURRENT binding here is what actually bounds this set, + // and it repairs stores that already accumulated these rows. + this.store.reconcileSshRemotePtyLeasesForTarget(this.targetId) // Why not `state !== 'expired'`: that state covers both a superseded sibling (re-adopting it is // the 2 -> 19 -> 20 fan-out) and an orphan whose reattach merely lost contact. Only the first // carries a retirement mark, and only it has to be skipped. diff --git a/tests/e2e/ssh-docker-transport-drop-recovery.spec.ts b/tests/e2e/ssh-docker-transport-drop-recovery.spec.ts index 9f8434ed51a..e18336d12c5 100644 --- a/tests/e2e/ssh-docker-transport-drop-recovery.spec.ts +++ b/tests/e2e/ssh-docker-transport-drop-recovery.spec.ts @@ -1,5 +1,9 @@ -import type { Page } from '@playwright/test' +import path from 'node:path' +import { readFileSync } from 'node:fs' +import type { ElectronApplication, Page } from '@playwright/test' import { test, expect } from './helpers/orca-app' +import { DEFAULT_LOCAL_ORCA_PROFILE_ID } from '../../src/shared/orca-profiles' +import { sshRemotePtyLeaseAllowsReattach, type SshRemotePtyLease } from '../../src/shared/ssh-types' import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' import { execInTerminal, @@ -49,6 +53,59 @@ async function readSshStatus(orcaPage: Page, targetId: string) { ) } +/** + * Every lease `reattachKnownPtys` would feed to `pty.attach` on the next connect, read from the + * durable store rather than from the renderer — leases are main-owned and never published. + * + * Goes through the shipped `sshRemotePtyLeaseAllowsReattach` predicate so the measurement cannot + * drift from the fan-out it exists to bound. + */ +function readSshLeases(userDataDir: string, targetId: string): SshRemotePtyLease[] { + const dataPath = path.join( + userDataDir, + 'profiles', + DEFAULT_LOCAL_ORCA_PROFILE_ID, + 'orca-data.json' + ) + const parsed = JSON.parse(readFileSync(dataPath, 'utf8')) as { + sshRemotePtyLeases?: SshRemotePtyLease[] + } + return (parsed.sshRemotePtyLeases ?? []).filter((lease) => lease.targetId === targetId) +} + +function readReattachablePtyIds(userDataDir: string, targetId: string): string[] { + return readSshLeases(userDataDir, targetId) + .filter(sshRemotePtyLeaseAllowsReattach) + .map((lease) => lease.ptyId) + .sort() +} + +/** + * Everything a cardinality failure needs to be diagnosable from the report alone. + * + * Worth keeping rather than reducing to a count: when this first failed, the count said only "2", + * and it was the per-row fields that ruled out the obvious causes — the rows agreed on worktree, + * tab and leaf, so the pane identity was never the problem. + */ +function describeSshLeases(userDataDir: string, targetId: string): string { + return JSON.stringify( + readSshLeases(userDataDir, targetId).map((lease) => ({ + ptyId: lease.ptyId, + state: lease.state, + worktreeId: lease.worktreeId, + leafId: lease.leafId, + tabId: lease.tabId, + supersededBy: lease.supersededBy, + relayIdRecycled: lease.relayIdRecycled, + reattachable: sshRemotePtyLeaseAllowsReattach(lease) + })) + ) +} + +function readUserDataDir(electronApp: ElectronApplication): Promise { + return electronApp.evaluate(({ app }) => app.getPath('userData')) +} + /** * Not covered here on purpose: park-then-reveal after a reconnect. ssh-terminal-parking already * covers the park/reveal round trip, and driving a park deterministically from this lane proved @@ -117,7 +174,9 @@ test.describe('SSH transport drop recovery', () => { // run after the flood produces no output within the poll budget. Same shape as #18018 (deaf pane // after a stalled host resumes), and not caused by this spec. Tracked there; the three verdict // assertions around it stay enforced. - test.fixme('stays bounded when a disconnected shell floods its pty', async ({ orcaPage }, testInfo) => { + test.fixme('stays bounded when a disconnected shell floods its pty', async ({ + orcaPage + }, testInfo) => { test.slow() // Timeouts here are deliberately generous: this guards memory, not latency. A 48MB flood plus a // reconnect lands near 60s wall-clock end to end, so a 60s bind timeout was marginal and made @@ -261,6 +320,89 @@ test.describe('SSH transport drop recovery', () => { } }) + /** + * The cardinality half of the same fault, which the verdict test above cannot see: it asserts the + * pane is re-backed, not what the pane's PREVIOUS shells left behind in the store. + * + * A pane re-leases under a new relay pty id on every relay restart, and nothing else retires the + * predecessor. When supersession fails, each generation leaves one more `expired`-but-unsuperseded + * lease that `reattachKnownPtys` still asks about — one extra `pty.attach` round trip on every + * later connect, forever, growing linearly with reconnect count. Measured as leases rather than + * as latency because latency hides the growth until it is already large. + * + * The reattachable set must stay at exactly one per pane. It must not go to zero either: a lease + * wrongly superseded is a running remote shell the pane can no longer find, which is the worse + * failure (docs/reference/ssh-execution-boundary.md). + */ + test('keeps one reattachable lease per pane across repeated relay restarts', async ({ + orcaPage, + electronApp + }, testInfo) => { + test.slow() + let target: DockerSshRelayTarget | null = null + try { + target = startDockerSshRelayTarget(testInfo) + enableDockerSshRelayTargetShellTitle(target) + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + const remote = await connectDockerSshRelayTarget(orcaPage, target) + await ensureTerminalVisible(orcaPage, 45_000) + await waitForActiveTerminalManager(orcaPage, 60_000) + await waitForActivePanePtyId(orcaPage, 60_000) + + const userDataDir = await readUserDataDir(electronApp) + const generations: string[][] = [] + + for (let generation = 1; generation <= 5; generation++) { + expect( + killDockerSshRelayDaemon(target), + 'no relay process was found to kill' + ).toBeGreaterThan(0) + await expect + .poll(() => readSshStatus(orcaPage, remote.targetId), { + timeout: 120_000, + message: `SSH target never reconnected after relay kill ${generation}` + }) + .toBe('connected') + await waitForActiveTerminalManager(orcaPage, 120_000) + // The pane must be usable again before the count is meaningful: recovery is what mints the + // successor lease that retires the generation before it. + const ptyId = await waitForActivePanePtyId(orcaPage, 120_000) + const marker = `LEASE_GEN_${generation}_${Date.now()}` + await execInTerminal(orcaPage, ptyId, `printf '%s\\n' ${marker}`) + await waitForTerminalOutput(orcaPage, marker, 60_000) + + try { + await expect + .poll(() => readReattachablePtyIds(userDataDir, remote.targetId).length, { + timeout: 60_000 + }) + .toBe(1) + } catch (error) { + // Why re-thrown with the rows: the count alone cannot say WHICH predecessor stayed + // reattachable, and the user-data dir is torn down before the report is read. + throw new Error( + `reattachable lease count never settled at 1 in generation ${generation}; leases: ${describeSshLeases(userDataDir, remote.targetId)}`, + { cause: error } + ) + } + generations.push(readReattachablePtyIds(userDataDir, remote.targetId)) + } + + // Stated as the whole sequence so a regression reports the growth, not just its endpoint — + // the reported shape was 2, 3, 4, 5, 6 across five restarts. + expect( + generations.map((ptyIds) => ptyIds.length), + `reattachable lease count per generation: ${JSON.stringify(generations)}` + ).toEqual([1, 1, 1, 1, 1]) + } finally { + if (target) { + clearDockerSshRelayFaults(target) + cleanupDockerSshRelayTarget(target) + } + } + }) + /** * The third fault shape: silence with the socket still established. `docker pause` freezes the * container, so nothing is closed or reset — the client simply stops hearing from a host that is