refactor(files): drop the distributed cache in front of the storages table

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);
 	}
 
 	/**

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

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

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

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

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);
 	}
 
 	/**

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'];

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();
 	}
 
 	/**

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(),

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,16 +39,23 @@ class RepairMimeTypes implements IRepairStep {
 	 */
 	protected $config;
 
+	/**
+	 * @var IMimeTypeLoader
+	 */
+	protected $mimeTypeLoader;
+
 	/**
 	 * @var int
 	 */
 	protected $folderMimeTypeId;
 
 	/**
 	 * @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');
+		}
 	}
 }