fix: deduplicate Stripe webhook credits via ledger reference index#124
fix: deduplicate Stripe webhook credits via ledger reference index#124antojoseph wants to merge 2 commits into
Conversation
Add a unique partial index on ledger_entries(entry_type, reference) where reference is non-empty. This enforces at the DB level that a given Stripe session ID can only be credited once, even under concurrent or duplicate webhook delivery. creditTx now also checks for an existing reference before updating the balance, returning early without crediting if a duplicate is detected. The ON CONFLICT DO NOTHING on the ledger insert is a final race-safe guard.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51e28db177
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| )`, | ||
| `CREATE INDEX IF NOT EXISTS idx_ledger_account ON ledger_entries(account_id, created_at DESC)`, | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_ledger_reference ON ledger_entries(entry_type, reference) WHERE reference <> ''`, |
There was a problem hiding this comment.
The new unique index on (entry_type, reference) is global, so any repeated non-empty reference for the same entry type will suppress later credits across all accounts. This now conflicts with existing callers that intentionally reuse references (for example LedgerRefund with "reservation_refund" in coordinator/internal/api/consumer.go and default "admin_credit" in coordinator/internal/api/billing_handlers.go), causing legitimate credits to be silently skipped after the first one.
Useful? React with 👍 / 👎.
| `INSERT INTO ledger_entries (account_id, entry_type, amount_micro_usd, balance_after, reference, created_at) | ||
| VALUES ($1, $2, $3, $4, $5, COALESCE($6, NOW()))`, | ||
| VALUES ($1, $2, $3, $4, $5, COALESCE($6, NOW())) | ||
| ON CONFLICT (entry_type, reference) WHERE reference <> '' DO NOTHING`, |
There was a problem hiding this comment.
This ON CONFLICT ... DO NOTHING runs after the balance increment, and the result is not checked. Under concurrent duplicate deliveries with the same reference, both transactions can pass the pre-check and credit balances, but one ledger insert will be skipped here and still return success, committing an extra balance increase with no corresponding ledger entry.
Useful? React with 👍 / 👎.
Three changes: 1. Unique index now covers (account_id, entry_type, reference) — not the previous global (entry_type, reference) — so reused generic references like 'reservation_refund' across different accounts remain valid 2. Pre-check also scopes to account_id 3. Ledger insert no longer uses ON CONFLICT DO NOTHING; instead the unique index causes a constraint error if a race slips through the pre-check, rolling back the balance update so no orphaned credit can occur. ErrAlreadyCredited is returned for clean idempotent duplicates.
|
|
||
| if err := creditTx(ctx, tx, accountID, amountMicroUSD, entryType, reference, time.Time{}); err != nil { | ||
| if errors.Is(err, ErrAlreadyCredited) { | ||
| return nil |
There was a problem hiding this comment.
🔴 This makes all repeated Credit calls with the same (account_id, entry_type, reference) silently no-op, not just Stripe checkout deposits. Existing callers reuse references inside one account: reservation refunds use "reservation_refund", and admin credits default to "admin_credit". After this change, a user with two early-failed reserved requests only gets the first refund; the second LedgerRefund is swallowed here. Either scope idempotency to external deposit refs / LedgerStripeDeposit, or make those other credit callers pass truly unique references before enforcing this globally.
| )`, | ||
| `CREATE INDEX IF NOT EXISTS idx_ledger_account ON ledger_entries(account_id, created_at DESC)`, | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_ledger_reference ON ledger_entries(account_id, entry_type, reference) WHERE reference <> ''`, | ||
|
|
There was a problem hiding this comment.
🟡 This startup migration will fail if production already has duplicate non-empty (account_id, entry_type, reference) rows. Given existing reused references like reservation_refund / default admin_credit, that seems plausible. Add a preflight/dedupe migration or narrow the unique index predicate to the Stripe deposit reference shape so deploys do not fail closed on existing ledger history.
| // ErrAlreadyCredited is returned by creditTx when the (account_id, entry_type, | ||
| // reference) tuple already exists, indicating an idempotent duplicate. | ||
| var ErrAlreadyCredited = errors.New("store: credit already applied for this reference") | ||
|
|
There was a problem hiding this comment.
🟡 The Store contract now differs between implementations: PostgresStore.Credit is idempotent for duplicate references, but MemoryStore.Credit still applies every duplicate. Most billing/API tests use memory, so the Stripe replay invariant and the legitimate repeated-refund/admin-credit cases are not covered. Please add focused tests for duplicate LedgerStripeDeposit and for non-Stripe repeated credit refs, ideally exercising Postgres semantics.
2 participants
Summary
idx_ledger_referenceonledger_entries(entry_type, reference) WHERE reference <> ''so duplicate Stripe webhook deliveries cannot double-credit an accountcreditTxto pre-check for an existing entry and useON CONFLICT DO NOTHINGon insert, returningErrAlreadyCreditedon duplicateSecurity impact
Without this fix a Stripe
checkout.session.completedwebhook replayed by Stripe (or by an attacker replaying the raw POST) would credit the same account multiple times. The index makes the operation idempotent at the database level.Test plan
go test ./internal/store/...