Skip to content

Commit 529da22

Browse files
sandervanhooftclaude
sandervanhooft
and
claude
authored
Lock webhook handlers against concurrent duplicate deliveries (#321)
* Harden webhook handlers against duplicate processing * Refine webhook concurrency follow-up * Add stale-read chargeback coverage and simplify comparison Replace `! lessThan` with `greaterThanOrEqual` for readability in the aftercare chargeback branch, and add a stale-read regression test that proves concurrent deliveries dispatch `ChargebackReceived` only once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent db5d8aa commit 529da22

7 files changed

Lines changed: 384 additions & 51 deletions

File tree

‎src/Http/Controllers/AftercareWebhookController.php‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use Illuminate\Http\Request;
66
use Illuminate\Support\Collection;
7+
use Illuminate\Support\Facades\DB;
78
use Illuminate\Support\Facades\Event;
89
use Laravel\Cashier\Cashier;
910
use Laravel\Cashier\Events\ChargebackReceived;
@@ -48,18 +49,41 @@ public function handleWebhook(Request $request)
4849
}
4950

5051
$molliePaymentAmountChargedBackTotal = mollie_object_to_money($molliePayment->amountChargedBack);
51-
$locallyKnownAmountChargedBack = $localPayment->getAmountChargedBack();
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.
64+
/** @var \Laravel\Cashier\Payment|null $localPayment */
65+
$localPayment = Cashier::$paymentModel::whereKey($localPayment->getKey())->lockForUpdate()->first();
66+
67+
if (! $localPayment) {
68+
return;
69+
}
70+
71+
$locallyKnownAmountChargedBack = $localPayment->getAmountChargedBack();
72+
73+
if ($locallyKnownAmountChargedBack->greaterThanOrEqual($molliePaymentAmountChargedBackTotal)) {
74+
return;
75+
}
5276

53-
if ($locallyKnownAmountChargedBack->lessThan($molliePaymentAmountChargedBackTotal)) {
5477
$localPayment->amount_charged_back = (int) $molliePaymentAmountChargedBackTotal->getAmount();
5578
$localPayment->save();
5679

57-
$amountChargedBackNow = $molliePaymentAmountChargedBackTotal->subtract(
58-
$locallyKnownAmountChargedBack
59-
);
80+
$updatedLocalPayment = $localPayment;
81+
$amountChargedBackNow = $molliePaymentAmountChargedBackTotal->subtract($locallyKnownAmountChargedBack);
82+
});
6083

84+
if ($updatedLocalPayment && $amountChargedBackNow) {
6185
Event::dispatch(
62-
new ChargebackReceived($localPayment, $amountChargedBackNow)
86+
new ChargebackReceived($updatedLocalPayment, $amountChargedBackNow)
6387
);
6488
}
6589
}

‎src/Order/Order.php‎

