From 1af5ac1ea6874fb0ede152e9858d766c34170f6d Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Mon, 7 Sep 2026 04:24:09 -0700 Subject: [PATCH 1/2] feat: let a fixed mailbox problem stop showing as a permanent error (issues #362, #363): resolve a mailbox's connection errors on the first sync pass that reaches the server again, because nothing but a credential reconnect ever resolved an error row, so a five-minute outage left a red needs-attention on the mailbox for good and held its health at warning; carry what the folder listing had to skip as sync state shown in the drawer's Sync card instead of raising an error row nobody can withdraw, so the note disappears once the mailbox is back under the folder limit or the duplicate folder id is renamed; and keep only a fingerprint per message in the IMAP flag scan rather than the whole flag set and Message-ID, which held 74 MB per mailbox at the window and folder limits against 14 MB for the digests, on a worker whose base capacity is 16 mailboxes --- .../docs/development/troubleshooting.mdx | 3 +- docs/content/docs/guides/mailboxes.mdx | 4 +- internal/app/consumer/event_sync_state.go | 29 +++++ .../app/consumer/event_sync_state_test.go | 106 ++++++++++++++++++ internal/app/worker/wmail/err.go | 7 +- internal/app/worker/wmail/sync_imap_flags.go | 82 +++++++------- internal/app/worker/wmail/sync_imap_test.go | 69 ++++-------- internal/app/worker/wmail/sync_state.go | 11 ++ internal/errx/email.go | 47 +------- internal/models/sync.go | 8 ++ .../components/app/emails/SyncStatusCard.tsx | 14 +++ .../lib/api/models/app/emails/SyncState.ts | 4 + 12 files changed, 240 insertions(+), 144 deletions(-) create mode 100644 internal/app/consumer/event_sync_state_test.go diff --git a/docs/content/docs/development/troubleshooting.mdx b/docs/content/docs/development/troubleshooting.mdx index 6f14e24a..58868703 100644 --- a/docs/content/docs/development/troubleshooting.mdx +++ b/docs/content/docs/development/troubleshooting.mdx @@ -79,8 +79,9 @@ Newer builds return the invite-only refusal with its own machine code, `registra | Connecting a mailbox fails with `SERVER_UNREACHABLE` on a reachable host | The security setting does not match the server. A server expecting STARTTLS looks unreachable to a client attempting implicit TLS, and vice versa. Any port from 1 to 65535 is accepted, so the port alone no longer decides: set **Security** to SSL / TLS for a server that is encrypted from the first byte (usually SMTP `465`, IMAP `993`) and STARTTLS for one that upgrades in place (usually SMTP `587` or `2525`, IMAP `143`) | | A mailbox stalls after about an hour | The worker is missing `BOX_GOOGLE_*` or `BOX_OUTLOOK_*`. The backend starts the OAuth flow but each worker refreshes the token. Set them and restart the worker | | An IMAP mailbox connects but no mail ever arrives | Check the sync card in the mailbox drawer for a folder count. Zero folders on a reachable server means the server refused `STATUS` for every folder; the worker log names each one it skipped. A connected mailbox whose inbox is genuinely empty is normal | +| A mailbox shows an error that is already fixed | Connection errors clear themselves: the first sync pass that reaches the server again resolves them. Errors that need you to act, such as wrong credentials or a domain-authentication refusal, stay until you reconnect the mailbox or fix the cause | | An IMAP mailbox stopped syncing and the log is quiet | Sessions dropped by the server, or by a firewall that removed the mapping without closing the connection, are re-dialed on the next pass, and every pass that cannot reach the server is retried on a widening interval up to five minutes. If a mailbox is still stuck, `make logs worker` shows the folder cursors; a mailbox held by the sync budget says so in its drawer instead | -| A folder is missing from the unibox | Up to `100` folders per mailbox are synced (**Instance settings > Limits**). Past that, the inbox and the special folders are kept and the rest follow the server's order, with a warning in the drawer naming how many were left out. Gmail's All Mail, Starred and Important are label views over other folders and are deliberately never synced. A folder can also be skipped when the mail server gives it the same internal id (`UIDVALIDITY`) as another folder, which happens on servers that derive that id from the creation time; the drawer says so, and renaming or recreating the folder gives it a new one | +| A folder is missing from the unibox | Up to `100` folders per mailbox are synced (**Instance settings > Limits**). Past that, the inbox and the special folders are kept and the rest follow the server's order, and the mailbox drawer's **Sync** card names how many were left out. Gmail's All Mail, Starred and Important are label views over other folders and are deliberately never synced. A folder can also be skipped when the mail server gives it the same internal id (`UIDVALIDITY`) as another folder, which happens on servers that derive that id from the creation time; the Sync card says so, and renaming or recreating the folder gives it a new one. Both notes clear themselves on the next pass once the cause is gone | | Scheduled sends never fire | Delayed sends run through the in-process Postgres task poller (`TASKS_PROVIDER=local`), so the backend must be running | | Every send dead-letters with `permission denied` on `/data/blobs` | The `blobs` volume was created before the images owned that path, so it is still `root:root` while the services run as uid 1000. Fix it once with `docker compose -p warmbly exec -u root backend chown -R warmbly:warmbly /data/blobs`. The `blob_fs_root` health check reports it, and volumes created from current images are already correct | | `email account not found in worker` | The mailbox is assigned to a worker that no longer exists, usually because the worker was recreated and came back with a fresh UUID. The reconciler releases and re-places it on a live worker within its interval. Compose workers now keep their id in the `worker_state` volume (`WORKER_STATE_DIR`), so this stops recurring once that volume exists; removing the volume or unsetting `WORKER_STATE_DIR` reintroduces the churn. See [worker identity](/development/deployment-guide/#worker-identity) | diff --git a/docs/content/docs/guides/mailboxes.mdx b/docs/content/docs/guides/mailboxes.mdx index ede79077..b510a96f 100644 --- a/docs/content/docs/guides/mailboxes.mdx +++ b/docs/content/docs/guides/mailboxes.mdx @@ -35,7 +35,7 @@ The form picks the right one from the port as you type, so standard setups need If a mailbox fails to connect with a server-unreachable error and the host and port are definitely right, the security setting is the first thing to check. A server expecting STARTTLS looks unreachable to a client attempting implicit TLS, and vice versa. -When a mail server goes down or stops answering, the mailbox is not deactivated. Warmbly says so once in the drawer, retries on a widening interval, and picks up where it left off when the server comes back. Nothing that arrived meanwhile is skipped. +When a mail server goes down or stops answering, the mailbox is not deactivated. Warmbly says so once in the drawer, retries on a widening interval, and picks up where it left off when the server comes back. Nothing that arrived meanwhile is skipped, and the error clears itself on the first pass that reaches the server again. With two-factor authentication on, generate an app password in your provider's security settings and use that. @@ -113,7 +113,7 @@ Connecting a mailbox does two things: it imports the mailbox's recent history, a **Live sync** then follows every folder the provider exposes, including junk (so placement problems are visible) and sent mail (so a conversation shows both sides). Read state, flags, deletions and moves are mirrored too. -Nested folders are followed as well, so mail in a subfolder of the inbox or under a label group arrives like anything else. Up to `100` folders per mailbox are synced. Past that the inbox, sent, drafts, spam, trash and archive are always kept and the rest are taken in the order the server lists them, with a note in the mailbox drawer saying how many were left out. +Nested folders are followed as well, so mail in a subfolder of the inbox or under a label group arrives like anything else. Up to `100` folders per mailbox are synced. Past that the inbox, sent, drafts, spam, trash and archive are always kept and the rest are taken in the order the server lists them, with a note in the mailbox drawer's **Sync** card saying how many were left out. The note goes away by itself once the mailbox is back under the limit. On IMAP, folders are identified by the standard attributes a server publishes, and by name when it publishes none. The common names are recognized in a dozen languages, so a mailbox whose Sent folder is called "Gesendete Elemente" or "Éléments envoyés" still files sent mail as sent rather than as inbox. diff --git a/internal/app/consumer/event_sync_state.go b/internal/app/consumer/event_sync_state.go index d9ef2f6d..1ffd1bff 100644 --- a/internal/app/consumer/event_sync_state.go +++ b/internal/app/consumer/event_sync_state.go @@ -3,7 +3,9 @@ package jobs import ( "context" + "github.com/google/uuid" "github.com/rs/zerolog/log" + "github.com/warmbly/warmbly/internal/errx" "github.com/warmbly/warmbly/internal/infrastructure/pubsub" "github.com/warmbly/warmbly/internal/models" ) @@ -28,6 +30,13 @@ func (s *JobsService) HandleSyncState(ctx context.Context, e *models.JobEventSyn return err } + // A relayed state means a pass completed: the worker reached the server, + // listed its folders and finished the tick. That is the only signal we + // get that an outage is over, and without it a five-minute blip left a + // red "needs attention" on the mailbox for good, because nothing but a + // credential reconnect ever resolved an error row. + s.resolveTransientMailErrors(ctx, e.EmailID) + if s.StreamingPublisher == nil { return nil } @@ -79,3 +88,23 @@ func (s *JobsService) HandleSyncState(ctx context.Context, e *models.JobEventSyn } return nil } + +// transientMailErrorCodes are the errors a completed sync pass disproves. +// Anything that needs the user to act (credentials, domain authentication, +// fair-use deactivation) is deliberately absent: those stay until the person +// fixes them or reconnects the mailbox. +var transientMailErrorCodes = []string{ + string(errx.MailErrorCodeServerUnreachable), + string(errx.MailErrorCodeConnectionLost), + string(errx.MailErrorCodeNotFound), +} + +func (s *JobsService) resolveTransientMailErrors(ctx context.Context, emailID uuid.UUID) { + if s.EmailAccountErrorRepository == nil { + return + } + if err := s.EmailAccountErrorRepository.ResolveByCodes(ctx, emailID, transientMailErrorCodes, "sync recovered"); err != nil { + log.Warn().Str("error", err.Message).Str("email_id", emailID.String()). + Msg("could not clear the mailbox's transient errors after a successful sync") + } +} diff --git a/internal/app/consumer/event_sync_state_test.go b/internal/app/consumer/event_sync_state_test.go new file mode 100644 index 00000000..17954d3a --- /dev/null +++ b/internal/app/consumer/event_sync_state_test.go @@ -0,0 +1,106 @@ +package jobs + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/warmbly/warmbly/internal/errx" + "github.com/warmbly/warmbly/internal/models" + "github.com/warmbly/warmbly/internal/repository" +) + +// stubSyncStateRepo accepts what the worker relays. +type stubSyncStateRepo struct { + repository.EmailSyncStateRepository + put *models.SyncState +} + +func (s *stubSyncStateRepo) Get(context.Context, uuid.UUID) (*models.SyncState, error) { + return nil, nil +} + +func (s *stubSyncStateRepo) Put(_ context.Context, _, _ uuid.UUID, st *models.SyncState) error { + s.put = st + return nil +} + +// stubErrorRepo records which codes were resolved. +type stubErrorRepo struct { + repository.EmailAccountErrorRepository + resolvedCodes []string + resolvedBy string + calls int +} + +func (s *stubErrorRepo) ResolveByCodes(_ context.Context, _ uuid.UUID, codes []string, by string) *errx.Error { + s.calls++ + s.resolvedCodes = append(s.resolvedCodes, codes...) + s.resolvedBy = by + return nil +} + +// Nothing but a credential reconnect ever resolved an error row, so a mailbox +// whose server was briefly unreachable kept a red "needs attention" for good +// and never returned to healthy. A relayed sync state means the worker +// reached the server and finished a pass, which is the only evidence we get +// that the outage is over. +func TestSyncStateClearsTransientMailErrors(t *testing.T) { + errRepo := &stubErrorRepo{} + s := &JobsService{ + EmailSyncStateRepository: &stubSyncStateRepo{}, + EmailAccountErrorRepository: errRepo, + } + emailID := uuid.New() + + if err := s.HandleSyncState(context.Background(), &models.JobEventSyncState{ + UserID: uuid.New(), + EmailID: emailID, + State: models.SyncState{BackfillStatus: models.SyncBackfillComplete}, + }); err != nil { + t.Fatalf("HandleSyncState: %v", err) + } + + if errRepo.calls != 1 { + t.Fatalf("resolved errors %d times, want once per relayed state", errRepo.calls) + } + want := map[string]bool{"SERVER_UNREACHABLE": true, "CONNECTION_LOST": true, "RESOURCE_NOT_FOUND": true} + for _, code := range errRepo.resolvedCodes { + if !want[code] { + t.Errorf("resolved %q, which a completed sync does not disprove", code) + } + delete(want, code) + } + for code := range want { + t.Errorf("%q was left unresolved after a successful sync", code) + } +} + +// A completed sync says nothing about credentials, domain authentication or a +// fair-use deactivation. Clearing those would hide a problem the mailbox's +// owner still has to fix. +func TestSyncStateLeavesActionableErrorsAlone(t *testing.T) { + errRepo := &stubErrorRepo{} + s := &JobsService{ + EmailSyncStateRepository: &stubSyncStateRepo{}, + EmailAccountErrorRepository: errRepo, + } + + if err := s.HandleSyncState(context.Background(), &models.JobEventSyncState{ + UserID: uuid.New(), + EmailID: uuid.New(), + State: models.SyncState{}, + }); err != nil { + t.Fatalf("HandleSyncState: %v", err) + } + + for _, code := range errRepo.resolvedCodes { + switch code { + case "INVALID_CREDENTIALS", "AUTHENTICATION_FAILED", "DOMAIN_AUTH_REJECTED", "SYNC_FLOOD", "SYNC_FAIR_USE": + t.Errorf("a successful sync cleared %q, which it does not disprove", code) + } + } + if errRepo.resolvedBy != "sync recovered" { + t.Errorf("resolvedBy = %q, want it to say what cleared the error", errRepo.resolvedBy) + } +} diff --git a/internal/app/worker/wmail/err.go b/internal/app/worker/wmail/err.go index 0df8c949..f46c31f4 100644 --- a/internal/app/worker/wmail/err.go +++ b/internal/app/worker/wmail/err.go @@ -75,12 +75,7 @@ func mailErrorToJobEventType(mailErr *errx.MailError) models.JobEventType { case errx.MailErrorCodeServerUnreachable, errx.MailErrorCodeConnectionLost, errx.MailErrorCodeNotFound, - errx.MailErrorCodeImapUnknown, - // The mailbox has more folders than the sync follows, or two of them - // share an id. Warnings, not failures: the inbox and the special - // folders are always among the ones kept, so mail keeps arriving. - errx.MailErrorCodeFolderLimit, - errx.MailErrorCodeFolderConflict: + errx.MailErrorCodeImapUnknown: return models.JobEventTypeEmailServerError } return "" diff --git a/internal/app/worker/wmail/sync_imap_flags.go b/internal/app/worker/wmail/sync_imap_flags.go index 0ef97dcb..7d760147 100644 --- a/internal/app/worker/wmail/sync_imap_flags.go +++ b/internal/app/worker/wmail/sync_imap_flags.go @@ -2,6 +2,8 @@ package wmail import ( "context" + "hash/fnv" + "strings" "time" "github.com/google/uuid" @@ -11,26 +13,17 @@ import ( "github.com/warmbly/warmbly/internal/models" ) -// reportFolderOverflow tells the user once per worker session that the -// mailbox has more folders than the sync follows. Once, not per pass: the -// condition is static until someone reorganizes their mail, and a warning a -// minute would bury every real error in the drawer. +// reportFolderOverflow records what the folder listing could not follow, as +// state rather than as an error raised once. +// +// Both conditions are things the user can fix (get under the folder cap, +// rename the folder the server gave a duplicate id), and an error row is +// never withdrawn once written, so raising one meant a red "needs attention" +// that stayed after the problem was gone. Relaying the counts every pass +// makes the warning disappear on its own. func (w *WMail) reportFolderOverflow() { - if w.SmtpImapData.overflowReported { - return - } client := w.SmtpImapData.ImapClient - over, conflicts := client.FolderOverflow(), client.FolderConflicts() - if over <= 0 && conflicts <= 0 { - return - } - w.SmtpImapData.overflowReported = true - if over > 0 { - w.CaptureError(errx.ErrMailFoldersOverflow(over)) - } - if conflicts > 0 { - w.CaptureError(errx.ErrMailFoldersConflict(conflicts)) - } + w.tracker.setFoldersSkipped(client.FolderOverflow(), client.FolderConflicts()) } // imapScanFlags mirrors read state and flag changes on a server without @@ -64,12 +57,16 @@ func (w *WMail) imapScanFlags(ctx context.Context, box *models.Mailbox, stats *t // The first scan of a folder only records the baseline: without a // previous scan every message would read as changed and the whole window // would be relayed for nothing. + next := make(map[uint32]uint64, len(flags)) + for uid, state := range flags { + next[uid] = flagFingerprint(state.Flags) + } if scan != nil { for uid, state := range flags { before, ok := scan.flags[uid] // Not in the previous scan means it arrived since; the UIDNEXT // path owns it and will store it with its flags. - if !ok || sameFlags(before.Flags, state.Flags) { + if !ok || before == next[uid] { continue } if err := w.relayFlags(ctx, box, uid, state, stats); err != nil { @@ -80,7 +77,7 @@ func (w *WMail) imapScanFlags(ctx context.Context, box *models.Mailbox, stats *t } } } - w.flagScan[box.UIDValidity] = &folderFlagScan{at: now, flags: flags} + w.flagScan[box.UIDValidity] = &folderFlagScan{at: now, flags: next} return nil } @@ -116,31 +113,32 @@ func (w *WMail) relayFlags(ctx context.Context, box *models.Mailbox, uid uint32, return nil } -// folderFlagScan is the previous flag snapshot of one folder, held in worker -// memory only: a replaced worker re-baselines on its first scan, which costs -// one FETCH and no wrong updates. +// folderFlagScan is the previous scan of one folder, held in worker memory +// only: a replaced worker re-baselines on its first scan, which costs one +// FETCH and no wrong updates. +// +// A fingerprint per UID, not the flags themselves. The scan only has to +// answer "did this change", and the current fetch already carries the +// Message-ID for any UID that did, so keeping the strings cost 74 MB per +// mailbox at the window and folder limits against 14 MB for the digests, on +// a worker whose base capacity is 16 mailboxes. type folderFlagScan struct { at time.Time - flags map[uint32]imap.FlagState + flags map[uint32]uint64 } -func sameFlags(a, b []string) bool { - if len(a) != len(b) { - return false +// flagFingerprint digests a message's flag set, order-independently: servers +// do not promise an order and a reordered set is not a change. XOR of the +// per-flag hashes gives that for free, and a duplicate flag cancelling itself +// out is not a case a server produces. +func flagFingerprint(flags []string) uint64 { + var sum uint64 + for _, f := range flags { + h := fnv.New64a() + _, _ = h.Write([]byte(strings.ToLower(f))) + sum ^= h.Sum64() } - // Flag sets are tiny (under ten) and usually identical, so the quadratic - // compare beats allocating a set per message per scan. - for _, x := range a { - found := false - for _, y := range b { - if x == y { - found = true - break - } - } - if !found { - return false - } - } - return true + // Distinguish "no flags" from "never seen": a zero fingerprint is a + // legitimate empty set, and the caller checks presence separately. + return sum } diff --git a/internal/app/worker/wmail/sync_imap_test.go b/internal/app/worker/wmail/sync_imap_test.go index 97c17bca..4ac37385 100644 --- a/internal/app/worker/wmail/sync_imap_test.go +++ b/internal/app/worker/wmail/sync_imap_test.go @@ -523,37 +523,10 @@ func TestImapFlagScanIsSkippedWithCondStore(t *testing.T) { } } -// The overflow warning is reported once, not once a minute: the condition is -// static until someone reorganizes their mail. -func TestFolderOverflowIsReportedOnce(t *testing.T) { - conn := &fakeImapConn{ - folders: []models.Mailbox{{Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}}, - overflow: 12, - } - w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, - &models.Mailbox{Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}) - - for i := 0; i < 3; i++ { - if err := w.Sync(t.Context()); err != nil { - t.Fatalf("Sync: %v", err) - } - } - warnings := 0 - for _, e := range *events { - if e.eventType == models.JobEventTypeEmailServerError { - warnings++ - } - } - if warnings != 1 { - t.Errorf("relayed %d folder-overflow warnings over three passes, want 1", warnings) - } -} - -// The two reasons a folder goes unsynced need their own codes, because the way -// out differs: get under the cap, versus rename the folder the server gave a -// duplicate id. A warning that never reaches the user is the same as no -// warning, so this checks the event as well as the code. -func TestFolderProblemsReachTheUserWithTheirOwnCodes(t *testing.T) { +// What the listing could not follow is state, not an event: an error row is +// never withdrawn, so raising one left a red "needs attention" after the user +// had fixed the problem. The counts ride the sync state and clear themselves. +func TestFoldersSkippedIsRelayedAsStateAndClears(t *testing.T) { conn := &fakeImapConn{ folders: []models.Mailbox{{Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}}, overflow: 3, @@ -565,25 +538,27 @@ func TestFolderProblemsReachTheUserWithTheirOwnCodes(t *testing.T) { if err := w.Sync(t.Context()); err != nil { t.Fatalf("Sync: %v", err) } - - codes := map[string]bool{} + if got := w.tracker.state.FoldersSkippedCap; got != 3 { + t.Errorf("FoldersSkippedCap = %d, want 3", got) + } + if got := w.tracker.state.FoldersSkippedConflict; got != 2 { + t.Errorf("FoldersSkippedConflict = %d, want 2", got) + } + // Never as an error: those are never withdrawn. for _, e := range *events { - if e.eventType != models.JobEventTypeEmailServerError { - continue - } - ev, ok := e.body.(models.EmailErrorEvent) - if !ok { - t.Fatalf("server error carried %T, want an EmailErrorEvent", e.body) - } - codes[ev.ErrorCode] = true - if ev.ErrorType != string(errx.MailErrorWarning) { - t.Errorf("%s was relayed as %q; neither reason deactivates a mailbox", ev.ErrorCode, ev.ErrorType) + if e.eventType == models.JobEventTypeEmailServerError { + t.Error("a folder problem was raised as an error row, which nothing ever resolves") } } - for _, want := range []string{string(errx.MailErrorCodeFolderLimit), string(errx.MailErrorCodeFolderConflict)} { - if !codes[want] { - t.Errorf("%s never reached the user; the warning would go nowhere", want) - } + + // The user moves folders until the mailbox fits again. + conn.overflow, conn.conflicts = 0, 0 + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("second Sync: %v", err) + } + if w.tracker.state.FoldersSkippedCap != 0 || w.tracker.state.FoldersSkippedConflict != 0 { + t.Errorf("counts stayed at %d/%d after the condition cleared", + w.tracker.state.FoldersSkippedCap, w.tracker.state.FoldersSkippedConflict) } } diff --git a/internal/app/worker/wmail/sync_state.go b/internal/app/worker/wmail/sync_state.go index 9a3d8d52..495344bc 100644 --- a/internal/app/worker/wmail/sync_state.go +++ b/internal/app/worker/wmail/sync_state.go @@ -69,6 +69,17 @@ func (t *syncTracker) setDeferred(n int) { } } +// setFoldersSkipped records what the folder listing could not follow. Both +// are conditions the user can fix, so they are relayed every pass and clear +// themselves rather than leaving a warning nobody can withdraw. +func (t *syncTracker) setFoldersSkipped(cap, conflict int) { + if t.state.FoldersSkippedCap != cap || t.state.FoldersSkippedConflict != conflict { + t.state.FoldersSkippedCap = cap + t.state.FoldersSkippedConflict = conflict + t.dirty = true + } +} + // touch stamps the tick and relays the state when it changed or the // heartbeat is due. func (t *syncTracker) touch(now time.Time) { diff --git a/internal/errx/email.go b/internal/errx/email.go index b33a401a..b8419f8d 100644 --- a/internal/errx/email.go +++ b/internal/errx/email.go @@ -5,7 +5,6 @@ import ( "time" "github.com/google/uuid" - "github.com/warmbly/warmbly/internal/config" ) type MailErrorLogType string @@ -30,12 +29,7 @@ const ( type MailErrorCode string const ( - MailErrorCodeFolderLimit MailErrorCode = "MAX_FOLDERS_REACHED" - // MailErrorCodeFolderConflict is two folders on one mailbox reporting - // the same UIDVALIDITY, the id a folder is stored under. Kept apart from - // the folder limit because the way out is renaming the folder, not - // deleting folders to get under a cap. - MailErrorCodeFolderConflict MailErrorCode = "FOLDER_ID_CONFLICT" + MailErrorCodeFolderLimit MailErrorCode = "MAX_FOLDERS_REACHED" MailErrorCodeUpdateLimit MailErrorCode = "MAX_FOLDERS_REACHED" MailErrorCodeGoogleAuth MailErrorCode = "GOOGLE_AUTHENTICATION_FAILED" MailErrorCodeGooglePayment MailErrorCode = "GOOGLE_PAYMENT_REQUIRED" @@ -125,18 +119,6 @@ func MError(eType MailErrorType, code MailErrorCode, message string, resolveMeth } var ( - // ErrMailFoldersOverflow is relayed once per worker session when a - // mailbox has more folders than the sync follows. The inbox and the - // special folders are always among the ones kept. - ErrMailFoldersOverflow = func(left int) *MailError { - return MError(MailErrorWarning, MailErrorCodeFolderLimit, fmt.Sprintf("This mailbox has %d more folders than the %d Warmbly follows. The inbox, sent, drafts, spam and trash are always synced; the rest are taken in the server's order.", left, config.MaxEmailFolders), MailErrorResolveMethodNone) - } - // ErrMailFoldersConflict is relayed when the mail server gives two - // folders the same UIDVALIDITY, which is the id everything downstream - // identifies a folder by. Only one of them can be followed. - ErrMailFoldersConflict = func(left int) *MailError { - return MError(MailErrorWarning, MailErrorCodeFolderConflict, fmt.Sprintf("%d folder(s) on this mailbox share an internal id with another folder, so only one of each pair is synced.", left), MailErrorResolveMethodNone) - } ErrMailUpdateLimit = MError(MailErrorCritical, MailErrorCodeUpdateLimit, "Your inbox has received an unusually large number of updates. Please reactivate your inbox once the issue is resolved.", MailErrorResolveMethodReload) ErrMailGoogleAuth = MError(MailErrorCritical, MailErrorCodeGoogleAuth, "Cannot access your Gmail account. Please re-authorize your account to restore mailbox access.", MailErrorResolveMethodReload) ErrMailGooglePayment = MError(MailErrorCritical, MailErrorCodeGooglePayment, "Gmail access blocked due to unpaid invoices. Please resolve the payment with Google.", MailErrorResolveMethodReload) @@ -251,33 +233,6 @@ func (e *MailError) GetUserErrorInfo() UserErrorInfo { case MailErrorCodeServerUnreachable: info.Title = "Connection Error" info.ActionRequired = "The email server is temporarily unavailable. We'll retry automatically." - case MailErrorCodeFolderLimit: - info.Title = "Some folders are not synced" - info.ActionRequired = "Move or delete folders you no longer need if one you rely on is missing from the unibox." - case MailErrorCodeFolderConflict: - info.Title = "Two folders share an internal id" - info.ActionRequired = "Your mail server gave two folders the same id, so only one of them is synced. Renaming the folder that is missing from the unibox, or recreating it, usually gives it a new one." - case MailErrorCodeNotFound: - info.Title = "Mailbox Item Missing" - info.ActionRequired = "The folder or message is no longer on the mail server. Nothing to do; we'll skip it." - case MailErrorCodeRateLimitExceeded: - info.Title = "Rate Limit Exceeded" - info.ActionRequired = "Your account has been temporarily limited due to unusual activity" - case MailErrorCodeSyncFlood: - info.Title = "Sync stopped: unusual volume" - info.ActionRequired = "Check what is delivering mail into this mailbox, then reactivate it under Mailboxes" - case MailErrorCodeSyncFairUse: - info.Title = "Sync stopped: fair use" - info.ActionRequired = "Reduce the volume landing in this mailbox or ask your administrator to raise the sync budget, then reactivate it" - case MailErrorCodeSendingTooFast: - info.Title = "Sending Too Fast" - info.ActionRequired = "Please wait before sending more emails" - case MailErrorCodeQuotaExceeded: - info.Title = "Quota Exceeded" - info.ActionRequired = "Your daily sending limit has been reached" - case MailErrorCodeAccountSuspended: - info.Title = "Account Suspended" - info.ActionRequired = "Contact your email provider to resolve this issue" case MailErrorCodeRecipientRejected: info.Title = "Recipient Rejected" info.ActionRequired = "The recipient address was not accepted" diff --git a/internal/models/sync.go b/internal/models/sync.go index 73c0537b..5534f152 100644 --- a/internal/models/sync.go +++ b/internal/models/sync.go @@ -81,6 +81,14 @@ type SyncState struct { // server but not yet stored. Drops back to zero once they are admitted. Deferred int `json:"deferred" avro:"deferred"` + // FoldersSkippedCap and FoldersSkippedConflict are what the last folder + // listing could not follow: more folders than the sync covers, and + // folders the server gave the same internal id. Carried as state rather + // than raised as an error once, so the warning goes away by itself when + // the user fixes it. + FoldersSkippedCap int `json:"folders_skipped_cap,omitempty" avro:"folders_skipped_cap"` + FoldersSkippedConflict int `json:"folders_skipped_conflict,omitempty" avro:"folders_skipped_conflict"` + LastSyncedAt *time.Time `json:"last_synced_at,omitempty" avro:"last_synced_at"` } diff --git a/web/src/components/app/emails/SyncStatusCard.tsx b/web/src/components/app/emails/SyncStatusCard.tsx index 3654b16b..cc8e5d9d 100644 --- a/web/src/components/app/emails/SyncStatusCard.tsx +++ b/web/src/components/app/emails/SyncStatusCard.tsx @@ -114,6 +114,20 @@ export default function SyncStatusCard({ mailboxId }: { mailboxId: string }) { {state!.deferred.toLocaleString()} message{state!.deferred === 1 ? "" : "s"} waiting on the server.

)} + + {(state?.folders_skipped_cap ?? 0) > 0 && ( +

+ {state!.folders_skipped_cap!.toLocaleString()} folder{state!.folders_skipped_cap === 1 ? " is" : "s are"} not synced: this mailbox has more + folders than Warmbly follows. Your inbox, sent, drafts, spam and trash are always included. +

+ )} + + {(state?.folders_skipped_conflict ?? 0) > 0 && ( +

+ {state!.folders_skipped_conflict!.toLocaleString()} folder{state!.folders_skipped_conflict === 1 ? "" : "s"} share an internal id with another + folder, so only one of each pair is synced. Renaming the missing folder on your mail server usually gives it a new id. +

+ )} ); } diff --git a/web/src/lib/api/models/app/emails/SyncState.ts b/web/src/lib/api/models/app/emails/SyncState.ts index eac94cf8..861b9bf7 100644 --- a/web/src/lib/api/models/app/emails/SyncState.ts +++ b/web/src/lib/api/models/app/emails/SyncState.ts @@ -20,6 +20,10 @@ export interface SyncState { throttle_reason?: SyncThrottleReason | ""; // Live messages seen on the server but waiting on budget. deferred: number; + /** Folders the last listing could not follow because the mailbox has more than the sync covers. */ + folders_skipped_cap?: number; + /** Folders the mail server gave the same internal id as another folder. */ + folders_skipped_conflict?: number; last_synced_at?: string; } From f1b634533748e04664fefdc60b0eed5e869c832f Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Mon, 7 Sep 2026 04:36:50 -0700 Subject: [PATCH 2/2] feat: address the CodeRabbit review on the mailbox error lifecycle PR by bounding the transient-error resolution to failures raised before the sync pass ran, because JetStream is configured with MaxDeliver and no MaxAckPending so a redelivered older success could otherwise clear a newer outage and leave the mailbox looking healthy until the next distinct one, naming the archive folder in the drawer's folder note alongside the others the cap always keeps, and giving the duplicate-folder note a singular verb --- internal/app/consumer/event_sync_state.go | 14 ++++-- .../app/consumer/event_sync_state_test.go | 47 ++++++++++++++++--- internal/repository/pg_email_error.go | 35 ++++++++++++++ .../components/app/emails/SyncStatusCard.tsx | 4 +- 4 files changed, 88 insertions(+), 12 deletions(-) diff --git a/internal/app/consumer/event_sync_state.go b/internal/app/consumer/event_sync_state.go index 1ffd1bff..c6c42604 100644 --- a/internal/app/consumer/event_sync_state.go +++ b/internal/app/consumer/event_sync_state.go @@ -2,6 +2,7 @@ package jobs import ( "context" + "time" "github.com/google/uuid" "github.com/rs/zerolog/log" @@ -35,7 +36,7 @@ func (s *JobsService) HandleSyncState(ctx context.Context, e *models.JobEventSyn // get that an outage is over, and without it a five-minute blip left a // red "needs attention" on the mailbox for good, because nothing but a // credential reconnect ever resolved an error row. - s.resolveTransientMailErrors(ctx, e.EmailID) + s.resolveTransientMailErrors(ctx, e.EmailID, e.State.LastSyncedAt) if s.StreamingPublisher == nil { return nil @@ -99,11 +100,18 @@ var transientMailErrorCodes = []string{ string(errx.MailErrorCodeNotFound), } -func (s *JobsService) resolveTransientMailErrors(ctx context.Context, emailID uuid.UUID) { +func (s *JobsService) resolveTransientMailErrors(ctx context.Context, emailID uuid.UUID, syncedAt *time.Time) { if s.EmailAccountErrorRepository == nil { return } - if err := s.EmailAccountErrorRepository.ResolveByCodes(ctx, emailID, transientMailErrorCodes, "sync recovered"); err != nil { + // Only errors raised before this pass ran. The bus can redeliver an older + // event after a newer one, and without the bound a stale success would + // clear a failure that happened after it, leaving the mailbox looking + // healthy while it was not. + if syncedAt == nil { + return + } + if err := s.EmailAccountErrorRepository.ResolveByCodesBefore(ctx, emailID, transientMailErrorCodes, *syncedAt, "sync recovered"); err != nil { log.Warn().Str("error", err.Message).Str("email_id", emailID.String()). Msg("could not clear the mailbox's transient errors after a successful sync") } diff --git a/internal/app/consumer/event_sync_state_test.go b/internal/app/consumer/event_sync_state_test.go index 17954d3a..0776d431 100644 --- a/internal/app/consumer/event_sync_state_test.go +++ b/internal/app/consumer/event_sync_state_test.go @@ -3,6 +3,7 @@ package jobs import ( "context" "testing" + "time" "github.com/google/uuid" "github.com/warmbly/warmbly/internal/errx" @@ -25,17 +26,19 @@ func (s *stubSyncStateRepo) Put(_ context.Context, _, _ uuid.UUID, st *models.Sy return nil } -// stubErrorRepo records which codes were resolved. +// stubErrorRepo records which codes were resolved and how far back. type stubErrorRepo struct { repository.EmailAccountErrorRepository - resolvedCodes []string - resolvedBy string - calls int + resolvedCodes []string + resolvedBy string + resolvedBefore time.Time + calls int } -func (s *stubErrorRepo) ResolveByCodes(_ context.Context, _ uuid.UUID, codes []string, by string) *errx.Error { +func (s *stubErrorRepo) ResolveByCodesBefore(_ context.Context, _ uuid.UUID, codes []string, before time.Time, by string) *errx.Error { s.calls++ s.resolvedCodes = append(s.resolvedCodes, codes...) + s.resolvedBefore = before s.resolvedBy = by return nil } @@ -53,14 +56,19 @@ func TestSyncStateClearsTransientMailErrors(t *testing.T) { } emailID := uuid.New() + syncedAt := time.Now() if err := s.HandleSyncState(context.Background(), &models.JobEventSyncState{ UserID: uuid.New(), EmailID: emailID, - State: models.SyncState{BackfillStatus: models.SyncBackfillComplete}, + State: models.SyncState{BackfillStatus: models.SyncBackfillComplete, LastSyncedAt: &syncedAt}, }); err != nil { t.Fatalf("HandleSyncState: %v", err) } + if !errRepo.resolvedBefore.Equal(syncedAt) { + t.Errorf("resolved errors raised before %v, want the pass's own timestamp %v", errRepo.resolvedBefore, syncedAt) + } + if errRepo.calls != 1 { t.Fatalf("resolved errors %d times, want once per relayed state", errRepo.calls) } @@ -86,10 +94,11 @@ func TestSyncStateLeavesActionableErrorsAlone(t *testing.T) { EmailAccountErrorRepository: errRepo, } + syncedAt := time.Now() if err := s.HandleSyncState(context.Background(), &models.JobEventSyncState{ UserID: uuid.New(), EmailID: uuid.New(), - State: models.SyncState{}, + State: models.SyncState{LastSyncedAt: &syncedAt}, }); err != nil { t.Fatalf("HandleSyncState: %v", err) } @@ -104,3 +113,27 @@ func TestSyncStateLeavesActionableErrorsAlone(t *testing.T) { t.Errorf("resolvedBy = %q, want it to say what cleared the error", errRepo.resolvedBy) } } + +// The bus redelivers: JetStream is configured with MaxDeliver and no +// MaxAckPending, so a stale "the sync succeeded" can arrive after a newer +// failure. Resolution is bounded to errors raised before the pass ran, and a +// state carrying no timestamp cannot be bounded, so it resolves nothing +// rather than clearing a failure it knows nothing about. +func TestSyncStateWithoutATimestampResolvesNothing(t *testing.T) { + errRepo := &stubErrorRepo{} + s := &JobsService{ + EmailSyncStateRepository: &stubSyncStateRepo{}, + EmailAccountErrorRepository: errRepo, + } + + if err := s.HandleSyncState(context.Background(), &models.JobEventSyncState{ + UserID: uuid.New(), + EmailID: uuid.New(), + State: models.SyncState{}, + }); err != nil { + t.Fatalf("HandleSyncState: %v", err) + } + if errRepo.calls != 0 { + t.Errorf("resolved errors %d times from a state with no timestamp to bound it by", errRepo.calls) + } +} diff --git a/internal/repository/pg_email_error.go b/internal/repository/pg_email_error.go index 28450b05..deb2a724 100644 --- a/internal/repository/pg_email_error.go +++ b/internal/repository/pg_email_error.go @@ -53,6 +53,10 @@ type EmailAccountErrorRepository interface { // ResolveByCodes resolves the account's unresolved errors carrying any of // the given codes, for a flow that just fixed that class of error. ResolveByCodes(ctx context.Context, accountID uuid.UUID, codes []string, resolvedBy string) *errx.Error + // ResolveByCodesBefore is ResolveByCodes bounded to errors raised before + // a given moment, so evidence of recovery cannot resolve a failure that + // happened after it. + ResolveByCodesBefore(ctx context.Context, accountID uuid.UUID, codes []string, before time.Time, resolvedBy string) *errx.Error ResolveAllForAccount(ctx context.Context, accountID uuid.UUID, resolvedBy string) *errx.Error } @@ -247,6 +251,37 @@ func (r *emailAccountErrorRepository) ResolveByCodes(ctx context.Context, accoun return nil } +// ResolveByCodesBefore resolves the account's unresolved errors carrying any +// of the codes that were raised before the given moment. +// +// The bound matters because the bus can redeliver: JetStream is configured +// with MaxDeliver and no MaxAckPending, so an older event can arrive after a +// newer one. Without it, a stale "the sync succeeded" could clear a failure +// that happened afterwards, and the mailbox would look healthy while it was +// not, until the next distinct outage raised a fresh row. +func (r *emailAccountErrorRepository) ResolveByCodesBefore(ctx context.Context, accountID uuid.UUID, codes []string, before time.Time, resolvedBy string) *errx.Error { + if len(codes) == 0 { + return nil + } + + query := ` + UPDATE email_account_errors + SET resolved_at = NOW(), resolved_by = $1 + WHERE email_account_id = $2 + AND error_code = ANY($3::text[]) + AND created_at < $4 + AND resolved_at IS NULL + ` + + _, err := r.DB.Exec(ctx, query, resolvedBy, accountID, codes, before) + if err != nil { + db.CaptureError(err, query, []any{resolvedBy, accountID, codes, before}, "exec") + return errx.InternalError() + } + + return nil +} + // ResolveAllForAccount resolves all unresolved errors for an email account func (r *emailAccountErrorRepository) ResolveAllForAccount(ctx context.Context, accountID uuid.UUID, resolvedBy string) *errx.Error { query := ` diff --git a/web/src/components/app/emails/SyncStatusCard.tsx b/web/src/components/app/emails/SyncStatusCard.tsx index cc8e5d9d..2a7c40f4 100644 --- a/web/src/components/app/emails/SyncStatusCard.tsx +++ b/web/src/components/app/emails/SyncStatusCard.tsx @@ -118,13 +118,13 @@ export default function SyncStatusCard({ mailboxId }: { mailboxId: string }) { {(state?.folders_skipped_cap ?? 0) > 0 && (

{state!.folders_skipped_cap!.toLocaleString()} folder{state!.folders_skipped_cap === 1 ? " is" : "s are"} not synced: this mailbox has more - folders than Warmbly follows. Your inbox, sent, drafts, spam and trash are always included. + folders than Warmbly follows. Your inbox, sent, drafts, archive, spam and trash are always included.

)} {(state?.folders_skipped_conflict ?? 0) > 0 && (

- {state!.folders_skipped_conflict!.toLocaleString()} folder{state!.folders_skipped_conflict === 1 ? "" : "s"} share an internal id with another + {state!.folders_skipped_conflict!.toLocaleString()} folder{state!.folders_skipped_conflict === 1 ? " shares" : "s share"} an internal id with another folder, so only one of each pair is synced. Renaming the missing folder on your mail server usually gives it a new id.

)}