Fix DeviceDetectorCache cache eviction flakiness

Author: tzi

CachedEntry.php

@@ -60,6 +60,9 @@ public static function getCached(string $userAgent): ?array
         if ($exists) {
             $values = @include($path);
             if (!empty($values) && is_array($values) && isset($values['os'])) {
+                // Keep the filesystem access metadata in sync with cache reads so later eviction
+                // can preserve entries that were just reused during the warm-cache command.
+                self::refreshAccessTime($path);
                 return $values;
             }
         }
@@ -165,6 +168,26 @@ private static function getCacheFilesInCacheDir(): array
         });
     }
 
+    private static function refreshAccessTime(string $path): void
+    {
+        // Read the current mtime first so we can move the cache file's recency marker forward even
+        // when the file was created earlier in the same second as the cache hit.
+        $modifiedTime = @filemtime($path);
+        if ($modifiedTime === false) {
+            return;
+        }
+
+        // Use a strictly newer timestamp for both mtime and atime so cache eviction has a stable
+        // ordering even on filesystems with one-second timestamp granularity.
+        $refreshedTime = max(time(), $modifiedTime + 1);
+
+        // Refresh the stat cache around touch() so the subsequent file metadata reads in the same
+        // PHP process observe the updated recency instead of stale stat data.
+        clearstatcache(true, $path);
+        @touch($path, $refreshedTime, $refreshedTime);
+        clearstatcache(true, $path);
+    }
+
     public static function deleteLeastAccessedFiles(int $numFilesToDelete): void
     {
         if ($numFilesToDelete < 1) {
@@ -173,7 +196,9 @@ public static function deleteLeastAccessedFiles(int $numFilesToDelete): void
         $files = self::getCacheFilesInCacheDir();
         $accessed = [];
         foreach ($files as $file) {
-            $accessed[$file] = fileatime($file);
+            // Read the same recency marker that getCached() updates so eviction keeps the cache
+            // entry that was actually reused during the current warm-cache run.
+            $accessed[$file] = self::getLastAccessTimestamp($file);
         }
 
         // have most recently accessed files at the end of the array and delete entries from the beginning of the array
@@ -189,4 +214,18 @@ public static function deleteLeastAccessedFiles(int $numFilesToDelete): void
             }
         }
     }
+
+    private static function getLastAccessTimestamp(string $path): int
+    {
+        // Clear stat cache before reading metadata so the eviction pass sees the timestamp updates
+        // performed earlier in the same process by refreshAccessTime().
+        clearstatcache(true, $path);
+
+        // Prefer the newest of atime and mtime because refreshAccessTime() updates both values to
+        // give us a deterministic recency order on filesystems with coarse timestamp precision.
+        $accessTime = @fileatime($path);
+        $modifiedTime = @filemtime($path);
+
+        return max((int) $accessTime, (int) $modifiedTime);
+    }
 }

tests/Integration/CachedEntryTest.php

@@ -190,6 +190,18 @@ public function test_writeToCache_GetCached()
         );
     }
 
+    public function testGetCached_refreshesAccessTime()
+    {
+        $filePath = CachedEntry::writeToCache('foo', []);
+        $accessTimeBefore = fileatime($filePath);
+
+        sleep(1);
+        CachedEntry::getCached('foo', []);
+        clearstatcache(true, $filePath);
+
+        $this->assertGreaterThan($accessTimeBefore, fileatime($filePath));
+    }
+
     public function test_getCachePath()
     {
         $path1 = CachedEntry::getCachePath('foo');

tests/Integration/WarmDeviceDetectorCacheTest.php

@@ -106,9 +106,12 @@ public function testDoesNotClearExistingFilesFromCacheByDefault()
 
     public function testDoesClearExistingFilesFromCacheByDefaultWhenTooManyEntriesExist()
     {
-        // was last accessed
-        $userAgentKept     = 'Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; rv:11.0) like Gecko';
-        $userAgentDeleted  = 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36';
+        // The first user agent is the most frequent entry in useragents1.csv, so the second warm
+        // run will read it from cache again and should therefore keep it after eviction.
+        $userAgentKept = 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36';
+        // This entry is less frequent and should be one of the files evicted once the cache is
+        // reduced back down to a single entry.
+        $userAgentDeleted = 'Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; rv:11.0) like Gecko';
         $cacheFilePathKept = CachedEntry::getCachePath($userAgentKept, true);
         $cacheFilePathDeleted = CachedEntry::getCachePath($userAgentDeleted, true);
 
@@ -128,7 +131,8 @@ public function testDoesClearExistingFilesFromCacheByDefaultWhenTooManyEntriesEx
 
         $this->setCountProcessNumEntries(1);
 
-        // now we run again and it should delete 2 of the entries
+        // Re-running with a single allowed entry exercises the same request path as production:
+        // the kept entry is read again, its atime is refreshed, and eviction should delete older files.
         $this->applicationTester->run([
             'command' => WarmDeviceDetectorCache::COMMAND_NAME,
         ]);