fix(cache): ensure unique lock acquisition (#1757)

Co-authored-by: Brent Roose <[email protected]>
Co-authored-by: Enzo Innocenzi <[email protected]>

Author: 3 people

packages/cache/src/Cache.php

@@ -24,10 +24,10 @@ interface Cache
      * Returns a lock for the specified key. The lock is not acquired until `acquire()` is called.
      *
      * @param Stringable|string $key The identifier of the lock.
-     * @param null|Duration|DateTimeInterface $expiration The expiration time for the lock. If not specified, the lock will not expire.
+     * @param null|Duration|DateTimeInterface $duration The duration for the lock, or an expiration date from which the duration will be calculated. If not specified, the lock will not expire.
      * @param null|Stringable|string $owner The owner of the lock, which will be used to identify the process releasing it. If not specified, a random string will be used.
      */
-    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $expiration = null, null|Stringable|string $owner = null): Lock;
+    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $duration = null, null|Stringable|string $owner = null): Lock;
 
     /**
      * Sets the specified key to the specified value in the cache. Optionally, specify an expiration.

packages/cache/src/GenericCache.php

@@ -23,17 +23,17 @@ public function __construct(
         public null|UnitEnum|string $tag = null,
     ) {}
 
-    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $expiration = null, null|Stringable|string $owner = null): Lock
+    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $duration = null, null|Stringable|string $owner = null): Lock
     {
-        if ($expiration instanceof Duration) {
-            $expiration = DateTime::now()->plus($expiration);
+        if ($duration instanceof DateTimeInterface) {
+            $duration = $duration->since(DateTime::now());
         }
 
         return new GenericLock(
             key: (string) $key,
             owner: $owner ? (string) $owner : Random\secure_string(length: 10),
             cache: $this,
-            expiration: $expiration,
+            duration: $duration,
         );
     }
 

packages/cache/src/GenericLock.php

@@ -14,7 +14,7 @@ public function __construct(
         private(set) string $key,
         private(set) string $owner,
         private readonly Cache $cache,
-        private(set) ?DateTimeInterface $expiration = null,
+        private(set) ?Duration $duration = null,
     ) {}
 
     public function locked(null|Stringable|string $by = null): bool
@@ -32,13 +32,17 @@ public function acquire(): bool
             return false;
         }
 
+        $expiration = $this->duration !== null
+            ? DateTime::now()->plus($this->duration)
+            : null;
+
         $this->cache->put(
             key: $this->key,
             value: $this->owner,
-            expiration: $this->expiration,
+            expiration: $expiration,
         );
 
-        return true;
+        return $this->cache->get($this->key) === $this->owner;
     }
 
     public function execute(Closure $callback, null|DateTimeInterface|Duration $wait = null): mixed

packages/cache/src/Lock.php

@@ -17,9 +17,9 @@ interface Lock
     }
 
     /**
-     * The expiration date of the lock. If null, the lock will not expire.
+     * The duration of the lock. If null, the lock will not expire.
      */
-    public ?DateTimeInterface $expiration {
+    public ?Duration $duration {
         get;
     }
 

packages/cache/src/Testing/RestrictedCache.php

@@ -25,7 +25,7 @@ private function resolveTag(): ?string
         return $this->tag instanceof UnitEnum ? $this->tag->name : $this->tag;
     }
 
-    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $expiration = null, null|Stringable|string $owner = null): Lock
+    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $duration = null, null|Stringable|string $owner = null): Lock
     {
         throw new CacheUsageWasForbidden($this->resolveTag());
     }

packages/cache/src/Testing/TestingCache.php

@@ -11,6 +11,7 @@
 use Tempest\Cache\Cache;
 use Tempest\Cache\GenericCache;
 use Tempest\Cache\GenericLock;
+use Tempest\DateTime\DateTime;
 use Tempest\DateTime\DateTimeInterface;
 use Tempest\DateTime\Duration;
 use Tempest\Support\Random;
@@ -33,13 +34,17 @@ public function __construct(
         $this->cache = new GenericCache($this->adapter);
     }
 
-    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $expiration = null, null|Stringable|string $owner = null): TestingLock
+    public function lock(Stringable|string $key, null|Duration|DateTimeInterface $duration = null, null|Stringable|string $owner = null): TestingLock
     {
+        if ($duration instanceof DateTimeInterface) {
+            $duration = $duration->since(DateTime::now());
+        }
+
         return new TestingLock(new GenericLock(
             key: (string) $key,
             owner: $owner ? (string) $owner : Random\secure_string(length: 10),
             cache: $this->cache,
-            expiration: $expiration,
+            duration: $duration,
         ));
     }
 
@@ -186,9 +191,9 @@ public function assertNotEmpty(): self
     /**
      * Asserts that the specified lock is being held.
      */
-    public function assertLocked(string|Stringable $key, null|Stringable|string $by = null, null|DateTimeInterface|Duration $until = null): self
+    public function assertLocked(string|Stringable $key, null|Stringable|string $by = null, null|DateTimeInterface|Duration $for = null): self
     {
-        $this->lock($key)->assertLocked($by, $until);
+        $this->lock($key)->assertLocked($by, $for);
 
         return $this;
     }

packages/cache/src/Testing/TestingLock.php

@@ -17,8 +17,8 @@ final class TestingLock implements Lock
         get => $this->lock->key;
     }
 
