Financial - Make Payment::create idempotent for a repeated trxn_id

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')

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']);
+  }
+
 }