Fix PTY descendant leaks on forced teardown

This commit is contained in:
Brennan Benson
2026-07-14 23:33:50 -07:00
parent b36cbd9a14
commit e76e23fd8e
6 changed files with 329 additions and 2 deletions
+4 -1
View File
@@ -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()
+6 -1
View File
@@ -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(
@@ -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<void> {
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<string, string>
})
const exited = new Promise<void>((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<never>((_, 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
)
@@ -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()
})
})
+130
View File
@@ -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
}
}
+5
View File
@@ -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)
}