-    public ?DateTimeInterface $expiration {
-        get => $this->lock->expiration;
+    public ?Duration $duration {
+        get => $this->lock->duration;
     }
 
     public string $owner {
@@ -52,7 +52,7 @@ public function release(bool $force = false): bool
     /**
      * Asserts that the specified lock is being held.
      */
-    public function assertLocked(null|Stringable|string $by = null, null|DateTimeInterface|Duration $until = null): self
+    public function assertLocked(null|Stringable|string $by = null, null|DateTimeInterface|Duration $for = null): self
     {
         Assert::assertTrue(
             condition: $this->locked($by),
@@ -61,19 +61,19 @@ public function assertLocked(null|Stringable|string $by = null, null|DateTimeInt
                 : "Lock `{$this->key}` is not being held.",
         );
 
-        if ($until) {
-            if ($until instanceof Duration) {
-                $until = DateTime::now()->plus($until);
+        if ($for) {
+            if ($for instanceof DateTimeInterface) {
+                $for = $for->since(DateTime::now());
             }
 
             Assert::assertNotNull(
-                actual: $this->expiration,
-                message: "Expected lock `{$this->key}` to have an expiration, but it has none.",
+                actual: $this->duration,
+                message: "Expected lock `{$this->key}` to have a duration, but it has none.",
             );
 
             Assert::assertTrue(
-                condition: $this->expiration->afterOrAtTheSameTime($until),
-                message: "Expected lock `{$this->key}` to expire at or after `{$until}`, but it expires at `{$this->expiration}`.",
+                condition: $this->duration->getTotalSeconds() >= $for->getTotalSeconds(),
+                message: "Expected lock `{$this->key}` to have a duration of at least `{$for->getTotalSeconds()}` seconds, but it has `{$this->duration->getTotalSeconds()}` seconds.",
             );
         }
 

rector.php

@@ -10,7 +10,7 @@
 use Rector\Php70\Rector\StaticCall\StaticCallOnNonStaticToInstanceCallRector;
 use Rector\Php74\Rector\Closure\ClosureToArrowFunctionRector;
 use Rector\Php74\Rector\Property\RestoreDefaultNullToNullableTypePropertyRector;
-use Rector\Php81\Rector\Array_\FirstClassCallableRector;
+use Rector\Php81\Rector\Array_\ArrayToFirstClassCallableRector;
 use Rector\Php81\Rector\FuncCall\NullToStrictStringFuncCallArgRector;
 use Rector\Php81\Rector\Property\ReadOnlyPropertyRector;
 use Rector\Php82\Rector\Class_\ReadOnlyClassRector;
@@ -43,7 +43,7 @@
         ArgumentAdderRector::class,
         ClosureToArrowFunctionRector::class,
         EmptyOnNullableObjectToInstanceOfRector::class,
-        FirstClassCallableRector::class,
+        ArrayToFirstClassCallableRector::class,
         NullToStrictStringFuncCallArgRector::class,
         ReadOnlyClassRector::class,
         ReadOnlyPropertyRector::class,

tests/Integration/Cache/LockTest.php

@@ -61,10 +61,10 @@ public function test_lock_with_ttl(): void
         $clock = $this->clock();
         $cache = new GenericCache(new ArrayAdapter(clock: $clock->toPsrClock()));
 
-        $lock = $cache->lock('processing', expiration: Duration::hours(1));
+        $lock = $cache->lock('processing', duration: Duration::hours(1));
 
         $this->assertTrue($lock->acquire());
-        $this->assertTrue($lock->expiration->equals($clock->now()->plusHours(1)));
+        $this->assertTrue($lock->duration->equals(Duration::hours(1)));
 
         // Still locked after 30 min
         $clock->plus(Duration::minutes(30));
@@ -73,7 +73,7 @@ public function test_lock_with_ttl(): void
         // No longer locked after another 30 min (total 1h)
         $clock->plus(Duration::minutes(30));
         $this->assertTrue($lock->acquire());
-        $this->assertFalse($lock->release());
+        $this->assertTrue($lock->release());
     }
 
     public function test_lock_execution_without_timeout(): void
@@ -106,7 +106,7 @@ public function test_lock_execution_when_already_locked_by_another_owner_with_ti
         $cache = new GenericCache(new ArrayAdapter(clock: $clock->toPsrClock()));
 
         // Lock externally for a set duration
-        $externalLock = $cache->lock('processing', expiration: Duration::hours(1));
+        $externalLock = $cache->lock('processing', duration: Duration::hours(1));
         $externalLock->acquire();
 
         // Skip the set duration
@@ -116,4 +116,31 @@ public function test_lock_execution_when_already_locked_by_another_owner_with_ti
         /** @phpstan-ignore-next-line */
         $this->assertTrue($cache->lock('processing')->execute(fn () => true, wait: Duration::hours(1)));
     }
+
+    public function test_lock_can_be_reacquired_after_expiration(): void
+    {
+        $clock = $this->clock();
+        $cache = new GenericCache(new ArrayAdapter(clock: $clock->toPsrClock()));
+
+        $lock = $cache->lock('processing', duration: Duration::hours(1));
+
+        // Acquire the lock
+        $this->assertTrue($lock->acquire());
+
+        // Skip the lock duration
+        $clock->plus(Duration::hours(1));
+
+        // Lock expired, so we can re-acquire it
+        $this->assertTrue($lock->acquire());
+
+        // Verify the lock is held
+        $this->assertTrue($lock->locked());
+
+        // Skip another hour
+        $clock->plus(Duration::hours(1));
+
+        // Lock expired again, so we can re-acquire it again
+        $this->assertTrue($lock->acquire());
+        $this->assertTrue($lock->release());
+    }
 }