Financial - Make Payment::create idempotent for a repeated trxn_id - #36426
Financial - Make Payment::create idempotent for a repeated trxn_id#36426mattwire wants to merge 1 commit into
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
PR commands & links...
|
| ->addWhere('trxn_id', '=', $params['trxn_id']) | ||
| ->addSelect('id') | ||
| ->execute() | ||
| ->first()['id'] ?? NULL; |
There was a problem hiding this comment.
Payment.get returns refunds as well as payments, so this will also match a negative-amount row carrying the same trxn_id. Stripe happens to be safe as refunds are recorded against $refund->id (re_...) rather than the charge ID so maybe this needs to be changed?
There was a problem hiding this comment.
That makes sense. I've added in a comparision on == total_amount as well
| ->addValue('contribution_id', $contributionID) | ||
| ->addValue('total_amount', 100) | ||
| ->addValue('trxn_id', 'ch_race_condition') | ||
| ->execute()->single(); |
There was a problem hiding this comment.
This covers the idempotency check but not the lock as the two sequential calls exercise the $existingTrxnID branch only, and would pass with the locking removed entirely.
Given the refund concern above, a second case would be valuable: payment > refund > payment with the same trxn_id, asserting whatever the intended behaviour is.
Also worth asserting fee_amount on the contribution afterwards as a symptom of this bug is a doubled fee_amount, not just a doubled payment count.
There was a problem hiding this comment.
Added second test to cover refund case - testCreatePaymentDuplicateTrxnIDDoesNotMatchRefundSharingIt and testCreateRunsUnderLock to partially test the lock
A payment processor webhook racing a synchronous confirmation of the same charge can both record a payment for a contribution. Lock per-contribution in create(), and treat a repeat trxn_id (matched on amount too, so a refund sharing the trxn_id isn't confused with the payment it reverses) as a no-op returning the existing payment.
fed193f to
de1e627
Compare
2 participants
Overview
A payment processor webhook racing a synchronous front-end/back-office confirmation of the same charge (e.g. paying an existing pending contribution via CRM_Contribute_Form_Contribution_Confirm) can both read the contribution as not-yet-completed and both go on to record a payment for it. Add a per-contribution lock via Civi::lockManager() around create(), and treat a second payment carrying an already-recorded trxn_id as a no-op success (returning the existing FinancialTrxn) rather than creating a duplicate. Every caller gets this for free with no changes needed at the call site. Includes a regression test using APIv4 Payment::create/get.
Alternative to the reject-with-exception approach in fix-payment-create-race-lock - see that branch for comparison.
Before
Two payments recorded (or crash for one of them) if user-side and webhook record payment at the same time.
After
One payment recorded and both sides think they recorded it - desired outcome.
Technical Details
Adds a lock and relies on trxn_id being unique (which is enforced by DB entity).
The downstream code has 15 seconds to complete otherwise it will throw a CRM_Core_Exception. If downstream code completes in time the payment record is returned just the same as for the first caller.
Comments
This should be a cleaner solution than #36401 because it doesn't require code changes anywhere else.