Skip to content

fix(sync): don't return the mailbox as new messages from the initial sync #13823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dillardblom wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(sync): don't return the mailbox as new messages from the initial sync #13823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/Service/Sync/SyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ public function syncMailbox(Account $account,
!$partialOnly,
);

if (!$partialOnly) {
// The initial sync fills the cache of a mailbox the client has not
// loaded yet; the client loads the first page from the message list
// right after. Returning the cache as new messages here would send
// every message of the mailbox through the preview enhancer.
return new Response([], [], [], $mailbox->getStats());

@coderabbitai coderabbitai Bot Nov 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

set -o pipefail
printf '%s\n' '--- SyncService relevant declarations and calls ---'
rg -n -F -- 'function syncMailbox' -e 'new Response' -e 'changedMessages' -e 'vanishedMessages' -e 'enhance' -e 'knownIds' lib/Service/Sync/SyncService.php
printf '%s\n' '--- SyncService bounded source ---'
nl -ba lib/Service/Sync/SyncService.php | sed -n '70,190p'
printf '%s\n' '--- Client sync application and first-page handling ---'
nl -ba src/store/mainStore/actions.js | sed -n '915,970p'
nl -ba src/store/mainStore/actions.js | sed -n '735,760p'

Repository: nextcloud/mail

Length of output: 8056

Reconcile retained IDs during initialization.

When init: true includes known IDs, derive changedMessages and vanishedMessages for those IDs before returning. Keep newMessages empty for initialization. Reuse the existing changed/vanished-ID queries, but do not run the initialization path through the full newMessages lookup or previewEnhancer->process(). This updates stale flags and removes deleted envelopes without restoring the memory-heavy full-mailbox response.

}

$query = $filter === null ? null : $this->filterStringParser->parse($filter);
return $this->getDatabaseSyncChanges(
$account,
Expand Down
58 changes: 58 additions & 0 deletions tests/Unit/Service/Sync/SyncServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,62 @@ public function testSyncMailboxReturnsFolderStats(): void {

$this->assertEquals($expectedResponse, $response);
}

public function testInitialSyncReturnsNoMessages(): void {
$account = $this->createMock(Account::class);
$account->method('getUserId')->willReturn('user');
$mailbox = new Mailbox();
$mailbox->setMessages(33000);
$mailbox->setUnseen(24000);
$this->messageConnector->expects($this->once())
->method('syncMailbox')
->willReturn(new \OCA\Mail\Protocol\SyncResult());
// The client loads the first page itself, so the cache is not read
$this->messageMapper->expects($this->never())
->method('findAllIds');
$this->messageMapper->expects($this->never())
->method('findNewIds');
$this->messageMapper->expects($this->never())
->method('findByMailboxAndIds');

$response = $this->syncService->syncMailbox(
$account,
$mailbox,
0,
false,
null,
[]
);

$this->assertEquals(new Response([], [], [], new MailboxStats(33000, 24000, null)), $response);
}

public function testPartialSyncReadsTheCache(): void {
$account = $this->createMock(Account::class);
$account->method('getUserId')->willReturn('user');
$mailbox = new Mailbox();
$mailbox->setMessages(3);
$mailbox->setUnseen(1);
$mailbox->setSyncNewToken('token');
$mailbox->setSyncChangedToken('token');
$mailbox->setSyncVanishedToken('token');
$this->messageConnector->expects($this->once())
->method('syncMailbox')
->willReturn(new \OCA\Mail\Protocol\SyncResult());
$this->messageMapper->expects($this->once())
->method('findNewIds')
->with($mailbox, [1, 2], null, 'DESC')
->willReturn([3]);
$this->messageMapper->method('findByMailboxAndIds')
->willReturn([]);

$this->syncService->syncMailbox(
$account,
$mailbox,
0,
true,
null,
[1, 2]
);
}
Comment on lines +166 to +193

@coderabbitai coderabbitai Bot Nov 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial · Functional Correctness · Quick win

Supported by static analysis

Script executed:

set -eu
printf '%s\n' '--- related test references ---'
rg -n -F --glob '*.php' -e 'syncMailbox(' -e 'getNewMessages' -e 'findByMailboxAndIds' -e 'findNewIds' tests/Unit/Service/Sync tests/Unit/Controller 2>/dev/null || true
printf '%s\n' '--- SyncServiceTest relevant range ---'
nl -ba tests/Unit/Service/Sync/SyncServiceTest.php | sed -n '1,240p'
printf '%s\n' '--- MailboxesControllerTest relevant matches and ranges ---'
rg -n -F --glob 'MailboxesControllerTest.php' -e 'sync' -e 'getNewMessages' tests/Unit/Controller 2>/dev/null || true
nl -ba tests/Unit/Controller/MailboxesControllerTest.php | sed -n '1,280p'
printf '%s\n' '--- bound production declarations ---'
rg -n -F --glob '*.php' -e 'function findByMailboxAndIds' -e 'function findNewIds' -e 'function process' lib tests | head -80
nl -ba lib/Service/Sync/SyncService.php | sed -n '110,180p'
nl -ba lib/IMAP/PreviewEnhancer.php | sed -n '1,90p'

Repository: nextcloud/mail

Length of output: 25728

Script executed:

set -eu
printf '%s\n' '--- MailboxesControllerTest sync-related block ---'
nl -ba tests/Unit/Controller/MailboxesControllerTest.php | sed -n '290,345p'
printf '%s\n' '--- MessageMapper methods ---'
nl -ba lib/Db/MessageMapper.php | sed -n '1288,1325p'
nl -ba lib/Db/MessageMapper.php | sed -n '1418,1455p'
printf '%s\n' '--- SyncService constructor and entry path ---'
nl -ba lib/Service/Sync/SyncService.php | sed -n '1,125p'
printf '%s\n' '--- Response definition ---'
nl -ba lib/IMAP/Sync/Response.php | sed -n '1,55p'

Repository: nextcloud/mail

Length of output: 11365

Assert the discovered message in the partial-sync test.

The related controller tests do not cover this syncMailbox() response path. The partial-sync test can pass when the service drops the message for ID 3.

Define the Message fixture, return it from the mapper callback, pass it through the preview enhancer, and assert getNewMessages().

Suggested fix 
 use OCA\Mail\Db\Mailbox;
+use OCA\Mail\Db\Message;
@@
+		$previewEnhancer = $this->createStub(PreviewEnhancer::class);
+		$previewEnhancer->method('process')->willReturnArgument(2);
 		$this->syncService = new SyncService(
@@
-			$this->createStub(PreviewEnhancer::class),
+			$previewEnhancer,
@@
 		$mailbox->setSyncVanishedToken('token');
+		$message = new Message();
@@
 		$this->messageMapper->method('findByMailboxAndIds')
-			->willReturn([]);
+			->willReturnCallback(static fn (Mailbox $mailbox, string $userId, array $ids): array => $ids === [3] ? [$message] : []);
 
-		$this->syncService->syncMailbox(
+		$response = $this->syncService->syncMailbox(
@@
 			[1, 2]
 		);
+
+		$this->assertSame([$message], $response->getNewMessages());

}
Loading