From c6e6473cf6995702b0fce71fe1fe8d196454bbea Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 4 May 2026 16:32:18 +0200 Subject: [PATCH 1/2] 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 fbe981c732b19faaf972ea6428a505a0090769f9 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 15 Jun 2026 15:32:52 +0200 Subject: [PATCH 2/2] 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 bff004ce2..5f2ecf572 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -827,7 +827,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 @@ -865,7 +865,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); }); } @@ -950,7 +950,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); } }); @@ -967,7 +967,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); }); } } @@ -1536,7 +1536,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(); @@ -1658,7 +1658,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. } }