fix(proxy): suppress Claude subscription on ctx in baseline failover exhaustion check#533
Conversation
|
PR author is not in the allowed authors list. |
109b6bc to
cbe4d87
Compare
steventohme
left a comment
There was a problem hiding this comment.
Thanks for chasing this down — but I think this one-liner is a no-op as written and doesn't actually fix the billing miscount. The bug is real; this just doesn't move it.
Why it's a no-op
servedOnSubscription(ctx) reads CredentialsFromContext(ctx).OAuth — the credential stamped by resolveAndInjectCredentials, not the suppression marker:
func servedOnSubscription(ctx context.Context) bool {
creds := CredentialsFromContext(ctx)
return creds != nil && creds.OAuth
}withSuppressedClaudeSubscription sets a separate context key (suppressClaudeSubscriptionContextKey), and that marker only takes effect on the next resolveAndInjectCredentials call. In ProxyMessages, ctx's credential is set exactly once — at line 1797. After the new line at 2097 there is no ctx = resolveAndInjectCredentials(ctx, …); only baselineCtx is re-resolved and dispatched. (Grepping the file, the only ctx = resolveAndInjectCredentials calls are 1797 and 3302, the latter in a different handler.)
So marking ctx suppressed at 2097 leaves the OAuth credential injected at 1797 untouched → servedOnSubscription(ctx) at line 2192 (cost.subscription_served) and line 2917 (SubscriptionServed) still returns true, and the deployment-key cost still goes unrecorded.
Why the test passes anyway
TestResolveAndInjectCredentials_BaselineFailoverRace manually calls resolveAndInjectCredentials(ctxFixed, …) after suppression:
ctxFixed := withSuppressedClaudeSubscription(ctx)
outFixed := resolveAndInjectCredentials(ctxFixed, providers.ProviderAnthropic, headers) // production never does this after 2097That proves the helper's behavior, not the integration. The production path never re-resolves ctx after 2097, so the assertion that's green here doesn't reflect what ProxyMessages actually does.
Note on the line numbers
The issue's trace references ctx = resolveAndInjectCredentials(ctx, ProviderAnthropic) at "line 2147", but current main resolves baselineCtx, not ctx — the baseline path was refactored to a separate variable since the issue was filed, which is what makes this line inert.
Suggested fix
One of:
- (preferred) After a successful baseline failover, set
ctx = baselineCtx, so every ctx-derived downstream attribute (billingSubscriptionServed,cost.subscription_served, and the credential prefix/suffix/source telemetry) reflects the credential that actually served the turn — not just billing. - Re-resolve right after the suppression line:
ctx = resolveAndInjectCredentials(ctx, providers.ProviderAnthropic, r.Header).
And the test should drive ProxyMessages (or at minimum assert on the post-baseline ctx), so it would fail against the current one-line change.
TestResolveAndInjectCredentials_BaselineFailoverRace tested the resolveAndInjectCredentials helper in isolation by calling it manually after suppression — a call production never makes at that point in ProxyMessages. It passed whether or not line 2098 existed in service.go and did not discriminate the bug from the fix. Replace with TestBaselineFailoverCtxSuppression which simulates the actual production chain: context.Background() as the starting ctx (OSS primary at line 1797 left no Anthropic credential on ctx), suppression applied or not at line 2098, re-resolution at line 2148 with ProviderAnthropic. fails_without_fix: no suppression on ctx before re-resolution -> resolveAndInjectCredentials injects OAuth -> servedOnSubscription = true (documents the bug). passes_with_fix: withSuppressedClaudeSubscription applied before re-resolution -> both guards in resolveAndInjectCredentials fire (line 2694 block skipped, line 2759 bearer nil'd) -> servedOnSubscription = false (proves the fix). Removing withSuppressedClaudeSubscription from passes_with_fix makes it go red -- the test correctly discriminates the fixed from the broken path. Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com>
…exhaustion check When claudeSubscriptionExhausted fires at the baseline check (line 2097) but not at the pre-dispatch check (line 1794), ctx carries an OAuth credential from OSS resolution at line 1797. The re-resolution at line 2148 (ctx = resolveAndInjectCredentials(ctx, finalProvider, r.Header)) then re-injects the OAuth token -- causing servedOnSubscription(ctx) to return true even though Weave's deployment key served the turn. Three misattributions result: - cost.subscription_served OTel attribute: true instead of false - SubscriptionServed billing field: delta = 0 instead of full cost - credentialKeyParts telemetry: subscription token instead of deployment key Fix: withSuppressedClaudeSubscription(ctx) at line 2098 alongside baselineCtx. The suppression marker survives untouched to line 2148 (ctx is not reassigned between 2098 and 2148). resolveAndInjectCredentials skips the OAuth block (line 2694: !suppressClaudeSub guard) and drops the OAuth bearer on the non-router-keyed path (line 2759 guard). All three downstream reads of ctx are corrected. Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com>
cbe4d87 to
a9c6e95
Compare
|
Thanks for the detailed review — you're right that the test was bad and I've fixed it. But I want to flag what I think is a factual error in the "no-op" diagnosis. Line 2148 existsgrep -n "ctx = resolveAndInjectCredentials" internal/proxy/service.go
1797: ctx = resolveAndInjectCredentials(ctx, decision.Provider, r.Header)
2148: ctx = resolveAndInjectCredentials(ctx, finalProvider, r.Header)
3317: ctx = resolveAndInjectCredentials(ctx, decision.Provider, r.Header)
3579: ctx = resolveAndInjectCredentials(ctx, finalProvider, r.Header)Your review says "the only
This is the re-resolution the fix depends on. Why the fix is not a no-opWith line 2098 in place, the chain is:
Where you're right — the testThe old The new
Removing Force-pushed. |
rohith500
left a comment
There was a problem hiding this comment.
Detailed response left as a PR comment above — the short version: line 2148
(ctx = resolveAndInjectCredentials(ctx, finalProvider, r.Header)) exists on
current main and was missed in the grep. The suppression marker from line 2098
flows through to that re-resolution, which is what makes the fix effective.
Test rewritten and force-pushed.
Problem
When a Claude subscription is healthy at the pre-dispatch exhaustion check (service.go:1794) but becomes exhausted during the primary OSS model dispatch (concurrent request burns the plan window), the baseline failover path has a race that causes the deployment key cost to be absorbed silently with no billing record.
Execution trace of the race:
claudeSubscriptionExhausted = false→ suppression NOT applied toctxresolveAndInjectCredentials(ctx, ProviderOSS)— OSS credentials setclaudeSubscriptionExhausted = true(snapshot now exhausted)baselineCtx = withSuppressedClaudeSubscription(baselineCtx)resolveAndInjectCredentials(baselineCtx, ProviderAnthropic)→ deployment key ✓ctx = resolveAndInjectCredentials(ctx, ProviderAnthropic)→ subscription token injectedservedOnSubscription(ctx)→true→delta = 0→ deployment key cost unrecordedThe deployment key pays for the turn, but the ledger records a zero-debit row (subscription rate). Three attributes are misattributed:
cost.subscription_servedOTel attribute (service.go:2200) — reportstrueinstead offalseSubscriptionServedbilling field (service.go:2931) — causesdelta = 0instead of full costcredentialKeyPrefix/Suffix/credSourcetelemetry (service.go:2229) — attributes the turn to the subscription token instead of the deployment keyFix
Apply
withSuppressedClaudeSubscriptiontoctxalongsidebaselineCtxat the baseline exhaustion check:With the fix,
ctxcarries the suppression flag through to the re-resolution at line 2147, which falls through to the deployment key. All three misattributed fields are corrected by the same change.Safety properties verified:
withSuppressedClaudeSubscriptionapplied twice is idempotent —context.WithValuewraps immutably,.Value()returnstrueeither wayVerification
TestResolveAndInjectCredentials_BaselineFailoverRaceinsubscription_exhaustion_internal_test.goproves both paths throughresolveAndInjectCredentialsandservedOnSubscription:ctx→ subscription token injected →servedOnSubscription = truectx→ falls through to deployment key →servedOnSubscription = falsemake check— all 30 packages pass.