OpenStackweb · romanetar · May 4, 2026
diff --git 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
@@ -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<string,string>  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
@@ -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
@@ -0,0 +1,156 @@
+<?php namespace Tests\Unit\Services;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Services\Utils\Exceptions\UnacquiredLockException;
+use App\Services\Utils\LockManagerService;
+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)
+ * 4. Broken exponential backoff (^ XOR → ** power)
+ *
+ * These tests use a mock ICacheService so they run without Redis.
+ * The Alice/Bob scenario demonstrates that a failed acquirer never
+ * deletes another process's lock key.
+ */
+class LockManagerServiceOwnershipTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+        \Illuminate\Support\Facades\Facade::clearResolvedInstances();
+        $container = new \Illuminate\Container\Container();
+        $container->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));
+    }
+}