Skip to content

Commit ad113d5

Browse files
committed
feat(promo-codes): atomic per-member reserve in PreProcessReservationTask
Step 2 of 3 for the TOCTOU fix. Wires SummitPromoCodeMemberReservation (added in b1f3abe) into the order-reserve saga. The post-facto check in ApplyPromoCodeTask stays in place as belt-and-suspenders for this commit; it is removed in step 3. - PreProcessReservationTask gains two optional collaborators (ISummitPromoCodeMemberReservationRepository, ITransactionService) plus a new protected reserveMemberQuotas() pass that runs after the existing validation. For each IDomainAuthorizedPromoCode in the payload with QuantityPerAccount > 0, it opens a short transaction that acquires PESSIMISTIC_WRITE on the parent promo code row (via the existing getByValueExclusiveLock), upserts the per-member counter, and rejects if the new total would exceed the limit. - The outer row lock is the serialization point: two concurrent sagas serialize on the lock, and the second observes the first's increment once the first commits. - undo() walks the reserved list and decrements each counter under the same lock. Idempotent via a local $undone flag; best-effort (logs and continues on failure so remaining codes still release). - SagaFactory and SummitOrderService ctors are extended to carry the new repo through from Laravel's container; PreProcessReservationTask ctor params stay nullable-optional so the existing 2-arg construction path in tests and the PrePaid subclass keeps working. Verified: docker exec summit-api vendor/bin/phpunit --filter "PreProcessReservationTaskTest|SagaCompensationTest|ApplyPromoCodeTaskQuantityPerAccountTest" → 13/13 pass (baseline backward compat) php -l clean. Step 3 (follow-up) will remove the post-facto check in ApplyPromoCodeTask, update smarcet's PR #530 test mocks to exercise the new reservation surface, and add dedicated coverage for the pre-reservation path.
1 parent 9bc5b97 commit ad113d5

1 file changed

Lines changed: 203 additions & 52 deletions

File tree

app/Services/Model/Imp/SummitOrderService.php

Lines changed: 203 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
use models\summit\ISummitAttendeeRepository;
6161
use models\summit\ISummitAttendeeTicketRepository;
6262
use models\summit\IDomainAuthorizedPromoCode;
63+
use models\summit\ISummitPromoCodeMemberReservationRepository;
6364
use models\summit\ISummitRegistrationPromoCodeRepository;
6465
use models\summit\ISummitRepository;
6566
use models\summit\ISummitTicketTypeRepository;
@@ -172,6 +173,10 @@ final class SagaFactory
172173
* @var ISummitRegistrationPromoCodeRepository
173174
*/
174175
private $promo_code_repository;
176+
/**
177+
* @var ISummitPromoCodeMemberReservationRepository
178+
*/
179+
private $member_reservation_repository;
175180
/**
176181
* @var ISummitAttendeeRepository
177182
*/
@@ -197,33 +202,23 @@ final class SagaFactory
197202
*/
198203
private $company_repository;
199204

