diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 0e665c5b0..a2330f792 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -64,12 +64,17 @@ jobs: - { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" } - { name: "AuditEventTypesTest", filter: "--filter AuditEventTypesTest" } - { name: "GuzzleTracingTest", filter: "--filter GuzzleTracingTest" } - - { name: "Repositories", filter: "tests/Repositories/" } - - { name: "Services", filter: "tests/Unit/Services/" } - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" } # Named by path because no job in this matrix runs the tests/ root, only its # subdirectories - a file added there runs nowhere unless it is listed here. - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php" } + - { name: "Repositories", filter: "--filter tests/Repositories/" } + - { name: "Services", filter: "--filter tests/Unit/Services/" } + - { name: "Integration", filter: "tests/Integration/" } + # Runs the same suite under PhpRedis - addSingleValue/incCounter's + # SET...NX miss sentinel differs between drivers (null vs false), + # so a driver-agnostic bug there only shows up here. + - { name: "IntegrationPhpRedis", filter: "tests/Integration/", redis_client: "phpredis" } env: OTEL_SERVICE_ENABLED: false APP_ENV: testing @@ -174,6 +179,8 @@ jobs: COMPOSER_AUTH: '{"github-oauth": {"github.com": "${{ secrets.COMPOSER_AUTH_TOKEN }}"} }' - name: Run ${{ matrix.suite.name }} + env: + REDIS_CLIENT: ${{ matrix.suite.redis_client || 'predis' }} run: | ./update_doctrine.sh php artisan db:create_initial_db --schema=config diff --git a/Libs/Utils/ICacheService.php b/Libs/Utils/ICacheService.php index 70497ea69..66ebc95fd 100644 --- a/Libs/Utils/ICacheService.php +++ b/Libs/Utils/ICacheService.php @@ -96,6 +96,16 @@ public function setSingleValue($key, $value, $ttl = 0); */ public function addSingleValue($key, $value, $ttl = 0); + /** + * Atomically compare-and-delete: DEL the key only when its current value + * equals $expectedValue. Implementations MUST use an atomic operation + * (Lua EVAL or equivalent) — never a separate GET + conditional DEL. + * @param string $key + * @param string $expectedValue + * @return bool true iff the key existed, matched, and was deleted + */ + public function deleteIfValueMatches(string $key, string $expectedValue): bool; + /** * Set time to live to a given key * @param $key diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 4004b8cf1..9fb376a24 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -16,6 +16,8 @@ use App\Services\Model\AbstractService; use App\Services\Model\IMemberService; use App\Services\Model\ISponsorUserSyncService; +use App\Services\Utils\Exceptions\UnacquiredLockException; +use App\Services\Utils\ILockManagerService; use Illuminate\Support\Facades\Log; use LaravelDoctrine\ORM\Facades\Registry; use libs\utils\ITransactionService; @@ -53,6 +55,15 @@ final class SponsorUserSyncService private ISponsorRepository $sponsor_repository; + private ILockManagerService $lock_service; + + /** + * Lifetime, in seconds, for the sponsor_user.*.ext_*.lock held around + * addSponsorUser / addSponsorUserToGroup. Matches the worst-case DB write + * time for these paths, not ILockManagerService's 3600s default. + */ + private const SponsorUserLockLifetime = 30; + /** * SponsorUserSyncService constructor. * @param ISummitRepository $summit_repository @@ -63,6 +74,7 @@ final class SponsorUserSyncService * @param IExternalUserApi $external_user_api * @param ISponsorRepository $sponsor_repository * @param ITransactionService $tx_service + * @param ILockManagerService $lock_service */ public function __construct ( @@ -73,7 +85,8 @@ public function __construct IMemberService $member_service, IExternalUserApi $external_user_api, ISponsorRepository $sponsor_repository, - ITransactionService $tx_service + ITransactionService $tx_service, + ILockManagerService $lock_service ) { parent::__construct($tx_service); @@ -84,6 +97,24 @@ public function __construct $this->member_service = $member_service; $this->external_user_api = $external_user_api; $this->sponsor_repository = $sponsor_repository; + $this->lock_service = $lock_service; + } + + /** + * Lock key guarding a (sponsor, external user) pair against the two MQ + * consumers (AddSponsorMemberMQJob / UpdateSponsorMemberGroupsMQJob) that + * can both observe a missing Sponsor_Users row and both insert one. + * Built from ids already present on both entry points' signatures - it + * must NOT depend on the local MemberID, since resolving that is part of + * what the lock protects. + * + * @param int $sponsor_id + * @param int $user_id external (IDP) user id + * @return string + */ + private function sponsorUserLockKey(int $sponsor_id, int $user_id): string + { + return "sponsor_user.{$sponsor_id}.ext_{$user_id}.lock"; } /** @@ -214,26 +245,40 @@ public function validateParams(int $summit_id, int $user_id): array /** * @inheritDoc + * @throws UnacquiredLockException propagated on purpose - same as any + * other failure here, it must burn one of the MQ job's retries rather + * than be swallowed (see the note below). */ public function addSponsorUser(int $summit_id, int $sponsor_id, int $user_id): void { // Do NOT swallow failures here: the MQ job (tries = 3) needs the // exception to apply its retry / failed_jobs machinery. A swallowed - // failure loses the membership event silently. + // failure loses the membership event silently. This also applies to + // UnacquiredLockException from the lock below. Log::debug( "SponsorUserSyncService::addSponsorUser summit {$summit_id} sponsor {$sponsor_id} user_id {$user_id}"); - list($summit, $member) = $this->validateParams($summit_id, $user_id); + // summit_sponsor_service->addSponsorUser already opens and commits its + // own transaction, so no transaction is needed at this level: by the + // time the lock callback returns, the insert is committed and visible. + $this->lock_service->lock( + $this->sponsorUserLockKey($sponsor_id, $user_id), + function () use ($summit_id, $sponsor_id, $user_id) { - Log::debug( - "SponsorUserSyncService::addSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); + list($summit, $member) = $this->validateParams($summit_id, $user_id); + + Log::debug( + "SponsorUserSyncService::addSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); - $member = $this->ensureSponsorGroupMembership($member, $user_id); + $member = $this->ensureSponsorGroupMembership($member, $user_id); - $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId()); + $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId()); - Log::info( - "SponsorUserSyncService::addSponsorUser member {$member->getId()} successfully added to sponsor {$sponsor_id}"); + Log::info( + "SponsorUserSyncService::addSponsorUser member {$member->getId()} successfully added to sponsor {$sponsor_id}"); + }, + self::SponsorUserLockLifetime + ); } /** @@ -297,6 +342,8 @@ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id /** * @inheritDoc + * @throws UnacquiredLockException propagated on purpose, same as any + * other failure in this path (see addSponsorUser). */ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $sponsor_id, int $summit_id): void { @@ -318,67 +365,79 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo "Sponsor {$sponsor_id} does not belong to summit {$summit_id}."); } - // Resolve (and, if needed, register from the IDP) OUTSIDE the transaction below. - // registerExternalUserById opens its own transaction and dispatches NewMember / - // MemberDataUpdatedExternally right after it. Those jobs are pushed immediately: - // afterCommit only defers dispatch for Eloquent-managed transactions, and this - // service uses the Doctrine DBAL connection directly. Keeping the registration - // outside guarantees the Member row is committed before any job references its id. - $member_id = $this->resolveMember($user_id)->getId(); - - $this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) { - - // Re-load inside the transaction: the tx service may have reset the entity - // manager, which would leave an entity resolved outside it detached. - $member = $this->member_repository->getById($member_id); - if (!$member instanceof Member) { - throw new EntityNotFoundException("Member with id {$member_id} not found"); - } - - // Grant the global group FIRST: Sponsor::addUser (reached through the - // eager-create path below) validates the member already belongs to a - // sponsor group, and for a brand-new sponsor user this very event is - // what delivers that group. - if (!$member->belongsToGroup($group_slug)) { - $group = $this->group_repository->getBySlug($group_slug); - if (is_null($group)) { - throw new EntityNotFoundException("Group {$group_slug} not found"); - } - $member->add2Group($group); - } - - // Add permission entry to the Sponsor_Users JSON column for this sponsor-member pair. - // If the row does not exist yet (MQ ordering race: group event arrived before membership - // event), create it eagerly so the permission is never silently dropped. - if ($member->addSponsorPermission($sponsor_id, $group_slug) === 0) { - Log::warning( - "SponsorUserSyncService::addSponsorUserToGroup no Sponsor_Users row found for " . - "member {$member->getId()} / sponsor {$sponsor_id} — creating it eagerly"); - - $summit = $this->summit_repository->getById($summit_id); - if (!$summit instanceof Summit) { - throw new EntityNotFoundException("Summit {$summit_id} not found"); - } - - $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId()); - - // Flush the UoW so the INSERT is visible to the raw SQL retry - // on the same connection within the active transaction. - Registry::getManager(SilverstripeBaseModel::EntityManager)->flush(); - - // Retry now that the row exists. - $retryResult = $member->addSponsorPermission($sponsor_id, $group_slug); - if ($retryResult === 0) { - throw new \RuntimeException( - "Failed to write permission after eager Sponsor_Users creation " . - "for member {$member->getId()} / sponsor {$sponsor_id}" - ); - } - } - - Log::info( - "SponsorUserSyncService::addSponsorUserToGroup member {$member->getId()} added to group {$group_slug} via sponsor {$sponsor_id}"); - }); + // Lock held OUTSIDE the transaction below: releasing it before commit + // would let a waiting AddSponsorMemberMQJob/UpdateSponsorMemberGroupsMQJob + // observe the row as still-missing and insert its own duplicate. + $this->lock_service->lock( + $this->sponsorUserLockKey($sponsor_id, $user_id), + function () use ($user_id, $group_slug, $sponsor_id, $summit_id) { + + // Resolve (and, if needed, register from the IDP) OUTSIDE the transaction below, + // but INSIDE the lock - resolving the local MemberID is part of what the lock + // protects. registerExternalUserById opens its own transaction and dispatches + // NewMember / MemberDataUpdatedExternally right after it. Those jobs are pushed + // immediately: afterCommit only defers dispatch for Eloquent-managed transactions, + // and this service uses the Doctrine DBAL connection directly. Keeping the + // registration outside the transaction guarantees the Member row is committed + // before any job references its id. + $member_id = $this->resolveMember($user_id)->getId(); + + $this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) { + + // Re-load inside the transaction: the tx service may have reset the entity + // manager, which would leave an entity resolved outside it detached. + $member = $this->member_repository->getById($member_id); + if (!$member instanceof Member) { + throw new EntityNotFoundException("Member with id {$member_id} not found"); + } + + // Grant the global group FIRST: Sponsor::addUser (reached through the + // eager-create path below) validates the member already belongs to a + // sponsor group, and for a brand-new sponsor user this very event is + // what delivers that group. + if (!$member->belongsToGroup($group_slug)) { + $group = $this->group_repository->getBySlug($group_slug); + if (is_null($group)) { + throw new EntityNotFoundException("Group {$group_slug} not found"); + } + $member->add2Group($group); + } + + // Add permission entry to the Sponsor_Users JSON column for this sponsor-member pair. + // If the row does not exist yet (MQ ordering race: group event arrived before membership + // event), create it eagerly so the permission is never silently dropped. + if ($member->addSponsorPermission($sponsor_id, $group_slug) === 0) { + Log::warning( + "SponsorUserSyncService::addSponsorUserToGroup no Sponsor_Users row found for " . + "member {$member->getId()} / sponsor {$sponsor_id} — creating it eagerly"); + + $summit = $this->summit_repository->getById($summit_id); + if (!$summit instanceof Summit) { + throw new EntityNotFoundException("Summit {$summit_id} not found"); + } + + $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId()); + + // Flush the UoW so the INSERT is visible to the raw SQL retry + // on the same connection within the active transaction. + Registry::getManager(SilverstripeBaseModel::EntityManager)->flush(); + + // Retry now that the row exists. + $retryResult = $member->addSponsorPermission($sponsor_id, $group_slug); + if ($retryResult === 0) { + throw new \RuntimeException( + "Failed to write permission after eager Sponsor_Users creation " . + "for member {$member->getId()} / sponsor {$sponsor_id}" + ); + } + } + + Log::info( + "SponsorUserSyncService::addSponsorUserToGroup member {$member->getId()} added to group {$group_slug} via sponsor {$sponsor_id}"); + }); + }, + self::SponsorUserLockLifetime + ); } /** diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 7a22d57c9..922cdc1f0 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -267,6 +267,7 @@ private function buildPrePaidSaga(Member $owner, Summit $summit, array $payload) $this->member_repository, $this->attendee_repository, $this->ticket_type_repository, + $this->promo_code_repository, $this->tx_service, $this->lock_service )); @@ -830,7 +831,7 @@ public function run(array $formerState): array $this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $qty, $owner_email) { $promo_code->addUsage($owner_email, $qty); - }); + }, 30); }); // mark a done @@ -868,7 +869,7 @@ public function undo() $this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $info, $owner_email) { $promo_code->removeUsage(intval($info['qty']), $owner_email); - }); + }, 30); }); } @@ -953,7 +954,7 @@ public function run(array $formerState): array $this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $reservations) { $ticket_type->sell($reservations[$ticket_type->getId()]); - }); + }, 30); } }); @@ -970,7 +971,7 @@ public function undo() if (is_null($ticket_type)) return; $this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $qty) { $ticket_type->restore($qty); - }); + }, 30); }); } } @@ -1477,6 +1478,11 @@ final class AutoAssignPrePaidTicketTask extends AbstractTask */ private $ticket_type_repository; + /** + * @var ISummitRegistrationPromoCodeRepository + */ + private $promo_code_repository; + /** * @var ILockManagerService */ @@ -1490,19 +1496,21 @@ final class AutoAssignPrePaidTicketTask extends AbstractTask * @param IMemberRepository $member_repository * @param ISummitAttendeeRepository $attendee_repository * @param ISummitTicketTypeRepository $ticket_type_repository + * @param ISummitRegistrationPromoCodeRepository $promo_code_repository * @param ITransactionService $tx_service * @param ILockManagerService $lock_service */ public function __construct ( - ?Member $owner, - Summit $summit, - array $payload, - IMemberRepository $member_repository, - ISummitAttendeeRepository $attendee_repository, - ISummitTicketTypeRepository $ticket_type_repository, - ITransactionService $tx_service, - ILockManagerService $lock_service + ?Member $owner, + Summit $summit, + array $payload, + IMemberRepository $member_repository, + ISummitAttendeeRepository $attendee_repository, + ISummitTicketTypeRepository $ticket_type_repository, + ISummitRegistrationPromoCodeRepository $promo_code_repository, + ITransactionService $tx_service, + ILockManagerService $lock_service ) { $this->tx_service = $tx_service; @@ -1513,6 +1521,7 @@ public function __construct $this->member_repository = $member_repository; $this->attendee_repository = $attendee_repository; $this->ticket_type_repository = $ticket_type_repository; + $this->promo_code_repository = $promo_code_repository; } public function run(array $formerState): array @@ -1539,8 +1548,8 @@ public function run(array $formerState): array if (empty($promo_code_val)) throw new ValidationException("Promo code is required."); $type_id = $ticket_dto['type_id']; - $order = $this->lock_service->lock('ticket_type.' . $type_id . 'promo_code.' . $promo_code_val . '.sell.lock', - function () use ($promo_code_val, $type_id) { + $order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock', + function () use ($promo_code_val, $type_id, $ticket_dto) { $attendee_email = $this->owner->getEmail(); // use what we have on payload first @@ -1558,7 +1567,7 @@ function () use ($promo_code_val, $type_id) { if (empty($attendee_last_name)) $attendee_last_name = $this->payload['owner_last_name'] ?? $this->owner->getLastName(); - $promo_code = $this->summit->getPromoCodeByCode($promo_code_val); + $promo_code = $this->promo_code_repository->getByValueExclusiveLock($this->summit, $promo_code_val); if (!PromoCodesUtils::isPrePaidPromoCode($promo_code)) throw new EntityNotFoundException("Promo code is not found."); @@ -1661,7 +1670,7 @@ function () use ($promo_code_val, $type_id) { return $order; - }); + }, 30); return ['order' => $order]; }); } diff --git a/app/Services/Utils/ILockManagerService.php b/app/Services/Utils/ILockManagerService.php index 4d5a7ff33..1928edb7f 100644 --- a/app/Services/Utils/ILockManagerService.php +++ b/app/Services/Utils/ILockManagerService.php @@ -24,14 +24,15 @@ interface ILockManagerService * @param string $name * @param int $lifetime * @throws UnacquiredLockException - * @return mixed + * @return string ownership token — must be passed to releaseLock */ - public function acquireLock(string $name,int $lifetime = self::DefaultLifetime); + public function acquireLock(string $name, int $lifetime = self::DefaultLifetime): string; + /** - * @param string $name - * @return mixed + * @param string $name + * @param string $token ownership token returned by acquireLock */ - public function releaseLock(string $name); + public function releaseLock(string $name, string $token): void; /** * @param string $name @@ -39,5 +40,5 @@ public function releaseLock(string $name); * @param int $lifetime * @return mixed */ - public function lock(string $name, Closure $callback, int $lifetime = self::DefaultLifetime); + public function lock(string $name, Closure $callback, int $lifetime = self::DefaultLifetime): mixed; } \ No newline at end of file diff --git a/app/Services/Utils/LockManagerService.php b/app/Services/Utils/LockManagerService.php index a031ff825..cb4e2c403 100644 --- a/app/Services/Utils/LockManagerService.php +++ b/app/Services/Utils/LockManagerService.php @@ -22,9 +22,10 @@ */ final class LockManagerService implements ILockManagerService { - const MaxRetries = 3; + const MaxRetries = 3; const BackOffMultiplier = 2.0; - const BackOffBaseInterval = 100000; // 1 ms + const BackOffBaseInterval = 100000; // microseconds + /** * @var ICacheService */ @@ -41,73 +42,80 @@ public function __construct(ICacheService $cache_service){ /** * @param string $name * @param int $lifetime - * @return LockManagerService + * @return string ownership token — pass to releaseLock * @throws UnacquiredLockException */ - public function acquireLock(string $name, int $lifetime = 3600):LockManagerService + public function acquireLock(string $name, int $lifetime = 3600): string { - Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s",$name, $lifetime)); - $attempt = 0 ; + Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s", $name, $lifetime)); + if ($lifetime <= 0) { + throw new \InvalidArgumentException("Lock lifetime must be greater than zero seconds."); + } + $token = bin2hex(random_bytes(16)); + $attempt = 0; do { - $time = time() + $lifetime + 1; - $success = $this->cache_service->addSingleValue($name, $time, $time); - if($success) return $this; - $wait_interval = self::BackOffBaseInterval * ( self::BackOffMultiplier ^ $attempt ); - Log::debug(sprintf("LockManagerService::acquireLock name %s retrying in %s microseconds (%s).", $name, $wait_interval, $attempt)); + $success = $this->cache_service->addSingleValue($name, $token, $lifetime); + if ($success) { + return $token; + } + $wait_interval = (int)(self::BackOffBaseInterval * (self::BackOffMultiplier ** $attempt)); + Log::debug(sprintf("LockManagerService::acquireLock name %s retrying in %s µs (attempt %s)", $name, $wait_interval, $attempt)); usleep($wait_interval); - if($attempt >= (self::MaxRetries - 1 )) { - // only one time we could use this handle + if ($attempt >= (self::MaxRetries - 1)) { Log::error(sprintf("LockManagerService::acquireLock name %s lifetime %s ERROR MAX RETRIES attempt %s", $name, $lifetime, $attempt)); throw new UnacquiredLockException(sprintf("lock name %s", $name)); } ++$attempt; - } while(1); + } while (1); } /** * @param string $name - * @return $this + * @param string $token ownership token returned by acquireLock */ - public function releaseLock(string $name):LockManagerService + public function releaseLock(string $name, string $token): void { - Log::debug(sprintf("LockManagerService::releaseLock name %s",$name)); - $this->cache_service->delete($name); - return $this; + Log::debug(sprintf("LockManagerService::releaseLock name %s", $name)); + $released = $this->cache_service->deleteIfValueMatches($name, $token); + if (!$released) { + Log::warning(sprintf("LockManagerService::releaseLock name %s token %s lock was not held by this token at release time (expired or stolen).", $name, $token)); + $this->cache_service->incCounter('lock_manager.release_mismatch'); + } } /** * @param string $name * @param Closure $callback * @param int $lifetime - * @return null + * @return mixed * @throws UnacquiredLockException * @throws Exception */ - public function lock(string $name, Closure $callback, int $lifetime = 3600) + public function lock(string $name, Closure $callback, int $lifetime = 3600): mixed { + $token = null; $result = null; Log::debug(sprintf("LockManagerService::lock name %s lifetime %s", $name, $lifetime)); - try - { - $this->acquireLock($name, $lifetime); + try { + $token = $this->acquireLock($name, $lifetime); Log::debug(sprintf("LockManagerService::lock name %s calling callback", $name)); $result = $callback($this); } - catch(UnacquiredLockException $ex) - { + catch(UnacquiredLockException $ex) { Log::warning($ex); throw $ex; } - catch(Exception $ex) - { + catch(Exception $ex) { Log::error($ex); throw $ex; } finally { - $this->releaseLock($name); + if ($token !== null) { + $this->releaseLock($name, $token); + } } return $result; } -} \ No newline at end of file +} diff --git a/app/Services/Utils/RedisCacheService.php b/app/Services/Utils/RedisCacheService.php index 22b9122b6..f54582a98 100644 --- a/app/Services/Utils/RedisCacheService.php +++ b/app/Services/Utils/RedisCacheService.php @@ -239,9 +239,10 @@ public function storeHash($name, array $values, $ttl = 0) public function incCounter($counter_name, $ttl = 0) { return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) { - if ($conn->setnx($counter_name, 1)) { - if ($ttl > 0) $conn->expire($counter_name, (int)$ttl); - return 1; + if ($ttl > 0) { + if ($this->setNxSucceeded($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX'))) return 1; + } else { + if ($this->setNxSucceeded($conn->set($counter_name, 1, 'NX'))) return 1; } return (int)$conn->incr($counter_name); }, 0); @@ -306,12 +307,21 @@ public function setSingleValue($key, $value, $ttl = 0) public function addSingleValue($key, $value, $ttl = 0) { return $this->retryOnConnectionError(function ($conn) use ($key, $value, $ttl) { - $res = $conn->setnx($key, $value); - if ($res && $ttl > 0) { - $conn->expire($key, $ttl); + if ($ttl > 0) { + return $this->setNxSucceeded($conn->set($key, $value, 'EX', (int)$ttl, 'NX')); } - return $res; - }); + return $this->setNxSucceeded($conn->set($key, $value, 'NX')); + }, false); + } + + /** + * SET ... NX reports a miss as null under Predis but as false under PhpRedis. + * Any real success value (a Predis\Response\Status object, or PhpRedis's true/1) + * is truthy against both checks. + */ + private function setNxSucceeded($result): bool + { + return $result !== null && $result !== false; } public function setKeyExpiration($key, $ttl) @@ -331,7 +341,21 @@ public function ttl($key) return (int)$conn->ttl($key); }, 0); } - + + public function deleteIfValueMatches(string $key, string $expectedValue): bool + { + $lua = <<<'LUA' +if redis.call('get', KEYS[1]) == ARGV[1] then + return redis.call('del', KEYS[1]) +else + return 0 +end +LUA; + return $this->retryOnConnectionError(function ($conn) use ($lua, $key, $expectedValue) { + return (int)$conn->eval($lua, 1, $key, $expectedValue) === 1; + }, false); + } + /** * @param string $cache_region_key * @return void diff --git a/tests/Integration/RedisCacheServiceAddSingleValueTest.php b/tests/Integration/RedisCacheServiceAddSingleValueTest.php new file mode 100644 index 000000000..df50fc637 --- /dev/null +++ b/tests/Integration/RedisCacheServiceAddSingleValueTest.php @@ -0,0 +1,148 @@ +redis = Redis::connection(); + $this->service = new RedisCacheService(); + // Start clean regardless of any leftover from a previous failed run. + $this->redis->del(self::TEST_KEY); + } + + protected function tearDown(): void + { + $this->redis->del(self::TEST_KEY); + parent::tearDown(); + } + + /** + * First call must succeed and leave a TTL on the key. + * Second call on the same key must return false (NX semantics). + */ + public function testAddSingleValueSetsKeyWithTtlAndNxSemanticsHold(): void + { + $token = bin2hex(random_bytes(16)); + + $acquired = $this->service->addSingleValue(self::TEST_KEY, $token, self::TTL); + $this->assertTrue($acquired, 'first addSingleValue must return true'); + + // Atomicity: TTL must already be set — no gap between key write and expire. + $ttl = (int)$this->redis->ttl(self::TEST_KEY); + $this->assertGreaterThanOrEqual(1, $ttl, 'key must have a positive TTL immediately after addSingleValue'); + $this->assertLessThanOrEqual(self::TTL, $ttl, 'TTL must not exceed the requested lifetime'); + + // NX semantics: a second call while the key still exists must fail. + $again = $this->service->addSingleValue(self::TEST_KEY, bin2hex(random_bytes(16)), self::TTL); + $this->assertFalse($again, 'addSingleValue must return false when key already exists (NX)'); + } + + /** + * After the key is deleted the lock can be re-acquired, confirming the + * return-value contract holds across both the true and false branches. + */ + public function testAddSingleValueReturnsTrueAfterKeyIsDeleted(): void + { + $token = bin2hex(random_bytes(16)); + + $this->assertTrue($this->service->addSingleValue(self::TEST_KEY, $token, self::TTL)); + $this->redis->del(self::TEST_KEY); + $this->assertTrue( + $this->service->addSingleValue(self::TEST_KEY, bin2hex(random_bytes(16)), self::TTL), + 'addSingleValue must return true once the key has been removed' + ); + } + + /** + * A release with the matching ownership token must delete the key. + */ + public function testDeleteIfValueMatchesDeletesKeyOnMatch(): void + { + $token = bin2hex(random_bytes(16)); + $this->redis->set(self::TEST_KEY, $token, 'EX', self::TTL); + + $released = $this->service->deleteIfValueMatches(self::TEST_KEY, $token); + + $this->assertTrue($released, 'deleteIfValueMatches must return true when the token matches'); + $this->assertSame(0, (int)$this->redis->exists(self::TEST_KEY), 'key must be gone after a matching release'); + } + + /** + * A release with a stale/foreign token must leave the key untouched — + * this is the ownership guarantee the whole lock relies on. + */ + public function testDeleteIfValueMatchesLeavesKeyIntactOnMismatch(): void + { + $token = bin2hex(random_bytes(16)); + $this->redis->set(self::TEST_KEY, $token, 'EX', self::TTL); + + $released = $this->service->deleteIfValueMatches(self::TEST_KEY, bin2hex(random_bytes(16))); + + $this->assertFalse($released, 'deleteIfValueMatches must return false when the token does not match'); + $this->assertSame($token, $this->redis->get(self::TEST_KEY), 'key must survive a non-matching release attempt'); + } + + /** + * incCounter shares the same SET...NX miss-detection as addSingleValue: + * the first call must create the counter at 1, and the second call must + * increment it rather than mistaking an NX-miss for a fresh creation. + */ + public function testIncCounterIncrementsExistingCounterInsteadOfResetting(): void + { + $first = $this->service->incCounter(self::TEST_KEY, self::TTL); + $second = $this->service->incCounter(self::TEST_KEY, self::TTL); + + $this->assertSame(1, $first, 'first incCounter call must create the counter at 1'); + $this->assertSame(2, $second, 'second incCounter call must increment the existing counter, not reset it'); + $this->assertSame('2', $this->redis->get(self::TEST_KEY)); + $this->assertGreaterThan(0, (int)$this->redis->ttl(self::TEST_KEY)); + } +} diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php index c2b4cf82c..2abb67205 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -384,6 +384,73 @@ public function testAutoAssignDifferentPrePaidTicketsUntilEmpty() { } } + public function testAutoAssignPrePaidTicketUsesTicketLevelAttendeeData() { + + // Fixture registration window starts tomorrow (see InsertSummitTestData); + // open it now so this test isn't blocked by an unrelated precondition. + self::$summit->setRegistrationBeginDate(new \DateTime('-1 day')); + self::$summit->setRegistrationEndDate(new \DateTime('+30 days')); + self::$em->persist(self::$summit); + self::$em->flush(); + + // Build a dedicated unassigned, paid, offline ticket so this test does not + // depend on the base fixture's order #0 (already-assigned attendee / online + // payment method, neither of which qualifies for prepaid pickup). + $owner = self::$defaultMember; + + $order = new SummitOrder(); + $order->setSummit(self::$summit); + $order->setOwner($owner); + $order->setPaymentMethodOffline(); + $order->generateNumber(); + + $ticket = new SummitAttendeeTicket(); + $ticket->setTicketType(self::$default_ticket_type); + $order->addTicket($ticket); + $ticket->activate(); + $ticket->generateNumber(); + $ticket->generateQRCode(); + + self::$summit->addOrder($order); + self::$em->persist($order); + self::$em->flush(); + + $order->setPaid(); + + self::$default_prepaid_discount_code->clearTickets(); + self::$default_prepaid_discount_code->addTicket($ticket); + self::$em->persist(self::$default_prepaid_discount_code); + self::$em->persist($order); + self::$em->flush(); + + $service = App::make(ISummitOrderService::class); + + $payload = [ + "owner_email" => $owner->getEmail(), + "owner_first_name" => $owner->getFirstName(), + "owner_last_name" => $owner->getLastName(), + "owner_company" => $owner->getCompany(), + "tickets" => [ + [ + "type_id" => self::$default_ticket_type->getId(), + "promo_code" => self::$default_prepaid_discount_code->getCode(), + // Attendee is a different person than the order owner - + // this is the scenario AutoAssignPrePaidTicketTask exists for. + "attendee_company" => "Attendee Co", + "attendee_first_name" => "Jane", + "attendee_last_name" => "Doe", + ], + ] + ]; + + $result_order = $service->reserve($owner, self::$summit, $payload); + $attendee = $result_order->getTickets()->first()->getOwner(); + + $this->assertEquals("Attendee Co", $attendee->getCompanyName()); + $this->assertEquals("Jane", $attendee->getFirstName()); + $this->assertEquals("Doe", $attendee->getSurname()); + } + /** * @param string $csv_content * @return ISummitOrderService diff --git a/tests/Unit/Services/LockManagerServiceOwnershipTest.php b/tests/Unit/Services/LockManagerServiceOwnershipTest.php new file mode 100644 index 000000000..121a2cb06 --- /dev/null +++ b/tests/Unit/Services/LockManagerServiceOwnershipTest.php @@ -0,0 +1,181 @@ +instance('app', $container); + $container->instance('log', new class { + public function __call($name, $args) {} + }); + \Illuminate\Support\Facades\Facade::setFacadeApplication($container); + } + + protected function tearDown(): void + { + \Illuminate\Support\Facades\Facade::clearResolvedInstances(); + \Illuminate\Support\Facades\Facade::setFacadeApplication(null); + Mockery::close(); + parent::tearDown(); + } + + /** + * Alice holds the lock. Bob's acquire exhausts retries and throws + * UnacquiredLockException. Bob's lock() finally block must NOT call + * deleteIfValueMatches — Bob never owned the key and must not delete it. + * + * On main (before fix) this test fails because releaseLock was called + * unconditionally from the finally block. + */ + public function testBobsFailedAcquireNeverDeletesAlicesKey(): void + { + // Alice: acquires once, releases once via deleteIfValueMatches. + $aliceCache = Mockery::mock(ICacheService::class); + $aliceCache->shouldReceive('addSingleValue')->once()->andReturn(true); + $aliceCache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); + + // Bob: fails to acquire on every retry; must never touch Redis for release. + $bobCache = Mockery::mock(ICacheService::class); + $bobCache->shouldReceive('addSingleValue') + ->times(LockManagerService::MaxRetries) + ->andReturn(false); + $bobCache->shouldReceive('deleteIfValueMatches')->never(); + + $alice = new LockManagerService($aliceCache); + $bob = new LockManagerService($bobCache); + + $alice->lock('resource.lock', function () { + // Alice's critical section. + }); + + $this->expectException(UnacquiredLockException::class); + $bob->lock('resource.lock', function () { + $this->fail('Bob must not enter the critical section.'); + }); + // Mockery tearDown asserts deleteIfValueMatches was never called on $bobCache. + } + + /** + * After a full acquire → callback → release cycle exactly one + * addSingleValue and one deleteIfValueMatches must have been issued. + * Mockery's ->once() expectations enforce this without inspecting internals. + */ + public function testSuccessfulLockCyclePairsAcquireAndRelease(): void + { + $cache = Mockery::mock(ICacheService::class); + $cache->shouldReceive('addSingleValue')->once()->andReturn(true); + $cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); + + $service = new LockManagerService($cache); + $callbackRan = false; + $service->lock('test.lock', function () use (&$callbackRan) { + $callbackRan = true; + }, 3600); + + $this->assertTrue($callbackRan, 'callback must execute inside the lock'); + // Mockery tearDown verifies addSingleValue and deleteIfValueMatches each fired once. + } + + /** + * Structural assertion: addSingleValue is called exactly once per + * acquisition attempt (not two separate calls for setnx + expire). + * The call must carry the lock name, a string token, and the lifetime. + */ + public function testAddSingleValueCalledOnceWithTokenAndLifetime(): void + { + $cache = Mockery::mock(ICacheService::class); + $token = null; + $cache->shouldReceive('addSingleValue') + ->once() + ->with( + 'test.lock', + Mockery::on(function ($value) use (&$token) { + if (!is_string($value) || $value === '') return false; + $token = $value; + return true; + }), + 3600 + ) + ->andReturn(true); + // Not an arrow fn: arrow functions capture $token by value at closure-creation + // time, when it is still null (addSingleValue's callback hasn't run yet). + // A `use (&$token)` closure reads the current value at call time instead. + $cache->shouldReceive('deleteIfValueMatches') + ->once() + ->with('test.lock', Mockery::on(function ($value) use (&$token) { + return $value === $token; + })) + ->andReturn(true); + + $service = new LockManagerService($cache); + $callbackRan = false; + $service->lock('test.lock', function () use (&$callbackRan) { + $callbackRan = true; + }, 3600); + + $this->assertTrue($callbackRan, 'callback must execute inside the lock'); + // Mockery tearDown verifies the single atomic SET NX EX call. + } + + /** + * Known failure mode: when deleteIfValueMatches returns false (Redis + * unavailable), the token is passed to the Lua script but deletion fails + * silently. The Redis key persists until TTL expiry; there is no + * application-level retry path after a failed release. releaseLock must + * surface this as a counter increment so the mismatch is observable. + */ + public function testReleaseLockWhenRedisDownLeavesKeyUntilTtl(): void + { + $cache = Mockery::mock(ICacheService::class); + $cache->shouldReceive('addSingleValue')->once()->andReturn(true); + // Simulate Redis unavailable — deletion silently fails. + $cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(false); + $cache->shouldReceive('incCounter')->once()->with('lock_manager.release_mismatch'); + + $service = new LockManagerService($cache); + $callbackRan = false; + $service->lock('resource.lock', function () use (&$callbackRan) { + $callbackRan = true; + }); + + $this->assertTrue($callbackRan, 'callback must run even when the subsequent release fails'); + + // The Redis key was NOT deleted; only TTL expiry can free it. + // A subsequent acquire attempt on the same resource will fail until the + // TTL elapses — there is no application-level retry path. + // Mockery tearDown verifies the mismatch counter was incremented. + } +} diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 915f54dfa..ed70662a7 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -14,6 +14,8 @@ use App\Models\Foundation\Main\IGroup; use App\Services\Model\ISponsorUserSyncService; +use App\Services\Utils\Exceptions\UnacquiredLockException; +use App\Services\Utils\ILockManagerService; use Tests\InsertMemberTestData; use Tests\InsertSummitTestData; use Tests\TestCase; @@ -1027,4 +1029,79 @@ public function testRemoveSponsorUserFromGroupRetainsGlobalGroupWhenAnotherSpons $member = self::$member_repository->find($member_id); $this->assertTrue($member->belongsToGroup(IGroup::Sponsors)); } + + // ------------------------------------------------------------------------- + // Concurrency guard (issue #583: duplicate Sponsor_Users rows) + // ------------------------------------------------------------------------- + + /** + * Reproduces the exact race #583 describes: AddSponsorMemberMQJob + * (addSponsorUser) and UpdateSponsorMemberGroupsMQJob (addSponsorUserToGroup) + * both reaching the same (sponsor, external user) pair concurrently, in + * different workers. Acquiring the pair's shared lock manually simulates + * one consumer already mid-flight; the other consumer racing in for the + * SAME pair must be rejected with UnacquiredLockException rather than + * sailing past the unlocked contains() check and inserting a duplicate + * row. Once the lock is free, the second consumer proceeds normally and + * the pair ends up with exactly one Sponsor_Users row. + */ + public function testAddSponsorUserAndAddSponsorUserToGroupShareTheSameLockForAPair(): void + { + $sponsor_id = self::$sponsors[1]->getId(); // no Sponsor_Users row yet + $member_id = self::$member->getId(); + $external_id = self::$member->getUserExternalId(); + $summit_id = self::$summit->getId(); + + $lock_service = app(ILockManagerService::class); + $key = "sponsor_user.{$sponsor_id}.ext_{$external_id}.lock"; + + // Simulate AddSponsorMemberMQJob (addSponsorUser) already mid-flight. + $token = $lock_service->acquireLock($key, 30); + + try { + $this->expectException(UnacquiredLockException::class); + // UpdateSponsorMemberGroupsMQJob (addSponsorUserToGroup) races in for the SAME pair. + $this->getService()->addSponsorUserToGroup($external_id, IGroup::Sponsors, $sponsor_id, $summit_id); + } finally { + $this->assertFalse( + $this->hasSponsorUserRow($sponsor_id, $member_id), + 'the blocked consumer must not have inserted a duplicate row while the lock was held' + ); + $lock_service->releaseLock($key, $token); + } + + // Once free, the second consumer proceeds normally. + $this->getService()->addSponsorUserToGroup($external_id, IGroup::Sponsors, $sponsor_id, $summit_id); + + $count = (int)self::$em->getConnection()->executeQuery( + 'SELECT COUNT(*) FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?', + [$sponsor_id, $member_id] + )->fetchOne(); + $this->assertEquals(1, $count, 'exactly one Sponsor_Users row must exist for the pair'); + } + + /** + * Same shared-lock contract, exercised from the other entry point: + * addSponsorUser racing against a lock already held for the same pair + * must also be rejected rather than proceeding. + */ + public function testAddSponsorUserThrowsUnacquiredLockExceptionWhenPairIsAlreadyLocked(): void + { + $sponsor_id = self::$sponsors[1]->getId(); + $member_id = self::$member->getId(); + $external_id = self::$member->getUserExternalId(); + $summit_id = self::$summit->getId(); + + $lock_service = app(ILockManagerService::class); + $key = "sponsor_user.{$sponsor_id}.ext_{$external_id}.lock"; + $token = $lock_service->acquireLock($key, 30); + + try { + $this->expectException(UnacquiredLockException::class); + $this->getService()->addSponsorUser($summit_id, $sponsor_id, $external_id); + } finally { + $this->assertFalse($this->hasSponsorUserRow($sponsor_id, $member_id)); + $lock_service->releaseLock($key, $token); + } + } }