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..1349b5951 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, ['NX' => true, 'EX' => (int)$ttl]) !== null; } - return $res; - }); + return $conn->set($key, $value, ['NX' => true]) !== null; + }, false); } public function setKeyExpiration($key, $ttl) @@ -332,6 +331,20 @@ public function 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)); + } +}