Merge pull request #10 from Vectorial1024/multirun

Fix file cache evictor race condition due to bad iteration style

Author: Vectorial1024

CHANGELOG.md

@@ -2,6 +2,9 @@
 Note: you may refer to `README.md` for description of features.
 
 ## Dev (WIP)
+- Fixed file cache evictor sometimes throwing `UnexpectedValueException` due to race conditions
+  - This could happen when multiple cleaners are running at the same time
+- Minor general codebase cleanup
 
 ## 2.0.1 (2025-01-13)
 - Added fallback of Laravel's `Number::fileSize()` if `ext-intl` is not available

src/AbstractEvictStrategy.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Vectorial1024\LaravelCacheEvict;
 
 use Illuminate\Console\OutputStyle;

src/CacheEvictStrategies.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Vectorial1024\LaravelCacheEvict;
 
 use Vectorial1024\LaravelCacheEvict\Database\DatabaseEvictStrategy;

src/Database/DatabaseEvictStrategy.php

@@ -1,8 +1,10 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Vectorial1024\LaravelCacheEvict\Database;
 
-use Illuminate\Contracts\Cache\Repository;
+use Illuminate\Cache\DatabaseStore;
 use Illuminate\Database\Connection;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\DB;
@@ -16,7 +18,7 @@ class DatabaseEvictStrategy extends AbstractEvictStrategy
 
     protected string $dbTable;
 
-    protected Repository $cacheStore;
+    protected DatabaseStore $cacheStore;
 
     protected int $deletedRecords = 0;
     protected int $deletedRecordSizes = 0;
@@ -31,7 +33,7 @@ public function __construct(string $storeName)
         $storeConn = config("cache.stores.{$storeName}.connection");
         $this->dbConn = DB::connection($storeConn);
         $this->dbTable = config("cache.stores.{$storeName}.table");
-        $this->cacheStore = Cache::store($this->storeName);
+        $this->cacheStore = Cache::store($this->storeName)->getStore();
     }
 
     public function execute(): void
@@ -65,7 +67,7 @@ public function execute(): void
             $currentExpiration = $cacheItem->expiration;
             $currentActualKey = "{$cachePrefix}{$currentUserKey}";
             // currently timestamps are 32-bit, so are 4 bytes