Lines changed: 48 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -445,35 +445,48 @@ public function creditApplied()
445445
*/
446446
public function handlePaymentFailed(MolliePayment $molliePayment)
447447
{
448-
$localPayment = DB::transaction(function () use ($molliePayment) {
449-
if ($this->creditApplied()) {
450-
$this->owner->addCredit($this->getCreditUsed());
448+
$handled = false;
449+
$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.
452+
/** @var static $order */
453+
$order = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
454+
455+
if ($order->mollie_payment_status === PaymentStatus::FAILED) {
456+
return null;
451457
}
452458

453-
$this->update([
454-
'mollie_payment_status' => 'failed',
459+
if ($order->creditApplied()) {
460+
$order->owner->addCredit($order->getCreditUsed());
461+
}
462+
463+
$order->update([
464+
'mollie_payment_status' => PaymentStatus::FAILED,
455465
'balance_before' => 0,
456466
'credit_used' => 0,
457467
]);
458468

459-
// It's possible a payment from Cashier v1 is not yet tracked in the Cashier database.
460-
// In that case we create a record here.
461-
$localPayment = Cashier::$paymentModel::findByMolliePaymentOrCreate($molliePayment, $this->owner);
469+
$localPayment = Cashier::$paymentModel::findByPaymentIdOrFail($molliePayment->id);
462470
$localPayment->update([
463-
'mollie_payment_status' => 'failed',
464-
'order_id' => $this->id,
471+
'mollie_payment_status' => PaymentStatus::FAILED,
472+
'order_id' => $order->id,
465473
]);
466474

467-
$this->items->each(function (OrderItem $item) {
475+
$order->items->each(function (OrderItem $item) {
468476
$item->handlePaymentFailed();
469477
});
470478

471-
$this->owner->validateMollieMandate();
479+
$order->owner->validateMollieMandate();
472480

481+
$handled = true;
473482
return $localPayment;
474483
});
475484

476-
Event::dispatch(new OrderPaymentFailed($this, $localPayment));
485+
$this->refresh();
486+
487+
if ($handled) {
488+
Event::dispatch(new OrderPaymentFailed($this, $localPayment));
489+
}
477490

478491
return $this;
479492
}
@@ -522,25 +535,38 @@ public function handlePaymentFailedDueToInvalidMandate()
522535
*/
523536
public function handlePaymentPaid(MolliePayment $molliePayment)
524537
{
525-
$localPayment = DB::transaction(function () use ($molliePayment) {
526-
$this->update(['mollie_payment_status' => 'paid']);
538+
$handled = false;
539+
$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.
542+
/** @var static $order */
543+
$order = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
544+
545+
if ($order->mollie_payment_status === PaymentStatus::PAID) {
546+
return null;
547+
}
527548

528-
// It's possible a payment from Cashier v1 is not yet tracked in the Cashier database.
529-
// In that case we create a record here.
530-
$localPayment = Cashier::$paymentModel::findByMolliePaymentOrCreate($molliePayment, $this->owner);
549+
$order->update(['mollie_payment_status' => PaymentStatus::PAID]);
550+
551+
$localPayment = Cashier::$paymentModel::findByPaymentIdOrFail($molliePayment->id);
531552
$localPayment->update([
532-
'mollie_payment_status' => 'paid',
533-
'order_id' => $this->id,
553+
'mollie_payment_status' => PaymentStatus::PAID,
554+
'order_id' => $order->id,
534555
]);
535556

536-
$this->items->each(function (OrderItem $item) {
557+
$order->items->each(function (OrderItem $item) {
537558
$item->handlePaymentPaid();
538559
});
539560

561+
$handled = true;
540562
return $localPayment;
541563
});
542564

543-
Event::dispatch(new OrderPaymentPaid($this, $localPayment));
565+
$this->refresh();
566+
567+
if ($handled) {
568+
Event::dispatch(new OrderPaymentPaid($this, $localPayment));
569+
}
544570

545571
return $this;
546572
}

‎src/Refunds/Refund.php‎

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,26 @@ public function order(): HasOne
7878

7979
public function handleProcessed(): self
8080
{
81-
$refundItems = $this->items;
81+
$handled = false;
8282

83-
DB::transaction(function () use ($refundItems) {
83+
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.
86+
/** @var static $refund */
87+
$refund = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
88+
89+
if ($refund->mollie_refund_status !== RefundStatus::PENDING) {
90+
return;
91+
}
92+
93+
$refundItems = $refund->items;
8494
$orderItems = $refundItems->toNewOrderItemCollection()->save();
8595
$order = Cashier::$orderModel::createProcessedFromItems($orderItems);
8696

87-
$this->order_id = $order->id;
88-
$this->mollie_refund_status = RefundStatus::REFUNDED;
97+
$refund->order_id = $order->id;
98+
$refund->mollie_refund_status = RefundStatus::REFUNDED;
8999

90-
$this->save();
100+
$refund->save();
91101

92102
$refundItems->each(function (RefundItem $refundItem) {
93103
$originalOrderItem = $refundItem->originalOrderItem;
@@ -98,29 +108,51 @@ public function handleProcessed(): self
98108
}
99109
});
100110

101-
$this->originalOrder->increment('amount_refunded', (int) $refundItems->getTotal()->getAmount());
111+
$refund->originalOrder->increment('amount_refunded', (int) $refundItems->getTotal()->getAmount());
112+
113+
$handled = true;
102114
});
103115

104-
event(new RefundProcessed($this));
116+
$this->refresh();
117+
118+
if ($handled) {
119+
event(new RefundProcessed($this));
120+
}
105121

106122
return $this;
107123
}
108124

109125
public function handleFailed(): self
110126
{
111-
$refundItems = $this->items;
127+
$handled = false;
112128

113-
DB::transaction(function () use ($refundItems) {
114-
$this->mollie_refund_status = RefundStatus::FAILED;
129+
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.
132+
/** @var static $refund */
133+
$refund = static::whereKey($this->getKey())->lockForUpdate()->firstOrFail();
115134

116-
$this->save();
135+
if ($refund->mollie_refund_status !== RefundStatus::PENDING) {
136+
return;
137+
}
138+
139+
$refundItems = $refund->items;
140+
$refund->mollie_refund_status = RefundStatus::FAILED;
141+
142+
$refund->save();
117143

118144
$refundItems->each(function (RefundItem $refundItem) {
119145
$refundItem->originalOrderItem->handlePaymentRefundFailed($refundItem);
120146
});
147+
148+
$handled = true;
121149
});
122150

123-
event(new RefundFailed($this));
151+
$this->refresh();
152+
153+
if ($handled) {
154+
event(new RefundFailed($this));
155+
}
124156

125157
return $this;
126158
}

‎tests/Fixtures/ChargebackStaleReadPayment.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php
2+
3+
namespace Laravel\Cashier\Tests\Fixtures;
4+
5+
class ChargebackStaleReadPayment extends Payment
6+
{
7+
protected static int $remainingStaleReads = 0;
8+
9+
protected static int $staleAmountChargedBack = 0;
10+
11+
public static function returnStaleChargebackAmountOnNextReads(int $reads, int $amountChargedBack = 0): void
12+
{
13+
static::$remainingStaleReads = $reads;
14+
static::$staleAmountChargedBack = $amountChargedBack;
15+
}
16+
17+
public static function resetStaleChargebackReads(): void
18+
{
19+
static::$remainingStaleReads = 0;
20+
static::$staleAmountChargedBack = 0;
21+
}
22+
23+
public static function findByPaymentId($id): ?self
24+
{
25+
/** @var self|null $payment */
26+
$payment = parent::findByPaymentId($id);
27+
28+
if (! $payment || static::$remainingStaleReads <= 0) {
29+
return $payment;
30+
}
31+
32+
static::$remainingStaleReads--;
33+
34+
/** @var self $stalePayment */
35+
$stalePayment = clone $payment;
36+
$stalePayment->amount_charged_back = static::$staleAmountChargedBack;
37+
38+
return $stalePayment;
39+
}
40+
}

0 commit comments

Comments
 (0)