fix(FileListener): Queue move actions correctly and fix FsActionMapper

lib/Classifiers/Images/ClusteringFaceClassifier.php

@@ -54,6 +54,7 @@ public function __construct(
 	 * @throws NotFoundException
 	 */
 	private function getUsersWithFileAccess(Node $node): array {
+		$this->userMountCache->clear();
 		$mountInfos = $this->userMountCache->getMountsForFileId($node->getId());
 		$userIds = array_map(static function (ICachedMountInfo $mountInfo) {
 			return $mountInfo->getUser()->getUID();

lib/Db/FsActionMapper.php

@@ -54,12 +54,8 @@ public function findByStorageId(string $className, int $storageId, int $limit =
 	 * @param class-string<FsCreation|FsDeletion|FsMove|FsAccessUpdate> $className
 	 * @return list<FsCreation|FsDeletion|FsMove|FsAccessUpdate>
 	 * @throws \OCP\DB\Exception
-	 * @throws \Exception
 	 */
 	public function find(string $className, int $limit = 0): array {
-		if (!in_array('storage_id', $className::$columns, true)) {
-			throw new \Exception('entity does not have a storage_id column');
-		}
 		$qb = $this->db->getQueryBuilder();
 		$qb->selectDistinct($className::$columns)
 			->from($className::$tableName);
@@ -243,7 +239,7 @@ public function insertMove(int $nodeId, string $owner, array $addedUsers, array
 			$move->setAddedUsers($addedUsers);
 			$move->setTargetUsers($targetUsers);
 			$this->insert($move);
-			$arguments = [ 'type' => FsDeletion::class ];
+			$arguments = [ 'type' => FsMove::class ];
 			if (!$this->jobList->has(ProcessFsActionsJob::class, $arguments)) {
 				$this->jobList->add(ProcessFsActionsJob::class, $arguments);
 			}

lib/Db/FsMove.php

@@ -48,14 +48,27 @@ public function __construct() {
 	 * @return list<string>
 	 */
 	public function getAddedUsers(): array {
-		return explode(',', $this->addedUsers ?? '');
+		return self::explodeUsers($this->addedUsers);
 	}
 
 	/**
 	 * @return list<string>
 	 */
 	public function getTargetUsers(): array {
-		return explode(',', $this->targetUsers ?? '');
+		return self::explodeUsers($this->targetUsers);
+	}
+
+	/**
+	 * explode() on an empty string yields [''], which would be treated as a user
+	 * with an empty user ID, so map the empty column to an empty list instead.
+	 *
+	 * @return list<string>
+	 */
+	private static function explodeUsers(?string $users): array {
+		if ($users === null || $users === '') {
+			return [];
+		}
+		return explode(',', $users);
 	}
 
 	/**

lib/Hooks/FileListener.php

@@ -283,18 +283,18 @@ public function postInsert(Node $node, bool $recurse = true, ?array $mimeTypes =
 	 * @throws Exception
 	 */
 	public function postRename(Node $source, Node $target): void {
-		$targetUserIds = $this->getUsersWithFileAccess($target->getId());
-
-		$usersToAdd = array_values(array_diff($targetUserIds, $this->sourceUserIds));
-		$existingUsers = array_diff($targetUserIds, $usersToAdd);
-		$sourceOwner = $source->getOwner();
-		$targetOwner = $target->getOwner();
-		$ownerId = $sourceOwner?->getUID() ?? $targetOwner?->getUID() ?? $existingUsers[0];
-
 		if (preg_match('#^/[^/]*?/files/#', $target->getPath()) !== 1 && preg_match('#^/groupfolders/#', $target->getPath()) !== 1) {
 			return;
 		}
 
+		$targetUserIds = $this->getUsersWithFileAccess($target->getId());
+		$usersToAdd = array_values(array_diff($targetUserIds, $this->sourceUserIds));
+
+		// Recorded for diagnostics only, and empty for group folder nodes, which have no
+		// owner: FsActionService picks the user to copy detections from by looking at who
+		// actually holds detections for the file.
+		$ownerId = $source->getOwner()?->getUID() ?? $target->getOwner()?->getUID() ?? '';
+
 		$this->fsActionMapper->insertMove($target->getId(), $ownerId, $usersToAdd, $targetUserIds);
 	}
 

lib/Service/FsActionService.php

@@ -125,7 +125,7 @@
 						break;
 					}
 					try {
-						$this->onMove($action->getOwner(), $action->getAddedUsers(), $action->getTargetUsers(), $node);
+						$this->onMove($action->getAddedUsers(), $action->getTargetUsers(), $node);
 					} catch (Exception|InvalidPathException|NotFoundException $e) {
 						$this->logger->warning('Failed to process move action: ' . $e->getMessage() . ' Continuing.', ['exception' => $e]);
 					}
@@ -168,33 +168,48 @@
 		$files = $this->storageService->getFilesInMount($storageId, $rootId, [ClusteringFaceClassifier::MODEL_NAME], 0, 0);
 		$userIdsToScheduleClustering = [];
 		foreach ($files as $fileInfo) {
-			$node = $this->rootFolder->getFirstNodeById($fileInfo['fileid']) ?: null;
-			$ownerId = $node?->getOwner()?->getUID();
-			if ($ownerId === null) {
+			$detectionCountByUser = $this->getDetectionCountByUser($fileInfo['fileid']);
+			if (count($detectionCountByUser) === 0) {
+				// Nothing detected for this file (yet): nothing to copy, nothing to prune
 				continue;
 			}
-			$detectionsForFile = $this->faceDetectionMapper->findByFileId($fileInfo['fileid']);
-			$userHasDetectionForFile = [];
-			foreach ($detectionsForFile as $detection) {
-				$userHasDetectionForFile[$detection->getUserId()] = true;
-			}
+			$sourceUserId = (string)array_key_first($detectionCountByUser);
 			foreach ($userIds as $userId) {
-				if ($userId === $ownerId) {
-					continue;
-				}
-				if ($userHasDetectionForFile[$userId] ?? false) {
+				if (isset($detectionCountByUser[(string)$userId])) {
 					continue;
 				}
-				$this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $ownerId, $userId);
+				$this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $sourceUserId, (string)$userId);
 				$userIdsToScheduleClustering[$userId] = true;
 			}
 			$this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $userIds);
 		}
 		foreach (array_keys($userIdsToScheduleClustering) as $userId) {
 			$this->jobList->add(ClusterFacesJob::class, ['userId' => (string)$userId]);
 		}
 	}
 
+	/**
+	 * How many face detections each user holds for a file, most complete set first.
+	 *
+	 * The first key is the user to copy detections from when granting access to further
+	 * users. The file system owner cannot be used for that: group folder nodes have no
+	 * owner when they are resolved outside of a user session, which is the case in the
+	 * background jobs that process fs actions. Whoever already holds detections for the
+	 * file is both a more reliable and a more direct answer to the question.
+	 *
+	 * @return array<string, int>
+	 * @throws Exception
+	 */
+	private function getDetectionCountByUser(int $fileId): array {
+		$detectionCountByUser = [];
+		foreach ($this->faceDetectionMapper->findByFileId($fileId) as $detection) {
+			$userId = (string)$detection->getUserId();
+			$detectionCountByUser[$userId] = ($detectionCountByUser[$userId] ?? 0) + 1;
+		}
+		arsort($detectionCountByUser);
+		return $detectionCountByUser;
+	}
+
 	/**
 	 * @throws \OCP\Files\InvalidPathException
 	 */
@@ -325,33 +340,38 @@
 	}
 
 	/**
-	 * @param string $ownerId
 	 * @param list<string> $usersToAdd
 	 * @param list<string> $targetUserIds
 	 * @param Node $node
 	 * @return void
 	 * @throws Exception|InvalidPathException|NotFoundException
 	 */
-	private function onMove(string $ownerId, array $usersToAdd, array $targetUserIds, Node $node): void {
+	private function onMove(array $usersToAdd, array $targetUserIds, Node $node): void {
 		if ($node instanceof Folder) {
 			try {
 				foreach ($node->getDirectoryListing() as $n) {
 					if (!in_array($n->getMimetype(), Constants::IMAGE_FORMATS)) {
 						continue;
 					}
-					$this->onMove($ownerId, $usersToAdd, $targetUserIds, $n);
+					$this->onMove($usersToAdd, $targetUserIds, $n);
 				}
 			} catch (NotFoundException|Exception|InvalidPathException $e) {
 				$this->logger->warning('Error in recognize file listener', ['exception' => $e]);
 			}
 			return;
 		}
+		$detectionCountByUser = $this->getDetectionCountByUser($node->getId());
+		if (count($detectionCountByUser) === 0) {
+			// Nothing detected for this file (yet): nothing to copy, nothing to prune
+			return;
+		}
+		$sourceUserId = (string)array_key_first($detectionCountByUser);
 		foreach ($usersToAdd as $userId) {
-			if (count($this->faceDetectionMapper->findByFileIdAndUser($node->getId(), $userId)) > 0) {
+			if (isset($detectionCountByUser[(string)$userId])) {
 				continue;
 			}
-			$this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $ownerId, $userId);
-			$this->jobList->add(ClusterFacesJob::class, ['userId' => $userId]);
+			$this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $sourceUserId, (string)$userId);
+			$this->jobList->add(ClusterFacesJob::class, ['userId' => (string)$userId]);
 		}
 		$this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($node->getId(), $targetUserIds);
 	}