-            $estimatedBytes = $cacheItem->key_bytes + $cacheItem->value_bytes + 4;
+            $estimatedBytes = (int) ($cacheItem->key_bytes + $cacheItem->value_bytes + 4);
             $progressBar->advance();
 
             if (time() < $currentExpiration) {
@@ -103,22 +105,19 @@ public function execute(): void
     protected function yieldCacheTableItems(): \Generator
     {
         // there might be a prefix for the cache store!
-        // not sure how to properly type-cast to DatabaseStore, but this should exist.
         $cachePrefix = $this->cacheStore->getPrefix();
         $currentUserKey = "";
         // loop until no more items
         while (true) {
             // find the next key
             $actualKey = "{$cachePrefix}{$currentUserKey}";
-            // Partyline::info("Checking DB key $actualKey");
             $record = $this->dbConn
                 ->table($this->dbTable)
                 ->select(['key', 'expiration', DB::raw('LENGTH(key) AS key_bytes'), DB::raw('LENGTH(value) AS value_bytes')])
                 ->where('key', '>', $actualKey)
                 ->where('key', 'LIKE', "$cachePrefix%")
                 ->limit(1)
                 ->first();
-            // Partyline::info(var_dump($record));
             if (!$record) {
                 // nothing more to get
                 break;

src/EvictionRefusedFeatureExistsException.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Vectorial1024\LaravelCacheEvict;
 
 use Exception;

src/File/FileEvictStrategy.php

@@ -1,12 +1,15 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Vectorial1024\LaravelCacheEvict\File;
 
 use DirectoryIterator;
 use ErrorException;
 use Illuminate\Contracts\Filesystem\Filesystem;
 use Illuminate\Support\Facades\Storage;
 use Symfony\Component\Console\Helper\ProgressBar;
+use UnexpectedValueException;
 use Vectorial1024\LaravelCacheEvict\AbstractEvictStrategy;
 use Wilderborn\Partyline\Facade as Partyline;
 
@@ -54,30 +57,25 @@ public function execute(): void
         $progressBar = $this->output->createProgressBar();
         $progressBar->setMaxSteps(count($allDirs));
 
-        foreach ($allDirs as $dir) {
-            // we will have some verbose printing for now to test this feature.
+        // since allDir is an array with contents like "0", "0/0", "0/1", ... "1", ...
+        // and we are trying to remove items during iteration
+        // we should iterate it in reverse as per literation best practices
+        // the reversal also cleanly avoids possible race conditions by aligning iteration direction across all cleaners
+        foreach (array_reverse($allDirs) as $dir) {
+            // handle cache files, then delete the directory in the same place
             $this->handleCacheFilesInDirectory($dir);
             $progressBar->advance();
-            // sleep(1);
+            // it's OK if we cannot remove directories; this usually means the directory is not empty.
+            $localPath = $this->filesystem->path($dir);
+            @rmdir($localPath);
+            $this->deletedDirs++;
         }
 
         $progressBar->finish();
         unset($progressBar);
         // progress bar next empty line
         Partyline::info("");
-        Partyline::info("Expired cache files evicted; checking empty directories...");
-        // since allDir is an array with contents like "0", "0/0", "0/1", ... "1", ...
-        // we can reverse it to effectively remove directories
-        // in theory removing directories is very fast, so no progress bars here
-        foreach (array_reverse($allDirs) as $dir) {
-            try {
-                $localPath = $this->filesystem->path($dir);
-                rmdir($localPath);
-                $this->deletedDirs++;
-            } catch (ErrorException) {
-                // it's OK if we cannot remove directories; this usually means the directory is not empty.
-            }
-        }
+        Partyline::info("Expired cache files evicted.");
 
         // all is done; print some stats
         $endUnix = microtime(true);
@@ -89,14 +87,23 @@ public function execute(): void
         Partyline::info("Removed {$this->deletedDirs} empty directories.");
     }
 
-    protected function handleCacheFilesInDirectory(string $dirName)
+    protected function handleCacheFilesInDirectory(string $dirName): void
     {
         $localPath = $this->filesystem->path($dirName);
-        // Partyline::info("Checking $localPath...");
 
         // remove files inside directory
+        try {
+            $dirIter = new DirectoryIterator($localPath);
+        } catch (UnexpectedValueException $x) {
+            // this indicates the directory is gone
+            // this might be caused by race conditions
+            // this should be rare (later execution catching up to earlier run), but better be safe than sorry
+            Partyline::warn("Cache directory {$dirName} was deleted earlier than expected; skipping.");
+            // then we have nothing to do here 
+            return;
+        }
         /** @var \SplFileInfo $fileInfo */
-        foreach (new DirectoryIterator($localPath) as $fileInfo) {
+        foreach ($dirIter as $fileInfo) {
             if ($fileInfo->isDot()) {
                 continue;
             }
@@ -106,7 +113,6 @@ protected function handleCacheFilesInDirectory(string $dirName)
 
             $realPath = $fileInfo->getRealPath();
             $shortFileName = $dirName . DIRECTORY_SEPARATOR . $fileInfo->getFilename();
-            // Partyline::info("Checking file $realPath...");
             try {
                 // read expiry
                 // the first 10 characters form the expiry timestamp
@@ -115,10 +121,8 @@ protected function handleCacheFilesInDirectory(string $dirName)
                 $expiry = (int) file_get_contents($realPath, length: 10);
                 if (time() < $expiry) {
                     // not expired yet
-                    // Partyline::info("Not expired");
                     continue;
                 }
-                // Partyline::info("Expired");
             } catch (ErrorException) {
                 // it's OK if we cannot read the file, this can happen when e.g. the cache file is deleted by other Laravel code
                 Partyline::warn("Could not read details of cache file $shortFileName; skipping.");