Skip to content

fix(lock): implement Redlock single-instance pattern in LockManagerService #537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
romanetar wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(lock): implement Redlock single-instance pattern in LockManagerService #537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6e6473
fix(lock): implement Redlock single-instance pattern in LockManagerSe…
romanetar May 4, 2026
fbe981c
fix: review feedback
romanetar Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Libs/Utils/ICacheService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,16 @@ public function setSingleValue($key, $value, $ttl = 0);
*/
public function addSingleValue($key, $value, $ttl = 0);

/**
* Atomically compare-and-delete: DEL the key only when its current value
* equals $expectedValue. Implementations MUST use an atomic operation
* (Lua EVAL or equivalent) — never a separate GET + conditional DEL.
* @param string $key
* @param string $expectedValue
* @return bool true iff the key existed, matched, and was deleted
*/
public function deleteIfValueMatches(string $key, string $expectedValue): bool;

/**
* Set time to live to a given key
* @param $key
Expand Down
12 changes: 6 additions & 6 deletions app/Services/Model/Imp/SummitOrderService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,7 @@ public function run(array $formerState): array

$this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $qty, $owner_email) {
$promo_code->addUsage($owner_email, $qty);
});
}, 30);

});
// mark a done
Expand Down Expand Up @@ -865,7 +865,7 @@ public function undo()

$this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $info, $owner_email) {
$promo_code->removeUsage(intval($info['qty']), $owner_email);
});
}, 30);

});
}
Expand Down Expand Up @@ -950,7 +950,7 @@ public function run(array $formerState): array

$this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $reservations) {
$ticket_type->sell($reservations[$ticket_type->getId()]);
});
}, 30);

}
});
Expand All @@ -967,7 +967,7 @@ public function undo()
if (is_null($ticket_type)) return;
$this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $qty) {
$ticket_type->restore($qty);
});
}, 30);
});
}
}
Expand Down Expand Up @@ -1536,7 +1536,7 @@ public function run(array $formerState): array
if (empty($promo_code_val)) throw new ValidationException("Promo code is required.");

$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',

@smarcet smarcet Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

function () use ($promo_code_val, $type_id) {

$attendee_email = $this->owner->getEmail();
Expand Down Expand Up @@ -1658,7 +1658,7 @@ function () use ($promo_code_val, $type_id) {


return $order;
});
}, 30);

@smarcet smarcet Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ['order' => $order];
});
}
Expand Down
13 changes: 7 additions & 6 deletions app/Services/Utils/ILockManagerService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,21 @@ interface ILockManagerService
* @param string $name
* @param int $lifetime
* @throws UnacquiredLockException
* @return mixed
* @return string ownership token — must be passed to releaseLock
*/
public function acquireLock(string $name,int $lifetime = self::DefaultLifetime);
public function acquireLock(string $name, int $lifetime = self::DefaultLifetime): string;

/**
* @param string $name
* @return mixed
* @param string $name
* @param string $token ownership token returned by acquireLock
*/
public function releaseLock(string $name);
public function releaseLock(string $name, string $token): void;

/**
* @param string $name
* @param Closure $callback
* @param int $lifetime
* @return mixed
*/
public function lock(string $name, Closure $callback, int $lifetime = self::DefaultLifetime);
public function lock(string $name, Closure $callback, int $lifetime = self::DefaultLifetime): mixed;
}
64 changes: 34 additions & 30 deletions app/Services/Utils/LockManagerService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@
*/
final class LockManagerService implements ILockManagerService {

const MaxRetries = 3;
const MaxRetries = 3;
const BackOffMultiplier = 2.0;
const BackOffBaseInterval = 100000; // 1 ms
const BackOffBaseInterval = 100000; // microseconds

/**
* @var ICacheService
*/
Expand All @@ -41,73 +42,76 @@ public function __construct(ICacheService $cache_service){
/**
* @param string $name
* @param int $lifetime
* @return LockManagerService
* @return string ownership token — pass to releaseLock
* @throws UnacquiredLockException
*/
public function acquireLock(string $name, int $lifetime = 3600):LockManagerService
public function acquireLock(string $name, int $lifetime = 3600): string
{
Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s",$name, $lifetime));
$attempt = 0 ;
Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s", $name, $lifetime));
if ($lifetime <= 0) {
throw new \InvalidArgumentException("Lock lifetime must be greater than zero seconds.");
}
$token = bin2hex(random_bytes(16));
$attempt = 0;
do {
$time = time() + $lifetime + 1;
$success = $this->cache_service->addSingleValue($name, $time, $time);
if($success) return $this;
$wait_interval = self::BackOffBaseInterval * ( self::BackOffMultiplier ^ $attempt );
Log::debug(sprintf("LockManagerService::acquireLock name %s retrying in %s microseconds (%s).", $name, $wait_interval, $attempt));
$success = $this->cache_service->addSingleValue($name, $token, $lifetime);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if ($success) {
return $token;
}
$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 )) {
// only one time we could use this handle
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));
}
++$attempt;
} while(1);
} while (1);
}

/**
* @param string $name
* @return $this
* @param string $token ownership token returned by acquireLock
*/
public function releaseLock(string $name):LockManagerService
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);
Comment on lines +76 to +79

@coderabbitai coderabbitai Bot Jun 15, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

@smarcet smarcet Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. Tri-state release result, not bool. deleteIfValueMatches() returns false in three distinct situations: lease genuinely lost (key gone or value mismatch), Redis unreachable (retryOnConnectionError falls back to false), 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 report RELEASED / NOT_OWNED / UNKNOWN, and lock() should throw only on NOT_OWNED and log UNKNOWN.
  2. Never mask the callback's own exception. The release runs in finally; if the callback already threw, a throw from finally replaces it as the visible exception type (PHP chains the original as previous, but upstream handlers see the new type). Only throw when the callback completed successfully; otherwise just log.
  3. Move non-transactional side effects out of the lock first. SendAttendeeInvitationEmail::dispatch currently runs inside the AutoAssignPrePaidTicketTask callback, 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.
  4. Type the exception (e.g. LockLostException) so the saga tasks — which already have undo() compensation paths — can distinguish it from validation failures instead of catching a generic RuntimeException.

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.

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 behind retryOnConnectionError, so a false today 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 any throw new LockLostException(...).
  • Sequencing dependency on item 3 (moving SendAttendeeInvitationEmail::dispatch outside the callback) is correct and should be a hard prerequisite/gate, not just a nice-to-have, since with LockLostException thrown mid-transaction any non-transactional side effect inside the callback becomes a correctness bug, not just a cosmetic one.
  • Typed LockLostException pairing with the saga undo() compensation paths makes sense given ReserveTicketsTask/ApplyPromoCodeTask already 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.

}

/**
* @param string $name
* @param Closure $callback
* @param int $lifetime
* @return null
* @return mixed
* @throws UnacquiredLockException
* @throws Exception
*/
public function lock(string $name, Closure $callback, int $lifetime = 3600)
public function lock(string $name, Closure $callback, int $lifetime = 3600): mixed
{
$token = null;
$result = null;
Log::debug(sprintf("LockManagerService::lock name %s lifetime %s", $name, $lifetime));

try
{
$this->acquireLock($name, $lifetime);
try {
$token = $this->acquireLock($name, $lifetime);
Log::debug(sprintf("LockManagerService::lock name %s calling callback", $name));
$result = $callback($this);
}
catch(UnacquiredLockException $ex)
{
catch(UnacquiredLockException $ex) {
Log::warning($ex);
throw $ex;
}
catch(Exception $ex)
{
catch(Exception $ex) {
Log::error($ex);
throw $ex;
}
finally {
$this->releaseLock($name);
if ($token !== null) {
$this->releaseLock($name, $token);
}
}
return $result;
}

}
}
28 changes: 20 additions & 8 deletions app/Services/Utils/RedisCacheService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,7 @@ public function storeHash($name, array $values, $ttl = 0)
public function incCounter($counter_name, $ttl = 0)
{
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) {

@smarcet smarcet Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return 1;
}
return (int)$conn->incr($counter_name);
Expand Down Expand Up @@ -306,12 +305,11 @@ public function setSingleValue($key, $value, $ttl = 0)
public function addSingleValue($key, $value, $ttl = 0)
{
return $this->retryOnConnectionError(function ($conn) use ($key, $value, $ttl) {
$res = $conn->setnx($key, $value);
if ($res && $ttl > 0) {
$conn->expire($key, $ttl);
if ($ttl > 0) {
return $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null;
}
return $res;
});
return $conn->set($key, $value, 'NX') !== null;
}, false);
}

