Skip to content

fix(dash): cap summarize credit at its span and price the first removal as a read - #546

Merged
amiddavid merged 4 commits into
mainfrom
feat/summarize-credit
Oct 15, 2026
Merged

amiddavid merged 4 commits into
mainfrom
feat/summarize-credit

Conversation

@amiddavid

@amiddavid amiddavid commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The dashboard credited summarize with $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

  1. Every summarizer is covered. One list, 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 run cache_aware_summarizer. Each summarizer has its own ledger. The headline is the sum.
  2. First removal is priced as a read when the cache was live. Event.uniqueRate (new) uses the cache-read rate for a summary's first removal when the verdict is hit or prefix_change. It keeps the write rate for ttl_expiry and cold_start. Other components keep the write rate: their content is new.
  3. One ledger caps the credit. SummarizeLedger (new, dash/compactepisode.go) sorts every summarize credit row:
    • Inside the span (ceiling − fill = 0.10 of the window): counted.
    • After the span closed, or after the client compacted: not credited.
    • A replay that missed with prefix_change inside a span: not credited. Its write is charged.
    • The headline, /api/components and the episode view all read this ledger.
    • The client-compaction check compares a request with the previous one on the same thread. A subagent's small request no longer ends the span.
    • The episode view no longer voids an episode when the client compacts. Credit up to that turn counts. New state: closed_client_compaction.
  4. Summarize's own LLM cost has an owner. /api/components now attributes cg_llm_cost_usd of requests where summarize ran and no extraction call owns the cost.
  5. The ttl_expiry verdict 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.Price stores 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):

  • 8621: real request, hit.
  • 8622: keep-alive ping, 296 s later. It read 412k tokens, so the cache was alive.
  • 8623: real request, 31 s after the ping. The gap from 8621 was 327 s, over the 300 s lifetime. The verdict said ttl_expiry.
  • 8623 really missed because a summary rewrote the prefix (352k → 65k tokens). The right verdict is 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

  • New unit tests for each fix (dash/summarizeattr_test.go, dash/compactepisode_test.go, proxy/entryage_test.go).
  • Mutation checks. Each mutation made the new tests fail:
    • uniqueRate always returns the write rate.
    • entryAgeMs ignores the ping.
    • Past-span credit is not removed.
    • prefix_change replay is credited.
    • Summarizer cost not attributed.
  • go test -race ./dash/... and go test -race ./proxy/... pass on the eval box, after rebase on main.
  • Golden files updated: new fields only (ledger, summarize_correction_usd).
  • Tests run for each name on the list: first-removal rate, ledger, span cap, cost attribution, episode view. Mutation: drop cache_aware_summarizer from the list, and the tests fail. Make the compaction check ignore threads, and the subagent test fails.
  • A test through capture.finish pins the fix 4 wiring (stored verdict with and without a ping refresh). The age >= 0 guard has its own test. Mutations of finish, noteKeepAlive and the guard all fail a test.
  • Not covered: the one call in proxy.go that hands arriveAt to noteKeepAlive. A test needs a live ping past the cache lifetime.
  • No live runs.

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 from snapshotPricer, so a few episode figures differ slightly from the live API.

Figure Before After
summarize saved $84.17 $16.70 (first removal $1.01, replay $15.68)
summarize LLM cost $0.00 (0 calls) $1.85 (7 calls)
summarize net $84.17 $14.85
extract_llm saved / net $45.41 / $45.41 unchanged (see Open questions)
extract_llm_sweep saved / LLM cost / net $2.37 / $4.52 / −$2.15 unchanged
Headline net from compaction (/api/stats: total_saved − keepalive_net) $134.99 (181.61 − 46.62) $67.51 (114.14 − 46.62)
Episode view, counted net −$0.00 (1 closed); 5 voided at $0 +$14.98 (1 closed, 5 client-compacted)

The headline row is read from the real /api/stats handler run on the snapshot. It gives $67.51 with and without lean=1, and with and without since=. The field summarize_correction_usd shows −$67.47.

