Hotfix/sponsors permissions - #582
Merged
Merged
Conversation
…sync - addSponsorUserToGroup: add member to the global group BEFORE writing permissions/eager-creating the Sponsor_Users row, so Sponsor::addUser's group validation passes for brand-new sponsor users (the group is delivered by this very event). - addSponsorUser: stop swallowing exceptions so the MQ job retry / failed_jobs machinery applies instead of losing membership events. - Tests: red-green covered in SponsorUserPermissionTrackingTest.
A swallowed removal failure silently leaves the user with access they should have lost. Remove the catch-and-log so RemoveSponsorMemberMQJob (tries = 3) retries and records the failure in failed_jobs. Red-green covered by testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist.
When a sponsor-users-api event arrives for a brand-new IDP user whose Member row was never synced (the user has not logged in yet), the sync exhausted its MQ retries against a missing member and the access grant was lost for good. SponsorUserSyncService now resolves members via resolveMember(): local lookup with a fallback to IMemberService::registerExternalUserById, which fetches the user from the IDP and creates the Member row. EntityNotFoundException is now only thrown when the user does not exist at the IDP either. Propagation tests updated accordingly: they now mock the IDP user API returning null (user unknown at the IDP).
|
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:
📝 WalkthroughWalkthroughSponsor synchronization now provisions missing members through the IDP, propagates failures, scopes revocation to summits, and orders group permission updates. Sponsor-service jobs preserve event types across delayed retries and redeliveries. ChangesSponsor synchronization
Sponsor-service message retries
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SponsorServicesMQJob
participant RabbitMQ
participant DelayQueue
participant SponsorServiceHandler
SponsorServicesMQJob->>RabbitMQ: Publish delayed payload with event type
RabbitMQ->>DelayQueue: Route message for delayed retry
DelayQueue->>RabbitMQ: Redeliver message
RabbitMQ->>SponsorServiceHandler: Resolve event type and handle message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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-582/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Hotfix to improve sponsor-permission synchronization reliability by (a) ensuring sponsor-group membership is granted before Sponsor_Users row creation/validation paths run, (b) registering missing Members on-demand from the IDP, and (c) allowing failures to propagate so MQ retry/failed_jobs handling applies.
Changes:
- Add
resolveMember()and injectIMemberServiceso sponsor sync can register missing users from the IDP. - Stop swallowing exceptions in
addSponsorUser/removeSponsorUserto enable MQ retry semantics. - Expand integration tests to cover “no sponsor group yet”, “member missing locally”, and “propagate missing-member failures”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/Services/Model/Imp/SponsorUserSyncService.php | Adds on-demand member resolution/registration and changes error-propagation + group-grant ordering for sponsor permission sync. |
| tests/Unit/Services/SponsorUserPermissionTrackingTest.php | Adds coverage for new edge cases: chicken-and-egg group validation, on-demand member registration, and exception propagation. |
Suppressed comments (1)
app/Services/Model/Imp/SponsorUserSyncService.php:234
- Same concern as addSponsorUserToGroup: resolveMember() may register the member (nested transaction) and dispatch jobs while the outer transaction is still open. Resolve/register before opening the transaction, then load the Member inside the transaction to apply permission/group removals.
$this->tx_service->transaction(function () use ($user_id, $group_slug, $sponsor_id, $summit_id) {
Log::debug(
"SponsorUserSyncService::removeSponsorUserFromGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}");
$member = $this->resolveMember($user_id);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
app/Services/Model/Imp/SponsorUserSyncService.php (1)
182-203: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the group-membership cache before eager sponsor-user creation.
Line 182 caches
falsefor$group_slug. Line 187 adds the group only to the Doctrine collection.Member::add2Group()does not invalidate or updategroupMembershipCache.When line 203 calls
Sponsor::addUser, its group validation can read the cachedfalsevalue. The new-user flow then fails before it creates theSponsor_Usersrow.Update
Member::add2Group()to invalidate or set the cache entry for the added group. This lets the subsequent membership validation observe the new group.🤖 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/SponsorUserSyncService.php` around lines 182 - 203, Update Member::add2Group() to invalidate or set the groupMembershipCache entry for the newly added group slug after adding it to the Doctrine collection. Ensure subsequent group-membership validation during addSponsorUser observes the new membership, including the eager creation path in SponsorUserSyncService.
🤖 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 `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 274-294: Wrap the dynamic-member registration, assertions, and
cleanup in a finally block. In the finally block, retrieve the member using
$external_id and remove and flush it when present, ensuring cleanup runs even if
a later assertion fails.
---
Outside diff comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 182-203: Update Member::add2Group() to invalidate or set the
groupMembershipCache entry for the newly added group slug after adding it to the
Doctrine collection. Ensure subsequent group-membership validation during
addSponsorUser observes the new membership, including the eager creation path in
SponsorUserSyncService.
🪄 Autofix
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 Plus
Run ID: 031f5f18-9b09-4a40-92ac-f3f773ada122
📒 Files selected for processing (2)
app/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
romanetar
reviewed
Aug 10, 2026
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
registerExternalUserById opens its own transaction and dispatches NewMember / MemberDataUpdatedExternally right after it, whose listeners enqueue MemberAssocSummitOrders, UpdateAttendeeInfo and CleanMemberCacheJob. Those pushes are not deferred to commit: JobDispatcher's afterCommit flag only works for transactions Laravel's DatabaseTransactionsManager can see, and DoctrineTransactionService opens directly on the DBAL connection. Registering the member inside addSponsorUserToGroup / removeSponsorUserFromGroup's transaction therefore left the jobs pointing at a member id that a later rollback erased, failing them permanently. Resolve the member before opening the transaction and re-load it by id inside, so an on-demand registration is always committed before the jobs that reference it.
smarcet
force-pushed
the
hotfix/sponsors-permissions
branch
from
August 10, 2026 17:37
58e7583 to
a2e9227
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ 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)
app/Services/Model/Imp/SponsorUserSyncService.php (1)
151-159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSnapshot
$member->getSponsorMemberships()before mutating sponsor user permissions.
$member->getSponsorMemberships()returns a Doctrine collection, andSummitSponsorService::removeSponsorUser()callssummit_sponsor->removeUser($member), which can mutate the same collection during the loop. A removal can skip later memberships, leaving sponsor-level access intact. Copy the memberships before iterating, e.g.[$sponsor_memberships] = $member->getSponsorMemberships()->toArray();, then iterate$sponsor_memberships.🤖 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/SponsorUserSyncService.php` around lines 151 - 159, Snapshot the Doctrine collection returned by member->getSponsorMemberships() into an array before the loop, then iterate that snapshot while calling SummitSponsorService::removeSponsorUser(). Preserve the existing sponsor ID assignment and logging behavior.
🧹 Nitpick comments (2)
tests/Unit/Services/SponsorUserPermissionTrackingTest.php (1)
352-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: assert the queued jobs as well.
The test proves the member row survives the rollback. The regression it documents is that jobs referencing the member id are pushed before commit. An assertion on the queue makes the contract explicit. Add
Queue::fake()before the call, then assert the member-related jobs were pushed and that the member exists.🤖 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 352 - 373, Add queue assertions to the transaction rollback test around addSponsorUserToGroup: call Queue::fake() before invoking the service, then assert the expected member-related jobs were dispatched and reference the surviving member. Keep the existing EntityNotFoundException and member persistence assertions intact.app/Services/Model/Imp/SponsorUserSyncService.php (1)
102-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename or document the write side effect in
validateParams.
validateParamsis public and now registers a member through the IDP when the local row is missing. The name states validation only. A caller can trigger a member creation and job dispatch without expecting it. Rename to something likeresolveParams, or state the side effect in the docblock.🤖 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/SponsorUserSyncService.php` around lines 102 - 111, Rename the public validateParams method and all its call sites to reflect that resolveMember may register missing members and dispatch work, using a name such as resolveParams; alternatively, add a docblock explicitly documenting this write side effect while preserving the existing summit and member resolution 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 `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 241-257: Update removeSponsorUserFromGroup and the shared
removeSponsorUser/validateParams flow to avoid calling resolveMember with
on-demand IDP registration enabled. For removal events, look up the existing
local Member and return without changes when it is absent; alternatively, add
and use an explicit resolver option that disables registration while preserving
registration for add paths.
---
Outside diff comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 151-159: Snapshot the Doctrine collection returned by
member->getSponsorMemberships() into an array before the loop, then iterate that
snapshot while calling SummitSponsorService::removeSponsorUser(). Preserve the
existing sponsor ID assignment and logging behavior.
---
Nitpick comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 102-111: Rename the public validateParams method and all its call
sites to reflect that resolveMember may register missing members and dispatch
work, using a name such as resolveParams; alternatively, add a docblock
explicitly documenting this write side effect while preserving the existing
summit and member resolution behavior.
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 352-373: Add queue assertions to the transaction rollback test
around addSponsorUserToGroup: call Queue::fake() before invoking the service,
then assert the expected member-related jobs were dispatched and reference the
surviving member. Keep the existing EntityNotFoundException and member
persistence assertions intact.
🪄 Autofix
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 Plus
Run ID: 9c7b5a82-efa0-48da-89d2-0f50b342f13f
📒 Files selected for processing (2)
app/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
Member::getSponsorMemberships() is a plain ManyToMany to Sponsor with no summit scoping, so the null-sponsor_id branch (the auth_user_removed_from_summit event, which carries no sponsor_id) also iterated sponsors belonging to OTHER summits. Those do not resolve against the event's summit, so SummitSponsorService::removeSponsorUser threw "Sponsor not found." and aborted the loop, leaving this summit's own memberships un-revoked. Iteration order is not guaranteed, so it could abort on the first pass and revoke nothing. Multi-summit sponsor users are legitimate: addSponsorUser only rejects summits whose dates overlap, so the same member can sponsor across different years. With the surrounding try/catch now removed, that abort no longer fails silently - it exhausts the job's 3 tries and lands in failed_jobs. Filter the loop by summit, and stop shadowing the $sponsor_id parameter with the loop variable so the log line reports the sponsor actually processed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
resolveMember() registers the member from the IDP when it was never synced. That is right for the add paths - it is what this branch set out to fix - but both revocation entry points went through it too, so a removal event for a member that does not exist locally would create a Member row, run a full synchronizeGroups and dispatch NewMember / MemberDataUpdatedExternally (and with them MemberAssocSummitOrders, UpdateAttendeeInfo, CleanMemberCacheJob) only to then revoke nothing: a member that did not exist owns no Sponsor_Users row and no group membership. The IDP-deleted case was worse. sponsor-users-api emits one removal event per access right when a user is deleted, and by then PublishUserDeleted has already removed the local Member, so getUserById returns null, resolveMember throws, and the job burns its 3 tries into a permanently unresolvable failed_jobs entry for an event that had nothing to do. Add findMember() (lookup without registration) and use it in removeSponsorUser and removeSponsorUserFromGroup: an unknown member is now a logged no-op. Extract resolveSummit() so removeSponsorUser keeps validating the summit without going through validateParams. The add paths and validateParams keep resolveMember. Skipping an unknown member does not turn these into swallow-everything handlers - a genuine failure still propagates, covered by testRemoveSponsorUserPropagatesErrorWhenSummitDoesNotExist. testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist asserted the old behaviour and is rewritten as testRemoveSponsorUserIsNoOpWhenMemberWasNeverSynced.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Unit/Services/SponsorUserPermissionTrackingTest.php (1)
302-323: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun dynamic-member cleanup in a
finallyblock.If the service call or an assertion fails after registration, lines 320-322 do not run. The dynamically created
Membercan affect later tests.This repeats a prior review finding that is still present in the current code.
Proposed fix
- $this->getService()->addSponsorUserToGroup( - $external_id, - IGroup::Sponsors, - $sponsor_id, - $summit_id - ); + try { + $this->getService()->addSponsorUserToGroup( + $external_id, + IGroup::Sponsors, + $sponsor_id, + $summit_id + ); - // Member must have been registered on demand from the IDP... - // (clear first: the in-service instance memoizes a pre-grant - // belongsToGroup(false) in its groupMembershipCache) - self::$em->clear(); - $member = self::$member_repository->getByExternalId($external_id); - $this->assertNotNull($member, 'Member should have been registered on demand'); + self::$em->clear(); + $member = self::$member_repository->getByExternalId($external_id); + $this->assertNotNull($member, 'Member should have been registered on demand'); - // ...with the Sponsor_Users row + permission written and the group granted. - $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member->getId())); - $this->assertTrue($member->belongsToGroup(IGroup::Sponsors)); - - // Cleanup: this member is created outside the trait's tearDown scope. - self::$em->remove($member); - self::$em->flush(); + $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member->getId())); + $this->assertTrue($member->belongsToGroup(IGroup::Sponsors)); + } finally { + self::$em = self::reopenEntityManager(); + $leftover = self::$member_repository->getByExternalId($external_id); + if (!is_null($leftover)) { + self::$em->remove($leftover); + self::$em->flush(); + } + }🤖 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 302 - 323, Wrap the dynamic-member registration, assertions, and cleanup in a finally block so the created Member is removed and flushed even when the service call or an assertion fails. Update the test method around addSponsorUserToGroup and the subsequent member lookup, preserving the existing assertions while ensuring cleanup runs only when a member was successfully registered.
🤖 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.
Duplicate comments:
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 302-323: Wrap the dynamic-member registration, assertions, and
cleanup in a finally block so the created Member is removed and flushed even
when the service call or an assertion fails. Update the test method around
addSponsorUserToGroup and the subsequent member lookup, preserving the existing
assertions while ensuring cleanup runs only when a member was successfully
registered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c6039c6-7823-4140-aaff-a32481b504a3
📒 Files selected for processing (2)
app/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/Model/Imp/SponsorUserSyncService.php
Two problems behind the same symptom: AddSponsorMemberMQJob failing and the
sponsor user never getting their Sponsor_Users row.
1. Stale local groups.
Sponsor::addUser rejects a member belonging to none of its AllowedMemberGroups.
sponsor-users-api grants that group at the IDP before publishing the membership
event (_sync_user_groups), so the IDP is already right - it is summit-api's copy
that is stale, and it only refreshes through the IDP's own user-updated event,
which races this one.
resolveMember already covers the member that does not exist locally: registering
it pulls fresh groups. The member that DOES exist was returned untouched and hit
the validation. ensureSponsorGroupMembership() now re-reads it from the IDP in
that case.
Nothing downstream would have repaired the failure: the producers of
auth_user_added_to_sponsor_and_summit (_import_user, _notify_approval) publish no
companion group event, so no eager-create path runs and the access is lost - this
is not just noise in failed_jobs.
2. The retry policy was inert.
Job::maxTries() and Job::backoff() read the job PAYLOAD, not the properties of
the handler class the payload names, so the `public int $tries = 3` on the four
SponsorServices handlers was never seen by the worker. With nothing in the
payload the worker falls back to the command options, and the entry point runs
`doctrine:queue:work sponsor_users_sync_consumer` with no flags - i.e. the
--tries=1 / --backoff=0 defaults. One failure was terminal.
Put maxTries and backoff ('30,120') in the payload so the declared policy
applies. This takes all three handlers from a single attempt to three spaced
ones; they are idempotent (addUser/removeUser early-return, add/removeSponsorPermission
are idempotent by design), so replaying them is safe.
Tests cover the refresh path and assert maxTries()/backoff() - what the worker
actually consults - rather than the shape of the payload array.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing removed the member it provisions only after its last assertion, so any failure above leaked it into the next test's database. That member is created on demand and therefore lives outside the trait's tearDown scope, so nothing else reclaims it. Not hypothetical: this database already carried a member from an earlier run (smarcet+ondemand_sfxvesem@gmail.com) left behind exactly this way. Leaked fixture rows are expensive here - a stray Group with a duplicate Code makes getBySlug return the wrong row and silently breaks an unrelated test. Wrap the body in try/finally and look the member up by external id in the finally rather than reusing $member, since the failure may predate its assignment. Verified by forcing an assertion failure: the leftover count stayed flat instead of growing. No manual Sponsor_Users cleanup is needed - measured before and after, Doctrine already clears the join table rows when the member is removed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
app/Services/Model/Imp/SponsorUserSyncService.php:237
- Log message contains a grammatical typo ("removed from to summit").
"SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from to summit {$summit_id} for sponsor {$sponsor_id}");
app/Services/Model/Imp/SponsorUserSyncService.php:330
- $summit_id is captured into the removeSponsorUserFromGroup transaction closure but never used, which adds noise and can confuse future edits.
$this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) {
tests/Unit/Services/SponsorUserPermissionTrackingTest.php:85
- This test helper creates Mockery mocks but the test class never calls Mockery::close(), which can cause Mockery to report an unclosed container / unmet expectations at the end of the test run. Register a once-per-test callback to close Mockery when the application is destroyed.
$api = \Mockery::mock(\App\Services\Apis\IExternalUserApi::class)
->shouldIgnoreMissing();
$api->shouldReceive('getUserById')->andReturn($user_data);
$this->app->instance(\App\Services\Apis\IExternalUserApi::class, $api);
$this->app->forgetInstance(\App\Services\Model\IMemberService::class);
sponsor-users-api's metamodel reconciler reaps sponsors summit-api stopped returning THROUGH remove_sponsor_show_permissions, so it emits auth_user_removed_from_sponsor_and_summit precisely when the sponsor no longer exists on this side. getSummitSponsorById() then returns null and SummitSponsorService::removeSponsorUser threw "Sponsor not found." on every attempt - burning the job's retries to revoke something already gone and parking a permanently unresolvable entry in failed_jobs. A sponsor that no longer resolves against the event's summit is now nothing-to-revoke (warn + skip), same contract as the never-synced member. A missing SUMMIT still propagates: summit_deleted flows to sponsor-users-api, which stops emitting for it, so an unknown summit remains a genuine anomaly.
…re-sync ensureSponsorGroupMembership used registerExternalUserById, whose synchronizeGroups(allow_removals: true) strips every non-skip-listed local group absent from the IDP payload - a sponsor-membership event could remove e.g. summit-administrators as a side effect (the previous test even baked that stripping in as expected behavior). Fetch the IDP profile and run the already-existing additive mode (synchronizeGroups(..., false)) instead: this event only ever grants access; removals stay owned by the IDP's own user_updated flow (PublishUserUpdated). resolveMember keeps the full registration - there the member is brand new and a complete sync is correct.
The MQ payload's group_slug was granted (or stripped) as-is: the shared broker vhost gives write access to several service users, so a forged or buggy auth_user_added_to_group / auth_user_removed_from_group event could add a member to - or remove one from - an arbitrary group like administrators. Both group entry points now reject any slug outside Sponsor::AllowedMemberGroups with a ValidationException, so a producer bug stays visible in failed_jobs instead of silently mutating memberships. The gate runs before resolveMember so a rejected event can never provision a member from the IDP as a side effect; the rollback-survival test now triggers its in-transaction failure with an unknown sponsor instead of an unknown group slug.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
app/Jobs/SponsorServices/SponsorServicesMQJob.php:66
getEventType()assumesjson_decode($this->getRawBody(), true)always returns an array. If the body is invalid JSON,$bodybecomesnulland$body[self::EventTypeKey]will trigger an “array offset on null” error, potentially breaking handler selection on retries/redeliveries. Default to an empty array when decoding fails.
$body = json_decode($this->getRawBody(), true);
return $body[self::EventTypeKey] ?? $routing_key;
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Jobs/SponsorServices/SponsorServicesMQJob.php`:
- Around line 148-153: Update the retry metadata assignment in
SponsorServicesMQJob’s release flow to always overwrite body[self::EventTypeKey]
with the resolved getEventType() value, rather than retaining a supplied
x_event_type. Preserve redelivery idempotency and add a regression test covering
a first-delivery message whose x_event_type conflicts with the routing-derived
event type.
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 300-303: Update
app/Services/Model/Imp/SponsorUserSyncService.php:300-303 in the group grant
handler to resolve summit_id and verify it owns sponsor_id before provisioning
or writing permissions; apply the same ownership check in
app/Services/Model/Imp/SponsorUserSyncService.php:375-375 before removing
permissions or global group membership. Add cross-summit sponsor grant and
removal coverage in
tests/Unit/Services/SponsorUserPermissionTrackingTest.php:207-239, asserting
neither Sponsor_Users permissions nor global group membership changes.
🪄 Autofix
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 Plus
Run ID: 2e615965-64e7-4d1d-9d65-2182674e35f2
📒 Files selected for processing (6)
app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.phpapp/Jobs/SponsorServices/SponsorServicesMQJob.phpapp/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.phpapp/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Jobs/SponsorServicesMQJobRetryTest.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
Two broker-side failure modes in the release() path, both invisible to the mocked unit tests and caught by the new live-broker integration test: 1. RabbitMQQueue::declareQueue() declares on the broker but does NOT record the name in the declared-names cache (only isQueueExists() populates it), so laterRaw() re-declared the delay queue release() had just created with the library's own dead-letter arguments, and the broker rejected the inequivalent x-dead-letter-exchange with PRECONDITION_FAILED on every single release. 2. Suppressing that re-declare by priming the cache is no fix: the delay queue carries x-expires, so the broker deletes it when idle - a once-per- worker-process declare means every release after an expiry publishes into a deleted queue and the retry is dropped silently. (laterRaw survives this only because it re-declares unconditionally.) release() now declares the delay queue and publishes the retry directly on the channel, bypassing laterRaw(): an unconditional queue_declare per release re-creates the queue when it expired and is a no-op (equivalent args) when it did not. The integration test red-greens both modes against the real broker: publish -> pop -> release(1) -> redelivered with the original event type and attempts=2, and again after sleeping past x-expires. It skips itself when no broker is reachable.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
The group handlers validated group_slug but never that sponsor_id belongs to the event's summit_id: a forged or buggy event carrying another summit's sponsor could write - or remove - that sponsor's Permissions entry, and the removal path could strip the member's global group. The producer derives both ids from the same AccessRight, so a mismatch is never legitimate. Grant path: reject with a ValidationException when the sponsor does not resolve on the event summit, BEFORE resolveMember - a rejected event must not provision a member from the IDP as a side effect, and the failure stays visible in failed_jobs. Removal path: skip (warn) ONLY when the sponsor exists on a DIFFERENT summit. A sponsor deleted entirely must still run the removal: that is what recomputes the remaining permission count and strips the global sponsors group when this was the member's last sponsor - requiring existence would leave residual show-admin access forever (pinned by the new cleanup test). Also in the touched test class: the rollback-survival test now triggers its in-transaction failure via an allowed group slug with no Group row (its previous trigger dies at the new ownership gate), and the force-initialize workaround in the global-group removal test is gone - its ORM-blaming comment misdiagnosed what was actually leaked duplicate Group fixture rows (fixed in the next commit); on a clean database the removal works without it. Originally flagged by CodeRabbit; severity assessed lower (the producer cannot emit a mismatch - the trigger is a forged/buggy publisher on the shared vhost), fix applied for defense in depth.
clearMemberTestData / clearSummitTestData already reopen the entity manager when a failed tx_service transaction closed it, but kept using the repository instances captured at setup - which are bound to the CLOSED manager. Every find() then threw "EntityManager is closed", clearMemberTestData's empty catch swallowed it, and the fixtures leaked into the shared test database. Those leaks are not benign: the local DB had accumulated 7 duplicate Group rows with Code='sponsors' (and ~1200 fixture members). With duplicates, getBySlug() resolves the oldest stale row while the member belongs to the fixture's row, so Member::removeFromGroup's identity-based contains() returns false and group removals become silent no-ops - the failure mode previously misattributed to ORM 3 EXTRA_LAZY collection semantics and worked around with a force-initialize in the removal test. Re-resolve the repositories from the fresh manager after a reset, and log cleanup failures to STDERR instead of swallowing them, so a future leak is visible the day it starts.
… on release release() preserved an x_event_type already present in the producer body. On a first delivery the routing key is authoritative and getEventType() ignores the body - but the preserved value would take over on the RETRY, so a forged or buggy body key could make a retried event run a different handler than its original delivery (e.g. an add retried as a remove). Always write the resolved event type instead: on a redelivery getEventType() already resolves from the body, so the rewrite stays idempotent (covered by the existing second-release test). No new capability for an attacker who can publish to the vhost (they already control the routing key) - this closes the inconsistency, not a privilege path. Originally flagged by CodeRabbit, confirmed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/Unit/Services/SponsorUserPermissionTrackingTest.php (1)
676-715: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
createCrossSummitSponsoranddeleteCrossSummitSponsorhere.Lines 677-682 duplicate
createCrossSummitSponsor, and thefinallyat lines 711-715 deletes the sponsor with raw SQL. The docblock ofdeleteCrossSummitSponsorat lines 260-266 states why raw SQL is wrong here: the managedSponsorentity stays in the unit of work and references aSummitthat teardown removes, so the next flush fails and the fixtures leak into the shared test database.The
self::$em->clear()at line 701 hides that only on the success path. IfremoveSponsorUserthrows, or if the pre-condition assertions fail, the entity is still managed when the raw DELETE runs.♻️ Proposed fix
- // setUp() ends with an em->clear(), so the static fixture entities are - // detached: re-fetch them or Doctrine treats them as new on persist. - $member = self::$member_repository->find(self::$member->getId()); - $summit2 = self::$summit_repository->getById(self::$summit2->getId()); - $company = self::$em->find(\models\main\Company::class, self::$companies[1]->getId()); - - // A second sponsor, belonging to a DIFFERENT summit, with the same member. - $other_sponsor = new \models\summit\Sponsor(); - $other_sponsor->setCompany($company); - $summit2->addSummitSponsor($other_sponsor); - $other_sponsor->addUser($member); - self::$em->persist($other_sponsor); - self::$em->flush(); + // setUp() ends with an em->clear(), so the static fixture entities are + // detached: re-fetch them or Doctrine treats them as new on persist. + $member = self::$member_repository->find(self::$member->getId()); + + // A second sponsor, belonging to a DIFFERENT summit, with the same member. + $other_sponsor = $this->createCrossSummitSponsor(); + $other_sponsor->addUser($member); + self::$em->flush();} finally { - $conn = self::$em->getConnection(); - $conn->executeStatement('DELETE FROM Sponsor_Users WHERE SponsorID = ?', [$other_sponsor_id]); - $conn->executeStatement('DELETE FROM Sponsor WHERE ID = ?', [$other_sponsor_id]); + $this->deleteCrossSummitSponsor($other_sponsor); }🤖 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 676 - 715, Replace the duplicated sponsor setup with the existing createCrossSummitSponsor helper, retaining its returned sponsor for IDs and assertions. Replace the raw SQL cleanup in the finally block with deleteCrossSummitSponsor, and ensure cleanup clears or detaches managed entities before deletion so it runs safely even when removeSponsorUser or precondition assertions fail.tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php (1)
35-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the broker test out of the unit suite or tag it.
This test opens a real AMQP connection, declares broker topology, and waits up to 10 seconds per poll. It sits in
tests/Unit, so a normal unit run becomes slow and dependent on external infrastructure. Add a PHPUnit group so CI can exclude it, or move the file to an integration directory.The
uniqid()hit from static analysis is a false positive. The value only makes fixture names unique.Proposed change
+use PHPUnit\Framework\Attributes\Group; + +#[Group('integration')] final class SponsorServicesMQJobReleaseIntegrationTest extends TestCase🤖 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/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php` around lines 35 - 65, Tag SponsorServicesMQJobReleaseIntegrationTest as an integration or broker-dependent PHPUnit group so standard unit runs can exclude it, while preserving the existing test behavior and unique uniqid()-based fixture names.Source: Linters/SAST tools
tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php (1)
146-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the zero-delay floor and the attempts header.
Two behaviors of
release()have no coverage in this file:
release(0)takes the$ttl <= 0branch and must publish into<queue>.delay.1000. That branch prevents an unroutable publish, so it deserves a pinned test.- The republished message carries
application_headerswithlaravel.attempts. No assertion checks that header, so a regression that drops it would only fail in the integration test, which skips when no broker is reachable.Proposed additional test
public function testReleaseWithoutDelayStillUsesTheDelayQueueAndCarriesTheAttemptHeader(): void { $queue_name = 'sponsor-users-api-summit-api-badge-scans-queue'; $message = new AMQPMessage(json_encode(['user_external_id' => 1])); $message->setDeliveryInfo(1, false, 'sponsor_users', EventTypes::AUTH_USER_ADDED_TO_GROUP); $rabbitmq = Mockery::mock(RabbitMQQueue::class); $channel = Mockery::mock(\PhpAmqpLib\Channel\AMQPChannel::class); $rabbitmq->shouldReceive('getChannel')->andReturn($channel); $channel->shouldReceive('queue_declare')->once(); $published = null; $channel->shouldReceive('basic_publish')->once()->with( Mockery::on(function ($msg) use (&$published) { $published = $msg; return $msg instanceof AMQPMessage; }), '', $queue_name . '.delay.1000' ); $rabbitmq->shouldReceive('ack')->once(); (new SponsorServicesMQJob(app(), $rabbitmq, $message, 'rabbitmq', $queue_name))->release(0); $headers = $published->get('application_headers')->getNativeData(); $this->assertSame(1, $headers['laravel']['attempts']); }🤖 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/Jobs/SponsorServicesMQJobRetryTest.php` around lines 146 - 206, Add a unit test alongside testReleaseDeadLettersBackThroughTheDefaultExchange to call SponsorServicesMQJob::release(0), assert the message is published through the default exchange with the queue’s .delay.1000 routing key, and verify application_headers contains laravel.attempts equal to 1. Configure the existing RabbitMQ/channel mocks to expect queue declaration and acknowledgment.app/Jobs/SponsorServices/SponsorServicesMQJob.php (1)
166-180: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffConsider publisher confirms before the ack.
The code publishes the retry and then acks the original delivery. Without publisher confirms, a broker-side publish failure is not reported, and the ack then drops the only copy of the event. If you want the retry path to be loss-free, enable confirm mode on the channel and ack only after the confirm.
The
uniqid('', true)hit from static analysis is a false positive here. The value is a correlation identifier for tracing, not a security token.🤖 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/Jobs/SponsorServices/SponsorServicesMQJob.php` around lines 166 - 180, Update the retry publish flow in the job method containing basic_publish so the channel uses publisher-confirm mode and waits for the broker confirmation before calling $this->rabbitmq->ack($this). Preserve the existing message payload and correlation identifier, and only acknowledge the original delivery after the publish is confirmed; propagate publish-confirmation failures without acknowledging it.Source: Linters/SAST tools
🤖 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/SponsorUserSyncService.php`:
- Around line 228-229: Remove PII from the debug logs in
SponsorUserSyncService::addSponsorUser and
SponsorUserSyncService::removeSponsorUser by replacing member email
interpolation with member ID interpolation. Apply this change at
app/Services/Model/Imp/SponsorUserSyncService.php lines 228-229 and 263-264,
preserving the existing log context.
In `@tests/InsertMemberTestData.php`:
- Around line 196-201: Update the exception handler around clearMemberTestData()
to rethrow the caught exception after writing the diagnostic to STDERR, ensuring
tearDown() propagates the cleanup failure and PHPUnit marks the test as failed.
- Around line 163-170: Refresh cleanup repositories from the current self::$em
before any find() calls in both clearMemberTestData() and clearSummitTestData().
In tests/InsertMemberTestData.php:163-170, resolve Group and Member
repositories; in tests/InsertSummitTestData.php:1054-1059, resolve Summit,
SummitAdministratorPermissionGroup, and SummitMediaFileType repositories. Ensure
both cleanup methods always use repositories from the current open entity
manager.
---
Nitpick comments:
In `@app/Jobs/SponsorServices/SponsorServicesMQJob.php`:
- Around line 166-180: Update the retry publish flow in the job method
containing basic_publish so the channel uses publisher-confirm mode and waits
for the broker confirmation before calling $this->rabbitmq->ack($this). Preserve
the existing message payload and correlation identifier, and only acknowledge
the original delivery after the publish is confirmed; propagate
publish-confirmation failures without acknowledging it.
In `@tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php`:
- Around line 35-65: Tag SponsorServicesMQJobReleaseIntegrationTest as an
integration or broker-dependent PHPUnit group so standard unit runs can exclude
it, while preserving the existing test behavior and unique uniqid()-based
fixture names.
In `@tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php`:
- Around line 146-206: Add a unit test alongside
testReleaseDeadLettersBackThroughTheDefaultExchange to call
SponsorServicesMQJob::release(0), assert the message is published through the
default exchange with the queue’s .delay.1000 routing key, and verify
application_headers contains laravel.attempts equal to 1. Configure the existing
RabbitMQ/channel mocks to expect queue declaration and acknowledgment.
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 676-715: Replace the duplicated sponsor setup with the existing
createCrossSummitSponsor helper, retaining its returned sponsor for IDs and
assertions. Replace the raw SQL cleanup in the finally block with
deleteCrossSummitSponsor, and ensure cleanup clears or detaches managed entities
before deletion so it runs safely even when removeSponsorUser or precondition
assertions fail.
🪄 Autofix
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 Plus
Run ID: fd922644-b222-41cc-9252-ed830a534a74
📒 Files selected for processing (9)
app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.phpapp/Jobs/SponsorServices/SponsorServicesMQJob.phpapp/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.phpapp/Services/Model/Imp/SponsorUserSyncService.phptests/InsertMemberTestData.phptests/InsertSummitTestData.phptests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.phptests/Unit/Jobs/SponsorServicesMQJobRetryTest.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
The previous hardening re-resolved the repositories only when self::$em was closed. That misses the case where the manager was reset mid-test and a test finally already reopened it: self::$em is then fresh and OPEN, the isOpen() check skips the refresh, and the repositories captured at setup still point at the closed manager - the cleanup can fail and leak fixtures all the same. Re-resolve them unconditionally at cleanup entry in both traits; the isOpen() check remains only to decide whether the manager itself needs a reset. Flagged by CodeRabbit on the previous hardening commit, confirmed.
Logging the failure to STDERR was not enough: a green test with leaked fixtures is still green, and nobody reads stderr in CI. The silent catch is how the shared test database accumulated months of leaked rows (7 duplicate 'sponsors' Group rows, ~1200 fixture members) that turned group removals into silent no-ops and got misdiagnosed as an ORM bug. Rethrow after logging so a cleanup failure fails the test the day it starts happening - clearSummitTestData already propagates, this makes both paths symmetric. Flagged by CodeRabbit, confirmed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
The rethrow in 491a885 did its job on CI: EntityModelUnitTests failed on SummitAttendeeTest::testAddSummitAttendee because its cleanup flush was ALREADY broken - the test builds an unpersisted object graph (ticket, ticket type, badge) hanging off managed fixtures, and clearMemberTestData's flush choked on it with 'non-persisted new entities found through the association graph'. The old empty catch had been swallowing exactly this for who knows how long, leaking the member/group fixtures every run of that test. Clear the entity manager at cleanup entry in both traits, then reload by id: the cleanup must only ever flush its own removals, never whatever the test left pending in the unit of work. Verified locally against the failing CI shard (tests/Unit/Entities/ 40/40), plus tests/Unit/Jobs/, tests/Unit/Services/ and tests/Repositories/ - all green, zero leaked fixture rows after the runs.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
…xture self::$speaker (base InsertMemberTestData fixture, Company="Tipit LLC") and speaker1 (created in insertSummitTestData for the same self::$defaultMember) both attach to the same Member. Member::$speaker is a OneToOne with orphanRemoval=true, and PresentationSpeaker::setMember() bidirectionally reassigns $member->speaker - so attaching speaker1 silently orphan-removes self::$speaker on the next flush. OAuth2SummitSpeakersApiTest's company-count tests only ever passed because leaked fixture rows from other test runs (the same leak fixed below) coincidentally supplied the missing "Tipit LLC" value via the un-scoped getAllCompanies query; once the leak stopped, the tests failed for real (CI: 1 matches expected 2, 0 matches expected 1). Give each of the two company-count tests its own dedicated speaker attached to self::$defaultMember2 (which has no existing speaker profile), so they observe real, independent distinct companies instead of depending on a speaker that gets deleted out from under them. Also close the leak itself: clearMemberTestData/clearSummitTestData never removed self::$speaker / the summit-fixture speaker, leaking one PresentationSpeaker row per test run and poisoning every un-scoped company-count query suite-wide (confirmed locally: 500+ leaked "Tipit LLC" rows, 600+ leaked "Tipit"/"FNTECH" members). Verified locally against the full tests/oauth2/ directory (1069 tests, matching the CI job's scope): both previously-failing tests now pass; 0 failures attributable to this change.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
smarcet
added a commit
that referenced
this pull request
Aug 18, 2026
* fix: grant sponsor group before eager Sponsor_Users creation in user sync
- addSponsorUserToGroup: add member to the global group BEFORE writing
permissions/eager-creating the Sponsor_Users row, so Sponsor::addUser's
group validation passes for brand-new sponsor users (the group is
delivered by this very event).
- addSponsorUser: stop swallowing exceptions so the MQ job retry /
failed_jobs machinery applies instead of losing membership events.
- Tests: red-green covered in SponsorUserPermissionTrackingTest.
* fix: propagate removeSponsorUser failures to MQ job retry machinery
A swallowed removal failure silently leaves the user with access they
should have lost. Remove the catch-and-log so RemoveSponsorMemberMQJob
(tries = 3) retries and records the failure in failed_jobs.
Red-green covered by testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist.
* feat: register member on demand from IDP in sponsor user sync
When a sponsor-users-api event arrives for a brand-new IDP user whose
Member row was never synced (the user has not logged in yet), the sync
exhausted its MQ retries against a missing member and the access grant
was lost for good.
SponsorUserSyncService now resolves members via resolveMember(): local
lookup with a fallback to IMemberService::registerExternalUserById,
which fetches the user from the IDP and creates the Member row.
EntityNotFoundException is now only thrown when the user does not exist
at the IDP either.
Propagation tests updated accordingly: they now mock the IDP user API
returning null (user unknown at the IDP).
* fix: resolve member outside the sponsor group sync transaction
registerExternalUserById opens its own transaction and dispatches
NewMember / MemberDataUpdatedExternally right after it, whose listeners
enqueue MemberAssocSummitOrders, UpdateAttendeeInfo and CleanMemberCacheJob.
Those pushes are not deferred to commit: JobDispatcher's afterCommit flag
only works for transactions Laravel's DatabaseTransactionsManager can see,
and DoctrineTransactionService opens directly on the DBAL connection.
Registering the member inside addSponsorUserToGroup /
removeSponsorUserFromGroup's transaction therefore left the jobs pointing
at a member id that a later rollback erased, failing them permanently.
Resolve the member before opening the transaction and re-load it by id
inside, so an on-demand registration is always committed before the jobs
that reference it.
* fix: scope removeSponsorUser membership loop to the event summit
Member::getSponsorMemberships() is a plain ManyToMany to Sponsor with no
summit scoping, so the null-sponsor_id branch (the auth_user_removed_from_summit
event, which carries no sponsor_id) also iterated sponsors belonging to OTHER
summits. Those do not resolve against the event's summit, so
SummitSponsorService::removeSponsorUser threw "Sponsor not found." and aborted
the loop, leaving this summit's own memberships un-revoked. Iteration order is
not guaranteed, so it could abort on the first pass and revoke nothing.
Multi-summit sponsor users are legitimate: addSponsorUser only rejects summits
whose dates overlap, so the same member can sponsor across different years.
With the surrounding try/catch now removed, that abort no longer fails
silently - it exhausts the job's 3 tries and lands in failed_jobs.
Filter the loop by summit, and stop shadowing the $sponsor_id parameter with
the loop variable so the log line reports the sponsor actually processed.
* fix: do not provision members from the IDP on revocation events
resolveMember() registers the member from the IDP when it was never synced.
That is right for the add paths - it is what this branch set out to fix - but
both revocation entry points went through it too, so a removal event for a
member that does not exist locally would create a Member row, run a full
synchronizeGroups and dispatch NewMember / MemberDataUpdatedExternally (and
with them MemberAssocSummitOrders, UpdateAttendeeInfo, CleanMemberCacheJob)
only to then revoke nothing: a member that did not exist owns no Sponsor_Users
row and no group membership.
The IDP-deleted case was worse. sponsor-users-api emits one removal event per
access right when a user is deleted, and by then PublishUserDeleted has already
removed the local Member, so getUserById returns null, resolveMember throws,
and the job burns its 3 tries into a permanently unresolvable failed_jobs entry
for an event that had nothing to do.
Add findMember() (lookup without registration) and use it in removeSponsorUser
and removeSponsorUserFromGroup: an unknown member is now a logged no-op. Extract
resolveSummit() so removeSponsorUser keeps validating the summit without going
through validateParams. The add paths and validateParams keep resolveMember.
Skipping an unknown member does not turn these into swallow-everything
handlers - a genuine failure still propagates, covered by
testRemoveSponsorUserPropagatesErrorWhenSummitDoesNotExist.
testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist asserted the old
behaviour and is rewritten as testRemoveSponsorUserIsNoOpWhenMemberWasNeverSynced.
* fix: refresh stale member groups from the IDP and make MQ retries real
Two problems behind the same symptom: AddSponsorMemberMQJob failing and the
sponsor user never getting their Sponsor_Users row.
1. Stale local groups.
Sponsor::addUser rejects a member belonging to none of its AllowedMemberGroups.
sponsor-users-api grants that group at the IDP before publishing the membership
event (_sync_user_groups), so the IDP is already right - it is summit-api's copy
that is stale, and it only refreshes through the IDP's own user-updated event,
which races this one.
resolveMember already covers the member that does not exist locally: registering
it pulls fresh groups. The member that DOES exist was returned untouched and hit
the validation. ensureSponsorGroupMembership() now re-reads it from the IDP in
that case.
Nothing downstream would have repaired the failure: the producers of
auth_user_added_to_sponsor_and_summit (_import_user, _notify_approval) publish no
companion group event, so no eager-create path runs and the access is lost - this
is not just noise in failed_jobs.
2. The retry policy was inert.
Job::maxTries() and Job::backoff() read the job PAYLOAD, not the properties of
the handler class the payload names, so the `public int $tries = 3` on the four
SponsorServices handlers was never seen by the worker. With nothing in the
payload the worker falls back to the command options, and the entry point runs
`doctrine:queue:work sponsor_users_sync_consumer` with no flags - i.e. the
--tries=1 / --backoff=0 defaults. One failure was terminal.
Put maxTries and backoff ('30,120') in the payload so the declared policy
applies. This takes all three handlers from a single attempt to three spaced
ones; they are idempotent (addUser/removeUser early-return, add/removeSponsorPermission
are idempotent by design), so replaying them is safe.
Tests cover the refresh path and assert maxTries()/backoff() - what the worker
actually consults - rather than the shape of the payload array.
* test: clean up the on-demand member in a finally
testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing removed the member
it provisions only after its last assertion, so any failure above leaked it into
the next test's database. That member is created on demand and therefore lives
outside the trait's tearDown scope, so nothing else reclaims it.
Not hypothetical: this database already carried a member from an earlier run
(smarcet+ondemand_sfxvesem@gmail.com) left behind exactly this way. Leaked
fixture rows are expensive here - a stray Group with a duplicate Code makes
getBySlug return the wrong row and silently breaks an unrelated test.
Wrap the body in try/finally and look the member up by external id in the
finally rather than reusing $member, since the failure may predate its
assignment. Verified by forcing an assertion failure: the leftover count stayed
flat instead of growing.
No manual Sponsor_Users cleanup is needed - measured before and after, Doctrine
already clears the join table rows when the member is removed.
* fix: correct removeSponsorUser log message
The single-sponsor branch read "removed from to summit" and interpolated the raw
$summit_id parameter while its sibling branch uses $summit->getId(). Fix the
wording and use the resolved summit so both branches emit the same shape, which
matters for grepping and alerting on these lines.
Flagged by Copilot on PR #582; the thread was resolved without the change being
applied.
* fix: make MQ retries actually redeliver by dead-lettering through the default exchange
The delay queue laterRaw() declares dead-letters into the consumer exchange
(sponsor-users-api-message-broker) with the QUEUE NAME as routing key. That
exchange is direct and only binds the five auth_user_* routing keys, so every
released retry was unroutable and silently dropped - worse than the old
tries=1 behavior, which at least parked the failure in queue_failed_jobs.
SponsorServicesMQJob::release() now declares the delay queue first (the
declared-names cache keeps laterRaw from re-declaring it) dead-lettering
through the DEFAULT exchange, which routes by queue name with no binding
required. Since redelivery rewrites the routing key to the queue name, the
original event type is preserved in the republished body (x_event_type) and
getEventType() recovers it - payload() and both handlers that branch on the
event type (RemoveSponsorMemberMQJob would otherwise treat a retried
sponsor-scoped removal as a summit-wide one, UpdateSponsorMemberGroupsMQJob
would match no branch and delete the job) now resolve through it.
* fix: treat removal events for an already-deleted sponsor as a no-op
sponsor-users-api's metamodel reconciler reaps sponsors summit-api stopped
returning THROUGH remove_sponsor_show_permissions, so it emits
auth_user_removed_from_sponsor_and_summit precisely when the sponsor no
longer exists on this side. getSummitSponsorById() then returns null and
SummitSponsorService::removeSponsorUser threw "Sponsor not found." on every
attempt - burning the job's retries to revoke something already gone and
parking a permanently unresolvable entry in failed_jobs.
A sponsor that no longer resolves against the event's summit is now
nothing-to-revoke (warn + skip), same contract as the never-synced member. A
missing SUMMIT still propagates: summit_deleted flows to sponsor-users-api,
which stops emitting for it, so an unknown summit remains a genuine anomaly.
* fix: refresh stale sponsor groups additively instead of via full IDP re-sync
ensureSponsorGroupMembership used registerExternalUserById, whose
synchronizeGroups(allow_removals: true) strips every non-skip-listed local
group absent from the IDP payload - a sponsor-membership event could remove
e.g. summit-administrators as a side effect (the previous test even baked
that stripping in as expected behavior).
Fetch the IDP profile and run the already-existing additive mode
(synchronizeGroups(..., false)) instead: this event only ever grants access;
removals stay owned by the IDP's own user_updated flow (PublishUserUpdated).
resolveMember keeps the full registration - there the member is brand new
and a complete sync is correct.
* fix: restrict sponsor group sync to Sponsor::AllowedMemberGroups
The MQ payload's group_slug was granted (or stripped) as-is: the shared
broker vhost gives write access to several service users, so a forged or
buggy auth_user_added_to_group / auth_user_removed_from_group event could
add a member to - or remove one from - an arbitrary group like
administrators. Both group entry points now reject any slug outside
Sponsor::AllowedMemberGroups with a ValidationException, so a producer bug
stays visible in failed_jobs instead of silently mutating memberships.
The gate runs before resolveMember so a rejected event can never provision
a member from the IDP as a side effect; the rollback-survival test now
triggers its in-transaction failure with an unknown sponsor instead of an
unknown group slug.
* fix: publish retries directly to the delay queue instead of via laterRaw
Two broker-side failure modes in the release() path, both invisible to the
mocked unit tests and caught by the new live-broker integration test:
1. RabbitMQQueue::declareQueue() declares on the broker but does NOT record
the name in the declared-names cache (only isQueueExists() populates it),
so laterRaw() re-declared the delay queue release() had just created with
the library's own dead-letter arguments, and the broker rejected the
inequivalent x-dead-letter-exchange with PRECONDITION_FAILED on every
single release.
2. Suppressing that re-declare by priming the cache is no fix: the delay
queue carries x-expires, so the broker deletes it when idle - a once-per-
worker-process declare means every release after an expiry publishes into
a deleted queue and the retry is dropped silently. (laterRaw survives this
only because it re-declares unconditionally.)
release() now declares the delay queue and publishes the retry directly on
the channel, bypassing laterRaw(): an unconditional queue_declare per release
re-creates the queue when it expired and is a no-op (equivalent args) when it
did not. The integration test red-greens both modes against the real broker:
publish -> pop -> release(1) -> redelivered with the original event type and
attempts=2, and again after sleeping past x-expires. It skips itself when no
broker is reachable.
* fix: validate sponsor/summit ownership on group events
The group handlers validated group_slug but never that sponsor_id belongs to
the event's summit_id: a forged or buggy event carrying another summit's
sponsor could write - or remove - that sponsor's Permissions entry, and the
removal path could strip the member's global group. The producer derives both
ids from the same AccessRight, so a mismatch is never legitimate.
Grant path: reject with a ValidationException when the sponsor does not
resolve on the event summit, BEFORE resolveMember - a rejected event must not
provision a member from the IDP as a side effect, and the failure stays
visible in failed_jobs.
Removal path: skip (warn) ONLY when the sponsor exists on a DIFFERENT summit.
A sponsor deleted entirely must still run the removal: that is what recomputes
the remaining permission count and strips the global sponsors group when this
was the member's last sponsor - requiring existence would leave residual
show-admin access forever (pinned by the new cleanup test).
Also in the touched test class: the rollback-survival test now triggers its
in-transaction failure via an allowed group slug with no Group row (its
previous trigger dies at the new ownership gate), and the force-initialize
workaround in the global-group removal test is gone - its ORM-blaming comment
misdiagnosed what was actually leaked duplicate Group fixture rows (fixed in
the next commit); on a clean database the removal works without it.
Originally flagged by CodeRabbit; severity assessed lower (the producer
cannot emit a mismatch - the trigger is a forged/buggy publisher on the
shared vhost), fix applied for defense in depth.
* fix(tests): stop fixture teardown from leaking rows after an EM reset
clearMemberTestData / clearSummitTestData already reopen the entity manager
when a failed tx_service transaction closed it, but kept using the repository
instances captured at setup - which are bound to the CLOSED manager. Every
find() then threw "EntityManager is closed", clearMemberTestData's empty
catch swallowed it, and the fixtures leaked into the shared test database.
Those leaks are not benign: the local DB had accumulated 7 duplicate Group
rows with Code='sponsors' (and ~1200 fixture members). With duplicates,
getBySlug() resolves the oldest stale row while the member belongs to the
fixture's row, so Member::removeFromGroup's identity-based contains() returns
false and group removals become silent no-ops - the failure mode previously
misattributed to ORM 3 EXTRA_LAZY collection semantics and worked around with
a force-initialize in the removal test.
Re-resolve the repositories from the fresh manager after a reset, and log
cleanup failures to STDERR instead of swallowing them, so a future leak is
visible the day it starts.
* fix: overwrite any smuggled x_event_type with the resolved event type on release
release() preserved an x_event_type already present in the producer body. On
a first delivery the routing key is authoritative and getEventType() ignores
the body - but the preserved value would take over on the RETRY, so a forged
or buggy body key could make a retried event run a different handler than its
original delivery (e.g. an add retried as a remove). Always write the
resolved event type instead: on a redelivery getEventType() already resolves
from the body, so the rewrite stays idempotent (covered by the existing
second-release test).
No new capability for an attacker who can publish to the vhost (they already
control the routing key) - this closes the inconsistency, not a privilege
path. Originally flagged by CodeRabbit, confirmed.
* fix(tests): refresh cleanup repositories unconditionally
The previous hardening re-resolved the repositories only when self::$em was
closed. That misses the case where the manager was reset mid-test and a test
finally already reopened it: self::$em is then fresh and OPEN, the isOpen()
check skips the refresh, and the repositories captured at setup still point
at the closed manager - the cleanup can fail and leak fixtures all the same.
Re-resolve them unconditionally at cleanup entry in both traits; the isOpen()
check remains only to decide whether the manager itself needs a reset.
Flagged by CodeRabbit on the previous hardening commit, confirmed.
* fix(tests): rethrow fixture cleanup failures instead of swallowing them
Logging the failure to STDERR was not enough: a green test with leaked
fixtures is still green, and nobody reads stderr in CI. The silent catch is
how the shared test database accumulated months of leaked rows (7 duplicate
'sponsors' Group rows, ~1200 fixture members) that turned group removals
into silent no-ops and got misdiagnosed as an ORM bug. Rethrow after logging
so a cleanup failure fails the test the day it starts happening -
clearSummitTestData already propagates, this makes both paths symmetric.
Flagged by CodeRabbit, confirmed.
* fix(tests): detach the unit of work before fixture cleanup flushes
The rethrow in 491a885 did its job on CI: EntityModelUnitTests failed on
SummitAttendeeTest::testAddSummitAttendee because its cleanup flush was
ALREADY broken - the test builds an unpersisted object graph (ticket, ticket
type, badge) hanging off managed fixtures, and clearMemberTestData's flush
choked on it with 'non-persisted new entities found through the association
graph'. The old empty catch had been swallowing exactly this for who knows
how long, leaking the member/group fixtures every run of that test.
Clear the entity manager at cleanup entry in both traits, then reload by id:
the cleanup must only ever flush its own removals, never whatever the test
left pending in the unit of work.
Verified locally against the failing CI shard (tests/Unit/Entities/ 40/40),
plus tests/Unit/Jobs/, tests/Unit/Services/ and tests/Repositories/ - all
green, zero leaked fixture rows after the runs.
* fix(tests): stop orphan-removal from silently dropping the company fixture
self::$speaker (base InsertMemberTestData fixture, Company="Tipit LLC") and
speaker1 (created in insertSummitTestData for the same self::$defaultMember)
both attach to the same Member. Member::$speaker is a OneToOne with
orphanRemoval=true, and PresentationSpeaker::setMember() bidirectionally
reassigns $member->speaker - so attaching speaker1 silently orphan-removes
self::$speaker on the next flush. OAuth2SummitSpeakersApiTest's company-count
tests only ever passed because leaked fixture rows from other test runs
(the same leak fixed below) coincidentally supplied the missing "Tipit LLC"
value via the un-scoped getAllCompanies query; once the leak stopped, the
tests failed for real (CI: 1 matches expected 2, 0 matches expected 1).
Give each of the two company-count tests its own dedicated speaker attached
to self::$defaultMember2 (which has no existing speaker profile), so they
observe real, independent distinct companies instead of depending on a
speaker that gets deleted out from under them.
Also close the leak itself: clearMemberTestData/clearSummitTestData never
removed self::$speaker / the summit-fixture speaker, leaking one
PresentationSpeaker row per test run and poisoning every un-scoped
company-count query suite-wide (confirmed locally: 500+ leaked "Tipit LLC"
rows, 600+ leaked "Tipit"/"FNTECH" members).
Verified locally against the full tests/oauth2/ directory (1069 tests,
matching the CI job's scope): both previously-failing tests now pass;
0 failures attributable to this change.
smarcet
added a commit
that referenced
this pull request
Aug 31, 2026
…gates Main's sponsors-permissions hardening (0399459, PR #582) added an assertAllowedSponsorGroup() gate that rejects any slug outside Sponsor::AllowedMemberGroups before the transaction starts, and moved the group lookup ahead of the nested eager-create call. Every failure path in addSponsorUserToGroup now throws before the nested transaction writes anything, so the written-then-rolled-back rollback proof this branch's testAddSponsorUserToGroupRollsBackAlreadyCommittedSponsorUserRowWhenGroupNotFound pinned is structurally unreachable; the test was dropped during the rebase onto that main. Move the pair from the coverage table to the 'no test is possible here' table and fix the 8/3 -> 7/4 counts.
smarcet
added a commit
that referenced
this pull request
Aug 31, 2026
…ntityManager on connection error (#533) * fix(transactions): prevent nested transaction from destroying outer EntityManager on connection error * fix(transactions): harden root/nested split against phantom writes and 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. * fix(tx): remove DBAL savepoints, propagate native isRollbackOnly guard 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). * test: nested-transaction rollback coverage for SummitOrderService, SummitService, 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. * test: HTTP-level exception-branch coverage for order/attendee ticket 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. * docs(adr): nested transaction rollback safety decision record 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. * test: nested-transaction rollback coverage for remaining ADR-003 gaps 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. * docs(adr): update nested-tx rollback ADR with remaining test coverage 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(adr): remove docs/plans references from ADR-003 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. * chore: add nested-tx rollback test classes to push.yml CI matrix 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). * docs(adr): clarify origin/main's actual pre-branch behavior in ADR-003 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. * fix(transactions): never retry ambiguous commits, refuse closed-EM re-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. * fix(transactions): discard broken manager/connection when rollback fails 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). * refactor(transactions): deduplicate failure-cleanup paths in DoctrineTransactionService 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. * fix(transactions): surface ambiguous commit failures as AmbiguousCommitException 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. * fix(registration): log-and-skip failing rows in CSV ticket data import 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. * fix(transactions): never classify rollback-only commit failures as ambiguous 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). * docs(adr): log AmbiguousCommitException consumer wiring as known gap 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. * fix(registration): keep import file when a CSV row commit outcome is 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. * fix(summit): re-fetch summit per row in processEventData CSV import 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. * test(imports): volume coverage for every CSV import service method 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). * fix(registration): support guest buyers on the reserve saga 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. * test(orders): revive reserve API tests and cover the order cancel endpoint 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). * fix(auth): flush email invalidation before reassigning a colliding email ResourceServerContext::syncMemberFields() invalidates the email of a member that already holds the login's claim email, then assigns that email to the resolved member - both as pending UPDATEs flushed together at the transaction commit. Member.Email carries a unique index and the UnitOfWork decides the UPDATE order (identity-map scheduling), so whenever the resolved member's UPDATE ran first it hit the index while the former owner still held the email: 1062 UniqueConstraintViolation and the whole user resolution - the login - died. Intermittent by entity-load order; deterministic in the reproducing test. The invalidation is now flushed immediately inside the still-open transaction (add($entity, true) - the same flush-now idiom the twin guard in MemberService::registerExternalUser already uses, which is why that guard was never affected), freeing the unique index entry before the resolved member's own email UPDATE can be scheduled. Atomicity is preserved: an intermediate flush inside an open transaction commits nothing; a later failure still rolls back both updates. Also fixes Member::belongsToGroup()'s per-instance memoization going stale: add2Group/removeFromGroup/clearGroups/setGroups now reset groupMembershipCache, so a membership check made before a mutation can no longer serve the old answer after it (the deferred-group-sync test asserts this on the same instance). Expands ResourceServerContextTest from 2 to 8 tests: anonymous null caching, lookup by external id, lookup by email plus external-id linking, deferred field sync (including the cache restore after Member setters invalidate it mid-sync), deferred group sync with the real user_groups claim shape ([['slug' => ...]]), and the email-collision guard (reproduced the 1062 first). NOTE: the pre-existing testSync passes user_groups as plain strings, which checkGroups() silently skips - its group claim has never synced anything; left untouched. * chore(ci): run MemberServiceTest in the integration matrix tests/MemberServiceTest.php (registerExternalUser email reassignment and invalidation, additive group sync) was not matched by any matrix filter nor by the unit-tests job (which only runs the OpenTelemetry suites), so it never ran in CI despite passing locally (3 tests, 12 assertions). * chore(ci): wire Stripe test credentials from repository secrets The integration-tests job hardcoded placeholder Stripe keys (sk_test_12345), so any test whose reservation reaches ReserveOrderTask::preProcessOrder() (order amount > 0) died with "Invalid API Key provided" - they only passed locally where the env carries a valid test key. The default registration gateway keys now read the TEST_STRIPE_SECRET_KEY / TEST_STRIPE_PUBLISHABLE_KEY repository secrets, falling back to the previous placeholders when the secrets are absent (fork PRs do not receive secrets, so those runs behave as before). The same secrets are also exposed under the env var names the test classes already read, which lets testReserveWithActivePaymentProfile stop skipping in CI and exercise the paid reservation flow against Stripe test mode. * fix(summit): keep import files when a row commit outcome is unknown SummitService::processEventData() and processRegistrationCompaniesData() had the same gap already closed in SummitOrderService::processTicketData: their per-row catch (Exception) swallowed AmbiguousCommitException as a generic row failure and the source file was still deleted unconditionally after the loop - destroying the only reconciliation artifact for a row whose commit may or may not be durable. Both now mirror the established pattern: AmbiguousCommitException is caught separately, the row is recorded as unknown outcome at error level, remaining rows keep processing, and the source file is kept (with an error-level summary naming the rows) instead of being deleted. Reproduced first by testProcessEventDataKeepsFileWhenRowCommitOutcomeUnknown and testProcessRegistrationCompaniesDataKeepsFileWhenRowCommitOutcomeUnknown, which bind a delegating ITransactionService wrapper into the container that surfaces AmbiguousCommitException on a chosen call (ISummitService's already-resolved singleton must be forgotten first so the rebuild picks the wrapper up). ADR-003 updated: the Known Gaps entry for AmbiguousCommitException no longer claims the import loops swallow it (that text predated the processTicketData fix and contradicted the coverage table - flagged by review); only the queue-job wiring remains outstanding. The coverage table intro now states explicitly that the bolded rows pin contrasting non-nested shapes rather than the nested-rollback contract. * fix(registration): normalize owner email comparison in ReserveOrderTask $attendee_email is lowercased at the top of the attendee block, but the owner side of the strict comparison was not - an owner whose stored email contains uppercase characters always failed the identity check and fell through to a redundant repository lookup (same member under the _ci collation, but one query per ticket and fragile against collation changes). The owner side is now lowercased too, the same idiom this task already uses for the owner-email guard at the top of run(). Flagged by review on PR #533. * refactor(imports): drop AmbiguousCommitException special-casing from CSV importers Reverts the per-row AmbiguousCommitException handling (separate catch, error-level row accounting, source-file preservation) from processTicketData, processEventData and processRegistrationCompaniesData, plus its three tests. The importers return to their generic per-row catch (Exception), which already logs the full exception including the marker type, and to unconditional file deletion after the loop. Rationale (recorded in ADR-003 as a deliberate non-decision): the scenario requires a connection loss inside a single row's COMMIT window; the preserved file duplicates a source the uploading admin already has; re-processing the preserved file would duplicate orders for the rows that DID commit (createOfflineOrder is not idempotent); and no operational pipeline consumes either the error log or the preserved artifact. An unknown-outcome import row is accepted as a tolerable, manually-recoverable loss - the admin's own source file is the recovery path. The protection that matters (never re-executing the callback after an attempted COMMIT) lives in DoctrineTransactionService and is unaffected. The queue-job wiring remains the real outstanding consumer of AmbiguousCommitException, tracked in ADR-003 Known Gaps. * chore(ci): read all Stripe test credentials from repository secrets The registration webhook secret and the three bookable-rooms Stripe values were still hardcoded placeholders. All Stripe test credentials in the integration-tests job now read from the same three repository secrets (TEST_STRIPE_SECRET_KEY, TEST_STRIPE_PUBLISHABLE_KEY, TEST_STRIPE_WEBHOOK_SECRET), falling back to the previous placeholders when a secret is absent so fork PRs behave as before. * test(payments): keep Stripe webhook creation out of CI runs With real Stripe test credentials now present in CI, the previously skipped payment tests started running - and every path that registers a webhook endpoint against the real Stripe API died with "Can not create the Stripe Webhook": Stripe rejects non-publicly-accessible URLs server-side (verified empirically: WebhookEndpoint::create with the CI APP_URL fails with "Invalid URL: URL must be publicly accessible"), so no localhost environment can ever exercise webhook creation, regardless of local configuration. Two treatments: - OAuth2SummitOrdersApiTest: the payment profile is built with pre-seeded webhook data (set_webhooks + test_web_hook_secret from the TEST_STRIPE_WEBHOOK_SECRET secret), the factory's escape hatch for pre-existing webhooks - activate()->buildWebHook() then short-circuits on existsWebHook() and never calls Stripe. This lets testReserveWithActivePaymentProfile actually exercise the paid reservation flow (real PaymentIntent in test mode) in CI. - OAuth2PaymentGatewayProfileApiTest (add/update/delete): profile creation through the API always registers a webhook endpoint (the API validation rules accept no pre-seeded webhook data), so these tests are environment-gated: they now also skip when APP_URL is localhost, with the real reason documented. They run only against an environment with a publicly routable URL. * docs(adr): move SponsorUserSync pair to unreachable after #582 group gates Main's sponsors-permissions hardening (0399459, PR #582) added an assertAllowedSponsorGroup() gate that rejects any slug outside Sponsor::AllowedMemberGroups before the transaction starts, and moved the group lookup ahead of the nested eager-create call. Every failure path in addSponsorUserToGroup now throws before the nested transaction writes anything, so the written-then-rolled-back rollback proof this branch's testAddSponsorUserToGroupRollsBackAlreadyCommittedSponsorUserRowWhenGroupNotFound pinned is structurally unreachable; the test was dropped during the rebase onto that main. Move the pair from the coverage table to the 'no test is possible here' table and fix the 8/3 -> 7/4 counts.
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/9014802374/86bbag3p8
Summary by CodeRabbit
New Features
Bug Fixes