fix(adapter-utils): runChildProcess escalates to SIGKILL on liveness, not child.killed (#8598)
## 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 <aaymeloglu@gmail.com>
This commit is contained in:
Justin Todd
GitHub
parent
1fe89eb8f8
commit
1e8ede4e1e
|
|
@ -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<number>((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(),
|
||||
|
|
|
|||
|
|
@ -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<RunningProcess, "child" | "processGroupId">,
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue