fix(files_external): S3 folder mtime updated on every read-only access

apps/files_external/lib/Lib/Storage/AmazonS3.php

@@ -320,6 +320,22 @@ public function stat(string $path): array|false {
 		return $stat;
 	}
 
+	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.
+			$stat = $this->stat($path);
+			if (isset($stat['storage_mtime'])) {
+				$data['storage_mtime'] = $stat['storage_mtime'];
+			}
+		}
+		return $data;
+	}
+
 	public function is_dir(string $path): bool {
 		$path = $this->normalizePath($path);
 
@@ -644,7 +660,9 @@ private function getDirectoryMetaData(string $path): ?array {
 		if ($this->versioningEnabled() && !$this->doesDirectoryExist($path)) {
 			return null;
 		}
-		$cacheEntry = $this->getCache()->get($path);
+		// normalizePath() converts '' to '.' for S3 object keys, but filecache stores the root as ''
+		$cachePath = $path === '.' ? '' : $path;
+		$cacheEntry = $this->getCache()->get($cachePath);
 		if ($cacheEntry instanceof CacheEntry) {
 			return $cacheEntry->getData();
 		} else {
@@ -687,8 +705,9 @@ protected function getVersioningStatusFromBucket(): bool {
 	}
 
 	public function hasUpdated(string $path, int $time): bool {
+		$path = $this->normalizePath($path);
 		// for files we can get the proper mtime
-		if ($path !== '' && $object = $this->headObject($path)) {
+		if (!$this->isRoot($path) && $object = $this->headObject($path)) {
 			$stat = $this->objectToMetaData($object);
 			return $stat['mtime'] > $time;
 		} else {

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,59 @@ 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 for Common::getMetaData discarding storage_mtime for S3 directories.
+	 *
+	 * Common::getMetaData sets storage_mtime = mtime unconditionally. For S3 virtual
+	 * directories, mtime can be updated by mtime propagation while storage_mtime reflects
+	 * the actual last storage change. Without the AmazonS3::getMetaData override the
+	 * scanner would see a spurious storage_mtime change on every read, triggering
+	 * propagateChange and stamping every ancestor folder with the current timestamp.
+	 */
+	public function testGetMetaDataDirectoryPreservesStorageMtimeSeparateFromMtime(): void {
+		$this->instance->mkdir('testmtimedir');
+		$this->instance->getScanner()->scan('testmtimedir', Scanner::SCAN_SHALLOW);
+
+		$cachedEntry = $this->instance->getCache()->get('testmtimedir');
+		$this->assertNotFalse($cachedEntry);
+		$originalStorageMtime = $cachedEntry['storage_mtime'];
+
+		// Simulate what mtime propagation does: bump mtime without touching storage_mtime.
+		// This mirrors the state after a child file is written and propagateChange fires.
+		$bumpedMtime = $originalStorageMtime + 100;
+		$this->instance->getCache()->put('testmtimedir', ['mtime' => $bumpedMtime]);
+
+		$data = $this->instance->getMetaData('testmtimedir');
+		$this->assertNotNull($data);
+		$this->assertEquals(
+			$originalStorageMtime,
+			$data['storage_mtime'],
+			'getMetaData for an S3 directory must return the cached storage_mtime, not the (possibly propagation-updated) mtime'
+		);
 	}
 }

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);
+				$storageMtimeBefore = $data['storage_mtime'] ?? null;
 				$watcher->update($internalPath, $data);
-				$storage->getPropagator()->propagateChange($internalPath, time());
 				$data = $cache->get($internalPath);
+				$storageMtimeAfter = $data['storage_mtime'] ?? null;
+
+				// Only propagate mtime change to parent folders if the storage actually reported a change,
+				// to avoid updating folder mtimes on every read for backends that unconditionally report directories as updated (e.g. S3)
+				if ($storageMtimeAfter !== $storageMtimeBefore) {
+					$storage->getPropagator()->propagateChange($internalPath, time());
+					$data = $cache->get($internalPath);
+				}
 				$this->unlockFile($relativePath, ILockingProvider::LOCK_SHARED);
 			}
 		} catch (LockedException $e) {

tests/lib/Files/ViewTest.php

@@ -76,6 +76,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() {
 	}
@@ -448,6 +458,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 storage_mtime actually
+	 * changed after watcher->update().
+	 */
+	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 finds that storage_mtime for 'folder' has not changed.
+		// 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 storage_mtime has not changed'
+		);
+	}
+
+
 	public function testCopyBetweenStorageNoCross(): void {
 		$storage1 = $this->getTestStorage(true, TemporaryNoCross::class);
 		$storage2 = $this->getTestStorage(true, TemporaryNoCross::class);