fix(payments): ack webhooks for users this plane does not own (COR-594) - #1506
fix(payments): ack webhooks for users this plane does not own (COR-594)#1506sejori wants to merge 2 commits into
Conversation
Stripe delivers account-level events to every configured endpoint regardless of which plane created the session. With one Stripe account and two regional planes — the decided model, since regional billing means regional ledgers rather than a second processor — each plane will receive the other's events as a matter of course. Today that ends in an infinite retry. An unknown creditor is only logged before processing continues, so the credit insert lands on a user_id with no row, its foreign key fails as a plain sqlx error, and PaymentError::Database maps to 500 — which is exactly the signal that tells Stripe to try again, forever, for an event this plane can never process. process_payment_session now returns UnknownReference before that write, and the webhook handler acks it. Retrying cannot make a user this plane does not own appear, so anything but 2xx is a loop. The log is warn, not error: cross-region delivery is expected, so error would be wrong. Not info either, because a genuinely orphaned *local* session is indistinguishable from here and must stay visible; the volume is the other region's payment count — events, not requests — so it stays quiet, and client_reference_id is structured for alerting if orphans ever need chasing. Tests cover both halves of the guarantee: UnknownReference acks, and Database still returns 500. The second matters as much as the first — widening the ack to cover genuine database failures would silently drop real payments.
There was a problem hiding this comment.
Pull request overview
This PR adjusts Stripe webhook/payment processing so that events referencing users not present in the current regional plane can be safely acknowledged, preventing cross-region Stripe event fan-out from causing repeated processing attempts against the wrong plane.
Changes:
- Introduces
PaymentError::UnknownReferenceto represent “session references a user unknown to this plane”. - Updates Stripe payment-session processing to bail out early on unknown-user references.
- Updates the payments webhook handler to explicitly ACK
UnknownReferencewith 200 and log it atwarn, and adds tests around status mapping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
dwctl/src/payment_providers/stripe.rs |
Returns UnknownReference when session user lookups fail; adds unit tests around HTTP status mapping. |
dwctl/src/payment_providers/mod.rs |
Adds PaymentError::UnknownReference variant and maps it to an HTTP status code. |
dwctl/src/api/handlers/payments.rs |
Explicitly handles UnknownReference in the webhook handler by logging and returning 200 OK. |
Suppressed comments (1)
dwctl/src/payment_providers/mod.rs:90
- Mapping
PaymentError::UnknownReferenceto200 OKin the globalFrom<PaymentError> for StatusCodeaffects non-webhook endpoints too. For example,PATCH /payments/{id}usesStatusCode::from(e)and would return 200 with an error payload for an unknown reference, which is confusing for API clients. Consider mapping this to a client error (e.g. 404) and keeping the explicit 2xx ACK behavior only in the webhook handler.
// 2xx on purpose: retrying cannot make a user this plane does not
// own appear, so any other status buys an infinite Stripe retry.
PaymentError::UnknownReference(_) => StatusCode::OK,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let creditor_user = users.get_by_id(payment_session.creditor_id).await?; | ||
| if creditor_user.is_none() { | ||
| tracing::error!( | ||
| "Creditor user {} not found for payment session {}. This indicates a data integrity issue.", | ||
| payment_session.creditor_id, | ||
| session_id | ||
| ); | ||
| return Err(PaymentError::UnknownReference(payment_session.creditor_id.to_string())); | ||
| } |
There was a problem hiding this comment.
Valid, and the sharper version of it: the insert writes the creditee as user_id, so checking only the creditor left an admin pay-on-behalf session with a foreign creditee still reaching the failing insert. Fixed in c99be38 — both are checked, creditee first because it is the one written, creditor after because it is read for the description and written by set_verified.
| // unknown creditor is the *expected* shape of a foreign region's | ||
| // session rather than a rarity. Continuing would drive the credit | ||
| // insert below onto a user_id with no row, failing its foreign key | ||
| // as a plain sqlx error — which maps to 500 and makes Stripe retry | ||
| // the other region's event against this plane indefinitely. |
There was a problem hiding this comment.
Correct, and this corrects the PR premise rather than just a comment. create_transaction returns DbError, From<DbError> maps everything but a unique violation to InvalidData, and the webhook catch-all already turns that into 200 — so there is no 500 and no retry storm today. Fixed in c99be38; the comment and the PR body now describe the real symptom: a routine cross-region event logged at ERROR as a "data integrity issue", a wasted Stripe round trip, and correctness resting on a foreign-key constraint rather than a decision. Still worth fixing, just not for the reason I first wrote.
| /// course. Treating that as an error makes Stripe retry a foreign event | ||
| /// against this plane forever, so it is deliberately a 2xx no-op — see the | ||
| /// StatusCode mapping below. |
There was a problem hiding this comment.
Agreed, and the concrete hazard is the front-channel PATCH /payments/{id}, which shares this mapping — a 200 there would report success for a session we cannot process. Fixed in c99be38: the shared mapping is now 404, and the webhook keeps its explicit 200 in its own match arm, which is the only place stopping redelivery matters. Docs updated to say exactly that.
Three points from Copilot, all valid, and one of them corrects this
PR's own premise.
The existence check was on the creditor, but the credit insert writes
the CREDITEE as user_id — so an admin pay-on-behalf session whose
creditee is foreign would still have reached the failing insert. Both
parties are now checked, creditee first because it is the one written.
The claim that the foreign key surfaced as a plain sqlx error mapping to
500, and therefore an infinite Stripe retry, was wrong.
Credits::create_transaction returns DbError, and From<DbError> maps
everything but a unique violation to InvalidData — which the webhook's
catch-all already turns into 200. The real present symptom is milder and
worth stating accurately: a routine cross-region event logged at ERROR
as "data integrity issue", plus a wasted Stripe round trip and a failed
write. Still worth fixing — it depends on a constraint rather than a
decision, would credit a nonexistent user if that constraint were ever
relaxed, and buries an expected event in the error stream — but the
comments and PR body now say so honestly.
UnknownReference no longer maps to 200 globally. That mapping is shared
with the front-channel PATCH /payments/{id}, where reporting success for
an unprocessable session would mislead the caller; it is 404 there. The
webhook keeps its explicit 200 in its own arm, which is where stopping
redelivery actually belongs.
Deploying control-layer with
|
| Latest commit: |
c99be38
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e23bf1a9.control-layer.pages.dev |
| Branch Preview URL: | https://cor-594-webhook-foreign-regi.control-layer.pages.dev |
Stops a foreign region's Stripe events being retried against this plane forever.
Linear: COR-594 · Project: Region Based Hosting
Why this exists
Stripe delivers account-level events to every configured endpoint regardless of which plane created the session. Regional billing means regional ledgers, not a second Stripe account — so with two planes and one account, each receives the other's events as a matter of course.
The bug, precisely
An unknown creditor is currently only logged, then processing continues:
Execution falls through to
credits.create_transaction(), which inserts against auser_idwith no row and fails its foreign key.Correction after review — my first draft claimed this became a 500 and an infinite Stripe retry. It doesn't:
create_transactionreturnsDbError, andFrom<DbError>maps everything but a unique violation toInvalidData, which the webhook's catch-all already turns into 200. The real symptom is milder, and worth stating accurately:Still worth fixing, just not for the reason first stated.
The fix
process_payment_sessionreturns a newPaymentError::UnknownReferencebefore that write, and the webhook handler acks it with 200 in its own match arm — retrying cannot make a user this plane doesn't own appear.Both parties are checked, creditee first, because the creditee is what the insert writes as
user_id. Checking only the creditor (as the ticket describes) would still let an admin pay-on-behalf session with a foreign creditee reach the failing insert.UnknownReferencemaps to 404, not 200, in the sharedStatusCodeconversion. That conversion also serves the front-channelPATCH /payments/{id}, where reporting success for a session we cannot process would mislead the caller. The 200 belongs to the webhook alone, which is where stopping redelivery actually matters.The log level is a deliberate choice
warn, and the reasoning is in the code:error— cross-region delivery is expected, so erroring would pollute exactly the dashboards that should stay meaningfulinfo— a genuinely orphaned local session is indistinguishable from here, and must remain visibleVolume is the other region's payment count — events, not requests — so it stays quiet.
client_reference_idis a structured field for alerting if orphans ever need chasing.Tests
Both halves of the guarantee, because the second matters as much as the first:
test_unknown_reference_is_not_found_for_direct_callersUnknownReference→ 404 for direct callerstest_database_errors_are_still_retriedDatabase→ 500Widening the ack to cover genuine database failures would silently drop real payments, so the distinction between "not ours" and "we couldn't process it" is pinned.
Note:
process_payment_sessioncan't be exercised end-to-end in a unit test — the session retrieve precedes the check and needs a live Stripe — so the tests pin the status mapping, which is where the regression risk actually sits. The ticket's acceptance (replay an EU event against a US deployment) remains a deployment-time check.Gate
This must land before the US plane registers its webhook endpoint, per the ticket. Without it, the first EU checkout after that registration starts an unbounded retry loop against the US plane.
🤖 Generated with Claude Code