From dc86edb9e2e04e8c3d3c1a2208fb17adea201914 Mon Sep 17 00:00:00 2001 From: Harsh Batheja Date: Sun, 17 May 2026 01:17:42 +0530 Subject: [PATCH] =?UTF-8?q?fix(pipeline):=20round=205=20=E2=80=94=20env-va?= =?UTF-8?q?r=20doc=20typo=20+=20tighten=20SIGKILL=20regression=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../pipeline-command-executor.test.ts | 51 ++++++++++++------- .../core/src/pipeline/executors/command.ts | 10 +++- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/packages/core/src/__tests__/pipeline-command-executor.test.ts b/packages/core/src/__tests__/pipeline-command-executor.test.ts index c6d7b9409..72a3dade1 100644 --- a/packages/core/src/__tests__/pipeline-command-executor.test.ts +++ b/packages/core/src/__tests__/pipeline-command-executor.test.ts @@ -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"); diff --git a/packages/core/src/pipeline/executors/command.ts b/packages/core/src/pipeline/executors/command.ts index 01c9d017f..1513dbecd 100644 --- a/packages/core/src/pipeline/executors/command.ts +++ b/packages/core/src/pipeline/executors/command.ts @@ -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: () => {