Skip to content

Commit 82c63a3

Browse files
AntreesyCopilot
andcommitted
fix(View#getCacheEntry): skip propagation when storage metadata is unchanged
Storage backends like AmazonS3 whose hasUpdated() always returns true (S3 has no cheap global change-detection mechanism) caused propagateChange() to be called on every watcher->update() after a folder was browsed, even when the underlying storage_mtime and etag were identical to the cached values. Before the fix, getCacheEntry() would call watcher->update() and then unconditionally call propagateChange() whenever the watcher reported a change. For S3 directories this meant every read bumped the parent mtime chain. After the fix, getCacheEntry() snapshots the cache entry before watcher->update() and compares it with the entry after. propagateChange() is only invoked when at least one metadata field (mtime, storage_mtime, size, or etag) actually changed. Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 57c47d5 commit 82c63a3

2 files changed

Lines changed: 59 additions & 1 deletion

File tree

lib/private/Files/View.php

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1410,9 +1410,17 @@ private function getCacheEntry($storage, $internalPath, $relativePath) {
14101410
$data = $cache->get($internalPath);
14111411
} elseif (!Scanner::isPartialFile($internalPath) && $watcher->needsUpdate($internalPath, $data)) {
14121412
$this->lockFile($relativePath, ILockingProvider::LOCK_SHARED);
1413+
$cacheDataBefore = $data instanceof CacheEntry ? $data->getData() : false;
14131414
$watcher->update($internalPath, $data);
1414-
$storage->getPropagator()->propagateChange($internalPath, time());
14151415
$data = $cache->get($internalPath);
1416+
$cacheDataAfter = $data instanceof CacheEntry ? $data->getData() : false;
1417+
1418+
// Only propagate mtime change to parent folders if the scanner actually changed the cached metadata,
1419+
// to avoid updating folder mtimes on every read for backends that conservatively report directories as updated (e.g. S3)
1420+
if ($cacheDataAfter !== $cacheDataBefore) {
1421+
$storage->getPropagator()->propagateChange($internalPath, time());
1422+
$data = $cache->get($internalPath);
1423+
}
14161424
$this->unlockFile($relativePath, ILockingProvider::LOCK_SHARED);
14171425
}
14181426
} catch (LockedException $e) {

tests/lib/Files/ViewTest.php

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,16 @@ public function instanceOfStorage(string $class): bool {
8080
}
8181
}
8282

83+
/**
84+
* Storage that always reports hasUpdated() = true for any path, simulating the
85+
* behavior of Amazon S3 external storage (which has no real directory objects).
86+
*/
87+
class TemporaryAlwaysUpdated extends Temporary {
88+
public function hasUpdated(string $path, int $time): bool {
89+
return true;
90+
}
91+
}
92+
8393
class TestEventHandler {
8494
public function umount() {
8595
}
@@ -456,6 +466,46 @@ public function testWatcher(): void {
456466
$this->assertEquals(3, $cachedData['size']);
457467
}
458468

469+
/**
470+
* Regression test for View::getCacheEntry unconditionally calling propagateChange
471+
* when the watcher reports needsUpdate = true, regardless of whether the scanner
472+
* found any actual storage change.
473+
*
474+
* Backends like Amazon S3 always return hasUpdated() = true for directory paths
475+
* because S3 has no real directory objects. Before the fix, every getFileInfo()
476+
* call on a folder fired propagateChange(path, time()), stamping all ancestor
477+
* folders in the filecache with the current request timestamp.
478+
*
479+
* After the fix, propagateChange is only called when watcher->update() actually
480+
* changes the cached metadata for the path.
481+
*/
482+
public function testWatcherDoesNotPropagateWhenStorageMtimeUnchanged(): void {
483+
$storage = $this->getTestStorage(true, TemporaryAlwaysUpdated::class);
484+
Filesystem::mount($storage, [], '/');
485+
$storage->getWatcher()->setPolicy(Watcher::CHECK_ALWAYS);
486+
487+
$rootView = new View('');
488+
489+
// Note the root mtime right after the initial scan.
490+
$rootMtimeBefore = $storage->getCache()->get('')['mtime'];
491+
492+
// Access a subfolder. The watcher will fire (hasUpdated always returns true),
493+
// but the scanner leaves the cached metadata for 'folder' unchanged.
494+
// getCacheEntry must therefore NOT call propagateChange('folder', time()),
495+
// which would update the root entry's mtime to the current timestamp.
496+
$rootView->getFileInfo('folder');
497+
498+
// Read the root mtime directly from the cache to avoid triggering another watcher cycle.
499+
$rootMtimeAfter = $storage->getCache()->get('')['mtime'];
500+
501+
$this->assertEquals(
502+
$rootMtimeBefore,
503+
$rootMtimeAfter,
504+
'Root folder mtime must not be updated when the watcher fires but cached metadata has not changed'
505+
);
506+
}
507+
508+
459509
public function testCopyBetweenStorageNoCross(): void {
460510
$storage1 = $this->getTestStorage(true, TemporaryNoCross::class);
461511
$storage2 = $this->getTestStorage(true, TemporaryNoCross::class);

0 commit comments

Comments
 (0)