From 93e08b6789da2d56aa0e00643d502aa392d99229 Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 06:19:41 +0000 Subject: [PATCH 1/8] fix: give gh SIGTERM before SIGKILL Signal the process group with SIGTERM, wait a short grace period, then keep SIGKILLing until close. This gives gh a chance to exit cleanly without leaving a forked child holding the pipe. --- packages/host/src/gh.ts | 27 +++++++++++++++++++-------- test/unit/gh.test.ts | 17 +++++++++++++++++ 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/packages/host/src/gh.ts b/packages/host/src/gh.ts index 8738509e..52aef700 100644 --- a/packages/host/src/gh.ts +++ b/packages/host/src/gh.ts @@ -37,6 +37,8 @@ export interface GhRunOptions { readonly host?: string; readonly ghPath?: string; readonly timeoutMs?: number; + /** How long SIGTERM gets before the process group is SIGKILLed. */ + readonly killGraceMs?: number; readonly maxOutputBytes?: number; /** Environment to copy only the proxy and PATH values from. */ readonly env?: NodeJS.ProcessEnv; @@ -85,6 +87,7 @@ export async function runGhAction(options: GhRunOptions): Promise { } const timeoutMs = options.timeoutMs ?? getGhAction(options.action).timeoutSeconds * 1000; + const killGraceMs = options.killGraceMs ?? 500; try { return await new Promise((resolve) => { @@ -98,6 +101,7 @@ export async function runGhAction(options: GhRunOptions): Promise { let timedOut = false; let settled = false; let timer: NodeJS.Timeout | undefined; + let termTimer: NodeJS.Timeout | undefined; let killTimer: NodeJS.Timeout | undefined; const child = spawn(options.ghPath ?? 'gh', argv, { @@ -112,6 +116,7 @@ export async function runGhAction(options: GhRunOptions): Promise { if (settled) return; settled = true; if (timer !== undefined) clearTimeout(timer); + if (termTimer !== undefined) clearTimeout(termTimer); if (killTimer !== undefined) clearInterval(killTimer); const fullStdout = stdout + stdoutDecoder.end(); const fullStderr = stderr + stderrDecoder.end(); @@ -121,22 +126,28 @@ export async function runGhAction(options: GhRunOptions): Promise { resolve({ ...result, stdout, stderr, truncated, timedOut }); }; - const signalGroup = (): void => { + const signalGroup = (signal: NodeJS.Signals): void => { if (child.pid === undefined) return; try { - // The child is its own process-group leader; kill the group so a gh-spawned helper does + // The child is its own process-group leader; signal the group so a gh-spawned helper does // not keep the pipe open after timeout or output overflow. - process.kill(-child.pid, 'SIGKILL'); + process.kill(-child.pid, signal); } catch { - child.kill('SIGKILL'); + child.kill(signal); } }; 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); + signalGroup('SIGTERM'); + // Give gh a chance to exit cleanly, then keep SIGKILLing the group until close: a child forked + // between the signal and the process-group scan can otherwise retain the pipe forever. + if (termTimer === undefined && killTimer === undefined && !settled) { + termTimer = setTimeout(() => { + if (settled) return; + signalGroup('SIGKILL'); + killTimer = setInterval(() => signalGroup('SIGKILL'), 100); + }, killGraceMs); + } }; const take = (stream: 'stdout' | 'stderr', chunk: Buffer): void => { diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 2f7fed4a..07a48a4f 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -54,6 +54,23 @@ describe('runGhAction', { skip: onWindows }, () => { assert.ok(Date.now() - started < 2000); }); + it('sends SIGTERM before SIGKILL', async () => { + const gh = shim('trap "exit 0" TERM; sleep 5'); + const result = await runGhAction({ + action: 'pr:view', + values: { number: 1 }, + repo: 'acme/widget', + token: 't', + ghPath: gh, + timeoutMs: 50, + killGraceMs: 250, + env: { PATH: process.env.PATH }, + }); + assert.equal(result.timedOut, true); + assert.equal(result.code, 0); + assert.equal(result.signal, null); + }); + it('caps output and reports truncation', async () => { const gh = shim('printf "0123456789"'); const result = await runGhAction({ From 0e834ce7df348758ce32a9d58b9291eee792e057 Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 06:29:08 +0000 Subject: [PATCH 2/8] fix: keep killing a gh group after its parent closes Child closure does not prove the process group is gone; keep the SIGKILL escalation until the group is empty, and test an orphan that ignores SIGTERM. --- packages/host/src/gh.ts | 45 ++++++++++++++++++++++++++++++++++------- test/unit/gh.test.ts | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/packages/host/src/gh.ts b/packages/host/src/gh.ts index 52aef700..33b66a97 100644 --- a/packages/host/src/gh.ts +++ b/packages/host/src/gh.ts @@ -100,6 +100,7 @@ export async function runGhAction(options: GhRunOptions): Promise { let truncated = false; let timedOut = false; let settled = false; + let terminating = false; let timer: NodeJS.Timeout | undefined; let termTimer: NodeJS.Timeout | undefined; let killTimer: NodeJS.Timeout | undefined; @@ -112,12 +113,30 @@ export async function runGhAction(options: GhRunOptions): Promise { stdio: ['ignore', 'pipe', 'pipe'], }); + const groupAlive = (): boolean => { + if (child.pid === undefined) return false; + try { + process.kill(-child.pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } + }; + + const stopEscalation = (): void => { + if (termTimer !== undefined) clearTimeout(termTimer); + if (killTimer !== undefined) clearInterval(killTimer); + termTimer = undefined; + killTimer = undefined; + }; + const finish = (result: Omit): void => { if (settled) return; settled = true; if (timer !== undefined) clearTimeout(timer); - if (termTimer !== undefined) clearTimeout(termTimer); - if (killTimer !== undefined) clearInterval(killTimer); + // A closed parent does not prove the process group is gone: a forked gh helper can keep + // running after the parent's stdio closes. Keep SIGKILLing until the group has exited. + if (!terminating || !groupAlive()) stopEscalation(); const fullStdout = stdout + stdoutDecoder.end(); const fullStderr = stderr + stderrDecoder.end(); stdout = clampUtf8(fullStdout, maxBytes); @@ -138,15 +157,27 @@ export async function runGhAction(options: GhRunOptions): Promise { }; const kill = (): void => { + terminating = true; signalGroup('SIGTERM'); - // Give gh a chance to exit cleanly, then keep SIGKILLing the group until close: a child forked - // between the signal and the process-group scan can otherwise retain the pipe forever. - if (termTimer === undefined && killTimer === undefined && !settled) { + // Give gh a chance to exit cleanly, then keep SIGKILLing the group until it is gone: a child + // forked between the signal and the process-group scan can otherwise retain the pipe forever. + if (termTimer === undefined && killTimer === undefined) { termTimer = setTimeout(() => { - if (settled) return; + if (!groupAlive()) { + stopEscalation(); + return; + } signalGroup('SIGKILL'); - killTimer = setInterval(() => signalGroup('SIGKILL'), 100); + killTimer = setInterval(() => { + if (!groupAlive()) { + stopEscalation(); + return; + } + signalGroup('SIGKILL'); + }, 100); + killTimer.unref(); }, killGraceMs); + termTimer.unref(); } }; diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 07a48a4f..00d1d5bb 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -169,3 +169,41 @@ describe('runGhAction options', { skip: onWindows }, () => { } }); }); + +describe('runGhAction orphan cleanup', { skip: onWindows }, () => { + it('keeps killing the group after the parent has closed', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-orphan-')); + const pidFile = path.join(dir, 'child.pid'); + const gh = shim( + [ + '( trap "" TERM; exec >/dev/null 2>&1; sleep 5 ) &', + 'echo $! > "$GH_TOKEN"', + 'trap "exit 0" TERM', + 'sleep 5', + ].join('\n'), + ); + try { + const result = await runGhAction({ + action: 'pr:view', + values: { number: 1 }, + repo: 'acme/widget', + token: pidFile, + ghPath: gh, + timeoutMs: 100, + killGraceMs: 50, + env: { PATH: process.env.PATH }, + }); + assert.equal(result.timedOut, true); + let pid = 0; + for (let i = 0; i < 20 && pid === 0; i += 1) { + await new Promise((resolve) => setTimeout(resolve, 10)); + if (fs.existsSync(pidFile)) pid = Number(fs.readFileSync(pidFile, 'utf8').trim()); + } + assert.ok(pid > 0, 'the orphan wrote its pid'); + await new Promise((resolve) => setTimeout(resolve, 300)); + assert.throws(() => process.kill(pid, 0), /ESRCH/); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); From 44163b242a085072b91f1ffede8382b31ae0c78f Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 06:32:42 +0000 Subject: [PATCH 3/8] refactor: simplify the gh group cleanup Keep the existing immediate SIGKILL behavior and only keep the repeat-kill interval alive until the process group is empty; drop the SIGTERM grace state. --- packages/host/src/gh.ts | 42 +++++++++++++---------------------------- test/unit/gh.test.ts | 22 ++------------------- 2 files changed, 15 insertions(+), 49 deletions(-) diff --git a/packages/host/src/gh.ts b/packages/host/src/gh.ts index 33b66a97..c4972d4c 100644 --- a/packages/host/src/gh.ts +++ b/packages/host/src/gh.ts @@ -37,8 +37,6 @@ export interface GhRunOptions { readonly host?: string; readonly ghPath?: string; readonly timeoutMs?: number; - /** How long SIGTERM gets before the process group is SIGKILLed. */ - readonly killGraceMs?: number; readonly maxOutputBytes?: number; /** Environment to copy only the proxy and PATH values from. */ readonly env?: NodeJS.ProcessEnv; @@ -87,7 +85,6 @@ export async function runGhAction(options: GhRunOptions): Promise { } const timeoutMs = options.timeoutMs ?? getGhAction(options.action).timeoutSeconds * 1000; - const killGraceMs = options.killGraceMs ?? 500; try { return await new Promise((resolve) => { @@ -100,9 +97,7 @@ export async function runGhAction(options: GhRunOptions): Promise { let truncated = false; let timedOut = false; let settled = false; - let terminating = false; let timer: NodeJS.Timeout | undefined; - let termTimer: NodeJS.Timeout | undefined; let killTimer: NodeJS.Timeout | undefined; const child = spawn(options.ghPath ?? 'gh', argv, { @@ -124,9 +119,7 @@ export async function runGhAction(options: GhRunOptions): Promise { }; const stopEscalation = (): void => { - if (termTimer !== undefined) clearTimeout(termTimer); if (killTimer !== undefined) clearInterval(killTimer); - termTimer = undefined; killTimer = undefined; }; @@ -136,7 +129,7 @@ export async function runGhAction(options: GhRunOptions): Promise { if (timer !== undefined) clearTimeout(timer); // A closed parent does not prove the process group is gone: a forked gh helper can keep // running after the parent's stdio closes. Keep SIGKILLing until the group has exited. - if (!terminating || !groupAlive()) stopEscalation(); + if (killTimer === undefined || !groupAlive()) stopEscalation(); const fullStdout = stdout + stdoutDecoder.end(); const fullStderr = stderr + stderrDecoder.end(); stdout = clampUtf8(fullStdout, maxBytes); @@ -145,39 +138,30 @@ export async function runGhAction(options: GhRunOptions): Promise { resolve({ ...result, stdout, stderr, truncated, timedOut }); }; - const signalGroup = (signal: NodeJS.Signals): void => { + const signalGroup = (): void => { if (child.pid === undefined) return; try { - // The child is its own process-group leader; signal the group so a gh-spawned helper does + // 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, signal); + process.kill(-child.pid, 'SIGKILL'); } catch { - child.kill(signal); + child.kill('SIGKILL'); } }; const kill = (): void => { - terminating = true; - signalGroup('SIGTERM'); - // Give gh a chance to exit cleanly, then keep SIGKILLing the group until it is gone: a child - // forked between the signal and the process-group scan can otherwise retain the pipe forever. - if (termTimer === undefined && killTimer === undefined) { - termTimer = setTimeout(() => { + signalGroup(); + // Keep SIGKILLing the group after the parent closes; close does not prove a forked helper + // is gone. The interval stops itself once the group is empty. + if (killTimer === undefined && !settled) { + killTimer = setInterval(() => { if (!groupAlive()) { stopEscalation(); return; } - signalGroup('SIGKILL'); - killTimer = setInterval(() => { - if (!groupAlive()) { - stopEscalation(); - return; - } - signalGroup('SIGKILL'); - }, 100); - killTimer.unref(); - }, killGraceMs); - termTimer.unref(); + signalGroup(); + }, 100); + killTimer.unref(); } }; diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 00d1d5bb..16af5bb9 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -54,23 +54,6 @@ describe('runGhAction', { skip: onWindows }, () => { assert.ok(Date.now() - started < 2000); }); - it('sends SIGTERM before SIGKILL', async () => { - const gh = shim('trap "exit 0" TERM; sleep 5'); - const result = await runGhAction({ - action: 'pr:view', - values: { number: 1 }, - repo: 'acme/widget', - token: 't', - ghPath: gh, - timeoutMs: 50, - killGraceMs: 250, - env: { PATH: process.env.PATH }, - }); - assert.equal(result.timedOut, true); - assert.equal(result.code, 0); - assert.equal(result.signal, null); - }); - it('caps output and reports truncation', async () => { const gh = shim('printf "0123456789"'); const result = await runGhAction({ @@ -171,12 +154,12 @@ describe('runGhAction options', { skip: onWindows }, () => { }); describe('runGhAction orphan cleanup', { skip: onWindows }, () => { - it('keeps killing the group after the parent has closed', async () => { + it('kills a forked child after the parent closes', async () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-orphan-')); const pidFile = path.join(dir, 'child.pid'); const gh = shim( [ - '( trap "" TERM; exec >/dev/null 2>&1; sleep 5 ) &', + '( exec >/dev/null 2>&1; sleep 5 ) &', 'echo $! > "$GH_TOKEN"', 'trap "exit 0" TERM', 'sleep 5', @@ -190,7 +173,6 @@ describe('runGhAction orphan cleanup', { skip: onWindows }, () => { token: pidFile, ghPath: gh, timeoutMs: 100, - killGraceMs: 50, env: { PATH: process.env.PATH }, }); assert.equal(result.timedOut, true); From 74b24886224b8ea52bced520819d9c91cca5bf92 Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 06:34:01 +0000 Subject: [PATCH 4/8] refactor: let the gh kill interval stop itself Drop the separate liveness probe; the repeated SIGKILL reports when the group is gone, so the interval clears itself after close. --- packages/host/src/gh.ts | 56 ++++++++++++++++------------------------- test/unit/gh.test.ts | 1 - 2 files changed, 22 insertions(+), 35 deletions(-) diff --git a/packages/host/src/gh.ts b/packages/host/src/gh.ts index c4972d4c..f2ff190b 100644 --- a/packages/host/src/gh.ts +++ b/packages/host/src/gh.ts @@ -108,28 +108,12 @@ export async function runGhAction(options: GhRunOptions): Promise { stdio: ['ignore', 'pipe', 'pipe'], }); - const groupAlive = (): boolean => { - if (child.pid === undefined) return false; - try { - process.kill(-child.pid, 0); - return true; - } catch (error) { - return (error as NodeJS.ErrnoException).code === 'EPERM'; - } - }; - - const stopEscalation = (): void => { - if (killTimer !== undefined) clearInterval(killTimer); - killTimer = undefined; - }; - const finish = (result: Omit): void => { if (settled) return; settled = true; if (timer !== undefined) clearTimeout(timer); - // A closed parent does not prove the process group is gone: a forked gh helper can keep - // running after the parent's stdio closes. Keep SIGKILLing until the group has exited. - if (killTimer === undefined || !groupAlive()) stopEscalation(); + // A closed parent does not prove the process group is gone; the repeat-kill interval below + // keeps going until signalGroup reports that the group no longer exists. const fullStdout = stdout + stdoutDecoder.end(); const fullStderr = stderr + stderrDecoder.end(); stdout = clampUtf8(fullStdout, maxBytes); @@ -138,31 +122,35 @@ 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'); + return true; } catch { - child.kill('SIGKILL'); + try { + child.kill('SIGKILL'); + return true; + } catch { + return false; + } } }; const kill = (): void => { - signalGroup(); - // Keep SIGKILLing the group after the parent closes; close does not prove a forked helper - // is gone. The interval stops itself once the group is empty. - if (killTimer === undefined && !settled) { - killTimer = setInterval(() => { - if (!groupAlive()) { - stopEscalation(); - return; - } - signalGroup(); - }, 100); - killTimer.unref(); - } + // 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 16af5bb9..78ddb6ff 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -161,7 +161,6 @@ describe('runGhAction orphan cleanup', { skip: onWindows }, () => { [ '( exec >/dev/null 2>&1; sleep 5 ) &', 'echo $! > "$GH_TOKEN"', - 'trap "exit 0" TERM', 'sleep 5', ].join('\n'), ); From b5055c7d2db29fcfd435cd56a80d5c960d5141de Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 06:45:19 +0000 Subject: [PATCH 5/8] fix: stop the gh kill interval when the group is gone child.kill returns false once the child has exited; returning it lets the interval clear. Give the orphan test a two-second clock and take the pid from stdout. --- packages/host/src/gh.ts | 11 ++++------- test/unit/gh.test.ts | 42 +++++++++++++++++------------------------ 2 files changed, 21 insertions(+), 32 deletions(-) diff --git a/packages/host/src/gh.ts b/packages/host/src/gh.ts index f2ff190b..ce4d4fc8 100644 --- a/packages/host/src/gh.ts +++ b/packages/host/src/gh.ts @@ -113,7 +113,8 @@ export async function runGhAction(options: GhRunOptions): Promise { settled = true; if (timer !== undefined) clearTimeout(timer); // A closed parent does not prove the process group is gone; the repeat-kill interval below - // keeps going until signalGroup reports that the group no longer exists. + // 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); @@ -130,12 +131,8 @@ export async function runGhAction(options: GhRunOptions): Promise { process.kill(-child.pid, 'SIGKILL'); return true; } catch { - try { - child.kill('SIGKILL'); - return true; - } catch { - return false; - } + // child.kill returns false rather than throwing once the child has gone. + return child.kill('SIGKILL'); } }; diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 78ddb6ff..065638dd 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -155,36 +155,28 @@ describe('runGhAction options', { skip: onWindows }, () => { describe('runGhAction orphan cleanup', { skip: onWindows }, () => { it('kills a forked child after the parent closes', async () => { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-orphan-')); - const pidFile = path.join(dir, 'child.pid'); const gh = shim( [ '( exec >/dev/null 2>&1; sleep 5 ) &', - 'echo $! > "$GH_TOKEN"', + 'echo $!', 'sleep 5', ].join('\n'), ); - try { - const result = await runGhAction({ - action: 'pr:view', - values: { number: 1 }, - repo: 'acme/widget', - token: pidFile, - ghPath: gh, - timeoutMs: 100, - env: { PATH: process.env.PATH }, - }); - assert.equal(result.timedOut, true); - let pid = 0; - for (let i = 0; i < 20 && pid === 0; i += 1) { - await new Promise((resolve) => setTimeout(resolve, 10)); - if (fs.existsSync(pidFile)) pid = Number(fs.readFileSync(pidFile, 'utf8').trim()); - } - assert.ok(pid > 0, 'the orphan wrote its pid'); - await new Promise((resolve) => setTimeout(resolve, 300)); - assert.throws(() => process.kill(pid, 0), /ESRCH/); - } finally { - fs.rmSync(dir, { recursive: true, force: true }); - } + const result = await runGhAction({ + action: 'pr:view', + values: { number: 1 }, + repo: 'acme/widget', + token: 't', + ghPath: gh, + timeoutMs: 2000, + env: { PATH: process.env.PATH }, + }); + assert.equal(result.timedOut, true); + const pid = Number(result.stdout.trim()); + assert.ok(pid > 0, 'the shim reported the forked pid'); + // With immediate SIGKILL, the group signal reaches the fork too; this is a smoke test that + // close does not leave a child behind, not a deterministic reproduction of the fork-vs-scan race. + await new Promise((resolve) => setTimeout(resolve, 300)); + assert.throws(() => process.kill(pid, 0), /ESRCH/); }); }); From 9c07d3800070815ebeeddf666b408c7cc6452a7e Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 07:32:33 +0000 Subject: [PATCH 6/8] test: drop the nondeterministic orphan smoke test It also passes on main and cannot create a fork after the group signal deterministically; the code fix stays, without a flaky pid-disappearance assertion. --- test/unit/gh.test.ts | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 065638dd..2f7fed4a 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -152,31 +152,3 @@ describe('runGhAction options', { skip: onWindows }, () => { } }); }); - -describe('runGhAction orphan cleanup', { skip: onWindows }, () => { - it('kills a forked child after the parent closes', async () => { - const gh = shim( - [ - '( exec >/dev/null 2>&1; sleep 5 ) &', - 'echo $!', - 'sleep 5', - ].join('\n'), - ); - const result = await runGhAction({ - action: 'pr:view', - values: { number: 1 }, - repo: 'acme/widget', - token: 't', - ghPath: gh, - timeoutMs: 2000, - env: { PATH: process.env.PATH }, - }); - assert.equal(result.timedOut, true); - const pid = Number(result.stdout.trim()); - assert.ok(pid > 0, 'the shim reported the forked pid'); - // With immediate SIGKILL, the group signal reaches the fork too; this is a smoke test that - // close does not leave a child behind, not a deterministic reproduction of the fork-vs-scan race. - await new Promise((resolve) => setTimeout(resolve, 300)); - assert.throws(() => process.kill(pid, 0), /ESRCH/); - }); -}); From 0fd1f2a394d9237f8e2db3de40f0930c2341c54c Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 07:43:07 +0000 Subject: [PATCH 7/8] test: cover the gh kill interval stopping Spy on the group kill and assert it stops shortly after the action resolves; the pre-fix interval kept firing. --- test/unit/gh.test.ts | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 2f7fed4a..7257f6c3 100644 --- a/test/unit/gh.test.ts +++ b/test/unit/gh.test.ts @@ -54,6 +54,36 @@ 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 kills: number[] = []; + process.kill = ((pid: number, signal?: string | number) => { + kills.push(Date.now()); + return realKill.call(process, pid, signal); + }) as typeof process.kill; + 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; + } + }); + it('caps output and reports truncation', async () => { const gh = shim('printf "0123456789"'); const result = await runGhAction({ From 93763ca1f2f383b3a3acae01ce7f33942b34a2b9 Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 26 Oct 2026 08:46:32 +0000 Subject: [PATCH 8/8] fix: stop the gh interval when the group is gone Treat ESRCH from the group kill as gone instead of falling back to the parent handle, and spy on both kill paths so the interval test catches a regression. --- packages/host/src/gh.ts | 8 +++++--- test/unit/gh.test.ts | 7 +++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/host/src/gh.ts b/packages/host/src/gh.ts index ce4d4fc8..0c2f637b 100644 --- a/packages/host/src/gh.ts +++ b/packages/host/src/gh.ts @@ -130,9 +130,11 @@ export async function runGhAction(options: GhRunOptions): Promise { // not keep the pipe open after timeout or output overflow. process.kill(-child.pid, 'SIGKILL'); return true; - } catch { - // child.kill returns false rather than throwing once the child has gone. - return child.kill('SIGKILL'); + } 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; } }; diff --git a/test/unit/gh.test.ts b/test/unit/gh.test.ts index 7257f6c3..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'; @@ -57,11 +58,16 @@ describe('runGhAction', { skip: onWindows }, () => { 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', @@ -81,6 +87,7 @@ describe('runGhAction', { skip: onWindows }, () => { ); } finally { process.kill = realKill; + ChildProcess.prototype.kill = realChildKill; } });