From 8b58cb7e47e166bcfdfd593b239a5a69bce79a89 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 13 Oct 2026 18:01:02 -0500 Subject: [PATCH] cli: release host.lock on a late fork or send throw; retry a vanishing lock (row 106, #1600) Darkwing round 1, approved by Dewey in #1600 comment 28319 on manifest ae38cbe280aa5885aec7648d38143920753db86ea1ca3934d0a8885fb61e5105. Sage gate rerun on f63c965f9 plus build.patch: every package suite and every scripts/test-*.sh green (cli 168, harness 73, test-release 14). readHostState now treats a host.json that vanishes mid-read as no host for stopHost and hostStatus too (Dewey note b); Sage accepts it as correct. Nothing goes live until a re-pin under Mos's go. Co-Authored-By: Claude Opus 5.5 --- packages/cli/README.md | 6 ++- packages/cli/src/host.mjs | 35 +++++++----- packages/cli/tests/host.test.mjs | 66 ++++++++++++++++++++++- packages/harness/tests/git-tools.test.mjs | 23 +++++--- roles/reviewer.md | 3 +- 5 files changed, 109 insertions(+), 24 deletions(-) diff --git a/packages/cli/README.md b/packages/cli/README.md index 5baebde27..3111fe03d 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -181,8 +181,10 @@ mosaic bus status [--json] host state file, socket, writer.lock (pid )`. A lock whose host no longer runs is taken over whatever its age: it is moved aside, checked to be the file that was read, and removed; if another host's claim was moved aside in that race, - it goes back. Each refusal below removes `host.lock` again, so a host - that didn't start leaves no file. + it goes back. Of three starts racing one stale lock, two can both claim + it, and then the broker's `writer.lock` refuses the second broker. Each + refusal below removes `host.lock` again, so a host that didn't start + leaves no file. 4. It forks `packages/bus/src/process.mjs` and waits up to 30 s for the boot reply. If the broker answers `{ok: false, error}`, the host exits 3 and prints `broker refused to start: `. A timeout or an early exit diff --git a/packages/cli/src/host.mjs b/packages/cli/src/host.mjs index 23150d4c1..02b15477d 100644 --- a/packages/cli/src/host.mjs +++ b/packages/cli/src/host.mjs @@ -44,12 +44,16 @@ export function readHostState(dataRoot) { return readState(hostFile(dataRoot)); } -function readState(file) { +// A file that goes between the check and the read counts as no file. +// `found` runs between the two; it is for the tests. +function readState(file, found = () => {}) { if (!existsSync(file)) return null; + found(); let state; try { state = JSON.parse(readFileSync(file, "utf8")); - } catch { + } catch (e) { + if (e.code === "ENOENT") return null; throw new CliError(`bus host state is unreadable: ${file}`, 3); } if (!state || !Number.isSafeInteger(state.pid) || typeof state.startTime !== "string" || typeof state.business !== "string") { @@ -68,9 +72,11 @@ const same = (a, b) => a?.pid === b?.pid && a?.startTime === b?.startTime && a?. // moved aside, checked to be the file that was read, and removed; another // host's claim moved aside by that race goes back. A third host racing the // same stale lock can still lose a claim there, and then the broker's writer -// lock refuses the second broker. `between` runs after the stale lock is -// read; it is for the tests. Exported for the tests. -export function claimHostLock(dataRoot, state, { between = () => {} } = {}) { +// lock refuses the second broker. A lock that goes before it is read is +// tried again, like one that goes before the check. `found` runs once the +// lock is found and before it is read, `between` after the stale lock is +// read; both are for the tests. Exported for the tests. +export function claimHostLock(dataRoot, state, { found = () => {}, between = () => {} } = {}) { mkdirSync(hostDir(dataRoot), { recursive: true, mode: 0o700 }); const file = hostLock(dataRoot); const tag = `${process.pid}.${randomBytes(4).toString("hex")}`; @@ -85,7 +91,7 @@ export function claimHostLock(dataRoot, state, { between = () => {} } = {}) { } catch (e) { if (e.code !== "EEXIST") throw e; } - const prior = readState(file); + const prior = readState(file, found); if (!prior) continue; if (prior.live) throw new CliError(`a bus host already runs for ${prior.business} (pid ${prior.pid})`, 3); between(); @@ -177,15 +183,20 @@ export async function startHost({ boot, business, notifier = null, bootTimeoutMs if (same(held, state)) rmSync(hostLock(dataRoot), { force: true }); }; - const broker = fork(BROKER, [], { stdio: ["ignore", "inherit", "inherit", "ipc"] }); - const reply = firstReply(broker, bootTimeoutMs, "broker"); - broker.send({ op: "boot", config: boot }); + // A fork or a send that throws gives the lock back too. The wait starts + // after the send, so a send that throws leaves no wait behind. The reply + // and the exit event come on a later tick, so the wait misses neither. + let broker = null; let ready; try { - ready = await reply; + broker = fork(BROKER, [], { stdio: ["ignore", "inherit", "inherit", "ipc"] }); + broker.send({ op: "boot", config: boot }); + ready = await firstReply(broker, bootTimeoutMs, "broker"); } catch (e) { - broker.kill("SIGTERM"); - await ended(broker, 5000); + if (broker) { + broker.kill("SIGTERM"); + await ended(broker, 5000); + } release(); throw e; } diff --git a/packages/cli/tests/host.test.mjs b/packages/cli/tests/host.test.mjs index 7dc05a7f9..1b55505c6 100644 --- a/packages/cli/tests/host.test.mjs +++ b/packages/cli/tests/host.test.mjs @@ -4,7 +4,7 @@ import { spawn, spawnSync } from "node:child_process"; import { subscribe, unsubscribe } from "node:diagnostics_channel"; import { createServer } from "node:http"; import { once } from "node:events"; -import { chmodSync, existsSync, readFileSync, readdirSync, statSync, symlinkSync, utimesSync, writeFileSync, mkdirSync } from "node:fs"; +import { chmodSync, existsSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, utimesSync, writeFileSync, mkdirSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { Client } from "../../bus/src/client.mjs"; @@ -108,6 +108,33 @@ function failSends(t, fake) { }); } +// Makes the next fork throw (`where` "fork") or the new child's sends throw +// ("send"), through the `send` that node installs on a new child: a fork +// throws when the install does. Every child it saw is killed after the test, +// the one a throwing fork started included. +function throwOn(t, where) { + const children = []; + const onChild = ({ process: child }) => { + children.push(child); + let send; + Object.defineProperty(child, "send", { + configurable: true, + get: () => send, + set() { + if (where === "fork") throw new Error("fork failed"); + send = () => { + throw new Error("send failed"); + }; + }, + }); + }; + subscribe("child_process", onChild); + t.after(() => { + unsubscribe("child_process", onChild); + for (const child of children) child.kill("SIGKILL"); + }); +} + const epipe = (child) => { child.kill("SIGKILL"); return Object.assign(new Error("write EPIPE"), { code: "EPIPE", errno: -32, syscall: "write" }); @@ -259,6 +286,27 @@ test("a host takes over a host.lock its dead host left, and puts back a live cla assert.deepEqual(left(), ["host.lock"]); }); +// Row 106 (#1600, Dewey a): a lock removed after the check and before the +// read is tried again, within CLAIM_TRIES, and never refused as unreadable. +test("a host.lock that goes between the check and the read is tried again; one that can't be parsed still refuses as unreadable", (t) => { + const dataRoot = tmp(t); + mkdirSync(hostDir(dataRoot), { recursive: true, mode: 0o700 }); + const stale = { pid: 999999999, startTime: "1", business: "acme", startedAt: "2026-10-13T00:00:00.000Z" }; + const mine = { hostVersion: 1, pid: process.pid, startTime: startTimeOf(process.pid), business: "acme", startedAt: "2026-10-13T00:00:01.000Z", notifier: null }; + writeFileSync(hostLock(dataRoot), JSON.stringify(stale), { mode: 0o600 }); + let found = 0; + const gone = () => found++ === 0 && rmSync(hostLock(dataRoot)); + claimHostLock(dataRoot, mine, { found: gone }); + assert.equal(found, 1, "the lock was found once, and gone before the read"); + assert.equal(JSON.parse(readFileSync(hostLock(dataRoot), "utf8")).startedAt, mine.startedAt); + assert.deepEqual(readdirSync(hostDir(dataRoot)), ["host.lock"]); + + writeFileSync(hostLock(dataRoot), "{not json", { mode: 0o600 }); + assert.throws(() => claimHostLock(dataRoot, mine), (e) => e.exitCode === 3 && e.message === `bus host state is unreadable: ${hostLock(dataRoot)}`); + assert.equal(readFileSync(hostLock(dataRoot), "utf8"), "{not json"); + assert.deepEqual(readdirSync(hostDir(dataRoot)), ["host.lock"]); +}); + test("a live host's lock stays, however old the file is", (t) => { const dataRoot = tmp(t); mkdirSync(hostDir(dataRoot), { recursive: true, mode: 0o700 }); @@ -330,6 +378,22 @@ test("a host started before host.lock existed, holding only host.json, still ref assert.deepEqual(readdirSync(hostDir(f.dataRoot)), ["host.json"]); }); +// Row 106 (#1600, Sage N1): a throw after the claim and before the boot +// reply gives host.lock back like a refusal does. +for (const where of ["fork", "send"]) + test(`a broker ${where} that throws leaves no host.lock and no host.json`, async (t) => { + const root = tmp(t); + const f = fixture(root); + makeDeployment(root); + const boot = bootConfig({ system: loadSystem({ env: f.env }), businessId: "acme", env: f.env }); + throwOn(t, where); + const started = startHost({ boot, business: "acme", log: () => {} }); + t.after(async () => (await started.catch(() => null))?.close(0)); + await assert.rejects(started, (e) => e.message === `${where} failed`); + assert.equal(existsSync(hostLock(f.dataRoot)), false); + assert.equal(existsSync(hostFile(f.dataRoot)), false); + }); + test("a broker that misses its boot wait leaves no host.lock and no host.json", async (t) => { const root = tmp(t); const f = fixture(root); diff --git a/packages/harness/tests/git-tools.test.mjs b/packages/harness/tests/git-tools.test.mjs index 2880b76f0..0e3bdffe9 100644 --- a/packages/harness/tests/git-tools.test.mjs +++ b/packages/harness/tests/git-tools.test.mjs @@ -5,7 +5,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; import { createServer } from "node:net"; import { join } from "node:path"; import { fetchReview, git as runnerGit, HAVE_MAX } from "../src/git-tools.mjs"; @@ -82,23 +82,30 @@ test("fetch_review's have lists the working refs before origin's, and no more th // Past HAVE_MAX the order is per clone: each clone's working refs newest // first, one clone after another, so an earlier clone's older refs come // ahead of a later clone's newer ones. The clones' order is the workspace's -// directory order, so the test takes it from the request. +// directory order, so the test reads it first and gives the first-listed +// clone the older refs: one order across clones by date would put the +// other clone first, whatever order the filesystem lists them in (row 106, +// #1600, Dewey b). test("fetch_review's have orders the working refs newest first within each clone, one clone after another", async (t) => { const dir = scratch(t); const workspace = join(dir, "ws"); - // 20 working refs in each clone; beta's are all newer than alpha's. - // Created in a shuffled order, so the name order isn't the date order. + mkdirSync(join(workspace, "alpha"), { recursive: true }); + mkdirSync(join(workspace, "beta")); + const [early, late] = readdirSync(workspace); + // 20 working refs in each clone; the late clone's are all newer than the + // early one's. Created in a shuffled order, so the name order isn't the + // date order. const order = [7, 2, 15, 0, 11, 19, 4, 9, 13, 1, 17, 6, 3, 18, 10, 5, 14, 8, 16, 12]; const refs = (task, base) => Object.fromEntries(order.map((i) => [`refs/remotes/working/work/vikunja-${task}-${i + 1}`, base + i * 60])); - const alpha = clone(join(workspace, "alpha"), refs(1, 1_700_000_000)); - const beta = clone(join(workspace, "beta"), refs(2, 1_750_000_000)); + const older = clone(join(workspace, early), refs(1, 1_700_000_000)); + const newer = clone(join(workspace, late), refs(2, 1_750_000_000)); + assert.deepEqual(readdirSync(workspace), [early, late], "the clones kept their directory order"); const host = await fakeHost(t, dir); await assert.rejects(fetchReview({ path: host.path, cap: "c".repeat(64), workspace, task_ref: "vikunja:1/2", request: "r1", tmp: dir, env: ENV }), (e) => e.code === "request-not-found"); const [req] = host.lines; // Newest first: the ref for i = 19 first, i = 0 last. const newest = (oids, task) => order.slice().sort((x, y) => y - x).map((i) => oids[`refs/remotes/working/work/vikunja-${task}-${i + 1}`]); - const [first, second] = new Set(Object.values(alpha)).has(req.have[0]) ? [newest(alpha, 1), newest(beta, 2)] : [newest(beta, 2), newest(alpha, 1)]; - assert.deepEqual(req.have, [...first, ...second].slice(0, HAVE_MAX)); + assert.deepEqual(req.have, [...newest(older, 1), ...newest(newer, 2)].slice(0, HAVE_MAX)); }); // A git on PATH that runs `body` in place of the real one. diff --git a/roles/reviewer.md b/roles/reviewer.md index 782aa156e..46211299f 100644 --- a/roles/reviewer.md +++ b/roles/reviewer.md @@ -9,7 +9,8 @@ candidates and record a verdict. You don't change the code you review. acceptance criteria. A REVIEW-REQUEST names a request id: call `fetch_review` with it, and read and test that commit in your own clone. Run the suites yourself when the request says they pass. -- A `nothing-to-fetch` refusal means a clone of yours has the commit; see "Git socket" in `packages/cli/README.md`. +- A `nothing-to-fetch` refusal means a clone of yours has the commit; + see "Git socket" in `packages/cli/README.md`. - Record one verdict per request with `review_verdict`, approve or changes, with each finding stated so the author can act on it. The bus takes one verdict per request; a changed mind needs a new request.