fix(cloud): partial-settle aborted /v1/messages streams — stop full-refunding delivered tokens (#11513) - #11561
NubsCarson wants to merge 1 commit into
Conversation
…borted A client abort mid-stream on POST /api/v1/messages settled the credit reservation to 0 — a FULL refund — in both the streamText onAbort callback and the outer stream catch, even though the tokens already streamed to the client were really generated and really billed to us by the provider. Every aborted stream was therefore under-collected (#11513). Port the merged /v1/chat/completions partial-settle pattern (#11472): - Accumulate deliveredText from the SSE text-delta loop. - On abort, bill max(estimatedInputTokens, finished-step input) + max(estimateTokens(deliveredText), finished-step output) via billUsage, settle the reservation at that cost, and record analytics with the client_aborted_stream marker (isSuccessful: false). If billing itself fails, fall back to a full refund so the hold is never leaked. - Wire the partial settle at BOTH the streaming onAbort AND the outer stream catch, gated on abortSignal?.aborted === true; non-abort errors still refund to 0. - Compose a single-flight settle-once guard over the existing first-call-wins settler so onFinish/onAbort/onError/outer-catch races cannot double-bill or double-record (createCreditReservationSettler itself is untouched — #11512 is a separate lane). Test: __tests__/messages-streaming-abort-billing.test.ts drives the REAL createCreditReservationSettler against a ledger-backed reservation through the route's streaming handler (mocked streamText boundary) and asserts an aborted stream settles > 0 at the delivered-token cost with exactly one billUsage call; mutation-checked by reverting to settleReservation(0) (3/4 tests fail, refund-path test still passes as designed). Fixes #11513
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 |
|
Closing as a duplicate of #11556 — both PRs were opened 6 minutes apart by sibling agents on this same account for the same #11513 fix (partial-settle aborted /v1/messages streams, port of #11472). Keeping #11556 as the canonical PR: equivalent money-path change plus red→green leak-test evidence, and it avoids the whole-config |
NubsCarson
left a comment
There was a problem hiding this comment.
[cloud-audit] LGTM
Verified the critical invariant for the focus: an aborted /v1/messages stream settles the ACTUAL delivered cost exactly once — never double-credits, never mints, never full-refunds delivered tokens on a client abort.
What I traced (not just the diff):
- Idempotence/fence (the #11484 class):
createCreditReservationSettler(packages/cloud/shared/src/lib/utils/credit-reservation.ts) assignssettlePromisesynchronously before awaitingreconcile, so it is genuinely first-call-wins with no async TOCTOU. The newsettleStreamingOnceguard (route.ts, handleStream) composes on top and single-flights the billUsage + analytics work — necessary because the settler alone would only dedupe the reconcile, not a duplicate bill/record. Test 3 proves the onAbort + outer-catch race collapses to one reconcile / one billUsage / one analytics record against the REAL settler. - No double-credit: worst-case failure interleaving —
settleReservation(billing.totalCost)succeeds, thenrecordUsageAnalyticsthrows → the catch's fallbacksettleReservation(0)hits the settler's cached first-call promise and returns the original settlement, it does NOT refund. The pre-existing route-levelawait settleReservation?.(0)catch (route.ts:734) is likewise absorbed by the settler. - No mint: abort cost is always >= 0, and
reconcile(actualCost)refunds at mosthold - actualCost(charges overage otherwise, per ai-billing.ts "refund excess or charge overage").billUsageis called WITHOUT the reservation param, so it only computes cost + affiliate earnings (deduped by sourceId) — deduction happens solely through the settler, identical to the pre-existing onFinish pattern. No double-charge. - No free inference on abort: both abort seams bill — SDK
onAbort({steps})and the outer catch gated onabortSignal?.aborted === true, whereabortSignalis verifiably the raw request signal (c.req.raw.signalat the handleStream call site). The mutation check (revert both tosettleReservation(0)→ 3/4 tests fail) confirms the tests are load-bearing, and the tests drive the real settler against a ledger, not a mock. - Parity claim checked: I diffed the structure against develop's merged
chat/completions/route.ts(#11472) —summarizeFinishedStepUsage,settleStreamingAbortReservation, the single-flight guard, and the outer-catch gate are line-for-line the same pattern. The shared settler is untouched as claimed (#11512 lane respected). Themessages-iac-fast-path.test.tsmock update correctly covers the newestimateTokensimport so the suite still loads.
Non-blocking caveats (both disclosed in the PR body, both under-collect in the user's favor, both exact parity with merged #11472):
onErrorrefunds to 0 without an abort check — if a client abort ever surfaces asonErrorbeforeonAbort/outer-catch, delivered tokens go unbilled. Narrow race; user-favor.- The outer-catch seam passes
steps: [], losing the finished-step usage floor (falls back to the deliveredText estimate; tool-call deltas not accumulated). User-favor corner.
Fixing either should be done in both routes together, not here. Money-path merge stays with @lalalune per lane rules.
[cloud-audit]
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Summary
Fixes #11513 (money — under-collection). An aborted streaming request on
POST /api/v1/messagessettled its upfront credit reservation to 0 — a full refund — in both thestreamTextonAbortcallback and the outer stream catch. The tokens already streamed to the client were really generated and really billed to us by the provider, so every client abort leaked the delivered-token cost. This ports the partial-settle pattern already merged for/v1/chat/completionsin #11472.Changes —
packages/cloud/api/v1/messages/route.tsdeliveredTextfrom the SSEtext-deltaloop.settleStreamingAbortReservation()+summarizeFinishedStepUsage()(messages-route equivalents of the chat route's non-exported helpers): on abort, billmax(estimatedInputTokens, finished-step input)+max(estimateTokens(deliveredText), finished-step output)viabillUsage, settle the reservation at that cost, and record analytics with theclient_aborted_streammarker (isSuccessful: false). If billing itself fails, fall back to a full refund so the hold is never leaked.onAbort(receives finishedsteps) and the abort-capable outer stream catch, gated onabortSignal?.aborted === true. Non-abort provider errors still refund to 0.settleStreamingOnceguard composed over the existing settler soonFinish/onAbort/onError/ outer-catch races cannot double-bill or double-record. The sharedcreateCreditReservationSettleris untouched (that's cloud/money: createCreditReservationSettler reset-on-throw + non-idempotent reconcile refund → cashable double-mint on monetized-app inference #11512, [cloud-money] lane) — the guard composes on top of its first-call-wins idempotency.__tests__/messages-iac-fast-path.test.ts's wholesale@/lib/pricingmock to includeestimateTokens(the route's new import otherwise fails that test's module load).Test —
packages/cloud/api/__tests__/messages-streaming-abort-billing.test.tsMirrors
chat-completions-streaming-credit-leak.test.ts: drives the route's real streaming handler (via a__streamingCreditTestHooksseam) with a mockedstreamTextboundary and the realcreateCreditReservationSettleragainst a ledger-backed reservation:onAbortafter delivered text deltas settles to estimated-input + delivered-output cost (> 0, exact),billUsagecalled once — not a full refund.AbortErrorthrow in the outer catch (SDKonAbortnever invoked) takes the same partial-settle path.onAbortracing the outer catch single-flights: one reconcile, onebillUsage, one analytics record.Mutation check
Reverted both abort settlements to
settleReservation(0)and re-ran:(The surviving test is the intentional non-abort refund path.) Fix restored → 4/4 green.
Verification
bunx biome check --writeclean on changed files.tsgo --noEmit): no errors in changed files; the only errors on latest develop are pre-existing in../shared/src/lib/services/team-credential-pool/*(introduced by feat(cloud): org api-key credential pool backend (#11332) #11487, missing@elizaos/app-core/account-poollocal build artifact — untouched here).bun test __tests__on this branch: 14 pre-existing local failures vs 21 on a clean develop baseline (stash/run/pop) — strictly fewer, none new, none in the changed surface; the chat-completions streaming-credit-leak suite and this new suite both pass in full-suite order.[cloud-security]
Adversarial review (independent Fable agent):
correct-with-nits, closes the leak, test is mutation-worthy. The only findings are nits that are parity with the merged #11472 (an onError-wins-race and tool-call-delta accounting corner) — both err toward under-collection (favor the user), not over-charging, and match the chat route exactly. SharedcreateCreditReservationSettleruntouched (#11512 lane). Money-path → @lalalune for merge; not self-merged.