Pre-deploy P0.2, P0.3, P0.4: admission control and a meter - #4
Open
31803smith wants to merge 3 commits into
Open
Pre-deploy P0.2, P0.3, P0.4: admission control and a meter#431803smith wants to merge 3 commits into
31803smith wants to merge 3 commits into
Conversation
The server had five `console.*` statements, four of them lifecycle noise.
Nothing recorded who asked for what, how long it took, what it cost, or why it
failed, and a successful `POST /api/<command>` was completely invisible —
`http.mjs` logged only on throw.
Everything needed was already on the wire and being discarded.
**The main turn.** `claude -p --output-format stream-json` ends each turn with a
`result` line carrying `usage`, `total_cost_usd`, `duration_ms`, `is_error`,
`stop_reason` and `api_error_status`. `fromResult` read `obj.result` — the text
— and dropped the rest.
**The review children, which is the part that mattered.** `runReviewRound` did
`child.stdout.resume()`, a pure drain, and `child.stderr.resume()`, so unlike
the main turn it did not even buffer the place a dying child prints why. Split
by phase across three real boards (weather-badge-10, -11 and -13), review
rounds are **23%, 27% and 39%** of a board's weighted spend. `pre-deploy.md`
guessed "plausibly the majority"; measured, it is less than that and far too
much to omit. A meter reading only the main turn undercounts every board by a
quarter to two fifths.
Stdout is still drained continuously — the deadlock the `resume()` prevented is
real on a ninety-minute craft round — but only the last non-empty line is kept,
which is this repo's own "one JSON line, last line wins" convention and costs
one string instead of a transcript. Stderr is buffered at the same 8192 cap and
for the same reason as the main turn. `spawnClaude` throwing now reports
`spawn_failed`: a review round that never started returned a bare `false`,
which is exactly what a round that ran and changed nothing returns.
**Four counters, not a total.** weather-badge-13, one board end to end:
input_tokens 420
cache_creation_input_tokens 2,248,535
cache_read_input_tokens 30,555,177
output_tokens 241,443
Raw input is a rounding error, cache reads are 99% of the volume, and the four
are priced roughly 1x / 1.25x / 0.1x / 5x — a 50x spread inside one turn, so a
`total_tokens` column cannot be turned back into money by anyone. Logging the
obvious two would have recorded 0.7% of what moved; there is a test that
asserts exactly that against these numbers. `total_cost_usd` is kept as well
and is not a duplicate: it is a price snapshot, and recomputing an old turn at
next quarter's rates gives a number nobody was ever charged.
A missing cost reads `null`, never `0` — "not reported" and "free" are
different claims and only one of them is safe to sum.
**Format.** Space-separated `key=value` on stdout, one line per turn, absent
fields omitted rather than printed as `null` so a short line means little
happened rather than something broke. Errors are collapsed to a single line and
capped; a stack trace here would break every consumer that splits on newlines.
**Command boundary.** Every command logs now, not only the ones that throw,
with the project it was about and how long it ran. Bodies are deliberately not
logged — they carry chat text and pasted images. An expected refusal
(`error.code`) is recorded as `refused` rather than `error`: reading those as
one event is how a broken deploy looks healthy.
Deliberately out of scope: the durable event log and the SSE backlog. The doc
argues they are the same work and architecturally they may be, but that is a
new subsystem and this is a meter.
`spawnTurn` gains `projectId` for the log line. `runReviewRound` and
`runReviewFixLoop` gain an optional `onMeter` callback rather than a changed
return type, so the boolean every caller reads is untouched.
13 new tests, built from a real board's counters. driver.test.mjs 49/49. viewer
13 failing files — the same set as clean upstream/main @ 6040efb, compared with
`comm`. Build passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
**P0.2.** Two `claude --resume <sessionId>` children on one session collide, and the footgun CLAUDE.md inherits — "Session ID already in use" — is exactly that collision. `turnInProgress(projectId, sessionId)` already existed and was already wired into `refuseIfBuilding()`, which protects *board-source writes* while a build owns the file. That is a real guard; it is just not this one. The turn that creates the collision was never gated on anything: `http.mjs` calls `chat.startTurn()` from three commands with no check, and `startTurn()` did not self-guard. Guarded inside `startTurn` rather than at the three call sites, so a fourth entry point cannot be added without it. Refused with `TURN_RUNNING` / 409, the same shape `refuseIfBuilding` already returns for the write path. The lock is per project, not a global mutex — a test pins that a second project still starts, because turning a collision guard into a one-user-at-a-time server would be a worse bug than the one being fixed. **P0.3.** `turns` was an unbounded in-memory Map in one process. Ten people pressing build is ten CLI children and ten routers on one box, and the multiplier is worse than it reads: autopilot chains a build turn after every plan turn, and a build turn spawns up to seven review children of its own, each rebuilding the board. `DEFAULT_MAX_TURNS = 4`, overridable with `CIRCUIT_MAX_TURNS`. Four is not a tuned number and the comment says so. The measured input is that one board is 43-147 minutes of near-constant CPU on this hardware, so concurrency here is not a throughput knob, it is a blast radius. Refused with `TOO_MANY_TURNS` / 503 — the request is fine and the server is full, which is what 503 means. The cap is checked at **admission only**. Work already admitted runs to completion: an autopilot build chains through `runTurn` directly and a review round is inside a turn that was already counted. Killing either halfway leaves a half-built board, which is worse than a slow one. Both errors are thrown as plain `Error`s carrying `code` and `statusCode` rather than importing the HTTP layer into the driver — `sendIpcError` reads those off any thrown value, and the command-boundary log added in this branch already classifies a coded refusal as `refused` rather than `error`. driver.test.mjs 49 -> 53. viewer 13 failing files, the same set as clean upstream/main @ 6040efb. Build passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The turn log printed a `user=` field that nothing ever set, and a column that is introduced at the same moment as the thing that populates it is a column nobody has ever read. `CIRCUIT_USER_ID` is one hardcoded identity, `"local"` by default, on both the turn line and the command-boundary line. Deliberately NOT the rest of P0.1. `pre-deploy.md` asks for per-user project directories so one account cannot read another's boards; that is a migration, not a constant. Changing the projects root also changes the encoded Claude session directory — `encodeCwd` maps every non-alphanumeric character to `-`, so `…-projects-<uuid>` becomes `…-users-local-projects-<uuid>` — and every existing board would keep its artifacts, lose its chat history, and hand `--resume` a session id it can no longer find, which is the "Session ID already in use" path CLAUDE.md warns about. Verified by resolving `sessionJsonlPath` for both layouts before writing any of it. This repo is R&D for now, so the partition waits for the migration that has to come with it. Sanitised anyway. Nothing can set this to anything odd today, but the day it becomes request-derived is the day a `..` in it reads someone else's boards. The first attempt was too weak — `../../etc` came out as `-..-etc`, still carrying a dot-segment — so `..` is now collapsed anywhere it appears rather than only at the front, and the test asserts the property (no `..`, no separator, never empty) over a list of hostile inputs instead of pinning one output string. usage.test.mjs 13 -> 16. driver.test.mjs 53/53. viewer 13 failing files, the same set as clean upstream/main @ 6040efb. Build passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three P0 items from
docs/pre-deploy.md, in the two server files they alltouch. Each is small; together they are the difference between deploying blind
and deploying.
P0.4 · Metering
The server had five
console.*statements, four of them lifecycle noise.Nothing recorded who asked for what, how long it took, what it cost, or why it
failed, and a successful
POST /api/<command>was invisible —http.mjslogged only on throw.
Everything needed was already on the wire and being thrown away.
Measured first, and it corrects the doc
pre-deploy.mdsays review is "plausibly the majority" of spend. Split byphase across three real boards, using each session's own per-message
usage:Less than the majority, and far too much to omit.
runReviewRounddidchild.stdout.resume()— a pure drain — so a meter reading only the main turnundercounts every board by a quarter to two fifths.
Four counters, not a total
weather-badge-13, one board end to end:
Raw input is a rounding error and cache reads are 99% of the volume. The four
are priced roughly 1× / 1.25× / 0.1× / 5× against base input — a 50× spread
inside one turn — so a
total_tokenscolumn cannot be multiplied back intomoney by anyone. Logging the obvious two would have captured 0.7% of what
moved; a test asserts exactly that against these numbers.
total_cost_usdis kept as well and is not a duplicate: it is a pricesnapshot, and recomputing an old turn at next quarter's rates gives a figure
nobody was ever charged. A missing cost reads
null, never0— "notreported" and "free" are different claims and only one is safe to sum.
What changed
usage.mjs(new, pure, 13 tests) — reads aresultline, folds records,formats one line.
Main turn —
fromResultreadobj.result, the text, and droppedusage,total_cost_usd,duration_ms,is_error,stop_reason,api_error_status.Review children — stdout is still drained continuously (the deadlock
resume()prevented is real on a ninety-minute craft round) but only the lastnon-empty line is kept: this repo's own "one JSON line, last line wins", at the
cost of one string rather than a transcript. stderr is now buffered at the same
8192 cap as the main turn.
spawnClaudethrowing reportsspawn_failed— areview round that never started returned a bare
false, exactly what a roundthat ran and changed nothing returns.
Command boundary — every command logs, with the project and elapsed time.
Bodies are deliberately not logged; they carry chat text and pasted images. An
expected refusal (
error.code) isrefused, noterror: reading those as oneevent is how a broken deploy looks healthy.
Deliberately not included: the durable event log and the SSE backlog. The
doc argues they are the same work and architecturally they may be, but that is
a new subsystem and this is a meter.
P0.2 · Two turns can run on the same project
Two
claude --resume <sessionId>children on one session collide, and thefootgun CLAUDE.md inherits — "Session ID already in use" — is that collision.
turnInProgress(projectId, sessionId)already existed and was already wiredinto
refuseIfBuilding(), which protects board-source writes while a buildowns the file. That is a real guard; it is just not this one. The turn that
creates the collision was gated on nothing:
http.mjscallschat.startTurn()from three commands with no check, andstartTurn()did notself-guard.
Guarded inside
startTurnrather than at the three call sites, so a fourthentry point cannot be added without it.
TURN_RUNNING/ 409, the same shaperefuseIfBuildingalready returns.The lock is per project, not a global mutex — a test pins that a second
project still starts, because turning a collision guard into a
one-user-at-a-time server would be a worse bug than the one being fixed.
P0.3 · No concurrency cap
turnswas an unbounded in-memory Map in one process. Ten people pressingbuild is ten CLI children and ten routers on one box, and the multiplier is
worse than it reads: autopilot chains a build turn after every plan turn, and a
build turn spawns up to seven review children of its own, each rebuilding the
board.
DEFAULT_MAX_TURNS = 4, overridable withCIRCUIT_MAX_TURNS. Four is not atuned number and the comment says so. The measured input is that one board is
43–147 minutes of near-constant CPU on this hardware, so concurrency here is
not a throughput knob, it is a blast radius.
TOO_MANY_TURNS/ 503 — therequest is fine and the server is full, which is what 503 means.
Checked at admission only. Work already admitted runs to completion: an
autopilot build chains through
runTurndirectly, and a review round is insidea turn already counted. Killing either halfway leaves a half-built board, which
is worse than a slow one.
Both errors are plain
Errors carryingcodeandstatusCoderather thanimporting the HTTP layer into the driver —
sendIpcErrorreads those off anythrown value, and the command-boundary log above already classifies a coded
refusal as
refused.Still open in P0
P0.1 · auth is untouched and is the one that gates a deploy. Note the
ordering the doc calls out: auth and metering have to land together, because
authenticating users without attributing their spend gives every logged-in user
every other user's boards. The
user=column exists now and is empty, waitingfor exactly that.
Checks
usage.test.mjs13/13, built from a real board's countersdriver.test.mjs49 → 53upstream/main@ 6040efb,captured fresh from a worktree and compared with
comm— empty both waysnpm run buildpassesNo new ChatEvent kind, so
docs/circuit-interfaces.md§3 is untouched.🤖 Generated with Claude Code