Add atomic locking to prevent race conditions in progress updates

src/Progressable.php

@@ -90,6 +90,41 @@ trait Progressable {
      */
     protected mixed $onComplete = null;
 
+    /**
+     * Indicates if the process is currently locked.
+     */
+    protected bool $isLocked = false;
+
+    /**
+     * Execute a callback within a cache lock.
+     */
+    protected function withLock(callable $callback): mixed
+    {
+        if ($this->customSaveData !== null || $this->customGetData !== null) {
+            return $callback();
+        }
+
+        if ($this->isLocked) {
+            return $callback();
+        }
+
+        $key = $this->getStorageKeyName().'_lock';
+
+        try {
+            return Cache::lock($key, 5)->block(5, function () use ($callback) {
+                $this->isLocked = true;
+
+                try {
+                    return $callback();
+                } finally {
+                    $this->isLocked = false;
+                }
+            });
+        } catch (\Exception $e) {
+            throw $e;
+        }
+    }
+
     /**
      * Set the callback function for saving cache data.
      *
@@ -244,17 +279,19 @@ public function isOverallComplete(): bool {
      * @throws UniqueNameNotSetException
      */
     public function removeLocalFromOverall(): static {
-        $progressData = $this->getOverallProgressData();
-        $localKey = $this->getLocalKey();
+        return $this->withLock(function () {
+            $progressData = $this->getOverallProgressData();
+            $localKey = $this->getLocalKey();
 
-        if (isset($progressData[$localKey])) {
-            unset($progressData[$localKey]);
-            $this->saveOverallProgressData($progressData);
-        }
+            if (isset($progressData[$localKey])) {
+                unset($progressData[$localKey]);
+                $this->saveOverallProgressData($progressData);
+            }
 
-        $this->progress = 0;
+            $this->progress = 0;
 
-        return $this;
+            return $this;
+        });
     }
 
     /**
@@ -287,23 +324,25 @@ public function setLocalProgress(float $progress): static {
      * Update the progress data in storage.
      */
     protected function updateLocalProgressData(float $progress): static {
-        $progressData = $this->getOverallProgressData();
+        return $this->withLock(function () use ($progress) {
+            $progressData = $this->getOverallProgressData();
 
-        $localData = [
-            'progress' => $progress,
-        ];
+            $localData = [
+                'progress' => $progress,
+            ];
 
-        if ($this->statusMessage !== null) {
-            $localData['message'] = $this->statusMessage;
-        }
+            if ($this->statusMessage !== null) {
+                $localData['message'] = $this->statusMessage;
+            }
 
-        if (! empty($this->metadata)) {
-            $localData['metadata'] = $this->metadata;
-        }
+            if (! empty($this->metadata)) {
+                $localData['metadata'] = $this->metadata;
+            }
 
-        $progressData[$this->getLocalKey()] = $localData;
+            $progressData[$this->getLocalKey()] = $localData;
 
-        return $this->saveOverallProgressData($progressData);
+            return $this->saveOverallProgressData($progressData);
+        });
     }
 
     /**
@@ -319,21 +358,23 @@ public function getLocalKey(): string {
      * @param  string  $name  The new local key name
      */
     public function setLocalKey(string $name): static {
-        $currentKey = $this->getLocalKey();
-        $overallProgressData = $this->getOverallProgressData();
-
-        if (isset($overallProgressData[$currentKey])) {
-            // Rename the local key preserving the data
-            $overallProgressData[$name] = $overallProgressData[$currentKey];
-            unset($overallProgressData[$currentKey]);
-            $this->saveOverallProgressData($overallProgressData);
-        }
+        return $this->withLock(function () use ($name) {
+            $currentKey = $this->getLocalKey();
+            $overallProgressData = $this->getOverallProgressData();
 
-        $this->localKey = $name;
+            if (isset($overallProgressData[$currentKey])) {
+                // Rename the local key preserving the data
+                $overallProgressData[$name] = $overallProgressData[$currentKey];
+                unset($overallProgressData[$currentKey]);
+                $this->saveOverallProgressData($overallProgressData);
+            }
 
-        $this->makeSureLocalIsPartOfTheCalc();
+            $this->localKey = $name;
 
-        return $this;
+            $this->makeSureLocalIsPartOfTheCalc();
+
+            return $this;
+        });
     }
 
     /**

tests/LockingTest.php

@@ -0,0 +1,77 @@
+<?php
+
+namespace Verseles\Progressable\Tests;
+
+use Illuminate\Support\Facades\Cache;
+use Orchestra\Testbench\TestCase;
+use Verseles\Progressable\Progressable;
+use Mockery;
+
+class LockingTest extends TestCase
+{
+    use Progressable;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->overallUniqueName = 'test_locking';
+        // Reset trait state
+        $this->isLocked = false;
+        $this->customSaveData = null;
+        $this->customGetData = null;
+        $this->progress = 0;
+        $this->metadata = [];
+        $this->statusMessage = null;
+    }
+
+    public function test_update_local_progress_uses_lock()
+    {
+        $lockMock = Mockery::mock(\Illuminate\Contracts\Cache\Lock::class);
+        $lockMock->shouldReceive('block')
+            ->once()
+            ->with(5, Mockery::type('callable'))
+            ->andReturnUsing(function ($seconds, $callback) {
+                return $callback();
+            });
+
+        Cache::shouldReceive('lock')
+            ->once()
+            ->with('progressable_test_locking_lock', 5)
+            ->andReturn($lockMock);
+
+        Cache::shouldReceive('get')
+            ->once()
+            ->andReturn([]);
+
+        Cache::shouldReceive('put')
+            ->once();
+
+        $this->setLocalProgress(50);
+    }
+
+    public function test_nested_locks_are_avoided()
+    {
+        $lockMock = Mockery::mock(\Illuminate\Contracts\Cache\Lock::class);
+        $lockMock->shouldReceive('block')
+            ->once()
+            ->with(5, Mockery::type('callable'))
+            ->andReturnUsing(function ($seconds, $callback) {
+                return $callback();
+            });
+
+        // The key for lock will be based on overallUniqueName
+        Cache::shouldReceive('lock')
+            ->once()
+            ->with('progressable_test_locking_lock', 5)
+            ->andReturn($lockMock);
+
+        Cache::shouldReceive('get')
+            ->atLeast()->once()
+            ->andReturn([]);
+
+        Cache::shouldReceive('put')
+            ->atLeast()->once();
+
+        $this->setLocalKey('new_key');
+    }
+}