Skip to content

Commit 86c2c73

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #61977 from nextcloud/backport/59539/stable34
[stable34] fix(files_external): S3 folder mtime updated on every read-only access
2 parents 2eeeac1 + 7b60056 commit 86c2c73

4 files changed

Lines changed: 142 additions & 7 deletions

File tree

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

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,11 @@ private function isRoot(string $path): bool {
7070
return $path === '.';
7171
}
7272

73+
private function getCachePath(string $path): string {
74+
// normalizePath() converts '' to '.' for S3 object keys, but filecache stores the root as ''
75+
return $this->isRoot($path) ? '' : $path;
76+
}
77+
7378
private function cleanKey(string $path): string {
7479
if ($this->isRoot($path)) {
7580
return '/';
@@ -326,6 +331,28 @@ public function stat(string $path): array|false {
326331
return $stat;
327332
}
328333

334+
#[\Override]
335+
public function getMetaData(string $path): ?array {
336+
$data = parent::getMetaData($path);
337+
if ($data !== null && $data['mimetype'] === FileInfo::MIMETYPE_FOLDER) {
338+
// Common::getMetaData sets storage_mtime = mtime, but for S3 virtual directories
339+
// mtime may have been updated by mtime propagation while storage_mtime should
340+
// reflect the actual last storage change. Without this override the scanner sees
341+
// data['storage_mtime'] != cacheData['storage_mtime'] and re-writes the cache,
342+
// causing View::getCacheEntry to trigger propagateChange on every read.
343+
$path = $this->normalizePath($path);
344+
$cacheEntry = $this->getCache()->get($this->getCachePath($path));
345+
if ($cacheEntry instanceof CacheEntry) {
346+
$data['storage_mtime'] = $cacheEntry->getStorageMTime();
347+
} elseif (!$this->isRoot($path) && $directoryMarker = $this->headObject($path . '/')) {
348+
if (isset($directoryMarker['LastModified'])) {
349+
$data['storage_mtime'] = strtotime($directoryMarker['LastModified']);
350+
}
351+
}
352+
}
353+
return $data;
354+
}
355+
329356
#[\Override]
330357
public function is_dir(string $path): bool {
331358
$path = $this->normalizePath($path);
@@ -649,12 +676,14 @@ public function getDirectoryContent(string $directory): \Traversable {
649676
}
650677

651678
private function objectToMetaData(array $object): array {
679+
$mtime = isset($object['LastModified']) ? strtotime($object['LastModified']) : time();
680+
$etag = isset($object['ETag']) ? trim($object['ETag'], '"') : '';
652681
return [
653682
'name' => basename($object['Key']),
654683
'mimetype' => $this->mimeDetector->detectPath($object['Key']),
655-
'mtime' => strtotime($object['LastModified']),
656-
'storage_mtime' => strtotime($object['LastModified']),
657-
'etag' => trim($object['ETag'], '"'),
684+
'mtime' => $mtime,
685+
'storage_mtime' => $mtime,
686+
'etag' => $etag,
658687
'permissions' => Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE,
659688
'size' => (int)($object['Size'] ?? $object['ContentLength']),
660689
];
@@ -667,7 +696,7 @@ private function getDirectoryMetaData(string $path): ?array {
667696
if ($this->versioningEnabled() && !$this->doesDirectoryExist($path)) {
668697
return null;
669698
}
670-
$cacheEntry = $this->getCache()->get($path);
699+
$cacheEntry = $this->getCache()->get($this->getCachePath($path));
671700
if ($cacheEntry instanceof CacheEntry) {
672701
return $cacheEntry->getData();
673702
} else {

‎apps/files_external/tests/Storage/Amazons3Test.php‎

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*/
99
namespace OCA\Files_External\Tests\Storage;
1010

11+
use OC\Files\Cache\Scanner;
1112
use OCA\Files_External\Lib\Storage\AmazonS3;
1213

1314
/**
@@ -38,7 +39,55 @@ protected function tearDown(): void {
3839
parent::tearDown();
3940
}
4041

41-
public function testStat(): void {
42-
$this->markTestSkipped('S3 doesn\'t update the parents folder mtime');
42+
/**
43+
* Regression test for the '.' vs '' root path mismatch in getDirectoryMetaData.
44+
*
45+
* normalizePath('') returns '.' for S3 object keys, but the filecache stores the
46+
* storage root under the key ''. Before the fix, getCache()->get('.') returned false,
47+
* causing getDirectoryMetaData to return a fabricated time() on every call, which
48+
* made getCacheEntry always see a changed storage_mtime and fire propagateChange.
49+
*/
50+
public function testStatRootPreservesStorageMtimeFromCache(): void {
51+
$this->instance->getScanner()->scan('', Scanner::SCAN_SHALLOW);
52+
53+
$cachedRoot = $this->instance->getCache()->get('');
54+
$this->assertNotFalse($cachedRoot, 'Root entry must exist in cache after scan');
55+
56+
$cachedStorageMtime = $cachedRoot['storage_mtime'];
57+
58+
$stat = $this->instance->stat('');
59+
$this->assertNotFalse($stat, 'stat(\'\') must return data');
60+
$this->assertEquals(
61+
$cachedStorageMtime,
62+
$stat['storage_mtime'],
63+
'stat(\'\') must return storage_mtime from the cache entry, not a fabricated time()'
64+
);
65+
}
66+
67+
/**
68+
* Regression test: Common::getMetaData sets storage_mtime = mtime, but for S3 virtual
69+
* directories mtime may have been bumped by propagation while storage_mtime should stay
70+
* stable. The override restores storage_mtime from the cache entry so the scanner does
71+
* not see a spurious mismatch and re-write the cache on every scan.
72+
*/
73+
public function testGetMetaDataDirectoryPreservesStorageMtimeSeparateFromMtime(): void {
74+
$this->instance->getScanner()->scan('', Scanner::SCAN_SHALLOW);
75+
76+
$cachedRoot = $this->instance->getCache()->get('');
77+
$this->assertNotFalse($cachedRoot, 'Root entry must exist in cache after scan');
78+
79+
// Simulate propagation bumping mtime without touching storage_mtime
80+
$originalStorageMtime = $cachedRoot['storage_mtime'];
81+
$this->instance->getCache()->update($cachedRoot->getId(), [
82+
'mtime' => $originalStorageMtime + 9999,
83+
]);
84+
85+
$meta = $this->instance->getMetaData('');
86+
$this->assertNotNull($meta, 'getMetaData(\'\') must return data');
87+
$this->assertEquals(
88+
$originalStorageMtime,
89+
$meta['storage_mtime'],
90+
'getMetaData must return storage_mtime from cache, not the propagated mtime'
91+
);
4392
}
4493
}

‎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)