From c8a69ba0cbbea046098883eb7f3f2e6abe1217b7 Mon Sep 17 00:00:00 2001 From: insign <1113045+insign@users.noreply.github.com> Date: Mon, 9 Feb 2026 03:29:51 +0000 Subject: [PATCH] feat: add atomic locking to progress updates This commit introduces `Cache::lock` to `Progressable` trait methods that perform read-modify-write operations on the progress data. This prevents race conditions when multiple processes update the same overall progress simultaneously. - Added `withLock` helper method to manage cache locks. - Wrapped `updateLocalProgressData`, `setLocalKey`, and `removeLocalFromOverall` in `withLock`. - Added `$isLocked` property to handle re-entrancy within the same instance. - Added `tests/LockingTest.php` to verify locking behavior. --- src/Progressable.php | 105 +++++++++++++++++++++++++++++------------- tests/LockingTest.php | 77 +++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 32 deletions(-) create mode 100644 tests/LockingTest.php diff --git a/src/Progressable.php b/src/Progressable.php index 0997237..cc9f57f 100644 --- a/src/Progressable.php +++ b/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; + }); } /** diff --git a/tests/LockingTest.php b/tests/LockingTest.php new file mode 100644 index 0000000..56e87cc --- /dev/null +++ b/tests/LockingTest.php @@ -0,0 +1,77 @@ +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'); + } +}