diff options
author | Joas Schilling <coding@schilljs.com> | 2024-03-12 10:55:22 +0100 |
---|---|---|
committer | Joas Schilling <coding@schilljs.com> | 2024-03-13 10:49:40 +0100 |
commit | 3aba674cb5e3f32c73feddbc54d2b407efd03eee (patch) | |
tree | b33e10328c3d54e65b301e7e3b201e65ccfc1f97 | |
parent | 9b46159b7e435863341524bc9654c71aa322477d (diff) |
fix(federation): Fix the handling of retrying OCM notifications when the remote was not available
Signed-off-by: Joas Schilling <coding@schilljs.com>
-rw-r--r-- | appinfo/info.xml | 3 | ||||
-rw-r--r-- | lib/BackgroundJob/RetryJob.php | 100 | ||||
-rw-r--r-- | lib/BackgroundJob/RetryNotificationsJob.php | 49 | ||||
-rw-r--r-- | lib/Federation/BackendNotifier.php | 150 | ||||
-rw-r--r-- | lib/Migration/Version19000Date20240312105627.php | 106 | ||||
-rw-r--r-- | lib/Model/RetryNotification.php | 66 | ||||
-rw-r--r-- | lib/Model/RetryNotificationMapper.php | 62 | ||||
-rw-r--r-- | tests/php/Federation/FederationTest.php | 10 |
8 files changed, 380 insertions, 166 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/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/Federation/BackendNotifier.php b/lib/Federation/BackendNotifier.php index ccb422b23..78401e319 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); } /** @@ -219,7 +206,7 @@ class BackendNotifier { int $remoteAttendeeId, #[SensitiveParameter] string $accessToken, - ): bool { + ): void { $remote = $this->prepareRemoteUrl($remoteServerUrl); $notification = $this->cloudFederationFactory->getCloudFederationNotification(); @@ -234,22 +221,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 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; - } + $this->sendUpdateToRemote($remote, $notification); } public function sendRemoteUnShare( @@ -344,29 +316,11 @@ 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); - } - - 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; } $this->logger->warning("Failed to send notification for share from $remote, received status code {code}\n{body}", [ @@ -377,14 +331,84 @@ 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' => json_encode($notification->getMessage(), JSON_THROW_ON_ERROR), - 'try' => $try, - ] + if ($retry && $try === 0) { + $now = $this->timeFactory->getTime(); + $now += $this->getRetryDelay(1); + + // Talk data + $retryNotification = new RetryNotification(); + $retryNotification->setRemoteServer($remote); + $retryNotification->setNumAttempts(1); + $retryNotification->setNextRetry($this->timeFactory->getDateTime('@' . $now)); + + // OCM notification data + $data = $notification->getMessage(); + $retryNotification->setNotificationType($data['notificationType']); + $retryNotification->setResourceType($data['resourceType']); + $retryNotification->setProviderId($data['providerId']); + $retryNotification->setNotification(json_encode($data['notification'])); + + $this->retryNotificationMapper->insert($retryNotification); + } + + return false; + } + + public function retrySendingFailedNotifications(\DateTimeInterface $dueDateTime): void { + $retryNotifications = $this->retryNotificationMapper->getAllDue($dueDateTime); + + foreach ($retryNotifications as $retryNotification) { + $this->retrySendingFailedNotification($retryNotification); + } + } + + protected function retrySendingFailedNotification(RetryNotification $retryNotification): void { + $notification = $this->cloudFederationFactory->getCloudFederationNotification(); + $notification->setMessage( + $retryNotification->getNotificationType(), + $retryNotification->getResourceType(), + $retryNotification->getProviderId(), + json_decode($retryNotification->getNotification(), true, flags: JSON_THROW_ON_ERROR), ); + + $success = $this->sendUpdateToRemote($retryNotification->getRemoteServer(), $notification, $retryNotification->getNumAttempts()); + + if ($success) { + $this->retryNotificationMapper->delete($retryNotification); + } elseif ($retryNotification->getNumAttempts() === RetryNotification::MAX_NUM_ATTEMPTS) { + $this->logger->error('Failed to send notification to ' . $retryNotification->getRemoteServer() . ' ' . RetryNotification::MAX_NUM_ATTEMPTS . ' times, giving up!'); + $this->retryNotificationMapper->delete($retryNotification); + } else { + $retryNotification->setNumAttempts($retryNotification->getNumAttempts() + 1); + + $now = $this->timeFactory->getTime(); + $now += $this->getRetryDelay($retryNotification->getNumAttempts()); + + $retryNotification->setNextRetry($this->timeFactory->getDateTime('@' . $now)); + $this->retryNotificationMapper->update($retryNotification); + } + } + + /** + * First 5 attempts are retried on the next cron run. + * Attempts 6-10 we back off to cover slightly longer maintenance/downtimes (5 minutes * per attempt) + * And the last tries 11-20 are retried with ~8 hours delay + * + * This means the last retry is after ~84 hours so a downtime from Friday to Monday would be covered + */ + protected function getRetryDelay(int $attempt): int { + if ($attempt < 5) { + // Retry after "attempt" minutes + return 5 * 60; + } + + if ($attempt > 10) { + // Retry after 8 hours + return 8 * 3600; + } + + // Retry after "attempt" * 5 minutes + return $attempt * 5 * 60; } protected function prepareRemoteUrl(string $remote): string { diff --git a/lib/Migration/Version19000Date20240312105627.php b/lib/Migration/Version19000Date20240312105627.php new file mode 100644 index 000000000..c8e3b45e5 --- /dev/null +++ b/lib/Migration/Version19000Date20240312105627.php @@ -0,0 +1,106 @@ +<?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\Migration; + +use Closure; +use OCP\DB\ISchemaWrapper; +use OCP\DB\Types; +use OCP\IDBConnection; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; + +/** + * Add table to queue federation notifications to retry + */ +class Version19000Date20240312105627 extends SimpleMigrationStep { + public function __construct( + protected IDBConnection $connection, + ) { + } + + /** + * @param IOutput $output + * @param Closure(): ISchemaWrapper $schemaClosure + * @param array $options + * @return null|ISchemaWrapper + */ + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + $table = $schema->createTable('talk_retry_ocm'); + $table->addColumn('id', Types::BIGINT, [ + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->addColumn('remote_server', Types::STRING, [ + 'notnull' => true, + 'length' => 255, + ]); + $table->addColumn('num_attempts', Types::INTEGER, [ + 'default' => 0, + 'unsigned' => true, + ]); + $table->addColumn('next_retry', Types::DATETIME, [ + 'notnull' => false, + ]); + $table->addColumn('notification_type', Types::STRING, [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('resource_type', Types::STRING, [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('provider_id', Types::STRING, [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('notification', Types::TEXT, [ + 'notnull' => true, + ]); + + + $table->setPrimaryKey(['id']); + $table->addIndex(['next_retry'], 'talk_retry_ocm_next'); + + return $schema; + } + + /** + * Remove legacy RetryJobs + */ + public function postSchemaChange(IOutput $output, \Closure $schemaClosure, array $options) { + /** @psalm-suppress UndefinedClass */ + $formerClassName = \OCA\Talk\BackgroundJob\RetryJob::class; + + $query = $this->connection->getQueryBuilder(); + $query->delete('jobs') + ->where($query->expr()->eq('class', $query->createNamedParameter($formerClassName))); + $query->executeStatement(); + } +} diff --git a/lib/Model/RetryNotification.php b/lib/Model/RetryNotification.php new file mode 100644 index 000000000..d460f934c --- /dev/null +++ b/lib/Model/RetryNotification.php @@ -0,0 +1,66 @@ +<?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. |