From 1e8ede4e1e11da28955c786e31bf4b9f06bf6fb7 Mon Sep 17 00:00:00 2001 From: Justin Todd Date: Fri, 10 Jul 2026 14:03:22 -0500 Subject: [PATCH] fix(adapter-utils): runChildProcess escalates to SIGKILL on liveness, not child.killed (#8598) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Those agents run as child processes spawned by `@paperclipai/adapter-utils`'s `runChildProcess`, which arms a parent-side wall-clock timer at `timeoutSec` to bound a hung run > - At the deadline `runChildProcess` sends SIGTERM, then after a grace window escalates to SIGKILL — the SIGKILL backstop is what turns a wedged child into a dead PID so the scheduler can reclaim and retry it > - On the **direct-child fallback** path (`signalRunningProcess`, used on win32 and whenever process-group signaling is unavailable or throws) the escalation was gated on `!child.killed` > - But Node sets `ChildProcess.killed` to `true` the instant a signal is *successfully sent*, not when the process exits — so once the earlier SIGTERM has been sent, `child.killed` is already `true`, the `!child.killed` guard is `false`, and the SIGKILL escalation never runs > - A child that ignores SIGTERM (e.g. a graceful-shutdown handler wedged on a socket) is therefore never force-killed, outlives its deadline, and for an unattended scheduler sits running forever with no terminal state > - This PR gates the fallback escalation on real liveness (`exitCode === null && signalCode === null`), so SIGKILL fires precisely while the child is still alive > - The benefit is the hard timeout actually guarantees termination (except true uninterruptible D-state) on every platform/configuration, not just where the process-group path is available ## Linked Issues or Issue Description No existing public issue — describing the bug inline, following `.github/ISSUE_TEMPLATE/bug_report.yml`: ### What happened? When `@paperclipai/adapter-utils`'s `runChildProcess` reaches `timeoutSec` and the spawned child ignores SIGTERM, the SIGKILL escalation on the **direct-child fallback** path (`signalRunningProcess`, taken on win32 or whenever `process.kill(-pgid, …)` is unavailable or throws) never fires, so the child outlives its deadline indefinitely. Root cause: the escalation is gated on `!running.child.killed`, and `ChildProcess.killed` reflects only that a signal was *successfully sent* (per the Node docs it "does not indicate that the child process has been terminated"). After the deadline SIGTERM, `child.killed` is already `true`, so `!child.killed` is `false` and the follow-up SIGKILL is suppressed. ### Expected behavior After the grace window, a child that is still alive is force-killed with SIGKILL regardless of whether SIGTERM was already sent — the hard timeout should guarantee termination (except true uninterruptible D-state) on every platform/configuration. ### Steps to reproduce 1. Spawn a child that installs a no-op `SIGTERM` handler and never exits (e.g. `process.on('SIGTERM', () => {}); setInterval(() => {}, 1000)`). 2. Drive it through the direct-child fallback, i.e. `signalRunningProcess({ child, processGroupId: null }, …)` (the path used on win32 / when group signaling is unavailable). 3. Send SIGTERM (the child swallows it; `child.killed` becomes `true`), then send SIGKILL. 4. On the pre-fix `!child.killed` guard the SIGKILL call is a no-op and the PID survives past its deadline. Covered by the new regression test in this PR. ### Paperclip version or commit Reproduces on `master` (the `signalRunningProcess` fallback). Also present in published `@paperclipai/adapter-utils` (e.g. `2026.325.0`), where the same `!child.killed` guard sits on the single direct-child escalation path. _Searched the open PR list for duplicates/related work on `runChildProcess` / `signalRunningProcess` / SIGKILL escalation; found none._ ## What Changed - `packages/adapter-utils/src/server-utils.ts`: in `signalRunningProcess`, replace the direct-child fallback guard `!running.child.killed` with `running.child.exitCode === null && running.child.signalCode === null` (real liveness). The process-group path is unchanged. - `packages/adapter-utils/src/server-utils.ts`: `export` `signalRunningProcess` so the fallback branch can be unit-tested directly. - `packages/adapter-utils/src/server-utils.test.ts`: add a companion regression test (POSIX-only, like the sibling timeout tests) that forces the fallback (`processGroupId: null`) — sends SIGTERM (child swallows it, `child.killed` becomes `true`), asserts the child is still alive, then sends SIGKILL and asserts the PID dies. Also keeps the end-to-end `runChildProcess` SIGTERM-ignoring test. ## Verification ``` npx vitest run packages/adapter-utils/src/server-utils.test.ts # 52 passed npx tsc --noEmit # clean ``` - **Regression proof:** reverting the guard to `!running.child.killed` makes the new fallback test fail (`waitForPidExit` → false; the child survives); the liveness guard makes it pass. This addresses the prior review note that the existing test only exercised the process-group path (which already escalated correctly on POSIX) and never reached the changed branch. ## Risks Low. A one-line guard change scoped to the direct-child fallback; the process-group path is untouched. SIGKILL is only sent when `exitCode`/`signalCode` are both still `null`, i.e. the process is provably alive, so the change cannot signal an already-reaped/recycled PID. New tests are POSIX-only and `skipIf(win32)`, consistent with the sibling timeout tests in this file. ## Model Used Anthropic **Claude Opus 4.8**, driven via the Cursor agent (extended reasoning + tool use, large context). Diff, tests, and the regression proof above were produced and run by the agent; reviewed by a human before pushing. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Andrew Aymeloglu --- .../adapter-utils/src/server-utils.test.ts | 93 +++++++++++++++++++ packages/adapter-utils/src/server-utils.ts | 8 +- 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/packages/adapter-utils/src/server-utils.test.ts b/packages/adapter-utils/src/server-utils.test.ts index c9f3e0db01..7c7071030b 100644 --- a/packages/adapter-utils/src/server-utils.test.ts +++ b/packages/adapter-utils/src/server-utils.test.ts @@ -1,3 +1,4 @@ +import { spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; @@ -16,6 +17,7 @@ import { runningProcesses, runChildProcess, sanitizeSshRemoteEnv, + signalRunningProcess, shapePaperclipWorkspaceEnvForExecution, rewriteWorkspaceCwdEnvVarsForExecution, stringifyPaperclipWakePayload, @@ -472,6 +474,97 @@ describe("runChildProcess", () => { expect(await waitForPidExit(descendantPid!, 2_000)).toBe(true); }); + it.skipIf(process.platform === "win32")( + "force-kills a child that ignores SIGTERM once the grace window elapses", + async () => { + // Residual hang case: a child that installs a SIGTERM handler which + // swallows the signal and keeps running. The timeout sends SIGTERM at + // timeoutSec, then must escalate to SIGKILL graceSec later. If the + // escalation were gated on `child.killed` (which is true the instant + // SIGTERM is *sent*, not when the process exits) the SIGKILL would be + // suppressed and this child would outlive its deadline. + const result = await runChildProcess( + randomUUID(), + process.execPath, + [ + "-e", + [ + "process.on('SIGTERM', () => {});", + "process.stdout.write(String(process.pid));", + "setInterval(() => {}, 1000);", + ].join(" "), + ], + { + cwd: process.cwd(), + env: {}, + timeoutSec: 1, + graceSec: 1, + onLog: async () => {}, + onSpawn: async () => {}, + }, + ); + + const childPid = Number.parseInt(result.stdout.trim(), 10); + expect(result.timedOut).toBe(true); + expect(result.signal).toBe("SIGKILL"); + expect(Number.isInteger(childPid) && childPid > 0).toBe(true); + expect(await waitForPidExit(childPid, 2_000)).toBe(true); + }, + ); + + it.skipIf(process.platform === "win32")( + "signalRunningProcess escalates SIGKILL on the direct-child fallback after SIGTERM is sent", + async () => { + // Directly cover the branch this PR changed: the direct-child fallback + // (processGroupId === null), which runChildProcess's POSIX timeout tests + // never reach because they always spawn detached and take the + // process-group path. This reproduces the exact regression: once SIGTERM + // has been *sent*, `child.killed` is already true, so the old + // `!child.killed` guard would suppress the SIGKILL escalation and leave a + // SIGTERM-ignoring child alive. The liveness guard + // (exitCode === null && signalCode === null) must still let SIGKILL through. + const child = spawn( + process.execPath, + [ + "-e", + [ + "process.on('SIGTERM', () => {});", + "process.stdout.write(String(process.pid));", + "setInterval(() => {}, 1000);", + ].join(" "), + ], + { detached: false, stdio: ["ignore", "pipe", "ignore"] }, + ); + try { + const pid = await new Promise((resolvePid, rejectPid) => { + child.stdout!.on("data", (d) => resolvePid(Number.parseInt(String(d).trim(), 10))); + child.on("error", rejectPid); + }); + expect(Number.isInteger(pid) && pid > 0).toBe(true); + + // First SIGTERM via the fallback (no process group). The child swallows + // it and stays alive — but child.killed is now true. + signalRunningProcess({ child, processGroupId: null }, "SIGTERM"); + await new Promise((r) => setTimeout(r, 300)); + expect(child.killed).toBe(true); // signal was sent… + expect(isPidAlive(pid)).toBe(true); // …but the process ignored it and lives + + // Escalation: with the old `!child.killed` guard this would be a no-op + // and the child would survive. The liveness guard must still fire. + signalRunningProcess({ child, processGroupId: null }, "SIGKILL"); + expect(await waitForPidExit(pid, 2_000)).toBe(true); + } finally { + if (child.exitCode === null && child.signalCode === null) { + try { + child.kill("SIGKILL"); + } catch { + /* already gone */ + } + } + } + }, + ); + it.skipIf(process.platform === "win32")("cleans up a lingering process group after terminal output and child exit", async () => { const result = await runChildProcess( randomUUID(), diff --git a/packages/adapter-utils/src/server-utils.ts b/packages/adapter-utils/src/server-utils.ts index 805a40e16f..5a4c1e04cf 100644 --- a/packages/adapter-utils/src/server-utils.ts +++ b/packages/adapter-utils/src/server-utils.ts @@ -73,7 +73,8 @@ function resolveProcessGroupId(child: ChildProcess) { return typeof child.pid === "number" && child.pid > 0 ? child.pid : null; } -function signalRunningProcess( +// Exported so the direct-child fallback branch can be unit-tested directly. +export function signalRunningProcess( running: Pick, signal: NodeJS.Signals, ) { @@ -85,7 +86,10 @@ function signalRunningProcess( // Fall back to the direct child signal if group signaling fails. } } - if (!running.child.killed) { + // Gate on real liveness: `child.killed` only means a signal was sent, not that + // the process exited, so escalating on it would suppress a follow-up SIGKILL. + // `exitCode`/`signalCode` are null until the child actually closes. + if (running.child.exitCode === null && running.child.signalCode === null) { running.child.kill(signal); } }