Skip to content

Fix DeviceDetectorCache cache eviction flakiness #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
tzi wants to merge 1 commit into
base: 5.x-dev
Choose a base branch
from
Draft

Fix DeviceDetectorCache cache eviction flakiness #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions CachedEntry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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);
}
}
58 changes: 43 additions & 15 deletions tests/Integration/CachedEntryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -224,42 +236,58 @@ 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);
}

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);
}
}
26 changes: 20 additions & 6 deletions tests/Integration/WarmDeviceDetectorCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -128,15 +131,16 @@ 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,
]);
// should now have deleted 2 files
$this->assertEquals(1, CachedEntry::getNumEntriesInCacheDir());

$this->assertFileExists($cacheFilePathKept);
$this->assertFileNotExists($cacheFilePathDeleted);
$this->assertFileMissing($cacheFilePathDeleted);
}

public function testDoesntProcessAllRowsWhenCounterSet()
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}
}
Loading