diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index c47f5c6c108..8e1f7024891 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -64,6 +64,7 @@ import { getAgentForegroundContextPaths } from '../providers/agent-foreground-co import { assertSafeAgentStartupCwd, resolveSafePtyDefaultCwd } from '../providers/pty-default-cwd' import { ORCA_HERMES_STARTUP_QUERY_ENV } from '../../shared/hermes-startup-query' import type { TuiAgent } from '../../shared/types' +import { forceKillPosixPtyProcessGroups } from '../pty/posix-pty-process-groups' const PANE_IDENTITY_ENV_KEYS = [ 'ORCA_PANE_KEY', @@ -1220,7 +1221,9 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl return } try { - process.kill(proc.pid, 'SIGKILL') + forceKillPosixPtyProcessGroups(proc.pid, () => { + process.kill(proc.pid, 'SIGKILL') + }) } catch (signalError) { try { proc.kill() diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index f44cae2ea08..9cbdbaa6095 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -55,6 +55,7 @@ import { resolveAgentForegroundProcessWithAvailability } from './agent-foregroun import { getAgentForegroundContextPaths } from './agent-foreground-context-paths' import { recognizeAgentProcessFromCommandLine } from '../../shared/agent-process-recognition' import { readWindowsConptyProcessIds } from './windows-conpty-process-membership' +import { forceKillPosixPtyProcessGroups } from '../pty/posix-pty-process-groups' import { shouldUseShellReadyStartupDelivery } from '../../shared/codex-startup-delivery' import { assertSafeAgentStartupCwd, resolveSafePtyDefaultCwd } from './pty-default-cwd' import { ORCA_HERMES_STARTUP_QUERY_ENV } from '../../shared/hermes-startup-query' @@ -232,7 +233,11 @@ function killLocalPtyProcess(proc: pty.IPty, immediate: boolean): void { proc.kill() return } - proc.kill(immediate ? 'SIGKILL' : 'SIGTERM') + if (!immediate) { + proc.kill('SIGTERM') + return + } + forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL')) } function armLocalPtyForceKill( diff --git a/src/main/pty/posix-pty-process-groups.integration.test.ts b/src/main/pty/posix-pty-process-groups.integration.test.ts new file mode 100644 index 00000000000..f8b191f9856 --- /dev/null +++ b/src/main/pty/posix-pty-process-groups.integration.test.ts @@ -0,0 +1,94 @@ +import { execFileSync } from 'node:child_process' +import * as pty from 'node-pty' +import { expect, it } from 'vitest' +import { forceKillPosixPtyProcessGroups } from './posix-pty-process-groups' + +type TaggedProcess = { pid: number; pgid: number } + +const itOnPosix = process.platform === 'win32' ? it.skip : it + +function findTaggedProcesses(token: string): TaggedProcess[] { + const output = execFileSync('ps', ['-axo', 'pid=,pgid=,state=,command='], { + encoding: 'utf8' + }) + const matches: TaggedProcess[] = [] + for (const line of output.split(/\r?\n/)) { + if (!line.includes(token)) { + continue + } + const match = /^\s*(\d+)\s+(\d+)\s+(\S+)\s+/.exec(line) + if (match && !match[3].includes('Z')) { + matches.push({ pid: Number(match[1]), pgid: Number(match[2]) }) + } + } + return matches +} + +async function waitFor( + predicate: () => boolean, + timeoutMs = 5_000, + intervalMs = 25 +): Promise { + const deadline = Date.now() + timeoutMs + while (!predicate()) { + if (Date.now() >= deadline) { + throw new Error('Timed out waiting for PTY process state') + } + await new Promise((resolve) => setTimeout(resolve, intervalMs)) + } +} + +function cleanupTaggedProcesses(token: string): void { + for (const pgid of new Set(findTaggedProcesses(token).map((process) => process.pgid))) { + try { + process.kill(-pgid, 'SIGKILL') + } catch { + // The tagged test group may already be gone. + } + } +} + +function readProcessGroup(pid: number): number { + return Number(execFileSync('ps', ['-p', String(pid), '-o', 'pgid='], { encoding: 'utf8' }).trim()) +} + +itOnPosix( + 'reaps a foreground job that ignores terminal shutdown signals', + async () => { + const token = `ORCA_PTY_GROUP_TEST_${process.pid}_${Date.now()}` + const proc = pty.spawn('/bin/sh', [], { + name: 'xterm-256color', + cols: 80, + rows: 24, + cwd: process.cwd(), + env: process.env as Record + }) + const exited = new Promise((resolve) => proc.onExit(() => resolve())) + + try { + proc.write(`/bin/sh -lc 'trap "" HUP TERM; while :; do sleep 60; done' ${token}\n`) + await waitFor(() => findTaggedProcesses(token).length > 0) + + const tagged = findTaggedProcesses(token) + const rootProcessGroup = readProcessGroup(proc.pid) + expect(tagged.some((process) => process.pgid !== rootProcessGroup)).toBe(true) + + forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL')) + await Promise.race([ + exited, + new Promise((_, reject) => + setTimeout(() => reject(new Error('PTY leader did not exit')), 5_000) + ) + ]) + await waitFor(() => findTaggedProcesses(token).length === 0) + } finally { + cleanupTaggedProcesses(token) + try { + proc.kill('SIGKILL') + } catch { + // node-pty may already have reaped and disposed the leader. + } + } + }, + 10_000 +) diff --git a/src/main/pty/posix-pty-process-groups.test.ts b/src/main/pty/posix-pty-process-groups.test.ts new file mode 100644 index 00000000000..59c287a303a --- /dev/null +++ b/src/main/pty/posix-pty-process-groups.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it, vi } from 'vitest' +import { + forceKillPosixPtyProcessGroups, + getPosixPtyProcessGroups +} from './posix-pty-process-groups' + +const TABLE = ` + 100 100 ttys001 + 101 101 ttys001 + 102 101 ttys001 + 103 103 ttys001 + 200 200 ttys002 + 300 300 ?? +` + +describe('POSIX PTY process-group termination', () => { + it('returns every group attached to the root PTY with the root group last', () => { + expect(getPosixPtyProcessGroups(TABLE, 100, 999)).toEqual([101, 103, 100]) + }) + + it('refuses an unbound root or a PTY shared with Orca itself', () => { + expect(getPosixPtyProcessGroups(TABLE, 300, 999)).toBeNull() + expect(getPosixPtyProcessGroups(TABLE, 100, 102)).toBeNull() + expect(getPosixPtyProcessGroups(TABLE, 999, 998)).toBeNull() + }) + + it('kills foreground and background groups before the PTY leader', () => { + const fallback = vi.fn() + const signalProcessGroup = vi.fn() + + forceKillPosixPtyProcessGroups(100, fallback, { + platform: 'darwin', + currentPid: 999, + readProcessTable: () => TABLE, + signalProcessGroup + }) + + expect(signalProcessGroup.mock.calls.map(([pgid]) => pgid)).toEqual([101, 103, 100]) + expect(fallback).not.toHaveBeenCalled() + }) + + it('falls back when the process table cannot prove PTY ownership', () => { + const fallback = vi.fn() + + forceKillPosixPtyProcessGroups(100, fallback, { + platform: 'linux', + currentPid: 102, + readProcessTable: () => TABLE, + signalProcessGroup: vi.fn() + }) + + expect(fallback).toHaveBeenCalledOnce() + }) + + it('ignores groups that exited after the snapshot but preserves real signal errors', () => { + const gone = Object.assign(new Error('gone'), { code: 'ESRCH' }) + const denied = Object.assign(new Error('denied'), { code: 'EPERM' }) + const signalProcessGroup = vi + .fn<(pgid: number) => void>() + .mockImplementationOnce(() => { + throw gone + }) + .mockImplementationOnce(() => { + throw denied + }) + + expect(() => + forceKillPosixPtyProcessGroups(100, vi.fn(), { + platform: 'darwin', + currentPid: 999, + readProcessTable: () => TABLE, + signalProcessGroup + }) + ).toThrow('denied') + expect(signalProcessGroup).toHaveBeenCalledTimes(3) + }) + + it('uses the existing fallback on Windows without reading ps', () => { + const fallback = vi.fn() + const readProcessTable = vi.fn(() => TABLE) + + forceKillPosixPtyProcessGroups(100, fallback, { + platform: 'win32', + readProcessTable + }) + + expect(fallback).toHaveBeenCalledOnce() + expect(readProcessTable).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/pty/posix-pty-process-groups.ts b/src/main/pty/posix-pty-process-groups.ts new file mode 100644 index 00000000000..21b18808d6d --- /dev/null +++ b/src/main/pty/posix-pty-process-groups.ts @@ -0,0 +1,130 @@ +import { execFileSync } from 'node:child_process' + +const PROCESS_TABLE_TIMEOUT_MS = 1_000 +const PROCESS_TABLE_MAX_BYTES = 1024 * 1024 + +type ProcessRow = { + pid: number + pgid: number + tty: string +} + +export type PosixPtyProcessGroupTerminationDeps = { + platform?: NodeJS.Platform + currentPid?: number + readProcessTable?: () => string + signalProcessGroup?: (pgid: number) => void +} + +function runPs(args: string[]): string { + return execFileSync('ps', args, { + encoding: 'utf8', + timeout: PROCESS_TABLE_TIMEOUT_MS, + maxBuffer: PROCESS_TABLE_MAX_BYTES + }) +} + +function readPtyProcessTable(rootPid: number): string { + const root = runPs(['-p', String(rootPid), '-o', 'pid=,pgid=,tty=']) + const rootRow = parseProcessRows(root).find((row) => row.pid === rootPid) + if (!rootRow || rootRow.tty === '?' || rootRow.tty === '??') { + return root + } + // Why: a whole-host `ps -ax` takes nearly a second on large machines. TTY + // selection keeps forced terminal teardown proportional to one terminal. + return `${root}\n${runPs(['-t', rootRow.tty, '-o', 'pid=,pgid=,tty='])}` +} + +function parseProcessRows(output: string): ProcessRow[] { + const rows: ProcessRow[] = [] + for (const line of output.split(/\r?\n/)) { + const match = /^\s*(\d+)\s+(\d+)\s+(\S+)/.exec(line) + if (!match) { + continue + } + const pid = Number(match[1]) + const pgid = Number(match[2]) + if (pid > 0 && pgid > 1) { + rows.push({ pid, pgid, tty: match[3] }) + } + } + return rows +} + +export function getPosixPtyProcessGroups( + output: string, + rootPid: number, + currentPid = process.pid +): number[] | null { + const rows = parseProcessRows(output) + const root = rows.find((row) => row.pid === rootPid) + if (!root || root.tty === '?' || root.tty === '??') { + return null + } + // Why: a development daemon can inherit its launch TTY. Never group-signal + // when Orca itself shares the PTY; fall back to the already-scoped root kill. + if (rows.some((row) => row.pid === currentPid && row.tty === root.tty)) { + return null + } + const groups = new Set(rows.filter((row) => row.tty === root.tty).map((row) => row.pgid)) + if (!groups.has(root.pgid)) { + return null + } + return [...groups].sort((left, right) => { + if (left === root.pgid) { + return 1 + } + if (right === root.pgid) { + return -1 + } + return left - right + }) +} + +function isProcessAlreadyGone(error: unknown): boolean { + return (error as NodeJS.ErrnoException | undefined)?.code === 'ESRCH' +} + +/** Force-kill every process group still attached to one POSIX PTY. */ +export function forceKillPosixPtyProcessGroups( + rootPid: number, + fallback: () => void, + deps: PosixPtyProcessGroupTerminationDeps = {} +): void { + if ((deps.platform ?? process.platform) === 'win32') { + fallback() + return + } + let groups: number[] | null + try { + groups = getPosixPtyProcessGroups( + (deps.readProcessTable ?? (() => readPtyProcessTable(rootPid)))(), + rootPid, + deps.currentPid ?? process.pid + ) + } catch { + groups = null + } + if (!groups || groups.length === 0) { + fallback() + return + } + + const signalProcessGroup = + deps.signalProcessGroup ?? ((pgid: number) => process.kill(-pgid, 'SIGKILL')) + let firstError: unknown + for (const pgid of groups) { + try { + signalProcessGroup(pgid) + } catch (error) { + // Why: the PTY exit callback may reap a group between `ps` and killpg. + // ESRCH is proof that this captured owner is already gone, not failure. + if (!isProcessAlreadyGone(error) && firstError === undefined) { + firstError = error + } + } + } + if (firstError !== undefined) { + throw firstError + } +} diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 4aac93d2a5c..c1de6809c15 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -36,6 +36,7 @@ import { mergeGitConfigEnvProtocol } from '../shared/git-credential-prompt-env' import { isTuiAgent } from '../shared/tui-agent-config' +import { forceKillPosixPtyProcessGroups } from '../main/pty/posix-pty-process-groups' // Why: node-pty is a native addon that may not be installed on the remote. // Dynamic import keeps the require() lazy so loadPty() returns null gracefully @@ -108,6 +109,10 @@ function killPtyProcess(pty: IPty, signal: string): void { pty.kill() return } + if (signal === 'SIGKILL') { + forceKillPosixPtyProcessGroups(pty.pid, () => pty.kill(signal)) + return + } pty.kill(signal) }