fix(cloud): partial-settle aborted /v1/messages streams instead of full-refund (#11513) - #11556
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
…ll-refund (#11513) A client abort on a streaming /api/v1/messages request settled the credit reservation to 0 (full refund) in both onAbort and the stream-catch backstop, even though the platform had already paid the upstream provider for the prompt and every token delivered before the disconnect — an uncollected-revenue leak. /v1/chat/completions was fixed for this in #11455/#11472; this ports the same mechanism to /v1/messages: - accumulate delivered text-delta output during the stream - on abort (onAbort AND the catch path when the request signal is aborted), bill max(estimated input, finished-step input) + max(estimateTokens of delivered text, finished-step output) and settle the reservation to that partial cost, recording the usage as client_aborted_stream - single-flight the terminal settlement across onFinish/onAbort/onError/catch so racing abort paths cannot double-bill (mirrors #11472) - provider errors (onError / catch without an aborted signal) still refund in full; a failed partial billing falls back to a full refund via the first-call-wins idempotent settler Regression test drives the REAL createCreditReservationSettler against a ledger-backed reservation through the exported handleStream seam: red on the old route (abort settled $0, billed nothing), green with the fix. Closes #11513 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5860062 to
68ece4a
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
lalalune
left a comment
There was a problem hiding this comment.
Reviewed and verified after rebasing onto current origin/develop (dcd5c7e).
Local evidence on rebased head 68ece4a:
git diff --check origin/develop...HEAD✅bunx @biomejs/biome@2.5.2 check packages/cloud/api/v1/messages/route.ts packages/cloud/api/__tests__/messages-abort-partial-settle.test.ts packages/cloud/api/__tests__/messages-iac-fast-path.test.ts .github/issue-evidence/11513-messages-abort-partial-settle.md✅bun test __tests__/messages-abort-partial-settle.test.ts✅ 5 pass / 0 failbun test --isolate __tests__/messages-abort-partial-settle.test.ts __tests__/messages-iac-fast-path.test.ts✅ 8 pass / 0 failbun test --isolate __tests__/chat-completions-streaming-credit-leak.test.ts __tests__/chat-stream-credit-leak.test.ts __tests__/chat-completions-optimistic-billing.test.ts __tests__/messages-abort-partial-settle.test.ts✅ 25 pass / 0 failbun run --cwd packages/cloud/api typecheck✅bun run --cwd packages/cloud/api codegen✅ idempotent, no diff
I also confirmed the sibling #11561 is a duplicate per the author comment; #11556 is the canonical branch and keeps the evidence artifact plus the explicit onError refund case. The only caveat I found: direct non-isolated bun test fileA fileB can collide with broad process-global mocks from messages-iac-fast-path.test.ts; the package runner uses isolated unit execution, and the isolated grouped run above passes. Money-path behavior is covered against the real createCreditReservationSettler ledger harness.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
The leak
POST /api/v1/messages(the Anthropic-compatible endpoint Claude Code / Anthropic SDK clients use) full-refunded aborted streams: bothonAbortand the stream-catch backstop inpackages/cloud/api/v1/messages/route.tscalledsettleReservation(0), releasing the entire upfront credit hold even after output had already been streamed to the client. The platform had already paid the upstream provider for the prompt + every delivered token, so each mid-stream disconnect was uncollected revenue./v1/chat/completionswas fixed for exactly this in #11455 (+ single-flight hardening in #11472);/v1/messagesnever got the same partial-settle.The fix (a port of #11455/#11472, same billing path)
text-deltaoutput during the stream.onAbortcallback AND the stream-catch backstop when the request signal is aborted — billmax(estimated input, finished-step input)+max(estimateTokens(deliveredText), finished-step output)via the sharedbillUsage, settle the reservation to that partial cost with the same first-call-wins idempotent settler (createCreditReservationSettler), and record the usage asclient_aborted_stream(isSuccessful: false).onFinish/onAbort/onError/catch (mirrors fix(cloud): single-flight chat abort billing #11472) — the settler is idempotent, but the abort path bills + records analytics before settling, so racing paths must share one settlement promise.onError, or the catch path without an aborted signal) still refund the hold in full; if the partial billing itself throws, the helper falls back to a full refund (idempotent, cannot double-refund).The abort-settle helper is a faithful local port rather than a cross-route import: the chat route's helper is private, chat-specific (chat billing context/audit-record/log prefixes), and these two routes already deliberately keep parallel local helpers (
convertTools,mapToolChoice, ...). The money path —billUsage→ idempotent settler →recordUsageAnalytics— is the identical shared code.Honest delivered-cost measure: the AI SDK emits no
finishpart (hence no exact usage) on abort, so delivered output is billed from the accumulated text-delta text viaestimateTokens, floored by any finished-step usage the SDK did report — the same best-available measure #11455 uses for chat completions, commented as such in the code.Test proof (red → green)
New
packages/cloud/api/__tests__/messages-abort-partial-settle.test.ts, mirroring the #11455 harness (chat-completions-streaming-credit-leak.test.ts): drives the REALcreateCreditReservationSettleragainst a ledger-backed reservation through an exported__messagesStreamingCreditTestHooks.handleStreamseam; onlystreamTextand thebillUsage/analytics boundary are mocked.Red — route at
origin/develop(+ test seam only):All three abort tests fail: the aborted stream settled $0 and billed nothing.
Green — with this fix:
Covers: onAbort partial settlement (exact expected cost + ledger balance), request-signal abort on the catch path, onAbort + catch racing single-flights (billed/recorded exactly once), fullStream provider error refunds in full and bills nothing, onError provider failure refunds in full.
Related suites stay green (each run isolated, as
test:unitdoes):messages-iac-fast-path.test.ts— 3 passchat-completions-streaming-credit-leak.test.ts— 9 passchat-stream-credit-leak.test.ts— 6 passchat-completions-optimistic-billing.test.ts— 5 passChecks
bun run --cwd packages/cloud/api typecheck— passbunx biome checkon all touched files — pass, no diagnosticsbun run --cwd packages/cloud/api codegen— idempotent, no router changesEvidence:
.github/issue-evidence/11513-messages-abort-partial-settle.mdCloses #11513
Money path — do not self-merge.
[cloud-security]
🤖 Generated with Claude Fable 5