diff --git a/src/main/daemon/daemon-file-log.test.ts b/src/main/daemon/daemon-file-log.test.ts index 90677b9dab8..77e6557cf2d 100644 --- a/src/main/daemon/daemon-file-log.test.ts +++ b/src/main/daemon/daemon-file-log.test.ts @@ -24,12 +24,12 @@ describe('createDaemonFileLog', () => { it('appends NDJSON lines with src/ts/pid/event and terse details', () => { const filePath = join(dir, 'daemon.log') const log = createDaemonFileLog(filePath) - log.log('startup', { protocolVersion: 18 }) + log.log('startup', { protocolVersion: 19 }) log.log('session-created', { sessionId: 'abc', pid: 42 }) const lines = readLines(filePath) expect(lines).toHaveLength(2) - expect(lines[0]).toMatchObject({ src: 'daemon', event: 'startup', protocolVersion: 18 }) + expect(lines[0]).toMatchObject({ src: 'daemon', event: 'startup', protocolVersion: 19 }) expect(typeof lines[0].ts).toBe('string') expect(lines[0].pid).toBe(process.pid) expect(lines[1]).toMatchObject({ event: 'session-created', sessionId: 'abc', pid: 42 }) diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index af7756e45e7..2c98f32d06d 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -936,6 +936,11 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { expect.objectContaining({ snapshotAnsi: expect.stringContaining('fresh output') }) ) expect(existsSync(join(historyDir, getHistorySessionDirName(id)))).toBe(true) + historyAdapter.dispose() + const meta = JSON.parse( + readFileSync(join(historyDir, getHistorySessionDirName(id), 'meta.json'), 'utf-8') + ) + expect(meta.endedAt).toBeNull() }) it('persists final take records that are not represented in the snapshot', async () => { @@ -959,6 +964,127 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { ]) }) + it('preserves output emitted after graceful keepHistory shutdown starts', async () => { + historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + + const { id } = await historyAdapter.spawn({ + cols: 80, + rows: 24, + cwd: '/home/user', + sessionId: 'sleep-graceful-tail' + }) + const exits: { id: string; code: number }[] = [] + historyAdapter.onExit((payload) => exits.push(payload)) + + lastSubprocess._simulateData('fresh output before graceful sleep\r\n') + vi.mocked(lastSubprocess.kill).mockImplementation(() => { + lastSubprocess._simulateData('tail after graceful signal\r\n') + setTimeout(() => lastSubprocess._simulateExit(0), 5) + }) + + await historyAdapter.shutdown(id, { immediate: false, keepHistory: true }) + await waitFor(() => exits.some((exit) => exit.id === id && exit.code === 0)) + + const meta = JSON.parse( + readFileSync(join(historyDir, getHistorySessionDirName(id), 'meta.json'), 'utf-8') + ) + expect(meta.endedAt).toBeNull() + + const result = await historyAdapter.spawn({ + cols: 80, + rows: 24, + cwd: '/home/user', + sessionId: id + }) + expect(result.coldRestore?.scrollback).toContain('tail after graceful signal') + }) + + it('does not preserve history for a respawned id after a leaked pending-exit flag', async () => { + historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + + const { id } = await historyAdapter.spawn({ + cols: 80, + rows: 24, + cwd: '/home/user', + sessionId: 'stale-flag-reuse' + }) + const exits: { id: string; code: number }[] = [] + historyAdapter.onExit((payload) => exits.push(payload)) + + // Session dies on its own; the exit is fully processed. + lastSubprocess._simulateExit(0) + await waitFor(() => exits.length === 1) + + // Hibernate races the death: the kill RPC hits a dead session and + // throws, deliberately leaving the pending-exit flag set for the + // possibly-in-flight exit event. + await expect( + historyAdapter.shutdown(id, { immediate: false, keepHistory: true }) + ).rejects.toThrow(/Session not found/i) + + // The renderer respawns the same persisted ptyId; its eventual real + // exit must take the normal closeSession path (endedAt set), not the + // preserve-history path the stale flag would trigger. + await historyAdapter.spawn({ cols: 80, rows: 24, cwd: '/home/user', sessionId: id }) + lastSubprocess._simulateExit(0) + await waitFor(() => exits.length === 2) + await waitFor(() => { + const meta = JSON.parse( + readFileSync(join(historyDir, getHistorySessionDirName(id), 'meta.json'), 'utf-8') + ) + return meta.endedAt !== null + }) + }) + + it('keeps a mid-persist exit from detaching a freshly respawned session', async () => { + historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + + const { id } = await historyAdapter.spawn({ + cols: 80, + rows: 24, + cwd: '/home/user', + sessionId: 'wake-race' + }) + lastSubprocess._simulateData('output before sleep\r\n') + vi.mocked(lastSubprocess.kill).mockImplementation(() => { + setTimeout(() => lastSubprocess._simulateExit(0), 5) + }) + + await historyAdapter.shutdown(id, { immediate: false, keepHistory: true }) + + // Park the exit handler inside persistFinalExitHistory's disk write. + const historyManager = historyAdapter.getHistoryManager()! + const originalCheckpoint = historyManager.checkpoint.bind(historyManager) + let releaseCheckpoint!: () => void + const checkpointGate = new Promise((resolve) => { + releaseCheckpoint = resolve + }) + const checkpointSpy = vi + .spyOn(historyManager, 'checkpoint') + .mockImplementation(async (sessionId, snapshot) => { + await checkpointGate + return originalCheckpoint(sessionId, snapshot) + }) + await waitFor(() => checkpointSpy.mock.calls.length > 0) + + // Wake respawns the same id while the persist is still in flight. + const result = await historyAdapter.spawn({ + cols: 80, + rows: 24, + cwd: '/home/user', + sessionId: id + }) + expect(result.coldRestore?.scrollback).toContain('output before sleep') + + releaseCheckpoint() + await new Promise((r) => setTimeout(r, 50)) + + // Why: the resumed handler must not tear down the woken generation — + // hasPty flipping false here means checkpoints silently stopped. + expect(historyAdapter.hasPty(id)).toBe(true) + checkpointSpy.mockRestore() + }) + it('returns cold restore data when disk history has unclean shutdown', async () => { // Simulate a previous daemon crash: write history files without endedAt const sessionId = 'cold-restore-test' diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index 648870abe1c..084900ee657 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -87,6 +87,15 @@ export class DaemonPtyAdapter implements IPtyProvider { private coldRestoreCache = new Map() private activeSessionIds = new Set() private dirtySessionVersions = new Map() + // Why: keepHistory graceful shutdowns must leave meta.endedAt null, but only + // after the real exit event has delivered any final daemon-side output. + private keepHistoryShutdownsPendingExit = new Set() + // Why: handleExitEvent awaits disk writes; a wake respawn of the same + // sessionId can land inside that window. The epoch (globally monotonic per + // spawn) lets the resumed handler detect the newer generation and leave its + // writer/tracking alone instead of tearing down the live woken session. + private spawnEpochCounter = 0 + private sessionSpawnEpochs = new Map() // Why: a cold-restored session is a fresh shell whose on-disk checkpoint and // log belong to the pre-crash session. Incremental appends would land on // that stale log (and be rejected by its sequence check on restore), so the @@ -242,6 +251,12 @@ export class DaemonPtyAdapter implements IPtyProvider { const wasAlreadyManaged = this.activeSessionIds.has(sessionId) this.activeSessionIds.add(sessionId) + this.spawnEpochCounter += 1 + this.sessionSpawnEpochs.set(sessionId, this.spawnEpochCounter) + // Why: a leaked pending-exit flag (kill RPC raced an already-dead session) + // must not make this new generation's eventual real exit skip closeSession + // — that would leave endedAt null and trigger a bogus cold restore later. + this.keepHistoryShutdownsPendingExit.delete(sessionId) // Cold restore: daemon created a new session but disk history shows // an unclean shutdown → return saved scrollback so the renderer can @@ -356,18 +371,28 @@ export class DaemonPtyAdapter implements IPtyProvider { if (opts.keepHistory) { await this.checkpointSessions([id], { final: true, teardown: true }) } - await this.client.request('kill', { sessionId: id, immediate: opts.immediate ?? false }) - this.activeSessionIds.delete(id) - this.dirtySessionVersions.delete(id) - this.coldRestoreCache.delete(id) - // Why: the !keepHistory close path doesn't take a final checkpoint, so a - // session stranded in sessionsNeedingFullCheckpoint would never be cleared. - // (Under keepHistory the final checkpoint above already cleared the flag, so - // this is a harmless no-op there — kept unconditional to cover both paths.) - this.sessionsNeedingFullCheckpoint.delete(id) - this.lastFullCheckpointAt.delete(id) - this.stopCheckpointTimerIfIdle() - this.initialCwds.delete(id) + const preserveHistoryOnExit = opts.keepHistory === true && opts.immediate !== true + if (preserveHistoryOnExit) { + this.keepHistoryShutdownsPendingExit.add(id) + } + try { + await this.client.request('kill', { + sessionId: id, + immediate: opts.immediate ?? false, + ...(preserveHistoryOnExit && this.protocolVersion >= 19 ? { collectFinalOutput: true } : {}) + }) + } catch (err) { + if (preserveHistoryOnExit && !isSessionNotFoundError(err)) { + this.keepHistoryShutdownsPendingExit.delete(id) + } + throw err + } + + if (preserveHistoryOnExit) { + return + } + + this.clearSessionTracking(id) // Why: history removal is for the "user explicitly closed this terminal" // path. Sleep also calls shutdown but expects scrollback to survive — wake // re-spawns and the cold-restore reader needs the dir intact. Caller @@ -376,6 +401,8 @@ export class DaemonPtyAdapter implements IPtyProvider { void this.historyManager .removeSession(id) .catch((err) => console.warn('[history] removeSession failed:', id, err)) + } else if (this.historyManager) { + this.historyManager.detachSessionWriter(id) } // Why: tombstone rejects reattach against a session the user explicitly @@ -629,6 +656,8 @@ export class DaemonPtyAdapter implements IPtyProvider { this.dirtySessionVersions.clear() this.lastFullCheckpointAt.clear() this.coldRestoreCache.clear() + this.keepHistoryShutdownsPendingExit.clear() + this.sessionSpawnEpochs.clear() this.removeEventListener?.() this.removeEventListener = null // Why: final checkpoints are written daemon-side in TerminalHost.dispose() @@ -665,6 +694,8 @@ export class DaemonPtyAdapter implements IPtyProvider { this.dirtySessionVersions.clear() this.lastFullCheckpointAt.clear() this.coldRestoreCache.clear() + this.keepHistoryShutdownsPendingExit.clear() + this.sessionSpawnEpochs.clear() this.removeEventListener?.() this.removeEventListener = null this.client.disconnect() @@ -690,6 +721,22 @@ export class DaemonPtyAdapter implements IPtyProvider { } } + private clearSessionTracking(sessionId: string): void { + this.activeSessionIds.delete(sessionId) + this.dirtySessionVersions.delete(sessionId) + this.coldRestoreCache.delete(sessionId) + // Why: exited/closed sessions can never be checkpointed again; keep the + // pending full-checkpoint and cooldown sets bounded across reused ids. + this.sessionsNeedingFullCheckpoint.delete(sessionId) + this.lastFullCheckpointAt.delete(sessionId) + this.keepHistoryShutdownsPendingExit.delete(sessionId) + // Why: the counter is globally monotonic, so deleting here keeps the map + // bounded without letting a reused id collide with a stale epoch. + this.sessionSpawnEpochs.delete(sessionId) + this.stopCheckpointTimerIfIdle() + this.initialCwds.delete(sessionId) + } + private scheduleCheckpointTimer(): void { if ( this.checkpointTimer || @@ -1013,30 +1060,53 @@ export class DaemonPtyAdapter implements IPtyProvider { listener({ id: event.sessionId, data: event.payload.data }) } } else if (event.event === 'exit') { - this.activeSessionIds.delete(event.sessionId) - this.dirtySessionVersions.delete(event.sessionId) - this.coldRestoreCache.delete(event.sessionId) - // Why: an exited session can never be checkpointed again, so its pending - // full-checkpoint flag is dead state. Without this, a cold-restored - // session that exits before its first checkpoint leaks a permanent entry. - this.sessionsNeedingFullCheckpoint.delete(event.sessionId) - // Why: a reused sessionId (renderer respawns a persisted ptyId) must - // not inherit the dead session's snapshot cooldown. - this.lastFullCheckpointAt.delete(event.sessionId) - this.stopCheckpointTimerIfIdle() - if (this.historyManager) { - void this.historyManager - .closeSession(event.sessionId, event.payload.code) - .catch((err) => console.warn('[history] closeSession failed:', event.sessionId, err)) - } - this.initialCwds.delete(event.sessionId) - // oxlint-disable-next-line unicorn/no-useless-spread -- copy-safe: listeners may unsubscribe during iteration - for (const listener of [...this.exitListeners]) { - listener({ id: event.sessionId, code: event.payload.code }) - } + void this.handleExitEvent(event) } }) } + + private async handleExitEvent(event: Extract): Promise { + const preserveHistory = this.keepHistoryShutdownsPendingExit.has(event.sessionId) + const epochAtExit = this.sessionSpawnEpochs.get(event.sessionId) ?? 0 + if (preserveHistory && this.historyManager) { + try { + await this.persistFinalExitHistory(event.sessionId, event.payload.finalOutput) + } catch (err) { + console.warn('[history] final exit checkpoint failed:', event.sessionId, err) + } finally { + // Why: only detach if no newer spawn re-registered this id's writer + // while the persist awaited — detaching the woken generation's writer + // would silently stop its persistence. + if ((this.sessionSpawnEpochs.get(event.sessionId) ?? 0) === epochAtExit) { + this.historyManager.detachSessionWriter(event.sessionId) + } + } + } else if (this.historyManager) { + void this.historyManager + .closeSession(event.sessionId, event.payload.code) + .catch((err) => console.warn('[history] closeSession failed:', event.sessionId, err)) + } + if ((this.sessionSpawnEpochs.get(event.sessionId) ?? 0) === epochAtExit) { + this.clearSessionTracking(event.sessionId) + } + // oxlint-disable-next-line unicorn/no-useless-spread -- copy-safe: listeners may unsubscribe during iteration + for (const listener of [...this.exitListeners]) { + listener({ id: event.sessionId, code: event.payload.code }) + } + } + + private async persistFinalExitHistory( + sessionId: string, + finalOutput: TakePendingOutputResult | undefined + ): Promise { + if (!this.historyManager || !finalOutput?.snapshot) { + return + } + await this.historyManager.checkpoint(sessionId, finalOutput.snapshot) + if (finalOutput.records.length > 0) { + await this.historyManager.appendIncrements(sessionId, finalOutput.seq, finalOutput.records) + } + } } // Why: ENOENT/ECONNREFUSED with syscall 'connect' mean the socket is @@ -1056,3 +1126,7 @@ function isDaemonGoneError(err: unknown): boolean { const msg = err.message return msg === 'Connection lost' || msg === 'Not connected' } + +function isSessionNotFoundError(err: unknown): boolean { + return err instanceof Error && /Session not found/i.test(err.message) +} diff --git a/src/main/daemon/daemon-pty-router.test.ts b/src/main/daemon/daemon-pty-router.test.ts index 47d44aa807c..f29459fc84d 100644 --- a/src/main/daemon/daemon-pty-router.test.ts +++ b/src/main/daemon/daemon-pty-router.test.ts @@ -145,6 +145,20 @@ describe('DaemonPtyRouter', () => { expect(current.hasPty).not.toHaveBeenCalledWith('legacy-session') }) + it('keeps the routed adapter between graceful shutdown and force escalation', async () => { + const current = createAdapter('current') + const legacy = createAdapter('legacy', ['legacy-session']) + const router = new DaemonPtyRouter({ current, legacy: [legacy] }) + + await router.discoverLegacySessions() + await router.shutdown('legacy-session', { immediate: false }) + await router.shutdown('legacy-session', { immediate: true }) + + expect(legacy.shutdown).toHaveBeenNthCalledWith(1, 'legacy-session', { immediate: false }) + expect(legacy.shutdown).toHaveBeenNthCalledWith(2, 'legacy-session', { immediate: true }) + expect(current.shutdown).not.toHaveBeenCalled() + }) + 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 1f80f770cd9..d603c20919d 100644 --- a/src/main/daemon/daemon-pty-router.ts +++ b/src/main/daemon/daemon-pty-router.ts @@ -84,7 +84,9 @@ export class DaemonPtyRouter implements IPtyProvider { // lived on a legacy adapter (different protocolVersion), the wake-side // createOrAttach lands on the wrong adapter and creates a fresh session, // losing the cold-restore from the legacy adapter's history dir. - if (!opts.keepHistory) { + // Graceful shutdown is only the first half of drain-then-force teardown; + // keep the route so timeout escalation still targets the owning daemon. + if (opts.immediate === true && !opts.keepHistory) { this.sessionAdapters.delete(id) } } diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 3a585a0896c..5ab80e36f12 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -21,6 +21,11 @@ import { type DaemonRequest } from './types' +/** Bounded graceful-exit window for live sessions on daemon shutdown, so + * agent CLIs can flush transcripts before dispose() force-kills leftovers. + * Kept short: daemon restart / app quit wait on this. */ +const SHUTDOWN_SESSION_DRAIN_MS = 2_000 + export type DaemonServerOptions = { socketPath: string tokenPath: string @@ -97,7 +102,8 @@ export class DaemonServer { } async shutdown(): Promise { - this.host.dispose() + await this.host.drainLiveSessions(SHUTDOWN_SESSION_DRAIN_MS) + await this.host.disposeAndWaitForForceKill({ checkpointDirtyOnly: true }) this.streamDataBatcher.clear() for (const [, client] of this.clients) { @@ -305,7 +311,7 @@ export class DaemonServer { flushMaxChars: DaemonServer.INTERACTIVE_OUTPUT_MAX_CHARS }) }, - onExit: (code) => { + onExit: (code, finalOutput) => { // Why: exit tears down renderer handlers; flush final output first // so the last few milliseconds of PTY data are not stranded. this.log.log('session-exited', { sessionId: p.sessionId, code }) @@ -317,7 +323,7 @@ export class DaemonServer { type: 'event', event: 'exit', sessionId: p.sessionId, - payload: { code } + payload: { code, ...(finalOutput ? { finalOutput } : {}) } }) ) } @@ -369,7 +375,14 @@ export class DaemonServer { sessionId: request.payload.sessionId, immediate: request.payload.immediate === true }) - this.host.kill(request.payload.sessionId, { immediate: request.payload.immediate }) + if (request.payload.immediate === true) { + await this.host.killAndWait(request.payload.sessionId, { immediate: true }) + } else { + this.host.kill(request.payload.sessionId, { + immediate: request.payload.immediate, + collectFinalOutput: request.payload.collectFinalOutput === true + }) + } return {} case 'signal': @@ -428,7 +441,10 @@ export class DaemonServer { killSessions: request.payload.killSessions === true }) if (request.payload.killSessions) { - this.host.dispose() + // Why: drain before dispose so agent CLIs flush transcripts; the + // client's shutdown RPC timeout (30s) comfortably covers the window. + await this.host.drainLiveSessions(SHUTDOWN_SESSION_DRAIN_MS) + await this.host.disposeAndWaitForForceKill({ checkpointDirtyOnly: true }) } process.nextTick(() => this.shutdown()) return {} diff --git a/src/main/daemon/degraded-daemon-pty-provider.test.ts b/src/main/daemon/degraded-daemon-pty-provider.test.ts index 7bd656fc6fb..9df1fc11a45 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.test.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.test.ts @@ -151,6 +151,20 @@ describe('DegradedDaemonPtyProvider', () => { expect(fallback.write).not.toHaveBeenCalled() }) + it('keeps the owning provider between graceful shutdown and force escalation', async () => { + const current = createDaemonAdapter('daemon', ['daemon-session']) + const fallback = createProvider('fallback') + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + + await provider.discoverDaemonSessions() + await provider.shutdown('daemon-session', { immediate: false }) + await provider.shutdown('daemon-session', { immediate: true }) + + expect(current.shutdown).toHaveBeenNthCalledWith(1, 'daemon-session', { immediate: false }) + expect(current.shutdown).toHaveBeenNthCalledWith(2, 'daemon-session', { immediate: true }) + expect(fallback.shutdown).not.toHaveBeenCalled() + }) + it('forwards replay output from fallback and daemon providers', () => { const current = createDaemonAdapter('daemon') 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 a7b8c68fa19..d067521a365 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.ts @@ -95,7 +95,10 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { async shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise { await this.providerFor(id).shutdown(id, opts) - if (!opts.keepHistory) { + // Why: graceful shutdown is followed by a forced escalation when the PTY + // ignores the signal; dropping ownership here would route that escalation + // to the fallback provider instead of the daemon/local provider that owns it. + if (opts.immediate === true && !opts.keepHistory) { this.sessionProviders.delete(id) } } diff --git a/src/main/daemon/history-manager.ts b/src/main/daemon/history-manager.ts index 0edcdb91ba2..dfd0aaab958 100644 --- a/src/main/daemon/history-manager.ts +++ b/src/main/daemon/history-manager.ts @@ -308,6 +308,13 @@ export class HistoryManager { }) } + // Why: hibernated terminals must leave meta.endedAt null for cold restore, but + // the dead session's writer must not be closed later by dispose(). + detachSessionWriter(sessionId: string): void { + this.writers.delete(sessionId) + this.disabledSessions.delete(sessionId) + } + isSessionDisabled(sessionId: string): boolean { return this.disabledSessions.has(sessionId) } diff --git a/src/main/daemon/pty-subprocess.test.ts b/src/main/daemon/pty-subprocess.test.ts index ad8f14cb068..5e5613daed0 100644 --- a/src/main/daemon/pty-subprocess.test.ts +++ b/src/main/daemon/pty-subprocess.test.ts @@ -59,6 +59,23 @@ vi.mock('../providers/agent-foreground-process', () => ({ resolveAgentForegroundProcess: resolveAgentForegroundProcessMock })) +// Why: never spawn a real `taskkill` from tests — on a Windows runner that +// would signal an arbitrary live pid. +const { + requestWindowsProcessTreeExitMock, + forceKillWindowsProcessTreeMock, + waitForWindowsProcessTreeForceKillMock +} = vi.hoisted(() => ({ + requestWindowsProcessTreeExitMock: vi.fn(), + forceKillWindowsProcessTreeMock: vi.fn(), + waitForWindowsProcessTreeForceKillMock: vi.fn() +})) +vi.mock('../pty/windows-process-tree-kill', () => ({ + requestWindowsProcessTreeExit: requestWindowsProcessTreeExitMock, + forceKillWindowsProcessTree: forceKillWindowsProcessTreeMock, + waitForWindowsProcessTreeForceKill: waitForWindowsProcessTreeForceKillMock +})) + import { createPtySubprocess, checkPtySpawnHealth } from './pty-subprocess' const ORCA_SHELL_WRAPPER_ENV = [ @@ -106,6 +123,10 @@ describe('createPtySubprocess', () => { beforeEach(() => { spawnMock.mockReset() isPwshAvailableMock.mockReset() + requestWindowsProcessTreeExitMock.mockReset() + forceKillWindowsProcessTreeMock.mockReset() + waitForWindowsProcessTreeForceKillMock.mockReset() + waitForWindowsProcessTreeForceKillMock.mockResolvedValue(undefined) resolveAgentForegroundProcessMock.mockReset() resolveAgentForegroundProcessMock.mockImplementation( async (_pid: number, fallbackProcess: string | null) => fallbackProcess @@ -2353,8 +2374,31 @@ describe('createPtySubprocess', () => { } }) - it('dispose() on Windows skips destroy after node-pty kill()', () => { - const proc = mockPtyProcess() as ReturnType & { + it('kill() on Windows requests a graceful tree exit and leaves destroy() live', () => { + const proc = mockPtyProcess(123456) as ReturnType & { + destroy: ReturnType + } + proc.destroy = vi.fn() + 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() + // Why: the graceful path must NOT tear the ConPTY down — that would + // collapse the drain window agent CLIs need to flush transcripts. + expect(requestWindowsProcessTreeExitMock).toHaveBeenCalledWith(123456) + expect(proc.kill).not.toHaveBeenCalled() + handle.dispose() + // dispose() still releases the ConPTY via destroy() (which is kill()). + expect(proc.destroy).toHaveBeenCalledOnce() + } finally { + restorePlatform(origPlatform) + } + }) + + it('dispose() on Windows skips destroy after forceKill kills the tree', () => { + const proc = mockPtyProcess(123456) as ReturnType & { destroy: ReturnType } proc.destroy = vi.fn(() => proc.kill()) @@ -2363,8 +2407,9 @@ describe('createPtySubprocess', () => { Object.defineProperty(process, 'platform', { value: 'win32' }) try { const handle = createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) - handle.kill() + handle.forceKill() handle.dispose() + expect(forceKillWindowsProcessTreeMock).toHaveBeenCalledWith(123456) expect(proc.kill).toHaveBeenCalledOnce() expect(proc.destroy).not.toHaveBeenCalled() } finally { @@ -2372,26 +2417,37 @@ describe('createPtySubprocess', () => { } }) - it('dispose() on Windows skips destroy after forceKill falls back to node-pty kill()', () => { - const proc = mockPtyProcess(123456) as ReturnType & { - destroy: ReturnType - } - proc.destroy = vi.fn(() => proc.kill()) + it('forceKillAndWait() on Windows waits for taskkill before closing ConPTY', async () => { + const proc = mockPtyProcess(123456) spawnMock.mockReturnValue(proc) - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { - throw new Error('already gone') - }) + let releaseForceKill = (): void => { + throw new Error('force kill waiter was not installed') + } + waitForWindowsProcessTreeForceKillMock.mockImplementation( + () => + new Promise((resolve) => { + releaseForceKill = resolve + }) + ) const origPlatform = Object.getOwnPropertyDescriptor(process, 'platform') Object.defineProperty(process, 'platform', { value: 'win32' }) try { const handle = createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) - handle.forceKill() - handle.dispose() - expect(killSpy).toHaveBeenCalledWith(123456, 'SIGKILL') + let settled = false + const forceKill = handle.forceKillAndWait?.().then(() => { + settled = true + }) + await Promise.resolve() + + expect(waitForWindowsProcessTreeForceKillMock).toHaveBeenCalledWith(123456) + expect(proc.kill).not.toHaveBeenCalled() + expect(settled).toBe(false) + + releaseForceKill() + await forceKill expect(proc.kill).toHaveBeenCalledOnce() - expect(proc.destroy).not.toHaveBeenCalled() + expect(settled).toBe(true) } finally { - killSpy.mockRestore() restorePlatform(origPlatform) } }) diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index 30eb7b40082..8ce52bd8afe 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -29,6 +29,11 @@ import { } from '../providers/windows-shell-fallback-chain' import { isPwshAvailable } from '../pwsh' import { isHostCodexHomeForWsl, isWslCodexHomeForHost } from '../pty/codex-home-wsl-env' +import { + forceKillWindowsProcessTree, + requestWindowsProcessTreeExit, + waitForWindowsProcessTreeForceKill +} from '../pty/windows-process-tree-kill' import { removeInheritedNoColor } from '../pty/terminal-color-env' import { removeAppImageRuntimeEnv } from '../pty/appimage-terminal-env' import { parseWslPath } from '../wsl' @@ -1082,6 +1087,14 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl if (dead) { return } + // Why: on Windows node-pty's kill() tears the ConPTY down immediately — + // no drain window for agent CLIs flushing transcripts. Ask the tree to + // close instead; session.ts's KILL_TIMEOUT_MS fallback force-kills any + // process that refuses. + if (process.platform === 'win32') { + requestWindowsProcessTreeExit(proc.pid) + return + } try { nodePtyKillIssued = true proc.kill() @@ -1097,6 +1110,44 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl if (dead) { return } + if (process.platform === 'win32') { + // Why: taskkill /T /F reaps the whole console tree (process.kill hits + // only the shell pid); proc.kill() then releases the ConPTY itself. + forceKillWindowsProcessTree(proc.pid) + try { + nodePtyKillIssued = true + proc.kill() + } catch { + // Process may already be dead + } + return + } + try { + process.kill(proc.pid, 'SIGKILL') + } catch { + try { + nodePtyKillIssued = true + proc.kill() + } catch { + // Process may already be dead + } + } + }, + forceKillAndWait: async () => { + if (dead) { + return + } + if (process.platform === 'win32') { + onExitCb = null + await waitForWindowsProcessTreeForceKill(proc.pid) + try { + nodePtyKillIssued = true + proc.kill() + } catch { + // Process may already be dead + } + return + } try { process.kill(proc.pid, 'SIGKILL') } catch { diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index d156aeb4802..6d57d876086 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -48,6 +48,9 @@ export type SubprocessHandle = { clear?(): void kill(): void forceKill(): void + /** Same force-kill signal as forceKill(), but resolves after platform-specific + * tree-kill work that callers need before deleting worktree files. */ + forceKillAndWait?(): Promise signal(sig: string): void onData(cb: (data: string) => void): void onExit(cb: (code: number) => void): void @@ -77,7 +80,7 @@ export type SessionOptions = { type AttachedClient = { token: symbol onData: (data: string) => void - onExit: (code: number) => void + onExit: (code: number, finalOutput?: TakePendingOutputResult) => void } export class Session { @@ -101,6 +104,7 @@ export class Session { private pendingOutputBytes = 0 private pendingOutputOverflowed = false private pendingOutputSeq = 0 + private captureFinalOutput = false constructor(opts: SessionOptions) { this.sessionId = opts.sessionId @@ -203,6 +207,18 @@ export class Session { }, KILL_TIMEOUT_MS) } + requestFinalOutputOnExit(): void { + this.captureFinalOutput = true + } + + hasPendingOutputForCheckpoint(): boolean { + return ( + this.pendingOutputOverflowed || + this.pendingOutputRecords.length > 0 || + this.shellReadyScanState !== null + ) + } + signal(sig: string): void { if (this._state === 'exited') { return @@ -210,7 +226,10 @@ export class Session { this.subprocess.signal(sig) } - attachClient(client: { onData: (data: string) => void; onExit: (code: number) => void }): symbol { + attachClient(client: { + onData: (data: string) => void + onExit: (code: number, finalOutput?: TakePendingOutputResult) => void + }): symbol { const token = Symbol('attach') this.attachedClients.push({ token, ...client }) return token @@ -404,6 +423,23 @@ export class Session { this.emulator.dispose() } + async forceKillAndDisposeSubprocessAndWait(): Promise { + try { + if (this.subprocess.forceKillAndWait) { + await this.subprocess.forceKillAndWait() + } else { + this.subprocess.forceKill() + } + } catch { + /* swallow — child may already be gone */ + } + this._exitCode = -1 + this._isTerminating = false + 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` — @@ -497,7 +533,10 @@ export class Session { this._exitCode = code this._state = 'exited' - this.releaseHeldShellReadyBytes() + const finalOutput = this.takeFinalOutputIfRequested() + if (!finalOutput) { + this.releaseHeldShellReadyBytes() + } if (this.killTimer) { clearTimeout(this.killTimer) @@ -523,7 +562,11 @@ export class Session { } for (const client of this.attachedClients) { - client.onExit(code) + if (finalOutput) { + client.onExit(code, finalOutput) + } else { + client.onExit(code) + } } // Why: hand off to the owner's reaper so the emulator is disposed and the @@ -545,6 +588,14 @@ export class Session { return heldBytes } + private takeFinalOutputIfRequested(): TakePendingOutputResult | null { + if (!this.captureFinalOutput) { + return null + } + this.captureFinalOutput = false + return this.takePendingOutput(true, { teardownSnapshot: true }) + } + private transitionToReady(postMarkerBytesObserved = false): void { this._shellState = 'ready' this.shellReadyScanState = null @@ -592,6 +643,7 @@ export class Session { } catch { /* already dead */ } + const finalOutput = this.takeFinalOutputIfRequested() this._exitCode = -1 this._isTerminating = false @@ -605,7 +657,11 @@ export class Session { this.emulator.dispose() for (const client of clients) { - client.onExit(-1) + if (finalOutput) { + client.onExit(-1, finalOutput) + } else { + client.onExit(-1) + } } // Why: reap from the host map on the kill-timeout path too (emulator already diff --git a/src/main/daemon/terminal-host-shutdown.ts b/src/main/daemon/terminal-host-shutdown.ts new file mode 100644 index 00000000000..74863301ea4 --- /dev/null +++ b/src/main/daemon/terminal-host-shutdown.ts @@ -0,0 +1,141 @@ +import type { TakePendingOutputResult, TerminalSnapshot } from './types' +import type { Session } from './session' + +export type ShutdownCallbacks = { + onFinalCheckpoint?: ( + sessionId: string, + snapshot: TerminalSnapshot, + records: TakePendingOutputResult['records'] + ) => void +} + +const DEFAULT_MAX_TOMBSTONES = 1000 + +/** Owns TerminalHost's teardown surface: killed-session tombstones, final + * checkpoints, the bounded graceful drain, and force-kill disposal. */ +export class TerminalHostShutdown { + private sessions: Map + private killedTombstones = new Map() + private maxTombstones: number + private onFinalCheckpoint: ShutdownCallbacks['onFinalCheckpoint'] + + constructor( + sessions: Map, + opts: { maxTombstones?: number; onFinalCheckpoint?: ShutdownCallbacks['onFinalCheckpoint'] } + ) { + this.sessions = sessions + this.maxTombstones = opts.maxTombstones ?? DEFAULT_MAX_TOMBSTONES + this.onFinalCheckpoint = opts.onFinalCheckpoint + } + + recordTombstone(sessionId: string): void { + this.killedTombstones.delete(sessionId) + this.killedTombstones.set(sessionId, Date.now()) + if (this.killedTombstones.size > this.maxTombstones) { + const oldest = this.killedTombstones.keys().next().value + if (oldest) { + this.killedTombstones.delete(oldest) + } + } + } + + clearTombstone(sessionId: string): void { + this.killedTombstones.delete(sessionId) + } + + isKilled(sessionId: string): boolean { + return this.killedTombstones.has(sessionId) + } + + private writeFinalCheckpoints(opts: { dirtyOnly?: boolean } = {}): void { + // Why: written before killing so graceful shutdown has zero data loss; + // the checkpoint callback writes synchronously to disk. + if (this.onFinalCheckpoint) { + for (const [sessionId, session] of this.sessions) { + if (!session.isAlive) { + continue + } + if (opts.dirtyOnly && !session.hasPendingOutputForCheckpoint()) { + continue + } + const take = session.takePendingOutput(true, { teardownSnapshot: true }) + if (take?.snapshot) { + try { + this.onFinalCheckpoint(sessionId, take.snapshot, take.records) + } catch { + // Best-effort — don't block shutdown + } + } + } + } + } + + private hasLiveSessions(): boolean { + for (const [, session] of this.sessions) { + if (session.isAlive) { + return true + } + } + return false + } + + async drainLiveSessions(drainMs: number): Promise { + this.writeFinalCheckpoints() + for (const [, session] of this.sessions) { + // Why: daemon shutdown must not fan exit events to connected apps — + // restartDaemon and external-termination recovery rely on sessions dying + // silently so history meta keeps endedAt=null for cold restore. + session.detachAllClients() + } + let anyLive = false + for (const [, session] of this.sessions) { + if (session.isAlive && !session.isTerminating) { + session.kill() + } + anyLive ||= session.isAlive + } + if (!anyLive) { + return + } + const deadline = Date.now() + drainMs + while (Date.now() < deadline) { + if (!this.hasLiveSessions()) { + return + } + await new Promise((resolve) => setTimeout(resolve, 50)) + } + } + + dispose(): void { + this.writeFinalCheckpoints() + for (const [, session] of this.sessions) { + session.detachAllClients() + // Why: SIGKILL on an already-exited session would target a reaped pid + // that POSIX can recycle to a stranger; dead sessions only release the + // ptmx fd via disposeSubprocess(). See docs/fix-pty-fd-leak.md. + if (session.isAlive) { + session.forceKillAndDisposeSubprocess() + } else { + session.disposeSubprocess() + } + } + this.sessions.clear() + this.killedTombstones.clear() + } + + async disposeAndWaitForForceKill(opts: { checkpointDirtyOnly?: boolean } = {}): Promise { + this.writeFinalCheckpoints({ dirtyOnly: opts.checkpointDirtyOnly === true }) + const pendingForceKills: Promise[] = [] + for (const [, session] of this.sessions) { + session.detachAllClients() + if (session.isAlive) { + pendingForceKills.push(session.forceKillAndDisposeSubprocessAndWait()) + } else { + session.disposeSubprocess() + } + } + await Promise.all(pendingForceKills) + this.sessions.clear() + this.killedTombstones.clear() + } +} diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index 4f92d4c1e1f..8b399ca1ae8 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -312,6 +312,112 @@ describe('TerminalHost', () => { }) }) + describe('drainLiveSessions', () => { + it('gracefully kills live sessions and resolves once they exit', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + + // The mock subprocess exits ~5ms after kill(), well inside the window. + await host.drainLiveSessions(1_000) + + expect(lastSubprocess.kill).toHaveBeenCalled() + expect(lastSubprocess.forceKill).not.toHaveBeenCalled() + expect(host.listSessions()).toEqual([]) + }) + + it('does not fan exit events to attached clients during the drain', async () => { + const onExit = vi.fn() + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit } + }) + + await host.drainLiveSessions(1_000) + + // Why: daemon shutdown must stay silent toward connected apps — an exit + // event here would write endedAt app-side and break cold restore after + // a daemon restart or external termination. + expect(onExit).not.toHaveBeenCalled() + expect(host.listSessions()).toEqual([]) + }) + + it('returns after the bounded window when a session ignores the signal', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + // Session ignores the graceful signal: never fires onExit. + ;(lastSubprocess.kill as ReturnType).mockImplementation(() => {}) + + const start = Date.now() + await host.drainLiveSessions(120) + expect(Date.now() - start).toBeLessThan(2_000) + + // dispose() (the caller's next step) force-kills the leftover. + host.dispose() + expect(lastSubprocess.forceKill).toHaveBeenCalled() + }) + + it('does not checkpoint quiet stubborn sessions twice before force dispose', async () => { + host.dispose() + const onFinalCheckpoint = vi.fn() + host = new TerminalHost({ + spawnSubprocess: spawnFn as MockSpawnFn, + onFinalCheckpoint + }) + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + lastSubprocess._onDataCb?.('before drain\r\n') + ;(lastSubprocess.kill as ReturnType).mockImplementation(() => {}) + + await host.drainLiveSessions(10) + await host.disposeAndWaitForForceKill({ checkpointDirtyOnly: true }) + + expect(onFinalCheckpoint).toHaveBeenCalledTimes(1) + }) + + it('checkpoints stubborn sessions again when they emit output during drain', async () => { + host.dispose() + const onFinalCheckpoint = vi.fn() + host = new TerminalHost({ + spawnSubprocess: spawnFn as MockSpawnFn, + onFinalCheckpoint + }) + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + lastSubprocess._onDataCb?.('before drain\r\n') + ;(lastSubprocess.kill as ReturnType).mockImplementation(() => { + lastSubprocess._onDataCb?.('tail during drain\r\n') + }) + + await host.drainLiveSessions(10) + await host.disposeAndWaitForForceKill({ checkpointDirtyOnly: true }) + + expect(onFinalCheckpoint).toHaveBeenCalledTimes(2) + expect(onFinalCheckpoint).toHaveBeenLastCalledWith( + 'session-1', + expect.objectContaining({ snapshotAnsi: expect.stringContaining('tail during drain') }), + [] + ) + }) + }) + describe('signal', () => { it('sends signal without entering kill state', async () => { await host.createOrAttach({ diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index 2a54ce2c19b..77657d7da0f 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -10,8 +10,7 @@ import type { ShellReadyState } from './types' import { SessionNotFoundError } from './types' - -const DEFAULT_MAX_TOMBSTONES = 1000 +import { TerminalHostShutdown } from './terminal-host-shutdown' export type CreateOrAttachOptions = { sessionId: string @@ -31,7 +30,10 @@ export type CreateOrAttachOptions = { terminalWindowsPowerShellImplementation?: 'auto' | 'powershell.exe' | 'pwsh.exe' shellReadySupported?: boolean shellReadyTimeoutMs?: number - streamClient: { onData: (data: string) => void; onExit: (code: number) => void } + streamClient: { + onData: (data: string) => void + onExit: (code: number, finalOutput?: TakePendingOutputResult) => void + } } export type CreateOrAttachResult = { @@ -71,15 +73,15 @@ export type TerminalHostOptions = { export class TerminalHost { private sessions = new Map() - private killedTombstones = new Map() private spawnSubprocess: TerminalHostOptions['spawnSubprocess'] - private onFinalCheckpoint: TerminalHostOptions['onFinalCheckpoint'] - private maxTombstones: number + private shutdownController: TerminalHostShutdown constructor(opts: TerminalHostOptions) { this.spawnSubprocess = opts.spawnSubprocess - this.onFinalCheckpoint = opts.onFinalCheckpoint - this.maxTombstones = opts.maxTombstones ?? DEFAULT_MAX_TOMBSTONES + this.shutdownController = new TerminalHostShutdown(this.sessions, { + maxTombstones: opts.maxTombstones, + onFinalCheckpoint: opts.onFinalCheckpoint + }) } /** @@ -116,7 +118,7 @@ export class TerminalHost { } // Clear tombstone if re-creating a killed session - this.killedTombstones.delete(opts.sessionId) + this.shutdownController.clearTombstone(opts.sessionId) const size = normalizePtySize(opts.cols, opts.rows) const subprocess = this.spawnSubprocess({ @@ -192,9 +194,9 @@ export class TerminalHost { this.getAliveSession(sessionId).resize(cols, rows) } - kill(sessionId: string, opts: { immediate?: boolean } = {}): void { + kill(sessionId: string, opts: { immediate?: boolean; collectFinalOutput?: boolean } = {}): void { const session = this.getAliveSession(sessionId) - this.recordTombstone(sessionId) + this.shutdownController.recordTombstone(sessionId) if (opts.immediate) { session.forceKillAndDisposeSubprocess() // Why: the immediate path tears down synchronously without firing the @@ -203,6 +205,26 @@ export class TerminalHost { this.reapSession(sessionId) return } + if (opts.collectFinalOutput) { + session.requestFinalOutputOnExit() + } + session.kill() + } + + async killAndWait( + sessionId: string, + opts: { immediate?: boolean; collectFinalOutput?: boolean } = {} + ): Promise { + const session = this.getAliveSession(sessionId) + this.shutdownController.recordTombstone(sessionId) + if (opts.immediate) { + await session.forceKillAndDisposeSubprocessAndWait() + this.reapSession(sessionId) + return + } + if (opts.collectFinalOutput) { + session.requestFinalOutputOnExit() + } session.kill() } @@ -295,7 +317,7 @@ export class TerminalHost { } isKilled(sessionId: string): boolean { - return this.killedTombstones.has(sessionId) + return this.shutdownController.isKilled(sessionId) } listSessions(): SessionInfo[] { @@ -321,43 +343,16 @@ export class TerminalHost { return result } - dispose(): void { - // Why: write final checkpoints before killing sessions so graceful shutdown - // has zero data loss. The checkpoint callback writes synchronously to disk. - if (this.onFinalCheckpoint) { - for (const [sessionId, session] of this.sessions) { - if (!session.isAlive) { - continue - } - const take = session.takePendingOutput(true, { teardownSnapshot: true }) - if (take?.snapshot) { - try { - this.onFinalCheckpoint(sessionId, take.snapshot, take.records) - } catch { - // Best-effort — don't block shutdown - } - } - } - } + async drainLiveSessions(drainMs: number): Promise { + await this.shutdownController.drainLiveSessions(drainMs) + } - for (const [, session] of this.sessions) { - session.detachAllClients() - // Why: live-vs-exited is load-bearing. For LIVE sessions we use - // forceKillAndDisposeSubprocess (SIGKILL + destroy) to reap stubborn - // children AND release the ptmx fd on the same tick, bypassing the 5s - // KILL_TIMEOUT_MS fallback that would otherwise outlive the daemon - // process. For sessions that have already exited but are still in the - // map, SIGKILL would target a reaped pid — on POSIX that pid can be - // recycled to an unrelated process, so we MUST only release the fd via - // disposeSubprocess() (destroy without kill). See docs/fix-pty-fd-leak.md. - if (session.isAlive) { - session.forceKillAndDisposeSubprocess() - } else { - session.disposeSubprocess() - } - } - this.sessions.clear() - this.killedTombstones.clear() + dispose(): void { + this.shutdownController.dispose() + } + + async disposeAndWaitForForceKill(opts: { checkpointDirtyOnly?: boolean } = {}): Promise { + await this.shutdownController.disposeAndWaitForForceKill(opts) } private getAliveSession(sessionId: string): Session { @@ -367,16 +362,4 @@ export class TerminalHost { } return session } - - private recordTombstone(sessionId: string): void { - this.killedTombstones.delete(sessionId) - this.killedTombstones.set(sessionId, Date.now()) - - if (this.killedTombstones.size > this.maxTombstones) { - const oldest = this.killedTombstones.keys().next().value - if (oldest) { - this.killedTombstones.delete(oldest) - } - } - } } diff --git a/src/main/daemon/types.ts b/src/main/daemon/types.ts index 10d27f2c8e0..54729d6776c 100644 --- a/src/main/daemon/types.ts +++ b/src/main/daemon/types.ts @@ -7,9 +7,9 @@ import type { StartupCommandDelivery } from '../../shared/codex-startup-delivery // 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 = 18 +export const PROTOCOL_VERSION = 19 export const PREVIOUS_DAEMON_PROTOCOL_VERSIONS = [ - 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17 + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18 ] as const // ─── Session State Machine ────────────────────────────────────────── @@ -136,6 +136,7 @@ export type KillRequest = { payload: { sessionId: string immediate?: boolean + collectFinalOutput?: boolean } } @@ -359,7 +360,7 @@ export type ExitEvent = { type: 'event' event: 'exit' sessionId: string - payload: { code: number } + payload: { code: number; finalOutput?: TakePendingOutputResult } } export type TerminalErrorEvent = { diff --git a/src/main/index.ts b/src/main/index.ts index 0eb2d9d9a4f..b0c6cf661fc 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -2236,9 +2236,10 @@ app.on('will-quit', (e) => { destroySystemTray() // Why: stats.flush() must run before killAllPty() so it can read the // live agent state and emit synthetic agent_stop events for agents that - // are still running. killAllPty() does not call runtime.onPtyExit(), - // so without this ordering, running agents would produce orphaned - // agent_start events with no matching stops. + // are still running. killAllPty()'s graceful drain can fire + // runtime.onPtyExit() for PTYs that exit inside the window, but stubborn + // ones are force-killed without it — so flush-first stays the only + // ordering that guarantees no orphaned agent_start events. starNag?.stop() automations?.stop() setUnreadDockBadgeCount(0) @@ -2252,7 +2253,9 @@ app.on('will-quit', (e) => { runtime?.getOffscreenBrowserBackend()?.destroyAll?.() const emulatorShutdown = runtime?.getEmulatorBridge()?.destroyAllSessions() ?? Promise.resolve() serveSimStateWatcher.stop() - killAllPty() + // Why: killAllPty drains agent CLIs (graceful signal → bounded force-kill) + // so buffered transcripts flush; awaited below alongside the daemon flush. + const ptyDrainOnQuit = killAllPty() const watcherShutdown = shutdownWatchersOnce() store?.flush() @@ -2302,7 +2305,13 @@ app.on('will-quit', (e) => { // Why: normal quits preserve the detached daemon for warm reattach, but a // dev parent dying means the temp/dev profile has no owner left to reattach. const daemonTeardown = isDevParentShutdownRequested() ? shutdownDaemon() : disconnectDaemon() - Promise.allSettled([daemonTeardown, rpcStopAndClear, watcherShutdown, emulatorShutdown]) + Promise.allSettled([ + daemonTeardown, + rpcStopAndClear, + watcherShutdown, + emulatorShutdown, + ptyDrainOnQuit + ]) .then(() => shutdownTelemetry()) .then(() => shutdownObservability()) .catch(() => { diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 9b4216d924d..4f4fe7defbf 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -174,7 +174,8 @@ vi.mock('../agent-hooks/migration-unsupported-pty-state', () => ({ clearMigrationUnsupportedPty: clearMigrationUnsupportedPtyMock, clearMigrationUnsupportedPtysForPaneKey: clearMigrationUnsupportedPtysForPaneKeyMock })) -import { LocalPtyProvider } from '../providers/local-pty-provider' +import { GRACEFUL_TEARDOWN_FORCE_KILL_MS, LocalPtyProvider } from '../providers/local-pty-provider' +import { PTY_EXIT_DRAIN_WINDOW_MS } from '../providers/pty-shutdown-drain' import { makePaneKey } from '../../shared/stable-pane-id' import { SETUP_AGENT_SEQUENCE_STARTUP_COMMAND_ENV } from '../../shared/setup-agent-sequencing' import { @@ -2484,12 +2485,14 @@ describe('registerPtyHandlers', () => { kill: (ptyId: string) => boolean } + vi.useFakeTimers() expect(controller.kill('remote-pty')).toBe(true) - // Why: kill's shutdown now runs through the exit-detection wrapper, - // which adds async hops; a single microtask flush is no longer enough. - await new Promise((resolve) => setImmediate(resolve)) + // Why: kill now drains gracefully before escalating; advance past the + // drain window so the escalation + lease cleanup complete. + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) expect(shutdown).toHaveBeenCalledWith('remote-pty', { immediate: false }) + expect(shutdown).toHaveBeenLastCalledWith('remote-pty', { immediate: true }) expect(store.markSshRemotePtyLease).toHaveBeenCalledWith( 'ssh-1', 'remote-pty', @@ -2652,10 +2655,15 @@ describe('registerPtyHandlers', () => { } const stopPromise = controller.stopAndWait('remote-pty', { keepHistory: true }) - await vi.advanceTimersByTimeAsync(1_200) + // Why: cover the graceful drain window plus the keepHistory settle poll. + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS + 1_200) await expect(stopPromise).resolves.toBe(true) expect(shutdown).toHaveBeenCalledWith('remote-pty', { + immediate: false, + keepHistory: true + }) + expect(shutdown).toHaveBeenLastCalledWith('remote-pty', { immediate: true, keepHistory: true }) @@ -2707,7 +2715,8 @@ describe('registerPtyHandlers', () => { } const stopPromise = controller.stopAndWait('local-pty', { keepHistory: true }) - await vi.advanceTimersByTimeAsync(200) + // Why: cover the graceful drain window plus the keepHistory settle poll. + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS + 1_200) await expect(stopPromise).resolves.toBe(false) expect(shutdown).toHaveBeenCalledWith('local-pty', { @@ -2828,8 +2837,10 @@ describe('registerPtyHandlers', () => { kill: (ptyId: string) => boolean } + vi.useFakeTimers() expect(controller.kill('ssh:ssh-1@@relay-pty')).toBe(true) - await new Promise((resolve) => setImmediate(resolve)) + // Why: kill drains gracefully before escalating; advance past the window. + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) expect(shutdown).toHaveBeenCalledWith('ssh:ssh-1@@relay-pty', { immediate: false }) expect(localShutdown).not.toHaveBeenCalled() @@ -3715,8 +3726,22 @@ describe('registerPtyHandlers', () => { it('lists duplicate SSH relay session ids as distinct app sessions', async () => { registerPtyHandlers(mainWindow as never) - const shutdownA = vi.fn(async () => undefined) - const shutdownB = vi.fn(async () => undefined) + const exitListenersA = new Set<(payload: { id: string; code: number }) => void>() + const exitListenersB = new Set<(payload: { id: string; code: number }) => void>() + const shutdownA = vi.fn(async (id: string, opts: { immediate?: boolean }) => { + if (!opts.immediate) { + for (const listener of exitListenersA) { + listener({ id, code: 0 }) + } + } + }) + const shutdownB = vi.fn(async (id: string, opts: { immediate?: boolean }) => { + if (!opts.immediate) { + for (const listener of exitListenersB) { + listener({ id, code: 0 }) + } + } + }) registerSshPtyProvider('ssh-a', { spawn: vi.fn(), write: vi.fn(), @@ -3729,7 +3754,10 @@ describe('registerPtyHandlers', () => { acknowledgeDataEvent: vi.fn(), onData: vi.fn(() => () => {}), onReplay: vi.fn(() => () => {}), - onExit: vi.fn(() => () => {}), + onExit: vi.fn((cb: (payload: { id: string; code: number }) => void) => { + exitListenersA.add(cb) + return () => exitListenersA.delete(cb) + }), listProcesses: vi.fn(async () => [ { id: 'ssh:ssh-a@@pty-1', cwd: '/repo-a', title: 'ssh-a' } ]), @@ -3753,7 +3781,10 @@ describe('registerPtyHandlers', () => { acknowledgeDataEvent: vi.fn(), onData: vi.fn(() => () => {}), onReplay: vi.fn(() => () => {}), - onExit: vi.fn(() => () => {}), + onExit: vi.fn((cb: (payload: { id: string; code: number }) => void) => { + exitListenersB.add(cb) + return () => exitListenersB.delete(cb) + }), listProcesses: vi.fn(async () => [ { id: 'ssh:ssh-b@@pty-1', cwd: '/repo-b', title: 'ssh-b' } ]), @@ -3783,11 +3814,11 @@ describe('registerPtyHandlers', () => { await handlers.get('pty:kill')!(null, { id: 'ssh:ssh-b@@pty-1' }) expect(shutdownA).toHaveBeenCalledWith('ssh:ssh-a@@pty-1', { - immediate: true, + immediate: false, keepHistory: false }) expect(shutdownB).toHaveBeenCalledWith('ssh:ssh-b@@pty-1', { - immediate: true, + immediate: false, keepHistory: false }) }) @@ -8370,13 +8401,21 @@ describe('registerPtyHandlers', () => { rows: 24 })) as { id: string } - await handlers.get('pty:kill')!(null, { id: spawnResult.id }) + vi.useFakeTimers() + const killPromise = handlers.get('pty:kill')!(null, { id: spawnResult.id }) + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) + await killPromise + // Why: the graceful signal (first kill call) intentionally keeps listeners + // attached so the natural exit path can flow. The dispose-before-kill + // NAPI-crash invariant now applies to the SIGKILL escalation. + expect(killSpy.mock.calls[0]).toEqual([]) + expect(killSpy.mock.calls[1]).toEqual(['SIGKILL']) expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + killSpy.mock.invocationCallOrder[1] ) expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + killSpy.mock.invocationCallOrder[1] ) }) @@ -8411,12 +8450,19 @@ describe('registerPtyHandlers', () => { kill: (ptyId: string) => boolean } + vi.useFakeTimers() expect(runtimeController.kill(spawnResult.id)).toBe(true) + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) + + // Why: the graceful signal (first kill call) intentionally keeps listeners + // attached; the dispose-before-kill invariant applies to the escalation. + expect(killSpy.mock.calls[0]).toEqual([]) + expect(killSpy.mock.calls[1]).toEqual(['SIGKILL']) expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + killSpy.mock.invocationCallOrder[1] ) expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + killSpy.mock.invocationCallOrder[1] ) }) @@ -8451,14 +8497,20 @@ describe('registerPtyHandlers', () => { // The first load after spawn only advances generation. The second one sees // this PTY as belonging to a prior page load and kills it as orphaned. + vi.useFakeTimers() didFinishLoad?.() didFinishLoad?.() + // Why: the orphan sweep drains gracefully first; the force-kill (and its + // dispose-before-kill invariant) fires after the fallback window. + await vi.advanceTimersByTimeAsync(GRACEFUL_TEARDOWN_FORCE_KILL_MS) + expect(killSpy.mock.calls[0]).toEqual([]) + expect(killSpy.mock.calls[1]).toEqual(['SIGKILL']) expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + killSpy.mock.invocationCallOrder[1] ) expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan( - killSpy.mock.invocationCallOrder[0] + killSpy.mock.invocationCallOrder[1] ) }) @@ -8607,11 +8659,15 @@ describe('registerPtyHandlers', () => { // First load only advances the generation; the second sees this PTY as a // prior-load orphan. With the flag false the guard must NOT suppress the sweep. + vi.useFakeTimers() didFinishLoad?.() didFinishLoad?.() expect(killSpy).toHaveBeenCalled() expect(runtime.onPtyExit).toHaveBeenCalledWith(spawnResult.id, -1) + // Why: the sweep drains gracefully; the force-kill fallback reaps the + // SIGHUP-ignoring PTY (this stub never exits) after the bounded window. + await vi.advanceTimersByTimeAsync(GRACEFUL_TEARDOWN_FORCE_KILL_MS) const listed = await getLocalPtyProvider().listProcesses() expect(listed.some((info) => info.id === spawnResult.id)).toBe(false) }) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 6848e411c06..debaa5f7efc 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -40,10 +40,14 @@ import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../providers import type { StartupCommandDelivery } from '../../shared/codex-startup-delivery' import { SSH_SESSION_EXPIRED_ERROR, - isSshPtyIdentityMismatchError, - isSshPtyNotFoundError + isSshPtyIdentityMismatchError } from '../providers/ssh-pty-provider' import { parseAppSshPtyId, toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' +import { + APP_QUIT_PTY_DRAIN_MS, + isPtyAlreadyGoneError, + shutdownPtyWithDrain +} from '../providers/pty-shutdown-drain' import { createPtySpawnTiming } from './pty-spawn-timing' import { mintPtySessionId, isSafePtySessionId } from '../daemon/pty-session-id' import { addNodePtyRecoveryHint } from '../daemon/node-pty-error-hints' @@ -428,11 +432,6 @@ function normalizeNodePtySpawnError(err: unknown): Error { return new Error(hintedMessage) } -function isPtyAlreadyGoneError(err: unknown): boolean { - const message = err instanceof Error ? err.message : String(err) - return isSshPtyNotFoundError(err) || /Session not found/i.test(message) -} - function delay(ms: number): Promise { return new Promise((resolve) => { const timer = setTimeout(resolve, ms) @@ -1791,25 +1790,6 @@ export function registerPtyHandlers( mainWindow.webContents.send('pty:exit', payload) } - async function shutdownProviderAndDetectExit( - provider: IPtyProvider, - id: string, - opts: { immediate?: boolean; keepHistory?: boolean } - ): Promise { - let providerExitObserved = false - const unsubscribe = provider.onExit((payload) => { - if (payload.id === id) { - providerExitObserved = true - } - }) - try { - await provider.shutdown(id, opts) - } finally { - unsubscribe() - } - return providerExitObserved - } - // Why: extracted so the "Restart daemon" flow can rebind against the fresh // adapter after replaceDaemonProvider runs. Both the startup registration // and the post-restart rebind go through the same code path — no risk of @@ -2025,6 +2005,10 @@ export function registerPtyHandlers( ptyOwnership.delete(id) markClaudePtyExited(id) runtime?.onPtyExit(id, -1) + // Why: the sweep now drains gracefully, so the spawn-time exit handler + // stays armed and can deliver the natural exit seconds later; swallow + // that duplicate so the fresh renderer never sees a stray pty:exit. + rememberSyntheticKillExit(id) } } didFinishLoadWebContents = mainWindow.webContents @@ -2465,7 +2449,7 @@ export function registerPtyHandlers( // Same synthetic-exit contract as the renderer pty:kill handler: when the // 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 }) + void shutdownPtyWithDrain(provider, ptyId, {}) .then((providerExitObserved) => { finishPtyShutdown(ptyId, connectionId, store) if (!providerExitObserved) { @@ -2514,8 +2498,9 @@ export function registerPtyHandlers( } let providerExitObserved = false try { - providerExitObserved = await shutdownProviderAndDetectExit(provider, ptyId, { - immediate: true, + // Why: graceful-drain-then-force (not immediate) so an agent CLI in + // the pane can flush its transcript before the SIGKILL escalation. + providerExitObserved = await shutdownPtyWithDrain(provider, ptyId, { keepHistory: opts?.keepHistory ?? false }) } catch (err) { @@ -3723,8 +3708,10 @@ export function registerPtyHandlers( const shutdownProvider = provider ?? getProviderForPty(args.id) let providerExitObserved = false try { - providerExitObserved = await shutdownProviderAndDetectExit(shutdownProvider, args.id, { - immediate: true, + // Why: tab close / worktree close land here. Drain gracefully so agent + // CLIs can flush transcripts; shutdownPtyWithDrain force-kills on + // timeout, so this stays bounded and the PTY is dead when we resolve. + providerExitObserved = await shutdownPtyWithDrain(shutdownProvider, args.id, { keepHistory: args.keepHistory ?? false }) } catch (err) { @@ -3929,9 +3916,15 @@ export function registerHeadlessPtyRuntime( /** * Kill all PTY processes. Call on app quit. + * + * Returns a bounded promise: agent CLIs get a short graceful-drain window to + * flush transcripts, then leftovers are force-killed. Quit paths should fold + * this into their async teardown wait (will-quit already defers for the + * daemon disconnect) so the drain completes before the process exits. */ -export function killAllPty(): void { +export function killAllPty(): Promise { if (localProvider instanceof LocalPtyProvider) { - localProvider.killAll() + return localProvider.drainAndKillAll(APP_QUIT_PTY_DRAIN_MS) } + return Promise.resolve() } diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 3fc96c897bf..551ac6a0ff0 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -9,7 +9,10 @@ const { mkdirSyncMock, writeFileSyncMock, spawnMock, - resolveAgentForegroundProcessMock + resolveAgentForegroundProcessMock, + requestWindowsProcessTreeExitMock, + forceKillWindowsProcessTreeMock, + waitForWindowsProcessTreeForceKillMock } = vi.hoisted(() => ({ existsSyncMock: vi.fn(), statSyncMock: vi.fn(), @@ -17,7 +20,18 @@ const { mkdirSyncMock: vi.fn(), writeFileSyncMock: vi.fn(), spawnMock: vi.fn(), - resolveAgentForegroundProcessMock: vi.fn() + resolveAgentForegroundProcessMock: vi.fn(), + requestWindowsProcessTreeExitMock: vi.fn(), + forceKillWindowsProcessTreeMock: vi.fn(), + waitForWindowsProcessTreeForceKillMock: vi.fn() +})) + +// Why: never let provider teardown paths spawn a real `taskkill` from tests — +// on a Windows runner that would signal an arbitrary live pid. +vi.mock('../pty/windows-process-tree-kill', () => ({ + requestWindowsProcessTreeExit: requestWindowsProcessTreeExitMock, + forceKillWindowsProcessTree: forceKillWindowsProcessTreeMock, + waitForWindowsProcessTreeForceKill: waitForWindowsProcessTreeForceKillMock })) vi.mock('fs', () => ({ @@ -122,15 +136,29 @@ describe('LocalPtyProvider', () => { resolveAgentForegroundProcessMock.mockImplementation( async (_pid: number, fallbackProcess: string | null) => fallbackProcess ) + requestWindowsProcessTreeExitMock.mockReset() + forceKillWindowsProcessTreeMock.mockReset() + waitForWindowsProcessTreeForceKillMock.mockReset() + waitForWindowsProcessTreeForceKillMock.mockResolvedValue(undefined) exitCb = undefined + // Why: real node-pty supports multiple onExit listeners (spawn's cleanup + // handler + drainAndKillAll's waiter); mirror that so both fire. + let exitCallbacks: ((info: { exitCode: number }) => void)[] = [] mockProc = { onData: vi.fn(() => ({ dispose: vi.fn() })), onExit: vi.fn((cb: (info: { exitCode: number }) => void) => { - exitCb = cb + exitCallbacks.push(cb) + exitCb = (info) => { + // oxlint-disable-next-line unicorn/no-useless-spread -- copy-safe: listeners may dispose during iteration + for (const registered of [...exitCallbacks]) { + registered(info) + } + } return { dispose: () => { - if (exitCb === cb) { + exitCallbacks = exitCallbacks.filter((registered) => registered !== cb) + if (exitCallbacks.length === 0) { exitCb = undefined } } @@ -954,6 +982,119 @@ describe('LocalPtyProvider', () => { await provider.shutdown('nonexistent', { immediate: true }) expect(mockProc.kill).not.toHaveBeenCalled() }) + + it('graceful shutdown signals SIGHUP and lets the natural exit path clean up', async () => { + const onExit = vi.fn() + provider.configure({ onExit }) + const killSpy = mockProc.kill + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + // mockProc.kill fires the registered exit callback, simulating a shell + // that exits promptly on SIGHUP. + await provider.shutdown(id, { immediate: false }) + + expect(killSpy).toHaveBeenCalledWith() + expect(onExit).toHaveBeenCalledWith(id, -1) + expect(provider.hasPty(id)).toBe(false) + }) + + it('graceful shutdown keeps a SIGHUP-ignoring PTY alive for the drain window', async () => { + const onExit = vi.fn() + provider.configure({ onExit }) + // Why: capture the spy — teardown neutralizes proc.kill afterwards. + const killSpy = vi.fn() // shell ignores SIGHUP: no exit fires + mockProc.kill = killSpy + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + await provider.shutdown(id, { immediate: false }) + + expect(onExit).not.toHaveBeenCalled() + expect(provider.hasPty(id)).toBe(true) + + // The drain caller escalates; the immediate path must SIGKILL. + await provider.shutdown(id, { immediate: true }) + expect(killSpy).toHaveBeenCalledWith('SIGKILL') + expect(onExit).toHaveBeenCalledWith(id, -1) + expect(provider.hasPty(id)).toBe(false) + }) + + it('graceful shutdown on Windows asks the process tree to close', async () => { + 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 }) + + await provider.shutdown(id, { immediate: false }) + + expect(requestWindowsProcessTreeExitMock).toHaveBeenCalledWith(12345) + expect(killSpy).not.toHaveBeenCalled() + expect(provider.hasPty(id)).toBe(true) + + await provider.shutdown(id, { immediate: true }) + expect(waitForWindowsProcessTreeForceKillMock).toHaveBeenCalledWith(12345) + expect(killSpy).toHaveBeenCalled() + expect(provider.hasPty(id)).toBe(false) + }) + + it('awaits Windows force taskkill before clearing local PTY state', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + let releaseForceKill = (): void => { + throw new Error('force kill waiter was not installed') + } + waitForWindowsProcessTreeForceKillMock.mockImplementation( + () => + new Promise((resolve) => { + releaseForceKill = resolve + }) + ) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + let settled = false + const shutdown = provider.shutdown(id, { immediate: true }).then(() => { + settled = true + }) + await Promise.resolve() + + expect(settled).toBe(false) + expect(provider.hasPty(id)).toBe(true) + releaseForceKill() + await shutdown + + expect(settled).toBe(true) + expect(provider.hasPty(id)).toBe(false) + }) + }) + + describe('drainAndKillAll', () => { + it('resolves once PTYs exit inside the drain window without force-killing', async () => { + const killSpy = mockProc.kill + await provider.spawn({ cols: 80, rows: 24 }) + + await provider.drainAndKillAll(1_000) + + // mockProc.kill fires exit synchronously, so no force-kill escalation. + expect(killSpy).toHaveBeenCalledTimes(1) + expect(killSpy).toHaveBeenCalledWith() + }) + + it('force-kills PTYs that outlive the drain window', async () => { + vi.useFakeTimers() + try { + // Why: capture the spy — teardown neutralizes proc.kill afterwards. + const killSpy = vi.fn() // ignores SIGHUP + mockProc.kill = killSpy + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + const drain = provider.drainAndKillAll(1_000) + await vi.advanceTimersByTimeAsync(1_000) + await drain + + expect(killSpy).toHaveBeenCalledWith('SIGKILL') + expect(provider.hasPty(id)).toBe(false) + } finally { + vi.useRealTimers() + } + }) }) describe('hasChildProcesses', () => { diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 5b0e5ea84da..985f338d64b 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -52,6 +52,11 @@ import { } from '../git-bash' import { WINDOWS_GIT_BASH_SHELL } from '../../shared/windows-terminal-shell' import { resolveAgentForegroundProcess } from './agent-foreground-process' +import { + forceKillWindowsProcessTree, + requestWindowsProcessTreeExit, + waitForWindowsProcessTreeForceKill +} from '../pty/windows-process-tree-kill' import { getAgentForegroundContextPaths } from './agent-foreground-context-paths' import { recognizeAgentProcessFromCommandLine } from '../../shared/agent-process-recognition' import { shouldUseShellReadyStartupDelivery } from '../../shared/codex-startup-delivery' @@ -268,20 +273,97 @@ function destroyPtyProcess(proc: pty.IPty, options: { alreadyKilled?: boolean } } /** - * Kills a local PTY and clears all associated local provider state. + * Asks a local PTY's process tree to exit without force-killing it. */ -function safeKillAndClean(id: string, proc: pty.IPty): void { - runPtyCleanup(id) - disposePtyListeners(id) +function requestGracefulPtyExit(proc: pty.IPty): void { + // Why: agent CLIs buffer transcript writes and need a drain window to flush. + // On POSIX, node-pty's kill() sends SIGHUP — the PTY-native hangup signal, + // which shells forward to their foreground job. Deliberately no destroy() + // here: closing the master fd right away would collapse the drain window. + if (process.platform === 'win32') { + requestWindowsProcessTreeExit(proc.pid) + return + } try { proc.kill() } catch { /* Process may already be dead */ } +} + +/** + * Force-kills a local PTY's process tree (the post-drain escalation). + */ +function forceKillPtyProcess(proc: pty.IPty): void { + try { + if (process.platform === 'win32') { + // Why: taskkill /T /F reaps the whole console tree; proc.kill() then + // releases the ConPTY handle itself. + forceKillWindowsProcessTree(proc.pid) + proc.kill() + } else { + // Why: SIGKILL, not SIGHUP — the graceful path already sent SIGHUP and + // this escalation must reap even a signal-ignoring child. + proc.kill('SIGKILL') + } + } catch { + /* Process may already be dead */ + } +} + +async function forceKillPtyProcessAndWait(proc: pty.IPty): Promise { + try { + if (process.platform === 'win32') { + // Why: worktree deletion on Windows needs the tree gone before return, + // otherwise live descendants can keep cwd/files locked with EPERM. + await waitForWindowsProcessTreeForceKill(proc.pid) + proc.kill() + } else { + proc.kill('SIGKILL') + } + } catch { + /* Process may already be dead */ + } +} + +/** + * Force-kills a local PTY and clears all associated local provider state. + */ +function safeKillAndClean(id: string, proc: pty.IPty): void { + runPtyCleanup(id) + disposePtyListeners(id) + forceKillPtyProcess(proc) destroyPtyProcess(proc, { alreadyKilled: true }) clearPtyState(id) } +async function safeKillAndCleanAndWait(id: string, proc: pty.IPty): Promise { + runPtyCleanup(id) + disposePtyListeners(id) + await forceKillPtyProcessAndWait(proc) + destroyPtyProcess(proc, { alreadyKilled: true }) + clearPtyState(id) +} + +/** Force-kill fallback for graceful teardowns that no caller escalates + * (orphan sweeps, app quit): bounded so nothing can linger forever. */ +export const GRACEFUL_TEARDOWN_FORCE_KILL_MS = 5_000 + +/** + * Starts a graceful teardown and arms a bounded force-kill fallback. + * The natural spawn()-time onExit handler owns cleanup when the process + * exits inside the window; the timer only fires for signal-ignoring children. + */ +function beginGracefulPtyTeardown(id: string, proc: pty.IPty, forceAfterMs: number): void { + requestGracefulPtyExit(proc) + const timer = setTimeout(() => { + if (ptyProcesses.get(id) === proc) { + safeKillAndClean(id, proc) + } + }, forceAfterMs) + timer.unref?.() +} + export type LocalPtyProviderOptions = { /** Why: `ctx.command` carries the renderer-chosen launch command (e.g. `pi`, * `omp`, `claude`). Pi vs OMP must drive overlay source-dir selection in @@ -855,11 +937,19 @@ 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 (!opts.immediate) { + // Why: graceful shutdown only signals; the spawn()-time onExit handler + // still owns cleanup + exit fanout when the child drains and exits. The + // caller (pty-shutdown-drain) bounds the wait and escalates to the + // immediate path below on timeout, so nothing can linger forever. + requestGracefulPtyExit(proc) + return + } // Why: disposePtyListeners removes the onExit callback, so the natural // exit cleanup path from node-pty won't fire. Cleanup and notification // must happen unconditionally after the try/catch. @@ -867,11 +957,7 @@ 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 */ - } + await forceKillPtyProcessAndWait(proc) destroyPtyProcess(proc, { alreadyKilled: true }) clearPtyState(id) this.opts.onExit?.(id, -1) @@ -1027,7 +1113,9 @@ export class LocalPtyProvider implements IPtyProvider { const killed: { id: string }[] = [] for (const [id, proc] of ptyProcesses) { if ((ptyLoadGeneration.get(id) ?? -1) < currentGeneration) { - safeKillAndClean(id, proc) + // Why: an orphan can still host an agent CLI mid-flush (reload while + // an agent runs); drain gracefully instead of an instant kill. + beginGracefulPtyTeardown(id, proc, GRACEFUL_TEARDOWN_FORCE_KILL_MS) killed.push({ id }) } } @@ -1044,10 +1132,57 @@ export class LocalPtyProvider implements IPtyProvider { return ptyProcesses.get(id) } - /** Kill all PTYs. Call on app quit. */ + /** Kill all PTYs immediately. Prefer drainAndKillAll on quit paths. */ killAll(): void { for (const [id, proc] of ptyProcesses) { safeKillAndClean(id, proc) } } + + /** Gracefully drain all PTYs, force-killing leftovers after `drainMs`. + * Resolves once every PTY has exited or been force-killed, so quit paths + * can await it inside their bounded async-teardown window. */ + async drainAndKillAll(drainMs: number): Promise { + const entries = Array.from(ptyProcesses.entries()) + await Promise.all( + entries.map( + ([id, proc]) => + new Promise((resolve) => { + let settled = false + let disposable: { dispose: () => void } | undefined + let timer: ReturnType | null = null + const finish = (): void => { + settled = true + if (timer) { + clearTimeout(timer) + timer = null + } + disposable?.dispose() + disposable = undefined + resolve() + } + try { + disposable = proc.onExit(() => finish()) + } catch { + /* already torn down — the timer path still resolves */ + } + requestGracefulPtyExit(proc) + // Why: a shell can exit synchronously inside the graceful signal + // (fast SIGHUP death); don't arm a stale force-kill timer then. + if (settled) { + return + } + timer = setTimeout(() => { + void (async () => { + if (ptyProcesses.get(id) === proc) { + await safeKillAndCleanAndWait(id, proc) + } + finish() + })() + }, drainMs) + timer.unref?.() + }) + ) + ) + } } diff --git a/src/main/providers/pty-shutdown-drain.test.ts b/src/main/providers/pty-shutdown-drain.test.ts new file mode 100644 index 00000000000..d0de8dc12a2 --- /dev/null +++ b/src/main/providers/pty-shutdown-drain.test.ts @@ -0,0 +1,157 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { IPtyProvider } from './types' +import { + PTY_EXIT_DRAIN_WINDOW_MS, + isPtyAlreadyGoneError, + shutdownPtyWithDrain +} from './pty-shutdown-drain' + +type ExitPayload = { id: string; code: number } + +function createProvider(): { + provider: IPtyProvider + shutdown: ReturnType + emitExit: (payload: ExitPayload) => void +} { + const exitListeners = new Set<(payload: ExitPayload) => void>() + const shutdown = vi.fn().mockResolvedValue(undefined) + const provider = { + shutdown, + onExit: (cb: (payload: ExitPayload) => void) => { + exitListeners.add(cb) + return () => exitListeners.delete(cb) + } + } as unknown as IPtyProvider + return { + provider, + shutdown, + emitExit: (payload) => { + for (const listener of exitListeners) { + listener(payload) + } + } + } +} + +describe('shutdownPtyWithDrain', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('does not escalate when the PTY exits inside the drain window', async () => { + const { provider, shutdown, emitExit } = createProvider() + shutdown.mockImplementation(async (id: string, opts: { immediate?: boolean }) => { + if (!opts.immediate) { + emitExit({ id, code: 0 }) + } + }) + + const exited = await shutdownPtyWithDrain(provider, 'pty-1', {}) + + expect(exited).toBe(true) + expect(shutdown).toHaveBeenCalledTimes(1) + expect(shutdown).toHaveBeenCalledWith('pty-1', { immediate: false, keepHistory: undefined }) + }) + + it('ignores exit events for other PTY ids', async () => { + vi.useFakeTimers() + const { provider, shutdown, emitExit } = createProvider() + shutdown.mockImplementation(async (_id: string, opts: { immediate?: boolean }) => { + if (!opts.immediate) { + emitExit({ id: 'unrelated', code: 0 }) + } + }) + + const result = shutdownPtyWithDrain(provider, 'pty-1', {}) + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) + + expect(await result).toBe(false) + expect(shutdown).toHaveBeenLastCalledWith('pty-1', { immediate: true, keepHistory: undefined }) + }) + + it('escalates to an immediate shutdown when the drain window expires', async () => { + vi.useFakeTimers() + const { provider, shutdown } = createProvider() + + const result = shutdownPtyWithDrain(provider, 'pty-1', { keepHistory: true }) + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) + + expect(await result).toBe(false) + expect(shutdown).toHaveBeenCalledTimes(2) + expect(shutdown).toHaveBeenNthCalledWith(1, 'pty-1', { immediate: false, keepHistory: true }) + expect(shutdown).toHaveBeenNthCalledWith(2, 'pty-1', { immediate: true, keepHistory: true }) + }) + + it('honors a caller-provided drain window', async () => { + vi.useFakeTimers() + const { provider, shutdown } = createProvider() + + const result = shutdownPtyWithDrain(provider, 'pty-1', { drainWindowMs: 50 }) + await vi.advanceTimersByTimeAsync(50) + + expect(await result).toBe(false) + expect(shutdown).toHaveBeenCalledTimes(2) + }) + + it('treats an already-gone session during escalation as success', async () => { + vi.useFakeTimers() + const { provider, shutdown } = createProvider() + shutdown.mockImplementation(async (_id: string, opts: { immediate?: boolean }) => { + if (opts.immediate) { + throw new Error('Session not found: pty-1') + } + }) + + const result = shutdownPtyWithDrain(provider, 'pty-1', {}) + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) + + await expect(result).resolves.toBe(false) + }) + + it('propagates non-already-gone escalation failures', async () => { + vi.useFakeTimers() + const { provider, shutdown } = createProvider() + shutdown.mockImplementation(async (_id: string, opts: { immediate?: boolean }) => { + if (opts.immediate) { + throw new Error('relay transport lost') + } + }) + + const result = shutdownPtyWithDrain(provider, 'pty-1', {}) + // Why: attach the rejection expectation before advancing timers so the + // rejection is never unhandled. + const assertion = expect(result).rejects.toThrow('relay transport lost') + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) + await assertion + }) + + it('still escalates to a forced shutdown when the graceful request fails', async () => { + const { provider, shutdown } = createProvider() + shutdown.mockImplementation(async (_id: string, opts: { immediate?: boolean }) => { + if (!opts.immediate) { + throw new Error('boom') + } + }) + + // Why: a failed graceful signal must not leave the PTY un-reaped — callers + // (worktree removal) delete files right after this resolves. + await expect(shutdownPtyWithDrain(provider, 'pty-1', {})).resolves.toBe(false) + expect(shutdown).toHaveBeenCalledTimes(2) + expect(shutdown).toHaveBeenLastCalledWith('pty-1', { immediate: true, keepHistory: undefined }) + }) + + it('propagates already-gone graceful failures without escalating', async () => { + const { provider, shutdown } = createProvider() + shutdown.mockRejectedValue(new Error('Session not found: pty-1')) + + await expect(shutdownPtyWithDrain(provider, 'pty-1', {})).rejects.toThrow('Session not found') + expect(shutdown).toHaveBeenCalledTimes(1) + }) +}) + +describe('isPtyAlreadyGoneError', () => { + it('matches daemon session-not-found errors', () => { + expect(isPtyAlreadyGoneError(new Error('Session not found: abc'))).toBe(true) + expect(isPtyAlreadyGoneError(new Error('relay transport lost'))).toBe(false) + }) +}) diff --git a/src/main/providers/pty-shutdown-drain.ts b/src/main/providers/pty-shutdown-drain.ts new file mode 100644 index 00000000000..aa91e1d952f --- /dev/null +++ b/src/main/providers/pty-shutdown-drain.ts @@ -0,0 +1,94 @@ +import type { IPtyProvider } from './types' +import { isSshPtyNotFoundError } from './ssh-pty-provider' + +/* Why: agent CLIs (Claude Code, Codex) buffer conversation transcript lines + * and only flush at checkpoints. Killing their PTY with SIGKILL in the wrong + * window persists eager metadata but drops the buffered conversation, leaving + * an un-resumable session on disk. Every user-facing teardown path therefore + * shuts down gracefully first (SIGTERM/SIGHUP or Windows taskkill /T inside + * the provider), waits a bounded drain window for the process to exit on its + * own, and only then escalates to a forced kill so nothing can hang forever. */ + +/** Bounded wait for a gracefully-shut-down PTY to exit before force-kill. + * Kept under the daemon/relay-internal 5s SIGKILL fallbacks so escalation + * here is deterministic rather than racing those timers. */ +export const PTY_EXIT_DRAIN_WINDOW_MS = 3_000 + +/** Shorter drain bound for app quit and updater relaunch so quitting stays + * responsive; the leftover force-kill still runs before the process exits. */ +export const APP_QUIT_PTY_DRAIN_MS = 2_000 + +export function isPtyAlreadyGoneError(err: unknown): boolean { + const message = err instanceof Error ? err.message : String(err) + return isSshPtyNotFoundError(err) || /Session not found/i.test(message) +} + +/** + * Shuts a PTY down gracefully, waits up to `drainWindowMs` for the provider + * to report the exit, and escalates to an immediate (forced) shutdown on + * timeout. Resolves only once the PTY is dead or force-killed, so callers + * that delete files afterwards (worktree removal) keep a safe ordering. + * + * Returns whether the provider emitted its own exit event; when false the + * caller owns synthesizing a pty:exit (same contract as the old + * shutdownProviderAndDetectExit helper in ipc/pty.ts). + */ +export async function shutdownPtyWithDrain( + provider: IPtyProvider, + id: string, + opts: { keepHistory?: boolean; drainWindowMs?: number } = {} +): Promise { + const drainWindowMs = opts.drainWindowMs ?? PTY_EXIT_DRAIN_WINDOW_MS + let exited = false + let signalExit: (() => void) | null = null + const exitSeen = new Promise((resolve) => { + signalExit = resolve + }) + const unsubscribe = provider.onExit((payload) => { + if (payload.id === id) { + exited = true + signalExit?.() + } + }) + let drainTimer: ReturnType | null = null + try { + let gracefulFailed = false + try { + await provider.shutdown(id, { immediate: false, keepHistory: opts.keepHistory }) + } catch (err) { + // Why: an already-gone session keeps the old contract (callers map it to + // a synthetic exit). Any other graceful failure must NOT skip the forced + // escalation — callers rely on the PTY being dead when this resolves. + if (isPtyAlreadyGoneError(err)) { + throw err + } + gracefulFailed = true + } + if (!exited && !gracefulFailed) { + await Promise.race([ + exitSeen, + new Promise((resolve) => { + drainTimer = setTimeout(resolve, drainWindowMs) + drainTimer.unref?.() + }) + ]) + } + if (!exited) { + try { + await provider.shutdown(id, { immediate: true, keepHistory: opts.keepHistory }) + } catch (err) { + // Why: the graceful shutdown can still win the race right as the + // window expires; a vanished session is success, not a failure. + if (!exited && !isPtyAlreadyGoneError(err)) { + throw err + } + } + } + return exited + } finally { + unsubscribe() + if (drainTimer) { + clearTimeout(drainTimer) + } + } +} diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index c8e2680366b..1516a2707b0 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -132,6 +132,11 @@ export type IPtyProvider = { */ getAppliedSize?: (id: string) => Promise<{ cols: number; rows: number } | null> + /** Stop the PTY. `immediate: false` is a graceful request (SIGTERM/SIGHUP + * or Windows taskkill /T) that lets agent CLIs flush transcripts before + * exiting; `immediate: true` force-kills. Callers that need death-before- + * return should use shutdownPtyWithDrain, which bounds the graceful wait + * and escalates to immediate on timeout. */ shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise sendSignal(id: string, signal: string): Promise getCwd(id: string): Promise diff --git a/src/main/pty/windows-process-tree-kill.test.ts b/src/main/pty/windows-process-tree-kill.test.ts new file mode 100644 index 00000000000..5dcdf01d961 --- /dev/null +++ b/src/main/pty/windows-process-tree-kill.test.ts @@ -0,0 +1,112 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() })) + +vi.mock('node:child_process', () => ({ spawn: spawnMock })) + +import { + forceKillWindowsProcessTree, + requestWindowsProcessTreeExit, + waitForWindowsProcessTreeForceKill +} from './windows-process-tree-kill' + +function createChildStub(): { + on: ReturnType + once: ReturnType + unref: ReturnType + kill: ReturnType + emit: (event: 'exit' | 'error') => void +} { + const onceHandlers = new Map void>() + return { + on: vi.fn(), + once: vi.fn((event: string, cb: () => void) => { + onceHandlers.set(event, cb) + return undefined + }), + unref: vi.fn(), + kill: vi.fn(), + emit: (event) => onceHandlers.get(event)?.() + } +} + +describe('windows-process-tree-kill', () => { + beforeEach(() => { + spawnMock.mockReset() + spawnMock.mockReturnValue(createChildStub()) + }) + + it('asks the tree to close without /F on the graceful path', () => { + requestWindowsProcessTreeExit(1234) + expect(spawnMock).toHaveBeenCalledWith('taskkill', ['/pid', '1234', '/T'], { + windowsHide: true, + stdio: 'ignore' + }) + }) + + it('forces the tree with /F on the escalation path', () => { + forceKillWindowsProcessTree(1234) + expect(spawnMock).toHaveBeenCalledWith('taskkill', ['/pid', '1234', '/T', '/F'], { + windowsHide: true, + stdio: 'ignore' + }) + }) + + it('detaches from the taskkill child so quit is never blocked', () => { + const child = createChildStub() + spawnMock.mockReturnValue(child) + requestWindowsProcessTreeExit(42) + expect(child.unref).toHaveBeenCalled() + expect(child.on).toHaveBeenCalledWith('error', expect.any(Function)) + }) + + it('ignores invalid pids instead of spawning taskkill', () => { + requestWindowsProcessTreeExit(0) + requestWindowsProcessTreeExit(-1) + requestWindowsProcessTreeExit(Number.NaN) + forceKillWindowsProcessTree(0) + expect(spawnMock).not.toHaveBeenCalled() + }) + + it('swallows spawn failures (taskkill missing)', () => { + spawnMock.mockImplementation(() => { + throw new Error('ENOENT') + }) + expect(() => forceKillWindowsProcessTree(99)).not.toThrow() + }) + + it('waits for force taskkill to exit', async () => { + const child = createChildStub() + spawnMock.mockReturnValue(child) + let settled = false + const wait = waitForWindowsProcessTreeForceKill(99).then(() => { + settled = true + }) + + await Promise.resolve() + expect(settled).toBe(false) + expect(spawnMock).toHaveBeenCalledWith('taskkill', ['/pid', '99', '/T', '/F'], { + windowsHide: true, + stdio: 'ignore' + }) + + child.emit('exit') + await wait + expect(settled).toBe(true) + }) + + it('bounds force taskkill waits', async () => { + vi.useFakeTimers() + try { + const child = createChildStub() + spawnMock.mockReturnValue(child) + const wait = waitForWindowsProcessTreeForceKill(100, 50) + await vi.advanceTimersByTimeAsync(50) + await expect(wait).resolves.toBeUndefined() + expect(child.kill).toHaveBeenCalled() + expect(child.unref).toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/src/main/pty/windows-process-tree-kill.ts b/src/main/pty/windows-process-tree-kill.ts new file mode 100644 index 00000000000..2b18e886897 --- /dev/null +++ b/src/main/pty/windows-process-tree-kill.ts @@ -0,0 +1,101 @@ +import { spawn } from 'node:child_process' + +/* Why: Windows has no SIGTERM-style catchable signal — node-pty's kill() + * tears the ConPTY down immediately, giving agent CLIs (Claude Code, Codex) + * zero time to flush buffered transcript writes. `taskkill /T` (no /F) asks + * every process in the tree to close; the caller's bounded timeout then + * escalates to `taskkill /T /F`. Both are best-effort: the caller always + * follows up with node-pty's own kill/destroy to release the ConPTY. */ + +export const WINDOWS_FORCE_KILL_WAIT_MS = 2_000 + +function isValidPid(pid: number): boolean { + return Number.isFinite(pid) && pid > 0 +} + +function runTaskkill(pid: number, args: string[]): ReturnType | null { + if (!isValidPid(pid)) { + return null + } + return spawn('taskkill', ['/pid', String(pid), ...args], { + windowsHide: true, + stdio: 'ignore' + }) +} + +function runTaskkillDetached(pid: number, args: string[]): void { + if (!Number.isFinite(pid) || pid <= 0) { + return + } + try { + const child = runTaskkill(pid, args) + if (!child) { + return + } + child.on('error', () => { + /* taskkill missing or pid already gone — force paths still run */ + }) + child.unref() + } catch { + /* spawn failure is non-fatal; the force-kill fallback still fires */ + } +} + +/** Ask a Windows process tree to exit gracefully (taskkill /T without /F). */ +export function requestWindowsProcessTreeExit(pid: number): void { + runTaskkillDetached(pid, ['/T']) +} + +/** Force-terminate a Windows process tree (taskkill /T /F). */ +export function forceKillWindowsProcessTree(pid: number): void { + runTaskkillDetached(pid, ['/T', '/F']) +} + +/** Force-terminate and wait a bounded window for taskkill to finish. */ +export function waitForWindowsProcessTreeForceKill( + pid: number, + timeoutMs = WINDOWS_FORCE_KILL_WAIT_MS +): Promise { + if (!isValidPid(pid)) { + return Promise.resolve() + } + return new Promise((resolve) => { + let settled = false + let timer: ReturnType | null = null + let child: ReturnType | null = null + const settle = (): void => { + if (settled) { + return + } + settled = true + if (timer) { + clearTimeout(timer) + timer = null + } + resolve() + } + const settleAfterTimeout = (): void => { + if (child) { + try { + child.kill() + } catch { + /* taskkill may have already exited */ + } + child.unref() + } + settle() + } + try { + child = runTaskkill(pid, ['/T', '/F']) + if (!child) { + settle() + return + } + child.once('exit', settle) + child.once('error', settle) + timer = setTimeout(settleAfterTimeout, timeoutMs) + } catch { + settle() + } + }) +} diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index f72d94c4f9f..cf4f09eeba1 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -54,6 +54,7 @@ import { recentTerminalOutputIncludesPath, type RuntimeTerminalAgentStatusEvent } from './orca-runtime' +import { WORKTREE_PTY_SHUTDOWN_CONCURRENCY } from './worktree-pty-shutdown-concurrency' import { HeadlessEmulator } from '../daemon/headless-emulator' import type { RuntimeMobileSessionTabsResult } from '../../shared/runtime-types' import { @@ -2214,9 +2215,20 @@ describe('OrcaRuntimeService', () => { } } let deletedWorktreeId = '' + // Why: worktree teardown drains via shutdownPtyWithDrain, which subscribes + // onExit and waits for the exit event; emit it so the sweep stays fast. + const providerExitListeners = new Set<(payload: { id: string; code: number }) => void>() const localProvider = { listProcesses: vi.fn(async () => [{ id: `${deletedWorktreeId}@@pty-1` }]), - shutdown: vi.fn(async () => undefined) + shutdown: vi.fn(async (id: string) => { + for (const listener of providerExitListeners) { + listener({ id, code: 0 }) + } + }), + onExit: vi.fn((cb: (payload: { id: string; code: number }) => void) => { + providerExitListeners.add(cb) + return () => providerExitListeners.delete(cb) + }) } const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { getLocalProvider: () => localProvider as never @@ -2279,7 +2291,7 @@ describe('OrcaRuntimeService', () => { deletedWorktreeId = result.worktree.id await expect(runtime.removeManagedWorktree(`id:${result.worktree.id}`)).resolves.toEqual({}) expect(localProvider.shutdown).toHaveBeenCalledWith(`${result.worktree.id}@@pty-1`, { - immediate: true + immediate: false }) expect(metaById[result.worktree.id]).toBeUndefined() expect(deleteWorktreeHistoryDirMock).toHaveBeenCalledWith(result.worktree.id) @@ -20480,6 +20492,56 @@ describe('OrcaRuntimeService', () => { expect(killed).toBe(false) }) + it('stops renderer-known worktree PTYs with bounded concurrency', async () => { + const runtime = new OrcaRuntimeService(store) + const ptyIds = Array.from( + { length: WORKTREE_PTY_SHUTDOWN_CONCURRENCY + 3 }, + (_, index) => `pty-${index}` + ) + let active = 0 + let maxActive = 0 + const kill = vi.fn(() => false) + runtime.setPtyController({ + write: () => true, + kill, + stopAndWait: async (ptyId) => { + active += 1 + maxActive = Math.max(maxActive, active) + await new Promise((resolve) => setTimeout(resolve, 2)) + runtime.onPtyExit(ptyId, -1) + active -= 1 + return true + }, + getForegroundProcess: async () => null + }) + + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { + tabs: [ + { + tabId: 'tab-1', + worktreeId: 'repo-1::/tmp/worktree-a', + title: 'Claude', + activeLeafId: 'pane:1', + layout: null + } + ], + leaves: ptyIds.map((ptyId, index) => ({ + tabId: 'tab-1', + worktreeId: 'repo-1::/tmp/worktree-a', + leafId: `pane:${index + 1}`, + paneRuntimeId: index + 1, + ptyId + })) + }) + + await expect(runtime.stopTerminalsForWorktree('id:repo-1::/tmp/worktree-a')).resolves.toEqual({ + stopped: ptyIds.length + }) + expect(kill).not.toHaveBeenCalled() + expect(maxActive).toBe(WORKTREE_PTY_SHUTDOWN_CONCURRENCY) + }) + it('stops exactly the expected live PTYs for a worktree', async () => { const runtime = new OrcaRuntimeService(store) const stopped: string[] = [] diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 305a15a2127..ac41e1d3e89 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -683,6 +683,7 @@ import { prepareLocalWorktreeRootForRepo } from '../worktree-root-preparation' import { closeLocalWatcherForWorktreePath } from '../ipc/filesystem-watcher' import { HeadlessEmulator, type HeadlessEmulatorOptions } from '../daemon/headless-emulator' import { killAllProcessesForWorktree } from './worktree-teardown' +import { runWorktreePtyShutdownsWithBoundedConcurrency } from './worktree-pty-shutdown-concurrency' import { MOBILE_SUBSCRIBE_SCROLLBACK_ROWS } from './scrollback-limits' import type { IFilesystemProvider, IPtyProvider, PtyProcessInfo } from '../providers/types' import { getSshFilesystemProvider } from '../providers/ssh-filesystem-dispatch' @@ -17380,9 +17381,22 @@ export class OrcaRuntimeService { } } + const controller = this.ptyController + if (!controller) { + return { stopped: 0 } + } + + const stopAndWait = controller.stopAndWait + if (stopAndWait) { + const results = await runWorktreePtyShutdownsWithBoundedConcurrency([...ptyIds], (ptyId) => + stopAndWait(ptyId) + ) + return { stopped: results.filter(Boolean).length } + } + let stopped = 0 for (const ptyId of ptyIds) { - if (this.ptyController?.kill(ptyId)) { + if (controller.kill(ptyId)) { stopped += 1 } } diff --git a/src/main/runtime/worktree-pty-shutdown-concurrency.ts b/src/main/runtime/worktree-pty-shutdown-concurrency.ts new file mode 100644 index 00000000000..e7ef27da498 --- /dev/null +++ b/src/main/runtime/worktree-pty-shutdown-concurrency.ts @@ -0,0 +1,25 @@ +export const WORKTREE_PTY_SHUTDOWN_CONCURRENCY = 6 + +export async function runWorktreePtyShutdownsWithBoundedConcurrency( + ids: string[], + stopPty: (id: string) => Promise +): Promise { + const results: T[] = Array.from({ length: ids.length }) + let nextIndex = 0 + + const runNext = async (): Promise => { + for (;;) { + const index = nextIndex + nextIndex += 1 + if (index >= ids.length) { + return + } + results[index] = await stopPty(ids[index]) + } + } + + await Promise.all( + Array.from({ length: Math.min(WORKTREE_PTY_SHUTDOWN_CONCURRENCY, ids.length) }, () => runNext()) + ) + return results +} diff --git a/src/main/runtime/worktree-teardown.test.ts b/src/main/runtime/worktree-teardown.test.ts index 63e207cd8da..18e5f78a99a 100644 --- a/src/main/runtime/worktree-teardown.test.ts +++ b/src/main/runtime/worktree-teardown.test.ts @@ -9,17 +9,29 @@ vi.mock('../memory/pty-registry', () => ({ })) import { killAllProcessesForWorktree } from './worktree-teardown' +import { WORKTREE_PTY_SHUTDOWN_CONCURRENCY } from './worktree-pty-shutdown-concurrency' import type { IPtyProvider } from '../providers/types' +import { PTY_EXIT_DRAIN_WINDOW_MS } from '../providers/pty-shutdown-drain' function createProviderStub( listProcesses: () => Promise<{ id: string; cwd: string; title: string }[]> ): IPtyProvider { + // Why: the teardown sweeps now shut down via shutdownPtyWithDrain, which + // waits for the provider's exit event. Emit it on the graceful call so + // tests exercise the fast path instead of the real drain-window timeout. + const exitListeners = new Set<(payload: { id: string; code: number }) => void>() return { spawn: vi.fn(), attach: vi.fn(), write: vi.fn(), resize: vi.fn(), - shutdown: vi.fn().mockResolvedValue(undefined), + shutdown: vi.fn().mockImplementation(async (id: string, opts: { immediate?: boolean }) => { + if (!opts.immediate) { + for (const listener of exitListeners) { + listener({ id, code: 0 }) + } + } + }), sendSignal: vi.fn(), getCwd: vi.fn(), getInitialCwd: vi.fn(), @@ -34,7 +46,10 @@ function createProviderStub( getProfiles: vi.fn(), onData: vi.fn().mockReturnValue(() => {}), onReplay: vi.fn().mockReturnValue(() => {}), - onExit: vi.fn().mockReturnValue(() => {}) + onExit: vi.fn().mockImplementation((cb: (payload: { id: string; code: number }) => void) => { + exitListeners.add(cb) + return () => exitListeners.delete(cb) + }) } as unknown as IPtyProvider } @@ -61,10 +76,10 @@ describe('killAllProcessesForWorktree', () => { expect(result.providerStopped).toBe(1) expect(result.registryStopped).toBe(1) - expect(localProvider.shutdown).toHaveBeenCalledWith('w1@@abcd1234', { immediate: true }) - expect(localProvider.shutdown).toHaveBeenCalledWith('w1-registry-1', { immediate: true }) - expect(localProvider.shutdown).not.toHaveBeenCalledWith('w2@@efef5678', { immediate: true }) - expect(localProvider.shutdown).not.toHaveBeenCalledWith('w2-registry-2', { immediate: true }) + expect(localProvider.shutdown).toHaveBeenCalledWith('w1@@abcd1234', { immediate: false }) + expect(localProvider.shutdown).toHaveBeenCalledWith('w1-registry-1', { immediate: false }) + expect(localProvider.shutdown).not.toHaveBeenCalledWith('w2@@efef5678', { immediate: false }) + expect(localProvider.shutdown).not.toHaveBeenCalledWith('w2-registry-2', { immediate: false }) expect(onPtyStopped).toHaveBeenCalledWith('w1@@abcd1234') expect(onPtyStopped).toHaveBeenCalledWith('w1-registry-1') expect(onPtyStopped).not.toHaveBeenCalledWith('w2@@efef5678') @@ -87,7 +102,7 @@ describe('killAllProcessesForWorktree', () => { // Prefix sweep must kill nothing; registry sweep must still fire. expect(result.providerStopped).toBe(0) expect(result.registryStopped).toBe(1) - expect(localProvider.shutdown).toHaveBeenCalledWith('1', { immediate: true }) + expect(localProvider.shutdown).toHaveBeenCalledWith('1', { immediate: false }) expect(localProvider.shutdown).toHaveBeenCalledTimes(1) expect(onPtyStopped).toHaveBeenCalledWith('1') }) @@ -136,16 +151,81 @@ describe('killAllProcessesForWorktree', () => { listRegisteredPtysMock.mockReturnValue([]) const r1 = await killAllProcessesForWorktree('w1', { localProvider: providerA }) - expect(providerA.shutdown).toHaveBeenCalledWith('w1@@aaaa', { immediate: true }) + expect(providerA.shutdown).toHaveBeenCalledWith('w1@@aaaa', { immediate: false }) expect(providerB.shutdown).not.toHaveBeenCalled() expect(r1.providerStopped).toBe(1) const r2 = await killAllProcessesForWorktree('w1', { localProvider: providerB }) - expect(providerB.shutdown).toHaveBeenCalledWith('w1@@bbbb', { immediate: true }) + expect(providerB.shutdown).toHaveBeenCalledWith('w1@@bbbb', { immediate: false }) expect(providerB.shutdown).toHaveBeenCalledTimes(1) expect(r2.providerStopped).toBe(1) }) + it('drains stubborn provider sessions concurrently', async () => { + vi.useFakeTimers() + try { + const localProvider = createProviderStub(async () => [ + { id: 'w1@@aaaa', cwd: '/tmp', title: 'shell' }, + { id: 'w1@@bbbb', cwd: '/tmp', title: 'shell' } + ]) + ;(localProvider.shutdown as unknown as ReturnType).mockResolvedValue(undefined) + listRegisteredPtysMock.mockReturnValue([]) + + let settled = false + const result = killAllProcessesForWorktree('w1', { localProvider }).then((value) => { + settled = true + return value + }) + + await vi.advanceTimersByTimeAsync(PTY_EXIT_DRAIN_WINDOW_MS) + expect(settled).toBe(true) + await expect(result).resolves.toMatchObject({ providerStopped: 2 }) + expect(localProvider.shutdown).toHaveBeenCalledWith('w1@@aaaa', { immediate: true }) + expect(localProvider.shutdown).toHaveBeenCalledWith('w1@@bbbb', { immediate: true }) + } finally { + vi.useRealTimers() + } + }) + + it('bounds concurrent provider shutdown work', async () => { + const ids = Array.from({ length: WORKTREE_PTY_SHUTDOWN_CONCURRENCY + 3 }, (_, index) => ({ + id: `w1@@${index}`, + cwd: '/tmp', + title: 'shell' + })) + const exitListeners = new Set<(payload: { id: string; code: number }) => void>() + let active = 0 + let maxActive = 0 + const localProvider = createProviderStub(async () => ids) + ;(localProvider.onExit as unknown as ReturnType).mockImplementation( + (cb: (payload: { id: string; code: number }) => void) => { + exitListeners.add(cb) + return () => exitListeners.delete(cb) + } + ) + ;(localProvider.shutdown as unknown as ReturnType).mockImplementation( + async (id: string, opts: { immediate?: boolean }) => { + if (opts.immediate) { + return + } + active += 1 + maxActive = Math.max(maxActive, active) + await new Promise((resolve) => setTimeout(resolve, 2)) + for (const listener of exitListeners) { + listener({ id, code: 0 }) + } + active -= 1 + } + ) + listRegisteredPtysMock.mockReturnValue([]) + + const result = await killAllProcessesForWorktree('w1', { localProvider }) + + expect(result.providerStopped).toBe(ids.length) + expect(maxActive).toBeGreaterThan(1) + expect(maxActive).toBeLessThanOrEqual(WORKTREE_PTY_SHUTDOWN_CONCURRENCY) + }) + it('invokes runtime.stopTerminalsForWorktree when runtime is provided', async () => { const stopTerminalsForWorktree = vi.fn().mockResolvedValue({ stopped: 3 }) const runtime = { diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 7572b9bbd77..60bad951109 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -1,6 +1,8 @@ import type { IPtyProvider } from '../providers/types' import type { OrcaRuntimeService } from './orca-runtime' import { listRegisteredPtys } from '../memory/pty-registry' +import { shutdownPtyWithDrain } from '../providers/pty-shutdown-drain' +import { runWorktreePtyShutdownsWithBoundedConcurrency } from './worktree-pty-shutdown-concurrency' export type WorktreeTeardownDeps = { runtime?: OrcaRuntimeService @@ -51,15 +53,18 @@ export async function killAllProcessesForWorktree( result.runtimeStopped = r.stopped } + const stoppedProviderPtys = new Set() result.providerStopped = await sweepProviderByPrefix( worktreeId, deps.localProvider, - deps.onPtyStopped + deps.onPtyStopped, + stoppedProviderPtys ) result.registryStopped = await sweepRegistryForWorktree( worktreeId, deps.localProvider, - deps.onPtyStopped + deps.onPtyStopped, + stoppedProviderPtys ) return result @@ -68,44 +73,60 @@ export async function killAllProcessesForWorktree( async function sweepProviderByPrefix( worktreeId: string, provider: IPtyProvider, - onPtyStopped?: (ptyId: string) => void + onPtyStopped: ((ptyId: string) => void) | undefined, + stoppedProviderPtys: Set ): Promise { const prefix = `${worktreeId}@@` const sessions = await provider.listProcesses().catch(() => []) - let killed = 0 - for (const s of sessions) { - if (!s.id.startsWith(prefix)) { - continue - } - 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 + const ids = [...new Set(sessions.filter((s) => s.id.startsWith(prefix)).map((s) => s.id))] + const results = await runWorktreePtyShutdownsWithBoundedConcurrency(ids, (id) => + stopPtyForWorktree(provider, id, onPtyStopped, true) + ) + for (const [index, result] of results.entries()) { + if (result.cleared) { + stoppedProviderPtys.add(ids[index]) } } - return killed + return results.filter((result) => result.counted).length } async function sweepRegistryForWorktree( worktreeId: string, localProvider: IPtyProvider, - onPtyStopped?: (ptyId: string) => void + onPtyStopped: ((ptyId: string) => void) | undefined, + stoppedProviderPtys: Set ): Promise { - const entries = listRegisteredPtys().filter((r) => r.worktreeId === worktreeId) - let killed = 0 - for (const entry of entries) { - try { - await localProvider.shutdown(entry.ptyId, { immediate: true }) - clearStoppedPtyState(entry.ptyId, onPtyStopped) - killed += 1 - } catch { - /* ignore — best-effort */ - } + const ids = [ + ...new Set( + listRegisteredPtys() + .filter((r) => r.worktreeId === worktreeId && !stoppedProviderPtys.has(r.ptyId)) + .map((r) => r.ptyId) + ) + ] + const results = await runWorktreePtyShutdownsWithBoundedConcurrency(ids, (id) => + stopPtyForWorktree(localProvider, id, onPtyStopped, false) + ) + return results.filter((result) => result.counted).length +} + +async function stopPtyForWorktree( + provider: IPtyProvider, + ptyId: string, + onPtyStopped: ((ptyId: string) => void) | undefined, + countErrorsAsStopped: boolean +): Promise<{ counted: boolean; cleared: boolean }> { + try { + // Why: drain-then-force keeps this bounded and reaps the PTY before + // git-level worktree removal deletes files it may hold open (best-effort + // on Windows — the taskkill wait resolves on a bounded timeout). + await shutdownPtyWithDrain(provider, ptyId, {}) + clearStoppedPtyState(ptyId, onPtyStopped) + return { counted: true, cleared: true } + } catch { + // Provider-list entries can disappear between list and kill; preserve the + // old best-effort count while registry cleanup stays stricter. + return { counted: countErrorsAsStopped, cleared: false } } - return killed } function clearStoppedPtyState(ptyId: string, onPtyStopped?: (ptyId: string) => void): void { diff --git a/src/main/updater.ts b/src/main/updater.ts index 42924bdf08c..0bf77a17d2d 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -567,7 +567,9 @@ async function performQuitAndInstall(): Promise { quittingForUpdate = true await runBeforeUpdateQuitCleanup() - killAllPty() + // Why: bounded drain (graceful signal → force-kill) so agent CLIs flush + // their transcripts before ShipIt replaces the app bundle. + await killAllPty() for (const win of BrowserWindow.getAllWindows()) { win.removeAllListeners('close') diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 7d5198e87a9..6d3f32015f6 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -31,6 +31,22 @@ vi.mock('node-pty', () => ({ spawn: mockPtySpawn })) +const { + requestWindowsProcessTreeExitMock, + forceKillWindowsProcessTreeMock, + waitForWindowsProcessTreeForceKillMock +} = vi.hoisted(() => ({ + requestWindowsProcessTreeExitMock: vi.fn(), + forceKillWindowsProcessTreeMock: vi.fn(), + waitForWindowsProcessTreeForceKillMock: vi.fn() +})) + +vi.mock('../main/pty/windows-process-tree-kill', () => ({ + requestWindowsProcessTreeExit: requestWindowsProcessTreeExitMock, + forceKillWindowsProcessTree: forceKillWindowsProcessTreeMock, + waitForWindowsProcessTreeForceKill: waitForWindowsProcessTreeForceKillMock +})) + import { PtyHandler, attachIdentityMismatches } from './pty-handler' import type { RelayDispatcher } from './dispatcher' @@ -100,6 +116,10 @@ describe('PtyHandler', () => { mockPtyInstance.resize.mockReset() mockPtyInstance.kill.mockReset() mockPtyInstance.clear.mockReset() + requestWindowsProcessTreeExitMock.mockReset() + forceKillWindowsProcessTreeMock.mockReset() + waitForWindowsProcessTreeForceKillMock.mockReset() + waitForWindowsProcessTreeForceKillMock.mockResolvedValue(undefined) mockPtySpawn.mockReturnValue({ ...mockPtyInstance }) @@ -1515,6 +1535,112 @@ describe('PtyHandler', () => { expect(handler.activePtyCount).toBe(0) }) + it('immediate Windows shutdown does not double-notify if onExit wins the taskkill wait', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { + configurable: true, + value: 'win32' + }) + let releaseForceKill = (): void => { + throw new Error('force kill waiter was not installed') + } + waitForWindowsProcessTreeForceKillMock.mockImplementation( + () => + new Promise((resolve) => { + releaseForceKill = resolve + }) + ) + let onExitCb: ((evt: { exitCode: number }) => void) | undefined + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + pid: 1234, + onData: vi.fn(), + onExit: vi.fn((cb: (evt: { exitCode: number }) => void) => { + onExitCb = cb + }) + }) + const exits: { id: string; paneKey?: string }[] = [] + handler.setExitListener((evt) => exits.push(evt)) + + try { + await dispatcher.callRequest('pty.spawn', { + env: { ORCA_PANE_KEY: 'tab-shutdown:0' } + }) + const shutdown = dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + await Promise.resolve() + expect(waitForWindowsProcessTreeForceKillMock).toHaveBeenCalledWith(1234) + + onExitCb!({ exitCode: 0 }) + releaseForceKill() + await shutdown + + expect(exits).toEqual([{ id: 'pty-1', paneKey: 'tab-shutdown:0' }]) + expect( + dispatcher._notifications.filter((notification) => notification.method === 'pty.exit') + ).toEqual([{ method: 'pty.exit', params: { id: 'pty-1', code: 0 } }]) + expect(handler.activePtyCount).toBe(0) + } finally { + Object.defineProperty(process, 'platform', { + configurable: true, + value: originalPlatform + }) + } + }) + + it('Windows graceful fallback does not synthesize exit after natural exit wins taskkill', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { + configurable: true, + value: 'win32' + }) + let releaseForceKill = (): void => { + throw new Error('force kill waiter was not installed') + } + waitForWindowsProcessTreeForceKillMock.mockImplementation( + () => + new Promise((resolve) => { + releaseForceKill = resolve + }) + ) + let onExitCb: ((evt: { exitCode: number }) => void) | undefined + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + pid: 1234, + onData: vi.fn(), + onExit: vi.fn((cb: (evt: { exitCode: number }) => void) => { + onExitCb = cb + }) + }) + const exits: { id: string; paneKey?: string }[] = [] + handler.setExitListener((evt) => exits.push(evt)) + + try { + await dispatcher.callRequest('pty.spawn', { + env: { ORCA_PANE_KEY: 'tab-fallback:0' } + }) + await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false }) + vi.advanceTimersByTime(5000) + await Promise.resolve() + expect(waitForWindowsProcessTreeForceKillMock).toHaveBeenCalledWith(1234) + + onExitCb!({ exitCode: 0 }) + releaseForceKill() + await Promise.resolve() + await Promise.resolve() + + expect(exits).toEqual([{ id: 'pty-1', paneKey: 'tab-fallback:0' }]) + expect( + dispatcher._notifications.filter((notification) => notification.method === 'pty.exit') + ).toEqual([{ method: 'pty.exit', params: { id: 'pty-1', code: 0 } }]) + expect(handler.activePtyCount).toBe(0) + } finally { + Object.defineProperty(process, 'platform', { + configurable: true, + value: originalPlatform + }) + } + }) + it('dispose kills all PTYs with SIGKILL and invokes exit listeners', async () => { const mockKill = vi.fn() mockPtySpawn.mockReturnValue({ diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 3a812415909..7e55503c532 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -2,6 +2,11 @@ import type { IPty } from 'node-pty' import type * as NodePty from 'node-pty' import { resolveWindowsGitBashShellPath } from '../main/git-bash' +import { + forceKillWindowsProcessTree, + requestWindowsProcessTreeExit, + waitForWindowsProcessTreeForceKill +} from '../main/pty/windows-process-tree-kill' import { WINDOWS_GIT_BASH_SHELL } from '../shared/windows-terminal-shell' import type { RelayDispatcher, RequestContext } from './dispatcher' import { @@ -83,6 +88,42 @@ type ManagedStartupCommand = { timer: ReturnType | null } +/** Ask a PTY's process tree to exit gracefully so agent CLIs can flush. */ +function requestPtyTreeExit(proc: IPty): void { + // Why: Windows node-pty kill() ignores the signal and tears the ConPTY down + // immediately; taskkill /T keeps the graceful window real on Windows hosts. + if (process.platform === 'win32') { + requestWindowsProcessTreeExit(proc.pid) + return + } + proc.kill('SIGTERM') +} + +/** Force-kill a PTY's process tree (post-drain escalation). */ +function forceKillPtyTree(proc: IPty): void { + if (process.platform === 'win32') { + forceKillWindowsProcessTree(proc.pid) + } + // Why: called from bare setTimeout callbacks — a throw on an already-torn- + // down ConPTY would otherwise crash the relay process. + try { + proc.kill('SIGKILL') + } catch { + /* child may already be dead */ + } +} + +async function forceKillPtyTreeAndWait(proc: IPty): Promise { + try { + if (process.platform === 'win32') { + await waitForWindowsProcessTreeForceKill(proc.pid) + } + proc.kill('SIGKILL') + } catch { + /* child may already be dead */ + } +} + function disposeManagedPty(managed: ManagedPty): void { if (managed.disposed) { return @@ -657,11 +698,11 @@ export class PtyHandler { // response is discarded and no renderer can own this PTY. Shut it down // immediately so it does not linger as an unreachable remote shell. this.releaseStartupCommand(managed) - term.kill('SIGTERM') + requestPtyTreeExit(term) managed.killTimer = setTimeout(() => { const still = this.ptys.get(id) if (still && !still.disposed) { - still.pty.kill('SIGKILL') + forceKillPtyTree(still.pty) // Why: stale-spawn cleanup has no client who will ever attach. If // SIGKILL's onExit is missed (kernel edge case, uninterruptible // sleep), the managed entry + ptmx fd would leak forever. Dispose @@ -786,7 +827,10 @@ export class PtyHandler { if (immediate) { this.releaseStartupCommand(managed) this.flushPtyOutput(id) - managed.pty.kill('SIGKILL') + await forceKillPtyTreeAndWait(managed.pty) + if (this.ptys.get(id) !== managed || managed.disposed) { + return + } // Why: SIGKILL has already reaped the child; release the ptmx fd on the // same tick. Deferring to onExit leaves a window where the fd is live // with a dead child. Idempotent via the disposed guard — if onExit fires @@ -805,7 +849,7 @@ export class PtyHandler { this.clearPtyFlowState(id) } else { this.releaseStartupCommand(managed) - managed.pty.kill('SIGTERM') + requestPtyTreeExit(managed.pty) // Why: Some processes ignore SIGTERM (e.g. a hung child, a custom signal // handler). Without a SIGKILL fallback the PTY process would leak and the @@ -819,7 +863,22 @@ export class PtyHandler { managed.killTimer = setTimeout(() => { const still = this.ptys.get(id) if (still && !still.disposed) { - still.pty.kill('SIGKILL') + if (process.platform === 'win32') { + void (async () => { + await forceKillPtyTreeAndWait(still.pty) + if (this.ptys.get(id) !== still || still.disposed) { + return + } + this.flushPtyOutput(id) + this.dispatcher.notify('pty.exit', { id, code: -1 }) + this.notifyExitListener(still) + disposeManagedPty(still) + this.ptys.delete(id) + this.clearPtyFlowState(id) + })() + return + } + forceKillPtyTree(still.pty) this.flushPtyOutput(id) // Why: emit pty.exit BEFORE disposeManagedPty sets disposed=true. // The natural onExit short-circuits on `managed.disposed`, so @@ -1068,7 +1127,7 @@ export class PtyHandler { // ptmx fd release via disposeManagedPty is synchronous, so there is // no graceful-shutdown window to preserve at this point. try { - managed.pty.kill('SIGKILL') + forceKillPtyTree(managed.pty) } catch { /* child may already be dead */ }