starred/warmbly
1
0
Fork 0
mirror of https://github.com/warmbly/warmbly.git synced 2026-10-27 08:02:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source
This commit is contained in:
q
2026-10-10 08:45:30 +02:00
parent 695ac14b24
commit 2c6c379feb
3 changed files with 76 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
AGENTS.md
+1 -1
View File
@@ -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
internal/client/smtpimap/imap/client.go
+42 -14
View File
@@ -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
internal/client/smtpimap/imap/search_modseq_test.go
+33
View File
@@ -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