From 0f353c3d172863c403bed9bfebd9cdbcff03b46f Mon Sep 17 00:00:00 2001 From: "Matthew Wire (MJW)" Date: Thu, 6 Aug 2026 17:06:18 +0100 Subject: [PATCH] Financial - Make Payment::create idempotent for a repeated trxn_id 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. --- CRM/Financial/BAO/Payment.php | 51 ++++++++++++++++++++++++++++ tests/phpunit/api/v3/PaymentTest.php | 46 +++++++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php index bdd0e9b384b4..4500b5fdf2d9 100644 --- a/CRM/Financial/BAO/Payment.php +++ b/CRM/Financial/BAO/Payment.php @@ -19,12 +19,21 @@ use Civi\Api4\FinancialItem; use Civi\Api4\LineItem; use Civi\Api4\EntityFinancialTrxn; +use Civi\Api4\Payment; /** * This class contains payment related functions. */ class CRM_Financial_BAO_Payment { + /** + * Seconds to wait for the per-contribution lock in create() before giving up. + * + * Needs to comfortably cover the slowest thing that can happen while the lock is held - + * completing an order can send a receipt email synchronously. + */ + const PAYMENT_CREATE_LOCK_TIMEOUT = 15; + /** * Function to process additional payment for partial and refund * contributions. @@ -44,6 +53,48 @@ class CRM_Financial_BAO_Payment { * @throws \CRM_Core_Exception */ public static function create(array $params, $disableActionsOnCompleteOrder = FALSE): CRM_Financial_DAO_FinancialTrxn { + // Serialise payment-recording per contribution, and treat a second payment carrying the same + // trxn_id as idempotent - returning the payment already recorded instead of creating a + // duplicate. Without this, 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. + $lock = \Civi::lockManager()->acquire('data.contribute.paymentCreate.' . $params['contribution_id'], self::PAYMENT_CREATE_LOCK_TIMEOUT); + if (!$lock->isAcquired()) { + throw new CRM_Core_Exception(ts('Could not acquire a lock to record a payment for contribution %1. Another payment may currently be being recorded for the same contribution.', [ + 1 => $params['contribution_id'], + ]), 'payment_create_lock_failed'); + } + try { + if (!empty($params['trxn_id'])) { + $existingTrxnID = Payment::get(FALSE) + ->addWhere('contribution_id', '=', $params['contribution_id']) + ->addWhere('trxn_id', '=', $params['trxn_id']) + ->addSelect('id') + ->execute() + ->first()['id'] ?? NULL; + if ($existingTrxnID) { + // A concurrent call (e.g. a payment processor webhook for this same charge) already + // recorded this exact payment - return it rather than creating a duplicate or erroring. + // From the caller's point of view this payment succeeded, so this is the correct result. + return CRM_Financial_DAO_FinancialTrxn::findById($existingTrxnID); + } + } + return self::completePayment($params, $disableActionsOnCompleteOrder); + } + finally { + $lock->release(); + } + } + + /** + * @param array $params + * @param bool $disableActionsOnCompleteOrder + * + * @return \CRM_Financial_DAO_FinancialTrxn + * @throws \CRM_Core_Exception + */ + private static function completePayment(array $params, $disableActionsOnCompleteOrder): CRM_Financial_DAO_FinancialTrxn { $contribution = Contribution::get(FALSE) ->addWhere('id', '=', $params['contribution_id']) ->addSelect('*', 'contribution_status_id:name', 'balance_amount', 'paid_amount') diff --git a/tests/phpunit/api/v3/PaymentTest.php b/tests/phpunit/api/v3/PaymentTest.php index 41bd7e26fb94..ace9894ab79f 100644 --- a/tests/phpunit/api/v3/PaymentTest.php +++ b/tests/phpunit/api/v3/PaymentTest.php @@ -9,8 +9,10 @@ +--------------------------------------------------------------------+ */ +use Civi\Api4\Contribution; use Civi\Api4\EntityFinancialTrxn; use Civi\Api4\Order; +use Civi\Api4\Payment; /** * Test APIv3 civicrm_contribute_* functions @@ -1539,4 +1541,48 @@ public function testFeeAmountTrxn(): void { Civi::settings()->set('always_post_to_accounts_receivable', 0); } + /** + * Test that a second Payment.create carrying a trxn_id already recorded for the contribution is + * idempotent - it returns the existing payment rather than creating a duplicate or erroring. + * + * This covers a payment processor webhook racing a synchronous front-end/back-office + * confirmation of the same charge, where both calls can read the contribution as not-yet-completed + * and both attempt to record the same payment. + */ + public function testCreatePaymentDuplicateTrxnIDIsIdempotent(): void { + $contributionID = $this->contributionCreate([ + 'contact_id' => $this->individualCreate(), + 'total_amount' => 100, + 'contribution_status_id' => 'Pending', + 'fee_amount' => 0, + ]); + + $firstPayment = Payment::create(FALSE) + ->addValue('contribution_id', $contributionID) + ->addValue('total_amount', 100) + ->addValue('trxn_id', 'ch_race_condition') + ->execute()->single(); + + $secondPayment = Payment::create(FALSE) + ->addValue('contribution_id', $contributionID) + ->addValue('total_amount', 100) + ->addValue('trxn_id', 'ch_race_condition') + ->execute()->single(); + + $this->assertEquals($firstPayment['id'], $secondPayment['id'], 'The second call should return the payment the first call recorded, not create a new one.'); + + $paymentCount = Payment::get(FALSE) + ->addWhere('contribution_id', '=', $contributionID) + ->selectRowCount() + ->execute() + ->count(); + $this->assertEquals(1, $paymentCount, 'Only one payment should have been recorded for the contribution.'); + + $contribution = Contribution::get(FALSE) + ->addWhere('id', '=', $contributionID) + ->addSelect('contribution_status_id:name') + ->execute()->single(); + $this->assertEquals('Completed', $contribution['contribution_status_id:name']); + } + }