Skip to content

Commit

Permalink
feat: implement periodic full sync job to repair cache inconsistencies
Browse files Browse the repository at this point in the history
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
  • Loading branch information
st3iny committed Sep 10, 2024
1 parent 45f2854 commit ca3d6ce
Show file tree
Hide file tree
Showing 10 changed files with 849 additions and 14 deletions.
90 changes: 90 additions & 0 deletions lib/BackgroundJob/SyncRepairJob.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Mail\BackgroundJob;

use OCA\Mail\Db\MailboxMapper;
use OCA\Mail\Events\SynchronizationEvent;
use OCA\Mail\Service\AccountService;
use OCA\Mail\Service\Sync\SyncService;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\IJobList;
use OCP\BackgroundJob\TimedJob;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;

class SyncRepairJob extends TimedJob {
public function __construct(
ITimeFactory $time,
private SyncService $syncService,
private AccountService $accountService,
private IUserManager $userManager,
private MailboxMapper $mailboxMapper,
private IJobList $jobList,
private LoggerInterface $logger,
private IEventDispatcher $dispatcher,
) {
parent::__construct($time);

$this->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) {
$isTrash = $trashMailboxId === $mailbox->getId();
$isSnooze = $snoozeMailboxId === $mailbox->getId();
$isSent = $sentMailboxId === $mailbox->getId();
$isJunk = $junkMailboxId === $mailbox->getId();
if ($isTrash || $isSnooze || $isSent || $isJunk) {
continue;
}

if ($this->syncService->repairSync($account, $mailbox) > 0) {
$rebuildThreads = true;
}
}

$this->dispatcher->dispatchTyped(
new SynchronizationEvent($account, $this->logger, $rebuildThreads),
);
}
}
14 changes: 14 additions & 0 deletions lib/Controller/MailboxesController.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
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\JSONResponse;
use OCP\IRequest;
Expand Down Expand Up @@ -286,4 +287,17 @@ public function clearMailbox(int $id): JSONResponse {
$this->mailManager->clearMailbox($account, $mailbox);
return new JSONResponse();
}

/**
* Delete all vanished mails that are still cached.
*/
#[TrapError]
#[NoAdminRequired]
public function repair(int $id): JSONResponse {
$mailbox = $this->mailManager->getMailbox($this->currentUserId, $id);
$account = $this->accountService->find($this->currentUserId, $mailbox->getAccountId());

$this->syncService->repairSync($account, $mailbox);
return new JsonResponse();
}
}
12 changes: 0 additions & 12 deletions lib/IMAP/IMAPClientFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,6 @@ public function getClient(Account $account, bool $useCache = true): Horde_Imap_C
$this->cacheFactory->createDistributed(md5((string)$account->getId())),
),
];
} else {
// WARNING: This is very dangerous! We **will** miss changes when using QRESYNC without
// actually persisting changes to the cache. Especially vanished messages will
// be missed.
/**
* If we don't use a cache we use a null cache to trick Horde into
* using QRESYNC/CONDSTORE if they are available
* @see \Horde_Imap_Client_Socket::_loginTasks
*/
$params['cache'] = [
'backend' => new Horde_Imap_Client_Cache_Backend_Null(),
];
}
if ($this->config->getSystemValue('debug', false)) {
$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log';
Expand Down
2 changes: 2 additions & 0 deletions lib/Migration/FixBackgroundJobs.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use OCA\Mail\BackgroundJob\PreviewEnhancementProcessingJob;
use OCA\Mail\BackgroundJob\QuotaJob;
use OCA\Mail\BackgroundJob\SyncJob;
use OCA\Mail\BackgroundJob\SyncRepairJob;
use OCA\Mail\BackgroundJob\TrainImportanceClassifierJob;
use OCA\Mail\Db\MailAccount;
use OCA\Mail\Db\MailAccountMapper;
Expand Down Expand Up @@ -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(SyncRepairJob::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()]);
Expand Down
52 changes: 51 additions & 1 deletion lib/Service/Sync/ImapToDbSynchronizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -42,7 +43,7 @@

class ImapToDbSynchronizer {
/** @var int */
public const MAX_NEW_MESSAGES = 5000;
public const MAX_NEW_MESSAGES = 5000000;

/** @var DatabaseMessageMapper */
private $dbMapper;
Expand Down Expand Up @@ -485,4 +486,53 @@ private function runPartialSync(

return $newOrVanished;
}

/**
* Run a (rather costly) sync to delete cached messages which are not present on IMAP anymore.
*
* @return int Number of detected vanished phantom messages
*
* @throws MailboxLockedException
* @throws ServiceException
*/
public function repairSync(
Account $account,
Mailbox $mailbox,
LoggerInterface $logger,
): int {
$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),
]);
if (count($phantomVanishedUids) > 0) {
$this->dbMapper->deleteByUid($mailbox, ...$phantomVanishedUids);

Check failure on line 519 in lib/Service/Sync/ImapToDbSynchronizer.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidArgument

lib/Service/Sync/ImapToDbSynchronizer.php:519:47: InvalidArgument: Method OCA\Mail\Db\MessageMapper::deleteByUid called with unpacked iterable Horde_Imap_Client_Ids with invalid key (must be int|string) (see https://psalm.dev/004)
}
} 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();

return count($phantomVanishedUids);
}
}
Loading

0 comments on commit ca3d6ce

Please sign in to comment.