Repository navigation
fix(gateway): don't bill upstream failures - #3505
Merged
Merged
Conversation
A streaming request that died mid-flight (an upstream socket close, an in-band provider error, a timeout, or a stream that never reached a terminal event) was still charged for every token the provider emitted before it failed, even though the caller received a 502. The same was true of a non-streaming response that came back empty and is recorded as an upstream_error. HTTP-level provider errors were already unbilled, so the same failure cost nothing when it happened before the stream started and full price when it happened halfway through. Zero the inference costs whenever the recorded outcome is a gateway- or upstream-side failure. Tokens are still logged for analytics. Two failure classes stay billable: client_error, because the caller's own malformed request still consumed upstream inference, and content_filter, which is a served response. Cancellations are untouched and keep billing by default so aborting a stream cannot be used as a billing bypass (GHSA-724j-f2pf-phf7). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe gateway now classifies billable failure reasons and zeros costs for upstream failures in streaming and empty non-streaming responses. Tests verify zero costs, upstream error classification, and retained token metrics. ChangesUpstream failure billing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Anthropic
participant Gateway
participant Billing
Anthropic->>Gateway: truncated stream or empty response
Gateway->>Billing: failure finish reason and token metrics
Billing-->>Gateway: non-billable classification
Gateway-->>Billing: zero inference costs, retain token metrics
Possibly related PRs
🚥 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 |
steebchen
enabled auto-merge (squash)
August 9, 2026 16:36
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/gateway/src/chat/chat.ts`:
- Around line 13450-13468: Update the hasEmptyNonStreamingResponse predicate to
remove the truthiness requirement on finishReason, while preserving the existing
excluded-reason checks and hasMeaningfulAssistantOutput validation so empty
responses without a terminal reason are passed to zeroInferenceCosts.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a782b914-b3e4-4f40-943f-dc098b05f880
📒 Files selected for processing (4)
apps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/lib/costs.spec.tsapps/gateway/src/lib/costs.ts
Comment on lines
+13450
to
+13468
| const hasEmptyNonStreamingResponse = | ||
| !!finishReason && | ||
| finishReason !== "incomplete" && | ||
| !isContentFilterResponse && | ||
| !isLengthLimitResponse && | ||
| !hasMeaningfulAssistantOutput({ | ||
| completionTokens: calculatedCompletionTokens, | ||
| reasoningTokens: calculatedReasoningTokens, | ||
| content, | ||
| toolResults, | ||
| images: convertedImages, | ||
| }); | ||
|
|
||
| // An empty response is recorded as an upstream error and hands the caller | ||
| // nothing usable, so it is not billed. Computed before | ||
| // transformResponseToOpenai so the cost echoed back to the client matches | ||
| // what the log is charged. | ||
| if (hasEmptyNonStreamingResponse) { | ||
| zeroInferenceCosts(costs); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle empty responses with no finish reason.
Line 13451 requires finishReason to be truthy. If an upstream response has no output and omits its terminal reason, this predicate is false. The gateway then leaves the calculated inference costs billable.
Remove the truthiness requirement. The existing excluded-reason checks and hasMeaningfulAssistantOutput check already distinguish valid content from an empty upstream response.
Proposed fix
const hasEmptyNonStreamingResponse =
- !!finishReason &&
- finishReason !== "incomplete" &&
+ finishReason !== "incomplete" &&
!isContentFilterResponse &&
!isLengthLimitResponse &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const hasEmptyNonStreamingResponse = | |
| !!finishReason && | |
| finishReason !== "incomplete" && | |
| !isContentFilterResponse && | |
| !isLengthLimitResponse && | |
| !hasMeaningfulAssistantOutput({ | |
| completionTokens: calculatedCompletionTokens, | |
| reasoningTokens: calculatedReasoningTokens, | |
| content, | |
| toolResults, | |
| images: convertedImages, | |
| }); | |
| // An empty response is recorded as an upstream error and hands the caller | |
| // nothing usable, so it is not billed. Computed before | |
| // transformResponseToOpenai so the cost echoed back to the client matches | |
| // what the log is charged. | |
| if (hasEmptyNonStreamingResponse) { | |
| zeroInferenceCosts(costs); | |
| const hasEmptyNonStreamingResponse = | |
| finishReason !== "incomplete" && | |
| !isContentFilterResponse && | |
| !isLengthLimitResponse && | |
| !hasMeaningfulAssistantOutput({ | |
| completionTokens: calculatedCompletionTokens, | |
| reasoningTokens: calculatedReasoningTokens, | |
| content, | |
| toolResults, | |
| images: convertedImages, | |
| }); | |
| // An empty response is recorded as an upstream error and hands the caller | |
| // nothing usable, so it is not billed. Computed before | |
| // transformResponseToOpenai so the cost echoed back to the client matches | |
| // what the log is charged. | |
| if (hasEmptyNonStreamingResponse) { | |
| zeroInferenceCosts(costs); |
🤖 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 `@apps/gateway/src/chat/chat.ts` around lines 13450 - 13468, Update the
hasEmptyNonStreamingResponse predicate to remove the truthiness requirement on
finishReason, while preserving the existing excluded-reason checks and
hasMeaningfulAssistantOutput validation so empty responses without a terminal
reason are passed to zeroInferenceCosts.
The expensive real-world shape: a long agentic run streams only tool calls (no assistant text) and dies before the provider sends a usage frame. calculateCosts then estimates the completion count from the accumulated tool-call JSON, an estimate that never reaches log.completionTokens — so the row shows 0 output tokens next to a large output cost. Verified this charges $0.02565 on 0 recorded completion tokens without the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
steebchen
added a commit
that referenced
this pull request
Aug 11, 2026
Follow-up to #3505. That PR stopped charging for failed requests; this one fixes the estimator that produced the charges in the first place, and makes any future estimated charge auditable. ## Problem A customer was charged **$17.48 for 1,165,619 output tokens on a log row recording 0 of them**. Two smaller charges ($2.76, $2.98) have the same shape. The stream died before OpenAI sent a usage frame, so `calculateCosts` fell back to estimating a completion count from the tool-call JSON that had accumulated over an hour of agentic work. Four things went wrong at once: 1. It estimated from a response that never completed — a guess about output we never saw the end of. 2. The estimate drove the charge but was **never written back to `log.completionTokens`**. Prompt tokens have a reconciliation step in `chat.ts`; completion tokens had none. So cost and tokens told different stories, and the discrepancy was invisible until a customer noticed. 3. Nothing bounded the estimate. 1,165,619 tokens is **9.1x** `gpt-5.6-sol`'s `maxOutput` of 128,000 — physically impossible output, billed anyway. 4. `JSON.stringify(toolResult.function.arguments)` runs on a value typed `arguments: string`, re-escaping every quote and newline and inflating the chars/4 estimate on exactly the tool-heavy payloads where it matters most. The arithmetic confirms it was never upstream-reported usage: input is `83,042 × $5/M × 0.5 (flex) = $0.20760500`, matching the log exactly, while output is `$17.484285 ÷ ($30/M × 0.5) = 1,165,619` — a number appearing nowhere else in the row. ## Approach Estimation is a fallback for an anomaly, not a revenue path: `prepare-request-body.ts` sets `stream_options: { include_usage: true }` across 11 provider branches, so a missing completion count means something broke. Each change below is independently sufficient to have caught this. - **Don't estimate output for a response that didn't complete.** New `allowOutputEstimate` option on `calculateCosts`, passed `false` from the streaming failure path. - **Write the billed completion count back to the log.** A row showing 0 output tokens beside a non-zero output cost is now structurally impossible. - **Clamp an estimated count to the mapping's `maxOutput`** (declared on 575 mappings). Provider-reported counts are never clamped — those are the provider's own measurement, and rewriting them would hide a real upstream disagreement. - **Stop re-encoding tool-call arguments.** ## Auditability `calculateCosts` now warns — `Billed a request on estimated token counts` — whenever a billed request used a count we made up rather than one the provider reported. It lives in the one place every call site goes through, so no path can miss it, and the logger attaches the trace id in production (also stored on the log row), so a hit pivots straight to the request it charged. Warn rather than info on purpose: this is the class of charge behind the incident, and it should be rare. If it turns out not to be rare, that is the finding. Writing that warning surfaced a related mislabel: `estimatedCost` was set on merely *entering* the estimation branch, so a request whose prompt tokens the provider reported and whose output estimation was declined was recorded as estimated. It is now derived from what was actually estimated, which keeps both the log column and the warning honest. A second, narrower warning fires when a *successful* stream reports no completion count and its output ends up billed at zero — that means the provider ignored our `include_usage`, and the right fix is in the request we send it. The two conditions are mutually exclusive, so one event never logs twice. ## Verification Both new integration tests were confirmed to fail against the pre-fix code: - `a successful stream logs the completion count it was billed for` → `expected 0 to be greater than 0` (the row logged 0 output tokens on a billed request). - `stream truncated after tool calls only is not billed` (added in #3505) → charged `$0.02565` on 0 recorded completion tokens. Unit tests in `costs.spec.ts` cover the disallowed estimate, the `maxOutput` clamp, the never-clamp-reported-counts guarantee, the double-encode, and all three warning cases (fires when estimated, silent when measured, silent when the estimate produced no charge). `api.spec.ts` (157), `fallback.spec.ts` (54), `responses.spec.ts` (73), `costs.spec.ts` (85), and the four pricing suites (338) pass, run serially. `pnpm format` and `pnpm build` clean. ## Not included Surfacing `estimatedCost` in the UI — it's on the log row and in the generated API types (`v1.d.ts:4472`) but rendered nowhere, so a user cannot tell an estimated charge from a measured one. Separate UI change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved cost and token tracking for streaming responses, including Anthropic tool-call responses. * Prevented failed streams from receiving guessed completion-token counts. * Improved handling when providers omit completion usage information. * Added safeguards to keep estimated output tokens within model limits while preserving provider-reported values. * Improved handling of tool-call content and added warnings when billing relies on estimated usage. * **Tests** * Added coverage for streaming costs, estimation limits, disabled estimation, warnings, and tool-call arguments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A streaming request that dies mid-flight is still charged for every token the provider emitted before it failed — even though the caller gets a 502 and the log is recorded as
upstream_error.Real example: a
google-vertex/gemini-3.6-flashstream produced ~15k tokens, then Vertex terminated it with a 429RESOURCE_EXHAUSTED. The request returned502 Upstream Stream Terminatedand was billed $0.0139.The inconsistency: an HTTP-level provider error (a 500/429 before the stream starts) already logs
cost: nulland is free. The exact same provider failure costs full price if it happens halfway through the stream, purely because the streaming path'sfinallyblock callscalculateCostsunconditionally. The same applies to a non-streaming response that comes back empty and is recorded asupstream_error.Approach
Zero the inference costs whenever the recorded outcome is a gateway- or upstream-side failure:
chat.ts): aftercostsis resolved in thefinallyblock, zero them whenstreamingErroris set. This covers upstream socket closes, read faults, timeouts, buffer overflows, in-band provider error events, truncated streams, and empty streaming responses.chat.ts): zero them forhasEmptyNonStreamingResponse. The empty-response check moved abovetransformResponseToOpenaiso the cost echoed back to the client matches what the log is charged.Token counts are still recorded in full, so analytics and the activity detail page are unchanged — only the cost columns go to zero.
Two failure classes stay billable, via
isBilledFailureFinishReasoninlib/costs.ts:client_error— the caller's own malformed request still consumed upstream inference, so it must not become a free-inference path.content_filter— a safety block is a served response, billed on purpose.Cancellations are a separate axis and are explicitly excluded from the new branch, so aborting a stream still bills by default and cannot be used as a billing bypass (GHSA-724j-f2pf-phf7).
Data storage cost is left untouched, matching the existing unbilled-refusal precedent — retention is billed separately from inference.
Verification
costs.spec.tsforisBilledFailureFinishReason.api.spec.ts: a truncated Anthropic stream with partial output, and an empty non-streaming response. Both assertcost/inputCost/outputCostare 0 whilepromptTokensis still recorded. Confirmed they fail against the pre-fix code ($0.0007 and $0.0005 charged respectively).api.spec.ts,fallback.spec.ts,cancellation.spec.ts,chat-resilience.spec.ts,responses.spec.ts,dev-plan-payg.spec.tsall pass (run serially — these suites interfere with each other in parallel).pnpm formatandpnpm buildclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests