Skip to content

Commit ec9beb1

Browse files
tzitzi
committed
Fix DeviceDetectorCache cache eviction flakiness
1 parent a92e044 commit ec9beb1

3 files changed

Lines changed: 104 additions & 23 deletions

File tree

‎CachedEntry.php‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ public static function getCached(string $userAgent): ?array
6060
if ($exists) {
6161
$values = @include($path);
6262
if (!empty($values) && is_array($values) && isset($values['os'])) {
63+
// Keep the filesystem access metadata in sync with cache reads so later eviction
64+
// can preserve entries that were just reused during the warm-cache command.
65+
self::refreshAccessTime($path);
6366
return $values;
6467
}
6568
}
@@ -165,6 +168,26 @@ private static function getCacheFilesInCacheDir(): array
165168
});
166169
}
167170

171+
private static function refreshAccessTime(string $path): void
172+
{
173+
// Read the current mtime first so we can move the cache file's recency marker forward even
174+
// when the file was created earlier in the same second as the cache hit.
175+
$modifiedTime = @filemtime($path);
176+
if ($modifiedTime === false) {
177+
return;
178+
}
179+
180+
// Use a strictly newer timestamp for both mtime and atime so cache eviction has a stable
181+
// ordering even on filesystems with one-second timestamp granularity.
182+
$refreshedTime = max(time(), $modifiedTime + 1);
183+
184+
// Refresh the stat cache around touch() so the subsequent file metadata reads in the same
185+
// PHP process observe the updated recency instead of stale stat data.
186+
clearstatcache(true, $path);
187+
@touch($path, $refreshedTime, $refreshedTime);
188+
clearstatcache(true, $path);
189+
}
190+
168191
public static function deleteLeastAccessedFiles(int $numFilesToDelete): void
169192
{
170193
if ($numFilesToDelete < 1) {
@@ -173,11 +196,13 @@ public static function deleteLeastAccessedFiles(int $numFilesToDelete): void
173196
$files = self::getCacheFilesInCacheDir();
174197
$accessed = [];
175198
foreach ($files as $file) {
176-
$accessed[$file] = fileatime($file);
199+
// Read the same recency marker that getCached() updates so eviction keeps the cache
200+
// entry that was actually reused during the current warm-cache run.
201+
$accessed[$file] = self::getLastAccessTimestamp($file);
177202
}
178203

179204
// have most recently accessed files at the end of the array and delete entries from the beginning of the array
180-
asort($accessed, SORT_NATURAL);
205+
asort($accessed, SORT_NUMERIC);
181206

182207
$numFilesDeleted = 1;
183208
foreach ($accessed as $file => $time) {
@@ -189,4 +214,18 @@ public static function deleteLeastAccessedFiles(int $numFilesToDelete): void
189214
}
190215
}
191216
}
217+
218+
private static function getLastAccessTimestamp(string $path): int
219+
{
220+
// Clear stat cache before reading metadata so the eviction pass sees the timestamp updates
221+
// performed earlier in the same process by refreshAccessTime().
222+
clearstatcache(true, $path);
223+
224+
// Prefer the newest of atime and mtime because refreshAccessTime() updates both values to
225+
// give us a deterministic recency order on filesystems with coarse timestamp precision.
226+
$accessTime = @fileatime($path);
227+
$modifiedTime = @filemtime($path);
228+
229+
return max((int) $accessTime, (int) $modifiedTime);
230+
}
192231
}

‎tests/Integration/CachedEntryTest.php‎

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,18 @@ public function test_writeToCache_GetCached()
190190
);
191191
}
192192

193+
public function testGetCached_refreshesAccessTime()
194+
{
195+
$filePath = CachedEntry::writeToCache('foo', []);
196+
$accessTimeBefore = fileatime($filePath);
197+
198+
sleep(1);
199+
CachedEntry::getCached('foo', []);
200+
clearstatcache(true, $filePath);
201+
202+
$this->assertGreaterThan($accessTimeBefore, fileatime($filePath));
203+
}
204+
193205
public function test_getCachePath()
194206
{
195207
$path1 = CachedEntry::getCachePath('foo');
@@ -224,42 +236,58 @@ public function test_deleteLeastAccessedFiles_nothingToDelete()
224236
public function test_deleteLeastAccessedFiles_deletesOnlyOldest()
225237
{
226238
$filePath1 = CachedEntry::writeToCache('file', []);
227-
sleep(1); // otherwise without sleep the sorting won't work properly
228239
$filePath2 = CachedEntry::writeToCache('bar', []);
229-
sleep(1);
230240
$filePath3 = CachedEntry::writeToCache('baz', []);
231-
sleep(1);
232241
$filePath4 = CachedEntry::writeToCache('foo', []);
233-
sleep(1);
242+
243+
// Set explicit timestamps so the eviction order does not depend on filesystem precision.
244+
$this->setFileTimestamp($filePath1, 100);
245+
$this->setFileTimestamp($filePath2, 200);
246+
$this->setFileTimestamp($filePath3, 300);
247+
$this->setFileTimestamp($filePath4, 400);
234248

235249
CachedEntry::deleteLeastAccessedFiles(2);
236250

237-
$this->assertFileNotExists($filePath1);
238-
$this->assertFileNotExists($filePath2);
251+
$this->assertFileMissing($filePath1);
252+
$this->assertFileMissing($filePath2);
239253
$this->assertFileExists($filePath3);
240254
$this->assertFileExists($filePath4);
241255
}
242256

243257
public function test_deleteLeastAccessedFiles_deletesOnlyOldest2()
244258
{
245259
$filePath1 = CachedEntry::writeToCache('file', []);
246-
sleep(1);
247260
$filePath2 = CachedEntry::writeToCache('bar', []);
248-
sleep(1);
249261
$filePath3 = CachedEntry::writeToCache('baz', []);
250-
sleep(1);
251262
$filePath4 = CachedEntry::writeToCache('foo', []);
252-
sleep(1);
253263

254-
touch($filePath1);
255-
sleep(1);
256-
touch($filePath3);
264+
// Simulate cache hits by moving selected files to the newest timestamps explicitly.
265+
$this->setFileTimestamp($filePath1, 300);
266+
$this->setFileTimestamp($filePath2, 100);
267+
$this->setFileTimestamp($filePath3, 400);
268+
$this->setFileTimestamp($filePath4, 200);
257269

258270
CachedEntry::deleteLeastAccessedFiles(2);
259271

260-
$this->assertFileNotExists($filePath2);
261-
$this->assertFileNotExists($filePath4);
272+
$this->assertFileMissing($filePath2);
273+
$this->assertFileMissing($filePath4);
262274
$this->assertFileExists($filePath1);
263275
$this->assertFileExists($filePath3);
264276
}
277+
278+
private function assertFileMissing(string $path): void
279+
{
280+
if (method_exists($this, 'assertFileDoesNotExist')) {
281+
$this->assertFileDoesNotExist($path);
282+
return;
283+
}
284+
285+
$this->assertFileNotExists($path);
286+
}
287+
288+
private function setFileTimestamp(string $path, int $timestamp): void
289+
{
290+
touch($path, $timestamp, $timestamp);
291+
clearstatcache(true, $path);
292+
}
265293
}

‎tests/Integration/WarmDeviceDetectorCacheTest.php‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,12 @@ public function testDoesNotClearExistingFilesFromCacheByDefault()
106106

107107
public function testDoesClearExistingFilesFromCacheByDefaultWhenTooManyEntriesExist()
108108
{
109-
// was last accessed
110-
$userAgentKept = 'Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; rv:11.0) like Gecko';
111-
$userAgentDeleted = 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36';
109+
// The first user agent is the most frequent entry in useragents1.csv, so the second warm
110+
// run will read it from cache again and should therefore keep it after eviction.
111+
$userAgentKept = 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36';
112+
// This entry is less frequent and should be one of the files evicted once the cache is
113+
// reduced back down to a single entry.
114+
$userAgentDeleted = 'Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; rv:11.0) like Gecko';
112115
$cacheFilePathKept = CachedEntry::getCachePath($userAgentKept, true);
113116
$cacheFilePathDeleted = CachedEntry::getCachePath($userAgentDeleted, true);
114117

@@ -128,15 +131,16 @@ public function testDoesClearExistingFilesFromCacheByDefaultWhenTooManyEntriesEx
128131

129132
$this->setCountProcessNumEntries(1);
130133

131-
// now we run again and it should delete 2 of the entries
134+
// Re-running with a single allowed entry exercises the same request path as production:
135+
// the kept entry is read again, its atime is refreshed, and eviction should delete older files.
132136
$this->applicationTester->run([
133137
'command' => WarmDeviceDetectorCache::COMMAND_NAME,
134138
]);
135139
// should now have deleted 2 files
136140
$this->assertEquals(1, CachedEntry::getNumEntriesInCacheDir());
137141

138142
$this->assertFileExists($cacheFilePathKept);
139-
$this->assertFileNotExists($cacheFilePathDeleted);
143+
$this->assertFileMissing($cacheFilePathDeleted);
140144
}
141145

142146
public function testDoesntProcessAllRowsWhenCounterSet()
@@ -179,7 +183,7 @@ public function testVeryLongUserAgent()
179183
private function assertUserAgentNotWrittenToFile($userAgent)
180184
{
181185
$expectedFilePath = CachedEntry::getCachePath($userAgent);
182-
$this->assertFileNotExists($expectedFilePath);
186+
$this->assertFileMissing($expectedFilePath);
183187
}
184188

185189
private function assertUserAgentWrittenToFile($userAgent)
@@ -204,4 +208,14 @@ private function assertUserAgentWrittenToFile($userAgent)
204208
$this->assertEquals($deviceDetectionParsed->getModel(), $deviceDetectionFromFile->getModel());
205209
$this->assertEquals($deviceDetectionParsed->getOs(), $deviceDetectionFromFile->getOs());
206210
}
211+
212+
private function assertFileMissing(string $path): void
213+
{
214+
if (method_exists($this, 'assertFileDoesNotExist')) {
215+
$this->assertFileDoesNotExist($path);
216+
return;
217+
}
218+
219+
$this->assertFileNotExists($path);
220+
}
207221
}

0 commit comments

Comments
 (0)