fix(lock): implement Redlock single-instance pattern in LockManagerService #537
+219
−29
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)); | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Missing test: Redis unavailable during There is no test for the case where Suggested outline: public function testReleaseLockWhenRedisDownLeavesKeyUntilTtl(): void
{
$cache = Mockery::mock(ICacheService::class);
$cache->shouldReceive('addSingleValue')->once()->andReturn(true);
// simulate Redis unavailable — deleteIfValueMatches returns false
$cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(false);
$lock = new LockManagerService($cache);
$lock->lock('resource.lock', fn() => null);
// Observable effect: a subsequent acquireLock on the same key will fail
// because the key was NOT deleted — only TTL expiry can free it.
}Documenting this as a test makes the failure mode explicit for future readers. |
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutable per-acquisition state on a singleton service
LockManagerServiceis bound as a singleton (BaseServicesProvider.php:143). Before this PR the service was stateless; this line introduces the first mutable instance state. In the current stack (PHP-FPM, synchronous queue workers) this is safe because only one coroutine runs at a time per process. However, if Laravel Octane / Swoole is ever introduced, two concurrent coroutines racing on the same key name would overwrite each other's token here — coroutine A'sreleaseLockwould present coroutine B's token todeleteIfValueMatches, releasing B's live lock.Suggested mitigation: either change the binding to
scoped(Octane-safe per-request singleton), or remove instance state entirely by returning the token fromacquireLockand accepting it as a parameter inreleaseLock.