-
Notifications
You must be signed in to change notification settings - Fork 259
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.
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
fix(imap): persist vanished messages immediately on EXAMINE commands #10036
Conversation
f57cc11
to
5421a93
Compare
7dbbf6f
to
44c5315
Compare
44c5315
to
06fa8a8
Compare
lib/IMAP/IMAPClientFactory.php
Outdated
} else { | ||
// WARNING: This is very dangerous! We **will** miss changes when using QRESYNC without |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Means: Don't use mail without redis/apcu?
We added Horde_Imap_Client_Cache_Backend_Null with #6880 to mitigate performance/memory issue with larger mailboxes. Without redis/apcu larger mailboxes still work, but you have a chance of seeing those ghost messages.
"Very dangerous" => should we disable the null cache again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, now I see the problem. Without a real cache we will miss unsolicited changes because Horde will only inform the null cache and don't persist changes anywhere. QRESYNC
has side effects because it is used each time a mailbox is opened.
"Very dangerous" => should we disable the null cache again?
This would be the easiest solution. But perhaps we should implement a "thin" cache that only persists vanished messages (so it should only contain the logic I added in this PR to the cache class). This way we will still prevent said memory leak while still being informed about unsolicited sync changes. And we won't require OCP's cache to be available.
PS: I removed the whole else block in #10038 but perhaps this needs second thoughts. Maybe allowing the cache to be fully disabled for this one special case of the repair sync. It must be run without any cache whatsoever to ensure QRESYNC
being disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our discussion, I removed all traces of the old in-memory Horde cache. Only relevant information for QRESYNC is fetched from the db now. That would be the list of all cached uids and the sync token (to extract values of highestmodseq
and uidvalidity
).
Otherwise, the new cache class will behave like the built-in null cache in a sense that it won't persist any incoming changes.
67a12d7
to
c7dff93
Compare
Signed-off-by: Richard Steinmetz <[email protected]>
33cc44d
to
3b577fe
Compare
@st3iny backport? |
/backport to stable4.0 |
/backport to stable3.7 |
Problem
When
QRESYNC
is used the server will report all changed and vanished messages right away on eachEXAMINE
command. This is a bandwith optimization for IMAP. However, our code doesn't expect any (sync) changes to happen outside of explicit sync runs so we miss unsolicited changes when they are pushed on arbitrary IMAP commands (not syncs).To solve this, Horde authors came up with a "cache" that will be notified if unsolicited changes are received. However, our Horde cache class stays in memory and doesn't actually reconcile changes with the main db cache which is the source of truth.
This can lead to situations were Horde informs us of vanished messages that we delete from the memory cache but don't actually propagate to the db. Because Horde already notified us of this change, it won't necessarily inform us again on the next explicit sync.
Solution
I also added an integration test to cover the case of unsolicited changes being pushed by the server.