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

[romanetar]

ref https://app.clickup.com/t/86b9f3a22

Recommended actions for a follow-up ticket:

1. Move the SendAttendeeInvitationEmail::dispatch call outside the lock callback (dispatch after the lock is released).
2. Consider a tighter explicit lifetime (e.g. 30 s) that matches the realistic worst-case DB write time rather than the 3600 s default.
3. Fix the missing . in the key: 'ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock'.

Summary by CodeRabbit

• New Features

  • Added atomic compare-and-delete operation for cache management.
  • Implemented ownership-based lock system using tokens to prevent unauthorized lock release.
  • Introduced exponential backoff retry strategy for lock acquisition.

• Tests

  • Added test coverage for lock ownership verification and cleanup behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR introduces ownership-based distributed locking by declaring an atomic deleteIfValueMatches interface method, implementing it via Lua scripts in Redis, and refactoring LockManagerService to generate per-lock random tokens, track ownership, apply exponential backoff on acquisition retries, and only permit release when ownership token matches—preventing lock-stealing between concurrent processes.

Changes

Ownership-Based Distributed Locking

Layer / File(s)	Summary
Interface Contract Libs/Utils/ICacheService.php	New deleteIfValueMatches(string $key, string $expectedValue): bool method declares atomic compare-and-delete semantics for conditional key deletion.
Atomic Implementation app/Services/Utils/RedisCacheService.php	deleteIfValueMatches uses Lua eval script for atomic value equality check and conditional deletion. incCounter and addSingleValue updated to use Redis SET ... NX with optional EX for atomic set-if-missing operations.
Ownership Tracking & Exponential Backoff app/Services/Utils/LockManagerService.php	acquireLock() generates random token per attempt, retries with exponential backoff (BackOffBaseInterval * BackOffMultiplier ** attempt), and stores token in private $tokens map on success. releaseLock() becomes ownership-aware: verifies token ownership and calls deleteIfValueMatches to atomically release only when tokens match. lock() wrapper acquires flag $acquired and conditionally releases only on successful acquisition.
Tests & Verification tests/Unit/Services/LockManagerServiceOwnershipTest.php	New test class validates ownership semantics: failed acquirer cannot delete another process's lock, release without acquisition is a no-op, tokens are cleared after successful lock cycles, and token/lifetime are passed correctly to addSingleValue.

Sequence Diagram

sequenceDiagram
    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 (Success Path)
    Client->>LMS: acquireLock(name, lifetime)
    LMS->>LMS: Generate token = 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: Acquisition complete
        else Retry needed
            RCS-->>LMS: false
            LMS->>LMS: Sleep with exponential backoff
        end
    end
    end

    rect rgba(200, 150, 100, 0.5)
    Note over Client,Redis: Lock Release (Ownership Check)
    Client->>LMS: releaseLock(name)
    LMS->>LMS: Check $tokens[name] exists?
    alt Owned by this process
        LMS->>RCS: deleteIfValueMatches(name, token)
        RCS->>Redis: EVAL Lua script (compare & delete)
        Redis->>Redis: GET name == token?
        alt Values match
            Redis->>Redis: DEL name
            Redis-->>RCS: 1 (deleted)
        else Mismatch (lock stolen)
            Redis-->>RCS: 0 (not deleted)
        end
        RCS-->>LMS: true/false
        LMS->>LMS: Unset $tokens[name]
        LMS-->>Client: Released
    else Not owned
        LMS-->>Client: Return (no-op)
    end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 A rabbit hops through locks so grand,
With tokens bright in paw so hand,
No theft allowed 'tween processes fair—
Each owner guards with Lua's care,
Exponential bounces, atomic delight! 🔐✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title refers to implementing the Redlock pattern, but the changes actually implement a simpler single-token ownership model with exponential backoff retry logic, not the full Redlock algorithm (which requires multiple independent Redis instances).	Revise the title to accurately reflect the actual implementation, e.g., 'fix(lock): add token-based ownership and retry logic to LockManagerService' or similar.
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.82% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/release-on-failed-acquire-and-add-ownership-tokens

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 PHPStan (2.1.51)

PHPStan was skipped because the sandbox runner could not parse its output.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 0/1 reviews remaining, refill in 60 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-537/

This page is automatically updated on each push to this PR.

[coderabbitai]

🧹 Nitpick comments (1)

app/Services/Utils/LockManagerService.php (1)

62-70: 💤 Low value

Minor: Unnecessary sleep before throwing on final retry.

When attempt >= MaxRetries - 1, the code still executes usleep before 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

📥 Commits

Reviewing files that changed from the base of the PR and between c39160a and b3dbd7a.

📒 Files selected for processing (4)

• Libs/Utils/ICacheService.php
• app/Services/Utils/LockManagerService.php
• app/Services/Utils/RedisCacheService.php
• tests/Unit/Services/LockManagerServiceOwnershipTest.php