From 718f1f41968cafa1b3d38b2c94a3fefe7bf3d3d6 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Mon, 27 Jul 2026 07:10:35 +1000 Subject: [PATCH] fix(review): correct three wrong claims in this PR, and add the regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diff was right; three things I wrote about it were not. An independent reviewer falsified each one, and leaving a correct fix on an incorrect rationale in git history is exactly what ALIGNMENT.md governance exists to prevent. Corrections: 1. "No regression test is possible here" — WRONG, and the test is now here. Two compounding errors. `test-features.mjs:955` already has `ltBoot`, a real-server integration fixture (spawns server.mjs, fake claude binary, HTTP) that I had used myself in #191; I reasoned from "cannot import server.mjs" straight to "cannot test" without grepping for it. And the synchronous throw is reachable from ENV ALONE, needing no production hook: buildCliArgs does `args.push("--allowedTools", ...ALLOWED_TOOLS)`, and a spread of enough elements throws RangeError synchronously, inside the leak window. Measured, zero production changes: origin/main : 3 requests -> 500 x3 -> stats.activeRequests == 3 this branch : 3 requests -> 500 x3 -> stats.activeRequests == 0 The committed test DISCOVERS the element count rather than hard-coding it (the threshold is stack-size dependent, so a fixed number would silently stop triggering on another machine and pass vacuously), and asserts the requests actually 500 before trusting the counter. Mutation-proven: moving the increment back before the spawn fails it with "got 3". 2. My reason for rejecting #180's approach was WRONG. I claimed reconciling to activeProcesses.size is racy because a request that incremented but has not yet registered is invisible in the Set. There is no `await` anywhere in that window — it is fully synchronous, so on Node's single thread that state is unobservable and the race cannot occur. The real reason reconciliation is unsafe: the decrement is wired to 'exit' while activeProcesses.delete is wired to 'close', and exit precedes close (measured exit @2ms, close @2017ms with a grandchild holding stdout), so activeProcesses.size OVER-reports during that window — the opposite direction from what I wrote. Separately, a throw after activeProcesses.add leaves permanent Set residue that reconciliation would inherit. Corrected publicly on #180; the contributor was owed an accurate rejection. 3. The alignment citation named the wrong authority. ALIGNMENT.md:17 scopes Rules 1-5 to Class A only, and Rule 2 is a prohibition, not an authorization — citing it as justification is a category error. Every reader of stats.activeRequests is a grandfathered Class B.2 endpoint (/health, /status, /usage). Correct citation, per ALIGNMENT.md:142 and the in-repo precedent at server.mjs:3271-3276 (the F6 spawn.isolated fix): Authorized by ADR 0006 (grandfathered as of v3.16.4). The field SET is unchanged — no field added, removed or renamed — only the VALUE is made truthful. Behaviour-preserving; no new ADR required. The "cli.js: no hit, declared" part stands: cli.js has no equivalent operation, and spawn argv and protocol are byte-identical. Also corrected the new comment at the increment: only 'exit' is wired inside spawnClaudeProcess; 'close'/'error' are wired by each caller, and that is REQUIRED because a failed spawn never emits 'exit'. Note for a follow-up issue, not folded in (Iron Rule 11): stats.errors stays 0 across all of these 500s — synchronous throws from spawnClaudeProcess are never counted. And CLAUDE.md hard-requirement #1 literally says "justify scope under Rule 2", which is what I followed; CLAUDE.md predates ADR 0006's Class B section and needs its own governance PR. Tests: 450 passed, 0 failed. Co-Authored-By: Hermes Agent Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8 --- server.mjs | 14 +++++++----- test-features.mjs | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/server.mjs b/server.mjs index 1871727..3950e3b 100644 --- a/server.mjs +++ b/server.mjs @@ -1414,11 +1414,15 @@ function spawnClaudeProcess(model, messages, conversationId, keyName, releaseSlo const proc = spawn(CLAUDE, cliArgs, spawnOpts); activeProcesses.add(proc); // Counter drift (#180, reported by @konceptnet): increment ONLY after the spawn has - // succeeded and the process is registered. cleanup() below is the SOLE decrement site and is - // wired to this proc's exit/close/error events, so increment and decrement are now - // structurally paired to one process lifecycle. Incrementing before the spawn (as this did) - // leaked +1 permanently on any synchronous throw in between — buildCliArgs, env assembly, the - // spawn decision, or spawn() itself — because cleanup() was not yet attached to anything. + // succeeded and the process is registered. Incrementing before the spawn (as this did) leaked + // +1 permanently on any synchronous throw in between — buildCliArgs, env assembly, the spawn + // decision, or spawn() itself — because nothing was yet attached that could undo it. + // + // cleanup() is the SOLE decrement site, but note how it is reached: only 'exit' is wired HERE + // (below); 'close' and 'error' are wired by each CALLER (callClaude / callClaudeStreaming). + // That caller wiring is REQUIRED, not belt-and-braces — a FAILED spawn emits 'error' and + // 'close' but never 'exit', so without it a spawn failure would never decrement. A future + // third caller of spawnClaudeProcess must wire them too. stats.activeRequests++; const t0 = Date.now(); diff --git a/test-features.mjs b/test-features.mjs index 5896ee0..f455766 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -1104,6 +1104,60 @@ test("integration: toggling OCP_LOCAL_TOOLS invalidates the standard response ca } finally { _ltRm(dir, { recursive: true, force: true }); } }); +// ── active-request counter is paired to the process lifecycle (#180 / #193) ── +// The counter used to be incremented ~40 lines before the spawn, while its only decrement +// (cleanup()) is wired to that proc's events — so any SYNCHRONOUS throw in between leaked +1 +// permanently. Driving that fault needs no production hook and no test double: buildCliArgs +// does `args.push("--allowedTools", ...ALLOWED_TOOLS)`, and a spread of enough elements throws +// RangeError synchronously, right inside the window. +// +// The element count is DISCOVERED, not hard-coded: the throw threshold depends on stack size, +// so a fixed number would silently stop triggering on a machine with a bigger stack and the +// test would pass vacuously. We find a count that throws in THIS process, then use it for the +// child (same binary, same default stack). The test also asserts the requests actually 500 — +// if the trigger ever stops firing, that assert fails loudly instead of going quietly green. +function ltSpreadThrowCount() { + for (const n of [1 << 16, 1 << 17, 1 << 18, 1 << 19]) { + try { const probe = []; probe.push("--allowedTools", ...Array(n).fill("x")); } + catch { return n; } + } + return null; +} +async function ltPostStatus(port, body) { + try { + const r = await fetch(`http://127.0.0.1:${port}/v1/chat/completions`, { + method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), + }); + return r.status; + } catch { return 0; } +} + +console.log("\nactive-request counter pairing (#180 / #193):"); + +test("integration: a synchronous pre-spawn throw must not leak stats.activeRequests", async () => { + if (!LT_POSIX) return; + const n = ltSpreadThrowCount(); + assert.ok(n, "could not find an element count whose spread throws — adjust ltSpreadThrowCount"); + const dir = ltMkdir(); const fake = ltFake(dir); + const { child, buf } = ltBoot({ + CLAUDE_BIN: fake, CLAUDE_PROXY_PORT: "39370", + CLAUDE_ALLOWED_TOOLS: Array(n).fill("x").join(","), + }, dir); + try { + assert.ok(await ltWait(() => buf.out.includes("listening on"), 20000), `did not start: ${buf.err.slice(0, 200)}`); + const req = { model: "haiku", messages: [{ role: "user", content: "leak-probe" }] }; + const codes = []; + for (let i = 0; i < 3; i++) codes.push(await ltPostStatus(39370, req)); + // Non-vacuous: if the spread stopped throwing these would be 200 and the counter would be + // 0 for the wrong reason. Assert the fault actually fired before trusting the counter. + assert.deepEqual(codes, [500, 500, 500], `expected the pre-spawn throw to surface as 500s, got ${codes}`); + const r = await fetch(`http://127.0.0.1:39370/status`); + const active = (await r.json()).requests.active; + assert.equal(active, 0, + `3 requests threw before their spawn; the counter must be back to 0, got ${active} (this is the #180 leak)`); + } finally { child.kill("SIGKILL"); _ltRm(dir, { recursive: true, force: true }); } +}); + // ── Upgrade Tests ── import { runUpgrade, postFlightOk } from "./scripts/upgrade.mjs";