Repository navigation
Fix #881 #882 #883: Stop counting session cost twice - #897
chrisdhaan wants to merge 5 commits into
Conversation
- #881: LocalSessionAggregator folds only the part of the persisted cost beyond what it already checkpointed, so its own checkpoint is not re-added on the first subagent turn. - #882: CostTracker keeps byte-size estimates in their own ledger, removes it on the first real parent token report, and persists it so a restart before that report doesn't keep it either. - #883: a --stdio engine no longer folds another session's exact token cost into its own CostTracker total or model usage. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Take main's version files and CHANGELOG, and carry the release note as .changeset/stop-double-counting-session-cost.md instead. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Claude claimed to find a bug with how the other properties are handled:
Latest-wins works for the ledger itself. But estimatedCostUsd (L597), costByDayUsd (L488) and tokensInput (L604) still merge by max. Once a checkpoint has written the estimate, the smaller total after the real report can't replace it. I ran this sequence against the branch: recordEstimatedTokens → saveSession → parent recordTokenUsage → saveSession → loadSession → seedFromPersisted into a fresh tracker.
estimate 0.105 | real 0.0105 | in-memory 0.0105
on disk 0.105 | byteEstimates.costUsd on disk 0
after restart seed 0.105
The file keeps the estimate while its ledger reads zero, so after a restart nothing can take the estimate out. With the 30s checkpoint, this is the normal path. Could the max-merged fields hold exact-only totals, with the ledger added back on load and seed? Then a shrinking ledger never competes with max.
There was a problem hiding this comment.
Confirmed and fixed in 2f16536. mergeSummaries now works out how much the incoming ledger shrank relative to the existing one and subtracts that from the existing estimatedCostUsd, tokensInput, tokensOutput and costByDayUsd before the max, so the on-disk fields keep their meaning and no reader has to add the ledger back. modelBreakdown needed no change because ModelUsageTracker never records estimates. The new SessionStore tests (see the test thread) fail without the mergeSummaries change and pass with it.
There was a problem hiding this comment.
Thanks, this fixes the single-writer case. I re-ran my sequence and the total is $0.0105 on disk and after a restart.
There's a case it gets wrong, though. The shrink comes out of whichever writer's figure won the max, and that isn't always the writer that holds the ledger. If another process (a --local dashboard or a provisional --stdio engine) has saved a larger exact total for the session, the estimate gets subtracted from that total instead:
estimate 0.105 | real 0.0105 | other writer's exact total 0.2
on disk 0.095 | after restart 0.095 (previous commit: 0.2)
The file doesn't record which writer's figure is the max, so I don't think mergeSummaries can tell these cases apart on its own. What if the max-merged fields held exact-only totals, with the ledger stored next to them and added in by whatever reads the total? Then another writer's larger figure wins the max on its own, and a shrinking ledger never touches it.
There was a problem hiding this comment.
Good catch. Fixed in latest commit.
| this.reportCount++; | ||
| // A subagent report says nothing about the parent's own turns, which is | ||
| // what the byte estimates stand in for. | ||
| if (ctx?.agentId === undefined) this.discardByteEstimates(); |
There was a problem hiding this comment.
This seems to solve the problem for the parent transcript watcher case that replays the transcript, but for the self-report tool (nr_observe_report_tokens) case (all harnesses but Claude Code) - which has no agentId and only reports the most recent API response - this seems like it'd discard those estimates.
Tested that path on the branch and 5 estimated turns ($0.105), then 1 self-reported turn ($0.021), leaves a total of $0.021 vs $0.126 on main.
Could the discard be limited to watcher reports? For example, onTokenEvent could set a flag on TokenRecordContext that handleReportTokens doesn't set.
There was a problem hiding this comment.
You're right, and this is fixed in 2f16536. The discard now needs a new replacesEstimates flag on TokenRecordContext, which only onTokenEvent (the transcript watcher) sets. handleReportTokens doesn't set it, so a self-report keeps the estimates for earlier turns. A test was added that records an estimated turn and then a self-report and checks both costs remain, and updated the existing ledger tests to pass the flag.
There was a problem hiding this comment.
Confirmed: 5 estimates plus 1 self-report now gives $0.126, the same as main.
One thing about the doc comment on replacesEstimates. It says only the transcript watcher sets the flag, but onTokenEvent handles every mode: 'token' source. CopilotUsageWatcher and the Copilot SDK extension both qualify. CopilotAppUsageWatcher mostly does too, because its first delta covers the whole backlog. The exception is when it skips a first-seen backlog that's over the tier threshold (copilot-app-usage-watcher.ts:290). Its next delta still drops the estimates, and the skipped turns are never reported. That case is narrow enough that updating the comment to list the actual sources seems like enough. Unless you'd rather that watcher not set the flag after a skip?
There was a problem hiding this comment.
Comment updated in latest commit.
…eign token events - Discard byte estimates only for transcript-watcher reports (replacesEstimates), so a single-response self-report keeps earlier turns' estimates - Take a shrunken byte-estimate ledger back out of the max-merged totals in mergeSummaries so a checkpoint cannot resurrect the estimate after a restart - Gate AiTurnCost emission on the same own-session check as CostTracker, since the event is stamped with the engine's session id - Cover the foreign-session decision with isTokenEventForThisProcess tests Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@jgibson02 this is ready for another review |
Summary
LocalSessionAggregatorfolds only the part of the session file's cost that exceeds what it last emitted viatoSummaries(). Its own 30s checkpoint is no longer re-added on the first subagent turn.CostTrackerkeeps byte-size estimates in their ownByteEstimateLedger. The first real parent token report removes it from the totals. Subagent reports don't, and platforms that never report tokens keep their estimate. The ledger is saved on the session summary asbyteEstimates(latest writer wins on merge, written all-zero once cleared so a stale copy is overwritten) and restored byseedFromPersisted, which counts as a report and so can't carry this itself.--stdiomode,onTokenEventno longer records another session's exact token cost intoCostTrackerorModelUsageTracker. It is priced via a new statelessCostTracker.priceUsageand still goes to that session's aggregator rollup.--localis unchanged.Test plan
npm testpasses after the version bump (214 suites, 8792 tests)npm run lint,npm run format:check,tsc --noEmit(main and web)Fixes #881
Fixes #882
Fixes #883