Compare commits

..
Author SHA1 Message Date
d9780e0460 fix(server): pair the active-request counter with the process lifecycle (#180)
`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 <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
2026-07-26 22:05:42 +10:00
+7 -1
View File
@@ -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;