Skip to content

Commit c6e6473

Browse files
committed
fix(lock): implement Redlock single-instance pattern in LockManagerService
Signed-off-by: romanetar <roman_ag@hotmail.com>
1 parent 34e47f9 commit c6e6473

4 files changed

Lines changed: 219 additions & 29 deletions

File tree

Libs/Utils/ICacheService.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,16 @@ public function setSingleValue($key, $value, $ttl = 0);
9696
*/
9797
public function addSingleValue($key, $value, $ttl = 0);
9898

99+
/**
100+
* Atomically compare-and-delete: DEL the key only when its current value
101+
* equals $expectedValue. Implementations MUST use an atomic operation
102+
* (Lua EVAL or equivalent) — never a separate GET + conditional DEL.
103+
* @param string $key
104+
* @param string $expectedValue
105+
* @return bool true iff the key existed, matched, and was deleted
106+
*/
107+
public function deleteIfValueMatches(string $key, string $expectedValue): bool;
108+
99109
/**
100110
* Set time to live to a given key
101111
* @param $key

app/Services/Utils/LockManagerService.php

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,18 @@
2222
*/
2323
final class LockManagerService implements ILockManagerService {
2424

25-
const MaxRetries = 3;
25+
const MaxRetries = 3;
2626
const BackOffMultiplier = 2.0;
27-
const BackOffBaseInterval = 100000; // 1 ms
27+
const BackOffBaseInterval = 100000; // microseconds
28+
2829
/**
2930
* @var ICacheService
3031
*/
3132
private $cache_service;
3233

34+
/** @var array<string,string> lock-name → per-call ownership token */
35+
private array $tokens = [];
36+
3337
/**
3438
* LockManagerService constructor.
3539
* @param ICacheService $cache_service
@@ -46,22 +50,24 @@ public function __construct(ICacheService $cache_service){
4650
*/
4751
public function acquireLock(string $name, int $lifetime = 3600):LockManagerService
4852
{
49-
Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s",$name, $lifetime));
50-
$attempt = 0 ;
53+
Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s", $name, $lifetime));
54+
$token = bin2hex(random_bytes(16));
55+
$attempt = 0;
5156
do {
52-
$time = time() + $lifetime + 1;
53-
$success = $this->cache_service->addSingleValue($name, $time, $time);
54-
if($success) return $this;
55-
$wait_interval = self::BackOffBaseInterval * ( self::BackOffMultiplier ^ $attempt );
56-
Log::debug(sprintf("LockManagerService::acquireLock name %s retrying in %s microseconds (%s).", $name, $wait_interval, $attempt));
57+
$success = $this->cache_service->addSingleValue($name, $token, $lifetime);
58+
if ($success) {
59+
$this->tokens[$name] = $token;
60+
return $this;
61+
}
62+
$wait_interval = (int)(self::BackOffBaseInterval * (self::BackOffMultiplier ** $attempt));
63+
Log::debug(sprintf("LockManagerService::acquireLock name %s retrying in %s µs (attempt %s)", $name, $wait_interval, $attempt));
5764
usleep($wait_interval);
58-
if($attempt >= (self::MaxRetries - 1 )) {
59-
// only one time we could use this handle
65+
if ($attempt >= (self::MaxRetries - 1)) {
6066
Log::error(sprintf("LockManagerService::acquireLock name %s lifetime %s ERROR MAX RETRIES attempt %s", $name, $lifetime, $attempt));
6167
throw new UnacquiredLockException(sprintf("lock name %s", $name));
6268
}
6369
++$attempt;
64-
} while(1);
70+
} while (1);
6571
}
6672

6773
/**
@@ -70,8 +76,12 @@ public function acquireLock(string $name, int $lifetime = 3600):LockManagerServi
7076
*/
7177
public function releaseLock(string $name):LockManagerService
7278
{
73-
Log::debug(sprintf("LockManagerService::releaseLock name %s",$name));
74-
$this->cache_service->delete($name);
79+
Log::debug(sprintf("LockManagerService::releaseLock name %s", $name));
80+
if (!isset($this->tokens[$name])) {
81+
return $this;
82+
}
83+
$this->cache_service->deleteIfValueMatches($name, $this->tokens[$name]);
84+
unset($this->tokens[$name]);
7585
return $this;
7686
}
7787

@@ -85,27 +95,28 @@ public function releaseLock(string $name):LockManagerService
8595
*/
8696
public function lock(string $name, Closure $callback, int $lifetime = 3600)
8797
{
88-
$result = null;
98+
$result = null;
99+
$acquired = false;
89100
Log::debug(sprintf("LockManagerService::lock name %s lifetime %s", $name, $lifetime));
90101

91-
try
92-
{
102+
try {
93103
$this->acquireLock($name, $lifetime);
104+
$acquired = true;
94105
Log::debug(sprintf("LockManagerService::lock name %s calling callback", $name));
95106
$result = $callback($this);
96107
}
97-
catch(UnacquiredLockException $ex)
98-
{
108+
catch(UnacquiredLockException $ex) {
99109
Log::warning($ex);
100110
throw $ex;
101111
}
102-
catch(Exception $ex)
103-
{
112+
catch(Exception $ex) {
104113
Log::error($ex);
105114
throw $ex;
106115
}
107116
finally {
108-
$this->releaseLock($name);
117+
if ($acquired) {
118+
$this->releaseLock($name);
119+
}
109120
}
110121
return $result;
111122
}

app/Services/Utils/RedisCacheService.php

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ public function storeHash($name, array $values, $ttl = 0)
239239
public function incCounter($counter_name, $ttl = 0)
240240
{
241241
return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) {
242-
if ($conn->setnx($counter_name, 1)) {
242+
if ($conn->set($counter_name, 1, ['NX' => true]) !== null) {
243243
if ($ttl > 0) $conn->expire($counter_name, (int)$ttl);
244244
return 1;
245245
}
@@ -306,12 +306,11 @@ public function setSingleValue($key, $value, $ttl = 0)
306306
public function addSingleValue($key, $value, $ttl = 0)
307307
{
308308
return $this->retryOnConnectionError(function ($conn) use ($key, $value, $ttl) {
309-
$res = $conn->setnx($key, $value);
310-
if ($res && $ttl > 0) {
311-
$conn->expire($key, $ttl);
309+
if ($ttl > 0) {
310+
return $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null;
312311
}
313-
return $res;
314-
});
312+
return $conn->set($key, $value, 'NX') !== null;
313+
}, false);
315314
}
316315

317316
public function setKeyExpiration($key, $ttl)
@@ -331,7 +330,21 @@ public function ttl($key)
331330
return (int)$conn->ttl($key);
332331
}, 0);
333332
}
334-
333+
334+
public function deleteIfValueMatches(string $key, string $expectedValue): bool
335+
{
336+
$lua = <<<'LUA'
337+
if redis.call('get', KEYS[1]) == ARGV[1] then
338+
return redis.call('del', KEYS[1])
339+
else
340+
return 0
341+
end
342+
LUA;
343+
return $this->retryOnConnectionError(function ($conn) use ($lua, $key, $expectedValue) {
344+
return (int)$conn->eval($lua, 1, $key, $expectedValue) === 1;
345+
}, false);
346+
}
347+
335348
/**
336349
* @param string $cache_region_key
337350
* @return void
Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
<?php namespace Tests\Unit\Services;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use App\Services\Utils\Exceptions\UnacquiredLockException;
16+
use App\Services\Utils\LockManagerService;
17+
use libs\utils\ICacheService;
18+
use Mockery;
19+
use PHPUnit\Framework\TestCase;
20+
use ReflectionClass;
21+
22+
/**
23+
* Regression tests for the four bugs fixed in LockManagerService:
24+
*
25+
* 1. Non-atomic acquisition (setnx + expire → SET NX EX)
26+
* 2. Missing ownership token (timestamp value → random token)
27+
* 3. Unconditional release in finally (guarded by $acquired flag)
28+
* 4. Broken exponential backoff (^ XOR → ** power)
29+
*
30+
* These tests use a mock ICacheService so they run without Redis.
31+
* The Alice/Bob scenario demonstrates that a failed acquirer never
32+
* deletes another process's lock key.
33+
*/
34+
class LockManagerServiceOwnershipTest extends TestCase
35+
{
36+
protected function setUp(): void
37+
{
38+
parent::setUp();
39+
\Illuminate\Support\Facades\Facade::clearResolvedInstances();
40+
$container = new \Illuminate\Container\Container();
41+
$container->instance('app', $container);
42+
$container->instance('log', new class {
43+
public function __call($name, $args) {}
44+
});
45+
\Illuminate\Support\Facades\Facade::setFacadeApplication($container);
46+
}
47+
48+
protected function tearDown(): void
49+
{
50+
\Illuminate\Support\Facades\Facade::clearResolvedInstances();
51+
\Illuminate\Support\Facades\Facade::setFacadeApplication(null);
52+
Mockery::close();
53+
parent::tearDown();
54+
}
55+
56+
/**
57+
* Alice holds the lock. Bob's acquire exhausts retries and throws
58+
* UnacquiredLockException. Bob's lock() finally block must NOT call
59+
* deleteIfValueMatches — Bob never owned the key and must not delete it.
60+
*
61+
* On main (before fix) this test fails because releaseLock was called
62+
* unconditionally from the finally block.
63+
*/
64+
public function testBobsFailedAcquireNeverDeletesAlicesKey(): void
65+
{
66+
// Alice: acquires once, releases once via deleteIfValueMatches.
67+
$aliceCache = Mockery::mock(ICacheService::class);
68+
$aliceCache->shouldReceive('addSingleValue')->once()->andReturn(true);
69+
$aliceCache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true);
70+
71+
// Bob: fails to acquire on every retry; must never touch Redis for release.
72+
$bobCache = Mockery::mock(ICacheService::class);
73+
$bobCache->shouldReceive('addSingleValue')
74+
->times(LockManagerService::MaxRetries)
75+
->andReturn(false);
76+
$bobCache->shouldReceive('deleteIfValueMatches')->never();
77+
78+
$alice = new LockManagerService($aliceCache);
79+
$bob = new LockManagerService($bobCache);
80+
81+
$alice->lock('resource.lock', function () {
82+
// Alice's critical section.
83+
});
84+
85+
$this->expectException(UnacquiredLockException::class);
86+
$bob->lock('resource.lock', function () {
87+
$this->fail('Bob must not enter the critical section.');
88+
});
89+
// Mockery tearDown asserts deleteIfValueMatches was never called on $bobCache.
90+
}
91+
92+
/**
93+
* Calling releaseLock on a name that was never acquired must be a
94+
* complete no-op — no Redis command issued, no exception thrown.
95+
*/
96+
public function testReleaseLockWithoutAcquireIsNoOp(): void
97+
{
98+
$cache = Mockery::mock(ICacheService::class);
99+
$cache->shouldReceive('deleteIfValueMatches')->never();
100+
$cache->shouldReceive('delete')->never();
101+
102+
$service = new LockManagerService($cache);
103+
$service->releaseLock('never.acquired.lock');
104+
105+
// Tokens map must still be empty — the no-op must not corrupt state.
106+
$ref = new ReflectionClass($service);
107+
$prop = $ref->getProperty('tokens');
108+
$prop->setAccessible(true);
109+
$this->assertEmpty($prop->getValue($service));
110+
}
111+
112+
/**
113+
* After a full acquire → callback → release cycle the internal tokens
114+
* map must be empty — no token leak that could cause a future
115+
* releaseLock call to issue a stale deleteIfValueMatches.
116+
*/
117+
public function testTokensClearedAfterSuccessfulLockCycle(): void
118+
{
119+
$cache = Mockery::mock(ICacheService::class);
120+
$cache->shouldReceive('addSingleValue')->once()->andReturn(true);
121+
$cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true);
122+
123+
$service = new LockManagerService($cache);
124+
$service->lock('test.lock', function () {}, 3600);
125+
126+
$ref = new ReflectionClass($service);
127+
$prop = $ref->getProperty('tokens');
128+
$prop->setAccessible(true);
129+
$this->assertEmpty($prop->getValue($service), 'tokens map must be empty after release');
130+
}
131+
132+
/**
133+
* Structural assertion: addSingleValue is called exactly once per
134+
* acquisition attempt (not two separate calls for setnx + expire).
135+
* The call must carry the lock name, a string token, and the lifetime.
136+
*/
137+
public function testAddSingleValueCalledOnceWithTokenAndLifetime(): void
138+
{
139+
$cache = Mockery::mock(ICacheService::class);
140+
$cache->shouldReceive('addSingleValue')
141+
->once()
142+
->with('test.lock', Mockery::type('string'), 3600)
143+
->andReturn(true);
144+
$cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true);
145+
146+
$service = new LockManagerService($cache);
147+
$service->lock('test.lock', function () {}, 3600);
148+
149+
// Tokens cleared — confirms the single addSingleValue call was paired
150+
// with exactly one deleteIfValueMatches (not a separate expire call).
151+
$ref = new ReflectionClass($service);
152+
$prop = $ref->getProperty('tokens');
153+
$prop->setAccessible(true);
154+
$this->assertEmpty($prop->getValue($service));
155+
}
156+
}

0 commit comments

Comments
 (0)