Fix waitlist race conditions and capacity double-counting

backend/app/Jobs/Waitlist/ProcessExpiredWaitlistOffersJob.php

@@ -11,6 +11,7 @@
 use HiEvents\Repository\Interfaces\WaitlistEntryRepositoryInterface;
 use Illuminate\Bus\Queueable;
 use Illuminate\Contracts\Queue\ShouldQueue;
+use Illuminate\Database\DatabaseManager;
 use Illuminate\Foundation\Bus\Dispatchable;
 use Illuminate\Queue\InteractsWithQueue;
 use Illuminate\Queue\SerializesModels;
@@ -25,6 +26,7 @@ public function handle(
         WaitlistEntryRepositoryInterface $repository,
         OrderRepositoryInterface         $orderRepository,
         ProductPriceRepositoryInterface  $productPriceRepository,
+        DatabaseManager                  $databaseManager,
     ): void
     {
         $expiredEntries = $repository->findWhere([
@@ -35,23 +37,37 @@ public function handle(
 
         foreach ($expiredEntries as $entry) {
             try {
-                if ($entry->getOrderId() !== null) {
-                    $orderRepository->deleteWhere([
-                        'id' => $entry->getOrderId(),
-                        'status' => OrderStatus::RESERVED->name,
-                    ]);
-                }
+                $databaseManager->transaction(function () use ($entry, $repository, $orderRepository) {
+                    $lockedEntry = $repository->findByIdLocked($entry->getId());
+
+                    if ($lockedEntry === null || $lockedEntry->getStatus() !== WaitlistEntryStatus::OFFERED->name) {
+                        return;
+                    }
+
+                    if ($lockedEntry->getOrderId() !== null) {
+                        $orderRepository->deleteWhere([
+                            'id' => $lockedEntry->getOrderId(),
+                            'status' => OrderStatus::RESERVED->name,
+                        ]);
+                    }
 
-                $repository->updateWhere(
-                    attributes: [
-                        'status' => WaitlistEntryStatus::OFFER_EXPIRED->name,
-                        'offer_token' => null,
-                        'offered_at' => null,
-                        'offer_expires_at' => null,
-                        'order_id' => null,
-                    ],
-                    where: ['id' => $entry->getId()],
-                );
+                    $repository->updateWhere(
+                        attributes: [
+                            'status' => WaitlistEntryStatus::OFFER_EXPIRED->name,
+                            'offer_token' => null,
+                            'offered_at' => null,
+                            'offer_expires_at' => null,
+                            'order_id' => null,
+                        ],
+                        where: ['id' => $entry->getId()],
+                    );
+                });
+
+                $freshEntry = $repository->findById($entry->getId());
+
+                if ($freshEntry->getStatus() !== WaitlistEntryStatus::OFFER_EXPIRED->name) {
+                    continue;
+                }
 
                 SendWaitlistOfferExpiredEmailJob::dispatch($entry);
 

backend/app/Repository/Eloquent/WaitlistEntryRepository.php

@@ -122,6 +122,20 @@ public function lockForProductPrice(int $productPriceId): void
             ->get();
     }
 
+    public function findByIdLocked(int $id): ?WaitlistEntryDomainObject
+    {
+        $model = WaitlistEntry::query()
+            ->where('id', $id)
+            ->lockForUpdate()
+            ->first();
+
+        if ($model === null) {
+            return null;
+        }
+
+        return $this->handleSingleResult($model);
+    }
+
     public function findByEventId(int $eventId, QueryParamsDTO $params): LengthAwarePaginator
     {
         $where = [

backend/app/Repository/Interfaces/WaitlistEntryRepositoryInterface.php

@@ -27,4 +27,6 @@ public function getMaxPosition(int $productPriceId): int;
     public function getNextWaitingEntries(int $productPriceId, int $limit): Collection;
 
     public function lockForProductPrice(int $productPriceId): void;
+
+    public function findByIdLocked(int $id): ?WaitlistEntryDomainObject;
 }

backend/app/Services/Domain/Waitlist/CreateWaitlistEntryService.php

@@ -32,11 +32,11 @@ public function createEntry(
     ): WaitlistEntryDomainObject
     {
         $this->validateWaitlistEnabled($product);
-        $this->validateNoDuplicate($dto);
 
         /** @var WaitlistEntryDomainObject $entry */
         $entry = $this->databaseManager->transaction(function () use ($dto) {
             $this->waitlistEntryRepository->lockForProductPrice($dto->product_price_id);
+            $this->validateNoDuplicate($dto);
             $position = $this->calculatePosition($dto);
 
             return $this->waitlistEntryRepository->create([

backend/app/Services/Domain/Waitlist/ProcessWaitlistService.php

@@ -53,32 +53,25 @@ public function offerToNext(
     ): Collection
     {
         return $this->databaseManager->transaction(function () use ($productPriceId, $quantity, $event, $eventSettings) {
+            $this->databaseManager->statement('SELECT pg_advisory_xact_lock(?)', [$event->getId()]);
             $this->waitlistEntryRepository->lockForProductPrice($productPriceId);
 
             $quantities = $this->availableQuantitiesService->getAvailableProductQuantities(
                 $event->getId(),
                 ignoreCache: true,
             );
 
-            $actualAvailable = $this->getAvailableCountForPrice($quantities, $productPriceId);
+            $availableCount = $this->getAvailableCountForPrice($quantities, $productPriceId);
 
-            $currentlyOfferedCount = $this->waitlistEntryRepository->countWhere([
-                'product_price_id' => $productPriceId,
-                'status' => WaitlistEntryStatus::OFFERED->name,
-            ]);
-
-            $effectiveAvailable = $actualAvailable - $currentlyOfferedCount;
-
-            if ($effectiveAvailable <= 0) {
+            if ($availableCount <= 0) {
                 throw new NoCapacityAvailableException(
-                    __('No capacity available. Available: :available, reserved by pending offers: :offered', [
-                        'available' => $actualAvailable,
-                        'offered' => $currentlyOfferedCount,
+                    __('No capacity available. Available: :available', [
+                        'available' => $availableCount,
                     ])
                 );
             }
 
-            $toOffer = min($quantity, $effectiveAvailable);
+            $toOffer = min($quantity, $availableCount);
             $entries = $this->waitlistEntryRepository->getNextWaitingEntries($productPriceId, $toOffer);
 
             if ($entries->isEmpty()) {
@@ -109,6 +102,8 @@ public function offerSpecificEntry(
     ): Collection
     {
         return $this->databaseManager->transaction(function () use ($entryId, $eventId, $event, $eventSettings) {
+            $this->databaseManager->statement('SELECT pg_advisory_xact_lock(?)', [$event->getId()]);
+
             /** @var WaitlistEntryDomainObject|null $entry */
             $entry = $this->waitlistEntryRepository->findFirstWhere([
                 'id' => $entryId,
@@ -133,18 +128,12 @@ public function offerSpecificEntry(
                 ignoreCache: true,
             );
 
-            $actualAvailable = $this->getAvailableCountForPrice($quantities, $entry->getProductPriceId());
-
-            $currentlyOfferedCount = $this->waitlistEntryRepository->countWhere([
-                'product_price_id' => $entry->getProductPriceId(),
-                'status' => WaitlistEntryStatus::OFFERED->name,
-            ]);
+            $availableCount = $this->getAvailableCountForPrice($quantities, $entry->getProductPriceId());
 
-            if ($actualAvailable <= $currentlyOfferedCount) {
+            if ($availableCount <= 0) {
                 throw new NoCapacityAvailableException(
-                    __('No capacity available to offer this waitlist entry. You will need to increase the available quantity for the product. Available: :available, already offered: :offered', [
-                        'available' => $actualAvailable,
-                        'offered' => $currentlyOfferedCount,
+                    __('No capacity available to offer this waitlist entry. You will need to increase the available quantity for the product. Available: :available', [
+                        'available' => $availableCount,
                     ])
                 );
             }

backend/tests/Unit/Jobs/Waitlist/ProcessExpiredWaitlistOffersJobTest.php

@@ -12,6 +12,7 @@
 use HiEvents\Repository\Interfaces\OrderRepositoryInterface;
 use HiEvents\Repository\Interfaces\ProductPriceRepositoryInterface;
 use HiEvents\Repository\Interfaces\WaitlistEntryRepositoryInterface;
+use Illuminate\Database\DatabaseManager;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Bus;
 use Illuminate\Support\Facades\Event;
@@ -24,13 +25,21 @@ class ProcessExpiredWaitlistOffersJobTest extends TestCase
     private WaitlistEntryRepositoryInterface $repository;
     private OrderRepositoryInterface $orderRepository;
     private ProductPriceRepositoryInterface $productPriceRepository;
+    private DatabaseManager $databaseManager;
 
     protected function setUp(): void
     {
         parent::setUp();
         $this->repository = m::mock(WaitlistEntryRepositoryInterface::class);
         $this->orderRepository = m::mock(OrderRepositoryInterface::class);
         $this->productPriceRepository = m::mock(ProductPriceRepositoryInterface::class);
+        $this->databaseManager = m::mock(DatabaseManager::class);
+
+        $this->databaseManager
+            ->shouldReceive('transaction')
+            ->andReturnUsing(function ($callback) {
+                return $callback();
+            });
 
         $productPrice = new ProductPriceDomainObject();
         $productPrice->setId(20);
@@ -59,6 +68,12 @@ public function testProcessesExpiredOffersAndDispatchesEmailAndEvent(): void
             ->once()
             ->andReturn(new Collection([$entry]));
 
+        $this->repository
+            ->shouldReceive('findByIdLocked')
+            ->once()
+            ->with(1)
+            ->andReturn($entry);
+
         $this->orderRepository
             ->shouldReceive('deleteWhere')
             ->once()
@@ -81,8 +96,20 @@ public function testProcessesExpiredOffersAndDispatchesEmailAndEvent(): void
                 ['id' => 1],
             );
 
+        $expiredEntry = new WaitlistEntryDomainObject();
+        $expiredEntry->setId(1);
+        $expiredEntry->setEventId(10);
+        $expiredEntry->setProductPriceId(20);
+        $expiredEntry->setStatus(WaitlistEntryStatus::OFFER_EXPIRED->name);
+
+        $this->repository
+            ->shouldReceive('findById')
+            ->once()
+            ->with(1)
+            ->andReturn($expiredEntry);
+
         $job = new ProcessExpiredWaitlistOffersJob();
-        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository);
+        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository, $this->databaseManager);
 
         Bus::assertDispatched(SendWaitlistOfferExpiredEmailJob::class);
         Event::assertDispatched(CapacityChangedEvent::class, function ($event) {
@@ -107,14 +134,32 @@ public function testSkipsOrderDeletionWhenNoOrderId(): void
             ->once()
             ->andReturn(new Collection([$entry]));
 
+        $this->repository
+            ->shouldReceive('findByIdLocked')
+            ->once()
+            ->with(2)
+            ->andReturn($entry);
+
         $this->orderRepository->shouldNotReceive('deleteWhere');
 
         $this->repository
             ->shouldReceive('updateWhere')
             ->once();
 
+        $expiredEntry = new WaitlistEntryDomainObject();
+        $expiredEntry->setId(2);
+        $expiredEntry->setEventId(10);
+        $expiredEntry->setProductPriceId(20);
+        $expiredEntry->setStatus(WaitlistEntryStatus::OFFER_EXPIRED->name);
+
+        $this->repository
+            ->shouldReceive('findById')
+            ->once()
+            ->with(2)
+            ->andReturn($expiredEntry);
+
         $job = new ProcessExpiredWaitlistOffersJob();
-        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository);
+        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository, $this->databaseManager);
 
         Bus::assertDispatched(SendWaitlistOfferExpiredEmailJob::class);
         Event::assertDispatched(CapacityChangedEvent::class);
@@ -131,7 +176,7 @@ public function testDoesNothingWhenNoExpiredEntries(): void
             ->andReturn(new Collection());
 
         $job = new ProcessExpiredWaitlistOffersJob();
-        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository);
+        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository, $this->databaseManager);
 
         Bus::assertNotDispatched(SendWaitlistOfferExpiredEmailJob::class);
         Event::assertNotDispatched(CapacityChangedEvent::class);
@@ -162,19 +207,63 @@ public function testCatchesExceptionAndLogsError(): void
             ->once()
             ->andReturn(new Collection([$entry]));
 
-        $this->orderRepository
-            ->shouldReceive('deleteWhere')
+        $this->repository
+            ->shouldReceive('findByIdLocked')
             ->once()
+            ->with(1)
             ->andThrow(new \RuntimeException('DB connection lost'));
 
         $job = new ProcessExpiredWaitlistOffersJob();
-        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository);
+        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository, $this->databaseManager);
 
         $this->assertTrue($logged, 'Error was logged for failed expired offer processing');
         Bus::assertNotDispatched(SendWaitlistOfferExpiredEmailJob::class);
         Event::assertNotDispatched(CapacityChangedEvent::class);
     }
 
+    public function testSkipsEntryWhenStatusChangedBeforeLock(): void
+    {
+        Bus::fake();
+        Event::fake();
+
+        $entry = new WaitlistEntryDomainObject();
+        $entry->setId(1);
+        $entry->setEventId(10);
+        $entry->setProductPriceId(20);
+        $entry->setOrderId(100);
+        $entry->setStatus(WaitlistEntryStatus::OFFERED->name);
+
+        $this->repository
+            ->shouldReceive('findWhere')
+            ->once()
+            ->andReturn(new Collection([$entry]));
+
+        $cancelledEntry = new WaitlistEntryDomainObject();
+        $cancelledEntry->setId(1);
+        $cancelledEntry->setStatus(WaitlistEntryStatus::CANCELLED->name);
+
+        $this->repository
+            ->shouldReceive('findByIdLocked')
+            ->once()
+            ->with(1)
+            ->andReturn($cancelledEntry);
+
+        $this->orderRepository->shouldNotReceive('deleteWhere');
+        $this->repository->shouldNotReceive('updateWhere');
+
+        $this->repository
+            ->shouldReceive('findById')
+            ->once()
+            ->with(1)
+            ->andReturn($cancelledEntry);
+
+        $job = new ProcessExpiredWaitlistOffersJob();
+        $job->handle($this->repository, $this->orderRepository, $this->productPriceRepository, $this->databaseManager);
+
+        Bus::assertNotDispatched(SendWaitlistOfferExpiredEmailJob::class);
+        Event::assertNotDispatched(CapacityChangedEvent::class);
+    }
+
     protected function tearDown(): void
     {
         m::close();

backend/tests/Unit/Services/Domain/Waitlist/CreateWaitlistEntryServiceTest.php

@@ -135,6 +135,11 @@ public function testPreventsDuplicateEntryForSameEmailAndProduct(): void
 
         $existingEntry = Mockery::mock(WaitlistEntryDomainObject::class);
 
+        $this->waitlistEntryRepository
+            ->shouldReceive('lockForProductPrice')
+            ->once()
+            ->with(10);
+
         $this->waitlistEntryRepository
             ->shouldReceive('findFirstWhere')
             ->once()