Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions packages/host/src/gh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,9 @@ export async function runGhAction(options: GhRunOptions): Promise<GhRunResult> {
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);
Expand All @@ -121,22 +123,33 @@ export async function runGhAction(options: GhRunOptions): Promise<GhRunResult> {
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 => {
Expand Down
37 changes: 37 additions & 0 deletions test/unit/gh.test.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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({
Expand Down
Loading