(MOT-4375) fix(harness,providers): session-total cost in the chip; stop double-billing cached tokens - #740
Conversation
…I-shaped wires The Usage contract is `input` = the cache-MISS slice, disjoint from `cache_read` — pricing bills the splits additively. deepseek and anthropic report disjoint splits natively and were already correct. openai, kimi, zai, llamacpp, and openai-codex mapped the wire's `prompt_tokens` / `input_tokens` TOTAL — which includes the cached slice — straight into `input` while also setting `cache_read`, billing every cached token at the full input rate plus the cache rate. On an agent loop resending a large cached prefix every turn, reported cost read near-double, and the exactified context totals inflated the same way. The miss slice is now derived (`total - cached`) in each provider's usage merge, with the contract documented at the site. Test fixtures that pinned the double-count as expected are corrected — a 12-token prompt with 4 cached asserts 8 in, 4 cache_read — and a new codex test pins the subtraction for the Responses API shape. The openai `responses` fixture carries no cached slice, so its expectation stays the full total.
…and chip The chip's cost line showed the LAST generation step's bill. On providers with steep cache discounts that number legitimately swings two orders of magnitude between cache-miss and cache-hit steps, so during a live turn it read as a bouncing total, and the final (heavily cached, near-free) step read as what the whole session cost. `ContextSnapshotV1` gains `session_cost_usd`, accumulated at the snapshot write site on top of the stored snapshot's total — the turn loop is the session's only writer, so the read-back is race-free, and seeding from the store keeps the total honest across turns and harness restarts. Wire schema golden regenerated for the new field. The chip moves per-step cost onto the line it describes — `last step 6 in · output 28 · $0.0127` — and renders `session total $0.1415` as its own line that only grows, with a tooltip saying exactly why the step number swings and this one does not.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds cumulative session cost to context snapshots and displays it in the context UI. It also normalizes provider input usage by subtracting cached tokens and recording cache-read usage. ChangesSession cost tracking
Provider usage normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Generation
participant TurnLoop
participant ContextSnapshot
participant ContextPopover
Generation->>TurnLoop: report usage and cost
TurnLoop->>ContextSnapshot: accumulate session_cost_usd
ContextSnapshot->>ContextPopover: provide snapshot and step usage
ContextPopover->>ContextPopover: display step and session cost
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skill-check — worker0 verified, 55 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
harness/src/context_snapshot.rs (1)
428-428: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an omitted-field compatibility fixture.
The test only covers
session_cost_usd: Some(1.37). Add a pre-change JSON case without this field and assert that it deserializes toNone. Also verify thatNoneis omitted during serialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/src/context_snapshot.rs` at line 428, Add an omitted-field compatibility fixture alongside the existing session_cost_usd case in the relevant context snapshot test: deserialize pre-change JSON without session_cost_usd and assert the field is None, then serialize that value and verify session_cost_usd is omitted from the JSON output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@harness/src/turn_loop.rs`:
- Around line 742-759: Make the cumulative cost update around snapshot get/set
failure-safe and idempotent: preserve and propagate snapshot read or parse
errors instead of defaulting to zero, and serialize the read-modify-write using
the session lock or an atomic compare-and-set. Track the accounted generation
identified by `(turn_id, step)` and skip cost accumulation when that generation
was already applied, while retaining the existing total for missing or repeated
updates.
---
Nitpick comments:
In `@harness/src/context_snapshot.rs`:
- Line 428: Add an omitted-field compatibility fixture alongside the existing
session_cost_usd case in the relevant context snapshot test: deserialize
pre-change JSON without session_cost_usd and assert the field is None, then
serialize that value and verify session_cost_usd is omitted from the JSON
output.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2aaeb3a4-6ab6-45e7-991f-d2b8869d8cdc
⛔ Files ignored due to path filters (4)
provider-kimi/Cargo.lockis excluded by!**/*.lockprovider-llamacpp/Cargo.lockis excluded by!**/*.lockprovider-openai-codex/Cargo.lockis excluded by!**/*.lockprovider-openai/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
harness/src/context_snapshot.rsharness/src/turn_loop.rsharness/tests/golden/schemas/harness.metrics.jsonharness/ui/src/context-chip/index.tsxharness/ui/src/lib/metrics.tsprovider-kimi/src/sse.rsprovider-kimi/src/upstream.rsprovider-llamacpp/src/sse.rsprovider-llamacpp/src/upstream.rsprovider-openai-codex/src/sse.rsprovider-openai-codex/src/upstream.rsprovider-openai/src/sse.rsprovider-openai/src/upstream.rsprovider-zai/src/sse.rsprovider-zai/src/upstream.rs
| // Accumulate the session's running cost on top of the stored | ||
| // snapshot's total: the loop is the only writer per session, so the | ||
| // read-back is race-free, and seeding from the store keeps the total | ||
| // honest across turns and harness restarts. | ||
| let step_cost = outcome | ||
| .message | ||
| .usage | ||
| .as_ref() | ||
| .and_then(|u| u.cost_usd) | ||
| .unwrap_or(0.0); | ||
| let prior_cost = | ||
| crate::context_snapshot::get(&deps.iii, &record.session_id, cfg.session_timeout_ms) | ||
| .await | ||
| .ok() | ||
| .flatten() | ||
| .and_then(|prev| prev.session_cost_usd) | ||
| .unwrap_or(0.0); | ||
| snapshot.session_cost_usd = Some(prior_cost + step_cost); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make cumulative session-cost persistence failure-safe and idempotent.
The get(...).await.ok().flatten().unwrap_or(0.0) chain collapses a missing snapshot, a storage error, and a parse failure into the same zero value. If the read fails but the write succeeds, this code can replace the previous session total with only step_cost.
The read-modify-write also runs after Line 688 drops _guard and before Line 801 reacquires it. A replay after a restart can add the same turn_id and step again because this code does not check an already-accounted generation. Preserve read errors instead of writing a replacement total. Serialize the update with the session lock or an atomic compare-and-set, and make the (turn_id, step) update idempotent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/src/turn_loop.rs` around lines 742 - 759, Make the cumulative cost
update around snapshot get/set failure-safe and idempotent: preserve and
propagate snapshot read or parse errors instead of defaulting to zero, and
serialize the read-modify-write using the session lock or an atomic
compare-and-set. Track the accounted generation identified by `(turn_id, step)`
and skip cost accumulation when that generation was already applied, while
retaining the existing total for missing or repeated updates.
Same defect as the five providers in the previous commit, missed because xai is not on the running rig: both the chat-completions and Responses usage merges mapped the wire's prompt/input token TOTAL — which includes the cached slice — straight into `input` while also setting `cache_read`, billing every cached token twice. The miss slice is now derived (`total - cached`) at both sites, and the fixtures that pinned the double-count as expected (12 total with 4 cached) now assert 8 in. provider-claude-code audited in the same sweep: Anthropic wire reports disjoint splits natively, correct as-is. That completes all nine providers on main; groq gets the fix on draft #712 before it merges.
…e, nullable TS type Review follow-up. The session-cost seed swallowed state read errors into the first-step default: a transient failure reset the running total to the current step's cost and displayed the fabricated number. A failed read now leaves `session_cost_usd` unset for the step — the chip hides the line rather than lying — with a warning log. Absent-snapshot seeding (genuine first step) is unchanged. No lock is added around the read-modify-write: turns are serialized per session by the fifo harness-turn queue grouped on session_id, and steps run sequentially within a turn, so the loop is the session's only writer. The comment at the site now says why. The TS snapshot type declares `session_cost_usd` nullable to match the wire schema (`["number","null"]`); the chip already guarded with `!= null`.
1 participant
What
Two defects found while chasing a "deepseek tokenizer counting seems wrong" report. The tokenizer was fine; the money was not.
1. Per-step cost read as a bouncing session total
The context chip's cost line showed the last generation step's bill. On providers with steep cache discounts the per-step number legitimately swings two orders of magnitude between cache-miss and cache-hit steps — so during a live turn the line bounced ($0.06 → $0.04 → $0.09 → $0.00014), and the final near-free cached step read as what the whole session cost.
ContextSnapshotV1gainssession_cost_usd, accumulated at the snapshot write site on top of the stored snapshot's total. The turn loop is the session's only writer, so the read-back is race-free; seeding from the store keeps the total honest across turns and harness restarts. Wire-schema golden regenerated.last step 6 in · output 28 · $0.0127— and renderssession total $0.1415as its own only-grows line, with a tooltip explaining why the step number swings and this one does not.2. Cached tokens double-billed on six providers
The
Usagecontract:inputis the cache-miss slice, disjoint fromcache_read— pricing bills the splits additively. deepseek, anthropic, and claude-code report disjoint splits natively and were correct. openai, kimi, zai, llamacpp, openai-codex, and xai (both its chat-completions and Responses merges) mapped the wire'sprompt_tokens/input_tokenstotal (which includes the cached slice) straight intoinputwhile also settingcache_read— billing every cached token at the full input rate plus the cache rate. On an agent loop resending a large cached prefix every turn, reported cost read near-double, and exactified context totals inflated the same way.Each provider's usage merge now derives the miss slice (
total − cached), with the contract documented at the site. Fixtures that pinned the double-count as expected are corrected (12-token prompt with 4 cached asserts8 in · 4 cache_read); a new codex test pins the subtraction for the Responses API shape; the openairesponsesfixture has no cached slice, so its expectation correctly stays the full total.That covers all nine providers on main;
provider-groqis not on main yet (draft #712) and gets the same fix before it merges.Verification
Live on a rig: two turns in one session — step cost $0.1288 (cold) then $0.0127 (99% cache hit), session total accumulating to $0.1415, popover rendering both lines as designed. Per-step billed token sequence for the reporting deepseek session confirmed strictly monotonic (7.5k → 105k over 14 steps), ruling out the tokenizer. Gates on all six crates:
cargo fmt --check,cargo clippy --all-targets -D warnings, full test suites green.Follow-up (not in this PR)
The deepseek curated catalog claims a 1,000,000-token window for both v4 models. If v4-flash is a 128k-class model, context percentages understate ~8× and compaction fires late — needs a spec check (tracked in MOT-4375).
Linear: MOT-4375.