Skip to content

fix(search): don't block message listing on a mailbox sync lock#13208

Open
ktogias wants to merge 6 commits into
nextcloud:mainfrom
ktogias:fix/listing-not-blocked-by-sync-lock
Open

fix(search): don't block message listing on a mailbox sync lock#13208
ktogias wants to merge 6 commits into
nextcloud:mainfrom
ktogias:fix/listing-not-blocked-by-sync-lock

Conversation

@ktogias

@ktogias ktogias commented Jul 3, 2026

Copy link
Copy Markdown

Summary

MailSearch::findMessages() -- the method that serves a folder's message list -- checked $mailbox->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.

Fix

Removes the hasLocks() check from findMessages(). isCached() (a separate, legitimate check -- has this mailbox ever completed an initial sync) is unaffected and stays in place. Also removed the now-unused MailboxLockedException import (this was its only use in the file). Left the now-unused $timeFactory constructor dependency alone to keep this PR focused -- happy to clean that up too if preferred.

Test plan

  • Updated the existing test that asserted the old locked-throws behavior (testFindMessagesLocked) to instead assert findMessages() succeeds normally despite a lock being present (testFindMessagesIgnoresLock).
  • php -l clean on both changed files.
  • Ran this exact change against a live production instance for several hours (an account with an intermittently unresponsive IMAP provider) -- every folder's message list now loads instantly from cache regardless of that account's lock/sync state.

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 <info@ktogias.gr>

@ChristophWurst ChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please stop your agent and the PR burst! See comments in other PRs.

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 00ea427 ("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 <info@ktogias.gr>
@ktogias

ktogias commented Jul 3, 2026

Copy link
Copy Markdown
Author

You're right, and I'm sorry. I missed the guideline about the disclosure.

Also pushed a follow-up commit that replaces this PR's original approach.
The diagnosis (locks blocking a read-only, already-cached listing) was
right, but removing hasLocks() entirely was more than needed. The actual
bug is a one-character mistake in hasLocks() itself: || where it
should be && between "lock is set" and "lock is still within
LOCK_TIMEOUT", so any lock was treated as active forever instead of
expiring after 5 minutes as intended (see commit 00ea427, which added
that timeout but the refactor kept || at the wrong level -- full
details in the new commit's message). Fixed that instead, added direct
test coverage for it, and reverted the earlier change to findMessages().

@ktogias ktogias requested a review from ChristophWurst July 3, 2026 18:32
ktogias added 2 commits July 4, 2026 01:36
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 <info@ktogias.gr>
…lock' into fix/listing-not-blocked-by-sync-lock
@ktogias

ktogias commented Jul 3, 2026

Copy link
Copy Markdown
Author

Sorry for the back-and-forth on this one -- found a real issue with my last
revision that's worth explaining plainly.

I'd replaced the original fix (drop the lock check from findMessages()
entirely) with a smaller one (fix the hasLocks() ||/&& typo instead),
reasoning the smaller fix covered the same problem. It didn't: hasLocks()
fixed correctly still returns true for a lock that's genuinely active right
now, not just a stale one -- and blocking a read-only listing on any active
lock, stale or not, is exactly what this PR set out to fix, since
findMessages() never touches IMAP at all.

Confirmed live: with only the typo fix in place, a real (not stuck) sync
lock still caused repeated 409s on plain message listing.

Pushed a commit restoring the original approach (drop the check from
findMessages()) on top of, not instead of, the hasLocks() typo fix --
they address different failure modes (a stale lock vs. any active lock
blocking a read that never needed to check locks) and aren't alternatives
to each other.

fix(search): don't block message listing on a mailbox sync lock

Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Konstantinos Togias <info@ktogias.gr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants