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

.github/workflows/push.yml

@@ -63,12 +63,17 @@ jobs:
                     - { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" }
                     - { name: "AuditEventTypesTest", filter: "--filter AuditEventTypesTest" }
                     - { name: "GuzzleTracingTest", filter: "--filter GuzzleTracingTest" }
-                    - { name: "Repositories", filter: "tests/Repositories/" }
-                    - { name: "Services", filter: "tests/Unit/Services/" }
                     - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" }
                     # 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: "tests/Repositories/" }
+                    - { name: "Services", filter: "tests/Unit/Services/" }
+                    - { name: "Integration", filter: "tests/Integration/" }
+                    # Runs the same suite under PhpRedis - addSingleValue/incCounter's
+                    # SET...NX miss sentinel differs between drivers (null vs false),
+                    # so a driver-agnostic bug there only shows up here.
+                    - { name: "IntegrationPhpRedis", filter: "tests/Integration/", redis_client: "phpredis" }
         env:
             OTEL_SERVICE_ENABLED: false
             APP_ENV: testing
@@ -173,6 +178,8 @@ jobs:
                   COMPOSER_AUTH: '{"github-oauth": {"github.com": "${{ secrets.COMPOSER_AUTH_TOKEN }}"} }'
 
             - name: Run ${{ matrix.suite.name }}
+              env:
+                  REDIS_CLIENT: ${{ matrix.suite.redis_client || 'predis' }}
               run: |
                   ./update_doctrine.sh
                   php artisan db:create_initial_db --schema=config

Libs/Utils/ICacheService.php

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

app/Services/Model/Imp/SummitOrderService.php

@@ -267,6 +267,7 @@ private function buildPrePaidSaga(Member $owner, Summit $summit, array $payload)
                 $this->member_repository,
                 $this->attendee_repository,
                 $this->ticket_type_repository,
+                $this->promo_code_repository,
                 $this->tx_service,
                 $this->lock_service
             ));
@@ -830,7 +831,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
@@ -868,7 +869,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);
 
             });
         }
@@ -953,7 +954,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);
 
             }
         });
@@ -970,7 +971,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);
             });
         }
     }
@@ -1477,6 +1478,11 @@ final class AutoAssignPrePaidTicketTask extends AbstractTask
      */
     private $ticket_type_repository;
 
+    /**
+     * @var ISummitRegistrationPromoCodeRepository
+     */
+    private $promo_code_repository;
+
     /**
      * @var ILockManagerService
      */
@@ -1490,19 +1496,21 @@ final class AutoAssignPrePaidTicketTask extends AbstractTask
      * @param IMemberRepository $member_repository
      * @param ISummitAttendeeRepository $attendee_repository
      * @param ISummitTicketTypeRepository $ticket_type_repository
+     * @param ISummitRegistrationPromoCodeRepository $promo_code_repository
      * @param ITransactionService $tx_service
      * @param ILockManagerService $lock_service
      */
     public function __construct
     (
-        ?Member                     $owner,
-        Summit                      $summit,
-        array                       $payload,
-        IMemberRepository           $member_repository,
-        ISummitAttendeeRepository   $attendee_repository,
-        ISummitTicketTypeRepository $ticket_type_repository,
-        ITransactionService         $tx_service,
-        ILockManagerService         $lock_service
+        ?Member                                 $owner,
+        Summit                                  $summit,
+        array                                   $payload,
+        IMemberRepository                       $member_repository,
+        ISummitAttendeeRepository               $attendee_repository,
+        ISummitTicketTypeRepository             $ticket_type_repository,
+        ISummitRegistrationPromoCodeRepository  $promo_code_repository,
+        ITransactionService                     $tx_service,
+        ILockManagerService                     $lock_service
     )
     {
         $this->tx_service = $tx_service;
@@ -1513,6 +1521,7 @@ public function __construct
         $this->member_repository = $member_repository;
         $this->attendee_repository = $attendee_repository;
         $this->ticket_type_repository = $ticket_type_repository;
+        $this->promo_code_repository = $promo_code_repository;
     }
 
     public function run(array $formerState): array
@@ -1539,8 +1548,8 @@ 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',
-                function () use ($promo_code_val, $type_id) {
+            $order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock',
+                function () use ($promo_code_val, $type_id, $ticket_dto) {
 
                     $attendee_email = $this->owner->getEmail();
                     // use what we have on payload first
@@ -1558,7 +1567,7 @@ function () use ($promo_code_val, $type_id) {
                     if (empty($attendee_last_name))
                         $attendee_last_name = $this->payload['owner_last_name'] ?? $this->owner->getLastName();
 
-                    $promo_code = $this->summit->getPromoCodeByCode($promo_code_val);
+                    $promo_code = $this->promo_code_repository->getByValueExclusiveLock($this->summit, $promo_code_val);
                     if (!PromoCodesUtils::isPrePaidPromoCode($promo_code))
                         throw new EntityNotFoundException("Promo code is not found.");
 
@@ -1661,7 +1670,7 @@ function () use ($promo_code_val, $type_id) {
 
 
                     return $order;
-                });
+                }, 30);
             return ['order' => $order];
         });
     }

app/Services/Utils/ILockManagerService.php

@@ -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;
 }

app/Services/Utils/LockManagerService.php

@@ -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
      */
@@ -41,73 +42,80 @@ 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);
+            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));
+        $released = $this->cache_service->deleteIfValueMatches($name, $token);
+        if (!$released) {
+            Log::warning(sprintf("LockManagerService::releaseLock name %s token %s lock was not held by this token at release time (expired or stolen).", $name, $token));
+            $this->cache_service->incCounter('lock_manager.release_mismatch');
+        }
     }
 
     /**
      * @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;
     }
 
-}
+}

app/Services/Utils/RedisCacheService.php

@@ -239,9 +239,10 @@ 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);
-                return 1;
+            if ($ttl > 0) {
+                if ($this->setNxSucceeded($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX'))) return 1;
+            } else {
+                if ($this->setNxSucceeded($conn->set($counter_name, 1, 'NX'))) return 1;
             }
             return (int)$conn->incr($counter_name);
         }, 0);
@@ -306,12 +307,21 @@ 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 $this->setNxSucceeded($conn->set($key, $value, 'EX', (int)$ttl, 'NX'));
             }
-            return $res;
-        });
+            return $this->setNxSucceeded($conn->set($key, $value, 'NX'));
+        }, false);
+    }
+
+    /**
+     * SET ... NX reports a miss as null under Predis but as false under PhpRedis.
+     * Any real success value (a Predis\Response\Status object, or PhpRedis's true/1)
+     * is truthy against both checks.
+     */
+    private function setNxSucceeded($result): bool
+    {
+        return $result !== null && $result !== false;
     }
 
     public function setKeyExpiration($key, $ttl)
@@ -331,7 +341,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