mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-26 23:45:08 +00:00
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
This commit is contained in:
co-authored by
Hermes Agent
Claude Opus 5
parent
a32bc9af17
commit
d9780e0460
+7
-1
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user