From 3dd79e74331ebeee4e5256a1eaedcce5e2fbaee5 Mon Sep 17 00:00:00 2001 From: Brennan Benson Date: Sun, 12 Jul 2026 16:31:19 -0700 Subject: [PATCH] fix(terminal): preserve teardown ownership until confirmed --- config/reliability-gates.jsonc | 48 ++++-- src/main/ipc/pty.test.ts | 34 ++-- src/main/ipc/pty.ts | 4 + src/main/persistence.test.ts | 5 +- src/main/persistence.ts | 6 +- src/main/providers/local-pty-provider.test.ts | 21 +++ src/main/providers/local-pty-provider.ts | 39 +++-- src/main/runtime/orca-runtime.test.ts | 38 +++++ src/main/runtime/orca-runtime.ts | 154 +++++++++++++----- .../ssh/ssh-relay-generation-reattach.test.ts | 133 +++++++++++++++ src/main/ssh/ssh-relay-session.test.ts | 26 --- src/main/ssh/ssh-relay-session.ts | 62 +++++-- .../src/lib/pty-kill-retry-ownership.test.ts | 17 +- .../src/lib/pty-kill-retry-ownership.ts | 11 +- src/shared/ssh-types.ts | 2 + 15 files changed, 455 insertions(+), 145 deletions(-) create mode 100644 src/main/ssh/ssh-relay-generation-reattach.test.ts diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 74e0d4db845..8dc7f4208ad 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3597,20 +3597,20 @@ "renderer teardown" ], "platforms": ["macos", "windows"], - "providers": ["ssh", "remote-runtime"], + "providers": ["local", "daemon", "ssh", "remote-runtime"], "coveredPlatforms": ["macos"], - "coveredProviders": ["ssh"], - "coverageNotes": "A macOS unit harness loads the installed patched node-pty WindowsTerminal prototype without constructing native state, proving pre-ready kill/destroy dispatch immediately. A relay artifact harness reproduces vanilla npm node-pty retaining one deferred kill with zero native close calls, then proves both Windows relay artifacts hash, ship, apply, include the local ConPTY console-list fallback, and idempotently verify the fix. Relay tests simulate win32 shutdown, repeated stale-spawn native failure, one-shot ownership, and retry after native failure without deleting the only handle before close is proven. Relay boot UUIDs scope recycled pty-N counters through initial spawn, durable-lease reattach, and SSH target-id migration, while pane identity protects older relays. Renderer pre-handler exits and retained shutdown retries are capped at 64, and lifecycle retry performs at most one attempt with backoff. Project removal keeps local or remote ownership until provider-verified shutdown and successful VM cleanup; runtime repo removal drains headless, renderer-backed mobile, renderer split, and selector-less renderer admissions before fresh liveness, stop, and metadata mutation. Targeted terminal.stop probes skip unrelated sessions. The same relay contract serves paired web/mobile clients; no live Windows ConPTY or Windows-over-SSH run is recorded yet.", + "coveredProviders": ["local", "ssh"], + "coverageNotes": "A macOS unit harness loads the installed patched node-pty WindowsTerminal prototype without constructing native state, proving pre-ready kill/destroy dispatch immediately. A relay artifact harness reproduces vanilla npm node-pty retaining one deferred kill with zero native close calls, then proves both Windows relay artifacts hash, ship, apply, include the local ConPTY console-list fallback, and idempotently verify the fix. Relay tests simulate win32 shutdown, repeated stale-spawn native failure, one-shot ownership, and retry after native failure without deleting the only handle before close is proven. Relay boot UUIDs scope recycled pty-N counters through initial spawn, durable-lease persistence, reattach, and SSH target-id migration; mismatched generations expire before raw attach, while pane identity protects legacy leases. Renderer retry ownership keeps one coalesced record per still-live unconfirmed PTY and lifecycle retry performs at most one attempt with backoff; it never drops a live owner merely to cap bookkeeping. Local Windows shutdown retains provider records and listeners after native throw unless onExit independently proves death. Project removal keeps local or remote ownership until provider-verified shutdown and successful VM cleanup; runtime repo removal drains create-first worktree creation plus headless, renderer-backed mobile, renderer split, and selector-less renderer terminal admissions before fresh ownership/liveness snapshots, stop, and metadata mutation. Targeted terminal.stop probes skip unrelated sessions. The same relay contract serves paired web/mobile clients; no live Windows ConPTY or Windows-over-SSH run is recorded yet.", "motivatingLinks": [ "https://github.com/stablyai/orca/issues/8260", "https://github.com/stablyai/orca/pull/8279" ], - "invariant": "A Windows relay PTY closed before its first output executes exactly one no-argument node-pty shutdown immediately; Orca does not delete ownership for a native kill failure, and a later retry can target only the same relay generation and pane while project removal fences new terminal admission.", - "oracle": "Load the installed patched WindowsTerminal prototype in a pre-ready state and assert kill and destroy each close the terminal and native agent once with zero deferred work, while signal arguments still throw. Build both Windows SSH relay artifacts, reproduce vanilla npm node-pty with close=0/native-kill=0/deferred=1, apply the shipped patch, and assert close=1/native-kill=1/deferred=0, AttachConsole failure falls back to the shell pid, and the artifact version derives from all patch bytes. Assert remote npm install executes patch enforcement before its runtime probe and never finalizes a failed patch. In the relay, assert immediate, graceful, stale-spawn fallback, and handler-dispose paths preserve ownership through native throw and issue at most one successful no-argument shutdown. Assert reattach and target migration preserve relay generation. In the renderer, reject the first kill, retain exact identity, and assert the next lifecycle/removal retry succeeds before ownership state is purged. Start each terminal creation surface before project removal and assert removal performs no liveness snapshot until the create or split publishes its PTY; failed and concurrent admissions must release without deadlock.", + "invariant": "A Windows PTY closed before its first output executes exactly one no-argument node-pty shutdown immediately; Orca never deletes ownership without accepted native shutdown or an observed exit, and a later retry can target only the same relay generation and pane while project removal fences both worktree and terminal creation.", + "oracle": "Load the installed patched WindowsTerminal prototype in a pre-ready state and assert kill and destroy each close the terminal and native agent once with zero deferred work, while signal arguments still throw. Build both Windows SSH relay artifacts, reproduce vanilla npm node-pty with close=0/native-kill=0/deferred=1, apply the shipped patch, and assert close=1/native-kill=1/deferred=0, AttachConsole failure falls back to the shell pid, and the artifact version derives from all patch bytes. Assert remote npm install executes patch enforcement before its runtime probe and never finalizes a failed patch. In local and relay providers, assert native throws retain exact ownership/listeners and retry succeeds, while a synchronous onExit is accepted as independent death proof. Persist relay generation with every durable lease, expire mismatches before attach, and preserve generation through target migration. Reject 65 renderer kills, retain all 65 exact owners, then prove one lifecycle event retries at most one and 65 events clear all 65. Start worktree and terminal creation surfaces before project removal and assert removal performs no ownership/liveness snapshot until they publish; removal-first and failed/concurrent admissions must reject or release without deadlock.", "commands": [ "pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts", - "pnpm exec vitest run --config config/vitest.config.ts src/shared/ssh-pty-id.test.ts src/main/providers/ssh-pty-provider.test.ts src/main/ssh/ssh-relay-session.test.ts src/main/ssh/ssh-target-id-migration.test.ts src/relay/pty-handler.test.ts src/relay/subprocess.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts src/renderer/src/lib/pty-kill-retry-ownership.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts src/renderer/src/store/slices/repos.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/pty.test.ts" + "pnpm exec vitest run --config config/vitest.config.ts src/shared/ssh-pty-id.test.ts src/shared/ssh-types.test.ts src/main/providers/local-pty-provider.test.ts src/main/providers/ssh-pty-provider.test.ts src/main/persistence.test.ts src/main/ssh/ssh-relay-session.test.ts src/main/ssh/ssh-relay-generation-reattach.test.ts src/main/ssh/ssh-target-id-migration.test.ts src/relay/pty-handler.test.ts src/relay/subprocess.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts src/renderer/src/lib/pty-kill-retry-ownership.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts src/renderer/src/store/slices/repos.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/pty.test.ts" ], "testFiles": [ "config/scripts/build-relay-windows-node-pty-patch.test.mjs", @@ -3624,8 +3624,12 @@ "src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", "src/main/runtime/orca-runtime.test.ts", "src/shared/ssh-pty-id.test.ts", + "src/shared/ssh-types.test.ts", + "src/main/providers/local-pty-provider.test.ts", "src/main/providers/ssh-pty-provider.test.ts", + "src/main/persistence.test.ts", "src/main/ssh/ssh-relay-session.test.ts", + "src/main/ssh/ssh-relay-generation-reattach.test.ts", "src/main/ssh/ssh-target-id-migration.test.ts", "src/relay/subprocess.test.ts", "src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts", @@ -3700,6 +3704,7 @@ "provider shutdown rejection fails terminal.stop closed", "project removal fences paired terminal admission through provider stop and repo mutation", "project removal drains create-first headless, renderer-backed mobile, renderer split, and selector-less renderer admissions before its liveness snapshot", + "project removal drains create-first worktree creation before its ownership snapshot and rejects removal-first worktree creation", "failed and concurrent admissions release without deadlocking project removal", "targeted terminal stop omits foreground probes for unrelated sessions" ] @@ -3717,9 +3722,22 @@ ] }, { - "file": "src/main/ssh/ssh-relay-session.test.ts", + "file": "src/main/providers/local-pty-provider.test.ts", "assertions": [ - "durable-lease reattach rebuilds app PTY ownership with the current relay generation" + "a synchronous Windows native kill throw retains PTY ownership and listeners so retry can succeed" + ] + }, + { + "file": "src/main/persistence.test.ts", + "assertions": [ + "durable SSH leases store raw relay ids together with their relay generation" + ] + }, + { + "file": "src/main/ssh/ssh-relay-generation-reattach.test.ts", + "assertions": [ + "durable leases attach only within their minting relay generation", + "an older-generation lease expires before its recycled raw id can attach" ] }, { @@ -3743,7 +3761,7 @@ { "file": "src/renderer/src/lib/pty-kill-retry-ownership.test.ts", "assertions": [ - "retained shutdown ids are capped at 64 and one lifecycle retries at most one id" + "all 65 unconfirmed live shutdown ids retain exact ownership while one lifecycle retries at most one id" ] }, { @@ -3761,7 +3779,7 @@ "command": "pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", "result": "passed", "durationSeconds": 2, - "summary": "200 tests passed, including built Windows relay red/green and ConPTY fallback parity, repeated stale-spawn throw containment, renderer retained retry identity, local/remote project removal fail-closed ownership, and VM cleanup propagation." + "summary": "202 tests passed, including built Windows relay red/green and ConPTY fallback parity, repeated stale-spawn throw containment, renderer retained retry identity, local/remote project removal fail-closed ownership, and VM cleanup propagation." }, { "date": "2026-07-12", @@ -3776,10 +3794,10 @@ "date": "2026-07-12", "runner": "local", "platform": "macos", - "command": "pnpm exec vitest run --config config/vitest.config.ts src/shared/ssh-pty-id.test.ts src/main/providers/ssh-pty-provider.test.ts src/main/ssh/ssh-relay-session.test.ts src/main/ssh/ssh-target-id-migration.test.ts src/relay/pty-handler.test.ts src/relay/subprocess.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts src/renderer/src/lib/pty-kill-retry-ownership.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts src/renderer/src/store/slices/repos.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/pty.test.ts", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/shared/ssh-pty-id.test.ts src/shared/ssh-types.test.ts src/main/providers/local-pty-provider.test.ts src/main/providers/ssh-pty-provider.test.ts src/main/persistence.test.ts src/main/ssh/ssh-relay-session.test.ts src/main/ssh/ssh-relay-generation-reattach.test.ts src/main/ssh/ssh-target-id-migration.test.ts src/relay/pty-handler.test.ts src/relay/subprocess.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts src/renderer/src/lib/pty-kill-retry-ownership.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts src/renderer/src/store/slices/repos.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/pty.test.ts", "result": "passed", "durationSeconds": 12, - "summary": "1247 tests passed with relay-generation identity preserved across reattach and migration, stale-spawn retained ownership, bounded renderer maps/retry work, VM cleanup success ordering, all renderer/runtime removal admissions drained, and targeted liveness probes." + "summary": "1678 tests passed with durable relay-generation persistence and mismatch rejection, local native-throw retry ownership, all 65 renderer shutdown owners retained and drained, stale-spawn retained ownership, worktree/terminal admission ordering, VM cleanup success ordering, and targeted liveness probes." }, { "date": "2026-07-12", @@ -3788,7 +3806,7 @@ "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts", "result": "passed", "durationSeconds": 18, - "summary": "668 runtime tests passed, including removal-first rejection and create-first admission drain for headless, renderer-backed mobile, renderer split, and selector-less renderer paths before provider liveness snapshots." + "summary": "669 runtime tests passed, including removal-first rejection and create-first admission drain for worktree creation, headless terminals, renderer-backed mobile, renderer split, and selector-less renderer paths before ownership/liveness snapshots." } ], "runtimeBudget": { @@ -3801,11 +3819,11 @@ }, "redGreenEvidence": { "status": "partial", - "evidence": "The built relay artifact harness records the vanilla npm dependency at close=0, native-kill=0, deferred=1 and the shipped patched artifact at close=1, native-kill=1, deferred=0. Without relay one-shot ownership, graceful fallback and dispose issue duplicate kills." + "evidence": "The built relay artifact harness records the vanilla npm dependency at close=0, native-kill=0, deferred=1 and the shipped patched artifact at close=1, native-kill=1, deferred=0. Without relay one-shot ownership, graceful fallback and dispose issue duplicate kills. The former 65-id renderer oracle proved the oldest still-live owner was dropped; the corrected oracle retries all 65. Native local kill throw now remains visible in listProcesses until retry, and a stale durable lease is rejected before attach when its persisted generation differs from the relay boot." }, "performanceBudget": { "required": true, - "evidence": "Shutdown removes indefinite deferred retention and preserves at most one in-flight native kill plus one existing graceful timer per PTY request. Failed renderer kills retain at most 64 generation-scoped ids; one lifecycle event attempts at most one retry with exponential backoff. Pre-handler exits are capped at 64. Targeted runtime stop still performs one provider verification per target PTY plus two liveness snapshots, while foreground probes are limited to the target worktree." + "evidence": "Shutdown removes indefinite deferred retention and preserves at most one in-flight native kill plus one existing graceful timer per PTY request. Failed renderer kills coalesce by exact live PTY id: repeated failures for one id remain one record, while distinct records correspond one-for-one with still-unconfirmed provider resources and are never discarded before accepted shutdown. One lifecycle event attempts at most one retry with exponential backoff; 65 events deterministically drain 65 owners after recovery. Pre-handler exits remain capped at 64. Worktree-create and terminal admissions add no polling and exist only for active operations. Targeted runtime stop still performs one provider verification per target PTY plus two liveness snapshots, while foreground probes are limited to the target worktree." }, "promotionCriteria": [ "Run the deterministic command across the required CI platforms without flakes.", diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 82f1c06f4da..2569de265be 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -10627,7 +10627,7 @@ describe('registerPtyHandlers', () => { expect(piClearPtyMock).toHaveBeenCalledWith(spawnResult.id) }) - it('disposes PTY listeners before manual kill IPC', async () => { + it('keeps PTY listeners until manual native kill is accepted', async () => { const onDataDisposable = makeDisposable() const onExitDisposable = makeDisposable() // Why: hold a stable reference to the kill spy. On POSIX, destroyPtyProcess @@ -10654,15 +10654,15 @@ describe('registerPtyHandlers', () => { await handlers.get('pty:kill')!(null, { id: spawnResult.id }) - expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan( + onDataDisposable.dispose.mock.invocationCallOrder[0] ) - expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan( + onExitDisposable.dispose.mock.invocationCallOrder[0] ) }) - it('disposes PTY listeners before runtime controller kill', async () => { + it('keeps PTY listeners until runtime-controller native kill is accepted', async () => { const onDataDisposable = makeDisposable() const onExitDisposable = makeDisposable() const killSpy = vi.fn() @@ -10695,11 +10695,11 @@ describe('registerPtyHandlers', () => { } expect(runtimeController.kill(spawnResult.id)).toBe(true) - expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan( + onDataDisposable.dispose.mock.invocationCallOrder[0] ) - expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan( + onExitDisposable.dispose.mock.invocationCallOrder[0] ) }) @@ -10994,7 +10994,7 @@ describe('registerPtyHandlers', () => { expect(ids).toContain(ptyB.id) }) - it('clears PTY state even when kill reports the process is already gone', async () => { + it('retains PTY state when native kill throws without an exit proof', async () => { const proc = { onData: vi.fn(() => makeDisposable()), onExit: vi.fn(() => makeDisposable()), @@ -11014,11 +11014,15 @@ describe('registerPtyHandlers', () => { rows: 24 })) as { id: string } - await handlers.get('pty:kill')!(null, { id: spawnResult.id }) + await expect(handlers.get('pty:kill')!(null, { id: spawnResult.id })).rejects.toThrow( + 'already dead' + ) - expect(await handlers.get('pty:hasChildProcesses')!(null, { id: spawnResult.id })).toBe(false) - expect(openCodeClearPtyMock).toHaveBeenCalledWith(spawnResult.id) - expect(piClearPtyMock).toHaveBeenCalledWith(spawnResult.id) + expect((await getLocalPtyProvider().listProcesses()).map(({ id }) => id)).toContain( + spawnResult.id + ) + expect(openCodeClearPtyMock).not.toHaveBeenCalledWith(spawnResult.id) + expect(piClearPtyMock).not.toHaveBeenCalledWith(spawnResult.id) }) describe('agent_started telemetry', () => { diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 91f1bc1f77c..f3e8d870a22 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -3174,6 +3174,7 @@ export function registerPtyHandlers( markNativeWindowsConptyPty(result.id) } const relayResultId = getRelayPtyId(args.connectionId, result.id) + const relayInstanceId = parseAppSshPtyId(result.id)?.relayInstanceId const persistSshLease = (): void => { if (!store || !args.connectionId) { return @@ -3183,6 +3184,7 @@ export function registerPtyHandlers( store.upsertSshRemotePtyLease({ targetId: args.connectionId, ptyId: relayResultId, + ...(relayInstanceId ? { relayInstanceId } : {}), ...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}), ...(typeof args.tabId === 'string' ? { tabId: args.tabId } : {}), ...(typeof args.leafId === 'string' && isTerminalLeafId(args.leafId) @@ -4153,6 +4155,7 @@ export function registerPtyHandlers( } } const relayResultId = getRelayPtyId(args.connectionId, result.id) + const relayInstanceId = parseAppSshPtyId(result.id)?.relayInstanceId if (store && args.connectionId) { // Why: remote PTYs live in the SSH relay grace window after Orca // detaches. Persist their IDs immediately so reconnect can reattach @@ -4160,6 +4163,7 @@ export function registerPtyHandlers( store.upsertSshRemotePtyLease({ targetId: args.connectionId, ptyId: relayResultId, + ...(relayInstanceId ? { relayInstanceId } : {}), ...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}), ...(typeof args.tabId === 'string' ? { tabId: args.tabId } : {}), ...(validatedLeafId ? { leafId: validatedLeafId } : {}), diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index b9acb16ae66..69fc5546cea 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -8783,12 +8783,12 @@ describe('Store', () => { expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({}) }) - it('stores scoped SSH remote PTY leases as raw relay ids', async () => { + it('stores scoped SSH remote PTY leases as raw ids with their relay generation', async () => { const store = await createStore() store.upsertSshRemotePtyLease({ targetId: 'ssh-1', - ptyId: 'ssh:ssh-1@@remote-pty', + ptyId: 'ssh:ssh-1@@relay-generation@@remote-pty', state: 'attached' }) @@ -8796,6 +8796,7 @@ describe('Store', () => { expect.objectContaining({ targetId: 'ssh-1', ptyId: 'remote-pty', + relayInstanceId: 'relay-generation', state: 'attached' }) ]) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 9d32c98d6bc..4e13e43caf1 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -116,7 +116,7 @@ import { toSshExecutionHostId, type ExecutionHostId } from '../shared/execution-host' -import { toRelaySshPtyId } from './providers/ssh-pty-id' +import { parseAppSshPtyId, toRelaySshPtyId } from './providers/ssh-pty-id' import { migrateUiHostScopeSshTargetId, migrateWorkspaceSessionSshTargetId @@ -6317,8 +6317,10 @@ export class Store { if (normalizedLease.leafId !== undefined && !isTerminalLeafId(normalizedLease.leafId)) { delete normalizedLease.leafId } + const parsedPtyId = parseAppSshPtyId(normalizedLease.ptyId) + normalizedLease.relayInstanceId ??= parsedPtyId?.relayInstanceId // Why: app-facing SSH PTY ids are globally scoped; durable relay leases - // stay target-local so reconnect can call relay pty.attach with raw ids. + // keep raw ids plus their boot generation so reconnect can reject reuse. normalizedLease.ptyId = this.getRelayPtyIdForSshLeaseStorage( normalizedLease.targetId, normalizedLease.ptyId diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 66f68d6e868..6cf14f593fc 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -980,6 +980,27 @@ describe('LocalPtyProvider', () => { expect(destroySpy).not.toHaveBeenCalled() }) + it('retains Windows ownership when native shutdown throws so retry can succeed', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const kill = vi.fn().mockImplementationOnce(() => { + throw new Error('native close failed') + }) + spawnMock.mockReturnValue({ ...mockProc, kill }) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow( + 'native close failed' + ) + expect(provider.hasPty(id)).toBe(true) + expect(await provider.listProcesses()).toEqual( + expect.arrayContaining([expect.objectContaining({ id })]) + ) + + await expect(provider.shutdown(id, { immediate: true })).resolves.toBeUndefined() + expect(kill).toHaveBeenCalledTimes(2) + expect(provider.hasPty(id)).toBe(false) + }) + it('cancels pending shell-ready startup delivery on forced shutdown', async () => { vi.useFakeTimers() try { diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 99f35fee47d..e8231d11c20 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -145,6 +145,9 @@ function disposePtyListeners(id: string): void { } } +const ptyShutdownInProgress = new Set() +const ptyExitDuringShutdown = new Map() + function runPtyCleanup(id: string): void { const cleanup = ptyCleanupCallbacks.get(id) if (!cleanup) { @@ -186,6 +189,8 @@ function clearPtyState(id: string): void { ptyAgentForegroundContextPaths.delete(id) ptyTerminalHandle.delete(id) ptyLoadGeneration.delete(id) + ptyShutdownInProgress.delete(id) + ptyExitDuringShutdown.delete(id) } /** @@ -797,6 +802,10 @@ export class LocalPtyProvider implements IPtyProvider { if (process.platform !== 'win32') { ;(proc as unknown as { kill: (sig?: string) => void }).kill = () => {} } + if (ptyShutdownInProgress.has(id)) { + ptyExitDuringShutdown.set(id, exitCode) + return + } if (shellReadyTimeout) { clearTimeout(shellReadyTimeout) shellReadyTimeout = null @@ -893,23 +902,31 @@ export class LocalPtyProvider implements IPtyProvider { if (!proc) { return } - // Why: disposePtyListeners removes the onExit callback, so the natural - // exit cleanup path from node-pty won't fire. Cleanup and notification - // must happen unconditionally after the try/catch. - // Timer/writer cleanup must happen here too: disposing listeners prevents - // the natural onExit callback from running the usual clearPtyState path. - runPtyCleanup(id) - disposePtyListeners(id) + // Why: keep listeners live until native shutdown is accepted so a thrown + // kill retains retry ownership. The in-progress marker makes a synchronous + // onExit an independent death proof without racing duplicate cleanup. + ptyShutdownInProgress.add(id) + let exitCode = -1 try { proc.kill() - } catch { - /* Process may already be dead */ + } catch (error) { + if (!ptyExitDuringShutdown.has(id)) { + throw error + } + } finally { + ptyShutdownInProgress.delete(id) + exitCode = ptyExitDuringShutdown.get(id) ?? -1 + ptyExitDuringShutdown.delete(id) } + // Timer/writer cleanup happens only after native shutdown was accepted; + // disposing first would suppress the natural exit path on a thrown kill. + runPtyCleanup(id) + disposePtyListeners(id) destroyPtyProcess(proc, { alreadyKilled: true }) clearPtyState(id) - this.opts.onExit?.(id, -1) + this.opts.onExit?.(id, exitCode) for (const cb of exitListeners) { - cb({ id, code: -1 }) + cb({ id, code: exitCode }) } } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index c553c710b08..3bc4056b461 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -22258,7 +22258,11 @@ describe('OrcaRuntimeService', () => { await expect(runtime.createTerminal(`id:${TEST_WORKTREE_ID}`)).rejects.toThrow( 'terminal_admission_blocked_for_project_removal' ) + await expect( + runtime.createManagedWorktree({ repoSelector: `id:${TEST_REPO_ID}`, name: 'too-late' }) + ).rejects.toThrow('project_removal_in_progress') expect(spawn).not.toHaveBeenCalled() + expect(addWorktree).not.toHaveBeenCalled() runtime.onPtyExit('pty-1', -1) stop.resolve(true) @@ -22301,6 +22305,40 @@ describe('OrcaRuntimeService', () => { expect(removeProject).toHaveBeenCalledWith(TEST_REPO_ID) }) + it('drains a create-first managed worktree before project removal snapshots ownership', async () => { + const removeProject = vi.fn() + const runtime = new OrcaRuntimeService({ ...store, removeProject } as never) + const runCreate = deferred() + const runtimeWithCreate = runtime as unknown as { + runCreateManagedWorktree: () => Promise + } + const runCreateSpy = vi + .spyOn(runtimeWithCreate, 'runCreateManagedWorktree') + .mockReturnValue(runCreate.promise) + runtime.setPtyController({ + write: () => true, + kill: () => false, + getForegroundProcess: async () => null, + listProcesses: async () => [] + }) + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { tabs: [], leaves: [] }) + + const create = runtime.createManagedWorktree({ + repoSelector: `id:${TEST_REPO_ID}`, + name: 'create-first' + }) + await vi.waitFor(() => expect(runCreateSpy).toHaveBeenCalledOnce()) + const removal = runtime.removeProject(`id:${TEST_REPO_ID}`) + await Promise.resolve() + expect(removeProject).not.toHaveBeenCalled() + + runCreate.resolve(undefined as never) + await expect(create).resolves.toBeUndefined() + await expect(removal).resolves.toEqual({ removed: true }) + expect(removeProject).toHaveBeenCalledWith(TEST_REPO_ID) + }) + it.each([ ['renderer-backed mobile create', 'mobile'], ['selector-less renderer create', 'global'] diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 6d1b5ce7862..1a56a7b0fdb 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2122,6 +2122,43 @@ async function hasLocalWorktreeBaseRef( ) } +export type CreateManagedWorktreeArgs = { + repoSelector: string + name: string + baseBranch?: string + compareBaseRef?: string + branchNameOverride?: string + linkedIssue?: number | null + linkedPR?: number | null + linkedLinearIssue?: string + linkedLinearIssueWorkspaceId?: string | null + linkedLinearIssueOrganizationUrlKey?: string | null + linkedGitLabMR?: number | null + linkedGitLabIssue?: number | null + linkedBitbucketPR?: number | null + linkedAzureDevOpsPR?: number | null + linkedGiteaPR?: number | null + comment?: string + displayName?: string + telemetrySource?: WorkspaceCreateTelemetrySource + workspaceStatus?: string + manualOrder?: number + sparseCheckout?: { directories: string[]; presetId?: string } + pushTarget?: GitPushTarget + runHooks?: boolean + activate?: boolean + setupDecision?: 'run' | 'skip' | 'inherit' + createdWithAgent?: TuiAgent + startupAgent?: TuiAgent + startupPrompt?: string + pendingFirstAgentMessageRename?: boolean + automationProvenance?: AutomationWorkspaceProvenance + startup?: WorktreeStartupLaunch + startupDraft?: string + startupDraftPaste?: WorktreeStartupDraftPaste + lineage?: WorktreeLineageInput +} + export class OrcaRuntimeService { private readonly runtimeId = randomUUID() private readonly startedAt = Date.now() @@ -2229,6 +2266,8 @@ export class OrcaRuntimeService { // respect to paired/mobile terminal creation, or a late spawn is orphaned. private terminalAdmissionBlockedWorktreeIds = new Set() private terminalAdmissionsByWorktreeId = new Map>>() + private projectRemovalBlockedRepoIds = new Set() + private worktreeCreateAdmissionsByRepoId = new Map>>() private titleObservationSequence = 0 private headlessTerminals = new Map() private ptyOutputSequenceById = new Map() @@ -12247,21 +12286,71 @@ export class OrcaRuntimeService { } } + private beginWorktreeCreateAdmission(repoId: string): () => void { + if (this.projectRemovalBlockedRepoIds.has(repoId)) { + throw new Error('project_removal_in_progress') + } + let resolveDone = (): void => {} + const done = new Promise((resolve) => { + resolveDone = resolve + }) + const admissions = this.worktreeCreateAdmissionsByRepoId.get(repoId) ?? new Set() + admissions.add(done) + this.worktreeCreateAdmissionsByRepoId.set(repoId, admissions) + let released = false + return () => { + if (released) { + return + } + released = true + admissions.delete(done) + if (admissions.size === 0) { + this.worktreeCreateAdmissionsByRepoId.delete(repoId) + } + resolveDone() + } + } + + private async withWorktreeCreateAdmission( + repoId: string, + action: () => Promise + ): Promise { + const release = this.beginWorktreeCreateAdmission(repoId) + try { + return await action() + } finally { + release() + } + } + + private async drainWorktreeCreateAdmissions(repoId: string): Promise { + await Promise.all([...(this.worktreeCreateAdmissionsByRepoId.get(repoId) ?? [])]) + } + async removeProject(repoSelector: string): Promise<{ removed: true }> { if (!this.store?.removeProject) { throw new Error('runtime_unavailable') } const repo = await this.resolveRepoSelector(repoSelector) - const worktreeIds = (await this.listResolvedWorktrees()) - .filter((worktree) => worktree.repoId === repo.id) - .map((worktree) => worktree.id) - if (worktreeIds.some((id) => this.terminalAdmissionBlockedWorktreeIds.has(id))) { + if (this.projectRemovalBlockedRepoIds.has(repo.id)) { throw new Error('project_removal_in_progress') } - for (const id of worktreeIds) { - this.terminalAdmissionBlockedWorktreeIds.add(id) - } + this.projectRemovalBlockedRepoIds.add(repo.id) + let worktreeIds: string[] = [] try { + // Why: a create that already resolved this repo can publish a new + // worktree and terminal after a one-time removal snapshot. Block later + // creates and let earlier ones finish before enumerating owned worktrees. + await this.drainWorktreeCreateAdmissions(repo.id) + worktreeIds = (await this.listResolvedWorktrees()) + .filter((worktree) => worktree.repoId === repo.id) + .map((worktree) => worktree.id) + if (worktreeIds.some((id) => this.terminalAdmissionBlockedWorktreeIds.has(id))) { + throw new Error('project_removal_in_progress') + } + for (const id of worktreeIds) { + this.terminalAdmissionBlockedWorktreeIds.add(id) + } // Why: a create that passed admission before the block can still have a // provider spawn in flight. Let it register first so the fresh stop // snapshot sees and terminates it. @@ -12280,6 +12369,7 @@ export class OrcaRuntimeService { for (const id of worktreeIds) { this.terminalAdmissionBlockedWorktreeIds.delete(id) } + this.projectRemovalBlockedRepoIds.delete(repo.id) } } @@ -14604,42 +14694,20 @@ export class OrcaRuntimeService { }) } - async createManagedWorktree(args: { - repoSelector: string - name: string - baseBranch?: string - compareBaseRef?: string - branchNameOverride?: string - linkedIssue?: number | null - linkedPR?: number | null - linkedLinearIssue?: string - linkedLinearIssueWorkspaceId?: string | null - linkedLinearIssueOrganizationUrlKey?: string | null - linkedGitLabMR?: number | null - linkedGitLabIssue?: number | null - linkedBitbucketPR?: number | null - linkedAzureDevOpsPR?: number | null - linkedGiteaPR?: number | null - comment?: string - displayName?: string - telemetrySource?: WorkspaceCreateTelemetrySource - workspaceStatus?: string - manualOrder?: number - sparseCheckout?: { directories: string[]; presetId?: string } - pushTarget?: GitPushTarget - runHooks?: boolean - activate?: boolean - setupDecision?: 'run' | 'skip' | 'inherit' - createdWithAgent?: TuiAgent - startupAgent?: TuiAgent - startupPrompt?: string - pendingFirstAgentMessageRename?: boolean - automationProvenance?: AutomationWorkspaceProvenance - startup?: WorktreeStartupLaunch - startupDraft?: string - startupDraftPaste?: WorktreeStartupDraftPaste - lineage?: WorktreeLineageInput - }): Promise { + async createManagedWorktree(args: CreateManagedWorktreeArgs): Promise { + if (!this.store) { + throw new Error('runtime_unavailable') + } + + const repo = await this.resolveRepoSelector(args.repoSelector) + return await this.withWorktreeCreateAdmission(repo.id, () => + this.runCreateManagedWorktree(args) + ) + } + + private async runCreateManagedWorktree( + args: CreateManagedWorktreeArgs + ): Promise { if (!this.store) { throw new Error('runtime_unavailable') } diff --git a/src/main/ssh/ssh-relay-generation-reattach.test.ts b/src/main/ssh/ssh-relay-generation-reattach.test.ts new file mode 100644 index 00000000000..403fa16af5f --- /dev/null +++ b/src/main/ssh/ssh-relay-generation-reattach.test.ts @@ -0,0 +1,133 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { SshRelaySession } from './ssh-relay-session' +import { createMockDeps, mockDeploySuccess } from './ssh-relay-session-test-fixtures' + +vi.mock('./ssh-relay-deploy', () => ({ deployAndLaunchRelay: vi.fn() })) + +vi.mock('./ssh-channel-multiplexer', () => ({ + SshChannelMultiplexer: class MockSshChannelMultiplexer { + notify = vi.fn() + request = vi.fn().mockResolvedValue([]) + onNotification = vi.fn().mockReturnValue(() => {}) + onRequest = vi.fn().mockReturnValue(() => {}) + onDispose = vi.fn().mockReturnValue(() => {}) + dispose = vi.fn() + isDisposed = vi.fn().mockReturnValue(false) + } +})) + +vi.mock('../providers/ssh-pty-provider', () => ({ + isSshPtyNotFoundError: () => false, + isSshPtyIdentityMismatchError: () => false, + SshPtyProvider: class MockSshPtyProvider { + onData = vi.fn().mockReturnValue(() => {}) + onReplay = vi.fn().mockReturnValue(() => {}) + onExit = vi.fn().mockReturnValue(() => {}) + dispose = vi.fn() + } +})) + +vi.mock('../providers/ssh-filesystem-provider', () => ({ + SshFilesystemProvider: class MockSshFilesystemProvider { + dispose = vi.fn() + } +})) + +vi.mock('../providers/ssh-git-provider', () => ({ + SshGitProvider: class MockSshGitProvider {} +})) + +vi.mock('../ipc/pty', () => ({ + registerSshPtyProvider: vi.fn(), + unregisterSshPtyProvider: vi.fn(), + getSshPtyProvider: vi.fn(), + getPtyIdsForConnection: vi.fn().mockReturnValue([]), + clearPtyOwnershipForConnection: vi.fn(), + clearProviderPtyState: vi.fn(), + deletePtyOwnership: vi.fn(), + setPtyOwnership: vi.fn(), + answerStartupTerminalColorQueriesForPty: vi.fn((_id: string, data: string) => data) +})) + +vi.mock('../providers/ssh-filesystem-dispatch', () => ({ + registerSshFilesystemProvider: vi.fn(), + unregisterSshFilesystemProvider: vi.fn(), + getSshFilesystemProvider: vi.fn().mockReturnValue({ dispose: vi.fn() }) +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + registerSshGitProvider: vi.fn(), + unregisterSshGitProvider: vi.fn() +})) + +const { getSshPtyProvider, getPtyIdsForConnection, setPtyOwnership } = await import('../ipc/pty') + +describe('SSH relay generation reattach', () => { + beforeEach(() => { + vi.clearAllMocks() + mockDeploySuccess() + }) + + it('reattaches a durable lease only within the relay generation that minted it', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const attachForReconnect = vi.fn().mockResolvedValue({}) + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect, + getRelayInstanceId: () => 'relay-generation', + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ + { + targetId: 'target-1', + ptyId: 'pty-live', + relayInstanceId: 'relay-generation', + state: 'detached' + }, + { targetId: 'target-1', ptyId: 'pty-expired', state: 'expired' } + ] as ReturnType) + + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + await session.establish(mockConn) + + expect(attachForReconnect).toHaveBeenCalledWith('pty-live') + expect(attachForReconnect).not.toHaveBeenCalledWith('pty-expired') + expect(setPtyOwnership).toHaveBeenCalledWith( + 'ssh:target-1@@relay-generation@@pty-live', + 'target-1' + ) + }) + + it('expires an older-generation lease before its recycled relay id can attach', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps() + const attachForReconnect = vi.fn() + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect, + getRelayInstanceId: () => 'relay-current', + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ + { + targetId: 'target-1', + ptyId: 'pty-recycled', + relayInstanceId: 'relay-old', + state: 'detached' + } + ] as ReturnType) + + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + await session.establish(mockConn) + + expect(attachForReconnect).not.toHaveBeenCalled() + expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + 'target-1', + 'pty-recycled', + 'expired' + ) + expect(mockWindow.webContents.send).toHaveBeenCalledWith('pty:exit', { + id: 'ssh:target-1@@relay-old@@pty-recycled', + code: -1 + }) + }) +}) diff --git a/src/main/ssh/ssh-relay-session.test.ts b/src/main/ssh/ssh-relay-session.test.ts index d05e33891d1..8f0984ca76e 100644 --- a/src/main/ssh/ssh-relay-session.test.ts +++ b/src/main/ssh/ssh-relay-session.test.ts @@ -556,32 +556,6 @@ describe('SshRelaySession', () => { expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('target-1', 'pty-1', 'attached') }) - it('establish re-attaches durable leases after app restart', async () => { - const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() - const { getSshPtyProvider } = await import('../ipc/pty') - const mockAttach = vi.fn().mockResolvedValue(undefined) - vi.mocked(getSshPtyProvider).mockReturnValue({ - attachForReconnect: mockAttach, - getRelayInstanceId: () => 'relay-generation', - dispose: vi.fn() - } as unknown as ReturnType) - vi.mocked(getPtyIdsForConnection).mockReturnValue([]) - vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([ - { targetId: 'target-1', ptyId: 'pty-live', state: 'detached' }, - { targetId: 'target-1', ptyId: 'pty-expired', state: 'expired' } - ] as ReturnType) - - const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) - - await session.establish(mockConn) - - expect(mockAttach).toHaveBeenCalledWith('pty-live') - expect(mockAttach).not.toHaveBeenCalledWith('pty-expired') - const expectedAppPtyId = 'ssh:target-1@@relay-generation@@pty-live' - expect(setPtyOwnership).toHaveBeenCalledWith(expectedAppPtyId, 'target-1') - expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('target-1', 'pty-live', 'attached') - }) - it('forwards a lease tab identity to reattach so a reset relay cannot cross-wire it', async () => { const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() const { getSshPtyProvider } = await import('../ipc/pty') diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 91abca9dfdb..eb62fc71830 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -19,7 +19,7 @@ import { isSshPtyIdentityMismatchError, isSshPtyNotFoundError } from '../providers/ssh-pty-provider' -import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' +import { parseAppSshPtyId, toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' import { SshFilesystemProvider } from '../providers/ssh-filesystem-provider' import { SshGitProvider } from '../providers/ssh-git-provider' import { agentHookServer } from '../agent-hooks/server' @@ -1200,16 +1200,43 @@ export class SshRelaySession { } } + private expireReattachPty(relayPtyId: string, appPtyId: string): void { + clearProviderPtyState(appPtyId) + deletePtyOwnership(appPtyId) + this.forwardedReattachReplayByPty.delete(appPtyId) + this.store.markSshRemotePtyLease(this.targetId, relayPtyId, 'expired') + const win = this.getMainWindow() + if (win && !win.isDestroyed()) { + win.webContents.send('pty:exit', { id: appPtyId, code: -1 }) + } + } + private async reattachKnownPtys(shouldContinue: () => boolean): Promise { + const ptyProvider = getSshPtyProvider(this.targetId) as SshPtyProvider | undefined + if (!ptyProvider) { + return + } + const relayInstanceId = ptyProvider.getRelayInstanceId?.() const activeLeases = this.store .getSshRemotePtyLeases(this.targetId) .filter((lease) => lease.state !== 'terminated' && lease.state !== 'expired') - const leasedPtyIds = activeLeases.map((lease) => lease.ptyId) + const attachableLeases = activeLeases.filter((lease) => { + if (!lease.relayInstanceId || !relayInstanceId || lease.relayInstanceId === relayInstanceId) { + return true + } + const staleAppPtyId = toAppSshPtyId(this.targetId, lease.ptyId, lease.relayInstanceId) + console.warn( + `[ssh-relay-session] Expiring stale PTY ${lease.ptyId} for ${this.targetId} after relay generation changed` + ) + this.expireReattachPty(lease.ptyId, staleAppPtyId) + return false + }) + const leasedPtyIds = attachableLeases.map((lease) => lease.ptyId) // Why: carry each lease's pane identity into the attach so the relay can // reject cross-generation id collisions even between split panes in one tab. // Older leases may lack leafId, so tabId remains the compatibility fallback. const expectedIdentityByPtyId = new Map( - activeLeases + attachableLeases .map((lease): [string, ExpectedPtyIdentity] | null => { const expected = expectedIdentityForLease(lease) return expected ? [lease.ptyId, expected] : null @@ -1218,19 +1245,22 @@ export class SshRelaySession { ) // Why: after app restart, ptyOwnership is empty but durable SSH leases // still describe remote PTYs that survived in the relay grace window. - const ptyIds = Array.from( - new Set([ - ...getPtyIdsForConnection(this.targetId).map((ptyId) => - toRelaySshPtyId(this.targetId, ptyId) - ), - ...leasedPtyIds - ]) - ) - const ptyProvider = getSshPtyProvider(this.targetId) as SshPtyProvider | undefined - if (!ptyProvider) { - return - } - const relayInstanceId = ptyProvider.getRelayInstanceId?.() + const ownedPtyIds = getPtyIdsForConnection(this.targetId).flatMap((appPtyId) => { + const parsed = parseAppSshPtyId(appPtyId) + if ( + parsed?.relayInstanceId && + relayInstanceId && + parsed.relayInstanceId !== relayInstanceId + ) { + console.warn( + `[ssh-relay-session] Expiring stale PTY ${parsed.relayPtyId} for ${this.targetId} after relay generation changed` + ) + this.expireReattachPty(parsed.relayPtyId, appPtyId) + return [] + } + return [toRelaySshPtyId(this.targetId, appPtyId)] + }) + const ptyIds = Array.from(new Set([...ownedPtyIds, ...leasedPtyIds])) for (const ptyId of ptyIds) { if (!shouldContinue()) { return diff --git a/src/renderer/src/lib/pty-kill-retry-ownership.test.ts b/src/renderer/src/lib/pty-kill-retry-ownership.test.ts index 3b968b70565..be2f0f0469d 100644 --- a/src/renderer/src/lib/pty-kill-retry-ownership.test.ts +++ b/src/renderer/src/lib/pty-kill-retry-ownership.test.ts @@ -16,7 +16,7 @@ describe('PTY kill retry ownership', () => { vi.restoreAllMocks() }) - it('bounds retained ids and retries at most one per lifecycle event', async () => { + it('retains every live id and retries at most one per lifecycle event', async () => { const kill = vi.fn().mockRejectedValue(new Error('provider disconnected')) vi.stubGlobal('window', { api: { pty: { kill } } }) vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -26,12 +26,15 @@ describe('PTY kill retry ownership', () => { } kill.mockResolvedValue(undefined) - retryRetainedPtyKills() - await Promise.resolve() - await Promise.resolve() + for (const _id of IDS) { + retryRetainedPtyKills() + await Promise.resolve() + await Promise.resolve() + } - expect(kill).toHaveBeenCalledTimes(66) - expect(kill.mock.calls.filter(([id]) => id === IDS[0])).toHaveLength(1) - expect(kill.mock.calls.filter(([id]) => id === IDS[1])).toHaveLength(2) + expect(kill).toHaveBeenCalledTimes(IDS.length * 2) + for (const id of IDS) { + expect(kill.mock.calls.filter(([calledId]) => calledId === id)).toHaveLength(2) + } }) }) diff --git a/src/renderer/src/lib/pty-kill-retry-ownership.ts b/src/renderer/src/lib/pty-kill-retry-ownership.ts index 1c4e5c137e9..5b910020553 100644 --- a/src/renderer/src/lib/pty-kill-retry-ownership.ts +++ b/src/renderer/src/lib/pty-kill-retry-ownership.ts @@ -11,7 +11,6 @@ export type PtyKillIdentity = { expectedTabId?: string } -const MAX_RETAINED_PTY_KILLS = 64 const MAX_RETRY_BACKOFF_MS = 30_000 const retainedPtyKills = new Map() @@ -26,13 +25,9 @@ export function killPtyRetainingRetryOwnership( ): Promise { let retained = retainedPtyKills.get(id) if (!retained) { - if (retainedPtyKills.size >= MAX_RETAINED_PTY_KILLS) { - const oldestId = retainedPtyKills.keys().next().value - if (typeof oldestId === 'string') { - retainedPtyKills.delete(oldestId) - console.warn('[pty] Dropped oldest retained shutdown retry at bounded capacity') - } - } + // Why: every entry represents a provider process whose shutdown is still + // unconfirmed. Dropping one to cap bookkeeping would orphan the real PTY; + // repeated failures for the same PTY still coalesce into this single record. retained = { diagnostic, inFlight: null, options, attempts: 0, nextRetryAt: 0 } } retained.diagnostic = diagnostic diff --git a/src/shared/ssh-types.ts b/src/shared/ssh-types.ts index 5794cb5226c..2b16b2ea1e8 100644 --- a/src/shared/ssh-types.ts +++ b/src/shared/ssh-types.ts @@ -119,6 +119,8 @@ export type SshRemotePtyLeaseState = 'attached' | 'detached' | 'terminated' | 'e export type SshRemotePtyLease = { targetId: string ptyId: string + /** Relay boot that minted ptyId; absent only on legacy persisted leases. */ + relayInstanceId?: string worktreeId?: string tabId?: string leafId?: string