fix(pipeline): round 5 — env-var doc typo + tighten SIGKILL regression test

Two findings addressed in one commit:

**Greptile P1: `loopRound` doc typo** — The JSDoc on `CommandStartInput.loopRound` said the value is surfaced as `AO_LOOP_ROUND` but `buildChildEnv` actually sets `AO_PIPELINE_LOOP_ROUND`. A shell script reading `$AO_LOOP_ROUND` to detect retry-vs-first-run would always get undefined and silently break. Doc-only fix; runtime behaviour was already correct.

**Self-review P1: SIGKILL escalation regression test was unfalsifiable** — The test added in round 4 asserted `outcome.errorMessage` contains "cancelled" and waited 3.5s for SIGKILL. But under the bug, `settle()` is still called synchronously inside `cancel()`, so `done` resolves with the same "cancelled" outcome whether or not SIGKILL ever fires. The trapped-SIGTERM shell process would survive as an orphan and the test would still pass green.

Fix:
- Added `pid?: number` to `RunningCommandStage` (optional — undefined for synthetic-fail handles that never spawned a child).
- The regression test now grabs `handle.pid` and after the SIGKILL grace window calls `process.kill(pid, 0)` (existence probe). Asserts the probe throws `ESRCH` (no such process). Under the round-4 bug this would have returned without throwing, failing the test.
- Also added a sanity check that the child is alive before cancel.

All 1373 core tests pass. Lint clean.
This commit is contained in:
Harsh Batheja 2026-05-17 01:17:42 +05:30
parent dbfb1ad25e
commit dc86edb9e2
2 changed files with 42 additions and 19 deletions

View File

@ -455,41 +455,56 @@ posixDescribe("command executor — startStage (POSIX subprocess)", () => {
// Regression for round-4 P1: `settle()` used to call `clearTimers()`,
// which cancelled the SIGKILL escalation timer that `cancel()` had
// just scheduled. A child that traps SIGTERM and keeps running would
// become an orphan because no SIGKILL ever followed. This test sets
// up exactly that scenario: a shell that traps SIGTERM and loops,
// then cancels and verifies the process eventually dies.
// become an orphan because no SIGKILL ever followed.
//
// This test must distinguish the buggy code from the fixed code by
// checking the actual process state, NOT just `handle.done`. Under
// the bug, `done` still resolves immediately (settle() is called
// synchronously inside cancel()) so any assertion on `done` would
// pass either way. We check `process.kill(pid, 0)` instead: it
// sends signal 0 (existence probe) and throws ESRCH iff the PID
// is dead and reaped.
const exec = createCommandExecutor();
const stage = makeCommandStage({
executor: {
kind: "command",
command: "/bin/sh",
// Trap SIGTERM with an empty handler so default kill is suppressed.
// The loop keeps the process alive indefinitely. Only SIGKILL ends it.
// The loop keeps the shell alive indefinitely. Only SIGKILL ends it.
args: ["-c", `trap '' TERM; while :; do sleep 1; done`],
},
});
const handle = await exec.startStage(makeInput({ stage }));
expect(handle.outcome).toBeNull();
const childPid = (handle as unknown as { __testChildPid?: number }).__testChildPid;
void childPid; // unused but documents intent
const childPid = handle.pid;
expect(typeof childPid).toBe("number");
if (typeof childPid !== "number") throw new Error("unreachable");
// Sanity: child is alive right now.
expect(() => process.kill(childPid, 0)).not.toThrow();
await exec.cancelStage(handle);
// SIGTERM has fired but the child traps it. The SIGKILL escalation
// is scheduled at COMMAND_KILL_GRACE_MS (2s). `done` is already
// resolved (cancel settles immediately), so wait long enough for
// SIGKILL to actually land AND for the close event to fire.
await handle.done;
// Give the OS / Node event loop a beat to deliver SIGKILL + reap.
// is scheduled at COMMAND_KILL_GRACE_MS (2s). Wait past it + buffer
// for the OS to deliver the signal and for Node to reap the child.
await new Promise((r) => setTimeout(r, 3_500));
// After SIGKILL + reap, the close event should have fired. We can't
// easily assert "process is dead" without root visibility, but we
// CAN assert that done resolved with the cancelled message, and
// that the test itself completed in bounded time (vitest's 5s
// default would catch infinite hangs). The real signal: the test
// run terminates cleanly — under the bug, the child loop would
// outlive the test process and leak into the CI worker.
// The real assertion: the child PID must no longer exist. Under the
// bug, the shell would still be running its `while :` loop and
// `process.kill(pid, 0)` would return without throwing.
let stillAlive = false;
try {
process.kill(childPid, 0);
stillAlive = true;
} catch (err) {
// ESRCH is the expected "no such process" — anything else is a surprise.
expect((err as NodeJS.ErrnoException).code).toBe("ESRCH");
}
expect(stillAlive).toBe(false);
// And the outcome we already had is the cancelled failure.
const outcome = await handle.done;
expect(outcome.status).toBe("failed");
if (outcome.status !== "failed") throw new Error("unreachable");

View File

@ -66,7 +66,7 @@ export interface CommandStartInput {
* non-PR runs (manual triggers, internal pipelines).
*/
isFromFork?: boolean;
/** Loop counter, surfaced as `AO_LOOP_ROUND` in the child env. */
/** Loop counter, surfaced as `AO_PIPELINE_LOOP_ROUND` in the child env. */
loopRound?: number;
}
@ -85,6 +85,13 @@ export interface RunningCommandStage {
stageRunId: StageRunId;
stageName: string;
startedAt: number;
/**
* PID of the spawned child process, or `undefined` when the handle
* represents a synthetic outcome (fork refusal, kind mismatch, spawn
* failure) and never actually spawned anything. Useful for tests and
* external observers verifying the child was reaped.
*/
pid?: number;
/**
* Set when the child exits, the timeout fires, spawn fails, or refusal
* triggers. `null` while the child is still running.
@ -298,6 +305,7 @@ export function createCommandExecutor(deps: CommandExecutorDeps = {}): CommandSt
stageRunId: input.stageRunId,
stageName: stage.name,
startedAt,
...(child.pid !== undefined ? { pid: child.pid } : {}),
outcome: null,
done,
cancel: () => {