Skip to content

Commit 11ce85c

Browse files
Antreesybackportbot[bot]
authored 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. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
1 parent 9ec92a8 commit 11ce85c

2 files changed

Lines changed: 58 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: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,16 @@ public function instanceOfStorage(string $class): bool {
8181
}
8282
}
8383

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

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

0 commit comments

Comments
 (0)