fix(credits): apply a due monthly refill at the gate before denying - #839
Conversation
The credit gate read the raw balance, so the first credit-gated request
after a refill boundary saw the stale pre-refill balance and 402d until
something called GET /accounts/{id}/credits, the only checkAndResetCredits
caller (recoupable/chat#1962).
On a shortfall the gate now runs checkAndResetCredits and re-evaluates with
the refreshed balance. The happy path stays a single read: the refill is a
floor, so it can only raise the balance and cannot turn a pass into a fail,
which makes skipping it safe whenever the raw balance already covers the
cost.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesCredit availability recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The credit gate now applies a due monthly refill before denying a request, with localized tests covering the changed behavior; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 2/5
- In
lib/credits/checkCreditsAvailable.ts, concurrent calls can both pass the refill path and apply the same monthly reset twice, which can over-credit an account and distort authorization decisions/billing outcomes — make the refill update atomic (conditional update/lock/transaction) so only one caller can perform it. - In
lib/credits/checkCreditsAvailable.ts, shortfall handling now routes throughcheckAndResetCredits, which can throw via subscription-state lookup or credits reads/writes; that turns a normal “insufficient credits” path into request failures under transient DB/service issues — add defensive error handling/fallback behavior and cover this path with failure-mode tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/credits/checkCreditsAvailable.ts">
<violation number="1" location="lib/credits/checkCreditsAvailable.ts:45">
P1: When concurrent requests hit a due, low balance, this call can apply the same monthly refill twice. Make the refill conditional or serialize it so only one caller can raise the balance before authorization.</violation>
<violation number="2" location="lib/credits/checkCreditsAvailable.ts:45">
P2: On a shortfall, the gate now calls `checkAndResetCredits`, which can throw: it runs `getAccountSubscriptionState` and, when a refill is due, `updateCreditsUsage` — and both `selectCreditsUsage`/`updateCreditsUsage` throw on Supabase errors. `ensureCreditsOrShortCircuit` has no try/catch, so any of these failures now propagates and turns what used to be a guaranteed graceful 402 into a 500 on the shortfall path. The updated JSDoc also still claims "It reads, and does nothing else", which is no longer true once a refill is applied. Decide explicitly whether the gate should swallow refill errors (return the 402 with the pre-refill balance) or let them bubble, and update the doc comment to reflect that the shortfall path performs a write/refill.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client / Caller
participant Gate as ensureCreditsOrShortCircuit
participant Checker as checkCreditsAvailable
participant DB as Supabase credits_usage
participant Refill as checkAndResetCredits
Note over Client,Refill: Credit Gating Flow
Client->>Gate: Request with creditsToDeduct
Gate->>Checker: checkCreditsAvailable(accountId, creditsToDeduct)
Note over Checker: Single-read happy path
Checker->>DB: selectCreditsUsage(accountId)
DB-->>Checker: remaining_credits row
alt Sufficient balance (remaining >= creditsToDeduct)
Checker-->>Gate: { kind: "available" }
Gate-->>Client: Allow request
else Shortfall (remaining < creditsToDeduct)
Note over Checker,Refill: Apply due monthly refill before denying
Checker->>Refill: checkAndResetCredits(accountId)
alt Refill due
Refill->>DB: Apply floor refill, advance timestamp
DB-->>Refill: Refreshed credits_usage row
Refill-->>Checker: creditsUsage with refreshed balance
else No refill due or already applied
Refill->>DB: Read current state
DB-->>Refill: Current credits_usage
Refill-->>Checker: creditsUsage (unchanged)
end
Checker->>Checker: Extract refreshed balance (null => 0)
alt Refilled balance covers cost
Checker-->>Gate: { kind: "available" }
Gate-->>Client: Allow request
else Still insufficient after refill
Checker-->>Gate: { kind: "insufficient_credits", remainingCredits: refreshed }
Gate-->>Client: Deny (402) with refreshed balance
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| if (remaining >= creditsToDeduct) return { kind: "available" }; | ||
|
|
||
| const { creditsUsage } = await checkAndResetCredits(accountId); |
There was a problem hiding this comment.
P1: When concurrent requests hit a due, low balance, this call can apply the same monthly refill twice. Make the refill conditional or serialize it so only one caller can raise the balance before authorization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/credits/checkCreditsAvailable.ts, line 45:
<comment>When concurrent requests hit a due, low balance, this call can apply the same monthly refill twice. Make the refill conditional or serialize it so only one caller can raise the balance before authorization.</comment>
<file context>
@@ -35,9 +42,14 @@ export async function checkCreditsAvailable(
if (remaining >= creditsToDeduct) return { kind: "available" };
+ const { creditsUsage } = await checkAndResetCredits(accountId);
+ const refreshed = creditsUsage?.remaining_credits ?? 0;
+
</file context>
|
|
||
| if (remaining >= creditsToDeduct) return { kind: "available" }; | ||
|
|
||
| const { creditsUsage } = await checkAndResetCredits(accountId); |
There was a problem hiding this comment.
P2: On a shortfall, the gate now calls checkAndResetCredits, which can throw: it runs getAccountSubscriptionState and, when a refill is due, updateCreditsUsage — and both selectCreditsUsage/updateCreditsUsage throw on Supabase errors. ensureCreditsOrShortCircuit has no try/catch, so any of these failures now propagates and turns what used to be a guaranteed graceful 402 into a 500 on the shortfall path. The updated JSDoc also still claims "It reads, and does nothing else", which is no longer true once a refill is applied. Decide explicitly whether the gate should swallow refill errors (return the 402 with the pre-refill balance) or let them bubble, and update the doc comment to reflect that the shortfall path performs a write/refill.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/credits/checkCreditsAvailable.ts, line 45:
<comment>On a shortfall, the gate now calls `checkAndResetCredits`, which can throw: it runs `getAccountSubscriptionState` and, when a refill is due, `updateCreditsUsage` — and both `selectCreditsUsage`/`updateCreditsUsage` throw on Supabase errors. `ensureCreditsOrShortCircuit` has no try/catch, so any of these failures now propagates and turns what used to be a guaranteed graceful 402 into a 500 on the shortfall path. The updated JSDoc also still claims "It reads, and does nothing else", which is no longer true once a refill is applied. Decide explicitly whether the gate should swallow refill errors (return the 402 with the pre-refill balance) or let them bubble, and update the doc comment to reflect that the shortfall path performs a write/refill.</comment>
<file context>
@@ -35,9 +42,14 @@ export async function checkCreditsAvailable(
if (remaining >= creditsToDeduct) return { kind: "available" };
+ const { creditsUsage } = await checkAndResetCredits(accountId);
+ const refreshed = creditsUsage?.remaining_credits ?? 0;
+
</file context>
Preview verification — the issue's exact Done-when, with a live prod contrastTested the PR-head preview ( Documented vs actual
Live prod contrast (prod has #835–#838 but not this PR)
That prod row is the bug reproduced live: the 402 didn't just deny the request, it left the refill unapplied for the next request too. State ledgerRestored to the true pre-test balance Local verification (from the PR body, unchanged)TDD red→green (2 RED on the behavior-changing cases), Ready to merge — closes the only open item chain in chat#1962. |
Implements recoupable/chat#1962 — the sole row in that issue's PR matrix.
The credit gate read the raw
credits_usagebalance, so the first credit-gated request after a monthly refill boundary saw the stale pre-refill balance and 402d spuriously until something calledGET /api/accounts/{id}/credits, the only place the refill ran.What changes
checkCreditsAvailablekeeps the single-read happy path, and on a shortfall runscheckAndResetCredits— the same refill the balance GET applies — then re-evaluates with the refreshed balance:checkAndResetCreditsnever called. Safe to skip because the refill is a floor (api#837): it can only raise a balance, so it cannot turn a pass into a fail, and can wait for the next balance read.remaining_credits.All eight credit-gated call sites go through
ensureCreditsOrShortCircuit, whose contract is unchanged — no call-site edits.Verification
TDD, red before green: added a
describe("a due-but-unapplied monthly refill")block with 4 cases (applied-on-shortfall passes, still-short 402s with the refreshed balance, happy path never consults the refill, missing row → zero). Ran it: RED, 2 failed (the two behavior-changing cases). Implemented: GREEN.vitest run lib/creditseslint lib/creditstsc --noEmitNot yet done: the issue's live Done-when (seed a spent balance + month-old
timestamp, confirm the first credit-gated call passes with no priorGET /credits, and the timestamp advances) against the preview. Flagging rather than implying it.Summary by cubic
Applies a due monthly credits refill during gating before denying, preventing false 402s right after a refill boundary. Previously the gate read the raw
credits_usagebalance; the first gated call after the boundary could 402 untilGET /api/accounts/{id}/creditsapplied the refill.checkCreditsAvailablenow callscheckAndResetCredits(accountId)and re-evaluates; it passes if the refilled balance covers the cost, otherwise 402s with the refreshedremainingCredits.ensureCreditsOrShortCircuit.Written for commit 0cc653d. Summary will update on new commits.
Summary by CodeRabbit