fix(transactions): prevent nested transaction from destroying outer EntityManager on connection error#533
Open
smarcet wants to merge 23 commits into
Open
fix(transactions): prevent nested transaction from destroying outer EntityManager on connection error#533smarcet wants to merge 23 commits into
smarcet wants to merge 23 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesTransaction rollback safety
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
actor Caller
participant DoctrineTransactionService
participant Connection
participant EntityManager
participant Callback
Caller->>DoctrineTransactionService: transaction(callback)
DoctrineTransactionService->>Connection: check transaction state
alt Root transaction
DoctrineTransactionService->>Connection: begin transaction
DoctrineTransactionService->>Callback: execute callback
Callback-->>DoctrineTransactionService: result or exception
DoctrineTransactionService->>EntityManager: flush
DoctrineTransactionService->>Connection: commit or rollback
else Nested transaction
DoctrineTransactionService->>Connection: begin nested level
DoctrineTransactionService->>Callback: execute callback
Callback-->>DoctrineTransactionService: result or exception
DoctrineTransactionService->>EntityManager: flush
DoctrineTransactionService->>Connection: commit or rollback nested level
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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-533/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/Unit/Services/DoctrineTransactionServiceTest.php (1)
74-80: Fix the helper array-shape annotation.
buildMocks()returns three values, but the PHPDoc only declares two. Please include the registry in the shape so destructuring and static analysis stay accurate.♻️ Suggested PHPDoc update
- * `@return` array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface} + * `@return` array{ + * EntityManagerInterface&\Mockery\MockInterface, + * Connection&\Mockery\MockInterface, + * ManagerRegistry&\Mockery\MockInterface + * }Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Services/DoctrineTransactionServiceTest.php` around lines 74 - 80, The PHPDoc for buildMocks() declares only two return elements but the method actually returns three (EntityManager, Connection, and the Registry), so update the `@return` array shape to include the third element as RegistryInterface&\Mockery\MockInterface (e.g. array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface, RegistryInterface&\Mockery\MockInterface}) so destructuring and static analysis are correct; apply the same correction to the other identical PHPDoc occurrence (the second buildMocks annotation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Services/Utils/DoctrineTransactionService.php`:
- Around line 231-237: The nested transaction path in runNestedTransaction does
not enable savepoints before calling beginTransaction, which can cause nested
rollbacks to poison outer transactions; update the runNestedTransaction
implementation (the method named runNestedTransaction and its use of $em and
$conn) to call $em->getConnection()->setNestTransactionsWithSavepoints(true) (or
$conn->setNestTransactionsWithSavepoints(true)) prior to
$conn->beginTransaction(), mirroring the root transaction path where
setNestTransactionsWithSavepoints(true) is set before beginning the transaction
so savepoints are used for nested transactions.
- Around line 164-178: The retry path is incorrectly triggered for exceptions
thrown during commit(), causing duplicate callback execution; add a commit-phase
guard (e.g. a boolean $commitStarted or $inCommitPhase) in
DoctrineTransactionService around the call to $conn->commit() inside the
transaction block (set it true immediately before calling commit and false only
after success) and update the outer catch that inspects shouldReconnect($ex) so
that if the exception occurred while $commitStarted is true you do not
retry/re-execute the callback but instead rethrow/propagate the commit failure
immediately; reference the inner transaction block where $result =
$callback($this), $em->flush(), $conn->commit() occur and the outer catch
handling $ex and shouldReconnect().
- Around line 243-248: The catch block in DoctrineTransactionService currently
rolls back the DB transaction but leaves the EntityManager’s in-memory state
intact (catch (\Throwable $ex) ... $conn->rollBack(); throw $ex;), which can
allow failed nested mutations to be persisted later; after performing
$conn->rollBack() update the catch to also reset or refresh the ORM state—either
call the EntityManager reset/close/clear (e.g. $this->entityManager->clear() or
via ManagerRegistry->resetManager()) or explicitly refresh/clear the specific
managed entities mutated by the nested callback—then rethrow; alternatively
enforce/document that nested callbacks must not be caught-and-continued so root
callers recreate a fresh EntityManager.
In `@tests/Unit/Services/DoctrineTransactionServiceTest.php`:
- Around line 775-783: The test currently asserts $callCount after calling
$service->transaction while expecting TestRetryableException, so that assertion
is never reached; wrap the transaction call in a try/catch (catching
TestRetryableException) and inside the catch assert that $callCount ===
DoctrineTransactionService::MaxRetries, then rethrow or let the test still
satisfy expectException; alternatively remove expectException and assert the
exception was thrown and $callCount equals
DoctrineTransactionService::MaxRetries within the caught exception path to
verify the retry behavior of the transaction() callback.
---
Nitpick comments:
In `@tests/Unit/Services/DoctrineTransactionServiceTest.php`:
- Around line 74-80: The PHPDoc for buildMocks() declares only two return
elements but the method actually returns three (EntityManager, Connection, and
the Registry), so update the `@return` array shape to include the third element as
RegistryInterface&\Mockery\MockInterface (e.g.
array{EntityManagerInterface&\Mockery\MockInterface,
Connection&\Mockery\MockInterface, RegistryInterface&\Mockery\MockInterface}) so
destructuring and static analysis are correct; apply the same correction to the
other identical PHPDoc occurrence (the second buildMocks annotation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d750a232-4142-4f82-b785-2fcb08e10a67
📒 Files selected for processing (2)
app/Services/Utils/DoctrineTransactionService.phptests/Unit/Services/DoctrineTransactionServiceTest.php
…ntityManager on connection error
…d masked errors Follow-up to the root/nested transaction split: closes review findings 1-3 plus xhigh code-review fixes on the accumulated diff. - Root non-retryable failures now discard the UnitOfWork (em->clear()) so a failed callback's pending persists/changesets can't leak into the next transaction on the same EntityManager (phantom writes in catch-and-continue loops, e.g. CSV import per-row transactions). - Nested transactions warn once when savepoints are disabled on the connection (outer transaction started outside this service), instead of failing later as an opaque rollback-only ConnectionException. - Root transactions fail fast with a clear error when the callback swallowed a nested flush failure (EntityManager closed mid-transaction) instead of dying with an opaque EntityManagerClosed on the root's own flush; docblock narrowed to state the pattern is only safe for errors thrown before the nested flush. - Rollback failures (root and nested) are now guarded so they can never mask the callback's original exception via Log::warning + finally. - \Error throwables (not just \Exception) now reach the closed-EM recovery branch in the outer catch. Adds 8 unit tests to the existing DoctrineTransactionServiceTest class (22 total) covering each of the above; re-validated against a local MySQL instance (real savepoints, real UniqueConstraintViolationException on nested flush, real registry recovery) in addition to the mocked suite.
DoctrineTransactionService no longer enables setNestTransactionsWithSavepoints. Nested transactions are now pure DBAL nesting-counter bookkeeping; a nested rollBack() marks the shared connection rollback-only (no SQL), and commit() at any level - nested, root, or ORM's own internal per-flush() commit - fails immediately once that flag is set. A nested failure can therefore never be silently absorbed into a successful root commit, even if an intermediate callback catches it and continues. Also: - transaction() now checks $em->isOpen() (not just isTransactionActive()) when deciding root vs nested routing, closing a narrow double-fault gap where a closed EntityManager could be routed into runNestedTransaction() with no reset path. - Extracted the duplicated post-callback closed-EM guard and rollback/log block (root vs nested) into shared private helpers. - Added functional tests against real MySQL (SummitOrderServiceTest) covering addTickets/createOfflineOrder's real nested-transaction chains: happy path commits across nested + sequential root transactions, and a deep failure (invalid promo code / missing default badge type) rolls back the entire chain, including ticket-type quantity_sold. Verified: DoctrineTransactionServiceTest 25/25, SummitOrderServiceTest 23/23, full tests/Unit 249/250 (1 pre-existing unrelated failure).
…mmitService, SpeakerService, PresentationService, SummitPromoCodeService Extends the isRollbackOnly-based nested-transaction rollback guarantee (docs/plans/2026-07-10-tx-post-flush-poison-guard.md) to the remaining outer/inner method pairs identified by investigation: - SummitOrderService::addTickets -> createTicketsForOrder (missing-default-badge-type rollback, exact pair requested) - SummitOrderService::requestRefundOrder -> requestRefundTicket (rolls back entire refund loop when one ticket is free) - SummitService::processRegistrationCompaniesData -> addCompany (per-row isolation - the opposite of full-abort, since this method catches each row's transaction failure locally) - SpeakerService::addSpeakerBySummit -> registerSummitPromoCodeByValue (rolls back the just-created speaker when a registration code collides with another speaker) - PresentationService::submitPresentation / updatePresentationSubmission -> saveOrUpdatePresentation (rolls back on invalid track) - SummitPromoCodeService::addPromoCode -> addPromoCodeTicketTypeRule (partial-commit: promo code survives even though the rules loop rolls back - two separate root transactions, not one) New dedicated test files: SummitServiceTest.php, PresentationServiceTest.php, SummitPromoCodeServiceTest.php, SpeakerServiceRegistrationTest.php (kept separate from the pre-existing SpeakerServiceTest.php, whose 3 tests depend on non-ephemeral externally-seeded summit ids that InsertSummitTestData's unscoped DELETE FROM Summit would otherwise destroy). SummitOrderServiceTest.php also gained disableDefaultBadgeType()/ restoreDefaultBadgeType() helpers to de-duplicate a 3x-repeated fixture block, per xhigh code-review workflow findings applied during spec-verify.
…creation Adds business-exception coverage for the API entry points into the nested-transaction rollback chain (docs/plans/2026-07-10-tx-post-flush-poison-guard.md), complementing the existing happy-path-only tests: - OAuth2AttendeesApiTest::testAddAttendeeTicketFailsOnInvalidPromoCode (addAttendeeTicket -> createOfflineOrder -> createTicketsForOrder, invalid promo code rolls back the whole chain, ticket count unchanged) - OAuth2SummitOrdersApiTest: un-skips testCreateSingleTicketOrder (fixed a 'summit_id' -> 'id' route param bug and a missing ticket_qty) and repurposes testCreateSingleTicketOrderNotComplete into testCreateSingleTicketOrderFailsOnInvalidPromoCode (order creation rolls back entirely on an invalid promo code, order count unchanged) Both previously-skipped tests had a stale skip reason that no longer matched current code behavior.
Documents the 3-iteration decision history behind DoctrineTransactionService's current no-savepoints design: the initial DBAL-savepoints approach, why it was discarded (Doctrine's UnitOfWork has no concept of savepoints, so a ROLLBACK TO SAVEPOINT at the DB level desyncs silently from the in-memory identity map/changesets), and the final native isRollbackOnly-propagation design. Lists every service/method pair covered by the resulting test suite, plus 11 additional outer/inner pairs found in a follow-up audit that are not yet covered, as future work.
Implements the 8 testable outer/inner nested-transaction pairs listed as Known Gaps in adr/003-nested-transaction-rollback-safety.md, each proving DoctrineTransactionService's isRollbackOnly-based rollback contract by showing an inner nested transaction's already-committed write gets undone by a later failure in the same outer call: - SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan -> updateExtraQuestion (new SelectionPlanOrderExtraQuestionTypeServiceTest.php) - SpeakerService::updateSpeakerBySummit -> registerSummitPromoCodeByValue (tests/SpeakerServiceRegistrationTest.php) - SponsorUserSyncService::addSponsorUserToGroup -> SummitSponsorService::addSponsorUser (tests/Unit/Services/SponsorUserPermissionTrackingTest.php) - SummitScheduleSettingsService::seedDefaults -> add (new SummitScheduleSettingsServiceTest.php) - SummitSelectedPresentationListService::assignPresentationToMyIndividualList -> createIndividualSelectionList (new SummitSelectedPresentationListServiceTest.php) - SummitService::unPublishEvents -> unPublishEvent and updateAndPublishEvents -> updateEvent (tests/SummitServiceTest.php) 3 of the original 11 ADR-listed pairs were excluded after verifying against real code that no committed-then-rolled-back proof is reachable through those specific call sites (documented in the plan's Out of Scope section): TagService::addTag's duplicate check (outer's pre-check already uses the same normalized comparison), SummitRSVPInvitationService's rsvpEvent call (no write before or reachable after the nested call), and SpeakerService's member_id-collision trigger (redundant with the registration_code case already covering the same production class). Post-review fix: corrected a mechanism misattribution in the updateAndPublishEvents test (the exception actually comes from updateEvent's unconditional location check, not publishEvent's gated one, since they share the same payload and updateEvent runs first) plus 3 test-duplication cleanups.
Moves the 8 newly-covered outer/inner pairs from docs/plans/2026-07-10-remaining-nested-tx-coverage.md into the Test Coverage Added table, including the mechanism nuance found during implementation (updateAndPublishEvents' rollback actually fires via updateEvent's unconditional location check, not publishEvent's gated one, since both run against the same payload and updateEvent executes first). Known Gaps / Future Work now lists only the 3 pairs confirmed structurally unreachable for a genuine committed-then-rolled-back test (TagService's duplicate check has no exploitable gap vs its caller's identical pre-check; SummitRSVPInvitationService's rsvpEvent call has no write before or reachable after the nested failure), plus notes on why two other candidates were dropped as redundant or non-distinguishing rather than untested gaps. The codebase-wide sweep for this transaction shape is now complete.
docs/plans/ is gitignored workflow state, not part of the committed repo - referencing those paths from a tracked ADR pointed readers at files that don't exist for them.
These test classes live directly under tests/ (not under any of the existing directory-based buckets: tests/oauth2/, tests/Unit/Entities/, tests/Unit/Audit/, tests/Repositories/, tests/Unit/Services/), so CI was silently skipping them - they only ever ran locally. Adds one matrix entry per class, matching the existing SummitOrderServiceTest/ SummitRSVPServiceTest pattern: - SummitServiceTest - SpeakerServiceRegistrationTest - PresentationServiceTest - SummitPromoCodeServiceTest - SummitScheduleSettingsServiceTest - SummitSelectedPresentationListServiceTest - SelectionPlanOrderExtraQuestionTypeServiceTest Verified each --filter matches exactly its intended class with no overlap with existing filters (--list-tests against the local Docker instance).
Adds a "Baseline (origin/main)" section before Iteration 1, verified directly against origin/main (988a6d3): main never used DBAL savepoints and had no root/nested distinction at all - every transaction() call, nested or not, ran an identical retry loop that unconditionally closed the connection and EntityManager and reset the registry on ANY exception, not just connection errors. Savepoints were introduced (and later discarded) entirely within this branch's own Iteration 1, not present on main. Prevents a reader from assuming main already had some form of scoped/partial nested-rollback handling.
smarcet
force-pushed
the
hotfix/doctrine-tx-manager
branch
from
e3a4b46 to
da8b221
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Services/SponsorUserPermissionTrackingTest.php (1)
162-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the rollback assertion by also verifying the row itself was removed.
getPermissions()returns[]both when theSponsor_Usersrow doesn't exist (correct rollback) and when the row exists with a NULL/empty Permissions column (incomplete rollback where the INSERT survived but the UPDATE didn't). The test already hasassertNoSponsorUsersRowExists()— calling it here would verify the INSERT was rolled back, not just the permission write.♻️ Proposed fix
self::$em->clear(); $this->assertEmpty($this->getPermissions($sponsor_id, $member_id)); + $this->assertNoSponsorUsersRowExists($sponsor_id, $member_id); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php` around lines 162 - 164, Strengthen the rollback test after self::$em->clear() by calling the existing assertNoSponsorUsersRowExists() helper with the same sponsor and member identifiers, in addition to assertEmpty(getPermissions(...)), so it verifies the Sponsor_Users INSERT itself was rolled back.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 162-164: Strengthen the rollback test after self::$em->clear() by
calling the existing assertNoSponsorUsersRowExists() helper with the same
sponsor and member identifiers, in addition to assertEmpty(getPermissions(...)),
so it verifies the Sponsor_Users INSERT itself was rolled back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c711c10-dd5a-41af-bc97-3ac92a40db37
📒 Files selected for processing (15)
.github/workflows/push.ymladr/003-nested-transaction-rollback-safety.mdapp/Services/Utils/DoctrineTransactionService.phptests/PresentationServiceTest.phptests/SelectionPlanOrderExtraQuestionTypeServiceTest.phptests/SpeakerServiceRegistrationTest.phptests/SummitOrderServiceTest.phptests/SummitPromoCodeServiceTest.phptests/SummitScheduleSettingsServiceTest.phptests/SummitSelectedPresentationListServiceTest.phptests/SummitServiceTest.phptests/Unit/Services/DoctrineTransactionServiceTest.phptests/Unit/Services/SponsorUserPermissionTrackingTest.phptests/oauth2/OAuth2AttendeesApiTest.phptests/oauth2/OAuth2SummitOrdersApiTest.php
…-entry Closes the three findings from the PR #533 deep review: - Root transactions no longer retry once the real COMMIT has been attempted: a connection failure during COMMIT is ambiguous (the server may have already made the transaction durable and only the ack was lost), so re-executing the callback could duplicate every write and side effect. Commit-phase failures now propagate as "operation state unknown". - transaction() now refuses to run when the EntityManager is closed while its connection still holds an active transaction: resetting onto a brand-new EM/connection would produce durable commits that survive the outer rollback (split-brain partial commit escaping the isRollbackOnly guarantee). - failFastIfEntityManagerClosed()'s message no longer repeats the retracted "safe before any flush" carve-out; catching a nested transaction() failure and continuing is never safe. Each fix landed with a unit test that reproduced the failure first (callback executed 10x on ambiguous commit; resetManager reached on closed-EM re-entry). The mis-modeled nested fail-fast test now genuinely exercises the nested path. ADR-003 documents the hardening.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
Closes the Codex companion review P2: safeRollback() swallows rollback failures by design (the original exception must never be masked or re-classified as retryable), but cleanup afterwards looked only at the original exception and em->isOpen(). A business exception followed by a rollback failure (connection died mid-callback) left an OPEN EM wired to a dead physical handle registered - and DBAL zeroes the nesting level before the physical rollback while clearing isRollbackOnly only after it succeeds, so the flag could be left stuck too. transaction() calls self-heal via the reconnect path, but direct Registry consumers (repositories, serializers, queue jobs reading outside transaction()) have no retry path and would fail in a chain on a long-lived worker. - safeRollback() now reports success/failure. - runRootTransaction() discards the broken pair on rollback failure (close EM, close connection, reset a fresh manager - best-effort, never masking the original exception, never retrying). - Same hygiene for connection-level commit-phase failures, which also left the dead handle registered. - Root-only by construction: with savepoints off a nested rollBack() executes no SQL, so it cannot fail on a dead connection; that case surfaces as the root's own rollback failing, which this covers. TDD: testRootTransactionDiscardsManagerWhenRollbackFails (new) and testRootTransactionDoesNotRetryWhenCommitFails (extended) reproduced the missing cleanup first. ADR-003 Post-Review Hardening updated (item 4).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
…TransactionService Behavior-preserving cleanup of the conditions duplicated across the root failure branches (all 27 unit tests stay green untouched): - transaction() asks isTransactionActive() once and branches nested/refusal/root from a single decision tree. - New restoreRegistryAfterFailure() helper replaces the "if (rollbackFailed) discard; elseif (!isOpen) reset" ladder that was copied in three catch branches (commitStarted, non-retryable, Throwable). - The reconnect path reuses discardBrokenManager() instead of an inline copy of the same clear/close/reset triple. Deliberate micro-delta: its resetManager call is now swallowed like the rest of the cleanup; a broken registry surfaces via getManager on the next iteration instead of masking the retryable error. - shouldReconnect() collapses four consecutive instanceof ifs into one condition (the PDOException switch keeps its own logging). Net -14 lines. The failFast+flush+commit sequence shared by root/nested stays duplicated on purpose: extracting it would hide the commit-phase boundary the ambiguous-commit guard depends on.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adr/003-nested-transaction-rollback-safety.md (1)
266-272: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAvoid describing nested work as committed
With savepoints disabled, inner work is only written/flushed inside the outer transaction; reserve “committed” for the separate root transactions above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adr/003-nested-transaction-rollback-safety.md` around lines 266 - 272, Update the ADR table entries for nested service calls, including SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan, SpeakerService::updateSpeakerBySummit, and the other listed nested operations, to replace “already-committed” or equivalent wording with language stating that inner changes were written or flushed within the outer transaction. Keep “committed” only for the separate root transaction cases, without changing the rollback behavior or implementation nuance.
🧹 Nitpick comments (1)
adr/003-nested-transaction-rollback-safety.md (1)
214-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the “never safe” claim to the active root transaction.
As written, this conflicts with the per-row isolation described at Line 261. Clarify that catching and continuing is unsafe while the same root transaction remains active; catching after that root transaction has unwound can safely proceed to the next row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adr/003-nested-transaction-rollback-safety.md` around lines 214 - 218, Update the fail-fast guidance in the ADR to scope the “never safe” claim to cases where the same root transaction remains active: catching a nested transaction failure and continuing is unsafe in that context, but handling the failure after the root transaction has unwound may safely proceed with the next row, consistent with the per-row isolation described near the referenced section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@adr/003-nested-transaction-rollback-safety.md`:
- Around line 266-272: Update the ADR table entries for nested service calls,
including
SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan,
SpeakerService::updateSpeakerBySummit, and the other listed nested operations,
to replace “already-committed” or equivalent wording with language stating that
inner changes were written or flushed within the outer transaction. Keep
“committed” only for the separate root transaction cases, without changing the
rollback behavior or implementation nuance.
---
Nitpick comments:
In `@adr/003-nested-transaction-rollback-safety.md`:
- Around line 214-218: Update the fail-fast guidance in the ADR to scope the
“never safe” claim to cases where the same root transaction remains active:
catching a nested transaction failure and continuing is unsafe in that context,
but handling the failure after the root transaction has unwound may safely
proceed with the next row, consistent with the per-row isolation described near
the referenced section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3478135d-c515-4287-b674-8f5fdf55752e
📒 Files selected for processing (3)
adr/003-nested-transaction-rollback-safety.mdapp/Services/Utils/DoctrineTransactionService.phptests/Unit/Services/DoctrineTransactionServiceTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/Utils/DoctrineTransactionService.php
…itException The commit-phase guard stops the in-service retry loop, but the raw driver exception (e.g. ConnectionLost) still looks retryable to the layers above - Laravel queue tries and caller-side retries would re-execute the whole callback, duplicating every write the server may have already made durable. Wrap commit-phase failures in a dedicated AmbiguousCommitException (driver exception preserved as previous) so queue jobs can catch it and fail() without retry. Extends plain RuntimeException so shouldReconnect() can never re-classify it as retryable. Covered by the extended testRootTransactionDoesNotRetryWhenCommitFails (asserts marker type, previous chain, and non-retryable classification). ADR-003 Post-Review Hardening item 1 amended accordingly.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
processTicketData() had no per-row try/catch: a business exception on any row (e.g. ValidationException from SummitTicketType::sell()) aborted the whole file, leaving every remaining row unprocessed. This violated the importer's log-and-skip posture already used by the sibling SummitService::processRegistrationCompaniesData(). Wrap each row's transaction in try/catch(Exception), logging and moving to the next row instead of propagating. That alone isn't sufficient: $summit was fetched once before the loop and reused via closure capture across all rows. DoctrineTransactionService clears (or, pre-hardening, closes+discards) the EntityManager's state on a failed root transaction, so a prior row's failure left $summit stale for every subsequent row. Each row's transaction now re-fetches $summit by id instead of reusing the pre-loop reference. Replaces testProcessTicketDataStopsProcessingRemainingRowsOnNestedTransactionFailure (pinned the abort-on-first-error behavior) with testProcessTicketDataSkipsFailingRowsAndContinuesProcessingRemainingRows, which proves a row failing for a row-specific reason does not block a later valid row from being committed. Also fixes a stale comment in SummitServiceTest.php that claimed processTicketData had no per-row catch.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/SummitOrderServiceTest.php (1)
427-443: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
flush()indisableDefaultBadgeType()to persist theisDefault = falsechange to the database.Without the flush, the DB still has
isDefault = trueand the service only sees the in-memory change if it reads from the identity map (getById). If the service ever switches togetByIdRefreshedorHINT_REFRESH, the test would fail at$this->fail(). Additionally,restoreDefaultBadgeType()becomes a silent no-op — it re-fetches from the DB whereisDefaultis stilltrue, sets it totrueagain, and flushes nothing — which contradicts the comment's claim that the flush "actually persists the flag."This affects both
testCreateOfflineOrderRollsBackEntireChainOnMissingDefaultBadgeType(line 1062) andtestAddTicketsRollsBackEntireChainOnMissingDefaultBadgeType(line 1147).🔧 Proposed fix
private function disableDefaultBadgeType(): int { $badge_type_id = self::$default_badge_type->getId(); self::$default_badge_type->setIsDefault(false); + self::$em->flush(); return $badge_type_id; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/SummitOrderServiceTest.php` around lines 427 - 443, Persist the disabled default badge state in disableDefaultBadgeType() by flushing self::$em after setIsDefault(false). Keep restoreDefaultBadgeType() restoring the entity from a fresh lookup and flushing it afterward so both rollback tests observe the database state correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Services/Model/Imp/SummitOrderService.php`:
- Around line 4673-4675: Handle AmbiguousCommitException separately from the
generic Exception catch in the surrounding import flow: re-throw it or log it as
a reconciliation failure and prevent the file from being deleted or marked
successfully processed. Keep the generic warning behavior for other exceptions,
using the relevant import method and cleanup logic near this catch.
---
Outside diff comments:
In `@tests/SummitOrderServiceTest.php`:
- Around line 427-443: Persist the disabled default badge state in
disableDefaultBadgeType() by flushing self::$em after setIsDefault(false). Keep
restoreDefaultBadgeType() restoring the entity from a fresh lookup and flushing
it afterward so both rollback tests observe the database state correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af6499d3-77ff-49be-a8e7-3fbc704f7afb
📒 Files selected for processing (7)
adr/003-nested-transaction-rollback-safety.mdapp/Services/Model/Imp/SummitOrderService.phpapp/Services/Utils/AmbiguousCommitException.phpapp/Services/Utils/DoctrineTransactionService.phptests/SummitOrderServiceTest.phptests/SummitServiceTest.phptests/Unit/Services/DoctrineTransactionServiceTest.php
✅ Files skipped from review due to trivial changes (1)
- adr/003-nested-transaction-rollback-safety.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/SummitServiceTest.php
- tests/Unit/Services/DoctrineTransactionServiceTest.php
…biguous
runRootTransaction() set the commit-phase flag before calling
$conn->commit(), but DBAL 3 throws
ConnectionException::commitFailedRollbackOnly() client-side - before the
COMMIT is ever sent to the server - so a deterministic, fully-rolled-back
failure surfaced as AmbiguousCommitException ("may or may not be durable -
reconcile, do not blind-retry"), sending operators on false reconciliation
work and contradicting ADR-003's own documented failure surface.
Reachable when a nested transaction rolled back (marking the connection
rollback-only), an intermediate callback caught the failure and continued,
and the root flush() had an empty changeset: UnitOfWork::commit()'s
"Nothing to do" early return never touches the connection, so the root's
own commit() is the first commit call in the whole chain. With a non-empty
changeset the flush itself already fails first (the ADR-documented
OptimisticLockException path), which is why the existing real-DB tests
never hit this corner.
runRootTransaction() now checks $conn->isRollbackOnly() right before
entering the commit phase and fails fast with a plain RuntimeException
naming the real cause (a nested failure caught mid-chain), mirroring the
other fail-fast guards; shouldReconnect() never matches it, so it can
never enter the retry loop. AmbiguousCommitException is now reserved for
a COMMIT that was actually sent to the server.
Covered by
testRootTransactionFailsDeterministicallyWhenConnectionIsRollbackOnly,
which reproduced the misclassification first; existing connection mocks
gain an isRollbackOnly() -> false default stub.
ADR-003 updated: Post-Review Hardening item 5, coverage table, and a
Known Gaps entry documenting the pre-existing broken race recovery in
RegistrationIngestionService::ingestExternalAttendee (recommended
follow-up: retry once from the ingest loop instead of catching around
the nested transaction() call).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
The in-service half of the ambiguous-commit protection is delivered (no retry once the real COMMIT has been attempted), but the caller-side half the exception's contract directs - queue jobs catching it and failing without retry - is not wired anywhere: no job or service in app/ catches it, Laravel's queue retries on any uncaught exception while tries remain (21 jobs declare tries of 2-5; ~35 top-level jobs inherit the worker default), and the one catch that does see it today (processTicketData's per-row log-and-skip) swallows it as a generic warning and still deletes the import file. Recorded in ADR-003 Known Gaps with the recommended follow-up: catch and fail() without retry in payment/order-critical jobs (error-level reconciliation event), and in log-and-skip loops handle it separately from the generic catch - record the row as unknown outcome and preserve the source artifact for reconciliation.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
…unknown Closes the one open review thread on PR #533 plus the two non-blocking review notes, in one pass: - processTicketData(): AmbiguousCommitException is now handled separately from the generic per-row catch. An unknown-outcome row (the commit may or may not be durable) is recorded at error level and the source file is NOT deleted, so the row can be reconciled against the DB instead of being treated as cleanly processed. Remaining rows still run - they are independent. Covered by testProcessTicketDataKeepsFileAndContinuesWhenRowCommitOutcomeUnknown, which reproduced the file deletion first. - SponsorUserPermissionTrackingTest: the rollback test now also asserts the Sponsor_Users row itself is gone - getPermissions() returns [] both when the row is absent and when it survived with an empty Permissions column, so the previous assertion alone could not tell a rolled-back INSERT from a half-rolled-back one. - ADR-003 wording: reserve "committed" for separate root transactions - nested work is written/flushed inside the still-open outer transaction, not committed. Also refreshed the processTicketData coverage row, which still described the pre-log-and-skip abort-on-first-failure behavior.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
processEventData() captured $summit once before the row loop and reused it across rows. A failing row's root-transaction cleanup clears the EntityManager (on origin/main's transaction service it closed the EM and connection outright), leaving that captured reference detached/orphaned - so every row AFTER the first failure died in the per-row log-and-skip catch, regardless of validity: the remainder of the file was silently discarded, the import "succeeded", and the file was deleted. Verified pre-existing on origin/main via A/B (main's DoctrineTransactionService + the unfixed method fails the new mixed-volume test identically), so this is a call-site bug, not a regression of the new transaction manager. Same defect and same fix as processTicketData (ed3e46c): re-fetch the summit by id inside each row's transaction. Reproduced first by testProcessEventDataImportsOnlyValidRowsWhenMostRowsFail (15 failing rows interleaved with 5 valid ones - the 5 valid rows were lost); testProcessEventDataImportsAllRowsWhenEveryRowIsValid pins the 20-row happy path and file deletion.
Adds the two volume scenarios (a 20-row all-valid file, and a 20-row file with 15 non-importable rows interleaved with 5 valid ones) to every CSV-processing service method that lacked coverage: - SummitOrderService::processTicketData - all-valid and mixed (sold-out ticket type rows roll back fully per row; the file is deleted in both cases since known failures need no reconciliation) - SummitPromoCodeService::importPromoCodes - mixed via invalid class_name (addPromoCode throws, the per-row catch logs and skips) - SummitPromoCodeService::importSponsorPromoCodes - mixed via a class_name outside the sponsor allow-list (the import's own `continue` guard). NOTE pinned in the test: an empty sponsor_id is NOT rejected - the service creates a SponsorSummitRegistrationPromoCode with sponsor = null (pre-existing gap, flagged for follow-up) - SummitRegistrationInvitationService::importInvitationData - mixed via a nonexistent allowed ticket type id; valid rows use a dedicated "With Invitation"-audience ticket type (any other audience is rejected by SummitRegistrationInvitation::addTicketType) - SummitSubmissionInvitationService::importInvitationData - repeated emails take the update path (last row wins), pinned as upsert semantics rather than failures - SummitSelectionPlanService::processAllowedMemberData - empty or already-present emails are skipped by the row guards, not failures; this loop has NO per-row catch, so a real exception would abort the remaining rows and leave the file undeleted The three new test classes are registered in the CI matrix (push.yml).
The reserve/checkout/cancel endpoints are exposed on the public API (routes/public_api.php) with no authenticated member, and the controller explicitly supports the guest path (it requires owner_* payload data when there is no current user) - but the service crashed on every guest reservation: - SagaFactory::build/buildPrePaidSaga/buildRegularSaga typed the owner as non-nullable Member, so reserve(null, ...) died with a TypeError before the saga even started (this was the actual reason four API tests sat skipped with "SagaFactory::build() requires non-null Member"). - ReserveOrderTask dereferenced $this->owner without a guard in three places (hasPaidRegistrationOrderForSummit, the auto-assign attendee data block, and the attendee_owner lookup), even though its constructor takes ?Member and the task already carries null-owner branches. Fix: the three factory signatures accept ?Member, and the three dereferences guard for null - falling back to the payload's owner_* fields for auto-assign attendee data, treating a guest as having no paid orders, and resolving attendee_owner by email lookup. For any authenticated request every changed expression evaluates identically to the previous code (the null branches are unreachable), so live traffic is unaffected. Pre-existing on origin/main (identical signatures) - a call-site bug, not a regression of this branch. Reproduced first by testReserveAsGuestWithoutMemberCreatesOrder (exact TypeError), plus testReserveAsGuestWithMultipleTicketsAutoAssignsFirstTicket for the guest auto-assign fallback. Also adds service-level coverage for the previously untested reserve/checkout/cancel conditions: sold-out ticket type (with saga compensation asserted), mixed currencies, closed registration period, free-order checkout marks the order paid, checkout guards (unknown hash, cancelled order), end-to-end cancel returning the consumed seat to inventory, and cancel with an unknown hash. The seed leaves the summit's registration period closed (dates relative to the future summit), which is why no live reserve-flow test existed - openRegistrationPeriod() opens it per test.
…point Un-skips the four reserve API tests that sat dead behind the "SagaFactory::build() requires non-null Member" note - authenticated calls never had that problem (the same treatment testCreateSingleTicketOrder already received): getAuthHeaders() instead of hand-built headers, the seeded company instead of a hardcoded id 5, and openRegistrationPeriod() because the seed leaves the summit's registration period closed. - testReserveWithoutActivePaymentProfile is renamed to testReserveSucceedsWithoutActivePaymentProfile and its commented-out 412 assertion removed: it always asserted 201 (the default payment gateway strategy provides a fallback), so the old name promised the opposite of what it verified. - testReserveWithActivePaymentProfile now skips conditionally on real Stripe test credentials (TEST_STRIPE_SECRET_KEY), the same pattern as OAuth2PaymentGatewayProfileApiTest - the seeded profile cannot even be activated without a secret key. The Stripe key statics get the sibling file's dummy-value defaults. Adds testCancelReservedOrder - the first test through OAuth2SummitOrdersApiController@cancel: reserve, DELETE by hash, 204, order cancelled. The shared test token in ProtectedApiTestCase gains the DeleteMyRegistrationOrders scope the endpoint requires (it returned 403 insufficient_scope with the previous scope list).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adr/003-nested-transaction-rollback-safety.md (1)
197-209: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftScope the “never retried” guarantee to the transaction service.
runRootTransaction()avoids retrying the callback, but queue workers still retry uncaughtAmbiguousCommitExceptions. Also, “no job or service catches it” conflicts with the generic catch described at Line 379. Clarify this as “no dedicated consumer handling exists” so operators do not assume duplicate side effects are impossible.Also applies to: 372-382
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adr/003-nested-transaction-rollback-safety.md` around lines 197 - 209, Update the ADR wording around runRootTransaction() and the generic catch at lines 372–382 to scope the “never retried” guarantee to the transaction service only. Clarify that queue workers may retry uncaught AmbiguousCommitException instances and that no dedicated consumer handling currently exists, replacing any claim that no job or service catches the exception; retain the guidance to catch and fail jobs without retrying when applicable.
🧹 Nitpick comments (2)
tests/SummitOrderServiceTest.php (1)
439-446: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
openRegistrationPeriod()is duplicated verbatim intests/oauth2/OAuth2SummitOrdersApiTest.php.Consider extracting to a shared test trait (e.g. alongside
InsertSummitTestData) to avoid maintaining two copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/SummitOrderServiceTest.php` around lines 439 - 446, Remove the duplicate openRegistrationPeriod() implementation from the test classes and extract it into a shared test trait near InsertSummitTestData. Import and use that trait in both SummitOrderServiceTest and OAuth2SummitOrdersApiTest, preserving the existing UTC registration window setup and persistence behavior.tests/SummitPromoCodeServiceTest.php (1)
95-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
buildCsvUploadto a shared trait to eliminate duplication.This helper is identical across at least three test files (
SummitPromoCodeServiceTest,SummitRegistrationInvitationServiceTest,SummitSubmissionInvitationServiceTest). Extracting it to a trait (e.g.,CsvUploadTestHelper) keeps any future changes—such as adding temp-file cleanup or adjusting the MIME type—in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/SummitPromoCodeServiceTest.php` around lines 95 - 100, Extract the duplicated buildCsvUpload helper from SummitPromoCodeServiceTest, SummitRegistrationInvitationServiceTest, and SummitSubmissionInvitationServiceTest into a shared CsvUploadTestHelper trait. Import and use the trait in each test class, remove their local helper implementations, and preserve the existing UploadedFile construction behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adr/003-nested-transaction-rollback-safety.md`:
- Line 285: Separate the SummitPromoCodeService::addPromoCode and
addPromoCodeTicketTypeRule case from the nested-transaction rollback coverage,
since it exercises sequential root transactions and partial commits. Move the
row into a distinct partial-commit coverage subsection in the ADR, or narrow the
surrounding subsection’s claim so it excludes this scenario.
- Line 279: Resolve the mismatch between the coverage table and the
implementation described in the ADR: either implement and test preservation of
the source CSV when SummitOrderService::processTicketData encounters an
AmbiguousCommitException, or revise the table entry to mark this behavior as
outstanding. Do not document preservation as supported while the generic catch
still deletes the reconciliation artifact.
In `@app/Services/Model/Imp/SummitOrderService.php`:
- Line 597: Update the owner-matching comparison in the attendee handling logic
to normalize `$this->owner->getEmail()` with `strtolower` before the strict
comparison, matching the already-normalized `$attendee_email`; preserve the
fallback to `member_repository->getByEmail()` when the normalized emails do not
match.
In `@app/Services/Model/Imp/SummitService.php`:
- Around line 2879-2886: Update the event import flow containing the transaction
closure and its post-loop file deletion to handle AmbiguousCommitException
separately from generic row failures. Track rows with unknown commit outcomes,
reconcile them using the established SummitOrderService::processTicketData
pattern, and only delete the source import file after all ambiguous rows are
resolved; preserve existing handling for ordinary exceptions.
---
Outside diff comments:
In `@adr/003-nested-transaction-rollback-safety.md`:
- Around line 197-209: Update the ADR wording around runRootTransaction() and
the generic catch at lines 372–382 to scope the “never retried” guarantee to the
transaction service only. Clarify that queue workers may retry uncaught
AmbiguousCommitException instances and that no dedicated consumer handling
currently exists, replacing any claim that no job or service catches the
exception; retain the guidance to catch and fail jobs without retrying when
applicable.
---
Nitpick comments:
In `@tests/SummitOrderServiceTest.php`:
- Around line 439-446: Remove the duplicate openRegistrationPeriod()
implementation from the test classes and extract it into a shared test trait
near InsertSummitTestData. Import and use that trait in both
SummitOrderServiceTest and OAuth2SummitOrdersApiTest, preserving the existing
UTC registration window setup and persistence behavior.
In `@tests/SummitPromoCodeServiceTest.php`:
- Around line 95-100: Extract the duplicated buildCsvUpload helper from
SummitPromoCodeServiceTest, SummitRegistrationInvitationServiceTest, and
SummitSubmissionInvitationServiceTest into a shared CsvUploadTestHelper trait.
Import and use the trait in each test class, remove their local helper
implementations, and preserve the existing UploadedFile construction behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f51f759a-b2a0-49e1-8250-83bdf9e466cb
📒 Files selected for processing (15)
.github/workflows/push.ymladr/003-nested-transaction-rollback-safety.mdapp/Services/Model/Imp/SummitOrderService.phpapp/Services/Model/Imp/SummitService.phpapp/Services/Utils/DoctrineTransactionService.phptests/ProtectedApiTestCase.phptests/SummitOrderServiceTest.phptests/SummitPromoCodeServiceTest.phptests/SummitRegistrationInvitationServiceTest.phptests/SummitSelectionPlanServiceTest.phptests/SummitServiceTest.phptests/SummitSubmissionInvitationServiceTest.phptests/Unit/Services/DoctrineTransactionServiceTest.phptests/Unit/Services/SponsorUserPermissionTrackingTest.phptests/oauth2/OAuth2SummitOrdersApiTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/ProtectedApiTestCase.php
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/push.yml
- tests/Unit/Services/SponsorUserPermissionTrackingTest.php
- tests/Unit/Services/DoctrineTransactionServiceTest.php
- app/Services/Utils/DoctrineTransactionService.php
| |---|---|---|---| | ||
| | `SummitOrderService::createOfflineOrder` | `createTicketsForOrder` | `tests/SummitOrderServiceTest.php` | Full rollback — invalid promo code | | ||
| | `SummitOrderService::addTickets` | `createTicketsForOrder` | `tests/SummitOrderServiceTest.php` | Full rollback — missing default badge type | | ||
| | `SummitOrderService::processTicketData` | `createOfflineOrder` | `tests/SummitOrderServiceTest.php` | Full rollback per CSV row, with per-row isolation: a failing row is logged and skipped, later rows still commit; a row surfacing `AmbiguousCommitException` is recorded as *unknown outcome* and the source file is kept for reconciliation instead of being deleted | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the contradictory CSV outcome contract.
The coverage table says an unknown row outcome preserves the source file, but the later section says the current generic catch deletes it and dedicated handling is not implemented. Either land and test the preservation behavior, or update Line 279 to mark it as outstanding; deleting the only reconciliation artifact after an ambiguous commit is unsafe.
Also applies to: 380-389
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adr/003-nested-transaction-rollback-safety.md` at line 279, Resolve the
mismatch between the coverage table and the implementation described in the ADR:
either implement and test preservation of the source CSV when
SummitOrderService::processTicketData encounters an AmbiguousCommitException, or
revise the table entry to mark this behavior as outstanding. Do not document
preservation as supported while the generic catch still deletes the
reconciliation artifact.
| | `SpeakerService::addSpeakerBySummit` | `addSpeaker` + `registerSummitPromoCodeByValue` | `tests/SpeakerServiceRegistrationTest.php` | Full rollback — a registration code already claimed by another speaker undoes the just-created speaker too | | ||
| | `PresentationService::submitPresentation` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track | | ||
| | `PresentationService::updatePresentationSubmission` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track, update never partially applies | | ||
| | `SummitPromoCodeService::addPromoCode` | `addPromoCodeTicketTypeRule` | `tests/SummitPromoCodeServiceTest.php` | **Partial commit, not full rollback** — this is actually two separate, sequential root transactions; the promo code from the first survives even when the second (ticket-type-rules) transaction fails | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Separate sequential-root coverage from nested-transaction coverage.
This row explicitly uses two sequential root transactions, so it does not prove the nested rollback contract described by the surrounding section. Move it to a separate partial-commit coverage subsection or narrow the opening claim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adr/003-nested-transaction-rollback-safety.md` at line 285, Separate the
SummitPromoCodeService::addPromoCode and addPromoCodeTicketTypeRule case from
the nested-transaction rollback coverage, since it exercises sequential root
transactions and partial commits. Move the row into a distinct partial-commit
coverage subsection in the ADR, or narrow the surrounding subsection’s claim so
it excludes this scenario.
| } | ||
|
|
||
| $attendee_owner = $this->owner->getEmail() === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email); | ||
| $attendee_owner = !is_null($this->owner) && $this->owner->getEmail() === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Email comparison is case-sensitive while the compared value was lowercased.
$attendee_email was normalized with strtolower(trim(...)) at line 585, but $this->owner->getEmail() is not lowercased before the === check. If the owner's stored email differs only in case, this attendee will incorrectly skip using $this->owner and instead resolve via member_repository->getByEmail(), potentially linking the ticket to a different Member object (or null) than the actual order owner. Line 442 in the same method already normalizes both sides with strtolower for exactly this kind of comparison — mirror that here for consistency.
🐛 Proposed fix
- $attendee_owner = !is_null($this->owner) && $this->owner->getEmail() === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email);
+ $attendee_owner = !is_null($this->owner) && strtolower($this->owner->getEmail()) === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $attendee_owner = !is_null($this->owner) && $this->owner->getEmail() === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email); | |
| $attendee_owner = !is_null($this->owner) && strtolower($this->owner->getEmail()) === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Services/Model/Imp/SummitOrderService.php` at line 597, Update the
owner-matching comparison in the attendee handling logic to normalize
`$this->owner->getEmail()` with `strtolower` before the strict comparison,
matching the already-normalized `$attendee_email`; preserve the fallback to
`member_repository->getByEmail()` when the normalized emails do not match.
Comment on lines
+2879
to
+2886
| $event = $this->tx_service->transaction(function () use ($summit_id, $row, &$new_speakers) { | ||
|
|
||
| // re-fetched per row: a prior row's failed transaction clears the entity | ||
| // manager's unit of work, detaching any entity (including $summit) captured | ||
| // before the loop started - a later row must not operate on a stale reference | ||
| $summit = $this->summit_repository->getById($summit_id); | ||
| if (is_null($summit) || !$summit instanceof Summit) | ||
| throw new EntityNotFoundException(sprintf("summit %s does not exists.", $summit_id)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding code.
git ls-files | rg '^(app/Services/Model/Imp/SummitService\.php|app/Services/Model/Imp/SummitOrderService\.php|adr/003-nested-transaction-rollback-safety\.md|.*DoctrineTransactionService.*)$'
printf '\n--- SummitService outline ---\n'
ast-grep outline app/Services/Model/Imp/SummitService.php --view expanded | sed -n '1,240p'
printf '\n--- SummitOrderService outline ---\n'
ast-grep outline app/Services/Model/Imp/SummitOrderService.php --view expanded | sed -n '1,240p'
printf '\n--- DoctrineTransactionService outline/search ---\n'
ast-grep outline app/Services/Model/Imp/DoctrineTransactionService.php --view expanded 2>/dev/null | sed -n '1,220p' || true
rg -n "AmbiguousCommitException|transaction\(" app/Services/Model/Imp -S
printf '\n--- SummitService relevant lines ---\n'
sed -n '2830,2965p' app/Services/Model/Imp/SummitService.php
printf '\n--- SummitOrderService relevant lines ---\n'
sed -n '1,260p' app/Services/Model/Imp/SummitOrderService.php
printf '\n--- ADR relevant lines ---\n'
sed -n '1,260p' adr/003-nested-transaction-rollback-safety.mdRepository: OpenStackweb/summit-api
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- SummitService: processEventData ---\n'
sed -n '2840,2945p' app/Services/Model/Imp/SummitService.php
printf '\n--- SummitService: surrounding import/delete helpers ---\n'
sed -n '2750,2849p' app/Services/Model/Imp/SummitService.php
printf '\n--- SummitOrderService: processTicketData and AmbiguousCommitException handling ---\n'
sed -n '4310,4715p' app/Services/Model/Imp/SummitOrderService.php
printf '\n--- DoctrineTransactionService ---\n'
sed -n '1,260p' app/Services/Utils/DoctrineTransactionService.php
printf '\n--- ADR: known gaps / ambiguous commit references ---\n'
rg -n "AmbiguousCommitException|Known Gaps|nested transaction|file" adr/003-nested-transaction-rollback-safety.md -n -S
sed -n '1,260p' adr/003-nested-transaction-rollback-safety.mdRepository: OpenStackweb/summit-api
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- SummitService: end of processEventData ---\n'
sed -n '2945,3035p' app/Services/Model/Imp/SummitService.php
printf '\n--- SummitService: imports / exception usage ---\n'
rg -n "AmbiguousCommitException|catch \(Exception|download_strategy->delete|processEventData" app/Services/Model/Imp/SummitService.php -n -S
printf '\n--- ADR: Known Gaps section ---\n'
sed -n '301,395p' adr/003-nested-transaction-rollback-safety.mdRepository: OpenStackweb/summit-api
Length of output: 17403
Keep the event import file on ambiguous commits app/Services/Model/Imp/SummitService.php:3238-3244 — catch (Exception $ex) swallows AmbiguousCommitException as a generic row failure, but the source file is still deleted unconditionally after the loop. Mirror SummitOrderService::processTicketData here: track unknown-outcome rows and skip deletion until they’re reconciled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Services/Model/Imp/SummitService.php` around lines 2879 - 2886, Update
the event import flow containing the transaction closure and its post-loop file
deletion to handle AmbiguousCommitException separately from generic row
failures. Track rows with unknown commit outcomes, reconcile them using the
established SummitOrderService::processTicketData pattern, and only delete the
source import file after all ambiguous rows are resolved; preserve existing
handling for ordinary exceptions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9gvnev
Summary by CodeRabbit