Repository navigation
fix(dash): cap summarize credit at its span and price the first removal as a read - #546
Conversation
Bug: a replay on a "summary started" turn is counted in full as a first removalWhere: What breaks
How I know (test run on the eval box, temporary, not pushed)
The same applies to a row with no events and Why it matters
Proposal
|
Bug: an empty or partial ledger wipes the summarize row's first-removal and replay splitWhere: What breaks
How I know (test run on the eval box, temporary, not pushed)
The partial case has the same fault. If some sessions are in the ledger and others are not, Proposal
|
Note: other surfaces still read the uncorrected summarize creditNot a blocker. This could make two numbers disagree on screen.
If this is on purpose, one line in the PR body would help. Checked and fine:
|
…an empty ledger Review round 1 on #546. - creditTurn counted a whole fresh-summary row as a first removal. A turn that commissions a new summary often also replays the old one. That replay now follows the verdict and span rules. On the row that opens an episode, only the first removal is the new summary's. The rest is past the old summary's span. - applySummarizeLedgerToRow overwrote the first-removal and replay split even when the ledger saw no rows. It now leaves the row alone then. For a partial ledger, the stored credit the ledger did not see stays in the replay column. Assisted-By: Claude Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Round 2: no blocking issues in d09ffbfBoth earlier bugs are fixed. What I checked:
Minor, no action needed: the opening row's replay part is always sent to past-span. If the row opens an episode while an older ghost span is still active, that replay is under-counted. This errs on the safe side. |
|
Round 3 (98d1b0a): no bugs found. The row-value |
Review of 98d1b0aVerdict: approve. I found no blocker in the four fixes. Each one does what the PR says, and I checked the arithmetic against real Claude Code sessions. Items 1 and 2 below should be tracked before anyone reads the headline as a decision number. What I ran
1. The arithmetic is right
2. Credit vs measured saving (3 matched ON/OFF pairs)
3. Fix 4 (ttl_expiry verdict), reproduced liveReal keep-alive ping, stub upstream that models a 300 s cache refreshed on every read. Request A, ping 272 s later (read 22.6k tokens), request B 327 s after A with a different system prefix and the same message prefix.
Azure's prefix cache has no TTL edge, so this could not be shown on the Azure runs. The stub run uses the real keeper, the real ping and the real capture path. 4. Findings (none blocking)
5. Mutation checks (own, on the head)
|
…al as a read The dashboard credited summarize with $84.17 on a local snapshot. A capped count gives $16.70. Four problems caused the gap. - First removal: Event.Price used the cache-write rate for content the previous turn had already read from cache. Event.uniqueRate now uses the read rate when that entry was live (hit, prefix_change) and keeps the write rate when it had lapsed (ttl_expiry, cold_start). - Attribution cap: the headline and /api/components credited every replay turn with no end. A SummarizeLedger now sorts each credit row. Rows inside span = ceiling - fill count. Rows after the span closed or after the client compacted do not. The headline, the component row and the episode view read the same ledger. The episode view no longer voids an episode the client compacted: credit up to that turn counts. - Own cost: summarize records no extraction_calls rows, so its model spend had no owner. /api/components now attributes it. - Verdict: a keep-alive ping that read the cache moved the provider clock, but the idle gap ran from the last real request. A turn 31 s after a ping read as ttl_expiry. The keeper now reports when the entry was last refreshed, and the verdict uses that age. Assisted-By: Claude Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…an empty ledger Review round 1 on #546. - creditTurn counted a whole fresh-summary row as a first removal. A turn that commissions a new summary often also replays the old one. That replay now follows the verdict and span rules. On the row that opens an episode, only the first removal is the new summary's. The rest is past the old summary's span. - applySummarizeLedgerToRow overwrote the first-removal and replay split even when the ledger saw no rows. It now leaves the row alone then. For a partial ledger, the stored credit the ledger did not see stays in the replay column. Assisted-By: Claude Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…tats for a window - The session filter used a correlated EXISTS per request row. On a 13.5k-row snapshot the ledger took about 60 s and /api/stats timed out. It is now an uncorrelated IN, and the ledger takes 3 s. - /api/stats?lean=1 with a time window and no session now applies the ledger too, so the status line agrees with the dashboard. Assisted-By: Claude Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…t-compaction detector thread-aware Review of #546 (Osher). - The fixes covered only the component named "summarize". The medium, high and xhigh presets run "cache_aware_summarizer", whose first removal was still priced as a write and which sat outside the ledger and the cost attribution. There is now ONE list, dash.SummarizerComponents. The first-removal rate, the baseline split, the SQL (one LEFT JOIN per listed name, and the session filter), the ledger (one per component, the headline is their sum) and the cost attribution all read it. - The client-compaction detector compared a request with the previous one on the same model. A small subagent request read as the client compacting, and ended the span. It now compares on the same thread. - fix 4 had no test through finish(). noteKeepAlive now takes the keeper's kaArrival whole, and a test checks the stored verdict through capture.finish. The age >= 0 guard in entryAgeMs is pinned. Assisted-By: Claude Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
98d1b0a to
1b652f7
Compare
|
Thank you for the review. Changes in 1b652f7 (rebased on main):
|
|
One more point on section 2 of the review (credit against measured saving). The repo owner decided to keep the 1.00 client-ceiling assumption. The cap and the default do not change in this PR. The review shows the ceiling is an assumption and not something the dashboard measures. Your runs show the client did not compact at about 1.00 in the OFF runs. The PR keeps the rule as the owner set it. The figure is a capped credit under that rule. |
amiddavid
left a comment
There was a problem hiding this comment.
Review of 1b652f7 (delta since 98d1b0a)
Verdict: approve. I found no blocker. The rebase lost nothing, and the new commit does what the author says.
What changed
git range-diffof the old head against5f159c97..1b652f75: commits 1 to 3 are identical (=). Only commit 4 (1b652f75) is new. So main's merges (#500, #542, #530, #536) were not lost or altered.
What I ran (eval box 1, foreground, race on)
go vet ./dash/... ./proxy/...: clean.go test -race -timeout 45m ./dash/...: pass (427 s).go test -race -timeout 45m ./proxy/...: pass (340 s).
Finding 1: one list for all summarizers. Complete.
- I grepped Go, SQL strings,
dash/uiJS and HTML,context-guru-plugin,metrics,tenant,deploy. No place still matches onlysummarizeas a component name. - Other matches are text, log lines, or already cover both names (
adaptcompaction.gousesContains(name, "summar")). - No double count. Each ledger reads one component's join. A ledger row is a row with that component's
saved_usd != 0.Rows,StoredUSDand the credits are per component, so the sum cannot count a row twice. - No row multiplication. One
LEFT JOINper name, each on(request_id, component). A request has onerequest_componentsrow per component. The schema has no UNIQUE on that pair (old, not new), but the writer makes one row per component. - Names go into the SQL by string splice.
IsSummarizerguards both places. Good. - The session filter picks sessions where any summarizer acted. The other component's walk sees those rows with zero credit. Harmless.
Finding 2: thread-aware detector. Correct.
- The key is
r.Thread, taken fromrequests.thread_id. It isNOT NULL DEFAULT '', so old rows scan as the main thread and act as before. - A first request on a new thread has
prev == 0, so it never counts as a compaction. internal/threadgives the post-compaction request the compacting thread's id. So a real client compaction stays on the main thread and is still caught.
Finding 3: wiring tests. They fail when the wiring breaks.
| Mutation | Result |
|---|---|
finish passes a zero refresh time |
caught (TestFinishUsesThePingRefreshTimeForTheStoredVerdict) |
noteKeepAlive drops refreshedAt |
caught (same test) |
age >= 0 guard removed |
caught (TestAFutureRefreshTimeNeverShortensTheGap) |
| detector key ignores the thread | caught (TestASubagentRequestIsNotAClientCompaction) |
list holds only summarize |
caught |
uniqueRate matches only summarize |
caught |
sumLedgers reads only the first name |
caught |
| own-cost loop reads only the first name | caught |
Known gap: kaArrival{} at the proxy.go call site
- Acceptable. The call is now one expression:
cp.noteKeepAlive(h.keeper.arriveAt(...)). A field can no longer be dropped by accident. Only a deliberate edit of the argument survives. arriveAtis tested apart, andfinishis tested fromnoteKeepAliveon.- A fake-clock test of
chat()would cost more than it protects.
Non-blocking
- Snapshot figures not pinned. No test in the repo asserts
$16.70or$67.51. I did not check them: I may not copy the dashboard DB. They stay a claim of the author. - Cost. The ledger now runs one query and one walk per summarizer (2 today). The stats path does this on each call. Fine now. Watch it if the list grows.
- No mixed-session test. No test has both components in one session. I found no double count by reading the code. A small test would pin it.
- Leftover box dir.
~/summarize-credit-reviewon box 1 was already gone.
Summary
The dashboard credited
summarizewith $84.17 on a local snapshot. The right figure is $16.70. The headline "net from compaction" drops from $134.99 to $67.51.This PR fixes 4 accounting problems. Problem 2 is the largest. Problem 1 is second.
Problem statement and evidence: the issue text in
/tmp/cg-summarize-credit-issue.md(no GitHub issue number yet).What changed
dash.SummarizerComponents(summarize,cache_aware_summarizer), drives the first-removal rate, the baseline split, the SQL, the ledger and the cost attribution. The medium, high and xhigh presets runcache_aware_summarizer. Each summarizer has its own ledger. The headline is the sum.Event.uniqueRate(new) uses the cache-read rate for a summary's first removal when the verdict ishitorprefix_change. It keeps the write rate forttl_expiryandcold_start. Other components keep the write rate: their content is new.SummarizeLedger(new,dash/compactepisode.go) sorts every summarize credit row:prefix_changeinside a span: not credited. Its write is charged./api/componentsand the episode view all read this ledger.closed_client_compaction./api/componentsnow attributescg_llm_cost_usdof requests where summarize ran and no extraction call owns the cost.ttl_expiryverdict uses the real entry age. A keep-alive ping that read the cache moves the provider clock. The idle gap ran from the last real request. The keeper now reports the last refresh time (arriveAt), and the verdict uses it.One pricing path:
Event.ComponentSavedUSD.Event.Pricestores it. The read side calls it again on stored rows. The episode view no longer prices turns on its own.Why problem 4 happens
Requests 8621, 8622, 8623 (session
6c456ae0):hit.ttl_expiry.prefix_change.Requests 9818 / 9827 have the same shape. Rows already stored keep their old verdict. New rows get the right one.
How it was tested
dash/summarizeattr_test.go,dash/compactepisode_test.go,proxy/entryage_test.go).uniqueRatealways returns the write rate.entryAgeMsignores the ping.prefix_changereplay is credited.go test -race ./dash/...andgo test -race ./proxy/...pass on the eval box, after rebase onmain.ledger,summarize_correction_usd).cache_aware_summarizerfrom the list, and the tests fail. Make the compaction check ignore threads, and the subagent test fails.capture.finishpins the fix 4 wiring (stored verdict with and without a ping refresh). Theage >= 0guard has its own test. Mutations offinish,noteKeepAliveand the guard all fail a test.proxy.gothat handsarriveAttonoteKeepAlive. A test needs a live ping past the cache lifetime.Before / after numbers
Snapshot of the live DB (13.5k requests, 2 sessions with summaries). "Before" is
origin/main, "after" is this branch, same snapshot, same harness. Rates come fromsnapshotPricer, so a few episode figures differ slightly from the live API./api/stats: total_saved − keepalive_net)The headline row is read from the real
/api/statshandler run on the snapshot. It gives $67.51 with and withoutlean=1, and with and withoutsince=. The fieldsummarize_correction_usdshows −$67.47.Where the −$67.47 on the headline comes from (ledger):
prefix_changereplay inside a span: −$1.21Only the unique part of a row is a first removal. On the row that opens an episode, the replay of an older summary is past that older span.
The episode view credits ($1.01 + $5.93 + $9.75 = $16.69) match the ledger ($16.70). The $0.01 gap is rounding.
Open questions
extract_llm? Not changed. Its replay credit is $52.07 and it also ends when the client compacts. But the span (ceiling − fill) comes from summarize's trigger.extract_llmhas no fill threshold. A rule needs a decision from the owner./api/savings,SavingsTotals(status line), the metrics export, per-session saved, the series buckets and the per-group totals. They sum to more than the corrected headline. This is on purpose for now: the ledger needs a walk over the session rows, and each of these is a bounded query. Move them in a follow-up if you agree.Technical notes
dash/summarizeattr.go(new):SummarizeLedgerFor,ApplySummarizeLedger,applySummarizeLedgerToRow,rePriceRow,summarizerOwnCost. The read side only re-prices a row that has a unique part. A pure replay keeps its stored value and its historical rates./api/statsand/api/componentsrun it.OverviewLeanruns it only when the filter names a session.summary_started. The first removal is read from that row (FirstUSD), not from the event name. A first removal with no event before it opens an inferred episode.EpisodeVoidedand thevoidedfields stay in the JSON for compatibility. They are always zero now.strategy NOT GLOB '*_billed'as the existing extraction code does.IN. A correlatedEXISTSmade the ledger take about 60 s on the snapshot; it now takes 3 s.proxy/keepalive.go:arriveis now a thin wrapper overarriveAt. After the rebase,arriveAtalso carries the in-flight count thatmainadded toarrive.Assisted-By: Claude
Closes #549