diff --git a/packages/host/src/gh.ts b/packages/host/src/gh.ts index 8738509e..0c2f637b 100644 --- a/packages/host/src/gh.ts +++ b/packages/host/src/gh.ts @@ -112,7 +112,9 @@ export async function runGhAction(options: GhRunOptions): Promise { if (settled) return; settled = true; if (timer !== undefined) clearTimeout(timer); - if (killTimer !== undefined) clearInterval(killTimer); + // A closed parent does not prove the process group is gone; the repeat-kill interval below + // chases a fork created between the group signal and its delivery. Keep it until the group + // no longer exists. const fullStdout = stdout + stdoutDecoder.end(); const fullStderr = stderr + stderrDecoder.end(); stdout = clampUtf8(fullStdout, maxBytes); @@ -121,22 +123,33 @@ export async function runGhAction(options: GhRunOptions): Promise { resolve({ ...result, stdout, stderr, truncated, timedOut }); }; - const signalGroup = (): void => { - if (child.pid === undefined) return; + const signalGroup = (): boolean => { + if (child.pid === undefined) return false; try { // The child is its own process-group leader; kill the group so a gh-spawned helper does // not keep the pipe open after timeout or output overflow. process.kill(-child.pid, 'SIGKILL'); - } catch { - child.kill('SIGKILL'); + return true; + } catch (error) { + // ESRCH means the group itself is gone; do not fall back to the parent handle, which can + // report success while only a zombie remains. + if ((error as NodeJS.ErrnoException).code === 'ESRCH') return false; + return child.exitCode === null && child.signalCode === null ? child.kill('SIGKILL') : false; } }; const kill = (): void => { - signalGroup(); - // A child forked between the signal and the process-group scan can retain the pipe. Keep - // signalling until close, then finish clears this interval. - if (killTimer === undefined && !settled) killTimer = setInterval(signalGroup, 100); + // Keep SIGKILLing after the parent closes: close does not prove a forked helper is gone. + // The interval stops itself when signalGroup can no longer find the group. + if (settled || killTimer !== undefined) return; + if (!signalGroup()) return; + const handle = setInterval(() => { + if (signalGroup()) return; + clearInterval(handle); + if (killTimer === handle) killTimer = undefined; + }, 100); + handle.unref(); + killTimer = handle; }; const take = (stream: 'stdout' | 'stderr', chunk: Buffer): void => { diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 2f7fed4a..b63ea091 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { ChildProcess } from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -54,6 +55,42 @@ describe('runGhAction', { skip: onWindows }, () => { assert.ok(Date.now() - started < 2000); }); + it('stops repeating the group kill once the group is gone', async () => { + const gh = shim('sleep 5'); + const realKill = process.kill; + const realChildKill = ChildProcess.prototype.kill; + const kills: number[] = []; + process.kill = ((pid: number, signal?: string | number) => { + kills.push(Date.now()); + return realKill.call(process, pid, signal); + }) as typeof process.kill; + ChildProcess.prototype.kill = function (signal?: NodeJS.Signals | number): boolean { + kills.push(Date.now()); + return realChildKill.call(this, signal); + }; + try { + const result = await runGhAction({ + action: 'pr:view', + values: { number: 1 }, + repo: 'acme/widget', + token: 't', + ghPath: gh, + timeoutMs: 50, + env: { PATH: process.env.PATH }, + }); + assert.equal(result.timedOut, true); + const atResolve = kills.length; + await new Promise((resolve) => setTimeout(resolve, 400)); + assert.ok( + kills.length - atResolve <= 2, + `kept killing: ${kills.length - atResolve} attempts in 400ms`, + ); + } finally { + process.kill = realKill; + ChildProcess.prototype.kill = realChildKill; + } + }); + it('caps output and reports truncation', async () => { const gh = shim('printf "0123456789"'); const result = await runGhAction({