Skip to content

fix(stripe): skip refunds on disputed and already-refunded charges - #6257

Closed
toommz wants to merge 13 commits into
mainfrom
ing-600
Closed

toommz wants to merge 13 commits into
mainfrom
ing-600

Conversation

@toommz

@toommz toommz commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Fixes ING-600

Context

CreditNotes::Refunds::StripeCreateJob sent refunds Stripe can never accept, and since ApplicationJob sets retry: 0 the job died straight into the dead queue. Two production incidents: a charge with an open dispute (charge_disputed, US) and a charge already refunded outside Lago (charge_already_refunded, EU).

Our only guard was invoice.payment_dispute_lost_at?, written solely by charge.dispute.closed, and nothing checked how much of the charge was still refundable.

Description

Two independent layers:

  • Stripe pre-check — the charge's remaining refundable amount is read before calling Stripe::Refund.create, and charge_disputed / charge_already_refunded now return without raising. This half needs no Stripe-side change and fixes both incidents on deploy.
  • Inbound Stripe dispute events — we now subscribe to charge.dispute.created / charge.dispute.updated and record dispute state on the invoice (new invoices.payment_refund_blocked_at), so the common case costs no API call.

The column is named for its effect rather than the event, because it is not a dispute record: an open inquiry leaves it nil (Stripe still allows refunds there), and a lost dispute leaves it set (refunds stay blocked). It holds current state only: history of past disputes is not modelled, and a charge carrying two concurrent disputes is not ref-counted (the widened rescue is the backstop for both). Because nothing else can clear the flag, the dispute handler reads the dispute's current state from Stripe rather than trusting a payload that may have been replayed out of order, falling back to the payload if Stripe cannot be reached.

No new outgoing Lago webhook is added: config/webhook_event_types.yml is untouched and there is no invoice.payment_dispute_opened event. The column is internal state, and is deliberately not exposed in the v1 serializer, GraphQL, InvoicesQuery, the CSV export or the exports_invoices view.

Every skip path stays visible: the credit note is marked failed, credit_note.provider_refund_failure fires, and the activity log is produced, exactly as when Stripe rejects the refund today.

Two points worth a look in review:

  • The flag mirrors Stripe's is_charge_refundable, not the event name. charge.dispute.created also fires for inquiries, where Stripe still accepts refunds and refunding is the normal way to resolve one. Gating on the event alone would silently block legitimate refunds. This is also why the flag is deliberately not cleared on a lost dispute.
  • The pre-check fails open, skipping only on a positive signal, so it can never become a new failure path.

Also adds retry_on for rate-limit/connection errors to the refund job, which the invoice payment job already has.

Deploy

A migration enqueues PaymentProviders::Stripe::RefreshWebhookJob for every provider with a secret key, mirroring 20250626175249_refresh_stripe_webhooks, so existing Stripe connections pick up the new events on upgrade without anyone running a rake task (rake stripe:refresh_registered_webhooks remains available to re-run it). The pre-check keeps the guard correct until then, and for self-hosted installs that never run it.

@toommz toommz self-assigned this Aug 31, 2026
@toommz
toommz force-pushed the ing-600 branch 2 times, most recently from 64bd409 to 3b29fcc Compare August 31, 2026 12:37
@toommz
toommz marked this pull request as ready for review September 17, 2026 15:26
@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — the dispute state can become stale or cross organization boundaries, and the new retry policy is untested.

  • ChargeDisputeCreatedService applies every created/updated event directly, so an older or replayed non-refundable event arriving after a refundable/closed event permanently re-blocks the invoice. Stripe does not guarantee webhook order; make the transition order-safe and add a close-then-stale-open regression test.
  • ChargeDisputeClosedService still uses an unscoped Payment.find_by, unlike the new organization-scoped created/updated handler. Scope it to the webhook organization and cover the cross-organization case so a shared Stripe account cannot mutate another organization's invoices.
  • Add job specs proving StripeCreateJob retries both newly configured transient Stripe errors (and stops at the configured limit); its existing spec only covers delegation.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — the new dispute state can become a permanent false refund block when webhook workers race or Stripe is temporarily unavailable.

  • Make the transition order-safe in ChargeDisputeCreatedService: rescuing retryable StripeErrors falls back to a stale event instead of using HandleEventJob's existing retries, and an open/update worker can still re-block after a concurrent close worker clears the flag. Propagate transient lookup failures and serialize or otherwise guard both open and close writes; add regression coverage for stale-event lookup failure and concurrent/out-of-order transitions.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — the retry and rollout paths can leave refunds incorrectly failed or disputes untracked.

  • Preserve idempotent recovery when Stripe completes a refund but the request raises APIConnectionError: the retry currently sees the increased amount_refunded and can fail the credit note before replaying the same idempotency key. Add coverage for that remote-success/connection-failure sequence.
  • Refresh existing Stripe webhook endpoints for charge.dispute.created and charge.dispute.updated; changing WEBHOOKS_EVENTS only affects newly registered or otherwise refreshed endpoints. Mirror the existing refresh migration pattern and cover the enqueueing.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — transient Stripe server errors can still drop the new recovery paths.

  • ChargeDisputeCreatedService#current_dispute and StripeService#existing_stripe_refund let Stripe::APIError escape, but neither HandleEventJob nor StripeCreateJob retries that sibling error class (unlike existing Stripe jobs). A Stripe 5xx can therefore leave the dispute flag unset or a refunded credit note unreconciled. Add retry handling and focused coverage for both paths.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — dispute unblocking is not order-safe.

  • ChargeDisputeClosedService trusts the closed-event payload and clears the invoice-level block, so an older refundable close delivered after a newer non-refundable dispute can erase current state. Derive current payment-level refundability before unblocking and add regression coverage for stale closed-after-open ordering, including multiple disputes on one PaymentIntent.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — dispute webhooks without a payment intent can mutate an unrelated payment.

  • In DisputeRefundability#payment, guard a blank provider_payment_id before querying: Stripe dispute objects may have a null payment_intent, and find_by(provider_payment_id: nil) can select an unrelated payment in the organization. Add regression coverage for the created/updated and closed handlers with a null payment intent.

@toommz

toommz commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by a four-PR stack, same code re-cut into reviewable steps (the tip reproduces this branch's tree exactly):

The review history here is worth reading alongside #6444, which is where most of it applies.

@toommz toommz closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant