Honor FT_PEEK at the protocol level instead of reverting \Seen with STORE - #625
Open
ralt wants to merge 1 commit into
Open
Honor FT_PEEK at the protocol level instead of reverting \Seen with STORE#625ralt wants to merge 1 commit into
ralt wants to merge 1 commit into
Conversation
…TORE
Fetching a body with RFC822.TEXT implicitly sets \Seen (rfc3501#section-6.4.5),
so FT_PEEK was implemented as fetch-then-revert: a STORE -FLAGS \Seen per
unseen message. That flickers messages read/unread in other connected clients,
leaves them permanently marked read when the revert fails ("flag could not be
removed") or the process dies mid-way, and costs two extra round trips per
unseen message.
Instead, fetch with BODY.PEEK[TEXT] (imap_fetchbody FT_PEEK for the legacy
protocol), which never sets \Seen, and drop the reconciliation STORE. Servers
answer a BODY.PEEK[...] request with an untagged BODY[...] response, so
ImapProtocol::fetch() now matches response items against the non-PEEK name.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related PRs
I only spotted these after opening this PR — flagging the overlap upfront:
$peekparameter oncontent(),BODY.PEEK[TEXT],FT_PEEKforimap_fetchbody(), and BODY.PEEK-aware response matching. On top of that, this PR also removes the now-pointless revertSTORE -FLAGS \SeeninMessage::peek()— without that, every unseen message still costs the two extra round trips and the "flag could not be removed" failure mode remains — and adds live regression tests plus a changelog entry.BODY.PEEK[...]→BODY[...]response-matching normalization (with mocked-protocol unit tests), alongside unrelated RFC-compliance fixes.Happy to rebase this on top of either if you'd rather land those first — the
Message::peek()change and the live tests here are complementary to both.Problem
IMAP::FT_PEEK(the defaultoptions.fetch) is documented as "Fetch the message without setting the 'seen' flag", but it is currently implemented as fetch-then-revert: bodies are fetched withRFC822.TEXT, which implicitly sets\Seen(RFC 3501 §6.4.5), andMessage::peek()then sends aSTORE -FLAGS \Seenper unseen message to undo it.This has three user-visible consequences:
MessageFlagException"flag could not be removed", e.g. on folders where flag writes are refused) or the process dies between the FETCH and the STORE.unsetFlag()).Fix
Make
FT_PEEKpeek at the wire level:ImapProtocol::content()fetches withBODY.PEEK[TEXT]when peeking (new optionalbool $peek = falseparameter;BODY.PEEK[TEXT]returns the same payload asRFC822.TEXT/BODY[TEXT]per RFC 3501, there is just no PEEK variant of theRFC822.TEXTsyntax).ImapProtocol::fetch()now matches response items against the non-PEEK item name, because servers answer aBODY.PEEK[...]request with an untaggedBODY[...]response.LegacyProtocol::content()passesFT_PEEKtoimap_fetchbody().Query::fetch()andMessage::parseBody()pass their existing fetch option down tocontent().Message::peek()no longer issues the reconciliation STORE inFT_PEEKmode — the flag was never set, so there is nothing to revert. The non-peek branch (mark as seen) is unchanged.History
This restores the wire behavior the library had before 2.0: in the ext-imap era,
FT_PEEKwas passed straight toimap_fetchbody(), and c-client fetches withBODY.PEEK[...]on the wire. The socket-protocol rewrite (f83ccda, Sept 2020) hardcodedRFC822.TEXTand dropped the peek option; the revert-STORE was then added as a fix for the resulting "messages get marked as read" reports (0c61e09, #33, d00b041).BODY.PEEKitself is core IMAP4 (RFC 1730/3501, no capability negotiation needed), so every server that worked with the pre-2.0 peek mode already handled it.Backwards compatibility
ProtocolInterface::content()gains an optionalbool $peek = falseparameter — anyone implementing the interface with a custom protocol class needs to add it. Callers are unaffected.RFC822.TEXT≡BODY[TEXT]per RFC 3501).FT_PEEKnow works as documented on servers/folders where the revert STORE used to fail, and in that other clients no longer see the transient\Seen.Tests
Two live tests added to
tests/live/MessageTest.php:testFetchingBodyLeavesMessageUnseen— fetches a body with the defaultFT_PEEKand asserts, via a fresh fetch from the server, that the body is returned and\Seenis still unset. On current master this only passes because of the revert STORE; with the patch no\Seen/STORE traffic happens at all.testFetchingBodyWithoutPeekMarksMessageSeen— asserts the non-peek path still ends with the message seen.Full suite run locally against a dovecot configured like
.github/docker/dovecot_setup.sh: the set of failing tests is byte-for-byte identical before and after the patch (pre-existing, environment-related), and the new tests pass.