fix(promo-codes): address Task 10 review follow-ups — race-safe quantity_per_account

caseylocker · claude · caseylocker · commit 82a28c371210 · 2026-04-08T22:12:02.000-05:00
Move ApplyPromoCodeTask after ReserveOrderTask in the saga chain so
ticket rows exist when the count query fires. Broaden
getTicketCountByMemberAndPromoCode to include 'Reserved' orders,
ensuring concurrent checkouts correctly see each other's reservations.
Remove the TOCTOU-vulnerable pre-check from PreProcessReservationTask
and relocate it inside ApplyPromoCodeTask's locked transaction, where
it naturally fires once per unique promo code.

Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php b/app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php
@@ -735,7 +735,7 @@ public function getTicketCountByMemberAndPromoCode(Member $member, SummitRegistr
 INNER JOIN SummitOrder o ON t.OrderID = o.ID
 WHERE t.PromoCodeID = :promo_code_id
 AND o.OwnerID = :member_id
-AND o.Status IN ('Paid', 'Confirmed')
+AND o.Status IN ('Reserved', 'Paid', 'Confirmed')
 AND t.Status != 'Cancelled'
 SQL;
         $stm = $this->getEntityManager()->getConnection()->executeQuery($sql, [
diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php
@@ -281,7 +281,6 @@ private function buildRegularSaga(Member $owner, Summit $summit, array $payload)
             ->addTask(new PreOrderValidationTask($summit, $payload, $this->ticket_type_repository, $this->tx_service))
             ->addTask(new PreProcessReservationTask($summit, $payload, $owner, $this->promo_code_repository))
             ->addTask(new ReserveTicketsTask($summit, $this->ticket_type_repository, $this->tx_service, $this->lock_service))
-            ->addTask(new ApplyPromoCodeTask($summit, $payload, $this->promo_code_repository, $this->tx_service, $this->lock_service))
             ->addTask(new ReserveOrderTask(
                 $owner,
                 $summit,
@@ -293,7 +292,8 @@ private function buildRegularSaga(Member $owner, Summit $summit, array $payload)
                 $this->company_repository,
                 $this->company_service,
                 $this->tx_service
-            ));
+            ))
+            ->addTask(new ApplyPromoCodeTask($summit, $payload, $owner, $this->promo_code_repository, $this->tx_service, $this->lock_service));
     }
 }
 
@@ -711,10 +711,16 @@ final class ApplyPromoCodeTask extends AbstractTask
      */
     private $lock_service;
 
