From 2d308abd169e1a119d6492ec5bb89817b934c889 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Fri, 9 Oct 2026 08:30:01 -0500 Subject: [PATCH] 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 --- BUILD-LOG.md | 14 +++++++++++ packages/conversation/tests/fake-pi.mjs | 33 ++++++++++++++++++++----- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/BUILD-LOG.md b/BUILD-LOG.md index 221493f1..a3eecd22 100644 --- a/BUILD-LOG.md +++ b/BUILD-LOG.md @@ -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. diff --git a/packages/conversation/tests/fake-pi.mjs b/packages/conversation/tests/fake-pi.mjs index b3014f86..a78dfebd 100644 --- a/packages/conversation/tests/fake-pi.mjs +++ b/packages/conversation/tests/fake-pi.mjs @@ -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));