From 1ac8cf6d959d0dc4ef2cf330b38f7b71ceaa4c94 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 df65fbe79..73e8b0cf4 100644 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -489,26 +489,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); } /** @@ -525,26 +530,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); } /** @@ -564,30 +574,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); } /** @@ -949,13 +964,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); } /** @@ -1037,23 +1057,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); } /**