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 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/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 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() {