fix(lock): implement Redlock single-instance pattern in LockManagerService - #537
fix(lock): implement Redlock single-instance pattern in LockManagerService#537romanetar wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds atomic Redis compare-and-delete support and ownership tokens for distributed locks. It updates lock consumers with explicit lifetimes, adds Redis and lock ownership tests, and enables the integration suite in CI. ChangesOwnership-Based Distributed Locking
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant LockManagerService
participant RedisCacheService
participant Redis
Caller->>LockManagerService: lock(name, callback, lifetime)
LockManagerService->>RedisCacheService: addSingleValue(name, token, lifetime)
RedisCacheService->>Redis: SET name token NX EX lifetime
Redis-->>RedisCacheService: acquisition result
RedisCacheService-->>LockManagerService: acquired or retry
LockManagerService->>Caller: execute callback
LockManagerService->>RedisCacheService: deleteIfValueMatches(name, token)
RedisCacheService->>Redis: EVAL compare-and-delete
Redis-->>RedisCacheService: deletion result
RedisCacheService-->>LockManagerService: release result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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
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
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
| return $conn->set($key, $value, 'NX') !== null; | ||
| }, false); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing integration test: atomic SET NX EX
All tests for this path mock ICacheService, so the actual Redis command added here is never exercised against a real server. Two things that only an integration test can verify:
- Driver compatibility — the variadic form
set($key, $value, 'EX', $ttl, 'NX')works with Predis 2.x. If the driver is ever switched to PhpRedis,set()returnsfalseon an NX-miss (notnull), silently breaking the!== nullcheck. - Atomicity — the fix guarantees the key cannot exist without a TTL; only a real
TTL keycall afteraddSingleValuecan confirm there is no gap.
Recommend a @group integration test that:
- calls
addSingleValue($key, $token, 30)against a real test Redis - reads
TTL $keyand asserts it is between 1 and 30 s - calls
addSingleValueagain on the same key and asserts it returnsfalse(NX semantics)
There was a problem hiding this comment.
Reopening — this thread has two distinct asks and only one was addressed.
Point 2 (atomicity / TTL-in-one-command integration coverage) was fixed and is covered by the Integration test suite added to this PR. But point 1 (driver compatibility — "If the driver is ever switched to PhpRedis, set() returns false on an NX-miss (not null), silently breaking the !== null check") was never touched, and this got marked resolved anyway.
I verified point 1 is a real, currently-live bug in the code as it stands today: under REDIS_CLIENT=phpredis, addSingleValue()/incCounter() both report success on an NX-miss, which lets a second acquireLock() succeed while the first token still owns the lock. Full writeup, reproduction, and suggested fix are in a new comment on RedisCacheService.php:311 (current line for addSingleValue) rather than here, since the diff context on this line has moved.
@romanetar — see that comment for the fix; leaving this one open until it's addressed there.
|
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
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. |
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.
There was a problem hiding this comment.
Verified both points against the code and applied fixes for both (will push shortly).
DB pessimistic lock for AutoAssignPrePaidTicketTask: injected ISummitRegistrationPromoCodeRepository into the task (wired from SagaFactory::$promo_code_repository, which was already available there) and replaced the unlocked $this->summit->getPromoCodeByCode($promo_code_val) with $this->promo_code_repository->getByValueExclusiveLock($this->summit, $promo_code_val) — same pattern as ApplyPromoCodeTask. That row lock (PESSIMISTIC_WRITE + HINT_REFRESH) now covers the whole check-then-act sequence (hasPrePaidTicketsAssignedBy → getNextAvailableTicketPerType → addTicket) and is held through the enclosing transaction's commit, closing both the TTL-expiry hole and the release-before-commit window. The Redis lock stays in place as the fail-fast layer.
releaseLock() observability: it now checks the deleteIfValueMatches() result and, on a mismatch, logs a warning (lock name + token) and increments a lock_manager.release_mismatch counter via the existing ICacheService::incCounter() — no new metrics infra introduced. Added a regression test asserting the counter fires when release fails.
Ran tests/SummitOrderServiceTest.php against the real DB/Redis containers — no regressions (the 3 prepaid-ticket tests are pre-existing markTestSkipped('broken test.'), unrelated to this change, left as-is/out of scope for this PR).
| 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.
There was a problem hiding this comment.
Confirmed and fixed. Reproduced the exact failure against the real Predis-backed Redis container before the fix — Array to string conversion warning followed by Predis\Response\ServerException: ERR syntax error on every call, uncaught by retryOnConnectionError since it only catches PredisConnectionException|\RedisException, exactly as described.
Applied your suggested fix, mirroring addSingleValue's variadic form:
public function incCounter($counter_name, $ttl = 0)
{
return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) {
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);
}, 0);
}Verified against the real container: first call sets the key (returns 1), subsequent calls hit the NX failure path and fall through to incr (2, 3, ...), both with and without a TTL. No exceptions.
Kept incCounter rather than reverting the hunk, since it's now the target of the lock_manager.release_mismatch counter added in LockManagerService::releaseLock for the other thread on this PR — so it does have a caller as of this branch.
| * | ||
| */ | ||
| #[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.
There was a problem hiding this comment.
Both done.
CI wiring: added { name: "Integration", filter: "tests/Integration/" } to the integration-tests matrix in .github/workflows/push.yml, right after the Services entry. Redis is already provisioned there via supercharge/redis-github-action, so no other setup was needed. Verified this is the correct invocation form (bare directory path, matching the OAuth2Tests/EntityModelUnitTests/AuditUnitTests entries) — vendor/bin/phpunit --filter 'tests/Integration/' actually matches nothing and prints "No tests executed!" since --filter runs a regex against test/class names, not paths, so that form would have silently run zero tests in CI.
deleteIfValueMatches coverage: extended RedisCacheServiceAddSingleValueTest with two cases — matching token deletes the key, non-matching token leaves it intact (asserted both the return value and the key's survival/state). Ran all 4 tests in the file against the real Redis container: pass.
|
|
||
| $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.
There was a problem hiding this comment.
Good flag, and confirmed against main's LockManagerService — releaseLock() there is an unconditional delete($name) and lock()'s finally calls it even after acquireLock throws UnacquiredLockException, so an old pod that loses contention on any key will delete it out from under whoever holds it, no ownership check. Combined with the key-namespace split you describe (missing dot on old pods vs. present on new), the two fleets neither mutually exclude on this path nor leave each other's locks alone during the rollout — exactly the window you're calling out.
No code change from this thread, but worth noting: the DB pessimistic lock added for your other comment on this same call site (getByValueExclusiveLock on the promo code, PESSIMISTIC_WRITE + HINT_REFRESH, held through commit) closes the concrete failure mode here — old and new pods share the same MySQL row lock regardless of what happens to the Redis key naming or unconditional deletes on either side. So the double-assignment scenario this thread worried about is now covered even mid-rollout; Redis staying inconsistent across fleets during the deploy window would at most cause spurious contention/retries, not an actual double-booked prepaid ticket.
Will add the release-note line about draining old workers / deploying during idle prepaid-assignment traffic anyway, since the underlying old-pod unconditional-release bug can still stomp on other locks in the system during the transition, not just this one.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please re review
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Services/Model/Imp/SummitOrderService.php (1)
1548-1548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the exact lock key.
Mock
ILockManagerService::lock()and assertticket_type.42.promo_code.SUMMER25.sell.lock. This prevents a valid-but-malformed key from splitting the prepaid-assignment lock namespace again.🤖 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` at line 1548, Add a regression test covering the lock call in SummitOrderService, mock ILockManagerService::lock(), and assert it receives the exact key ticket_type.42.promo_code.SUMMER25.sell.lock. Keep the test focused on preventing malformed keys from creating a separate prepaid-assignment lock namespace.
🤖 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 `@app/Services/Model/Imp/SummitOrderService.php`:
- Line 1548: Add a regression test covering the lock call in SummitOrderService,
mock ILockManagerService::lock(), and assert it receives the exact key
ticket_type.42.promo_code.SUMMER25.sell.lock. Keep the test focused on
preventing malformed keys from creating a separate prepaid-assignment lock
namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a2189a5-79d1-4403-86c3-72deb415d4a8
📒 Files selected for processing (7)
.github/workflows/push.ymlapp/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 (3)
- app/Services/Utils/RedisCacheService.php
- app/Services/Utils/ILockManagerService.php
- tests/Unit/Services/LockManagerServiceOwnershipTest.php
…rvice Signed-off-by: romanetar <roman_ag@hotmail.com>
Signed-off-by: romanetar <roman_ag@hotmail.com>
Signed-off-by: romanetar <roman_ag@hotmail.com>
4b0e311 to
3ce71a3
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/ This page is automatically updated on each push to this PR. |
| # Named by path because no job in this matrix runs the tests/ root, only its | ||
| # subdirectories - a file added there runs nowhere unless it is listed here. | ||
| - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php" } | ||
| - { name: "Repositories", filter: "--filter tests/Repositories/" } |
There was a problem hiding this comment.
@romanetar this is a CI coverage regression. These entries changed from PHPUnit path arguments to --filter tests/..., but PHPUnit --filter matches test/class names, not directories. As a result, the Services and Repositories jobs now execute zero tests while still exiting 0.
I reproduced it with:
vendor/bin/phpunit --filter tests/Unit/Services/ --do-not-cache-result
vendor/bin/phpunit --filter tests/Repositories/ --do-not-cache-result
Both print No tests executed! and exit 0. Please keep tests/Unit/Services/ and tests/Repositories/ as path arguments, and add tests/Integration/ as a path argument too.
| $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', | ||
| function () use ($promo_code_val, $type_id) { |
There was a problem hiding this comment.
@romanetar Missing $ticket_dto in the closure's use() clause — attendee-submitted data silently dropped.
The closure at this line (function () use ($promo_code_val, $type_id) { ... }) reads $ticket_dto['attendee_company'], ['attendee_first_name'], and ['attendee_last_name'] in its body, but $ticket_dto isn't captured. PHP nested closures don't inherit an enclosing closure's locals automatically — only use()'d variables (and $this, which is why $this->owner/$this->payload work here without a use entry). Inside this closure $ticket_dto is undefined, so every one of these reads silently falls through the ??/empty() fallback to $this->payload['owner_*'] / $this->owner->get*() — the order owner's own profile, not the attendee actually being assigned the ticket.
I confirmed this against the branch directly: a regression test that submits attendee_company = "Attendee Co" on the ticket while the order owner's company is "Tipit" gets back an attendee with company "Tipit" — the ticket-level value never reaches SummitAttendeeFactory. This is CodeRabbit's earlier finding on this same line (cr-comment:v1:4850e25b); it looks like it slipped through unaddressed while the other three review threads on this file were fixed.
Suggested 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) {I verified this one-line change is sufficient — with it applied, the regression test below passes (attendee gets the ticket-level company/first/last name), and fails without it (Expected 'Attendee Co' / Actual 'Tipit'), so it's a genuine RED/GREEN pair, not a fixture artifact:
public function testAutoAssignPrePaidTicketUsesTicketLevelAttendeeData() {
// Fixture registration window starts tomorrow (see InsertSummitTestData);
// open it now so this test isn't blocked by an unrelated precondition.
self::$summit->setRegistrationBeginDate(new \DateTime('-1 day'));
self::$summit->setRegistrationEndDate(new \DateTime('+30 days'));
self::$em->persist(self::$summit);
self::$em->flush();
// Build a dedicated unassigned, paid, offline ticket so this test does not
// depend on the base fixture's order #0 (already-assigned attendee / online
// payment method, neither of which qualifies for prepaid pickup).
$owner = self::$defaultMember;
$order = new SummitOrder();
$order->setSummit(self::$summit);
$order->setOwner($owner);
$order->setPaymentMethodOffline();
$order->generateNumber();
$ticket = new SummitAttendeeTicket();
$ticket->setTicketType(self::$default_ticket_type);
$order->addTicket($ticket);
$ticket->activate();
$ticket->generateNumber();
$ticket->generateQRCode();
self::$summit->addOrder($order);
self::$em->persist($order);
self::$em->flush();
$order->setPaid();
self::$default_prepaid_discount_code->clearTickets();
self::$default_prepaid_discount_code->addTicket($ticket);
self::$em->persist(self::$default_prepaid_discount_code);
self::$em->persist($order);
self::$em->flush();
$service = App::make(ISummitOrderService::class);
$payload = [
"owner_email" => $owner->getEmail(),
"owner_first_name" => $owner->getFirstName(),
"owner_last_name" => $owner->getLastName(),
"owner_company" => $owner->getCompany(),
"tickets" => [
[
"type_id" => self::$default_ticket_type->getId(),
"promo_code" => self::$default_prepaid_discount_code->getCode(),
// Attendee is a different person than the order owner -
// this is the scenario AutoAssignPrePaidTicketTask exists for.
"attendee_company" => "Attendee Co",
"attendee_first_name" => "Jane",
"attendee_last_name" => "Doe",
],
]
];
$result_order = $service->reserve($owner, self::$summit, $payload);
$attendee = $result_order->getTickets()->first()->getOwner();
$this->assertEquals("Attendee Co", $attendee->getCompanyName());
$this->assertEquals("Jane", $attendee->getFirstName());
$this->assertEquals("Doe", $attendee->getSurname());
}It doesn't reuse the base fixture's order #0 or touch the three pre-existing markTestSkipped('broken test.') tests in this file — those are broken for unrelated reasons (a hardcoded summit_repository->find(3800) lookup, and the base fixture's registration window starting tomorrow rather than today).
| if ($res && $ttl > 0) { | ||
| $conn->expire($key, $ttl); | ||
| if ($ttl > 0) { | ||
| return $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null; |
There was a problem hiding this comment.
@romanetar !== null to detect a SET ... NX miss is Predis-specific — under PhpRedis it silently defeats the lock's mutual exclusion.
addSingleValue() here and incCounter() (RedisCacheService.php:243) both check $conn->set(..., 'NX') !== null to detect whether the SET acquired the key. That's correct for Predis, which returns null on an NX-miss — but PhpRedis (ext-redis, the C extension) returns false on an NX-miss for this same variadic form, and false !== null evaluates to true. So under REDIS_CLIENT=phpredis, a second addSingleValue() call on a key that's still held reports success instead of failure.
Concretely: LockManagerService::acquireLock() is built entirely on this return value — a second caller racing for an already-held lock would get back a token and believe it holds the lock, while the first token is still the actual owner. That's the exact mutual-exclusion guarantee this PR exists to add. incCounter() degrades the same way: the lock_manager.release_mismatch counter this PR just wired up would silently stick at 1 instead of incrementing.
This isn't a new observation — it's the first point of an existing, still-open review comment on this same line (driver-compatibility, "If the driver is ever switched to PhpRedis, set() returns false on an NX-miss (not null), silently breaking the !== null check"). The reply that followed fixed the CI wiring and added deleteIfValueMatches coverage but didn't touch this point, so it's been carried unresolved since.
Verified directly: docker exec -e REDIS_CLIENT=phpredis summit-api vendor/bin/phpunit tests/Integration/RedisCacheServiceAddSingleValueTest.php fails at the NX-reacquire assertion (Failed asserting that true is false), and a two-call incCounter($key, 30) probe returns [1, 1, "1", 30] under PhpRedis vs. the correct [1, 2, "2", 30] under Predis.
One mitigating note: I checked argocd-apps/summit-api's values.yaml / values-prod.yaml / values-stage.yaml — REDIS_CLIENT isn't set in either, so both currently run on the predis default and aren't hitting this today. But the redis (PhpRedis) extension is already compiled into the app image, nothing in config or CI guards against someone setting the env var, and CI never runs this suite under PhpRedis either — so it's a live landmine with zero coverage, not a hypothetical one.
Suggested fix — treat both driver's failure sentinels the same way, consistent for addSingleValue and incCounter:
- if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1;
+ if ($this->setNxSucceeded($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX'))) return 1;
...
- if ($conn->set($counter_name, 1, 'NX') !== null) return 1;
+ if ($this->setNxSucceeded($conn->set($counter_name, 1, 'NX'))) return 1;
...
- return $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null;
+ return $this->setNxSucceeded($conn->set($key, $value, 'EX', (int)$ttl, 'NX'));
...
- return $conn->set($key, $value, 'NX') !== null;
+ return $this->setNxSucceeded($conn->set($key, $value, 'NX'));with a small private helper setNxSucceeded($result): bool { return $result !== null && $result !== false; } (any real success value — a Predis\Response\Status object or PhpRedis's true/1 — is truthy against both checks; only the two drivers' respective failure sentinels are excluded). Worth also adding a deleteIfValueMatches-style @group integration case that runs under both REDIS_CLIENT=predis and REDIS_CLIENT=phpredis in CI, so this class of bug can't reappear silently again.
| $cache = Mockery::mock(ICacheService::class); | ||
| $cache->shouldReceive('addSingleValue') | ||
| ->once() | ||
| ->with('test.lock', Mockery::type('string'), 3600) |
There was a problem hiding this comment.
@romanetar Token identity isn't asserted between addSingleValue and deleteIfValueMatches — the one guarantee this whole PR adds.
Mockery::type('string') on line 122 accepts any string, and deleteIfValueMatches on line 124 has no with() constraint at all. So this test would still pass if releaseLock() released using a hardcoded literal, a stale token from a previous call, or a completely wrong value — as long as it's some string. The one thing this PR is actually about (the release only succeeds when the token matches the one acquireLock handed out) is exactly what's left unverified here.
This has been flagged twice already by CodeRabbit on earlier revisions of this same test (once as "assert the token is non-empty," once more precisely as "assert release uses the exact acquired token," with a ready-to-use diff) and both times it was left unaddressed while the file kept changing around it.
To be clear, this isn't a live bug — LockManagerService::lock() trivially threads the same $token variable from acquireLock() into releaseLock(), so nothing is broken today. But it means a future refactor that breaks that threading (e.g. re-deriving the token instead of reusing it, or introducing the mutable-instance-state pattern flagged elsewhere on this PR) would sail through this suite undetected.
Suggested fix — capture the token on the addSingleValue expectation and assert deleteIfValueMatches receives the same value:
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);
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please re review i do still see some pending issues
ref https://app.clickup.com/t/86b9f3a22
Recommended actions for a follow-up ticket:
Summary by CodeRabbit