fix(lock): implement Redlock single-instance pattern in LockManagerService#537
fix(lock): implement Redlock single-instance pattern in LockManagerService#537romanetar wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR refactors distributed locking to use atomic ownership tokens and compare-and-delete semantics. It adds ChangesOwnership-Based Distributed Locking
Sequence DiagramsequenceDiagram
participant Client
participant LMS as LockManagerService
participant RCS as RedisCacheService
participant Redis
rect rgba(100, 150, 200, 0.5)
Note over Client,Redis: Lock Acquisition with Ownership Token
Client->>LMS: acquireLock(name, lifetime)
LMS->>LMS: token = bin2hex(random_bytes(16))
loop Exponential Backoff Retry Loop
LMS->>RCS: addSingleValue(name, token, lifetime)
RCS->>Redis: SET name token NX EX lifetime
Redis-->>RCS: OK or nil
alt First attempt succeeds
RCS-->>LMS: true
LMS->>LMS: Store tokens[name] = token
LMS-->>Client: Return token
else Retry needed
RCS-->>LMS: false
LMS->>LMS: Sleep exponential backoff microseconds
end
end
end
rect rgba(200, 150, 100, 0.5)
Note over Client,Redis: Lock Release with Ownership Verification
Client->>LMS: releaseLock(name, token)
LMS->>LMS: Check tokens[name] == token?
alt Token owned
LMS->>RCS: deleteIfValueMatches(name, token)
RCS->>Redis: EVAL Lua (GET==token? DEL : noop)
Redis->>Redis: GET name == token?
alt Values match
Redis->>Redis: DEL name
Redis-->>RCS: 1
else Mismatch
Redis-->>RCS: 0
end
RCS-->>LMS: boolean
LMS->>LMS: Unset tokens[name]
LMS-->>Client: Released
else Not owned
LMS-->>Client: Return (no-op)
end
end
rect rgba(150, 200, 100, 0.5)
Note over Client,Redis: Wrapper Guards Release on Failure
Client->>LMS: lock(name, callback, lifetime)
LMS->>LMS: acquireLock -> success? (token acquired)
alt Acquisition succeeded
LMS->>Client: Execute callback
LMS->>LMS: finally: releaseLock(name, token)
else Acquisition failed
LMS->>LMS: finally: skip release (token is null)
LMS->>Client: Throw UnacquiredLockException
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Services/Utils/LockManagerService.php (1)
62-70: 💤 Low valueMinor: Unnecessary sleep before throwing on final retry.
When
attempt >= MaxRetries - 1, the code still executesusleepbefore throwing. This adds ~400ms of unnecessary delay on the final failed attempt.Consider moving the retry check before the sleep:
Suggested reorder
- $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)) { 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)); } + $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); ++$attempt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Utils/LockManagerService.php` around lines 62 - 70, The loop in LockManagerService::acquireLock sleeps (usleep) even when $attempt >= (self::MaxRetries - 1), causing an unnecessary delay before throwing UnacquiredLockException; reorder the logic so the check for final retry (if $attempt >= (self::MaxRetries - 1)) occurs before calling usleep and before incrementing $attempt, log and throw immediately on final attempt, otherwise perform the usleep, increment $attempt and continue the loop to preserve backoff behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Services/Utils/LockManagerService.php`:
- Around line 62-70: The loop in LockManagerService::acquireLock sleeps (usleep)
even when $attempt >= (self::MaxRetries - 1), causing an unnecessary delay
before throwing UnacquiredLockException; reorder the logic so the check for
final retry (if $attempt >= (self::MaxRetries - 1)) occurs before calling usleep
and before incrementing $attempt, log and throw immediately on final attempt,
otherwise perform the usleep, increment $attempt and continue the loop to
preserve backoff behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4fb76b4-55ed-4ec3-a248-fdf0d070fc95
📒 Files selected for processing (4)
Libs/Utils/ICacheService.phpapp/Services/Utils/LockManagerService.phpapp/Services/Utils/RedisCacheService.phptests/Unit/Services/LockManagerServiceOwnershipTest.php
romanetar
force-pushed
the
fix/release-on-failed-acquire-and-add-ownership-tokens
branch
from
b3dbd7a to
acb8447
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Services/LockManagerServiceOwnershipTest.php (1)
139-144: 💤 Low valueConsider asserting the token is non-empty for stronger ownership guarantee coverage.
Mockery::type('string')accepts any string including''. A complementary assertion withMockery::on(fn($v) => strlen($v) >= 16)(or similar) would confirm the service is actually generating a meaningful random token rather than an empty or trivial value.♻️ Tighter token constraint
- ->with('test.lock', Mockery::type('string'), 3600) + ->with('test.lock', Mockery::on(fn(string $v) => strlen($v) >= 16), 3600)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Services/LockManagerServiceOwnershipTest.php` around lines 139 - 144, Replace the loose Mockery::type('string') expectation in LockManagerServiceOwnershipTest (the mock of ICacheService used with addSingleValue) with a stricter constraint that asserts the token is non-empty/strong (e.g. Mockery::on(fn($v) => is_string($v) && strlen($v) >= 16)) or add an additional expectation using Mockery::on to verify token length, keeping the same call to addSingleValue and the deleteIfValueMatches expectation; target the mock for addSingleValue on the ICacheService to ensure the generated token is meaningful rather than allowing an empty string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Unit/Services/LockManagerServiceOwnershipTest.php`:
- Around line 139-144: Replace the loose Mockery::type('string') expectation in
LockManagerServiceOwnershipTest (the mock of ICacheService used with
addSingleValue) with a stricter constraint that asserts the token is
non-empty/strong (e.g. Mockery::on(fn($v) => is_string($v) && strlen($v) >= 16))
or add an additional expectation using Mockery::on to verify token length,
keeping the same call to addSingleValue and the deleteIfValueMatches
expectation; target the mock for addSingleValue on the ICacheService to ensure
the generated token is meaningful rather than allowing an empty string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 757f3521-79bb-45c3-a48d-09cc2ce97243
📒 Files selected for processing (4)
Libs/Utils/ICacheService.phpapp/Services/Utils/LockManagerService.phpapp/Services/Utils/RedisCacheService.phptests/Unit/Services/LockManagerServiceOwnershipTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Services/Utils/RedisCacheService.php
- app/Services/Utils/LockManagerService.php
- Libs/Utils/ICacheService.php
…rvice Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
fix/release-on-failed-acquire-and-add-ownership-tokens
branch
from
acb8447 to
c6e6473
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Services/LockManagerServiceOwnershipTest.php (1)
139-145: ⚡ Quick winAssert token identity across acquire and release, not just token type.
At Line [142] and Line [144], the test validates string token creation and release call count, but it does not verify
deleteIfValueMatchesreceives the same token captured duringaddSingleValue. A token-mismatch regression could still pass.Proposed test hardening
public function testAddSingleValueCalledOnceWithTokenAndLifetime(): void { $cache = Mockery::mock(ICacheService::class); + $token = null; $cache->shouldReceive('addSingleValue') ->once() - ->with('test.lock', Mockery::type('string'), 3600) + ->with( + 'test.lock', + Mockery::on(function ($value) use (&$token) { + if (!is_string($value) || $value === '') return false; + $token = $value; + return true; + }), + 3600 + ) ->andReturn(true); - $cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); + $cache->shouldReceive('deleteIfValueMatches') + ->once() + ->with('test.lock', Mockery::on(fn($value) => $value === $token)) + ->andReturn(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Services/LockManagerServiceOwnershipTest.php` around lines 139 - 145, The test currently only asserts the token is a string and that deleteIfValueMatches is called, but not that it's the same token; update LockManagerServiceOwnershipTest to capture the token passed to ICacheService::addSingleValue (use Mockery capture or an on/closure) and then assert ICacheService::deleteIfValueMatches is invoked with the same captured token (e.g., expect deleteIfValueMatches('test.lock', <capturedToken>)). Keep addSingleValue and deleteIfValueMatches expectations tied to the captured variable so the test fails on token mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Unit/Services/LockManagerServiceOwnershipTest.php`:
- Around line 139-145: The test currently only asserts the token is a string and
that deleteIfValueMatches is called, but not that it's the same token; update
LockManagerServiceOwnershipTest to capture the token passed to
ICacheService::addSingleValue (use Mockery capture or an on/closure) and then
assert ICacheService::deleteIfValueMatches is invoked with the same captured
token (e.g., expect deleteIfValueMatches('test.lock', <capturedToken>)). Keep
addSingleValue and deleteIfValueMatches expectations tied to the captured
variable so the test fails on token mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5de5ba1-752b-4d1c-9d24-3a6a5e3e917a
📒 Files selected for processing (4)
Libs/Utils/ICacheService.phpapp/Services/Utils/LockManagerService.phpapp/Services/Utils/RedisCacheService.phptests/Unit/Services/LockManagerServiceOwnershipTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- Libs/Utils/ICacheService.php
- app/Services/Utils/LockManagerService.php
- app/Services/Utils/RedisCacheService.php
|
Malformed lock key — missing dot separator () This line cannot be commented inline since The lock key is missing a // Current — produces e.g. "ticket_type.42promo_code.SUMMER25.sell.lock"
$this->lock_service->lock('ticket_type.' . $type_id . 'promo_code.' . $promo_code_val . '.sell.lock', ...)
// Intended — "ticket_type.42.promo_code.SUMMER25.sell.lock"
$this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock', ...)With integer type IDs there is no key collision today, but the key is semantically malformed and will confuse any tooling (monitoring, manual Redis inspection, key-expiry scripts) that parses the key pattern. There is also no test asserting the exact key string passed to The PR description already flags this as a follow-up: "Fix the missing . in the key." Recommend addressing it in a dedicated follow-up ticket before the ownership token changes are deployed, so the key format stabilises. |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review comments
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Unit/Services/LockManagerServiceOwnershipTest.php (1)
119-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert release uses the exact acquired token.
This test accepts any string on acquire and any release token, so it would still pass if
releaseLock()used the wrong token. Capture the acquired token and assertdeleteIfValueMatches()receives that exact value.Proposed fix
$cache = Mockery::mock(ICacheService::class); +$capturedToken = null; $cache->shouldReceive('addSingleValue') ->once() - ->with('test.lock', Mockery::type('string'), 3600) + ->withArgs(function ($name, $token, $lifetime) use (&$capturedToken) { + $capturedToken = $token; + return $name === 'test.lock' + && is_string($token) + && preg_match('/\A[0-9a-f]{32}\z/', $token) === 1 + && $lifetime === 3600; + }) ->andReturn(true); -$cache->shouldReceive('deleteIfValueMatches')->once()->andReturn(true); +$cache->shouldReceive('deleteIfValueMatches') + ->once() + ->withArgs(function ($name, $token) use (&$capturedToken) { + return $name === 'test.lock' && $token === $capturedToken; + }) + ->andReturn(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Services/LockManagerServiceOwnershipTest.php` around lines 119 - 124, The test is not validating that releaseLock() uses the exact same token that was acquired, since the mocks accept any string values without verification. Capture the token value that is returned when addSingleValue() is called on the cache mock, and then modify the deleteIfValueMatches mock assertion to verify it receives that exact captured token value instead of accepting any string parameter.app/Services/Model/Imp/SummitOrderService.php (1)
1539-1554:⚠️ Potential issue | 🟠 MajorAdd
$ticket_dtoto the closure'suse()clause.The callback accesses
$ticket_dto['attendee_company'],$ticket_dto['attendee_first_name'], and$ticket_dto['attendee_last_name'](lines 1545, 1550, 1554), but$ticket_dtois not captured in the closure. Under PHP closure scoping, this variable is unavailable and falls back to null through the??operator, causing submitted attendee data to be ignored in favor of$this->payloador$this->ownerdefaults.Proposed fix
$order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock', - function () use ($promo_code_val, $type_id) { + function () use ($promo_code_val, $type_id, $ticket_dto) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/Model/Imp/SummitOrderService.php` around lines 1539 - 1554, The closure passed to the lock_service->lock() method is missing the $ticket_dto variable in its use() clause. The callback function accesses $ticket_dto array elements (attendee_company, attendee_first_name, attendee_last_name) but without capturing this variable, it will be unavailable in the closure scope. Add $ticket_dto to the use() clause alongside $promo_code_val and $type_id so the submitted attendee data from the ticket_dto parameter is properly accessible within the callback function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Services/Utils/LockManagerService.php`:
- Around line 73-76: The releaseLock method calls deleteIfValueMatches() but
does not capture or check its return value. This means when
deleteIfValueMatches() returns false (indicating the key was not deleted due to
token mismatch or other reasons), the failure is silently dropped and not
logged. Capture the boolean return value from the deleteIfValueMatches() call
and add logging to record when the deletion fails, so that stuck locks, Redis
release failures, or ownership mismatches become observable through logs rather
than remaining hidden.
- Around line 48-54: The acquireLock method does not validate the $lifetime
parameter before passing it to addSingleValue. Add validation at the start of
the acquireLock method to ensure $lifetime is positive (greater than 0), and
reject or throw an exception for non-positive values. This prevents the creation
of locks with no expiration when addSingleValue receives a zero or negative TTL
value.
---
Outside diff comments:
In `@app/Services/Model/Imp/SummitOrderService.php`:
- Around line 1539-1554: The closure passed to the lock_service->lock() method
is missing the $ticket_dto variable in its use() clause. The callback function
accesses $ticket_dto array elements (attendee_company, attendee_first_name,
attendee_last_name) but without capturing this variable, it will be unavailable
in the closure scope. Add $ticket_dto to the use() clause alongside
$promo_code_val and $type_id so the submitted attendee data from the ticket_dto
parameter is properly accessible within the callback function.
In `@tests/Unit/Services/LockManagerServiceOwnershipTest.php`:
- Around line 119-124: The test is not validating that releaseLock() uses the
exact same token that was acquired, since the mocks accept any string values
without verification. Capture the token value that is returned when
addSingleValue() is called on the cache mock, and then modify the
deleteIfValueMatches mock assertion to verify it receives that exact captured
token value instead of accepting any string parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca02481e-92fd-46d7-b26b-1b9c5ed0f1f9
📒 Files selected for processing (6)
app/Services/Model/Imp/SummitOrderService.phpapp/Services/Utils/ILockManagerService.phpapp/Services/Utils/LockManagerService.phpapp/Services/Utils/RedisCacheService.phptests/Integration/RedisCacheServiceAddSingleValueTest.phptests/Unit/Services/LockManagerServiceOwnershipTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/Utils/RedisCacheService.php
| public function releaseLock(string $name, string $token): void | ||
| { | ||
| Log::debug(sprintf("LockManagerService::releaseLock name %s",$name)); | ||
| $this->cache_service->delete($name); | ||
| return $this; | ||
| Log::debug(sprintf("LockManagerService::releaseLock name %s", $name)); | ||
| $this->cache_service->deleteIfValueMatches($name, $token); |
There was a problem hiding this comment.
Surface failed ownership-aware releases.
deleteIfValueMatches() returns false when the key was not deleted, but releaseLock() drops that signal. Log this so stuck locks, Redis release failures, or callbacks exceeding TTL are observable.
Proposed fix
public function releaseLock(string $name, string $token): void
{
Log::debug(sprintf("LockManagerService::releaseLock name %s", $name));
- $this->cache_service->deleteIfValueMatches($name, $token);
+ $released = $this->cache_service->deleteIfValueMatches($name, $token);
+ if (!$released) {
+ Log::warning(sprintf(
+ "LockManagerService::releaseLock name %s was not released; token may have expired, been reacquired, or Redis delete failed",
+ $name
+ ));
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Services/Utils/LockManagerService.php` around lines 73 - 76, The
releaseLock method calls deleteIfValueMatches() but does not capture or check
its return value. This means when deleteIfValueMatches() returns false
(indicating the key was not deleted due to token mismatch or other reasons), the
failure is silently dropped and not logged. Capture the boolean return value
from the deleteIfValueMatches() call and add logging to record when the deletion
fails, so that stuck locks, Redis release failures, or ownership mismatches
become observable through logs rather than remaining hidden.
e1a0952 to
9ef1bf6
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/ This page is automatically updated on each push to this PR. |
Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
fix/release-on-failed-acquire-and-add-ownership-tokens
branch
from
9ef1bf6 to
fbe981c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/ This page is automatically updated on each push to this PR. |
2 participants
ref https://app.clickup.com/t/86b9f3a22
Recommended actions for a follow-up ticket:
Summary by CodeRabbit