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.
There was a problem hiding this comment.
@romanetar Building on this suggestion: logging the discarded false is the right baseline, but a mismatch detected at release time is actionable, not just observable — consider escalating it to a typed exception so the caller can roll back.
Every lock() call site in SummitOrderService runs inside tx_service->transaction, and the release happens in lock()'s finally before the transaction closure returns. Throwing on a genuine ownership mismatch therefore propagates out of the closure and triggers a DB rollback: the holder that lost its lease aborts instead of committing on top of a competitor's work. That converts silent exclusion loss into "lease-loser aborts" semantics — effectively fencing at commit time, which is the strongest guarantee a single-instance Redlock can offer without real fencing tokens in the storage layer.
Four conditions to do it safely:
- Tri-state release result, not bool.
deleteIfValueMatches()returnsfalsein three distinct situations: lease genuinely lost (key gone or value mismatch), Redis unreachable (retryOnConnectionErrorfalls back tofalse), and a retried Lua call whose first response was lost after a successful delete. Throwing on all three would turn a Redis blip into a rollback of every in-flight locked operation — an availability regression. The cache layer should reportRELEASED/NOT_OWNED/UNKNOWN, andlock()should throw only onNOT_OWNEDand logUNKNOWN. - Never mask the callback's own exception. The release runs in
finally; if the callback already threw, a throw fromfinallyreplaces it as the visible exception type (PHP chains the original asprevious, but upstream handlers see the new type). Only throw when the callback completed successfully; otherwise just log. - Move non-transactional side effects out of the lock first.
SendAttendeeInvitationEmail::dispatchcurrently runs inside theAutoAssignPrePaidTicketTaskcallback, and its queue write does not participate in the DB transaction — a rollback would leave a scheduled email pointing at an assignment that no longer exists. Item 1 of the PR description (dispatch after the lock is released) is a prerequisite for this design, not just a follow-up. - Type the exception (e.g.
LockLostException) so the saga tasks — which already haveundo()compensation paths — can distinguish it from validation failures instead of catching a genericRuntimeException.
Combined with the pessimistic row lock suggested in my comment on AutoAssignPrePaidTicketTask, this becomes defense in depth: the DB lock prevents the race, the release check detects anything that slips through.
There was a problem hiding this comment.
@smarcet Agreed — this is the right way to close the loop without turning a transient Redis hiccup into a swarm of rollbacks. A few notes on sequencing:
- The tri-state split is important specifically because
deleteIfValueMatches()sits behindretryOnConnectionError, so afalsetoday already conflates "key gone/mismatched" with "couldn't reach Redis." Collapsing those into a single boolean throw would make the lock subsystem less available than it is today. - The finally-masking point is easy to miss in review — worth calling out explicitly in the follow-up ticket description so whoever implements it adds an early-return/guard (
if ($callback_threw) { log; return; }) before anythrow new LockLostException(...). - Sequencing dependency on item 3 (moving
SendAttendeeInvitationEmail::dispatchoutside the callback) is correct and should be a hard prerequisite/gate, not just a nice-to-have, since withLockLostExceptionthrown mid-transaction any non-transactional side effect inside the callback becomes a correctness bug, not just a cosmetic one. - Typed
LockLostExceptionpairing with the sagaundo()compensation paths makes sense givenReserveTicketsTask/ApplyPromoCodeTaskalready implement that pattern.
I'd suggest bundling this whole design (tri-state cache result, LockLostException, dispatch-after-release, and the pessimistic row lock from your other comment) into the single follow-up ticket already referenced in the PR description, rather than doing it piecemeal — since the tri-state contract change and the exception-throwing behavior are tightly coupled and should land together with tests asserting each of RELEASED/NOT_OWNED/UNKNOWN paths.
(๑˘•ω•˘)ﻭ
✏️ Learnings added
Learnt from: smarcet
Repo: OpenStackweb/summit-api PR: 537
File: app/Services/Utils/LockManagerService.php:76-79
Timestamp: 2026-07-13T02:33:49.962Z
Learning: In `app/Services/Utils/LockManagerService.php` (PHP, Laravel), the desired future design for `releaseLock()`/`lock()` per reviewer smarcet: `ICacheService::deleteIfValueMatches()` should return a tri-state result (e.g. RELEASED / NOT_OWNED / UNKNOWN) instead of a boolean, because a `false` return currently conflates "lease genuinely lost" (key gone or value mismatch), "Redis unreachable" (retryOnConnectionError fallback), and "ambiguous retried Lua eval outcome." `lock()` should throw a typed `LockLostException` only on `NOT_OWNED`, and only log (not throw) on `UNKNOWN`, to avoid turning transient Redis blips into DB transaction rollbacks. Since `releaseLock()` runs in `lock()`'s `finally` block, it must never throw when the wrapped callback itself already threw an exception (PHP finally-throws mask the original exception type for upstream catch-by-type handlers, even though it's chained as `previous`). Also, moving `SendAttendeeInvitationEmail::dispatch` out of the `AutoAssignPrePaidTicketTask` lock callback (already tracked as a PR `#537` follow-up item) is a hard prerequisite for enabling throw-on-mismatch, since dispatch is a non-transactional side effect (queue write) that would become inconsistent if the lock callback's transaction rolls back due to a thrown `LockLostException`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
romanetar
force-pushed
the
fix/release-on-failed-acquire-and-add-ownership-tokens
branch
from
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. |
|
|
||
| return $order; | ||
| }); | ||
| }, 30); |
There was a problem hiding this comment.
@romanetar This 30 s lifetime makes TTL expiry a silent loss of mutual exclusion, and this call site is the only one of the four where the Redis lock is the sole guard of the invariant — the fix should be a DB pessimistic lock, matching the sibling tasks.
The other three sites are already backed by row locks — ReserveTicketsTask::run/undo hold getByIdsExclusiveLock/getByIdExclusiveLock on the ticket types and RedeemPromoCodesTask holds getByValueExclusiveLock on the promo code — so the DB serializes competing writers regardless of what Redis does. This callback instead reads the promo code via $this->summit->getPromoCodeByCode() with no lock, and the check-then-act sequence (hasPrePaidTicketsAssignedBy → getNextAvailableTicketPerType → addTicket) relies entirely on this Redis lock. If the callback ever outlives the 30 s TTL, a concurrent request acquires the lock and can hand the same prepaid ticket to two attendees. This failure mode is new to this PR: on main the key effectively never expired (TTL was a unix timestamp, ≈55 years). There is also a second window the Redis lock cannot cover even when it works: it is released when the callback returns, but the enclosing tx_service->transaction commits afterwards, so a competitor can read pre-commit state.
Suggested fix — same pattern as RedeemPromoCodesTask in this file: inject ISummitRegistrationPromoCodeRepository into AutoAssignPrePaidTicketTask and fetch the promo code with getByValueExclusiveLock($this->summit, $promo_code_val) inside the transaction, instead of the unlocked getPromoCodeByCode(). That method takes PESSIMISTIC_WRITE with HINT_REFRESH (DoctrineSummitRegistrationPromoCodeRepository.php:617), so the entity is re-hydrated from committed DB truth after the row lock is granted, and the lock is held through commit — closing both the TTL-expiry hole and the release-before-commit window in one move. The Redis lock can stay as a fail-fast layer, consistent with the sibling sites.
Independently, releaseLock() discarding the deleteIfValueMatches() === false signal still deserves the log + metric suggested in the CodeRabbit thread on LockManagerService.php:79 — a mismatch at release is the only runtime evidence that a lock expired mid-callback anywhere in the system.
| return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) { | ||
| if ($conn->setnx($counter_name, 1)) { | ||
| if ($ttl > 0) $conn->expire($counter_name, (int)$ttl); | ||
| if ($conn->set($counter_name, 1, ['EX' => (int)$ttl, 'NX' => true]) !== null) { |
There was a problem hiding this comment.
@romanetar incCounter now fails on every invocation under the Redis client this app actually uses.
The options-array form set($key, 1, ['EX' => $ttl, 'NX' => true]) is phpredis API, but the app pins Predis (config/database.php:176, REDIS_CLIENT default predis), and Predis's SET command passes arguments through verbatim — StreamConnection::writeRequest serializes each argument with strlen(strval($argument)). With an array argument that's an "Array to string conversion": under Laravel's error handler it throws ErrorException; otherwise the wire command becomes SET key 1 "Array" and Redis replies ERR syntax error (Predis\Response\ServerException). Neither exception is caught by retryOnConnectionError, which only catches PredisConnectionException|\RedisException. Additionally, the default $ttl = 0 would produce EX 0, which Redis rejects even in the correct form.
addSingleValue a few lines below uses the correct Predis variadic form — mirroring it fixes both problems:
if ($ttl > 0) {
if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1;
} else {
if ($conn->set($counter_name, 1, 'NX') !== null) return 1;
}
return (int)$conn->incr($counter_name);incCounter has no callers in the repo today, so reverting this hunk entirely is also reasonable — it's outside the lock scope of this PR.
| * | ||
| */ | ||
| #[Group("integration")] | ||
| final class RedisCacheServiceAddSingleValueTest extends TestCase |
There was a problem hiding this comment.
@romanetar This test never runs in CI: the push.yml suite matrix (.github/workflows/push.yml:57-68) is a fixed list of paths/filters with no entry for tests/Integration/, so the driver-compatibility and atomicity regressions this file exists to catch would pass silently. Redis is already provisioned in that workflow (supercharge/redis-github-action, line 148), so wiring it up is one line:
- { name: "Integration", filter: "tests/Integration/" }While extending this class, please also add a case for deleteIfValueMatches — it's the Lua compare-and-delete the whole ownership guarantee rests on, and today it's only ever exercised against mocks. A broken script (or a driver change breaking the Predis-style eval($lua, 1, $key, $value) signature) would silently no-op every release, holding all locks to full TTL. Two assertions suffice: matching token deletes the key; non-matching token leaves it intact.
|
|
||
| $type_id = $ticket_dto['type_id']; | ||
| $order = $this->lock_service->lock('ticket_type.' . $type_id . 'promo_code.' . $promo_code_val . '.sell.lock', | ||
| $order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock', |
There was a problem hiding this comment.
@romanetar The key fix is correct, but note it splits the lock namespace between fleets during a rolling deploy: old pods lock ticket_type.7promo_code.X.sell.lock while new pods lock ticket_type.7.promo_code.X.sell.lock, so the two fleets don't mutually exclude on this path for the duration of the rollout. Compounding it, old pods still run the unconditional-DEL-on-failed-acquire behavior this PR fixes, so they can delete new pods' token locks on any key. Since this call site is the one place where the Redis lock is the sole concurrency guard (no DB row lock — see my other comment), the deploy window is exactly where a race could materialize.
No code change needed — suggest a line in the release notes: deploy while prepaid-assignment traffic is idle, or drain old workers before starting the new fleet.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please re review
ref https://app.clickup.com/t/86b9f3a22
Recommended actions for a follow-up ticket:
Summary by CodeRabbit