From ec9beb1ea34f573a728f3b0a9427817edfc04da4 Mon Sep 17 00:00:00 2001 From: Thomas ZILLIOX Date: Wed, 11 Mar 2026 21:30:51 +0100 Subject: [PATCH] Fix DeviceDetectorCache cache eviction flakiness --- CachedEntry.php | 43 +++++++++++++- tests/Integration/CachedEntryTest.php | 58 ++++++++++++++----- .../WarmDeviceDetectorCacheTest.php | 26 +++++++-- 3 files changed, 104 insertions(+), 23 deletions(-) diff --git a/CachedEntry.php b/CachedEntry.php index 5291e71a..926eebc7 100644 --- a/CachedEntry.php +++ b/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,11 +196,13 @@ 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 - asort($accessed, SORT_NATURAL); + asort($accessed, SORT_NUMERIC); $numFilesDeleted = 1; foreach ($accessed as $file => $time) { @@ -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); + } } diff --git a/tests/Integration/CachedEntryTest.php b/tests/Integration/CachedEntryTest.php index c2e29043..e855aceb 100644 --- a/tests/Integration/CachedEntryTest.php +++ b/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'); @@ -224,18 +236,20 @@ public function test_deleteLeastAccessedFiles_nothingToDelete() public function test_deleteLeastAccessedFiles_deletesOnlyOldest() { $filePath1 = CachedEntry::writeToCache('file', []); - sleep(1); // otherwise without sleep the sorting won't work properly $filePath2 = CachedEntry::writeToCache('bar', []); - sleep(1); $filePath3 = CachedEntry::writeToCache('baz', []); - sleep(1); $filePath4 = CachedEntry::writeToCache('foo', []); - sleep(1); + + // Set explicit timestamps so the eviction order does not depend on filesystem precision. + $this->setFileTimestamp($filePath1, 100); + $this->setFileTimestamp($filePath2, 200); + $this->setFileTimestamp($filePath3, 300); + $this->setFileTimestamp($filePath4, 400); CachedEntry::deleteLeastAccessedFiles(2); - $this->assertFileNotExists($filePath1); - $this->assertFileNotExists($filePath2); + $this->assertFileMissing($filePath1); + $this->assertFileMissing($filePath2); $this->assertFileExists($filePath3); $this->assertFileExists($filePath4); } @@ -243,23 +257,37 @@ public function test_deleteLeastAccessedFiles_deletesOnlyOldest() public function test_deleteLeastAccessedFiles_deletesOnlyOldest2() { $filePath1 = CachedEntry::writeToCache('file', []); - sleep(1); $filePath2 = CachedEntry::writeToCache('bar', []); - sleep(1); $filePath3 = CachedEntry::writeToCache('baz', []); - sleep(1); $filePath4 = CachedEntry::writeToCache('foo', []); - sleep(1); - touch($filePath1); - sleep(1); - touch($filePath3); + // Simulate cache hits by moving selected files to the newest timestamps explicitly. + $this->setFileTimestamp($filePath1, 300); + $this->setFileTimestamp($filePath2, 100); + $this->setFileTimestamp($filePath3, 400); + $this->setFileTimestamp($filePath4, 200); CachedEntry::deleteLeastAccessedFiles(2); - $this->assertFileNotExists($filePath2); - $this->assertFileNotExists($filePath4); + $this->assertFileMissing($filePath2); + $this->assertFileMissing($filePath4); $this->assertFileExists($filePath1); $this->assertFileExists($filePath3); } + + private function assertFileMissing(string $path): void + { + if (method_exists($this, 'assertFileDoesNotExist')) { + $this->assertFileDoesNotExist($path); + return; + } + + $this->assertFileNotExists($path); + } + + private function setFileTimestamp(string $path, int $timestamp): void + { + touch($path, $timestamp, $timestamp); + clearstatcache(true, $path); + } } diff --git a/tests/Integration/WarmDeviceDetectorCacheTest.php b/tests/Integration/WarmDeviceDetectorCacheTest.php index 8b3f760c..bcf9f8e5 100644 --- a/tests/Integration/WarmDeviceDetectorCacheTest.php +++ b/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, ]); @@ -136,7 +140,7 @@ public function testDoesClearExistingFilesFromCacheByDefaultWhenTooManyEntriesEx $this->assertEquals(1, CachedEntry::getNumEntriesInCacheDir()); $this->assertFileExists($cacheFilePathKept); - $this->assertFileNotExists($cacheFilePathDeleted); + $this->assertFileMissing($cacheFilePathDeleted); } public function testDoesntProcessAllRowsWhenCounterSet() @@ -179,7 +183,7 @@ public function testVeryLongUserAgent() private function assertUserAgentNotWrittenToFile($userAgent) { $expectedFilePath = CachedEntry::getCachePath($userAgent); - $this->assertFileNotExists($expectedFilePath); + $this->assertFileMissing($expectedFilePath); } private function assertUserAgentWrittenToFile($userAgent) @@ -204,4 +208,14 @@ private function assertUserAgentWrittenToFile($userAgent) $this->assertEquals($deviceDetectionParsed->getModel(), $deviceDetectionFromFile->getModel()); $this->assertEquals($deviceDetectionParsed->getOs(), $deviceDetectionFromFile->getOs()); } + + private function assertFileMissing(string $path): void + { + if (method_exists($this, 'assertFileDoesNotExist')) { + $this->assertFileDoesNotExist($path); + return; + } + + $this->assertFileNotExists($path); + } }