From 8cb4b5486905d14c807e377dab82947ca883ce17 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 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 [skip ci] --- lib/FilesHooks.php | 177 ++++++++++++++++++++++++++------------------- 1 file changed, 101 insertions(+), 76 deletions(-) diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index e401f9fde..7b27595e9 100644 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -466,26 +466,31 @@ protected function generateDeleteActivities($users, $pathMap, $fileId, $oldFileN [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, $users); $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); } /** @@ -502,26 +507,31 @@ protected function generateAddActivities($users, $pathMap, $fileId, $fileName) { [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, $users); $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); } /** @@ -541,30 +551,35 @@ protected function generateMoveActivities($users, $beforePathMap, $afterPathMap, [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, $users); $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); } /** @@ -923,13 +938,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); } /** @@ -1011,23 +1031,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); } /**