From dab44f8407735bf309d4a7a2644cba9d1fe28a87 Mon Sep 17 00:00:00 2001 From: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com> Date: Thu, 7 May 2026 09:02:53 +0200 Subject: [PATCH] perf(imap): reuse client connection during initial sync AI-assisted: Claude Code (Claude Sonnet 4.6) Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com> --- lib/Service/Sync/ImapToDbSynchronizer.php | 89 +++++++++---------- .../Service/Sync/ImapToDbSynchronizerTest.php | 12 +-- 2 files changed, 48 insertions(+), 53 deletions(-) diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index a48805dd41..8b5361cf2c 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -335,57 +335,56 @@ private function runInitialSync( $logger ); - // Need a client without a cache - $client->logout(); - $client = $this->clientFactory->getClient($account, false); - - $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); + // Use a no-cache client for findAll. Horde accumulates cache state on + // every iteration of the findAll loop, causing a memory leak on large + // mailboxes (commit e50c214ff). Do NOT logout $client — the caller owns + // it and we need it below for getSyncToken. + $noCacheClient = $this->clientFactory->getClient($account, false); try { - $imapMessages = $this->imapMapper->findAll( - $client, - $mailbox->getName(), - self::MAX_NEW_MESSAGES, - $highestKnownUid ?? 0, - $logger, - $perf, - $account->getUserId(), - ); - $perf->step(sprintf('fetch %d messages from IMAP', count($imapMessages))); - } catch (Horde_Imap_Client_Exception $e) { - throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); - } - - foreach (array_chunk($imapMessages['messages'], 500) as $chunk) { - $messages = array_map(static fn (IMAPMessage $imapMessage) => $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()), $chunk); - $this->dbMapper->insertBulk($account, ...$messages); - $perf->step(sprintf('persist %d messages in database', count($chunk))); - // Free the memory - unset($messages); - } + $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); + try { + $imapMessages = $this->imapMapper->findAll( + $noCacheClient, + $mailbox->getName(), + self::MAX_NEW_MESSAGES, + $highestKnownUid ?? 0, + $logger, + $perf, + $account->getUserId(), + ); + $perf->step(sprintf('fetch %d messages from IMAP', count($imapMessages))); + } catch (Horde_Imap_Client_Exception $e) { + throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); + } - if (!$imapMessages['all']) { - // We might need more attempts to fill the cache - $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); - $total = $imapMessages['total']; - $cached = count($this->dbMapper->findAllUids($mailbox)); - $perf->step('find number of cached UIDs'); + foreach (array_chunk($imapMessages['messages'], 500) as $chunk) { + $messages = array_map(static fn (IMAPMessage $imapMessage) => $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()), $chunk); + $this->dbMapper->insertBulk($account, ...$messages); + $perf->step(sprintf('persist %d messages in database', count($chunk))); + // Free the memory + unset($messages); + } - $perf->end(); - throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); - } + if (!$imapMessages['all']) { + // We might need more attempts to fill the cache + $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); + $total = $imapMessages['total']; + $cached = count($this->dbMapper->findAllUids($mailbox)); + $perf->step('find number of cached UIDs'); - // Get the sync token from a cache-enabled client. The non-cache client - // used above does not activate CONDSTORE/QRESYNC, so its sync token - // lacks HIGHESTMODSEQ. Without it, the first partial sync falls into - // Horde's "no MODSEQ" fallback that resolves ALL UIDs, causing OOM on - // large mailboxes. - $client->logout(); - $cacheClient = $this->clientFactory->getClient($account); - try { - $syncToken = $cacheClient->getSyncToken($mailbox->getName()); + $perf->end(); + throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); + } } finally { - $cacheClient->logout(); + $noCacheClient->logout(); } + + // Use the cache-enabled $client passed in for the sync token. The no-cache + // client does not activate CONDSTORE/QRESYNC (commit 7980d9e40), so its + // token lacks HIGHESTMODSEQ — causing the first partial sync to resolve ALL + // UIDs and run OOM on large mailboxes. Do NOT logout $client here; the + // caller (syncAccount) owns and closes it. + $syncToken = $client->getSyncToken($mailbox->getName()); $mailbox->setSyncNewToken($syncToken); $mailbox->setSyncChangedToken($syncToken); $mailbox->setSyncVanishedToken($syncToken); diff --git a/tests/Unit/Service/Sync/ImapToDbSynchronizerTest.php b/tests/Unit/Service/Sync/ImapToDbSynchronizerTest.php index 98c3d04692..cc11db3ba6 100644 --- a/tests/Unit/Service/Sync/ImapToDbSynchronizerTest.php +++ b/tests/Unit/Service/Sync/ImapToDbSynchronizerTest.php @@ -81,18 +81,14 @@ public function testInitialSyncGetsSyncTokenFromCacheClient(): void { $initialClient = $this->createMock(Horde_Imap_Client_Socket::class); $initialClient->method('__get')->with('capability')->willReturn($capability); $noCacheClient = $this->createStub(Horde_Imap_Client_Socket::class); - $cacheClient = $this->createMock(Horde_Imap_Client_Socket::class); - $cacheClient->expects($this->once()) + $initialClient->expects($this->once()) ->method('getSyncToken') ->with('INBOX') ->willReturn('dG9rZW5XaXRoSA=='); - $cacheClient->expects($this->once())->method('logout'); - $this->clientFactory->expects($this->exactly(2)) + $this->clientFactory->expects($this->once()) ->method('getClient') - ->willReturnMap([ - [$account, false, $noCacheClient], - [$account, true, $cacheClient], - ]); + ->with($account, false) + ->willReturn($noCacheClient); $this->dbMapper->method('findHighestUid')->willReturn(null); $this->imapMapper->method('findAll')->willReturn([ 'messages' => [],