test(conversation): fake-pi tool child signals ready after its TERM handler (row 46, #1528)
Fixes the K1, K3 and K10 race: a child could get SIGTERM before it installed its handler. Author: Darkwing. Reviewer: Dewey (approve, comment 26873). BUILD-LOG landing entry with gate counts and the correction to decision 72's probe claim. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
@@ -3683,3 +3683,17 @@ The reference check runs again just before each removal. A network that gains a
|
||||
### 2026-10-09 — Sage, Docker network removal result
|
||||
|
||||
After: the reference check ran again before each removal, and all 17 networks listed above were removed. None was skipped. That leaves 15 networks: 13 bridge, plus `host` and `none`. All 55 containers are still there. No volume and no `daemon.json` was touched. A probe network got 172.23.0.0/16 and was removed right away, so new compose projects can get a subnet again. `COMPOSE_PROJECT_NAME=gate2` is no longer needed for test-release and test-task. Receipts are `removal.txt`, `networks.txt` and `containers.txt` in `~/sage-scratch/netclean/`.
|
||||
|
||||
### 2026-10-09 — Sage, row 46 landed: cohort K1, K3 and K10 fixture race fixed (#1528, lead decision 72)
|
||||
|
||||
Before: K1, K3 and K10 in `packages/conversation/tests/cohort.test.mjs` failed on this host at the row 38 and row 39 gates, and in isolation on the canonical tree.
|
||||
|
||||
After: Darkwing found a race in `spawnChild` in `packages/conversation/tests/fake-pi.mjs`. A tool child could receive SIGTERM before it installed its handler. It then died by the default action, so the three cases measured how fast Node starts and not the cgroup kill. TMPDIR on `/mnt/storage/scratch/tmp` is fast enough to expose it. The child now writes a ready byte after installing the handler, and the `child` op resolves only on that byte. A 10 s timeout, an exit or an error fails it. The ops dispatcher passes a rejection to `failed`. No assertion changed, and `cohort.mjs` is untouched.
|
||||
|
||||
Correction: decision 72 and the brief say an `ignoreTerm` child spawned outside a scope survives SIGTERM, citing my probe. My probe waited 500 ms before the signal, so it never hit the window. The claim holds only for a child that has finished starting, and it pointed the diagnosis at the scope instead of the fixture.
|
||||
|
||||
Review: Dewey approved in round 1 (comment 26873, queue rev 209). Mutants D1, D2 and D4 are killed. D3, the ready byte written before the handler, survives because the tests can't observe the order; the code has it right.
|
||||
|
||||
Gate: a worktree at 7478ee2c with `build.patch` (`04234ce1…629d9`), manifest 1/1 OK. Conversation 152/152, K1, K3 and K10 isolated 3/3 three times, webui 14/14. No package or script changed between 7478ee2c and the landing commit. Receipts are in `~/sage-scratch/r46-out/`.
|
||||
|
||||
Follow-ups, not blocking (Dewey notes a and b): `crashDuringStop` reads `.result.pid` without checking `ok`, and the 10 s timeout leaves the child running. They wait for a later conversation row.
|
||||
|
||||
@@ -615,14 +615,34 @@ const children = [];
|
||||
// K2); `forkLoop` forks every 5 ms (K12); `ignoreTerm` survives SIGTERM, so
|
||||
// only the kill phase ends it (K3, K10, K11). With `pidLog`, the fork loop
|
||||
// appends each child's pid and a `term` line when it gets SIGTERM (K12).
|
||||
//
|
||||
// It resolves only once the child writes its ready byte, which it does after
|
||||
// installing its TERM handler. Node takes 15 to 80 ms to get there, and a
|
||||
// force stop can send TERM sooner (12 ms when the claim store is on a fast
|
||||
// disk). A TERM before the handler kills a child that is meant to ignore it
|
||||
// (#1528).
|
||||
function spawnChild({ setsid = false, forkLoop = false, ignoreTerm = false, pidLog = null } = {}) {
|
||||
const note = pidLog ? `const note=(s)=>require('node:fs').appendFileSync(${JSON.stringify(pidLog)},s+'\\n');` : "const note=()=>{};";
|
||||
const code = note + (ignoreTerm ? "process.on('SIGTERM',()=>note('term'));" : "") + (forkLoop
|
||||
const code = note + (ignoreTerm ? "process.on('SIGTERM',()=>note('term'));" : "") + "process.stdout.write('r');" + (forkLoop
|
||||
? "const {spawn}=require('node:child_process');setInterval(()=>{try{const c=spawn('sleep',['1000'],{stdio:'ignore'});if(c.pid)note(String(c.pid))}catch{}},5);setInterval(()=>{},1e9)"
|
||||
: "setInterval(()=>{},1e9)");
|
||||
const child = spawn(process.execPath, ["-e", code], { stdio: "ignore", detached: setsid });
|
||||
const child = spawn(process.execPath, ["-e", code], { stdio: ["ignore", "pipe", "ignore"], detached: setsid });
|
||||
children.push(child.pid);
|
||||
return child.pid;
|
||||
return new Promise((resolve, reject) => {
|
||||
const fail = (why) => {
|
||||
clearTimeout(timer);
|
||||
reject(new Error(`tool child ${child.pid} ${why} before it was ready`));
|
||||
};
|
||||
const timer = setTimeout(() => fail("took 10 s"), 10000);
|
||||
child.once("error", (err) => fail(err.code ?? err.message));
|
||||
child.once("exit", (code, signal) => fail(`exited (${signal ?? code})`));
|
||||
child.stdout.once("data", () => {
|
||||
clearTimeout(timer);
|
||||
child.removeAllListeners("exit");
|
||||
child.stdout.destroy();
|
||||
resolve(child.pid);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
// K13: a member writes its own pid to another cgroup's cgroup.procs.
|
||||
@@ -671,7 +691,7 @@ async function main() {
|
||||
drop: () => fake.dropResponse(req.type, req.n ?? 1),
|
||||
extension: () => void fake.extensionPrompt(req.args ?? {}),
|
||||
state: () => ({ streaming: fake.streaming, runs: fake.runs.length, commands: fake.commands, pid: process.pid, children, appends: fake.appends }),
|
||||
child: () => ({ pid: spawnChild(req.args ?? {}) }),
|
||||
child: () => spawnChild(req.args ?? {}).then((pid) => ({ pid })),
|
||||
escape: () => escape(req.target),
|
||||
cgroup: () => readFileSync(`/proc/${req.pid ?? process.pid}/cgroup`, "utf8"),
|
||||
waitPaused: () => fake.waitPaused(req.point),
|
||||
@@ -679,12 +699,13 @@ async function main() {
|
||||
stall: () => void process.stdin.pause(),
|
||||
};
|
||||
if (!ops[req.op]) return sock.write(encodeLine({ id: req.id, ok: false, error: `unknown op ${req.op}` }));
|
||||
const failed = (err) => sock.write(encodeLine({ id: req.id, ok: false, error: String(err.message) }));
|
||||
try {
|
||||
const out = ops[req.op]();
|
||||
if (out && typeof out.then === "function") out.then(reply);
|
||||
if (out && typeof out.then === "function") out.then(reply, failed);
|
||||
else reply(out ?? null);
|
||||
} catch (err) {
|
||||
sock.write(encodeLine({ id: req.id, ok: false, error: String(err.message) }));
|
||||
failed(err);
|
||||
}
|
||||
});
|
||||
sock.on("data", (c) => splitter.push(c));
|
||||
|
||||
Reference in New Issue
Block a user