From c0bed86544f5a62fc3924839da669cfe5a6251c1 Mon Sep 17 00:00:00 2001 From: Richard Steinmetz Date: Wed, 21 Aug 2024 14:18:22 +0200 Subject: [PATCH] feat: implement periodic full sync job to repair cache inconsistencies Signed-off-by: Richard Steinmetz --- appinfo/routes.php | 5 + lib/BackgroundJob/RepairSyncJob.php | 92 ++++++++++ lib/Controller/MailboxesController.php | 20 +++ lib/IMAP/IMAPClientFactory.php | 8 +- lib/Migration/FixBackgroundJobs.php | 2 + lib/Service/Sync/ImapToDbSynchronizer.php | 46 +++++ lib/Service/Sync/SyncService.php | 10 ++ src/components/NavigationMailbox.vue | 48 +++++- src/service/MailboxService.js | 14 ++ .../Sync/ImapToDbSynchronizerTest.php | 158 ++++++++++++++++++ 10 files changed, 398 insertions(+), 5 deletions(-) create mode 100644 lib/BackgroundJob/RepairSyncJob.php create mode 100644 tests/Integration/Sync/ImapToDbSynchronizerTest.php diff --git a/appinfo/routes.php b/appinfo/routes.php index a7f988e4aa..eddd1a5ee3 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -180,6 +180,11 @@ 'url' => '/api/mailboxes/{id}/stats', 'verb' => 'GET' ], + [ + 'name' => 'mailboxes#repair', + 'url' => '/api/mailboxes/{id}/repair', + 'verb' => 'POST' + ], [ 'name' => 'messages#downloadAttachment', 'url' => '/api/messages/{id}/attachment/{attachmentId}', diff --git a/lib/BackgroundJob/RepairSyncJob.php b/lib/BackgroundJob/RepairSyncJob.php new file mode 100644 index 0000000000..3588a5b768 --- /dev/null +++ b/lib/BackgroundJob/RepairSyncJob.php @@ -0,0 +1,92 @@ +setInterval(3600 * 24 * 7); + $this->setTimeSensitivity(self::TIME_INSENSITIVE); + } + + protected function run($argument): void { + $accountId = (int)$argument['accountId']; + + try { + $account = $this->accountService->findById($accountId); + } catch (DoesNotExistException $e) { + $this->logger->debug('Could not find account <' . $accountId . '> removing from jobs'); + $this->jobList->remove(self::class, $argument); + return; + } + + if (!$account->getMailAccount()->canAuthenticateImap()) { + $this->logger->debug('No authentication on IMAP possible, skipping background sync job'); + return; + } + + $user = $this->userManager->get($account->getUserId()); + if ($user === null || !$user->isEnabled()) { + $this->logger->debug(sprintf( + 'Account %d of user %s could not be found or was disabled, skipping background sync', + $account->getId(), + $account->getUserId() + )); + return; + } + + $rebuildThreads = false; + $trashMailboxId = $account->getMailAccount()->getTrashMailboxId(); + $snoozeMailboxId = $account->getMailAccount()->getSnoozeMailboxId(); + $sentMailboxId = $account->getMailAccount()->getSentMailboxId(); + $junkMailboxId = $account->getMailAccount()->getJunkMailboxId(); + foreach ($this->mailboxMapper->findAll($account) as $mailbox) { + $isExcluded = [ + $trashMailboxId === $mailbox->getId(), + $snoozeMailboxId === $mailbox->getId(), + $sentMailboxId === $mailbox->getId(), + $junkMailboxId === $mailbox->getId(), + ]; + if (in_array(true, $isExcluded, true)) { + continue; + } + + if ($this->syncService->repairSync($account, $mailbox) > 0) { + $rebuildThreads = true; + } + } + + $this->dispatcher->dispatchTyped( + new SynchronizationEvent($account, $this->logger, $rebuildThreads), + ); + } +} diff --git a/lib/Controller/MailboxesController.php b/lib/Controller/MailboxesController.php index e45cb49d4b..b3485df6d6 100644 --- a/lib/Controller/MailboxesController.php +++ b/lib/Controller/MailboxesController.php @@ -23,7 +23,9 @@ use OCA\Mail\Service\Sync\SyncService; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; +use OCP\AppFramework\Http\Attribute\UserRateLimit; use OCP\AppFramework\Http\JSONResponse; use OCP\IRequest; @@ -286,4 +288,22 @@ public function clearMailbox(int $id): JSONResponse { $this->mailManager->clearMailbox($account, $mailbox); return new JSONResponse(); } + + /** + * Delete all vanished mails that are still cached. + */ + #[TrapError] + #[NoAdminRequired] + #[UserRateLimit(limit: 10, period: 600)] + public function repair(int $id): JSONResponse { + if ($this->currentUserId === null) { + return new JSONResponse([], Http::STATUS_FORBIDDEN); + } + + $mailbox = $this->mailManager->getMailbox($this->currentUserId, $id); + $account = $this->accountService->find($this->currentUserId, $mailbox->getAccountId()); + + $this->syncService->repairSync($account, $mailbox); + return new JsonResponse(); + } } diff --git a/lib/IMAP/IMAPClientFactory.php b/lib/IMAP/IMAPClientFactory.php index 0098e4f611..3f6d5d2c45 100644 --- a/lib/IMAP/IMAPClientFactory.php +++ b/lib/IMAP/IMAPClientFactory.php @@ -112,9 +112,11 @@ public function getClient(Account $account, bool $useCache = true): Horde_Imap_C json_encode($params) ]), ); - $params['cache'] = [ - 'backend' => $this->hordeCacheFactory->newCache($account), - ]; + if ($useCache) { + $params['cache'] = [ + 'backend' => $this->hordeCacheFactory->newCache($account), + ]; + } if ($this->config->getSystemValue('debug', false)) { $params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log'; } diff --git a/lib/Migration/FixBackgroundJobs.php b/lib/Migration/FixBackgroundJobs.php index 91621e300c..2ee5d3de10 100644 --- a/lib/Migration/FixBackgroundJobs.php +++ b/lib/Migration/FixBackgroundJobs.php @@ -10,6 +10,7 @@ use OCA\Mail\BackgroundJob\PreviewEnhancementProcessingJob; use OCA\Mail\BackgroundJob\QuotaJob; +use OCA\Mail\BackgroundJob\RepairSyncJob; use OCA\Mail\BackgroundJob\SyncJob; use OCA\Mail\BackgroundJob\TrainImportanceClassifierJob; use OCA\Mail\Db\MailAccount; @@ -43,6 +44,7 @@ public function run(IOutput $output) { $output->startProgress(count($accounts)); foreach ($accounts as $account) { $this->jobList->add(SyncJob::class, ['accountId' => $account->getId()]); + $this->jobList->add(RepairSyncJob::class, ['accountId' => $account->getId()]); $this->jobList->add(TrainImportanceClassifierJob::class, ['accountId' => $account->getId()]); $this->jobList->add(PreviewEnhancementProcessingJob::class, ['accountId' => $account->getId()]); $this->jobList->add(QuotaJob::class, ['accountId' => $account->getId()]); diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index fabf4674f4..76d68c15db 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -12,6 +12,7 @@ use Horde_Imap_Client; use Horde_Imap_Client_Base; use Horde_Imap_Client_Exception; +use Horde_Imap_Client_Ids; use OCA\Mail\Account; use OCA\Mail\Contracts\IMailManager; use OCA\Mail\Db\Mailbox; @@ -496,4 +497,49 @@ private function runPartialSync( return $newOrVanished; } + + /** + * Run a (rather costly) sync to delete cached messages which are not present on IMAP anymore. + * + * @throws MailboxLockedException + * @throws ServiceException + */ + public function repairSync( + Account $account, + Mailbox $mailbox, + LoggerInterface $logger, + ): void { + $this->mailboxMapper->lockForVanishedSync($mailbox); + + $perf = $this->performanceLogger->startWithLogger( + 'Repair sync for ' . $account->getId() . ':' . $mailbox->getName(), + $logger, + ); + + // Need to use a client without a cache here (to disable QRESYNC entirely) + $client = $this->clientFactory->getClient($account, false); + try { + $knownUids = $this->dbMapper->findAllUids($mailbox); + $hordeMailbox = new \Horde_Imap_Client_Mailbox($mailbox->getName()); + $phantomVanishedUids = $client->vanished($hordeMailbox, 0, [ + 'ids' => new Horde_Imap_Client_Ids($knownUids), + ])->ids; + if (count($phantomVanishedUids) > 0) { + $this->dbMapper->deleteByUid($mailbox, ...$phantomVanishedUids); + } + } catch (Throwable $e) { + $message = sprintf( + 'Repair sync failed for %d:%s: %s', + $account->getId(), + $mailbox->getName(), + $e->getMessage(), + ); + throw new ServiceException($message, 0, $e); + } finally { + $this->mailboxMapper->unlockFromVanishedSync($mailbox); + $client->logout(); + } + + $perf->end(); + } } diff --git a/lib/Service/Sync/SyncService.php b/lib/Service/Sync/SyncService.php index e4bed5f96a..b32b6449a1 100644 --- a/lib/Service/Sync/SyncService.php +++ b/lib/Service/Sync/SyncService.php @@ -85,6 +85,16 @@ public function clearCache(Account $account, $this->synchronizer->clearCache($account, $mailbox); } + /** + * Run a (rather costly) sync to delete cached messages which are not present on IMAP anymore. + * + * @throws MailboxLockedException + * @throws ServiceException + */ + public function repairSync(Account $account, Mailbox $mailbox): void { + $this->synchronizer->repairSync($account, $mailbox, $this->logger); + } + /** * @param Account $account * @param Mailbox $mailbox diff --git a/src/components/NavigationMailbox.vue b/src/components/NavigationMailbox.vue index 004616443a..e35b80439f 100644 --- a/src/components/NavigationMailbox.vue +++ b/src/components/NavigationMailbox.vue @@ -111,6 +111,14 @@ {{ t('mail', 'Move mailbox') }} + + + {{ t('mail', 'Repair mailbox') }} + } + */ + async repair() { + this.repairing = true + + const mailboxId = this.mailbox.databaseId + try { + await repairMailbox(mailboxId) + + // Reload the page to start with a clean mailbox state + await this.$router.push({ + name: 'mailbox', + params: { + mailboxId: this.$route.params.mailboxId, + }, + }) + window.location.reload() + } catch (error) { + // Only reset state in case of an error because the page will be reloaded anyway + this.repairing = false + + // Handle rate limit: 429 Too Many Requests + // Ref https://axios-http.com/docs/handling_errors + if (error.response?.status === 429) { + showError(t('mail', 'Please wait 10 minutes before repairing again')) + } else { + throw error + } + } + }, }, } diff --git a/src/service/MailboxService.js b/src/service/MailboxService.js index 98c2e83b4c..ff5f42f030 100644 --- a/src/service/MailboxService.js +++ b/src/service/MailboxService.js @@ -66,3 +66,17 @@ export const clearMailbox = async (id) => { await axios.post(url) } + +/** + * Delete all vanished emails that are still cached. + * + * @param {number} id Mailbox database id + * @return {Promise} + */ +export const repairMailbox = async (id) => { + const url = generateUrl('/apps/mail/api/mailboxes/{id}/repair', { + id, + }) + + await axios.post(url) +} diff --git a/tests/Integration/Sync/ImapToDbSynchronizerTest.php b/tests/Integration/Sync/ImapToDbSynchronizerTest.php new file mode 100644 index 0000000000..0554106c3d --- /dev/null +++ b/tests/Integration/Sync/ImapToDbSynchronizerTest.php @@ -0,0 +1,158 @@ +synchronizer = Server::get(ImapToDbSynchronizer::class); + $this->account = $this->createTestAccount(); + } + + public function testRepairSync(): void { + // Create some test messages + $mailbox = 'INBOX'; + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Message 1') + ->finish(); + $uid1 = $this->saveMessage($mailbox, $message, $this->account); + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Message 2') + ->finish(); + $uid2 = $this->saveMessage($mailbox, $message, $this->account); + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Message 3') + ->finish(); + $uid3 = $this->saveMessage($mailbox, $message, $this->account); + + // Retrieve mailbox object + $mailManager = Server::get(IMailManager::class); + $mailBoxes = $mailManager->getMailboxes(new Account($this->account)); + $inbox = null; + foreach ($mailBoxes as $mailBox) { + if ($mailBox->getName() === 'INBOX') { + $inbox = $mailBox; + break; + } + } + + // Do an initial sync to pull in all created messages + $syncService = Server::get(SyncService::class); + $syncService->syncMailbox( + new Account($this->account), + $inbox, + Horde_Imap_Client::SYNC_NEWMSGSUIDS | Horde_Imap_Client::SYNC_FLAGSUIDS | Horde_Imap_Client::SYNC_VANISHEDUIDS, + false, + null, + [], + ); + + // Assert that there are 3 messages and nothing changes when deleting a message externally + $dbMessageMapper = Server::get(DbMessageMapper::class); + self::assertCount(3, $dbMessageMapper->findAllUids($inbox)); + $this->deleteMessagesExternally($mailbox, [$uid3]); + self::assertCount(3, $dbMessageMapper->findAllUids($inbox)); + + // Do a repair sync to get rid of the vanished message that is still in the cache + $this->synchronizer->repairSync( + new Account($this->account), + $inbox, + Server::get(LoggerInterface::class), + ); + + // Assert that the cached state has been reconciled with IMAP + self::assertEquals([$uid1, $uid2], $dbMessageMapper->findAllUids($inbox)); + } + + public function testRepairSyncNoopIfNoneVanished(): void { + // Create some test messages + $mailbox = 'INBOX'; + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Message 1') + ->finish(); + $uid1 = $this->saveMessage($mailbox, $message, $this->account); + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Message 2') + ->finish(); + $uid2 = $this->saveMessage($mailbox, $message, $this->account); + $message = $this->getMessageBuilder() + ->from('ralph@buffington@domain.tld') + ->to('user@domain.tld') + ->subject('Message 3') + ->finish(); + $uid3 = $this->saveMessage($mailbox, $message, $this->account); + + // Retrieve mailbox object + $mailManager = Server::get(IMailManager::class); + $mailBoxes = $mailManager->getMailboxes(new Account($this->account)); + $inbox = null; + foreach ($mailBoxes as $mailBox) { + if ($mailBox->getName() === 'INBOX') { + $inbox = $mailBox; + break; + } + } + + // Do an initial sync to pull in all created messages + $syncService = Server::get(SyncService::class); + $syncService->syncMailbox( + new Account($this->account), + $inbox, + Horde_Imap_Client::SYNC_NEWMSGSUIDS | Horde_Imap_Client::SYNC_FLAGSUIDS | Horde_Imap_Client::SYNC_VANISHEDUIDS, + false, + null, + [], + ); + + // Assert that there are 3 messages and nothing changes when deleting a message externally + $dbMessageMapper = Server::get(DbMessageMapper::class); + self::assertCount(3, $dbMessageMapper->findAllUids($inbox)); + + // Do a repair sync to get rid of the vanished message that is still in the cache + $this->synchronizer->repairSync( + new Account($this->account), + $inbox, + Server::get(LoggerInterface::class), + ); + + // Assert that the cached state has been reconciled with IMAP + self::assertEquals([$uid1, $uid2, $uid3], $dbMessageMapper->findAllUids($inbox)); + } +}