Skip to content

Commit f337dee

Browse files
ChristophWurstChristophWurst
authored
Merge pull request #59539 from nextcloud/fix/noid/s3-mtime-debug
fix(files_external): S3 folder mtime updated on every read-only access
2 parents 3aac8be + 5cb2786 commit f337dee

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
@@ -71,6 +71,11 @@ private function isRoot(string $path): bool {
7171
return $path === '.';
7272
}
7373

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

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

650677
private function objectToMetaData(array $object): array {
678+
$mtime = isset($object['LastModified']) ? strtotime($object['LastModified']) : time();
679+
$etag = isset($object['ETag']) ? trim($object['ETag'], '"') : '';
651680
return [
652681
'name' => basename($object['Key']),
653682
'mimetype' => $this->mimeDetector->detectPath($object['Key']),
654-
'mtime' => strtotime($object['LastModified']),
655-
'storage_mtime' => strtotime($object['LastModified']),
656-
'etag' => trim($object['ETag'], '"'),
683+
'mtime' => $mtime,
684+
'storage_mtime' => $mtime,
685+
'etag' => $etag,
657686
'permissions' => Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE,
658687
'size' => (int)($object['Size'] ?? $object['ContentLength']),
659688
];
@@ -666,7 +695,7 @@ private function getDirectoryMetaData(string $path): ?array {
666695
if ($this->versioningEnabled() && !$this->doesDirectoryExist($path)) {
667696
return null;
668697
}
669-
$cacheEntry = $this->getCache()->get($path);
698+
$cacheEntry = $this->getCache()->get($this->getCachePath($path));
670699
if ($cacheEntry instanceof CacheEntry) {
671700
return $cacheEntry->getData();
672701
} else {

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

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
namespace OCA\Files_External\Tests\Storage;
1111

12+
use OC\Files\Cache\Scanner;
1213
use OCA\Files_External\Lib\Storage\AmazonS3;
1314

1415
/**
@@ -39,7 +40,55 @@ protected function tearDown(): void {
3940
parent::tearDown();
4041
}
4142

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

‎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
}
@@ -443,6 +453,45 @@ public function testWatcher(): void {
443453
$this->assertEquals(3, $cachedData['size']);
444454
}
445455

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

0 commit comments

Comments
 (0)