fix(credits): make the monthly refill a floor, not an assignment - #837
Conversation
checkAndResetCredits SET remaining_credits to the plan total once a refill was due, so any account holding more than its plan total was cut on its next balance read. Because the refill is lazy and read-triggered, the loss fired on a GET: reading a balance was what destroyed it, and nothing recorded that it had happened. On prod 2026-08-17, 119 accounts held more than the 333 free-plan total and 50 of those had a reset already pending (recoupable/chat#1960). The refill now raises a balance up to the plan total and never lowers one. The timestamp still advances on every due refill, including the no-op ones, or the account re-evaluates as refill-due on every subsequent read. remaining_credits is omitted from the update rather than written back as max(remaining, planTotal) when the balance already clears the total: writing a value read moments earlier would resurrect credits a concurrent deduction had spent. chat#1948 deferred this on the grounds that protecting a grant "would mean teaching checkAndResetCredits about grant provenance". It does not: a refill that never reduces protects a granted balance automatically, without the read path knowing where the balance came from. There is a test for exactly that.
|
Warning Review limit reached
Next review available in: 17 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ 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)
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/checkAndResetCredits.ts, the non-atomic absolute write during refill can overwrite a concurrent debit or top-up, causing users to lose or gain credits incorrectly under load — move the floor/refill logic into a single atomic database update so it evaluates and writes the same row state. - In
lib/credits/__tests__/checkAndResetCredits.test.ts, the two new tests currently cover the same free-tier/9999-credits path, so a real edge-case regression could still slip through undetected — split them to assert different branches or inputs (for example, distinct tiers or balance boundaries).
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/__tests__/checkAndResetCredits.test.ts">
<violation number="1" location="lib/credits/__tests__/checkAndResetCredits.test.ts:249">
P3: The new tests "leaves a balance ABOVE the plan total untouched" and "protects an admin grant on a free account" exercise the exact same code path: free tier, remaining_credits 9999 (above DEFAULT_CREDITS), same updateCreditsUsage mock, and identical assertions on the update payload and returned balance. The only difference is the narrative comment. If the intent is just to document that a grant survives without provenance, keep it; otherwise the two are functionally duplicate coverage and one could be dropped or merged to avoid implying they guard distinct branches.</violation>
</file>
<file name="lib/credits/checkAndResetCredits.ts">
<violation number="1" location="lib/credits/checkAndResetCredits.ts:72">
P1: When the balance is below the plan total, this absolute write can erase a concurrent debit or top-up. Apply the floor atomically in the database so the refill observes the balance it updates.</violation>
</file>
Architecture diagram
sequenceDiagram
participant API as API Route / Balance Read
participant C as checkAndResetCredits()
participant DB as Credits Usage DB
participant Sub as Subscription Service
Note over API,Sub: Monthly Credit Refill Flow (Lazy, Read-Triggered)
API->>C: Check credits usage for account
C->>DB: SELECT credits_usage row
DB-->>C: Row (remaining_credits, timestamp, account_id)
C->>Sub: Get account subscription state
Sub-->>C: isPro (boolean)
alt Refill due (>= 1 month since last update OR subscription started after)
C->>C: Determine planTotal (PRO_CREDITS or DEFAULT_CREDITS)
alt Balance below planTotal
C->>DB: UPDATE remaining_credits = planTotal, timestamp = now
else Balance at or above planTotal
Note over C: Floor rule applies - never lower balance
C->>DB: UPDATE timestamp = now only (leave remaining_credits untouched)
end
DB-->>C: Updated row
C-->>API: Return updated credits + isPro
else No refill due
C-->>API: Return existing credits + isPro
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const updates: Partial<Pick<CreditsUsage, "remaining_credits" | "timestamp">> = { | ||
| timestamp: new Date().toISOString(), | ||
| }; | ||
| if (remaining < planTotal) updates.remaining_credits = planTotal; |
There was a problem hiding this comment.
P1: When the balance is below the plan total, this absolute write can erase a concurrent debit or top-up. Apply the floor atomically in the database so the refill observes the balance it updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/credits/checkAndResetCredits.ts, line 72:
<comment>When the balance is below the plan total, this absolute write can erase a concurrent debit or top-up. Apply the floor atomically in the database so the refill observes the balance it updates.</comment>
<file context>
@@ -51,13 +58,20 @@ export async function checkAndResetCredits(accountId: string): Promise<CheckAndR
+ const updates: Partial<Pick<CreditsUsage, "remaining_credits" | "timestamp">> = {
+ timestamp: new Date().toISOString(),
+ };
+ if (remaining < planTotal) updates.remaining_credits = planTotal;
+
+ const refilled = await updateCreditsUsage({ account_id: accountId, updates });
</file context>
| expect(result.creditsUsage?.remaining_credits).toBe(25000); | ||
| }); | ||
|
|
||
| it("protects an admin grant on a free account without knowing it is a grant", async () => { |
There was a problem hiding this comment.
P3: The new tests "leaves a balance ABOVE the plan total untouched" and "protects an admin grant on a free account" exercise the exact same code path: free tier, remaining_credits 9999 (above DEFAULT_CREDITS), same updateCreditsUsage mock, and identical assertions on the update payload and returned balance. The only difference is the narrative comment. If the intent is just to document that a grant survives without provenance, keep it; otherwise the two are functionally duplicate coverage and one could be dropped or merged to avoid implying they guard distinct branches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/credits/__tests__/checkAndResetCredits.test.ts, line 249:
<comment>The new tests "leaves a balance ABOVE the plan total untouched" and "protects an admin grant on a free account" exercise the exact same code path: free tier, remaining_credits 9999 (above DEFAULT_CREDITS), same updateCreditsUsage mock, and identical assertions on the update payload and returned balance. The only difference is the narrative comment. If the intent is just to document that a grant survives without provenance, keep it; otherwise the two are functionally duplicate coverage and one could be dropped or merged to avoid implying they guard distinct branches.</comment>
<file context>
@@ -163,4 +163,124 @@ describe("checkAndResetCredits", () => {
+ expect(result.creditsUsage?.remaining_credits).toBe(25000);
+ });
+
+ it("protects an admin grant on a free account without knowing it is a grant", async () => {
+ // 9,999 granted to a free-tier account: no provenance is consulted, the
+ // floor rule alone keeps it.
</file context>
|
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. |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Preview verification — with a live before/after on the same accountTested the PR-head preview ( Documented vs actual
The before/after, live on the shared DBProd still runs the SET-refill, so the same procedure against
Both runs restored to the pre-test balance immediately (verified by re-read, final state Notes
Ready to merge. api#838 follows this one. |
Implements the third defect in recoupable/chat#1960. Row 5 of that issue's PR matrix. Independent of the auto-recharge PRs; no shared files.
checkAndResetCredits.tsL54-60 writesremaining_credits: isPro ? PRO_CREDITS : DEFAULT_CREDITSunconditionally once a monthly refill is due. A "refill" that lowers a balance is not a refill: any account holding more than its plan total, from a top-up or an admin grant, is cut on its next balance read. And because the refill is lazy and read-triggered, the loss fires on aGET— reading a balance is what destroys it, and nothing records that it happened.Measured on prod 2026-08-17:
What changes
Two decisions worth flagging, because the issue's Fix text asks for something slightly self-contradictory:
The timestamp always advances, so the write is not skipped. The issue says to "skip the
updateCreditsUsagecall entirely whenremaining_credits >= planTotal" and to "still bumptimestampin that branch". Those cannot both hold — and the issue's own reasoning says why the timestamp must win: without it the account re-evaluates as refill-due on every subsequent read, forever. So the write stays; what is skipped is theremaining_creditsfield, not the call. The issue's Done-when ("credits_usage.timestampadvances in all three cases") agrees.remaining_creditsis omitted rather than written asmax(remaining, planTotal). Both satisfy the floor rule, but writing back a value read moments earlier would resurrect credits a concurrent deduction had spent in between. Omitting the field leaves the column alone and closes that window instead of preserving it.No grant provenance is needed. chat#1948 deferred this fix on the grounds that protecting a grant "would mean teaching
checkAndResetCreditsabout grant provenance", and said to revisit if a granted account were ever found reset in the wild. It does not need provenance: a refill that never reduces protects a granted balance automatically, without the read path knowing where the balance came from. That is why the fix is much smaller than #1948 assumed, and there is a test asserting exactly it.Verification
TDD, red before green. Added a
describe("the refill is a floor, not an assignment")block with six cases, then ran it: RED, 4 failed / 9 passed (the four asserting the update payload for at-or-above-total balances). Implemented, then GREEN, 13 passed.{ remaining_credits: 333, timestamp }{ timestamp }only, balance stays 9,999{ timestamp }only{ timestamp }only, balance stays 25,000remaining_creditsin the update, balance stays 9,999{ timestamp }only, balance stays 12,000The seven pre-existing tests, including both original refill-to-plan-total cases, still pass unmodified.
Full runs:
vitest run lib/credits lib/admins app/api/accountseslint lib/creditstsc --noEmitNot yet done: the issue's live Done-when checks — seed an account at 9,999 on the free plan with a month-old
timestampon preview and read it back, and after deploy confirm none of the 50 at-risk prod accounts lose credits. Flagging rather than implying it.Merge order
Independent of the auto-recharge PRs and can land at any point. Two things depend on it:
getGrantExpiresAtis only correct once this has shipped (row 6 of chat#1960).Summary by cubic
Stops monthly refills from cutting balances by making the refill a floor instead of an assignment. Previously a due refill set
remaining_creditsto the plan total; now it only raises balances below the total and always advancestimestamp.checkAndResetCredits: compute the plan total; on a due refill writetimestampand only setremaining_creditswhen below the total. Balances at or above the total are left unchanged.remaining_creditsinstead of writing backmax(remaining, planTotal)to avoid resurrecting credits in concurrent-deduction races.Rollout
Written for commit 2f65b4f. Summary will update on new commits.