diff --git a/internal/app/worker/wmail/folder_test.go b/internal/app/worker/wmail/folder_test.go index bd6053c6..7c3ad2a9 100644 --- a/internal/app/worker/wmail/folder_test.go +++ b/internal/app/worker/wmail/folder_test.go @@ -26,6 +26,7 @@ func TestImapCanonicalFolder(t *testing.T) { {"sent by nested name", models.Mailbox{Name: "INBOX.Sent"}, models.FolderSent}, {"spam by name", models.Mailbox{Name: "Junk E-Mail"}, models.FolderSpam}, {"trash by name", models.Mailbox{Name: "Deleted Items"}, models.FolderTrash}, + {"gmail bin is trash", models.Mailbox{Name: "[Gmail]/Bin"}, models.FolderTrash}, {"drafts by name", models.Mailbox{Name: "Drafts"}, models.FolderDrafts}, // Anything unrecognised stays visible rather than vanishing into a // scope the user never opens. @@ -40,6 +41,73 @@ func TestImapCanonicalFolder(t *testing.T) { } } +// Gmail's label views duplicate every message under another UID; a pass that +// followed them would re-file INBOX mail as archive and swap the (mailbox, +// uid) pair warmup actions address. Sent must NOT be virtual: it is the folder +// the "*" listing exists to reach. +func TestImapVirtualFolder(t *testing.T) { + for _, tc := range []struct { + box models.Mailbox + want bool + }{ + {models.Mailbox{Name: "[Gmail]/All Mail", Attrs: []string{"\\All", "\\HasNoChildren"}}, true}, + {models.Mailbox{Name: "[Gmail]/Starred", Attrs: []string{"\\Flagged"}}, true}, + {models.Mailbox{Name: "[Gmail]/Important", Attrs: []string{"\\Important"}}, true}, + // A plain LIST (no SPECIAL-USE) carries only \HasNoChildren; the name + // fallback applies inside Gmail's namespace only. + {models.Mailbox{Name: "[Gmail]/All Mail", Attrs: []string{"\\HasNoChildren"}}, true}, + {models.Mailbox{Name: "[Gmail]/Starred"}, true}, + {models.Mailbox{Name: "[Google Mail]/Important"}, true}, + // Ordinary IMAP folders that happen to share the names are real. + {models.Mailbox{Name: "Important"}, false}, + {models.Mailbox{Name: "INBOX.Starred"}, false}, + {models.Mailbox{Name: "All Mail"}, false}, + {models.Mailbox{Name: "[Gmail]/Sent Mail", Attrs: []string{"\\Sent"}}, false}, + {models.Mailbox{Name: "[Gmail]/Bin", Attrs: []string{"\\Trash"}}, false}, + {models.Mailbox{Name: "INBOX"}, false}, + } { + if got := imapVirtualFolder(&tc.box); got != tc.want { + t.Errorf("imapVirtualFolder(%q) = %v, want %v", tc.box.Name, got, tc.want) + } + } +} + +// A virtual folder is never baselined, and one a previous build did baseline +// is retired through the deletion sweep so its cursor leaves the store. +func TestImapSyncSkipsVirtualFolders(t *testing.T) { + conn := &fakeImapConn{folders: []models.Mailbox{ + {Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}, + {Name: "[Gmail]/All Mail", UIDValidity: 9, HighestModSeq: 100, Attrs: []string{"\\All"}}, + {Name: "[Gmail]/Starred", UIDValidity: 11, HighestModSeq: 100, Attrs: []string{"\\Flagged"}}, + }} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, &models.Mailbox{Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}) + w.SmtpImapData.Mailboxes = append(w.SmtpImapData.Mailboxes, + &models.Mailbox{Name: "[Gmail]/Starred", UIDValidity: 11, HighestModSeq: 100}) + + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("Sync: %v", err) + } + retired := false + for _, e := range *events { + switch e.eventType { + case models.JobEventTypeMailboxUpdate: + if got := e.body.(*models.JobEventMailboxUpdate).Data.UIDValidity; got == 9 { + t.Fatal("All Mail was baselined; virtual folders must be skipped") + } + case models.JobEventTypeMailboxDelete: + if e.body.(*models.JobEventMailboxDelete).UIDValidity == 11 { + retired = true + } + } + } + if !retired { + t.Error("the stale Starred cursor was not retired with a MAILBOX_DELETE") + } + if len(w.SmtpImapData.Mailboxes) != 1 { + t.Fatalf("tracked %d folders, want just INBOX", len(w.SmtpImapData.Mailboxes)) + } +} + // Drafts is imported now that the folder sidebar gives it a destination; the // rest of the eligibility matrix lives in TestImapBackfillEligible. func TestImapBackfillEligible_DraftsByName(t *testing.T) { diff --git a/internal/app/worker/wmail/sync_imap.go b/internal/app/worker/wmail/sync_imap.go index 2c7282e5..df665239 100644 --- a/internal/app/worker/wmail/sync_imap.go +++ b/internal/app/worker/wmail/sync_imap.go @@ -40,6 +40,9 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { if err != nil { return err } + // Dropped here, not skipped below, so a label view a previous build + // baselined falls into the deletion sweep and its cursor is retired. + folders = slices.DeleteFunc(folders, func(b models.Mailbox) bool { return imapVirtualFolder(&b) }) for i := range folders { box := &folders[i] @@ -405,6 +408,32 @@ func (w *WMail) imapBackfill(ctx context.Context, folders []models.Mailbox, stat return nil } +// imapVirtualFolder is a Gmail label view (All Mail, Starred, Important): +// every message in it also lives in a real folder under a different UID, so +// syncing it would re-file known mail (All Mail reads as archive) and swap +// the (mailbox, uid) pair the warmup actions address. Neither lane looks at +// these; a message archived out of every real folder stays unsynced, which +// is the ceiling of Gmail-over-IMAP — the OAuth Gmail path has no such gap. +func imapVirtualFolder(box *models.Mailbox) bool { + for _, a := range box.Attrs { + switch strings.ToLower(a) { + case "\\all", "\\flagged", "\\important": + return true + } + } + // Name fallback only inside Gmail's own namespace: a plain IMAP server + // can legitimately have a user folder called "Important" or "Starred". + name := strings.ToLower(box.Name) + if !strings.HasPrefix(name, "[gmail]/") && !strings.HasPrefix(name, "[google mail]/") { + return false + } + switch name[strings.Index(name, "/")+1:] { + case "all mail", "starred", "important": + return true + } + return false +} + // imapBackfillEligible excludes folders whose history is not worth importing: // trash, spam and Gmail's virtual "All Mail" (a duplicate of every other // folder). Live sync still follows them for placement signals and to file new @@ -416,6 +445,9 @@ func (w *WMail) imapBackfill(ctx context.Context, folders []models.Mailbox, stat // Special-use attributes are authoritative, with a name fallback for servers // that do not advertise them. func imapBackfillEligible(box *models.Mailbox) bool { + if imapVirtualFolder(box) { + return false + } for _, a := range box.Attrs { switch strings.ToLower(a) { case "\\noselect", "\\nonexistent", "\\trash", "\\junk", "\\all": @@ -427,7 +459,7 @@ func imapBackfillEligible(box *models.Mailbox) bool { name = name[i+1:] } switch name { - case "trash", "junk", "spam", "deleted items", "deleted messages", "junk e-mail", "junk email", "bulk mail": + case "trash", "bin", "junk", "spam", "deleted items", "deleted messages", "junk e-mail", "junk email", "bulk mail": return false } return true @@ -463,7 +495,7 @@ func imapCanonicalFolder(box *models.Mailbox) string { return models.FolderDrafts case "junk", "spam", "junk e-mail", "junk email", "bulk mail": return models.FolderSpam - case "trash", "deleted", "deleted items", "deleted messages": + case "trash", "bin", "deleted", "deleted items", "deleted messages": return models.FolderTrash case "archive", "archives", "all mail": return models.FolderArchive diff --git a/internal/client/smtpimap/imap/append_sent.go b/internal/client/smtpimap/imap/append_sent.go index dfd5271c..73cad089 100644 --- a/internal/client/smtpimap/imap/append_sent.go +++ b/internal/client/smtpimap/imap/append_sent.go @@ -30,7 +30,12 @@ func (c *Client) AppendToSent(ctx context.Context, raw []byte, sentAt time.Time) if len(raw) == 0 { return nil } + if merr := c.ensureConnected(); merr != nil { + return merr + } + // Resolved before the read lock: sentMailbox takes mu, and mu is ordered + // before lifecycle. mailbox, err := c.sentMailbox() if err != nil { return err @@ -39,6 +44,8 @@ func (c *Client) AppendToSent(ctx context.Context, raw []byte, sentAt time.Time) if sentAt.IsZero() { sentAt = time.Now() } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() cmd := c.client.Append(mailbox, int64(len(raw)), &imap.AppendOptions{ // The sender has, by definition, read what they just sent. Flags: []imap.Flag{imap.FlagSeen}, @@ -68,6 +75,8 @@ func (c *Client) sentMailbox() (string, error) { if c.sentMailboxName != "" { return c.sentMailboxName, nil } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() // RETURN (SPECIAL-USE) is only legal when the server advertises it; without // the capability the attributes may still arrive on an ordinary LIST. diff --git a/internal/client/smtpimap/imap/client.go b/internal/client/smtpimap/imap/client.go index eac96611..dd0ebaac 100644 --- a/internal/client/smtpimap/imap/client.go +++ b/internal/client/smtpimap/imap/client.go @@ -62,9 +62,45 @@ type Client struct { // When nil, WORKER_BIND_IP is consulted; when still unset, the OS default // route is used. BindIP *net.TCPAddr + + // lifecycle guards the client field itself. A reconnect holds the write + // lock through dial, auth and assignment; every command holds the read + // lock for its duration, so a reconnect never swaps the session out from + // under a command, and two paths that both see the drop dial once. + // Lock order: mu before lifecycle, and never nest a read lock. + lifecycle sync.RWMutex +} + +// ensureConnected re-dials after the server has dropped the session. go-imap +// parks a dead client in the Logout state and fails every later command with +// net.ErrClosed; nothing re-dialed, so one drop (Gmail closes sessions after a +// while) left the mailbox a zombie until the worker restarted: no sync, no +// sent copies. Every entry point that starts a command runs through here, +// before taking its own read lock. +func (c *Client) ensureConnected() *errx.MailError { + c.lifecycle.Lock() + defer c.lifecycle.Unlock() + + // Only a session that got past auth is worth keeping: a failed Login + // leaves go-imap in NotAuthenticated, which is just as unusable as Logout. + if c.client != nil { + switch c.client.State() { + case imap.ConnStateAuthenticated, imap.ConnStateSelected: + return nil + } + } + return c.connectLocked() } func (c *Client) Connect() *errx.MailError { + c.lifecycle.Lock() + defer c.lifecycle.Unlock() + return c.connectLocked() +} + +// connectLocked dials and authenticates a fresh session. lifecycle must be +// held for writing. +func (c *Client) connectLocked() *errx.MailError { var addr, host, security string var port int switch c.AuthType { @@ -113,6 +149,9 @@ func (c *Client) Connect() *errx.MailError { xerr = c.oauth2Auth() } if xerr != nil { + // Drop the half-open session so the next ensureConnected re-dials + // instead of reusing an unauthenticated client. + _ = client.Close() return xerr } @@ -120,6 +159,7 @@ func (c *Client) Connect() *errx.MailError { // Dovecot, ...) typically advertise it only after authentication, so the // check must run post-auth. if !c.client.Caps().Has(imap.CapCondStore) { + _ = client.Close() return errx.ErrMailCondStoreNotSupported } @@ -127,6 +167,11 @@ func (c *Client) Connect() *errx.MailError { } func (c *Client) Close() error { + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() + if c.client == nil { + return nil + } return c.client.Close() } @@ -167,17 +212,37 @@ func (c *Client) oauth2Auth() *errx.MailError { func (c *Client) Folders() ([]models.Mailbox, *errx.MailError) { var resp []models.Mailbox + if err := c.ensureConnected(); err != nil { + return nil, err + } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() + // LIST-STATUS: without requesting these, f.Status is nil for every // folder and the sync loop sees an empty account. - cmd := c.client.List("", "%", &imap.ListOptions{ + // + // "*", not "%": "%" stops at the top level, and on Gmail-over-IMAP every + // folder but INBOX lives under "[Gmail]/" (Dovecot commonly under + // "INBOX."), so Sent, Spam and Trash were never listed and never synced. + opts := &imap.ListOptions{ ReturnStatus: &imap.StatusOptions{ UIDValidity: true, HighestModSeq: true, }, - }) + } + // Gmail attaches \Sent, \Trash, \Junk, \All ... only when asked; on a + // plain LIST every folder is just \HasNoChildren and the canonical-folder + // mapping is left guessing from names ("Bin" filed as inbox). + if c.client.Caps().Has(imap.CapSpecialUse) { + opts.ReturnSpecialUse = true + } + cmd := c.client.List("", "*", opts) for f := cmd.Next(); f != nil; f = cmd.Next() { if len(resp) >= config.MaxEmailFolders { + // Drain the command first: unread LIST results would sit in the + // decoder channel and stall the next command on this session. + _ = cmd.Close() return nil, errx.ErrMailFoldersMax } @@ -207,6 +272,8 @@ func (c *Client) Folders() ([]models.Mailbox, *errx.MailError) { } func (c *Client) Mailbox(mailbox string, uidvali, opts *imap.SelectOptions) error { + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() if _, err := c.selectMailbox(mailbox, opts); err != nil { return err } @@ -217,7 +284,7 @@ func (c *Client) Mailbox(mailbox string, uidvali, opts *imap.SelectOptions) erro // selectMailbox is the single SELECT funnel: every path that changes the // selected mailbox goes through it so ReleaseMailbox knows whether there is // one to release. A failed SELECT leaves the session with no mailbox -// selected (RFC 3501 6.3.1). +// selected (RFC 3501 6.3.1). The caller holds the lifecycle read lock. func (c *Client) selectMailbox(mailbox string, opts *imap.SelectOptions) (*imap.SelectData, error) { data, err := c.client.Select(mailbox, opts).Wait() c.selected.Store(err == nil) @@ -230,6 +297,8 @@ func (c *Client) selectMailbox(mailbox string, opts *imap.SelectOptions) (*imap. // what arms ChangedSince. The count lets the caller skip the fetch entirely // for an empty mailbox, where a 1:* set is a server error. func (c *Client) SelectForSync(mailbox string) (uint32, *errx.MailError) { + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() data, err := c.selectMailbox(mailbox, &imap.SelectOptions{ReadOnly: true, CondStore: true}) if err != nil { return 0, c.handleError(err) @@ -247,6 +316,8 @@ func (c *Client) SelectForSync(mailbox string) (uint32, *errx.MailError) { func (c *Client) ReleaseMailbox() { c.mu.Lock() defer c.mu.Unlock() + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() if c.client == nil || !c.selected.Load() || !c.client.Caps().Has(imap.CapUnselect) { return @@ -282,6 +353,8 @@ func (c *Client) SearchChangedSince(modSeq uint64) ([]imap.UID, *errx.MailError) } func (c *Client) uidSearch(criteria *imap.SearchCriteria) ([]imap.UID, *errx.MailError) { + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() data, err := c.client.UIDSearch(criteria, nil).Wait() if err != nil { return nil, c.handleError(err) @@ -300,6 +373,8 @@ func (c *Client) FetchEnvelopes(ctx context.Context, uids []imap.UID) ([]*Fetche for _, uid := range uids { set.AddNum(uid) } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() cmd := c.client.Fetch(set, &imap.FetchOptions{ UID: true, Envelope: true, @@ -382,6 +457,8 @@ func (c *Client) FetchBody(f *Fetched) { if f == nil || f.Email == nil { return } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() f.Email.BodyPlain, f.Email.BodyHTML = fetchTextParts(c.client, f.uid, f.body) } diff --git a/internal/client/smtpimap/imap/err.go b/internal/client/smtpimap/imap/err.go index 9ddbbb8c..767bb509 100644 --- a/internal/client/smtpimap/imap/err.go +++ b/internal/client/smtpimap/imap/err.go @@ -25,5 +25,15 @@ func (c *Client) handleError(err error) *errx.MailError { } } - return nil + if err == nil { + return nil + } + + // Anything that is not a tagged IMAP response is the transport: a server + // that dropped the session (net.ErrClosed once go-imap parks the client in + // Logout), an EOF, a timeout. These used to map to nil, which turned a dead + // connection into a "clean pass with no folders" — no log, no error record, + // no new mail, forever. Retry-level, so the loop reconnects at the next + // pass instead of deactivating the mailbox. + return errx.ErrMailServerUnreachable } diff --git a/internal/client/smtpimap/imap/err_test.go b/internal/client/smtpimap/imap/err_test.go new file mode 100644 index 00000000..6eb719a4 --- /dev/null +++ b/internal/client/smtpimap/imap/err_test.go @@ -0,0 +1,28 @@ +package imap + +import ( + "io" + "net" + "testing" + + "github.com/warmbly/warmbly/internal/errx" +) + +// A transport error must never read as success: mapping net.ErrClosed to nil +// is what let a dropped Gmail session run as a clean "no folders" pass every +// minute for days, with nothing logged and no mail synced. +func TestHandleErrorTransportIsNotNil(t *testing.T) { + c := &Client{} + for _, err := range []error{net.ErrClosed, io.EOF, io.ErrUnexpectedEOF} { + got := c.handleError(err) + if got == nil { + t.Fatalf("handleError(%v) = nil, want a retryable mail error", err) + } + if got.Code != errx.MailErrorCodeServerUnreachable { + t.Errorf("handleError(%v).Code = %q, want %q", err, got.Code, errx.MailErrorCodeServerUnreachable) + } + } + if c.handleError(nil) != nil { + t.Error("handleError(nil) must stay nil") + } +} diff --git a/internal/client/smtpimap/imap/warmup_actions.go b/internal/client/smtpimap/imap/warmup_actions.go index 327f8088..83b1598f 100644 --- a/internal/client/smtpimap/imap/warmup_actions.go +++ b/internal/client/smtpimap/imap/warmup_actions.go @@ -15,6 +15,11 @@ func (c *Client) MarkAsRead(ctx context.Context, mailboxName string, uid uint32) c.mu.Lock() defer c.mu.Unlock() + if merr := c.ensureConnected(); merr != nil { + return merr + } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() if _, err := c.selectMailbox(mailboxName, nil); err != nil { return fmt.Errorf("select %q: %w", mailboxName, err) } @@ -36,6 +41,11 @@ func (c *Client) MarkImportant(ctx context.Context, mailboxName string, uid uint c.mu.Lock() defer c.mu.Unlock() + if merr := c.ensureConnected(); merr != nil { + return merr + } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() if _, err := c.selectMailbox(mailboxName, nil); err != nil { return fmt.Errorf("select %q: %w", mailboxName, err) } @@ -67,6 +77,11 @@ func (c *Client) MoveToFolder(ctx context.Context, sourceMailbox, dstFolder stri c.mu.Lock() defer c.mu.Unlock() + if merr := c.ensureConnected(); merr != nil { + return merr + } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() dst := c.qualifyMailboxLocked(dstFolder) if err := c.ensureMailboxExists(dst); err != nil { return err @@ -107,6 +122,11 @@ func (c *Client) qualifyMailboxLocked(name string) string { func (c *Client) moveUID(ctx context.Context, src, dst string, uid uint32) error { c.mu.Lock() defer c.mu.Unlock() + if merr := c.ensureConnected(); merr != nil { + return merr + } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() return c.moveUIDLocked(src, dst, uid) }