From b9238ab019f8fca63a66fcd0ac143f5e55c80546 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Wed, 23 Sep 2026 19:11:00 +0200 Subject: [PATCH] fix(imap): stop re-probing genuine multi-folder copies every sync repairRelocated() probed every row whose Message-ID was also filed in another folder. A message that genuinely lives in two folders survived every probe, collapse() returned 0, and it was back on the work list next sync, costing a round trip per copy per poll and filling the 100-row slice so real ghosts behind it waited. Confirmed copies now get message.copies_confirmed_at (new migration) and are left out of findMessageIdsAlsoFiledElsewhere() for 30 days. Co-Authored-By: Claude Opus 5.5 --- migrations/Version20260923101300.php | 33 ++++++++++ src/Entity/Mail/Message.php | 15 +++++ src/Repository/Mail/MessageRepository.php | 10 ++- src/Service/Imap/SentCopyReconciler.php | 66 ++++++++++++++----- .../Imap/MovedMessageReconciliationTest.php | 14 ++++ 5 files changed, 120 insertions(+), 18 deletions(-) create mode 100644 migrations/Version20260923101300.php diff --git a/migrations/Version20260923101300.php b/migrations/Version20260923101300.php new file mode 100644 index 00000000..67b3cd8a --- /dev/null +++ b/migrations/Version20260923101300.php @@ -0,0 +1,33 @@ +addSql('ALTER TABLE message ADD copies_confirmed_at TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL'); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE message DROP copies_confirmed_at'); + } +} diff --git a/src/Entity/Mail/Message.php b/src/Entity/Mail/Message.php index d0442733..26ea4bd7 100644 --- a/src/Entity/Mail/Message.php +++ b/src/Entity/Mail/Message.php @@ -85,6 +85,21 @@ class Message extends MessageModel #[ORM\Column(nullable: true)] public ?\DateTimeImmutable $vanishedAt = null; + /** + * When the server last confirmed that this message really does live in + * more than one folder. + * + * SentCopyReconciler::repairRelocated() probes every row whose Message-ID + * is also filed elsewhere, looking for ghosts a move left behind. A message + * that is genuinely in two folders survives every probe, and without a + * record of that it came back on the work list every sync, for ever — a + * round trip per copy per poll on exactly the accounts that file mail in + * several places. Rows confirmed here are left out of the list for + * SentCopyReconciler::COPIES_RECHECK_DAYS. + */ + #[ORM\Column(nullable: true)] + public ?\DateTimeImmutable $copiesConfirmedAt = null; + #[ORM\Column(type: Types::TEXT, nullable: true)] public ?string $messageId = null; diff --git a/src/Repository/Mail/MessageRepository.php b/src/Repository/Mail/MessageRepository.php index d546cfb7..09384b05 100644 --- a/src/Repository/Mail/MessageRepository.php +++ b/src/Repository/Mail/MessageRepository.php @@ -2063,10 +2063,16 @@ class MessageRepository extends ServiceEntityRepository * for the same reason: a sync must not turn into an unbounded probing run, * so the backlog drains a slice per pass. * + * Rows the server confirmed as genuine copies since $recheckAfter are left + * out: probing them again would find them all present again, and without + * this they filled the slice every sync and starved the real ghosts behind + * them. See Message::$copiesConfirmedAt. + * * @return list */ - public function findMessageIdsAlsoFiledElsewhere(Mailbox $mailbox, int $limit): array + public function findMessageIdsAlsoFiledElsewhere(Mailbox $mailbox, int $limit, \DateTimeImmutable $recheckAfter): array { + $rows = $this->createQueryBuilder('message') ->select('message.messageId') ->innerJoin( @@ -2081,8 +2087,10 @@ class MessageRepository extends ServiceEntityRepository ->andWhere('message.imapUid IS NOT NULL') ->andWhere('twin.imapUid IS NOT NULL') ->andWhere('twin.mailbox IS NOT NULL') + ->andWhere('message.copiesConfirmedAt IS NULL OR message.copiesConfirmedAt < :recheckAfter') ->groupBy('message.messageId') ->setParameter('mailbox', $mailbox) + ->setParameter('recheckAfter', $recheckAfter) ->setMaxResults($limit) ->getQuery() ->getSingleColumnResult(); diff --git a/src/Service/Imap/SentCopyReconciler.php b/src/Service/Imap/SentCopyReconciler.php index 30d6c5b9..510f28d7 100644 --- a/src/Service/Imap/SentCopyReconciler.php +++ b/src/Service/Imap/SentCopyReconciler.php @@ -79,6 +79,16 @@ final readonly class SentCopyReconciler */ private const int REPAIR_BATCH = 100; + /** + * How long a message the server confirmed as a genuine multi-folder copy + * stays off the repair pass's work list. + * + * Not for ever: a copy confirmed today can still be moved away tomorrow and + * leave a ghost of its own. A month bounds that at one probe per copy per + * month instead of one per sync. + */ + public const int COPIES_RECHECK_DAYS = 30; + public function __construct( private MessageRepository $messages, private StateManager $stateManager, @@ -334,20 +344,29 @@ final readonly class SentCopyReconciler return 0; } - $duplicated = $this->messages->findMessageIdsAlsoFiledElsewhere($mailbox, self::REPAIR_BATCH); + $now = new \DateTimeImmutable(); + $duplicated = $this->messages->findMessageIdsAlsoFiledElsewhere( + $mailbox, + self::REPAIR_BATCH, + $now->modify('-' . self::COPIES_RECHECK_DAYS . ' days'), + ); if (0 === count($duplicated)) { return 0; } - $removed = 0; + $removed = 0; + $confirmed = 0; foreach ($duplicated as $rfcMessageId) { - $removed += $this->collapse($account, $rfcMessageId, $stillExists); + $removed += $this->collapse($account, $rfcMessageId, $stillExists, $now, $confirmed); + } + + if ($removed > 0 || $confirmed > 0) { + $this->em->flush(); } if ($removed > 0) { - $this->em->flush(); $this->logger->info('Removed rows left behind by moves', [ 'mailbox' => $mailbox->fullPath, @@ -360,17 +379,24 @@ final readonly class SentCopyReconciler /** * @param callable(Mailbox, int): ?bool $stillExists + * @param int $confirmed counts rows marked as + * genuine copies */ - private function collapse(Account $account, string $rfcMessageId, callable $stillExists): int - { + private function collapse( + Account $account, + string $rfcMessageId, + callable $stillExists, + \DateTimeImmutable $now, + int &$confirmed, + ): int { $rows = $this->messages->findLocatedByMessageId($account, $rfcMessageId); if (count($rows) < 2) { return 0; } - $ghosts = []; - $survivors = 0; + $ghosts = []; + $survivors = []; foreach ($rows as $row) { $mailbox = $row->mailbox; @@ -383,7 +409,7 @@ final readonly class SentCopyReconciler $present = $stillExists($mailbox, $uid); if (true === $present) { - ++$survivors; + $survivors[] = $row; continue; } @@ -395,18 +421,24 @@ final readonly class SentCopyReconciler // null — the server did not say. Neither a survivor nor a ghost. } + // More than one real copy is a message that genuinely lives in two + // folders. Its ghosts, if any, belong to whichever copy moved, and + // pairing them up is guesswork this refuses to do. It is written down, + // though, so the next sync does not ask the server the same question. + if (count($survivors) > 1) { + foreach ($survivors as $survivor) { + $survivor->copiesConfirmedAt = $now; + ++$confirmed; + } + + return 0; + } + // Nothing to anchor the removal to. A message whose every copy has // vanished may simply have been deleted on the server, and deciding // that is not this pass's job; a probe that failed outright must never // read as deletion at all. - if (0 === $survivors || 0 === count($ghosts)) { - return 0; - } - - // More than one real copy is a message that genuinely lives in two - // folders. Its ghosts, if any, belong to whichever copy moved, and - // pairing them up is guesswork this refuses to do. - if ($survivors > 1) { + if (0 === count($survivors) || 0 === count($ghosts)) { return 0; } diff --git a/tests/Service/Imap/MovedMessageReconciliationTest.php b/tests/Service/Imap/MovedMessageReconciliationTest.php index c7852502..772f77d3 100644 --- a/tests/Service/Imap/MovedMessageReconciliationTest.php +++ b/tests/Service/Imap/MovedMessageReconciliationTest.php @@ -289,6 +289,20 @@ final class MovedMessageReconciliationTest extends KernelTestCase self::assertSame(0, $removed); self::assertSame(2, $this->countRowsFor($messageId), 'both copies are real, so both rows stay'); + + // And the answer is remembered. Without that the pair came back on the + // work list every sync and was probed over IMAP again, for ever. + $probes = 0; + $this->reconciler->repairRelocated( + $this->trash, + static function () use (&$probes): bool { + ++$probes; + + return true; + }, + ); + + self::assertSame(0, $probes, 'a confirmed pair of copies is not probed again on the next sync'); } /**