diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index a45019b98f9de..2699385af4cbf 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -70,6 +70,11 @@ private function isRoot(string $path): bool { return $path === '.'; } + private function getCachePath(string $path): string { + // normalizePath() converts '' to '.' for S3 object keys, but filecache stores the root as '' + return $this->isRoot($path) ? '' : $path; + } + private function cleanKey(string $path): string { if ($this->isRoot($path)) { return '/'; @@ -326,6 +331,28 @@ public function stat(string $path): array|false { return $stat; } + #[\Override] + public function getMetaData(string $path): ?array { + $data = parent::getMetaData($path); + if ($data !== null && $data['mimetype'] === FileInfo::MIMETYPE_FOLDER) { + // Common::getMetaData sets storage_mtime = mtime, but for S3 virtual directories + // mtime may have been updated by mtime propagation while storage_mtime should + // reflect the actual last storage change. Without this override the scanner sees + // data['storage_mtime'] != cacheData['storage_mtime'] and re-writes the cache, + // causing View::getCacheEntry to trigger propagateChange on every read. + $path = $this->normalizePath($path); + $cacheEntry = $this->getCache()->get($this->getCachePath($path)); + if ($cacheEntry instanceof CacheEntry) { + $data['storage_mtime'] = $cacheEntry->getStorageMTime(); + } elseif (!$this->isRoot($path) && $directoryMarker = $this->headObject($path . '/')) { + if (isset($directoryMarker['LastModified'])) { + $data['storage_mtime'] = strtotime($directoryMarker['LastModified']); + } + } + } + return $data; + } + #[\Override] public function is_dir(string $path): bool { $path = $this->normalizePath($path); @@ -649,12 +676,14 @@ public function getDirectoryContent(string $directory): \Traversable { } private function objectToMetaData(array $object): array { + $mtime = isset($object['LastModified']) ? strtotime($object['LastModified']) : time(); + $etag = isset($object['ETag']) ? trim($object['ETag'], '"') : ''; return [ 'name' => basename($object['Key']), 'mimetype' => $this->mimeDetector->detectPath($object['Key']), - 'mtime' => strtotime($object['LastModified']), - 'storage_mtime' => strtotime($object['LastModified']), - 'etag' => trim($object['ETag'], '"'), + 'mtime' => $mtime, + 'storage_mtime' => $mtime, + 'etag' => $etag, 'permissions' => Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE, 'size' => (int)($object['Size'] ?? $object['ContentLength']), ]; @@ -667,7 +696,7 @@ private function getDirectoryMetaData(string $path): ?array { if ($this->versioningEnabled() && !$this->doesDirectoryExist($path)) { return null; } - $cacheEntry = $this->getCache()->get($path); + $cacheEntry = $this->getCache()->get($this->getCachePath($path)); if ($cacheEntry instanceof CacheEntry) { return $cacheEntry->getData(); } else { diff --git a/apps/files_external/tests/Storage/Amazons3Test.php b/apps/files_external/tests/Storage/Amazons3Test.php index 90e36d5fdbd74..7a1c86aabf556 100644 --- a/apps/files_external/tests/Storage/Amazons3Test.php +++ b/apps/files_external/tests/Storage/Amazons3Test.php @@ -8,6 +8,7 @@ */ namespace OCA\Files_External\Tests\Storage; +use OC\Files\Cache\Scanner; use OCA\Files_External\Lib\Storage\AmazonS3; /** @@ -38,7 +39,56 @@ protected function tearDown(): void { parent::tearDown(); } - public function testStat(): void { - $this->markTestSkipped('S3 doesn\'t update the parents folder mtime'); + /** + * Regression test for the '.' vs '' root path mismatch in getDirectoryMetaData. + * + * normalizePath('') returns '.' for S3 object keys, but the filecache stores the + * storage root under the key ''. Before the fix, getCache()->get('.') returned false, + * causing getDirectoryMetaData to return a fabricated time() on every call, which + * made getCacheEntry always see a changed storage_mtime and fire propagateChange. + */ + public function testStatRootPreservesStorageMtimeFromCache(): void { + $this->instance->getScanner()->scan('', Scanner::SCAN_SHALLOW); + + $cachedRoot = $this->instance->getCache()->get(''); + $this->assertNotFalse($cachedRoot, 'Root entry must exist in cache after scan'); + + $cachedStorageMtime = $cachedRoot['storage_mtime']; + + $stat = $this->instance->stat(''); + $this->assertNotFalse($stat, 'stat(\'\') must return data'); + $this->assertEquals( + $cachedStorageMtime, + $stat['storage_mtime'], + 'stat(\'\') must return storage_mtime from the cache entry, not a fabricated time()' + ); } + + /** + * Regression test: Common::getMetaData sets storage_mtime = mtime, but for S3 virtual + * directories mtime may have been bumped by propagation while storage_mtime should stay + * stable. The override restores storage_mtime from the cache entry so the scanner does + * not see a spurious mismatch and re-write the cache on every scan. + */ + public function testGetMetaDataDirectoryPreservesStorageMtimeSeparateFromMtime(): void { + $this->instance->getScanner()->scan('', Scanner::SCAN_SHALLOW); + + $cachedRoot = $this->instance->getCache()->get(''); + $this->assertNotFalse($cachedRoot, 'Root entry must exist in cache after scan'); + + // Simulate propagation bumping mtime without touching storage_mtime + $originalStorageMtime = $cachedRoot['storage_mtime']; + $this->instance->getCache()->update($cachedRoot->getId(), [ + 'mtime' => $originalStorageMtime + 9999, + ]); + + $meta = $this->instance->getMetaData(''); + $this->assertNotNull($meta, 'getMetaData(\'\') must return data'); + $this->assertEquals( + $originalStorageMtime, + $meta['storage_mtime'], + 'getMetaData must return storage_mtime from cache, not the propagated mtime' + ); + } + } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index da3900bff4a66..95c6cf602905d 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1410,9 +1410,17 @@ private function getCacheEntry($storage, $internalPath, $relativePath) { $data = $cache->get($internalPath); } elseif (!Scanner::isPartialFile($internalPath) && $watcher->needsUpdate($internalPath, $data)) { $this->lockFile($relativePath, ILockingProvider::LOCK_SHARED); + $cacheDataBefore = $data instanceof CacheEntry ? $data->getData() : false; $watcher->update($internalPath, $data); - $storage->getPropagator()->propagateChange($internalPath, time()); $data = $cache->get($internalPath); + $cacheDataAfter = $data instanceof CacheEntry ? $data->getData() : false; + + // Only propagate mtime change to parent folders if the scanner actually changed the cached metadata, + // to avoid updating folder mtimes on every read for backends that conservatively report directories as updated (e.g. S3) + if ($cacheDataAfter !== $cacheDataBefore) { + $storage->getPropagator()->propagateChange($internalPath, time()); + $data = $cache->get($internalPath); + } $this->unlockFile($relativePath, ILockingProvider::LOCK_SHARED); } } catch (LockedException $e) { diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index feb7800c12845..0b8a5dcb1a288 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -80,6 +80,16 @@ public function instanceOfStorage(string $class): bool { } } +/** + * Storage that always reports hasUpdated() = true for any path, simulating the + * behavior of Amazon S3 external storage (which has no real directory objects). + */ +class TemporaryAlwaysUpdated extends Temporary { + public function hasUpdated(string $path, int $time): bool { + return true; + } +} + class TestEventHandler { public function umount() { } @@ -456,6 +466,46 @@ public function testWatcher(): void { $this->assertEquals(3, $cachedData['size']); } + /** + * Regression test for View::getCacheEntry unconditionally calling propagateChange + * when the watcher reports needsUpdate = true, regardless of whether the scanner + * found any actual storage change. + * + * Backends like Amazon S3 always return hasUpdated() = true for directory paths + * because S3 has no real directory objects. Before the fix, every getFileInfo() + * call on a folder fired propagateChange(path, time()), stamping all ancestor + * folders in the filecache with the current request timestamp. + * + * After the fix, propagateChange is only called when watcher->update() actually + * changes the cached metadata for the path. + */ + public function testWatcherDoesNotPropagateWhenStorageMtimeUnchanged(): void { + $storage = $this->getTestStorage(true, TemporaryAlwaysUpdated::class); + Filesystem::mount($storage, [], '/'); + $storage->getWatcher()->setPolicy(Watcher::CHECK_ALWAYS); + + $rootView = new View(''); + + // Note the root mtime right after the initial scan. + $rootMtimeBefore = $storage->getCache()->get('')['mtime']; + + // Access a subfolder. The watcher will fire (hasUpdated always returns true), + // but the scanner leaves the cached metadata for 'folder' unchanged. + // getCacheEntry must therefore NOT call propagateChange('folder', time()), + // which would update the root entry's mtime to the current timestamp. + $rootView->getFileInfo('folder'); + + // Read the root mtime directly from the cache to avoid triggering another watcher cycle. + $rootMtimeAfter = $storage->getCache()->get('')['mtime']; + + $this->assertEquals( + $rootMtimeBefore, + $rootMtimeAfter, + 'Root folder mtime must not be updated when the watcher fires but cached metadata has not changed' + ); + } + + public function testCopyBetweenStorageNoCross(): void { $storage1 = $this->getTestStorage(true, TemporaryNoCross::class); $storage2 = $this->getTestStorage(true, TemporaryNoCross::class);