modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: address Copilot review round 4 (#2339)

Browse source 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-12 22:57:30 -04:00
commit 17e85a8745
2 changed files with 66 additions and 51 deletions
Download patch file Download diff file Expand all files Collapse all files

6
scripts/gate-lease.mjs
View file

@ -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 stoppedBy = null;
let escalation = null; let escalation = null;
let handlers = []; let handlers = [];

111
scripts/gate-lease.test.mjs
View file

@ -397,54 +397,65 @@ test("main: no command is a usage error", async () => {
assert.match(lines.join("\n"), /usage:/); assert.match(lines.join("\n"), /usage:/);
}); });
test("a signal to the wrapper stops the whole tree, releases the lease and exits 128+signal", async () => { // Ctrl-C and a plain kill are the obvious ones; Ctrl-\ (SIGQUIT) is the one
const dir = freshDir(); // that was missed (Copilot), and it matters for the same reason: the child
const pidFile = join(dir, "grandchild.pid"); // is in its own process group, so a signal the wrapper does not handle ends
// The gate stand-in: starts a descendant of its own (as `npm run` does, // the wrapper alone and orphans the gate.
// four levels deep), records both pids, then idles forever unless for (const [signal, code] of [
// signalled. The descendant is what proves the whole *tree* is stopped — ["SIGTERM", 143],
// a regression to `child.kill()` or a non-detached spawn would leave it ["SIGQUIT", 131],
// running and fail below. ]) {
const idle = test(`${signal} to the wrapper stops the whole tree, releases the lease and exits ${code}`, async () => {
`const d = require("child_process").spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });` + const dir = freshDir();
`require("fs").writeFileSync(${JSON.stringify(pidFile)}, process.pid + " " + d.pid);` + const pidFile = join(dir, "grandchild.pid");
`setInterval(() => {}, 1000)`; // The gate stand-in: starts a descendant of its own (as `npm run` does,
const wrapper = spawn( // four levels deep), records both pids, then idles forever unless
process.execPath, // signalled. The descendant is what proves the whole *tree* is stopped —
[SCRIPT, process.execPath, "-e", idle], // a regression to `child.kill()` or a non-detached spawn would leave it
{ // running and fail below.
env: { ...process.env, [DIR_ENV]: dir }, const idle =
stdio: ["ignore", "ignore", "pipe"], `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)`;
let stderr = ""; const wrapper = spawn(
wrapper.stderr.on("data", (chunk) => (stderr += chunk)); process.execPath,
const exited = new Promise((resolve) => [SCRIPT, process.execPath, "-e", idle],
wrapper.once("exit", (code, signal) => resolve({ code, signal })), {
); 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 = []; let tree = [];
try { try {
await waitFor(() => existsSync(pidFile)); await waitFor(() => existsSync(pidFile));
tree = readFileSync(pidFile, "utf8").split(" ").map(Number); tree = readFileSync(pidFile, "utf8").split(" ").map(Number);
assert.equal(tree.length, 2); assert.equal(tree.length, 2);
for (const pid of tree) assert.ok(isAlive(pid), `pid ${pid} started`); for (const pid of tree) assert.ok(isAlive(pid), `pid ${pid} started`);
assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease");
} finally { } finally {
// On every path — a failed setup assertion included — the wrapper is // On every path — a failed setup assertion included — the wrapper is
// told to stop, so an idle grandchild can never outlive the suite. // told to stop, so an idle grandchild can never outlive the suite.
wrapper.kill("SIGTERM"); wrapper.kill(signal);
} }
const outcome = await exited; const outcome = await exited;
assert.deepEqual(outcome, { code: 143, signal: null }, stderr); assert.deepEqual(outcome, { code, signal: null }, stderr);
for (const pid of tree) for (const pid of tree)
await waitFor(() => !isAlive(pid)).catch(() => { await waitFor(() => !isAlive(pid)).catch(() => {
process.kill(pid, "SIGKILL"); process.kill(pid, "SIGKILL");
assert.fail(`pid ${pid} outlived the wrapper — the tree was not stopped`); 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.ok(
); !existsSync(lockPathOf(dir)),
assert.match(stderr, /received SIGTERM; stopping the gate/); "the lease was released on the way out",
}); );
assert.match(stderr, new RegExp(`received ${signal}; stopping the gate`));
});
}