fix(sync): mailboxes not being synced due to short circuiting

[st3iny]

Fix #8223

If a mailbox causes threads to be rebuilt during background sync, all succeeding mailboxes will be skipped due to a short circuit evaluation of the logical or operator.

Consider this piece of code: https://3v4l.org/kK4Dc

[meichthys]

I'm not sure if it's entirely relevant, but I only have a single account and i'm still seeing the error despite patching in these changes.

[st3iny]

You might need to resync your account by removing it and adding it again. This fix will only prevent the issue from happening in the future. It does not necessarily fix all emails that are already missing.

[ChristophWurst]

If we reset sync tokens, would we able to fix historically incorrect data?

[meichthys]

@st3iny Even after removing my account and re-adding it I'm still seeing the same issue for old and new emails.
Do i need to remove the mail app and all it's data somehow before re-configuring the account?

[st3iny]

@meichthys That is bad. Are you sure that you patched the mail app and didn't update it since? Updating will overwrite the patch.

Adding the account again should be sufficient.

[meichthys]

@st3iny Yes, here is the content of my ImapToDbSynchronizer.php:

        public function syncAccount(Account $account,
                                                                LoggerInterface $logger,
                                                                bool $force = false,
                                                                int $criteria = Horde_Imap_Client::SYNC>
                $rebuildThreads = false;
                foreach ($this->mailboxMapper->findAll($account) as $mailbox) {
                        if (!$mailbox->isInbox() && !$mailbox->getSyncInBackground()) {
                                $logger->debug("Skipping mailbox sync for " . $mailbox->getId());
                                continue;
                        }
                        $logger->debug("Syncing " . $mailbox->getId());
                        if ($this->sync(
                                        $account,
                                        $mailbox,
                                        $logger,
                                        $criteria,
                                        null,
                                        $force,
                                        true
                        )) {
                                $rebuildThreads = true;
                        }
                }
                $this->dispatcher->dispatchTyped(
                        new SynchronizationEvent(
                                $account,
                                $logger,
                                $rebuildThreads,
                        )
                );
        }

There is however, another ImapToDbSynchronizer.php.save file in the same directory, but i'm assuming that does not get loaded since it is named with the .save suffix.

[st3iny]

So I've been testing this PR at my personal instance for about a week now and it fixes the issue for me. I think we should move on with this PR.

---

If we reset sync tokens, would we able to fix historically incorrect data?

Hmm, that will trigger a potential costly initial sync. It should fix the issue though. Alternatively, it would also be sufficient to trigger a vanished sync without QRESYNC once, which will just send all UIDs to the server and correctly detect vanished UIDs. That would require some extra code.

mail/lib/IMAP/Sync/Synchronizer.php

Lines 160 to 166 in ae8d245

	private function getVanishedMessageUids(Horde_Imap_Client_Base $imapClient, Horde_Imap_Client_Mailbox $mailbox, Request $request): array {
	if ($imapClient->capability->isEnabled('QRESYNC')) {
	return $imapClient->sync($mailbox, $request->getToken(), [
	'criteria' => Horde_Imap_Client::SYNC_VANISHEDUIDS,
	])->vanisheduids->ids;
	}

[st3iny]

/backport to stable3.2

[st3iny]

/backport to stable2.2

[st3iny]

Static analysis errors are unrelated.

[ChristophWurst]

Let's get this in to have new accounts sync properly and tackle historically incorrect data separately.

Alternatively, it would also be sufficient to trigger a vanished sync without QRESYNC once, which will just send all UIDs to the server and correctly detect vanished UIDs. That would require some extra code.

We'll probably have to do this with our own logic because Horde uses QRESYNC if supported. We can fetch all known UIDs in chunks and delete the ones that are not returned. That sounds doable 👍