Repository navigation
fix(registration): delete rejected reservation orders on saga rollback - #621
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSaga compensation now continues after an undo operation throws. Order cleanup reloads the order and summit before removal. Promo-code quota checking no longer runs after order creation; quota enforcement occurs during reservation preprocessing. ChangesReservation compensation
Priority: ⚪ Not assessed Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No merge-blocking issue was identified in the changed reservation rollback behavior. Normal checks can proceed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-621/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The rollback behavior is correctly implemented and covered by focused unit and integration regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes reservation saga rollback so rejected orders are removed while all compensations continue.
Changes:
- Reloads detached orders and summits before deletion.
- Continues rollback after individual compensation failures.
- Adds regression coverage for cleanup, stock restoration, and exception propagation.
| File | Description |
|---|---|
app/Services/Model/Imp/SummitOrderService.php |
Makes saga compensation resilient and reloads entities before order removal. |
tests/SummitOrderServiceTest.php |
Tests rejected reservation cleanup and stock restoration. |
tests/Unit/Services/SagaCompensationTest.php |
Tests entity reload and continued compensation after failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ref: https://app.clickup.com/t/86bcd8mh1
Problem
When a reservation is rejected after its order was created (for example
ApplyPromoCodeTaskrejecting a speakers promo code that is not valid for the buyer), the saga rollback fails and the order is left behind:ReserveOrderTask::undo()throwsDetached entity SummitAttendeeBadge@N cannot be removed.Reserveduntil the revocation job cancels them (~20 minutes).ValidationException.ReserveTicketsTask::undo()(ticket stock) andPreProcessReservationTask::undo()(member quota) never run, because the exception aborts the compensation loop.Seen on OCP Global (2026-10-05): a speaker retried a
SPEAKERS_DISCOUNT_CODEreservation three times in 21 seconds. The one-use rule worked and rejected attempts 2 and 3, but the failed rollback left their ordersReserved, which looked like three redemptions.Root cause
DoctrineTransactionService::runRootTransactionclears the EntityManager when a root transaction fails (#533). The order and summit cached in the saga state were loaded by an earlier, already committed transaction, so they are detached whenReserveOrderTask::undo()runs.Fix
ReserveOrderTask::undo()reloads the order (getByIdExclusiveLock) and the summit from their repositories inside its transaction, the same wayApplyPromoCodeTaskandprocessOrder2Revokedo. Only the order id is read from the saga state.Saga::abort()catches and logs a failingundo()and continues with the remaining ones, so one broken compensation neither masks the original exception nor skips the stock and quota release.Tests
SummitOrderServiceTest::testReserveRejectedByPromoCodeLeavesNoOrderAndRestoresStock: reserving with a speakers discount code not assigned to the buyer throws the originalValidationException, leaves no order and restoresquantity_sold. Failed before the fix with the production error, passes now.SagaCompensationTest::testSagaAbortKeepsUndoingAndRethrowsOriginalWhenAnUndoFails: failed before the fix, passes now.SagaCompensationTest::testUndoDeletesOrderAndDetachesTicketsFromAttendeesupdated: it pinned the old behaviour (same instances passed in by mock). It now models the reload through the repositories.Run inside the
summit-apicontainer:tests/SummitOrderServiceTest.php: 42 tests, 0 failures, 3 skipped (existingmarkTestSkipped).tests/Unit/Services/has 2 failures inSponsorUserPermissionTrackingTestthat fail identically without this change.Not covered
The cron revocation still clears a speaker's redeemed flag by email when it cancels any order carrying the code (
SpeakersPromoCodeTrait::removeUsage). Orders orphaned by this bug were one source of that; other abandoned orders remain legitimate. Not changed here.Summary by CodeRabbit