mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-27 07:55:07 +00:00
fix(review): correct three wrong claims in this PR, and add the regression test
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 <hermes-agent@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8
This commit is contained in:
co-authored by
Hermes Agent
Claude Opus 5
parent
d9780e0460
commit
718f1f4196
+9
-5
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user