Merge pull request #11780 from nextcloud/bugfix/noid/fixing-retry-OCM

Bugfix/noid/fixing retry ocm

18 files changed, 532 insertions, 198 deletions

diff --git a/appinfo/info.xml b/appinfo/info.xml
index e141bb015..d0d84c689 100644
--- a/appinfo/info.xml
+++ b/appinfo/info.xml
@@ -16,7 +16,7 @@ And in the works for the [coming versions](https://github.com/nextcloud/spreed/m
 
 	]]></description>
 
-	<version>19.0.0-beta.1</version>
+	<version>19.0.0-beta.1.1</version>
 	<licence>agpl</licence>
 
 	<author>Daniel Calviño Sánchez</author>
@@ -65,6 +65,7 @@ And in the works for the [coming versions](https://github.com/nextcloud/spreed/m
 		<job>OCA\Talk\BackgroundJob\Reminder</job>
 		<job>OCA\Talk\BackgroundJob\RemoveEmptyRooms</job>
 		<job>OCA\Talk\BackgroundJob\ResetAssignedSignalingServer</job>
+		<job>OCA\Talk\BackgroundJob\RetryNotificationsJob</job>
 	</background-jobs>
 
 	<repair-steps>
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 88b40cbb1..c627779fe 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -84,6 +84,7 @@ use OCA\Talk\Events\SystemMessageSentEvent;
 use OCA\Talk\Events\SystemMessagesMultipleSentEvent;
 use OCA\Talk\Events\UserJoinedRoomEvent;
 use OCA\Talk\Federation\CloudFederationProviderTalk;
+use OCA\Talk\Federation\Proxy\TalkV1\Notifier\CancelRetryOCMListener as TalkV1CancelRetryOCMListener;
 use OCA\Talk\Federation\Proxy\TalkV1\Notifier\MessageSentListener as TalkV1MessageSentListener;
 use OCA\Talk\Federation\Proxy\TalkV1\Notifier\RoomModifiedListener as TalkV1RoomModifiedListener;
 use OCA\Talk\Files\Listener as FilesListener;
@@ -284,6 +285,7 @@ class Application extends App implements IBootstrap {
 		$context->registerEventListener(ChatMessageSentEvent::class, TalkV1MessageSentListener::class);
 		$context->registerEventListener(SystemMessageSentEvent::class, TalkV1MessageSentListener::class);
 		$context->registerEventListener(SystemMessagesMultipleSentEvent::class, TalkV1MessageSentListener::class);
+		$context->registerEventListener(AttendeeRemovedEvent::class, TalkV1CancelRetryOCMListener::class);
 
 		// Signaling listeners (External)
 		$context->registerEventListener(AttendeesAddedEvent::class, SignalingListener::class);
diff --git a/lib/BackgroundJob/RetryJob.php b/lib/BackgroundJob/RetryJob.php
deleted file mode 100644
index 591f88611..000000000
--- a/lib/BackgroundJob/RetryJob.php
+++ /dev/null
@@ -1,100 +0,0 @@
-<?php
-
-declare(strict_types=1);
-/**
- * @copyright Copyright (c) 2016, ownCloud, Inc.
- * @copyright Copyright (c) 2021 Gary Kim <gary@garykim.dev>
- *
- * @author Bjoern Schiessle <bjoern@schiessle.org>
- * @author Björn Schießle <bjoern@schiessle.org>
- * @author Joas Schilling <coding@schilljs.com>
- * @author Lukas Reschke <lukas@statuscode.ch>
- * @author Morris Jobke <hey@morrisjobke.de>
- * @author Roeland Jago Douma <roeland@famdouma.nl>
- * @author Gary Kim <gary@garykim.dev>
- *
- * @license AGPL-3.0
- *
- * This code is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License, version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License, version 3,
- * along with this program. If not, see <http://www.gnu.org/licenses/>
- *
- */
-namespace OCA\Talk\BackgroundJob;
-
-use OCA\Talk\Federation\BackendNotifier;
-use OCP\AppFramework\Utility\ITimeFactory;
-use OCP\BackgroundJob\IJobList;
-use OCP\BackgroundJob\Job;
-use OCP\ILogger;
-
-/**
- * Class RetryJob
- *
- * Background job to re-send update of federated re-shares to the remote server in
- * case the server was not available on the first try
- *
- * @package OCA\Talk\BackgroundJob
- */
-class RetryJob extends Job {
-
-	/** @var int max number of attempts to send the request */
-	private int $maxTry = 20;
-
-
-	public function __construct(
-		private BackendNotifier $backendNotifier,
-		ITimeFactory $timeFactory,
-	) {
-		parent::__construct($timeFactory);
-	}
-
-	/**
-	 * run the job, then remove it from the jobList
-	 *
-	 * @param IJobList $jobList
-	 * @param ILogger|null $logger
-	 */
-	public function execute(IJobList $jobList, ?ILogger $logger = null): void {
-		if (((int)$this->argument['try']) > $this->maxTry) {
-			$jobList->remove($this, $this->argument);
-			return;
-		}
-		if ($this->shouldRun($this->argument)) {
-			parent::execute($jobList, $logger);
-			$jobList->remove($this, $this->argument);
-		}
-	}
-
-	protected function run($argument): void {
-		$remote = $argument['remote'];
-		$data = json_decode($argument['data'], true, flags: JSON_THROW_ON_ERROR);
-		$try = (int)$argument['try'] + 1;
-
-		$this->backendNotifier->sendUpdateDataToRemote($remote, $data, $try);
-	}
-
-	/**
-	 * test if it is time for the next run
-	 *
-	 * @param array $argument
-	 * @return bool
-	 */
-	protected function shouldRun(array $argument): bool {
-		$lastRun = (int)$argument['lastRun'];
-		$try = (int)$argument['try'];
-		return (($this->time->getTime() - $lastRun) > $this->nextRunBreak($try));
-	}
-
-	protected function nextRunBreak(int $try): int {
-		return min(($try + 1) * 300, 3600);
-	}
-}
diff --git a/lib/BackgroundJob/RetryNotificationsJob.php b/lib/BackgroundJob/RetryNotificationsJob.php
new file mode 100644
index 000000000..de0454a9a
--- /dev/null
+++ b/lib/BackgroundJob/RetryNotificationsJob.php
@@ -0,0 +1,49 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2024 Joas Schilling <coding@schilljs.com>
+ *
+ * @author Joas Schilling <coding@schilljs.com>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+namespace OCA\Talk\BackgroundJob;
+
+use OCA\Talk\Federation\BackendNotifier;
+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\BackgroundJob\TimedJob;
+
+/**
+ * Retry to send OCM notifications
+ */
+class RetryNotificationsJob extends TimedJob {
+	public function __construct(
+		private BackendNotifier $backendNotifier,
+		ITimeFactory $timeFactory,
+	) {
+		parent::__construct($timeFactory);
+
+		// Every time the jobs run
+		$this->setInterval(1);
+	}
+
+	protected function run($argument): void {
+		$this->backendNotifier->retrySendingFailedNotifications($this->time->getDateTime());
+	}
+}
diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php
index e725eb663..e9ba26e55 100644
--- a/lib/Controller/RoomController.php
+++ b/lib/Controller/RoomController.php
@@ -39,7 +39,7 @@ use OCA\Talk\Exceptions\ParticipantNotFoundException;
 use OCA\Talk\Exceptions\RoomNotFoundException;
 use OCA\Talk\Exceptions\UnauthorizedException;
 use OCA\Talk\Federation\Authenticator;
-use OCA\Talk\Federation\BackendNotifier;
+use OCA\Talk\Federation\FederationManager;
 use OCA\Talk\GuestManager;
 use OCA\Talk\Manager;
 use OCA\Talk\MatterbridgeManager;
@@ -126,7 +126,7 @@ class RoomController extends AEnvironmentAwareController {
 		protected LoggerInterface $logger,
 		protected Authenticator $federationAuthenticator,
 		protected Capabilities $capabilities,
-		protected BackendNotifier $federationBackendNotifier,
+		protected FederationManager $federationManager,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -1263,11 +1263,7 @@ class RoomController extends AEnvironmentAwareController {
 	 */
 	protected function removeSelfFromRoomLogic(Room $room, Participant $participant): DataResponse {
 		if ($room->getRemoteServer() !== '') {
-			$this->federationBackendNotifier->sendShareDeclined(
-				$room->getRemoteServer(),
-				(int) $participant->getAttendee()->getRemoteId(),
-				$participant->getAttendee()->getAccessToken(),
-			);
+			$this->federationManager->rejectByRemoveSelf($room, $this->userId);
 		}
 
 		if ($room->getType() !== Room::TYPE_ONE_TO_ONE && $room->getType() !== Room::TYPE_ONE_TO_ONE_FORMER) {
diff --git a/lib/Federation/BackendNotifier.php b/lib/Federation/BackendNotifier.php
index ccb422b23..15cfb0ac3 100644
--- a/lib/Federation/BackendNotifier.php
+++ b/lib/Federation/BackendNotifier.php
@@ -27,15 +27,16 @@ namespace OCA\Talk\Federation;
 
 use OCA\FederatedFileSharing\AddressHandler;
 use OCA\Federation\TrustedServers;
-use OCA\Talk\BackgroundJob\RetryJob;
 use OCA\Talk\Config;
 use OCA\Talk\Exceptions\RoomHasNoModeratorException;
 use OCA\Talk\Model\Attendee;
+use OCA\Talk\Model\RetryNotification;
+use OCA\Talk\Model\RetryNotificationMapper;
 use OCA\Talk\Room;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Services\IAppConfig;
-use OCP\BackgroundJob\IJobList;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\DB\Exception;
 use OCP\Federation\ICloudFederationFactory;
 use OCP\Federation\ICloudFederationNotification;
@@ -56,12 +57,13 @@ class BackendNotifier {
 		private AddressHandler $addressHandler,
 		private LoggerInterface $logger,
 		private ICloudFederationProviderManager $federationProviderManager,
-		private IJobList $jobList,
 		private IUserManager $userManager,
 		private IURLGenerator $url,
 		private IAppManager $appManager,
 		private Config $talkConfig,
 		private IAppConfig $appConfig,
+		private RetryNotificationMapper $retryNotificationMapper,
+		private ITimeFactory $timeFactory,
 	) {
 	}
 
@@ -192,22 +194,7 @@ class BackendNotifier {
 			]
 		);
 
-		try {
-			$response = $this->federationProviderManager->sendCloudNotification($remote, $notification);
-			if ($response->getStatusCode() === Http::STATUS_CREATED) {
-				return true;
-			}
-
-			$this->logger->warning("Failed to send share accepted notification for share from $remote, received status code {code}\n{body}", [
-				'code' => $response->getStatusCode(),
-				'body' => (string) $response->getBody(),
-			]);
-
-			return false;
-		} catch (OCMProviderException $e) {
-			$this->logger->error("Failed to send share accepted notification for share from $remote, received OCMProviderException", ['exception' => $e]);
-			return false;
-		}
+		return $this->sendUpdateToRemote($remote, $notification, retry: false) === true;
 	}
 
 	/**
@@ -219,7 +206,7 @@ class BackendNotifier {
 		int $remoteAttendeeId,
 		#[SensitiveParameter]
 		string $accessToken,
-	): bool {
+	): void {
 		$remote = $this->prepareRemoteUrl($remoteServerUrl);
 
 		$notification = $this->cloudFederationFactory->getCloudFederationNotification();
@@ -234,22 +221,9 @@ class BackendNotifier {
 			]
 		);
 
-		try {
-			$response = $this->federationProviderManager->sendCloudNotification($remote, $notification);
-			if ($response->getStatusCode() === Http::STATUS_CREATED) {
-				return true;
-			}
-
-			$this->logger->warning("Failed to send share declined notification for share from $remote, received status code {code}\n{body}", [
-				'code' => $response->getStatusCode(),
-				'body' => (string) $response->getBody(),
-			]);
-
-			return false;
-		} catch (OCMProviderException $e) {
-			$this->logger->error("Failed to send share declined notification for share from $remote, received OCMProviderException", ['exception' => $e]);
-			return false;
-		}
+		// We don't handle the return here as all local data is already deleted.
+		// If the retry ever aborts due to "unknown" we are fine with it.
+		$this->sendUpdateToRemote($remote, $notification);
 	}
 
 	public function sendRemoteUnShare(
@@ -272,6 +246,8 @@ class BackendNotifier {
 			]
 		);
 
+		// We don't handle the return here as when the retry ever
+		// aborts due to "unknown" we are fine with it.
 		$this->sendUpdateToRemote($remote, $notification);
 	}
 
@@ -288,7 +264,7 @@ class BackendNotifier {
 		string $changedProperty,
 		string|int|bool|null $newValue,
 		string|int|bool|null $oldValue,
-	): void {
+	): ?bool {
 		$remote = $this->prepareRemoteUrl($remoteServer);
 
 		$notification = $this->cloudFederationFactory->getCloudFederationNotification();
@@ -306,7 +282,7 @@ class BackendNotifier {
 			],
 		);
 
-		$this->sendUpdateToRemote($remote, $notification);
+		return $this->sendUpdateToRemote($remote, $notification);
 	}
 
 	/**
@@ -324,7 +300,7 @@ class BackendNotifier {
 		string $localToken,
 		array $messageData,
 		array $unreadInfo,
-	): void {
+	): ?bool {
 		$remote = $this->prepareRemoteUrl($remoteServer);
 
 		$notification = $this->cloudFederationFactory->getCloudFederationNotification();
@@ -341,32 +317,23 @@ class BackendNotifier {
 			],
 		);
 
-		$this->sendUpdateToRemote($remote, $notification);
-	}
-
-	/**
-	 * @param string $remote
-	 * @param array{notificationType: string, resourceType: string, providerId: string, notification: array} $data
-	 * @param int $try
-	 * @return void
-	 * @internal Used to send retries in background jobs
-	 */
-	public function sendUpdateDataToRemote(string $remote, array $data, int $try): void {
-		$notification = $this->cloudFederationFactory->getCloudFederationNotification();
-		$notification->setMessage(
-			$data['notificationType'],
-			$data['resourceType'],
-			$data['providerId'],
-			$data['notification']
-		);
-		$this->sendUpdateToRemote($remote, $notification, $try);
+		return $this->sendUpdateToRemote($remote, $notification);
 	}
 
-	protected function sendUpdateToRemote(string $remote, ICloudFederationNotification $notification, int $try = 0): void {
+	protected function sendUpdateToRemote(string $remote, ICloudFederationNotification $notification, int $try = 0, bool $retry = true): ?bool {
 		try {
 			$response = $this->federationProviderManager->sendCloudNotification($remote, $notification);
 			if ($response->getStatusCode() === Http::STATUS_CREATED) {
-				return;
+				return true;
+			}
+
+			if ($response->getStatusCode() === Http::STATUS_BAD_REQUEST) {
+				$ocmBody = json_decode((string) $response->getBody(), true) ?? [];
+				if (isset($ocmBody['message']) && $ocmBody['message'] === FederationManager::OCM_RESOURCE_NOT_FOUND) {
+					// Remote exists but tells us the OCM notification can not be received (invalid invite data)
+					// So we stop retrying
+					return null;
+				}
 			}
 
 			$this->logger->warning("Failed to send notification for share from $remote, received status code {code}\n{body}", [
@@ -377,14 +344,87 @@ class BackendNotifier {
 			$this->logger->error("Failed to send notification for share from $remote, received OCMProviderException", ['exception' => $e]);
 		}
 
-		$this->jobList->add(
-			RetryJob::class,
-			[
-				'remote' => $remote,
-				'data' => js