fix(cloud): thread requestId into /v1/messages billUsage (affiliate dedupe) + unconditional settlement cache (#12871) - #12938
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 |
lalalune
left a comment
There was a problem hiding this comment.
Blocking on the billing requestId source.
billUsage.requestId feeds the affiliate/creator earnings dedupe key. This PR sets it from getRequestIdempotencyKey() at packages/cloud/api/v1/messages/route.ts:724-728, which is header/client-derived. The chat/completions route documents the exact hazard this creates at packages/cloud/api/v1/chat/completions/route.ts:877-887: billing requestId must not be client-controllable, or a caller can pin x-request-id/an idempotency key across two real billed requests and suppress the second affiliate/creator credit while still paying the org charge.
Please mirror chat/completions more closely: server-generate the billing requestId once per request (crypto.randomUUID()), and keep the client retry key only as the reservation idempotency key, e.g. const requestId = crypto.randomUUID(); const idempotencyKey = getRequestIdempotencyKey() ?? requestId; then thread the server-generated requestId into all billUsage contexts.
Local checks: git diff --check origin/develop...pr-12938 passed. bun test packages/cloud/api/__tests__/messages-abort-partial-settle.test.ts printed all 5 tests as passing, then hung in coverage/report finalization and was interrupted.
|
Focused local validation in an isolated checkout: Passed:
Blocked/baseline in this temp checkout:
Read-through: the route now creates one billing One test hardening request before this money-path PR is merged: the added abort assertion should verify the exact stable id, not just truthy string. The helper passes |
… earnings dedupe (no double-accrue) (#12871) /v1/messages billUsage calls (non-stream, stream-finish, abort settler) carried no requestId, so getAffiliateEarningsSourceId fell back to legacy_<uuid> per call — defeating dedupeBySourceId. A retried request could double-accrue cashable affiliate/creator earnings. chat/completions threads a stable requestId (#11588); messages diverged. Fix: define a request-stable requestId (getRequestIdempotencyKey ?? uuid, so a client retry with an idempotency key dedupes) at the top of the handler and thread it through handleStream / handleNonStream / settleStreamingAbortReservation into all three billUsage contexts. Also (Part A, #11512): settleStreamingOnce now caches the settlement promise unconditionally — a rejected settlement must never be re-run by a racing path (re-bill/re-record); the inner settler is first-call-wins idempotent and retries reconcile legs safely. Test: extended messages-abort-partial-settle — asserts the abort billing context carries a stable requestId. 5/0 green; red without the fix (4/1, requestId undefined). typecheck + biome clean. Closes item 1 of #12871. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7b74b87 to
9d1393d
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Local validation after rebasing onto current
Attempted but blocked in this sparse worktree / borrowed install:
I also read the money-path diff manually against issue #12871: it threads the stable request id through all three messages billing contexts and changes |
|
@lalalune — your CHANGES_REQUESTED was correct and landed unaddressed: the merged head ( const requestId = getRequestIdempotencyKey() ?? crypto.randomUUID();Follow-up PR #12994 applies exactly the pattern your review asked for — — [cloud-security] |
…e client-controllable affiliate-dedupe key (#12938 review, #11588 class) lalalune's CHANGES_REQUESTED on #12938 landed unaddressed: billUsage's requestId was still getRequestIdempotencyKey() ?? randomUUID(), i.e. header/client-derived. The billing requestId feeds the affiliate-earnings dedupe sourceId while the org charge is unconditional, so a caller pinning x-request-id/Idempotency-Key across two real billed requests suppresses the second affiliate/creator credit while the org still pays — the exact #11588 class chat/completions already guards against (route.ts:877-887). Mirror chat/completions: requestId = crypto.randomUUID() once per request (still request-stable across the stream-finish/abort/non-stream settle contexts, preserving the single-flight dedupe #12938 added); the client retry key remains only the reservation idempotencyKey (#10423).
…e client-controllable affiliate-dedupe key (#12938 review, #11588 class) (#12994) lalalune's CHANGES_REQUESTED on #12938 landed unaddressed: billUsage's requestId was still getRequestIdempotencyKey() ?? randomUUID(), i.e. header/client-derived. The billing requestId feeds the affiliate-earnings dedupe sourceId while the org charge is unconditional, so a caller pinning x-request-id/Idempotency-Key across two real billed requests suppresses the second affiliate/creator credit while the org still pays — the exact #11588 class chat/completions already guards against (route.ts:877-887). Mirror chat/completions: requestId = crypto.randomUUID() once per request (still request-stable across the stream-finish/abort/non-stream settle contexts, preserving the single-flight dedupe #12938 added); the client retry key remains only the reservation idempotencyKey (#10423). Co-authored-by: NubsCarson <nubscarson@users.noreply.github.com>
|
Thanks @lalalune — you're right, that's a real hazard (client-pinned |
Closes item 1 of #12871 (demo-golden-path hunt).
[cloud-money].Bug:
/v1/messagesbillUsage contexts (all 3: non-stream, stream, abort) carried norequestId→getAffiliateEarningsSourceIdfell back tolegacy_<uuid>→ defeateddedupeBySourceId→ a retried request could double-accrue cashable affiliate/creator earnings. chat/completions threads it (#11588); messages diverged.Fix: define a request-stable
requestId(getRequestIdempotencyKey() ?? uuid) and thread it throughhandleStream/handleNonStream/settleStreamingAbortReservationinto all 3 billUsage contexts. Part A (#11512):settleStreamingOncecaches unconditionally (no reset-on-throw) so a racing path can't re-run a failed settlement.Proof: extended the abort test to assert the billing context carries a stable requestId — 5/0 green; red without the fix (requestId undefined). typecheck + biome clean. Money path — no self-merge.