Skip to content

Commit a16ffcd

Browse files
sandervanhooftsandervanhooft
committed
Refine webhook concurrency follow-up
1 parent 5975008 commit a16ffcd

5 files changed

Lines changed: 32 additions & 45 deletions

File tree

‎src/Http/Controllers/AftercareWebhookController.php‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,32 +49,41 @@ public function handleWebhook(Request $request)
4949
}
5050

5151
$molliePaymentAmountChargedBackTotal = mollie_object_to_money($molliePayment->amountChargedBack);
52-
$chargeback = DB::transaction(function () use ($localPayment, $molliePaymentAmountChargedBackTotal) {
52+
$updatedLocalPayment = null;
53+
$amountChargedBackNow = null;
54+
55+
DB::transaction(function () use (
56+
$localPayment,
57+
$molliePaymentAmountChargedBackTotal,
58+
&$updatedLocalPayment,
59+
&$amountChargedBackNow
60+
) {
61+
// A transaction alone does not lock rows that were read earlier. Re-read this
62+
// payment with `SELECT ... FOR UPDATE` so concurrent aftercare deliveries cannot
63+
// both compute the same old charged-back total and dispatch the event twice.
5364
/** @var \Laravel\Cashier\Payment|null $localPayment */
5465
$localPayment = Cashier::$paymentModel::whereKey($localPayment->getKey())->lockForUpdate()->first();
5566

5667
if (! $localPayment) {
57-
return null;
68+
return;
5869
}
5970

6071
$locallyKnownAmountChargedBack = $localPayment->getAmountChargedBack();
6172

6273
if (! $locallyKnownAmountChargedBack->lessThan($molliePaymentAmountChargedBackTotal)) {
63-
return null;
74+
return;
6475
}
6576

6677
$localPayment->amount_charged_back = (int) $molliePaymentAmountChargedBackTotal->getAmount();
6778
$localPayment->save();
6879

69-
return [
70-
$localPayment,
71-
$molliePaymentAmountChargedBackTotal->subtract($locallyKnownAmountChargedBack),
72-
];
80+
$updatedLocalPayment = $localPayment;
81+
$amountChargedBackNow = $molliePaymentAmountChargedBackTotal->subtract($locallyKnownAmountChargedBack);
7382
});
7483

75-
if ($chargeback) {
84+
if ($updatedLocalPayment && $amountChargedBackNow) {
7685
Event::dispatch(
77-
new ChargebackReceived($chargeback[0], $chargeback[1])
86+
new ChargebackReceived($updatedLocalPayment, $amountChargedBackNow)
7887
);
7988
}
8089
}

‎src/Http/Controllers/WebhookController.php‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,11 @@ public function handleWebhook(Request $request)
2828
switch ($payment->status) {
2929
case PaymentStatus::PAID:
3030
$order->handlePaymentPaid($payment);
31+
$payment->webhookUrl = route('webhooks.mollie.aftercare');
3132

32-
if ($order->wasPaymentStatusHandled()) {
33-
$payment->webhookUrl = route('webhooks.mollie.aftercare');
34-
35-
/** @var UpdateMolliePayment $updateMolliePayment */
36-
$updateMolliePayment = app()->make(UpdateMolliePayment::class);
37-
$updateMolliePayment->execute($payment);
38-
}
33+
/** @var UpdateMolliePayment $updateMolliePayment */
34+
$updateMolliePayment = app()->make(UpdateMolliePayment::class);
35+
$updateMolliePayment->execute($payment);
3936

4037
break;
4138
case PaymentStatus::FAILED:

‎src/Order/Order.php‎

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,6 @@ class Order extends Model
7878

7979
protected $guarded = [];
8080

81-
/** @var bool */
82-
protected $paymentStatusHandled = false;
83-
8481
/**
8582
* @return int
8683
*/
@@ -448,9 +445,10 @@ public function creditApplied()
448445
*/
449446
public function handlePaymentFailed(MolliePayment $molliePayment)
450447
{
451-
$this->paymentStatusHandled = false;
452448
$handled = false;
453449
$localPayment = DB::transaction(function () use ($molliePayment, &$handled) {
450+
// Webhook retries can arrive while callers still hold stale Order instances.
451+
// Reload the current row under lock so credit restoration and item hooks run once.
454452
/** @var static $order */
455453
$order = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
456454

@@ -468,9 +466,7 @@ public function handlePaymentFailed(MolliePayment $molliePayment)
468466
'credit_used' => 0,
469467
]);
470468

471-
// It's possible a payment from Cashier v1 is not yet tracked in the Cashier database.
472-
// In that case we create a record here.
473-
$localPayment = Cashier::$paymentModel::findByMolliePaymentOrCreate($molliePayment, $order->owner);
469+
$localPayment = Cashier::$paymentModel::findByPaymentIdOrFail($molliePayment->id);
474470
$localPayment->update([
475471
'mollie_payment_status' => PaymentStatus::FAILED,
476472
'order_id' => $order->id,
@@ -487,7 +483,6 @@ public function handlePaymentFailed(MolliePayment $molliePayment)
487483
});
488484

489485
$this->refresh();
490-
$this->paymentStatusHandled = $handled;
491486

492487
if ($handled) {
493488
Event::dispatch(new OrderPaymentFailed($this, $localPayment));
@@ -540,9 +535,10 @@ public function handlePaymentFailedDueToInvalidMandate()
540535
*/
541536
public function handlePaymentPaid(MolliePayment $molliePayment)
542537
{
543-
$this->paymentStatusHandled = false;
544538
$handled = false;
545539
$localPayment = DB::transaction(function () use ($molliePayment, &$handled) {
540+
// Paid webhooks have the same stale-instance problem: re-check under lock so the
541+
// order transition, local payment update, and item hooks are idempotent.
546542
/** @var static $order */
547543
$order = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
548544

@@ -552,9 +548,7 @@ public function handlePaymentPaid(MolliePayment $molliePayment)
552548

553549
$order->update(['mollie_payment_status' => PaymentStatus::PAID]);
554550

555-
// It's possible a payment from Cashier v1 is not yet tracked in the Cashier database.
556-
// In that case we create a record here.
557-
$localPayment = Cashier::$paymentModel::findByMolliePaymentOrCreate($molliePayment, $order->owner);
551+
$localPayment = Cashier::$paymentModel::findByPaymentIdOrFail($molliePayment->id);
558552
$localPayment->update([
559553
'mollie_payment_status' => PaymentStatus::PAID,
560554
'order_id' => $order->id,
@@ -569,7 +563,6 @@ public function handlePaymentPaid(MolliePayment $molliePayment)
569563
});
570564

571565
$this->refresh();
572-
$this->paymentStatusHandled = $handled;
573566

574567
if ($handled) {
575568
Event::dispatch(new OrderPaymentPaid($this, $localPayment));
@@ -578,16 +571,6 @@ public function handlePaymentPaid(MolliePayment $molliePayment)
578571
return $this;
579572
}
580573

581-
/**
582-
* Determine if the last payment status handler changed this order.
583-
*
584-
* @return bool
585-
*/
586-
public function wasPaymentStatusHandled()
587-
{
588-
return $this->paymentStatusHandled;
589-
}
590-
591574
/**
592575
* @return \Money\Money
593576
*/

‎src/Refunds/Refund.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ public function handleProcessed(): self
8181
$handled = false;
8282

8383
DB::transaction(function () use (&$handled) {
84+
// Aftercare retries can process the same pending refund twice unless the refund row is
85+
// locked and re-checked before creating the compensating order.
8486
/** @var static $refund */
8587
$refund = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
8688

@@ -125,6 +127,8 @@ public function handleFailed(): self
125127
$handled = false;
126128

127129
DB::transaction(function () use (&$handled) {
130+
// Only a pending refund may transition to failed; locking keeps duplicate deliveries
131+
// from re-running refund item failure hooks.
128132
/** @var static $refund */
129133
$refund = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
130134

‎tests/Order/OrderTest.php‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -243,9 +243,6 @@ public function handlesDuplicateFailedPaymentFromStaleOrderOnce()
243243
$firstOrderInstance->handlePaymentFailed($molliePayment);
244244
$secondOrderInstance->handlePaymentFailed($molliePayment);
245245

246-
$this->assertTrue($firstOrderInstance->wasPaymentStatusHandled());
247-
$this->assertFalse($secondOrderInstance->wasPaymentStatusHandled());
248-
249246
$order->refresh();
250247
$this->assertEquals(PaymentStatus::FAILED, $order->mollie_payment_status);
251248
$this->assertMoneyEURCents(0, $order->getBalanceBefore());
@@ -286,9 +283,6 @@ public function handlesDuplicatePaidPaymentFromStaleOrderOnce()
286283
$firstOrderInstance->handlePaymentPaid($molliePayment);
287284
$secondOrderInstance->handlePaymentPaid($molliePayment);
288285

289-
$this->assertTrue($firstOrderInstance->wasPaymentStatusHandled());
290-
$this->assertFalse($secondOrderInstance->wasPaymentStatusHandled());
291-
292286
$order->refresh();
293287
$this->assertEquals(PaymentStatus::PAID, $order->mollie_payment_status);
294288
$this->assertEquals(PaymentStatus::PAID, Cashier::$paymentModel::first()->mollie_payment_status);

0 commit comments

Comments
 (0)