From 406faa526f2b2a7eccc97f6745429f55af59e8b0 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 6 Oct 2026 17:54:39 -0300 Subject: [PATCH] fix(registration): delete rejected reservation orders on saga rollback When a reservation is rejected after the order was created (e.g. a speakers promo code that is not valid for the buyer, raised by ApplyPromoCodeTask), ReserveOrderTask::undo() failed with "Detached entity SummitAttendeeBadge cannot be removed". The order and its tickets stayed Reserved until the revocation job cancelled them, and the caller received the Doctrine error instead of the ValidationException. Since the root transaction rollback clears the EntityManager (#533), the order and summit cached in the task state are detached by the time undo() runs. undo() now reloads both from their repositories, as ApplyPromoCodeTask and processOrder2Revoke already do. Saga::abort() also keeps running the remaining undo() calls when one fails and no longer replaces the original exception, so ticket stock and member quota are still released. --- app/Services/Model/Imp/SummitOrderService.php | 29 ++++++++-- tests/SummitOrderServiceTest.php | 52 ++++++++++++++++++ tests/Unit/Services/SagaCompensationTest.php | 54 ++++++++++++++++++- 3 files changed, 130 insertions(+), 5 deletions(-) diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 0c216b7c69..f488d63ab8 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -153,7 +153,14 @@ private function markAsRan(AbstractTask $task) private function abort() { foreach (array_reverse($this->already_run_tasks) as $task) { - $task->undo(); + // a failing compensation must neither mask the exception that aborted the saga + // nor prevent the remaining tasks (ticket stock, member quota) from being undone + try { + $task->undo(); + } catch (\Throwable $ex) { + Log::error(sprintf("Saga::abort undo failed for %s: %s", get_class($task), $ex->getMessage())); + Log::error($ex); + } } } } @@ -689,7 +696,21 @@ public function undo() return; } - $this->tx_service->transaction(function () use ($order) { + $order_id = $order->getId(); + + $this->tx_service->transaction(function () use ($order_id) { + // The order cached in formerState (and $this->summit) belong to the transaction that + // created them. When a later task fails, the root transaction rollback clears the + // EntityManager, so they are detached by now and can not be removed: reload them. + $order_repository = App::make(ISummitOrderRepository::class); + $order = $order_repository->getByIdExclusiveLock($order_id); + if (!$order instanceof SummitOrder) { + Log::warning(sprintf("ReserveOrderTask::undo: order id %s not found, nothing to compensate", $order_id)); + return; + } + // re attach + $summit = App::make(ISummitRepository::class)->getById($this->summit->getId()); + Log::info(sprintf("ReserveOrderTask::undo: removing reserved order id %s number %s", $order->getId(), $order->getNumber())); @@ -703,8 +724,8 @@ public function undo() } } - $this->summit->removeOrder($order); - App::make(ISummitOrderRepository::class)->delete($order); + $summit->removeOrder($order); + $order_repository->delete($order); }); } } diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php index 85f1640bbe..24c42ffee6 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -42,6 +42,8 @@ use models\main\ICompanyRepository; use models\main\IMemberRepository; use models\main\ITagRepository; +use App\Models\Foundation\Summit\PromoCodes\PromoCodesConstants; +use models\summit\SpeakersRegistrationDiscountCode; use models\summit\ISummitAttendeeRepository; use models\summit\ISummitAttendeeTicketRepository; use models\summit\ISummitRegistrationPromoCodeRepository; @@ -710,6 +712,56 @@ public function testReserveFailsWhenTicketTypeSoldOut() $this->assertEquals($initial_order_count, self::$summit->getOrders()->count()); } + /** + * ApplyPromoCodeTask (last saga task) rejects a speakers promo code that is not + * assigned to the buyer. The saga compensation must then delete the order that + * ReserveOrderTask already committed and restore the ticket type stock, and the + * caller must see the original ValidationException, not a detached-entity error. + */ + public function testReserveRejectedByPromoCodeLeavesNoOrderAndRestoresStock() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $promo_code = new SpeakersRegistrationDiscountCode(); + $promo_code->setCode('TEST_SPK_DC_' . uniqid()); + $promo_code->setType(PromoCodesConstants::SpeakerSummitRegistrationPromoCodeTypes[0]); + $promo_code->setAmount(100); + $promo_code->addAllowedTicketType(self::$default_ticket_type); + self::$summit->addPromoCode($promo_code); + self::$em->persist(self::$summit); + self::$em->flush(); + + $ticket_type_id = self::$default_ticket_type->getId(); + $initial_quantity_sold = self::$default_ticket_type->getQuantitySold(); + $initial_order_count = self::$summit->getOrders()->count(); + $buyer_email = sprintf('not-a-speaker-%s@test.com', uniqid()); + + $payload = [ + "owner_email" => $buyer_email, + "owner_first_name" => "Not", + "owner_last_name" => "Speaker", + "owner_company" => "Pumant", + "tickets" => [ + ["type_id" => $ticket_type_id, "promo_code" => $promo_code->getCode()], + ], + ]; + + try { + $service->reserve(null, self::$summit, $payload); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('is not valid for', $ex->getMessage()); + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $this->assertEquals($initial_order_count, self::$summit->getOrders()->count()); + + $reFetchedTicketType = self::$em->find(SummitTicketType::class, $ticket_type_id); + $this->assertEquals($initial_quantity_sold, $reFetchedTicketType->getQuantitySold()); + } + /** * ReserveTicketsTask rejects orders mixing ticket types of different currencies. */ diff --git a/tests/Unit/Services/SagaCompensationTest.php b/tests/Unit/Services/SagaCompensationTest.php index a1beb7cbc4..b671a53097 100644 --- a/tests/Unit/Services/SagaCompensationTest.php +++ b/tests/Unit/Services/SagaCompensationTest.php @@ -19,6 +19,7 @@ use libs\utils\ITransactionService; use Mockery; use models\main\Member; +use models\summit\ISummitRepository; use models\summit\Summit; use models\summit\SummitAttendee; use models\summit\SummitAttendeeTicket; @@ -119,16 +120,24 @@ public function testUndoDeletesOrderAndDetachesTicketsFromAttendees(): void $order->shouldReceive('getTickets')->andReturn([$ticket1, $ticket2, $ticket3]); $summit = Mockery::mock(Summit::class); + $summit->shouldReceive('getId')->andReturn(77); $summit->shouldReceive('removeOrder')->once()->with($order); $owner = Mockery::mock(Member::class); + // undo() reloads the order and the summit: the instances cached by run() are + // detached once the failing task's root transaction cleared the EntityManager. $order_repo = Mockery::mock(ISummitOrderRepository::class); + $order_repo->shouldReceive('getByIdExclusiveLock')->once()->with(9001)->andReturn($order); $order_repo->shouldReceive('delete')->once()->with($order); - // Bind the repo into the container so App::make() inside undo() resolves it. + $summit_repo = Mockery::mock(ISummitRepository::class); + $summit_repo->shouldReceive('getById')->once()->with(77)->andReturn($summit); + + // Bind the repos into the container so App::make() inside undo() resolves them. $container = \Illuminate\Support\Facades\Facade::getFacadeApplication(); $container->instance(ISummitOrderRepository::class, $order_repo); + $container->instance(ISummitRepository::class, $summit_repo); $tx_service = Mockery::mock(ITransactionService::class); $tx_service->shouldReceive('transaction')->once()->andReturnUsing(function ($fn) { @@ -179,6 +188,49 @@ public function undo() { /* never runs — it threw in run() */ } ); } + /** + * A compensation that itself fails must neither mask the exception that aborted + * the saga nor stop the remaining compensations (ticket stock, member quota) + * from running. + */ + public function testSagaAbortKeepsUndoingAndRethrowsOriginalWhenAnUndoFails(): void + { + $order_of_calls = []; + + $first = new RecordingTask('first', $order_of_calls); + $broken_undo = new class extends AbstractTask { + public function run(array $formerState): array + { + return $formerState; + } + public function undo() + { + throw new \LogicException('undo failure'); + } + }; + $failing = new class extends AbstractTask { + public function run(array $formerState): array + { + throw new \RuntimeException('downstream failure'); + } + public function undo() { /* never runs — it threw in run() */ } + }; + + $saga = Saga::start() + ->addTask($first) + ->addTask($broken_undo) + ->addTask($failing); + + try { + $saga->run(); + $this->fail('Expected saga to propagate the downstream exception'); + } catch (\RuntimeException $ex) { + $this->assertSame('downstream failure', $ex->getMessage()); + } + + $this->assertSame(['run:first', 'undo:first'], $order_of_calls); + } + /** * Construct a ReserveOrderTask with only the fields undo() needs. run() is * not exercised here, so most collaborators can be plain Mockery doubles.