diff --git a/docs/design/hook-process-tree-cancellation.md b/docs/design/hook-process-tree-cancellation.md new file mode 100644 index 00000000000..b32b5239346 --- /dev/null +++ b/docs/design/hook-process-tree-cancellation.md @@ -0,0 +1,38 @@ +# Hook Process Tree Cancellation + +## Problem + +Command hooks run through a shell and can create nested processes. HookRunner currently signals only the direct shell process and uses `ChildProcess.killed` to decide whether to escalate from SIGTERM to SIGKILL. That property records that a signal was sent, not that the process exited, so an unresponsive shell can prevent escalation and descendants can survive as orphans. + +## Design + +On POSIX, HookRunner starts each command hook as a detached child so the shell leads an owned process group. Normal completion is unchanged. When the hook times out or receives an AbortSignal, HookRunner starts one idempotent termination operation: + +1. Send SIGTERM to the owned process group. +2. Poll for group existence for up to two seconds. +3. If the group still exists, send SIGKILL to the group. +4. Return the existing timeout or cancellation result only after termination reaches one of those terminal actions. + +The root child closing does not cancel escalation because descendants can remain in the group after the root exits. After SIGKILL is accepted, HookRunner does not wait for process IDs to disappear: terminated processes may remain briefly visible as zombies until their new parent reaps them. + +While a POSIX command hook is running, HookRunner keeps its owned process group registered with a synchronous process-exit fallback. If the parent reaches Node's exit event before normal cancellation completes, the fallback sends SIGKILL to every active hook group instead of leaving the detached tree behind. Temporary SIGHUP, SIGINT, SIGQUIT, and SIGTERM handlers reclaim active groups before either re-raising an otherwise unhandled signal or leaving graceful parent shutdown to an application handler. + +Windows does not expose POSIX process-group signals. HookRunner instead invokes the absolute System32 `taskkill.exe` path asynchronously with `/f /t /pid` and a bounded execution time. A failed taskkill falls back to force-killing the direct child and emits a diagnostic warning. If the root process has already exited, taskkill cannot reconstruct descendants from that former PID; reclaiming that case requires a Windows Job Object or descendant tracking and remains outside this change. + +Timeout and AbortSignal races share the same termination promise. Abort retains its existing result precedence, and normal hook success, output parsing, exit-code handling, and timeout defaults remain unchanged. + +After tree termination, HookRunner waits up to one second for the root child to close so its stdout and stderr streams can drain. If the close event never arrives, it destroys the streams and returns the cancellation result, keeping the cancellation path bounded. + +## Non-goals + +- Propagating the ACP session initialization deadline into SessionStart. +- Managing the ACP child process tree. +- Reaping processes that deliberately leave the owned group through daemonization or a new session. +- Preserving controlling-terminal access for POSIX command hooks. Creating the owned process group uses a detached session, so commands that open `/dev/tty` directly are unsupported. +- Reclaiming a detached group after an untrappable parent SIGKILL or host failure, because those failures prevent all JavaScript exit handling from running. +- Changing extension configuration, hook timeout defaults, or AsyncHookRegistry's unused process field. + +## Test plan + +- Unit-test process-group ownership, TERM-to-KILL timing, root-close races, timeout/abort races, parent-exit fallback, normal completion, spawn errors, and Windows taskkill fallback. +- Run a POSIX process test whose descendant confirms receipt of SIGTERM, ignores it, and is then made non-running by group SIGKILL. diff --git a/packages/core/src/hooks/hook-runner.process.test.ts b/packages/core/src/hooks/hook-runner.process.test.ts new file mode 100644 index 00000000000..6c5161ee2dd --- /dev/null +++ b/packages/core/src/hooks/hook-runner.process.test.ts @@ -0,0 +1,432 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { spawn, spawnSync } from 'node:child_process'; +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { HookRunner } from './hookRunner.js'; +import { HookEventName, HooksConfigSource, HookType } from './types.js'; +import type { HookInput } from './types.js'; + +const waitFor = async ( + predicate: () => boolean | Promise, + timeoutMs: number, +): Promise => { + const deadline = Date.now() + timeoutMs; + while (!(await predicate())) { + if (Date.now() >= deadline) { + throw new Error(`Condition not met within ${timeoutMs}ms`); + } + await new Promise((resolve) => setTimeout(resolve, 25)); + } +}; + +const readPid = async (path: string): Promise => { + try { + const pid = Number.parseInt(await readFile(path, 'utf8'), 10); + return Number.isSafeInteger(pid) && pid > 0 ? pid : undefined; + } catch { + return undefined; + } +}; + +const isRunning = (pid: number): boolean => { + try { + process.kill(pid, 0); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ESRCH') { + return false; + } + return true; + } + + const ps = process.platform === 'linux' ? '/usr/bin/ps' : '/bin/ps'; + let result: ReturnType; + try { + result = spawnSync(ps, ['-o', 'stat=', '-p', pid.toString()], { + encoding: 'utf8', + }); + } catch { + return true; + } + if (result.error || typeof result.stdout !== 'string') { + return true; + } + if (result.status === 1) { + return false; + } + if (result.status !== 0) { + return true; + } + return !result.stdout.trim().startsWith('Z'); +}; + +describe.skipIf(process.platform === 'win32')( + 'HookRunner process tree cancellation', + () => { + it('reaps a descendant that ignores SIGTERM before returning', async () => { + const tempDir = await mkdtemp(join(tmpdir(), 'qwen-hook-tree-')); + const fixturePath = join(tempDir, 'hook-tree.mjs'); + const descendantFixturePath = join(tempDir, 'descendant.mjs'); + const rootPidPath = join(tempDir, 'root.pid'); + const descendantPidPath = join(tempDir, 'descendant.pid'); + const descendantReadyPath = join(tempDir, 'descendant.ready'); + const descendantTermPath = join(tempDir, 'descendant.term'); + const controller = new AbortController(); + let rootPid: number | undefined; + let descendantPid: number | undefined; + + try { + await writeFile( + fixturePath, + `import { spawn } from 'node:child_process'; +import { writeFileSync } from 'node:fs'; + +writeFileSync(process.argv[2], String(process.pid)); +const descendant = spawn(process.execPath, [process.argv[4], process.argv[5], process.argv[6]], { stdio: 'ignore' }); +writeFileSync(process.argv[3], String(descendant.pid)); +setInterval(() => {}, 1000); +`, + ); + await writeFile( + descendantFixturePath, + `import { writeFileSync } from 'node:fs'; + +process.on('SIGTERM', () => writeFileSync(process.argv[3], 'received')); +writeFileSync(process.argv[2], 'ready'); +setInterval(() => {}, 1000); +`, + ); + + const runner = new HookRunner(); + const input: HookInput = { + session_id: 'process-tree-test', + transcript_path: join(tempDir, 'transcript.jsonl'), + cwd: tempDir, + hook_event_name: HookEventName.PreToolUse, + timestamp: new Date().toISOString(), + }; + const command = `exec ${JSON.stringify(process.execPath)} ${JSON.stringify(fixturePath)} ${JSON.stringify(rootPidPath)} ${JSON.stringify(descendantPidPath)} ${JSON.stringify(descendantFixturePath)} ${JSON.stringify(descendantReadyPath)} ${JSON.stringify(descendantTermPath)}`; + + const resultPromise = runner.executeHook( + { + type: HookType.Command, + command, + source: HooksConfigSource.Project, + shell: 'bash', + timeout: 10_000, + }, + HookEventName.PreToolUse, + input, + controller.signal, + ); + + await waitFor(async () => { + rootPid = await readPid(rootPidPath); + descendantPid = await readPid(descendantPidPath); + return ( + rootPid !== undefined && + descendantPid !== undefined && + (await readFile(descendantReadyPath, 'utf8').catch(() => '')) === + 'ready' + ); + }, 5000); + + controller.abort(); + const result = await resultPromise; + + expect(result.error?.message).toBe( + 'Hook execution cancelled (aborted)', + ); + expect(await readFile(descendantTermPath, 'utf8')).toBe('received'); + await waitFor( + () => + !isRunning(rootPid as number) && + !isRunning(descendantPid as number), + 3000, + ); + } finally { + controller.abort(); + const rootStillRunning = rootPid ? isRunning(rootPid) : false; + const descendantStillRunning = descendantPid + ? isRunning(descendantPid) + : false; + if (rootPid && (rootStillRunning || descendantStillRunning)) { + try { + process.kill(-rootPid, 'SIGKILL'); + } catch { + // Already gone. + } + } + if (descendantPid && descendantStillRunning) { + try { + process.kill(descendantPid, 'SIGKILL'); + } catch { + // Already gone. + } + } + await rm(tempDir, { recursive: true, force: true }); + } + }, 15_000); + + it.each(['process-exit', 'signal-exit', 'handled-signal-exit'] as const)( + 'reaps an active hook tree on parent %s', + async (exitMode) => { + const tempDir = await mkdtemp(join(tmpdir(), 'qwen-hook-exit-')); + const driverPath = join(tempDir, 'driver.mjs'); + const fixturePath = join(tempDir, 'hook-tree.mjs'); + const descendantFixturePath = join(tempDir, 'descendant.mjs'); + const rootPidPath = join(tempDir, 'root.pid'); + const descendantPidPath = join(tempDir, 'descendant.pid'); + const descendantReadyPath = join(tempDir, 'descendant.ready'); + const driverReadyPath = join(tempDir, 'driver.ready'); + const upperCompletedPath = join(tempDir, 'upper.completed'); + let driverPid: number | undefined; + let rootPid: number | undefined; + let descendantPid: number | undefined; + + try { + await writeFile( + driverPath, + `import { readFileSync, writeFileSync } from 'node:fs'; + +const { HookRunner } = await import(process.argv[2]); +const [tempDir, fixturePath, rootPidPath, descendantPidPath, descendantFixturePath, descendantReadyPath, driverReadyPath, upperCompletedPath, exitMode] = process.argv.slice(3); +const runner = new HookRunner(); +const controller = new AbortController(); +if (exitMode === 'handled-signal-exit') { + process.once('SIGTERM', async () => { + await resultPromise; + writeFileSync(upperCompletedPath, 'completed'); + process.exit(77); + }); +} +const resultPromise = runner.executeHook( + { type: 'command', command: \`exec \${JSON.stringify(process.execPath)} \${JSON.stringify(fixturePath)} \${JSON.stringify(rootPidPath)} \${JSON.stringify(descendantPidPath)} \${JSON.stringify(descendantFixturePath)} \${JSON.stringify(descendantReadyPath)}\`, source: 'project', shell: 'bash', timeout: 60_000 }, + 'PreToolUse', + { session_id: 'parent-exit-test', transcript_path: \`\${tempDir}/transcript.jsonl\`, cwd: tempDir, hook_event_name: 'PreToolUse', timestamp: new Date().toISOString() }, + controller.signal, +); +while (true) { + try { + if (readFileSync(descendantReadyPath, 'utf8') === 'ready') break; + } catch {} + await new Promise((resolve) => setTimeout(resolve, 25)); +} +writeFileSync(driverReadyPath, 'ready'); +if (exitMode === 'process-exit') process.exit(0); +setInterval(() => {}, 1000); +`, + ); + await writeFile( + fixturePath, + `import { spawn } from 'node:child_process'; +import { writeFileSync } from 'node:fs'; + +writeFileSync(process.argv[2], String(process.pid)); +const descendant = spawn(process.execPath, [process.argv[4], process.argv[5]], { stdio: 'ignore' }); +writeFileSync(process.argv[3], String(descendant.pid)); +setInterval(() => {}, 1000); +`, + ); + await writeFile( + descendantFixturePath, + `import { writeFileSync } from 'node:fs'; + +process.on('SIGTERM', () => {}); +process.on('SIGHUP', () => {}); +writeFileSync(process.argv[2], 'ready'); +setInterval(() => {}, 1000); +`, + ); + + const driver = spawn( + process.execPath, + [ + '--import=tsx/esm', + driverPath, + new URL('./hookRunner.ts', import.meta.url).href, + tempDir, + fixturePath, + rootPidPath, + descendantPidPath, + descendantFixturePath, + descendantReadyPath, + driverReadyPath, + upperCompletedPath, + exitMode, + ], + { + cwd: fileURLToPath(new URL('../../../../', import.meta.url)), + stdio: 'ignore', + }, + ); + driverPid = driver.pid; + const driverExit = new Promise<{ + code: number | null; + signal: NodeJS.Signals | null; + }>((resolve, reject) => { + driver.on('error', reject); + driver.on('exit', (code, signal) => resolve({ code, signal })); + }); + + await waitFor(async () => { + rootPid = await readPid(rootPidPath); + descendantPid = await readPid(descendantPidPath); + return ( + rootPid !== undefined && + descendantPid !== undefined && + (await readFile(driverReadyPath, 'utf8').catch(() => '')) === + 'ready' + ); + }, 5000); + if (exitMode !== 'process-exit') { + process.kill(driverPid as number, 'SIGTERM'); + } + + const exit = await driverExit; + expect(exit).toEqual( + exitMode === 'process-exit' + ? { code: 0, signal: null } + : exitMode === 'signal-exit' + ? { code: null, signal: 'SIGTERM' } + : { code: 77, signal: null }, + ); + if (exitMode === 'handled-signal-exit') { + expect(await readFile(upperCompletedPath, 'utf8')).toBe( + 'completed', + ); + } + await waitFor( + () => + !isRunning(rootPid as number) && + !isRunning(descendantPid as number), + 3000, + ); + } finally { + if (driverPid && isRunning(driverPid)) { + try { + process.kill(driverPid, 'SIGKILL'); + } catch { + // Already gone. + } + } + if (rootPid && isRunning(rootPid)) { + try { + process.kill(-rootPid, 'SIGKILL'); + } catch { + // Already gone. + } + } + if (descendantPid && isRunning(descendantPid)) { + try { + process.kill(descendantPid, 'SIGKILL'); + } catch { + // Already gone. + } + } + await rm(tempDir, { recursive: true, force: true }); + } + }, + 15_000, + ); + + it.each([ + ['a MessageDisplay hook', false], + ['an async command hook', true], + ] as const)('lets %s finish after parent exit', async (_, isAsync) => { + const tempDir = await mkdtemp(join(tmpdir(), 'qwen-hook-survive-')); + const driverPath = join(tempDir, 'driver.mjs'); + const fixturePath = join(tempDir, 'hook.mjs'); + const readyPath = join(tempDir, 'hook.ready'); + const completedPath = join(tempDir, 'hook.completed'); + const pidPath = join(tempDir, 'hook.pid'); + let hookPid: number | undefined; + + try { + await writeFile( + driverPath, + `import { readFileSync } from 'node:fs'; + +const { HookRunner } = await import(process.argv[2]); +const [tempDir, fixturePath, readyPath, completedPath, pidPath, isAsync] = process.argv.slice(3); +const runner = new HookRunner(); +void runner.executeHook( + { type: 'command', command: \`exec \${JSON.stringify(process.execPath)} \${JSON.stringify(fixturePath)} \${JSON.stringify(readyPath)} \${JSON.stringify(completedPath)} \${JSON.stringify(pidPath)}\`, source: 'project', shell: 'bash', timeout: 60_000, async: isAsync === 'true' }, + isAsync === 'true' ? 'PreToolUse' : 'MessageDisplay', + { session_id: 'parent-exit-survival-test', transcript_path: \`\${tempDir}/transcript.jsonl\`, cwd: tempDir, hook_event_name: isAsync === 'true' ? 'PreToolUse' : 'MessageDisplay', timestamp: new Date().toISOString() }, +); +while (true) { + try { + if (readFileSync(readyPath, 'utf8') === 'ready') break; + } catch {} + await new Promise((resolve) => setTimeout(resolve, 25)); +} +process.exit(0); +`, + ); + await writeFile( + fixturePath, + `import { writeFileSync } from 'node:fs'; + +writeFileSync(process.argv[4], String(process.pid)); +writeFileSync(process.argv[2], 'ready'); +await new Promise((resolve) => setTimeout(resolve, 250)); +writeFileSync(process.argv[3], 'completed'); +`, + ); + + const driver = spawn( + process.execPath, + [ + '--import=tsx/esm', + driverPath, + new URL('./hookRunner.ts', import.meta.url).href, + tempDir, + fixturePath, + readyPath, + completedPath, + pidPath, + String(isAsync), + ], + { + cwd: fileURLToPath(new URL('../../../../', import.meta.url)), + stdio: 'ignore', + }, + ); + const exit = await new Promise<{ + code: number | null; + signal: NodeJS.Signals | null; + }>((resolve, reject) => { + driver.on('error', reject); + driver.on('exit', (code, signal) => resolve({ code, signal })); + }); + + expect(exit).toEqual({ code: 0, signal: null }); + hookPid = await readPid(pidPath); + expect(hookPid).toBeDefined(); + await waitFor( + async () => + (await readFile(completedPath, 'utf8').catch(() => '')) === + 'completed', + 3000, + ); + } finally { + if (hookPid && isRunning(hookPid)) { + try { + process.kill(-hookPid, 'SIGKILL'); + } catch { + // Already gone. + } + } + await rm(tempDir, { recursive: true, force: true }); + } + }); + }, +); diff --git a/packages/core/src/hooks/hookRunner.test.ts b/packages/core/src/hooks/hookRunner.test.ts index f832f495628..6e8fbe5cdf5 100644 --- a/packages/core/src/hooks/hookRunner.test.ts +++ b/packages/core/src/hooks/hookRunner.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { HookRunner } from './hookRunner.js'; import { HookEventName, @@ -21,12 +21,14 @@ import type { // Hoisted mock const mockSpawn = vi.hoisted(() => vi.fn()); +const mockExecFile = vi.hoisted(() => vi.fn()); vi.mock('node:child_process', async () => { const actual = await vi.importActual('node:child_process'); return { ...actual, spawn: mockSpawn, + execFile: mockExecFile, }; }); @@ -38,6 +40,11 @@ describe('HookRunner', () => { vi.clearAllMocks(); }); + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + const createMockInput = (overrides: Partial = {}): HookInput => ({ session_id: 'test-session', transcript_path: '/test/transcript', @@ -82,6 +89,59 @@ describe('HookRunner', () => { return mockProcess; }; + const createControllableMockProcess = (pid = 4321) => { + type Listener = (...args: unknown[]) => void; + const listeners = new Map(); + const addListener = (event: string, callback: Listener) => { + const eventListeners = listeners.get(event) ?? []; + eventListeners.push(callback); + listeners.set(event, eventListeners); + }; + const createStream = () => { + const dataListeners: Listener[] = []; + return { + on: vi.fn((event: string, callback: Listener) => { + if (event === 'data') { + dataListeners.push(callback); + } + }), + destroy: vi.fn(), + emitData: (data: Buffer) => { + for (const listener of dataListeners) { + listener(data); + } + }, + }; + }; + const stdin = createStream(); + const stdout = createStream(); + const stderr = createStream(); + const mockProcess = { + pid, + stdin: { + ...stdin, + write: vi.fn(), + end: vi.fn(), + }, + stdout, + stderr, + killed: true, + exitCode: null, + signalCode: null, + kill: vi.fn(), + on: vi.fn((event: string, callback: Listener) => { + addListener(event, callback); + return mockProcess; + }), + emit: (event: string, ...args: unknown[]) => { + for (const listener of listeners.get(event) ?? []) { + listener(...args); + } + }, + }; + return mockProcess; + }; + describe('executeHook', () => { it('should return error when hook command is missing', async () => { const hookConfig: HookConfig = { @@ -1027,6 +1087,615 @@ describe('HookRunner', () => { }); }); + describe('process tree cancellation', () => { + const hookConfig: HookConfig = { + type: HookType.Command, + command: 'long-running-command', + source: HooksConfigSource.Project, + timeout: 10_000, + }; + + const createNoSuchProcessError = () => + Object.assign(new Error('no such process'), { code: 'ESRCH' }); + + it('owns a POSIX process group without signalling it on normal completion', async () => { + const mockProcess = createMockProcess(0, 'done'); + mockSpawn.mockReturnValue(mockProcess); + const killSpy = vi.spyOn(process, 'kill'); + + const result = await hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + ); + + expect(result.success).toBe(true); + expect(mockSpawn.mock.calls[0][2].detached).toBe( + process.platform !== 'win32', + ); + expect(killSpy).not.toHaveBeenCalled(); + }); + + it('force-kills an active POSIX hook group when the parent exits', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const exitListenersBefore = process.listeners('exit'); + const sighupListenersBefore = process.listeners('SIGHUP'); + const sigintListenersBefore = process.listeners('SIGINT'); + const sigquitListenersBefore = process.listeners('SIGQUIT'); + const sigtermListenersBefore = process.listeners('SIGTERM'); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const killSpy = vi.spyOn(process, 'kill').mockReturnValue(true); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + ); + const exitListener = process + .listeners('exit') + .find((listener) => !exitListenersBefore.includes(listener)); + + expect(exitListener).toBeDefined(); + exitListener?.(0); + expect(killSpy).toHaveBeenCalledWith(-mockProcess.pid, 'SIGKILL'); + + mockProcess.emit('close', null); + await resultPromise; + expect(process.listeners('exit')).toEqual(exitListenersBefore); + expect(process.listeners('SIGHUP')).toEqual(sighupListenersBefore); + expect(process.listeners('SIGINT')).toEqual(sigintListenersBefore); + expect(process.listeners('SIGQUIT')).toEqual(sigquitListenersBefore); + expect(process.listeners('SIGTERM')).toEqual(sigtermListenersBefore); + }); + + it('kills active hooks while leaving parent signals to an application handler', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const exitListenersBefore = process.listeners('exit'); + const listenersBefore = process.listeners('SIGTERM'); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const killSpy = vi.spyOn(process, 'kill').mockReturnValue(true); + const applicationHandler = vi.fn(); + process.on('SIGTERM', applicationHandler); + + try { + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + ); + const hookSignalHandler = process + .listeners('SIGTERM') + .find( + (listener) => + listener !== applicationHandler && + !listenersBefore.includes(listener), + ); + const exitListener = process + .listeners('exit') + .find((listener) => !exitListenersBefore.includes(listener)); + + expect(hookSignalHandler).toBeDefined(); + expect(exitListener).toBeDefined(); + hookSignalHandler?.('SIGTERM'); + expect(killSpy).toHaveBeenCalledWith(-mockProcess.pid, 'SIGKILL'); + expect(killSpy).not.toHaveBeenCalledWith(process.pid, 'SIGTERM'); + expect(process.listeners('exit')).toContain(exitListener); + + mockProcess.emit('close', 0); + await resultPromise; + } finally { + process.removeListener('SIGTERM', applicationHandler); + } + }); + + it.each(['SIGHUP', 'SIGINT', 'SIGQUIT'] as const)( + 'force-kills active hooks and re-raises %s when unhandled', + async (signal) => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const listenersBefore = process.listeners(signal); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const killSpy = vi.spyOn(process, 'kill').mockReturnValue(true); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + ); + const hookSignalHandler = process + .listeners(signal) + .find((listener) => !listenersBefore.includes(listener)); + + expect(hookSignalHandler).toBeDefined(); + hookSignalHandler?.(signal); + expect(killSpy).toHaveBeenCalledWith(-mockProcess.pid, 'SIGKILL'); + expect(killSpy).toHaveBeenCalledWith(process.pid, signal); + + mockProcess.emit('close', null); + await resultPromise; + }, + ); + + it('keeps parent cleanup registered while another hook is active', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const exitListenersBefore = process.listeners('exit'); + const firstProcess = createControllableMockProcess(4321); + const secondProcess = createControllableMockProcess(4322); + mockSpawn + .mockReturnValueOnce(firstProcess) + .mockReturnValueOnce(secondProcess); + const killSpy = vi.spyOn(process, 'kill').mockReturnValue(true); + + const firstResult = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + ); + const secondResult = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + ); + firstProcess.emit('close', 0); + await firstResult; + const exitListener = process + .listeners('exit') + .find((listener) => !exitListenersBefore.includes(listener)); + + expect(exitListener).toBeDefined(); + exitListener?.(0); + expect(killSpy).toHaveBeenCalledWith(-secondProcess.pid, 'SIGKILL'); + + secondProcess.emit('close', 0); + await secondResult; + }); + + it('escalates to SIGKILL for the process group even after the root closes', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + vi.useFakeTimers(); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + let groupAlive = true; + const killSpy = vi + .spyOn(process, 'kill') + .mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + if (groupAlive) { + return true; + } + throw createNoSuchProcessError(); + } + if (target === -mockProcess.pid && signal === 'SIGKILL') { + groupAlive = false; + } + return true; + }); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('close', null); + let resolved = false; + void resultPromise.then(() => { + resolved = true; + }); + await vi.advanceTimersByTimeAsync(1999); + expect(resolved).toBe(false); + expect(killSpy.mock.calls).not.toContainEqual([ + -mockProcess.pid, + 'SIGKILL', + ]); + await vi.advanceTimersByTimeAsync(1); + const result = await resultPromise; + + expect(result.error?.message).toBe('Hook execution cancelled (aborted)'); + expect(killSpy.mock.calls).toContainEqual([-mockProcess.pid, 'SIGTERM']); + expect(killSpy.mock.calls).toContainEqual([-mockProcess.pid, 'SIGKILL']); + }); + + it('does not send SIGKILL when the process group exits after SIGTERM', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const killSpy = vi + .spyOn(process, 'kill') + .mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + throw createNoSuchProcessError(); + } + return true; + }); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('close', null); + const result = await resultPromise; + + expect(result.error?.message).toBe('Hook execution cancelled (aborted)'); + expect(killSpy.mock.calls).toContainEqual([-mockProcess.pid, 'SIGTERM']); + expect(killSpy.mock.calls).not.toContainEqual([ + -mockProcess.pid, + 'SIGKILL', + ]); + }); + + it('returns the timeout result after process group cleanup', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + vi.useFakeTimers(); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const killSpy = vi + .spyOn(process, 'kill') + .mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + throw createNoSuchProcessError(); + } + return true; + }); + + const resultPromise = hookRunner.executeHook( + { ...hookConfig, timeout: 100 }, + HookEventName.PreToolUse, + createMockInput(), + ); + await vi.advanceTimersByTimeAsync(100); + mockProcess.emit('close', null); + const result = await resultPromise; + + expect(result.error?.message).toBe('Hook timed out after 100ms'); + expect(killSpy.mock.calls).toContainEqual([-mockProcess.pid, 'SIGTERM']); + }); + + it('shares one termination when timeout and abort race, with abort taking precedence', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + vi.useFakeTimers(); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + let groupAlive = true; + const killSpy = vi + .spyOn(process, 'kill') + .mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + if (groupAlive) { + return true; + } + throw createNoSuchProcessError(); + } + if (target === -mockProcess.pid && signal === 'SIGKILL') { + groupAlive = false; + } + return true; + }); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + { ...hookConfig, timeout: 100 }, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + await vi.advanceTimersByTimeAsync(100); + controller.abort(); + await vi.advanceTimersByTimeAsync(2000); + mockProcess.emit('close', null); + const result = await resultPromise; + + expect(result.error?.message).toBe('Hook execution cancelled (aborted)'); + expect( + killSpy.mock.calls.filter( + ([target, signal]) => + target === -mockProcess.pid && signal === 'SIGTERM', + ), + ).toHaveLength(1); + expect( + killSpy.mock.calls.filter( + ([target, signal]) => + target === -mockProcess.pid && signal === 'SIGKILL', + ), + ).toHaveLength(1); + }); + + it('tree-kills through the absolute taskkill path on Windows', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + let taskkillCallback: ((error: Error | null) => void) | undefined; + mockExecFile.mockImplementation( + ( + _file: string, + _args: string[], + _options: object, + callback: (error: Error | null) => void, + ) => { + taskkillCallback = callback; + }, + ); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('close', null); + let resolved = false; + void resultPromise.then(() => { + resolved = true; + }); + await Promise.resolve(); + + expect(resolved).toBe(false); + taskkillCallback?.(null); + const result = await resultPromise; + + expect(result.error?.message).toBe('Hook execution cancelled (aborted)'); + expect(mockSpawn.mock.calls[0][2].detached).toBe(false); + expect(mockExecFile).toHaveBeenCalledWith( + expect.stringMatching(/\\System32\\taskkill\.exe$/i), + ['/f', '/t', '/pid', mockProcess.pid.toString()], + { + windowsHide: true, + timeout: 2000, + }, + expect.any(Function), + ); + expect(mockProcess.kill).not.toHaveBeenCalled(); + }); + + it('falls back to killing the direct child when taskkill fails', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + mockExecFile.mockImplementation( + ( + _file: string, + _args: string[], + _options: object, + callback: (error: Error | null) => void, + ) => { + callback(new Error('taskkill failed')); + }, + ); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('close', null); + await resultPromise; + + expect(mockProcess.kill).toHaveBeenCalledOnce(); + expect(mockProcess.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('falls back to the direct child when POSIX group signals fail', async () => { + vi.useFakeTimers(); + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const permissionError = Object.assign(new Error('not permitted'), { + code: 'EPERM', + }); + vi.spyOn(process, 'kill').mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + return true; + } + if (target === -mockProcess.pid) { + throw permissionError; + } + return true; + }); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('close', null); + await vi.advanceTimersByTimeAsync(2000); + await resultPromise; + + expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM'); + expect(mockProcess.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('force-kills the direct child when cancellation has no pid', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const mockProcess = { + ...createControllableMockProcess(), + pid: undefined, + }; + mockSpawn.mockReturnValue(mockProcess); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('close', null); + await resultPromise; + + expect(mockProcess.kill).toHaveBeenCalledOnce(); + expect(mockProcess.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('falls back to the direct child when taskkill throws synchronously', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + mockExecFile.mockImplementation(() => { + throw new Error('EMFILE'); + }); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('close', null); + await resultPromise; + + expect(mockProcess.kill).toHaveBeenCalledOnce(); + expect(mockProcess.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('waits for close after a cancellation-time child error', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + vi.useFakeTimers(); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + vi.spyOn(process, 'kill').mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + throw createNoSuchProcessError(); + } + return true; + }); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.emit('error', new Error('signal delivery failed')); + mockProcess.stdout.emitData(Buffer.from('final stdout')); + let resolved = false; + void resultPromise.then(() => { + resolved = true; + }); + await vi.advanceTimersByTimeAsync(0); + + expect(resolved).toBe(false); + mockProcess.emit('close', null); + const result = await resultPromise; + + expect(result.stdout).toBe('final stdout'); + }); + + it('drains final output before resolving cancellation', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + vi.spyOn(process, 'kill').mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + throw createNoSuchProcessError(); + } + return true; + }); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + mockProcess.stdout.emitData(Buffer.from('final stdout')); + mockProcess.stderr.emitData(Buffer.from('final stderr')); + let resolved = false; + void resultPromise.then(() => { + resolved = true; + }); + await Promise.resolve(); + await Promise.resolve(); + + expect(resolved).toBe(false); + mockProcess.emit('close', null); + const result = await resultPromise; + + expect(result.stdout).toBe('final stdout'); + expect(result.stderr).toBe('final stderr'); + expect(mockProcess.stdout.destroy).not.toHaveBeenCalled(); + expect(mockProcess.stderr.destroy).not.toHaveBeenCalled(); + }); + + it('bounds the output drain wait when close never arrives', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + vi.useFakeTimers(); + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + vi.spyOn(process, 'kill').mockImplementation((target, signal) => { + if (target === -mockProcess.pid && signal === 0) { + throw createNoSuchProcessError(); + } + return true; + }); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + controller.abort(); + let resolved = false; + void resultPromise.then(() => { + resolved = true; + }); + await vi.advanceTimersByTimeAsync(999); + expect(resolved).toBe(false); + await vi.advanceTimersByTimeAsync(1); + const result = await resultPromise; + + expect(result.error?.message).toBe('Hook execution cancelled (aborted)'); + expect(mockProcess.stdin.destroy).toHaveBeenCalledOnce(); + expect(mockProcess.stdout.destroy).toHaveBeenCalledOnce(); + expect(mockProcess.stderr.destroy).toHaveBeenCalledOnce(); + }); + + it('removes cancellation handling after a spawn error', async () => { + const mockProcess = createControllableMockProcess(); + mockSpawn.mockReturnValue(mockProcess); + const killSpy = vi.spyOn(process, 'kill'); + const controller = new AbortController(); + + const resultPromise = hookRunner.executeHook( + hookConfig, + HookEventName.PreToolUse, + createMockInput(), + controller.signal, + ); + mockProcess.emit('error', new Error('spawn failed')); + const result = await resultPromise; + controller.abort(); + + expect(result.error?.message).toBe('spawn failed'); + expect(killSpy).not.toHaveBeenCalled(); + }); + }); + describe('shell configuration', () => { it('should use global shell configuration when hookConfig.shell is not specified', async () => { const mockProcess = createMockProcess(0, '{"continue": true}'); diff --git a/packages/core/src/hooks/hookRunner.ts b/packages/core/src/hooks/hookRunner.ts index 4b9be7535a7..e3d2c96d8f6 100644 --- a/packages/core/src/hooks/hookRunner.ts +++ b/packages/core/src/hooks/hookRunner.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { spawn } from 'node:child_process'; +import { execFile, spawn, type ChildProcess } from 'node:child_process'; import { createHookOutput, HookEventName, HookType } from './types.js'; import type { HookConfig, @@ -46,12 +46,208 @@ const DEFAULT_HOOK_TIMEOUT = 60000; */ const MAX_OUTPUT_LENGTH = 1024 * 1024; +const HOOK_TERMINATE_GRACE_MS = 2000; +const HOOK_PROCESS_GROUP_POLL_MS = 50; +const HOOK_CHILD_CLOSE_WAIT_MS = 1000; +const WINDOWS_TASKKILL_TIMEOUT_MS = 2000; +const WINDOWS_TASKKILL = `${process.env['SystemRoot'] || 'C:\\Windows'}\\System32\\taskkill.exe`; + +const activePosixHookProcesses = new Set(); +let parentExitCleanupRegistered = false; + /** * Exit code constants for hook execution */ const EXIT_CODE_SUCCESS = 0; const EXIT_CODE_NON_BLOCKING_ERROR = 1; +function isNoSuchProcessError(error: unknown): boolean { + return (error as NodeJS.ErrnoException)?.code === 'ESRCH'; +} + +function signalProcessGroup( + pid: number, + signal: NodeJS.Signals, +): 'sent' | 'gone' | 'failed' { + try { + process.kill(-pid, signal); + return 'sent'; + } catch (error) { + if (isNoSuchProcessError(error)) { + return 'gone'; + } + debugLogger.warn( + `Failed to send ${signal} to hook process group ${pid}: ${error instanceof Error ? error.message : String(error)}`, + ); + return 'failed'; + } +} + +function isProcessGroupAlive(pid: number): boolean { + try { + process.kill(-pid, 0); + return true; + } catch (error) { + return !isNoSuchProcessError(error); + } +} + +async function waitForProcessGroupExit( + pid: number, + timeoutMs: number, +): Promise { + const deadline = Date.now() + timeoutMs; + while (isProcessGroupAlive(pid)) { + const remaining = deadline - Date.now(); + if (remaining <= 0) { + return false; + } + await new Promise((resolve) => + setTimeout(resolve, Math.min(HOOK_PROCESS_GROUP_POLL_MS, remaining)), + ); + } + return true; +} + +function killDirectChild(child: ChildProcess, signal: NodeJS.Signals): void { + try { + child.kill(signal); + } catch { + // The process already exited. + } +} + +function forceKillActivePosixHookProcesses(): void { + for (const child of activePosixHookProcesses) { + const pid = child.pid; + if (!pid) { + killDirectChild(child, 'SIGKILL'); + continue; + } + if (signalProcessGroup(pid, 'SIGKILL') === 'failed') { + killDirectChild(child, 'SIGKILL'); + } + } +} + +function handleParentSignal(signal: NodeJS.Signals): void { + forceKillActivePosixHookProcesses(); + if ( + process + .listeners(signal) + .some((listener) => listener !== handleParentSignal) + ) { + return; + } + + process.removeListener(signal, handleParentSignal); + process.kill(process.pid, signal); +} + +function registerActivePosixHookProcess(child: ChildProcess): void { + if (process.platform === 'win32' || !child.pid) { + return; + } + activePosixHookProcesses.add(child); + if (!parentExitCleanupRegistered) { + process.on('exit', forceKillActivePosixHookProcesses); + process.prependListener('SIGHUP', handleParentSignal); + process.prependListener('SIGINT', handleParentSignal); + process.prependListener('SIGQUIT', handleParentSignal); + process.prependListener('SIGTERM', handleParentSignal); + parentExitCleanupRegistered = true; + } +} + +function unregisterActivePosixHookProcess(child: ChildProcess): void { + activePosixHookProcesses.delete(child); + if (activePosixHookProcesses.size === 0 && parentExitCleanupRegistered) { + process.removeListener('exit', forceKillActivePosixHookProcesses); + process.removeListener('SIGHUP', handleParentSignal); + process.removeListener('SIGINT', handleParentSignal); + process.removeListener('SIGQUIT', handleParentSignal); + process.removeListener('SIGTERM', handleParentSignal); + parentExitCleanupRegistered = false; + } +} + +async function terminatePosixHookProcessTree( + child: ChildProcess, +): Promise { + // executeCommandHook makes child.pid the process-group leader on POSIX. + const pid = child.pid; + if (!pid) { + killDirectChild(child, 'SIGKILL'); + return; + } + + const termResult = signalProcessGroup(pid, 'SIGTERM'); + if (termResult === 'gone') { + return; + } + if (termResult === 'failed') { + killDirectChild(child, 'SIGTERM'); + } + + if (await waitForProcessGroupExit(pid, HOOK_TERMINATE_GRACE_MS)) { + return; + } + + debugLogger.debug( + `Hook process group ${pid} did not exit within ${HOOK_TERMINATE_GRACE_MS}ms after SIGTERM; escalating to SIGKILL`, + ); + const killResult = signalProcessGroup(pid, 'SIGKILL'); + if (killResult === 'failed') { + killDirectChild(child, 'SIGKILL'); + } +} + +async function terminateWindowsHookProcessTree( + child: ChildProcess, +): Promise { + const pid = child.pid; + if (!pid) { + killDirectChild(child, 'SIGKILL'); + return; + } + + await new Promise((resolve) => { + try { + execFile( + WINDOWS_TASKKILL, + ['/f', '/t', '/pid', pid.toString()], + { + windowsHide: true, + timeout: WINDOWS_TASKKILL_TIMEOUT_MS, + }, + (error) => { + if (error) { + debugLogger.warn( + `taskkill failed for hook process tree ${pid}: ${error.message}`, + ); + killDirectChild(child, 'SIGKILL'); + } + resolve(); + }, + ); + } catch (error) { + debugLogger.warn( + `taskkill threw for hook process tree ${pid}: ${error instanceof Error ? error.message : String(error)}`, + ); + killDirectChild(child, 'SIGKILL'); + resolve(); + } + }); +} + +async function terminateHookProcessTree(child: ChildProcess): Promise { + if (process.platform === 'win32') { + await terminateWindowsHookProcessTree(child); + return; + } + await terminatePosixHookProcessTree(child); +} + /** * Hook runner that executes command, HTTP, function, and prompt hooks */ @@ -578,6 +774,13 @@ export class HookRunner { let stderr = ''; let timedOut = false; let aborted = false; + let settled = false; + let terminationPromise: Promise | undefined; + let childClosed = false; + let resolveChildClosed: () => void; + const childClosedPromise = new Promise((resolve) => { + resolveChildClosed = resolve; + }); // Use hook-specific shell configuration if specified const shellConfig = this.getShellConfigForHook(hookConfig); @@ -606,37 +809,113 @@ export class HookRunner { cwd: input.cwd, stdio: ['pipe', 'pipe', 'pipe'], shell: false, + // Own a process group so cancellation can signal the entire tree. + detached: process.platform !== 'win32', }, ); + const survivesParentExit = + eventName === HookEventName.MessageDisplay || + this.isAsyncHook(hookConfig); + if (!survivesParentExit) { + registerActivePosixHookProcess(child); + } - // Helper to kill child process - const killChild = () => { - if (!child.killed) { - child.kill('SIGTERM'); - // Force kill after 2 seconds - setTimeout(() => { - if (!child.killed) { - child.kill('SIGKILL'); - } - }, 2000); + let abortListenerAttached = false; + + const cleanup = () => { + clearTimeout(timeoutHandle); + if (!survivesParentExit) { + unregisterActivePosixHookProcess(child); + } + if (signal && abortListenerAttached) { + signal.removeEventListener('abort', abortHandler); + abortListenerAttached = false; + } + }; + + const finish = (result: HookExecutionResult) => { + if (settled) { + return; + } + settled = true; + cleanup(); + resolve(result); + }; + + const finishCancellation = async () => { + try { + await terminationPromise; + } catch (error) { + debugLogger.warn( + `Unexpected hook process tree cleanup failure: ${error instanceof Error ? error.message : String(error)}`, + ); + } + + if (!childClosed) { + await new Promise((resolve) => { + const closeWaitHandle = setTimeout( + resolve, + HOOK_CHILD_CLOSE_WAIT_MS, + ); + void childClosedPromise.then(() => { + clearTimeout(closeWaitHandle); + resolve(); + }); + }); + } + + if (!childClosed) { + debugLogger.debug( + `Hook process ${child.pid ?? 'unknown'} did not close within ${HOOK_CHILD_CLOSE_WAIT_MS}ms after cancellation; destroying streams`, + ); + child.stdin?.destroy(); + child.stdout?.destroy(); + child.stderr?.destroy(); + } + + const duration = Date.now() - startTime; + finish({ + hookConfig, + eventName, + success: false, + error: new Error( + aborted + ? 'Hook execution cancelled (aborted)' + : `Hook timed out after ${timeout}ms`, + ), + stdout, + stderr, + duration, + }); + }; + + const startTermination = () => { + if (!terminationPromise) { + terminationPromise = terminateHookProcessTree(child); + void finishCancellation(); } }; // Set up timeout const timeoutHandle = setTimeout(() => { timedOut = true; - killChild(); + startTermination(); }, timeout); // Set up abort handler const abortHandler = () => { aborted = true; clearTimeout(timeoutHandle); - killChild(); + startTermination(); }; if (signal) { - signal.addEventListener('abort', abortHandler, { once: true }); + if (signal.aborted) { + abortHandler(); + } else { + signal.addEventListener('abort', abortHandler, { once: true }); + abortListenerAttached = true; + } } // Send input to stdin @@ -689,38 +968,12 @@ export class HookRunner { // Handle process exit child.on('close', (exitCode) => { - clearTimeout(timeoutHandle); - // Clean up abort listener - if (signal) { - signal.removeEventListener('abort', abortHandler); - } - const duration = Date.now() - startTime; - - if (aborted) { - resolve({ - hookConfig, - eventName, - success: false, - error: new Error('Hook execution cancelled (aborted)'), - stdout, - stderr, - duration, - }); - return; - } - - if (timedOut) { - resolve({ - hookConfig, - eventName, - success: false, - error: new Error(`Hook timed out after ${timeout}ms`), - stdout, - stderr, - duration, - }); + childClosed = true; + resolveChildClosed(); + if (aborted || timedOut) { return; } + const duration = Date.now() - startTime; // Parse output // Exit code 2 is a blocking error - ignore stdout, use stderr only @@ -757,7 +1010,7 @@ export class HookRunner { } const killedBySignal = exitCode === null; - resolve({ + finish({ hookConfig, eventName, success: exitCode === EXIT_CODE_SUCCESS, @@ -774,14 +1027,12 @@ export class HookRunner { // Handle process errors child.on('error', (error) => { - clearTimeout(timeoutHandle); - // Clean up abort listener - if (signal) { - signal.removeEventListener('abort', abortHandler); + if (aborted || timedOut) { + return; } const duration = Date.now() - startTime; - resolve({ + finish({ hookConfig, eventName, success: false,