From 3f6f8b0adb6698736d50d6b1a1f7c96417dbee09 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 4 May 2026 16:32:18 +0200 Subject: [PATCH 01/10] 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 face963b1aeb38054273469c9225f679d982a0ea Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 15 Jun 2026 15:32:52 +0200 Subject: [PATCH 02/10] 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 84e9fe9d8..6cee42369 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -834,7 +834,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 @@ -872,7 +872,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); }); } @@ -957,7 +957,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); } }); @@ -974,7 +974,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); }); } } @@ -1543,7 +1543,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(); @@ -1665,7 +1665,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 3edd45a635a8a8806c081d09e3175086acc7ebf2 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 3 Aug 2026 15:17:46 +0200 Subject: [PATCH 03/10] 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 559568a0b..4e234f07f 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -75,12 +75,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 tests/PresentationSubmissionReopenedEmailTest.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 6cee42369..7a1e4f7d0 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -268,6 +268,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 )); @@ -1481,6 +1482,11 @@ final class AutoAssignPrePaidTicketTask extends AbstractTask */ private $ticket_type_repository; + /** + * @var ISummitRegistrationPromoCodeRepository + */ + private $promo_code_repository; + /** * @var ILockManagerService */ @@ -1494,19 +1500,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; @@ -1517,6 +1525,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 @@ -1562,7 +1571,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 ab4a59a36ae6b111b13a4bf2d65c920126a2589b Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 10 Aug 2026 20:20:51 +0200 Subject: [PATCH 04/10] 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 7a1e4f7d0..0c216b7c6 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -1553,7 +1553,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 1fdd32415..85f1640bb 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -387,6 +387,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 46ea9b3f61df8fb47bcd6c37915d3e43e730ce9a Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 10 Aug 2026 20:27:24 +0200 Subject: [PATCH 05/10] 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 4e234f07f..2803f53cb 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -82,6 +82,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 @@ -188,6 +192,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 03c5e8fcb80a7213f4a8c512538bb374c2cf79bc Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 10 Aug 2026 20:30:56 +0200 Subject: [PATCH 06/10] 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 59f19fcfb56d334cbd7e0f767065af26284a692c Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 17 Aug 2026 14:55:38 +0200 Subject: [PATCH 07/10] 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 3c49aa00b472a478ccb0c522211664895e8705a5 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 17 Aug 2026 15:24:22 +0200 Subject: [PATCH 08/10] 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 18a62e894..dcb915973 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; @@ -1031,4 +1033,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); + } + } } From 2d598ebb885842420dbfea0ab99b586cf28c1f87 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 31 Aug 2026 14:31:03 -0300 Subject: [PATCH 09/10] fix(ci): run Repositories and Services suites by path so PHPUnit executes them PHPUnit's --filter matches test/class names, not directories, so '--filter tests/Unit/Services/' and '--filter tests/Repositories/' print 'No tests executed!' and exit 0 - both jobs were green while running zero tests, including this branch's own regression suites (LockManagerServiceOwnershipTest, SponsorUserPermissionTrackingTest). --- .github/workflows/push.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 2803f53cb..dc58a7100 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -79,8 +79,8 @@ jobs: # 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 tests/PresentationSubmissionReopenedEmailTest.php" } - - { name: "Repositories", filter: "--filter tests/Repositories/" } - - { name: "Services", filter: "--filter tests/Unit/Services/" } + - { name: "Repositories", filter: "tests/Repositories/" } + - { name: "Services", 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), From 290357fd03b6f922f0c7887e0295b477d5eeb1b0 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 31 Aug 2026 14:52:39 -0300 Subject: [PATCH 10/10] fix(sponsor-users): raise pair-lock lifetime above the IDP call timeout Both addSponsorUser and addSponsorUserToGroup resolve the member inside the lock, which for a brand-new user makes a synchronous IDP HTTP call bounded by curl.timeout (60s default). A 30s lifetime could expire mid-callback on a slow IDP, right as the competing MQ job's first retry lands (RetryBackoff starts at 30s), silently dissolving the mutual exclusion the lock exists to provide. 120s exceeds the IDP timeout while staying below the job's last retry at +150s, so a worker killed mid-callback still frees the pair before the final attempt. --- app/Services/Model/Imp/SponsorUserSyncService.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 9fb376a24..bb4deb8ac 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -59,10 +59,16 @@ final class SponsorUserSyncService /** * 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. + * addSponsorUser / addSponsorUserToGroup. Both paths resolve the member + * INSIDE the lock, which for a brand-new user makes a synchronous IDP + * call (registerExternalUserById) bounded by curl.timeout (60s default) - + * so the lifetime must exceed that, or a slow IDP dissolves the lock right + * as the competing MQ job's first retry lands (RetryBackoff starts at 30s). + * It must also stay below the job's LAST retry (+150s with tries=3 and + * backoff '30,120'), so a worker killed mid-callback leaves a lock that + * expires before the final retry rather than burning it too. */ - private const SponsorUserLockLifetime = 30; + private const SponsorUserLockLifetime = 120; /** * SponsorUserSyncService constructor.