Repository navigation
fix(core): Reclaim command hook process trees #10100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
doudouOUC
merged 5 commits into
QwenLM:main
from
doudouOUC:fix/hook-process-tree-cancellation
Aug 27, 2026
+1,441
−51
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
1959a40
fix(core): reclaim command hook process trees
doudouOUC 6359a95
codex: address PR review feedback (#10100)
doudouOUC abdafee
codex: address PR review feedback (#10100)
doudouOUC 86ecb96
codex: address PR review feedback (#10100)
doudouOUC fb0ed9f
codex: address PR review feedback (#10100)
doudouOUC File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # 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. | ||
|
|
||
| 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. | ||
|
|
||
| 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. | ||
| - 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, 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { 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 { 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<boolean>, | ||
| timeoutMs: number, | ||
| ): Promise<void> => { | ||
| 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<number | undefined> => { | ||
| 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<typeof spawnSync>; | ||
| 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); | ||
| }, | ||
| ); | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.