Repository navigation
fix(usage): include cache tokens in input token counts - #822
Conversation
|
|
||
| -- Update tokens_input to include cache tokens (only if not already done) | ||
| UPDATE usage_history | ||
| SET tokens_input = tokens_input + tokens_cache_read + tokens_cache_creation |
There was a problem hiding this comment.
WARNING: Migration logic may be flawed. The condition tokens_input < tokens_cache_read + tokens_cache_creation can produce incorrect results for edge cases:
- If base
input_tokenswas very small (e.g., 10) and cache_read is large (e.g., 4000), the condition10 < 4000 = truewould allow migration, but the sum would incorrectly double-add - If tokens_input already includes cache tokens correctly, but happens to be numerically smaller than the sum, this would add them again
Recommendation: Add a version flag or timestamp column to track whether migration has been applied, rather than trying to infer from the data values.
Code Review SummaryStatus: Issues Resolved | Recommendation: Merge Overview
Previous Issues - RESOLVEDAll issues from the previous review have been addressed:
Files Reviewed (7 files)
AssessmentThe PR correctly implements the fix for Claude token accounting per API documentation. All code changes are appropriate, and the migration logic is now correct. No new issues found. |
There was a problem hiding this comment.
Code Review
This pull request updates token accounting to correctly include Claude's cache tokens in the total prompt token count, refactors usage logging, and moves the CacheStatsCard to the cache dashboard. A critical issue was found in the database migration where the idempotency check in the WHERE clause is logically flawed and could cause the migration to skip records that require updates.
| UPDATE usage_history | ||
| SET tokens_input = tokens_input + tokens_cache_read + tokens_cache_creation | ||
| WHERE (tokens_cache_read > 0 OR tokens_cache_creation > 0) | ||
| AND tokens_input < tokens_cache_read + tokens_cache_creation; |
There was a problem hiding this comment.
The idempotency check in the WHERE clause is flawed and will cause the migration to silently skip some records that need to be updated.
The condition AND tokens_input < tokens_cache_read + tokens_cache_creation assumes that for any un-migrated record, the number of base input tokens is less than the sum of cached tokens. This assumption is not always valid. For example, a request could have a large number of new input tokens and only a small number of cached tokens, causing this condition to be false and preventing the record from being corrected.
Since the pull request description mentions that migrations are tracked in _omniroute_migrations, the migration runner likely ensures that each migration is executed only once. If that's the case, this manual idempotency check is not needed and should be removed to ensure all relevant historical records are fixed.
UPDATE usage_history
SET tokens_input = tokens_input + tokens_cache_read + tokens_cache_creation
WHERE (tokens_cache_read > 0 OR tokens_cache_creation > 0);ce14278 to
e9656c6
Compare
| @@ -88,7 +88,6 @@ export default function SettingsPage() { | |||
| <ThinkingBudgetTab /> | |||
There was a problem hiding this comment.
WARNING: Unused import - The CacheStatsCard import at line 20 remains but the component is no longer used here (moved to dashboard/cache/page.tsx). The import should be removed.
- Fix getLoggedInputTokens to return full prompt_tokens (input + cache_read + cache_creation) - Fix usageExtractor for non-streaming Claude responses to calculate total correctly - Add formatUsageLog helper to show CR=<cache_read> in logs - Add migration 012 to fix historical token counts in usage_history - Move prompt cache metrics from Settings to /dashboard/cache page Per Claude API docs: Total input tokens = input_tokens + cache_creation_input_tokens + cache_read_input_tokens Fixes issue where totalInputTokens (71k) was less than totalCacheCreationTokens (1.35M). Tested: - All 1134 unit tests pass - Cache metrics API returns correct totals - Migration is idempotent and tracked in _omniroute_migrations - Logs show cache read tokens: 'in=6055 | out=211 | CR=22399' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e9656c6 to
c6eadc5
Compare
Integration test was failing because CacheStatsCard was moved from settings page to cache page in previous commit. Split the test into two separate describe blocks for accurate page-specific verification. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks @tombii for this critical fix! 🎉 The token accounting improvement allows precise cost tracking. This has been approved and will be shipped in the incoming release. Highly appreciated! |
|
I'm just solving the merge conflict, or did you merge it already? |
|
Follow-up PR #835 adds UI polish for the cache page:\n- Consistent header sizing\n- Request count context in cache hit rate labels |
Summary
getLoggedInputTokensto return fullprompt_tokens(input + cache_read + cache_creation) per Claude API docsusageExtractorfor non-streaming Claude responses to calculate total correctlyformatUsageLoghelper to showCR=<cache_read>in logsusage_history/dashboard/cachepageProblem
Cache metrics showed
totalInputTokens(71,907) was less thantotalCacheCreationTokens(1,350,674), which is impossible. This was because:getLoggedInputTokens()was incorrectly subtracting cached tokens fromprompt_tokensSolution
Per Claude API documentation:
Code Fixes
src/lib/usage/tokenAccounting.ts: Return fullprompt_tokenswithout subtractionopen-sse/handlers/usageExtractor.ts: CalculatepromptTokens = inputTokens + cacheRead + cacheCreationopen-sse/handlers/chatCore.ts: Show cache read tokens in logs asCR=<value>012_fix_token_input_cache_tokens.sql(idempotent)t.rich()hydration error in cache pageUX Improvement
Moved
CacheStatsCardfrom/dashboard/settings?tab=aito/dashboard/cacheso all cache metrics (semantic + prompt cache) are in one unified location.Test Results
✅ All 1157 unit tests pass
✅ Cache metrics API returns correct totals
✅ Migration is idempotent and tracked in
_omniroute_migrations✅ Logs show cache read tokens:
in=6055 | out=211 | CR=22399✅ Fixed hydration mismatch error in cache page
Before/After
Before:
{ "totalInputTokens": 71190, "totalCachedTokens": 2298850, "totalCacheCreationTokens": 1350674 }After:
{ "totalInputTokens": 3720714, "totalCachedTokens": 2298850, "totalCacheCreationTokens": 1350674 }Cache read ratio now correctly shows 61% instead of an impossible 3229%.
🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com