From b45695158ac4523af825f56dceb6a99e13a34caa Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 4 May 2026 16:32:18 +0200 Subject: [PATCH 1/8] fix(lock): implement Redlock single-instance pattern in LockManagerService Signed-off-by: romanetar --- Libs/Utils/ICacheService.php | 10 ++ app/Services/Utils/LockManagerService.php | 55 +++--- app/Services/Utils/RedisCacheService.php | 27 ++- .../LockManagerServiceOwnershipTest.php | 156 ++++++++++++++++++ 4 files changed, 219 insertions(+), 29 deletions(-) create mode 100644 tests/Unit/Services/LockManagerServiceOwnershipTest.php 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/Utils/LockManagerService.php b/app/Services/Utils/LockManagerService.php index a031ff825..6c9220d38 100644 --- a/app/Services/Utils/LockManagerService.php +++ b/app/Services/Utils/LockManagerService.php @@ -22,14 +22,18 @@ */ 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 */ private $cache_service; + /** @var array lock-name → per-call ownership token */ + private array $tokens = []; + /** * LockManagerService constructor. * @param ICacheService $cache_service @@ -46,22 +50,24 @@ public function __construct(ICacheService $cache_service){ */ public function acquireLock(string $name, int $lifetime = 3600):LockManagerService { - Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s",$name, $lifetime)); - $attempt = 0 ; + Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s", $name, $lifetime)); + $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) { + $this->tokens[$name] = $token; + return $this; + } + $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); } /** @@ -70,8 +76,12 @@ public function acquireLock(string $name, int $lifetime = 3600):LockManagerServi */ public function releaseLock(string $name):LockManagerService { - Log::debug(sprintf("LockManagerService::releaseLock name %s",$name)); - $this->cache_service->delete($name); + Log::debug(sprintf("LockManagerService::releaseLock name %s", $name)); + if (!isset($this->tokens[$name])) { + return $this; + } + $this->cache_service->deleteIfValueMatches($name, $this->tokens[$name]); + unset($this->tokens[$name]); return $this; } @@ -85,27 +95,28 @@ public function releaseLock(string $name):LockManagerService */ public function lock(string $name, Closure $callback, int $lifetime = 3600) { - $result = null; + $result = null; + $acquired = false; Log::debug(sprintf("LockManagerService::lock name %s lifetime %s", $name, $lifetime)); - try - { + try { $this->acquireLock($name, $lifetime); + $acquired = true; 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 ($acquired) { + $this->releaseLock($name); + } } return $result; } diff --git a/app/Services/Utils/RedisCacheService.php b/app/Services/Utils/RedisCacheService.php index 22b9122b6..0336f9547 100644 --- a/app/Services/Utils/RedisCacheService.php +++ b/app/Services/Utils/RedisCacheService.php @@ -239,7 +239,7 @@ 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 ($conn->set($counter_name, 1, ['NX' => true]) !== null) { if ($ttl > 0) $conn->expire($counter_name, (int)$ttl); return 1; } @@ -306,12 +306,11 @@ 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 $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null; } - return $res; - }); + return $conn->set($key, $value, 'NX') !== null; + }, false); } public function setKeyExpiration($key, $ttl) @@ -331,7 +330,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/Unit/Services/LockManagerServiceOwnershipTest.php b/tests/Unit/Services/LockManagerServiceOwnershipTest.php new file mode 100644 index 000000000..bf682c2dc --- /dev/null +++ b/tests/Unit/Services/LockManagerServiceOwnershipTest.php @@ -0,0 +1,156 @@ +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. + } + + /** + * Calling releaseLock on a name that was never acquired must be a + * complete no-op — no Redis command issued, no exception thrown. + */ + public function testReleaseLockWithoutAcquireIsNoOp(): void + { + $cache = Mockery::mock(ICacheService::class); + $cache->shouldReceive('deleteIfValueMatches')->never(); + $cache->shouldReceive('delete')->never(); + + $service = new LockManagerService($cache); + $service->releaseLock('never.acquired.lock'); + + // Tokens map must still be empty — the no-op must not corrupt state. + $ref = new ReflectionClass($service); + $prop = $ref->getProperty('tokens'); + $prop->setAccessible(true); + $this->assertEmpty($prop->getValue($service)); + } + + /** + * After a full acquire → callback → release cycle the internal tokens + * map must be empty — no token leak that could cause a future + * releaseLock call to issue a stale deleteIfValueMatches. + */ + public function testTokensClearedAfterSuccessfulLockCycle(): void + { + $cache = Mockery::mock(ICacheService::class); + $cache->shouldReceive('addSingleValue')->once()->andReturn(true); + $cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); + + $service = new LockManagerService($cache); + $service->lock('test.lock', function () {}, 3600); + + $ref = new ReflectionClass($service); + $prop = $ref->getProperty('tokens'); + $prop->setAccessible(true); + $this->assertEmpty($prop->getValue($service), 'tokens map must be empty after release'); + } + + /** + * 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); + $cache->shouldReceive('addSingleValue') + ->once() + ->with('test.lock', Mockery::type('string'), 3600) + ->andReturn(true); + $cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); + + $service = new LockManagerService($cache); + $service->lock('test.lock', function () {}, 3600); + + // Tokens cleared — confirms the single addSingleValue call was paired + // with exactly one deleteIfValueMatches (not a separate expire call). + $ref = new ReflectionClass($service); + $prop = $ref->getProperty('tokens'); + $prop->setAccessible(true); + $this->assertEmpty($prop->getValue($service)); + } +} From 27acc78bf061dff0c55553fd6ffc4535e358e3de Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 15 Jun 2026 15:32:52 +0200 Subject: [PATCH 2/8] fix: review feedback Signed-off-by: romanetar --- app/Services/Model/Imp/SummitOrderService.php | 12 +-- app/Services/Utils/ILockManagerService.php | 13 +-- app/Services/Utils/LockManagerService.php | 41 ++++---- app/Services/Utils/RedisCacheService.php | 3 +- .../RedisCacheServiceAddSingleValueTest.php | 99 +++++++++++++++++++ .../LockManagerServiceOwnershipTest.php | 85 ++++++++-------- 6 files changed, 175 insertions(+), 78 deletions(-) create mode 100644 tests/Integration/RedisCacheServiceAddSingleValueTest.php diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 7a22d57c9..565b31413 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -830,7 +830,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 +868,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 +953,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 +970,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); }); } } @@ -1539,7 +1539,7 @@ 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', + $order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock', function () use ($promo_code_val, $type_id) { $attendee_email = $this->owner->getEmail(); @@ -1661,7 +1661,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 6c9220d38..74db3a20b 100644 --- a/app/Services/Utils/LockManagerService.php +++ b/app/Services/Utils/LockManagerService.php @@ -31,9 +31,6 @@ final class LockManagerService implements ILockManagerService { */ private $cache_service; - /** @var array lock-name → per-call ownership token */ - private array $tokens = []; - /** * LockManagerService constructor. * @param ICacheService $cache_service @@ -45,19 +42,21 @@ 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)); + if ($lifetime <= 0) { + throw new \InvalidArgumentException("Lock lifetime must be greater than zero seconds."); + } $token = bin2hex(random_bytes(16)); $attempt = 0; do { $success = $this->cache_service->addSingleValue($name, $token, $lifetime); if ($success) { - $this->tokens[$name] = $token; - return $this; + 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)); @@ -72,36 +71,30 @@ public function acquireLock(string $name, int $lifetime = 3600):LockManagerServi /** * @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)); - if (!isset($this->tokens[$name])) { - return $this; - } - $this->cache_service->deleteIfValueMatches($name, $this->tokens[$name]); - unset($this->tokens[$name]); - return $this; + $this->cache_service->deleteIfValueMatches($name, $token); } /** * @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 { - $result = null; - $acquired = false; + $token = null; + $result = null; Log::debug(sprintf("LockManagerService::lock name %s lifetime %s", $name, $lifetime)); try { - $this->acquireLock($name, $lifetime); - $acquired = true; + $token = $this->acquireLock($name, $lifetime); Log::debug(sprintf("LockManagerService::lock name %s calling callback", $name)); $result = $callback($this); } @@ -114,11 +107,11 @@ public function lock(string $name, Closure $callback, int $lifetime = 3600) throw $ex; } finally { - if ($acquired) { - $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 0336f9547..bdaeb2635 100644 --- a/app/Services/Utils/RedisCacheService.php +++ b/app/Services/Utils/RedisCacheService.php @@ -239,8 +239,7 @@ 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->set($counter_name, 1, ['NX' => true]) !== null) { - if ($ttl > 0) $conn->expire($counter_name, (int)$ttl); + if ($conn->set($counter_name, 1, ['EX' => (int)$ttl, 'NX' => true]) !== null) { return 1; } return (int)$conn->incr($counter_name); diff --git a/tests/Integration/RedisCacheServiceAddSingleValueTest.php b/tests/Integration/RedisCacheServiceAddSingleValueTest.php new file mode 100644 index 000000000..cdfbcd732 --- /dev/null +++ b/tests/Integration/RedisCacheServiceAddSingleValueTest.php @@ -0,0 +1,99 @@ +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' + ); + } +} diff --git a/tests/Unit/Services/LockManagerServiceOwnershipTest.php b/tests/Unit/Services/LockManagerServiceOwnershipTest.php index bf682c2dc..2cd226790 100644 --- a/tests/Unit/Services/LockManagerServiceOwnershipTest.php +++ b/tests/Unit/Services/LockManagerServiceOwnershipTest.php @@ -17,14 +17,13 @@ use libs\utils\ICacheService; use Mockery; use PHPUnit\Framework\TestCase; -use ReflectionClass; /** * Regression tests for the four bugs fixed in LockManagerService: * * 1. Non-atomic acquisition (setnx + expire → SET NX EX) * 2. Missing ownership token (timestamp value → random token) - * 3. Unconditional release in finally (guarded by $acquired flag) + * 3. Unconditional release in finally (guarded by $token !== null) * 4. Broken exponential backoff (^ XOR → ** power) * * These tests use a mock ICacheService so they run without Redis. @@ -90,43 +89,24 @@ public function testBobsFailedAcquireNeverDeletesAlicesKey(): void } /** - * Calling releaseLock on a name that was never acquired must be a - * complete no-op — no Redis command issued, no exception thrown. + * 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 testReleaseLockWithoutAcquireIsNoOp(): void - { - $cache = Mockery::mock(ICacheService::class); - $cache->shouldReceive('deleteIfValueMatches')->never(); - $cache->shouldReceive('delete')->never(); - - $service = new LockManagerService($cache); - $service->releaseLock('never.acquired.lock'); - - // Tokens map must still be empty — the no-op must not corrupt state. - $ref = new ReflectionClass($service); - $prop = $ref->getProperty('tokens'); - $prop->setAccessible(true); - $this->assertEmpty($prop->getValue($service)); - } - - /** - * After a full acquire → callback → release cycle the internal tokens - * map must be empty — no token leak that could cause a future - * releaseLock call to issue a stale deleteIfValueMatches. - */ - public function testTokensClearedAfterSuccessfulLockCycle(): void + 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); - $service->lock('test.lock', function () {}, 3600); + $service = new LockManagerService($cache); + $callbackRan = false; + $service->lock('test.lock', function () use (&$callbackRan) { + $callbackRan = true; + }, 3600); - $ref = new ReflectionClass($service); - $prop = $ref->getProperty('tokens'); - $prop->setAccessible(true); - $this->assertEmpty($prop->getValue($service), 'tokens map must be empty after release'); + $this->assertTrue($callbackRan, 'callback must execute inside the lock'); + // Mockery tearDown verifies addSingleValue and deleteIfValueMatches each fired once. } /** @@ -143,14 +123,39 @@ public function testAddSingleValueCalledOnceWithTokenAndLifetime(): void ->andReturn(true); $cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); - $service = new LockManagerService($cache); - $service->lock('test.lock', function () {}, 3600); + $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. + */ + 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); + + $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'); - // Tokens cleared — confirms the single addSingleValue call was paired - // with exactly one deleteIfValueMatches (not a separate expire call). - $ref = new ReflectionClass($service); - $prop = $ref->getProperty('tokens'); - $prop->setAccessible(true); - $this->assertEmpty($prop->getValue($service)); + // 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. } } From 69bd86b2c4be7ec807b0081d4c37e276a3a92e8a Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 3 Aug 2026 15:17:46 +0200 Subject: [PATCH 3/8] fix: PR review feedback Signed-off-by: romanetar --- .github/workflows/push.yml | 5 ++- app/Services/Model/Imp/SummitOrderService.php | 27 ++++++++----- app/Services/Utils/LockManagerService.php | 6 ++- app/Services/Utils/RedisCacheService.php | 6 ++- .../RedisCacheServiceAddSingleValueTest.php | 40 ++++++++++++++++++- .../LockManagerServiceOwnershipTest.php | 5 ++- 6 files changed, 72 insertions(+), 17 deletions(-) diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 0e665c5b0..10541eae6 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -64,12 +64,13 @@ 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/" } env: OTEL_SERVICE_ENABLED: false APP_ENV: testing diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 565b31413..1077ab0b0 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 )); @@ -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 @@ -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."); diff --git a/app/Services/Utils/LockManagerService.php b/app/Services/Utils/LockManagerService.php index 74db3a20b..cb4e2c403 100644 --- a/app/Services/Utils/LockManagerService.php +++ b/app/Services/Utils/LockManagerService.php @@ -76,7 +76,11 @@ public function acquireLock(string $name, int $lifetime = 3600): string public function releaseLock(string $name, string $token): void { Log::debug(sprintf("LockManagerService::releaseLock name %s", $name)); - $this->cache_service->deleteIfValueMatches($name, $token); + $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'); + } } /** diff --git a/app/Services/Utils/RedisCacheService.php b/app/Services/Utils/RedisCacheService.php index bdaeb2635..23e296bb4 100644 --- a/app/Services/Utils/RedisCacheService.php +++ b/app/Services/Utils/RedisCacheService.php @@ -239,8 +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->set($counter_name, 1, ['EX' => (int)$ttl, 'NX' => true]) !== null) { - return 1; + if ($ttl > 0) { + if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1; + } else { + if ($conn->set($counter_name, 1, 'NX') !== null) return 1; } return (int)$conn->incr($counter_name); }, 0); diff --git a/tests/Integration/RedisCacheServiceAddSingleValueTest.php b/tests/Integration/RedisCacheServiceAddSingleValueTest.php index cdfbcd732..1bf450c22 100644 --- a/tests/Integration/RedisCacheServiceAddSingleValueTest.php +++ b/tests/Integration/RedisCacheServiceAddSingleValueTest.php @@ -19,9 +19,10 @@ use Tests\TestCase; /** - * Integration tests for RedisCacheService::addSingleValue. + * Integration tests for RedisCacheService::addSingleValue and + * RedisCacheService::deleteIfValueMatches. * - * These tests require a live Redis instance and verify two properties that + * These tests require a live Redis instance and verify properties that * mocks cannot exercise: * * 1. Driver compatibility — the variadic SET NX EX form works with the @@ -33,6 +34,12 @@ * window where the key exists without a TTL. Verified by reading TTL * immediately after addSingleValue returns. * + * 3. Ownership — deleteIfValueMatches (the Lua compare-and-delete the lock's + * ownership guarantee rests on) only deletes the key when the token + * matches, and never touches a key it does not own. A broken script or a + * driver change breaking the eval($lua, 1, $key, $value) signature would + * silently no-op every release, holding all locks to full TTL. + * */ #[Group("integration")] final class RedisCacheServiceAddSingleValueTest extends TestCase @@ -96,4 +103,33 @@ public function testAddSingleValueReturnsTrueAfterKeyIsDeleted(): void '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'); + } } diff --git a/tests/Unit/Services/LockManagerServiceOwnershipTest.php b/tests/Unit/Services/LockManagerServiceOwnershipTest.php index 2cd226790..5f8b71993 100644 --- a/tests/Unit/Services/LockManagerServiceOwnershipTest.php +++ b/tests/Unit/Services/LockManagerServiceOwnershipTest.php @@ -137,7 +137,8 @@ public function testAddSingleValueCalledOnceWithTokenAndLifetime(): void * 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. + * 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 { @@ -145,6 +146,7 @@ public function testReleaseLockWhenRedisDownLeavesKeyUntilTtl(): void $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; @@ -157,5 +159,6 @@ public function testReleaseLockWhenRedisDownLeavesKeyUntilTtl(): void // 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. } } From 01850d775cc55ab0083b889eab1376424d69e834 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 10 Aug 2026 20:20:51 +0200 Subject: [PATCH 4/8] fix(orders): capture ticket_dto in AutoAssignPrePaidTicketTask closure The inner lock() closure read $ticket_dto['attendee_company'/'attendee_first_name'/ 'attendee_last_name'] but never captured it via use(), so PHP treated it as undefined and every read silently fell back to the order owner's own profile instead of the attendee actually being assigned the ticket. Adds a regression test that reproduces the RED/GREEN pair from the review. Co-Authored-By: Claude Sonnet 5 --- app/Services/Model/Imp/SummitOrderService.php | 2 +- tests/SummitOrderServiceTest.php | 67 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 1077ab0b0..922cdc1f0 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -1549,7 +1549,7 @@ public function run(array $formerState): array $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) { + function () use ($promo_code_val, $type_id, $ticket_dto) { $attendee_email = $this->owner->getEmail(); // use what we have on payload first 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 From 211f1df445c9c7ca310f294f5ceaec4b84dcf4fc Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 10 Aug 2026 20:27:24 +0200 Subject: [PATCH 5/8] fix(cache): detect SET...NX miss consistently across Predis/PhpRedis Predis returns null on a SET...NX miss, but PhpRedis's C extension returns false for the same variadic form; `!== null` treats the PhpRedis miss as a success. addSingleValue and incCounter both used this check, so under REDIS_CLIENT=phpredis a second caller racing for an already-held lock would report success while the first token still owns it, defeating LockManagerService's mutual exclusion, and incCounter's lock_manager.release_mismatch counter would stick at 1 instead of incrementing. Adds a shared setNxSucceeded() helper that excludes both drivers' failure sentinels, an incCounter regression test, and runs the Integration suite under REDIS_CLIENT=phpredis in CI so this can't regress silently again. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/push.yml | 6 ++++++ app/Services/Utils/RedisCacheService.php | 18 ++++++++++++++---- .../RedisCacheServiceAddSingleValueTest.php | 16 ++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 10541eae6..a2330f792 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -71,6 +71,10 @@ jobs: - { 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 @@ -175,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/app/Services/Utils/RedisCacheService.php b/app/Services/Utils/RedisCacheService.php index 23e296bb4..f54582a98 100644 --- a/app/Services/Utils/RedisCacheService.php +++ b/app/Services/Utils/RedisCacheService.php @@ -240,9 +240,9 @@ public function incCounter($counter_name, $ttl = 0) { return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) { if ($ttl > 0) { - if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1; + if ($this->setNxSucceeded($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX'))) return 1; } else { - if ($conn->set($counter_name, 1, 'NX') !== null) return 1; + if ($this->setNxSucceeded($conn->set($counter_name, 1, 'NX'))) return 1; } return (int)$conn->incr($counter_name); }, 0); @@ -308,12 +308,22 @@ public function addSingleValue($key, $value, $ttl = 0) { return $this->retryOnConnectionError(function ($conn) use ($key, $value, $ttl) { if ($ttl > 0) { - return $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null; + return $this->setNxSucceeded($conn->set($key, $value, 'EX', (int)$ttl, 'NX')); } - return $conn->set($key, $value, 'NX') !== null; + 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) { return $this->retryOnConnectionError(function ($conn) use ($key, $ttl) { diff --git a/tests/Integration/RedisCacheServiceAddSingleValueTest.php b/tests/Integration/RedisCacheServiceAddSingleValueTest.php index 1bf450c22..3d3dd6185 100644 --- a/tests/Integration/RedisCacheServiceAddSingleValueTest.php +++ b/tests/Integration/RedisCacheServiceAddSingleValueTest.php @@ -132,4 +132,20 @@ public function testDeleteIfValueMatchesLeavesKeyIntactOnMismatch(): void $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)); + } } From d474455755d124a9204efcbb66acfecb9137507e Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 10 Aug 2026 20:30:56 +0200 Subject: [PATCH 6/8] test(lock): assert deleteIfValueMatches receives the exact acquired token testAddSingleValueCalledOnceWithTokenAndLifetime only checked that addSingleValue got some string token; deleteIfValueMatches had no constraint at all, so a future refactor that broke token threading between acquireLock and releaseLock would pass this suite undetected. Captures the token from addSingleValue and asserts deleteIfValueMatches receives that same value, verified by injecting a token-threading regression locally and confirming the test catches it. Co-Authored-By: Claude Sonnet 5 --- .../LockManagerServiceOwnershipTest.php | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/tests/Unit/Services/LockManagerServiceOwnershipTest.php b/tests/Unit/Services/LockManagerServiceOwnershipTest.php index 5f8b71993..121a2cb06 100644 --- a/tests/Unit/Services/LockManagerServiceOwnershipTest.php +++ b/tests/Unit/Services/LockManagerServiceOwnershipTest.php @@ -117,11 +117,28 @@ public function testSuccessfulLockCyclePairsAcquireAndRelease(): void public function testAddSingleValueCalledOnceWithTokenAndLifetime(): void { $cache = Mockery::mock(ICacheService::class); + $token = null; $cache->shouldReceive('addSingleValue') ->once() - ->with('test.lock', Mockery::type('string'), 3600) + ->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); - $cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); $service = new LockManagerService($cache); $callbackRan = false; From b8d88a7824a58ebb661c1d2609fa456eac51952e Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 17 Aug 2026 14:55:38 +0200 Subject: [PATCH 7/8] fix(test): remove redundant CreatesApplication trait usage Tests\TestCase already applies the trait; re-declaring it on the subclass is dead weight. Addresses the outstanding PR #537 review comment on this file. --- tests/Integration/RedisCacheServiceAddSingleValueTest.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/Integration/RedisCacheServiceAddSingleValueTest.php b/tests/Integration/RedisCacheServiceAddSingleValueTest.php index 3d3dd6185..df50fc637 100644 --- a/tests/Integration/RedisCacheServiceAddSingleValueTest.php +++ b/tests/Integration/RedisCacheServiceAddSingleValueTest.php @@ -15,7 +15,6 @@ use Illuminate\Support\Facades\Redis; use PHPUnit\Framework\Attributes\Group; use services\utils\RedisCacheService; -use Tests\CreatesApplication; use Tests\TestCase; /** @@ -44,8 +43,6 @@ #[Group("integration")] final class RedisCacheServiceAddSingleValueTest extends TestCase { - use CreatesApplication; - private const TEST_KEY = 'test:add_single_value:lock'; private const TTL = 30; From a48a60082ba551aeee9a07e24ff06f279a466ba5 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 17 Aug 2026 15:24:22 +0200 Subject: [PATCH 8/8] fix(sponsor-users): serialize addSponsorUser/addSponsorUserToGroup per pair AddSponsorMemberMQJob and UpdateSponsorMemberGroupsMQJob can both observe a missing Sponsor_Users row for the same (sponsor, external user) pair - Sponsor::addUser's contains() guard is an unlocked read - and both insert, leaving two rows whose Permissions JSON diverges and never reconverges. Wrap both entry points in an ILockManagerService lock keyed on sponsor_user.{sponsor_id}.ext_{user_id}.lock, held outside the transaction so a waiter never observes the row as still-missing before commit. UnacquiredLockException is left to propagate like any other failure here, consuming one of the MQ job's retries rather than being swallowed. Closes #583. --- .../Model/Imp/SponsorUserSyncService.php | 199 ++++++++++++------ .../SponsorUserPermissionTrackingTest.php | 77 +++++++ 2 files changed, 206 insertions(+), 70 deletions(-) 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/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); + } + } }