diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 10cfd209354..bdc4294cd57 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3558,9 +3558,9 @@ { "file": "src/relay/pty-handler.test.ts", "assertions": [ - "Windows immediate, graceful/fallback, and relay-dispose shutdown issue one no-argument kill", + "every Windows shutdown attempt stays signal-free and repeats only while physical exit remains unproved", "stale spawn native failures retain one managed PTY and retry automatically within eight bounded attempts", - "explicit shutdown native failure retains one managed PTY and a later caller retry clears it" + "explicit shutdown retains one managed PTY until onExit or OS liveness proves the child is gone" ] }, { @@ -3607,6 +3607,7 @@ "host-scoped terminal stop succeeds when an unrelated provider is unavailable without disconnecting that host's cached PTYs", "workspace resolution on an unrelated host proceeds while the removed host remains admission-fenced", "a matching host removal that starts and finishes during unresolved workspace selection still rejects the stale create", + "a different host removal that starts and finishes during unresolved workspace selection does not reject the resolved owner", "worktree removal rejects a late renderer terminal admission while destructive preflight is still pending", "already-missing exceptional removal verifies terminal stop before metadata purge", "failed and concurrent admissions release without deadlocking project removal", @@ -3630,6 +3631,7 @@ "file": "src/main/providers/local-pty-provider.test.ts", "assertions": [ "a synchronous Windows native kill throw retains PTY ownership and listeners so retry can succeed", + "an accepted native kill retains the PTY until an exit callback proves physical shutdown", "a stale pane identity is rejected before the local PTY can be killed", "immediate POSIX local shutdown uses SIGKILL while Windows remains signal-free" ] @@ -3637,7 +3639,8 @@ { "file": "src/main/daemon/daemon-pty-adapter.test.ts", "assertions": [ - "daemon shutdown carries pane identity across RPC and rejects a stale kill" + "daemon shutdown carries pane identity across RPC and rejects a stale kill", + "preserved pre-v22 sessions fail closed when restart cannot recover pane identity for attach or shutdown" ] }, { @@ -3701,7 +3704,10 @@ "paired/mobile controller close requires immediate provider shutdown and fails closed when offline SSH intent cannot be made durable", "host-scoped liveness lists only the selected provider instead of fanning out to unrelated SSH hosts", "relay generation reuse removes stale retry ownership without touching the new PTY", - "lease-less SSH shutdown survives provider unregister and re-drives the replacement transport" + "lease-less SSH shutdown survives provider unregister and re-drives the replacement transport", + "accepted local and SSH kills retain retry ownership until provider readback proves physical exit", + "an old provider exit readback cannot settle replacement-provider retry ownership", + "unprovable pre-v22 restart intent retires once without synthesizing exit, retrying forever, or killing by recycled id" ] }, { @@ -3762,7 +3768,8 @@ "file": "src/renderer/src/store/slices/worktrees.test.ts", "assertions": [ "renderer worktree creation sends the active duplicate repo owner host through desktop IPC", - "a focus switch during create cannot restamp the result onto another duplicate host" + "a focus switch during create cannot restamp the result onto another duplicate host", + "runtime-owned SSH cleanup removes duplicate repo ids only on each destroyed target host" ] }, { @@ -3928,11 +3935,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. 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, a disconnected SSH provider retains its lease without a synthetic exit, and a stale durable lease is rejected before attach when its persisted generation differs from the relay boot. A real worktree-removal preflight deterministically rejects a late renderer terminal admission before mutation; host-scoped removal drains or rejects both race orderings, and hidden-session cleanup retry preserves expected tab identity after rejection." + "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. Accepted local and relay kills now remain provider-visible until onExit or conservative OS liveness proves physical exit; retries stay signal-free on Windows. The former 65-id renderer oracle proved the oldest still-live owner was dropped; the corrected oracle retries all 65. A disconnected SSH provider retains its lease without a synthetic exit, and a stale durable lease is rejected before attach when its persisted generation differs from the relay boot. A real worktree-removal preflight deterministically rejects a late matching-host admission while allowing an unrelated host removal; duplicate-id runtime SSH cleanup is host-qualified." }, "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 local and SSH kills coalesce in main by exact live PTY id, while failed remote-runtime closes coalesce by environment, runtime generation, and handle: distinct durable records correspond one-for-one with still-unconfirmed provider resources and survive renderer/app restart. Each main-owned retry map owns one shared one-shot timer for its earliest deadline, retries all due owners, caps backoff at 30 seconds, and logs only attempts 1, 2, and 8 to prevent outage backpressure. Successful SSH-target and runtime-environment removal release unreachable records and cancel the last retry timer; provider replacement rebinds the same generation but relay/runtime generation replacement drops stale intent before touching a reused id. The deterministic 65-local plus 65-remote-runtime burst performs zero synchronous full-store flushes: 260 O(1) append records stay below 100 KiB and 1 KiB each, survive immediate add/remove reloads, and compact to one checkpoint after the full-state save; a two-crash oracle proves a torn final record forces an atomic recovered checkpoint before a new owner is added, while a monotonic revision fences the rare full-store I/O-failure fallback against older journal records. Remote-runtime close adds one generation status probe per teardown attempt and is not on input, output, render, polling, or startup-visible paths. Pre-handler exits remain capped at 64. Worktree-create, worktree-removal, whole-project, and host-scoped admissions retain one promise only per active operation, including pre-spawn asynchronous trust/team/command preparation; removal drains only the target worktree's current admissions. Targeted runtime stop performs two liveness snapshots against only the selected provider, while foreground probes are limited to the target worktree; whole-project inventory retains the all-provider fanout." + "evidence": "Shutdown removes indefinite deferred retention and preserves at most one provider entry plus one existing graceful timer per PTY request until physical exit is proved. Failed local and SSH kills coalesce in main by exact live PTY id, while failed remote-runtime closes coalesce by environment, runtime generation, and handle: distinct durable records correspond one-for-one with still-unconfirmed provider resources and survive renderer/app restart. Each main-owned retry map owns one shared one-shot timer for its earliest deadline, retries all due owners, caps backoff at 30 seconds, and logs only attempts 1, 2, and 8 to prevent outage backpressure. Successful SSH-target and runtime-environment removal release unreachable records and cancel the last retry timer; provider replacement rebinds the same generation but relay/runtime generation replacement drops stale intent before touching a reused id. The deterministic 65-local plus 65-remote-runtime burst performs zero synchronous full-store flushes: 260 O(1) append records stay below 100 KiB and 1 KiB each, survive immediate add/remove reloads, and compact to one checkpoint after the full-state save; a two-crash oracle proves a torn final record forces an atomic recovered checkpoint before a new owner is added, while a monotonic revision fences the rare full-store I/O-failure fallback against older journal records. Remote-runtime close adds one generation status probe per teardown attempt and is not on input, output, render, polling, or startup-visible paths. Pre-handler exits remain capped at 64. Worktree-create, worktree-removal, whole-project, and host-scoped admissions retain one promise only per active operation; unresolved selection retains scope-generation history only while at least one selection is active and clears it at zero. Targeted runtime stop performs two liveness snapshots against only the selected provider, while foreground probes are limited to the target worktree; whole-project inventory retains the all-provider fanout." }, "promotionCriteria": [ "Run the deterministic command across the required CI platforms without flakes.", diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index d7e5560e4e3..92d9432f01c 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -133,6 +133,40 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { expect(typeof result.id).toBe('string') }) + it('fails closed when a preserved legacy session has no observable pane identity', async () => { + const legacy = new DaemonPtyAdapter({ socketPath, tokenPath, protocolVersion: 21 }) + const internals = legacy as unknown as { + client: { + ensureConnected: () => Promise + request: () => Promise + } + } + vi.spyOn(internals.client, 'ensureConnected').mockResolvedValue(undefined) + const request = vi.spyOn(internals.client, 'request').mockResolvedValue({ + sessions: [ + { + sessionId: 'legacy-session', + isAlive: true, + cwd: '/repo' + } + ] + }) + try { + await legacy.listProcesses() + await expect( + legacy.spawn({ + sessionId: 'legacy-session', + cols: 80, + rows: 24, + env: { ORCA_PANE_KEY: 'tab-b:leaf-b', ORCA_TAB_ID: 'tab-b' } + }) + ).rejects.toThrow('Legacy PTY identity unavailable') + expect(request).toHaveBeenCalledOnce() + } finally { + legacy.dispose() + } + }) + it('uses worktreeId as session prefix when provided', async () => { const result = await adapter.spawn({ cols: 80, rows: 24, worktreeId: 'wt-1' }) expect(result.id).toContain('wt-1') @@ -377,6 +411,21 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { }) describe('shutdown', () => { + it('retires identity-bearing shutdowns that cannot be proven on a restarted legacy adapter', async () => { + const legacy = new DaemonPtyAdapter({ socketPath, tokenPath, protocolVersion: 21 }) + try { + await expect( + legacy.shutdown('legacy-session', { + immediate: true, + expectedPaneKey: 'tab-a:leaf-a', + expectedTabId: 'tab-a' + }) + ).rejects.toThrow('Legacy PTY identity unavailable') + } finally { + legacy.dispose() + } + }) + it('carries pane identity through the daemon and rejects a stale kill', async () => { const { id } = await adapter.spawn({ cols: 80, diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index be38e03ce4b..8422de7636f 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -202,6 +202,10 @@ export class DaemonPtyAdapter implements IPtyProvider { expectedPaneKey: requestedIdentity.paneKey ?? undefined, expectedTabId: requestedIdentity.tabId ?? undefined }) + } else if (requestedIdentity.paneKey || requestedIdentity.tabId) { + // Why: preserved legacy daemons cannot report identity, so attaching a + // new pane by a reused ID could later authorize killing the old pane. + throw new Error(`Legacy PTY identity unavailable for "${sessionId}"`) } } @@ -521,11 +525,11 @@ export class DaemonPtyAdapter implements IPtyProvider { ) { // Why: preserved pre-v22 daemons ignore kill identity fields. Fail // closed unless this main process observed the exact attached pane. - assertPtyPaneIdentity( - id, - this.paneIdentityBySessionId.get(id) ?? { paneKey: null, tabId: null }, - opts - ) + const knownIdentity = this.paneIdentityBySessionId.get(id) + if (!knownIdentity) { + throw new Error(`Legacy PTY identity unavailable for "${id}"`) + } + assertPtyPaneIdentity(id, knownIdentity, opts) } // Why: sleep/exact-stop kills the live PTY before the periodic checkpoint may run. // Force a final snapshot so wake can restore the pane users left. @@ -796,6 +800,13 @@ export class DaemonPtyAdapter implements IPtyProvider { const result = await this.client.request('listSessions', undefined) for (const session of result.sessions) { this.rememberSessionIdentity(session) + if (session.isAlive) { + // Why: router startup discovers preserved legacy sessions through this + // method; spawn must know the ID is already live before createOrAttach. + this.activeSessionIds.add(session.sessionId) + } else { + this.activeSessionIds.delete(session.sessionId) + } } return result.sessions .filter((s) => s.isAlive) diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 72d82eb4b10..a4447ff4eb4 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -411,12 +411,16 @@ describe('registerPtyHandlers', () => { } ) isPwshAvailableMock.mockReturnValue(false) + let defaultExitHandler: ((event: { exitCode: number }) => void) | null = null spawnMock.mockReturnValue({ onData: vi.fn(() => makeDisposable()), - onExit: vi.fn(() => makeDisposable()), + onExit: vi.fn((handler: (event: { exitCode: number }) => void) => { + defaultExitHandler = handler + return makeDisposable() + }), write: vi.fn(), resize: vi.fn(), - kill: vi.fn(), + kill: vi.fn(() => defaultExitHandler?.({ exitCode: -1 })), process: 'zsh', pid: 12345 }) @@ -3494,10 +3498,124 @@ describe('registerPtyHandlers', () => { }) expect(store.removePendingLocalPtyShutdown).toHaveBeenCalledWith('local-pty') await vi.advanceTimersByTimeAsync(30_000) + await vi.advanceTimersByTimeAsync(30_000) + vi.useRealTimers() + }) + + it('keeps accepted local kills until provider readback proves physical exit', async () => { + vi.useFakeTimers() + const shutdown = vi.fn().mockResolvedValue(undefined) + const listProcesses = vi + .fn() + .mockResolvedValueOnce([{ id: 'local-pty', cwd: '', title: 'shell' }]) + .mockResolvedValue([]) + const removePendingLocalPtyShutdown = vi.fn() + setLocalPtyProvider({ + requiresShutdownExitProof: true, + shutdown, + listProcesses, + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + } as never) + handlers.clear() + registerPtyHandlers(mainWindow as never, undefined, undefined, undefined, undefined, { + getPendingLocalPtyShutdowns: () => [], + upsertPendingLocalPtyShutdown: vi.fn(), + removePendingLocalPtyShutdown + } as never) + + await expect(handlers.get('pty:kill')!(null, { id: 'local-pty' })).rejects.toThrow( + 'process exit is not yet confirmed' + ) + expect(removePendingLocalPtyShutdown).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(250) + + expect(shutdown).toHaveBeenCalledTimes(2) + expect(removePendingLocalPtyShutdown).toHaveBeenCalledWith('local-pty') + vi.useRealTimers() + }) + + it('does not let an old provider exit readback settle replacement retry ownership', async () => { + vi.useFakeTimers() + type ProcessInfo = { id: string; cwd: string; title: string } + let resolveOldReadback!: (processes: ProcessInfo[]) => void + const oldReadback = new Promise((resolve) => { + resolveOldReadback = resolve + }) + const oldListProcesses = vi.fn(() => oldReadback) + const newShutdown = vi.fn().mockResolvedValue(undefined) + const provider = ( + shutdown: (id: string) => Promise, + listProcesses: () => Promise + ) => + ({ + requiresShutdownExitProof: true, + shutdown, + listProcesses, + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + }) as never + setLocalPtyProvider(provider(vi.fn().mockResolvedValue(undefined), oldListProcesses)) + handlers.clear() + registerPtyHandlers(mainWindow as never, undefined, undefined, undefined, undefined, { + getPendingLocalPtyShutdowns: () => [], + upsertPendingLocalPtyShutdown: vi.fn(), + removePendingLocalPtyShutdown: vi.fn() + } as never) + + const kill = handlers.get('pty:kill')!(null, { id: 'local-pty' }) as Promise + await vi.waitFor(() => expect(oldListProcesses).toHaveBeenCalledOnce()) + setLocalPtyProvider(provider(newShutdown, vi.fn().mockResolvedValue([]))) + resolveOldReadback([]) + + await expect(kill).resolves.toBeUndefined() + expect(newShutdown).toHaveBeenCalledOnce() + await vi.advanceTimersByTimeAsync(30_000) expect(vi.getTimerCount()).toBe(0) vi.useRealTimers() }) + it('retires a persisted legacy-daemon shutdown that cannot recover pane identity', async () => { + vi.useFakeTimers() + const removePendingLocalPtyShutdown = vi.fn() + const onPtyExit = vi.fn() + setLocalPtyProvider({ + shutdown: vi.fn().mockRejectedValue(new Error('Legacy PTY identity unavailable for "p"')), + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + } as never) + handlers.clear() + registerPtyHandlers( + mainWindow as never, + { onPtyExit, setPtyController: vi.fn() } as never, + undefined, + undefined, + undefined, + { + getPendingLocalPtyShutdowns: () => [ + { + ptyId: 'legacy-pty', + expectedPaneKey: 'tab-a:leaf-a', + expectedTabId: 'tab-a', + requestedAt: 1 + } + ], + upsertPendingLocalPtyShutdown: vi.fn(), + removePendingLocalPtyShutdown + } as never + ) + + await vi.advanceTimersByTimeAsync(0) + + expect(removePendingLocalPtyShutdown).toHaveBeenCalledWith('legacy-pty') + expect(onPtyExit).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(30_000) + vi.useRealTimers() + }) + it('re-drives a direct local kill when its provider is replaced in flight', async () => { vi.useFakeTimers() const oldShutdown = makeDeferred() @@ -11331,10 +11449,14 @@ describe('registerPtyHandlers', () => { // in local-pty-provider reassigns proc.kill to a no-op to defuse the // SIGHUP-to-recycled-pid hazard (see docs/fix-pty-fd-leak.md). Reading // proc.kill.mock after that runs would yield a non-mock and crash. - const killSpy = vi.fn() + let exitCb: ((info: { exitCode: number }) => void) | null = null + const killSpy = vi.fn(() => exitCb?.({ exitCode: -1 })) const proc = { onData: vi.fn(() => onDataDisposable), - onExit: vi.fn(() => onExitDisposable), + onExit: vi.fn((cb: (info: { exitCode: number }) => void) => { + exitCb = cb + return onExitDisposable + }), write: vi.fn(), resize: vi.fn(), kill: killSpy, @@ -11362,10 +11484,14 @@ describe('registerPtyHandlers', () => { it('keeps PTY listeners until runtime-controller native kill is accepted', async () => { const onDataDisposable = makeDisposable() const onExitDisposable = makeDisposable() - const killSpy = vi.fn() + let exitCb: ((info: { exitCode: number }) => void) | null = null + const killSpy = vi.fn(() => exitCb?.({ exitCode: -1 })) const proc = { onData: vi.fn(() => onDataDisposable), - onExit: vi.fn(() => onExitDisposable), + onExit: vi.fn((cb: (info: { exitCode: number }) => void) => { + exitCb = cb + return onExitDisposable + }), write: vi.fn(), resize: vi.fn(), kill: killSpy, diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 539a341eb42..fbed685ef2b 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -188,6 +188,7 @@ type RetainedPtyShutdown = { } const pendingSshShutdownRetries = new Map() const pendingLocalShutdownRetries = new Map() +const sshRetainedExitUnsubscribers = new Map void>() let retainedShutdownRetryTimer: ReturnType | null = null const MAX_RETAINED_SHUTDOWN_BACKOFF_MS = 30_000 const SYNTHETIC_KILL_EXIT_DUPLICATE_WINDOW_MS = 30_000 @@ -627,6 +628,19 @@ function completeRetainedPtyShutdown( } } +function retireUnprovableLegacyPtyShutdown(retained: RetainedPtyShutdown): void { + const ownerMap = retained.connectionId ? pendingSshShutdownRetries : pendingLocalShutdownRetries + if (ownerMap.get(retained.id) !== retained) { + return + } + // Why: legacy restart loses pane identity permanently. Drop only the stale + // durable request; without exit proof, provider/UI ownership must survive. + ownerMap.delete(retained.id) + if (!retained.connectionId) { + retained.store?.removePendingLocalPtyShutdown?.(retained.id) + } +} + function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise { if (retained.inFlight) { return retained.inFlight @@ -642,11 +656,22 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise { + .then(async (providerExitObserved) => { // Why: provider replacement can finish while an older shutdown is in flight. if (retained.provider !== currentProvider) { return } + if (currentProvider.requiresShutdownExitProof && !providerExitObserved) { + const stopped = await verifyPtyStopped(currentProvider, retained.id, retained.options) + // Why: provider replacement can also occur during the async liveness + // readback; only the provider that still owns this retry may settle it. + if (retained.provider !== currentProvider) { + return + } + if (!stopped) { + throw new Error('PTY shutdown accepted but process exit is not yet confirmed') + } + } completeRetainedPtyShutdown(retained, providerExitObserved) }) .catch((error: unknown) => { @@ -663,6 +688,12 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise { + const retained = pendingSshShutdownRetries.get(payload.id) + if ( + retained?.connectionId === connectionId && + retained.provider === provider && + sshProviders.get(connectionId) === provider + ) { + // Why: exit may arrive after shutdown RPC acceptance. Settle the exact + // provider owner here so its next timer cannot synthesize a duplicate. + completeRetainedPtyShutdown(retained, true) + } + }) + ) + } // Durable leases reconcile generation first; a stale boot-qualified owner // must be dropped before any replacement provider can observe it. retryPersistedSshShutdowns(connectionId, provider) @@ -1363,6 +1413,8 @@ export function registerSshPtyProvider(connectionId: string, provider: IPtyProvi /** Remove an SSH PTY provider when a connection is closed. */ export function unregisterSshPtyProvider(connectionId: string): void { + sshRetainedExitUnsubscribers.get(connectionId)?.() + sshRetainedExitUnsubscribers.delete(connectionId) sshProviders.delete(connectionId) for (const retained of pendingSshShutdownRetries.values()) { if (retained.connectionId === connectionId) { @@ -2010,6 +2062,10 @@ export function registerPtyHandlers( }, onSpawned: (id) => runtime?.onPtySpawned(id), onExit: (id, code) => { + const retained = pendingLocalShutdownRetries.get(id) + if (retained?.provider === localProvider) { + completeRetainedPtyShutdown(retained, true) + } clearProviderPtyState(id) ptyOwnership.delete(id) markClaudePtyExited(id) @@ -3145,6 +3201,10 @@ export function registerPtyHandlers( return } if (!isLocalProvider) { + const retained = pendingLocalShutdownRetries.get(payload.id) + if (retained?.provider === localProvider) { + completeRetainedPtyShutdown(retained, true) + } clearProviderPtyState(payload.id) ptyOwnership.delete(payload.id) markClaudePtyExited(payload.id) diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 4107ab26135..29731e1cb50 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -978,6 +978,20 @@ describe('LocalPtyProvider', () => { expect(killSpy).toHaveBeenCalledWith('SIGKILL') }) + it('retains the native PTY until an accepted kill produces exit proof', async () => { + mockProc.kill.mockImplementation(() => {}) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + await provider.shutdown(id, { immediate: true }) + + expect(provider.hasPty(id)).toBe(true) + expect(await provider.listProcesses()).toEqual( + expect.arrayContaining([expect.objectContaining({ id })]) + ) + exitCb?.({ exitCode: 137 }) + expect(provider.hasPty(id)).toBe(false) + }) + it('invokes onExit callback via the node-pty exit handler', async () => { const onExit = vi.fn() provider.configure({ onExit }) @@ -988,7 +1002,7 @@ describe('LocalPtyProvider', () => { it('does not destroy after an intentional Windows shutdown kill', async () => { Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - const killSpy = vi.fn() + const killSpy = vi.fn(() => exitCb?.({ exitCode: -1 })) const destroySpy = vi.fn(() => { killSpy() }) @@ -1007,9 +1021,12 @@ describe('LocalPtyProvider', () => { 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') - }) + const kill = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('native close failed') + }) + .mockImplementationOnce(() => exitCb?.({ exitCode: -1 })) spawnMock.mockReturnValue({ ...mockProc, kill }) const { id } = await provider.spawn({ cols: 80, rows: 24 }) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 42ed79bae34..df410e4196c 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -328,6 +328,7 @@ export type LocalPtyProviderOptions = { } export class LocalPtyProvider implements IPtyProvider { + readonly requiresShutdownExitProof = true private opts: LocalPtyProviderOptions constructor(opts: LocalPtyProviderOptions = {}) { @@ -916,6 +917,7 @@ export class LocalPtyProvider implements IPtyProvider { // onExit an independent death proof without racing duplicate cleanup. ptyShutdownInProgress.add(id) let exitCode = -1 + let exitObserved = false try { // Why: a renderer or paired/mobile close is destructive. POSIX's // default SIGHUP can be ignored; Windows node-pty requires no argument. @@ -931,17 +933,20 @@ export class LocalPtyProvider implements IPtyProvider { } finally { ptyShutdownInProgress.delete(id) exitCode = ptyExitDuringShutdown.get(id) ?? -1 + exitObserved = ptyExitDuringShutdown.has(id) 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. + // Why: kill returning is request acceptance, not process-death proof. Keep + // the native entry and listeners until onExit (possibly async) confirms it. runPtyCleanup(id) - disposePtyListeners(id) - destroyPtyProcess(proc, { alreadyKilled: true }) - clearPtyState(id) - this.opts.onExit?.(id, exitCode) - for (const cb of exitListeners) { - cb({ id, code: exitCode }) + if (exitObserved) { + disposePtyListeners(id) + destroyPtyProcess(proc, { alreadyKilled: true }) + clearPtyState(id) + this.opts.onExit?.(id, exitCode) + for (const cb of exitListeners) { + cb({ id, code: exitCode }) + } } } diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index ca4171bdb5a..0b2b3a6e4d3 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -39,6 +39,7 @@ export function isSshPtyIdentityMismatchError(err: unknown): boolean { * as LocalPtyProvider so the dispatch layer can route transparently. */ export class SshPtyProvider implements IPtyProvider { + readonly requiresShutdownExitProof = true private mux: SshChannelMultiplexer private connectionId: string private dataListeners = new Set() diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index d304d586607..10565e422dd 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -161,6 +161,9 @@ export type PtyProcessInfo = { } export type IPtyProvider = { + /** A returned kill request is only acceptance; retain ownership until an + * exit event or listProcesses readback proves the backing process is gone. */ + readonly requiresShutdownExitProof?: boolean spawn(opts: PtySpawnOptions): Promise attach(id: string): Promise hasPty?: (id: string) => boolean diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 9d2e49b49c8..58b2f25980e 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -23406,6 +23406,59 @@ describe('OrcaRuntimeService', () => { await localRemoval }) + it('allows a resolved create when only another host removal starts during resolution', async () => { + const runtime = new OrcaRuntimeService(store) + const resolution = deferred<{ + id: string + path: string + hostId: 'ssh:target-1' + connectionId: 'target-1' + repo: ReturnType + folderWorkspace: null + }>() + const runtimeWithResolver = runtime as unknown as { + resolveTerminalWorkspaceLaunchScope: () => typeof resolution.promise + } + vi.spyOn(runtimeWithResolver, 'resolveTerminalWorkspaceLaunchScope').mockReturnValue( + resolution.promise + ) + const spawn = vi.fn().mockResolvedValue({ id: 'ssh-pty' }) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => false, + getForegroundProcess: async () => null, + listProcesses: async () => [] + }) + const holdLocalRemoval = deferred() + const localRemoval = runtime.runWithWorktreeRemovalAdmission( + TEST_WORKTREE_ID, + () => holdLocalRemoval.promise, + 'local' + ) + await Promise.resolve() + + const create = runtime.createTerminal(`id:${TEST_WORKTREE_ID}`) + await runtime.runWithWorktreeRemovalAdmission( + TEST_WORKTREE_ID, + async () => undefined, + 'ssh:target-2' + ) + resolution.resolve({ + id: TEST_WORKTREE_ID, + path: '/home/dev/orca', + hostId: 'ssh:target-1', + connectionId: 'target-1', + repo: store.getRepo(TEST_REPO_ID), + folderWorkspace: null + }) + + await expect(create).resolves.toMatchObject({ ptyId: 'ssh-pty' }) + expect(spawn).toHaveBeenCalledWith(expect.objectContaining({ connectionId: 'target-1' })) + holdLocalRemoval.resolve() + await localRemoval + }) + 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) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 4f582c862f5..087a532443c 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2263,6 +2263,8 @@ export class OrcaRuntimeService { // respect to paired/mobile terminal creation, or a late spawn is orphaned. private terminalAdmissionBlockedWorktreeIds = new Set() private terminalRemovalGeneration = 0 + private terminalRemovalGenerationByScope = new Map() + private unresolvedTerminalAdmissionCount = 0 private terminalAdmissionsByWorktreeId = new Map>>() private projectRemovalBlockedRepoIds = new Set() private worktreeCreateAdmissionsByRepoId = new Map>>() @@ -12421,9 +12423,21 @@ export class OrcaRuntimeService { private blockTerminalAdmissionScope(scopeKey: string): void { this.terminalRemovalGeneration += 1 + // Why: unresolved selectors only need short-lived history while they await + // host discovery; clearing it at zero avoids retaining removed worktree IDs. + if (this.unresolvedTerminalAdmissionCount > 0) { + this.terminalRemovalGenerationByScope.set(scopeKey, this.terminalRemovalGeneration) + } this.terminalAdmissionBlockedWorktreeIds.add(scopeKey) } + private terminalRemovalGenerationForScope(worktreeId: string, hostId: ExecutionHostId): number { + return Math.max( + this.terminalRemovalGenerationByScope.get(worktreeId) ?? 0, + this.terminalRemovalGenerationByScope.get(this.admissionScopeKey(worktreeId, hostId)) ?? 0 + ) + } + private async drainTerminalAdmissions( worktreeIds: readonly string[], hostId?: ExecutionHostId | null, @@ -12445,6 +12459,7 @@ export class OrcaRuntimeService { let releaseResolutionAdmission: (() => void) | null = null let removalsActiveAtResolutionStart: ReadonlySet | null = null let removalGenerationAtResolutionStart: number | null = null + let tracksRemovalGeneration = false try { releaseResolutionAdmission = this.beginTerminalAdmission(GLOBAL_TERMINAL_ADMISSION_KEY) } catch (error) { @@ -12459,12 +12474,15 @@ export class OrcaRuntimeService { // cannot reopen the exact stale scope before the atomic transfer. removalsActiveAtResolutionStart = new Set(this.terminalAdmissionBlockedWorktreeIds) removalGenerationAtResolutionStart = this.terminalRemovalGeneration + this.unresolvedTerminalAdmissionCount += 1 + tracksRemovalGeneration = true } try { const workspace = await this.resolveTerminalWorkspaceLaunchScope(selector, hostId) if ( removalsActiveAtResolutionStart && - (removalGenerationAtResolutionStart !== this.terminalRemovalGeneration || + ((removalGenerationAtResolutionStart ?? 0) < + this.terminalRemovalGenerationForScope(workspace.id, workspace.hostId) || this.isAdmissionBlocked(removalsActiveAtResolutionStart, workspace.id, workspace.hostId)) ) { throw new Error('terminal_admission_blocked_for_project_removal') @@ -12475,6 +12493,13 @@ export class OrcaRuntimeService { } catch (error) { releaseResolutionAdmission?.() throw error + } finally { + if (tracksRemovalGeneration) { + this.unresolvedTerminalAdmissionCount -= 1 + if (this.unresolvedTerminalAdmissionCount === 0) { + this.terminalRemovalGenerationByScope.clear() + } + } } } diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 2b7846bf93d..bd4e4d9e7ba 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -696,10 +696,21 @@ describe('PtyHandler', () => { it('keeps stale-spawn fallback ownership when the first native kill throws', async () => { const firstError = new Error('native ConPTY kill failed') - const killSpy = vi.fn().mockImplementationOnce(() => { - throw firstError - }) - const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() } + let onExitCb: ((event: { exitCode: number }) => void) | null = null + const killSpy = vi + .fn() + .mockImplementationOnce(() => { + throw firstError + }) + .mockImplementationOnce(() => onExitCb?.({ exitCode: 137 })) + const term = { + ...mockPtyInstance, + kill: killSpy, + onData: vi.fn(), + onExit: vi.fn((cb: (event: { exitCode: number }) => void) => { + onExitCb = cb + }) + } mockPtySpawn.mockReturnValue(term) await expect(dispatcher.callRequest('pty.spawn', {}, { isStale: () => true })).rejects.toThrow( @@ -713,6 +724,7 @@ describe('PtyHandler', () => { }) it('contains repeated stale-spawn throws and retains ownership until a proven retry', async () => { + let onExitCb: ((event: { exitCode: number }) => void) | null = null const killSpy = vi .fn() .mockImplementationOnce(() => { @@ -721,8 +733,16 @@ describe('PtyHandler', () => { .mockImplementationOnce(() => { throw new Error('native ConPTY kill failed') }) + .mockImplementationOnce(() => onExitCb?.({ exitCode: 137 })) const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) - const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() } + const term = { + ...mockPtyInstance, + kill: killSpy, + onData: vi.fn(), + onExit: vi.fn((cb: (event: { exitCode: number }) => void) => { + onExitCb = cb + }) + } mockPtySpawn.mockReturnValue(term) try { @@ -1118,24 +1138,24 @@ describe('PtyHandler', () => { }) }) - it('on stale-spawn cleanup without a duplicate fallback kill', async () => { + it('on stale-spawn cleanup with a signal-free proof retry', async () => { await withWindowsPlatform(async () => { const mockKill = mockKillablePty() await dispatcher.callRequest('pty.spawn', {}, { isStale: () => true }) expectBareKills(mockKill, 1) vi.advanceTimersByTime(5000) - expectBareKills(mockKill, 1) + expectBareKills(mockKill, 2) }) }) - it('on graceful shutdown without a duplicate fallback kill', async () => { + it('on graceful shutdown and its force-close fallback', async () => { await withWindowsPlatform(async () => { const mockKill = mockKillablePty() await dispatcher.callRequest('pty.spawn', {}) await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false }) expectBareKills(mockKill, 1) vi.advanceTimersByTime(5000) - expectBareKills(mockKill, 1) + expectBareKills(mockKill, 2) }) }) @@ -1193,7 +1213,7 @@ describe('PtyHandler', () => { expect(mockKill).toHaveBeenCalledWith('SIGTERM') expect(mockKill).toHaveBeenCalledWith('SIGKILL') - expect(dispatcher.notify).toHaveBeenCalledWith('pty.exit', { id: 'pty-1', code: -1 }) + expect(dispatcher.notify).toHaveBeenCalledWith('pty.exit', { id: 'pty-1', code: 137 }) expect(exits).toEqual([{ id: 'pty-1', paneKey: 'tab-fallback:0' }]) expect(handler.activePtyCount).toBe(0) }) @@ -1241,14 +1261,20 @@ describe('PtyHandler', () => { it('keeps Windows graceful shutdown and its fallback signal-free', async () => { const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') - const mockKill = vi.fn() + let onExitCb: ((event: { exitCode: number }) => void) | null = null + const mockKill = vi + .fn() + .mockImplementationOnce(() => {}) + .mockImplementationOnce(() => onExitCb?.({ exitCode: 137 })) const destroy = vi.fn(() => mockKill()) mockPtySpawn.mockReturnValue({ ...mockPtyInstance, kill: mockKill, destroy, onData: vi.fn(), - onExit: vi.fn() + onExit: vi.fn((cb: (event: { exitCode: number }) => void) => { + onExitCb = cb + }) }) Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) @@ -1257,7 +1283,7 @@ describe('PtyHandler', () => { await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false }) vi.advanceTimersByTime(5000) - expect(mockKill.mock.calls).toEqual([[]]) + expect(mockKill.mock.calls).toEqual([[], []]) expect(destroy).not.toHaveBeenCalled() expect(handler.activePtyCount).toBe(0) } finally { @@ -1269,19 +1295,22 @@ describe('PtyHandler', () => { it('keeps a Windows PTY retryable when immediate native shutdown throws', async () => { const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + let onExitCb: ((event: { exitCode: number }) => void) | null = null const mockKill = vi .fn() .mockImplementationOnce(() => { throw new Error('invalid ConPTY handle') }) - .mockImplementationOnce(() => {}) + .mockImplementationOnce(() => onExitCb?.({ exitCode: 137 })) const destroy = vi.fn(() => mockKill()) mockPtySpawn.mockReturnValue({ ...mockPtyInstance, kill: mockKill, destroy, onData: vi.fn(), - onExit: vi.fn() + onExit: vi.fn((cb: (event: { exitCode: number }) => void) => { + onExitCb = cb + }) }) Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) @@ -1305,19 +1334,23 @@ describe('PtyHandler', () => { it('does not arm Windows graceful fallback until native shutdown succeeds', async () => { const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + let onExitCb: ((event: { exitCode: number }) => void) | null = null const mockKill = vi .fn() .mockImplementationOnce(() => { throw new Error('invalid ConPTY handle') }) .mockImplementationOnce(() => {}) + .mockImplementationOnce(() => onExitCb?.({ exitCode: 137 })) const destroy = vi.fn(() => mockKill()) mockPtySpawn.mockReturnValue({ ...mockPtyInstance, kill: mockKill, destroy, onData: vi.fn(), - onExit: vi.fn() + onExit: vi.fn((cb: (event: { exitCode: number }) => void) => { + onExitCb = cb + }) }) Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) @@ -1331,7 +1364,7 @@ describe('PtyHandler', () => { await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false }) vi.advanceTimersByTime(5000) - expect(mockKill.mock.calls).toEqual([[], []]) + expect(mockKill.mock.calls).toEqual([[], [], []]) expect(handler.activePtyCount).toBe(0) } finally { if (platformDescriptor) { @@ -2000,6 +2033,8 @@ describe('PtyHandler', () => { env: { ORCA_PANE_KEY: 'tab-shutdown:0' } }) await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + expect(exits).toEqual([]) + expect(handler.activePtyCount).toBe(1) onExitCb!({ exitCode: 0 }) expect(mockKill).toHaveBeenCalledWith('SIGKILL') @@ -2008,12 +2043,15 @@ describe('PtyHandler', () => { }) it('rejects shutdown when a recycled id belongs to another pane generation', async () => { - const mockKill = vi.fn() + let onExitCb: ((event: { exitCode: number }) => void) | null = null + const mockKill = vi.fn(() => onExitCb?.({ exitCode: 137 })) mockPtySpawn.mockReturnValue({ ...mockPtyInstance, kill: mockKill, onData: vi.fn(), - onExit: vi.fn() + onExit: vi.fn((cb: (event: { exitCode: number }) => void) => { + onExitCb = cb + }) }) await dispatcher.callRequest('pty.spawn', { paneKey: 'tab-new:leaf-new', diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 2f5c35610f1..7148d13fe02 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -29,11 +29,14 @@ function killPtyForShutdown(managed: ManagedPty, signal: 'SIGTERM' | 'SIGKILL'): // Why: node-pty's Windows backend rejects signal arguments; its no-arg kill // is the equivalent shutdown operation for ConPTY. if (process.platform === 'win32') { - if (!managed.windowsShutdownKillIssued) { + managed.windowsShutdownKillIssued = true + try { managed.pty.kill() - // Why: a native kill throw can leave the process alive. Mark success only - // after return so provider ownership remains retryable on failure. - managed.windowsShutdownKillIssued = true + } catch (error) { + // Why: a native throw is not acceptance; allow the retained owner to + // retry, while the pre-call mark prevents synchronous onExit double-kill. + managed.windowsShutdownKillIssued = false + throw error } } else { managed.pty.kill(signal) @@ -486,6 +489,25 @@ export class PtyHandler { } } + private reapManagedPtyIfProvablyExited(managed: ManagedPty, code = -1): boolean { + if (managed.disposed || this.ptys.get(managed.id) !== managed) { + return true + } + const pid = managed.pty.pid + if (!pid || isProcessAlive(pid)) { + return false + } + // Why: node-pty can omit onExit after accepting a native kill. ESRCH is + // independent OS proof, so synthesize the same one-shot cleanup here. + this.flushPtyOutput(managed.id) + this.dispatcher.notify('pty.exit', { id: managed.id, code }) + this.notifyExitListener(managed) + this.ptys.delete(managed.id) + this.clearPtyFlowState(managed.id) + disposeManagedPty(managed) + return true + } + private registerHandlers(): void { this.dispatcher.onRequest('pty.spawn', (p, context) => this.spawn(p, context)) this.dispatcher.onRequest('pty.attach', (p) => this.attach(p)) @@ -735,6 +757,7 @@ export class PtyHandler { managed.killTimer = setTimeout(() => { const still = this.ptys.get(id) if (still && !still.disposed) { + still.killTimer = undefined try { killPtyForShutdown(still, 'SIGKILL') } catch (error) { @@ -753,10 +776,12 @@ export class PtyHandler { } return } - this.notifyExitListener(still) - disposeManagedPty(still) - this.ptys.delete(id) - this.clearPtyFlowState(id) + if (!this.reapManagedPtyIfProvablyExited(still)) { + fallbackAttempts += 1 + if (fallbackAttempts < STALE_SPAWN_KILL_RETRY_LIMIT) { + scheduleFallback() + } + } } }, STALE_SPAWN_KILL_RETRY_MS) } @@ -891,22 +916,9 @@ export class PtyHandler { this.releaseStartupCommand(managed) this.flushPtyOutput(id) killPtyForShutdown(managed, 'SIGKILL') - // Why: SIGKILL has already reaped the child; release the ptmx fd on the - // same tick. Deferring to onExit leaves a window where the fd is live - // with a dead child. Idempotent via the disposed guard — if onExit fires - // later and also calls disposeManagedPty, the second call is a no-op. - disposeManagedPty(managed) - // Why: mirror the graceful-shutdown killTimer cleanup. If SIGKILL's - // onExit never fires (kernel edge case: uninterruptible sleep, - // D-state child on a bad NFS mount), the disposed managed entry would - // linger in this.ptys forever. Each stranded entry consumes a slot in - // the 50-PTY cap and is returned by listProcesses/serialize. Deleting - // here makes the map hygiene a hard guarantee, not "hopefully onExit - // runs". If onExit DOES fire later, its own `this.ptys.delete(id)` is - // a no-op. - this.notifyExitListener(managed) - this.ptys.delete(id) - this.clearPtyFlowState(id) + // Why: kill returning is only acceptance. Preserve the PTY and listeners + // unless synchronous onExit or an ESRCH readback proves physical exit. + this.reapManagedPtyIfProvablyExited(managed) } else { this.releaseStartupCommand(managed) killPtyForShutdown(managed, 'SIGTERM') @@ -923,23 +935,9 @@ export class PtyHandler { managed.killTimer = setTimeout(() => { const still = this.ptys.get(id) if (still && !still.disposed) { + still.killTimer = undefined killPtyForShutdown(still, 'SIGKILL') - this.flushPtyOutput(id) - // Why: emit pty.exit BEFORE disposeManagedPty sets disposed=true. - // The natural onExit short-circuits on `managed.disposed`, so - // without this notify the renderer never learns the pane is dead - // when the SIGKILL fallback fires for a SIGTERM-ignoring child. - this.dispatcher.notify('pty.exit', { id, code: -1 }) - // Why: if SIGKILL's onExit never fires (kernel edge case, - // uninterruptible sleep, child wedged on a bad NFS mount), the - // fd and map entry would leak forever. Dispose synchronously so - // graceful-shutdown's SIGKILL fallback is a hard guarantee, not - // "hopefully onExit will run". The disposed guard inside - // disposeManagedPty makes a later onExit's dispose a no-op. - this.notifyExitListener(still) - disposeManagedPty(still) - this.ptys.delete(id) - this.clearPtyFlowState(id) + this.reapManagedPtyIfProvablyExited(still) } }, 5000) } diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index e0c32076af8..45c24d6a6e0 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -3512,6 +3512,120 @@ describe('removeWorktree state cleanup', () => { expect(deleteProjectHostSetup).toHaveBeenCalledWith({ setupId: 'setup-runtime-ssh' }) }) + it('purges a destroyed runtime SSH project by host when its repo id also exists locally', async () => { + const store = createTestStore() + const wt = makeWorktree({ + id: 'same-repo::/path/wt1', + repoId: 'same-repo', + path: '/path/wt1', + hostId: 'local' + }) + const removeProject = vi.fn().mockResolvedValue(undefined) + store.setState({ + repos: [ + { + id: 'same-repo', + path: '/local/repo', + displayName: 'local', + badgeColor: '#000', + addedAt: 0, + executionHostId: 'local' + }, + { + id: 'same-repo', + path: '/remote/repo', + displayName: 'remote', + badgeColor: '#111', + addedAt: 1, + connectionId: 'runtime-ssh-orca-1', + executionHostId: 'ssh:runtime-ssh-orca-1' + } + ], + worktreesByRepo: { 'same-repo': [wt] }, + projectHostSetups: [], + removeProject + } as unknown as Partial) + mockApi.ephemeralVm.listRuntimes.mockResolvedValueOnce([ + { + id: 'runtime-1', + workspaceId: wt.id, + cleanupStatus: 'not_started', + sshTargetId: 'runtime-ssh-orca-1' + } + ]) + + const result = await store.getState().removeWorktree(wt.id) + + expect(result).toEqual({ ok: true }) + expect(mockApi.ephemeralVm.cleanup).toHaveBeenCalledWith({ runtimeId: 'runtime-1' }) + expect(removeProject).toHaveBeenCalledWith('same-repo', { + hostId: 'ssh:runtime-ssh-orca-1' + }) + }) + + it('tracks setup purges by host when destroyed runtime SSH repos share an id', async () => { + const store = createTestStore() + const wt = makeWorktree({ + id: 'same-repo::/path/wt1', + repoId: 'same-repo', + hostId: 'local' + }) + const runtimeRepo = (targetId: string) => ({ + id: 'same-repo', + path: '/remote/repo', + displayName: targetId, + badgeColor: '#000', + addedAt: 0, + connectionId: targetId, + executionHostId: `ssh:${targetId}` as const + }) + const firstRepo = runtimeRepo('runtime-ssh-orca-1') + const secondRepo = runtimeRepo('runtime-ssh-orca-2') + const localRepo = { + id: 'same-repo', + path: '/local/repo', + displayName: 'local', + badgeColor: '#000', + addedAt: 0, + executionHostId: 'local' as const + } + const removeProject = vi.fn().mockResolvedValue(undefined) + const deleteProjectHostSetup = vi.fn().mockResolvedValue({ repo: firstRepo }) + store.setState({ + repos: [localRepo, firstRepo, secondRepo], + worktreesByRepo: { 'same-repo': [wt] }, + projectHostSetups: [ + { + id: 'setup-runtime-ssh-1', + hostId: 'ssh:runtime-ssh-orca-1' + } as unknown as AppState['projectHostSetups'][number] + ], + removeProject, + deleteProjectHostSetup + } as unknown as Partial) + mockApi.ephemeralVm.listRuntimes.mockResolvedValueOnce([ + { + id: 'runtime-1', + workspaceId: wt.id, + cleanupStatus: 'not_started', + sshTargetId: 'runtime-ssh-orca-1' + }, + { + id: 'runtime-2', + workspaceId: wt.id, + cleanupStatus: 'not_started', + sshTargetId: 'runtime-ssh-orca-2' + } + ]) + + await store.getState().removeWorktree(wt.id) + + expect(deleteProjectHostSetup).toHaveBeenCalledWith({ setupId: 'setup-runtime-ssh-1' }) + expect(removeProject).toHaveBeenCalledWith('same-repo', { + hostId: 'ssh:runtime-ssh-orca-2' + }) + }) + it('cleans up editorDrafts for files in the removed worktree', async () => { const store = createTestStore() const wt = makeWorktree({ id: 'repo1::/path/wt1', repoId: 'repo1', path: '/path/wt1' }) diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 4e70b213cd3..cf248ebbf58 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -25,7 +25,7 @@ import { getRepoIdFromWorktreeId, type WorktreeSlice } from './worktree-helpers' -import { findRepoForHost } from './repo-host-identity' +import { findRepoForHost, getRepoHostIdentity } from './repo-host-identity' import { ensureHooksConfirmed } from '@/lib/ensure-hooks-confirmed' import { cleanupEphemeralVmRuntimesForDeleted } from '@/lib/ephemeral-vm-runtime-cleanup' import { tabHasLivePty } from '@/lib/tab-has-live-pty' @@ -1276,12 +1276,12 @@ async function purgeOrphanedRuntimeSshProjects( const orphanedSetupIds = get() .projectHostSetups.filter((setup) => destroyedHostIds.has(setup.hostId)) .map((setup) => setup.id) - const purgedRepoIds = new Set() + const purgedRepoIdentities = new Set() for (const setupId of orphanedSetupIds) { try { const result = await get().deleteProjectHostSetup({ setupId }) if (result?.repo) { - purgedRepoIds.add(result.repo.id) + purgedRepoIdentities.add(getRepoHostIdentity(result.repo)) } } catch (error) { console.error('Failed to purge orphaned per-workspace-env project:', error) @@ -1289,14 +1289,19 @@ async function purgeOrphanedRuntimeSshProjects( } // A repo whose only host was the destroyed runtime can outlive its setup (the setup may be pruned // by a projection refresh first). Remove it directly so no dead, never-connectable project lingers. - const orphanedRepoIds = get() - .repos.filter( - (repo) => destroyedTargetIds.has(repo.connectionId ?? '') && !purgedRepoIds.has(repo.id) - ) - .map((repo) => repo.id) - for (const repoId of orphanedRepoIds) { + const orphanedRepos = get() + .repos.filter((repo) => { + return ( + destroyedTargetIds.has(repo.connectionId ?? '') && + !purgedRepoIdentities.has(getRepoHostIdentity(repo)) + ) + }) + .map((repo) => ({ repoId: repo.id, hostId: getRepoExecutionHostId(repo) })) + for (const { repoId, hostId } of orphanedRepos) { try { - await get().removeProject(repoId) + // Why: repo IDs are only host-local; an equal local ID must survive the + // runtime-owned SSH target cleanup. + await get().removeProject(repoId, { hostId }) } catch (error) { console.error('Failed to purge orphaned per-workspace-env repo:', error) }