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); } }