diff --git a/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts b/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts index 4fbb3047d6a..6bdbe5049d6 100644 --- a/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts +++ b/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts @@ -3,7 +3,10 @@ import { PREVIOUS_DAEMON_PROTOCOL_VERSIONS, PROTOCOL_VERSION } from './types' describe('foreground-confirmation daemon protocol', () => { it('rejects daemons from before the fresh-confirmation RPC', () => { - expect(PROTOCOL_VERSION).toBeGreaterThan(19) + expect(PROTOCOL_VERSION).toBe(22) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(19) + // Why: v21 lacks full-session teardown, so the client must replace it + // instead of accepting a legacy daemon that can strand descendants. + expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(21) }) }) diff --git a/src/main/daemon/daemon-pty-router.test.ts b/src/main/daemon/daemon-pty-router.test.ts index eb8acb86488..a5de628ef65 100644 --- a/src/main/daemon/daemon-pty-router.test.ts +++ b/src/main/daemon/daemon-pty-router.test.ts @@ -239,6 +239,43 @@ describe('DaemonPtyRouter', () => { expect(current.hasPty).not.toHaveBeenCalledWith('legacy-session') }) + it('keeps multi-PTY post-shutdown verification O(T) across retained adapters', async () => { + const adapters = Array.from({ length: 21 }, (_, index) => createAdapter(`adapter-${index}`)) + const sessionIds = Array.from({ length: 50 }, (_, index) => `session-${index}`) + for (const [index, sessionId] of sessionIds.entries()) { + const adapter = adapters[index % adapters.length]! + await adapter.spawn({ sessionId, cols: 80, rows: 24 }) + } + const router = new DaemonPtyRouter({ current: adapters[0]!, legacy: adapters.slice(1) }) + await router.discoverLegacySessions() + for (const adapter of adapters) { + vi.mocked(adapter.hasPty).mockClear() + } + + for (const sessionId of sessionIds) { + await router.shutdown(sessionId, { immediate: true }) + expect(router.hasPty(sessionId)).toBe(false) + } + + expect( + adapters.reduce((count, adapter) => count + vi.mocked(adapter.shutdown).mock.calls.length, 0) + ).toBe(50) + expect( + adapters.reduce((count, adapter) => count + vi.mocked(adapter.hasPty).mock.calls.length, 0) + ).toBe(50) + }) + + it('invalidates stopped evidence when the same session id spawns again', async () => { + const current = createAdapter('current', ['reused-session']) + const router = new DaemonPtyRouter({ current, legacy: [] }) + + await router.shutdown('reused-session', { immediate: true }) + expect(router.hasPty('reused-session')).toBe(false) + await router.spawn({ sessionId: 'reused-session', cols: 80, rows: 24 }) + + expect(router.hasPty('reused-session')).toBe(true) + }) + it('fails listProcesses closed when any routed adapter cannot list sessions', async () => { const current = createAdapter('current', ['current-session']) const legacy = createAdapter('legacy', ['legacy-session']) diff --git a/src/main/daemon/daemon-pty-router.ts b/src/main/daemon/daemon-pty-router.ts index 18bf0f926b5..dc17340f72a 100644 --- a/src/main/daemon/daemon-pty-router.ts +++ b/src/main/daemon/daemon-pty-router.ts @@ -7,11 +7,13 @@ import type { PtySpawnOptions, PtySpawnResult } from '../providers/types' +import { ShutdownVerificationOwnerCache } from './shutdown-verification-owner-cache' export class DaemonPtyRouter implements IPtyProvider { private current: DaemonPtyAdapter private legacy: DaemonPtyAdapter[] private sessionAdapters = new Map() + private shutdownVerificationAdapters = new ShutdownVerificationOwnerCache() private unsubscribers: (() => void)[] = [] private dataListeners: ((payload: { id: string @@ -47,6 +49,7 @@ export class DaemonPtyRouter implements IPtyProvider { const sessions = await adapter.listProcesses() for (const session of sessions) { this.sessionAdapters.set(session.id, adapter) + this.shutdownVerificationAdapters.delete(session.id) } } catch (error) { console.warn('[daemon] Failed to discover legacy daemon sessions', error) @@ -59,14 +62,20 @@ export class DaemonPtyRouter implements IPtyProvider { const target = adapter ?? this.current const result = await target.spawn(opts) this.sessionAdapters.set(result.id, target) + this.shutdownVerificationAdapters.delete(result.id) return result } async attach(id: string): Promise { await this.adapterFor(id).attach(id) + this.shutdownVerificationAdapters.delete(id) } hasPty(id: string): boolean { + const shutdownOwner = this.shutdownVerificationAdapters.take(id) + if (shutdownOwner) { + return shutdownOwner.hasPty(id) + } const routed = this.sessionAdapters.get(id) if (routed) { return routed.hasPty(id) @@ -95,7 +104,8 @@ export class DaemonPtyRouter implements IPtyProvider { } async shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise { - await this.adapterFor(id).shutdown(id, opts) + const owner = this.adapterFor(id) + await owner.shutdown(id, opts) // Why: sleep passes keepHistory=true and re-spawns against the same // sessionId on wake. If we delete the routing entry here, adapterFor() // falls back to `this.current` on wake — for a session that originally @@ -104,6 +114,7 @@ export class DaemonPtyRouter implements IPtyProvider { // losing the cold-restore from the legacy adapter's history dir. if (!opts.keepHistory) { this.sessionAdapters.delete(id) + this.shutdownVerificationAdapters.remember(id, owner) } } @@ -156,6 +167,9 @@ export class DaemonPtyRouter implements IPtyProvider { async revive(state: string): Promise { await this.current.revive(state) + // Why: serialized revive state is opaque here; discard negative evidence + // so a restored id is discovered from its owning adapter instead. + this.shutdownVerificationAdapters.clear() } async listProcesses(): Promise { @@ -236,15 +250,18 @@ export class DaemonPtyRouter implements IPtyProvider { } for (const id of result.alive) { this.sessionAdapters.set(id, adapter) + this.shutdownVerificationAdapters.delete(id) } for (const id of result.killed) { this.sessionAdapters.delete(id) + this.shutdownVerificationAdapters.delete(id) } } return { alive, killed } } dispose(): void { + this.shutdownVerificationAdapters.clear() for (const unsubscribe of this.unsubscribers.splice(0)) { unsubscribe() } diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index fe87022e5c5..3f22d49d916 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -498,7 +498,9 @@ export class DaemonServer { sessionId: request.payload.sessionId, immediate: request.payload.immediate === true }) - this.host.kill(request.payload.sessionId, { immediate: request.payload.immediate }) + await this.host.killAndWait(request.payload.sessionId, { + immediate: request.payload.immediate + }) return {} case 'signal': diff --git a/src/main/daemon/degraded-daemon-pty-provider.test.ts b/src/main/daemon/degraded-daemon-pty-provider.test.ts index 188173816db..3a34db0c700 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.test.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.test.ts @@ -168,6 +168,65 @@ describe('DegradedDaemonPtyProvider', () => { expect(fallback.write).not.toHaveBeenCalled() }) + it('keeps multi-PTY post-shutdown verification O(T) across retained providers', async () => { + const adapters = Array.from({ length: 21 }, (_, index) => + createDaemonAdapter(`adapter-${index}`) + ) + const fallback = createProvider('fallback') + const sessionIds = Array.from({ length: 50 }, (_, index) => `session-${index}`) + for (const [index, sessionId] of sessionIds.entries()) { + const adapter = adapters[index % adapters.length]! + await adapter.spawn({ sessionId, cols: 80, rows: 24 }) + } + const provider = new DegradedDaemonPtyProvider({ + current: adapters[0]!, + legacy: adapters.slice(1), + fallback + }) + await provider.discoverDaemonSessions() + for (const adapter of adapters) { + vi.mocked(adapter.hasPty).mockClear() + } + vi.mocked(fallback.hasPty!).mockClear() + + for (const sessionId of sessionIds) { + await provider.shutdown(sessionId, { immediate: true }) + expect(provider.hasPty(sessionId)).toBe(false) + } + + expect( + adapters.reduce((count, adapter) => count + vi.mocked(adapter.shutdown).mock.calls.length, 0) + ).toBe(50) + expect( + adapters.reduce((count, adapter) => count + vi.mocked(adapter.hasPty).mock.calls.length, 0) + ).toBe(50) + expect(fallback.hasPty).not.toHaveBeenCalled() + }) + + it('invalidates stopped evidence when the same session id spawns again', async () => { + const current = createDaemonAdapter('daemon') + const fallback = createProvider('fallback', ['reused-session']) + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + + await provider.shutdown('reused-session', { immediate: true }) + expect(provider.hasPty('reused-session')).toBe(false) + await provider.spawn({ sessionId: 'reused-session', cols: 80, rows: 24 }) + + expect(provider.hasPty('reused-session')).toBe(true) + }) + + it('keeps shutdown verification conservative when the cached owner lacks hasPty', async () => { + const current = createDaemonAdapter('daemon') + const fallback = createProvider('fallback') + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + const session = await provider.spawn({ cols: 80, rows: 24 }) + delete (fallback as { hasPty?: (id: string) => boolean }).hasPty + + await provider.shutdown(session.id, { immediate: true }) + + expect(provider.hasPty(session.id)).toBe(true) + }) + it('routes authoritative recovery snapshots to the owning daemon', async () => { const current = createDaemonAdapter('daemon', ['daemon-session']) const fallback = createProvider('fallback') diff --git a/src/main/daemon/degraded-daemon-pty-provider.ts b/src/main/daemon/degraded-daemon-pty-provider.ts index 572cb0aca37..06696291114 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.ts @@ -8,6 +8,9 @@ import type { PtySpawnOptions, PtySpawnResult } from '../providers/types' +import { ShutdownVerificationOwnerCache } from './shutdown-verification-owner-cache' +import { reconcileDegradedDaemonSessions } from './degraded-daemon-session-reconciliation' +import { subscribeToProviderReplay } from './provider-replay-subscription' type ManagedPtyProvider = IPtyProvider & { disconnectOnly?: () => Promise @@ -25,6 +28,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { private legacy: DaemonPtyAdapter[] private fallback: ManagedPtyProvider private sessionProviders = new Map() + private shutdownVerificationProviders = new ShutdownVerificationOwnerCache() private unsubscribers: (() => void)[] = [] private dataListeners: ((payload: { id: string @@ -65,6 +69,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { const sessions = await adapter.listProcesses() for (const session of sessions) { this.sessionProviders.set(session.id, adapter) + this.shutdownVerificationProviders.delete(session.id) } } catch (error) { console.warn('[daemon] Failed to discover degraded daemon sessions', error) @@ -77,14 +82,20 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { const target = mapped ?? this.fallback const result = await target.spawn(opts) this.sessionProviders.set(result.id, target) + this.shutdownVerificationProviders.delete(result.id) return result } async attach(id: string): Promise { await this.providerFor(id).attach(id) + this.shutdownVerificationProviders.delete(id) } hasPty(id: string): boolean { + const shutdownOwner = this.shutdownVerificationProviders.take(id) + if (shutdownOwner) { + return shutdownOwner.hasPty?.(id) ?? true + } const mapped = this.sessionProviders.get(id) if (mapped) { return mapped.hasPty?.(id) ?? true @@ -113,9 +124,11 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } async shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise { - await this.providerFor(id).shutdown(id, opts) + const owner = this.providerFor(id) + await owner.shutdown(id, opts) if (!opts.keepHistory) { this.sessionProviders.delete(id) + this.shutdownVerificationProviders.remember(id, owner) } } @@ -170,6 +183,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { async revive(state: string): Promise { await this.fallback.revive(state) + this.shutdownVerificationProviders.clear() } async listProcesses(): Promise { @@ -211,23 +225,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } onReplay(callback: (payload: { id: string; data: string }) => void): () => void { - const unsubscribes = this.allProviders().map((provider) => provider.onReplay(callback)) - let active = true - const trackedUnsubscribe = (): void => { - if (!active) { - return - } - active = false - const idx = this.unsubscribers.indexOf(trackedUnsubscribe) - if (idx !== -1) { - this.unsubscribers.splice(idx, 1) - } - for (const unsubscribe of unsubscribes) { - unsubscribe() - } - } - this.unsubscribers.push(trackedUnsubscribe) - return trackedUnsubscribe + return subscribeToProviderReplay(this.allProviders(), callback, this.unsubscribers) } onExit(callback: (payload: { id: string; code: number }) => void): () => void { @@ -252,23 +250,22 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { alive: string[] killed: string[] }> { - const alive: string[] = [] - const killed: string[] = [] - for (const adapter of this.allDaemonAdapters()) { - const result = await adapter.reconcileOnStartup(validWorktreeIds) - for (const id of result.alive) { - alive.push(id) + return reconcileDegradedDaemonSessions( + this.allDaemonAdapters(), + validWorktreeIds, + (id, adapter) => { this.sessionProviders.set(id, adapter) - } - for (const id of result.killed) { - killed.push(id) + this.shutdownVerificationProviders.delete(id) + }, + (id) => { this.sessionProviders.delete(id) + this.shutdownVerificationProviders.delete(id) } - } - return { alive, killed } + ) } dispose(): void { + this.shutdownVerificationProviders.clear() this.disposeProviderOnly() for (const adapter of this.allDaemonAdapters()) { adapter.dispose() @@ -292,6 +289,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { fanoutCurrentDaemonSyntheticExits(code: number): void { for (const id of this.getCurrentDaemonSessionIds()) { this.sessionProviders.delete(id) + this.shutdownVerificationProviders.delete(id) // Why: sessions discovered from listProcesses may not exist in the // adapter's active-session set, but restart still kills that daemon. // oxlint-disable-next-line unicorn/no-useless-spread -- copy-safe: listeners may unsubscribe during iteration diff --git a/src/main/daemon/degraded-daemon-session-reconciliation.ts b/src/main/daemon/degraded-daemon-session-reconciliation.ts new file mode 100644 index 00000000000..04884240044 --- /dev/null +++ b/src/main/daemon/degraded-daemon-session-reconciliation.ts @@ -0,0 +1,23 @@ +import type { DaemonPtyAdapter } from './daemon-pty-adapter' + +export async function reconcileDegradedDaemonSessions( + adapters: readonly DaemonPtyAdapter[], + validWorktreeIds: Set, + onAlive: (id: string, adapter: DaemonPtyAdapter) => void, + onKilled: (id: string) => void +): Promise<{ alive: string[]; killed: string[] }> { + const alive: string[] = [] + const killed: string[] = [] + for (const adapter of adapters) { + const result = await adapter.reconcileOnStartup(validWorktreeIds) + for (const id of result.alive) { + alive.push(id) + onAlive(id, adapter) + } + for (const id of result.killed) { + killed.push(id) + onKilled(id) + } + } + return { alive, killed } +} diff --git a/src/main/daemon/provider-replay-subscription.ts b/src/main/daemon/provider-replay-subscription.ts new file mode 100644 index 00000000000..a87e459fd89 --- /dev/null +++ b/src/main/daemon/provider-replay-subscription.ts @@ -0,0 +1,25 @@ +import type { IPtyProvider } from '../providers/types' + +export function subscribeToProviderReplay( + providers: readonly IPtyProvider[], + callback: (payload: { id: string; data: string }) => void, + trackedSubscriptions: (() => void)[] +): () => void { + const unsubscribes = providers.map((provider) => provider.onReplay(callback)) + let active = true + const trackedUnsubscribe = (): void => { + if (!active) { + return + } + active = false + const index = trackedSubscriptions.indexOf(trackedUnsubscribe) + if (index !== -1) { + trackedSubscriptions.splice(index, 1) + } + for (const unsubscribe of unsubscribes) { + unsubscribe() + } + } + trackedSubscriptions.push(trackedUnsubscribe) + return trackedUnsubscribe +} diff --git a/src/main/daemon/pty-subprocess.test.ts b/src/main/daemon/pty-subprocess.test.ts index 99a35047743..659a84c52aa 100644 --- a/src/main/daemon/pty-subprocess.test.ts +++ b/src/main/daemon/pty-subprocess.test.ts @@ -10,12 +10,14 @@ const { isPwshAvailableMock, validateWorkingDirectoryMock, resolveUnixShellPathMock, - resolveAgentForegroundProcessMock + resolveAgentForegroundProcessMock, + killPosixPtySessionMock } = vi.hoisted(() => ({ spawnMock: vi.fn(), isPwshAvailableMock: vi.fn(), resolveUnixShellPathMock: vi.fn((shellPath: string) => shellPath), resolveAgentForegroundProcessMock: vi.fn(), + killPosixPtySessionMock: vi.fn(), validateWorkingDirectoryMock: vi.fn((cwd: string) => { if (cwd.includes('definitely-missing')) { throw new Error( @@ -29,6 +31,10 @@ vi.mock('node-pty', () => ({ spawn: spawnMock })) +vi.mock('../../relay/pty-session-kill', () => ({ + killPosixPtySession: killPosixPtySessionMock +})) + vi.mock('../pwsh', () => ({ isPwshAvailable: isPwshAvailableMock })) @@ -113,6 +119,8 @@ describe('createPtySubprocess', () => { beforeEach(() => { spawnMock.mockReset() + killPosixPtySessionMock.mockReset() + killPosixPtySessionMock.mockResolvedValue(true) isPwshAvailableMock.mockReset() resolveAgentForegroundProcessMock.mockReset() resolveAgentForegroundProcessMock.mockImplementation( @@ -1251,6 +1259,26 @@ describe('createPtySubprocess', () => { killSpy.mockRestore() }) + itOnPosixHost('proves the complete POSIX PTY session stopped before acknowledging', async () => { + const proc = mockPtyProcess(77) + Object.assign(proc, { ptsName: '/dev/ttys042' }) + spawnMock.mockReturnValue(proc) + const handle = createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) + + await expect(handle.forceKillAndWait?.()).resolves.toBe(true) + + expect(killPosixPtySessionMock).toHaveBeenCalledWith(77, '/dev/ttys042') + }) + + itOnPosixHost('fails closed when complete POSIX PTY session death is unverified', async () => { + const proc = mockPtyProcess(77) + spawnMock.mockReturnValue(proc) + killPosixPtySessionMock.mockResolvedValue(false) + const handle = createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) + + await expect(handle.forceKillAndWait?.()).resolves.toBe(false) + }) + it('routes onData events', () => { const proc = mockPtyProcess() spawnMock.mockReturnValue(proc) @@ -2630,6 +2658,28 @@ describe('createPtySubprocess', () => { } }) + it('waits through node-pty Windows process enumeration before verifying exit', async () => { + vi.useFakeTimers() + const proc = mockPtyProcess(123456) + spawnMock.mockReturnValue(proc) + const origPlatform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { value: 'win32' }) + try { + const handle = createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) + handle.kill() + + const stopped = handle.forceKillAndWait!() + await vi.advanceTimersByTimeAsync(5100) + proc._simulateExit(0) + + await expect(stopped).resolves.toBe(true) + expect(proc.kill).toHaveBeenCalledOnce() + } finally { + vi.useRealTimers() + restorePlatform(origPlatform) + } + }) + it('dispose() on Windows skips destroy after forceKill falls back to node-pty kill()', () => { const proc = mockPtyProcess(123456) as ReturnType & { destroy: ReturnType diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index ce1b1faf35c..0294b0a43e5 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -58,6 +58,7 @@ import { parsePtySessionId } from './pty-session-id' import { getAgentForegroundContextPaths } from '../providers/agent-foreground-context-paths' import { assertSafeAgentStartupCwd, resolveSafePtyDefaultCwd } from '../providers/pty-default-cwd' import { ORCA_HERMES_STARTUP_QUERY_ENV } from '../../shared/hermes-startup-query' +import { killPosixPtySession } from '../../relay/pty-session-kill' const PANE_IDENTITY_ENV_KEYS = [ 'ORCA_PANE_KEY', @@ -81,6 +82,9 @@ const PTY_SPAWN_HEALTH_TIMEOUT_MS = 4_000 // fallback (losing daemon persistence) until a manual restart. const PTY_SPAWN_HEALTH_RETRY_ATTEMPTS = 2 const PENDING_PRE_LISTENER_DATA_MAX_CHARS = 512 * 1024 +// Why: node-pty can spend up to 5s enumerating a Windows console tree before +// its fallback runs; leave bounded headroom for the native exit event to land. +const WINDOWS_PTY_EXIT_TIMEOUT_MS = 8_000 export type PtySubprocessOptions = { sessionId: string @@ -893,6 +897,7 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl let dead = false let disposed = false let nodePtyKillIssued = false + const exitWaiters = new Set<() => void>() let cachedAgentForeground: { processName: string; refreshedAt: number } | null = null const agentForegroundContextPaths = getAgentForegroundContextPaths({ cwd: opts.cwd, @@ -1001,6 +1006,10 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl } proc.onExit(() => { dead = true + for (const resolve of exitWaiters) { + resolve() + } + exitWaiters.clear() cachedAgentForeground = null startupAgentForeground = null // Why: UnixTerminal.destroy() registers `_socket.once('close', () => this.kill('SIGHUP'))` @@ -1204,6 +1213,36 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl } } }, + forceKillAndWait: async () => { + if (dead) { + return true + } + if (process.platform !== 'win32') { + return killPosixPtySession(proc.pid, (proc as unknown as { ptsName?: unknown }).ptsName) + } + if (!nodePtyKillIssued) { + try { + nodePtyKillIssued = true + proc.kill() + } catch { + return false + } + } + if (dead) { + return true + } + return new Promise((resolve) => { + const onExit = (): void => { + clearTimeout(timeout) + resolve(true) + } + const timeout = setTimeout(() => { + exitWaiters.delete(onExit) + resolve(false) + }, WINDOWS_PTY_EXIT_TIMEOUT_MS) + exitWaiters.add(onExit) + }) + }, signal: (sig) => { // Why: same recycled-pid hazard as forceKill. Once dead, silently drop // the signal rather than risk sending it to an unrelated process. diff --git a/src/main/daemon/session.test.ts b/src/main/daemon/session.test.ts index ade8b18fe15..530c7f6f511 100644 --- a/src/main/daemon/session.test.ts +++ b/src/main/daemon/session.test.ts @@ -365,6 +365,19 @@ describe('Session', () => { expect(subprocess.written).toEqual([]) }) + it('still disposes when the legacy force-kill fallback races with process exit', async () => { + createSession() + subprocess.forceKill = vi.fn(() => { + throw new Error('process already exited') + }) + subprocess.dispose = vi.fn() + + await expect(session.forceKillAndDisposeSubprocessAndWait()).resolves.toBeUndefined() + + expect(subprocess.dispose).toHaveBeenCalledOnce() + expect(session.state).toBe('exited') + }) + it('transitions to timed_out after 15 seconds', () => { createSession({ shellReadySupported: true }) session.write('waiting input') diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index b9c82a48174..160b344c0bf 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -65,6 +65,8 @@ export type SubprocessHandle = { clear?(): void kill(): void forceKill(): void + /** Force-stop the complete native PTY and prove no runnable owner remains. */ + forceKillAndWait?(): Promise signal(sig: string): void onData(cb: (data: string) => void): void onExit(cb: (code: number) => void): void @@ -494,6 +496,28 @@ export class Session { this.emulator.dispose() } + async forceKillAndDisposeSubprocessAndWait(): Promise { + let stopped: boolean + if (this.subprocess.forceKillAndWait) { + stopped = await this.subprocess.forceKillAndWait() + } else { + try { + this.subprocess.forceKill() + } catch { + // Why: the child can exit between liveness inspection and the legacy kill call. + } + stopped = true + } + if (!stopped) { + // Why: disposing here would erase the only retryable owner while its + // process may still hold the worktree. The daemon request must fail. + throw new Error(`Unable to verify full PTY session teardown: ${this.sessionId}`) + } + this.#teardownSubprocess() + this._state = 'exited' + this.emulator.dispose() + } + /** Private: shared teardown helper called by dispose(), forceDispose(), and * forceKillAndDisposeSubprocess(). Flips `_disposed`, clears pending timers, * and forwards to subprocess.dispose() exactly once. Does NOT set `_state` — diff --git a/src/main/daemon/shutdown-verification-owner-cache.ts b/src/main/daemon/shutdown-verification-owner-cache.ts new file mode 100644 index 00000000000..b928e707796 --- /dev/null +++ b/src/main/daemon/shutdown-verification-owner-cache.ts @@ -0,0 +1,30 @@ +export class ShutdownVerificationOwnerCache { + private readonly owners = new Map() + + constructor(private readonly limit = 4_096) {} + + delete(id: string): void { + this.owners.delete(id) + } + + clear(): void { + this.owners.clear() + } + + take(id: string): T | undefined { + const owner = this.owners.get(id) + this.owners.delete(id) + return owner + } + + remember(id: string, owner: T): void { + this.owners.delete(id) + this.owners.set(id, owner) + if (this.owners.size > this.limit) { + const oldest = this.owners.keys().next().value + if (oldest !== undefined) { + this.owners.delete(oldest) + } + } + } +} diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index 46e495e5f2e..6aba258dc48 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -390,6 +390,41 @@ describe('TerminalHost', () => { expect(host.isKilled('session-1')).toBe(true) }) + it('awaits verified immediate teardown before reaping the session', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + lastSubprocess.forceKillAndWait = vi.fn(async () => true) + + await host.killAndWait('session-1', { immediate: true }) + + expect(lastSubprocess.forceKillAndWait).toHaveBeenCalledOnce() + expect(lastSubprocess.dispose).toHaveBeenCalled() + expect(host.listSessions()).toEqual([]) + expect(host.isKilled('session-1')).toBe(true) + }) + + it('retains a retryable owner when immediate teardown cannot be verified', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + lastSubprocess.forceKillAndWait = vi.fn(async () => false) + + await expect(host.killAndWait('session-1', { immediate: true })).rejects.toThrow( + 'Unable to verify full PTY session teardown: session-1' + ) + + expect(lastSubprocess.dispose).not.toHaveBeenCalled() + expect(host.listSessions()).toHaveLength(1) + expect(host.isKilled('session-1')).toBe(false) + }) + it('throws for non-existent session', () => { expect(() => host.kill('missing')).toThrow('Session not found') }) diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index b15420fed14..92c5e6b036f 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -207,6 +207,17 @@ export class TerminalHost { session.kill() } + async killAndWait(sessionId: string, opts: { immediate?: boolean } = {}): Promise { + if (!opts.immediate) { + this.kill(sessionId, opts) + return + } + const session = this.getAliveSession(sessionId) + await session.forceKillAndDisposeSubprocessAndWait() + this.recordTombstone(sessionId) + this.reapSession(sessionId) + } + // Why: dispose a dead session's headless emulator and drop it from the map so // exited terminals don't pin ~5000 rows of scrollback for the daemon's life. // No-ops on live sessions (a live session must never be disposed here) and on diff --git a/src/main/daemon/types.ts b/src/main/daemon/types.ts index 550b211aa74..1643a3e2a80 100644 --- a/src/main/daemon/types.ts +++ b/src/main/daemon/types.ts @@ -16,9 +16,9 @@ import type { TuiAgent } from '../../shared/types' // when daemon-baked behavior cannot be delivered by on-disk wrapper refresh. // Why: bump when adding daemon wire behavior so same-version old daemons do // not silently accept the handshake and then reject new RPCs. -export const PROTOCOL_VERSION = 21 +export const PROTOCOL_VERSION = 22 export const PREVIOUS_DAEMON_PROTOCOL_VERSIONS = [ - 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20 + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21 ] as const // ─── Session State Machine ────────────────────────────────────────── diff --git a/src/main/index.ts b/src/main/index.ts index b76964ae872..fc30fdef463 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1777,6 +1777,7 @@ app.whenReady().then(async () => { // provider reference eagerly here would freeze the pre-daemon LocalPtyProvider // and defeat the teardown helper's prefix sweep (design §4.3 wire-up). getLocalProvider: () => getLocalPtyProvider(), + awaitLocalPtyStartup: () => localPtyStartupReady, onPtyStopped: clearProviderPtyState, onTerminalAgentStatus: (event) => { agentHookServer.ingestTerminalStatus(event) diff --git a/src/main/ipc/pty-startup-barrier-ordering.test.ts b/src/main/ipc/pty-startup-barrier-ordering.test.ts index 78f7477f468..3ab97765291 100644 --- a/src/main/ipc/pty-startup-barrier-ordering.test.ts +++ b/src/main/ipc/pty-startup-barrier-ordering.test.ts @@ -11,7 +11,7 @@ describe('PTY startup barrier ordering', () => { const runtimeSpawnStart = source.indexOf('spawn: async (args) => {') const runtimeSpawnEnd = source.indexOf(' write:', runtimeSpawnStart) const runtimeSpawn = source.slice(runtimeSpawnStart, runtimeSpawnEnd) - const rendererSpawnStart = source.indexOf("ipcMain.handle(\n 'pty:spawn'") + const rendererSpawnStart = source.indexOf("registerWorktreePtySpawnHandler(\n 'pty:spawn'") const rendererSpawnEnd = source.indexOf("ipcMain.handle(\n 'pty:kill'", rendererSpawnStart) const rendererSpawn = source.slice(rendererSpawnStart, rendererSpawnEnd) diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 35d386eb473..4214d51e513 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -57,7 +57,8 @@ const { setMigrationUnsupportedPtyMock, clearMigrationUnsupportedPtyMock, clearMigrationUnsupportedPtysForPaneKeyMock, - clearPaneKeyAliasesForPtyMock + clearPaneKeyAliasesForPtyMock, + killPosixPtySessionMock } = vi.hoisted(() => ({ handleMock: vi.fn(), onMock: vi.fn(), @@ -88,7 +89,8 @@ const { setMigrationUnsupportedPtyMock: vi.fn(), clearMigrationUnsupportedPtyMock: vi.fn(), clearMigrationUnsupportedPtysForPaneKeyMock: vi.fn(), - clearPaneKeyAliasesForPtyMock: vi.fn() + clearPaneKeyAliasesForPtyMock: vi.fn(), + killPosixPtySessionMock: vi.fn() })) vi.mock('electron', () => ({ @@ -128,6 +130,10 @@ vi.mock('node-pty', () => ({ spawn: spawnMock })) +vi.mock('../../relay/pty-session-kill', () => ({ + killPosixPtySession: killPosixPtySessionMock +})) + vi.mock('../opencode/hook-service', () => ({ openCodeHookService: { buildPtyEnv: openCodeBuildPtyEnvMock, @@ -209,6 +215,7 @@ import { isHiddenRendererPty } from './pty-hidden-delivery-gate' import { OrcaRuntimeService } from '../runtime/orca-runtime' +import { WorktreePtyAdmission } from '../runtime/worktree-pty-admission' import { hasLiveClaudePtys, markClaudePtySpawned } from '../claude-accounts/live-pty-gate' import * as livePtyGate from '../claude-accounts/live-pty-gate' import { @@ -217,8 +224,10 @@ import { } from '../powershell-osc133-bootstrap' import { SSH_PTY_IDENTITY_MISMATCH_ERROR, - SSH_SESSION_EXPIRED_ERROR + SSH_SESSION_EXPIRED_ERROR, + SshPtyProvider } from '../providers/ssh-pty-provider' +import { JsonRpcErrorCode } from '../ssh/relay-protocol' import { _resetWslCachesForTests, _setWslCachesForTests } from '../wsl' const POWERSHELL_OSC133_ARGS = [ @@ -324,6 +333,8 @@ describe('registerPtyHandlers', () => { chmodSyncMock.mockReset() getPathMock.mockReset() spawnMock.mockReset() + killPosixPtySessionMock.mockReset() + killPosixPtySessionMock.mockResolvedValue(true) openCodeBuildPtyEnvMock.mockReset() mimoCodeBuildPtyEnvMock.mockReset() openCodeClearPtyMock.mockReset() @@ -822,6 +833,83 @@ describe('registerPtyHandlers', () => { } describe('spawn environment', () => { + it('holds renderer spawns in worktree admission even when IPC input claims a lease', async () => { + const releaseWorktreeSpawn = vi.fn() + const runtime = { + setPtyController: vi.fn(), + beginWorktreePtySpawn: vi.fn(() => releaseWorktreeSpawn), + preAllocateHandleForPty: vi.fn(() => undefined), + registerPty: vi.fn(), + noteTerminalSpawnCommand: vi.fn(), + onPtySpawned: vi.fn(), + onPtyExit: vi.fn(), + onPtyData: vi.fn() + } + registerPtyHandlers(mainWindow as never, runtime as never) + + const result = await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + worktreeId: 'wt-renderer', + // Renderer payloads are structurally open at runtime; an injected field + // must never reach the controller-internal admission bypass. + worktreePtyAdmissionHeld: true + }) + + expect(result).toMatchObject({ id: expect.any(String) }) + expect(runtime.beginWorktreePtySpawn).toHaveBeenCalledWith('wt-renderer', undefined) + expect(runtime.registerPty).toHaveBeenCalledWith( + expect.any(String), + 'wt-renderer', + null, + undefined + ) + expect(releaseWorktreeSpawn).toHaveBeenCalledTimes(1) + }) + + it('blocks deletion while renderer launch preparation is still in flight', async () => { + const startup = makeDeferred() + const admission = new WorktreePtyAdmission() + const order: string[] = [] + const runtime = { + setPtyController: vi.fn(), + beginWorktreePtySpawn: (worktreeId: string) => admission.beginSpawn(worktreeId), + preAllocateHandleForPty: vi.fn(() => undefined), + registerPty: vi.fn(() => order.push('registered')), + noteTerminalSpawnCommand: vi.fn(), + onPtySpawned: vi.fn(), + onPtyExit: vi.fn(), + onPtyData: vi.fn() + } + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + undefined, + { awaitLocalPtyStartup: () => startup.promise } + ) + + const spawning = handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd: '/outside/worktree', + worktreeId: 'wt-renderer-prep' + }) as Promise<{ id: string }> + const deletion = admission.runTeardown('wt-renderer-prep', async () => { + order.push('deleted') + }) + await Promise.resolve() + + expect(order).toEqual([]) + startup.resolve() + + await spawning + await deletion + expect(order).toEqual(['registered', 'deleted']) + }) + it('marks local Claude launches live until the PTY is killed', async () => { const prepareClaudeAuth = vi.fn(async () => ({ configDir: '/tmp/claude', @@ -1741,8 +1829,10 @@ describe('registerPtyHandlers', () => { }): Promise<{ id: string }> } const daemonSpawn = setupDaemonAdapter() + const releaseWorktreeSpawn = vi.fn() const runtime = { setPtyController: vi.fn(), + beginWorktreePtySpawn: vi.fn(() => releaseWorktreeSpawn), registerPty: vi.fn(), noteTerminalSpawnCommand: vi.fn(), onPtySpawned: vi.fn(), @@ -1764,6 +1854,8 @@ describe('registerPtyHandlers', () => { ) expect(spawnOptions.env.ORCA_AGENT_HOOK_PORT).toBe('5678') expect(spawnOptions.env.ORCA_AGENT_HOOK_TOKEN).toBe('agent-token') + expect(runtime.beginWorktreePtySpawn).toHaveBeenCalledWith('wt-runtime', undefined) + expect(releaseWorktreeSpawn).toHaveBeenCalledTimes(1) }) it('threads the validated pane identity into registerPty for a runtime-created daemon PTY (#7587)', async () => { @@ -2644,6 +2736,59 @@ describe('registerPtyHandlers', () => { expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', -1) }) + it('keeps graceful SSH ownership while the provider still reports the PTY live', async () => { + const shutdown = vi.fn(async () => undefined) + const store = { markSshRemotePtyLease: vi.fn() } + const runtime = { setPtyController: vi.fn(), onPtyExit: vi.fn() } + registerSshPtyProvider('ssh-1', { + spawn: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + shutdown, + sendSignal: vi.fn(), + getCwd: vi.fn(), + getInitialCwd: vi.fn(), + clearBuffer: vi.fn(), + acknowledgeDataEvent: vi.fn(), + hasChildProcesses: vi.fn(), + hasPtyAsync: vi.fn(async () => true), + getForegroundProcess: vi.fn(), + serialize: vi.fn(), + revive: vi.fn(), + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}), + listProcesses: vi.fn(async () => []), + attach: vi.fn(), + getDefaultShell: vi.fn(), + getProfiles: vi.fn() + } as never) + setPtyOwnership('remote-pty', 'ssh-1') + handlers.clear() + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + store as never + ) + const controller = runtime.setPtyController.mock.calls[0]?.[0] as { + kill: (ptyId: string) => boolean + } + + try { + expect(controller.kill('remote-pty')).toBe(true) + await new Promise((resolve) => setImmediate(resolve)) + } finally { + deletePtyOwnership('remote-pty') + } + + expect(shutdown).toHaveBeenCalledWith('remote-pty', { immediate: false }) + expect(store.markSshRemotePtyLease).not.toHaveBeenCalled() + expect(runtime.onPtyExit).not.toHaveBeenCalled() + }) + it('controller kill does not duplicate exits when the provider emits exit during shutdown', async () => { const exitListeners = new Set<(payload: { id: string; code: number }) => void>() const shutdown = vi.fn(async (id: string) => { @@ -2700,6 +2845,8 @@ describe('registerPtyHandlers', () => { it('controller stopAndWait skips the synthetic exit when the provider emitted one', async () => { vi.useFakeTimers() const exitListeners = new Set<(payload: { id: string; code: number }) => void>() + const hasPty = vi.fn(() => false) + const listProcesses = vi.fn(async () => []) const shutdown = vi.fn(async (id: string) => { for (const listener of exitListeners) { listener({ id, code: 0 }) @@ -2711,6 +2858,7 @@ describe('registerPtyHandlers', () => { } setLocalPtyProvider({ spawn: vi.fn(), + hasPty, write: vi.fn(), resize: vi.fn(), shutdown, @@ -2729,7 +2877,7 @@ describe('registerPtyHandlers', () => { exitListeners.add(listener) return () => exitListeners.delete(listener) }), - listProcesses: vi.fn(async () => []), + listProcesses, attach: vi.fn(), getDefaultShell: vi.fn(), getProfiles: vi.fn() @@ -2744,6 +2892,8 @@ describe('registerPtyHandlers', () => { await vi.advanceTimersByTimeAsync(1_200) await expect(stopPromise).resolves.toBe(true) + expect(hasPty).toHaveBeenCalledWith('local-pty') + expect(listProcesses).not.toHaveBeenCalled() expect(runtime.onPtyExit).toHaveBeenCalledTimes(1) expect(runtime.onPtyExit).toHaveBeenCalledWith('local-pty', 0) expect( @@ -2751,6 +2901,164 @@ describe('registerPtyHandlers', () => { ).toEqual([['pty:exit', { id: 'local-pty', code: 0 }]]) }) + it('controller stopAndWait uses targeted SSH liveness for multiple PTYs', async () => { + const hasPtyAsync = vi.fn(async (_ptyId: string) => false) + const listProcesses = vi.fn(async () => []) + const shutdown = vi.fn(async () => undefined) + const runtime = { + setPtyController: vi.fn(), + onPtyExit: vi.fn() + } + registerSshPtyProvider('ssh-1', { + spawn: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + shutdown, + sendSignal: vi.fn(), + getCwd: vi.fn(), + getInitialCwd: vi.fn(), + clearBuffer: vi.fn(), + acknowledgeDataEvent: vi.fn(), + hasChildProcesses: vi.fn(), + hasPtyAsync, + getForegroundProcess: vi.fn(), + serialize: vi.fn(), + revive: vi.fn(), + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}), + listProcesses, + attach: vi.fn(), + getDefaultShell: vi.fn(), + getProfiles: vi.fn() + } as never) + const ptyIds = ['ssh:ssh-1@@relay-1', 'ssh:ssh-1@@relay-2', 'ssh:ssh-1@@relay-3'] + for (const ptyId of ptyIds) { + setPtyOwnership(ptyId, 'ssh-1') + } + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + const controller = runtime.setPtyController.mock.calls[0]?.[0] as { + stopAndWait: (ptyId: string) => Promise + } + + await expect( + Promise.all(ptyIds.map((ptyId) => controller.stopAndWait(ptyId))) + ).resolves.toEqual([true, true, true]) + + expect(shutdown).toHaveBeenCalledTimes(ptyIds.length) + expect(hasPtyAsync).toHaveBeenCalledTimes(ptyIds.length) + expect(hasPtyAsync.mock.calls.map(([ptyId]) => ptyId)).toEqual(ptyIds) + expect(listProcesses).not.toHaveBeenCalled() + }) + + it('finishes headless current-relay SSH shutdown before remote removal without inventory', async () => { + const order: string[] = [] + const request = vi.fn(async (method: string) => { + if (method === 'pty.shutdownSession') { + order.push('shutdown') + return undefined + } + if (method === 'pty.hasPty') { + order.push('hasPty') + return false + } + throw new Error(`Unexpected request: ${method}`) + }) + const provider = new SshPtyProvider('ssh-1', { + request, + notify: vi.fn(), + onNotification: vi.fn(() => () => {}), + dispose: vi.fn(), + isDisposed: vi.fn(() => false) + } as never) + const runtime = new OrcaRuntimeService() + Object.assign(runtime, { + resolveWorktreeSelector: vi.fn(async () => ({ id: 'wt-headless-current' })) + }) + registerSshPtyProvider('ssh-1', provider) + setPtyOwnership('ssh:ssh-1@@headless-current', 'ssh-1') + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + runtime.registerPty('ssh:ssh-1@@headless-current', 'wt-headless-current', 'ssh-1') + + await runtime.runWithWorktreePtyTeardown('wt-headless-current', async () => { + await expect( + runtime.stopTerminalsForWorktree('id:wt-headless-current', { + worktreeTeardown: true + }) + ).resolves.toEqual({ stopped: 1 }) + order.push('git-remove') + }) + + expect(order).toEqual(['shutdown', 'hasPty', 'git-remove']) + expect( + request.mock.calls.filter(([method]) => method === 'pty.listProcesses') + ).toHaveLength(0) + deletePtyOwnership('ssh:ssh-1@@headless-current') + }) + + it('fails headless legacy-relay teardown closed before inventory or removal', async () => { + const order: string[] = [] + const request = vi.fn(async (method: string) => { + if (method === 'pty.shutdownSession') { + order.push('shutdown-miss') + throw Object.assign(new Error('Method not found'), { + code: JsonRpcErrorCode.MethodNotFound + }) + } + if (method === 'pty.hasPty') { + order.push('hasPty-miss') + throw Object.assign(new Error('Method not found'), { + code: JsonRpcErrorCode.MethodNotFound + }) + } + if (method === 'pty.listProcesses') { + order.push('inventory') + return [] + } + throw new Error(`Unexpected request: ${method}`) + }) + const provider = new SshPtyProvider('ssh-1', { + request, + notify: vi.fn(), + onNotification: vi.fn(() => () => {}), + dispose: vi.fn(), + isDisposed: vi.fn(() => false) + } as never) + const runtime = new OrcaRuntimeService() + Object.assign(runtime, { + resolveWorktreeSelector: vi.fn(async () => ({ id: 'wt-headless-legacy' })) + }) + registerSshPtyProvider('ssh-1', provider) + const ptyIds = ['ssh:ssh-1@@headless-legacy-1', 'ssh:ssh-1@@headless-legacy-2'] + for (const ptyId of ptyIds) { + setPtyOwnership(ptyId, 'ssh-1') + } + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + for (const ptyId of ptyIds) { + runtime.registerPty(ptyId, 'wt-headless-legacy', 'ssh-1') + } + + await runtime.runWithWorktreePtyTeardown('wt-headless-legacy', async () => { + await expect( + runtime.stopTerminalsForWorktree('id:wt-headless-legacy', { + worktreeTeardown: true + }) + ).resolves.toEqual({ stopped: 0, failedPtyIds: ptyIds }) + }) + + expect(order.filter((step) => step === 'shutdown-miss')).toHaveLength(2) + expect(request.mock.calls.filter(([method]) => method === 'pty.hasPty')).toHaveLength(0) + expect( + request.mock.calls.filter(([method]) => method === 'pty.listProcesses') + ).toHaveLength(0) + for (const ptyId of ptyIds) { + deletePtyOwnership(ptyId) + } + }) + it('passes keepHistory through runtime controller stopAndWait', async () => { vi.useFakeTimers() const shutdown = vi.fn(async () => undefined) @@ -3063,6 +3371,33 @@ describe('registerPtyHandlers', () => { expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', -1) }) + it('fails deletion-only stop closed when a detached SSH provider is absent', async () => { + const store = { markSshRemotePtyLease: vi.fn() } + const runtime = { setPtyController: vi.fn(), onPtyExit: vi.fn() } + setPtyOwnership('remote-pty', 'ssh-1') + handlers.clear() + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + store as never + ) + const controller = runtime.setPtyController.mock.calls[0]?.[0] as { + stopAndWait: (ptyId: string) => Promise + } + + try { + await expect(controller.stopAndWait('remote-pty')).resolves.toBe(false) + } finally { + deletePtyOwnership('remote-pty') + } + + expect(store.markSshRemotePtyLease).not.toHaveBeenCalled() + expect(runtime.onPtyExit).not.toHaveBeenCalled() + }) + it('preserves an SSH lease when runtime controller kill shutdown fails transiently', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) const store = { @@ -3121,7 +3456,7 @@ describe('registerPtyHandlers', () => { 'remote-pty', 'terminated' ) - expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', -1) + expect(runtime.onPtyExit).not.toHaveBeenCalled() }) it('strips ORCA_PANE_KEY/TAB_ID/WORKTREE_ID from SSH spawn env when remote agent hooks are disabled', async () => { @@ -10621,7 +10956,7 @@ describe('registerPtyHandlers', () => { expect(piClearPtyMock).toHaveBeenCalledWith(spawnResult.id) }) - it('disposes PTY listeners before manual kill IPC', async () => { + it('keeps the retryable owner until manual kill IPC verifies session death', async () => { const onDataDisposable = makeDisposable() const onExitDisposable = makeDisposable() // Why: hold a stable reference to the kill spy. On POSIX, destroyPtyProcess @@ -10648,12 +10983,13 @@ describe('registerPtyHandlers', () => { await handlers.get('pty:kill')!(null, { id: spawnResult.id }) - expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + expect(killPosixPtySessionMock.mock.invocationCallOrder[0]).toBeLessThan( + onDataDisposable.dispose.mock.invocationCallOrder[0] ) - expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + expect(killPosixPtySessionMock.mock.invocationCallOrder[0]).toBeLessThan( + onExitDisposable.dispose.mock.invocationCallOrder[0] ) + expect(killSpy).not.toHaveBeenCalled() }) it('disposes PTY listeners before runtime controller kill', async () => { diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 305ebabfc14..75ace5e6e71 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -16,7 +16,11 @@ import { powerMonitor } from 'electron' export { getBashShellReadyRcfileContent } from '../providers/local-pty-shell-ready' -import type { OrcaRuntimeService } from '../runtime/orca-runtime' +import type { + OrcaRuntimeService, + RuntimePtyController, + RuntimePtySpawnOptions +} from '../runtime/orca-runtime' import type { Store } from '../persistence' import type { GlobalSettings, TuiAgent } from '../../shared/types' import { terminalOutputBacklogCapChars } from '../../shared/terminal-scrollback-policy' @@ -490,6 +494,14 @@ function delay(ms: number): Promise { } async function isProviderPtyLive(provider: IPtyProvider, ptyId: string): Promise { + // Why: worktree teardown verifies each stopped PTY. Local/daemon providers + // can answer by ID, avoiding one full session inventory per split pane. + if (provider.hasPty) { + return provider.hasPty(ptyId) + } + if (provider.hasPtyAsync) { + return provider.hasPtyAsync(ptyId) + } return (await provider.listProcesses()).some((session) => session.id === ptyId) } @@ -1160,6 +1172,9 @@ export function clearProviderPtyState(id: string): void { } }) rendererSerializerByPtyId.delete(id) + // Why: direct provider teardown can bypass the ordinary exit listener. The + // provider router must not retain a dead SSH/local owner after verified stop. + ptyOwnership.delete(id) // Why: the hook server's per-paneKey caches (lastPrompt / lastTool) would // otherwise accumulate entries for dead panes over the process lifetime. // Use the spawn-time paneKey mapping since the server has no other way to @@ -2916,7 +2931,8 @@ export function registerPtyHandlers( // Why: the runtime controller must route through getProviderForPty() so that // CLI commands (terminal.send, terminal.stop) work for both local and remote PTYs. // Hardcoding localProvider.getPtyProcess() would silently fail for remote PTYs. - runtime?.setPtyController({ + const admissionHeldRuntimeSpawnArgs = new WeakSet() + const runtimePtyController: RuntimePtyController = { spawn: async (args) => { const startupPromise = getLocalPtyStartupPromise(args.connectionId) if (startupPromise) { @@ -3110,6 +3126,12 @@ export function registerPtyHandlers( if (existingPaneSpawn) { return await existingPaneSpawn.promise } + const releaseWorktreeSpawn = + !admissionHeldRuntimeSpawnArgs.has(args) && + args.worktreeId && + runtime?.beginWorktreePtySpawn + ? runtime.beginWorktreePtySpawn(args.worktreeId, args.connectionId) + : null const paneSpawnReservation = materializedPaneKey ? reservePaneSpawn(materializedPaneKey) : null @@ -3284,6 +3306,18 @@ export function registerPtyHandlers( // no-op once the reservation has already resolved. rejectPaneSpawnReservation(materializedPaneKey, paneSpawnReservation, err) throw err + } finally { + releaseWorktreeSpawn?.() + } + }, + // Why: runtime create/split already hold admission across launch preparation; + // this controller-only entry point avoids a nested lease without trusting IPC input. + spawnWithWorktreePtyAdmissionHeld: async (args) => { + admissionHeldRuntimeSpawnArgs.add(args) + try { + return await runtimePtyController.spawn!(args) + } finally { + admissionHeldRuntimeSpawnArgs.delete(args) } }, write: (ptyId, data) => { @@ -3323,7 +3357,23 @@ export function registerPtyHandlers( // provider emitted its own exit during shutdown, the exit listener already // delivered runtime + renderer exits — synthesizing again would double-fire. void shutdownProviderAndDetectExit(provider, ptyId, { immediate: false }) - .then((providerExitObserved) => { + .then(async (providerExitObserved) => { + if (!providerExitObserved) { + try { + if (await isProviderPtyLive(provider, ptyId)) { + // Why: graceful shutdown only acknowledges signal delivery. + // Keep ownership until the provider's real exit event arrives. + return + } + } catch (err) { + console.warn( + `[pty] Failed to verify graceful PTY stop ${ptyId}: ${ + err instanceof Error ? err.message : String(err) + }` + ) + return + } + } finishPtyShutdown(ptyId, connectionId, store) if (!providerExitObserved) { runtime?.onPtyExit(ptyId, -1) @@ -3342,11 +3392,8 @@ export function registerPtyHandlers( console.warn( `[pty] Failed to stop PTY ${ptyId}: ${err instanceof Error ? err.message : String(err)}` ) - // Why: callers of controller.kill must observe a kill→exit pair so - // runtime tail buffers close and agents stop treating the pane as - // live. Preserve provider/lease state so a retry can still target - // the remote PTY if it survived the transient failure. - runtime?.onPtyExit(ptyId, -1) + // Why: preserve runtime/provider ownership so deletion can still + // target a PTY whose graceful close failed or remains unverified. }) return true }, @@ -3359,13 +3406,9 @@ export function registerPtyHandlers( provider = connectionId ? getProvider(connectionId) : getProviderForPty(ptyId) } catch { if (connectionId) { - // Why: an absent SSH provider means there is no live target left to - // await, but the relay lease must still be tombstoned. - finishPtyShutdown(ptyId, connectionId, store) - runtime?.onPtyExit(ptyId, -1) - rememberSyntheticKillExit(ptyId) - sendPtyExitToRenderer({ id: ptyId, code: -1 }) - return true + // Why: disconnect unregisters the provider while the relay PTY and + // durable lease intentionally survive. Deletion must fail closed. + return false } return false } @@ -3475,7 +3518,8 @@ export function registerPtyHandlers( return false } } - }) + } + runtime?.setPtyController(runtimePtyController) // ─── IPC Handlers (thin dispatch layer) ───────────────────────── @@ -3571,7 +3615,29 @@ export function registerPtyHandlers( resetPtyRendererDeliveryDebug() }) - ipcMain.handle( + const registerWorktreePtySpawnHandler = < + T extends { worktreeId?: string; connectionId?: string | null }, + R + >( + channel: string, + handler: (event: IpcMainInvokeEvent, args: T) => Promise + ): void => { + ipcMain.handle(channel, async (event, args: T) => { + const releaseWorktreeSpawn = + typeof args.worktreeId === 'string' && runtime?.beginWorktreePtySpawn + ? runtime.beginWorktreePtySpawn(args.worktreeId, args.connectionId) + : null + try { + return await handler(event, args) + } finally { + // Why: deletion must own renderer launch preparation as well as provider + // spawn and registration, including failures before a PTY id exists. + releaseWorktreeSpawn?.() + } + }) + } + + registerWorktreePtySpawnHandler( 'pty:spawn', async ( _event, diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 9f4dea40f92..1c1e3e07cb2 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -290,6 +290,9 @@ describe('registerWorktreeHandlers', () => { createTerminal: ReturnType splitTerminal: ReturnType notifyWorktreesChangedForRemoteClients: ReturnType + runWithWorktreeRemovalDedup: ReturnType + runWithWorktreePtyTeardown: ReturnType + beginWorktreePtyTeardown: ReturnType } beforeEach(() => { @@ -501,7 +504,23 @@ describe('registerWorktreeHandlers', () => { tabId: 'tab-startup', paneRuntimeId: -1 }), - notifyWorktreesChangedForRemoteClients: vi.fn() + notifyWorktreesChangedForRemoteClients: vi.fn(), + runWithWorktreeRemovalDedup: vi.fn( + async ( + _worktreeId: string, + _opts: { + force: boolean + runHooks: boolean + hostId?: string + mode?: 'remove' | 'forget' + }, + operation: () => Promise + ) => await operation() + ), + runWithWorktreePtyTeardown: vi.fn( + async (_worktreeId: string, operation: () => Promise) => await operation() + ), + beginWorktreePtyTeardown: vi.fn(async () => () => {}) } registerWorktreeHandlers(mainWindow as never, store as never, runtimeStub as never) }) @@ -6650,7 +6669,7 @@ describe('registerWorktreeHandlers', () => { preservedBranch: { branchName: 'feature', head: 'feature' } }) expect(runHookMock).not.toHaveBeenCalled() - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, expect.any(Object)) expect(removeWorktreeMock).not.toHaveBeenCalled() expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { cwd: '/workspace/repo' @@ -6730,6 +6749,7 @@ describe('registerWorktreeHandlers', () => { expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, { runtime: runtimeStub, localProvider: ptyProvider, + connectionId: null, onPtyStopped: clearProviderPtyStateMock }) expect(killAllProcessesForWorktreeMock.mock.invocationCallOrder[0]).toBeLessThan( @@ -6743,6 +6763,40 @@ describe('registerWorktreeHandlers', () => { }) }) + it('does not remove worktree state when verified PTY teardown fails', async () => { + const worktreeId = 'repo-1::/workspace/feature-wt' + mockKnownFeatureWorktree() + store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) + killAllProcessesForWorktreeMock.mockRejectedValue( + new Error(`Failed to stop local worktree terminals: ${worktreeId}@@daemon`) + ) + + await expect(handlers['worktrees:remove'](null, { worktreeId })).rejects.toThrow( + 'Failed to stop local worktree terminals' + ) + + expect(removeWorktreeMock).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + }) + + it('holds PTY admission until Git and metadata removal both complete', async () => { + const worktreeId = 'repo-1::/workspace/feature-wt' + const releaseTeardown = vi.fn() + mockKnownFeatureWorktree() + store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) + removeWorktreeMock.mockResolvedValue({}) + runtimeStub.beginWorktreePtyTeardown.mockResolvedValue(releaseTeardown) + + await handlers['worktrees:remove'](null, { worktreeId }) + + expect(removeWorktreeMock.mock.invocationCallOrder[0]).toBeLessThan( + store.removeWorktreeMeta.mock.invocationCallOrder[0] + ) + expect(store.removeWorktreeMeta.mock.invocationCallOrder[0]).toBeLessThan( + releaseTeardown.mock.invocationCallOrder[0] + ) + }) + it('runs the archive hook on remove when skipArchive is not set', async () => { mockKnownFeatureWorktree() removeWorktreeMock.mockResolvedValue(undefined) @@ -7656,7 +7710,10 @@ describe('registerWorktreeHandlers', () => { force: true }) - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + 'repo-1::/workspace/already-deleted-wt', + expect.any(Object) + ) expect(runHookMock).not.toHaveBeenCalled() expect(removeWorktreeMock).not.toHaveBeenCalled() expect(runtimeStub.clearOptimisticReconcileToken).toHaveBeenCalledWith( @@ -7681,7 +7738,7 @@ describe('registerWorktreeHandlers', () => { await handlers['worktrees:remove'](null, { worktreeId, force: true }) - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, expect.any(Object)) expect(runHookMock).not.toHaveBeenCalled() expect(removeWorktreeMock).not.toHaveBeenCalled() expect(runtimeStub.clearOptimisticReconcileToken).toHaveBeenCalledWith(worktreeId) @@ -7695,12 +7752,20 @@ describe('registerWorktreeHandlers', () => { it('treats normal deletion of an already-missing unregistered worktree as cleanup', async () => { mockKnownFeatureWorktree('/workspace/real-feature') store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) + const callOrder: string[] = [] + killAllProcessesForWorktreeMock.mockImplementation(async () => { + callOrder.push('pty-teardown') + return { runtimeStopped: 1, providerStopped: 0, registryStopped: 0 } + }) + store.removeWorktreeMeta.mockImplementation(() => { + callOrder.push('metadata-removal') + }) await handlers['worktrees:remove'](null, { worktreeId: 'repo-1::/workspace/already-deleted-wt' }) - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(callOrder).toEqual(['pty-teardown', 'metadata-removal']) expect(runHookMock).not.toHaveBeenCalled() expect(removeWorktreeMock).not.toHaveBeenCalled() expect(runtimeStub.clearOptimisticReconcileToken).toHaveBeenCalledWith( @@ -7745,7 +7810,7 @@ describe('registerWorktreeHandlers', () => { }) await expect(lstat(orphanPath)).rejects.toMatchObject({ code: 'ENOENT' }) - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, expect.any(Object)) expect(runHookMock).not.toHaveBeenCalled() expect(removeWorktreeMock).not.toHaveBeenCalled() expect(runtimeStub.clearOptimisticReconcileToken).toHaveBeenCalledWith(worktreeId) @@ -7837,7 +7902,7 @@ describe('registerWorktreeHandlers', () => { ).resolves.toEqual({}) await expect(lstat(leftoverPath)).rejects.toMatchObject({ code: 'ENOENT' }) - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, expect.any(Object)) expect(runHookMock).not.toHaveBeenCalled() expect(removeWorktreeMock).not.toHaveBeenCalled() expect(runtimeStub.clearOptimisticReconcileToken).toHaveBeenCalledWith(worktreeId) @@ -8014,6 +8079,75 @@ describe('registerWorktreeHandlers', () => { expect(mainWindow.webContents.send).toHaveBeenCalledTimes(1) }) + it('joins concurrent renderer and runtime removals before either repeats preflight', async () => { + mockKnownFeatureWorktree() + const shared = new Map }>() + runtimeStub.runWithWorktreeRemovalDedup.mockImplementation( + async ( + worktreeId: string, + opts: { + force: boolean + runHooks: boolean + hostId?: string + mode?: 'remove' | 'forget' + }, + operation: () => Promise + ) => { + const key = `${opts.hostId ?? ''}\0${worktreeId}` + const optionsKey = `${opts.mode ?? 'remove'}:${opts.force}:${opts.runHooks}` + const existing = shared.get(key) + if (existing) { + if (existing.optionsKey !== optionsKey) { + throw new Error('Worktree deletion already in progress') + } + return await existing.promise + } + const promise = operation() + shared.set(key, { optionsKey, promise }) + try { + return await promise + } finally { + shared.delete(key) + } + } + ) + let finishRemoval = () => {} + removeWorktreeMock.mockImplementation( + () => + new Promise((resolve) => { + finishRemoval = resolve + }) + ) + const worktreeId = 'repo-1::/workspace/feature-wt' + const rendererRemoval = handlers['worktrees:remove'](null, { + worktreeId, + force: true + }) as Promise + await vi.waitFor(() => expect(removeWorktreeMock).toHaveBeenCalledTimes(1)) + const runtimeBackend = vi.fn(async () => ({ source: 'runtime' })) + const runRuntimeRemoval = runtimeStub.runWithWorktreeRemovalDedup as unknown as ( + worktreeId: string, + opts: { + force: boolean + runHooks: boolean + hostId?: string + mode?: 'remove' | 'forget' + }, + operation: () => Promise + ) => Promise + const runtimeRemoval = runRuntimeRemoval( + worktreeId, + { force: true, runHooks: true, hostId: 'local' }, + runtimeBackend + ) + + finishRemoval() + await expect(Promise.all([rendererRemoval, runtimeRemoval])).resolves.toEqual([{}, {}]) + expect(runtimeBackend).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledTimes(1) + expect(removeWorktreeMock).toHaveBeenCalledTimes(1) + }) + it('rejects concurrent deletes for the same worktree id with different options', async () => { mockKnownFeatureWorktree() let removalStarted!: () => void @@ -8330,17 +8464,19 @@ describe('registerWorktreeHandlers', () => { worktreeId: 'repo-1::/workspace/feature-wt' }) - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + 'repo-1::/workspace/feature-wt', + expect.any(Object) + ) expect(removeWorktreeMock).toHaveBeenCalled() expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { cwd: '/workspace/repo' }) }) - it('skips the PTY teardown for SSH-backed repos (design §6 out-of-scope)', async () => { - // Why: SSH-backed PTYs live on the remote host and are handled by the - // remote provider's own teardown. The local-host helper must not run for - // SSH repos, because it would sweep registry entries for other worktrees. + it('does not start teardown before resolving the SSH removal provider', async () => { + // Why: an unresolved SSH target is a rejected removal, not a successful + // cleanup path, so admission and PTY state must remain untouched. const repo = { id: 'repo-ssh', path: '/remote/repo', @@ -8353,9 +8489,7 @@ describe('registerWorktreeHandlers', () => { store.getRepos.mockReturnValue([repo]) store.getRepo.mockReturnValue(repo) - // The test can't easily mock the SSH provider without more plumbing — the - // call will throw about 'no git provider for connection'. What matters - // here is that the kill helper was NOT called for the SSH branch. + // The call rejects before it has an authoritative remote removal target. await ( handlers['worktrees:remove'](null, { worktreeId: 'repo-ssh::/remote/feature-wt' @@ -8505,6 +8639,20 @@ describe('registerWorktreeHandlers', () => { }) describe('worktrees:forgetLocal', () => { + it('rejects a forget race while another removal owns PTY admission', async () => { + const worktreeId = 'repo-1::/workspace/feature-wt' + runtimeStub.runWithWorktreePtyTeardown.mockRejectedValueOnce( + new Error(`Worktree teardown is already in progress: ${worktreeId}`) + ) + + await expect(handlers['worktrees:forgetLocal'](null, { worktreeId })).rejects.toThrow( + 'Worktree teardown is already in progress' + ) + + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + }) + it('forgets a workspace pinned to a removed SSH target without touching the provider', async () => { const repo = { id: 'repo-1', @@ -8518,17 +8666,27 @@ describe('registerWorktreeHandlers', () => { const ptyProvider = {} as never const worktreeId = 'repo-1::/workspace/feature-wt' store.getRepo.mockReturnValue(repo) + store.getRepos.mockReturnValue([repo]) getLocalPtyProviderMock.mockReturnValue(ptyProvider) // Why: a removed/disconnected SSH target has no live provider; forgetLocal // must not reach for one. getSshGitProviderMock.mockReturnValue(undefined) - const result = await handlers['worktrees:forgetLocal'](null, { worktreeId }) + const result = await handlers['worktrees:forgetLocal'](null, { + worktreeId, + hostId: 'ssh:ssh-dead' + }) expect(result).toEqual({}) + expect(runtimeStub.runWithWorktreePtyTeardown).toHaveBeenCalledWith( + worktreeId, + expect.any(Function), + 'ssh-dead' + ) expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, { runtime: runtimeStub, localProvider: ptyProvider, + connectionId: 'ssh-dead', onPtyStopped: clearProviderPtyStateMock }) expect(runtimeStub.clearOptimisticReconcileToken).toHaveBeenCalledWith(worktreeId) @@ -8558,6 +8716,28 @@ describe('registerWorktreeHandlers', () => { expect(getSshGitProviderMock).not.toHaveBeenCalled() }) + it('keeps metadata when local terminal teardown cannot be verified', async () => { + const worktreeId = 'repo-1::/workspace/feature-wt' + store.getRepo.mockReturnValue({ + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'removed-ssh-target' + }) + killAllProcessesForWorktreeMock.mockRejectedValue( + new Error('Failed to stop remote worktree terminals') + ) + + await expect(handlers['worktrees:forgetLocal'](null, { worktreeId })).rejects.toThrow( + 'Failed to stop remote worktree terminals' + ) + + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + expect(deleteWorktreeHistoryDirMock).not.toHaveBeenCalled() + }) + it('rejects forgetting a folder project root', async () => { const repo = { id: 'repo-folder', diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 06397b341db..f0d4d745fc9 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -1,5 +1,5 @@ /* oxlint-disable max-lines */ -import type { BrowserWindow } from 'electron' +import type { BrowserWindow, IpcMainInvokeEvent } from 'electron' import { ipcMain } from 'electron' import { readFile, stat } from 'node:fs/promises' import { randomUUID } from 'node:crypto' @@ -1393,8 +1393,70 @@ export function registerWorktreeHandlers( ) const worktreeRemovalsInFlight = new Map() + const runWithWorktreePtyTeardown = ( + worktreeId: string, + operation: () => Promise, + ptyScopeId?: string | null + ): Promise => + typeof runtime.runWithWorktreePtyTeardown === 'function' + ? runtime.runWithWorktreePtyTeardown(worktreeId, operation, ptyScopeId) + : operation() + const beginWorktreePtyTeardown = ( + worktreeId: string, + ptyScopeId?: string | null + ): Promise<() => void> => + typeof runtime.beginWorktreePtyTeardown === 'function' + ? runtime.beginWorktreePtyTeardown(worktreeId, ptyScopeId) + : Promise.resolve(() => {}) + const runWithWorktreeRemovalDedup = ( + worktreeId: string, + opts: { force: boolean; runHooks: boolean; hostId?: string; mode?: 'remove' | 'forget' }, + operation: () => Promise + ): Promise => + typeof runtime.runWithWorktreeRemovalDedup === 'function' + ? runtime.runWithWorktreeRemovalDedup(worktreeId, opts, operation) + : operation() + const runWithSuccessfulCleanupTeardown = ( + worktreeId: string, + connectionId: string | null | undefined, + operation: () => Promise + ): Promise => + runWithWorktreePtyTeardown( + worktreeId, + async () => { + // Why: orphan and stale-registration cleanup still retires an Orca + // workspace; its terminals must not outlive the last addressable owner. + await killAllProcessesForWorktree(worktreeId, { + runtime, + localProvider: getLocalPtyProvider(), + connectionId, + onPtyStopped: clearProviderPtyState + }) + return operation() + }, + connectionId + ) - ipcMain.handle( + const registerCrossSurfaceWorktreeRemovalHandler = ( + channel: string, + handler: (event: IpcMainInvokeEvent, args: RemoveWorktreeArgs) => Promise + ): void => { + ipcMain.handle(channel, (event, args: RemoveWorktreeArgs) => { + const { repoId } = parseWorktreeId(args.worktreeId) + const repo = getRepoForWorktreeRemoval(store, repoId, args.hostId) + return runWithWorktreeRemovalDedup( + args.worktreeId, + { + force: args.force === true, + runHooks: args.skipArchive !== true, + ...(repo ? { hostId: args.hostId ?? getRepoExecutionHostId(repo) } : {}) + }, + () => handler(event, args) + ) + }) + } + + registerCrossSurfaceWorktreeRemovalHandler( 'worktrees:remove', async (_event, args: RemoveWorktreeArgs): Promise => { const { repoId, worktreePath } = parseWorktreeId(args.worktreeId) @@ -1404,7 +1466,7 @@ export function registerWorktreeHandlers( } const inFlightKey = getWorktreeRemovalInFlightKey( args.worktreeId, - getRepoExecutionHostId(repo) + args.hostId ?? getRepoExecutionHostId(repo) ) const optionsKey = getWorktreeRemovalOptionsKey(args) const inFlightRemoval = worktreeRemovalsInFlight.get(inFlightKey) @@ -1425,19 +1487,24 @@ export function registerWorktreeHandlers( 'Cannot delete the project root workspace. Remove the folder project instead.' ) } - // Why: folder workspaces share one filesystem root, so there is no Git - // remove step to close shells; sweep PTYs before dropping metadata. - await killAllProcessesForWorktree(args.worktreeId, { - runtime, - localProvider: getLocalPtyProvider(), - onPtyStopped: clearProviderPtyState - }).catch((err) => { - console.warn(`[worktree-teardown] failed for ${args.worktreeId}:`, err) - }) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - preservedBranchCleanupByWorktreeId.delete(args.worktreeId) - notifyWorktreesChanged(mainWindow, repoId) - return {} + return runWithWorktreePtyTeardown( + args.worktreeId, + async () => { + // Why: folder workspaces share one filesystem root, so there is no Git + // remove step to close shells; sweep PTYs before dropping metadata. + await killAllProcessesForWorktree(args.worktreeId, { + runtime, + localProvider: getLocalPtyProvider(), + connectionId: repo.connectionId ?? null, + onPtyStopped: clearProviderPtyState + }) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + preservedBranchCleanupByWorktreeId.delete(args.worktreeId) + notifyWorktreesChanged(mainWindow, repoId) + return {} + }, + repo.connectionId ?? null + ) } // Why: the renderer-supplied worktreeId contains a filesystem path. @@ -1497,32 +1564,38 @@ export function registerWorktreeHandlers( if (!args.force) { throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) } - if (repo.connectionId) { - await fsProvider!.deletePath(worktreePath, true) - await cleanupUnusedWorktreePushTargetRemoteSsh( - provider!, - repo.path, - args.worktreeId, - removedPushTarget, - store - ) - } else { - await closeLocalWatcherForRemoval(worktreePath) - await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) - invalidateAuthorizedRootsCache() - } - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - preservedBranchCleanupByWorktreeId.delete(args.worktreeId) - notifyWorktreesChanged(mainWindow, repoId) - return {} + return runWithSuccessfulCleanupTeardown( + args.worktreeId, + repo.connectionId ?? null, + async () => { + if (repo.connectionId) { + await fsProvider!.deletePath(worktreePath, true) + await cleanupUnusedWorktreePushTargetRemoteSsh( + provider!, + repo.path, + args.worktreeId, + removedPushTarget, + store + ) + } else { + await closeLocalWatcherForRemoval(worktreePath) + await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + args.worktreeId, + removedPushTarget, + store, + localWorktreeGitOptions + ) + invalidateAuthorizedRootsCache() + } + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + preservedBranchCleanupByWorktreeId.delete(args.worktreeId) + notifyWorktreesChanged(mainWindow, repoId) + return {} + } + ) } if (!repo.connectionId) { const access = getLocalWorktreePathAccess(localWorktreeGitOptions) @@ -1545,21 +1618,27 @@ export function registerWorktreeHandlers( if (!args.force) { throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) } - await closeLocalWatcherForRemoval(worktreePath) - await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, + return runWithSuccessfulCleanupTeardown( args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions + repo.connectionId ?? null, + async () => { + await closeLocalWatcherForRemoval(worktreePath) + await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + args.worktreeId, + removedPushTarget, + store, + localWorktreeGitOptions + ) + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + preservedBranchCleanupByWorktreeId.delete(args.worktreeId) + invalidateAuthorizedRootsCache() + notifyWorktreesChanged(mainWindow, repoId) + return {} + } ) - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - preservedBranchCleanupByWorktreeId.delete(args.worktreeId) - invalidateAuthorizedRootsCache() - notifyWorktreesChanged(mainWindow, repoId) - return {} } } if (await isAlreadyRemovedWorktreePath(repo, worktreePath, localWorktreeGitOptions)) { @@ -1571,29 +1650,35 @@ export function registerWorktreeHandlers( // Why: a manually deleted worktree is already gone from Git and disk. // The sidebar delete action has persisted metadata proving this was // an Orca-known row, so no force confirmation is needed. - if (repo.connectionId) { - await cleanupUnusedWorktreePushTargetRemoteSsh( - provider!, - repo.path, - args.worktreeId, - removedPushTarget, - store - ) - } else { - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) - invalidateAuthorizedRootsCache() - } - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - preservedBranchCleanupByWorktreeId.delete(args.worktreeId) - notifyWorktreesChanged(mainWindow, repoId) - return {} + return runWithSuccessfulCleanupTeardown( + args.worktreeId, + repo.connectionId ?? null, + async () => { + if (repo.connectionId) { + await cleanupUnusedWorktreePushTargetRemoteSsh( + provider!, + repo.path, + args.worktreeId, + removedPushTarget, + store + ) + } else { + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + args.worktreeId, + removedPushTarget, + store, + localWorktreeGitOptions + ) + invalidateAuthorizedRootsCache() + } + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + preservedBranchCleanupByWorktreeId.delete(args.worktreeId) + notifyWorktreesChanged(mainWindow, repoId) + return {} + } + ) } throw new Error(`Refusing to delete unregistered worktree path: ${worktreePath}`) } @@ -1621,35 +1706,40 @@ export function registerWorktreeHandlers( removedMeta && (await isAlreadyRemovedWorktreePath(repo, canonicalWorktreePath, localWorktreeGitOptions)) ) { - const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ - canonicalWorktreePath, - repoPath: repo.path, - localWorktreeGitOptions, - registeredWorktree, - deleteBranch - }) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) - rememberPreservedBranchCleanupTarget( + return runWithSuccessfulCleanupTeardown( args.worktreeId, - removalResult, - registeredWorktree.head, - removedPushTarget + repo.connectionId ?? null, + async () => { + const removalResult = + await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + canonicalWorktreePath, + repoPath: repo.path, + localWorktreeGitOptions, + registeredWorktree, + deleteBranch + }) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + args.worktreeId, + removedPushTarget, + store, + localWorktreeGitOptions + ) + rememberPreservedBranchCleanupTarget( + args.worktreeId, + removalResult, + registeredWorktree.head, + removedPushTarget + ) + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + invalidateAuthorizedRootsCache() + notifyWorktreesChanged(mainWindow, repoId) + return removalResult ?? {} + } ) - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - invalidateAuthorizedRootsCache() - notifyWorktreesChanged(mainWindow, repoId) - return removalResult ?? {} } - let shouldTearDownPtys = true - // Run archive hook before removal so teardown scripts still see the worktree directory. const hooks = await getArchiveHooksForRemoval(repo) if (hooks?.scripts.archive && !args.skipArchive) { @@ -1683,30 +1773,42 @@ export function registerWorktreeHandlers( } const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} - const rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 - ? provider!.removeWorktree(canonicalWorktreePath, args.force, remoteRemoveOptions) - : provider!.removeWorktree(canonicalWorktreePath, args.force)) - const removalResult = preserveBranchHeadFallback( - rawRemovalResult, - registeredWorktree.head - ) - await cleanupUnusedWorktreePushTargetRemoteSsh( - provider!, - repo.path, - args.worktreeId, - removedPushTarget, - store - ) - rememberPreservedBranchCleanupTarget( + return runWithWorktreePtyTeardown( args.worktreeId, - removalResult, - registeredWorktree.head, - removedPushTarget + async () => { + await killAllProcessesForWorktree(args.worktreeId, { + runtime, + localProvider: getLocalPtyProvider(), + connectionId: repo.connectionId ?? null, + onPtyStopped: clearProviderPtyState + }) + const rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 + ? provider!.removeWorktree(canonicalWorktreePath, args.force, remoteRemoveOptions) + : provider!.removeWorktree(canonicalWorktreePath, args.force)) + const removalResult = preserveBranchHeadFallback( + rawRemovalResult, + registeredWorktree.head + ) + await cleanupUnusedWorktreePushTargetRemoteSsh( + provider!, + repo.path, + args.worktreeId, + removedPushTarget, + store + ) + rememberPreservedBranchCleanupTarget( + args.worktreeId, + removalResult, + registeredWorktree.head, + removedPushTarget + ) + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + notifyWorktreesChanged(mainWindow, repoId) + return removalResult ?? {} + }, + repo.connectionId ?? null ) - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - notifyWorktreesChanged(mainWindow, repoId) - return removalResult ?? {} } const refreshedWorktrees = hasLocalWorktreeGitOptions @@ -1754,137 +1856,137 @@ export function registerWorktreeHandlers( formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false) ) } - // Why: orphan cleanup does not need live shells to be killed first, - // and preflight did not prove the worktree is cleanly removable. - shouldTearDownPtys = false + // Why: missing/non-repo paths may still have a live deleted-cwd PTY; + // continue to the fenced teardown before orphan recovery mutates state. } - await closeLocalWatcherForRemoval(canonicalWorktreePath) + const releaseWorktreePtyTeardown = await beginWorktreePtyTeardown( + args.worktreeId, + repo.connectionId ?? null + ) + let removalResult: RemoveWorktreeResult | undefined + try { + await closeLocalWatcherForRemoval(canonicalWorktreePath) - if (shouldTearDownPtys) { - // Why: once preflight proves normal deletion is clean, kill PTYs before - // git-level removal so Windows handles cannot keep the directory busy. await killAllProcessesForWorktree(args.worktreeId, { runtime, localProvider: getLocalPtyProvider(), + connectionId: repo.connectionId ?? null, onPtyStopped: clearProviderPtyState + }).then((r) => { + const total = r.runtimeStopped + r.providerStopped + r.registryStopped + if (total > 0) { + console.info( + `[worktree-teardown] ${args.worktreeId} killed runtime=${r.runtimeStopped} provider=${r.providerStopped} registry=${r.registryStopped}` + ) + } }) - .then((r) => { - const total = r.runtimeStopped + r.providerStopped + r.registryStopped - if (total > 0) { - console.info( - `[worktree-teardown] ${args.worktreeId} killed runtime=${r.runtimeStopped} provider=${r.providerStopped} registry=${r.registryStopped}` - ) - } - }) - .catch((err) => { - console.warn(`[worktree-teardown] failed for ${args.worktreeId}:`, err) - }) - } - let removalResult: RemoveWorktreeResult | undefined - try { - const removeOptions = { - ...(!deleteBranch ? { deleteBranch } : {}), - // Why: this handler already paid for an authoritative worktree - // list to validate the target; reuse it instead of rescanning - // every sibling worktree during the hot delete path. - knownRemovedWorktree: refreshedRegisteredWorktree, - ...(hasLocalWorktreeGitOptions ? localWorktreeGitOptions : {}) - } - removalResult = preserveBranchHeadFallback( - await removeWorktree( - repo.path, - canonicalWorktreePath, - args.force ?? false, - removeOptions - ), - refreshedRegisteredWorktree.head - ) - } catch (error) { - // Why: Git for Windows can deregister a clean worktree before its - // recursive filesystem deletion fails transiently. - const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ - error, - force: args.force ?? false, - canonicalWorktreePath, - repoPath: repo.path, - localWorktreeGitOptions, - registeredWorktree: refreshedRegisteredWorktree, - deleteBranch, - closeWatcher: closeLocalWatcherForRemoval - }) - if (recoveredRemovalResult) { - removalResult = recoveredRemovalResult - } else if (isOrphanedWorktreeError(error)) { - // If git no longer tracks this worktree, clean up the directory and metadata - console.warn( - `[worktrees] Orphaned worktree detected at ${canonicalWorktreePath}, cleaning up` + try { + const removeOptions = { + ...(!deleteBranch ? { deleteBranch } : {}), + // Why: this handler already paid for an authoritative worktree + // list to validate the target; reuse it instead of rescanning + // every sibling worktree during the hot delete path. + knownRemovedWorktree: refreshedRegisteredWorktree, + ...(hasLocalWorktreeGitOptions ? localWorktreeGitOptions : {}) + } + removalResult = preserveBranchHeadFallback( + await removeWorktree( + repo.path, + canonicalWorktreePath, + args.force ?? false, + removeOptions + ), + refreshedRegisteredWorktree.head ) - const access = getLocalWorktreePathAccess(localWorktreeGitOptions) - if ( - await canSafelyRemoveOrphanedWorktreeDirectory( - toLocalWorktreeRuntimePath(canonicalWorktreePath, localWorktreeGitOptions), - toLocalWorktreeRuntimePath(repo.path, localWorktreeGitOptions), - access.statPath, - access.readPath + } catch (error) { + // Why: Git for Windows can deregister a clean worktree before its + // recursive filesystem deletion fails transiently. + const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ + error, + force: args.force ?? false, + canonicalWorktreePath, + repoPath: repo.path, + localWorktreeGitOptions, + registeredWorktree: refreshedRegisteredWorktree, + deleteBranch, + closeWatcher: closeLocalWatcherForRemoval + }) + if (recoveredRemovalResult) { + removalResult = recoveredRemovalResult + } else if (isOrphanedWorktreeError(error)) { + // If git no longer tracks this worktree, clean up the directory and metadata + console.warn( + `[worktrees] Orphaned worktree detected at ${canonicalWorktreePath}, cleaning up` ) - ) { - await closeLocalWatcherForRemoval(canonicalWorktreePath) - await removeLocalWorktreePath(canonicalWorktreePath, localWorktreeGitOptions).catch( - () => {} + const access = getLocalWorktreePathAccess(localWorktreeGitOptions) + if ( + await canSafelyRemoveOrphanedWorktreeDirectory( + toLocalWorktreeRuntimePath(canonicalWorktreePath, localWorktreeGitOptions), + toLocalWorktreeRuntimePath(repo.path, localWorktreeGitOptions), + access.statPath, + access.readPath + ) + ) { + await closeLocalWatcherForRemoval(canonicalWorktreePath) + await removeLocalWorktreePath(canonicalWorktreePath, localWorktreeGitOptions).catch( + () => {} + ) + } else { + console.warn( + `[worktrees] Refusing recursive cleanup for unproven worktree directory: ${canonicalWorktreePath}` + ) + } + // Why: `git worktree remove` failed, so git's internal worktree tracking + // (`.git/worktrees/`) is still intact. Without pruning, `git worktree + // list` continues to show the stale entry and the branch it had checked out + // remains locked — other worktrees cannot check it out. + await gitExecFileAsync(['worktree', 'prune'], { + cwd: repo.path, + ...localWorktreeGitOptions + }).catch(() => {}) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + args.worktreeId, + removedPushTarget, + store, + localWorktreeGitOptions ) + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + preservedBranchCleanupByWorktreeId.delete(args.worktreeId) + invalidateAuthorizedRootsCache() + notifyWorktreesChanged(mainWindow, repoId) + return {} } else { - console.warn( - `[worktrees] Refusing recursive cleanup for unproven worktree directory: ${canonicalWorktreePath}` + throw new Error( + formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false) ) } - // Why: `git worktree remove` failed, so git's internal worktree tracking - // (`.git/worktrees/`) is still intact. Without pruning, `git worktree - // list` continues to show the stale entry and the branch it had checked out - // remains locked — other worktrees cannot check it out. - await gitExecFileAsync(['worktree', 'prune'], { - cwd: repo.path, - ...localWorktreeGitOptions - }).catch(() => {}) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - preservedBranchCleanupByWorktreeId.delete(args.worktreeId) - invalidateAuthorizedRootsCache() - notifyWorktreesChanged(mainWindow, repoId) - return {} - } else { - throw new Error( - formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false) - ) } - } - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) - rememberPreservedBranchCleanupTarget( - args.worktreeId, - removalResult, - refreshedRegisteredWorktree.head, - removedPushTarget - ) - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - invalidateAuthorizedRootsCache() + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + args.worktreeId, + removedPushTarget, + store, + localWorktreeGitOptions + ) + rememberPreservedBranchCleanupTarget( + args.worktreeId, + removalResult, + refreshedRegisteredWorktree.head, + removedPushTarget + ) + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + invalidateAuthorizedRootsCache() - notifyWorktreesChanged(mainWindow, repoId) - return removalResult ?? {} + notifyWorktreesChanged(mainWindow, repoId) + return removalResult ?? {} + } finally { + releaseWorktreePtyTeardown() + } })() worktreeRemovalsInFlight.set(inFlightKey, { optionsKey, promise: removal }) try { @@ -1897,13 +1999,36 @@ export function registerWorktreeHandlers( } ) + const registerCrossSurfaceWorktreeForgetHandler = ( + channel: string, + handler: ( + event: IpcMainInvokeEvent, + args: Pick + ) => Promise + ): void => { + ipcMain.handle(channel, (event, args: Pick) => { + const { repoId } = parseWorktreeId(args.worktreeId) + const repo = getRepoForWorktreeRemoval(store, repoId, args.hostId) + return runWithWorktreeRemovalDedup( + args.worktreeId, + { + force: false, + runHooks: false, + mode: 'forget', + ...(repo ? { hostId: args.hostId ?? getRepoExecutionHostId(repo) } : {}) + }, + () => handler(event, args) + ) + }) + } + // Why: forget-locally drops a workspace from Orca without any remote Git or // filesystem work. It exists so a workspace pinned to a removed/disconnected // SSH target — whose provider is gone and whose `worktrees:remove` therefore // throws at requireSshGitProvider before any cleanup runs — can still be // cleared from the app. It never touches the remote: no worktree registration, // no branches, no files are deleted there. - ipcMain.handle( + registerCrossSurfaceWorktreeForgetHandler( 'worktrees:forgetLocal', async ( _event, @@ -1919,7 +2044,7 @@ export function registerWorktreeHandlers( // mutate metadata. A forget takes no force/skipArchive options. const inFlightKey = getWorktreeRemovalInFlightKey( args.worktreeId, - getRepoExecutionHostId(repo) + args.hostId ?? getRepoExecutionHostId(repo) ) const optionsKey = 'forget-local' const inFlight = worktreeRemovalsInFlight.get(inFlightKey) @@ -1930,30 +2055,32 @@ export function registerWorktreeHandlers( throw new Error(`Worktree deletion already in progress: ${args.worktreeId}`) } - const forget = (async (): Promise => { - if (isFolderRepo(repo) && args.worktreeId === getFolderWorkspaceRootId(repo)) { - throw new Error( - 'Cannot delete the project root workspace. Remove the folder project instead.' - ) - } + const forget = runWithWorktreePtyTeardown( + args.worktreeId, + async () => { + if (isFolderRepo(repo) && args.worktreeId === getFolderWorkspaceRootId(repo)) { + throw new Error( + 'Cannot delete the project root workspace. Remove the folder project instead.' + ) + } - // Why: best-effort PTY sweep. killAllProcessesForWorktree resolves - // synchronously for a dead SSH relay (the provider tombstones the lease - // and returns without awaiting the remote), so this never hangs. - await killAllProcessesForWorktree(args.worktreeId, { - runtime, - localProvider: getLocalPtyProvider(), - onPtyStopped: clearProviderPtyState - }).catch((err) => { - console.warn(`[worktree-teardown] forget-local failed for ${args.worktreeId}:`, err) - }) + // Why: forgetting metadata releases Orca's last addressable owner. An + // unverified terminal must keep that owner intact so cleanup can retry. + await killAllProcessesForWorktree(args.worktreeId, { + runtime, + localProvider: getLocalPtyProvider(), + connectionId: repo.connectionId ?? null, + onPtyStopped: clearProviderPtyState + }) - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState(store, args.worktreeId) - preservedBranchCleanupByWorktreeId.delete(args.worktreeId) - notifyWorktreesChanged(mainWindow, repoId) - return {} - })() + runtime.clearOptimisticReconcileToken(args.worktreeId) + removeWorktreeMetadataAndTransientState(store, args.worktreeId) + preservedBranchCleanupByWorktreeId.delete(args.worktreeId) + notifyWorktreesChanged(mainWindow, repoId) + return {} + }, + repo.connectionId ?? null + ) worktreeRemovalsInFlight.set(inFlightKey, { optionsKey, promise: forget }) try { return await forget diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 66f68d6e868..4b64c02845c 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -9,7 +9,8 @@ const { mkdirSyncMock, writeFileSyncMock, spawnMock, - resolveAgentForegroundProcessMock + resolveAgentForegroundProcessMock, + killPosixPtySessionMock } = vi.hoisted(() => ({ existsSyncMock: vi.fn(), statSyncMock: vi.fn(), @@ -17,7 +18,8 @@ const { mkdirSyncMock: vi.fn(), writeFileSyncMock: vi.fn(), spawnMock: vi.fn(), - resolveAgentForegroundProcessMock: vi.fn() + resolveAgentForegroundProcessMock: vi.fn(), + killPosixPtySessionMock: vi.fn() })) vi.mock('fs', () => ({ @@ -40,6 +42,10 @@ vi.mock('node-pty', () => ({ spawn: spawnMock })) +vi.mock('../../relay/pty-session-kill', () => ({ + killPosixPtySession: killPosixPtySessionMock +})) + // Resolve PowerShell family names to deterministic absolute paths (the fs mock // above otherwise makes every probe miss). The real resolver — which skips the // Store App Execution Alias stub — is covered in @@ -123,6 +129,8 @@ describe('LocalPtyProvider', () => { accessSyncMock.mockReturnValue(undefined) mkdirSyncMock.mockReset() writeFileSyncMock.mockReset() + killPosixPtySessionMock.mockReset() + killPosixPtySessionMock.mockResolvedValue(true) resolveAgentForegroundProcessMock.mockReset() resolveAgentForegroundProcessMock.mockImplementation( async (_pid: number, fallbackProcess: string | null) => fallbackProcess @@ -942,7 +950,7 @@ describe('LocalPtyProvider', () => { }) describe('shutdown', () => { - it('kills the PTY process', async () => { + it('kills the complete POSIX PTY session', async () => { // Why: capture the spy reference before shutdown triggers onExit → // POSIX kill neutralization. After neutralization, mockProc.kill is // replaced with a non-spy no-op to close the UnixTerminal.destroy() → @@ -950,7 +958,8 @@ describe('LocalPtyProvider', () => { const killSpy = mockProc.kill const { id } = await provider.spawn({ cols: 80, rows: 24 }) await provider.shutdown(id, { immediate: true }) - expect(killSpy).toHaveBeenCalled() + expect(killPosixPtySessionMock).toHaveBeenCalledWith(12345, undefined) + expect(killSpy).not.toHaveBeenCalled() }) it('invokes onExit callback via the node-pty exit handler', async () => { @@ -963,7 +972,9 @@ 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() }) @@ -980,6 +991,56 @@ describe('LocalPtyProvider', () => { expect(destroySpy).not.toHaveBeenCalled() }) + it('awaits Windows native exit and does not double-kill after a timeout', async () => { + vi.useFakeTimers() + try { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const killSpy = vi.fn() + spawnMock.mockReturnValue({ + ...mockProc, + kill: killSpy + }) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + const firstStop = provider.shutdown(id, { immediate: true }) + const firstStopRejected = expect(firstStop).rejects.toThrow( + `Unable to verify full PTY session teardown: ${id}` + ) + await vi.advanceTimersByTimeAsync(8000) + await firstStopRejected + expect((await provider.listProcesses()).some((session) => session.id === id)).toBe(true) + + const retry = provider.shutdown(id, { immediate: false }) + expect(killSpy).toHaveBeenCalledTimes(1) + exitCb?.({ exitCode: -1 }) + await expect(retry).resolves.toBeUndefined() + expect((await provider.listProcesses()).some((session) => session.id === id)).toBe(false) + } finally { + vi.useRealTimers() + } + }) + + it('retains the ConPTY owner when the native Windows kill throws', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const killError = new Error('native close failed') + spawnMock.mockReturnValue({ + ...mockProc, + kill: vi.fn(() => { + throw killError + }) + }) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow(killError) + + expect((await provider.listProcesses()).some((session) => session.id === String(id))).toBe( + true + ) + Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) + killPosixPtySessionMock.mockResolvedValue(true) + await provider.shutdown(id, { immediate: true }) + }) + it('cancels pending shell-ready startup delivery on forced shutdown', async () => { vi.useFakeTimers() try { @@ -999,6 +1060,21 @@ describe('LocalPtyProvider', () => { await provider.shutdown('nonexistent', { immediate: true }) expect(mockProc.kill).not.toHaveBeenCalled() }) + + it('retains the PTY owner when complete POSIX teardown is unverified', async () => { + killPosixPtySessionMock.mockResolvedValue(false) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow( + `Unable to verify full PTY session teardown: ${id}` + ) + + expect((await provider.listProcesses()).some((session) => session.id === String(id))).toBe( + true + ) + killPosixPtySessionMock.mockResolvedValue(true) + await provider.shutdown(id, { immediate: true }) + }) }) describe('hasChildProcesses', () => { diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 90ccae11ea6..dd7c126e4fe 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -58,6 +58,7 @@ import { readWindowsConptyProcessIds } from './windows-conpty-process-membership import { shouldUseShellReadyStartupDelivery } from '../../shared/codex-startup-delivery' import { assertSafeAgentStartupCwd, resolveSafePtyDefaultCwd } from './pty-default-cwd' import { ORCA_HERMES_STARTUP_QUERY_ENV } from '../../shared/hermes-startup-query' +import { killPosixPtySession } from '../../relay/pty-session-kill' const PANE_IDENTITY_ENV_KEYS = [ 'ORCA_PANE_KEY', @@ -77,6 +78,11 @@ const ptyTerminalHandle = new Map() // to invoke/clean them up on a destroyed environment, triggering a SIGABRT. const ptyDisposables = new Map void }[]>() const ptyCleanupCallbacks = new Map void>() +const windowsImmediateKills = new WeakSet() + +// Why: node-pty can spend up to 5s enumerating a Windows console tree before +// its fallback runs; leave bounded headroom for the native exit event to land. +const WINDOWS_PTY_EXIT_TIMEOUT_MS = 8_000 let loadGeneration = 0 const ptyLoadGeneration = new Map() @@ -182,6 +188,44 @@ function clearPtyState(id: string): void { ptyLoadGeneration.delete(id) } +function waitForPtyExit( + id: string, + proc: pty.IPty +): { + promise: Promise + cancel: () => void +} { + let settled = false + let timer: NodeJS.Timeout | undefined + let resolvePromise: (exited: boolean) => void = () => {} + const finish = (exited: boolean): void => { + if (settled) { + return + } + settled = true + if (timer) { + clearTimeout(timer) + } + exitListeners.delete(onExit) + resolvePromise(exited) + } + const onExit: ExitCallback = (payload) => { + if (payload.id === id) { + finish(true) + } + } + const promise = new Promise((resolve) => { + resolvePromise = resolve + exitListeners.add(onExit) + if (ptyProcesses.get(id) !== proc) { + finish(true) + return + } + timer = setTimeout(() => finish(false), WINDOWS_PTY_EXIT_TIMEOUT_MS) + }) + return { promise, cancel: () => finish(false) } +} + /** * Allocates either a stable caller-provided PTY id or a new numeric id. */ @@ -800,7 +844,7 @@ export class LocalPtyProvider implements IPtyProvider { // Why: release the master ptmx fd on the natural-exit path — without // this, a shell that exits cleanly (the common case) never releases its // fd until the next GC. See docs/fix-pty-fd-leak.md. - destroyPtyProcess(proc) + destroyPtyProcess(proc, { alreadyKilled: windowsImmediateKills.has(proc) }) this.opts.onExit?.(id, exitCode) for (const cb of exitListeners) { cb({ id, code: exitCode }) @@ -882,11 +926,48 @@ export class LocalPtyProvider implements IPtyProvider { return { cols: proc.cols, rows: proc.rows } } - async shutdown(id: string, _opts: { immediate?: boolean; keepHistory?: boolean }): Promise { + async shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise { const proc = ptyProcesses.get(id) if (!proc) { return } + if (process.platform === 'win32' && windowsImmediateKills.has(proc)) { + if (!(await waitForPtyExit(id, proc).promise)) { + throw new Error(`Unable to verify full PTY session teardown: ${id}`) + } + return + } + let alreadyKilled = false + if (opts.immediate) { + if (process.platform === 'win32') { + // Why: ConPTY close completes asynchronously. Deletion must await the + // native exit event, while a timeout keeps the owner retryable. + const exitWaiter = waitForPtyExit(id, proc) + try { + windowsImmediateKills.add(proc) + proc.kill() + } catch (error) { + exitWaiter.cancel() + windowsImmediateKills.delete(proc) + if (ptyProcesses.get(id) === proc) { + throw error + } + } + if (!(await exitWaiter.promise)) { + throw new Error(`Unable to verify full PTY session teardown: ${id}`) + } + return + } else { + const killedSession = await killPosixPtySession( + proc.pid, + (proc as unknown as { ptsName?: unknown }).ptsName + ) + if (!killedSession) { + throw new Error(`Unable to verify full PTY session teardown: ${id}`) + } + alreadyKilled = true + } + } // 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. @@ -894,10 +975,12 @@ export class LocalPtyProvider implements IPtyProvider { // the natural onExit callback from running the usual clearPtyState path. runPtyCleanup(id) disposePtyListeners(id) - try { - proc.kill() - } catch { - /* Process may already be dead */ + if (!alreadyKilled) { + try { + proc.kill() + } catch { + /* Generic terminal close preserves the previous already-dead behavior. */ + } } destroyPtyProcess(proc, { alreadyKilled: true }) clearPtyState(id) diff --git a/src/main/providers/ssh-pty-liveness.test.ts b/src/main/providers/ssh-pty-liveness.test.ts new file mode 100644 index 00000000000..59a00e00057 --- /dev/null +++ b/src/main/providers/ssh-pty-liveness.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it, vi } from 'vitest' +import { JsonRpcErrorCode } from '../ssh/relay-protocol' +import { SshPtyLiveness } from './ssh-pty-liveness' + +function methodNotFound(): Error & { code: number } { + return Object.assign(new Error('Method not found'), { code: JsonRpcErrorCode.MethodNotFound }) +} + +describe('SshPtyLiveness', () => { + it('clears transient legacy overrides after an inventory rejection', async () => { + let rejectInventory = (_error: Error): void => {} + const listIds = vi.fn( + () => + new Promise((_resolve, reject) => { + rejectInventory = reject + }) + ) + const liveness = new SshPtyLiveness({ + probe: vi.fn().mockRejectedValue(methodNotFound()), + listIds + }) + const checking = liveness.hasPty('pty-probe') + await vi.waitFor(() => expect(listIds).toHaveBeenCalledTimes(1)) + + for (let index = 0; index < 100; index += 1) { + liveness.markStopped(`pty-${index}`) + } + rejectInventory(new Error('connection lost')) + await expect(checking).rejects.toThrow('connection lost') + + expect( + (liveness as unknown as { legacyMembershipOverrides: Map }) + .legacyMembershipOverrides.size + ).toBe(0) + }) + + it('bounds override growth and invalidates the in-flight legacy snapshot', async () => { + let resolveInventory = (_ids: string[]): void => {} + const listIds = vi.fn( + () => + new Promise((resolve) => { + resolveInventory = resolve + }) + ) + const liveness = new SshPtyLiveness({ + probe: vi.fn().mockRejectedValue(methodNotFound()), + listIds + }) + const checking = liveness.hasPty('pty-probe') + await vi.waitFor(() => expect(listIds).toHaveBeenCalledTimes(1)) + + for (let index = 0; index < 10_000; index += 1) { + liveness.markStopped(`pty-${index}`) + } + expect( + (liveness as unknown as { legacyMembershipOverrides: Map }) + .legacyMembershipOverrides.size + ).toBe(0) + + resolveInventory([]) + await expect(checking).rejects.toThrow( + 'SSH legacy PTY inventory invalidated during liveness updates' + ) + }) +}) diff --git a/src/main/providers/ssh-pty-liveness.ts b/src/main/providers/ssh-pty-liveness.ts new file mode 100644 index 00000000000..091e2c5ddd4 --- /dev/null +++ b/src/main/providers/ssh-pty-liveness.ts @@ -0,0 +1,154 @@ +import { JsonRpcErrorCode } from '../ssh/relay-protocol' + +type SshPtyLivenessOptions = { + probe: (id: string) => Promise + listIds: () => Promise +} + +const MAX_LEGACY_MEMBERSHIP_OVERRIDES = 256 + +export class SshPtyLiveness { + private supportsTargetedProbe: boolean | undefined + private targetedProbeInFlight: Promise | undefined + private legacyIds: Set | undefined + private legacyInventoryInFlight: Promise> | undefined + private legacyInventoryAcceptsOverrides = false + private legacyInventoryGeneration = 0 + private legacyMembershipOverrides = new Map() + + constructor(private readonly options: SshPtyLivenessOptions) {} + + dispose(): void { + this.targetedProbeInFlight = undefined + this.legacyInventoryGeneration += 1 + this.legacyIds = undefined + this.legacyInventoryInFlight = undefined + this.legacyInventoryAcceptsOverrides = false + this.legacyMembershipOverrides.clear() + } + + markLive(id: string): void { + this.updateLegacyMembership(id, true) + } + + markStopped(id: string): void { + this.updateLegacyMembership(id, false) + } + + async hasPty(id: string): Promise { + if (this.supportsTargetedProbe === false) { + return (await this.getLegacyIds()).has(id) + } + if (this.supportsTargetedProbe === true) { + return this.options.probe(id) + } + if (this.targetedProbeInFlight) { + await this.targetedProbeInFlight + return this.hasPty(id) + } + const probe = this.probeCapability(id) + this.targetedProbeInFlight = probe + try { + return await probe + } finally { + if (this.targetedProbeInFlight === probe) { + this.targetedProbeInFlight = undefined + } + } + } + + private async probeCapability(id: string): Promise { + try { + const live = await this.options.probe(id) + this.supportsTargetedProbe = true + this.legacyIds = undefined + return live + } catch (error) { + if ( + !error || + typeof error !== 'object' || + (error as { code?: unknown }).code !== JsonRpcErrorCode.MethodNotFound + ) { + throw error + } + // Why: a new desktop may reconnect to a preserved older relay. Cache the + // narrow protocol miss and one inventory for the multi-PTY stop sequence. + this.supportsTargetedProbe = false + return (await this.getLegacyIds()).has(id) + } + } + + private updateLegacyMembership(id: string, live: boolean): void { + if (this.legacyIds) { + if (live) { + this.legacyIds.add(id) + } else { + this.legacyIds.delete(id) + } + } else if ( + this.supportsTargetedProbe === false && + this.legacyInventoryInFlight && + this.legacyInventoryAcceptsOverrides + ) { + // Why: lifecycle changes can settle while the legacy inventory RPC is + // in flight; replay them onto its result before any liveness read. + if ( + !this.legacyMembershipOverrides.has(id) && + this.legacyMembershipOverrides.size >= MAX_LEGACY_MEMBERSHIP_OVERRIDES + ) { + // Why: a corrupt/hostile relay must not grow this transient map without + // bound. Invalidate the snapshot so no caller trusts dropped updates. + this.legacyInventoryGeneration += 1 + this.legacyMembershipOverrides.clear() + this.legacyInventoryAcceptsOverrides = false + return + } + this.legacyMembershipOverrides.set(id, live) + } + } + + private async getLegacyIds(): Promise> { + if (this.legacyIds) { + return this.legacyIds + } + if (this.legacyInventoryInFlight) { + return this.legacyInventoryInFlight + } + const generation = this.legacyInventoryGeneration + this.legacyInventoryAcceptsOverrides = true + const inventory = this.options + .listIds() + .then((ids) => { + if (this.legacyInventoryGeneration !== generation) { + throw new Error('SSH legacy PTY inventory invalidated during liveness updates') + } + const liveIds = new Set(ids) + for (const [id, live] of this.legacyMembershipOverrides) { + if (live) { + liveIds.add(id) + } else { + liveIds.delete(id) + } + } + this.legacyMembershipOverrides.clear() + this.legacyIds = liveIds + return liveIds + }) + .catch((error) => { + // Why: a failed/invalidated inventory is not a reusable baseline. Its + // transient overrides would otherwise accumulate forever across retries. + this.legacyMembershipOverrides.clear() + throw error + }) + this.legacyInventoryInFlight = inventory + try { + return await inventory + } finally { + if (this.legacyInventoryInFlight === inventory) { + this.legacyInventoryInFlight = undefined + this.legacyInventoryAcceptsOverrides = false + this.legacyMembershipOverrides.clear() + } + } + } +} diff --git a/src/main/providers/ssh-pty-provider.test.ts b/src/main/providers/ssh-pty-provider.test.ts index ca0a9fd8261..032f0bf9de5 100644 --- a/src/main/providers/ssh-pty-provider.test.ts +++ b/src/main/providers/ssh-pty-provider.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it, vi, beforeEach } from 'vitest' -import { SshPtyProvider } from './ssh-pty-provider' +import { + SSH_PTY_IMMEDIATE_SHUTDOWN_TIMEOUT_MS, + SSH_PTY_LEGACY_INVENTORY_TIMEOUT_MS, + SSH_PTY_TEARDOWN_LIVENESS_TIMEOUT_MS, + SshPtyProvider +} from './ssh-pty-provider' import { POWERLEVEL10K_WIZARD_DISABLE_ENV } from '../pty/powerlevel10k-wizard-env' +import { JsonRpcErrorCode } from '../ssh/relay-protocol' type MockMultiplexer = { request: ReturnType @@ -416,22 +422,59 @@ describe('SshPtyProvider', () => { expect(mux.notify).toHaveBeenCalledWith('pty.resize', { id: 'pty-1', cols: 120, rows: 40 }) }) - it('shutdown sends pty.shutdown request', async () => { + it('immediate shutdown requires the full-session relay method', async () => { await provider.shutdown(scopedPty1, { immediate: true }) - expect(mux.request).toHaveBeenCalledWith('pty.shutdown', { - id: 'pty-1', - immediate: true, - keepHistory: false + expect(mux.request).toHaveBeenCalledWith( + 'pty.shutdownSession', + { + id: 'pty-1', + immediate: true, + keepHistory: false + }, + { timeoutMs: SSH_PTY_IMMEDIATE_SHUTDOWN_TIMEOUT_MS } + ) + }) + + it('keeps graceful legacy liveness authoritative until the relay exit event', async () => { + let notification: ((method: string, params: Record) => void) | undefined + mux.onNotification.mockImplementation((callback) => { + notification = callback + return () => {} + }) + mux.request.mockImplementation(async (method: string) => { + if (method === 'pty.shutdown') { + return undefined + } + if (method === 'pty.hasPty') { + throw Object.assign(new Error('Method not found'), { + code: JsonRpcErrorCode.MethodNotFound + }) + } + if (method === 'pty.listProcesses') { + return [{ id: 'pty-1', cwd: '/work/demo', title: 'shell' }] + } + throw new Error(`Unexpected request: ${method}`) }) + provider = new SshPtyProvider('conn-1', mux as never) + + await provider.shutdown(scopedPty1, { immediate: false }) + await expect(provider.hasPtyAsync(scopedPty1)).resolves.toBe(true) + + notification?.('pty.exit', { id: 'pty-1', code: 0 }) + await expect(provider.hasPtyAsync(scopedPty1)).resolves.toBe(false) }) it('shutdown forwards keepHistory: true over the relay', async () => { await provider.shutdown(scopedPty1, { immediate: true, keepHistory: true }) - expect(mux.request).toHaveBeenCalledWith('pty.shutdown', { - id: 'pty-1', - immediate: true, - keepHistory: true - }) + expect(mux.request).toHaveBeenCalledWith( + 'pty.shutdownSession', + { + id: 'pty-1', + immediate: true, + keepHistory: true + }, + { timeoutMs: SSH_PTY_IMMEDIATE_SHUTDOWN_TIMEOUT_MS } + ) }) it('sendSignal sends pty.sendSignal request', async () => { @@ -492,6 +535,136 @@ describe('SshPtyProvider', () => { expect(result).toEqual([{ id: scopedPty1, cwd: '/home', title: 'zsh' }]) }) + it('uses targeted relay liveness without listing sessions', async () => { + mux.request.mockImplementation(async (method: string, params?: { id?: string }) => { + if (method === 'pty.hasPty') { + return params?.id === 'pty-1' + } + throw new Error(`Unexpected request: ${method}`) + }) + + await expect(provider.hasPtyAsync(scopedPty1)).resolves.toBe(true) + await expect(provider.hasPtyAsync('ssh:conn-1@@pty-2')).resolves.toBe(false) + + expect(mux.request.mock.calls.filter(([method]) => method === 'pty.hasPty')).toHaveLength(2) + for (const call of mux.request.mock.calls.filter(([method]) => method === 'pty.hasPty')) { + expect(call[2]).toEqual({ timeoutMs: SSH_PTY_TEARDOWN_LIVENESS_TIMEOUT_MS }) + } + expect( + mux.request.mock.calls.filter(([method]) => method === 'pty.listProcesses') + ).toHaveLength(0) + }) + + it('caches an older relay method miss and falls back to inventories', async () => { + mux.request.mockImplementation(async (method: string) => { + if (method === 'pty.hasPty') { + throw Object.assign(new Error('Method not found'), { + code: JsonRpcErrorCode.MethodNotFound + }) + } + if (method === 'pty.listProcesses') { + return [ + { id: 'pty-1', cwd: '/home', title: 'zsh' }, + { id: 'pty-2', cwd: '/home', title: 'zsh' } + ] + } + if (method === 'pty.spawn') { + return { id: 'pty-3' } + } + if (method === 'pty.shutdownSession' || method === 'pty.attach') { + return undefined + } + throw new Error(`Unexpected request: ${method}`) + }) + + await expect( + Promise.all([provider.hasPtyAsync(scopedPty1), provider.hasPtyAsync('ssh:conn-1@@pty-2')]) + ).resolves.toEqual([true, true]) + + expect(mux.request.mock.calls.filter(([method]) => method === 'pty.hasPty')).toHaveLength(1) + expect( + mux.request.mock.calls.filter(([method]) => method === 'pty.listProcesses') + ).toHaveLength(1) + expect(mux.request.mock.calls.find(([method]) => method === 'pty.listProcesses')?.[2]).toEqual({ + timeoutMs: SSH_PTY_LEGACY_INVENTORY_TIMEOUT_MS + }) + + await provider.shutdown(scopedPty1, { immediate: true }) + await expect(provider.hasPtyAsync(scopedPty1)).resolves.toBe(false) + + const spawned = await provider.spawn({ cols: 80, rows: 24 }) + await expect(provider.hasPtyAsync(spawned.id)).resolves.toBe(true) + + const attached = 'ssh:conn-1@@pty-4' + await provider.attach(attached) + await expect(provider.hasPtyAsync(attached)).resolves.toBe(true) + const notificationHandler = mux.onNotification.mock.calls[0]?.[0] + notificationHandler?.('pty.exit', { id: 'pty-4', code: 0 }) + await expect(provider.hasPtyAsync(attached)).resolves.toBe(false) + + expect(mux.request.mock.calls.filter(([method]) => method === 'pty.hasPty')).toHaveLength(1) + expect( + mux.request.mock.calls.filter(([method]) => method === 'pty.listProcesses') + ).toHaveLength(1) + }) + + it('applies shutdowns that complete while the legacy inventory is in flight', async () => { + let resolveInventory = (_sessions: { id: string; cwd: string; title: string }[]): void => { + throw new Error('inventory resolver not initialized') + } + const inventory = new Promise<{ id: string; cwd: string; title: string }[]>((resolve) => { + resolveInventory = resolve + }) + mux.request.mockImplementation(async (method: string) => { + if (method === 'pty.hasPty') { + throw Object.assign(new Error('Method not found'), { + code: JsonRpcErrorCode.MethodNotFound + }) + } + if (method === 'pty.listProcesses') { + return inventory + } + if (method === 'pty.shutdownSession') { + return undefined + } + throw new Error(`Unexpected request: ${method}`) + }) + + const liveness = provider.hasPtyAsync(scopedPty1) + await vi.waitFor(() => { + expect(mux.request.mock.calls.some(([method]) => method === 'pty.listProcesses')).toBe(true) + }) + await provider.shutdown(scopedPty1, { immediate: true }) + resolveInventory([{ id: 'pty-1', cwd: '/home', title: 'zsh' }]) + + await expect(liveness).resolves.toBe(false) + expect(mux.request.mock.calls.filter(([method]) => method === 'pty.hasPty')).toHaveLength(1) + expect( + mux.request.mock.calls.filter(([method]) => method === 'pty.listProcesses') + ).toHaveLength(1) + }) + + it('fails closed when an older relay lacks full-session shutdown', async () => { + mux.request.mockImplementation(async (method: string) => { + if (method === 'pty.shutdownSession') { + throw Object.assign(new Error('Method not found'), { + code: JsonRpcErrorCode.MethodNotFound + }) + } + if (method === 'pty.listProcesses') { + return [] + } + throw new Error(`Unexpected request: ${method}`) + }) + + await expect(provider.shutdown(scopedPty1, { immediate: true })).rejects.toThrow( + 'Method not found' + ) + expect( + mux.request.mock.calls.filter(([method]) => method === 'pty.listProcesses') + ).toHaveLength(0) + }) + it('getDefaultShell returns shell path', async () => { mux.request.mockResolvedValue('/bin/bash') const result = await provider.getDefaultShell() diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index e386e89dbba..85b25adac70 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -1,6 +1,7 @@ import type { SshChannelMultiplexer } from '../ssh/ssh-channel-multiplexer' import type { IPtyProvider, PtyProcessInfo, PtySpawnOptions, PtySpawnResult } from './types' import { toAppSshPtyId, toRelaySshPtyId } from './ssh-pty-id' +import { SshPtyLiveness } from './ssh-pty-liveness' import { seedPowerlevel10kWizardEnv } from '../pty/powerlevel10k-wizard-env' type DataCallback = (payload: { id: string; data: string }) => void @@ -16,6 +17,11 @@ type RemoteCliBridgeEnv = { export const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED' export const SSH_PTY_IDENTITY_MISMATCH_ERROR = 'SSH_PTY_IDENTITY_MISMATCH' +// Why: eight bounded workers keep a 50-PTY failure near one 30s batch, +// while leaving enough time for the relay's targeted 3s kill-and-verify path. +export const SSH_PTY_IMMEDIATE_SHUTDOWN_TIMEOUT_MS = 3250 +export const SSH_PTY_TEARDOWN_LIVENESS_TIMEOUT_MS = 1000 +export const SSH_PTY_LEGACY_INVENTORY_TIMEOUT_MS = 2000 export function isSshPtyNotFoundError(err: unknown): boolean { const message = err instanceof Error ? err.message : String(err) @@ -38,6 +44,7 @@ export class SshPtyProvider implements IPtyProvider { private dataListeners = new Set() private replayListeners = new Set() private exitListeners = new Set() + private ptyLiveness: SshPtyLiveness // Why: store the unsubscribe handle so dispose() can detach from the // multiplexer. Without this, notification callbacks keep firing after // the provider is torn down on disconnect, routing events to stale state. @@ -50,6 +57,18 @@ export class SshPtyProvider implements IPtyProvider { ) { this.connectionId = connectionId this.mux = mux + this.ptyLiveness = new SshPtyLiveness({ + probe: async (id) => + (await this.mux.request( + 'pty.hasPty', + { id: this.toRelayPtyId(id) }, + { timeoutMs: SSH_PTY_TEARDOWN_LIVENESS_TIMEOUT_MS } + )) === true, + listIds: async () => + (await this.requestProcessList(SSH_PTY_LEGACY_INVENTORY_TIMEOUT_MS)).map( + (session) => session.id + ) + }) // Subscribe to relay notifications for PTY events this.unsubscribeNotifications = mux.onNotification((method, params) => { @@ -67,6 +86,7 @@ export class SshPtyProvider implements IPtyProvider { break case 'pty.exit': + this.ptyLiveness.markStopped(this.toAppPtyId(params.id as string)) for (const cb of this.exitListeners) { cb({ id: this.toAppPtyId(params.id as string), code: params.code as number }) } @@ -83,6 +103,7 @@ export class SshPtyProvider implements IPtyProvider { this.dataListeners.clear() this.replayListeners.clear() this.exitListeners.clear() + this.ptyLiveness.dispose() } getConnectionId(): string { @@ -124,8 +145,10 @@ export class SshPtyProvider implements IPtyProvider { console.warn( `[ssh-pty] pty.attach succeeded for ${opts.sessionId}, replay=${!!attachResult.replay}` ) + const appPtyId = this.toAppPtyId(relaySessionId) + this.ptyLiveness.markLive(appPtyId) return { - id: this.toAppPtyId(relaySessionId), + id: appPtyId, isReattach: true, ...(attachResult.replay ? { replay: attachResult.replay } : {}) } @@ -168,9 +191,11 @@ export class SshPtyProvider implements IPtyProvider { ...(opts.paneKey ? { paneKey: opts.paneKey } : {}), ...(opts.tabId ? { tabId: opts.tabId } : {}) }) + const appPtyId = this.toAppPtyId((result as PtySpawnResult).id) + this.ptyLiveness.markLive(appPtyId) return { ...(result as PtySpawnResult), - id: this.toAppPtyId((result as PtySpawnResult).id), + id: appPtyId, ...(opts.sessionId ? { sessionExpired: true } : {}) } } @@ -207,6 +232,7 @@ export class SshPtyProvider implements IPtyProvider { async attach(id: string): Promise { await this.mux.request('pty.attach', { id: this.toRelayPtyId(id) }) + this.ptyLiveness.markLive(id) } async attachForReconnect( @@ -223,6 +249,7 @@ export class SshPtyProvider implements IPtyProvider { ...(expected?.paneKey ? { expectedPaneKey: expected.paneKey } : {}), ...(expected?.tabId ? { expectedTabId: expected.tabId } : {}) })) as { replay?: string } | undefined + this.ptyLiveness.markLive(id) return result ?? {} } @@ -235,11 +262,20 @@ export class SshPtyProvider implements IPtyProvider { } async shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise { - await this.mux.request('pty.shutdown', { - id: this.toRelayPtyId(id), - immediate: opts.immediate ?? false, - keepHistory: opts.keepHistory ?? false - }) + await this.mux.request( + opts.immediate ? 'pty.shutdownSession' : 'pty.shutdown', + { + id: this.toRelayPtyId(id), + immediate: opts.immediate ?? false, + keepHistory: opts.keepHistory ?? false + }, + opts.immediate ? { timeoutMs: SSH_PTY_IMMEDIATE_SHUTDOWN_TIMEOUT_MS } : undefined + ) + // Why: graceful shutdown only acknowledges SIGTERM delivery; the relay + // can remain alive until its exit event or 5s fallback proves death. + if (opts.immediate) { + this.ptyLiveness.markStopped(id) + } } async sendSignal(id: string, signal: string): Promise { @@ -286,11 +322,24 @@ export class SshPtyProvider implements IPtyProvider { } async listProcesses(): Promise { - const result = await this.mux.request('pty.listProcesses') - return (result as PtyProcessInfo[]).map((session) => ({ + return this.requestProcessList() + } + + private async requestProcessList(timeoutMs?: number): Promise { + const result = await this.mux.request( + 'pty.listProcesses', + undefined, + timeoutMs === undefined ? undefined : { timeoutMs } + ) + const sessions = (result as PtyProcessInfo[]).map((session) => ({ ...session, id: this.toAppPtyId(session.id) })) + return sessions + } + + async hasPtyAsync(id: string): Promise { + return this.ptyLiveness.hasPty(id) } async getDefaultShell(): Promise { diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index 66642e38ad1..0c5a6c3bde3 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -156,6 +156,8 @@ export type IPtyProvider = { spawn(opts: PtySpawnOptions): Promise attach(id: string): Promise hasPty?: (id: string) => boolean + /** Why: remote providers need an ID-only probe without building a full session inventory. */ + hasPtyAsync?: (id: string) => Promise write(id: string, data: string): void resize(id: string, cols: number, rows: number): void /** diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 9aa5c00a853..acfc55335ca 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -17307,6 +17307,50 @@ describe('OrcaRuntimeService', () => { }) }) + it('holds runtime terminal admission from resolved workspace through launch preparation', async () => { + const launchPreparation = deferred>() + const spawn = vi.fn().mockResolvedValue({ id: 'runtime-prepared-pty' }) + const spawnWithWorktreePtyAdmissionHeld = vi + .fn() + .mockResolvedValue({ id: 'runtime-prepared-pty' }) + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + spawn, + spawnWithWorktreePtyAdmissionHeld, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + const resolveLaunchOptions = vi + .spyOn( + runtime as unknown as { + resolveAgentTerminalCreateOptions: () => Promise> + }, + 'resolveAgentTerminalCreateOptions' + ) + .mockReturnValue(launchPreparation.promise) + + const creating = runtime.createTerminal(`id:${TEST_WORKTREE_ID}`) + await vi.waitFor(() => expect(resolveLaunchOptions).toHaveBeenCalled()) + + let teardownEntered = false + const teardown = runtime.runWithWorktreePtyTeardown(TEST_WORKTREE_ID, async () => { + teardownEntered = true + }) + await new Promise((resolve) => setImmediate(resolve)) + + expect(teardownEntered).toBe(false) + expect(spawnWithWorktreePtyAdmissionHeld).not.toHaveBeenCalled() + + launchPreparation.resolve({}) + await creating + await teardown + + expect(spawn).not.toHaveBeenCalled() + expect(spawnWithWorktreePtyAdmissionHeld).toHaveBeenCalledTimes(1) + expect(teardownEntered).toBe(true) + }) + it('keeps background-presentation PTY-backed mobile session tabs inactive', async () => { const spawn = vi.fn().mockResolvedValue({ id: 'laptop-created-pty' }) const runtime = new OrcaRuntimeService(store) @@ -17430,6 +17474,64 @@ describe('OrcaRuntimeService', () => { ) }) + it('holds runtime split admission through registration and reveal', async () => { + const splitSpawn = deferred<{ id: string }>() + const splitReveal = deferred<{ tabId: string }>() + const spawnWithWorktreePtyAdmissionHeld = vi + .fn() + .mockResolvedValueOnce({ id: 'runtime-parent-pty' }) + .mockReturnValueOnce(splitSpawn.promise) + const revealTerminalSession = vi + .fn() + .mockResolvedValueOnce({ tabId: 'runtime-parent-tab' }) + .mockReturnValueOnce(splitReveal.promise) + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + spawn: vi.fn(), + spawnWithWorktreePtyAdmissionHeld, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + runtime.setNotifier({ + worktreesChanged: vi.fn(), + reposChanged: vi.fn(), + activateWorktree: vi.fn(), + createTerminal: vi.fn(), + revealTerminalSession, + splitTerminal: vi.fn(), + renameTerminal: vi.fn(), + focusTerminal: vi.fn(), + closeTerminal: vi.fn(), + sleepWorktree: vi.fn(), + terminalFitOverrideChanged: vi.fn(), + terminalDriverChanged: vi.fn() + }) + const parent = await runtime.createTerminal(`id:${TEST_WORKTREE_ID}`, { + tabId: 'runtime-parent-tab', + leafId: HEADLESS_LEAF_ID, + activate: true + }) + + const splitting = runtime.splitTerminal(parent.handle) + await vi.waitFor(() => expect(spawnWithWorktreePtyAdmissionHeld).toHaveBeenCalledTimes(2)) + let teardownEntered = false + const teardown = runtime.runWithWorktreePtyTeardown(TEST_WORKTREE_ID, async () => { + teardownEntered = true + }) + + splitSpawn.resolve({ id: 'runtime-split-pty' }) + await vi.waitFor(() => expect(revealTerminalSession).toHaveBeenCalledTimes(2)) + await new Promise((resolve) => setImmediate(resolve)) + expect(teardownEntered).toBe(false) + + splitReveal.resolve({ tabId: 'runtime-parent-tab' }) + await splitting + await teardown + + expect(teardownEntered).toBe(true) + }) + it('pushes PTY-backed mobile session tab title and agent status changes to subscribers', async () => { const spawn = vi.fn().mockResolvedValue({ id: 'laptop-created-pty' }) const runtime = new OrcaRuntimeService(store) @@ -22795,6 +22897,25 @@ describe('OrcaRuntimeService', () => { expect(killed).toBe(false) }) + it('uses authoritative runtime PTYs for teardown while the renderer graph is reloading', async () => { + const stopAndWait = vi.fn(async (_ptyId: string) => true) + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait, + getForegroundProcess: async () => null + }) + runtime.registerPty('ssh:ssh-1@@reloading', TEST_WORKTREE_ID, 'ssh-1') + runtime.attachWindow(1) + runtime.markRendererReloading(1) + + await expect( + runtime.stopTerminalsForWorktree(TEST_WORKTREE_ID, { worktreeTeardown: true }) + ).resolves.toEqual({ stopped: 1 }) + expect(stopAndWait).toHaveBeenCalledWith('ssh:ssh-1@@reloading') + }) + it('fails terminal listing closed if the graph reloads during selector resolution', async () => { const runtime = new OrcaRuntimeService(store) @@ -22885,6 +23006,275 @@ describe('OrcaRuntimeService', () => { expect(killed).toBe(false) }) + it('waits for verified terminal stops before completing a worktree stop', async () => { + const runtime = new OrcaRuntimeService(store) + let releaseStop = () => {} + const stopAndWait = vi.fn( + () => + new Promise((resolve) => { + releaseStop = () => resolve(true) + }) + ) + const kill = vi.fn(() => true) + runtime.setPtyController({ + write: () => true, + kill, + stopAndWait, + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, 'pty-1') + + let completed = false + const stopping = runtime + .stopTerminalsForWorktree(TEST_WORKTREE_ID, { worktreeTeardown: true }) + .then((result) => { + completed = true + return result + }) + await vi.waitFor(() => expect(stopAndWait).toHaveBeenCalledWith('pty-1')) + + expect(completed).toBe(false) + expect(kill).not.toHaveBeenCalled() + releaseStop() + + await expect(stopping).resolves.toEqual({ stopped: 1 }) + }) + + it('reports exact teardown PTYs whose verified stop failed', async () => { + const runtime = new OrcaRuntimeService(store) + const stopAndWait = vi.fn(async () => false) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait, + getForegroundProcess: async () => null + }) + runtime.registerPty('ssh:ssh-1@@failed', TEST_WORKTREE_ID, 'ssh-1') + + await expect( + runtime.stopTerminalsForWorktree(TEST_WORKTREE_ID, { worktreeTeardown: true }) + ).resolves.toEqual({ stopped: 0, failedPtyIds: ['ssh:ssh-1@@failed'] }) + }) + + it('targets lease-only SSH PTYs during worktree teardown', async () => { + const runtime = new OrcaRuntimeService({ + ...store, + getSshRemotePtyLeases: () => [ + { + targetId: 'ssh-1', + ptyId: 'relay-only', + worktreeId: TEST_WORKTREE_ID, + state: 'detached' as const, + createdAt: 1, + updatedAt: 2 + } + ] + }) + const stopAndWait = vi.fn(async () => false) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait, + getForegroundProcess: async () => null + }) + + await expect( + runtime.stopTerminalsForWorktree(TEST_WORKTREE_ID, { worktreeTeardown: true }) + ).resolves.toEqual({ + stopped: 0, + failedPtyIds: ['ssh:ssh-1@@relay-only'] + }) + expect(stopAndWait).toHaveBeenCalledWith('ssh:ssh-1@@relay-only') + }) + + it('filters runtime PTYs and durable leases to the deleting SSH host', async () => { + const runtime = new OrcaRuntimeService({ + ...store, + getSshRemotePtyLeases: () => [ + { + targetId: 'ssh-a', + ptyId: 'lease-a', + worktreeId: TEST_WORKTREE_ID, + state: 'detached' as const, + createdAt: 1, + updatedAt: 2 + }, + { + targetId: 'ssh-b', + ptyId: 'lease-b', + worktreeId: TEST_WORKTREE_ID, + state: 'detached' as const, + createdAt: 1, + updatedAt: 2 + } + ] + }) + const stopAndWait = vi.fn(async (_ptyId: string) => true) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait, + getForegroundProcess: async () => null + }) + runtime.registerPty('ssh:ssh-a@@runtime-a', TEST_WORKTREE_ID, 'ssh-a') + runtime.registerPty('ssh:ssh-b@@runtime-b', TEST_WORKTREE_ID, 'ssh-b') + + await expect( + runtime.stopTerminalsForWorktree(TEST_WORKTREE_ID, { + worktreeTeardown: true, + connectionId: 'ssh-a' + }) + ).resolves.toEqual({ stopped: 2 }) + expect(stopAndWait.mock.calls.map(([ptyId]) => ptyId).sort()).toEqual([ + 'ssh:ssh-a@@lease-a', + 'ssh:ssh-a@@runtime-a' + ]) + }) + + it('scopes spawn admission independently for identical worktree ids on two SSH hosts', async () => { + const runtime = new OrcaRuntimeService(store) + const releaseHostA = runtime.beginWorktreePtySpawn(TEST_WORKTREE_ID, 'ssh-a') + const hostBOperation = vi.fn(async () => 'host-b-removed') + + await expect( + runtime.runWithWorktreePtyTeardown(TEST_WORKTREE_ID, hostBOperation, 'ssh-b') + ).resolves.toBe('host-b-removed') + expect(hostBOperation).toHaveBeenCalledTimes(1) + + let hostARan = false + const hostATeardown = runtime.runWithWorktreePtyTeardown( + TEST_WORKTREE_ID, + async () => { + hostARan = true + }, + 'ssh-a' + ) + await Promise.resolve() + expect(hostARan).toBe(false) + releaseHostA() + await hostATeardown + expect(hostARan).toBe(true) + }) + + it('does not collide local admission with an SSH connection named local', async () => { + const runtime = new OrcaRuntimeService(store) + const releaseLocal = runtime.beginWorktreePtySpawn(TEST_WORKTREE_ID) + const sshOperation = vi.fn(async () => 'ssh-removed') + + await expect( + runtime.runWithWorktreePtyTeardown(TEST_WORKTREE_ID, sshOperation, 'local') + ).resolves.toBe('ssh-removed') + expect(sshOperation).toHaveBeenCalledTimes(1) + releaseLocal() + }) + + it('bounds concurrent verified worktree stops', async () => { + const runtime = new OrcaRuntimeService(store) + const releases: (() => void)[] = [] + let active = 0 + let peak = 0 + const stopAndWait = vi.fn( + () => + new Promise((resolve) => { + active += 1 + peak = Math.max(peak, active) + releases.push(() => { + active -= 1 + resolve(true) + }) + }) + ) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait, + getForegroundProcess: async () => null + }) + for (let index = 0; index < 20; index += 1) { + runtime.registerPty(`pty-${index}`, TEST_WORKTREE_ID, null) + } + + const stopping = runtime.stopTerminalsForWorktree(TEST_WORKTREE_ID, { + worktreeTeardown: true + }) + await vi.waitFor(() => expect(stopAndWait).toHaveBeenCalledTimes(8)) + while (stopAndWait.mock.calls.length < 20) { + const callsBeforeRelease = stopAndWait.mock.calls.length + releases.splice(0).forEach((release) => release()) + await vi.waitFor(() => + expect(stopAndWait.mock.calls.length).toBeGreaterThan(callsBeforeRelease) + ) + } + releases.splice(0).forEach((release) => release()) + + await expect(stopping).resolves.toEqual({ stopped: 20 }) + expect(peak).toBe(8) + }) + + it('keeps generic worktree terminal stops graceful', async () => { + const runtime = new OrcaRuntimeService(store) + const kill = vi.fn(() => true) + const stopAndWait = vi.fn(async () => true) + runtime.setPtyController({ + write: () => true, + kill, + stopAndWait, + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, 'pty-1') + + await expect(runtime.stopTerminalsForWorktree(TEST_WORKTREE_ID)).resolves.toEqual({ + stopped: 1 + }) + + expect(kill).toHaveBeenCalledWith('pty-1') + expect(stopAndWait).not.toHaveBeenCalled() + }) + + it('joins identical cross-surface worktree removals in the runtime owner', async () => { + const runtime = new OrcaRuntimeService(store) + const removal = deferred<{ warning: string }>() + const operation = vi.fn(() => removal.promise) + const opts = { force: true, runHooks: false, hostId: 'local' } + + const first = runtime.runWithWorktreeRemovalDedup(TEST_WORKTREE_ID, opts, operation) + const second = runtime.runWithWorktreeRemovalDedup(TEST_WORKTREE_ID, opts, vi.fn()) + removal.resolve({ warning: 'joined' }) + + await expect(Promise.all([first, second])).resolves.toEqual([ + { warning: 'joined' }, + { warning: 'joined' } + ]) + expect(operation).toHaveBeenCalledTimes(1) + }) + + it('rejects cross-surface worktree removals with different options', async () => { + const runtime = new OrcaRuntimeService(store) + const removal = deferred>() + const first = runtime.runWithWorktreeRemovalDedup( + TEST_WORKTREE_ID, + { force: false, runHooks: true, hostId: 'local' }, + () => removal.promise + ) + + await expect( + runtime.runWithWorktreeRemovalDedup( + TEST_WORKTREE_ID, + { force: true, runHooks: true, hostId: 'local' }, + vi.fn() + ) + ).rejects.toThrow('Worktree deletion already in progress') + await expect( + runtime.runWithWorktreeRemovalDedup( + TEST_WORKTREE_ID, + { force: false, runHooks: false, hostId: 'local', mode: 'forget' }, + vi.fn() + ) + ).rejects.toThrow('Worktree deletion already in progress') + removal.resolve({}) + await first + }) + it('stops exactly the expected live PTYs for a worktree', async () => { const runtime = new OrcaRuntimeService(store) const stopped: string[] = [] @@ -28360,7 +28750,24 @@ describe('OrcaRuntimeService', () => { const missingWorktreePath = 'C:\\workspace\\already-removed' const worktreeId = `${TEST_REPO_ID}::${missingWorktreePath}` const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(worktreeId) - const runtime = new OrcaRuntimeService(runtimeStore as never) + const callOrder: string[] = [] + const localProvider = { listProcesses: vi.fn().mockResolvedValue([]) } + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getLocalProvider: () => localProvider as never + }) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait: vi.fn(async (ptyId: string) => { + callOrder.push(`stop:${ptyId}`) + return true + }), + getForegroundProcess: async () => null + }) + runtime.registerPty('stale-registration-pty', worktreeId) + removeWorktreeMeta.mockImplementation(() => { + callOrder.push('metadata-removal') + }) const registeredWorktrees = [ { path: TEST_REPO_PATH, @@ -28395,6 +28802,8 @@ describe('OrcaRuntimeService', () => { expect(result).toEqual({ preservedBranch: { branchName: 'feature/foo', head: 'abc' } }) + expect(callOrder).toEqual(['stop:stale-registration-pty', 'metadata-removal']) + expect(localProvider.listProcesses).toHaveBeenCalledTimes(1) expect(runHook).not.toHaveBeenCalled() expect(removeWorktree).not.toHaveBeenCalled() expect(gitSpy).toHaveBeenCalledWith(['worktree', 'prune'], { @@ -28801,7 +29210,24 @@ describe('OrcaRuntimeService', () => { const missingWorktreePath = join(parentDir, 'already-deleted') const worktreeId = `${TEST_REPO_ID}::${missingWorktreePath}` const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(worktreeId) - const runtime = new OrcaRuntimeService(runtimeStore as never) + const callOrder: string[] = [] + const localProvider = { listProcesses: vi.fn().mockResolvedValue([]) } + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getLocalProvider: () => localProvider as never + }) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait: vi.fn(async (ptyId: string) => { + callOrder.push(`stop:${ptyId}`) + return true + }), + getForegroundProcess: async () => null + }) + runtime.registerPty('missing-worktree-pty', worktreeId) + removeWorktreeMeta.mockImplementation(() => { + callOrder.push('metadata-removal') + }) const notifier = { worktreesChanged: vi.fn() } runtime.setNotifier(notifier as never) @@ -28810,6 +29236,8 @@ describe('OrcaRuntimeService', () => { await expect(runtime.removeManagedWorktree(worktreeId)).resolves.toEqual({}) + expect(callOrder).toEqual(['stop:missing-worktree-pty', 'metadata-removal']) + expect(localProvider.listProcesses).toHaveBeenCalledTimes(1) expect(removeWorktree).not.toHaveBeenCalled() expect(removeWorktreeMeta).toHaveBeenCalledWith(worktreeId) expect(deleteWorktreeHistoryDirMock).toHaveBeenCalledWith(worktreeId) @@ -29264,21 +29692,36 @@ describe('OrcaRuntimeService', () => { }) it('falls through to orphan cleanup when preflight reports missing/non-repo worktree', async () => { - const runtime = new OrcaRuntimeService(store) + const callOrder: string[] = [] + const provider = { + listProcesses: vi + .fn() + .mockResolvedValue([ + { id: `${TEST_WORKTREE_ID}@@orphan`, cwd: TEST_WORKTREE_PATH, title: 'shell' } + ]), + shutdown: vi.fn(async () => { + callOrder.push('pty-shutdown') + }) + } + const runtime = new OrcaRuntimeService(store, undefined, { + getLocalProvider: () => provider as never + }) vi.mocked(getEffectiveHooks).mockReturnValue(null) vi.mocked(assertWorktreeCleanForRemoval).mockRejectedValue( Object.assign(new Error('status failed'), { stderr: 'fatal: not a git repository (or any of the parent directories): .git\n' }) ) - vi.mocked(removeWorktree).mockRejectedValue( - Object.assign(new Error('git worktree remove failed'), { + vi.mocked(removeWorktree).mockImplementation(async () => { + callOrder.push('git-remove') + throw Object.assign(new Error('git worktree remove failed'), { stderr: `fatal: '${TEST_WORKTREE_PATH}' is not a working tree` }) - ) + }) vi.spyOn(gitRunner, 'gitExecFileAsync').mockResolvedValue({ stdout: '', stderr: '' }) await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID)).resolves.toEqual({}) + expect(callOrder).toEqual(['pty-shutdown', 'git-remove']) expect(removeWorktree).toHaveBeenCalledWith( TEST_REPO_PATH, TEST_WORKTREE_PATH, @@ -29950,6 +30393,48 @@ describe('OrcaRuntimeService', () => { expect(gitIdx).toBeGreaterThan(killIdx) }) + it('does not invoke Git removal when local provider teardown remains unverified', async () => { + const ptyId = `${TEST_WORKTREE_ID}@@daemon` + const localProvider = createProviderStub(async () => [ + { id: ptyId, cwd: '/tmp/worktree-a', title: 'shell' } + ]) + localProvider.shutdown.mockRejectedValue(new Error('daemon still alive')) + const runtime = new OrcaRuntimeService(store, undefined, { + getLocalProvider: () => localProvider as never + }) + + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID)).rejects.toThrow( + `Failed to stop local worktree terminals: ${ptyId}` + ) + + expect(removeWorktree).not.toHaveBeenCalled() + }) + + it('keeps spawn admission closed through post-Git metadata cleanup', async () => { + const localProvider = createProviderStub(async () => []) + const runtime = new OrcaRuntimeService(store, undefined, { + getLocalProvider: () => localProvider as never + }) + const originalRemoveWorktreeMeta = store.removeWorktreeMeta + let rejectedDuringMetadataCleanup = false + store.removeWorktreeMeta = () => { + expect(() => runtime.beginWorktreePtySpawn(TEST_WORKTREE_ID)).toThrow( + 'Worktree teardown is in progress' + ) + rejectedDuringMetadataCleanup = true + } + + try { + await runtime.removeManagedWorktree(TEST_WORKTREE_ID) + const release = runtime.beginWorktreePtySpawn(TEST_WORKTREE_ID) + release() + + expect(rejectedDuringMetadataCleanup).toBe(true) + } finally { + store.removeWorktreeMeta = originalRemoveWorktreeMeta + } + }) + it('thunk resolves the installed provider lazily, not at construction time', async () => { // Simulates the daemon adapter being installed AFTER OrcaRuntimeService // construction (setLocalPtyProvider(routedAdapter) in daemon-init). @@ -29984,6 +30469,260 @@ describe('OrcaRuntimeService', () => { // The pre-daemon provider must not have been consulted for the kill. expect(preDaemonProvider.shutdown).not.toHaveBeenCalled() }) + + it('waits for delayed daemon startup before CLI removal captures the provider', async () => { + const startup = deferred() + const fallbackProvider = createProviderStub(async () => []) + const daemonPtyId = `${TEST_WORKTREE_ID}@@daemon-startup` + const daemonProvider = createProviderStub(async () => [ + { id: daemonPtyId, cwd: TEST_WORKTREE_PATH, title: 'shell' } + ]) + let currentProvider = fallbackProvider + const runtime = new OrcaRuntimeService(store, undefined, { + getLocalProvider: () => currentProvider as never, + awaitLocalPtyStartup: () => startup.promise + }) + + const removal = runtime.removeManagedWorktree(TEST_WORKTREE_ID) + await Promise.resolve() + expect(removeWorktree).not.toHaveBeenCalled() + expect(fallbackProvider.listProcesses).not.toHaveBeenCalled() + + currentProvider = daemonProvider + startup.resolve() + await removal + + expect(daemonProvider.shutdown).toHaveBeenCalledWith(daemonPtyId, { immediate: true }) + expect(fallbackProvider.shutdown).not.toHaveBeenCalled() + expect(removeWorktree).toHaveBeenCalledTimes(1) + }) + + it('drains headless and late runtime-backed SSH spawns before remote removal', async () => { + const remoteRepo = { + ...store.getRepo(TEST_REPO_ID)!, + path: '/remote/repo', + connectionId: 'ssh-1', + executionHostId: 'runtime:runtime-a' as const + } + const remoteWorktree = { + path: '/remote/feature-wt', + head: 'def456', + branch: 'feature/test', + isBare: false, + isMainWorktree: false + } + const remoteWorktreeId = `${remoteRepo.id}::${remoteWorktree.path}` + let runtime: OrcaRuntimeService + let rejectedDuringRemoteMetadataCleanup = false + const runtimeStore = { + ...store, + getRepos: () => [remoteRepo], + getRepo: (id: string) => (id === remoteRepo.id ? remoteRepo : undefined), + getAllWorktreeMeta: () => ({ [remoteWorktreeId]: makeWorktreeMeta() }), + getWorktreeMeta: (id: string) => (id === remoteWorktreeId ? makeWorktreeMeta() : undefined), + removeWorktreeMeta: () => { + expect(() => runtime.beginWorktreePtySpawn(remoteWorktreeId, 'ssh-1')).toThrow( + 'Worktree teardown is in progress' + ) + rejectedDuringRemoteMetadataCleanup = true + } + } + const callOrder: string[] = [] + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: remoteRepo.path, + head: 'main', + branch: 'main', + isBare: false, + isMainWorktree: true + }, + remoteWorktree + ]), + removeWorktree: vi.fn(async () => { + callOrder.push('git-remove') + return {} + }) + } + const localProvider = createProviderStub(async () => []) + registerSshGitProvider('ssh-1', gitProvider as never) + runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getLocalProvider: () => localProvider as never + }) + let releaseFirstStop = () => {} + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait: vi.fn( + (ptyId: string) => + new Promise((resolve) => { + callOrder.push(`stop:${ptyId}`) + if (ptyId === 'ssh:ssh-1@@existing') { + releaseFirstStop = () => resolve(true) + } else { + resolve(true) + } + }) + ), + getForegroundProcess: async () => null + }) + runtime.registerPty('ssh:ssh-1@@existing', remoteWorktreeId, 'ssh-1') + const releaseLateSpawn = runtime.beginWorktreePtySpawn(remoteWorktreeId, 'ssh-1') + + try { + const removal = runtime.removeManagedWorktree(remoteWorktreeId) + await vi.waitFor(() => expect(gitProvider.listWorktrees).toHaveBeenCalled()) + expect(callOrder).toEqual([]) + + runtime.registerPty('ssh:ssh-1@@late', remoteWorktreeId, 'ssh-1') + releaseLateSpawn() + await vi.waitFor(() => expect(callOrder).toContain('stop:ssh:ssh-1@@existing')) + expect(callOrder).toContain('stop:ssh:ssh-1@@late') + expect(callOrder).not.toContain('git-remove') + + releaseFirstStop() + await removal + const releasePostRemovalSpawn = runtime.beginWorktreePtySpawn(remoteWorktreeId, 'ssh-1') + releasePostRemovalSpawn() + + expect(callOrder).toEqual([ + 'stop:ssh:ssh-1@@existing', + 'stop:ssh:ssh-1@@late', + 'git-remove' + ]) + expect(rejectedDuringRemoteMetadataCleanup).toBe(true) + } finally { + unregisterSshGitProvider('ssh-1') + } + }) + + it('does not remove a remote worktree when its SSH PTY stop is unverified', async () => { + const remoteRepo = { + ...store.getRepo(TEST_REPO_ID)!, + path: '/remote/repo', + connectionId: 'ssh-1' + } + const remoteWorktree = { + path: '/remote/feature-wt', + head: 'def456', + branch: 'feature/test', + isBare: false, + isMainWorktree: false + } + const remoteWorktreeId = `${remoteRepo.id}::${remoteWorktree.path}` + const runtimeStore = { + ...store, + getRepos: () => [remoteRepo], + getRepo: (id: string) => (id === remoteRepo.id ? remoteRepo : undefined), + getAllWorktreeMeta: () => ({ [remoteWorktreeId]: makeWorktreeMeta() }), + getWorktreeMeta: (id: string) => (id === remoteWorktreeId ? makeWorktreeMeta() : undefined) + } + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: remoteRepo.path, + head: 'main', + branch: 'main', + isBare: false, + isMainWorktree: true + }, + remoteWorktree + ]), + removeWorktree: vi.fn() + } + const localProvider = createProviderStub(async () => []) + registerSshGitProvider('ssh-1', gitProvider as never) + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getLocalProvider: () => localProvider as never + }) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait: vi.fn(async () => false), + getForegroundProcess: async () => null + }) + runtime.registerPty('ssh:ssh-1@@survivor', remoteWorktreeId, 'ssh-1') + + try { + await expect(runtime.removeManagedWorktree(remoteWorktreeId)).rejects.toThrow( + 'Failed to stop remote worktree terminals: ssh:ssh-1@@survivor' + ) + expect(gitProvider.removeWorktree).not.toHaveBeenCalled() + } finally { + unregisterSshGitProvider('ssh-1') + } + }) + + it('preserves remote metadata when a lease-only PTY provider is disconnected', async () => { + const remoteRepo = { + ...store.getRepo(TEST_REPO_ID)!, + path: '/remote/repo', + connectionId: 'ssh-1' + } + const remoteWorktree = { + path: '/remote/feature-wt', + head: 'def456', + branch: 'feature/test', + isBare: false, + isMainWorktree: false + } + const remoteWorktreeId = `${remoteRepo.id}::${remoteWorktree.path}` + const removeWorktreeMeta = vi.fn() + const runtimeStore = { + ...store, + getRepos: () => [remoteRepo], + getRepo: (id: string) => (id === remoteRepo.id ? remoteRepo : undefined), + getAllWorktreeMeta: () => ({ [remoteWorktreeId]: makeWorktreeMeta() }), + getWorktreeMeta: (id: string) => (id === remoteWorktreeId ? makeWorktreeMeta() : undefined), + removeWorktreeMeta, + getSshRemotePtyLeases: () => [ + { + targetId: 'ssh-1', + ptyId: 'detached-survivor', + worktreeId: remoteWorktreeId, + state: 'detached' as const, + createdAt: 1, + updatedAt: 2 + } + ] + } + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: remoteRepo.path, + head: 'main', + branch: 'main', + isBare: false, + isMainWorktree: true + }, + remoteWorktree + ]), + removeWorktree: vi.fn() + } + const localProvider = createProviderStub(async () => []) + registerSshGitProvider('ssh-1', gitProvider as never) + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getLocalProvider: () => localProvider as never + }) + const stopAndWait = vi.fn(async () => false) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait, + getForegroundProcess: async () => null + }) + + try { + await expect(runtime.removeManagedWorktree(remoteWorktreeId)).rejects.toThrow( + 'Failed to stop remote worktree terminals: ssh:ssh-1@@detached-survivor' + ) + expect(stopAndWait).toHaveBeenCalledWith('ssh:ssh-1@@detached-survivor') + expect(gitProvider.removeWorktree).not.toHaveBeenCalled() + expect(removeWorktreeMeta).not.toHaveBeenCalled() + } finally { + unregisterSshGitProvider('ssh-1') + } + }) }) describe('stale terminal handle resolution (#7718)', () => { function syncSingleTerminalGraph(runtime: OrcaRuntimeService, ptyId: string): void { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 293075176e9..b56501dbc6d 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -204,7 +204,7 @@ import { import { TASK_PROVIDERS } from '../../shared/task-providers' import { FIRST_PANE_ID } from '../../shared/pane-key' import { isTerminalLeafId, makePaneKey, parsePaneKey } from '../../shared/stable-pane-id' -import { parseAppSshPtyId } from '../../shared/ssh-pty-id' +import { parseAppSshPtyId, toAppSshPtyId } from '../../shared/ssh-pty-id' import { isValidHostTerminalTabId, isValidTerminalTabId } from '../../shared/terminal-tab-id' import { buildAgentDraftLaunchPlan, buildAgentStartupPlan } from '../../shared/tui-agent-startup' import { repoIsRemote } from '../../shared/agent-launch-remote' @@ -719,6 +719,8 @@ import { registerTerminalViewAttributesApplier } from './terminal-view-attribute-store' import { killAllProcessesForWorktree } from './worktree-teardown' +import { mapPtyStopsWithConcurrency } from './pty-stop-concurrency' +import { WorktreePtyAdmission } from './worktree-pty-admission' import { MOBILE_SUBSCRIBE_SCROLLBACK_ROWS } from './scrollback-limits' import { createMobileSessionTabsNotifyCoalescer, @@ -825,6 +827,7 @@ type RuntimeStore = { getWorkspaceSession?: Store['getWorkspaceSession'] setWorkspaceSession?: Store['setWorkspaceSession'] persistPtyBinding?: Store['persistPtyBinding'] + getSshRemotePtyLeases?: Store['getSshRemotePtyLeases'] getUI?: Store['getUI'] updateUI?: Store['updateUI'] recordFeatureInteraction?: Store['recordFeatureInteraction'] @@ -1171,25 +1174,28 @@ type HeadlessSeedMetadata = { kittyKeyboardFlags?: number } -type RuntimePtyController = { - spawn?(opts: { - cols: number - rows: number - cwd?: string - command?: string - commandDelivery?: 'renderer' | 'provider' - startupCommandDelivery?: WorktreeStartupLaunch['startupCommandDelivery'] - env?: Record - envToDelete?: string[] - telemetry?: WorktreeStartupLaunch['telemetry'] - connectionId?: string | null - worktreeId?: string - preAllocatedHandle?: string - tabId?: string - leafId?: string - sessionId?: string - persistHostSessionBinding?: boolean - }): Promise<{ id: string }> +export type RuntimePtySpawnOptions = { + cols: number + rows: number + cwd?: string + command?: string + commandDelivery?: 'renderer' | 'provider' + startupCommandDelivery?: WorktreeStartupLaunch['startupCommandDelivery'] + env?: Record + envToDelete?: string[] + telemetry?: WorktreeStartupLaunch['telemetry'] + connectionId?: string | null + worktreeId?: string + preAllocatedHandle?: string + tabId?: string + leafId?: string + sessionId?: string + persistHostSessionBinding?: boolean +} + +export type RuntimePtyController = { + spawn?(opts: RuntimePtySpawnOptions): Promise<{ id: string }> + spawnWithWorktreePtyAdmissionHeld?(opts: RuntimePtySpawnOptions): Promise<{ id: string }> write(ptyId: string, data: string): boolean kill(ptyId: string): boolean stopAndWait?(ptyId: string, opts?: { keepHistory?: boolean }): Promise @@ -2486,8 +2492,11 @@ export class OrcaRuntimeService { private canonicalFetchKeyCache = new Map() private optimisticReconcileTokens = new Map() private removeManagedWorktreeInFlight = new Map() + private crossSurfaceWorktreeRemovalInFlight = new Map() + private readonly worktreePtyAdmission = new WorktreePtyAdmission() private preservedBranchCleanupByWorktreeId = new Map() private readonly getLocalProviderFn: (() => IPtyProvider) | null + private readonly awaitLocalPtyStartup: (() => Promise) | null private readonly onPtyStopped: ((ptyId: string) => void) | null private readonly onTerminalAgentStatus: ((event: RuntimeTerminalAgentStatusEvent) => void) | null private readonly onTerminalSideEffects: ((batch: TerminalSideEffectBatch) => void) | null @@ -2513,6 +2522,7 @@ export class OrcaRuntimeService { stats?: StatsCollector, deps?: { getLocalProvider?: () => IPtyProvider + awaitLocalPtyStartup?: () => Promise onPtyStopped?: (ptyId: string) => void onTerminalAgentStatus?: (event: RuntimeTerminalAgentStatusEvent) => void onTerminalSideEffects?: (batch: TerminalSideEffectBatch) => void @@ -2549,6 +2559,7 @@ export class OrcaRuntimeService { // lazily via thunk so teardown always sees the currently-installed // provider (design §4.3 wire-up). this.getLocalProviderFn = deps?.getLocalProvider ?? null + this.awaitLocalPtyStartup = deps?.awaitLocalPtyStartup ?? null this.onPtyStopped = deps?.onPtyStopped ?? null this.onTerminalAgentStatus = deps?.onTerminalAgentStatus ?? null this.buildAgentHookPtyEnv = deps?.buildAgentHookPtyEnv ?? null @@ -16951,8 +16962,45 @@ export class OrcaRuntimeService { if (!this.store) { throw new Error('runtime_unavailable') } - const store = this.store + const exactTarget = parseExactWorktreeIdSelector(worktreeSelector) + if (exactTarget) { + const repo = this.store.getRepo(exactTarget.repoId) + return this.runWithWorktreeRemovalDedup( + exactTarget.id, + { + force, + runHooks, + ...(repo ? { hostId: getRepoExecutionHostId(repo) } : {}) + }, + async () => { + const resolvedTarget = await this.resolveWorktreeRemovalTarget(worktreeSelector) + return this.removeManagedWorktreeCore(force, runHooks, resolvedTarget) + } + ) + } const removalTarget = await this.resolveWorktreeRemovalTarget(worktreeSelector) + const repo = this.store.getRepo(removalTarget.repoId) + return this.runWithWorktreeRemovalDedup( + removalTarget.id, + { + force, + runHooks, + ...(repo ? { hostId: getRepoExecutionHostId(repo) } : {}) + }, + () => this.removeManagedWorktreeCore(force, runHooks, removalTarget) + ) + } + + private async removeManagedWorktreeCore( + force: boolean, + runHooks: boolean, + resolvedRemovalTarget: RuntimeWorktreeRemovalTarget + ): Promise { + if (!this.store) { + throw new Error('runtime_unavailable') + } + const store = this.store + const removalTarget = resolvedRemovalTarget const optionsKey = getRuntimeWorktreeRemovalOptionsKey(force, runHooks) const inFlightRemoval = this.removeManagedWorktreeInFlight.get(removalTarget.id) if (inFlightRemoval) { @@ -16975,23 +17023,28 @@ export class OrcaRuntimeService { 'Cannot delete the project root workspace. Remove the folder project instead.' ) } - const localProvider = this.getLocalProvider() - if (localProvider) { - // Why: folder workspace deletion has no Git removal phase where PTYs - // would otherwise be swept; tear them down before hiding the workspace. - await killAllProcessesForWorktree(removalTarget.id, { - runtime: this, - localProvider, - onPtyStopped: this.onPtyStopped ?? undefined - }).catch((err) => { - console.warn(`[worktree-teardown] failed for ${removalTarget.id}:`, err) - }) - } - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) - this.invalidateResolvedWorktreeCache() - this.notifyWorktreesChanged(repo.id) - return {} + return this.runWithWorktreePtyTeardown( + removalTarget.id, + async () => { + const localProvider = this.getLocalProvider() + if (localProvider) { + // Why: folder workspace deletion has no Git removal phase where PTYs + // would otherwise be swept; tear them down before hiding the workspace. + await killAllProcessesForWorktree(removalTarget.id, { + runtime: this, + localProvider, + connectionId: repo.connectionId ?? null, + onPtyStopped: this.onPtyStopped ?? undefined + }) + } + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) + this.invalidateResolvedWorktreeCache() + this.notifyWorktreesChanged(repo.id) + return {} + }, + repo.connectionId ?? null + ) } const provider = repo.connectionId ? requireSshGitProvider(repo.connectionId) : null const fsProvider = repo.connectionId ? getSshFilesystemProvider(repo.connectionId) : null @@ -17048,32 +17101,38 @@ export class OrcaRuntimeService { if (!force) { throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) } - if (repo.connectionId) { - await fsProvider!.deletePath(removalTarget.path, true) - await cleanupUnusedWorktreePushTargetRemoteSsh( - provider!, - repo.path, - removalTarget.id, - removedPushTarget, - store - ) - } else { - await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - removalTarget.id, - removedPushTarget, - store, - localWorktreeGitOptions - ) - } - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - return {} + return this.runWithSuccessfulCleanupTeardown( + removalTarget.id, + repo.connectionId ?? null, + async () => { + if (repo.connectionId) { + await fsProvider!.deletePath(removalTarget.path, true) + await cleanupUnusedWorktreePushTargetRemoteSsh( + provider!, + repo.path, + removalTarget.id, + removedPushTarget, + store + ) + } else { + await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + removalTarget.id, + removedPushTarget, + store, + localWorktreeGitOptions + ) + } + this.clearOptimisticReconcileToken(removalTarget.id) + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyWorktreesChanged(repo.id) + return {} + } + ) } if (!repo.connectionId) { const access = getLocalWorktreePathAccess(localWorktreeGitOptions) @@ -17096,24 +17155,30 @@ export class OrcaRuntimeService { if (!force) { throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) } - await closeLocalWatcherForWorktreePath(removalTarget.path).catch((err) => { - console.warn(`[filesystem-watcher] failed to close ${removalTarget.path}:`, err) - }) - await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, + return this.runWithSuccessfulCleanupTeardown( removalTarget.id, - removedPushTarget, - store, - localWorktreeGitOptions + repo.connectionId ?? null, + async () => { + await closeLocalWatcherForWorktreePath(removalTarget.path).catch((err) => { + console.warn(`[filesystem-watcher] failed to close ${removalTarget.path}:`, err) + }) + await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + removalTarget.id, + removedPushTarget, + store, + localWorktreeGitOptions + ) + this.clearOptimisticReconcileToken(removalTarget.id) + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyWorktreesChanged(repo.id) + return {} + } ) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - return {} } } if (await isRuntimeWorktreePathMissing(repo, removalTarget.path, localWorktreeGitOptions)) { @@ -17125,28 +17190,34 @@ export class OrcaRuntimeService { // Why: a manually deleted worktree is already gone from Git and disk. // Finish runtime metadata cleanup without requiring force or touching // any unregistered path that still exists. - await (repo.connectionId - ? cleanupUnusedWorktreePushTargetRemoteSsh( - provider!, - repo.path, - removalTarget.id, - removedPushTarget, - store - ) - : cleanupUnusedWorktreePushTargetRemote( - repo.path, - removalTarget.id, - removedPushTarget, - store, - localWorktreeGitOptions - )) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - return {} + return this.runWithSuccessfulCleanupTeardown( + removalTarget.id, + repo.connectionId ?? null, + async () => { + await (repo.connectionId + ? cleanupUnusedWorktreePushTargetRemoteSsh( + provider!, + repo.path, + removalTarget.id, + removedPushTarget, + store + ) + : cleanupUnusedWorktreePushTargetRemote( + repo.path, + removalTarget.id, + removedPushTarget, + store, + localWorktreeGitOptions + )) + this.clearOptimisticReconcileToken(removalTarget.id) + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyWorktreesChanged(repo.id) + return {} + } + ) } throw new Error(`Refusing to delete unregistered worktree path: ${removalTarget.path}`) } @@ -17171,61 +17242,82 @@ export class OrcaRuntimeService { removedMeta && (await isRuntimeWorktreePathMissing(repo, canonicalWorktreePath, localWorktreeGitOptions)) ) { - const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ - canonicalWorktreePath, - repoPath: repo.path, - localWorktreeGitOptions, - registeredWorktree, - deleteBranch - }) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, + return this.runWithSuccessfulCleanupTeardown( removalTarget.id, - removedPushTarget, - store, - localWorktreeGitOptions - ) - this.rememberPreservedBranchCleanupTarget( - removalTarget.id, - removalResult, - registeredWorktree.head, - removedPushTarget + repo.connectionId ?? null, + async () => { + const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + canonicalWorktreePath, + repoPath: repo.path, + localWorktreeGitOptions, + registeredWorktree, + deleteBranch + }) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + removalTarget.id, + removedPushTarget, + store, + localWorktreeGitOptions + ) + this.rememberPreservedBranchCleanupTarget( + removalTarget.id, + removalResult, + registeredWorktree.head, + removedPushTarget + ) + this.clearOptimisticReconcileToken(removalTarget.id) + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyWorktreesChanged(repo.id) + return removalResult ?? {} + } ) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - return removalResult ?? {} } if (repo.connectionId) { const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} - const rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 - ? provider!.removeWorktree(canonicalWorktreePath, force, remoteRemoveOptions) - : provider!.removeWorktree(canonicalWorktreePath, force)) - const removalResult = this.preserveBranchHeadFallback( - rawRemovalResult, - registeredWorktree.head - ) - await cleanupUnusedWorktreePushTargetRemoteSsh( - provider!, - repo.path, + return this.runWithWorktreePtyTeardown( removalTarget.id, - removedPushTarget, - store - ) - this.rememberPreservedBranchCleanupTarget( - removalTarget.id, - removalResult, - registeredWorktree.head, - removedPushTarget + async () => { + const localProvider = this.getLocalProvider() + if (localProvider) { + await killAllProcessesForWorktree(removalTarget.id, { + runtime: this, + localProvider, + connectionId: repo.connectionId ?? null, + onPtyStopped: this.onPtyStopped ?? undefined + }) + } + const rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 + ? provider!.removeWorktree(canonicalWorktreePath, force, remoteRemoveOptions) + : provider!.removeWorktree(canonicalWorktreePath, force)) + const removalResult = this.preserveBranchHeadFallback( + rawRemovalResult, + registeredWorktree.head + ) + await cleanupUnusedWorktreePushTargetRemoteSsh( + provider!, + repo.path, + removalTarget.id, + removedPushTarget, + store + ) + this.rememberPreservedBranchCleanupTarget( + removalTarget.id, + removalResult, + registeredWorktree.head, + removedPushTarget + ) + this.clearOptimisticReconcileToken(removalTarget.id) + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyWorktreesChanged(repo.id) + return removalResult ?? {} + }, + repo.connectionId ?? null ) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - return removalResult ?? {} } const hooks = getEffectiveHooks(repo) @@ -17268,7 +17360,6 @@ export class OrcaRuntimeService { throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) } - let shouldTearDownPtys = true if (repo.symlinkPaths && repo.symlinkPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalWorktreePath, repo.symlinkPaths) } @@ -17280,25 +17371,29 @@ export class OrcaRuntimeService { if (!isOrphanCompatiblePreflightError(error)) { throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) } - // Why: orphan cleanup does not need live shells to be killed first, - // and preflight did not prove the worktree is cleanly removable. - shouldTearDownPtys = false + // Why: missing/non-repo paths may still have a live deleted-cwd PTY; + // continue to the fenced teardown before orphan recovery mutates state. } - const localProvider = this.getLocalProvider() - await closeLocalWatcherForWorktreePath(canonicalWorktreePath).catch((err) => { - console.warn(`[filesystem-watcher] failed to close ${canonicalWorktreePath}:`, err) - }) - if (localProvider && shouldTearDownPtys) { - // Why: once preflight proves normal deletion is clean, kill PTYs before - // git-level removal so Windows handles cannot keep the directory busy. This also - // closes the headless-CLI leak for confirmed-removable worktrees. - await killAllProcessesForWorktree(removalTarget.id, { - runtime: this, - localProvider, - onPtyStopped: this.onPtyStopped ?? undefined + const releaseWorktreePtyTeardown = await this.beginWorktreePtyTeardown( + removalTarget.id, + repo.connectionId ?? null + ) + let removalResult: RemoveWorktreeResult | undefined + try { + const localProvider = this.getLocalProvider() + await closeLocalWatcherForWorktreePath(canonicalWorktreePath).catch((err) => { + console.warn(`[filesystem-watcher] failed to close ${canonicalWorktreePath}:`, err) }) - .then((r) => { + if (localProvider) { + // Why: kill PTYs before git-level removal so Windows handles cannot + // keep the directory busy and deleted-cwd shells cannot survive recovery. + await killAllProcessesForWorktree(removalTarget.id, { + runtime: this, + localProvider, + connectionId: repo.connectionId ?? null, + onPtyStopped: this.onPtyStopped ?? undefined + }).then((r) => { const total = r.runtimeStopped + r.providerStopped + r.registryStopped if (total > 0) { // Why (design §4.4 observability): breadcrumb lets ops @@ -17311,113 +17406,111 @@ export class OrcaRuntimeService { ) } }) - .catch((err) => { - console.warn(`[worktree-teardown] failed for ${removalTarget.id}:`, err) - }) - } - - let removalResult: RemoveWorktreeResult | undefined - try { - const removeOptions = { - ...(!deleteBranch ? { deleteBranch } : {}), - // Why: removal already validated the Git row under the selected - // project runtime; keep branch cleanup on that same canonical row. - knownRemovedWorktree: refreshedRegisteredWorktree, - ...localWorktreeGitOptions } - removalResult = this.preserveBranchHeadFallback( - await removeWorktree(repo.path, canonicalWorktreePath, force, removeOptions), - refreshedRegisteredWorktree.head - ) - } catch (error) { - // Why: Git for Windows can deregister a clean worktree before its - // recursive filesystem deletion fails transiently. - const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ - error, - force, - canonicalWorktreePath, - repoPath: repo.path, - localWorktreeGitOptions, - registeredWorktree: refreshedRegisteredWorktree, - deleteBranch, - closeWatcher: (worktreePath) => - closeLocalWatcherForWorktreePath(worktreePath).catch((err) => { - console.warn(`[filesystem-watcher] failed to close ${worktreePath}:`, err) - }) - }) - if (recoveredRemovalResult) { - removalResult = recoveredRemovalResult - } else if (isOrphanedWorktreeError(error)) { - const access = getLocalWorktreePathAccess(localWorktreeGitOptions) - if ( - await canSafelyRemoveOrphanedWorktreeDirectory( - toLocalWorktreeRuntimePath(canonicalWorktreePath, localWorktreeGitOptions), - toLocalWorktreeRuntimePath(repo.path, localWorktreeGitOptions), - access.statPath, - access.readPath - ) - ) { - await closeLocalWatcherForWorktreePath(canonicalWorktreePath).catch((err) => { - console.warn(`[filesystem-watcher] failed to close ${canonicalWorktreePath}:`, err) - }) - await removeLocalWorktreePath(canonicalWorktreePath, localWorktreeGitOptions).catch( - () => {} - ) - } else { - console.warn( - `[worktrees] Refusing recursive cleanup for unproven worktree directory: ${canonicalWorktreePath}` - ) - } - // Why: `git worktree remove` failed, so git's internal worktree tracking - // (`.git/worktrees/`) is still intact. Without pruning, `git worktree - // list` continues to show the stale entry and the branch it had checked out - // remains locked — other worktrees cannot check it out. - await gitExecFileAsync(['worktree', 'prune'], { - cwd: repo.path, + + try { + const removeOptions = { + ...(!deleteBranch ? { deleteBranch } : {}), + // Why: removal already validated the Git row under the selected + // project runtime; keep branch cleanup on that same canonical row. + knownRemovedWorktree: refreshedRegisteredWorktree, ...localWorktreeGitOptions - }).catch(() => {}) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - removalTarget.id, - removedPushTarget, - store, - localWorktreeGitOptions + } + removalResult = this.preserveBranchHeadFallback( + await removeWorktree(repo.path, canonicalWorktreePath, force, removeOptions), + refreshedRegisteredWorktree.head ) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - return { - ...(warning ? { warning } : {}) + } catch (error) { + // Why: Git for Windows can deregister a clean worktree before its + // recursive filesystem deletion fails transiently. + const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ + error, + force, + canonicalWorktreePath, + repoPath: repo.path, + localWorktreeGitOptions, + registeredWorktree: refreshedRegisteredWorktree, + deleteBranch, + closeWatcher: (worktreePath) => + closeLocalWatcherForWorktreePath(worktreePath).catch((err) => { + console.warn(`[filesystem-watcher] failed to close ${worktreePath}:`, err) + }) + }) + if (recoveredRemovalResult) { + removalResult = recoveredRemovalResult + } else if (isOrphanedWorktreeError(error)) { + const access = getLocalWorktreePathAccess(localWorktreeGitOptions) + if ( + await canSafelyRemoveOrphanedWorktreeDirectory( + toLocalWorktreeRuntimePath(canonicalWorktreePath, localWorktreeGitOptions), + toLocalWorktreeRuntimePath(repo.path, localWorktreeGitOptions), + access.statPath, + access.readPath + ) + ) { + await closeLocalWatcherForWorktreePath(canonicalWorktreePath).catch((err) => { + console.warn(`[filesystem-watcher] failed to close ${canonicalWorktreePath}:`, err) + }) + await removeLocalWorktreePath(canonicalWorktreePath, localWorktreeGitOptions).catch( + () => {} + ) + } else { + console.warn( + `[worktrees] Refusing recursive cleanup for unproven worktree directory: ${canonicalWorktreePath}` + ) + } + // Why: `git worktree remove` failed, so git's internal worktree tracking + // (`.git/worktrees/`) is still intact. Without pruning, `git worktree + // list` continues to show the stale entry and the branch it had checked out + // remains locked — other worktrees cannot check it out. + await gitExecFileAsync(['worktree', 'prune'], { + cwd: repo.path, + ...localWorktreeGitOptions + }).catch(() => {}) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + removalTarget.id, + removedPushTarget, + store, + localWorktreeGitOptions + ) + this.clearOptimisticReconcileToken(removalTarget.id) + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.preservedBranchCleanupByWorktreeId.delete(removalTarget.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyWorktreesChanged(repo.id) + return { + ...(warning ? { warning } : {}) + } + } else { + throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) } - } else { - throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) } - } - - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - removalTarget.id, - removedPushTarget, - store, - localWorktreeGitOptions - ) - this.rememberPreservedBranchCleanupTarget( - removalTarget.id, - removalResult, - refreshedRegisteredWorktree.head, - removedPushTarget - ) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - return { - ...removalResult, - ...(warning ? { warning } : {}) + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + removalTarget.id, + removedPushTarget, + store, + localWorktreeGitOptions + ) + this.rememberPreservedBranchCleanupTarget( + removalTarget.id, + removalResult, + refreshedRegisteredWorktree.head, + removedPushTarget + ) + this.clearOptimisticReconcileToken(removalTarget.id) + this.removeWorktreeMetadataAndHistory(store, removalTarget.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyWorktreesChanged(repo.id) + return { + ...removalResult, + ...(warning ? { warning } : {}) + } + } finally { + releaseWorktreePtyTeardown() } })() this.removeManagedWorktreeInFlight.set(removalTarget.id, { optionsKey, promise: removal }) @@ -17550,187 +17643,191 @@ export class OrcaRuntimeService { throw new Error('runtime_unavailable') } const workspace = await this.resolveTerminalWorkspaceLaunchScope(worktreeSelector) - const launchOpts = await this.resolveAgentTerminalCreateOptions(workspace, opts) - const cwd = - this.resolveWorkspaceTerminalStartupCwd(workspace, launchOpts.cwd) ?? workspace.path - const preAllocatedHandle = this.createPreAllocatedTerminalHandle() - // Why: mint tabId in main before spawn so paneKey is known at PTY env - // build time. Hook-based agent status (Claude/Codex/Cursor/Gemini) keys - // off `${tabId}:${leafId}` — without these vars set on the PTY, the - // hook payload arrives with an empty paneKey and the renderer cannot - // attribute the event. Use a stable UUID leaf because hooks reject the - // legacy numeric pane keys after the pane-id migration. - const hintedTabId = launchOpts.tabId?.trim() - const canAdoptPaneIdentity = - hintedTabId !== undefined && - isValidHostTerminalTabId(hintedTabId) && - launchOpts.leafId !== undefined && - isTerminalLeafId(launchOpts.leafId) - const tabId = canAdoptPaneIdentity ? (hintedTabId as string) : randomUUID() - const leafId = canAdoptPaneIdentity ? (launchOpts.leafId as string) : randomUUID() - const paneKey = makePaneKey(tabId, leafId) - const launchToken = launchOpts.launchConfig - ? (launchOpts.launchToken ?? randomUUID()) - : undefined - const baseEnv = { - ...launchOpts.env, - ...(launchToken ? { ORCA_AGENT_LAUNCH_TOKEN: launchToken } : {}) - } - const claudeAgentTeamsSourceCommand = - launchOpts.claudeAgentTeamsSourceCommand?.trim() || launchOpts.command?.trim() || undefined - const claudeAgentTeamsMode = this.store?.getSettings?.().claudeAgentTeamsMode - const effectiveClaudeAgentTeamsMode = inferCapturedClaudeAgentTeamsMode( - launchOpts.launchConfig, - claudeAgentTeamsSourceCommand, - claudeAgentTeamsMode - ) - const agentTeamsPlan = await buildClaudeAgentTeamsLaunchPlan({ - command: claudeAgentTeamsSourceCommand, - mode: effectiveClaudeAgentTeamsMode, - baseEnv: { - ...process.env, - ...baseEnv - }, - createTeamEnv: (shimDir, shimBin) => - this.claudeAgentTeams.createLaunchEnv({ - leaderHandle: preAllocatedHandle, - baseEnv: { - ...process.env, - ...baseEnv - }, - shimDir, - shimBin - }).env - }) - const sequencedStartupCommand = - agentTeamsPlan && - claudeAgentTeamsSourceCommand && - launchOpts.command && - claudeAgentTeamsSourceCommand !== launchOpts.command - ? agentTeamsPlan.command + return await this.runWithWorktreePtySpawn(workspace.id, workspace.connectionId, async () => { + const launchOpts = await this.resolveAgentTerminalCreateOptions(workspace, opts) + const cwd = + this.resolveWorkspaceTerminalStartupCwd(workspace, launchOpts.cwd) ?? workspace.path + const preAllocatedHandle = this.createPreAllocatedTerminalHandle() + // Why: mint tabId in main before spawn so paneKey is known at PTY env + // build time. Hook-based agent status (Claude/Codex/Cursor/Gemini) keys + // off `${tabId}:${leafId}` — without these vars set on the PTY, the + // hook payload arrives with an empty paneKey and the renderer cannot + // attribute the event. Use a stable UUID leaf because hooks reject the + // legacy numeric pane keys after the pane-id migration. + const hintedTabId = launchOpts.tabId?.trim() + const canAdoptPaneIdentity = + hintedTabId !== undefined && + isValidHostTerminalTabId(hintedTabId) && + launchOpts.leafId !== undefined && + isTerminalLeafId(launchOpts.leafId) + const tabId = canAdoptPaneIdentity ? (hintedTabId as string) : randomUUID() + const leafId = canAdoptPaneIdentity ? (launchOpts.leafId as string) : randomUUID() + const paneKey = makePaneKey(tabId, leafId) + const launchToken = launchOpts.launchConfig + ? (launchOpts.launchToken ?? randomUUID()) : undefined - const effectiveLaunchConfig = - launchOpts.launchConfig && agentTeamsPlan - ? { - ...launchOpts.launchConfig, - agentCommand: launchOpts.launchConfig.agentCommand - ? effectiveClaudeAgentTeamsMode === 'in-process' || process.platform === 'win32' - ? addClaudeTeammateModeInProcess(launchOpts.launchConfig.agentCommand) - : addClaudeTeammateModeAuto(launchOpts.launchConfig.agentCommand) - : agentTeamsPlan.command, - agentEnv: { - ...launchOpts.launchConfig.agentEnv, - ...agentTeamsPlan.env - } - } - : launchOpts.launchConfig - // Why: setup/agent sequencing wraps the PTY launch in a wait shell before - // Claude Agent Teams runs. Preserve the direct Claude command separately - // so the wrapper can exec the teammate-mode variant after setup completes. - const env = this.buildTerminalWorkspaceEnv( - workspace, - { - ...baseEnv, - ...(sequencedStartupCommand - ? { [SETUP_AGENT_SEQUENCE_STARTUP_COMMAND_ENV]: sequencedStartupCommand } - : {}) - }, - paneKey, - tabId, - agentTeamsPlan?.env - ) - const result = await this.ptyController.spawn({ - cols: 120, - rows: 40, - cwd, - command: sequencedStartupCommand - ? launchOpts.command - : (agentTeamsPlan?.command ?? launchOpts.command), - commandDelivery: 'provider', - startupCommandDelivery: launchOpts.startupCommandDelivery, - env, - envToDelete: agentTeamsPlan?.envToDelete, - telemetry: launchOpts.telemetry, - connectionId: workspace.connectionId, - worktreeId: workspace.id, - preAllocatedHandle, - tabId, - leafId, - ...(launchOpts.sessionId ? { sessionId: launchOpts.sessionId } : {}), - ...(launchOpts.persistHostSessionBinding ? { persistHostSessionBinding: true } : {}) - }) - this.registerPreAllocatedHandleForPty(result.id, preAllocatedHandle) - this.registerPty(result.id, workspace.id, workspace.connectionId) - const pty = this.getOrCreatePtyWorktreeRecord(result.id) - if (pty) { - if (launchOpts.title) { - const observedAt = this.nextTitleObservationSequence() - pty.title = launchOpts.title - pty.titleUpdatedAt = observedAt - this.setPtyManagementTitleFromObservedTitle(pty, launchOpts.title, observedAt) - } else { - pty.title = null - pty.titleUpdatedAt = null + const baseEnv = { + ...launchOpts.env, + ...(launchToken ? { ORCA_AGENT_LAUNCH_TOKEN: launchToken } : {}) } - pty.tabId = tabId - pty.paneKey = paneKey - pty.launchConfig = effectiveLaunchConfig - ? copySleepingAgentLaunchConfig(effectiveLaunchConfig) - : null - pty.launchToken = launchToken ?? null - pty.launchAgent = launchOpts.launchAgent ?? null - } - const handle = pty ? this.issuePtyHandle(pty) : preAllocatedHandle - if (pty && launchOpts.deferMobileSessionPublish !== true) { - this.publishPtyBackedMobileSessionTerminal(workspace.id, pty, { + const claudeAgentTeamsSourceCommand = + launchOpts.claudeAgentTeamsSourceCommand?.trim() || + launchOpts.command?.trim() || + undefined + const claudeAgentTeamsMode = this.store?.getSettings?.().claudeAgentTeamsMode + const effectiveClaudeAgentTeamsMode = inferCapturedClaudeAgentTeamsMode( + launchOpts.launchConfig, + claudeAgentTeamsSourceCommand, + claudeAgentTeamsMode + ) + const agentTeamsPlan = await buildClaudeAgentTeamsLaunchPlan({ + command: claudeAgentTeamsSourceCommand, + mode: effectiveClaudeAgentTeamsMode, + baseEnv: { + ...process.env, + ...baseEnv + }, + createTeamEnv: (shimDir, shimBin) => + this.claudeAgentTeams.createLaunchEnv({ + leaderHandle: preAllocatedHandle, + baseEnv: { + ...process.env, + ...baseEnv + }, + shimDir, + shimBin + }).env + }) + const sequencedStartupCommand = + agentTeamsPlan && + claudeAgentTeamsSourceCommand && + launchOpts.command && + claudeAgentTeamsSourceCommand !== launchOpts.command + ? agentTeamsPlan.command + : undefined + const effectiveLaunchConfig = + launchOpts.launchConfig && agentTeamsPlan + ? { + ...launchOpts.launchConfig, + agentCommand: launchOpts.launchConfig.agentCommand + ? effectiveClaudeAgentTeamsMode === 'in-process' || process.platform === 'win32' + ? addClaudeTeammateModeInProcess(launchOpts.launchConfig.agentCommand) + : addClaudeTeammateModeAuto(launchOpts.launchConfig.agentCommand) + : agentTeamsPlan.command, + agentEnv: { + ...launchOpts.launchConfig.agentEnv, + ...agentTeamsPlan.env + } + } + : launchOpts.launchConfig + // Why: setup/agent sequencing wraps the PTY launch in a wait shell before + // Claude Agent Teams runs. Preserve the direct Claude command separately + // so the wrapper can exec the teammate-mode variant after setup completes. + const env = this.buildTerminalWorkspaceEnv( + workspace, + { + ...baseEnv, + ...(sequencedStartupCommand + ? { [SETUP_AGENT_SEQUENCE_STARTUP_COMMAND_ENV]: sequencedStartupCommand } + : {}) + }, + paneKey, + tabId, + agentTeamsPlan?.env + ) + const result = await this.spawnPtyWithAdmissionHeld({ + cols: 120, + rows: 40, + cwd, + command: sequencedStartupCommand + ? launchOpts.command + : (agentTeamsPlan?.command ?? launchOpts.command), + commandDelivery: 'provider', + startupCommandDelivery: launchOpts.startupCommandDelivery, + env, + envToDelete: agentTeamsPlan?.envToDelete, + telemetry: launchOpts.telemetry, + connectionId: workspace.connectionId, + worktreeId: workspace.id, + preAllocatedHandle, tabId, leafId, - title: launchOpts.title ?? null, - activate: presentation === 'focused', - // Why: explicit background presentation may carry legacy activate - // metadata from an already-owned renderer pane; don't select it on mobile. - selectIfNoActiveTab: presentation !== 'background', - ...(cwd !== workspace.path ? { startupCwd: cwd } : {}) + ...(launchOpts.sessionId ? { sessionId: launchOpts.sessionId } : {}), + ...(launchOpts.persistHostSessionBinding ? { persistHostSessionBinding: true } : {}) }) - } - let surface: RuntimeTerminalCreate['surface'] = 'background' - let warning: string | undefined - if (presentation !== 'background' && this.notifier?.revealTerminalSession) { - try { - // Why: after the PTY is spawned, renderer tab adoption is best-effort; - // failing here must not strand a live process without returning a handle. - // Pass the pre-minted tabId so the renderer adopts under the same id - // already baked into the PTY env — keeps paneKey hook attribution intact. - await this.notifier.revealTerminalSession(workspace.id, { - ptyId: result.id, + this.registerPreAllocatedHandleForPty(result.id, preAllocatedHandle) + this.registerPty(result.id, workspace.id, workspace.connectionId) + const pty = this.getOrCreatePtyWorktreeRecord(result.id) + if (pty) { + if (launchOpts.title) { + const observedAt = this.nextTitleObservationSequence() + pty.title = launchOpts.title + pty.titleUpdatedAt = observedAt + this.setPtyManagementTitleFromObservedTitle(pty, launchOpts.title, observedAt) + } else { + pty.title = null + pty.titleUpdatedAt = null + } + pty.tabId = tabId + pty.paneKey = paneKey + pty.launchConfig = effectiveLaunchConfig + ? copySleepingAgentLaunchConfig(effectiveLaunchConfig) + : null + pty.launchToken = launchToken ?? null + pty.launchAgent = launchOpts.launchAgent ?? null + } + const handle = pty ? this.issuePtyHandle(pty) : preAllocatedHandle + if (pty && launchOpts.deferMobileSessionPublish !== true) { + this.publishPtyBackedMobileSessionTerminal(workspace.id, pty, { + tabId, + leafId, title: launchOpts.title ?? null, - ...(cwd !== workspace.path ? { cwd } : {}), - ...(effectiveLaunchConfig ? { launchConfig: effectiveLaunchConfig } : {}), - ...(launchToken ? { launchToken } : {}), - ...(launchOpts.launchAgent ? { launchAgent: launchOpts.launchAgent } : {}), activate: presentation === 'focused', - ...(presentation ? { presentation } : {}), - tabId, - leafId + // Why: explicit background presentation may carry legacy activate + // metadata from an already-owned renderer pane; don't select it on mobile. + selectIfNoActiveTab: presentation !== 'background', + ...(cwd !== workspace.path ? { startupCwd: cwd } : {}) }) - surface = 'visible' - } catch (err) { - console.warn(`[terminal-create] failed to create inactive tab for ${result.id}:`, err) - warning = createTerminalRevealWarning(handle, err) } - } else if (presentation !== 'background') { - warning = createTerminalRevealWarning(handle) - } - return { - handle, - tabId, - paneKey, - ptyId: result.id, - worktreeId: workspace.id, - title: launchOpts.title ?? null, - surface, - ...(warning ? { warning } : {}) - } + let surface: RuntimeTerminalCreate['surface'] = 'background' + let warning: string | undefined + if (presentation !== 'background' && this.notifier?.revealTerminalSession) { + try { + // Why: after the PTY is spawned, renderer tab adoption is best-effort; + // failing here must not strand a live process without returning a handle. + // Pass the pre-minted tabId so the renderer adopts under the same id + // already baked into the PTY env — keeps paneKey hook attribution intact. + await this.notifier.revealTerminalSession(workspace.id, { + ptyId: result.id, + title: launchOpts.title ?? null, + ...(cwd !== workspace.path ? { cwd } : {}), + ...(effectiveLaunchConfig ? { launchConfig: effectiveLaunchConfig } : {}), + ...(launchToken ? { launchToken } : {}), + ...(launchOpts.launchAgent ? { launchAgent: launchOpts.launchAgent } : {}), + activate: presentation === 'focused', + ...(presentation ? { presentation } : {}), + tabId, + leafId + }) + surface = 'visible' + } catch (err) { + console.warn(`[terminal-create] failed to create inactive tab for ${result.id}:`, err) + warning = createTerminalRevealWarning(handle, err) + } + } else if (presentation !== 'background') { + warning = createTerminalRevealWarning(handle) + } + return { + handle, + tabId, + paneKey, + ptyId: result.id, + worktreeId: workspace.id, + title: launchOpts.title ?? null, + surface, + ...(warning ? { warning } : {}) + } + }) } this.assertGraphReady() @@ -18665,64 +18762,70 @@ export class OrcaRuntimeService { } const direction = opts.direction ?? 'horizontal' const workspace = await this.resolveTerminalWorkspaceLaunchScope(`id:${pty.worktreeId}`) - const leafId = randomUUID() - const preAllocatedHandle = this.createPreAllocatedTerminalHandle() - const paneKey = makePaneKey(parentTabId, leafId) - const result = await this.ptyController.spawn({ - cols: 120, - rows: 40, - cwd: workspace.path, - command: opts.command, - commandDelivery: 'provider', - env: this.buildTerminalWorkspaceEnv(workspace, opts.env ?? {}, paneKey, parentTabId), - envToDelete: opts.envToDelete, - connectionId: workspace.connectionId, - worktreeId: workspace.id, - preAllocatedHandle - }) - this.registerPreAllocatedHandleForPty(result.id, preAllocatedHandle) - this.registerPty(result.id, workspace.id, workspace.connectionId) - const createdPty = this.getOrCreatePtyWorktreeRecord(result.id) - if (createdPty) { - createdPty.tabId = parentTabId - createdPty.paneKey = paneKey - } - - try { - await this.notifier?.revealTerminalSession?.(workspace.id, { - ptyId: result.id, - title: null, - activate: opts.activate !== false, - tabId: parentTabId, - leafId, - splitFromLeafId: parsedPaneKey.leafId, - splitDirection: direction, - splitTelemetrySource: opts.telemetrySource - }) - } catch (error) { - this.ptyController.kill?.(result.id) - throw error - } - if (createdPty) { - this.publishPtyBackedMobileSessionTerminal(workspace.id, createdPty, { - tabId: parentTabId, - leafId, - title: null, - activate: opts.activate !== false, - split: { splitFromLeafId: parsedPaneKey.leafId, direction } - }) - // Why: persist the split into the workspace session so a later snapshot - // rebuild keeps it instead of collapsing back to a single pane. - this.persistHeadlessTerminalSplit({ - tabId: parentTabId, - leafId, - ptyId: createdPty.ptyId, - splitFromLeafId: parsedPaneKey.leafId, - direction + return await this.runWithWorktreePtySpawn(workspace.id, workspace.connectionId, async () => { + const leafId = randomUUID() + const preAllocatedHandle = this.createPreAllocatedTerminalHandle() + const paneKey = makePaneKey(parentTabId, leafId) + const result = await this.spawnPtyWithAdmissionHeld({ + cols: 120, + rows: 40, + cwd: workspace.path, + command: opts.command, + commandDelivery: 'provider', + env: this.buildTerminalWorkspaceEnv(workspace, opts.env ?? {}, paneKey, parentTabId), + envToDelete: opts.envToDelete, + connectionId: workspace.connectionId, + worktreeId: workspace.id, + preAllocatedHandle }) - } + this.registerPreAllocatedHandleForPty(result.id, preAllocatedHandle) + this.registerPty(result.id, workspace.id, workspace.connectionId) + const createdPty = this.getOrCreatePtyWorktreeRecord(result.id) + if (createdPty) { + createdPty.tabId = parentTabId + createdPty.paneKey = paneKey + } + + try { + await this.notifier?.revealTerminalSession?.(workspace.id, { + ptyId: result.id, + title: null, + activate: opts.activate !== false, + tabId: parentTabId, + leafId, + splitFromLeafId: parsedPaneKey.leafId, + splitDirection: direction, + splitTelemetrySource: opts.telemetrySource + }) + } catch (error) { + this.ptyController?.kill(result.id) + throw error + } + if (createdPty) { + this.publishPtyBackedMobileSessionTerminal(workspace.id, createdPty, { + tabId: parentTabId, + leafId, + title: null, + activate: opts.activate !== false, + split: { splitFromLeafId: parsedPaneKey.leafId, direction } + }) + // Why: persist the split into the workspace session so a later snapshot + // rebuild keeps it instead of collapsing back to a single pane. + this.persistHeadlessTerminalSplit({ + tabId: parentTabId, + leafId, + ptyId: createdPty.ptyId, + splitFromLeafId: parsedPaneKey.leafId, + direction + }) + } - return { handle: this.issuePtyHandle(createdPty ?? pty), tabId: parentTabId, paneRuntimeId: -1 } + return { + handle: this.issuePtyHandle(createdPty ?? pty), + tabId: parentTabId, + paneRuntimeId: -1 + } + }) } async handleAgentTeamsTmuxCompat( @@ -18814,31 +18917,197 @@ export class OrcaRuntimeService { }) } - async stopTerminalsForWorktree(worktreeSelector: string): Promise<{ stopped: number }> { - // Why: this mutates live PTYs, so the runtime must reject it while the - // renderer graph is reloading instead of acting on cached leaf ownership. - const graphEpoch = this.captureReadyGraphEpoch() - const worktree = await this.resolveWorktreeSelector(worktreeSelector) - this.assertStableReadyGraph(graphEpoch) + beginWorktreePtySpawn(worktreeId: string, connectionId?: string | null): () => void { + return this.worktreePtyAdmission.beginSpawn( + this.getWorktreePtyScopeKey(worktreeId, connectionId) + ) + } + + private async runWithWorktreePtySpawn( + worktreeId: string, + connectionId: string | null | undefined, + operation: () => Promise + ): Promise { + const release = this.beginWorktreePtySpawn(worktreeId, connectionId) + try { + return await operation() + } finally { + release() + } + } + + private async spawnPtyWithAdmissionHeld(opts: RuntimePtySpawnOptions): Promise<{ id: string }> { + const controller = this.ptyController + const spawn = controller?.spawnWithWorktreePtyAdmissionHeld ?? controller?.spawn + if (!controller || !spawn) { + throw new Error('runtime_unavailable') + } + return await spawn.call(controller, opts) + } + + async beginWorktreePtyTeardown( + worktreeId: string, + connectionId?: string | null + ): Promise<() => void> { + // Why: an existing daemon can own PTYs before the renderer/runtime graph + // hydrates. Do not snapshot the fallback provider while startup is swapping it. + if (connectionId == null) { + await this.awaitLocalPtyStartup?.() + } + return this.worktreePtyAdmission.closeForTeardown( + this.getWorktreePtyScopeKey(worktreeId, connectionId) + ) + } + + async runWithWorktreePtyTeardown( + worktreeId: string, + operation: () => Promise, + connectionId?: string | null + ): Promise { + // Why: closing admission before snapshots makes deletion own every spawn + // already in flight and keeps new desktop/mobile/SSH spawns out until Git settles. + const release = await this.beginWorktreePtyTeardown(worktreeId, connectionId) + try { + return await operation() + } finally { + release() + } + } + + private getWorktreePtyScopeKey(worktreeId: string, connectionId?: string | null): string { + return JSON.stringify([connectionId ?? null, worktreeId]) + } + + private runWithSuccessfulCleanupTeardown( + worktreeId: string, + connectionId: string | null | undefined, + operation: () => Promise + ): Promise { + return this.runWithWorktreePtyTeardown( + worktreeId, + async () => { + // Why: orphan and stale-registration cleanup still retires an Orca + // workspace; its terminals must not outlive the last addressable owner. + const localProvider = this.getLocalProvider() + if (localProvider) { + await killAllProcessesForWorktree(worktreeId, { + runtime: this, + localProvider, + connectionId, + onPtyStopped: this.onPtyStopped ?? undefined + }) + } + return operation() + }, + connectionId + ) + } + + async runWithWorktreeRemovalDedup( + worktreeId: string, + opts: { force: boolean; runHooks: boolean; hostId?: string; mode?: 'remove' | 'forget' }, + operation: () => Promise + ): Promise { + const optionsKey = `${opts.mode ?? 'remove'}:${getRuntimeWorktreeRemovalOptionsKey( + opts.force, + opts.runHooks + )}` + const removalKey = `${opts.hostId ?? ''}\0${worktreeId}` + const existing = this.crossSurfaceWorktreeRemovalInFlight.get(removalKey) + if (existing) { + if (existing.optionsKey === optionsKey) { + return existing.promise as Promise + } + throw new Error(`Worktree deletion already in progress: ${worktreeId}`) + } + const promise = operation() + this.crossSurfaceWorktreeRemovalInFlight.set(removalKey, { + optionsKey, + promise: promise as Promise + }) + try { + return await promise + } finally { + if (this.crossSurfaceWorktreeRemovalInFlight.get(removalKey)?.promise === promise) { + this.crossSurfaceWorktreeRemovalInFlight.delete(removalKey) + } + } + } + + async stopTerminalsForWorktree( + worktreeSelector: string, + opts: { worktreeTeardown?: boolean; connectionId?: string | null } = {} + ): Promise<{ stopped: number; failedPtyIds?: string[] }> { + // Why: generic terminal.stop must not act on a stale renderer graph, while + // deletion runs under admission and owns main's authoritative PTY records. + const graphEpoch = opts.worktreeTeardown ? null : this.captureReadyGraphEpoch() + const exactTeardownTarget = opts.worktreeTeardown + ? parseExactWorktreeIdSelector(worktreeSelector) + : null + // Why: successful orphan/missing-path cleanup has authoritative metadata + // but no live Git row for selector resolution; deletion owns the exact ID. + const worktreeId = + exactTeardownTarget?.id ?? (await this.resolveWorktreeSelector(worktreeSelector)).id + if (graphEpoch !== null) { + this.assertStableReadyGraph(graphEpoch) + } const ptyIds = new Set() - for (const leaf of this.leaves.values()) { - if (leaf.worktreeId === worktree.id && leaf.ptyId) { - ptyIds.add(leaf.ptyId) + if (!opts.worktreeTeardown) { + for (const leaf of this.leaves.values()) { + if (leaf.worktreeId === worktreeId && leaf.ptyId) { + ptyIds.add(leaf.ptyId) + } } } for (const pty of this.ptysById.values()) { - if (pty.worktreeId === worktree.id && pty.connected) { + if ( + pty.worktreeId === worktreeId && + pty.connected && + (opts.connectionId === undefined || pty.connectionId === opts.connectionId) + ) { ptyIds.add(pty.ptyId) } } + if (opts.worktreeTeardown && opts.connectionId !== null) { + for (const lease of this.store?.getSshRemotePtyLeases?.() ?? []) { + if ( + lease.worktreeId === worktreeId && + (opts.connectionId === undefined || lease.targetId === opts.connectionId) && + lease.state !== 'terminated' && + lease.state !== 'expired' + ) { + // Why: durable leases are the only authoritative owner after a + // restart or disconnect has removed renderer/runtime PTY records. + ptyIds.add(toAppSshPtyId(lease.targetId, lease.ptyId)) + } + } + } + const stopPty = async (ptyId: string): Promise => { + // Why: worktree removal immediately follows this with provider/registry + // sweeps. Await the verified stop so those sweeps cannot overlap a + // provider shutdown with a second ConPTY teardown on Windows (#8275). + // Why: deletion needs provider acknowledgement before its fallback sweeps, + // while the shared terminal.stop RPC must preserve graceful shell teardown. + if (opts.worktreeTeardown && this.ptyController?.stopAndWait) { + return this.ptyController.stopAndWait(ptyId) + } + return this.ptyController?.kill(ptyId) ?? false + } + const ids = [...ptyIds] + const stopResults = opts.worktreeTeardown + ? await mapPtyStopsWithConcurrency(ids, stopPty) + : await Promise.all(ids.map(stopPty)) let stopped = 0 - for (const ptyId of ptyIds) { - if (this.ptyController?.kill(ptyId)) { + const failedPtyIds: string[] = [] + for (const [index, didStop] of stopResults.entries()) { + if (didStop) { stopped += 1 + } else if (opts.worktreeTeardown) { + failedPtyIds.push(ids[index]) } } - return { stopped } + return failedPtyIds.length > 0 ? { stopped, failedPtyIds } : { stopped } } async stopExactTerminalsForWorktree( diff --git a/src/main/runtime/pty-stop-concurrency.test.ts b/src/main/runtime/pty-stop-concurrency.test.ts new file mode 100644 index 00000000000..4496da6348c --- /dev/null +++ b/src/main/runtime/pty-stop-concurrency.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it, vi } from 'vitest' +import { mapPtyStopsWithConcurrency, WORKTREE_PTY_STOP_CONCURRENCY } from './pty-stop-concurrency' + +describe('mapPtyStopsWithConcurrency', () => { + it('attempts and awaits every PTY before surfacing the first rejection', async () => { + const ids = Array.from( + { length: WORKTREE_PTY_STOP_CONCURRENCY + 3 }, + (_, index) => `pty-${index}` + ) + const pending = new Map void; reject: (error: Error) => void }>() + const stopPty = vi.fn( + (ptyId: string) => + new Promise((resolve, reject) => { + pending.set(ptyId, { resolve: () => resolve(ptyId), reject }) + }) + ) + + const stopping = mapPtyStopsWithConcurrency(ids, stopPty) + let settled = false + const observed = stopping.then( + () => { + settled = true + return null + }, + (error: unknown) => { + settled = true + return error + } + ) + await vi.waitFor(() => expect(stopPty).toHaveBeenCalledTimes(WORKTREE_PTY_STOP_CONCURRENCY)) + pending.get('pty-0')!.reject(new Error('first failed')) + await vi.waitFor(() => expect(stopPty).toHaveBeenCalledTimes(WORKTREE_PTY_STOP_CONCURRENCY + 1)) + await Promise.resolve() + expect(settled).toBe(false) + + for (const id of ids.slice(1)) { + pending.get(id)?.resolve() + await Promise.resolve() + } + await vi.waitFor(() => expect(stopPty).toHaveBeenCalledTimes(ids.length)) + for (const id of ids) { + pending.get(id)?.resolve() + } + + await expect(observed).resolves.toEqual(expect.objectContaining({ message: 'first failed' })) + expect(stopPty.mock.calls.map(([ptyId]) => ptyId)).toEqual(ids) + }) +}) diff --git a/src/main/runtime/pty-stop-concurrency.ts b/src/main/runtime/pty-stop-concurrency.ts new file mode 100644 index 00000000000..75952f30f40 --- /dev/null +++ b/src/main/runtime/pty-stop-concurrency.ts @@ -0,0 +1,38 @@ +export const WORKTREE_PTY_STOP_CONCURRENCY = 8 + +export async function mapPtyStopsWithConcurrency( + ptyIds: readonly string[], + stopPty: (ptyId: string) => Promise +): Promise { + const results = Array(ptyIds.length) + let nextIndex = 0 + let didFail = false + let firstError: unknown + const stopNext = async (): Promise => { + while (nextIndex < ptyIds.length) { + const index = nextIndex + nextIndex += 1 + try { + results[index] = await stopPty(ptyIds[index]) + } catch (error) { + // Why: deletion must attempt and await every snapshotted PTY even when + // one provider rejects, otherwise active workers can outlive Git removal. + if (!didFail) { + firstError = error + } + didFail = true + } + } + } + // Why: remote tools may each consume the full timeout. A small worker pool + // bounds resource use without making teardown latency linear in PTY count. + const workers = Array.from( + { length: Math.min(WORKTREE_PTY_STOP_CONCURRENCY, ptyIds.length) }, + () => stopNext() + ) + await Promise.all(workers) + if (didFail) { + throw firstError + } + return results +} diff --git a/src/main/runtime/rpc/terminal-stop.test.ts b/src/main/runtime/rpc/terminal-stop.test.ts new file mode 100644 index 00000000000..29ce3a7f78e --- /dev/null +++ b/src/main/runtime/rpc/terminal-stop.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it, vi } from 'vitest' +import type { OrcaRuntimeService } from '../orca-runtime' +import type { RpcRequest } from './core' +import { RpcDispatcher } from './dispatcher' +import { TERMINAL_METHODS } from './methods/terminal' + +describe('terminal stop RPC', () => { + it('uses the generic graceful worktree-stop mode', async () => { + const stopTerminalsForWorktree = vi.fn().mockResolvedValue({ stopped: 2 }) + const runtime = { + getRuntimeId: () => 'test-runtime', + stopTerminalsForWorktree + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: TERMINAL_METHODS }) + const request: RpcRequest = { + id: 'req-1', + authToken: 'tok', + method: 'terminal.stop', + params: { worktree: 'repo-1::/worktree' } + } + + const response = await dispatcher.dispatch(request) + + expect(response.ok).toBe(true) + expect(stopTerminalsForWorktree).toHaveBeenCalledWith('repo-1::/worktree') + }) +}) diff --git a/src/main/runtime/worktree-pty-admission.test.ts b/src/main/runtime/worktree-pty-admission.test.ts new file mode 100644 index 00000000000..fc7815b62ce --- /dev/null +++ b/src/main/runtime/worktree-pty-admission.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it, vi } from 'vitest' +import { WORKTREE_PTY_SPAWN_DRAIN_TIMEOUT_MS, WorktreePtyAdmission } from './worktree-pty-admission' + +describe('WorktreePtyAdmission', () => { + it('drains admitted spawns and rejects new ones until teardown settles', async () => { + const admission = new WorktreePtyAdmission() + const releaseSpawn = admission.beginSpawn('w1') + let releaseTeardown = () => {} + const operation = vi.fn( + () => + new Promise((resolve) => { + releaseTeardown = () => resolve('removed') + }) + ) + + const teardown = admission.runTeardown('w1', operation) + expect(operation).not.toHaveBeenCalled() + expect(() => admission.beginSpawn('w1')).toThrow('Worktree teardown is in progress') + + releaseSpawn() + await vi.waitFor(() => expect(operation).toHaveBeenCalledTimes(1)) + expect(() => admission.beginSpawn('w1')).toThrow('Worktree teardown is in progress') + releaseTeardown() + + await expect(teardown).resolves.toBe('removed') + const releaseNextSpawn = admission.beginSpawn('w1') + releaseNextSpawn() + }) + + it('keeps different worktrees independent', async () => { + const admission = new WorktreePtyAdmission() + const releaseSpawn = admission.beginSpawn('w1') + const teardown = admission.runTeardown('w1', async () => undefined) + + const releaseOtherSpawn = admission.beginSpawn('w2') + releaseOtherSpawn() + releaseSpawn() + + await expect(teardown).resolves.toBeUndefined() + }) + + it('rejects a duplicate teardown owner from another removal entry point', async () => { + const admission = new WorktreePtyAdmission() + let releaseFirst = () => {} + const order: string[] = [] + const first = admission.runTeardown( + 'w1', + () => + new Promise((resolve) => { + order.push('first') + releaseFirst = resolve + }) + ) + const second = admission.runTeardown('w1', async () => order.push('second')) + const secondRejection = expect(second).rejects.toThrow( + 'Worktree teardown is already in progress' + ) + + await vi.waitFor(() => expect(order).toEqual(['first'])) + await secondRejection + releaseFirst() + + await first + expect(order).toEqual(['first']) + }) + + it('fails a stuck drain closed and reopens admission without leaking the waiter', async () => { + vi.useFakeTimers() + try { + const admission = new WorktreePtyAdmission() + const releaseStuckSpawn = admission.beginSpawn('w1') + const teardown = admission.runTeardown('w1', vi.fn()) + const rejection = expect(teardown).rejects.toThrow('Timed out draining PTY spawns: w1') + + await vi.advanceTimersByTimeAsync(WORKTREE_PTY_SPAWN_DRAIN_TIMEOUT_MS) + await rejection + + const releaseNextSpawn = admission.beginSpawn('w1') + releaseNextSpawn() + releaseStuckSpawn() + await expect(admission.runTeardown('w1', async () => 'removed')).resolves.toBe('removed') + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/src/main/runtime/worktree-pty-admission.ts b/src/main/runtime/worktree-pty-admission.ts new file mode 100644 index 00000000000..b530603c67c --- /dev/null +++ b/src/main/runtime/worktree-pty-admission.ts @@ -0,0 +1,112 @@ +type WorktreePtyAdmissionState = { + activeSpawns: number + teardownOwners: number + drainWaiters: Set<() => void> +} + +export const WORKTREE_PTY_SPAWN_DRAIN_TIMEOUT_MS = 30_000 + +export class WorktreePtyAdmission { + private readonly states = new Map() + + beginSpawn(worktreeId: string): () => void { + // Why: the lease spans provider creation through runtime registration, so + // deletion can drain every PTY that could become visible after its snapshot. + const state = this.getOrCreate(worktreeId) + if (state.teardownOwners > 0) { + throw new Error(`Worktree teardown is in progress: ${worktreeId}`) + } + state.activeSpawns += 1 + let released = false + return () => { + if (released) { + return + } + released = true + state.activeSpawns -= 1 + if (state.activeSpawns === 0) { + for (const resolve of state.drainWaiters) { + resolve() + } + state.drainWaiters.clear() + } + this.deleteIfIdle(worktreeId, state) + } + } + + async runTeardown(worktreeId: string, operation: () => Promise): Promise { + const release = await this.closeForTeardown(worktreeId) + try { + return await operation() + } finally { + release() + } + } + + async closeForTeardown(worktreeId: string): Promise<() => void> { + const state = this.getOrCreate(worktreeId) + if (state.teardownOwners > 0) { + // Why: teardown-only callers and mismatched remove/forget operations do + // not join the shared removal promise; never queue their stale state. + throw new Error(`Worktree teardown is already in progress: ${worktreeId}`) + } + state.teardownOwners += 1 + if (state.activeSpawns > 0) { + let resolveDrain = (): void => {} + let timer: NodeJS.Timeout | undefined + const drain = new Promise((resolve) => { + resolveDrain = resolve + state.drainWaiters.add(resolve) + }) + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(new Error(`Timed out draining PTY spawns: ${worktreeId}`)), + WORKTREE_PTY_SPAWN_DRAIN_TIMEOUT_MS + ) + }) + try { + await Promise.race([drain, timeout]) + } catch (error) { + // Why: deletion failed before touching Git. Reopen admission so the + // still-running spawn can finish in the worktree that remains. + state.drainWaiters.delete(resolveDrain) + state.teardownOwners -= 1 + this.deleteIfIdle(worktreeId, state) + throw error + } finally { + if (timer) { + clearTimeout(timer) + } + } + } + let released = false + return () => { + if (released) { + return + } + released = true + state.teardownOwners -= 1 + this.deleteIfIdle(worktreeId, state) + } + } + + private getOrCreate(worktreeId: string): WorktreePtyAdmissionState { + const existing = this.states.get(worktreeId) + if (existing) { + return existing + } + const state: WorktreePtyAdmissionState = { + activeSpawns: 0, + teardownOwners: 0, + drainWaiters: new Set() + } + this.states.set(worktreeId, state) + return state + } + + private deleteIfIdle(worktreeId: string, state: WorktreePtyAdmissionState): void { + if (state.activeSpawns === 0 && state.teardownOwners === 0) { + this.states.delete(worktreeId) + } + } +} diff --git a/src/main/runtime/worktree-teardown.test.ts b/src/main/runtime/worktree-teardown.test.ts index 63e207cd8da..8748860e3f0 100644 --- a/src/main/runtime/worktree-teardown.test.ts +++ b/src/main/runtime/worktree-teardown.test.ts @@ -38,6 +38,24 @@ function createProviderStub( } as unknown as IPtyProvider } +async function releaseBoundedShutdownWaves( + shutdown: ReturnType, + pending: { settle: () => void }[], + targetCount: number +): Promise { + await vi.waitFor(() => expect(shutdown).toHaveBeenCalledTimes(8)) + let waves = 0 + while (shutdown.mock.calls.length < targetCount) { + waves += 1 + const callsBeforeRelease = shutdown.mock.calls.length + pending.splice(0).forEach(({ settle }) => settle()) + await vi.waitFor(() => expect(shutdown.mock.calls.length).toBeGreaterThan(callsBeforeRelease)) + } + waves += 1 + pending.splice(0).forEach(({ settle }) => settle()) + return waves +} + describe('killAllProcessesForWorktree', () => { beforeEach(() => { listRegisteredPtysMock.mockReset() @@ -92,7 +110,7 @@ describe('killAllProcessesForWorktree', () => { expect(onPtyStopped).toHaveBeenCalledWith('1') }) - it('best-effort: swallows errors from listProcesses and shutdown', async () => { + it('fails closed when the provider inventory cannot be read', async () => { const localProvider = createProviderStub(() => Promise.reject(new Error('boom'))) listRegisteredPtysMock.mockReturnValue([ { ptyId: 'x', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 10 } @@ -101,12 +119,7 @@ describe('killAllProcessesForWorktree', () => { new Error('already dead') ) - const result = await killAllProcessesForWorktree('w1', { localProvider }) - - // listProcesses rejected → provider sweep returns 0; registry shutdown - // rejected → counted as not-killed (registry sweep currently swallows). - expect(result.providerStopped).toBe(0) - expect(result.registryStopped).toBe(0) + await expect(killAllProcessesForWorktree('w1', { localProvider })).rejects.toThrow('boom') }) it('does not let cleanup hook failures abort teardown', async () => { @@ -157,11 +170,269 @@ describe('killAllProcessesForWorktree', () => { const result = await killAllProcessesForWorktree('w1', { runtime, localProvider }) - expect(stopTerminalsForWorktree).toHaveBeenCalledWith('w1') + expect(stopTerminalsForWorktree).toHaveBeenCalledWith('w1', { + worktreeTeardown: true + }) expect(result.runtimeStopped).toBe(3) }) - it('tolerates runtime.stopTerminalsForWorktree throwing (headless assertGraphReady reject)', async () => { + it('fails closed when an SSH-owned runtime PTY cannot be verified stopped', async () => { + const stopTerminalsForWorktree = vi.fn().mockResolvedValue({ + stopped: 0, + failedPtyIds: ['ssh:ssh-1@@relay-pty'] + }) + const runtime = { + stopTerminalsForWorktree + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub(async () => []) + listRegisteredPtysMock.mockReturnValue([]) + + await expect(killAllProcessesForWorktree('w1', { runtime, localProvider })).rejects.toThrow( + 'Failed to stop remote worktree terminals: ssh:ssh-1@@relay-pty' + ) + expect(localProvider.listProcesses).toHaveBeenCalledTimes(1) + }) + + it('does not sweep a colliding local-provider worktree during SSH teardown', async () => { + const stopTerminalsForWorktree = vi.fn().mockResolvedValue({ stopped: 1 }) + const runtime = { + stopTerminalsForWorktree + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub(async () => [ + { id: 'w1@@local-witness', cwd: '/tmp/w1', title: 'shell' } + ]) + + await expect( + killAllProcessesForWorktree('w1', { + runtime, + localProvider, + connectionId: 'ssh-1' + }) + ).resolves.toEqual({ runtimeStopped: 1, providerStopped: 0, registryStopped: 0 }) + expect(stopTerminalsForWorktree).toHaveBeenCalledWith('w1', { + worktreeTeardown: true, + connectionId: 'ssh-1' + }) + expect(localProvider.listProcesses).not.toHaveBeenCalled() + expect(localProvider.shutdown).not.toHaveBeenCalled() + }) + + it('fails closed when a local runtime PTY remains after fallback shutdown rejects', async () => { + const runtime = { + stopTerminalsForWorktree: vi.fn().mockResolvedValue({ + stopped: 0, + failedPtyIds: ['w1@@daemon'] + }) + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub(async () => [ + { id: 'w1@@daemon', cwd: '/tmp/w1', title: 'shell' } + ]) + vi.mocked(localProvider.shutdown).mockRejectedValue(new Error('daemon still alive')) + listRegisteredPtysMock.mockReturnValue([]) + + await expect(killAllProcessesForWorktree('w1', { runtime, localProvider })).rejects.toThrow( + 'Failed to stop local worktree terminals: w1@@daemon' + ) + }) + + it('recovers a failed local runtime stop through one deduplicated provider shutdown', async () => { + const runtime = { + stopTerminalsForWorktree: vi.fn().mockResolvedValue({ + stopped: 0, + failedPtyIds: ['w1@@daemon'] + }) + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub(async () => [ + { id: 'w1@@daemon', cwd: '/tmp/w1', title: 'shell' } + ]) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'w1@@daemon', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 10 } + ]) + + await expect(killAllProcessesForWorktree('w1', { runtime, localProvider })).resolves.toEqual({ + runtimeStopped: 0, + providerStopped: 1, + registryStopped: 0 + }) + expect(localProvider.shutdown).toHaveBeenCalledTimes(1) + }) + + it('accepts a stale duplicate registry row only after authoritative absence verification', async () => { + const localProvider = createProviderStub( + vi + .fn() + .mockResolvedValueOnce([{ id: 'w1@@daemon', cwd: '/tmp/w1', title: 'shell' }]) + .mockResolvedValueOnce([]) + ) + vi.mocked(localProvider.shutdown).mockRejectedValue(new Error('not found')) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'w1@@daemon', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 10 } + ]) + + await expect(killAllProcessesForWorktree('w1', { localProvider })).resolves.toEqual({ + runtimeStopped: 0, + providerStopped: 0, + registryStopped: 0 + }) + expect(localProvider.listProcesses).toHaveBeenCalledTimes(2) + expect(localProvider.shutdown).toHaveBeenCalledTimes(1) + }) + + it('retires runtime and provider state after authoritative fallback absence', async () => { + const onPtyExit = vi.fn() + const runtime = { + stopTerminalsForWorktree: vi.fn().mockResolvedValue({ + stopped: 0, + failedPtyIds: ['w1@@daemon'] + }), + onPtyExit + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub( + vi + .fn() + .mockResolvedValueOnce([{ id: 'w1@@daemon', cwd: '/tmp/w1', title: 'shell' }]) + .mockResolvedValueOnce([]) + ) + vi.mocked(localProvider.shutdown).mockRejectedValue(new Error('not found')) + listRegisteredPtysMock.mockReturnValue([]) + const onPtyStopped = vi.fn() + + await expect( + killAllProcessesForWorktree('w1', { runtime, localProvider, onPtyStopped }) + ).resolves.toEqual({ runtimeStopped: 0, providerStopped: 0, registryStopped: 0 }) + expect(onPtyStopped).toHaveBeenCalledWith('w1@@daemon') + expect(onPtyExit).toHaveBeenCalledWith('w1@@daemon', -1) + }) + + it('bounds a 50-session headless fallback sweep to seven shutdown waves', async () => { + const sessions = Array.from({ length: 50 }, (_, index) => ({ + id: `w1@@${index}`, + cwd: '/tmp/w1', + title: 'shell' + })) + const localProvider = createProviderStub(async () => sessions) + const pending: { settle: () => void }[] = [] + let active = 0 + let peak = 0 + vi.mocked(localProvider.shutdown).mockImplementation( + () => + new Promise((resolve) => { + active += 1 + peak = Math.max(peak, active) + pending.push({ + settle: () => { + active -= 1 + resolve() + } + }) + }) + ) + listRegisteredPtysMock.mockReturnValue([]) + + const teardown = killAllProcessesForWorktree('w1', { localProvider }) + const waves = await releaseBoundedShutdownWaves( + vi.mocked(localProvider.shutdown), + pending, + sessions.length + ) + + await expect(teardown).resolves.toEqual({ + runtimeStopped: 0, + providerStopped: 50, + registryStopped: 0 + }) + expect(peak).toBe(8) + expect(waves).toBe(7) + }) + + it('waits for seven bounded failure waves before rejecting a 50-session sweep', async () => { + const sessions = Array.from({ length: 50 }, (_, index) => ({ + id: `w1@@${index}`, + cwd: '/tmp/w1', + title: 'shell' + })) + const localProvider = createProviderStub(async () => sessions) + const pending: { settle: () => void }[] = [] + let active = 0 + let peak = 0 + vi.mocked(localProvider.shutdown).mockImplementation( + () => + new Promise((_resolve, reject) => { + active += 1 + peak = Math.max(peak, active) + pending.push({ + settle: () => { + active -= 1 + reject(new Error('shutdown timed out after 3000ms')) + } + }) + }) + ) + listRegisteredPtysMock.mockReturnValue([]) + + const teardown = killAllProcessesForWorktree('w1', { localProvider }) + const waves = await releaseBoundedShutdownWaves( + vi.mocked(localProvider.shutdown), + pending, + sessions.length + ) + + await expect(teardown).rejects.toThrow('Failed to stop local worktree terminals') + expect(localProvider.listProcesses).toHaveBeenCalledTimes(2) + expect(peak).toBe(8) + expect(waves).toBe(7) + }) + + it('awaits each runtime stop before fallback sweeps and preserves unrelated sessions', async () => { + const releases = new Map void>() + const liveSessions = new Set(['w1@@target', 'w2@@target', 'witness@@unrelated']) + const stopTerminalsForWorktree = vi.fn( + (worktreeId: string) => + new Promise<{ stopped: number }>((resolve) => { + releases.set(worktreeId, () => { + liveSessions.delete(`${worktreeId}@@target`) + // Why: a spawn that finishes after the runtime snapshot still has + // to be caught by the later provider sweep before deletion. + liveSessions.add(`${worktreeId}@@late`) + resolve({ stopped: 1 }) + }) + }) + ) + const runtime = { + stopTerminalsForWorktree + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub(async () => + [...liveSessions].map((id) => ({ id, cwd: '/tmp', title: 'shell' })) + ) + listRegisteredPtysMock.mockReturnValue([]) + + for (const worktreeId of ['w1', 'w2']) { + const listCountBeforeStop = vi.mocked(localProvider.listProcesses).mock.calls.length + const shutdownCountBeforeStop = vi.mocked(localProvider.shutdown).mock.calls.length + const removal = killAllProcessesForWorktree(worktreeId, { runtime, localProvider }) + await vi.waitFor(() => expect(releases.has(worktreeId)).toBe(true)) + + expect(localProvider.listProcesses).toHaveBeenCalledTimes(listCountBeforeStop) + expect(localProvider.shutdown).toHaveBeenCalledTimes(shutdownCountBeforeStop) + releases.get(worktreeId)?.() + + await expect(removal).resolves.toEqual({ + runtimeStopped: 1, + providerStopped: 1, + registryStopped: 0 + }) + expect(localProvider.shutdown).toHaveBeenCalledWith(`${worktreeId}@@late`, { + immediate: true + }) + } + + expect(localProvider.shutdown).not.toHaveBeenCalledWith('witness@@unrelated', { + immediate: true + }) + expect(liveSessions.has('witness@@unrelated')).toBe(true) + }) + + it('fails closed on an unexpected runtime worktree-stop failure', async () => { const stopTerminalsForWorktree = vi.fn().mockRejectedValue(new Error('graph not ready')) const runtime = { stopTerminalsForWorktree @@ -170,8 +441,9 @@ describe('killAllProcessesForWorktree', () => { const localProvider = createProviderStub(async () => []) listRegisteredPtysMock.mockReturnValue([]) - const result = await killAllProcessesForWorktree('w1', { runtime, localProvider }) - - expect(result.runtimeStopped).toBe(0) + await expect(killAllProcessesForWorktree('w1', { runtime, localProvider })).rejects.toThrow( + 'graph not ready' + ) + expect(localProvider.listProcesses).not.toHaveBeenCalled() }) }) diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 7572b9bbd77..5761fab9691 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -1,10 +1,13 @@ import type { IPtyProvider } from '../providers/types' import type { OrcaRuntimeService } from './orca-runtime' import { listRegisteredPtys } from '../memory/pty-registry' +import { parseAppSshPtyId } from '../../shared/ssh-pty-id' +import { mapPtyStopsWithConcurrency } from './pty-stop-concurrency' export type WorktreeTeardownDeps = { runtime?: OrcaRuntimeService localProvider: IPtyProvider + connectionId?: string | null onPtyStopped?: (ptyId: string) => void } @@ -32,9 +35,9 @@ export type WorktreeTeardownResult = { * canonical source for memory attribution; it also redundantly backstops * daemon spawns. * - * Best-effort throughout: each sweep catches its own errors. The caller - * (removeManagedWorktree, worktrees:remove IPC) must run the git-level - * removal regardless of what this returns. + * Provider and registry ownership overlap, so their targets are deduplicated + * before shutdown. Any stop that cannot be verified fails closed: Git removal + * must never race a shell whose cwd still belongs to the worktree. */ export async function killAllProcessesForWorktree( worktreeId: string, @@ -45,67 +48,103 @@ export async function killAllProcessesForWorktree( providerStopped: 0, registryStopped: 0 } + let failedPtyIds: string[] = [] if (deps.runtime) { - const r = await deps.runtime.stopTerminalsForWorktree(worktreeId).catch(() => ({ stopped: 0 })) + const r = await deps.runtime.stopTerminalsForWorktree(worktreeId, { + worktreeTeardown: true, + ...(deps.connectionId !== undefined ? { connectionId: deps.connectionId } : {}) + }) result.runtimeStopped = r.stopped + failedPtyIds = 'failedPtyIds' in r ? (r.failedPtyIds ?? []) : [] } - result.providerStopped = await sweepProviderByPrefix( - worktreeId, - deps.localProvider, - deps.onPtyStopped - ) - result.registryStopped = await sweepRegistryForWorktree( - worktreeId, - deps.localProvider, - deps.onPtyStopped - ) + const failedRemotePtyIds = failedPtyIds.filter((ptyId) => parseAppSshPtyId(ptyId) !== null) + const failedLocalPtyIds = failedPtyIds.filter((ptyId) => parseAppSshPtyId(ptyId) === null) + const fallbackResult = deps.connectionId + ? { providerStopped: 0, registryStopped: 0 } + : await sweepLocalProvider(worktreeId, deps.localProvider, failedLocalPtyIds, (ptyId) => { + clearStoppedPtyState(ptyId, deps.onPtyStopped) + // Why: provider shutdown and already-absent fallbacks do not always emit + // an exit event, so retire the runtime record before Git removes its cwd. + deps.runtime?.onPtyExit?.(ptyId, -1) + }) + result.providerStopped = fallbackResult.providerStopped + result.registryStopped = fallbackResult.registryStopped + + if (failedRemotePtyIds.length > 0) { + // Why: the local provider cannot prove an SSH-owned PTY dead; remote Git + // removal must not proceed after an unverified exact stop. + throw new Error(`Failed to stop remote worktree terminals: ${failedRemotePtyIds.join(', ')}`) + } return result } -async function sweepProviderByPrefix( +async function sweepLocalProvider( worktreeId: string, provider: IPtyProvider, - onPtyStopped?: (ptyId: string) => void -): Promise { + failedLocalPtyIds: readonly string[], + retirePty?: (ptyId: string) => void +): Promise<{ providerStopped: number; registryStopped: number }> { const prefix = `${worktreeId}@@` - const sessions = await provider.listProcesses().catch(() => []) - let killed = 0 - for (const s of sessions) { - if (!s.id.startsWith(prefix)) { - continue + const sessions = await provider.listProcesses() + const targets = new Map() + for (const session of sessions) { + if (session.id.startsWith(prefix)) { + targets.set(session.id, 'provider') } - try { - await provider.shutdown(s.id, { immediate: true }) - clearStoppedPtyState(s.id, onPtyStopped) - killed += 1 - } catch { - // Already dead, or the backend dropped the session — treat as success. - killed += 1 + } + for (const entry of listRegisteredPtys()) { + if (entry.worktreeId === worktreeId && !targets.has(entry.ptyId)) { + targets.set(entry.ptyId, 'registry') + } + } + for (const ptyId of failedLocalPtyIds) { + if (!targets.has(ptyId)) { + targets.set(ptyId, 'registry') } } - return killed -} -async function sweepRegistryForWorktree( - worktreeId: string, - localProvider: IPtyProvider, - onPtyStopped?: (ptyId: string) => void -): Promise { - const entries = listRegisteredPtys().filter((r) => r.worktreeId === worktreeId) - let killed = 0 - for (const entry of entries) { + const failedShutdowns = new Map() + const stopped = new Set() + await mapPtyStopsWithConcurrency([...targets.keys()], async (ptyId) => { try { - await localProvider.shutdown(entry.ptyId, { immediate: true }) - clearStoppedPtyState(entry.ptyId, onPtyStopped) - killed += 1 - } catch { - /* ignore — best-effort */ + await provider.shutdown(ptyId, { immediate: true }) + stopped.add(ptyId) + retirePty?.(ptyId) + } catch (error) { + failedShutdowns.set(ptyId, error) + } + }) + + if (failedShutdowns.size > 0) { + // Why: duplicate/stale registry rows may report "not found" after another + // owner already stopped the PTY. Only an authoritative post-sweep absence + // converts that rejection into verified success. + const remainingIds = new Set((await provider.listProcesses()).map((session) => session.id)) + const unverified = [...failedShutdowns.keys()].filter((ptyId) => remainingIds.has(ptyId)) + if (unverified.length > 0) { + throw new Error(`Failed to stop local worktree terminals: ${unverified.join(', ')}`) + } + for (const ptyId of failedShutdowns.keys()) { + retirePty?.(ptyId) + } + } + + let providerStopped = 0 + let registryStopped = 0 + for (const [ptyId, owner] of targets) { + if (!stopped.has(ptyId)) { + continue + } + if (owner === 'provider') { + providerStopped += 1 + } else { + registryStopped += 1 } } - return killed + return { providerStopped, registryStopped } } function clearStoppedPtyState(ptyId: string, onPtyStopped?: (ptyId: string) => void): void { diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index c90e874800e..c0f9b722797 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -27,10 +27,18 @@ const { mockPtySpawn, mockPtyInstance } = vi.hoisted(() => ({ } })) +const { mockKillPosixPtySession } = vi.hoisted(() => ({ + mockKillPosixPtySession: vi.fn().mockResolvedValue(false) +})) + vi.mock('node-pty', () => ({ spawn: mockPtySpawn })) +vi.mock('./pty-session-kill', () => ({ + killPosixPtySession: mockKillPosixPtySession +})) + import { PtyHandler, attachIdentityMismatches } from './pty-handler' import type { RelayDispatcher } from './dispatcher' @@ -100,6 +108,8 @@ describe('PtyHandler', () => { mockPtyInstance.resize.mockReset() mockPtyInstance.kill.mockReset() mockPtyInstance.clear.mockReset() + mockKillPosixPtySession.mockReset() + mockKillPosixPtySession.mockResolvedValue(true) mockPtySpawn.mockReturnValue({ ...mockPtyInstance }) @@ -117,12 +127,14 @@ describe('PtyHandler', () => { expect(methods).toContain('pty.spawn') expect(methods).toContain('pty.attach') expect(methods).toContain('pty.shutdown') + expect(methods).toContain('pty.shutdownSession') expect(methods).toContain('pty.sendSignal') expect(methods).toContain('pty.getCwd') expect(methods).toContain('pty.getInitialCwd') expect(methods).toContain('pty.clearBuffer') expect(methods).toContain('pty.hasChildProcesses') expect(methods).toContain('pty.getForegroundProcess') + expect(methods).toContain('pty.hasPty') expect(methods).toContain('pty.listProcesses') expect(methods).toContain('pty.getDefaultShell') @@ -174,6 +186,13 @@ describe('PtyHandler', () => { expect(handler.activePtyCount).toBe(1) }) + it('answers targeted PTY liveness without building a process inventory', async () => { + await dispatcher.callRequest('pty.spawn', { cols: 80, rows: 24 }) + + await expect(dispatcher.callRequest('pty.hasPty', { id: 'pty-1' })).resolves.toBe(true) + await expect(dispatcher.callRequest('pty.hasPty', { id: 'missing' })).resolves.toBe(false) + }) + it('uses an explicit shell override and falls back to the default shell otherwise', async () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { @@ -727,7 +746,7 @@ describe('PtyHandler', () => { expect(handler.retainedStartupCommandCount).toBe(0) expect(term.write).not.toHaveBeenCalled() - expect(killSpy).toHaveBeenCalledWith('SIGKILL') + expect(killSpy).not.toHaveBeenCalledWith('SIGKILL') }) it('increments PTY ids on each spawn', async () => { @@ -1062,10 +1081,21 @@ describe('PtyHandler', () => { it('on immediate shutdown', async () => { await withWindowsPlatform(async () => { - const mockKill = mockKillablePty() + let onExitCb: ((evt: { exitCode: number }) => void) | undefined + const mockKill = vi.fn() + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + onData: vi.fn(), + onExit: vi.fn((cb: (evt: { exitCode: number }) => void) => { + onExitCb = cb + }) + }) await dispatcher.callRequest('pty.spawn', {}) - await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + const shutdown = dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) expectBareKills(mockKill, 1) + onExitCb!({ exitCode: 0 }) + await shutdown }) }) @@ -1120,7 +1150,7 @@ describe('PtyHandler', () => { id: 'pty-1', data: 'last words' }) - expect(mockKill).toHaveBeenCalledWith('SIGKILL') + expect(mockKill).not.toHaveBeenCalledWith('SIGKILL') }) it('notifies pty.exit when graceful shutdown falls back to SIGKILL', async () => { @@ -1149,7 +1179,7 @@ describe('PtyHandler', () => { expect(handler.activePtyCount).toBe(0) }) - it('kills PTY on shutdown with SIGKILL when immediate', async () => { + it('kills the full POSIX PTY session through the capability method', async () => { const mockKill = vi.fn() mockPtySpawn.mockReturnValue({ ...mockPtyInstance, @@ -1158,9 +1188,142 @@ describe('PtyHandler', () => { onExit: vi.fn() }) + await dispatcher.callRequest('pty.spawn', {}) + await dispatcher.callRequest('pty.shutdownSession', { id: 'pty-1' }) + expect(mockKillPosixPtySession).toHaveBeenCalledWith(process.pid, undefined) + expect(mockKill).not.toHaveBeenCalledWith('SIGKILL') + }) + + it('awaits Windows ConPTY exit after immediate shutdown without a signal argument', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + let onExitCb: ((evt: { exitCode: number }) => void) | undefined + const mockKill = vi.fn() + const mockDestroy = vi.fn() + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + destroy: mockDestroy, + onData: vi.fn(), + onExit: vi.fn((cb: (evt: { exitCode: number }) => void) => { + onExitCb = cb + }) + }) + try { + await dispatcher.callRequest('pty.spawn', {}) + const shutdown = dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + + expect(mockKill).toHaveBeenCalledWith() + expect(handler.activePtyCount).toBe(1) + onExitCb!({ exitCode: 0 }) + await expect(shutdown).resolves.toBeUndefined() + + expect(mockKill).toHaveBeenCalledTimes(1) + expect(mockDestroy).not.toHaveBeenCalled() + expect(handler.activePtyCount).toBe(0) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + + it('retains a Windows ConPTY owner when immediate exit times out', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const mockKill = vi.fn() + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + onData: vi.fn(), + onExit: vi.fn() + }) + try { + await dispatcher.callRequest('pty.spawn', {}) + const shutdown = dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + const rejection = expect(shutdown).rejects.toThrow( + 'Timed out waiting for Windows PTY teardown: pty-1' + ) + await vi.advanceTimersByTimeAsync(3_000) + + await rejection + expect(mockKill).toHaveBeenCalledTimes(1) + expect(handler.activePtyCount).toBe(1) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + + it('waits on retry without killing a Windows ConPTY twice', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + let onExitCb: ((evt: { exitCode: number }) => void) | undefined + const mockKill = vi.fn() + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + onData: vi.fn(), + onExit: vi.fn((cb: (evt: { exitCode: number }) => void) => { + onExitCb = cb + }) + }) + try { + await dispatcher.callRequest('pty.spawn', {}) + const first = dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + const firstRejection = expect(first).rejects.toThrow( + 'Timed out waiting for Windows PTY teardown: pty-1' + ) + await vi.advanceTimersByTimeAsync(3_000) + await firstRejection + + const retry = dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + expect(mockKill).toHaveBeenCalledTimes(1) + onExitCb!({ exitCode: 0 }) + await expect(retry).resolves.toBeUndefined() + expect(mockKill).toHaveBeenCalledTimes(1) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + + it('fails closed when full POSIX PTY session teardown cannot be verified', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) + const mockKill = vi.fn() + mockKillPosixPtySession.mockResolvedValue(false) + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + onData: vi.fn(), + onExit: vi.fn() + }) + + try { + await dispatcher.callRequest('pty.spawn', {}) + + await expect( + dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + ).rejects.toThrow('Unable to verify full PTY session teardown: pty-1') + expect(mockKill).not.toHaveBeenCalled() + expect(handler.activePtyCount).toBe(1) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + + it('does not signal a recycled root pid after the POSIX session kill succeeds', async () => { + const mockKill = vi.fn() + mockKillPosixPtySession.mockResolvedValue(true) + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + onData: vi.fn(), + onExit: vi.fn() + }) + await dispatcher.callRequest('pty.spawn', {}) await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) - expect(mockKill).toHaveBeenCalledWith('SIGKILL') + + expect(mockKillPosixPtySession).toHaveBeenCalledWith(process.pid, undefined) + expect(mockKill).not.toHaveBeenCalled() }) it('throws for attach on nonexistent PTY', async () => { @@ -1825,7 +1988,7 @@ describe('PtyHandler', () => { await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) onExitCb!({ exitCode: 0 }) - expect(mockKill).toHaveBeenCalledWith('SIGKILL') + expect(mockKill).not.toHaveBeenCalledWith('SIGKILL') expect(exits).toEqual([{ id: 'pty-1', paneKey: 'tab-shutdown:0' }]) expect(handler.activePtyCount).toBe(0) }) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index d7dd995005a..659db4139d5 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -24,6 +24,7 @@ import { scanForShellReady, type ShellReadyScanState } from '../main/shell-ready-marker-scanner' +import { killPosixPtySession } from './pty-session-kill' // Why: node-pty is a native addon that may not be installed on the remote. // Dynamic import keeps the require() lazy so loadPty() returns null gracefully @@ -49,6 +50,8 @@ type ManagedPty = { buffered: string /** Timer for SIGKILL fallback after a graceful SIGTERM shutdown. */ killTimer?: ReturnType + /** Windows ConPTY kill has no signal argument and completes asynchronously. */ + windowsImmediateKillIssued?: boolean /** True once disposeManagedPty has run. Prevents double-dispose (onExit + an * explicit shutdown can both fire for the same PTY) and converts post-dispose * entry-point calls into a clean "not found" error instead of a silent no-op @@ -120,13 +123,16 @@ function disposeManagedPty(managed: ManagedPty): void { if (process.platform !== 'win32') { ;(managed.pty as unknown as { kill: (sig?: string) => void }).kill = () => {} } - try { - ;(managed.pty as unknown as { destroy?: () => void }).destroy?.() - } catch { - /* swallow */ + if (!managed.windowsImmediateKillIssued) { + try { + ;(managed.pty as unknown as { destroy?: () => void }).destroy?.() + } catch { + /* swallow */ + } } } const DEFAULT_GRACE_TIME_MS = DEFAULT_SSH_RELAY_GRACE_PERIOD_SECONDS * 1000 +const WINDOWS_IMMEDIATE_EXIT_TIMEOUT_MS = 3_000 export const REPLAY_BUFFER_MAX = 100 * 1024 const PTY_OUTPUT_BATCH_INTERVAL_MS = 8 const PTY_OUTPUT_DRAIN_CONTINUE_MS = 1 @@ -249,6 +255,7 @@ export class PtyHandler { private pendingOutputByPty = new Map() private lastInputAtByPty = new Map() private interactiveOutputCharsByPty = new Map() + private exitWaitersByPty = new Map void>>() // Why: external observers need to drop per-pane state when a PTY exits. // Today the relay composes multiple consumers (hook-server cache eviction // and plugin-overlay dir cleanup) into a single callback at the call site @@ -425,6 +432,7 @@ export class PtyHandler { this.enqueuePtyOutput(managed.id, data) }) managed.pty.onExit(({ exitCode }: { exitCode: number }) => { + this.resolveExitWaiters(managed.id) if (managed.disposed) { return } @@ -458,6 +466,61 @@ export class PtyHandler { }) } + private waitForExit(id: string): { promise: Promise; cancel: () => void } { + let settled = false + let resolvePromise = (_exited: boolean): void => {} + const waiter = (): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + resolvePromise(true) + } + const waiters = this.exitWaitersByPty.get(id) ?? new Set<() => void>() + waiters.add(waiter) + this.exitWaitersByPty.set(id, waiters) + const timer = setTimeout(() => { + if (settled) { + return + } + settled = true + waiters.delete(waiter) + if (waiters.size === 0) { + this.exitWaitersByPty.delete(id) + } + resolvePromise(false) + }, WINDOWS_IMMEDIATE_EXIT_TIMEOUT_MS) + const promise = new Promise((resolve) => { + resolvePromise = resolve + }) + return { + promise, + cancel: () => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + waiters.delete(waiter) + if (waiters.size === 0) { + this.exitWaitersByPty.delete(id) + } + } + } + } + + private resolveExitWaiters(id: string): void { + const waiters = this.exitWaitersByPty.get(id) + if (!waiters) { + return + } + this.exitWaitersByPty.delete(id) + for (const resolve of waiters) { + resolve() + } + } + private notifyExitListener(managed: ManagedPty): void { if (managed.exitListenerNotified) { return @@ -482,12 +545,18 @@ export class PtyHandler { this.dispatcher.onRequest('pty.spawn', (p, context) => this.spawn(p, context)) this.dispatcher.onRequest('pty.attach', (p) => this.attach(p)) this.dispatcher.onRequest('pty.shutdown', (p) => this.shutdown(p)) + // Why: method presence proves the relay can kill and verify the whole PTY + // process tree. Older relays only removed the forkpty leader/map entry. + this.dispatcher.onRequest('pty.shutdownSession', (p) => + this.shutdown({ ...p, immediate: true }) + ) this.dispatcher.onRequest('pty.sendSignal', (p) => this.sendSignal(p)) this.dispatcher.onRequest('pty.getCwd', (p) => this.getCwd(p)) this.dispatcher.onRequest('pty.getInitialCwd', (p) => this.getInitialCwd(p)) this.dispatcher.onRequest('pty.clearBuffer', (p) => this.clearBuffer(p)) this.dispatcher.onRequest('pty.hasChildProcesses', (p) => this.hasChildProcesses(p)) this.dispatcher.onRequest('pty.getForegroundProcess', (p) => this.getForegroundProcess(p)) + this.dispatcher.onRequest('pty.hasPty', (p) => this.hasPty(p)) this.dispatcher.onRequest('pty.listProcesses', () => this.listProcesses()) this.dispatcher.onRequest('pty.getDefaultShell', async () => resolveDefaultShell()) this.dispatcher.onRequest('pty.serialize', (p) => this.serialize(p)) @@ -851,9 +920,39 @@ export class PtyHandler { if (immediate) { this.releaseStartupCommand(managed) this.flushPtyOutput(id) - killPtyProcess(managed.pty, '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 + if (process.platform === 'win32') { + // Why: WindowsTerminal rejects signal arguments and ConPTY teardown is + // asynchronous. Git removal must wait for the native exit event. + const exit = this.waitForExit(id) + if (!managed.windowsImmediateKillIssued) { + managed.windowsImmediateKillIssued = true + try { + killPtyProcess(managed.pty, 'SIGKILL') + } catch (error) { + managed.windowsImmediateKillIssued = false + exit.cancel() + throw error + } + } + if (!(await exit.promise)) { + throw new Error(`Timed out waiting for Windows PTY teardown: ${id}`) + } + return + } + const killedSession = await killPosixPtySession( + managed.pty.pid, + (managed.pty as unknown as { ptsName?: unknown }).ptsName + ) + if (!killedSession && !managed.disposed) { + // Why: killing only the forkpty leader can strand HUP-resistant job + // groups in a deleted cwd. Fail closed so worktree removal is aborted. + throw new Error(`Unable to verify full PTY session teardown: ${id}`) + } + if (managed.disposed) { + return + } + // Why: SIGKILL has been delivered to the session; 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) @@ -966,6 +1065,11 @@ export class PtyHandler { return await getForegroundProcessName(managed.pty.pid, managed.pty.process || null) } + private async hasPty(params: Record): Promise { + const managed = this.ptys.get(params.id as string) + return managed !== undefined && !managed.disposed + } + private async listProcesses(): Promise { const results: PtyProcessSummary[] = [] for (const [id, managed] of this.ptys) { @@ -1139,6 +1243,7 @@ export class PtyHandler { this.lastInputAtByPty.clear() this.interactiveOutputCharsByPty.clear() for (const [, managed] of this.ptys) { + this.resolveExitWaiters(managed.id) if (managed.killTimer) { clearTimeout(managed.killTimer) managed.killTimer = undefined diff --git a/src/relay/pty-session-kill.test.ts b/src/relay/pty-session-kill.test.ts new file mode 100644 index 00000000000..b55c668c96b --- /dev/null +++ b/src/relay/pty-session-kill.test.ts @@ -0,0 +1,331 @@ +import { describe, expect, it, vi } from 'vitest' +import { + killPosixPtySession, + PTY_SESSION_COMMAND_TIMEOUT_MS, + PTY_SESSION_VERIFY_TIMEOUT_MS +} from './pty-session-kill' + +function emptySelection(): Error & { code: number; stdout: string } { + return Object.assign(new Error('no processes'), { code: 1, stdout: '' }) +} + +describe('killPosixPtySession', () => { + it('freezes and kills a Linux descendant that escaped into a new session', async () => { + const run = vi.fn(async (file: string, args: string[]) => { + if (file === 'ps' && args.includes('sid=')) { + return { stdout: '4242 pts/7\n' } + } + if (file === 'pgrep') { + const parents = new Set(args[1]?.split(',')) + if (parents.has('4242')) { + return { stdout: '4243\n4244\n' } + } + if (parents.has('4244')) { + return { stdout: '4245\n' } + } + throw emptySelection() + } + if (file === 'ps' && args.includes('ppid=')) { + const parents = new Map([ + [4243, 4242], + [4244, 4242], + [4245, 4244] + ]) + const selected = args[1]!.split(',').map(Number) + return { stdout: selected.map((child) => `${child} ${parents.get(child)}`).join('\n') } + } + if (file === 'ps' && args.includes('stat=')) { + return { stdout: '4242 Z\n4243 Z\n4244 Z\n4245 Z\n' } + } + throw new Error(`Unexpected command: ${file} ${args.join(' ')}`) + }) + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + true + ) + + expect(killProcess.mock.calls).toEqual([ + [4242, 'SIGSTOP'], + [4243, 'SIGSTOP'], + [4244, 'SIGSTOP'], + [4245, 'SIGSTOP'], + [4245, 'SIGKILL'], + [4244, 'SIGKILL'], + [4243, 'SIGKILL'], + [4242, 'SIGKILL'] + ]) + expect(run).not.toHaveBeenCalledWith('ps', expect.arrayContaining(['-e']), expect.anything()) + expect(run.mock.calls.filter(([file]) => file === 'pgrep')).toHaveLength(3) + }) + + it('validates Darwin root ownership before targeted descendant teardown', async () => { + const run = vi.fn(async (file: string, args: string[]) => { + if (file === 'ps' && args.includes('pgid=')) { + return { stdout: '4242 ttys042\n' } + } + if (file === 'pgrep') { + if (args[1] === '4242') { + return { stdout: '4243\n' } + } + throw emptySelection() + } + if (file === 'ps' && args.includes('ppid=')) { + return { stdout: '4243 4242\n' } + } + if (file === 'ps' && args.includes('stat=')) { + throw emptySelection() + } + throw new Error(`Unexpected command: ${file} ${args.join(' ')}`) + }) + const killProcess = vi.fn() + + await expect( + killPosixPtySession(4242, '/dev/ttys042', 'darwin', run, killProcess) + ).resolves.toBe(true) + expect(run).toHaveBeenCalledTimes(6) + expect(run).toHaveBeenNthCalledWith(1, 'ps', ['-p', '4242', '-o', 'pgid=', '-o', 'tty='], { + timeout: PTY_SESSION_COMMAND_TIMEOUT_MS + }) + expect(run).toHaveBeenNthCalledWith(2, 'ps', ['-p', '4242', '-o', 'pgid=', '-o', 'tty='], { + timeout: PTY_SESSION_COMMAND_TIMEOUT_MS + }) + expect(run).toHaveBeenLastCalledWith('ps', ['-p', '4242,4243', '-o', 'pid=', '-o', 'stat='], { + timeout: expect.any(Number) + }) + expect(killProcess.mock.calls).toEqual([ + [4242, 'SIGSTOP'], + [4243, 'SIGSTOP'], + [4243, 'SIGKILL'], + [4242, 'SIGKILL'] + ]) + }) + + it('leaves Windows ConPTY teardown to node-pty', async () => { + const run = vi.fn() + + await expect(killPosixPtySession(4242, undefined, 'win32', run)).resolves.toBe(false) + expect(run).not.toHaveBeenCalled() + }) + + it('does not signal a root whose session identity no longer matches', async () => { + const run = vi.fn().mockResolvedValue({ stdout: '9999 pts/7\n' }) + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + false + ) + expect(killProcess).not.toHaveBeenCalled() + }) + + it('does not signal a reused Linux session leader on a different TTY', async () => { + const run = vi.fn().mockResolvedValue({ stdout: '4242 pts/99\n' }) + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + false + ) + expect(killProcess).not.toHaveBeenCalled() + }) + + it('fails closed when node-pty cannot provide the exact controlling TTY', async () => { + const run = vi.fn() + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, undefined, 'linux', run, killProcess)).resolves.toBe( + false + ) + expect(run).not.toHaveBeenCalled() + expect(killProcess).not.toHaveBeenCalled() + }) + + it('resumes the root when its ownership changes after SIGSTOP', async () => { + const run = vi + .fn() + .mockResolvedValueOnce({ stdout: '4242 pts/7\n' }) + .mockResolvedValueOnce({ stdout: '9999 pts/7\n' }) + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + false + ) + expect(killProcess.mock.calls).toEqual([ + [4242, 'SIGSTOP'], + [4242, 'SIGCONT'] + ]) + }) + + it('resumes the frozen root when a discovered child exits before it can be frozen', async () => { + const run = vi + .fn() + .mockResolvedValueOnce({ stdout: '4242 pts/7\n' }) + .mockResolvedValueOnce({ stdout: '4242 pts/7\n' }) + .mockResolvedValueOnce({ stdout: '4243\n' }) + const killProcess = vi.fn((pid: number, signal: NodeJS.Signals) => { + if (pid === 4243 && signal === 'SIGSTOP') { + throw Object.assign(new Error('gone'), { code: 'ESRCH' }) + } + }) + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + false + ) + expect(killProcess.mock.calls).toEqual([ + [4242, 'SIGSTOP'], + [4243, 'SIGSTOP'], + [4242, 'SIGCONT'] + ]) + }) + + it('resumes frozen processes when descendant discovery fails', async () => { + const run = vi + .fn() + .mockResolvedValueOnce({ stdout: '4242 pts/7\n' }) + .mockResolvedValueOnce({ stdout: '4242 pts/7\n' }) + .mockRejectedValueOnce(new Error('pgrep unavailable')) + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + false + ) + expect(killProcess.mock.calls).toEqual([ + [4242, 'SIGSTOP'], + [4242, 'SIGCONT'] + ]) + }) + + it('fails closed when a captured process remains runnable after SIGKILL', async () => { + const run = vi.fn(async (file: string, args: string[]) => { + if (file === 'pgrep') { + throw emptySelection() + } + if (file === 'ps' && args.includes('sid=')) { + return { stdout: '4242 pts/7\n' } + } + return { stdout: '4242 D\n' } + }) + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + false + ) + }) + + it('uses bounded targeted command timeouts for ownership, children, and verification', async () => { + const run = vi.fn(async (file: string, args?: string[], _options?: { timeout: number }) => { + if (file === 'pgrep') { + throw emptySelection() + } + if (args?.includes('sid=')) { + return { stdout: '4242 pts/7\n' } + } + throw emptySelection() + }) + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, vi.fn())).resolves.toBe(true) + expect(run).toHaveBeenNthCalledWith(1, 'ps', ['-p', '4242', '-o', 'sid=', '-o', 'tty='], { + timeout: PTY_SESSION_COMMAND_TIMEOUT_MS + }) + expect(run).toHaveBeenNthCalledWith(2, 'ps', ['-p', '4242', '-o', 'sid=', '-o', 'tty='], { + timeout: PTY_SESSION_COMMAND_TIMEOUT_MS + }) + expect(run).toHaveBeenNthCalledWith(3, 'pgrep', ['-P', '4242'], { + timeout: expect.any(Number) + }) + expect(run.mock.calls[2]?.[2]?.timeout).toBeLessThanOrEqual(PTY_SESSION_COMMAND_TIMEOUT_MS) + expect(run).toHaveBeenNthCalledWith(4, 'ps', ['-p', '4242', '-o', 'pid=', '-o', 'stat='], { + timeout: expect.any(Number) + }) + expect(run.mock.calls[3]?.[2]?.timeout).toBeLessThanOrEqual(PTY_SESSION_VERIFY_TIMEOUT_MS) + }) + + it('resumes every frozen process when a stopped child was reparented', async () => { + const run = vi.fn(async (file: string, args: string[]) => { + if (file === 'ps' && args.includes('sid=')) { + return { stdout: '4242 pts/7\n' } + } + if (file === 'pgrep') { + return { stdout: '4243\n' } + } + if (file === 'ps' && args.includes('ppid=')) { + return { stdout: '4243 9999\n' } + } + throw new Error(`Unexpected command: ${file} ${args.join(' ')}`) + }) + const killProcess = vi.fn() + + await expect(killPosixPtySession(4242, '/dev/pts/7', 'linux', run, killProcess)).resolves.toBe( + false + ) + expect(killProcess.mock.calls).toEqual([ + [4242, 'SIGSTOP'], + [4243, 'SIGSTOP'], + [4242, 'SIGCONT'], + [4243, 'SIGCONT'] + ]) + }) + + it('batches large Darwin trees under one final verification deadline', async () => { + const rootPid = 5000 + const childPids = Array.from({ length: 129 }, (_, index) => rootPid + index + 1) + let now = 10_000 + const dateNow = vi.spyOn(Date, 'now').mockImplementation(() => { + const current = now + now += 5 + return current + }) + const run = vi.fn(async (file: string, args: string[], _options?: { timeout: number }) => { + if (file === 'ps' && args.includes('pgid=')) { + return { stdout: `${rootPid} ttys042\n` } + } + if (file === 'pgrep') { + if (args[1] === String(rootPid)) { + return { stdout: childPids.join('\n') } + } + throw emptySelection() + } + if (file === 'ps' && args.includes('ppid=')) { + return { + stdout: args[1]! + .split(',') + .map((pid) => `${pid} ${rootPid}`) + .join('\n') + } + } + if (file === 'ps' && args.includes('stat=')) { + return { + stdout: args[1]! + .split(',') + .map((pid) => `${pid} Z`) + .join('\n') + } + } + throw new Error(`Unexpected command: ${file} ${args.join(' ')}`) + }) + + try { + await expect( + killPosixPtySession(rootPid, '/dev/ttys042', 'darwin', run, vi.fn()) + ).resolves.toBe(true) + } finally { + dateNow.mockRestore() + } + + const parentChecks = run.mock.calls.filter( + ([file, args]) => file === 'ps' && args.includes('ppid=') + ) + const finalChecks = run.mock.calls.filter( + ([file, args]) => file === 'ps' && args.includes('stat=') + ) + expect(parentChecks).toHaveLength(3) + expect(finalChecks).toHaveLength(3) + for (const [, args, options] of [...parentChecks, ...finalChecks]) { + expect(args[1]!.split(',').length).toBeLessThanOrEqual(64) + expect(options!.timeout).toBeGreaterThan(0) + expect(options!.timeout).toBeLessThanOrEqual(PTY_SESSION_COMMAND_TIMEOUT_MS) + } + const finalTimeouts = finalChecks.map(([, , options]) => options!.timeout) + expect(finalTimeouts.at(-1)).toBeLessThan(finalTimeouts[0]) + expect(finalTimeouts[0]).toBeLessThanOrEqual(PTY_SESSION_VERIFY_TIMEOUT_MS) + }) +}) diff --git a/src/relay/pty-session-kill.ts b/src/relay/pty-session-kill.ts new file mode 100644 index 00000000000..286c578d09c --- /dev/null +++ b/src/relay/pty-session-kill.ts @@ -0,0 +1,263 @@ +import { execFile as execFileCallback } from 'node:child_process' +import { promisify } from 'node:util' + +type ExecFile = (file: string, args: string[], options: { timeout: number }) => Promise +type KillProcess = (pid: number, signal: NodeJS.Signals) => void + +const execFile = promisify(execFileCallback) as ExecFile +export const PTY_SESSION_COMMAND_TIMEOUT_MS = 2500 +export const PTY_SESSION_VERIFY_TIMEOUT_MS = 500 +export const MAX_PTY_PROCESS_TREE_SIZE = 1024 +const VERIFY_PID_BATCH_SIZE = 64 +const PARENT_PID_BATCH_SIZE = 64 + +export async function killPosixPtySession( + pid: number, + ptsName: unknown, + platform: NodeJS.Platform = process.platform, + run: ExecFile = execFile, + killProcess: KillProcess = process.kill +): Promise { + if ((platform !== 'linux' && platform !== 'darwin') || !Number.isSafeInteger(pid) || pid <= 0) { + return false + } + + const stopped = new Set() + try { + if (!(await rootStillOwnsPty(pid, ptsName, platform, run))) { + return false + } + if (!stopProcess(pid, stopped, killProcess)) { + return false + } + // Why: the PID can exit and be reused between the ownership probe and + // SIGSTOP. Re-prove the frozen root before discovering or signaling children. + if (!(await rootStillOwnsPty(pid, ptsName, platform, run))) { + resumeProcesses(stopped, killProcess) + return false + } + const processTree = await freezeProcessTree(pid, stopped, run, killProcess) + if (!processTree) { + resumeProcesses(stopped, killProcess) + return false + } + // Why: remote relays support Node 18, which predates Array#toReversed. + for (let index = processTree.length - 1; index >= 0; index -= 1) { + const candidate = processTree[index]! + if (!signalProcess(candidate, 'SIGKILL', killProcess)) { + resumeProcesses(stopped, killProcess) + return false + } + stopped.delete(candidate) + } + return await verifyProcessesStopped(processTree, run) + } catch { + resumeProcesses(stopped, killProcess) + return false + } +} + +async function rootStillOwnsPty( + pid: number, + ptsName: unknown, + platform: NodeJS.Platform, + run: ExecFile +): Promise { + if (typeof ptsName !== 'string') { + // Why: without the exact controlling TTY, a recycled session-leader PID + // is not enough authority to signal an unrelated user's process tree. + return false + } + const ownerColumn = platform === 'darwin' ? 'pgid=' : 'sid=' + const result = (await run('ps', ['-p', String(pid), '-o', ownerColumn, '-o', 'tty='], { + timeout: PTY_SESSION_COMMAND_TIMEOUT_MS + })) as { stdout?: string | Buffer } + const [ownerText, tty = ''] = String(result.stdout ?? '') + .trim() + .split(/\s+/) + if (Number(ownerText) !== pid) { + return false + } + const expectedTty = ptsName.startsWith('/dev/') ? ptsName.slice('/dev/'.length) : ptsName + return /^[A-Za-z0-9._/-]+$/.test(expectedTty) && tty === expectedTty +} + +async function freezeProcessTree( + rootPid: number, + stopped: Set, + run: ExecFile, + killProcess: KillProcess +): Promise { + const processTree = [rootPid] + const seen = new Set(processTree) + let frontier = [rootPid] + const discoveryDeadline = Date.now() + PTY_SESSION_COMMAND_TIMEOUT_MS + while (frontier.length > 0) { + const nextFrontier: number[] = [] + for (let index = 0; index < frontier.length; index += PARENT_PID_BATCH_SIZE) { + const remainingMs = discoveryDeadline - Date.now() + if (remainingMs <= 0) { + return null + } + const parents = frontier.slice(index, index + PARENT_PID_BATCH_SIZE) + const children = await listChildren(parents, run, remainingMs) + const newlyStopped: number[] = [] + for (const child of children) { + if (seen.has(child)) { + continue + } + if (seen.size >= MAX_PTY_PROCESS_TREE_SIZE) { + return null + } + seen.add(child) + if (!stopProcess(child, stopped, killProcess)) { + return null + } + newlyStopped.push(child) + } + const childVerificationRemainingMs = discoveryDeadline - Date.now() + if ( + newlyStopped.length > 0 && + (childVerificationRemainingMs <= 0 || + !(await frozenChildrenStillBelongToParents( + newlyStopped, + parents, + run, + childVerificationRemainingMs + ))) + ) { + return null + } + for (const child of newlyStopped) { + processTree.push(child) + nextFrontier.push(child) + } + } + frontier = nextFrontier + } + return processTree +} + +async function listChildren( + parentPids: readonly number[], + run: ExecFile, + timeout: number +): Promise { + try { + const result = (await run('pgrep', ['-P', parentPids.join(',')], { + timeout + })) as { stdout?: string | Buffer } + return String(result.stdout ?? '') + .trim() + .split(/\s+/) + .map(Number) + .filter((candidate) => Number.isSafeInteger(candidate) && candidate > 0) + } catch (error) { + if (isEmptyProcessSelection(error)) { + return [] + } + throw error + } +} + +async function frozenChildrenStillBelongToParents( + childPids: readonly number[], + parentPids: readonly number[], + run: ExecFile, + timeout: number +): Promise { + const expectedParents = new Set(parentPids) + const verifiedChildren = new Set() + const verificationDeadline = Date.now() + timeout + for (let index = 0; index < childPids.length; index += VERIFY_PID_BATCH_SIZE) { + const remainingMs = verificationDeadline - Date.now() + if (remainingMs <= 0) { + return false + } + const batch = childPids.slice(index, index + VERIFY_PID_BATCH_SIZE) + const result = (await run('ps', ['-p', batch.join(','), '-o', 'pid=', '-o', 'ppid='], { + timeout: remainingMs + })) as { stdout?: string | Buffer } + for (const line of String(result.stdout ?? '') + .trim() + .split('\n')) { + const [pidText, parentPidText] = line.trim().split(/\s+/) + const pid = Number(pidText) + const parentPid = Number(parentPidText) + if (!batch.includes(pid) || !expectedParents.has(parentPid) || verifiedChildren.has(pid)) { + return false + } + verifiedChildren.add(pid) + } + } + return verifiedChildren.size === childPids.length +} + +function stopProcess(pid: number, stopped: Set, killProcess: KillProcess): boolean { + try { + killProcess(pid, 'SIGSTOP') + stopped.add(pid) + return true + } catch { + return false + } +} + +function signalProcess(pid: number, signal: NodeJS.Signals, killProcess: KillProcess): boolean { + try { + killProcess(pid, signal) + return true + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'ESRCH' + } +} + +function resumeProcesses(stopped: Set, killProcess: KillProcess): void { + for (const pid of stopped) { + signalProcess(pid, 'SIGCONT', killProcess) + } + stopped.clear() +} + +async function verifyProcessesStopped(pids: number[], run: ExecFile): Promise { + const verificationDeadline = Date.now() + PTY_SESSION_VERIFY_TIMEOUT_MS + for (let index = 0; index < pids.length; index += VERIFY_PID_BATCH_SIZE) { + const remainingMs = verificationDeadline - Date.now() + if (remainingMs <= 0) { + return false + } + const batch = pids.slice(index, index + VERIFY_PID_BATCH_SIZE) + try { + const result = (await run('ps', ['-p', batch.join(','), '-o', 'pid=', '-o', 'stat='], { + timeout: remainingMs + })) as { stdout?: string | Buffer } + if (!hasOnlyExitedProcesses(result.stdout)) { + return false + } + } catch (error) { + if (!isEmptyProcessSelection(error)) { + return false + } + } + } + return true +} + +function hasOnlyExitedProcesses(stdout: string | Buffer | undefined): boolean { + return String(stdout ?? '') + .trim() + .split('\n') + .map((line) => line.trim()) + .filter(Boolean) + .every((line) => line.split(/\s+/).at(-1)?.startsWith('Z') === true) +} + +function isEmptyProcessSelection(error: unknown): boolean { + if (!error || typeof error !== 'object') { + return false + } + const processError = error as { code?: unknown; stdout?: unknown } + // Why: BSD/procps ps/pgrep exit 1 for an empty selector. Only that exact + // empty result proves absence; ENOENT, timeout, and malformed output fail closed. + return processError.code === 1 && String(processError.stdout ?? '').trim() === '' +} diff --git a/tests/e2e/terminal-push-delivery-loss-recovery.spec.ts b/tests/e2e/terminal-push-delivery-loss-recovery.spec.ts index 395353cbe48..9d3b4914147 100644 --- a/tests/e2e/terminal-push-delivery-loss-recovery.spec.ts +++ b/tests/e2e/terminal-push-delivery-loss-recovery.spec.ts @@ -95,8 +95,8 @@ test.describe('terminal push-delivery loss recovery', () => { async () => orcaPage.evaluate( () => - (window as DeliveryWatchdogWindow).__terminalDeliveryWatchdog?.snapshot()?.healCount ?? - 0 + (window as DeliveryWatchdogWindow).__terminalDeliveryWatchdog?.snapshot() + ?.healCount ?? 0 ), { timeout: 30_000 } )