Repository navigation
fix(gateway): adapt failed requests billing - #3546
Merged
Merged
Conversation
A truncated agentic stream was charged $17.48 for 1,165,619 output tokens on a log row recording 0 of them. The provider never reported a completion count — the stream died before any usage frame — so calculateCosts fell back to estimating one from the accumulated tool-call JSON. That estimate drove the charge but was never written back to the log, so cost and tokens told different stories and nobody noticed until a customer did. Four changes, each independently sufficient to have caught it: - Don't estimate output tokens for a response that did not complete. A dead stream leaves only partial text to guess from; billing that guess is not a measurement. New `allowOutputEstimate` option, passed false from the streaming failure path. - Write the billed completion count back to the log. Prompt tokens already had this reconciliation; completion tokens did not, which is the only reason the divergence stayed invisible. A row showing 0 output tokens beside a non-zero output cost is now impossible. - Clamp an estimated count to the mapping's maxOutput. The incident billed 9.1x what the deployment can physically emit. Provider-reported counts are never clamped — those are the provider's measurement. - Stop re-encoding tool-call arguments. `arguments` is already a JSON string, so JSON.stringify escaped every quote and inflated the chars/4 estimate on exactly the tool-heavy payloads where it matters most. Also warns when a successful stream reports no completion count, since that means the provider ignored our include_usage and its output is now billed at 0 rather than at a guess — the fix belongs in the request we send it. 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
WalkthroughThe gateway adds controlled completion-token estimation for streaming responses. Estimated counts are capped and logged, while provider-reported counts remain unchanged. Failed streams disable estimation, and successful streams synchronize logged usage with billing results. ChangesOutput-token estimation and billing
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 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 |
![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: 2
🤖 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 11233-11236: Use zero output billing whenever a stream lacks
provider-reported completion usage: update allowOutputEstimate in the chat
stream handling to disable estimation for that case, retain the warning only
after the billed count is guaranteed to be zero, and update the missing-usage
integration test to assert zero completion tokens and zero output cost. Apply
changes at apps/gateway/src/chat/chat.ts lines 11233-11236 and 11285-11306, and
apps/gateway/src/api.spec.ts lines 8668-8775.
In `@apps/gateway/src/lib/costs.ts`:
- Line 379: Update the completion-token estimation condition in
apps/gateway/src/lib/costs.ts lines 379-379 to estimate only when the reported
completion count is null or undefined, preserving an explicit provider-reported
0. Add a test case in apps/gateway/src/lib/costs.spec.ts lines 2008-2020 with
zero completion tokens plus tool calls or completion content, and assert that
output billing remains zero.
🪄 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: f9f84b39-0be5-4d35-a089-8bf3b8738bec
📒 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
+11233
to
+11236
| // A stream that died never delivered a usage frame, so | ||
| // there is nothing to estimate from but the partial text | ||
| // and tool-call JSON that happened to arrive. Don't guess. | ||
| allowOutputEstimate: streamingError === null, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one billing rule for successful streams without completion usage. The chat path enables estimation, while its warning and the PR objective state that output must bill as zero. The integration test currently enforces the conflicting estimated-charge behavior.
apps/gateway/src/chat/chat.ts#L11233-L11236: disable output estimation when the stream has no provider-reported completion usage.apps/gateway/src/chat/chat.ts#L11285-L11306: retain the warning only after the billed count is guaranteed to be zero.apps/gateway/src/api.spec.ts#L8668-L8775: assert zero completion tokens and zero output cost for the missing-usage stream.
📍 Affects 2 files
apps/gateway/src/chat/chat.ts#L11233-L11236(this comment)apps/gateway/src/chat/chat.ts#L11285-L11306apps/gateway/src/api.spec.ts#L8668-L8775
🤖 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 11233 - 11236, Use zero output
billing whenever a stream lacks provider-reported completion usage: update
allowOutputEstimate in the chat stream handling to disable estimation for that
case, retain the warning only after the billed count is guaranteed to be zero,
and update the missing-usage integration test to assert zero completion tokens
and zero output cost. Apply changes at apps/gateway/src/chat/chat.ts lines
11233-11236 and 11285-11306, and apps/gateway/src/api.spec.ts lines 8668-8775.
|
|
||
| // Calculate completion tokens | ||
| if (!completionTokens && fullOutput) { | ||
| if (!completionTokens && fullOutput && allowOutputEstimate) { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve reported zero completion counts. A completion count of 0 is a provider-reported value, not an absent value. The current truthiness check can replace it with an estimated, billable count.
apps/gateway/src/lib/costs.ts#L379-L379: use explicit null checks before estimating completion tokens.apps/gateway/src/lib/costs.spec.ts#L2008-L2020: add a provider-reported0case with tool calls or completion content and assert zero output billing.
📍 Affects 2 files
apps/gateway/src/lib/costs.ts#L379-L379(this comment)apps/gateway/src/lib/costs.spec.ts#L2008-L2020
🤖 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/lib/costs.ts` at line 379, Update the completion-token
estimation condition in apps/gateway/src/lib/costs.ts lines 379-379 to estimate
only when the reported completion count is null or undefined, preserving an
explicit provider-reported 0. Add a test case in
apps/gateway/src/lib/costs.spec.ts lines 2008-2020 with zero completion tokens
plus tool calls or completion content, and assert that output billing remains
zero.
steebchen
changed the title
Makes an invented charge auditable after the fact. calculateCosts now warns whenever a billed request used a token count we estimated rather than one the provider reported, from the one place every call site goes through. The logger attaches the trace id in production, which is 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 phantom-billing incident, and it should be rare since every streaming provider is asked for usage via include_usage. If it turns out not to be rare, that is the finding. Also fixes estimatedCost mislabelling requests that estimated nothing. The flag 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 new warning honest. The narrower streaming warning added in the previous commit now fires only when the output ended up billed at zero, so one event never logs twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
steebchen
enabled auto-merge (squash)
August 11, 2026 18:35
This was referenced Aug 12, 2026
steebchen
added a commit
that referenced
this pull request
Aug 12, 2026
## Problem When investigating "output cost doesn't match output tokens" reports (e.g. an hour whose blended output rate multiplies out far above any real tier rate), the phantom rows come from streams that died before the provider sent usage: the pre-#3546 estimator billed an estimated output count that was never written back to `completion_tokens`, leaving cost with no matching tokens. #3546 fixed this going forward but did not backfill, so historical rows still carry the mismatch and each report needs an ad-hoc query. ## Approach Adds `sql/phantom_output_cost_by_project.sql`, a reusable two-part audit: 1. Per-row detail: every log row for a given account + model where `output_cost` exceeds `completion_tokens × served-tier output rate × 1.6`, with `expected_output_cost` / `excess_output_cost` columns and the diagnostic fields (`canceled`, `estimated_cost`, `unified_finish_reason`, `pricing_tier`). 2. Totals: phantom row count and the summed excess, i.e. the refund amount. Scalability is the point of the shape: the project-id resolution is a `MATERIALIZED` CTE so it is the first node of the plan, and the log access is a `LATERAL` join on `project_id` — verified with `EXPLAIN` that the planner runs a nested loop probing a `project_id`-leading index (`log_project_id_used_model_idx` locally) instead of scanning `log`. The `created_at` lower bound keeps `log_project_id_created_at_idx` usable too. The 1.6× headroom keeps legitimate rows out (discounts only lower costs; the long-context tier tops out at 1.5× base). Per-row `::numeric` casts avoid float4 accumulation drift in the sum. The committed file uses placeholder values (`customer@example.com`, `openai/gpt-5.6-sol` rates as the example CASE) — fill in account email, model, rates and date bound before running. ## Verification - Both statements run clean against the local dev database (seeded schema). - `EXPLAIN` confirms the materialized-CTE → nested-loop → index-scan plan shape. - `pnpm format` and full `pnpm build` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added account-scoped audit queries to identify projects with potentially excessive billed output costs. * Included detailed results with calculated excess costs and an aggregated summary of suspect records. * Supports filtering by model, date range, output cost, and organization type. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Fable 5 <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.
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
calculateCostsfell 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:log.completionTokens. Prompt tokens have a reconciliation step inchat.ts; completion tokens had none. So cost and tokens told different stories, and the discrepancy was invisible until a customer noticed.gpt-5.6-sol'smaxOutputof 128,000 — physically impossible output, billed anyway.JSON.stringify(toolResult.function.arguments)runs on a value typedarguments: 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.tssetsstream_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.allowOutputEstimateoption oncalculateCosts, passedfalsefrom the streaming failure path.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.Auditability
calculateCostsnow 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:
estimatedCostwas 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 fix(gateway): don't bill upstream failures #3505) → charged$0.02565on 0 recorded completion tokens.Unit tests in
costs.spec.tscover the disallowed estimate, themaxOutputclamp, 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 formatandpnpm buildclean.Not included
Surfacing
estimatedCostin 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
Summary by CodeRabbit