From fc71e859aa833de47acae89b5d8112f3a7da0abc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 27 Jul 2026 14:49:52 +0200 Subject: [PATCH 1/3] perf(cache): keep host local asset and mimetype caches in the local tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The image paths of the active theme and the id <-> mimetype map were both stored in the distributed memory cache, although both are derived purely from the files and the database of the instance running the request. A value that only makes sense for one host does not belong behind a network socket, so both now use the host local tier via LocalCacheFactory. Neither cache had a TTL and neither is invalidated on the nodes that did not cause the change, so both got one: without it a moved installation or a repaired mimetype table would keep serving the old values indefinitely. This also bounds the staleness that comes with the local tier, where occ can only clear the cache of the node it runs on. Two coherence problems around the mimetype cache come along with it: - OC\Repair\RepairMimeTypes deletes rows from the mimetypes table and never invalidated the cache in front of it, so the id of a deleted mimetype stayed cached. It now takes an optional IMimeTypeLoader and resets it when it actually repaired something. - occ upgrade cleared the cache through ICacheFactory::create(), which only ever reaches the distributed tier. It now clears both. URLGenerator resolves its cache once in the constructor instead of on every imagePath() call. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41734 | 11 ++++ core/Command/Upgrade.php | 18 +++++-- lib/private/Files/Type/Loader.php | 22 +++++--- lib/private/Repair.php | 2 +- lib/private/Repair/RepairMimeTypes.php | 23 ++++++++- lib/private/URLGenerator.php | 21 +++++--- tests/lib/Files/Type/LoaderTest.php | 44 +++++++++++++--- tests/lib/Repair/RepairMimeTypesTest.php | 41 ++++++++++++++- tests/lib/UrlGeneratorTest.php | 65 +++++++++++++++++++++++- 9 files changed, 219 insertions(+), 28 deletions(-) create mode 100644 changelog/unreleased/41734 diff --git a/changelog/unreleased/41734 b/changelog/unreleased/41734 new file mode 100644 index 000000000000..b126871f9925 --- /dev/null +++ b/changelog/unreleased/41734 @@ -0,0 +1,11 @@ +Change: Keep host local caches in the local cache tier + +The image paths of the active theme and the mimetype id map were stored in the +distributed memory cache although both are derived from the files and the +database of a single instance. They now use the host local cache tier and their +entries expire, so a stale entry is scoped to one node and no longer lives +forever. The repair step for mimetypes deletes rows from the mimetype table and +now clears the mimetype cache afterwards, and occ upgrade clears both cache +tiers instead of only the distributed one. + +https://github.com/owncloud/core/pull/41734 diff --git a/core/Command/Upgrade.php b/core/Command/Upgrade.php index 19d529975f2f..549cae3a5b0d 100644 --- a/core/Command/Upgrade.php +++ b/core/Command/Upgrade.php @@ -30,6 +30,7 @@ namespace OC\Core\Command; use OC\Console\TimestampFormatter; +use OC\Memcache\LocalCacheFactory; use OC\Updater; use OCP\IConfig; use OCP\ILogger; @@ -277,10 +278,10 @@ function ($success) use ($output) { } // Clear caches after successful upgrade. - // Caches were created before the upgrade, so the cache prefix will be the old one - // TODO: Note that only the "create" method is available in the interface. It isn't - // possible to create local or distributed caches explicitly - $this->cacheFactory->create()->clear(); + // Caches were created before the upgrade, so the cache prefix will be the old one. + // Note that clearing the local cache only reaches the node running occ - other + // nodes rely on the TTLs the individual caches set on their entries. + $this->clearCaches(); return self::ERROR_SUCCESS; } elseif ($this->config->getSystemValue('maintenance', false)) { //Possible scenario: ownCloud core is updated but an app failed @@ -296,6 +297,15 @@ function ($success) use ($output) { } } + /** + * Clear both cache tiers - host local values (image paths, mime types, ...) + * live in the local tier, everything else in the distributed one. + */ + private function clearCaches() { + $this->cacheFactory->create()->clear(); + LocalCacheFactory::create($this->cacheFactory)->clear(); + } + /** * Perform a post upgrade check (specific to the command line tool) * diff --git a/lib/private/Files/Type/Loader.php b/lib/private/Files/Type/Loader.php index 3fd774575c12..4bc49f1ee020 100644 --- a/lib/private/Files/Type/Loader.php +++ b/lib/private/Files/Type/Loader.php @@ -22,6 +22,7 @@ namespace OC\Files\Type; use Doctrine\DBAL\Exception; +use OC\Memcache\LocalCacheFactory; use OCP\Files\IMimeTypeLoader; use OCP\IDBConnection; use OCP\ICacheFactory; @@ -38,6 +39,13 @@ class Loader implements IMimeTypeLoader { public const CACHE_PREFIX_FOR_ID = ':id:'; public const CACHE_PREFIX_FOR_MIME = ':mime:'; + /** + * The mimetype table only ever grows, so a cached entry cannot become + * wrong - but reset() only clears the cache of the node it runs on, so + * entries need to expire for the others to pick up a repair. + */ + public const CACHE_TTL = 24 * 3600; + /** @var IDBConnection */ private $dbConnection; @@ -55,7 +63,7 @@ class Loader implements IMimeTypeLoader { */ public function __construct(IDBConnection $dbConnection, ICacheFactory $cacheFactory) { $this->dbConnection = $dbConnection; - $this->memcache = $cacheFactory->create('mimetypes'); + $this->memcache = LocalCacheFactory::create($cacheFactory, 'mimetypes'); $this->mimetypes = []; $this->mimetypeIds = []; } @@ -171,8 +179,8 @@ protected function store($mimetype) { $r->free(); // update cache - $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $mimetype); - $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $mimetype, $row['id']); + $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $mimetype, self::CACHE_TTL); + $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $mimetype, $row['id'], self::CACHE_TTL); // update local vars $this->mimetypes[$row['id']] = $mimetype; @@ -232,8 +240,8 @@ private function getIdFromDB($mimetype) { $id = $row['id']; // update cache - $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype']); - $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id']); + $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype'], self::CACHE_TTL); + $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id'], self::CACHE_TTL); // update local vars $this->mimetypes[$row['id']] = $row['mimetype']; @@ -266,8 +274,8 @@ private function getMimetypeFromDB($id) { $mimetype = $row['mimetype']; // update cache - $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype']); - $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id']); + $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype'], self::CACHE_TTL); + $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id'], self::CACHE_TTL); // update local vars $this->mimetypes[$row['id']] = $row['mimetype']; diff --git a/lib/private/Repair.php b/lib/private/Repair.php index dc5310a740dc..63248aa43aec 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -126,7 +126,7 @@ public function addStep($repairStep) { */ public static function getRepairSteps() { return [ - new RepairMimeTypes(\OC::$server->getConfig()), + new RepairMimeTypes(\OC::$server->getConfig(), \OC::$server->getMimeTypeLoader()), new RepairMismatchFileCachePath( \OC::$server->getDatabaseConnection(), \OC::$server->getMimeTypeLoader(), diff --git a/lib/private/Repair/RepairMimeTypes.php b/lib/private/Repair/RepairMimeTypes.php index 2afbeefce8ea..e06c032a9ce8 100644 --- a/lib/private/Repair/RepairMimeTypes.php +++ b/lib/private/Repair/RepairMimeTypes.php @@ -29,6 +29,7 @@ namespace OC\Repair; +use OCP\Files\IMimeTypeLoader; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; @@ -38,6 +39,11 @@ class RepairMimeTypes implements IRepairStep { */ protected $config; + /** + * @var IMimeTypeLoader + */ + protected $mimeTypeLoader; + /** * @var int */ @@ -45,9 +51,11 @@ class RepairMimeTypes implements IRepairStep { /** * @param \OCP\IConfig $config + * @param IMimeTypeLoader|null $mimeTypeLoader */ - public function __construct($config) { + public function __construct($config, IMimeTypeLoader $mimeTypeLoader = null) { $this->config = $config; + $this->mimeTypeLoader = $mimeTypeLoader ?? \OC::$server->getMimeTypeLoader(); } public function getName() { @@ -313,12 +321,15 @@ private function introduceRichDocumentsMimeTypes() { */ public function run(IOutput $out) { $ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); + $repaired = false; // NOTE TO DEVELOPERS: when adding new mime types, please make sure to // add a version comparison to avoid doing it every time // only update mime types if necessary as it can be expensive if (\version_compare($ocVersionFromBeforeUpdate, '8.2.0', '<')) { + $repaired = true; + $this->fixOfficeMimeTypes(); $out->info('Fixed office mime types'); @@ -346,6 +357,7 @@ public function run(IOutput $out) { // Mimetype updates from #19272 if (\version_compare($ocVersionFromBeforeUpdate, '8.2.0.8', '<')) { + $repaired = true; $this->introduceJavaMimeType(); $out->info('Fixed java/class mime types'); @@ -360,8 +372,17 @@ public function run(IOutput $out) { } if (\version_compare($ocVersionFromBeforeUpdate, '9.0.0.10', '<')) { + $repaired = true; $this->introduceRichDocumentsMimeTypes(); $out->info('Fixed richdocuments additional office mime types'); } + + if ($repaired) { + // rows have been inserted into and deleted from the mimetypes table, + // so anything holding on to the old id <-> mimetype mapping has to + // let go of it + $this->mimeTypeLoader->reset(); + $out->info('Cleared the mime type cache'); + } } } diff --git a/lib/private/URLGenerator.php b/lib/private/URLGenerator.php index 34abd493fc9e..48c3aa696808 100644 --- a/lib/private/URLGenerator.php +++ b/lib/private/URLGenerator.php @@ -32,8 +32,10 @@ namespace OC; use OC\Helper\EnvironmentHelper; +use OC\Memcache\LocalCacheFactory; use OCP\Theme\ITheme; use OC_Defaults; +use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; use OCP\IURLGenerator; @@ -44,10 +46,18 @@ * Class to generate URLs */ class URLGenerator implements IURLGenerator { + /** + * Image paths depend on the server root and the theme, so they are cached + * per node. The TTL only exists as a safety net - nothing invalidates these + * entries, so without it a moved installation would keep serving the old + * paths for the lifetime of the cache. + */ + private const IMAGE_PATH_TTL = 24 * 3600; + /** @var IConfig */ private $config; - /** @var ICacheFactory */ - private $cacheFactory; + /** @var ICache */ + private $imagePathCache; /** @var IRouter */ private $router; /** @var ITheme */ @@ -69,7 +79,7 @@ public function __construct( EnvironmentHelper $environmentHelper ) { $this->config = $config; - $this->cacheFactory = $cacheFactory; + $this->imagePathCache = LocalCacheFactory::create($cacheFactory, 'imagePath'); $this->router = $router; $this->environmentHelper = $environmentHelper; $this->theme = \OC_Util::getTheme(); @@ -159,16 +169,15 @@ public function linkTo($app, $file, $args = []) { * Returns the path to the image. */ public function imagePath($app, $image) { - $cache = $this->cacheFactory->create('imagePath'); $cacheKey = $this->theme->getName().'-'.$app.'-'.$image; - if ($key = $cache->get($cacheKey)) { + if ($key = $this->imagePathCache->get($cacheKey)) { return $key; } $path = $this->getImagePath($app, $image); if ($path !== '' && $path !== null) { - $cache->set($cacheKey, $path); + $this->imagePathCache->set($cacheKey, $path, self::IMAGE_PATH_TTL); return $path; } else { throw new RuntimeException( diff --git a/tests/lib/Files/Type/LoaderTest.php b/tests/lib/Files/Type/LoaderTest.php index 5fd7efef4b3b..124e50a42bda 100644 --- a/tests/lib/Files/Type/LoaderTest.php +++ b/tests/lib/Files/Type/LoaderTest.php @@ -23,8 +23,8 @@ use OC\Files\Type\Loader; use OCP\IDBConnection; -use OCP\ICacheFactory; use OCP\ICache; +use Test\Memcache\FixedCacheFactory; class LoaderTest extends \Test\TestCase { /** @var IDBConnection */ @@ -33,16 +33,15 @@ class LoaderTest extends \Test\TestCase { protected $loader; /** @var ICache */ protected $memcache; + /** @var FixedCacheFactory */ + protected $cacheFactory; protected function setUp(): void { $this->memcache = $this->createMock(ICache::class); - $cacheFactoryMock = $this->createMock(ICacheFactory::class); - $cacheFactoryMock->expects($this->once()) - ->method('create') - ->willReturn($this->memcache); + $this->cacheFactory = new FixedCacheFactory($this->memcache); $this->db = \OC::$server->getDatabaseConnection(); - $this->loader = new Loader($this->db, $cacheFactoryMock); + $this->loader = new Loader($this->db, $this->cacheFactory); } protected function tearDown(): void { @@ -99,4 +98,37 @@ public function testStoreExists() { $this->assertEquals($mimetypeId, $mimetypeId2); } + + /** + * The mimetype map is derived from the database of this instance, so it + * belongs in the local tier - not in a distributed cache that anything able + * to reach it could remap. + */ + public function testUsesTheLocalCacheTier() { + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->once()) + ->method('createLocal') + ->with('mimetypes') + ->willReturn($this->createMock(ICache::class)); + $cacheFactory->expects($this->never())->method('createDistributed'); + $cacheFactory->expects($this->never())->method('create'); + + new Loader($this->db, $cacheFactory); + } + + /** + * Nothing invalidates the cache of the other nodes, so entries have to + * expire on their own. + */ + public function testCachedEntriesExpire() { + $this->memcache->expects($this->atLeastOnce()) + ->method('set') + ->with( + $this->anything(), + $this->anything(), + Loader::CACHE_TTL + ); + + $this->loader->getId('testing/mymimetype'); + } } diff --git a/tests/lib/Repair/RepairMimeTypesTest.php b/tests/lib/Repair/RepairMimeTypesTest.php index e922543b727d..4cb9624ff129 100644 --- a/tests/lib/Repair/RepairMimeTypesTest.php +++ b/tests/lib/Repair/RepairMimeTypesTest.php @@ -51,7 +51,7 @@ protected function setUp(): void { $this->storage = new Temporary([]); - $this->repair = new RepairMimeTypes($config); + $this->repair = new RepairMimeTypes($config, $this->mimetypeLoader); } protected function tearDown(): void { @@ -461,6 +461,45 @@ public function testRenameFontsMimeTypesWhenExist() { $this->assertNull($this->getMimeTypeIdFromDB('font/opentype')); } + /** + * The repair step deletes rows from the mimetypes table, so it has to drop + * the id <-> mimetype mapping the loader caches - otherwise the mapping of + * a deleted mimetype survives the repair. + */ + public function testMimeTypeCacheIsResetAfterRepair() { + /** @var IMimeTypeLoader | MockObject $loader */ + $loader = $this->createMock(IMimeTypeLoader::class); + $loader->expects($this->once())->method('reset'); + + /** @var IConfig | MockObject $config */ + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue')->with('version')->willReturn('8.0.0.0'); + + /** @var IOutput | MockObject $outputMock */ + $outputMock = $this->createMock(IOutput::class); + + (new RepairMimeTypes($config, $loader))->run($outputMock); + } + + /** + * Nothing was changed, so there is no reason to throw the mimetype cache of + * every node away. + */ + public function testMimeTypeCacheIsKeptWhenNothingIsRepaired() { + /** @var IMimeTypeLoader | MockObject $loader */ + $loader = $this->createMock(IMimeTypeLoader::class); + $loader->expects($this->never())->method('reset'); + + /** @var IConfig | MockObject $config */ + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue')->with('version')->willReturn('10.0.0.0'); + + /** @var IOutput | MockObject $outputMock */ + $outputMock = $this->createMock(IOutput::class); + + (new RepairMimeTypes($config, $loader))->run($outputMock); + } + /** * Test that nothing happens and no error happens when all mimetypes are * already correct and no old ones exist.. diff --git a/tests/lib/UrlGeneratorTest.php b/tests/lib/UrlGeneratorTest.php index c3456dd315d3..cf32987eed28 100644 --- a/tests/lib/UrlGeneratorTest.php +++ b/tests/lib/UrlGeneratorTest.php @@ -8,11 +8,12 @@ namespace Test; use OC\Helper\EnvironmentHelper; +use OC\Memcache\ArrayCache; use OC\URLGenerator; -use OCP\ICacheFactory; use OCP\IConfig; use OCP\IURLGenerator; use OCP\Route\IRouter; +use Test\Memcache\FixedCacheFactory; /** * Class UrlGeneratorTest @@ -26,10 +27,14 @@ class UrlGeneratorTest extends TestCase { /** @var EnvironmentHelper | \PHPUnit\Framework\MockObject\MockObject */ private $environmentHelper; + /** @var ArrayCache */ + private $imagePathCache; + public function setUp(): void { parent::setUp(); $config = $this->createMock(IConfig::class); - $cacheFactory = $this->createMock(ICacheFactory::class); + $this->imagePathCache = new ArrayCache(); + $cacheFactory = new FixedCacheFactory($this->imagePathCache); $this->router = $this->createMock(IRouter::class); $this->environmentHelper = $this->createMock(EnvironmentHelper::class); $this->urlGenerator = new URLGenerator( @@ -141,4 +146,60 @@ public function provideSubDirURLs() { ["apps/index.php", "http://localhost/owncloud/apps/index.php"], ]; } + + public function testImagePathIsCached() { + $this->environmentHelper->expects($this->any()) + ->method('getWebRoot') + ->willReturn('/owncloud'); + $this->environmentHelper->expects($this->any()) + ->method('getServerRoot') + ->willReturn(\OC::$SERVERROOT); + + $path = $this->urlGenerator->imagePath('', 'favicon.png'); + $this->assertEquals('/owncloud/core/img/favicon.png', $path); + + // the theme name is part of the key, so that switching themes does not + // serve the paths of the previous one + $theme = \OC_Util::getTheme()->getName(); + $this->assertEquals($path, $this->imagePathCache->get($theme . '--favicon.png')); + + // a second call is served from the cache - proven by handing out a + // value the filesystem would never produce + $this->imagePathCache->set($theme . '--favicon.png', '/from/the/cache.png'); + $this->assertEquals('/from/the/cache.png', $this->urlGenerator->imagePath('', 'favicon.png')); + } + + /** + * Image paths are host local, so they must not be stored in the distributed + * cache where another host - or anything else reaching it - could serve them + * back. + */ + public function testImagePathsUseTheLocalCacheTier() { + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->once()) + ->method('createLocal') + ->with('imagePath') + ->willReturn(new ArrayCache()); + $cacheFactory->expects($this->never())->method('createDistributed'); + $cacheFactory->expects($this->never())->method('create'); + + new URLGenerator( + $this->createMock(IConfig::class), + $cacheFactory, + $this->router, + $this->environmentHelper + ); + } + + public function testImagePathThrowsForMissingImage() { + $this->environmentHelper->expects($this->any()) + ->method('getWebRoot') + ->willReturn('/owncloud'); + $this->environmentHelper->expects($this->any()) + ->method('getServerRoot') + ->willReturn(\OC::$SERVERROOT); + + $this->expectException(\RuntimeException::class); + $this->urlGenerator->imagePath('', 'this-image-does-not-exist.gif'); + } } From b0c16fc670ccd308f2daad686ccf79b458ecfcb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 27 Jul 2026 14:54:10 +0200 Subject: [PATCH 2/3] fix(integrity): clear stale per app results and use the local cache tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit storeResults() writes one cache entry per checked scope in addition to the entry holding the combined results, but cleanResults() only removed the latter. The per app entries had no TTL either, so a verdict about an app was cached forever and kept being served through getVerifiedAppsFromCache() even after the app had been repaired or replaced. cleanResults() now clears the whole prefix - ICache::clear() is prefix scoped in every backend - and the entries expire. The results describe the files on disk of the host that produced them, so they also move to the host local cache tier. getResults() already falls back to appconfig, so a check run through occ stays visible to the web requests. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41735 | 11 +++ lib/private/IntegrityCheck/Checker.php | 24 ++++-- tests/lib/IntegrityCheck/CheckerTest.php | 105 ++++++++++++++++------- 3 files changed, 102 insertions(+), 38 deletions(-) create mode 100644 changelog/unreleased/41735 diff --git a/changelog/unreleased/41735 b/changelog/unreleased/41735 new file mode 100644 index 000000000000..599c250e4d1d --- /dev/null +++ b/changelog/unreleased/41735 @@ -0,0 +1,11 @@ +Bugfix: Clear stale integrity check results when rescanning + +The code integrity checker stores one cache entry per checked scope, but a +rescan only removed the entry holding the combined results. The per app entries +had no expiry either, so a verdict about an app was cached indefinitely and was +served even after the app had been repaired or replaced. Rescanning now clears +all of them, the entries expire, and the results are kept in the host local +cache tier - they describe the files on disk of one host and are of no use to +another. + +https://github.com/owncloud/core/pull/41735 diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index edd9a3b018c7..143fe54c3608 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -55,6 +55,14 @@ */ class Checker implements OnDiskHasher { public const CACHE_KEY = 'oc.integritycheck.checker'; + + /** + * The results describe the files on disk of this instance, so cached entries + * have to expire for a node that never runs a check itself to notice a + * repaired or replaced installation. + */ + public const CACHE_TTL = 24 * 3600; + /** @var EnvironmentHelper */ private $environmentHelper; /** @var AppLocator */ @@ -104,7 +112,9 @@ public function __construct( $this->fileAccessHelper = $fileAccessHelper; $this->appLocator = $appLocator; $this->config = $config; - $this->cache = $cacheFactory ? $cacheFactory->create(self::CACHE_KEY) : new \OC\Memcache\NullCache(); + $this->cache = $cacheFactory + ? \OC\Memcache\LocalCacheFactory::create($cacheFactory, self::CACHE_KEY) + : new \OC\Memcache\NullCache(); $this->appManager = $appManager; $this->tempManager = $tempManager; $this->verifier = $verifier; @@ -400,17 +410,21 @@ private function storeResults($scope, array $result) { $this->setAppValue(self::CACHE_KEY, \json_encode($resultArray)); //Set cache for each app - $this->cache->set($scope, \json_encode($resultArray)); - $this->cache->set(self::CACHE_KEY, \json_encode($resultArray)); + $this->cache->set($scope, \json_encode($resultArray), self::CACHE_TTL); + $this->cache->set(self::CACHE_KEY, \json_encode($resultArray), self::CACHE_TTL); } /** + * Clean previous results for a proper rescanning. Otherwise a stale verdict + * would be served instead of the one the rescan is about to produce. * - * Clean previous results for a proper rescanning. Otherwise + * storeResults() writes one entry per scope in addition to CACHE_KEY, so the + * whole prefix is cleared - removing CACHE_KEY alone left every per app entry + * behind. */ private function cleanResults() { $this->deleteAppValue(self::CACHE_KEY); - $this->cache->remove(self::CACHE_KEY); + $this->cache->clear(); } /** diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php index 8cc07152f474..3a28796c8e9a 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -27,11 +27,12 @@ use OC\IntegrityCheck\Helpers\FileAccessHelper; use OC\IntegrityCheck\Verifier\Verifier; use OC\IntegrityCheck\Verifier\VerificationResult; +use OC\Memcache\ArrayCache; use OC\Memcache\NullCache; use OC\Memcache\Redis; use OCP\App\IAppManager; -use OCP\ICacheFactory; use OCP\IConfig; +use Test\Memcache\FixedCacheFactory; use Test\TestCase; /** @@ -48,7 +49,7 @@ class CheckerTest extends TestCase { private $fileAccessHelper; /** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */ private $config; - /** @var ICacheFactory | \PHPUnit\Framework\MockObject\MockObject */ + /** @var FixedCacheFactory */ private $cacheFactory; /** @var IAppManager | \PHPUnit\Framework\MockObject\MockObject */ private $appManager; @@ -61,7 +62,7 @@ public function setUp(): void { $this->fileAccessHelper = $this->createMock(FileAccessHelper::class); $this->appLocator = $this->createMock(AppLocator::class); $this->config = $this->createMock(IConfig::class); - $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory = new FixedCacheFactory(new NullCache()); $this->appManager = $this->createMock(IAppManager::class); $this->verifier = $this->createMock(Verifier::class); @@ -87,12 +88,6 @@ public function setUp(): void { ->method('getAllApps') ->willReturn([]); - $this->cacheFactory - ->expects($this->any()) - ->method('create') - ->with('oc.integritycheck.checker') - ->willReturn(new NullCache()); - $this->checker = new Checker( $this->environmentHelper, $this->fileAccessHelper, @@ -103,6 +98,69 @@ public function setUp(): void { \OC::$server->getTempManager(), $this->verifier ); + + $this->assertSame([Checker::CACHE_KEY], $this->cacheFactory->getRequestedPrefixes()); + } + + /** + * The results describe the files on disk of this host, so they belong in the + * host local cache tier - not in a distributed one where another host could + * hand back a verdict about an installation it cannot see. + */ + public function testUsesTheLocalCacheTier() { + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->once()) + ->method('createLocal') + ->with(Checker::CACHE_KEY) + ->willReturn(new NullCache()); + $cacheFactory->expects($this->never())->method('createDistributed'); + $cacheFactory->expects($this->never())->method('create'); + + new Checker( + $this->environmentHelper, + $this->fileAccessHelper, + $this->appLocator, + $this->config, + $cacheFactory, + $this->appManager, + \OC::$server->getTempManager(), + $this->verifier + ); + } + + /** + * storeResults() writes one entry per scope next to CACHE_KEY, and a rescan + * has to invalidate all of them - removing CACHE_KEY alone left every per app + * verdict cached forever. + */ + public function testRescanningDropsThePerAppResults() { + $cache = new ArrayCache(); + $checker = new Checker( + $this->environmentHelper, + $this->fileAccessHelper, + $this->appLocator, + $this->config, + new FixedCacheFactory($cache), + $this->appManager, + \OC::$server->getTempManager(), + $this->verifier + ); + + $this->environmentHelper->method('getChannel')->willReturn('stable'); + $this->environmentHelper->method('getServerRoot')->willReturn(\OC::$SERVERROOT); + $this->verifier->method('verify')->willReturn(VerificationResult::passed()); + + $cache->set('SomeApp', '{"SomeApp":[]}'); + $cache->set('SomeOtherApp', '{"SomeOtherApp":[]}'); + $cache->set(Checker::CACHE_KEY, '{"SomeApp":[]}'); + + $checker->runInstanceVerification(); + + // only the results of this run are left - the verification passed, so + // there is nothing to report + $this->assertNull($cache->get('SomeApp')); + $this->assertNull($cache->get('SomeOtherApp')); + $this->assertSame('[]', $cache->get(Checker::CACHE_KEY)); } public function testIgnoredAppSignatureWithoutSignatureData() { @@ -630,12 +688,7 @@ public function testVerifyCachedAppSignatureCheck() { $redisObj->method('get') ->with('SomeApp') ->willReturn('[]'); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory - ->expects($this->any()) - ->method('create') - ->with('oc.integritycheck.checker') - ->will($this->returnValue($redisObj)); + $cacheFactory = new FixedCacheFactory($redisObj); $checker = new Checker( $this->environmentHelper, $this->fileAccessHelper, @@ -654,12 +707,7 @@ public function testAppNotCachedSignatureCheck() { $redisObj->method('get') ->with('SomeApp') ->willReturn(null); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory - ->expects($this->any()) - ->method('create') - ->with('oc.integritycheck.checker') - ->will($this->returnValue($redisObj)); + $cacheFactory = new FixedCacheFactory($redisObj); $checker = new Checker( $this->environmentHelper, $this->fileAccessHelper, @@ -702,10 +750,7 @@ public function testHasPassedCheckWithExceptionResult() { ] ])); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory->expects($this->any()) - ->method('create') - ->willReturn(new NullCache()); + $cacheFactory = new FixedCacheFactory(new NullCache()); $checker = new Checker( $this->environmentHelper, @@ -732,10 +777,7 @@ public function testHasPassedCheckWithEmptyResults() { ->with('core', 'oc.integritycheck.checker', '{}') ->willReturn('{}'); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory->expects($this->any()) - ->method('create') - ->willReturn(new NullCache()); + $cacheFactory = new FixedCacheFactory(new NullCache()); $checker = new Checker( $this->environmentHelper, @@ -766,10 +808,7 @@ public function testHasPassedCheckWithFileMissing() { ] ])); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory->expects($this->any()) - ->method('create') - ->willReturn(new NullCache()); + $cacheFactory = new FixedCacheFactory(new NullCache()); $checker = new Checker( $this->environmentHelper, From 77f390132969e5f2faf0c5e8a43aa4ffe0c747b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 27 Jul 2026 15:07:21 +0200 Subject: [PATCH 3/3] refactor(files): drop the distributed cache in front of the storages table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mapping between the string id and the numeric id of a storage was kept in the distributed cache for five minutes, on top of the CappedMemoryCache that already memoizes it for the duration of the request. The second layer bought about 0.1ms per lookup and cost correctness: a storage that one node marked unavailable was still reported available by the other nodes until the entry expired, and anything able to write to the distributed cache could remap a storage string id onto a different numeric id. The lookup now falls through to the database, so the request scoped cache stays and the mapping is read at most once per request per storage. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- apps/files_sharing/tests/TestCase.php | 8 --- changelog/unreleased/41736 | 10 +++ lib/private/Files/Cache/Storage.php | 54 ++------------- tests/lib/Files/Cache/StorageTest.php | 99 +++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 55 deletions(-) create mode 100644 changelog/unreleased/41736 create mode 100644 tests/lib/Files/Cache/StorageTest.php diff --git a/apps/files_sharing/tests/TestCase.php b/apps/files_sharing/tests/TestCase.php index 29bacbe45974..89dfaffc725d 100644 --- a/apps/files_sharing/tests/TestCase.php +++ b/apps/files_sharing/tests/TestCase.php @@ -188,14 +188,6 @@ protected static function resetStorage() { $localCache->clear(); } $property->setAccessible(false); - $property = $storage->getProperty('distributedCache'); - $property->setAccessible(true); - /** @var ICache $localCache */ - $distributedCache = $property->getValue(); - if ($distributedCache instanceof ICache) { - $distributedCache->clear(); - } - $property->setAccessible(false); } /** diff --git a/changelog/unreleased/41736 b/changelog/unreleased/41736 new file mode 100644 index 000000000000..442e49f65d74 --- /dev/null +++ b/changelog/unreleased/41736 @@ -0,0 +1,10 @@ +Change: Drop the distributed cache in front of the storages table + +The mapping between the string and the numeric id of a storage was cached in +the distributed cache for five minutes on top of the cache that already +memoizes it for the duration of the request. A storage marked unavailable was +therefore still considered available by the other nodes until that entry +expired. The mapping is now looked up in the database once per request, so a +change of the availability is seen everywhere immediately. + +https://github.com/owncloud/core/pull/41736 diff --git a/lib/private/Files/Cache/Storage.php b/lib/private/Files/Cache/Storage.php index 3c328f156f25..39d140ce8720 100644 --- a/lib/private/Files/Cache/Storage.php +++ b/lib/private/Files/Cache/Storage.php @@ -27,7 +27,6 @@ namespace OC\Files\Cache; use OC\Cache\CappedMemoryCache; -use OCP\ICache; /** * Handle the mapping between the string and numeric storage ids @@ -38,6 +37,11 @@ * * A mapping between the two storage ids is stored in the database and accessible trough this class * + * The mapping is memoized for the duration of the request only. It used to be + * cached in the distributed cache on top of that, which bought about 0.1ms per + * lookup at the price of setAvailability() changes taking up to five minutes to + * be seen by the other nodes. + * * @package OC\Files\Cache */ class Storage { @@ -47,11 +51,6 @@ class Storage { /** @var CappedMemoryCache */ protected static $localCache = null; - /** @var ICache */ - private static $distributedCache = null; - - private static $distributedCacheTTL = 300; // 5 Min - /** * @param \OC\Files\Storage\Storage|string $storage * @param bool $isAvailable @@ -83,13 +82,6 @@ public function __construct($storage, $isAvailable = true) { // local cache has been initialized by self::getStorageById self::$localCache->set($this->storageId, $storageData); - - // distributed cache may need initializing - self::getDistributedCache()->set( - $this->storageId, - $storageData, - self::$distributedCacheTTL - ); } else { if ($row = self::getStorageById($this->storageId)) { $this->numericId = (int)$row['numeric_id']; @@ -101,7 +93,7 @@ public function __construct($storage, $isAvailable = true) { } /** - * query the local cache, a distributed cache and the db for a storageid + * query the request scoped cache and the db for a storageid * @param string $storageId * @return array|false */ @@ -110,38 +102,9 @@ public static function getStorageById($storageId) { self::$localCache = new CappedMemoryCache(); } $result = self::$localCache->get($storageId); - if ($result === null || !isset($result['numeric_id'])) { - $result = self::getStorageByIdFromCache($storageId); - self::$localCache->set($storageId, $result); - } - return $result; - } - - /** - * @return ICache - */ - private static function getDistributedCache() { - if (self::$distributedCache === null) { - self::$distributedCache = - \OC::$server->getMemCacheFactory()->create('getStorageById'); - } - return self::$distributedCache; - } - - /** - * query the distributed cache for a storageid - * @param string $storageId - * @return array|false - */ - private static function getStorageByIdFromCache($storageId) { - $result = self::getDistributedCache()->get($storageId); if ($result === null || !isset($result['numeric_id'])) { $result = self::getStorageByIdFromDb($storageId); - self::getDistributedCache()->set( - $storageId, - $result, - self::$distributedCacheTTL - ); + self::$localCache->set($storageId, $result); } return $result; } @@ -162,12 +125,9 @@ private static function getStorageByIdFromDb($storageId) { } private static function unsetCache($storageId) { - // delete from local cache if (self::$localCache !== null) { self::$localCache->remove($storageId); } - // delete from distributed cache - self::getDistributedCache()->remove($storageId); } /** diff --git a/tests/lib/Files/Cache/StorageTest.php b/tests/lib/Files/Cache/StorageTest.php new file mode 100644 index 000000000000..a6a7d1a118cc --- /dev/null +++ b/tests/lib/Files/Cache/StorageTest.php @@ -0,0 +1,99 @@ + + * + * @copyright Copyright (c) 2026, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace Test\Files\Cache; + +use OC\Files\Cache\Storage; +use Test\Memcache\FixedCacheFactory; +use Test\TestCase; + +/** + * Tests for the string id <-> numeric id mapping of the storages table. + * + * @group DB + */ +class StorageTest extends TestCase { + /** @var string */ + private $storageId; + + protected function setUp(): void { + parent::setUp(); + $this->storageId = 'test::' . $this->getUniqueID(); + } + + protected function tearDown(): void { + Storage::remove($this->storageId); + parent::tearDown(); + } + + public function testStorageIsInsertedOnce() { + $storage = new Storage($this->storageId); + $numericId = $storage->getNumericId(); + + $this->assertSame($numericId, (new Storage($this->storageId))->getNumericId()); + } + + public function testAvailabilityChangeIsVisibleImmediately() { + $storage = new Storage($this->storageId); + $this->assertTrue($storage->getAvailability()['available']); + + $storage->setAvailability(false); + + // a separately constructed instance - as another request would build it - + // has to see the change + $this->assertFalse((new Storage($this->storageId))->getAvailability()['available']); + } + + /** + * The mapping used to be cached in the distributed cache for five minutes on + * top of the request scoped memoization, which meant a storage marked + * unavailable on one node kept being reported as available by the others until + * the entry expired. It also let anything able to write to that cache remap a + * string storage id onto the numeric id - and hence the file cache - of + * another storage. + */ + public function testMappingIsNotCachedBeyondTheRequest() { + $this->assertFalse( + (new \ReflectionClass(Storage::class))->hasProperty('distributedCache'), + 'the mapping is no longer kept in a memory cache' + ); + + // nothing in this class reaches for a cache factory any more + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->never())->method('create'); + $cacheFactory->expects($this->never())->method('createLocal'); + $cacheFactory->expects($this->never())->method('createDistributed'); + $this->overwriteService('MemCacheFactory', $cacheFactory); + + try { + $storage = new Storage($this->storageId); + $this->assertSame( + $storage->getNumericId(), + Storage::getNumericStorageId($this->storageId) + ); + $storage->setAvailability(false); + Storage::remove($this->storageId); + $this->assertFalse(Storage::exists($this->storageId)); + } finally { + $this->restoreService('MemCacheFactory'); + } + } +}