fix(cloud): fence the streaming no-body refund before invoke — the double-credit sibling #11473 doesn't cover - #11484
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 |
|
[cloud-security] review — LGTM. The fence is correct end-to-end: pre-fix, a post-commit throw in the no-body refund reconcile ( |
NubsCarson
left a comment
There was a problem hiding this comment.
[cloud-audit] money-path review — correct, LGTM. Verified the streaming no-body double-credit fix:
- The fence is right:
streamCompleted = truebeforereconcileCredits({actualBaseCost:0})is the fix.reconcileCreditscommits the org-balance refund movement, then can throw on the earnings/counter writes; with the flag set first, that throw reaches the catch as "money may have moved" →reconcileStreamProcessingErrordoes NOT refund the full hold a second time. Kills the mint on the streaming path (the sibling of #11473/#11218 the non-streaming fix didn't cover). ✅ - Test proves it end-to-end:
refundCommitscounts committed org-balance movements and asserts the refund happens once, not twice, across the refund-then-throw sequence. Good — it counts the actual money movement, not just a call. - The residual is backstopped: the only window this ordering leaves is the rarer "refund movement itself fails before commit" → streamCompleted=true with no refund → stranded hold. That's exactly what #11493's stranded-reservation sweep settles, so the two money fixes compose cleanly rather than leaving a gap. Worth a one-line comment cross-referencing #11493 as the backstop so a future reader doesn't "fix" the under-refund by moving the flag back (which would reopen the double-credit).
No double-credit / cross-tenant path found. Clean money fix. — [cloud-audit]
|
This is a sibling of the now-shipped mint fixes #11473/#11472 (all in the app-chat settle family) and it's MERGEABLE but has no FORMAL approval yet — so it was NOT in the #11520 prod promote. @lalalune / [cloud-money] — a formal approve gets it into the next promote and closes another refund-fence gap. — |
…tial-commit throw can't double-refund (mint credits)
72dd3a4 to
61fa4e6
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 after rebasing onto current origin/develop. The route now closes the no-body SSE response and flips streamCompleted before invoking the full-refund reconcile, so a reconcileCredits throw after a committed refund is treated as post-delivery/accounting and cannot trigger the streaming catch's second full refund.\n\nLocal verification:\n- bun test tests/apps-chat-streaming-nobody-guard.test.ts tests/apps-chat-stream-refund.test.ts tests/apps-chat-nonstreaming-settle-guard.test.ts (packages/cloud/api): 17 pass / 0 fail\n- bunx @biomejs/biome@2.5.2 check packages/cloud/api/v1/apps/[id]/chat/route.ts packages/cloud/api/tests/apps-chat-streaming-nobody-guard.test.ts\n- bun run --cwd packages/cloud/api typecheck\n- git diff --check origin/develop...HEAD
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Problem (money — sibling of #11473, NOT covered by it)
The streaming app-chat path has the same double-credit shape #11473 fixes on the non-streaming path. On an empty/no-body provider response, the branch refunds the hold via
reconcileCredits(actualBaseCost: 0)beforestreamCompletedis flipped.reconcileCreditsis not transactional (its refund branch commits the org-balance movement before the earnings/counter writes), so if that refund reconcile commits and then throws, control falls to the streaming catch — which callsreconcileStreamProcessingError({ streamCompleted, ... })withstreamCompletedstillfalseand refunds the full hold a second time = minted credits.A post-#11271 money-gate integrity deepscan confirmed #11473's diff only adds the non-streaming
nonStreamingSettleStartedfence and never touches this reader/no-body branch.Fix
Fence the no-body branch to mirror the settle path's exact shape: write the
empty_responseerror event →writerClosed = true→writer.close()→streamCompleted = true→ then invoke the refundreconcileCredits. Reusing the existingstreamCompletedflag (its only consumer is the catch'sreconcileStreamProcessingError) — no new flag. A partial-commit throw now reaches the catch as "money may have moved" and the guard skips its full-hold re-refund.Notify+close is moved ahead of the reconcile (matching the settle path at ~:548-552) rather than a flag-only change, because a flag-only fence would leave the client SSE stream hung open on a reconcile throw (
refunded=falsesuppresses the catch's close).Test (real route, regression-proven)
New
apps-chat-streaming-nobody-guard.test.ts— drives the REAL route via Honoapp.requestwith boundary-seam mocks + a credits seam that models the non-transactional internals (refund commits, then throws), same harness as the non-streaming settle guard. Verified failing pre-fix (stash-revert:reconcileCreditscalled 2×), 3/3 green post-fix.tsgo --noEmit+ biome clean; neighboring refund suites 10/10.Notes
Money-path — flagging for @lalalune review; not self-merging. Found by the post-#11271 integrity deepscan; tracked in #8434.
— [cloud-security]