Comment thread
romanetar marked this conversation as resolved.
public function setKeyExpiration($key, $ttl)
Expand All @@ -331,7 +329,21 @@ public function ttl($key)
return (int)$conn->ttl($key);
}, 0);
}


public function deleteIfValueMatches(string $key, string $expectedValue): bool
{
$lua = <<<'LUA'
if redis.call('get', KEYS[1]) == ARGV[1] then
return redis.call('del', KEYS[1])
else
return 0
end
LUA;
return $this->retryOnConnectionError(function ($conn) use ($lua, $key, $expectedValue) {
return (int)$conn->eval($lua, 1, $key, $expectedValue) === 1;
}, false);
}

/**
* @param string $cache_region_key
* @return void
Expand Down
99 changes: 99 additions & 0 deletions tests/Integration/RedisCacheServiceAddSingleValueTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?php namespace Tests\Integration;
/**
* Copyright 2026 OpenStack Foundation
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
**/

use Illuminate\Support\Facades\Redis;
use PHPUnit\Framework\Attributes\Group;
use services\utils\RedisCacheService;
use Tests\CreatesApplication;
use Tests\TestCase;

/**
* Integration tests for RedisCacheService::addSingleValue.
*
* These tests require a live Redis instance and verify two properties that
* mocks cannot exercise:
*
* 1. Driver compatibility — the variadic SET NX EX form works with the
* configured Predis/PhpRedis driver. If the driver is switched to
* PhpRedis, set() returns false on an NX-miss (not null), which would
* silently break the `!== null` check; this test catches that regression.
*
* 2. Atomicity — key and TTL are written in a single command; there is no
* window where the key exists without a TTL. Verified by reading TTL
* immediately after addSingleValue returns.
*
*/
#[Group("integration")]
final class RedisCacheServiceAddSingleValueTest extends TestCase

@smarcet smarcet Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

{
use CreatesApplication;

private const TEST_KEY = 'test:add_single_value:lock';
private const TTL = 30;

private RedisCacheService $service;
private mixed $redis;

protected function setUp(): void
{
parent::setUp();
$this->redis = Redis::connection();
$this->service = new RedisCacheService();
// Start clean regardless of any leftover from a previous failed run.
$this->redis->del(self::TEST_KEY);
}

protected function tearDown(): void
{
$this->redis->del(self::TEST_KEY);
parent::tearDown();
}

/**
* First call must succeed and leave a TTL on the key.
* Second call on the same key must return false (NX semantics).
*/
public function testAddSingleValueSetsKeyWithTtlAndNxSemanticsHold(): void
{
$token = bin2hex(random_bytes(16));

$acquired = $this->service->addSingleValue(self::TEST_KEY, $token, self::TTL);
$this->assertTrue($acquired, 'first addSingleValue must return true');

// Atomicity: TTL must already be set — no gap between key write and expire.
$ttl = (int)$this->redis->ttl(self::TEST_KEY);
$this->assertGreaterThanOrEqual(1, $ttl, 'key must have a positive TTL immediately after addSingleValue');
$this->assertLessThanOrEqual(self::TTL, $ttl, 'TTL must not exceed the requested lifetime');

// NX semantics: a second call while the key still exists must fail.
$again = $this->service->addSingleValue(self::TEST_KEY, bin2hex(random_bytes(16)), self::TTL);
$this->assertFalse($again, 'addSingleValue must return false when key already exists (NX)');
}

/**
* After the key is deleted the lock can be re-acquired, confirming the
* return-value contract holds across both the true and false branches.
*/
public function testAddSingleValueReturnsTrueAfterKeyIsDeleted(): void
{
$token = bin2hex(random_bytes(16));

$this->assertTrue($this->service->addSingleValue(self::TEST_KEY, $token, self::TTL));
$this->redis->del(self::TEST_KEY);
$this->assertTrue(
$this->service->addSingleValue(self::TEST_KEY, bin2hex(random_bytes(16)), self::TTL),
'addSingleValue must return true once the key has been removed'
);
}
}
Loading
Loading