From d7395ec59ada97c444965f88aea1d055fd166a2c Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Wed, 22 Jul 2026 20:06:28 +0200 Subject: [PATCH] fix(files): roll back activity transaction when activity publishing fails The activity fan-out loops wrapped in startActivityTransaction() / commitActivityTransaction() had no error path: any exception while publishing activities left an open transaction on the shared database connection for the rest of the request and never reset the deferred state of the notification manager, silently swallowing all subsequent notifications and putting later writers inside a doomed transaction. Wrap the loops in try/catch, roll the transaction back on failure and flush the notification manager to reset its deferred state before rethrowing. Signed-off-by: Frank Karlitschek Co-Authored-By: Claude Fable 5 --- lib/FilesHooks.php | 187 +++++++++++++++++++++++++++------------------ 1 file changed, 111 insertions(+), 76 deletions(-) diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index 84d7ca8ee..b8648c2e2 100644 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -487,26 +487,31 @@ protected function generateDeleteActivities($users, $pathMap, $fileId, $oldFileN } $shouldFlush = $this->startActivityTransaction(); - foreach ($users as $user) { - $path = $pathMap[$user]; + try { + foreach ($users as $user) { + $path = $pathMap[$user]; - if ($user === $this->currentUser->getUID()) { - $userSubject = 'deleted_self'; - $userParams = [[$fileId => $path . '/' . $oldFileName]]; - } else { - $userSubject = 'deleted_by'; - $userParams = [[$fileId => $path . '/' . $oldFileName], $this->currentUser->getUserIdentifier()]; - } + if ($user === $this->currentUser->getUID()) { + $userSubject = 'deleted_self'; + $userParams = [[$fileId => $path . '/' . $oldFileName]]; + } else { + $userSubject = 'deleted_by'; + $userParams = [[$fileId => $path . '/' . $oldFileName], $this->currentUser->getUserIdentifier()]; + } - $this->addNotificationsForUser( - $user, $userSubject, $userParams, - $fileId, $path . '/' . $oldFileName, true, - $filteredEmailUsers[$user] ?? false, - $filteredNotificationUsers[$user] ?? false, - Files::TYPE_SHARE_DELETED, - ); + $this->addNotificationsForUser( + $user, $userSubject, $userParams, + $fileId, $path . '/' . $oldFileName, true, + $filteredEmailUsers[$user] ?? false, + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_SHARE_DELETED, + ); + } + $this->commitActivityTransaction($shouldFlush); + } catch (\Throwable $e) { + $this->rollbackActivityTransaction($shouldFlush); + throw $e; } - $this->commitActivityTransaction($shouldFlush); } /** @@ -523,26 +528,31 @@ protected function generateAddActivities($users, $pathMap, $fileId, $fileName, a } $shouldFlush = $this->startActivityTransaction(); - foreach ($users as $user) { - $path = $pathMap[$user]; + try { + foreach ($users as $user) { + $path = $pathMap[$user]; - if ($user === $this->currentUser->getUID()) { - $userSubject = 'created_self'; - $userParams = [[$fileId => $path . '/' . $fileName]]; - } else { - $userSubject = 'created_by'; - $userParams = [[$fileId => $path . '/' . $fileName], $this->currentUser->getUserIdentifier()]; - } + if ($user === $this->currentUser->getUID()) { + $userSubject = 'created_self'; + $userParams = [[$fileId => $path . '/' . $fileName]]; + } else { + $userSubject = 'created_by'; + $userParams = [[$fileId => $path . '/' . $fileName], $this->currentUser->getUserIdentifier()]; + } - $this->addNotificationsForUser( - $user, $userSubject, $userParams, - $fileId, $path . '/' . $fileName, true, - $filteredEmailUsers[$user] ?? false, - $filteredNotificationUsers[$user] ?? false, - Files::TYPE_FILE_CHANGED, - ); + $this->addNotificationsForUser( + $user, $userSubject, $userParams, + $fileId, $path . '/' . $fileName, true, + $filteredEmailUsers[$user] ?? false, + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_FILE_CHANGED, + ); + } + $this->commitActivityTransaction($shouldFlush); + } catch (\Throwable $e) { + $this->rollbackActivityTransaction($shouldFlush); + throw $e; } - $this->commitActivityTransaction($shouldFlush); } /** @@ -562,30 +572,35 @@ protected function generateMoveActivities($users, $beforePathMap, $afterPathMap, } $shouldFlush = $this->startActivityTransaction(); - foreach ($users as $user) { - if ($oldFileName === $fileName) { - $userParams = [[$newParentId => $afterPathMap[$user] . '/']]; - } else { - $userParams = [[$fileId => $afterPathMap[$user] . '/' . $fileName]]; - } + try { + foreach ($users as $user) { + if ($oldFileName === $fileName) { + $userParams = [[$newParentId => $afterPathMap[$user] . '/']]; + } else { + $userParams = [[$fileId => $afterPathMap[$user] . '/' . $fileName]]; + } - if ($user === $this->currentUser->getUID()) { - $userSubject = 'moved_self'; - } else { - $userSubject = 'moved_by'; - $userParams[] = $this->currentUser->getUserIdentifier(); - } - $userParams[] = [$fileId => $beforePathMap[$user] . '/' . $oldFileName]; + if ($user === $this->currentUser->getUID()) { + $userSubject = 'moved_self'; + } else { + $userSubject = 'moved_by'; + $userParams[] = $this->currentUser->getUserIdentifier(); + } + $userParams[] = [$fileId => $beforePathMap[$user] . '/' . $oldFileName]; - $this->addNotificationsForUser( - $user, $userSubject, $userParams, - $fileId, $afterPathMap[$user] . '/' . $fileName, true, - $filteredEmailUsers[$user] ?? false, - $filteredNotificationUsers[$user] ?? false, - Files::TYPE_FILE_CHANGED, - ); + $this->addNotificationsForUser( + $user, $userSubject, $userParams, + $fileId, $afterPathMap[$user] . '/' . $fileName, true, + $filteredEmailUsers[$user] ?? false, + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_FILE_CHANGED, + ); + } + $this->commitActivityTransaction($shouldFlush); + } catch (\Throwable $e) { + $this->rollbackActivityTransaction($shouldFlush); + throw $e; } - $this->commitActivityTransaction($shouldFlush); } /** @@ -944,13 +959,18 @@ protected function unshareFromGroup(IShare $share) { $offset = 0; $users = $group->searchUsers('', self::USER_BATCH_SIZE, $offset); $shouldFlush = $this->startActivityTransaction(); - while (!empty($users)) { - $userIds = \array_map(fn (IUser $user) => $user->getUID(), $users); - $this->addNotificationsForUsers($userIds, $actionUser, $share->getNode(), $share->getTarget(), (int)$share->getId()); - $offset += self::USER_BATCH_SIZE; - $users = $group->searchUsers('', self::USER_BATCH_SIZE, $offset); + try { + while (!empty($users)) { + $userIds = \array_map(fn (IUser $user) => $user->getUID(), $users); + $this->addNotificationsForUsers($userIds, $actionUser, $share->getNode(), $share->getTarget(), (int)$share->getId()); + $offset += self::USER_BATCH_SIZE; + $users = $group->searchUsers('', self::USER_BATCH_SIZE, $offset); + } + $this->commitActivityTransaction($shouldFlush); + } catch (\Throwable $e) { + $this->rollbackActivityTransaction($shouldFlush); + throw $e; } - $this->commitActivityTransaction($shouldFlush); } /** @@ -1032,23 +1052,28 @@ protected function addNotificationsForUsers(array $usersIds, string $actionUser, $affectedUsers = $this->fixPathsForShareExceptions($affectedUsers, $shareId); $shouldFlush = $this->startActivityTransaction(); - foreach ($affectedUsers as $user => $path) { - $emailSetting = $filteredEmailUsersInGroup[$user] ?? false; - $notificationSetting = $filteredNotificationUsers[$user] ?? false; - if ($emailSetting || $notificationSetting) { - $this->addNotificationsForUser( - $user, - $actionUser, - [[$fileSource->getId() => $path], $this->currentUser->getUserIdentifier()], - $fileSource->getId(), - $path, - $fileSource instanceof File, - $emailSetting, - $notificationSetting, - ); + try { + foreach ($affectedUsers as $user => $path) { + $emailSetting = $filteredEmailUsersInGroup[$user] ?? false; + $notificationSetting = $filteredNotificationUsers[$user] ?? false; + if ($emailSetting || $notificationSetting) { + $this->addNotificationsForUser( + $user, + $actionUser, + [[$fileSource->getId() => $path], $this->currentUser->getUserIdentifier()], + $fileSource->getId(), + $path, + $fileSource instanceof File, + $emailSetting, + $notificationSetting, + ); + } } + $this->commitActivityTransaction($shouldFlush); + } catch (\Throwable $e) { + $this->rollbackActivityTransaction($shouldFlush); + throw $e; } - $this->commitActivityTransaction($shouldFlush); } /** @@ -1241,6 +1266,16 @@ protected function commitActivityTransaction(bool $shouldFlush): void { $this->connection->commit(); } + protected function rollbackActivityTransaction(bool $shouldFlush): void { + $this->connection->rollBack(); + if ($shouldFlush) { + // The notification manager has no way to discard deferred + // notifications, so flush to reset its deferred state. Notifications + // of rolled back activities are dropped when they are prepared. + $this->notificationGenerator->flushNotifications(); + } + } + /** * @param int $fileId *