diff --git a/docs/reference/terminal-session-lifecycle.md b/docs/reference/terminal-session-lifecycle.md index 29161cd6fdf..6836fce7357 100644 --- a/docs/reference/terminal-session-lifecycle.md +++ b/docs/reference/terminal-session-lifecycle.md @@ -74,8 +74,13 @@ could invalidate intentional long-lived `worktree-sleep` checkpoints. - Changing terminal output, snapshot, replay, query-response, or hidden-delivery behavior. -- Killing processes that deliberately daemonize away from the terminal process - group. This contract covers the PTY and its attached foreground process tree. +- Killing processes that deliberately daemonize away from a plain user + terminal's process group (nohup-style survivors remain user intent there). + For **agent sessions**, close/kill additionally terminates the snapshotted + descendant tree — including detached-pgid children the PTY's SIGHUP cannot + reach — via `pty-descendant-termination.ts` (SIGTERM, grace window, then + identity-checked SIGKILL). Windows and SSH-hosted PTYs keep the previous + foreground-tree contract for now. - Changing agent-provider resume commands or permission flags. - Making a UI close wait for a remote process to exit before the tab disappears. - Replacing worktree sleep with tab close. Sleep remains resumable by design. diff --git a/src/main/daemon/session.test.ts b/src/main/daemon/session.test.ts index ade8b18fe15..98192d19fe7 100644 --- a/src/main/daemon/session.test.ts +++ b/src/main/daemon/session.test.ts @@ -1,6 +1,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { PRODUCER_PAUSE_FAILSAFE_MS, Session } from './session' import type { SessionState, ShellReadyState } from './types' +import type { TuiAgent } from '../../shared/types' + +const killWithDescendantSweepMock = vi.hoisted(() => vi.fn()) +vi.mock('../pty-descendant-termination', () => ({ + killWithDescendantSweep: killWithDescendantSweepMock +})) // Stub the subprocess — Session talks to it via an interface, not child_process directly. function createMockSubprocess() { @@ -86,6 +92,7 @@ describe('Session', () => { beforeEach(() => { vi.useFakeTimers() subprocess = createMockSubprocess() + killWithDescendantSweepMock.mockReset() }) afterEach(() => { @@ -98,11 +105,13 @@ describe('Session', () => { shellReadyTimeoutMs?: number cols?: number rows?: number + launchAgent?: TuiAgent }): Session { session = new Session({ sessionId: 'test-session', cols: opts?.cols ?? 80, rows: opts?.rows ?? 24, + ...(opts?.launchAgent ? { launchAgent: opts.launchAgent } : {}), subprocess, shellReadySupported: opts?.shellReadySupported ?? false, ...(opts?.shellReadyTimeoutMs !== undefined @@ -406,6 +415,33 @@ describe('Session', () => { expect(session.isTerminating).toBe(true) }) + it('non-agent kill stays synchronous and never routes through the descendant sweep', () => { + createSession() + session.kill() + expect(subprocess.killed).toBe(true) + expect(killWithDescendantSweepMock).not.toHaveBeenCalled() + }) + + it('agent kill routes through the descendant sweep with the subprocess as root', () => { + createSession({ launchAgent: 'claude' }) + session.kill() + expect(killWithDescendantSweepMock).toHaveBeenCalledWith(subprocess.pid, expect.any(Function)) + // The root kill is deferred to the sweep's snapshot-first sequencing. + expect(subprocess.killed).toBe(false) + const killRoot = killWithDescendantSweepMock.mock.calls[0][1] as () => void + killRoot() + expect(subprocess.killed).toBe(true) + }) + + it('agent kill root callback is a no-op after the session already exited', () => { + createSession({ launchAgent: 'claude' }) + session.kill() + const killRoot = killWithDescendantSweepMock.mock.calls[0][1] as () => void + subprocess.simulateExit(0) + killRoot() + expect(subprocess.killed).toBe(false) + }) + it('notifies attached clients on exit after kill', async () => { vi.useRealTimers() createSession() diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index b9c82a48174..b42113d3aea 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -9,6 +9,7 @@ import { type ShellReadyScanState } from '../shell-ready-marker-scanner' import { isPowerShellProcess } from '../../shared/shell-process-detection' +import { killWithDescendantSweep } from '../pty-descendant-termination' import type { TuiAgent } from '../../shared/types' import type { PendingOutputRecord, @@ -267,7 +268,19 @@ export class Session { // Why: a paused child can be blocked inside write(); resume before // signalling so it can run signal handlers and actually exit. this.releaseProducerPause({ resume: true }) - this.subprocess.kill() + if (!this.launchAgent) { + this.subprocess.kill() + } else { + // Why: agent tool children live in detached process groups a dying + // shell's SIGHUP never reaches. The bounded snapshot briefly defers the + // signal; the kill timer below starts now, so force-dispose timing is + // unaffected. + killWithDescendantSweep(this.subprocess.pid, () => { + if (this._state !== 'exited') { + this.subprocess.kill() + } + }) + } this.killTimer = setTimeout(() => { if (this._state !== 'exited') { diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index 46e495e5f2e..9a31cc385b4 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -2,6 +2,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { Session, type SubprocessHandle } from './session' import { TerminalHost } from './terminal-host' +const killWithDescendantSweepMock = vi.hoisted(() => vi.fn()) +vi.mock('../pty-descendant-termination', () => ({ + killWithDescendantSweep: killWithDescendantSweepMock +})) + function createMockSubprocess( options: { startupCommandDeliveredInShellArgs?: boolean; shellPath?: string } = {} ): SubprocessHandle { @@ -56,6 +61,7 @@ describe('TerminalHost', () => { } beforeEach(() => { + killWithDescendantSweepMock.mockReset() spawnFn = vi.fn(() => { const sub = createMockSubprocess() as ReturnType & { _onDataCb: ((data: string) => void) | null @@ -393,6 +399,43 @@ describe('TerminalHost', () => { it('throws for non-existent session', () => { expect(() => host.kill('missing')).toThrow('Session not found') }) + + it('non-agent immediate kill stays synchronous and never routes through the descendant sweep', async () => { + await host.createOrAttach({ + sessionId: 'plain-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + + host.kill('plain-1', { immediate: true }) + + expect(lastSubprocess.forceKill).toHaveBeenCalled() + expect(killWithDescendantSweepMock).not.toHaveBeenCalled() + }) + + it('agent immediate kill routes through the descendant sweep and defers the force-kill to it', async () => { + await host.createOrAttach({ + sessionId: 'agent-1', + cols: 80, + rows: 24, + launchAgent: 'claude', + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + + host.kill('agent-1', { immediate: true }) + + // Why order matters: force-killing first would let orphans reparent to + // pid 1 and escape the sweep's ppid walk entirely. + expect(killWithDescendantSweepMock).toHaveBeenCalledWith(99999, expect.any(Function)) + expect(lastSubprocess.forceKill).not.toHaveBeenCalled() + expect(host.isKilled('agent-1')).toBe(true) + + const finish = killWithDescendantSweepMock.mock.calls[0][1] as () => void + finish() + expect(lastSubprocess.forceKill).toHaveBeenCalled() + expect(lastSubprocess.dispose).toHaveBeenCalled() + }) }) describe('signal', () => { diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index b15420fed14..ddd43071eb3 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -6,6 +6,7 @@ import type { StartupCommandDelivery } from '../../shared/codex-startup-delivery import { buildStartupCommandSubmission } from '../../shared/startup-command-submission' import type { SessionInfo, TakePendingOutputResult, TerminalSnapshot } from './types' import { SessionNotFoundError } from './types' +import { killWithDescendantSweep } from '../pty-descendant-termination' import type { CreateOrAttachOptions, CreateOrAttachResult } from './terminal-host-create-contract' export type { CreateOrAttachOptions, CreateOrAttachResult } from './terminal-host-create-contract' @@ -197,11 +198,20 @@ export class TerminalHost { const session = this.getAliveSession(sessionId) this.recordTombstone(sessionId) if (opts.immediate) { - session.forceKillAndDisposeSubprocess() - // Why: the immediate path tears down synchronously without firing the - // session's onExit hook, so reap it here. The graceful path below funnels - // through Session.handleSubprocessExit -> onExit -> reapSession. - this.reapSession(sessionId) + const finish = (): void => { + session.forceKillAndDisposeSubprocess() + // Why: the immediate path tears down synchronously without firing the + // session's onExit hook, so reap it here. The graceful path below + // funnels through Session.handleSubprocessExit -> onExit -> reapSession. + this.reapSession(sessionId) + } + if (!session.launchAgent) { + finish() + return + } + // Why: agent tool children live in detached process groups the shell's + // death never signals; the bounded snapshot defers teardown at most ~1s. + killWithDescendantSweep(session.pid, finish) return } session.kill() diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 3d9962fcb48..e8176df72fc 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -54,6 +54,10 @@ import { WINDOWS_GIT_BASH_SHELL } from '../../shared/windows-terminal-shell' import { resolveAgentForegroundProcessWithAvailability } from './agent-foreground-process' import { getAgentForegroundContextPaths } from './agent-foreground-context-paths' import { recognizeAgentProcessFromCommandLine } from '../../shared/agent-process-recognition' +import { + captureDescendantSnapshot, + terminateDescendantSnapshot +} from '../pty-descendant-termination' import { readWindowsConptyProcessIds } from './windows-conpty-process-membership' import { shouldUseShellReadyStartupDelivery } from '../../shared/codex-startup-delivery' import { assertSafeAgentStartupCwd, resolveSafePtyDefaultCwd } from './pty-default-cwd' @@ -68,6 +72,11 @@ const PANE_IDENTITY_ENV_KEYS = [ let ptyCounter = 0 const ptyProcesses = new Map() +// Why: only agent sessions get descendant tree-kill on shutdown. Agent CLIs +// spawn tool children in detached process groups the PTY's SIGHUP can never +// reach; plain user terminals keep classic semantics where deliberately +// detached (nohup-style) children survive the pane. +const ptyAgentSessionIds = new Set() const ptyShellName = new Map() const ptyAgentForegroundContextPaths = new Map() const ptyTerminalHandle = new Map() @@ -180,6 +189,7 @@ function clearPtyState(id: string): void { runPtyCleanup(id) disposePtyListeners(id) ptyProcesses.delete(id) + ptyAgentSessionIds.delete(id) ptyShellName.delete(id) ptyAgentForegroundContextPaths.delete(id) ptyTerminalHandle.delete(id) @@ -689,6 +699,12 @@ export class LocalPtyProvider implements IPtyProvider { const proc = spawnResult.process ptyProcesses.set(id, proc) + // Why both signals: launchAgent is the caller's explicit intent and + // survives command rewriting (e.g. auth env prefixes); recognition covers + // callers that pass a bare agent command line without the flag. + if (args.launchAgent || startupAgentRecognition) { + ptyAgentSessionIds.add(id) + } ptyShellName.set(id, getSpawnedShellName(shellPath)) if (finalEnv.ORCA_TERMINAL_HANDLE) { ptyTerminalHandle.set(id, finalEnv.ORCA_TERMINAL_HANDLE) @@ -891,23 +907,36 @@ export class LocalPtyProvider implements IPtyProvider { if (!proc) { return } - // Why: disposePtyListeners removes the onExit callback, so the natural - // exit cleanup path from node-pty won't fire. Cleanup and notification - // must happen unconditionally after the try/catch. - // Timer/writer cleanup must happen here too: disposing listeners prevents - // the natural onExit callback from running the usual clearPtyState path. - runPtyCleanup(id) - disposePtyListeners(id) - try { - proc.kill() - } catch { - /* Process may already be dead */ + // Why: the snapshot must precede any signal/destroy — once the shell dies, + // surviving descendants reparent to pid 1 and a ppid walk can't find them. + const descendants = ptyAgentSessionIds.has(id) + ? await captureDescendantSnapshot(proc.pid) + : null + // Why the handle re-check: the snapshot is this method's only await, and a + // natural exit (or same-id respawn) may have raced it. That path already + // ran clearPtyState and notified exit — only the descendant sweep remains. + if (ptyProcesses.get(id) === proc) { + // Why: disposePtyListeners removes the onExit callback, so the natural + // exit cleanup path from node-pty won't fire. Cleanup and notification + // must happen unconditionally after the try/catch. + // Timer/writer cleanup must happen here too: disposing listeners prevents + // the natural onExit callback from running the usual clearPtyState path. + runPtyCleanup(id) + disposePtyListeners(id) + try { + proc.kill() + } catch { + /* Process may already be dead */ + } + destroyPtyProcess(proc, { alreadyKilled: true }) + clearPtyState(id) + this.opts.onExit?.(id, -1) + for (const cb of exitListeners) { + cb({ id, code: -1 }) + } } - destroyPtyProcess(proc, { alreadyKilled: true }) - clearPtyState(id) - this.opts.onExit?.(id, -1) - for (const cb of exitListeners) { - cb({ id, code: -1 }) + if (descendants) { + terminateDescendantSnapshot(descendants) } } diff --git a/src/main/pty-descendant-termination.test.ts b/src/main/pty-descendant-termination.test.ts new file mode 100644 index 00000000000..aa059d362df --- /dev/null +++ b/src/main/pty-descendant-termination.test.ts @@ -0,0 +1,190 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + captureDescendantSnapshot, + collectDescendantRows, + DESCENDANT_KILL_GRACE_MS, + killWithDescendantSweep, + parseProcessTable, + terminateDescendantSnapshot, + type ProcessTableRow +} from './pty-descendant-termination' + +function row( + pid: number, + ppid: number, + pgid: number, + startedAt = `Mon Jul 13 12:54:47 2026 #${pid}` +): ProcessTableRow { + return { pid, ppid, pgid, startedAt } +} + +describe('parseProcessTable', () => { + it('parses pid/ppid/pgid and keeps the space-containing lstart verbatim', () => { + const rows = parseProcessTable( + [ + ' 101 1 101 Mon Jul 13 12:54:47 2026', + '42017 101 42017 Tue Jul 14 01:02:03 2026 ', + '', + 'not a process line' + ].join('\n') + ) + expect(rows).toEqual([ + { pid: 101, ppid: 1, pgid: 101, startedAt: 'Mon Jul 13 12:54:47 2026' }, + { pid: 42017, ppid: 101, pgid: 42017, startedAt: 'Tue Jul 14 01:02:03 2026' } + ]) + }) +}) + +describe('collectDescendantRows', () => { + it('walks multi-level descendants including detached-pgid grandchildren', () => { + // shell(10) -> agent(20, own job pgid) -> detached tool shell(30, own + // session-style pgid) -> git(31). 99 is unrelated. + const table = [ + row(10, 1, 10), + row(20, 10, 20), + row(30, 20, 30), + row(31, 30, 30), + row(99, 1, 99) + ] + const snapshot = collectDescendantRows(10, table) + expect(snapshot.rootPgid).toBe(10) + expect(snapshot.descendants.map((r) => r.pid)).toEqual([20, 30, 31]) + }) + + it('returns a null root pgid when the root row is already gone', () => { + const snapshot = collectDescendantRows(10, [row(20, 10, 20)]) + expect(snapshot.rootPgid).toBeNull() + expect(snapshot.descendants.map((r) => r.pid)).toEqual([20]) + }) +}) + +describe('captureDescendantSnapshot', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + afterEach(() => { + vi.useRealTimers() + }) + + it('resolves the descendant tree on POSIX', async () => { + const readTable = vi.fn().mockResolvedValue([row(10, 1, 10), row(20, 10, 20)]) + const snapshot = await captureDescendantSnapshot(10, { readTable, platform: 'darwin' }) + expect(snapshot).toEqual({ rootPgid: 10, descendants: [row(20, 10, 20)] }) + }) + + it('is a null no-op on Windows', async () => { + const readTable = vi.fn() + expect(await captureDescendantSnapshot(10, { readTable, platform: 'win32' })).toBeNull() + expect(readTable).not.toHaveBeenCalled() + }) + + it('degrades to null when ps fails', async () => { + const readTable = vi.fn().mockRejectedValue(new Error('ps exploded')) + expect(await captureDescendantSnapshot(10, { readTable, platform: 'linux' })).toBeNull() + }) + + it('degrades to null when ps hangs past the timeout instead of blocking teardown', async () => { + const readTable = vi.fn().mockReturnValue(new Promise(() => {})) + const pending = captureDescendantSnapshot(10, { + readTable, + platform: 'darwin', + timeoutMs: 1_000 + }) + await vi.advanceTimersByTimeAsync(1_000) + expect(await pending).toBeNull() + }) +}) + +describe('terminateDescendantSnapshot', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + afterEach(() => { + vi.useRealTimers() + }) + + it('SIGTERMs the root group and every descendant immediately', () => { + const sendSignal = vi.fn() + terminateDescendantSnapshot( + { rootPgid: 10, descendants: [row(20, 10, 20), row(30, 20, 30)] }, + { sendSignal, readTable: vi.fn().mockResolvedValue([]) } + ) + expect(sendSignal.mock.calls).toEqual([ + [-10, 'SIGTERM'], + [20, 'SIGTERM'], + [30, 'SIGTERM'] + ]) + }) + + it('SIGKILLs only identity-matched survivors after the grace window', async () => { + const survivor = row(30, 20, 30) + const exited = row(20, 10, 20) + const recycled = row(40, 30, 40) + const sendSignal = vi.fn() + // At escalation time: 30 survives unchanged, 20 is gone, 40's pid now + // belongs to a different (recycled) process with a different start time. + const readTable = vi + .fn() + .mockResolvedValue([survivor, { ...recycled, startedAt: 'Tue Jul 14 09:00:00 2026' }]) + terminateDescendantSnapshot( + { rootPgid: 10, descendants: [exited, survivor, recycled] }, + { sendSignal, readTable } + ) + sendSignal.mockClear() + await vi.advanceTimersByTimeAsync(DESCENDANT_KILL_GRACE_MS) + expect(sendSignal.mock.calls).toEqual([[30, 'SIGKILL']]) + }) + + it('never escalates when the identity re-read fails', async () => { + const sendSignal = vi.fn() + const readTable = vi.fn().mockRejectedValue(new Error('ps exploded')) + terminateDescendantSnapshot( + { rootPgid: 10, descendants: [row(20, 10, 20)] }, + { sendSignal, readTable } + ) + sendSignal.mockClear() + await vi.advanceTimersByTimeAsync(DESCENDANT_KILL_GRACE_MS) + expect(sendSignal).not.toHaveBeenCalled() + }) + + it('schedules no escalation for an empty descendant set', () => { + const sendSignal = vi.fn() + const readTable = vi.fn() + terminateDescendantSnapshot({ rootPgid: 10, descendants: [] }, { sendSignal, readTable }) + expect(vi.getTimerCount()).toBe(0) + expect(sendSignal.mock.calls).toEqual([[-10, 'SIGTERM']]) + }) +}) + +describe('killWithDescendantSweep', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + afterEach(() => { + vi.useRealTimers() + }) + + it('kills the root only after the snapshot resolves, then sweeps it', async () => { + const sendSignal = vi.fn() + const readTable = vi.fn().mockResolvedValue([row(10, 1, 10), row(20, 10, 20)]) + const killRoot = vi.fn() + killWithDescendantSweep(10, killRoot, { readTable, sendSignal, platform: 'darwin' }) + expect(killRoot).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(0) + expect(killRoot).toHaveBeenCalledOnce() + expect(sendSignal.mock.calls).toEqual([ + [-10, 'SIGTERM'], + [20, 'SIGTERM'] + ]) + }) + + it('still kills the root when the snapshot is unavailable', async () => { + const sendSignal = vi.fn() + const readTable = vi.fn().mockRejectedValue(new Error('ps exploded')) + const killRoot = vi.fn() + killWithDescendantSweep(10, killRoot, { readTable, sendSignal, platform: 'darwin' }) + await vi.advanceTimersByTimeAsync(0) + expect(killRoot).toHaveBeenCalledOnce() + expect(sendSignal).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/pty-descendant-termination.ts b/src/main/pty-descendant-termination.ts new file mode 100644 index 00000000000..224b2e2b57c --- /dev/null +++ b/src/main/pty-descendant-termination.ts @@ -0,0 +1,202 @@ +import { execFile } from 'node:child_process' + +export const DESCENDANT_KILL_GRACE_MS = 2_000 +export const DESCENDANT_SNAPSHOT_TIMEOUT_MS = 1_000 +// Why: a full process table on a busy host can exceed execFile's 1MB default; +// truncation would silently drop descendants from the snapshot. +const PS_MAX_BUFFER_BYTES = 32 * 1024 * 1024 + +export type ProcessTableRow = { + pid: number + ppid: number + pgid: number + /** ps lstart text, kept verbatim. (pid, startedAt) is the identity token that + * gates the delayed SIGKILL so it can never land on a recycled pid. */ + startedAt: string +} + +export type DescendantSnapshot = { + rootPgid: number | null + descendants: ProcessTableRow[] +} + +export type ProcessTableReader = () => Promise +export type SignalSender = (pid: number, signal: NodeJS.Signals) => void + +export function parseProcessTable(psOutput: string): ProcessTableRow[] { + const rows: ProcessTableRow[] = [] + for (const line of psOutput.split('\n')) { + // lstart itself contains spaces ("Mon Jul 13 12:54:47 2026"), so only the + // three leading numeric columns are positional. + const match = line.match(/^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.+?)\s*$/) + if (!match) { + continue + } + rows.push({ + pid: Number(match[1]), + ppid: Number(match[2]), + pgid: Number(match[3]), + startedAt: match[4] + }) + } + return rows +} + +function readProcessTable(): Promise { + return new Promise((resolve, reject) => { + execFile( + 'ps', + ['-axo', 'pid=,ppid=,pgid=,lstart='], + { maxBuffer: PS_MAX_BUFFER_BYTES }, + (error, stdout) => { + if (error) { + reject(error) + return + } + resolve(parseProcessTable(stdout)) + } + ) + }) +} + +export function collectDescendantRows( + rootPid: number, + table: ProcessTableRow[] +): DescendantSnapshot { + const childrenByPpid = new Map() + let rootRow: ProcessTableRow | null = null + for (const row of table) { + if (row.pid === rootPid) { + rootRow = row + continue + } + const siblings = childrenByPpid.get(row.ppid) + if (siblings) { + siblings.push(row) + } else { + childrenByPpid.set(row.ppid, [row]) + } + } + const descendants: ProcessTableRow[] = [] + const queue = [rootPid] + while (queue.length > 0) { + const pid = queue.shift()! + for (const child of childrenByPpid.get(pid) ?? []) { + descendants.push(child) + queue.push(child.pid) + } + } + return { rootPgid: rootRow?.pgid ?? null, descendants } +} + +type SnapshotDeps = { + readTable?: ProcessTableReader + platform?: NodeJS.Platform + timeoutMs?: number +} + +/** + * Snapshots a PTY root's live descendant tree. Must run BEFORE the root is + * signalled: once the root dies, surviving descendants reparent to pid 1 and + * can no longer be found by a ppid walk. Resolves null (never rejects) on + * Windows, ps failure, or timeout — callers then degrade to today's + * shell-only kill. + */ +export async function captureDescendantSnapshot( + rootPid: number, + deps: SnapshotDeps = {} +): Promise { + const platform = deps.platform ?? process.platform + if (platform === 'win32' || !Number.isInteger(rootPid) || rootPid <= 0) { + return null + } + const readTable = deps.readTable ?? readProcessTable + const timeoutMs = deps.timeoutMs ?? DESCENDANT_SNAPSHOT_TIMEOUT_MS + try { + // Why the timeout: the snapshot sits in front of the actual kill; a wedged + // ps must degrade to shell-only kill, not block session teardown. + const table = await Promise.race([ + readTable(), + new Promise((resolve) => setTimeout(() => resolve(null), timeoutMs).unref?.()) + ]) + if (!table) { + return null + } + return collectDescendantRows(rootPid, table) + } catch { + return null + } +} + +/** + * Standard agent-session kill sequencing: snapshot the descendant tree, only + * then run the caller's root kill, then sweep the snapshot. Callers must not + * signal the root before this runs — a dead root's descendants reparent to + * pid 1 and become unfindable. Snapshot failure degrades to killRoot alone. + */ +export function killWithDescendantSweep( + rootPid: number, + killRoot: () => void, + deps: SnapshotDeps & TerminateDeps = {} +): void { + void captureDescendantSnapshot(rootPid, deps).then((snapshot) => { + killRoot() + if (snapshot) { + terminateDescendantSnapshot(snapshot, deps) + } + }) +} + +function defaultSendSignal(pid: number, signal: NodeJS.Signals): void { + try { + process.kill(pid, signal) + } catch { + /* already gone */ + } +} + +type TerminateDeps = { + readTable?: ProcessTableReader + sendSignal?: SignalSender + graceMs?: number +} + +/** + * Terminates a snapshotted descendant tree: SIGTERM now (root process group + + * every descendant, reaching detached-pgid children the PTY's SIGHUP cannot), + * then after a grace window SIGKILL whatever from the snapshot is still alive. + * The SIGKILL wave re-reads the process table and only signals pids whose + * start time still matches the snapshot, so a recycled pid is never killed. + */ +export function terminateDescendantSnapshot( + snapshot: DescendantSnapshot, + deps: TerminateDeps = {} +): void { + const sendSignal = deps.sendSignal ?? defaultSendSignal + const readTable = deps.readTable ?? readProcessTable + if (snapshot.rootPgid !== null && snapshot.rootPgid > 0) { + sendSignal(-snapshot.rootPgid, 'SIGTERM') + } + for (const row of snapshot.descendants) { + sendSignal(row.pid, 'SIGTERM') + } + if (snapshot.descendants.length === 0) { + return + } + const timer = setTimeout(() => { + void readTable().then( + (table) => { + const liveByPid = new Map(table.map((row) => [row.pid, row])) + for (const row of snapshot.descendants) { + if (liveByPid.get(row.pid)?.startedAt === row.startedAt) { + sendSignal(row.pid, 'SIGKILL') + } + } + }, + () => { + /* table read failed — leave survivors rather than risk recycled pids */ + } + ) + }, deps.graceMs ?? DESCENDANT_KILL_GRACE_MS) + timer.unref?.() +} diff --git a/tests/e2e/agent-descendant-process-kill.spec.ts b/tests/e2e/agent-descendant-process-kill.spec.ts new file mode 100644 index 00000000000..244b82437c9 --- /dev/null +++ b/tests/e2e/agent-descendant-process-kill.spec.ts @@ -0,0 +1,97 @@ +import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test, expect } from './helpers/orca-app' +import { waitForActiveWorktree, waitForSessionReady } from './helpers/store' + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +// Reproduces the orphaned-descendant battery-drain incident (STA-1800): an +// agent CLI spawns a tool child in a detached process group, the session is +// killed, and the child must not survive. The stand-in agent's first command +// token is literally `claude` so PTY spawn recognition marks the session as an +// agent; tab close → pty.kill routing is already covered by +// terminal-parked-close-retirement.spec.ts, so this spec drives pty.kill. +test('killing an agent PTY terminates its detached-pgid descendants', async ({ orcaPage }) => { + test.skip(process.platform === 'win32', 'descendant tree-kill is POSIX-only for now') + + const stage = mkdtempSync(join(tmpdir(), 'orca-agent-descendant-')) + const markerPath = join(stage, 'detached-child.pid') + const spawnerPath = join(stage, 'spawn-detached.cjs') + writeFileSync( + spawnerPath, + [ + "const { spawn } = require('node:child_process')", + // detached:true → setsid → own pgid/session, exactly the topology of an + // agent CLI's tool subprocess that a dying shell's SIGHUP cannot reach. + "const child = spawn('sleep', ['31337'], { detached: true, stdio: 'ignore' })", + 'child.unref()', + "require('node:fs').writeFileSync(process.argv[2], String(child.pid))", + // Stay alive like a real agent at its prompt: the detached child's ppid + // must remain intact at kill time — a pre-orphaned child is the separate + // crash-path scenario that only the PR-2 sweep can catch. + 'setInterval(() => {}, 1000)', + '' + ].join('\n') + ) + const fakeAgentPath = join(stage, 'claude') + writeFileSync(fakeAgentPath, `#!/bin/sh\nexec "${process.execPath}" "${spawnerPath}" "$1"\n`) + chmodSync(fakeAgentPath, 0o755) + + let detachedChildPid = 0 + try { + await waitForSessionReady(orcaPage) + const worktreeId = await waitForActiveWorktree(orcaPage) + + const ptyId = await orcaPage.evaluate( + async ({ command, cwd, worktreeId: wt }) => { + const result = await window.api.pty.spawn({ + cols: 120, + rows: 40, + cwd, + command, + launchAgent: 'claude', + worktreeId: wt + }) + return result.id + }, + { command: `'${fakeAgentPath}' '${markerPath}'`, cwd: stage, worktreeId } + ) + expect(ptyId).toBeTruthy() + + await expect + .poll(() => existsSync(markerPath), { + timeout: 20_000, + message: 'stand-in agent never spawned its detached child' + }) + .toBe(true) + detachedChildPid = Number(readFileSync(markerPath, 'utf8').trim()) + expect(detachedChildPid).toBeGreaterThan(0) + expect(isProcessAlive(detachedChildPid)).toBe(true) + + await orcaPage.evaluate((id) => window.api.pty.kill(id), ptyId) + + await expect + .poll(() => isProcessAlive(detachedChildPid), { + timeout: 15_000, + message: `detached descendant ${detachedChildPid} survived the agent PTY kill` + }) + .toBe(false) + } finally { + if (detachedChildPid > 0 && isProcessAlive(detachedChildPid)) { + try { + process.kill(detachedChildPid, 'SIGKILL') + } catch { + /* already gone */ + } + } + rmSync(stage, { recursive: true, force: true }) + } +})