Merge pull request #30056 from nextcloud/backport/29735/stable21

[stable21] find users for background scan one by one

Author: artonge

apps/files/lib/BackgroundJob/ScanFiles.php

@@ -30,7 +30,6 @@
 use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\ILogger;
-use OCP\IUserManager;
 
 /**
  * Class ScanFiles is a background job used to run the file scanner over the user
@@ -41,8 +40,6 @@
 class ScanFiles extends \OC\BackgroundJob\TimedJob {
 	/** @var IConfig */
 	private $config;
-	/** @var IUserManager */
-	private $userManager;
 	/** @var IEventDispatcher */
 	private $dispatcher;
 	/** @var ILogger */
@@ -54,14 +51,12 @@ class ScanFiles extends \OC\BackgroundJob\TimedJob {
 
 	/**
 	 * @param IConfig $config
-	 * @param IUserManager $userManager
 	 * @param IEventDispatcher $dispatcher
 	 * @param ILogger $logger
 	 * @param IDBConnection $connection
 	 */
 	public function __construct(
 		IConfig $config,
-		IUserManager $userManager,
 		IEventDispatcher $dispatcher,
 		ILogger $logger,
 		IDBConnection $connection
@@ -70,7 +65,6 @@ public function __construct(
 		$this->setInterval(60 * 10);
 
 		$this->config = $config;
-		$this->userManager = $userManager;
 		$this->dispatcher = $dispatcher;
 		$this->logger = $logger;
 		$this->connection = $connection;
@@ -82,10 +76,10 @@ public function __construct(
 	protected function runScanner(string $user) {
 		try {
 			$scanner = new Scanner(
-					$user,
-					null,
-					$this->dispatcher,
-					$this->logger
+				$user,
+				null,
+				$this->dispatcher,
+				$this->logger
 			);
 			$scanner->backgroundScan('');
 		} catch (\Exception $e) {
@@ -95,20 +89,20 @@ protected function runScanner(string $user) {
 	}
 
 	/**
-	 * Find all storages which have unindexed files and return a user for each
+	 * Find a storage which have unindexed files and return a user with access to the storage
 	 *
-	 * @return string[]
+	 * @return string|false
 	 */
-	private function getUsersToScan(): array {
+	private function getUserToScan() {
 		$query = $this->connection->getQueryBuilder();
-		$query->select($query->func()->max('user_id'))
+		$query->select('user_id')
 			->from('filecache', 'f')
 			->innerJoin('f', 'mounts', 'm', $query->expr()->eq('storage_id', 'storage'))
 			->where($query->expr()->lt('size', $query->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
-			->groupBy('storage_id')
-			->setMaxResults(self::USERS_PER_SESSION);
+			->andWhere($query->expr()->gt('parent', $query->createNamedParameter(-1, IQueryBuilder::PARAM_INT)))
+			->setMaxResults(1);
 
-		return $query->execute()->fetchAll(\PDO::FETCH_COLUMN);
+		return $query->execute()->fetchOne();
 	}
 
 	/**
@@ -120,10 +114,18 @@ protected function run($argument) {
 			return;
 		}
 
-		$users = $this->getUsersToScan();
-
-		foreach ($users as $user) {
+		$usersScanned = 0;
+		$lastUser = '';
+		$user = $this->getUserToScan();
+		while ($user && $usersScanned < self::USERS_PER_SESSION && $lastUser !== $user) {
 			$this->runScanner($user);
+			$lastUser = $user;
+			$user = $this->getUserToScan();
+			$usersScanned += 1;
+		}
+
+		if ($lastUser === $user) {
+			$this->logger->warning("User $user still has unscanned files after running background scan, background scan might be stopped prematurely");
 		}
 	}
 }

apps/files/tests/BackgroundJob/ScanFilesTest.php

@@ -31,7 +31,6 @@
 use OCP\IConfig;
 use OCP\ILogger;
 use OCP\IUser;
-use OCP\IUserManager;
 use Test\TestCase;
 use Test\Traits\MountProviderTrait;
 use Test\Traits\UserTrait;
@@ -55,7 +54,6 @@ protected function setUp(): void {
 		parent::setUp();
 
 		$config = $this->createMock(IConfig::class);
-		$userManager = $this->createMock(IUserManager::class);
 		$dispatcher = $this->createMock(IEventDispatcher::class);
 		$logger = $this->createMock(ILogger::class);
 		$connection = \OC::$server->getDatabaseConnection();
@@ -64,7 +62,6 @@ protected function setUp(): void {
 		$this->scanFiles = $this->getMockBuilder('\OCA\Files\BackgroundJob\ScanFiles')
 			->setConstructorArgs([
 				$config,
-				$userManager,
 				$dispatcher,
 				$logger,
 				$connection,

apps/files_sharing/lib/ISharedStorage.php

@@ -24,5 +24,7 @@
 
 namespace OCA\Files_Sharing;
 
-interface ISharedStorage {
+use OCP\Files\Storage\IStorage;
+
+interface ISharedStorage extends IStorage {
 }

apps/workflowengine/lib/Check/FileSystemTags.php

@@ -130,8 +130,8 @@ protected function getFileIds(ICache $cache, $path, $isExternalStorage) {
 		// TODO: Fix caching inside group folders
 		// Do not cache file ids inside group folders because multiple file ids might be mapped to
 		// the same combination of cache id + path.
-		$shouldCacheFileIds = !$this->storage
-			->instanceOfStorage(\OCA\GroupFolders\Mount\GroupFolderStorage::class);
+		/** @psalm-suppress InvalidArgument */
+		$shouldCacheFileIds = !$this->storage->instanceOfStorage(\OCA\GroupFolders\Mount\GroupFolderStorage::class);
 		$cacheId = $cache->getNumericStorageId();
 		if ($shouldCacheFileIds && isset($this->fileIds[$cacheId][$path])) {
 			return $this->fileIds[$cacheId][$path];

lib/private/Files/Cache/Scanner.php

@@ -38,6 +38,7 @@
 
 use Doctrine\DBAL\Exception;
 use OC\Files\Filesystem;
+use OC\Files\Storage\Wrapper\Jail;
 use OC\Files\Storage\Wrapper\Encoding;
 use OC\Hooks\BasicEmitter;
 use OCP\Files\Cache\IScanner;
@@ -510,19 +511,31 @@ public static function isPartialFile($file) {
 	 * walk over any folders that are not fully scanned yet and scan them
 	 */
 	public function backgroundScan() {
-		if (!$this->cache->inCache('')) {
-			$this->runBackgroundScanJob(function () {
-				$this->scan('', self::SCAN_RECURSIVE, self::REUSE_ETAG);
-			}, '');
+		if ($this->storage->instanceOfStorage(Jail::class)) {
+			// for jail storage wrappers (shares, groupfolders) we run the background scan on the source storage
+			// this is mainly done because the jail wrapper doesn't implement `getIncomplete` (because it would be inefficient).
+			//
+			// Running the scan on the source storage might scan more than "needed", but the unscanned files outside the jail will
+			// have to be scanned at some point anyway.
+			$unJailedScanner = $this->storage->getUnjailedStorage()->getScanner();
+			$unJailedScanner->backgroundScan();
 		} else {
-			$lastPath = null;
-			while (($path = $this->cache->getIncomplete()) !== false && $path !== $lastPath) {
-				$this->runBackgroundScanJob(function () use ($path) {
-					$this->scan($path, self::SCAN_RECURSIVE_INCOMPLETE, self::REUSE_ETAG | self::REUSE_SIZE);
-				}, $path);
-				// FIXME: this won't proceed with the next item, needs revamping of getIncomplete()
-				// to make this possible
-				$lastPath = $path;
+			if (!$this->cache->inCache('')) {
+				// if the storage isn't in the cache yet, just scan the root completely
+				$this->runBackgroundScanJob(function () {
+					$this->scan('', self::SCAN_RECURSIVE, self::REUSE_ETAG);
+				}, '');
+			} else {
+				$lastPath = null;
+				// find any path marked as unscanned and run the scanner until no more paths are unscanned (or we get stuck)
+				while (($path = $this->cache->getIncomplete()) !== false && $path !== $lastPath) {
+					$this->runBackgroundScanJob(function () use ($path) {
+						$this->scan($path, self::SCAN_RECURSIVE_INCOMPLETE, self::REUSE_ETAG | self::REUSE_SIZE);
+					}, $path);
+					// FIXME: this won't proceed with the next item, needs revamping of getIncomplete()
+					// to make this possible
+					$lastPath = $path;
+				}
 			}
 		}
 	}

lib/private/Files/Utils/Scanner.php

@@ -167,10 +167,6 @@ public function backgroundScan($dir) {
 				continue;
 			}
 
-			// don't scan received local shares, these can be scanned when scanning the owner's storage
-			if ($storage->instanceOfStorage(SharedStorage::class)) {
-				continue;
-			}
 			$scanner = $storage->getScanner();
 			$this->attachListener($mount);
 

lib/public/Files/IHomeStorage.php

@@ -32,10 +32,12 @@
 
 namespace OCP\Files;
 
+use OCP\Files\Storage\IStorage;
+
 /**
  * Interface IHomeStorage
  *
  * @since 7.0.0
  */
-interface IHomeStorage {
+interface IHomeStorage extends IStorage {
 }

lib/public/Files/Storage.php

@@ -374,9 +374,12 @@ public function isLocal();
 	/**
 	 * Check if the storage is an instance of $class or is a wrapper for a storage that is an instance of $class
 	 *
+	 * @template T of IStorage
 	 * @param string $class
+	 * @psalm-param class-string<T> $class
 	 * @return bool
 	 * @since 7.0.0
+	 * @psalm-assert-if-true T $this
 	 */
 	public function instanceOfStorage($class);
 

lib/public/Files/Storage/IDisableEncryptionStorage.php

@@ -29,5 +29,5 @@
  *
  * @since 16.0.0
  */
-interface IDisableEncryptionStorage {
+interface IDisableEncryptionStorage extends IStorage {
 }

lib/public/Files/Storage/IStorage.php

@@ -362,9 +362,12 @@ public function isLocal();
 	/**
 	 * Check if the storage is an instance of $class or is a wrapper for a storage that is an instance of $class
 	 *
+	 * @template T of IStorage
 	 * @param string $class
+	 * @psalm-param class-string<T> $class
 	 * @return bool
 	 * @since 9.0.0
+	 * @psalm-assert-if-true T $this
 	 */
 	public function instanceOfStorage($class);