From af0885d8ec3aac08259a37f42b51818a2576f91c Mon Sep 17 00:00:00 2001 From: Bogdan Date: Tue, 14 Jul 2026 09:40:32 +0300 Subject: [PATCH] fix(bg): revalidate process identity before signals (#1937) * fix(bg): revalidate process identity before signals * fix(bg): sanitize process identity probe failures * fix(bg): skip signals for terminal sessions --- src/cli/bg.test.ts | 375 +++++++++++++++++++++++++++++++++++++ src/cli/bg.ts | 143 ++++++++++++-- src/cli/bgRegistry.test.ts | 87 +++++++++ src/cli/bgRegistry.ts | 101 ++++++++-- 4 files changed, 673 insertions(+), 33 deletions(-) diff --git a/src/cli/bg.test.ts b/src/cli/bg.test.ts index 8a5cf4742..328853735 100644 --- a/src/cli/bg.test.ts +++ b/src/cli/bg.test.ts @@ -7,12 +7,18 @@ import { buildBackgroundSessionLaunch, buildBackgroundChildProcessConfig, followLogFile, + killBackgroundSession, printExistingLog, + terminateBackgroundSessionProcessTree, terminateBackgroundProcessTree, LOG_STREAM_CHUNK_SIZE, parseBackgroundInvocation, parseLogsInvocation, } from './bg.js' +import type { + BackgroundSession, + BackgroundSessionProcessIdentity, +} from './bgRegistry.js' class TestOutput extends EventEmitter { chunks: Buffer[] = [] @@ -498,6 +504,375 @@ describe('background session CLI parsing', () => { }) }) +describe('background session process termination safety', () => { + const session: BackgroundSession = { + id: 'bg-safety', + name: 'safety', + pid: 4242, + cwd: '/repo', + status: 'running', + startedAt: '2026-07-10T08:00:00.000Z', + updatedAt: '2026-07-10T08:00:00.000Z', + sessionId: 'conversation-safety', + command: ['node', 'openclaude', '--session-id', 'conversation-safety'], + stdoutLogPath: '/tmp/stdout.log', + stderrLogPath: '/tmp/stderr.log', + } + + function identity( + state: BackgroundSessionProcessIdentity['state'], + overrides: Partial = {}, + ): BackgroundSessionProcessIdentity { + return { + state, + backgroundSessionId: session.id, + pid: session.pid, + ...overrides, + } + } + + it('verifies the selected session immediately before SIGTERM', async () => { + const calls: string[] = [] + let aliveChecks = 0 + + await terminateBackgroundSessionProcessTree(session, { + isProcessAlive: () => ++aliveChecks <= 2, + getProcessCommand: pid => { + calls.push(`verify:${pid}`) + return 'node openclaude --session-id conversation-safety' + }, + killTree: async (pid, signal) => { + calls.push(`signal:${pid}:${signal}`) + }, + sleep: async () => {}, + termGraceMs: 1, + pollIntervalMs: 1, + }) + + expect(calls).toEqual([ + 'verify:4242', + 'signal:4242:SIGTERM', + ]) + }) + + it('refuses a mismatched identity before SIGTERM and does not mark killed', async () => { + const calls: string[] = [] + + let refusal: unknown + try { + await killBackgroundSession( + { + ...session, + status: 'running', + command: [...session.command, 'private prompt value'], + }, + { + isProcessAlive: () => true, + verifySessionIdentity: () => identity('mismatch'), + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + markKilled: async selected => { + calls.push(`mark:${selected.id}`) + return { ...selected, status: 'killed' } + }, + }, + ) + } catch (error) { + refusal = error + } + + expect(String(refusal)).toContain('refused to signal an unverified process') + expect(String(refusal)).not.toContain('private prompt value') + expect(calls).toEqual([]) + }) + + it('does not verify or signal terminal records when a reused PID would match', async () => { + for (const status of ['stale', 'killed', 'exited', 'failed'] as const) { + const calls: string[] = [] + + const killed = await killBackgroundSession( + { ...session, status }, + { + verifySessionIdentity: () => { + calls.push('verify') + return identity('matches') + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + markKilled: async selected => { + calls.push(`mark:${selected.id}`) + return { ...selected, status: 'killed' } + }, + }, + ) + + expect(killed.status).toBe('killed') + expect(calls).toEqual(['mark:bg-safety']) + } + }) + + it('freshly verifies an unknown session that becomes readable before killing', async () => { + const calls: string[] = [] + const states: BackgroundSessionProcessIdentity['state'][] = [ + 'matches', + 'matches', + ] + + const killed = await killBackgroundSession( + { ...session, status: 'unknown' }, + { + isProcessAlive: () => false, + verifySessionIdentity: () => { + const state = states.shift()! + calls.push(`verify:${state}`) + return identity(state) + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + markKilled: async selected => { + calls.push(`mark:${selected.id}`) + return { ...selected, status: 'killed' } + }, + }, + ) + + expect(killed.status).toBe('killed') + expect(calls).toEqual([ + 'verify:matches', + 'verify:matches', + 'signal:SIGTERM', + 'mark:bg-safety', + ]) + }) + + it('treats an unknown session that exits during fresh verification as terminated', async () => { + const calls: string[] = [] + let aliveChecks = 0 + + const killed = await killBackgroundSession( + { ...session, status: 'unknown' }, + { + isProcessAlive: () => ++aliveChecks === 1, + getProcessCommand: () => null, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + markKilled: async selected => { + calls.push(`mark:${selected.id}`) + return { ...selected, status: 'killed' } + }, + }, + ) + + expect(killed.status).toBe('killed') + expect(calls).toEqual(['mark:bg-safety']) + }) + + it('does not escalate when identity changes during the SIGTERM grace period', async () => { + const calls: string[] = [] + const states: BackgroundSessionProcessIdentity['state'][] = [ + 'matches', + 'mismatch', + ] + + await expect( + terminateBackgroundSessionProcessTree(session, { + isProcessAlive: () => true, + verifySessionIdentity: () => { + const state = states.shift()! + calls.push(`verify:${state}`) + return identity(state) + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + sleep: async () => { + calls.push('sleep') + }, + termGraceMs: 1, + pollIntervalMs: 1, + }), + ).rejects.toThrow('refused to signal an unverified process') + + expect(calls).toEqual([ + 'verify:matches', + 'signal:SIGTERM', + 'sleep', + 'verify:mismatch', + ]) + }) + + it('fails closed when the live process identity is unreadable', async () => { + const signals: Array = [] + + await expect( + terminateBackgroundSessionProcessTree(session, { + isProcessAlive: () => true, + getProcessCommand: () => null, + killTree: async (_pid, signal) => { + signals.push(signal) + }, + }), + ).rejects.toThrow('refused to signal an unverified process') + + expect(signals).toEqual([]) + }) + + it('refuses signalling when liveness becomes unreadable during command lookup', async () => { + const calls: string[] = [] + let probes = 0 + + await expect( + terminateBackgroundSessionProcessTree(session, { + signalProcess: () => { + probes++ + calls.push(`probe:${probes}`) + if (probes > 1) { + throw Object.assign(new Error('access denied'), { code: 'EPERM' }) + } + }, + getProcessCommand: () => { + calls.push('command') + return 'node openclaude --session-id conversation-safety' + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + sleep: async () => {}, + termGraceMs: 1, + pollIntervalMs: 1, + }), + ).rejects.toThrow('refused to signal an unverified process') + + expect(calls).toEqual(['probe:1', 'command', 'probe:2']) + }) + + it('succeeds without signalling when the process exits before verification', async () => { + const calls: string[] = [] + + const killed = await killBackgroundSession(session, { + isProcessAlive: () => false, + getProcessCommand: () => { + calls.push('command') + return null + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + markKilled: async selected => { + calls.push(`mark:${selected.id}`) + return { ...selected, status: 'killed' } + }, + }) + + expect(killed.status).toBe('killed') + expect(calls).toEqual(['mark:bg-safety']) + }) + + it('accepts a natural exit after verification without a misleading error', async () => { + const calls: string[] = [] + + await terminateBackgroundSessionProcessTree(session, { + isProcessAlive: () => false, + verifySessionIdentity: () => { + calls.push('verify') + return identity('matches') + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + sleep: async () => { + calls.push('sleep') + }, + termGraceMs: 1, + pollIntervalMs: 1, + }) + + expect(calls).toEqual(['verify', 'signal:SIGTERM']) + }) + + it('revalidates a stable identity immediately before SIGKILL', async () => { + const calls: string[] = [] + let aliveChecks = 0 + + await terminateBackgroundSessionProcessTree(session, { + isProcessAlive: () => ++aliveChecks < 4, + verifySessionIdentity: () => { + calls.push('verify') + return identity('matches') + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + sleep: async () => { + calls.push('sleep') + }, + termGraceMs: 1, + killGraceMs: 1, + pollIntervalMs: 1, + }) + + expect(calls).toEqual([ + 'verify', + 'signal:SIGTERM', + 'sleep', + 'verify', + 'signal:SIGKILL', + 'sleep', + ]) + }) + + it('rejects stale verifier results for another session or PID', async () => { + for (const staleIdentity of [ + identity('matches', { backgroundSessionId: 'bg-unrelated' }), + identity('matches', { pid: 9001 }), + ]) { + const signals: Array = [] + + await expect( + terminateBackgroundSessionProcessTree(session, { + isProcessAlive: () => true, + verifySessionIdentity: () => staleIdentity, + killTree: async (_pid, signal) => { + signals.push(signal) + }, + }), + ).rejects.toThrow('refused to signal an unverified process') + + expect(signals).toEqual([]) + } + }) + + it('sanitizes throwing injected identity verifiers without signalling or marking', async () => { + const calls: string[] = [] + let refusal: unknown + + try { + await killBackgroundSession(session, { + verifySessionIdentity: () => { + throw new Error('private verifier details') + }, + killTree: async (_pid, signal) => { + calls.push(`signal:${signal}`) + }, + markKilled: async selected => { + calls.push(`mark:${selected.id}`) + return { ...selected, status: 'killed' } + }, + }) + } catch (error) { + refusal = error + } + + expect(String(refusal)).toContain('refused to signal an unverified process') + expect(String(refusal)).not.toContain('private verifier details') + expect(calls).toEqual([]) + }) +}) + describe('background session log streaming', () => { it('emits a multi-megabyte existing log exactly with bounded allocations', async () => { await withTempFile('stdout.log', async path => { diff --git a/src/cli/bg.ts b/src/cli/bg.ts index 77caa5993..13e46b193 100644 --- a/src/cli/bg.ts +++ b/src/cli/bg.ts @@ -16,6 +16,12 @@ import { markBackgroundSessionKilled, refreshBackgroundSessionStatuses, resolveBackgroundSession, + getBackgroundSessionProcessLiveness, + isTerminalBackgroundSession, + verifyBackgroundSessionProcessIdentity, + type BackgroundSession, + type BackgroundSessionProcessIdentity, + type BackgroundSessionProcessIdentityOptions, } from './bgRegistry.js' export type ParsedBackgroundInvocation = { @@ -720,6 +726,10 @@ export async function terminateBackgroundProcessTree( termGraceMs?: number killGraceMs?: number pollIntervalMs?: number + verifyBeforeSignal?: ( + pid: number, + signal: string | number, + ) => Promise<'matches' | 'not-running'> }, ): Promise { const isProcessAlive = options?.isProcessAlive ?? isProcessRunning @@ -728,7 +738,13 @@ export async function terminateBackgroundProcessTree( const pollIntervalMs = options?.pollIntervalMs ?? DEFAULT_KILL_POLL_INTERVAL_MS - if (!isProcessAlive(pid)) return + if (options?.verifyBeforeSignal) { + if ((await options.verifyBeforeSignal(pid, 'SIGTERM')) === 'not-running') { + return + } + } else if (!isProcessAlive(pid)) { + return + } await killTree(pid, 'SIGTERM') if ( await waitForProcessExit(pid, { @@ -741,6 +757,9 @@ export async function terminateBackgroundProcessTree( return } + if ((await options?.verifyBeforeSignal?.(pid, 'SIGKILL')) === 'not-running') { + return + } await killTree(pid, 'SIGKILL') if ( await waitForProcessExit(pid, { @@ -756,6 +775,113 @@ export async function terminateBackgroundProcessTree( throw new Error(`Process ${pid} did not exit after SIGKILL`) } +type BackgroundSessionTerminationOptions = BackgroundSessionProcessIdentityOptions & { + killTree?: (pid: number, signal: string | number) => Promise + sleep?: (ms: number) => Promise + termGraceMs?: number + killGraceMs?: number + pollIntervalMs?: number + verifySessionIdentity?: ( + session: BackgroundSession, + ) => BackgroundSessionProcessIdentity +} + +function unverifiedProcessError( + session: BackgroundSession, + reason: string, +): Error { + return new Error( + `OpenClaude refused to signal an unverified process for background session ${session.id} (PID ${session.pid}): ${reason}. Re-run \`openclaude ps\` and retry after confirming the session identity.`, + ) +} + +export async function terminateBackgroundSessionProcessTree( + session: BackgroundSession, + options: BackgroundSessionTerminationOptions = {}, +): Promise { + const getLiveness = (pid: number) => + getBackgroundSessionProcessLiveness(pid, options) + + await terminateBackgroundProcessTree(session.pid, { + ...options, + isProcessAlive: pid => getLiveness(pid) !== 'not-running', + verifyBeforeSignal: async pid => { + const identity = verifySelectedBackgroundSessionIdentity(session, options) + if (pid !== session.pid) { + throw unverifiedProcessError( + session, + 'the identity check did not correspond to the selected session and PID', + ) + } + return authorizeBackgroundSessionSignal(session, identity) + }, + }) +} + +function verifySelectedBackgroundSessionIdentity( + session: BackgroundSession, + options: BackgroundSessionTerminationOptions, +): BackgroundSessionProcessIdentity { + let identity: BackgroundSessionProcessIdentity + if (options.verifySessionIdentity) { + try { + identity = options.verifySessionIdentity(session) + } catch { + throw unverifiedProcessError( + session, + 'the live process identity could not be read', + ) + } + } else { + identity = verifyBackgroundSessionProcessIdentity(session, options) + } + if ( + identity.backgroundSessionId !== session.id || + identity.pid !== session.pid + ) { + throw unverifiedProcessError( + session, + 'the identity check did not correspond to the selected session and PID', + ) + } + return identity +} + +function authorizeBackgroundSessionSignal( + session: BackgroundSession, + identity: BackgroundSessionProcessIdentity, +): 'matches' | 'not-running' { + if (identity.state === 'not-running') return 'not-running' + if (identity.state === 'matches') return 'matches' + throw unverifiedProcessError( + session, + identity.state === 'mismatch' + ? 'the PID now belongs to a different process' + : 'the live process identity could not be read', + ) +} + +export async function killBackgroundSession( + session: BackgroundSession, + options: BackgroundSessionTerminationOptions & { + markKilled?: (session: BackgroundSession) => Promise + } = {}, +): Promise { + const markKilled = + options.markKilled ?? + (async (selected: BackgroundSession) => + await markBackgroundSessionKilled(selected.id)) + + if (isTerminalBackgroundSession(session)) return await markKilled(session) + + const identity = verifySelectedBackgroundSessionIdentity(session, options) + if (authorizeBackgroundSessionSignal(session, identity) === 'matches') { + await terminateBackgroundSessionProcessTree(session, options) + } + + return await markKilled(session) +} + export async function psHandler(_args: string[]): Promise { const sessions = await refreshBackgroundSessionStatuses() printSessionTable(sessions) @@ -811,20 +937,11 @@ export async function killHandler( await refreshBackgroundSessionStatuses() const session = await resolveSessionOrExit(target) - if (session.status === 'unknown' && isProcessRunning(session.pid)) { + const killed = await killBackgroundSession(session).catch(error => { fail( - `Cannot safely kill background session ${session.id}: process identity could not be verified`, + `Failed to kill background session ${session.id}: ${errorMessage(error)}`, ) - } - if (session.status === 'running' && isProcessRunning(session.pid)) { - await terminateBackgroundProcessTree(session.pid).catch(error => { - fail( - `Failed to kill background session ${session.id}: ${errorMessage(error)}`, - ) - }) - } - - const killed = await markBackgroundSessionKilled(session.id) + }) console.log(`Killed background session ${killed.id}.`) } diff --git a/src/cli/bgRegistry.test.ts b/src/cli/bgRegistry.test.ts index b1243db3b..c1defa3bf 100644 --- a/src/cli/bgRegistry.test.ts +++ b/src/cli/bgRegistry.test.ts @@ -12,6 +12,7 @@ import { markBackgroundSessionKilled, refreshBackgroundSessionStatuses, resolveBackgroundSession, + verifyBackgroundSessionProcessIdentity, type BackgroundSession, } from './bgRegistry.js' @@ -956,4 +957,90 @@ describe('isBackgroundSessionProcessAlive process identity', () => { }) expect(alive).toBe(false) }) + + it('distinguishes missing, matching, mismatched, and unreadable live processes', () => { + const states = [ + verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => false, + getProcessCommand: () => 'not read', + }), + verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => true, + getProcessCommand: () => 'node openclaude 1642 --serve', + }), + verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => true, + getProcessCommand: () => 'node unrelated --serve', + }), + verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => true, + getProcessCommand: () => null, + }), + ] + + expect(states.map(result => result.state)).toEqual([ + 'not-running', + 'matches', + 'mismatch', + 'unreadable', + ]) + expect( + states.every(result => result.backgroundSessionId === session.id), + ).toBe(true) + expect(states.every(result => result.pid === session.pid)).toBe(true) + }) + + it('treats exit during command lookup as not running', () => { + let aliveChecks = 0 + + const result = verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => ++aliveChecks === 1, + getProcessCommand: () => null, + }) + + expect(result.state).toBe('not-running') + expect(aliveChecks).toBe(2) + }) + + it('treats an access-denied liveness probe as unreadable', () => { + const result = verifyBackgroundSessionProcessIdentity(session, { + signalProcess: () => { + throw Object.assign(new Error('access denied'), { code: 'EPERM' }) + }, + getProcessCommand: () => { + throw new Error('command lookup must not run without confirmed liveness') + }, + }) + + expect(result.state).toBe('unreadable') + }) + + it('maps throwing injected liveness and command probes to unreadable', () => { + const livenessError = verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => { + throw new Error('private liveness details') + }, + getProcessCommand: () => 'not read', + }) + const commandError = verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => true, + getProcessCommand: () => { + throw new Error('private command details') + }, + }) + + expect(livenessError.state).toBe('unreadable') + expect(commandError.state).toBe('unreadable') + }) + + it('treats empty command output as unreadable', () => { + for (const command of ['', ' ']) { + const result = verifyBackgroundSessionProcessIdentity(session, { + isProcessAlive: () => true, + getProcessCommand: () => command, + }) + + expect(result.state).toBe('unreadable') + } + }) }) diff --git a/src/cli/bgRegistry.ts b/src/cli/bgRegistry.ts index 90969619f..0eef9c080 100644 --- a/src/cli/bgRegistry.ts +++ b/src/cli/bgRegistry.ts @@ -478,11 +478,14 @@ export async function refreshBackgroundSessionStatuses(options?: { continue } - const processState = getBackgroundSessionProcessState(session, options) + const processState = verifyBackgroundSessionProcessIdentity( + session, + options, + ).state const nextStatus: BackgroundSessionStatus = - processState === 'alive' + processState === 'matches' ? 'running' - : processState === 'unknown' + : processState === 'unreadable' ? 'unknown' : 'stale' @@ -503,7 +506,22 @@ export async function refreshBackgroundSessionStatuses(options?: { return refreshed } -type BackgroundSessionProcessState = 'alive' | 'dead' | 'unknown' +export type BackgroundSessionProcessIdentity = { + state: 'not-running' | 'matches' | 'mismatch' | 'unreadable' + backgroundSessionId: string + pid: number +} + +export type BackgroundSessionProcessLiveness = + | 'alive' + | 'not-running' + | 'unreadable' + +export type BackgroundSessionProcessIdentityOptions = { + isProcessAlive?: (pid: number) => boolean + signalProcess?: (pid: number, signal: 0) => unknown + getProcessCommand?: (pid: number) => string | null +} // A spaced path or prompt is a single argv entry, but the raw command line // quotes it, so a whitespace split fuses a quote onto the edge tokens. Windows @@ -570,30 +588,73 @@ function commandLineMatchesBackgroundSession( return commandLineContainsArgs(commandLine, session.command) } -function getBackgroundSessionProcessState( +export function verifyBackgroundSessionProcessIdentity( session: BackgroundSession, - options?: { - isProcessAlive?: (pid: number) => boolean - getProcessCommand?: (pid: number) => string | null - }, -): BackgroundSessionProcessState { - const isAlive = options?.isProcessAlive ?? isProcessRunning - if (!isAlive(session.pid)) return 'dead' + options?: BackgroundSessionProcessIdentityOptions, +): BackgroundSessionProcessIdentity { + const result = ( + state: BackgroundSessionProcessIdentity['state'], + ): BackgroundSessionProcessIdentity => ({ + state, + backgroundSessionId: session.id, + pid: session.pid, + }) + const getLiveness = () => + getBackgroundSessionProcessLiveness(session.pid, options) + const liveness = getLiveness() + if (liveness !== 'alive') return result(liveness) const readCommand = options?.getProcessCommand ?? getProcessCommand - const command = readCommand(session.pid) - if (command == null) return 'unknown' - return commandLineMatchesBackgroundSession(command, session) ? 'alive' : 'dead' + let command: string | null + try { + command = readCommand(session.pid) + } catch { + const latestLiveness = getLiveness() + return result( + latestLiveness === 'alive' ? 'unreadable' : latestLiveness, + ) + } + const latestLiveness = getLiveness() + if (latestLiveness !== 'alive') return result(latestLiveness) + if (command == null || command.trim() === '') { + return result('unreadable') + } + return result( + commandLineMatchesBackgroundSession(command, session) + ? 'matches' + : 'mismatch', + ) +} + +export function getBackgroundSessionProcessLiveness( + pid: number, + options?: BackgroundSessionProcessIdentityOptions, +): BackgroundSessionProcessLiveness { + if (options?.isProcessAlive) { + try { + return options.isProcessAlive(pid) ? 'alive' : 'not-running' + } catch { + return 'unreadable' + } + } + if (pid <= 1) return 'not-running' + + const signalProcess = options?.signalProcess ?? process.kill + try { + signalProcess(pid, 0) + return 'alive' + } catch (error) { + return isErrno(error, 'ESRCH') ? 'not-running' : 'unreadable' + } } export function isBackgroundSessionProcessAlive( session: BackgroundSession, - options?: { - isProcessAlive?: (pid: number) => boolean - getProcessCommand?: (pid: number) => string | null - }, + options?: BackgroundSessionProcessIdentityOptions, ): boolean { - return getBackgroundSessionProcessState(session, options) === 'alive' + return ( + verifyBackgroundSessionProcessIdentity(session, options).state === 'matches' + ) } export async function markBackgroundSessionKilled(