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
dtzp555-maxGitHubtaodengClaude <claude-opus-4-8> <noreply@anthropic.com>
a32bc9af17 fix(prompt): OCP_LOCAL_TOOLS wrapper no longer hard-codes a tool list (closes #185) (#191)
The positive local-tools wrapper (server.mjs) claimed the model has "full access
to the local filesystem, working directory, and shell through your available
tools (Bash, Read, Write, Edit, Glob, Grep, etc.)". That over-promises when an
operator narrows CLAUDE_ALLOWED_TOOLS (e.g. to Read,Grep): the model is told it
has Bash/Write/shell it doesn't hold, then attempts a non-granted tool and gets
denied. The default set matches the text, so the primary/OpenClaw path was fine —
but the enumeration was a prompt-honesty mismatch (flagged in the #182 fact-check).

Reworded to flip the POSTURE only ("you may use your available local tools … do
not assume access beyond the tool set provided to you in this session") without
enumerating specific tools or asserting shell/write capability. The model learns
its real tools from the tool definitions claude is given (--allowedTools), so the
enumeration was redundant as well as fragile — now honest under any allowed set.

No const reordering / no CONFIG_EPOCH change (still a constant; the epoch-fold
test confirms toggling still invalidates the cache with the new text). Test
markers updated (selectPromptWrapper unit + the boot-server integration assertion
that the positive wrapper reaches the -p spawn). Suite 449/0.

Rule 2: OCP-owned prompt composition, no wire change, no cli.js citation.

Closes #185

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude <claude-opus-4-8> <noreply@anthropic.com>
2026-07-23 17:38:41 +10:00
+7 -1
View File
@@ -1371,7 +1371,6 @@ function spawnClaudeProcess(model, messages, conversationId, keyName, releaseSlo
promptChars = stdinPayload.length; promptChars = stdinPayload.length;
} }
stats.activeRequests++;
stats.totalRequests++; stats.totalRequests++;
stats.oneOffRequests++; stats.oneOffRequests++;
if (conversationId) { if (conversationId) {
@@ -1414,6 +1413,13 @@ function spawnClaudeProcess(model, messages, conversationId, keyName, releaseSlo
const proc = spawn(CLAUDE, cliArgs, spawnOpts); const proc = spawn(CLAUDE, cliArgs, spawnOpts);
activeProcesses.add(proc); 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(); const t0 = Date.now();
let gotFirstByte = false; let gotFirstByte = false;