Sponsor_Users: concurrent MQ consumers can create duplicate (SponsorID, MemberID) rows

[smarcet]

Summary

Two MQ consumers can insert the same (SponsorID, MemberID) pair into Sponsor_Users concurrently, producing duplicate rows whose Permissions JSON then diverges and never reconverges. A unique index fixes the data-integrity side; this issue tracks the application-level serialization that keeps the collision from reaching failed_jobs.

Blocked by #537 — see Dependency below.

Background

Sponsor_Users is the join table of the Sponsor ⇄ Member ManyToMany (app/Models/Foundation/Summit/Sponsor.php:124-128), but it carries a surrogate auto-increment PK inherited from Silverstripe instead of the composite (SponsorID, MemberID) PK Doctrine would generate, and only two non-unique indexes:

PRIMARY KEY (`ID`),
KEY `MemberID` (`MemberID`),
KEY `SponsorID` (`SponsorID`)

The only guard against a duplicate pair is Sponsor::addUser:

if ($this->members->contains($user)) return;   // Sponsor.php:370

That is an unlocked read. Two consumers reach it for the same pair, in different jobs and therefore different workers:

Job	Path
AddSponsorMemberMQJob	→ SponsorUserSyncService::addSponsorUser → SummitSponsorService::addSponsorUser:500
UpdateSponsorMemberGroupsMQJob	→ SponsorUserSyncService::addSponsorUserToGroup → eager create at SponsorUserSyncService.php:203

Both can observe false and both insert.

Impact

Duplicated rows make Permissions ambiguous. Member::addSponsorPermission documents itself as "Returns 0 when the Sponsor_Users row does not exist, 1 when it does" and its UPDATE carries no LIMIT, so a slug written before the second row appeared lives on in only one of them and is never replicated to the other. Observed in dev: one pair with two rows, one holding ["sponsors-services", "sponsors"] and the other only ["sponsors"].

Authorization is not affected — the read paths use SELECT DISTINCT(SponsorID) (Member.php:1938) and COUNT(...) > 0 (Member.php:1966), which are immune to duplicates. The damage is the divergent permission state.

Proposal

Serialize the two write paths with the existing ILockManagerService (app/Services/Utils/ILockManagerService.php), already used for exactly this class of problem in SummitOrderService:831,869,954,971,1542.

1. Inject ILockManagerService into SponsorUserSyncService.
2. Key: sponsor_user.{$sponsor_id}.ext_{$user_id}.lock. Both entry points receive sponsor_id and the external user id in their signature, so the key can be built before any query — it must not depend on the local MemberID, since resolving that is part of what needs protecting.
3. Wrap the two write paths, with the lock outside the transaction:
   • addSponsorUserToGroup: lock(key, fn() => tx_service->transaction(<current body>))
   • addSponsorUser: lock(key, fn() => <current body>)
4. Explicit lifetime of 30s, matching what fix(lock): implement Redlock single-instance pattern in LockManagerService #537 does at its own call sites.
5. Decide the handler's policy for UnacquiredLockException (propagating burns one of the job's tries).

The lock must survive the commit: if placed inside the transaction it is released while the transaction is still open, and the waiter proceeds without seeing the uncommitted row — reintroducing the race.

addSponsorUser does not need its own transaction: its call to summit_sponsor_service->addSponsorUser already opens one that commits for real, so by the time the lock closure returns, the insert is committed and visible.

Dependency

This must land after #537 (fix(lock): implement Redlock single-instance pattern in LockManagerService). Building on main's LockManagerService is pointless — two measured defects there void the mutual exclusion in precisely the contended case this issue is about:

• TTL is passed as an absolute epoch. acquireLock computes $time = time() + $lifetime + 1 and passes it as the TTL; RedisCacheService::addSingleValue:311 hands it to expire(), which expects relative seconds. Measured: a requested 30s lifetime produced a TTL of 1786381957 (≈ 56 years). A worker dying while holding the lock would block that pair permanently.
• The finally releases someone else's lock. lock() calls releaseLock($name) in finally, which also runs when acquireLock threw UnacquiredLockException — i.e. when another process holds it. Measured: process A holds the lock, B fails to acquire, and A's key is gone afterwards.

#537 fixes both (ownership tokens + Lua compare-and-delete, relative TTL via atomic SET NX EX), plus the ^-instead-of-** backoff bug.

Alternative considered and rejected

Switching SponsorUserSyncService::resolveMember to IMemberRepository::getByExternalIdExclusiveLock (a SELECT ... FOR UPDATE on the Member row). Rejected because Query::setLockMode throws TransactionRequiredException without an active transaction (vendor/doctrine/orm/src/Query.php:634-640), and addSponsorUser / removeSponsorUser do not open one. Adding transactions there would make summit_sponsor_service calls nested and would re-extend the "jobs enqueued before commit" problem those paths were just fixed for.

Scope

Only the two write paths. removeSponsorUser / removeSponsorUserFromGroup do not create rows, and the former can be called with sponsor_id = null, so it has no key to lock on.

Acceptance criteria

• SponsorUserSyncService::addSponsorUser and addSponsorUserToGroup run their bodies inside ILockManagerService::lock with the shared key, lock acquired outside the transaction.
• A test proves two interleaved invocations for the same pair produce exactly one Sponsor_Users row.
• The handler's behaviour on UnacquiredLockException is explicit and covered.
• OAuth2SummitSponsorApiTest and the sponsor permission suites stay green.

Related

• Depends on fix(lock): implement Redlock single-instance pattern in LockManagerService #537
• A UNIQUE KEY (SponsorID, MemberID) on Sponsor_Users is the hard guarantee; this issue is the application-level layer on top of it, not a replacement. Without the index, no amount of locking is a guarantee.
• Context: PR Hotfix/sponsors permissions #582