From 89e456dfb563c11cb67bca5449b28e91c1ca5c6b Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Sat, 10 Oct 2026 13:57:48 +0000 Subject: [PATCH] feat: preserve IMAP folder sync state on incomplete provider status and honor Graph service retry windows --- docs/content/docs/guides/mailboxes.mdx | 2 + internal/app/worker/wmail/governor_test.go | 21 ++--- internal/app/worker/wmail/start_sync.go | 9 +-- internal/app/worker/wmail/sync_imap_test.go | 4 + internal/app/worker/wmail/sync_truth_test.go | 76 ++++++++++++++++++ internal/client/smtpimap/imap/folders.go | 21 ++--- internal/client/smtpimap/imap/server_test.go | 82 ++++++++++++++++++++ 7 files changed, 188 insertions(+), 27 deletions(-) diff --git a/docs/content/docs/guides/mailboxes.mdx b/docs/content/docs/guides/mailboxes.mdx index 26545d32c..2341f8324 100644 --- a/docs/content/docs/guides/mailboxes.mdx +++ b/docs/content/docs/guides/mailboxes.mdx @@ -280,6 +280,8 @@ Connecting a mailbox does two things: it imports the mailbox's recent history, a The Sync card distinguishes a completed history import from a recent successful live check. An unfinished pass, provider failure, recovery scan or budget hold does not refresh the last successful check or erase the last observed deferred count. That count is an observation from sync, not a measurement of all mail still on the provider. A worker heartbeat alone is not evidence that a mailbox is current. Repeated connection failures refresh the warning's observation time without generating a new notification each pass; no later successful check means recovery is unverified. +Sync respects an explicit provider retry window for temporary service failures as well as rate limits. On IMAP, an incomplete folder-status response or a temporary status failure keeps the existing folders and their sync positions for the next pass. Missing status metadata is requested separately, not treated as proof that a folder was deleted. + If Gmail rejects an ordinary import continuation token, the import restarts inside its original fixed date window. Already mapped mail is deduplicated, and the existing message cap and sync budgets still apply. Outlook recovers an expired delta checkpoint by replaying the configured recent-history window, reconciling known messages and following changes from a fresh checkpoint only after recovery completes. Older messages never imported are outside that recovery window. Authentication failures, throttles and transport errors are not treated as expired checkpoints. On IMAP, discovering a new folder or a changed UIDVALIDITY generation during or after the initial import schedules a bounded recent-history scan. Generation recovery reconciles known message handles and read state without restarting the global import count. In folders excluded from history imports, such as Junk and Trash, recovery updates only known messages and does not import unseen history. An unreadable or incomplete message body remains retryable rather than being stored as a successful empty message. A folder without usable modification sequences falls back to UIDNEXT checks and periodic flags even if the server advertises CONDSTORE. Changes are read with the standard CONDSTORE fetch. If a server answers that in a form Warmbly cannot read, the worker asks the other supported ways, and when none can be read it uses the same UIDNEXT checks. The mailbox keeps syncing either way. Failed flag publication keeps the previous baseline so it can retry. Background lookups do not mark unread messages as seen. diff --git a/internal/app/worker/wmail/governor_test.go b/internal/app/worker/wmail/governor_test.go index 56cc4bcfc..623667ceb 100644 --- a/internal/app/worker/wmail/governor_test.go +++ b/internal/app/worker/wmail/governor_test.go @@ -188,14 +188,17 @@ func TestNextSyncDelay(t *testing.T) { } func TestNextSyncDelayDoesNotUndercutProviderRetryAfter(t *testing.T) { - w := &WMail{} - retryAfter := 12 * time.Minute - mailErr := errx.MError(errx.MailErrorWarning, errx.MailErrorCodeSendingTooFast, "throttled", errx.MailErrorResolveMethodRetry) - mailErr.RetryAfter = retryAfter - - for i := 0; i < 50; i++ { - if got := w.nextSyncDelay(time.Minute, mailErr); got < retryAfter { - t.Fatalf("delay %v retried before provider's %v window", got, retryAfter) - } + for _, code := range []errx.MailErrorCode{errx.MailErrorCodeSendingTooFast, errx.MailErrorCodeServerUnreachable, errx.MailErrorCodeConnectionLost} { + t.Run(string(code), func(t *testing.T) { + w := &WMail{transportFailures: 1} + retryAfter := 12 * time.Minute + mailErr := errx.MError(errx.MailErrorWarning, code, "retry later", errx.MailErrorResolveMethodRetry) + mailErr.RetryAfter = retryAfter + for i := 0; i < 50; i++ { + if got := w.nextSyncDelay(time.Minute, mailErr); got < retryAfter { + t.Fatalf("delay %v retried before provider's %v window", got, retryAfter) + } + } + }) } } diff --git a/internal/app/worker/wmail/start_sync.go b/internal/app/worker/wmail/start_sync.go index 2db54e559..394aedc5a 100644 --- a/internal/app/worker/wmail/start_sync.go +++ b/internal/app/worker/wmail/start_sync.go @@ -46,13 +46,12 @@ func (w *WMail) nextSyncDelay(base time.Duration, last *errx.MailError) time.Dur d := base minimum := base / 2 switch { + case last != nil && last.RetryAfter > 0 && (isTransportError(last) || last.Code == errx.MailErrorCodeSendingTooFast): + // Service-unavailable responses can supply the same retry window as 429. + d = max(last.RetryAfter, base) + minimum = d case last != nil && last.Code == errx.MailErrorCodeSendingTooFast: - // The provider returned 429. Respect its window when supplied. d = syncBackoffMax - if last.RetryAfter > 0 { - d = max(last.RetryAfter, base) - minimum = d - } case last != nil && isTransportError(last): // The server is unreachable. Retry soon after the first failure (a // dropped session reconnects on the next pass and costs one dial), diff --git a/internal/app/worker/wmail/sync_imap_test.go b/internal/app/worker/wmail/sync_imap_test.go index 5cc493ee7..edfe7bfab 100644 --- a/internal/app/worker/wmail/sync_imap_test.go +++ b/internal/app/worker/wmail/sync_imap_test.go @@ -21,6 +21,7 @@ import ( type fakeImapConn struct { ImapConn folders []models.Mailbox + folderErr *errx.MailError changed []goimap.UID fetches int released int @@ -54,6 +55,9 @@ type fakeImapConn struct { // in place, and a fake that shared its backing array would lose folders // between passes. func (c *fakeImapConn) Folders() ([]models.Mailbox, *errx.MailError) { + if c.folderErr != nil { + return nil, c.folderErr + } return append([]models.Mailbox(nil), c.folders...), nil } diff --git a/internal/app/worker/wmail/sync_truth_test.go b/internal/app/worker/wmail/sync_truth_test.go index 306f2fe48..500b8751f 100644 --- a/internal/app/worker/wmail/sync_truth_test.go +++ b/internal/app/worker/wmail/sync_truth_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "sync/atomic" "testing" "time" @@ -63,6 +64,81 @@ func assertSyncCatchUp(t *testing.T, w *WMail, previous time.Time) { } } +func TestSyncTruthGraphServiceUnavailableHonorsRetryWindowAndRecovers(t *testing.T) { + var recovered atomic.Bool + srv := httptest.NewServer(http.HandlerFunc(func(out http.ResponseWriter, req *http.Request) { + out.Header().Set("Content-Type", "application/json") + if !recovered.Load() { + out.Header().Set("Retry-After", "720") + out.WriteHeader(http.StatusServiceUnavailable) + _, _ = out.Write([]byte(`{"error":{"code":"ServiceUnavailable"}}`)) + return + } + _, _ = fmt.Fprintf(out, `{"value":[],"@odata.deltaLink":"https://graph.microsoft.com%s?$deltatoken=recovered"}`, req.URL.Path) + })) + defer srv.Close() + var events []captured + var relayed []models.SyncState + w := newGraphTestMail(t, srv, &events, &relayed) + previous := seedSyncTruth(w, &relayed) + before := w.GraphData.Client.DeltaLinks[msgraph.FolderInbox] + + for range 2 { + err := w.syncOnce(t.Context()) + if err == nil || err.Code != errx.MailErrorCodeServerUnreachable || err.RetryAfter != 12*time.Minute { + t.Fatalf("missing retryable provider evidence: %v", err) + } + if delay := w.nextSyncDelay(time.Minute, err); delay < err.RetryAfter { + t.Fatalf("service retry window undercut: %v < %v", delay, err.RetryAfter) + } + assertNoSyncSuccess(t, w, previous) + if w.GraphData.Client.DeltaLinks[msgraph.FolderInbox] != before { + t.Fatal("failed service request advanced delta cursor") + } + } + if w.transportFailures != 2 { + t.Fatalf("failed passes reset transport evidence: %d", w.transportFailures) + } + recovered.Store(true) + if err := w.syncOnce(t.Context()); err != nil { + t.Fatal(err) + } + assertSyncCatchUp(t, w, previous) + if w.transportFailures != 0 || w.GraphData.Client.DeltaLinks[msgraph.FolderInbox] == before { + t.Fatal("provider-confirmed complete pass failed to update recovery state") + } +} + +func TestSyncTruthImapFolderFailurePreservesSavedStateUntilRecovery(t *testing.T) { + box := &models.Mailbox{Name: "Sent", UIDValidity: 7, UIDNext: 10, HighestModSeq: 30} + conn := &fakeImapConn{folders: []models.Mailbox{*box}, folderErr: errx.ErrMailServerUnreachable} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 100}, box) + w.EmailType = models.InboxProviderSMTPIMAP + var relayed []models.SyncState + previous := seedSyncTruth(w, &relayed) + w.tracker.state.BackfillCursor.Folders = map[string]models.SyncFolderCursor{"Sent": {Done: true, UID: 7}} + for range 2 { + if err := w.syncOnce(t.Context()); err == nil || err.Code != errx.MailErrorCodeServerUnreachable { + t.Fatalf("folder failure hidden: %v", err) + } + assertNoSyncSuccess(t, w, previous) + if hasEvent(*events, models.JobEventTypeMailboxDelete) || len(w.SmtpImapData.Mailboxes) != 1 || w.SmtpImapData.Mailboxes[0] != box { + t.Fatal("failed folder listing retired saved mailbox state") + } + if cursor, ok := w.tracker.state.BackfillCursor.Folders["Sent"]; !ok || cursor.UID != 7 || !cursor.Done { + t.Fatal("failed folder listing lost the durable recovery cursor") + } + } + conn.folderErr = nil + if err := w.syncOnce(t.Context()); err != nil { + t.Fatal(err) + } + assertSyncCatchUp(t, w, previous) + if hasEvent(*events, models.JobEventTypeMailboxDelete) || w.transportFailures != 0 { + t.Fatal("complete provider recovery did not preserve folder state") + } +} + func TestSyncTruthGoogleFailureHeartbeatAndRecovery(t *testing.T) { failing := true srv := httptest.NewServer(http.HandlerFunc(func(out http.ResponseWriter, _ *http.Request) { diff --git a/internal/client/smtpimap/imap/folders.go b/internal/client/smtpimap/imap/folders.go index 07d70e342..4f8830108 100644 --- a/internal/client/smtpimap/imap/folders.go +++ b/internal/client/smtpimap/imap/folders.go @@ -1,7 +1,6 @@ package imap import ( - "errors" "fmt" "sort" "strings" @@ -47,7 +46,7 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { // Asking a server without CONDSTORE for HIGHESTMODSEQ is a BAD. HighestModSeq: caps.Has(imap.CapCondStore), } - opts, listStatus := listOptionsFor(caps, status) + opts, _ := listOptionsFor(caps, status) var all []models.Mailbox statuses := map[string]*imap.StatusData{} @@ -83,24 +82,20 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { for _, box := range kept { st := statuses[box.Name] if st == nil { - if listStatus { - // The server was asked and said nothing: the folder is not - // one it can open for us. - continue - } data, err := c.client.Status(box.Name, status).Wait() if err != nil { - var imapErr *imap.Error - if errors.As(err, &imapErr) { - // One folder the server will not report on must not - // take the rest of the account with it. - log.Warn().Err(err).Str("folder", box.Name).Msg("imap: STATUS refused; folder skipped") + merr := c.handleError(err) + if merr.Code == errx.ErrMailResourceNotFound.Code { + // Only explicit absence can retire a folder's stored state. continue } - return nil, c.handleError(err) + return nil, merr } st = data } + if st == nil || st.UIDValidity == 0 || st.UIDNext == 0 { + return nil, errx.ErrMailServerUnreachable + } box.UIDValidity = st.UIDValidity box.UIDNext = uint32(st.UIDNext) box.HighestModSeq = st.HighestModSeq diff --git a/internal/client/smtpimap/imap/server_test.go b/internal/client/smtpimap/imap/server_test.go index 07185a593..b818bef5d 100644 --- a/internal/client/smtpimap/imap/server_test.go +++ b/internal/client/smtpimap/imap/server_test.go @@ -13,6 +13,7 @@ import ( "github.com/emersion/go-imap/v2" "github.com/emersion/go-imap/v2/imapserver" "github.com/emersion/go-imap/v2/imapserver/imapmemserver" + "github.com/warmbly/warmbly/internal/errx" "github.com/warmbly/warmbly/internal/models" ) @@ -262,6 +263,87 @@ func TestFoldersWithoutListStatus(t *testing.T) { } } +func TestFoldersStatusFailureDoesNotBecomeFolderAbsence(t *testing.T) { + for _, tc := range []struct { + name, reply string + code errx.MailErrorCode + }{ + {"unavailable", "NO [UNAVAILABLE] Try later", errx.MailErrorCodeServerUnreachable}, + {"server bug", "NO [SERVERBUG] Try later", errx.MailErrorCodeServerUnreachable}, + {"connection limit", "NO [LIMIT] Try later", errx.MailErrorCodeSendingTooFast}, + {"authentication", "NO [AUTHENTICATIONFAILED] Sign in again", errx.MailErrorCodeInvalidCredentials}, + {"authorization", "NO [AUTHORIZATIONFAILED] Not permitted", errx.MailErrorCodeAuthorizationFailed}, + {"unknown refusal", "NO Could not obtain status", errx.MailErrorCodeImapUnknown}, + } { + t.Run(tc.name, func(t *testing.T) { + c, _ := interceptingServer(t, nil, func(line string) string { + fields := strings.Fields(line) + if len(fields) >= 3 && strings.EqualFold(fields[1], "STATUS") && strings.Contains(line, "Sent") { + return fields[0] + " " + tc.reply + } + return "" + }, "Sent") + if err := c.Connect(); err != nil { + t.Fatal(err) + } + boxes, err := c.Folders() + if err == nil || err.Code != tc.code || len(boxes) != 0 { + t.Fatalf("partial listing could retire Sent: boxes=%v err=%v, want %s", boxes, err, tc.code) + } + }) + } +} + +func TestFoldersExplicitlyMissingStatusFolderCanBeSkipped(t *testing.T) { + c, _ := interceptingServer(t, nil, func(line string) string { + fields := strings.Fields(line) + if len(fields) >= 3 && strings.EqualFold(fields[1], "STATUS") && strings.Contains(line, "Sent") { + return fields[0] + " NO [NONEXISTENT] Folder removed" + } + return "" + }, "Sent") + boxes, err := c.Folders() + if err != nil || len(boxes) != 1 || boxes[0].Name != "INBOX" { + t.Fatalf("authoritative missing folder did not preserve remaining listing: %v %v", boxes, err) + } +} + +func TestFoldersMissingListStatusFallsBackToStatus(t *testing.T) { + suppressed := 0 + c, wire := rewritingServer(t, imap.CapSet{imap.CapIMAP4rev1: {}, imap.CapListExtended: {}, imap.CapListStatus: {}}, nil, func(line string) string { + if strings.HasPrefix(strings.ToUpper(line), "* STATUS ") && suppressed < 2 { + suppressed++ + return "" + } + return line + }, "Sent") + boxes, err := c.Folders() + if err != nil || len(boxes) != 2 || len(wire.commands("STATUS")) != 2 { + t.Fatalf("missing LIST-STATUS metadata became folder absence: boxes=%v err=%v fallback=%v", boxes, err, wire.commands("STATUS")) + } +} + +func TestFoldersIncompleteStatusRemainsRetryable(t *testing.T) { + for _, reply := range []string{"OK Status unavailable", "OK Status completed"} { + t.Run(reply, func(t *testing.T) { + c, _ := interceptingServer(t, nil, func(line string) string { + fields := strings.Fields(line) + if len(fields) >= 3 && strings.EqualFold(fields[1], "STATUS") && strings.Contains(line, "Sent") { + if reply == "OK Status completed" { + return "* STATUS Sent (MESSAGES 0)\r\n" + fields[0] + " " + reply + } + return fields[0] + " " + reply + } + return "" + }, "Sent") + boxes, err := c.Folders() + if err == nil || err.Code != errx.MailErrorCodeServerUnreachable || len(boxes) != 0 { + t.Fatalf("incomplete status was treated as authoritative: boxes=%v err=%v", boxes, err) + } + }) + } +} + // A nested folder must be listed: "%" stopped at the top level, which is how // Gmail's [Gmail]/Sent Mail and Dovecot's INBOX.Sent went unsynced. func TestFoldersListsNested(t *testing.T) {