From 2e47a64d8198f3229f127be3c0bd661ccbb3273b Mon Sep 17 00:00:00 2001 From: Konstantinos Togias Date: Fri, 3 Jul 2026 17:02:37 +0300 Subject: [PATCH 1/4] fix(search): don't block message listing on a mailbox sync lock MailSearch::findMessages() -- the method that serves a folder's message list -- checked hasLocks() and threw MailboxLockedException before doing anything else. But the rest of the method only ever reads already-cached messages from the local DB (messageMapper->findByIds()/ findIdsByQuery()), with no live IMAP call at all outside an unrelated body-text-search path. The sync lock exists to coordinate concurrent writes (two sync attempts racing on the same mailbox); it was never a correctness requirement for a read, just an overly cautious check that happened to also block them. On the frontend, Mailbox.vue's loadEnvelopes() responds to a MailboxLockedError by waiting 15s and recursively retrying -- with Mailbox::LOCK_TIMEOUT at 300s, that's up to ~20 silent retry cycles with the loading state up the whole time, for an operation that could have just returned the already-cached list immediately. This was especially visible for any account slow to sync (large mailbox, temporarily rate-limited provider, etc.): every folder's message list would refuse to load for minutes at a time even though the data was sitting right there in the DB the whole time. isCached() (a separate, legitimate check -- has this mailbox ever completed an initial sync) is unaffected and stays in place. Removed the now-unused MailboxLockedException import. Updated the existing test that asserted the old locked-throws behavior to instead assert findMessages() succeeds normally despite a lock being present. Signed-off-by: Konstantinos Togias --- lib/Service/Search/MailSearch.php | 10 ++++++---- tests/Unit/Service/Search/MailSearchTest.php | 19 ++++++++++++++----- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/lib/Service/Search/MailSearch.php b/lib/Service/Search/MailSearch.php index 6212fa5214..54223aaa05 100644 --- a/lib/Service/Search/MailSearch.php +++ b/lib/Service/Search/MailSearch.php @@ -16,7 +16,6 @@ use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; use OCA\Mail\Exception\ClientException; -use OCA\Mail\Exception\MailboxLockedException; use OCA\Mail\Exception\MailboxNotCachedException; use OCA\Mail\Exception\ServiceException; use OCA\Mail\IMAP\PreviewEnhancer; @@ -77,9 +76,12 @@ public function findMessages(Account $account, ?int $limit, ?string $userId, ?string $view): array { - if ($mailbox->hasLocks($this->timeFactory->getTime())) { - throw MailboxLockedException::from($mailbox); - } + // A mailbox sync lock coordinates concurrent *writes* (two sync + // attempts racing on the same mailbox); it was never a correctness + // requirement for a *read*. This method only ever reads from the + // local DB below (aside from an unrelated body-text-search path), + // so a mailbox mid-sync or currently locked can still be listed + // from its already-cached messages instead of failing outright. if (!$mailbox->isCached()) { throw MailboxNotCachedException::from($mailbox); } diff --git a/tests/Unit/Service/Search/MailSearchTest.php b/tests/Unit/Service/Search/MailSearchTest.php index 3f7ce83821..66095d361d 100644 --- a/tests/Unit/Service/Search/MailSearchTest.php +++ b/tests/Unit/Service/Search/MailSearchTest.php @@ -14,7 +14,6 @@ use OCA\Mail\Db\Mailbox; use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; -use OCA\Mail\Exception\MailboxLockedException; use OCA\Mail\Exception\MailboxNotCachedException; use OCA\Mail\IMAP\PreviewEnhancer; use OCA\Mail\IMAP\Search\Provider; @@ -81,13 +80,21 @@ public function testFindMessagesNotCached() { ); } - public function testFindMessagesLocked() { - $account = $this->createStub(Account::class); + public function testFindMessagesIgnoresLock() { + $account = $this->createMock(Account::class); + $account->expects($this->once()) + ->method('getUserId') + ->willReturn('admin'); $mailbox = new Mailbox(); + $mailbox->setSyncNewToken('abc'); + $mailbox->setSyncChangedToken('def'); + $mailbox->setSyncVanishedToken('ghi'); + // A concurrent sync attempt is (or recently was) holding a lock on + // this mailbox -- findMessages() only reads already-cached + // messages, so it must not be blocked by it. $mailbox->setSyncNewLock(123); - $this->expectException(MailboxLockedException::class); - $this->search->findMessages( + $messages = $this->search->findMessages( $account, $mailbox, 'DESC', @@ -97,6 +104,8 @@ public function testFindMessagesLocked() { null, null, ); + + $this->assertEmpty($messages); } public function testNoFindMessages() { From 28946a1f3a510994efd5219affb361400dc2e9da Mon Sep 17 00:00:00 2001 From: Konstantinos Togias Date: Fri, 3 Jul 2026 21:10:09 +0300 Subject: [PATCH 2/4] fix(mailbox): correct hasLocks() expiry check hasLocks() used `||` where it should be `&&` between "lock is set" and "lock is still within LOCK_TIMEOUT", so any non-null lock was treated as active forever instead of expiring after 5 minutes. Whenever a lock field is set, the first term alone already makes the condition true, so the timeout comparison can never actually flip the result -- it's dead code in every case. This was introduced in 00ea427332 ("Add time check to hasLocks", 2021), which explicitly intended to add this timeout; the refactor from a single `||` across the three different lock fields into per-field checks kept `||` at the wrong level. `MailboxMapper:: lockForSync()` has the correctly-scoped version of this same check (`$lock !== null && $lock > ($now - Mailbox::LOCK_TIMEOUT)`) for comparison. No existing test caught this: the only indirect coverage (MailSearchTest::testFindMessagesLocked) uses an unconfigured ITimeFactory mock, so `getTime()` defaults to 0 and any positive lock value looks "fresh" regardless of the real clock. Reverts the earlier, broader change to findMessages() (which removed the lock check entirely) in favor of this smaller, targeted fix, and adds direct hasLocks() coverage with a realistic `$now`. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Konstantinos Togias --- lib/Db/Mailbox.php | 6 +++--- lib/Service/Search/MailSearch.php | 10 ++++------ tests/Unit/Db/MailboxTest.php | 12 ++++++++++++ tests/Unit/Service/Search/MailSearchTest.php | 19 +++++-------------- 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/lib/Db/Mailbox.php b/lib/Db/Mailbox.php index 30be4ea423..ed562fcbe3 100644 --- a/lib/Db/Mailbox.php +++ b/lib/Db/Mailbox.php @@ -122,13 +122,13 @@ public function isCached(): bool { } public function hasLocks(int $now): bool { - if ($this->getSyncNewLock() !== null || $this->getSyncNewLock() > ($now - self::LOCK_TIMEOUT)) { + if ($this->getSyncNewLock() !== null && $this->getSyncNewLock() > ($now - self::LOCK_TIMEOUT)) { return true; } - if ($this->getSyncChangedLock() !== null || $this->getSyncChangedLock() > ($now - self::LOCK_TIMEOUT)) { + if ($this->getSyncChangedLock() !== null && $this->getSyncChangedLock() > ($now - self::LOCK_TIMEOUT)) { return true; } - if ($this->getSyncVanishedLock() !== null || $this->getSyncVanishedLock() > ($now - self::LOCK_TIMEOUT)) { + if ($this->getSyncVanishedLock() !== null && $this->getSyncVanishedLock() > ($now - self::LOCK_TIMEOUT)) { return true; } return false; diff --git a/lib/Service/Search/MailSearch.php b/lib/Service/Search/MailSearch.php index 54223aaa05..6212fa5214 100644 --- a/lib/Service/Search/MailSearch.php +++ b/lib/Service/Search/MailSearch.php @@ -16,6 +16,7 @@ use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; use OCA\Mail\Exception\ClientException; +use OCA\Mail\Exception\MailboxLockedException; use OCA\Mail\Exception\MailboxNotCachedException; use OCA\Mail\Exception\ServiceException; use OCA\Mail\IMAP\PreviewEnhancer; @@ -76,12 +77,9 @@ public function findMessages(Account $account, ?int $limit, ?string $userId, ?string $view): array { - // A mailbox sync lock coordinates concurrent *writes* (two sync - // attempts racing on the same mailbox); it was never a correctness - // requirement for a *read*. This method only ever reads from the - // local DB below (aside from an unrelated body-text-search path), - // so a mailbox mid-sync or currently locked can still be listed - // from its already-cached messages instead of failing outright. + if ($mailbox->hasLocks($this->timeFactory->getTime())) { + throw MailboxLockedException::from($mailbox); + } if (!$mailbox->isCached()) { throw MailboxNotCachedException::from($mailbox); } diff --git a/tests/Unit/Db/MailboxTest.php b/tests/Unit/Db/MailboxTest.php index 08fbb2505f..5d7aa17cbd 100644 --- a/tests/Unit/Db/MailboxTest.php +++ b/tests/Unit/Db/MailboxTest.php @@ -60,4 +60,16 @@ public function testJsonSerializeCacheBuster( $this->assertArrayHasKey('cacheBuster', $json); $this->assertEquals($expectedCacheBuster, $json['cacheBuster']); } + + public function testHasLocksIgnoresExpiredLock(): void { + $this->mailbox->setSyncNewLock(1000); + + $this->assertFalse($this->mailbox->hasLocks(1000 + Mailbox::LOCK_TIMEOUT + 1)); + } + + public function testHasLocksRespectsFreshLock(): void { + $this->mailbox->setSyncNewLock(1000); + + $this->assertTrue($this->mailbox->hasLocks(1000 + Mailbox::LOCK_TIMEOUT - 1)); + } } diff --git a/tests/Unit/Service/Search/MailSearchTest.php b/tests/Unit/Service/Search/MailSearchTest.php index 66095d361d..3f7ce83821 100644 --- a/tests/Unit/Service/Search/MailSearchTest.php +++ b/tests/Unit/Service/Search/MailSearchTest.php @@ -14,6 +14,7 @@ use OCA\Mail\Db\Mailbox; use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; +use OCA\Mail\Exception\MailboxLockedException; use OCA\Mail\Exception\MailboxNotCachedException; use OCA\Mail\IMAP\PreviewEnhancer; use OCA\Mail\IMAP\Search\Provider; @@ -80,21 +81,13 @@ public function testFindMessagesNotCached() { ); } - public function testFindMessagesIgnoresLock() { - $account = $this->createMock(Account::class); - $account->expects($this->once()) - ->method('getUserId') - ->willReturn('admin'); + public function testFindMessagesLocked() { + $account = $this->createStub(Account::class); $mailbox = new Mailbox(); - $mailbox->setSyncNewToken('abc'); - $mailbox->setSyncChangedToken('def'); - $mailbox->setSyncVanishedToken('ghi'); - // A concurrent sync attempt is (or recently was) holding a lock on - // this mailbox -- findMessages() only reads already-cached - // messages, so it must not be blocked by it. $mailbox->setSyncNewLock(123); + $this->expectException(MailboxLockedException::class); - $messages = $this->search->findMessages( + $this->search->findMessages( $account, $mailbox, 'DESC', @@ -104,8 +97,6 @@ public function testFindMessagesIgnoresLock() { null, null, ); - - $this->assertEmpty($messages); } public function testNoFindMessages() { From 383c137be3f766f124704aedf907308d6c50080b Mon Sep 17 00:00:00 2001 From: Konstantinos Togias Date: Sat, 4 Jul 2026 01:36:33 +0300 Subject: [PATCH 3/4] fix(search): findMessages() must never block on a sync lock at all The earlier fix in this branch corrected Mailbox::hasLocks()'s ||/&& bug so a *stale* lock expires after LOCK_TIMEOUT instead of blocking forever. That's necessary but not sufficient: a lock that's still genuinely fresh (an actual sync legitimately in progress right now) was still blocking findMessages() too -- and this method never needed locking in the first place, since it only ever reads already-cached messages from the local DB (aside from an unrelated body-text-search path). A mailbox sync lock exists to coordinate concurrent *writes*, not to guard reads. Confirmed live: with the hasLocks() check still in place, a genuinely active (not stale) lock on a large/slow account caused repeated 409s on the plain message-listing endpoint, surfacing to the user as the message list getting stuck loading -- exactly the failure mode this branch originally set out to fix, just re-triggered by a fresh lock instead of a stuck one. Drops the hasLocks() check from findMessages() entirely (restoring the original, broader approach from this branch's first commit, combined with the now-fixed hasLocks() rather than instead of it -- both fixes address different failure modes and are not alternatives). Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Konstantinos Togias --- lib/Service/Search/MailSearch.php | 14 ++++++++++---- tests/Unit/Service/Search/MailSearchTest.php | 20 +++++++++++++++----- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/lib/Service/Search/MailSearch.php b/lib/Service/Search/MailSearch.php index 6212fa5214..bacd764919 100644 --- a/lib/Service/Search/MailSearch.php +++ b/lib/Service/Search/MailSearch.php @@ -16,7 +16,6 @@ use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; use OCA\Mail\Exception\ClientException; -use OCA\Mail\Exception\MailboxLockedException; use OCA\Mail\Exception\MailboxNotCachedException; use OCA\Mail\Exception\ServiceException; use OCA\Mail\IMAP\PreviewEnhancer; @@ -77,9 +76,16 @@ public function findMessages(Account $account, ?int $limit, ?string $userId, ?string $view): array { - if ($mailbox->hasLocks($this->timeFactory->getTime())) { - throw MailboxLockedException::from($mailbox); - } + // A mailbox sync lock coordinates concurrent *writes* (two sync + // attempts racing on the same mailbox). It was never a correctness + // requirement for a *read* -- this method only ever reads from the + // local DB below (aside from an unrelated body-text-search path), + // so a mailbox mid-sync, or one whose lock happens to still be + // genuinely fresh (not just a stale bug), can still be listed from + // its already-cached messages instead of failing outright. Fixing + // hasLocks() itself (see Mailbox.php) only stops a *stale* lock + // from blocking forever -- it doesn't stop a legitimately fresh + // one from unnecessarily blocking a read that never needed it. if (!$mailbox->isCached()) { throw MailboxNotCachedException::from($mailbox); } diff --git a/tests/Unit/Service/Search/MailSearchTest.php b/tests/Unit/Service/Search/MailSearchTest.php index 3f7ce83821..4ea70e751e 100644 --- a/tests/Unit/Service/Search/MailSearchTest.php +++ b/tests/Unit/Service/Search/MailSearchTest.php @@ -14,7 +14,6 @@ use OCA\Mail\Db\Mailbox; use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; -use OCA\Mail\Exception\MailboxLockedException; use OCA\Mail\Exception\MailboxNotCachedException; use OCA\Mail\IMAP\PreviewEnhancer; use OCA\Mail\IMAP\Search\Provider; @@ -81,13 +80,22 @@ public function testFindMessagesNotCached() { ); } - public function testFindMessagesLocked() { - $account = $this->createStub(Account::class); + public function testFindMessagesIgnoresLock() { + $account = $this->createMock(Account::class); + $account->expects($this->once()) + ->method('getUserId') + ->willReturn('admin'); $mailbox = new Mailbox(); + $mailbox->setSyncNewToken('abc'); + $mailbox->setSyncChangedToken('def'); + $mailbox->setSyncVanishedToken('ghi'); + // A concurrent sync attempt is (or recently was) holding a lock on + // this mailbox -- findMessages() only reads already-cached + // messages, so it must not be blocked by it, whether the lock is + // stale or genuinely still fresh. $mailbox->setSyncNewLock(123); - $this->expectException(MailboxLockedException::class); - $this->search->findMessages( + $messages = $this->search->findMessages( $account, $mailbox, 'DESC', @@ -97,6 +105,8 @@ public function testFindMessagesLocked() { null, null, ); + + $this->assertEmpty($messages); } public function testNoFindMessages() { From 666048bbed4df74a733d673e9d4fc40f0fd8bad0 Mon Sep 17 00:00:00 2001 From: Konstantinos Togias Date: Sat, 4 Jul 2026 02:31:46 +0300 Subject: [PATCH 4/4] amend! fix(search): don't block message listing on a mailbox sync lock fix(search): don't block message listing on a mailbox sync lock Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Konstantinos Togias