fix(billing): allocate stored refunds to one charge - #3256
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const matchingCharge = chargeRows | ||
| .filter( | ||
| (charge) => | ||
| charge.created_at < refund.created_at && |
There was a problem hiding this comment.
When a deferred quantity update bulk-inserts its refund and replacement charge, both rows receive the same database created_at; the strict < comparison rejects the charge and leaves the refund unallocated, causing a later preview or update to treat the charge as unrefunded and issue too much credit.
Knowledge Base Used: Billing lifecycle and payment flows
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.ts
Line: 71
Comment:
**Equal timestamps drop refunds**
When a deferred quantity update bulk-inserts its refund and replacement charge, both rows receive the same database `created_at`; the strict `<` comparison rejects the charge and leaves the refund unallocated, causing a later preview or update to treat the charge as unrefunded and issue too much credit.
**Knowledge Base Used:** [Billing lifecycle and payment flows](https://app.greptile.com/autumn-org-2/-/custom-context/knowledge-base/useautumn/autumn/-/docs/billing-lifecycle.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 4 files
Confidence score: 2/5
server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.tsuses persistedcreated_atfor ordering, so asynchronous writes can misallocate historical refunds; persist and use Stripe event time or another business chronology.- The refund allocation logic in
server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.tsmay assign a refund to the latest eligible charge without verifying the actual charge relationship, risking incorrect invoice credits; validate the refund-to-charge correspondence. server/tests/unit/billing/invoice-matched-credits/invoice-credit-matcher.spec.tsdoes not catch refunds being allocated to every matching charge because the invoice-sharing filter excludes the replacement charge; add an assertion that distinguishes single-charge allocation.server/tests/integration/licenses/billing/update/update-license-quantity.test.tsnever performs the documented 7-to-9 update, leaving the claimed two-seat billing behavior unverified; execute and assert the second update.
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="server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.ts">
<violation number="1" location="server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.ts:64">
P1: When invoice rows are persisted asynchronously, `created_at` is storage time rather than Stripe event time, so this ordering can misallocate historical refunds. Persist and use a source/business chronology for allocation, with a deterministic tie-breaker for rows sharing a timestamp.</violation>
<violation number="2" location="server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.ts:76">
P2: The greedy allocation attributes each refund to the single latest charge created before it (that doesn't share the refund's invoice), without confirming that charge is the one the refund actually corresponds to. When a refund of an earlier charge is not stored on the immediate replacement's invoice — e.g. a proration refund landed on a standalone/separate invoice, or a later replacement charge was created before the refund was recorded — the refund is deducted from the wrong (newest) charge while the charge it genuinely refunds keeps a full credit. This under-credits one charge and over-credits another, which is a billing correctness regression compared to attributing the refund to the charge whose period/price it matches.</violation>
</file>
<file name="server/tests/unit/billing/invoice-matched-credits/invoice-credit-matcher.spec.ts">
<violation number="1" location="server/tests/unit/billing/invoice-matched-credits/invoice-credit-matcher.spec.ts:188">
P2: This test passes even if computeAlreadyRefundedByCharge allocated a refund to every matching charge, because replacementCharge is filtered out by the invoice-sharing check (both use invoice_id 'inv_update') rather than by the latest-charge selection it claims to verify. Give replacementCharge a distinct invoice_id (e.g. 'inv_other') so it is a genuine candidate, then assert the refund is allocated only to it (the latest, created at PERIOD_START + 500) and not to originalCharge. That would actually guard the one-charge allocation this PR is fixing.</violation>
</file>
<file name="server/tests/integration/licenses/billing/update/update-license-quantity.test.ts">
<violation number="1" location="server/tests/integration/licenses/billing/update/update-license-quantity.test.ts:137">
P2: This test never executes the second update (7 -> 9): after the 5 -> 7 billing.update it only calls previewUpdate and asserts the line-item pair. The docstring and title claim 'the second update bills only 2 seats', but no executed invoice total or Stripe check verifies that, so a regression that only manifests on the execute/finalize path would pass. Execute the 7 -> 9 update and assert the resulting invoice (e.g. latestTotal 2 * DEV_SEAT_PRICE) and expectStripeSubscriptionCorrect, matching the other update tests in this file.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }): Map<string, number> => { | ||
| const alreadyRefundedByCharge = new Map<string, number>(); | ||
| const chronologicalRefunds = [...refundRows].sort( | ||
| (a, b) => a.created_at - b.created_at, |
There was a problem hiding this comment.
P1: When invoice rows are persisted asynchronously, created_at is storage time rather than Stripe event time, so this ordering can misallocate historical refunds. Persist and use a source/business chronology for allocation, with a deterministic tie-breaker for rows sharing a timestamp.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.ts, line 64:
<comment>When invoice rows are persisted asynchronously, `created_at` is storage time rather than Stripe event time, so this ordering can misallocate historical refunds. Persist and use a source/business chronology for allocation, with a deterministic tie-breaker for rows sharing a timestamp.</comment>
<file context>
@@ -49,25 +52,40 @@ export const computeProratedCredit = ({
+}): Map<string, number> => {
+ const alreadyRefundedByCharge = new Map<string, number>();
+ const chronologicalRefunds = [...refundRows].sort(
+ (a, b) => a.created_at - b.created_at,
);
</file context>
| expect(result.size).toBe(0); | ||
| }); | ||
|
|
||
| test("allocates a refund only to the latest matching earlier charge", () => { |
There was a problem hiding this comment.
P2: This test passes even if computeAlreadyRefundedByCharge allocated a refund to every matching charge, because replacementCharge is filtered out by the invoice-sharing check (both use invoice_id 'inv_update') rather than by the latest-charge selection it claims to verify. Give replacementCharge a distinct invoice_id (e.g. 'inv_other') so it is a genuine candidate, then assert the refund is allocated only to it (the latest, created at PERIOD_START + 500) and not to originalCharge. That would actually guard the one-charge allocation this PR is fixing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/unit/billing/invoice-matched-credits/invoice-credit-matcher.spec.ts, line 188:
<comment>This test passes even if computeAlreadyRefundedByCharge allocated a refund to every matching charge, because replacementCharge is filtered out by the invoice-sharing check (both use invoice_id 'inv_update') rather than by the latest-charge selection it claims to verify. Give replacementCharge a distinct invoice_id (e.g. 'inv_other') so it is a genuine candidate, then assert the refund is allocated only to it (the latest, created at PERIOD_START + 500) and not to originalCharge. That would actually guard the one-charge allocation this PR is fixing.</comment>
<file context>
@@ -164,16 +173,44 @@ describe(chalk.yellowBright("computeAlreadyRefundedForCharge"), () => {
+ expect(result.size).toBe(0);
+ });
+
+ test("allocates a refund only to the latest matching earlier charge", () => {
+ const originalCharge = makeChargeRow({
+ id: "li_charge_5",
</file context>
| }, | ||
| ); | ||
|
|
||
| await expectLicenseUpdatePreviewCorrect({ |
There was a problem hiding this comment.
P2: This test never executes the second update (7 -> 9): after the 5 -> 7 billing.update it only calls previewUpdate and asserts the line-item pair. The docstring and title claim 'the second update bills only 2 seats', but no executed invoice total or Stripe check verifies that, so a regression that only manifests on the execute/finalize path would pass. Execute the 7 -> 9 update and assert the resulting invoice (e.g. latestTotal 2 * DEV_SEAT_PRICE) and expectStripeSubscriptionCorrect, matching the other update tests in this file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/licenses/billing/update/update-license-quantity.test.ts, line 137:
<comment>This test never executes the second update (7 -> 9): after the 5 -> 7 billing.update it only calls previewUpdate and asserts the line-item pair. The docstring and title claim 'the second update bills only 2 seats', but no executed invoice total or Stripe check verifies that, so a regression that only manifests on the execute/finalize path would pass. Execute the 7 -> 9 update and assert the resulting invoice (e.g. latestTotal 2 * DEV_SEAT_PRICE) and expectStripeSubscriptionCorrect, matching the other update tests in this file.</comment>
<file context>
@@ -104,6 +106,45 @@ test.concurrent(
+ },
+ );
+
+ await expectLicenseUpdatePreviewCorrect({
+ preview,
+ customerId,
</file context>
| isWithinPeriod(refund, charge) && | ||
| hasSamePrice(refund, charge), | ||
| ) | ||
| .sort((a, b) => b.created_at - a.created_at)[0]; |
There was a problem hiding this comment.
P2: The greedy allocation attributes each refund to the single latest charge created before it (that doesn't share the refund's invoice), without confirming that charge is the one the refund actually corresponds to. When a refund of an earlier charge is not stored on the immediate replacement's invoice — e.g. a proration refund landed on a standalone/separate invoice, or a later replacement charge was created before the refund was recorded — the refund is deducted from the wrong (newest) charge while the charge it genuinely refunds keeps a full credit. This under-credits one charge and over-credits another, which is a billing correctness regression compared to attributing the refund to the charge whose period/price it matches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/billing/v2/utils/lineItems/storedLineItemUtils.ts, line 76:
<comment>The greedy allocation attributes each refund to the single latest charge created before it (that doesn't share the refund's invoice), without confirming that charge is the one the refund actually corresponds to. When a refund of an earlier charge is not stored on the immediate replacement's invoice — e.g. a proration refund landed on a standalone/separate invoice, or a later replacement charge was created before the refund was recorded — the refund is deducted from the wrong (newest) charge while the charge it genuinely refunds keeps a full credit. This under-credits one charge and over-credits another, which is a billing correctness regression compared to attributing the refund to the charge whose period/price it matches.</comment>
<file context>
@@ -49,25 +52,40 @@ export const computeProratedCredit = ({
+ isWithinPeriod(refund, charge) &&
+ hasSamePrice(refund, charge),
+ )
+ .sort((a, b) => b.created_at - a.created_at)[0];
+ if (!matchingCharge) continue;
+
</file context>
81d2e94 to
be269cc
Compare
Prevent historical refunds from reducing multiple replacement charges when reconstructing prorated invoice credits. Co-authored-by: Cursor <cursoragent@cursor.com>
be269cc to
caeaaf9
Compare
1 participant
Prevent historical refunds from reducing multiple replacement charges when reconstructing prorated invoice credits.
Co-authored-by: Cursor cursoragent@cursor.com
Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by cubic
Prevents historical refunds from being counted against multiple replacement charges when reconstructing prorated invoice credits. Refunds now allocate only to the latest matching earlier charge, and charges on the same invoice as the refund are excluded, so sequential quantity updates no longer reuse prior refunds against newer replacement charges.
Written for commit caeaaf9. Summary will update on new commits.
Greptile Summary
Prevents a historical refund from reducing multiple replacement charges by assigning each refund to one earlier charge.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains established.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Update as Quantity update participant Store as Stored line items participant Matcher as Refund matcher participant Credit as Credit calculation Update->>Store: Persist refund and replacement charge Store->>Matcher: Load current-period rows Matcher->>Matcher: Assign refund to one matching earlier charge Matcher->>Credit: Refunded amount by charge ID Credit-->>Update: Prorated preview or invoice creditReviews (3): Last reviewed commit: "fix(billing): allocate stored refunds to..." | Re-trigger Greptile