fix(funding): close the three pipeline races + a live-Stripe backstop (#12)

[keithfawcett]

Closes the rest of #12 from docs/audit-remaining-work.md (the pagination subfix shipped as #71).

HOSTED_FUNDING_ENABLED is OFF, so all of this is latent — but each one costs the brand real money the day the flag flips, and the handoff was explicit that a money state machine gets one coherent pass, not piecemeal patches. The staging matrix is extended to match.

1. Ambiguous PaymentIntent create re-created instead of searching

A create whose response was lost (timeout, crash) left the batch funding_failed with no PI stamped — even though Stripe may well have made a real ACH debit. Inside Stripe's ~24h idempotency window the frozen key fbpi:<batchId> replays harmlessly; past it the retry created a SECOND PaymentIntent — the brand debited twice.

Now: once a batch has attempted at all, the retry searches by our metadata stamp and adopts what it finds (routing on the PI's actual status — succeeded goes through the verified confirm path), and only creates when Stripe confirms there is nothing.

Related, found while writing it: paymentIntents.create({confirm: true}) throws but still creates the intent when the bank account declines. That id was being discarded, so the retry would create another. It's now recorded off the error, and the retry confirms that intent (fbpc: key) instead.

2. Release vs in-flight create

The PI id is only stamped when create returns. A release landing in that window saw no PI, skipped terminalization, and freed the allocations — collecting money for commissions that were already back in the pool. Fixed from both sides:

• Create side: the stamp is now status-predicated. Losing that CAS means a release took the batch, so the orphaned PI is canceled. If Stripe won't cancel it (already processing), the batch is frozen recovery_required with failureReason=orphan_payment_intent:<pi> and a loud alert — never left silent.
• Release side: it no longer trusts "no id on the row" — it asks Stripe before freeing anything, and a search that fails returns pi_not_terminal. Don't know ⇒ don't free.

Search is eventually consistent, so it can't cover the milliseconds-old case on its own; that's exactly what the create-side CAS is for. Belt and braces, deliberately.

3. Inbox claimed before processing

The inbox row was written before the handler ran, so a crash mid-handler made every Stripe redelivery a no-op — the transition it carried was lost permanently.

The claim is now a lease: only a stamped outcome is terminal, an unfinished claim older than 5 minutes can be taken over by a redelivery, and handlers are CAS-based so a takeover racing a live worker degrades to a lost CAS rather than a double transition. A claim still unfinished an hour later (crashed and never redelivered) is alerted by the daily reconcile — no schema change needed.

Plus: the gap the handoff flagged for confirmation

Refunds, disputes and transfer reversals had no live-Stripe backstop. The daily reconcile only inspected locally-flagged state, which by definition cannot see a lost webhook — so a missed reversal left a payout recorded paid and its batch unfrozen.

The job now sweeps settled money against Stripe: a refunded/disputed funding charge freezes the batch exactly as the webhook would, and a reversed transfer is recorded through the same reversal handler (PayoutReversal + derived payout state + compensating CommissionAdjustment). Bounded at 50 reads per run, and it logs what it didn't reach — an unchecked batch must never look like a clean one.

Tests

funding-races.test.ts (19, DB-backed, Stripe hand-mocked). The two that matter most can only be seen by interleaving: a release landing inside the create call (via a hook that runs mid-paymentIntents.create), and a redelivery arriving after a claim's worker died. Also: adopt-instead-of-create, adopt-a-succeeded-PI, first-attempt-doesn't-pay-for-a-search, decline-records-the-intent, release-searches-before-freeing, payment-wins-the-release, search-failure-doesn't-free, orphan-cancel + orphan-freeze, lease semantics (finished / live / expired / released), handler-throws-stays-replayable, and the four reconcile-sweep cases.

runFundingCollector had no test coverage at all before this. Full suite: 266 passing. pnpm typecheck + pnpm lint clean. No migration.

Runbook

docs/payout-funding-staging-runbook.md gains section H — eight staging scenarios that prove the Stripe half of these, each with how to force it (drop the create response with a proxy, hang inside create and release from another process, SIGKILL between claim and handler…) — a G4 row for the stuck-claim alert, and the four invariants worth re-reading before anyone touches this pipeline again.

🤖 Generated with Claude Code

[keithfawcett]

Adversarial review pass (Codex, xhigh) — seven fixes in 18b72eb

All four claims were refuted. The one that matters most is that my own inbox fix was incomplete.

1. The inbox still lost events (high). claimInboxEvent returned a bare boolean, so "already finished" and "another worker holds it" both became inbox_replay — and the route answered 2xx. A worker that crashed mid-handler therefore lost its event whenever Stripe's redelivery landed inside the 5-minute lease: the second worker acked it and Stripe never delivered again. I'd turned "lost forever" into "lost on a five-minute fuse". The claim now reports claimed/done/held; held raises InboxEventHeldError and the route answers 409 so Stripe redelivers after the lease expires.

2. The lease had no owner (high). A worker whose lease had been taken over could still stamp an outcome onto — or delete — the new owner's claim, since both keyed on event id alone. The claim's processedAt now doubles as an owner token; a takeover swaps it, and stamp/release are scoped to the token they were issued.

3. The reversal sweep skipped partially_reversed forever (high). Once a partial was recorded, a lost webhook completing the reversal was invisible to the only backstop that exists. Only a fully reversed payout is finished.

4. Both sweep caps starved (medium). Taking the oldest 50 of an ever-growing list re-checked the same 50 nightly while newer batches — the only ones that can still change — were never checked. Now bounded by the 180-day reversal horizon, and anything past the cap is reported by id, not silently truncated.

5. An orphaned PaymentIntent left its allocations freed (high). When a release won the race and the created PI couldn't be canceled, the batch froze recovery_required but its allocations stayed released — so the same commissions could be re-batched and the brand charged twice. The orphan path now reclaims allocations no newer batch has taken.

6. release_requested was terminal by accident (high, pre-existing, now reachable). A release stopping on a Stripe failure returned pi_not_terminal, but no collector state matched that status and a second releaseBatch just lost the CAS — the batch sat forever with frozen allocations and no alert. My comment claimed "next tick retries"; it didn't. Now a re-entrant source state, resumed every tick, alerted after a day.

7. Payment-wins skipped verification (high, pre-existing). Release CASed straight to funded without stripeChargeId/fundedAt, so the executor froze the batch as recovery_required on the next tick. It now goes through confirmFundingFromPaymentIntent and freezes explicitly if verification refuses.

Plus: the executor re-reads batch status before each partner transfer (a dispute freezing the batch mid-run now actually stops it), and the CommissionAdjustment clawback is serialized under a row lock — check-then-insert with no unique constraint could double-record when a redelivery and the sweep overlap.

12 new/updated tests; suite 274 passing. Runbook gains H9–H12 and the corrected invariants.

Still true after this pass (limits, not bugs)

paymentIntents.search is eventually consistent, so a false empty is always possible — the search reduces the double-charge window but cannot eliminate it. That's why the status-predicated stamp and the orphan-cancel path exist on the other side, and why the staging matrix (section H) is the real gate. Codex confirmed the stripe-node error shape used by paymentIntentIdFromError is correct and the claim SQL is race-free.

[keithfawcett]

Round-2 review (Codex, xhigh) — five more, fixed in f0f7578

The headline finding is that round-1's orphan recovery could not run in the case it was written for.

1. BLOCKER — the reclaim sat behind a call that raises. abandonOrphanPaymentIntent escalated the batch to recovery_required before reclaiming its allocations. But recovery_required is inside the "one open batch per tenant/currency" partial unique index, so moving a released batch back into it violates the index whenever a newer batch is already open — and casBatch raises rather than returning null. The throw skipped the reclaim entirely, leaving a live ACH debit and freed commissions: precisely the double-charge the path exists to prevent. Reclaim now runs first and unconditionally; the escalation is best-effort; the alert names how many allocations a newer batch had already taken.
2. The same index broke refund/dispute handling (high). charge.refunded on a settled batch tried to drag it back to funding_disputed and took the webhook down with it (and the sweep logged an exception). A settled batch has already transferred, so freezing buys nothing — non-terminal batches are frozen, terminal ones record the clawback and alert. Webhook and sweep now share one function so they can't drift.
3. The sweep still starved (high). Reporting skipped ids made the cap visible without changing the scheduling — a fixed order plus a fixed prefix re-checked the same head nightly. Rows are now dealt by a per-day hash, so coverage is eventually complete.
4. The 180-day horizon was wrong for transfers (high). Refunds have a deadline; transfer reversals don't — Stripe places no age limit on them. The horizon now applies only to the charge sweep.
5. Resuming a release reset its own alert clock (medium). casBatch bumps updatedAt and reconcile decides "stuck" from updatedAt, so a release failing every tick refreshed the 24h alert forever. Re-entry no longer rewrites the row.

Also: transfer reversals are now paged rather than trusting the ten embedded in the Transfer object — under-counting them is what decides partially_reversed vs reversed, and therefore whether the clawback adjustments get written at all.

Honest limits, unchanged. The 409-on-held fix makes delivery at-least-once within Stripe's retry budget, not unconditionally durable: an event held by a wedged worker on Stripe's final attempt can still need the manual replay the reconcile alert asks for. paymentIntents.search remains eventually consistent. Both are why section H of the staging runbook is the actual gate.

Confirmed sound by the review: the timestamp owner token (exact through the JS↔Postgres round-trip), the reclaim SQL's correlated alias, the payment-wins confirm path, the executor's per-partner status re-read (settlement still runs when it should), and the lock ordering on the row-locked CommissionAdjustment insert.

5 new tests, including the blocker with a competing newer batch. Suite: 279 passing.