Where the −$67.47 on the headline comes from (ledger):

  • Credit after the span ended: −$55.12
  • First removal priced as a read: about −$11.1 (stored first removal about $12.2, now $1.01)
  • prefix_change replay inside a span: −$1.21

Only 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

  • Second summary inside an open episode (request 15818 inside 15653's episode). It rolls the span forward. The span does not restart and does not grow. Its cost and its write are charged to the same episode. Its first removal is credited at the corrected rate. This is the rule the episode view already had; the ledger now follows it too.
  • Does the cap apply to 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_llm has no fill threshold. A rule needs a decision from the owner.
  • Credit with no summary event. One session has 296 replay turns ($30.82) from a summary older than the events. The ledger anchors a span at the first turn it can see. This may over-credit the part of the span that came before. The episode view does not list it, because there is no t0 to cost.
  • Other consumers. These still read the stored credit and do not apply the ledger: /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.
  • The ledger loads only sessions where summarize ever acted. The full /api/stats and /api/components run it. OverviewLean runs it only when the filter names a session.
  • The async path puts the unique part of a summary on the turn that replays the checkpoint, one turn after 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.
  • EpisodeVoided and the voided fields stay in the JSON for compatibility. They are always zero now.
  • Cost attribution counts one call per request. A request owned by an extraction call is left to that component.
  • The summarize-cost query matches strategy NOT GLOB '*_billed' as the existing extraction code does.
  • The session filter is an uncorrelated IN. A correlated EXISTS made the ledger take about 60 s on the snapshot; it now takes 3 s.
  • proxy/keepalive.go: arrive is now a thin wrapper over arriveAt. After the rebase, arriveAt also carries the in-flight count that main added to arrive.

Assisted-By: Claude

Closes #549

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Bug: a replay on a "summary started" turn is counted in full as a first removal

Where: dash/compactepisode.go:842-844 (creditTurn).

What breaks

  • When fresh is true and r.FirstUSD == 0, the code sets first = r.SavedUSD.
  • That puts the whole row into FirstRemovalUSD. It skips the verdict rules and the span cap.
  • A turn that commissions a new summary often also replays the old one. It has SavedUnique == 0, so FirstUSD is 0.
  • The commissioning turn often has verdict ttl_expiry. The old replay then counts at the write rate. On prefix_change it counts too, although that verdict is meant to be excluded.

How I know (test run on the eval box, temporary, not pushed)
Rows: commission, replay with a first removal of 500k, one row that closes the span, one past-span row, then a "summary started" row that replays 500k with ttl_expiry.

  • Result: first = 0.675, past_span = 0.05.
  • Right answer: first = 0.05 (the one real first removal).
  • The extra 0.625 is 500k at the write rate (1.25e-6) on a replay row.
  • With prefix_change on the last row, first is 0.10 instead of 0.05.

The same applies to a row with no events and Acted set. isFreshSummary returns "inferred" for it, so any such replay row counts in full as a first removal.

Why it matters

  • It is the same error as problem 1 (write rate on content the turn before already paid for), now on the commissioning turn.
  • It repeats once per summary.
  • It also gives the headline and the Components tab a credit that sits outside the span.

Proposal

  • Use first = r.SavedUSD only when the row has a unique part (SavedUnique > 0).
  • Send the rest of a fresh row through the same verdict and span rules as any replay row.

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Bug: an empty or partial ledger wipes the summarize row's first-removal and replay split

Where: dash/summarizeattr.go:108-126 (applySummarizeLedgerToRow), called from dash/api.go (components).

What breaks

  • The function always overwrites SavedUSDFirstRemoval, SavedUSDReplay, SavedUSDDecomposed, NetUSDFirstRemoval and ReplayMultiple from the ledger.
  • It does not check that the ledger saw any rows.
  • If the ledger is empty, the row loses the split that decomposeFromGroups computed. SavedUSD stays at the stored value, because the delta is 0.
  • An empty ledger happens when the model window is a guess (WindowUnknown), when the trigger is above the client ceiling (NoAttributableSpan), or when a.windows is nil. The PR says these keep "the stored credit". Only saved_usd does.

How I know (test run on the eval box, temporary, not pushed)
A row with saved=5, first=1, replay=4, decomposed=5, net_first=0.9, passed through applySummarizeLedgerToRow(row, &SummarizeLedger{}):

  • Result: saved=5 first=0 replay=0 decomposed=0 net_first=-0.1 mult=0.
  • So the UI shows saved_usd = 5 but decomposed = 0, and the first-removal verdict reads as net negative.

The partial case has the same fault. If some sessions are in the ledger and others are not, decomposed covers only the ledger sessions, while saved_usd covers all of them.

Proposal

  • Apply the decomposition fields only for the sessions the ledger covers. Or add the ledger split to the existing split instead of replacing it: first += l.FirstRemovalUSD - (stored first share of covered rows).
  • At minimum, skip the overwrite when l.Rows == 0.
  • Add a test that runs applySummarizeLedgerToRow with an empty ledger. The current tests only check that the ledger itself is empty.

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Note: other surfaces still read the uncorrected summarize credit

Not a blocker. This could make two numbers disagree on screen.

  • /api/stats?lean=1 with a time window and no session= is not corrected (dash/api.go:1233 only runs the ledger when f.Session != ""). The status line then shows a different total than the dashboard for the same window.
  • Per-session saved (dash/query.go:613), the series buckets (dash/query.go:1395), the per-group totals (dash/query.go:1549) and dash/savings.go all use the stored baseline_cost_usd. They sum to more than the corrected headline when summarize credit is capped.

If this is on purpose, one line in the PR body would help.

Checked and fine:

  • entryAgeMs and arriveAt: refreshedAt is read under k.mu. The inflight bookkeeping is unchanged.
  • summarizeSessionsOnly: tenant scoped. The cost is linear in rows (2k, 4k, 8k rows took 4, 8, 17 ms).
  • rePriceRow: gives the same result as Event.Price for rows written under the new rule.
  • summarizerOwnCost: the NOT EXISTS guard avoids double counting with extraction calls.

amiddavid added a commit that referenced this pull request Oct 15, 2026
…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>
@amiddavid

Copy link
Copy Markdown
Collaborator Author

Round 2: no blocking issues in d09ffbf

Both earlier bugs are fixed. What I checked:

  • Random walk test (3000 random 12-row sessions: commissioning rows, unique plus replay parts, client compaction, all five verdicts). Every credit row is counted once. stored == counted + past_span + prefix_change held in every case. ledger.Rows always matched the number of credit rows. The test was temporary and is not pushed.
  • Opening row with a unique part and a replay part: only the unique part counts. The replay part goes to past-span.
  • Closing plus opening row: credited once, by the closing episode.
  • Empty and partial ledger: the row is left alone when l.Rows == 0. For a partial ledger, saved_usd equals first plus replay plus the uncovered stored credit.
  • Targeted dash tests pass on the eval box.

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.

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Round 3 (98d1b0a): no bugs found. The row-value IN returns the same rows as the old EXISTS form (tested with two tenants, an empty session id and a 30k-row session). tenant_id and session_id are NOT NULL, so there is no NULL case. Tenant scoping holds, and the dataset query matches. The new form took 0.3 ms against 112 ms for the old one on that table. Targeted dash tests pass on the eval box.

@OsherElhadad

Copy link
Copy Markdown
Collaborator

Review of 98d1b0a

Verdict: 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

  • go test -race ./dash/... ./proxy/... at the head: both pass (dash 516 s, proxy 521 s). The proxy package needs -timeout 45m on a loaded machine. With the default 10 m it hit the timeout in TestOversizedAgentCompactionRequestSucceeds, which this PR does not touch.
  • Real Claude Code 2.1.299 sessions through a proxy built at the head, with a proxy built at the merge base (46c46d5) as the "before". Model: gpt-5-mini on Azure, through a LiteLLM shim. Each session is the same scripted task: 60 cat calls of 2.5k-token files, one call per turn, no commentary.
  • Settings I changed, so the numbers are not read as more than they are: the price table gives the model an exact window of 62,500, and the client is started with CLAUDE_AUTOCOMPACT_PCT_OVERRIDE=35, which makes it compact at about 61.5k billed input, i.e. about 1.0 of that window. summarize runs at its defaults (fires at 0.90, model.source: incoming). OFF is the same proxy with an empty pipeline.
  • "Measured" is the provider usage log, summed per session, including the summarizer's own calls. Spend for the whole task was $2.25.

1. The arithmetic is right

Check Result
Independent recompute from raw rows (own script, written from the PR text, none of its code) vs the PR's ledger equal to 6 decimals on the two sessions I read back through the API, one written by each binary (e.g. counted $0.007630 / $0.007630; past-span $0.062274 / $0.062274). The other three ON sessions I checked with the script only.
Same script with main's rule vs what the merge-base binary actually stored $0.104236 / $0.104236
Removed tokens on the first-removal turn: ledger vs billed difference to the OFF run 50,851 vs 51.9k to 52.2k (the tokenizer count is 2% low for this model)
Credited rows (first removal + 1 replay): tokens × read rate vs ledger $0.00256 to $0.00258 vs $0.00254
Same rows, merge-base binary vs head binary, headline "summarize net" $0.0966 vs $0.00004

2. Credit vs measured saving (3 matched ON/OFF pairs)

Pair OFF bill ON bill Measured saving Credit before (merge base) Credit after (this PR) Summarizer cost
1 $0.1930 $0.1708 $0.0222 $0.1272 $0.0076 $0.0090
2 $0.1874 $0.1559 $0.0315 $0.1044 $0.0076 $0.0089
3 $0.1945 $0.1786 $0.0158 $0.1157 $0.0076 $0.0082
mean (sample sd) $0.1916 $0.1684 $0.0232 ($0.0079) $0.1158 $0.0076 $0.0087
  • Credit / measured: before 5.7, 3.3, 7.3 (mean 5.5). After 0.34, 0.24, 0.48 (mean 0.36).
  • Against the mean of 4 OFF runs, and including a 4th ON run: measured $0.0376 (sd $0.0135), before 3.0×, after 0.20×.
  • A fifth ON run is excluded: the client process exited while it was compacting, so that session never finished its 60 turns.
  • So the old figure over-credits by 3 to 7×, the new one under-credits by 2 to 4×, and the sign of the net changes: after − summarizer cost the dashboard says about −$0.0011 per session, while the bill says +$0.023.
  • Why the new figure is small: the cap ends the credit two turns after the first removal (span = 6.25k new tokens). That cap assumes the client would compact at about 1.00 anyway. In my OFF runs it did not. In one OFF run the client compacted once at 62k and then grew to 113k without compacting again. In the other three it tried three times, got "summarization produced empty response" from this model, and stopped trying (context reached 154k). This is a property of the model and client here and not of the PR, but it shows the ceiling is an assumption, not something the dashboard measured. If the owner keeps the 1.00 rule, I would say so on the figure.
  • Inside the span, in dollars: the two credited rows saved $-0.00001, +$0.00264, $-0.00075, +$0.00160 in the four ON runs (mean +$0.0009 vs credit $0.0025). Azure's cache hit rate moves one row by up to $0.01, so this does not resolve a $0.0025 credit. The token-level check in section 1 does, and it agrees. The first-removal turn also pays fresh tokens for the rewritten prefix, which the headline credit does not net.

3. Fix 4 (ttl_expiry verdict), reproduced live

Real 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.

Binary Verdict on B
merge base ttl_expiry
head prefix_change

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)

  1. Scope: only the component named summarize. dash/event.go:772, SQL literals at dash/compactepisode.go:1159, :1167, dash/summarizeattr.go:156. The medium/high/xhigh presets run cache_aware_summarizer, and its rows carry that name (checked in a live run; it did not splice a summary within my session, so I could not show its credit). Its first removal is still priced at the write rate (uniqueRate falls through to p.CacheWrite), and it is outside the ledger and the cost attribution. If those tiers are deployed, the 3 to 7× over-credit stays for them. Worth an issue, or a name list instead of a constant.
  2. The client-compaction detector is not thread-aware. dash/compactepisode.go:639 and :647, with conversations keyed on (tenant, session, model) at :1045. A subagent or auxiliary request on the same model has a small tokens_before, which reads as "the client compacted": the span ends and the main thread's remaining in-span credit moves to past-span. Unit repro (a test I wrote and did not push): one interleaved small row halves the counted credit (0.02 → 0.01). The direction is conservative, but the headline now removes money on this signal, so I would key it on the thread, or require the drop to hold on the next row.
  3. The wiring of fix 4 has no test. proxy/dashcapture.go:500 and proxy/proxy.go:1249. Replacing c.kaRefreshedAt with a zero time in finish passes the whole ./proxy suite; only entryAgeMs and arriveAt are tested apart. A test through capture.finish would pin it. The shipped wiring is correct (section 3).
  4. Fix 4 edge cases. proxy/keepalive.go:1538 and :1776 move the refresh time only for a ping that read (CacheRead > 0). A ping that missed also re-created the entry and restarted the clock; the next miss still says ttl_expiry. Rare at a 270 s idle setting. The age >= 0 guard in entryAgeMs (proxy/dashcapture.go:577) is not pinned by a test (mutant survives).
  5. Non-additive over windows. The ledger runs per filter, so a since= window that cuts a span restarts the anchor for the "no summary event" case. Credit over two adjacent windows will not sum to the credit over their union. Rows stored before this PR keep their old verdict, and the first-removal part is re-priced at today's rates.