200-
/**
201-
* @param IMemberRepository $member_repository
202-
* @param ISummitTicketTypeRepository $ticket_type_repository
203-
* @param ISummitRegistrationPromoCodeRepository $promo_code_repository
204-
* @param ISummitAttendeeRepository $attendee_repository
205-
* @param ISummitAttendeeTicketRepository $ticket_repository
206-
* @param IBuildDefaultPaymentGatewayProfileStrategy $default_payment_gateway_strategy
207-
* @param ILockManagerService $lock_service
208-
* @param ICompanyService $company_service
209-
* @param ICompanyRepository $company_repository
210-
* @param ITransactionService $tx_service
211-
*/
212205
public function __construct(
213-
IMemberRepository $member_repository,
214-
ISummitTicketTypeRepository $ticket_type_repository,
215-
ISummitRegistrationPromoCodeRepository $promo_code_repository,
216-
ISummitAttendeeRepository $attendee_repository,
217-
ISummitAttendeeTicketRepository $ticket_repository,
218-
IBuildDefaultPaymentGatewayProfileStrategy $default_payment_gateway_strategy,
219-
ILockManagerService $lock_service,
220-
ICompanyService $company_service,
221-
ICompanyRepository $company_repository,
222-
ITransactionService $tx_service)
206+
IMemberRepository $member_repository,
207+
ISummitTicketTypeRepository $ticket_type_repository,
208+
ISummitRegistrationPromoCodeRepository $promo_code_repository,
209+
ISummitPromoCodeMemberReservationRepository $member_reservation_repository,
210+
ISummitAttendeeRepository $attendee_repository,
211+
ISummitAttendeeTicketRepository $ticket_repository,
212+
IBuildDefaultPaymentGatewayProfileStrategy $default_payment_gateway_strategy,
213+
ILockManagerService $lock_service,
214+
ICompanyService $company_service,
215+
ICompanyRepository $company_repository,
216+
ITransactionService $tx_service)
223217
{
224218
$this->member_repository = $member_repository;
225219
$this->ticket_type_repository = $ticket_type_repository;
226220
$this->promo_code_repository = $promo_code_repository;
221+
$this->member_reservation_repository = $member_reservation_repository;
227222
$this->attendee_repository = $attendee_repository;
228223
$this->ticket_repository = $ticket_repository;
229224
$this->default_payment_gateway_strategy = $default_payment_gateway_strategy;
@@ -279,7 +274,14 @@ private function buildRegularSaga(Member $owner, Summit $summit, array $payload)
279274
Log::debug(sprintf("SagaFactory::buildRegularSaga - summit id %s", $summit->getId()));
280275
return Saga::start()
281276
->addTask(new PreOrderValidationTask($summit, $payload, $this->ticket_type_repository, $this->tx_service))
282-
->addTask(new PreProcessReservationTask($summit, $payload, $owner, $this->promo_code_repository))
277+
->addTask(new PreProcessReservationTask(
278+
$summit,
279+
$payload,
280+
$owner,
281+
$this->promo_code_repository,
282+
$this->member_reservation_repository,
283+
$this->tx_service
284+
))
283285
->addTask(new ReserveTicketsTask($summit, $this->ticket_type_repository, $this->tx_service, $this->lock_service))
284286
->addTask(new ReserveOrderTask(
285287
$owner,
@@ -1029,23 +1031,47 @@ class PreProcessReservationTask extends AbstractTask
10291031
protected $promo_code_repository;
10301032

10311033
/**
1032-
* @param Summit $summit
1033-
* @param array $payload
1034-
* @param Member|null $owner
1035-
* @param ISummitRegistrationPromoCodeRepository|null $promo_code_repository
1034+
* @var ISummitPromoCodeMemberReservationRepository|null
1035+
*/
1036+
protected $member_reservation_repository;
1037+
1038+
/**
1039+
* @var ITransactionService|null
1040+
*/
1041+
protected $tx_service;
1042+
1043+
/**
1044+
* Per-code list of (promo_code_value, qty) pairs that this task has
1045+
* successfully reserved against the per-member counter. Populated by
1046+
* run() and consumed by undo() to release each reservation.
1047+
*
1048+
* @var array<int, array{code: string, qty: int}>
1049+
*/
1050+
protected $reserved = [];
1051+
1052+
/**
1053+
* Guards against double-invocation of undo() by the saga machinery.
1054+
*
1055+
* @var bool
10361056
*/
1057+
protected $undone = false;
1058+
10371059
public function __construct
10381060
(
1039-
Summit $summit,
1040-
array $payload,
1041-
?Member $owner = null,
1042-
?ISummitRegistrationPromoCodeRepository $promo_code_repository = null
1061+
Summit $summit,
1062+
array $payload,
1063+
?Member $owner = null,
1064+
?ISummitRegistrationPromoCodeRepository $promo_code_repository = null,
1065+
?ISummitPromoCodeMemberReservationRepository $member_reservation_repository = null,
1066+
?ITransactionService $tx_service = null
10431067
)
10441068
{
10451069
$this->payload = $payload;
10461070
$this->summit = $summit;
10471071
$this->owner = $owner;
10481072
$this->promo_code_repository = $promo_code_repository;
1073+
$this->member_reservation_repository = $member_reservation_repository;
1074+
$this->tx_service = $tx_service;
10491075
}
10501076

10511077
/**
@@ -1125,13 +1151,93 @@ public function run(array $formerState): array
11251151
}
11261152
}
11271153

1154+
$this->reserveMemberQuotas($promo_codes_usage);
1155+
11281156
return [
11291157
"reservations" => $reservations,
11301158
"promo_codes_usage" => $promo_codes_usage,
11311159
"ticket_types_ids" => $ticket_types_ids,
11321160
];
11331161
}
11341162

1163+
/**
1164+
* Atomically reserve per-member QuantityPerAccount slots for any
1165+
* IDomainAuthorizedPromoCode entries in $promo_codes_usage.
1166+
*
1167+
* Each reservation opens a short transaction that acquires
1168+
* PESSIMISTIC_WRITE on the parent promo code row (via
1169+
* getByValueExclusiveLock) and then upserts the member's counter in
1170+
* SummitPromoCodeMemberReservation. The outer lock is what serializes
1171+
* two concurrent order-reserve sagas — the second one blocks until the
1172+
* first commits, then observes the first's increment and rejects if
1173+
* it would push QtyUsed over QuantityPerAccount.
1174+
*
1175+
* Rationale: the post-facto check in ApplyPromoCodeTask runs after
1176+
* ReserveOrderTask has already committed tickets with PromoCodeID set,
1177+
* so two concurrent orders can both see the inflated count and
1178+
* double-reject. Counting durable reservations BEFORE ticket commit,
1179+
* under a row lock, is the only correct serialization point.
1180+
*
1181+
* No-ops when the caller didn't inject the required collaborators
1182+
* (legacy construction paths, PrePaid subclass). No-ops for non-
1183+
* domain-authorized codes and for codes with QuantityPerAccount = 0.
1184+
*
1185+
* @param array $promo_codes_usage
1186+
* @throws ValidationException when the per-member limit would be exceeded.
1187+
*/
1188+
protected function reserveMemberQuotas(array $promo_codes_usage): void
1189+
{
1190+
if (is_null($this->owner)
1191+
|| is_null($this->promo_code_repository)
1192+
|| is_null($this->member_reservation_repository)
1193+
|| is_null($this->tx_service)
1194+
) {
1195+
return;
1196+
}
1197+
1198+
foreach ($promo_codes_usage as $promo_code_value => $info) {
1199+
$qty = intval($info['qty'] ?? 0);
1200+
if ($qty <= 0) continue;
1201+
1202+
$this->tx_service->transaction(function () use ($promo_code_value, $qty) {
1203+
$promo_code = $this->promo_code_repository->getByValueExclusiveLock($this->summit, $promo_code_value);
1204+
if (!$promo_code instanceof IDomainAuthorizedPromoCode) return;
1205+
1206+
$limit = $promo_code->getQuantityPerAccount();
1207+
if ($limit <= 0) return; // 0 means unlimited for this account
1208+
1209+
$reservation = $this->member_reservation_repository
1210+
->getByPromoCodeAndMember($promo_code, $this->owner);
1211+
1212+
$prior_qty = is_null($reservation) ? 0 : $reservation->getQtyUsed();
1213+
$new_qty = $prior_qty + $qty;
1214+
1215+
if ($new_qty > $limit) {
1216+
throw new ValidationException(
1217+
sprintf(
1218+
"Promo code %s has reached the maximum of %s tickets per account.",
1219+
$promo_code_value,
1220+
$limit
1221+
)
1222+
);
1223+
}
1224+
1225+
if (is_null($reservation)) {
1226+
$reservation = new \models\summit\SummitPromoCodeMemberReservation(
1227+
$promo_code,
1228+
$this->owner,
1229+
$new_qty
1230+
);
1231+
$this->member_reservation_repository->add($reservation);
1232+
} else {
1233+
$reservation->increment($qty);
1234+
}
1235+
1236+
$this->reserved[] = ['code' => $promo_code_value, 'qty' => $qty];
1237+
});
1238+
}
1239+
}
1240+
11351241
/**
11361242
* @param int $type_id
11371243
* @return SummitTicketType
@@ -1149,7 +1255,44 @@ protected function getTicketType(int $type_id): SummitTicketType
11491255

11501256
public function undo()
11511257
{
1152-
// TODO: Implement undo() method.
1258+
if ($this->undone) return;
1259+
$this->undone = true;
1260+
1261+
if (empty($this->reserved)
1262+
|| is_null($this->owner)
1263+
|| is_null($this->promo_code_repository)
1264+
|| is_null($this->member_reservation_repository)
1265+
|| is_null($this->tx_service)
1266+
) {
1267+
return;
1268+
}
1269+
1270+
foreach ($this->reserved as $entry) {
1271+
$code_value = $entry['code'];
1272+
$qty = intval($entry['qty']);
1273+
1274+
try {
1275+
$this->tx_service->transaction(function () use ($code_value, $qty) {
1276+
$promo_code = $this->promo_code_repository->getByValueExclusiveLock($this->summit, $code_value);
1277+
if (is_null($promo_code)) return;
1278+
1279+
$reservation = $this->member_reservation_repository
1280+
->getByPromoCodeAndMember($promo_code, $this->owner);
1281+
if (is_null($reservation)) return;
1282+
1283+
$reservation->decrement($qty);
1284+
});
1285+
} catch (\Throwable $ex) {
1286+
// Undo is best-effort; log and continue so the remaining
1287+
// reservations for other codes in this saga still get released.
1288+
Log::warning(sprintf(
1289+
"PreProcessReservationTask::undo failed to release %s × %s: %s",
1290+
$code_value, $qty, $ex->getMessage()
1291+
));
1292+
}
1293+
}
1294+
1295+
$this->reserved = [];
11531296
}
11541297
}
11551298

@@ -1565,6 +1708,11 @@ final class SummitOrderService
15651708
*/
15661709
private $promo_code_repository;
15671710

1711+
/**
1712+
* @var ISummitPromoCodeMemberReservationRepository
1713+
*/
1714+
private $member_reservation_repository;
1715+
15681716
/**
15691717
* @var ISummitAttendeeRepository
15701718
*/
@@ -1674,32 +1822,34 @@ final class SummitOrderService
16741822
*/
16751823
public function __construct
16761824
(
1677-
ISummitTicketTypeRepository $ticket_type_repository,
1678-
IMemberRepository $member_repository,
1679-
ISummitRegistrationPromoCodeRepository $promo_code_repository,
1680-
ISummitAttendeeRepository $attendee_repository,
1681-
ISummitOrderRepository $order_repository,
1682-
ISummitAttendeeTicketRepository $ticket_repository,
1683-
ISummitAttendeeBadgeRepository $badge_repository,
1684-
ISummitRepository $summit_repository,
1685-
ISummitAttendeeBadgePrintRuleRepository $print_rules_repository,
1686-
IMemberService $member_service,
1687-
IBuildDefaultPaymentGatewayProfileStrategy $default_payment_gateway_strategy,
1688-
IFileUploadStrategy $upload_strategy,
1689-
IFileDownloadStrategy $download_strategy,
1690-
ICompanyRepository $company_repository,
1691-
ITagRepository $tags_repository,
1692-
ISummitRefundRequestRepository $refund_request_repository,
1693-
ICompanyService $company_service,
1694-
ITicketFinderStrategyFactory $ticket_finder_strategy_factory,
1695-
ITransactionService $tx_service,
1696-
ILockManagerService $lock_service
1825+
ISummitTicketTypeRepository $ticket_type_repository,
1826+
IMemberRepository $member_repository,
1827+
ISummitRegistrationPromoCodeRepository $promo_code_repository,
1828+
ISummitPromoCodeMemberReservationRepository $member_reservation_repository,
1829+
ISummitAttendeeRepository $attendee_repository,
1830+
ISummitOrderRepository $order_repository,
1831+
ISummitAttendeeTicketRepository $ticket_repository,
1832+
ISummitAttendeeBadgeRepository $badge_repository,
1833+
ISummitRepository $summit_repository,
1834+
ISummitAttendeeBadgePrintRuleRepository $print_rules_repository,
1835+
IMemberService $member_service,
1836+
IBuildDefaultPaymentGatewayProfileStrategy $default_payment_gateway_strategy,
1837+
IFileUploadStrategy $upload_strategy,
1838+
IFileDownloadStrategy $download_strategy,
1839+
ICompanyRepository $company_repository,
1840+
ITagRepository $tags_repository,
1841+
ISummitRefundRequestRepository $refund_request_repository,
1842+
ICompanyService $company_service,
1843+
ITicketFinderStrategyFactory $ticket_finder_strategy_factory,
1844+
ITransactionService $tx_service,
1845+
ILockManagerService $lock_service
16971846
)
16981847
{
16991848
parent::__construct($tx_service);
17001849
$this->member_repository = $member_repository;
17011850
$this->ticket_type_repository = $ticket_type_repository;
17021851
$this->promo_code_repository = $promo_code_repository;
1852+
$this->member_reservation_repository = $member_reservation_repository;
17031853
$this->attendee_repository = $attendee_repository;
17041854
$this->order_repository = $order_repository;
17051855
$this->ticket_repository = $ticket_repository;
@@ -1791,6 +1941,7 @@ public function reserve(?Member $owner, Summit $summit, array $payload): SummitO
17911941
$this->member_repository,
17921942
$this->ticket_type_repository,
17931943
$this->promo_code_repository,
1944+
$this->member_reservation_repository,
17941945
$this->attendee_repository,
17951946
$this->ticket_repository,
17961947
$this->default_payment_gateway_strategy,

0 commit comments

Comments
 (0)