+    /**
+     * @var Member|null
+     */
+    private $owner;
+
     /**
      * ApplyPromoCodeTask constructor.
      * @param Summit $summit
      * @param array $payload
+     * @param Member|null $owner
      * @param ISummitRegistrationPromoCodeRepository $promo_code_repository
      * @param ITransactionService $tx_service
      * @param ILockManagerService $lock_service
@@ -723,6 +729,7 @@ public function __construct
     (
         Summit                                 $summit,
         array                                  $payload,
+        ?Member                                $owner,
         ISummitRegistrationPromoCodeRepository $promo_code_repository,
         ITransactionService                    $tx_service,
         ILockManagerService                    $lock_service
@@ -731,6 +738,7 @@ public function __construct
         $this->tx_service = $tx_service;
         $this->summit = $summit;
         $this->payload = $payload;
+        $this->owner = $owner;
         $this->promo_code_repository = $promo_code_repository;
         $this->lock_service = $lock_service;
     }
@@ -780,6 +788,26 @@ public function run(array $formerState): array
                     }
                 }
 
+                // QuantityPerAccount enforcement for domain-authorized promo codes
+                // Runs inside the locked transaction, after ReserveOrderTask has created ticket rows
+                if ($promo_code instanceof IDomainAuthorizedPromoCode
+                    && !is_null($this->owner)
+                ) {
+                    $quantityPerAccount = $promo_code->getQuantityPerAccount();
+                    if ($quantityPerAccount > 0) {
+                        $existingCount = $this->promo_code_repository->getTicketCountByMemberAndPromoCode($this->owner, $promo_code);
+                        if ($existingCount > $quantityPerAccount) {
+                            throw new ValidationException(
+                                sprintf(
+                                    "Promo code %s has reached the maximum of %s tickets per account.",
+                                    $promo_code_value,
+                                    $quantityPerAccount
+                                )
+                            );
+                        }
+                    }
+                }
+
                 Log::debug(sprintf("adding %s usage to promo code %s", $qty, $promo_code->getId()));
 
                 $this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $qty, $owner_email) {
@@ -1039,27 +1067,6 @@ public function run(array $formerState): array
                         )
                     );
 
-                // QuantityPerAccount enforcement for domain-authorized promo codes
-                if ($promo_code instanceof IDomainAuthorizedPromoCode
-                    && !is_null($this->owner)
-                    && !is_null($this->promo_code_repository)
-                ) {
-                    $quantityPerAccount = $promo_code->getQuantityPerAccount();
-                    if ($quantityPerAccount > 0) {
-                        $existingCount = $this->promo_code_repository->getTicketCountByMemberAndPromoCode($this->owner, $promo_code);
-                        $newCount = $info['qty'];
-                        if (($existingCount + $newCount) > $quantityPerAccount) {
-                            throw new ValidationException(
-                                sprintf(
-                                    "Promo code %s has reached the maximum of %s tickets per account.",
-                                    $promo_code_value,
-                                    $quantityPerAccount
-                                )
-                            );
-                        }
-                    }
-                }
-
                 if (!in_array($type_id, $info['types']))
                     $info['types'] = array_merge($info['types'], [$type_id]);
 
diff --git a/doc/promo-codes-for-early-registration-access.md b/doc/promo-codes-for-early-registration-access.md
@@ -226,15 +226,15 @@ Deviations from the SDS captured during implementation. Each entry is either **O
 | D1 | Trait file locations | NIT | ACCEPTED | 2 | SDS specifies traits in `PromoCodes/` directly. Existing codebase convention puts traits in `PromoCodes/Traits/`. Implementation followed SDS paths. Acceptable — no functional impact, but future cleanup may move them to `Traits/` for consistency. |
 | D2 | `addTicketTypeRule` accesses private parent field via getter | NIT | ACCEPTED | 3 | SDS implies direct `$this->ticket_types_rules->add()` but parent declares `$ticket_types_rules` as `private`. Implementation uses `$this->getTicketTypesRules()->add()` and `canBeAppliedTo()` for the allowed_ticket_types membership check. Functionally equivalent. |
 | D3 | `allowed_email_domains` validation uses `sometimes|json` instead of custom rule | SHOULD-FIX | OPEN | 6 | SDS explicitly states generic `'sometimes|json'` is insufficient — would accept `[123, null, ""]` which silently never matches. Needs a custom validation rule enforcing each entry matches `@domain`, `.tld`, or `user@email` format. |
-| D4 | `quantity_per_account` check lacks pessimistic lock | MUST-FIX | OPEN | 10 | SDS specifies `SELECT ... FOR UPDATE` on the promo code row within the quantity check. Implementation adds the check in `PreProcessReservationTask` which runs before `ApplyPromoCodeTask` (which holds the lock). This creates a TOCTOU window — two concurrent requests could both pass the pre-check. The quantity check needs to move inside the locked transaction boundary, or `PreProcessReservationTask` needs its own pessimistic lock. |
+| D4 | `quantity_per_account` check lacks pessimistic lock AND count query is too narrow | MUST-FIX | OPEN | 10 | SDS specifies `SELECT ... FOR UPDATE` on the promo code row within the quantity check. Implementation adds the check in `PreProcessReservationTask` which runs before `ApplyPromoCodeTask` (which holds the lock). This creates a TOCTOU window. Additionally, even moving the check inside `ApplyPromoCodeTask`'s lock is insufficient: the count query (`getTicketCountByMemberAndPromoCode`) only counts 'Paid'/'Confirmed' orders, but ticket rows for the current request aren't created until `ReserveOrderTask` (the next saga step), so concurrent fresh checkouts both see count=0 inside the lock and both pass. Full fix requires task reorder + broader count — see Task 10 Review Follow-ups #1 and #3 for the complete fix specification. |
 | D5 | Discovery response uses manual array instead of `PagingResponse` object | NIT | ACCEPTED | 9 | SDS says "uses the standard `PagingResponse` envelope." Implementation constructs an identical JSON shape manually. Acceptable — output is identical, and the endpoint doesn't actually paginate. |
 | D6 | Task 8 implemented before Task 11 (dependency violation) | NIT | ACCEPTED | 8, 11 | SDS declares Task 8 depends on Task 11. Implementation order was reversed. No functional issue — the repository query fetches member/speaker entities by type regardless of whether `AutoApplyPromoCodeTrait` is applied yet. |
 | D7 | `addAllowedTicketType` overrides are no-ops | NIT | ACCEPTED | 3, 4 | SDS specifies overriding `addAllowedTicketType()` on both types. The override just calls `parent::addAllowedTicketType()` which already accepts any ticket type. Present for documentation intent per SDS, but functionally dead code. |
 
 ### Resolution Plan
 
 - **D3 (OPEN):** Create a custom Laravel validation rule class (e.g., `AllowedEmailDomainsRule`) that decodes the JSON and validates each entry matches `^@[\w.-]+$`, `^\.\w+$`, or `^[^@]+@[\w.-]+$`. Apply in both `buildForAdd` and `buildForUpdate` for domain-authorized types.
-- **D4 (OPEN):** Move the `quantity_per_account` check into `ApplyPromoCodeTask` (which already holds a pessimistic lock via `getByValueExclusiveLock`), or add a `SELECT ... FOR UPDATE` on the promo code row in `PreProcessReservationTask` by passing the transaction service. The former is cleaner since the lock already exists.
+- **D4 (OPEN):** Moving the check into `ApplyPromoCodeTask` alone is insufficient. The count query only covers 'Paid'/'Confirmed' orders, but the current request's tickets don't exist until `ReserveOrderTask` (the next saga step). See Task 10 Review Follow-ups #1 and #3 for the full fix specification — the preferred approach is to move `ApplyPromoCodeTask` after `ReserveOrderTask` in the saga chain AND widen the count query to include 'Reserved' status orders.
 
 ## Implementation Tasks
 
@@ -748,7 +748,14 @@ Deviations from the SDS captured during implementation. Each entry is either **O
 - Integration test: concurrent checkouts by same member cannot exceed limit
 
 **Review Follow-ups:**
-- None
+- [x] **[MUST-FIX] Quantity check runs outside the locked transaction (TOCTOU).** The `quantity_per_account` enforcement block added at `SummitOrderService.php:1043–1061` is inside `PreProcessReservationTask::run()`, which executes with no enclosing `ITransactionService::transaction()`. The exclusive row lock (`getByValueExclusiveLock`) is not acquired until `ApplyPromoCodeTask` — three saga steps later. Two concurrent checkouts by the same member can both pass the pre-check before either reaches the lock, violating the DoD concurrency requirement. **Fix:** see follow-up #3 below (the check must be relocated and the count query broadened together; fixing placement alone is not sufficient).
+
+- [x] **[SHOULD-FIX] `getTicketCountByMemberAndPromoCode` called once per ticket instead of once per promo code.** In `PreProcessReservationTask::run()` the loop at `SummitOrderService.php:993–1067` iterates per ticket DTO. On every iteration where a domain-authorized promo code is present, `getTicketCountByMemberAndPromoCode` is issued at line 1049 — returning the same value each time because nothing is written between calls. For an order with N tickets under the same promo code, N identical DB queries fire when one would suffice. **Fix:** after the existing per-ticket loop completes and `$promo_codes_usage` is fully populated, add a second loop that iterates over the aggregated `$promo_codes_usage` map (one entry per unique promo code value) and performs the count + threshold check once per code. Alternatively, if the check moves into `ApplyPromoCodeTask` per follow-up #3, it naturally fires once per unique promo code since that task already iterates over `$promo_codes_usage`.
+
+- [x] **[MUST-FIX] The proposed D4 fix (move check into `ApplyPromoCodeTask`) is necessary but not sufficient — the count query must also be broadened to include Reserved orders, AND the check must run after ticket rows exist.** Even with the check inside `ApplyPromoCodeTask`'s locked transaction, `getTicketCountByMemberAndPromoCode` only counts `o.Status IN ('Paid', 'Confirmed')` (`DoctrineSummitRegistrationPromoCodeRepository.php:738`). Ticket rows for the current order are not created until `ReserveOrderTask` — the next saga step — at `SummitOrderService.php:550–570` where `$ticket->setPromoCode($this)` writes `PromoCodeID`. So when Request B acquires the promo code lock after Request A commits, it still sees count=0 (A's tickets do not yet exist, and once they do they are 'Reserved', not 'Paid'/'Confirmed'). Both requests proceed to `ReserveOrderTask`, both create reservations, and both can be paid — exceeding the limit. **Two viable fix approaches:**
+  - **(Preferred — task reorder + broader count):** Move `ApplyPromoCodeTask` to run AFTER `ReserveOrderTask` in the saga chain (`buildRegularSaga`). At that point the current request's tickets already exist in the DB with `PromoCodeID` set and `o.Status = 'Reserved'`. Update `getTicketCountByMemberAndPromoCode` to count non-cancelled tickets across all non-void order statuses: change `o.Status IN ('Paid', 'Confirmed')` to `o.Status IN ('Reserved', 'Paid', 'Confirmed')` (the existing `t.Status != 'Cancelled'` filter already excludes expired/cancelled tickets). With this change, Request B — after acquiring the lock — sees Request A's 'Reserved' ticket in the count and correctly fails. Note: the `undo()` method on `ApplyPromoCodeTask` already handles rollback via `removeUsage`, so the task order swap does not affect compensation logic. Also update the `ApplyPromoCodeTask::run()` call site to add the owner and a quantity-per-account check block (mirroring the logic currently in `PreProcessReservationTask`) after the `getByValueExclusiveLock` call and before `addUsage`.
+  - **(Alternative — application-level lock spanning the saga):** Use `$lock_service->lock('member.{memberId}.promocode.{promoCodeId}.qty.lock', ...)` keyed by both member and promo code ID, held from the count check through the end of `ReserveOrderTask`. This avoids reordering tasks but requires passing both `$owner` and `$lock_service` into either `PreProcessReservationTask` or a new dedicated task inserted between `ApplyPromoCodeTask` and `ReserveOrderTask`. The lock must be released only after `ReserveOrderTask` commits.
+  - **Note — `ReserveOrderTask::undo()` stub:** The preferred fix (task reorder) means a failed `ApplyPromoCodeTask` now leaves an orphaned 'Reserved' order because `ReserveOrderTask::undo()` (`SummitOrderService.php:671`) is a pre-existing `// TODO` stub introduced in commit `39e3c8e33` (original Summit Registration model) — predating this SDS entirely. Since the count query now includes 'Reserved' orders, orphaned reservations temporarily inflate the member's quota until the reservation expiry job (`revokeReservedOrdersOlderThanNMinutes`) clears them. This is pre-existing technical debt that was dormant when `ApplyPromoCodeTask` ran before `ReserveOrderTask`. It is out of scope for this SDS and should be tracked separately.
 
 ---
 
