From 17e85a8745bc113577c458dd11cf711c1bab06a5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 22:57:30 -0400 Subject: [PATCH] fix: address Copilot review round 4 (#2339) Handle SIGQUIT (Ctrl-\) on the same tree-shutdown path as SIGINT, SIGTERM and SIGHUP. The gate runs in its own process group, so a signal the wrapper does not handle ends the wrapper alone and orphans the gate behind a lease that then goes stale under it. The signal test is parametrized over SIGTERM (143) and SIGQUIT (131). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- scripts/gate-lease.mjs | 6 +- scripts/gate-lease.test.mjs | 111 ++++++++++++++++++++---------------- 2 files changed, 66 insertions(+), 51 deletions(-) diff --git a/scripts/gate-lease.mjs b/scripts/gate-lease.mjs index d4eea50c..19612857 100644 --- a/scripts/gate-lease.mjs +++ b/scripts/gate-lease.mjs @@ -468,7 +468,11 @@ export async function runUnderLease({ ); }; - const signals = ["SIGINT", "SIGTERM", "SIGHUP"]; + // Every signal a terminal or a supervisor sends to end a run: Ctrl-C, a + // plain kill, a closed terminal, and Ctrl-\ — the child is in its own + // group, so any of these left unhandled would end this process and + // orphan the gate behind a lease that then goes stale under it. + const signals = ["SIGINT", "SIGTERM", "SIGHUP", "SIGQUIT"]; let stoppedBy = null; let escalation = null; let handlers = []; diff --git a/scripts/gate-lease.test.mjs b/scripts/gate-lease.test.mjs index 494c37e3..fe012d3c 100644 --- a/scripts/gate-lease.test.mjs +++ b/scripts/gate-lease.test.mjs @@ -397,54 +397,65 @@ test("main: no command is a usage error", async () => { assert.match(lines.join("\n"), /usage:/); }); -test("a signal to the wrapper stops the whole tree, releases the lease and exits 128+signal", async () => { - const dir = freshDir(); - const pidFile = join(dir, "grandchild.pid"); - // The gate stand-in: starts a descendant of its own (as `npm run` does, - // four levels deep), records both pids, then idles forever unless - // signalled. The descendant is what proves the whole *tree* is stopped — - // a regression to `child.kill()` or a non-detached spawn would leave it - // running and fail below. - const idle = - `const d = require("child_process").spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });` + - `require("fs").writeFileSync(${JSON.stringify(pidFile)}, process.pid + " " + d.pid);` + - `setInterval(() => {}, 1000)`; - const wrapper = spawn( - process.execPath, - [SCRIPT, process.execPath, "-e", idle], - { - env: { ...process.env, [DIR_ENV]: dir }, - stdio: ["ignore", "ignore", "pipe"], - }, - ); - let stderr = ""; - wrapper.stderr.on("data", (chunk) => (stderr += chunk)); - const exited = new Promise((resolve) => - wrapper.once("exit", (code, signal) => resolve({ code, signal })), - ); +// Ctrl-C and a plain kill are the obvious ones; Ctrl-\ (SIGQUIT) is the one +// that was missed (Copilot), and it matters for the same reason: the child +// is in its own process group, so a signal the wrapper does not handle ends +// the wrapper alone and orphans the gate. +for (const [signal, code] of [ + ["SIGTERM", 143], + ["SIGQUIT", 131], +]) { + test(`${signal} to the wrapper stops the whole tree, releases the lease and exits ${code}`, async () => { + const dir = freshDir(); + const pidFile = join(dir, "grandchild.pid"); + // The gate stand-in: starts a descendant of its own (as `npm run` does, + // four levels deep), records both pids, then idles forever unless + // signalled. The descendant is what proves the whole *tree* is stopped — + // a regression to `child.kill()` or a non-detached spawn would leave it + // running and fail below. + const idle = + `const d = require("child_process").spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });` + + `require("fs").writeFileSync(${JSON.stringify(pidFile)}, process.pid + " " + d.pid);` + + `setInterval(() => {}, 1000)`; + const wrapper = spawn( + process.execPath, + [SCRIPT, process.execPath, "-e", idle], + { + env: { ...process.env, [DIR_ENV]: dir }, + stdio: ["ignore", "ignore", "pipe"], + }, + ); + let stderr = ""; + wrapper.stderr.on("data", (chunk) => (stderr += chunk)); + const exited = new Promise((resolve) => + wrapper.once("exit", (code, signal) => resolve({ code, signal })), + ); - let tree = []; - try { - await waitFor(() => existsSync(pidFile)); - tree = readFileSync(pidFile, "utf8").split(" ").map(Number); - assert.equal(tree.length, 2); - for (const pid of tree) assert.ok(isAlive(pid), `pid ${pid} started`); - assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); - } finally { - // On every path — a failed setup assertion included — the wrapper is - // told to stop, so an idle grandchild can never outlive the suite. - wrapper.kill("SIGTERM"); - } - const outcome = await exited; - assert.deepEqual(outcome, { code: 143, signal: null }, stderr); - for (const pid of tree) - await waitFor(() => !isAlive(pid)).catch(() => { - process.kill(pid, "SIGKILL"); - assert.fail(`pid ${pid} outlived the wrapper — the tree was not stopped`); - }); - assert.ok( - !existsSync(lockPathOf(dir)), - "the lease was released on the way out", - ); - assert.match(stderr, /received SIGTERM; stopping the gate/); -}); + let tree = []; + try { + await waitFor(() => existsSync(pidFile)); + tree = readFileSync(pidFile, "utf8").split(" ").map(Number); + assert.equal(tree.length, 2); + for (const pid of tree) assert.ok(isAlive(pid), `pid ${pid} started`); + assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); + } finally { + // On every path — a failed setup assertion included — the wrapper is + // told to stop, so an idle grandchild can never outlive the suite. + wrapper.kill(signal); + } + const outcome = await exited; + assert.deepEqual(outcome, { code, signal: null }, stderr); + for (const pid of tree) + await waitFor(() => !isAlive(pid)).catch(() => { + process.kill(pid, "SIGKILL"); + assert.fail( + `pid ${pid} outlived the wrapper — the tree was not stopped`, + ); + }); + assert.ok( + !existsSync(lockPathOf(dir)), + "the lease was released on the way out", + ); + assert.match(stderr, new RegExp(`received ${signal}; stopping the gate`)); + }); +}