5. Mutation checks (own, on the head)

Mutation Caught?
uniqueRate returns the write rate yes (summarizeattr_test.go:128)
span never closes (span*1000) yes
baselineDeltaUSD ignores the summarize unique part yes (summarizeattr_test.go:40)
own-cost query drops the NOT EXISTS extraction_calls guard yes (summarizeattr_test.go:204)
age >= 0 guard removed in entryAgeMs no
finish passes a zero refresh time to entryAgeMs no (whole ./proxy suite)
proxy.go passes a zero refresh time to noteKeepAlive no (targeted tests)

…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>
@amiddavid
amiddavid force-pushed the feat/summarize-credit branch from 98d1b0a to 1b652f7 Compare October 15, 2026 14:12
@amiddavid

Copy link
Copy Markdown
Collaborator Author

Thank you for the review. Changes in 1b652f7 (rebased on main):

  • Finding 1 (scope). One list, dash.SummarizerComponents, now has summarize and cache_aware_summarizer. The first-removal rate, the baseline split, the SQL (one 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. Tests run each name through the rate, the ledger, the span cap, the cost and the episode view. Dropping a name from the list fails them.
  • Finding 2 (thread). The client-compaction check now compares a request with the previous one on the same thread. Test: an interleaved subagent row no longer ends the span. A drop on the main thread still does.
  • Finding 3 (wiring). noteKeepAlive takes the keeper's arrival whole. A test through capture.finish checks the stored verdict with and without a ping refresh. The age >= 0 guard has a test. Mutations of finish, noteKeepAlive and the guard fail. The single call in proxy.go is not covered by a test.
  • Findings 4 and 5: not in this PR.
  • go test -race ./dash/... and ./proxy/... pass on the box. Summarize numbers on the snapshot are unchanged ($16.70, headline $67.51).

@amiddavid

Copy link
Copy Markdown
Collaborator Author

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 amiddavid left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-diff of the old head against 5f159c97..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/ui JS and HTML, context-guru-plugin, metrics, tenant, deploy. No place still matches only summarize as a component name.
  • Other matches are text, log lines, or already cover both names (adaptcompaction.go uses Contains(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, StoredUSD and the credits are per component, so the sum cannot count a row twice.
  • No row multiplication. One LEFT JOIN per name, each on (request_id, component). A request has one request_components row 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. IsSummarizer guards 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 from requests.thread_id. It is NOT 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/thread gives 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.
  • arriveAt is tested apart, and finish is tested from noteKeepAlive on.
  • 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.70 or $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-review on box 1 was already gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

dash: summarize savings are overstated — 4 problems in how the credit is priced

2 participants