fix(proxy): tell an unreadable usage block apart from a provider that reported none, and record the shape once - #205
fix(proxy): tell an unreadable usage block apart from a provider that reported none, and record the shape once#205amiddavid wants to merge 5 commits into
Conversation
… reported none, and record the shape once Closes #200. `parseUsage` returning ok=false meant FIVE different things and nothing distinguished them. Two are benign — the provider genuinely reported no usage, or a recognised block whose every tier is zero. The rest mean TOKEN ACCOUNTING IS OFFLINE, on a request that otherwise looks perfect: a healthy 200, correct savings counters, correct latency, correct everything else, and fresh_input_tokens / cache_read_tokens / cache_write_tokens quietly at 0. It ran that way for 4,015 of 4,015 requests in one benchmark iteration and was found two iterations later, in a post-mortem chasing a different question. The usage WAS in the body — LOCA priced those requests from it — so the proxy read a response that carried usage and found nothing in it. WHAT IS CLASSIFIED, AND WHY IT IS NOT ONE COUNTER usageMiss names all five, and the reason rides on the cg.request log line as `usage_miss` so a benign miss is legible without being alertable: absent no usage block anywhere we know to look BENIGN all_zero recognised, every tier zero LEGITIMATE no_body empty response BENIGN unparsed_dialect a block IS there, no recognised spelling read it unreadable_body the bytes are not a whole document, so the block was hidden The last two get counters, and they get SEPARATE ones because their remedies are opposite: `unparsed` means add the dialect the provider is speaking, `unreadable` means the bytes examined were not a whole document and the fix is upstream of any parser. One counter would have said "accounting is offline" without saying which half of the stack to look at — and putting either of them in with the benign cases is the defect this issue is about, restated. Same argument as expand/unresolved.go's malformed/missing split and #188's stash_refused/stash_missing. A FOURTH CAUSE THE ISSUE DID NOT LIST `sniffer.bytes` returns head+"\n"+tail once a response passes sniffMax (64 KiB) each way, and that is not valid JSON. Reported as an unrecognised dialect it would send someone hunting for a field name that is not missing. It is NARROWER than "the response was truncated", and the code and tests both say so: gjson SCANS rather than walking a tree, so a spliced document whose top-level `usage` survived is read correctly and never reaches this classification. It fires only when the splice actually hid the block, which is the only case where anything is lost. Both test tables carry a note about it, because the obvious fixture — a malformed body — is read correctly and would have made the test vacuous. It did, on the first run. THE SHAPE RECORD, WHICH IS THE PART THAT UNBLOCKS THE DIALECT FIX On the FIRST unaccounted response per process, cg.usage_unaccounted logs the response's KEY NAMES: the top-level keys, where a usage block was found, and the key names inside it. No values, and never the body. That is what turns "usage_reported is false" into "the provider is sending camelCase", and it needs no captured body, no instrumented capture hop and no extra request — so it works from any deployment that hits the gap rather than from the one rig with a capture hop in the path. A body dump on this workload writes kilobytes of transcript content to disk per response; this record cannot, by construction, which is why it can ship on by default at DEBUG. Once per process because a run with this gap has it on EVERY request — 4,015 of 4,015 in the iteration above — so a per-response record is a log line per request. DELIBERATELY NOT HERE: THE DIALECTS THEMSELVES Adding the camelCase or nested spellings needs a real response body to get the field names right. Guessing between `cacheWriteInputTokens` and `cacheCreationInputTokens` produces a parser that looks correct and reads zero, which is this issue's own failure reproduced in the code meant to fix it. The nested paths here are probed for EXISTENCE ONLY and never read, which is what keeps the classification dialect-agnostic. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, go test -race clean over proxy and metrics (Go 1.26.4, eval box). Four new tests, all revert-verified against a mutation that collapses the classification back to one undifferentiated miss: TestEveryUsageOutcomeIsDistinguishable twelve shapes, one per outcome per transport, including the Bedrock Converse camelCase and nested `response.usage` shapes that were indistinguishable from "the provider said nothing". Reverted -> "classified absent, want unparsed_dialect". TestOnlyAnAccountingOutageMovesTheUsageCounters the rule itself: the two alertable cases move their own counter and nothing else, and all four benign shapes move neither. Reverted -> "usage_unparsed moved by 0, want 1". TestTheShapeRecordNamesTheDialectAndCarriesNoContent that the record answers the dialect question, that it is once per process, and that it carries NO content and no values. Reverted -> the record is absent entirely. TestTheShapeRecordSaysWhenTheBytesWereNotAWholeDocument valid_json=false in the record, so a spliced window cannot be misread as a field-name gap. The no-content assertion was verified against a SECOND mutation, because "the record does not contain this string" passes trivially when there is no record and would also have passed on a record that logged the body under a different key. With usageShapeAttrs changed to log the body, it fails on its own subject: "the shape record leaked response CONTENT, which is the one thing it must never do". Two contract tests updated as designed: TestStatsShapeIsUnchanged (both fields added to the reviewed top-level contract) and TestEverySnapshotFieldIsExportedOrExempt. The second is listed in notExportedWhy rather than read off `s`, for the reason that map's own preamble gives — the /stats handler fills these AFTER renderMetrics takes its snapshot, so a promLine off `s` would export a permanent 0 while passing the test, which is precisely the silent-zero failure this change exists to report. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…window Comment and docs only; no behaviour and no test outcome changes. `unreadable_body` is reachable ONLY from the sniffed path — Handler.stream, taken when neither proxy-injected tool is advertised on the request — because that is the only place usage is read from a bounded head+tail window rather than the whole body. With either tool advertised, which `inject_expand: always` guarantees from the first turn, a non-streamed response is read whole at proxy.go's `default:` branch and the window cannot apply. Recorded because the call-site distinction has now misled two sessions in opposite directions. I floated the spliced window as a competing explanation for iteration 024's usage_reported=false on 4,015 of 4,015 requests; it cannot be one, because those runs set INJECT_EXPAND=always so `advertised` was true and the full body was read. The reviewer on #188 hit the mirror image, writing a parseUsage test that passed on the stream branch while the traffic in question took the buffered one. Both are the same error: reading the two `responseUsage` call sites as interchangeable when the bytes reaching them are not. Neither counter changes, and `valid_json` in the shape record already settles which case a reader has without knowing their path — this only saves them forming the wrong hypothesis first. Credit for the correction, and for checking it against the actual line numbers rather than the plausible reading, to the coref validation session holding the iteration 024 measurement. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
|
Follow-up in 2da9bd5, from the coref validation session's correction — comment and docs only, no behaviour change.
Worth recording because the call-site distinction has now misled two sessions in opposite directions. I floated the spliced window as a competing explanation for iteration 024's 4,015-of-4,015; it cannot be one, since those runs set Neither counter changes, and Scoping consequence for #199 and the dialect fix: for traffic that advertises either injected tool — the common case — camelCase-or-nested is the only remaining explanation for a miss, so the shape record's |
amiddavid
left a comment
There was a problem hiding this comment.
The classification is the right shape and the unreadable_body cause you added is a real one — the narrowness argument (gjson scans, so a splice that leaves the top-level usage visible is read correctly) checks out, and the fixture note is worth keeping.
But I think the change is currently net-negative on the streamed transport, and the run it is meant to unblock will probably produce no shape record at all. Two findings, both verified by running probes against this branch on the eval box rather than by reading:
=== proxy, branch 2da9bd5, go test -run TestReviewProbe
openai stream chunk with usage:null -> why=unparsed_dialect ok=false unparsed+1
sse recognised all-zero -> why=unparsed_dialect ok=false unparsed+1
sse usage empty object -> why=unparsed_dialect ok=false unparsed+1
spliced window first, camelCase next -> NO RECORD (the once was consumed)
- Three benign streamed shapes move
usage_unparsed, the counter whose whole meaning is "add the dialect". One of them —"usage": null— is what the OpenAI dialect puts in every streamed chunk unlessstream_options.include_usageis set, so on theopenai_upstreamroute this counter is driven by healthy traffic. - Because the shape record is one
sync.Oncefor both alertable classes, the first of those benign streams (or anyunreadable_body) consumes it, and the camelCase response that follows logs nothing. The datum #199 and the dialect fix are waiting on is the first thing this loses.
(1) also explains why review did not catch it: TestOnlyAnAccountingOutageMovesTheUsageCounters has no SSE row, so it asserts the rule on the one transport where the rule holds. Details inline. Everything else — the counter split, the no-content assertion and its second mutation, valid_json in the record, 2da9bd5's call-site scoping — I have no objection to.
| switch { | ||
| case found: | ||
| return out, usageMissNone | ||
| case sawBlock: |
There was a problem hiding this comment.
sawBlock makes three benign streamed shapes alertable, and one of them is the OpenAI dialect's normal output.
The comment concedes "a stream whose only usage block is all-zero over-reports by one". It is worse than one, because gjson.Result.Exists() is t.Type != Null || len(t.Raw) != 0 — a JSON null has Raw == "null", so "usage": null sets sawBlock, parseUsage falls to its default: branch, and the stream is classified unparsed_dialect.
OpenAI-dialect streaming sends "usage": null on every chunk unless the caller sets stream_options.include_usage, and this proxy routes those through Handler.stream → responseUsageWhy (proxy.go:1489, the non-Anthropic case isSSE:). So on the openai_upstream route, usage_unparsed — documented as "add the dialect the provider is speaking" — is incremented by every healthy streamed response. Verified on this branch:
openai stream chunk with usage:null -> why=unparsed_dialect ok=false unparsed+1 unreadable+0
sse recognised all-zero -> why=unparsed_dialect ok=false unparsed+1 unreadable+0
sse usage empty object -> why=unparsed_dialect ok=false unparsed+1 unreadable+0
That is this issue's own defect with the sign flipped: instead of an outage reading as healthy, healthy traffic reads as an outage — and it costs the same thing, an operator who cannot tell from the counter which they have.
The cheapest fix that keeps the coarseness you want: only set sawBlock when the block is a non-empty object (u.IsObject() && len(u.Get("@keys").Array()) > 0, or just u.IsObject() plus a Raw != "{}" check), and treat "every block seen was recognised and all-zero" as usageMissZero. parseUsage already distinguishes those two internally — parseUsageWhy per event would give you the exact reason without a second pass, which is the same asymmetry the split at :118 was introduced for.
| {"an unreadable window", "application/json", | ||
| `{"content":[{"text":"x"` + "\n" + `"usage":{"input_tokens":1}}`, 0, 1}, | ||
| {"a provider that said nothing", "application/json", `{"stop_reason":"end_turn"}`, 0, 0}, | ||
| {"a recognised all-zero block", "application/json", `{"usage":{"input_tokens":0}}`, 0, 0}, |
There was a problem hiding this comment.
This table is transport-blind, which is why the counter rule it names is violated on the stream path.
Every row is application/json. TestEveryUsageOutcomeIsDistinguishable above deliberately covers "one per outcome per transport" and is the stronger table for it — but it does not assert on the counters, and this one does. So the rule the counter must not be moved by a benign outcome is asserted only where parseUsageWhy runs, and never where parseSSEUsageWhy does, which is the half that breaks it (see the note at usage.go:247).
Two rows would have caught it and are worth adding whichever way you fix the classifier:
{"a streamed all-zero block", "text/event-stream",
"data: {\"type\":\"message_delta\",\"usage\":{\"output_tokens\":0}}\n", 0, 0},
{"a streamed null usage field", "text/event-stream",
"data: {\"choices\":[{\"delta\":{}}],\"usage\":null}\n", 0, 0},And TestEveryUsageOutcomeIsDistinguishable has no usageMissZero row for SSE either, so that value is currently unreachable-by-test on the transport where it is misclassified. Both tables want the same discipline the first one already states: one row per outcome per transport, with the transport axis complete rather than sampled.
| // diagnostic, not a metric: one record names the gap, and one per response would put a line in | ||
| // the log for every request of a run that has the gap on all of them (4,015 of 4,015, in the | ||
| // iteration that motivated this). | ||
| var usageShapeOnce sync.Once |
There was a problem hiding this comment.
One Once for two classes means the first alertable response wins, and it is unlikely to be the one that answers the question.
The record is the part of this change that unblocks #199 and the dialect fix. It is gated on a single process-wide sync.Once shared by unparsed_dialect and unreadable_body, so whichever fires first consumes it. Verified on this branch — a spliced window, then the camelCase response:
spliced window first, camelCase next -> NO RECORD: the once was consumed by the
unreadable_body, so the camelCase question
is unanswered
Combine that with the sawBlock finding at :247 and the odds get worse: on any deployment carrying OpenAI-dialect streamed traffic, a benign "usage": null chunk is alertable and will almost certainly be the first alertable response in the process, so the record is spent before the interesting response ever arrives. The run this ships for then produces the counters and no shape.
One Once per usageMiss value is the minimum. Better, and still bounded: key it on the record's own identity — usage_at plus the sorted usage_keys — in a small sync.Map capped at, say, 8 distinct shapes. That is one line per distinct dialect rather than one per process, which is what the diagnostic is actually for (a multi-provider deployment has more than one answer), and it still cannot write a line per request: a run with the gap on all 4,015 requests has one shape.
Also: ResetUsageShapeRecordForTest assigning usageShapeOnce = sync.Once{} is a plain data race against a concurrent Do. Harmless as used today (both callers are serial tests), but -race will find it the moment someone marks one of those tests t.Parallel(). Whatever replaces the Once should get a mutex-guarded reset.
| usage, usageOK, usageWhy = u, true, why | ||
| } else { | ||
| usage.StopReason = u.StopReason // same reasoning as the stream path above | ||
| usageWhy = why |
There was a problem hiding this comment.
Across expand rounds, usageWhy is last-write-wins while usageOK is sticky-true, so the log line can contradict itself.
usageOK is only ever set to true (:1560, :1492, :1544) and never reset; usageWhy is assigned unconditionally on every round. A request whose round 1 parsed usage and whose round 2 (post-expand continuation) carried none logs usage_reported=true usage_miss=absent. The reverse pairing is reachable too. Since the whole point of usage_miss is to let someone read usage_reported=false and know which of the five it was, a pair that can disagree undoes some of that.
Smallest fix: keep the reason of the round that left the request unaccounted — usageWhy = why in the ok branch, and if !usageOK { usageWhy = why } in the else.
Separately, worth a sentence in UsageGaps' doc: these count responses, not requests, so a multi-round request can move them more than once. The docstring already says "how many responses", so it is consistent — but docs/reference/routes.md invites comparison against a request count, and the 4,015-of-4,015 framing in the PR body is a per-request figure.
| // than walking a tree, so a spliced document whose top-level `usage` survived is read | ||
| // correctly above and never reaches here. This fires only when the splice actually hid | ||
| // the block, which is the only case where anything is lost. | ||
| if !gjson.ValidBytes(body) { |
There was a problem hiding this comment.
Minor, and possibly deliberate: the nestedUsagePaths probe runs before ValidBytes, so a spliced window that hid the top-level usage but left, say, response.usage scannable is classified unparsed_dialect rather than unreadable_body. I think the current order is defensible — a block that was found really does mean a dialect is missing, whatever else is wrong with the bytes — but it is the one ordering that can produce the mis-signal the paragraph above says it exists to prevent, and valid_json in the record is what saves the reader. One clause saying the precedence is chosen (found-block wins over unparseable-document) would keep the next person from "fixing" it the other way.
… give each shape its own record Review round 1 on #205. Two of the five findings were live defects the tests missed, and the first is this issue's own failure with the sign flipped. A NULL USAGE FIELD WAS COUNTED AS AN UNRECOGNISED DIALECT. gjson's Exists() is `Type != Null || len(Raw) != 0`, and a JSON null has Raw == "null" — so `"usage": null` looked like a present block that no recognised spelling could read, and was classified unparsed_dialect. OpenAI-dialect streaming sends exactly that on every chunk unless the caller sets stream_options.include_usage, and those responses reach parseSSEUsageWhy through the non-Anthropic `case isSSE:` at proxy.go:1489. So on the openai_upstream route the counter documented as "add the dialect the provider is speaking" was incremented by ordinary healthy traffic. That is exactly what this issue is about, reversed: instead of an outage reading as healthy, healthy traffic reads as an outage, and it costs the same thing — an operator who cannot tell from the counter which one they have. WORSE THAN THE REVIEW FOUND. The reviewer verified three streamed shapes. The non-streamed path had it too: `{"usage":null}` and `{"usage":{}}` on application/json both counted as unparsed, because parseUsageWhy's top-level probe and its nestedUsagePaths loop used the same Exists(). Five rows now fail against the old code, not three. usagePresent() replaces every Exists() on a usage block: an object carrying at least one field. `{}` is treated as absent for the same reason a null is — a provider that sent an empty object told us nothing, which needs no action, where a missing spelling does. AND THE STREAMED ALL-ZERO CASE IS NO LONGER LUMPED IN. parseSSEUsageWhy kept a bare sawBlock flag and reported every rejected block as a dialect gap; the comment conceded it "over-reports by one". It now calls parseUsageWhy per event and keeps the WORST reason any event produced, so a streamed all-zero block classifies as all_zero. That made the usageMiss constants' ORDER load-bearing, which is now stated at the constants: ascending in severity, insert at position. ONE `Once` FOR TWO CLASSES SPENT THE DIAGNOSTIC ON WHICHEVER RESPONSE CAME FIRST. The shape record is the part of this change that unblocks the dialect fix, and both alertable classes shared one process-wide sync.Once. A spliced window followed by the camelCase response produced a record about the window and left the dialect question unanswered — and combined with the null-usage defect above, a benign OpenAI chunk was alertable and would almost certainly BE that first response on any deployment carrying streamed OpenAI traffic. The run this ships for would have produced the counters and no shape. Now keyed on the record's own identity — where the block was, plus its sorted key names — in a mutex-guarded set capped at 8 distinct shapes. That is one line per DIALECT, which is what the diagnostic is for (a multi-provider deployment has more than one answer), while a run with the same gap on 4,015 of 4,015 requests still produces one line. Deliberately NOT keyed on the top-level keys: those vary with a response's content, so one dialect would burn the whole budget. The reset helper is mutex-guarded too. `usageShapeOnce = sync.Once{}` was a plain data race against a concurrent Do — harmless as used, and -race would have found it the moment one of those tests was marked t.Parallel(). THE LOG LINE COULD CONTRADICT ITSELF. usageOK is sticky-true across expand rounds — a request whose usage was read once IS accounted — while the reason was last-write-wins, so a round 2 continuation carrying no usage relabelled an accounted request `absent` and logged `usage_reported=true usage_miss=absent`. noteUsageMiss folds them: accounted by any round means `parsed`, and while nothing has, keep the worst reason seen so a benign later round cannot mask a dialect gap an earlier one found. TWO CLARIFICATIONS, NO BEHAVIOUR CHANGE. The nestedUsagePaths probe running before ValidBytes is now stated as CHOSEN: a found block wins over an unparseable document, because a block we did find really does mean a spelling is missing whatever else is wrong with the bytes, and valid_json in the record tells the reader the document was also truncated. With a note not to "fix" it the other way round, which would hide a real dialect behind a transport problem. And UsageGaps counts RESPONSES, not requests — one request can drive several upstream rounds — so the 4,015-of-4,015 per-request figure is not directly comparable. Said in the docstring and at docs/reference/routes.md, which invited the comparison. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, go test -race ./proxy/... clean (Go 1.26.4, eval box). The counter table was TRANSPORT-BLIND, which is why it asserted the rule only where it held: every row was application/json, so the half that broke it was never exercised. Six rows added there and five to the outcome table, completing the transport axis rather than sampling it — including a stream whose first event is null and whose second carries real tiers, which must take the GOOD outcome, and a streamed positive control so "no benign row moves it" cannot pass by the classifier having stopped counting on that path altogether. Three new/changed assertions, each revert-verified against the specific pre-review behaviour: usagePresent -> Exists() 8 subtests fail; "usage_unparsed moved by 1, want 0" on both transports one gate for every shape "the camelCase shape was never recorded — an earlier unrelated response spent the one record" noteUsageMiss -> last-write-wins "an accounted request reported absent" AND "a later benign round masked the round that found a dialect gap" Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…e it, and report the capped horizon Review round 1 on #204. All five findings accepted; the load-bearing claim survived the attack, the UNIVERSAL QUANTIFIER around it did not. "EVERY OFFLOADER REPLAYS ON EVERY TURN" IS FALSE IN TWO WAYS, WITH DIFFERENT CONSEQUENCES. Both verified before writing: - summarize returns at summarize.go:157 on its trigger and at :162/:165 when no model client resolves, all ahead of tryReuse where its commitRefresh lives; extract_llm returns at extract_llm.go:659 on no_goal_keywords, ahead of Phase 1. A skipped turn refreshes none of their payloads. And summarize's trigger skip is RECURRING rather than the single-request event the exposure paragraph assumed — the agent's own compaction shrinks the incoming request and can drop it back under Trigger.MinRequestTokens for consecutive turns, and "the cheap model is down" persists for many turns by nature. The mitigating half, which the review did not have to give me and which I state because it bounds the severity: a skipped component splices NOTHING, so no marker of its goes upstream on those turns and none dangles. The payload's reclamation is harmless while the skip lasts; the exposure is only that its next firing may find the payload gone and the reserve full at the same moment. - dedup, extract, linecap and smartcrush have NO replay path at all — no reapplyFrozen, no commitRefresh (confirmed: one commitMark each, zero of either). They redo the transformation from the re-sent original every turn, so their per-turn write goes through the REFUSABLE commitMark. While the payload is live that is PutStash's refresh branch, retained unconditionally; once reclaimed it is a NEW stash, and a new stash into a saturated reserve is refused, the component declines, and the message goes upstream verbatim after earlier turns sent it compacted. So for those four the outcome is stash_refused PLUS a representation flip — not the stash_missing my paragraph promised. And stash_refused's operator-facing text promises "nothing became irreversible", which is true about reversibility and silent about the cache-write actually paid. Reachable at 10,000s too, so not introduced here; this horizon shortens the distance to it by 5.5x. Both are now a per-offloader table in the comment and in docs/reference/config.md, rather than a claim that holds for seven of thirteen. I did NOT hoist summarize's replay above its model gate, which the reviewer mildly preferred: it is a behaviour change in a component with its own test burden, and the narrowed claim is honest without it. Left as a follow-up. ONE HELPER FOR EVERY EXPIRY WRITE. The reviewer audited all five e.expires sites and found no sibling bug, but noted the two unnoted ones are safe only as a CONSEQUENCE of stashTTL <= ttl plus a monotonic clock — so the completeness rests on a cap elsewhere and has to be redone by hand if that ever changes. setExpiry now stamps the deadline and lowers the sweep bound in one place, used at all five sites. noteExpiry on a later-only deadline is a no-op by construction, so the unconditional version costs one comparison and makes a sixth site unable to get it wrong. Same completeness argument #198 is open about for gateExempt. THE CAP WAS SILENT AND /config PUBLISHED THE PRE-CAP VALUE. `stash_ttl_seconds: 20000` with `ttl_seconds: 10000` displayed 20000 on /config and the dashboard while the store used 10000 — an operator told one thing while another runs, which is #205's shape in the config surface. store.EffectiveStashTTLSeconds derives the value from the same code path NewMemory uses, so the two cannot drift, and main.go publishes that. The cap itself stays, with no escape hatch, for the reason the review gives: wanting payloads to outlive decisions is asking for #190 by configuration. Also narrowed the justification, which was overstated: a payload outliving its decision is a slot held for ALMOST nothing, not for nothing that can "ever" be read — the model can call expand on a marker it read in an earlier turn's context, since the marker lives in the conversation it reasons over and not only in the request the proxy just built. Rare and short-lived, and it does not change the conclusion. A ZERO ON THE NEW PAIR IS NOT EVIDENCE. sweepExpired runs only from StashRoom, PutStash's pre-refusal path and evictOldest — only once a budget is already binding — and PutStash's refresh branch does not check expiry, so on an unsaturated run an expired-but-unswept payload is resurrected in place and NEITHER stash_expired nor stash_revived moves. So the PR's claim that the measurement "arrives on the first run" holds only for a run that actually saturates the reserve, which is the same precondition iteration 024 failed for stash_refused — and failing it is how #190 became undecidable from data in the first place. Said at metrics.Snapshot and in the docs: both at zero means THE RESERVE NEVER BOUND, and what distinguishes that from "the horizon works" is stash_refused and stash_live against stash_capacity. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, go test -race clean over store and components/offload (Go 1.26.4, eval box). One new test, revert-verified: TestTheConfigSurfaceReportsTheEffectivePayloadHorizon — with EffectiveStashTTLSeconds returning the raw field again it fails on "/config would advertise a horizon the store does not use". It also cross-checks the helper against a store built from the same Options for four option shapes, so the two cannot drift apart silently, which is the actual defect rather than the one wrong number. The other four findings are comment and documentation only, so no test changes: the existing suite passes unchanged, which is the correct outcome for a claim that was too broad rather than a behaviour that was wrong. Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
amiddavid
left a comment
There was a problem hiding this comment.
Re-verified all five on the eval box against a41843c. Every one is fixed, and the null-usage finding is fixed more broadly than I reported it — you were right that parseUsageWhy's top-level and nested probes had the same defect, so five rows rather than three:
json usage null -> why=absent unparsed+0
json usage empty obj -> why=absent unparsed+0
sse usage null per chunk -> why=absent unparsed+0
sse all-zero -> why=all_zero unparsed+0 (its own value, not the coarse one)
sse empty obj -> why=absent unparsed+0
sse camelCase -> why=unparsed_dialect unparsed+1
worst with the severity-ordered constants is a better answer than the coarsening your original comment conceded, and stating that the order is part of the type is the right way to keep it. noteUsageMiss, usageShapeKey on usage_at + sorted usage_keys (and not the top-level keys — agreed, and for the reason you give), the mutex-guarded reset, and the RESPONSES-not-requests note all read correctly to me.
Two things still wrong, both in the record rather than the classification — so the counters are now right and the diagnostic they point at can still come out empty. Neither is large; the first is the one I would fix before merge, the second is a word.
On #204 I have nothing further — separate review there. And thank you for the bound on the four no-replay offloaders: a skipped component splices nothing, so nothing dangles while the skip lasts. That is the right narrowing and it is a better statement of the exposure than mine was.
| "top_level_keys", objectKeys(gjson.ParseBytes(doc))} | ||
| for _, p := range append([]string{"usage"}, nestedUsagePaths[:]...) { | ||
| if u := gjson.GetBytes(doc, p); u.Exists() { | ||
| return append(attrs, "usage_at", p, "usage_keys", objectKeys(u)) |
There was a problem hiding this comment.
usageShapeAttrs still uses Exists() where the classifier now uses usagePresent, so the record can name the wrong location.
The loop here is if u := gjson.GetBytes(doc, p); u.Exists(). parseUsageWhy was changed to usagePresent for exactly this reason, and the two now disagree: a body with a top-level "usage": null and a real nested block is classified from the nested block, then described from the null one. Verified on this branch with {"usage":null,"response":{"usage":{"inputTokens":10,"cacheReadInputTokens":9}}}:
why=unparsed_dialect
cg.usage_unaccounted ... top_level_keys="[response usage]" usage_at=usage usage_keys=[]
usage_at=usage usage_keys=[] — it points at the null and reports no field names, while response.usage held the answer. It also keys the shape as usage|, so a second provider with a genuinely empty top-level usage collides with it.
One word: usagePresent(u) instead of u.Exists(). Worth doing even though the shape is speculative, because the whole reason usagePresent exists is that Exists() on a usage block does not mean what it looks like — leaving one caller on the old predicate is how that comes back.
| if sse { | ||
| // One event, so the record describes a JSON object rather than a transport. The last | ||
| // data: line is where both dialects put their terminal usage. | ||
| doc = []byte(lastSSEPayload(body)) |
There was a problem hiding this comment.
The classifier scans every event; the recorder reads only the last one. An unrecognised dialect in a non-terminal event produces a record that names nothing.
parseSSEUsageWhy walks all data: lines and keeps the worst reason any of them produced. usageShapeAttrs builds its record from lastSSEPayload(body) — a single event. When the block that drove the classification was not in the terminal event, the record describes the wrong object. Verified on this branch, a stream whose message_start carries camelCase and whose last event is message_stop:
why=unparsed_dialect
cg.usage_unaccounted sse=true bytes=255 valid_json=true top_level_keys=[type]
GAP: the record does not name the dialect that was actually found
No usage_at, no usage_keys, and top_level_keys=[type] from message_stop. Two consequences: the counter fires with nothing to act on, and via usageShapeKey this takes the "|" slot — the stable key you reserved for "no block found at all" — so the shape budget is spent on a record that says nothing.
This is the Anthropic-family transport shape, where the input-token block lives in message_start. Since the motivating provider is aws/claude-* through Bedrock, a streamed instance of exactly the gap this change exists to name can land here.
The fix that keeps the two in agreement is to record the event that produced the classification rather than the last one: have parseSSEUsageWhy also return the raw payload of the event that set worst, and pass that to usageShapeAttrs instead of calling lastSSEPayload. That also removes the current duplication, where the transport is parsed once to classify and again to describe. A cheaper version — scan events backwards for the first one containing a usagePresent block, falling back to the last — fixes the record but leaves the two walks able to disagree.
… last one Review round 2 on #205. Both remaining findings were in the shape record — the deliverable — rather than in the classification, and the first one broke it exactly where the motivating gap lives. THE RECORD DESCRIBED THE WRONG EVENT. parseSSEUsageWhy scans every event of a stream; usageShapeAttrs described only the LAST `data:` payload. On the Anthropic-family transport usage arrives in `message_start` and the stream ENDS with `message_stop` — so a streamed unrecognised dialect produced `why=unparsed_dialect` and a record of the terminal event: `top_level_keys=[type]`, no usage_at, no usage_keys, nothing about the dialect. And because it had no block, it took the `"|"` key reserved for "no block found", so one of the eight bounded slots was spent on a record that says nothing. That is the shape a streamed Bedrock `aws/claude-*` response has, which is where a streamed instance of the gap this record exists to diagnose would sit. The record is this change's whole deliverable, so failing on that shape is failing at the one job. parseSSEUsageWhy now returns the payload of the event that set `worst`, and responseUsageWhy hands that to the record as the document to describe — which also collapses the two walks over the transport into one, as suggested. `bytes` stays the WHOLE response's size, because that is the figure that says a window was spliced; only the described document narrows. lastSSEPayload is deleted rather than left unused. THE RECORD LOOKED WHERE THE CLASSIFIER NO LONGER DID. usageShapeAttrs still used Exists() where parseUsageWhy uses usagePresent after round 1. With `{"usage":null,"response":{"usage":{"inputTokens":10,...}}}` the record read `usage_at=usage usage_keys=[]` — pointing at the null, naming no fields, while `response.usage` held the answer — and keyed as `usage|`, colliding with a genuinely empty top-level block. One word, and a comment saying the record must look where the classifier looked, since that is the invariant that broke. VERIFICATION gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, go test -race ./proxy/... clean (Go 1.26.4, eval box). Two new tests, both revert-verified against the specific pre-fix behaviour, and both worth the care because a test that asserts "the record contains X" passes vacuously when there is no record at all — so each was checked to fail with the record PRESENT and wrong, not absent: never carry the judged event -> "does not contain cacheReadInputTokens", with the record shown as `sse=true bytes=125 valid_json=false top_level_keys=[]` usagePresent -> Exists() -> "points at the null block instead of the one carrying the fields", with the record shown as `usage_at=usage usage_keys=[]` Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
amiddavid
left a comment
There was a problem hiding this comment.
Both fixed, verified from the record's own text on the eval box against 7a70d76:
sse dialect in a non-terminal event:
cg.usage_unaccounted sse=true bytes=125 valid_json=true
top_level_keys="[message type]" usage_at=message.usage
usage_keys="[cacheReadInputTokens inputTokens]"
null top-level beside a real nested block:
cg.usage_unaccounted sse=false bytes=71 valid_json=true
top_level_keys="[response usage]" usage_at=response.usage
usage_keys="[inputTokens outputTokens]"
And every shape from all three rounds re-checked for regressions — the five null/empty/all-zero cases still benign with no counter movement, camelCase still unparsed_dialect, both healthy shapes still parsed. Keeping bytes as the whole response while narrowing only the described document is the right split; deleting lastSSEPayload rather than leaving it unused is what makes the one-walk property hold. No objection left on this branch — LGTM (this account cannot file a formal approval on its own PR).
One last observation below. It is a residual gap rather than a defect in this change, it is non-blocking, and I would be equally happy with a comment as with code — I am raising it because it is #200's own shape on the one transport that is not covered, and because the signal it needs already exists.
I have nothing further on #204 either — separate note there.
| // Every block that appeared was rejected, and `worst` says why — an unrecognised spelling is | ||
| // alertable, an all-zero block is not, and no block at all is `absent` because that is what | ||
| // `worst` starts as. | ||
| return out, worst, worstPayload |
There was a problem hiding this comment.
Non-blocking, and last thing from me: parseSSEUsageWhy can never return usageMissUnreadable, so a spliced window that hides usage on the streamed transport is classified benign.
A data: line cut mid-JSON does not yield a usagePresent block, so worst stays at its initial usageMissAbsent and the response is reported as "the provider streamed no usage" — no counter, no record. Verified on this branch:
spliced sse window -> why=absent unparsed+0 unreadable+0 record: (none)
The reason this is worth a line: 2da9bd5 established that unreadable_body is reachable only from the sniffed path, and the sniffed path is exactly where SSE responses get a head+"\n"+tail window. So usage_unreadable fires for sniffed JSON bodies (which is what the fixture models, via ValidBytes) and is structurally unreachable for sniffed streams — where a splice that hides the block reads as absent, which is an accounting outage indistinguishable from a provider that reported none. That is #200's defect, in the corner this classification does not reach.
In practice the head window saves the common case: Anthropic puts input_tokens in message_start, which is at the front of the head, so found is true and the classification is parsed. So this is narrow, and I would not hold the PR for it.
If you do want it closed, the signal is already there and needs no new parsing: sniffer knows it spliced — s.total > len(s.head) in bytes(). Exposing that as a spliced() bool and letting Handler.stream pass it into responseUsageWhy would turn absent on a spliced window into unreadable, and it would make the buffered case exact rather than inferred from ValidBytes failing. Either way it belongs on the issue rather than in this diff, and a sentence at worst's initialiser saying the streamed transport has no unreadable value would be enough to stop the next reader assuming symmetry between the two parsers.
Comment only; no behaviour and no test outcome changes. parseSSEUsageWhy can never return usageMissUnreadable, and the next reader would reasonably assume the two parsers are symmetric. It only calls parseUsageWhy with a block usagePresent has already accepted, and that function reaches its ValidBytes branch only when no block was found anywhere — so `worst` ranges over absent/all_zero/unparsed_dialect. A `data:` line cut mid-JSON yields no present block, so the stream reads as "the provider streamed no usage". The consequence, stated where the gap is rather than left for someone to rediscover: usage_unreadable is reachable for a sniffed JSON response and structurally unreachable for a sniffed STREAM — which is exactly where a head+tail window would hide a block, so it is #200's own defect in the corner this classification does not cover. Narrow in practice, because Anthropic puts input_tokens in message_start at the front of the head window, so `found` is true and the answer is `parsed`. Closing it properly means passing the sniffer's own knowledge that it spliced (s.total > len(s.head)) into responseUsageWhy, which would also make the buffered case exact instead of inferred from ValidBytes. Filed as an issue rather than guessed at on this branch — the classification is already the part of #200 that ships, and this needs its own decision about threading a transport fact into a parser. Reviewer's finding, including the observation that the two-parser asymmetry is the thing a reader will get wrong. Verification: gofmt -l . clean, go vet ./... clean, go test ./proxy/ passes (Go 1.26.4, eval box). Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Closes #200. Independent of #204 (#190) — both branch from
main.The defect
parseUsagereturningok=falsemeant five different things and nothing distinguished them. Two are benign. The rest mean token accounting is offline, on a request that otherwise looks perfect: a healthy 200, correct savings counters, correct latency, correct everything else, andfresh_input_tokens/cache_read_tokens/cache_write_tokensquietly at 0.It ran that way for 4,015 of 4,015 requests in one benchmark iteration and was found two iterations later, in a post-mortem chasing a different question. The usage was in the body — LOCA priced those requests from it — so the proxy read a response that carried usage and found nothing in it.
What is classified, and why it is not one counter
usageMissnames all five, and the reason rides on thecg.requestlog line asusage_missso a benign miss is legible without being alertable:usage_missabsentall_zerono_bodyunparsed_dialectusage_unparsedunreadable_bodyusage_unreadablesniffMax/ bufferThe last two get counters, and separate ones, because their remedies are opposite: one means add the dialect the provider is speaking, the other means the bytes examined were not a whole document and the fix is upstream of any parser. One counter would have said "accounting is offline" without saying which half of the stack to look at — and putting either in with the benign cases is this issue's own defect, restated. Same argument as
expand/unresolved.go's malformed/missing split and #188'sstash_refused/stash_missing.A fourth cause the issue did not list
sniffer.bytesreturnshead + "\n" + tailonce a response passessniffMax(64 KiB) each way, and that is not valid JSON. Reported as an unrecognised dialect it would send someone hunting for a field name that is not missing.It is narrower than "the response was truncated", and the code and both test tables say so: gjson scans rather than walking a tree, so a spliced document whose top-level
usagesurvived is read correctly and never reaches this classification. It fires only when the splice actually hid the block — the only case where anything is lost. Worth flagging because the obvious fixture (a malformed body) is read correctly and would have made the test vacuous. It did, on the first run; both tables now carry the note and the shape that genuinely hides the block.The shape record, which is the part that unblocks the dialect fix
On the first unaccounted response per process,
cg.usage_unaccountedlogs the response's key names: the top-level keys, where a usage block was found, and the key names inside it. No values, and never the body.That turns "usage_reported is false" into "the provider is sending camelCase", and it needs no captured body, no instrumented capture hop and no extra request — so it works from any deployment that hits the gap rather than only from the one rig with a capture hop in the path. A body dump on this workload writes kilobytes of transcript content to disk per response; this record cannot, by construction, which is why it can ship on by default at DEBUG.
Once per process because a run with this gap has it on every request — 4,015 of 4,015 above — so a per-response record is a log line per request.
Deliberately not here: the dialects themselves
Adding the camelCase or nested spellings needs a real response body to get the field names right. Guessing between
cacheWriteInputTokensandcacheCreationInputTokensproduces a parser that looks correct and reads zero — this issue's failure reproduced in the code meant to fix it. The nested paths here are probed for existence only and never read, which is what keeps the classification dialect-agnostic.Verification
gofmt -l .clean,go vet ./...clean,go test ./...all packages pass,go test -raceclean overproxyandmetrics(Go 1.26.4, eval box).Four new tests, all revert-verified against a mutation collapsing the classification back to one undifferentiated miss:
TestEveryUsageOutcomeIsDistinguishable— 12 shapes, one per outcome per transport, incl. Bedrock camelCase and nestedresponse.usageTestOnlyAnAccountingOutageMovesTheUsageCounters— the rule itself: alertable cases move their own counter, all four benign shapes move neitherTestTheShapeRecordNamesTheDialectAndCarriesNoContent— answers the dialect question, once per process, no content and no valuesTestTheShapeRecordSaysWhenTheBytesWereNotAWholeDocument—valid_json=false, so a spliced window is not misread as a field-name gapThe no-content assertion was verified against a second mutation, because "the record does not contain this string" passes trivially when there is no record, and would also pass on a record that logged the body under a different key. With
usageShapeAttrschanged to log the body it fails on its own subject: "the shape record leaked response CONTENT, which is the one thing it must never do".Two contract tests updated as designed:
TestStatsShapeIsUnchanged(both fields added to the reviewed contract) andTestEverySnapshotFieldIsExportedOrExempt— the latter listed innotExportedWhyrather than read offs, for the reason that map's own preamble gives: the/statshandler fills these afterrenderMetricstakes its snapshot, so apromLineoffswould export a permanent 0 while passing the test, which is precisely the silent-zero failure this change exists to report.What this unblocks
One request through a proxy on this branch produces the key-shape record, which answers the camelCase question that #199 and the
parseUsagedialect fix are both waiting on — and #201's cost claim, which is unmeasurable while the three cache-token fields read 0. Per the validation session, no captured body or usage shape exists anywhere today (the capture hop's flap log records request metadata only; LOCA's 11 MB log has none of the twelve candidate spellings), so this is the cheapest available instrument.🤖 Generated with Claude Code