From d9780e0460c5079e67daa810be14127ae0581881 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Sun, 26 Jul 2026 22:05:42 +1000 Subject: [PATCH] fix(server): pair the active-request counter with the process lifecycle (#180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stats.activeRequests` was incremented ~40 lines before the child process was spawned, but its only decrement site — cleanup() — is wired to that process's exit/close/error events. Any synchronous throw in the window between them (buildCliArgs, env assembly, applying the spawn decision, or spawn() itself) therefore leaked +1 permanently: the increment had happened, and cleanup() was not yet attached to anything that could undo it. /health and /stats then over-report in-flight work forever, and the drift is monotonic. Fix: move the increment to immediately after `activeProcesses.add(proc)`, so increment and decrement are structurally paired to exactly one process lifecycle. No try/catch and no reconciliation pass is needed — there is no longer a window in which the counter is incremented but unowned. `stats.activeRequests` is observability-only, not a gate: the old `if (activeRequests >= MAX_CONCURRENT) throw` admission check was removed in favour of the releaseSlot semaphore (see the comment at server.mjs:1327), and the only remaining readers are /health, /stats and the settings snapshot. So deferring the increment past the spawn changes no admission decision. Reproduced and verified on the real server (no test double). A deliberate throw injected between the increment and the spawn, then three requests: before the fix : 3 requests -> http 500 -> stats.activeRequests == 3 (leaked) after the fix : 3 requests -> http 500 -> stats.activeRequests == 0 happy path : real completion "OK", activeRequests 0, total 1, errors 0 The fault injection was removed before committing (no residue: `grep -rn OCP_FAULT_INJECT` is clean) — it is documented in the PR so any reviewer can re-run the same three commands. cli.js citation: NOT APPLICABLE — this touches no cli.js-derived wire behavior. Scope is justified under ALIGNMENT.md Rule 2 as OCP-owned internal bookkeeping: the counter is OCP's own observability state, cli.js has no equivalent operation, and the spawn arguments and protocol are untouched. Supersedes the approach in #180, which additionally reconciled the counter to `activeProcesses.size`. That reconciliation is racy: a concurrent request that has incremented but not yet registered its process is invisible in `activeProcesses`, so reconciling stomps it back down and under-reports live work. Pairing the increment to the spawn removes the drift at its source, so no reconciliation is required. Original report and diagnosis by @konceptnet. Tests: 449 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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server.mjs b/server.mjs index 2025862..1871727 100644 --- a/server.mjs +++ b/server.mjs @@ -1371,7 +1371,6 @@ function spawnClaudeProcess(model, messages, conversationId, keyName, releaseSlo promptChars = stdinPayload.length; } - stats.activeRequests++; stats.totalRequests++; stats.oneOffRequests++; if (conversationId) { @@ -1414,6 +1413,13 @@ 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. + stats.activeRequests++; const t0 = Date.now(); let gotFirstByte = false;