fix: prevent double counting of Anthropic streaming cache tokens - #2236
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the Anthropic streaming token usage parsing to prevent double-counting cache-related tokens when both message_start and message_delta report usage, and adds a regression test to validate the behavior.
Changes:
- Adjust
message_deltahandling to treat usage values as cumulative and avoid accumulating cache/input tokens twice. - Add a new table-driven test covering several cache token combinations across
message_startandmessage_delta. - Introduce
testify/assertusage in the new test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/translator/anthropic_helper.go | Changes message_delta token accounting to avoid double-counting and sets output tokens as cumulative. |
| internal/translator/anthropic_helper_test.go | Adds a regression test ensuring cache tokens aren’t double-counted across streaming events. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2236 +/- ##
==========================================
+ Coverage 84.87% 84.89% +0.01%
==========================================
Files 144 144
Lines 21317 21345 +28
==========================================
+ Hits 18093 18121 +28
Misses 2138 2138
Partials 1086 1086 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e951b39 to
33f745d
Compare
The message_delta event handler in anthropicStreamParser was using Add* methods for input tokens, cached tokens, and cache creation tokens. However, the Anthropic API reports cumulative (not incremental) token counts in message_delta, and message_start already sets these values correctly via ExtractTokenUsageFromExplicitCaching. Using Add on top of the already-set cumulative values caused cache tokens to be counted twice. For example, with input_tokens=9 and cache_read_input_tokens=1: - message_start correctly set InputTokens=10 (9+1), CachedInputTokens=1 - message_delta then AddInputTokens(1), AddCachedInputTokens(1) resulting in InputTokens=11, CachedInputTokens=2 (double counted) Fix: Only update output tokens from message_delta using SetOutputTokens, since message_start typically reports output_tokens=0 and message_delta provides the final count. This matches the pattern already used in anthropic_anthropic.go's reflectStreamingEvent. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
Signed-off-by: liuhy <liuhongyu@apache.org>
Signed-off-by: liuhy <liuhongyu@apache.org>
c62a63c to
f867645
Compare
The previous stream parser avoided double-counting by ignoring message_delta input and cache fields. That left delta-only or corrected cumulative usage undercounted. The parser now inspects raw JSON field presence so absent fields do not erase message_start values, while present message_delta fields can override and recompute derived input totals. Constraint: Anthropic SDK MessageDeltaUsage uses non-pointer numeric fields, so normal unmarshalling cannot distinguish absent fields from explicit zero values Rejected: Always set from MessageDeltaUsage | absent fields would overwrite message_start usage with zero defaults Confidence: high Scope-risk: narrow Tested: go test ./internal/translator Tested: make lint Signed-off-by: liuhy <liuhongyu@apache.org>
00d6d0e to
291f8a8
Compare
|
close: #2279 |
- Add detailed comments explaining message_delta token handling logic - Clarify why Set (not Add) is used for cumulative token counts - Document handling of delta-only cache tokens and corrected values - Add test cases for message_delta with partial cache fields - Add test for message_delta without usage field - Improve coverage from 73.3% to 96.7% for updateInputUsageFromMessageDelta Addresses PR review comments: - Copilot comment on lines 1054-1064 about ignoring message_delta tokens - Copilot comment on lines 833-846 about missing test coverage - Codecov patch coverage requirement (now 87.4% > 80% target) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
- Add test for message_delta with cache tokens when message_start already has cache - Add test for invalid JSON in message_delta usage fields (error handling) - Achieve 100% coverage for updateInputUsageFromMessageDelta function - Improve overall coverage to 88.0% Addresses Codecov patch coverage requirement (89.19% -> 100%) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
06aea8e to
404019f
Compare
Run make precommit to fix comment alignment issue Signed-off-by: liuhy <liuhongyu@apache.org>
|
/retest |
Signed-off-by: liuhy <liuhongyu@apache.org>
Inference Extension conformance can create multiple HTTPRoutes for the same InferencePool in quick succession. A reconcile may observe only one referenced Gateway through the cache and write a partial Parents status, then receive no later event to recompute the full parent set. Return a short requeue only when the InferencePool status actually changes. Status equality ignores LastTransitionTime so the follow-up reconcile converges and stops instead of looping forever. Constraint: controller-runtime cache-backed route and gateway lists can lag related object events. Rejected: Increasing conformance timeouts, because stale status can persist indefinitely without another reconcile. Confidence: high Scope-risk: moderate; changes InferencePool reconcile scheduling after status updates. Tested: go test ./internal/controller -run TestInferencePoolController Tested: go test ./internal/controller Tested: go test ./internal/translator Tested: make lint Not-tested: full inference extension e2e locally. Signed-off-by: liuhy <liuhongyu@apache.org>
This reverts commit 6fcf39b. Signed-off-by: liuhy <liuhongyu@apache.org>
a99202d to
1b6ef2e
Compare
|
/retest |
…resence The message_delta handler set output and reasoning tokens from the SDK's anthropic.MessageDeltaEvent.Usage fields guarded only by `>= 0`. Those fields are non-pointer int64s that decode to 0 when usage (or a subfield) is omitted, and 0 >= 0 is always true — so a later message_delta that omits usage overwrote an already-set non-zero output/reasoning count with zero. Anthropic's streaming protocol reports cumulative usage and a stream may emit multiple message_delta events, so this case is real, not theoretical. Replace the no-op `>= 0` guards with the SDK's JSON presence fields (u.JSON.OutputTokens.Valid() and u.OutputTokensDetails.JSON.ThinkingTokens.Valid(), via respjson.Field.Valid()), matching the presence-based approach already used for input/cache tokens in updateInputUsageFromMessageDelta. A message_delta that omits these fields now leaves prior values untouched. Also fix the misleading test TestAnthropicStreamParserTokenUsage_MessageDeltaNoUsage, which encoded the bug as expected behavior (asserting output=0 when usage is absent). Renamed to ...NoUsagePreservesPrior: it emits a message_delta with output_tokens=16 and thinking_tokens=4, then a second message_delta without usage, and asserts both counts survive. Tested: go test ./internal/translator/... -run AnthropicStreamParserTokenUsage Tested: go test ./internal/translator/... Tested: go vet ./internal/translator/... && go build ./... Signed-off-by: liuhy <liuhongyu@apache.org>
|
/retest |
…oyproxy#2236) **Description** The `message_delta` event handler in `anthropicStreamParser` was using `Add*` methods for input tokens, cached tokens, and cache creation tokens. However, the Anthropic API reports cumulative (not incremental) token counts in `message_delta`, and `message_start` already sets these values correctly via `ExtractTokenUsageFromExplicitCaching`. Using `Add` on top of the already-set cumulative values caused cache tokens to be counted twice. ### Bug Example With `input_tokens=9` and `cache_read_input_tokens=1`: | Step | Action | InputTokens | CachedInputTokens | |------|--------|-------------|-------------------| | `message_start` | `SetInputTokens(10)`, `SetCachedInputTokens(1)` | 10 ✅ | 1 ✅ | | `message_delta` (before fix) | `AddInputTokens(1)`, `AddCachedInputTokens(1)` | 11 ❌ | 2 ❌ | | `message_delta` (after fix) | Only `SetOutputTokens(16)` | 10 ✅ | 1 ✅ | ### Root Cause `ExtractTokenUsageFromExplicitCaching` computes `InputTokens = input_tokens + cache_read + cache_creation`. In `message_start`, this is correctly set via `Set*` methods. In `message_delta`, the same cumulative values were being added via `Add*` methods, causing double counting. ### Fix The `message_delta` handler now only updates output tokens using `SetOutputTokens`, since `message_start` typically reports `output_tokens=0` and `message_delta` provides the final count. This matches the pattern already used in `anthropic_anthropic.go`'s `reflectStreamingEvent`. The `MessageDeltaUsage` struct uses non-pointer `int64` fields that default to 0 when absent from JSON, making it impossible to distinguish "not provided" from "actually zero" — so we cannot safely `Set` override from delta either, as it would erase correct `message_start` values with zeros. ### Changes - `internal/translator/anthropic_helper.go`: Removed `Add*` calls for input/cache tokens from `message_delta` handler; replaced `AddOutputTokens` with `SetOutputTokens`; removed unused `ExtractTokenUsageFromExplicitCaching` call; added `>= 0` guard for `ThinkingTokens` - `internal/translator/anthropic_helper_test.go`: Added `TestAnthropicStreamParserTokenUsage_NoDoubleCounting` with 6 test cases ### Test Coverage - Cache tokens in both `message_start` and `message_delta` (core double-count scenario) - Cache creation tokens in both events - Both cache_read and cache_creation in both events - No cache tokens (baseline correctness) - Cache tokens only in `message_start`, not in `message_delta` - Cache tokens only in `message_delta` — verifies they are ignored (non-pointer int64 fields cannot distinguish absent from zero) --------- Signed-off-by: liuhy <liuhongyu@apache.org> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io> Signed-off-by: Daniel Chernovsky <daniel.chernovsky@doubleverify.com>
Description
The
message_deltaevent handler inanthropicStreamParserwas usingAdd*methods for input tokens, cached tokens, and cache creation tokens. However, the Anthropic API reports cumulative (not incremental) token counts inmessage_delta, andmessage_startalready sets these values correctly viaExtractTokenUsageFromExplicitCaching. UsingAddon top of the already-set cumulative values caused cache tokens to be counted twice.Bug Example
With
input_tokens=9andcache_read_input_tokens=1:message_startSetInputTokens(10),SetCachedInputTokens(1)message_delta(before fix)AddInputTokens(1),AddCachedInputTokens(1)message_delta(after fix)SetOutputTokens(16)Root Cause
ExtractTokenUsageFromExplicitCachingcomputesInputTokens = input_tokens + cache_read + cache_creation. Inmessage_start, this is correctly set viaSet*methods. Inmessage_delta, the same cumulative values were being added viaAdd*methods, causing double counting.Fix
The
message_deltahandler now only updates output tokens usingSetOutputTokens, sincemessage_starttypically reportsoutput_tokens=0andmessage_deltaprovides the final count. This matches the pattern already used inanthropic_anthropic.go'sreflectStreamingEvent. TheMessageDeltaUsagestruct uses non-pointerint64fields that default to 0 when absent from JSON, making it impossible to distinguish "not provided" from "actually zero" — so we cannot safelySetoverride from delta either, as it would erase correctmessage_startvalues with zeros.Changes
internal/translator/anthropic_helper.go: RemovedAdd*calls for input/cache tokens frommessage_deltahandler; replacedAddOutputTokenswithSetOutputTokens; removed unusedExtractTokenUsageFromExplicitCachingcall; added>= 0guard forThinkingTokensinternal/translator/anthropic_helper_test.go: AddedTestAnthropicStreamParserTokenUsage_NoDoubleCountingwith 6 test casesTest Coverage
message_startandmessage_delta(core double-count scenario)message_start, not inmessage_deltamessage_delta— verifies they are ignored (non-pointer int64 fields cannot distinguish absent from zero)