From 2c6c379feba7b0acf59be8ffd2f4182e8a1b8006 Mon Sep 17 00:00:00 2001 From: q Date: Sat, 10 Oct 2026 08:45:30 +0200 Subject: [PATCH] feat: step IMAP change queries down to the next form when a server refuses one against a folder that stayed selected, retrying in the same pass, while transient NO answers stay retryable, with refusal and transient dialect tests --- AGENTS.md | 2 +- internal/client/smtpimap/imap/client.go | 56 ++++++++++++++----- .../smtpimap/imap/search_modseq_test.go | 33 +++++++++++ 3 files changed, 76 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f4603b8f4..95ca9f2cc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -525,7 +525,7 @@ What makes this sharp is that go-imap closes the whole session on any reply it c Rules: - **ask in the form the most clients exercise.** A server's mainstream path is the one Thunderbird, Apple Mail and Outlook hit every minute; its rarely used corners are where the bugs are. The CONDSTORE incremental set is `UID FETCH 1:* (UID) (CHANGEDSINCE n)` for that reason, not `SEARCH MODSEQ`: Dovecot answers the ESEARCH form with a parenthesised `(MODSEQ n)` go-imap cannot read, and Zoho answers an empty plain one with a double space -- **an optional capability degrades, it never fails the mailbox.** When a server's answer to one form cannot be read, the client steps down to the next form for good and says so once (`changedSinceMode` in `client.go`; `sinceRefused` is the same pattern for dated SEARCH). The state lives on the `Client`, so it outlives a reconnect, and the last rung is the plain UIDNEXT path every server supports. Step down only on evidence about the dialect (an unreadable reply or a refusal against a folder that stayed selected), never on a network error +- **an optional capability degrades, it never fails the mailbox.** When a server's answer to one form cannot be read, the client steps down to the next form for good and says so once (`changedSinceMode` in `client.go`; `sinceRefused` is the same pattern for dated SEARCH). The state lives on the `Client`, so it outlives a reconnect, and the last rung is the plain UIDNEXT path every server supports. Step down only on evidence about the dialect: an unreadable reply, or a refusal of the form against a folder that stayed selected, which leaves the session usable so the next form is asked in the same pass. Never on a network error or a transient NO (`UNAVAILABLE`, `INUSE`, `SERVERBUG`, a bare `UID FETCH failed`) - **read capabilities while the session is healthy.** A session the server's reply just closed reports none, so anything the ladder needs (`eSearch`, `condStore`) is recorded at connect - **a fix for one provider must not move the others.** #858 fixed Zoho by moving every server onto ESEARCH, which stalled every OVH mailbox for days. Add the provider's real reply to the dialect table in `search_modseq_test.go` (or its equivalent for the command you touch) and keep the whole table green, rather than writing a test that only knows the one server - **check a change to what the sync sends against real servers before it ships.** The cheap version: one login per distinct host, from the worker each mailbox already sits on (a new client IP can earn a provider security challenge), running the changed command and reporting only per-host results. Never print a credential or message content while doing it diff --git a/internal/client/smtpimap/imap/client.go b/internal/client/smtpimap/imap/client.go index daedcf5f4..5ddac476b 100644 --- a/internal/client/smtpimap/imap/client.go +++ b/internal/client/smtpimap/imap/client.go @@ -599,16 +599,39 @@ func (c *Client) SearchChangedSince(modSeq uint64) ([]imap.UID, *errx.MailError) if err := c.resumeSyncLocked(); err != nil { return nil, c.handleError(err) } - mode := changedSinceMode(c.changedSince.Load()) - uids, err := c.changedSinceLocked(mode, modSeq) - if err != nil { - if unreadableReply(err) { - c.stepDownChangedSince(mode, err) + for { + mode := changedSinceMode(c.changedSince.Load()) + sel := c.selection.Load() + uids, err := c.changedSinceLocked(mode, modSeq) + if err == nil { + slices.Sort(uids) + return uids, nil + } + switch { + case unreadableReply(err): + // The session is gone; the next pass reconnects and asks the next way. + c.stepDownChangedSince(mode, err, false) + case changedSinceRefused(err, sel, c.selection.Load()) && c.stepDownChangedSince(mode, err, true): + // A refusal leaves the session usable, so the next form is asked now. + if c.condStore.Load() { + continue + } + // Nothing left to ask: the next pass takes the UIDNEXT path, so this one only retries. + return nil, errx.ErrMailServerUnreachable } return nil, c.handleError(err) } - slices.Sort(uids) - return uids, nil +} + +// changedSinceRefused is the server declining the form itself, against a +// folder that stayed selected. A transient NO (UNAVAILABLE, INUSE, SERVERBUG) +// and the bare "UID FETCH failed" OVH and Zoho send under load stay retryable. +func changedSinceRefused(err error, before, after *selection) bool { + if before == nil || before != after || !searchRefused(err) { + return false + } + var imapErr *imap.Error + return !errors.As(err, &imapErr) || !bareCommandFailure(imapErr.Text) } // changedSinceMode is one way of asking what changed since a mod-sequence, @@ -674,26 +697,31 @@ func (c *Client) changedSinceLocked(mode changedSinceMode, modSeq uint64) ([]ima return data.AllUIDs(), nil } -// stepDownChangedSince moves past a mode whose answer could not be read. The -// session is already gone, so the next pass reconnects and asks the next way. -func (c *Client) stepDownChangedSince(from changedSinceMode, cause error) { +// stepDownChangedSince moves past a mode the server could not answer usably, +// reporting whether this call made the move. +func (c *Client) stepDownChangedSince(from changedSinceMode, cause error, refused bool) bool { next := from + 1 if next == changedSinceESearch && !c.eSearch.Load() { next++ } if next > changedSinceNone { - return + return false } if !c.changedSince.CompareAndSwap(int32(from), int32(next)) { - return + return false } if next == changedSinceNone { c.condStore.Store(false) } + ev := log.Warn().Str("host", c.host()).Str("from", from.String()).Str("to", next.String()).Str("reply", cause.Error()) + if refused { + ev.Msg("imap: server refused a change query; asking another way from now on") + return true + } msg := cause.Error() c.lastUnreadable.Store(&msg) - log.Warn().Str("host", c.host()).Str("from", from.String()).Str("to", next.String()).Str("reply", cause.Error()). - Msg("imap: server answered a change query in a form that cannot be read; asking another way from now on") + ev.Msg("imap: server answered a change query in a form that cannot be read; asking another way from now on") + return true } // SearchAll returns every UID in the selected mailbox, ascending. An expunge diff --git a/internal/client/smtpimap/imap/search_modseq_test.go b/internal/client/smtpimap/imap/search_modseq_test.go index 2dfcc72ba..53db0d09a 100644 --- a/internal/client/smtpimap/imap/search_modseq_test.go +++ b/internal/client/smtpimap/imap/search_modseq_test.go @@ -72,6 +72,13 @@ const ( searchOK = "* SEARCH 5 7 (MODSEQ 21)\r\n$TAG OK Search completed" // Zoho answers an empty match with a double space. searchZohoEmpty = "* SEARCH (MODSEQ 21)\r\n$TAG OK Search completed" + // Refusals of the form itself, which leave the session usable. + fetchRefused = "$TAG BAD Unknown FETCH modifier CHANGEDSINCE" + esearchRefused = "$TAG BAD Unknown search return option" + searchRefusal = "$TAG NO [CANNOT] MODSEQ search not supported" + // Answers that say nothing about the form and must stay retryable. + fetchBareFailure = "$TAG NO UID FETCH failed" + fetchUnavailable = "$TAG NO [UNAVAILABLE] Try again later" ) // runChangedSince is one sync step as the worker drives it: reconnect when the @@ -103,6 +110,9 @@ func TestSearchChangedSinceFindsAReadableForm(t *testing.T) { {"fetch unreadable, esearch fine", changedSinceDialect{fetch: fetchUnreadable, esearch: esearchOK, search: searchZohoEmpty}, true, changedSinceESearch, 1, true}, {"fetch and esearch unreadable", changedSinceDialect{fetch: fetchUnreadable, esearch: esearchDovecot, search: searchOK}, true, changedSinceSearch, 2, true}, {"no esearch skips that form", changedSinceDialect{fetch: fetchUnreadable, search: searchOK}, false, changedSinceSearch, 1, true}, + {"fetch refused, esearch asked in the same pass", changedSinceDialect{fetch: fetchRefused, esearch: esearchOK}, true, changedSinceESearch, 0, true}, + {"refused then unreadable", changedSinceDialect{fetch: fetchRefused, esearch: esearchDovecot, search: searchOK}, true, changedSinceSearch, 1, true}, + {"every form refused falls back to uidnext", changedSinceDialect{fetch: fetchRefused, esearch: esearchRefused, search: searchRefusal}, true, changedSinceNone, 1, false}, {"nothing readable falls back to uidnext", changedSinceDialect{fetch: fetchUnreadable, esearch: esearchDovecot, search: searchZohoEmpty}, true, changedSinceNone, 3, false}, } for _, tc := range cases { @@ -177,6 +187,29 @@ func TestSearchChangedSinceEmptyViewSkipsTheQuery(t *testing.T) { } } +// A transient NO is about the moment, not the form: it retries on the next +// pass and never costs the server its CONDSTORE path. +func TestSearchChangedSinceKeepsTheFormOnTransientAnswers(t *testing.T) { + for name, fetch := range map[string]string{"bare failure": fetchBareFailure, "unavailable": fetchUnavailable} { + t.Run(name, func(t *testing.T) { + d := changedSinceDialect{fetch: fetch, esearch: esearchOK, search: searchOK, exists: 2} + c, _ := rewritingServer(t, imap.CapSet{imap.CapIMAP4rev1: {}}, d.reply, advertising("CONDSTORE ESEARCH")) + for range 3 { + _, err := runChangedSince(t, c) + if err == nil || err.Code != errx.MailErrorCodeServerUnreachable { + t.Fatalf("err = %v, want a retryable SERVER_UNREACHABLE", err) + } + } + if got := changedSinceMode(c.changedSince.Load()); got != changedSinceFetch { + t.Fatalf("mode = %v, want %v", got, changedSinceFetch) + } + if !c.HasCondStore() { + t.Fatal("a transient answer turned CONDSTORE off") + } + }) + } +} + func TestUnreadableReplyIsNotTheNetwork(t *testing.T) { for _, tc := range []struct { msg string