fix(cloud): refund the app-chat hold when the NON-streaming settle throws (#11169 part 1) - #11218
Merged
Merged
Conversation
…rows (#11169 part 1) The non-streaming app-chat path debits the upfront hold, then reads the provider body + calculateCost + reconcileCredits. Those run AFTER the provider-failure try/catch, and the route's outer catch returns 500 WITHOUT refunding — so a malformed body / cost / reconcile throw stranded the reserved hold. #10837's refund helper covered only the streaming branch. Fix: mirror #10837 — extend stream-refund.ts with reconcileNonStreamingSettleError (refund iff the settle did NOT complete; a throw after reconcile already charged must NOT refund, avoiding a double-credit), and wrap the non-streaming settle so any pre-reconcile throw refunds the hold (actualBaseCost 0) then rethrows to the outer handler. Refund is tagged streaming:false / refundReason for ledger clarity. Test (apps-chat-stream-refund, +3): not-settled → full refund; settled-then-threw → no refund (no double-credit); refund metadata tagged non-streaming. Part 1 of 3 in #11169 (part 2 = #11206). Part 3 (stranded synchronous reservation sweep) follows. Money-path — flagging for maintainer review/merge. [cloud-security]
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
|
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 |
6 tasks
…antics — no refund once reconcileCredits was invoked reconcileCredits is not transactional: its org-balance movement (refundCredits / reserveAndDeductCredits) commits before the earnings/counter writes and carries no idempotency key. Refunding on a mid-reconcile throw could therefore double-credit (settle already refunded reserved-actual, guard refunds full reserved again). Mirror the streaming branch, which flips streamCompleted BEFORE its settle for the same reason. A hold stranded by that rare window is recovered by the stranded-reservation sweep (#11169 part 3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
5 of 23 tasks
Member
|
QA (subagent-verified): nonStreamingSettleStarted flips before reconcile and the catch-side helper refunds only when false — refund and success reconcile are mutually exclusive by construction, including throws inside reconcile (stranded-hold case correctly deferred to part 3). 7/7 tests, red-proof confirms. Merging. |
lalalune
added a commit
that referenced
this pull request
Jul 2, 2026
…ps-chat settle (#11218 hardening) (#11286) The #11218 fix flips nonStreamingSettleStarted IMMEDIATELY BEFORE invoking appCreditsService.reconcileCredits — the whole point, since reconcileCredits is not transactional: its refund branch commits creditsService.refundCredits and can then throw from reverseCreatorEarnings / the apps-aggregate update. With the flag set only after return, that throw would reach the settle catch as "never settled" and the guard would refund the FULL hold a second time. The existing helper tests (apps-chat-stream-refund.test.ts) drive reconcileNonStreamingSettleError with a pre-computed boolean, so they stay green even if the route's flag ordering regresses. This suite drives the REAL route (mocked auth/app/provider/pricing seams, real stream-refund guard) with a credits seam modeling the real non-transactional internals — refund committed, then reverseCreatorEarnings throws — and asserts the committed refund COUNT end to end: - settle reconcile throws AFTER its refund committed → exactly ONE refund commit, no guard refund, original error surfaced unmasked (500) - malformed provider body / calculateCost throw (pre-reconcile) → guard auto-refunds exactly once, tagged refundReason non_streaming_settle_error - settle success → 200 with the provider body, exactly one reconcile Fail-without-fix verified: moving the flag assignment to after the reconcile await (the pre-hardening state) makes the post-reconcile-throw test red with reconcileCredits called 2× — the double-credit this guards against. Co-authored-by: lalalune <shaw.nicola.walters@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 2, 2026
#11271 regression tracker: ~260 files still silently reverted across lanes — per-lane cleanup
#11419
Closed
lalalune
pushed a commit
that referenced
this pull request
Jul 2, 2026
…sh (#11413) (#11422) PR #11271 (intended scope: one escrow file) squashed from a stale base and byte-reverted ~302 files, incl. merged cloud money gates still missing at develop tip. Blob-identity audit (develop-tip == clobbered blob, != correct pre-clobber blob at 5b714c7^) confirms these are exact reverts; restored from the pre-clobber parent: - provisioning-agent/route.ts — #11240 org-credit gate on dedicated-agent provisioning (checkAgentCreditGate → 402 insufficient_credits) was gone; zero/negative-balance orgs could provision free compute. - v1/apps/[id]/chat/stream-refund.ts — #11218 app-chat stream refund path. - shared/lib/steward-sync.ts — #11270 awaited-provisioning fix. - + restored provisioning-agent-default-image + apps-chat-stream-refund tests. Excluded #11271's INTENDED change (influencer-marketplace.ts escrow fix) — that content is correct and kept. Verified: cloud/api + cloud/shared typecheck clean (0 errors); restored tests 11 pass / 0 fail. Part of the #11413 umbrella restore (full blob audit posted there). Refs #11240 #11218 #11270.
Contributor
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
This was referenced Jul 2, 2026
NubsCarson
added a commit
that referenced
this pull request
Jul 2, 2026
…d-settle throw can't double-refund (mint credits) (#11473) The #11271 mass-revert reintroduced a double-credit on POST /api/v1/apps/:id/chat non-streaming: #11278 re-fixed the stranded-hold refund but set the settle flag AFTER reconcileCredits returns. reconcileCredits is not transactional — it commits its org-balance refund before its earnings/counter writes — so a mid-settle throw (e.g. reverseCreatorEarnings deadlock) reached the catch as 'never settled' and refunded the FULL hold a second time (total credited = 2x reserved − actual). Restore the #11218 fence: flip nonStreamingSettleStarted IMMEDIATELY BEFORE invoking reconcileCredits and route the catch through reconcileNonStreamingSettleError (settleStarted gating + non_streaming_settle_error ledger tag), wrapped in .catch so a refund failure can't mask the original error. Collapse the divergent dead reconcileNonStreamProcessingError helper (+ its test) into the single settle helper. The route-level guard test (apps-chat-nonstreaming-settle-guard) was RED at develop tip (3 fail/1 pass, reconcileCredits called 2x, masked by concurrency-cancelled CI); now 4/4 green. apps-chat-stream-refund 7/7 green.
This was referenced Jul 2, 2026
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
2 participants
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.
Part 1 of #11169.
Problem (MED — credit leak)
The non-streaming
apps/[id]/chatpath debits the upfront hold (deductCredits), then reads the provider body (await providerResponse.json()), runscalculateCost, andreconcileCredits. Those run after the provider-failure try/catch, and the route's outer catch returns 500 without refunding. So a malformed body / pricing-catalog throw after the debit strands the reserved hold. #10837's refund helper (reconcileStreamProcessingError) covered only the streaming branch.Fix (mirror #10837's testable-extraction pattern)
reconcileNonStreamingSettleErrorinstream-refund.ts: refund iff the settle reconcile was never invoked (settleStartedflips immediately beforereconcileCredits). A throw before the settle means the caller received no billable answer and no money moved beyond the hold — full refund, taggedstreaming: false+refundReasonfor ledger clarity.reconcileCreditsis not transactional: its org-balance movement (refundCredits/reserveAndDeductCredits) commits before the (non-co-transactional) earnings/counter writes and carries no idempotency key, so a blind refund on a mid-reconcile throw could double-credit (settle already refundedreserved−actual; guard would refund the fullreservedagain → minted credits, systemically during a DB blip). This mirrors the streaming branch, which flipsstreamCompletedbefore its settle for the same reason. A hold stranded by that rare window is recovered by the stranded-reservation sweep (part 3).Test (
apps-chat-stream-refund.test.ts, +3)actualBaseCost 0);packages/cloud/apitypecheck + biome clean.Part 1 of 3 in #11169 (part 2 = #11206 reserve-sizing). Part 3 (stranded synchronous reservation sweep) follows. Money-path — flagging for maintainer review/merge. `[cloud-security]`