From bb1a29eee3d7d06d6d6d746a7a89af13bcb6140d Mon Sep 17 00:00:00 2001 From: NubsCarson Date: Thu, 2 Jul 2026 06:13:15 +0000 Subject: [PATCH 1/2] fix(cloud): refund the app-chat hold when the NON-streaming settle throws (#11169 part 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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] --- .../__tests__/apps-chat-stream-refund.test.ts | 57 ++++++ packages/cloud/api/v1/apps/[id]/chat/route.ts | 166 +++++++++++------- .../api/v1/apps/[id]/chat/stream-refund.ts | 66 +++++++ 3 files changed, 229 insertions(+), 60 deletions(-) diff --git a/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts b/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts index 4e09be3c8ebaa..7b7a9315f6c40 100644 --- a/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts +++ b/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts @@ -12,6 +12,7 @@ */ import { describe, expect, mock, test } from "bun:test"; import { + reconcileNonStreamingSettleError, reconcileStreamProcessingError, type StreamRefundCredits, } from "../v1/apps/[id]/chat/stream-refund"; @@ -106,3 +107,59 @@ describe("reconcileStreamProcessingError (#10837)", () => { expect(credits.calls.every((c) => c.actualBaseCost === 0)).toBe(true); }); }); + +const nonStreamBase = { + appId: "app-1", + userId: "user-1", + reservedBaseCost: 0.05, + model: "openai/gpt-oss-120b", + provider: "openai", + billingSource: "openai", + errorMessage: "provider body was not valid JSON", +}; + +describe("reconcileNonStreamingSettleError (#11169 part 1)", () => { + test("settle threw BEFORE reconcile (not settled) → full refund (actualBaseCost 0)", async () => { + const credits = makeCredits(); + const result = await reconcileNonStreamingSettleError( + { ...nonStreamBase, settled: false }, + credits, + ); + expect(result.refunded).toBe(true); + expect(credits.reconcileCredits).toHaveBeenCalledTimes(1); + expect(credits.calls[0]).toEqual({ + estimatedBaseCost: 0.05, + actualBaseCost: 0, + }); + }); + + test("throw AFTER reconcile already charged (settled) → keep the charge, NO refund (no double-credit)", async () => { + const credits = makeCredits(); + const result = await reconcileNonStreamingSettleError( + { ...nonStreamBase, settled: true }, + credits, + ); + expect(result.refunded).toBe(false); + expect(credits.reconcileCredits).not.toHaveBeenCalled(); + }); + + test("the refund is tagged non-streaming (streaming:false) so it's distinguishable in the ledger", async () => { + const metaCalls: Array | undefined> = []; + const credits = { + reconcileCredits: mock( + async (args: { metadata?: Record }) => { + metaCalls.push(args.metadata); + return null; + }, + ), + } as unknown as StreamRefundCredits; + await reconcileNonStreamingSettleError( + { ...nonStreamBase, settled: false }, + credits, + ); + expect(metaCalls[0]).toMatchObject({ + streaming: false, + refundReason: "non_streaming_settle_error", + }); + }); +}); diff --git a/packages/cloud/api/v1/apps/[id]/chat/route.ts b/packages/cloud/api/v1/apps/[id]/chat/route.ts index 2cfb11f4fab56..774af84eb653a 100644 --- a/packages/cloud/api/v1/apps/[id]/chat/route.ts +++ b/packages/cloud/api/v1/apps/[id]/chat/route.ts @@ -46,7 +46,10 @@ import { logger } from "@/lib/utils/logger"; import { getRouteTimeoutMs } from "@/lib/utils/request-timeout"; import type { AppEnv } from "@/types/cloud-worker-env"; import { reservationOutputTokens } from "./chat-reservation"; -import { reconcileStreamProcessingError } from "./stream-refund"; +import { + reconcileNonStreamingSettleError, + reconcileStreamProcessingError, +} from "./stream-refund"; const ROUTE_MAX_DURATION = 800; @@ -659,73 +662,116 @@ async function handlePOST( ); } - // Non-streaming response - const responseData = (await providerResponse.json()) as { - usage?: { prompt_tokens?: number; completion_tokens?: number }; - choices?: Array<{ message?: { content?: string } }>; - }; - - // Calculate actual cost - use fallback estimation if provider doesn't return usage - let actualInputTokens = responseData.usage?.prompt_tokens || 0; - let actualOutputTokens = responseData.usage?.completion_tokens || 0; - - // Fallback: estimate tokens if usage not provided (matching streaming behavior) - if (actualInputTokens === 0 && actualOutputTokens === 0) { - const outputContent = responseData.choices?.[0]?.message?.content || ""; - actualInputTokens = estimatedInputTokens; // Use pre-calculated estimate - actualOutputTokens = estimateTokens(outputContent); - - logger.warn("[App Chat] No usage data in response, using estimates", { - appId, + // Non-streaming response. The body-read + cost-calc + reconcile below run + // AFTER the upfront debit, and the outer catch returns 500 WITHOUT refunding + // — so a malformed body / calculateCost / reconcile throw here stranded the + // reserved hold (#11169 part 1; the streaming branch was already guarded). + // Guard the settle path: any throw before the reconcile lands refunds the + // hold, then rethrows to the outer error handler. + let nonStreamingSettled = false; + try { + const responseData = (await providerResponse.json()) as { + usage?: { prompt_tokens?: number; completion_tokens?: number }; + choices?: Array<{ message?: { content?: string } }>; + }; + + // Calculate actual cost - use fallback estimation if provider doesn't return usage + let actualInputTokens = responseData.usage?.prompt_tokens || 0; + let actualOutputTokens = responseData.usage?.completion_tokens || 0; + + // Fallback: estimate tokens if usage not provided (matching streaming behavior) + if (actualInputTokens === 0 && actualOutputTokens === 0) { + const outputContent = responseData.choices?.[0]?.message?.content || ""; + actualInputTokens = estimatedInputTokens; // Use pre-calculated estimate + actualOutputTokens = estimateTokens(outputContent); + + logger.warn("[App Chat] No usage data in response, using estimates", { + appId, + actualInputTokens, + actualOutputTokens, + }); + } + + const { totalCost: actualBaseCost } = await calculateCost( + normalizedModel, + provider, actualInputTokens, actualOutputTokens, - }); - } + billingSource, + ); - const { totalCost: actualBaseCost } = await calculateCost( - normalizedModel, - provider, - actualInputTokens, - actualOutputTokens, - billingSource, - ); + // Reconcile the difference between reserved and actual costs + // Pass app to avoid N+1 query (app already fetched above) + const reconciliation = await appCreditsService.reconcileCredits({ + appId, + userId: user.id, + estimatedBaseCost: reservedBaseCost, + actualBaseCost, + description: `Chat reconciliation: ${model}`, + metadata: { + model, + provider, + billingSource, + inputTokens: actualInputTokens, + outputTokens: actualOutputTokens, + streaming: false, + }, + app, + }); + // The hold is now settled (charged to actual) — no refund on later throws. + nonStreamingSettled = true; - // Reconcile the difference between reserved and actual costs - // Pass app to avoid N+1 query (app already fetched above) - const reconciliation = await appCreditsService.reconcileCredits({ - appId, - userId: user.id, - estimatedBaseCost: reservedBaseCost, - actualBaseCost, - description: `Chat reconciliation: ${model}`, - metadata: { + const duration = Date.now() - startTime; + logger.info("[App Chat] Request completed", { + appId, + userId: user.id, model, - provider, - billingSource, + duration, inputTokens: actualInputTokens, outputTokens: actualOutputTokens, - streaming: false, - }, - app, - }); - - const duration = Date.now() - startTime; - logger.info("[App Chat] Request completed", { - appId, - userId: user.id, - model, - duration, - inputTokens: actualInputTokens, - outputTokens: actualOutputTokens, - reservedBaseCost, - actualBaseCost, - reconciliation: { - action: reconciliation.action, - amount: reconciliation.adjustedAmount, - }, - }); + reservedBaseCost, + actualBaseCost, + reconciliation: { + action: reconciliation.action, + amount: reconciliation.adjustedAmount, + }, + }); - return withCors(Response.json(responseData)); + return withCors(Response.json(responseData)); + } catch (nonStreamingError) { + // Refund the upfront hold if the settle didn't complete (#11169 part 1). + // Best-effort: a refund failure is logged inside the helper's caller but + // never masks the original error surfaced to the client. + await reconcileNonStreamingSettleError( + { + settled: nonStreamingSettled, + appId, + userId: user.id, + reservedBaseCost, + model, + provider, + billingSource, + errorMessage: + nonStreamingError instanceof Error + ? nonStreamingError.message + : String(nonStreamingError), + }, + appCreditsService, + ).catch((refundError) => { + logger.error( + "[App Chat] refund after non-streaming settle failure ALSO failed — hold stranded", + { + appId, + userId: user.id, + error: + refundError instanceof Error + ? refundError.message + : String(refundError), + }, + ); + }); + throw nonStreamingError; + } } catch (error) { logger.error("[App Chat] Error:", error); diff --git a/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts b/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts index 5891a4b097dcf..1f81e9982212a 100644 --- a/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts +++ b/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts @@ -68,3 +68,69 @@ export async function reconcileStreamProcessingError( }); return { refunded: true }; } + +/** + * Money-critical (#11169 part 1): the NON-streaming app-chat path debits the + * upfront hold, then reads the provider body + runs `calculateCost` + + * `reconcileCredits`. If any of those throw AFTER the debit, the route's outer + * catch returns 500 WITHOUT refunding — stranding the reserved hold. Unlike the + * streaming case there is no "already delivered" ambiguity: a non-streaming + * settle failure means the caller received no billable answer, so refund the + * hold whenever the settle did NOT complete. + * + * `settled` is true once `reconcileCredits` has charged the actual cost; a throw + * after that point must NOT refund (it would double-credit the org). + */ +export async function reconcileNonStreamingSettleError( + params: { + settled: boolean; + appId: string; + userId: string; + reservedBaseCost: number; + model: string; + provider: string; + billingSource: string; + errorMessage: string; + }, + credits: StreamRefundCredits, +): Promise<{ refunded: boolean }> { + const { + settled, + appId, + userId, + reservedBaseCost, + model, + provider, + billingSource, + errorMessage, + } = params; + + if (settled) { + logger.error( + "[App Chat] Non-streaming post-settle threw AFTER reconcile; keeping charge (NOT refunding)", + { appId, userId, reservedBaseCost, error: errorMessage }, + ); + return { refunded: false }; + } + + logger.error( + "[App Chat] Non-streaming settle failed after debit; refunding reserved hold (#11169)", + { appId, userId, reservedBaseCost, error: errorMessage }, + ); + await credits.reconcileCredits({ + appId, + userId, + estimatedBaseCost: reservedBaseCost, + actualBaseCost: 0, // Full refund — nothing was billed. + description: `Chat refund (non-streaming settle failed): ${model}`, + metadata: { + error: true, + streaming: false, + model, + provider, + billingSource, + refundReason: "non_streaming_settle_error", + }, + }); + return { refunded: true }; +} From d09af306f7e76e655e79a7dc718b826d7a2fdb97 Mon Sep 17 00:00:00 2001 From: lalalune Date: Wed, 1 Jul 2026 23:54:16 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(cloud):=20flip=20the=20non-streaming=20?= =?UTF-8?q?refund=20guard=20to=20settle-STARTED=20semantics=20=E2=80=94=20?= =?UTF-8?q?no=20refund=20once=20reconcileCredits=20was=20invoked?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/apps-chat-stream-refund.test.ts | 10 +++--- packages/cloud/api/v1/apps/[id]/chat/route.ts | 29 ++++++++++------- .../api/v1/apps/[id]/chat/stream-refund.ts | 31 ++++++++++++------- 3 files changed, 42 insertions(+), 28 deletions(-) diff --git a/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts b/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts index 7b7a9315f6c40..42ce94efd774d 100644 --- a/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts +++ b/packages/cloud/api/__tests__/apps-chat-stream-refund.test.ts @@ -119,10 +119,10 @@ const nonStreamBase = { }; describe("reconcileNonStreamingSettleError (#11169 part 1)", () => { - test("settle threw BEFORE reconcile (not settled) → full refund (actualBaseCost 0)", async () => { + test("throw BEFORE the settle reconcile was invoked → full refund (actualBaseCost 0)", async () => { const credits = makeCredits(); const result = await reconcileNonStreamingSettleError( - { ...nonStreamBase, settled: false }, + { ...nonStreamBase, settleStarted: false }, credits, ); expect(result.refunded).toBe(true); @@ -133,10 +133,10 @@ describe("reconcileNonStreamingSettleError (#11169 part 1)", () => { }); }); - test("throw AFTER reconcile already charged (settled) → keep the charge, NO refund (no double-credit)", async () => { + test("throw at/after the settle reconcile (incl. from INSIDE it — movement may have committed) → NO refund (no double-credit)", async () => { const credits = makeCredits(); const result = await reconcileNonStreamingSettleError( - { ...nonStreamBase, settled: true }, + { ...nonStreamBase, settleStarted: true }, credits, ); expect(result.refunded).toBe(false); @@ -154,7 +154,7 @@ describe("reconcileNonStreamingSettleError (#11169 part 1)", () => { ), } as unknown as StreamRefundCredits; await reconcileNonStreamingSettleError( - { ...nonStreamBase, settled: false }, + { ...nonStreamBase, settleStarted: false }, credits, ); expect(metaCalls[0]).toMatchObject({ diff --git a/packages/cloud/api/v1/apps/[id]/chat/route.ts b/packages/cloud/api/v1/apps/[id]/chat/route.ts index 774af84eb653a..4672d49e6ae07 100644 --- a/packages/cloud/api/v1/apps/[id]/chat/route.ts +++ b/packages/cloud/api/v1/apps/[id]/chat/route.ts @@ -664,11 +664,16 @@ async function handlePOST( // Non-streaming response. The body-read + cost-calc + reconcile below run // AFTER the upfront debit, and the outer catch returns 500 WITHOUT refunding - // — so a malformed body / calculateCost / reconcile throw here stranded the - // reserved hold (#11169 part 1; the streaming branch was already guarded). - // Guard the settle path: any throw before the reconcile lands refunds the - // hold, then rethrows to the outer error handler. - let nonStreamingSettled = false; + // — so a malformed body / calculateCost throw here stranded the reserved + // hold (#11169 part 1; the streaming branch was already guarded). + // Guard the settle path: any throw BEFORE the reconcile is invoked refunds + // the hold, then rethrows to the outer error handler. Once the reconcile + // has been invoked we never refund — reconcileCredits is not transactional, + // so a mid-flight throw may have already committed the org-balance movement + // and a blind refund would double-credit (same reason the streaming branch + // flips streamCompleted before ITS settle). A hold stranded by that rare + // window is recovered by the stranded-reservation sweep (#11169 part 3). + let nonStreamingSettleStarted = false; try { const responseData = (await providerResponse.json()) as { usage?: { prompt_tokens?: number; completion_tokens?: number }; @@ -702,6 +707,10 @@ async function handlePOST( // Reconcile the difference between reserved and actual costs // Pass app to avoid N+1 query (app already fetched above) + // Flag BEFORE the call: reconcileCredits commits its org-balance movement + // before its (non-co-transactional) earnings/counter writes, so a throw + // from inside it must NOT trigger the refund below (double-credit). + nonStreamingSettleStarted = true; const reconciliation = await appCreditsService.reconcileCredits({ appId, userId: user.id, @@ -718,8 +727,6 @@ async function handlePOST( }, app, }); - // The hold is now settled (charged to actual) — no refund on later throws. - nonStreamingSettled = true; const duration = Date.now() - startTime; logger.info("[App Chat] Request completed", { @@ -739,12 +746,12 @@ async function handlePOST( return withCors(Response.json(responseData)); } catch (nonStreamingError) { - // Refund the upfront hold if the settle didn't complete (#11169 part 1). - // Best-effort: a refund failure is logged inside the helper's caller but - // never masks the original error surfaced to the client. + // Refund the upfront hold if the settle reconcile was never invoked + // (#11169 part 1). Best-effort: a refund failure is logged but never + // masks the original error surfaced to the client. await reconcileNonStreamingSettleError( { - settled: nonStreamingSettled, + settleStarted: nonStreamingSettleStarted, appId, userId: user.id, reservedBaseCost, diff --git a/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts b/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts index 1f81e9982212a..fe56d71f3c8de 100644 --- a/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts +++ b/packages/cloud/api/v1/apps/[id]/chat/stream-refund.ts @@ -72,18 +72,25 @@ export async function reconcileStreamProcessingError( /** * Money-critical (#11169 part 1): the NON-streaming app-chat path debits the * upfront hold, then reads the provider body + runs `calculateCost` + - * `reconcileCredits`. If any of those throw AFTER the debit, the route's outer - * catch returns 500 WITHOUT refunding — stranding the reserved hold. Unlike the - * streaming case there is no "already delivered" ambiguity: a non-streaming - * settle failure means the caller received no billable answer, so refund the - * hold whenever the settle did NOT complete. + * `reconcileCredits`. If the body-read or cost-calc throws AFTER the debit, the + * route's outer catch returns 500 WITHOUT refunding — stranding the reserved + * hold. Refund it: the caller received no billable answer and no settle was + * ever attempted. * - * `settled` is true once `reconcileCredits` has charged the actual cost; a throw - * after that point must NOT refund (it would double-credit the org). + * `settleStarted` is true once `reconcileCredits` has been INVOKED — not once + * it returned. `reconcileCredits` is not transactional: it commits its + * org-balance movement (refund or extra charge) before its earnings/counter + * writes, and that movement carries no idempotency key. A throw from inside it + * may therefore have already moved money, so refunding blindly would + * double-credit the org (mint credits during a DB blip, systemically across + * concurrent requests). Mirror of 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). */ export async function reconcileNonStreamingSettleError( params: { - settled: boolean; + settleStarted: boolean; appId: string; userId: string; reservedBaseCost: number; @@ -95,7 +102,7 @@ export async function reconcileNonStreamingSettleError( credits: StreamRefundCredits, ): Promise<{ refunded: boolean }> { const { - settled, + settleStarted, appId, userId, reservedBaseCost, @@ -105,16 +112,16 @@ export async function reconcileNonStreamingSettleError( errorMessage, } = params; - if (settled) { + if (settleStarted) { logger.error( - "[App Chat] Non-streaming post-settle threw AFTER reconcile; keeping charge (NOT refunding)", + "[App Chat] Non-streaming throw at/after the settle reconcile; NOT refunding (movement may have committed — sweep recovers a stranded hold)", { appId, userId, reservedBaseCost, error: errorMessage }, ); return { refunded: false }; } logger.error( - "[App Chat] Non-streaming settle failed after debit; refunding reserved hold (#11169)", + "[App Chat] Non-streaming settle never started after debit; refunding reserved hold (#11169)", { appId, userId, reservedBaseCost, error: errorMessage }, ); await credits.reconcileCredits({