From 04d73acafdd8de4edee7e9763799324e6e706757 Mon Sep 17 00:00:00 2001 From: SUMAN JANA Date: Mon, 7 Sep 2026 06:44:54 +0000 Subject: [PATCH 1/3] fix(worker): reconnect dropped IMAP sessions and sync nested Gmail folders The worker opened one IMAP session per mailbox at load and never re-dialed. When the server dropped it (Gmail does after a while), handleError mapped net.ErrClosed to nil, so every pass ran as a clean "no folders" pass: nothing logged, no error record, last_synced_at kept moving, no new mail for days, and sent copies failed with "use of closed network connection". - imap.Client: ensureConnected re-dials a client parked in Logout state; every entry point (Folders, AppendToSent, warmup actions) runs through it. - handleError: transport errors return a retryable server-unreachable error instead of nil, so the sync loop logs and retries. - Folders: LIST "*" instead of "%", which stopped at the top level and never reached [Gmail]/Sent Mail (or Dovecot's INBOX.*); request SPECIAL-USE, without which Gmail reports no \Sent/\Trash/\Junk/\All. - Sync: skip Gmail's virtual label views (All Mail, Starred, Important) so known mail is not re-filed as archive under a different UID; retire a cursor an earlier build baselined for them. "Bin" is trash. --- internal/app/worker/wmail/folder_test.go | 62 +++++++++++++++++++ internal/app/worker/wmail/sync_imap.go | 34 +++++++++- internal/client/smtpimap/imap/append_sent.go | 3 + internal/client/smtpimap/imap/client.go | 39 +++++++++++- internal/client/smtpimap/imap/err.go | 12 +++- internal/client/smtpimap/imap/err_test.go | 28 +++++++++ .../client/smtpimap/imap/warmup_actions.go | 12 ++++ 7 files changed, 185 insertions(+), 5 deletions(-) create mode 100644 internal/client/smtpimap/imap/err_test.go diff --git a/internal/app/worker/wmail/folder_test.go b/internal/app/worker/wmail/folder_test.go index bd6053c6..a6db2ef0 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,67 @@ 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. + {models.Mailbox{Name: "[Gmail]/All Mail", Attrs: []string{"\\HasNoChildren"}}, true}, + {models.Mailbox{Name: "[Gmail]/Starred"}, true}, + {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..79b44d42 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,30 @@ 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 := strings.ToLower(box.Name) + if i := strings.LastIndexAny(name, "/."); i >= 0 { + name = name[i+1:] + } + switch name { + 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 +443,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 +457,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 +493,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..9c07e2d9 100644 --- a/internal/client/smtpimap/imap/append_sent.go +++ b/internal/client/smtpimap/imap/append_sent.go @@ -30,6 +30,9 @@ 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 + } mailbox, err := c.sentMailbox() if err != nil { diff --git a/internal/client/smtpimap/imap/client.go b/internal/client/smtpimap/imap/client.go index eac96611..051bd2cc 100644 --- a/internal/client/smtpimap/imap/client.go +++ b/internal/client/smtpimap/imap/client.go @@ -62,6 +62,26 @@ type Client struct { // When nil, WORKER_BIND_IP is consulted; when still unset, the OS default // route is used. BindIP *net.TCPAddr + + // reconnectMu serializes ensureConnected between the sync loop (which does + // not hold mu) and the send/warmup paths (which do), so a drop seen by both + // at once dials one new session, not two. + reconnectMu sync.Mutex +} + +// 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. +func (c *Client) ensureConnected() *errx.MailError { + c.reconnectMu.Lock() + defer c.reconnectMu.Unlock() + + if c.client != nil && c.client.State() != imap.ConnStateLogout { + return nil + } + return c.Connect() } func (c *Client) Connect() *errx.MailError { @@ -167,14 +187,29 @@ 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 + } + // 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 { 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..8053e146 100644 --- a/internal/client/smtpimap/imap/warmup_actions.go +++ b/internal/client/smtpimap/imap/warmup_actions.go @@ -15,6 +15,9 @@ 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 + } if _, err := c.selectMailbox(mailboxName, nil); err != nil { return fmt.Errorf("select %q: %w", mailboxName, err) } @@ -36,6 +39,9 @@ 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 + } if _, err := c.selectMailbox(mailboxName, nil); err != nil { return fmt.Errorf("select %q: %w", mailboxName, err) } @@ -67,6 +73,9 @@ 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 + } dst := c.qualifyMailboxLocked(dstFolder) if err := c.ensureMailboxExists(dst); err != nil { return err @@ -107,6 +116,9 @@ 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 + } return c.moveUIDLocked(src, dst, uid) } From b9fa144d1cf6d34b57b8a535abece04a68bc4101 Mon Sep 17 00:00:00 2001 From: SUMAN JANA Date: Mon, 7 Sep 2026 07:15:20 +0000 Subject: [PATCH 2/3] fix(worker): guard the IMAP client across reconnects; Gmail-only name fallback Address review: - The client field was read by commands while ensureConnected could swap it. A lifecycle RWMutex now holds the write lock through dial, auth and assignment, and every command holds the read lock for its duration. Lock order is mu before lifecycle; sentMailbox resolves under mu before AppendToSent takes the read lock. - The virtual-folder name fallback applied to any server, so a plain IMAP account with a real folder called "Important" or "Starred" would have been dropped from sync. It now applies only inside Gmail's own "[Gmail]/" and "[Google Mail]/" namespace; regression cases added. --- internal/app/worker/wmail/folder_test.go | 8 +++- internal/app/worker/wmail/sync_imap.go | 8 ++-- internal/client/smtpimap/imap/append_sent.go | 6 +++ internal/client/smtpimap/imap/client.go | 48 +++++++++++++++---- .../client/smtpimap/imap/warmup_actions.go | 8 ++++ 5 files changed, 65 insertions(+), 13 deletions(-) diff --git a/internal/app/worker/wmail/folder_test.go b/internal/app/worker/wmail/folder_test.go index a6db2ef0..7c3ad2a9 100644 --- a/internal/app/worker/wmail/folder_test.go +++ b/internal/app/worker/wmail/folder_test.go @@ -53,9 +53,15 @@ func TestImapVirtualFolder(t *testing.T) { {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. + // 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}, diff --git a/internal/app/worker/wmail/sync_imap.go b/internal/app/worker/wmail/sync_imap.go index 79b44d42..df665239 100644 --- a/internal/app/worker/wmail/sync_imap.go +++ b/internal/app/worker/wmail/sync_imap.go @@ -421,11 +421,13 @@ func imapVirtualFolder(box *models.Mailbox) bool { 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 i := strings.LastIndexAny(name, "/."); i >= 0 { - name = name[i+1:] + if !strings.HasPrefix(name, "[gmail]/") && !strings.HasPrefix(name, "[google mail]/") { + return false } - switch name { + switch name[strings.Index(name, "/")+1:] { case "all mail", "starred", "important": return true } diff --git a/internal/client/smtpimap/imap/append_sent.go b/internal/client/smtpimap/imap/append_sent.go index 9c07e2d9..73cad089 100644 --- a/internal/client/smtpimap/imap/append_sent.go +++ b/internal/client/smtpimap/imap/append_sent.go @@ -34,6 +34,8 @@ func (c *Client) AppendToSent(ctx context.Context, raw []byte, sentAt time.Time) 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 @@ -42,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}, @@ -71,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 051bd2cc..ca30de39 100644 --- a/internal/client/smtpimap/imap/client.go +++ b/internal/client/smtpimap/imap/client.go @@ -63,28 +63,39 @@ type Client struct { // route is used. BindIP *net.TCPAddr - // reconnectMu serializes ensureConnected between the sync loop (which does - // not hold mu) and the send/warmup paths (which do), so a drop seen by both - // at once dials one new session, not two. - reconnectMu sync.Mutex + // 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. +// 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.reconnectMu.Lock() - defer c.reconnectMu.Unlock() + c.lifecycle.Lock() + defer c.lifecycle.Unlock() if c.client != nil && c.client.State() != imap.ConnStateLogout { return nil } - return c.Connect() + 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 { @@ -147,6 +158,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() } @@ -190,6 +206,8 @@ func (c *Client) Folders() ([]models.Mailbox, *errx.MailError) { 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. @@ -242,6 +260,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 } @@ -252,7 +272,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) @@ -265,6 +285,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) @@ -282,6 +304,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 @@ -317,6 +341,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) @@ -335,6 +361,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, @@ -417,6 +445,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/warmup_actions.go b/internal/client/smtpimap/imap/warmup_actions.go index 8053e146..83b1598f 100644 --- a/internal/client/smtpimap/imap/warmup_actions.go +++ b/internal/client/smtpimap/imap/warmup_actions.go @@ -18,6 +18,8 @@ func (c *Client) MarkAsRead(ctx context.Context, mailboxName string, uid uint32) 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) } @@ -42,6 +44,8 @@ func (c *Client) MarkImportant(ctx context.Context, mailboxName string, uid uint 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) } @@ -76,6 +80,8 @@ func (c *Client) MoveToFolder(ctx context.Context, sourceMailbox, dstFolder stri 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 @@ -119,6 +125,8 @@ func (c *Client) moveUID(ctx context.Context, src, dst string, uid uint32) error if merr := c.ensureConnected(); merr != nil { return merr } + c.lifecycle.RLock() + defer c.lifecycle.RUnlock() return c.moveUIDLocked(src, dst, uid) } From 61348a55f73b7a8916cba45ba4a2e4b6df1b2075 Mon Sep 17 00:00:00 2001 From: SUMAN JANA Date: Mon, 7 Sep 2026 07:25:21 +0000 Subject: [PATCH 3/3] fix(worker): reuse only an authenticated IMAP session; drain LIST on the folder cap Address review: - ensureConnected kept any client not in Logout, so a session whose Login failed (NotAuthenticated) was reused instead of re-dialed. Reuse only Authenticated or Selected, and close the half-open session when auth or the CONDSTORE check fails. - Folders returned from inside the LIST loop on the folder cap without closing the command, leaving unread results to stall the next command. --- internal/client/smtpimap/imap/client.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/client/smtpimap/imap/client.go b/internal/client/smtpimap/imap/client.go index ca30de39..dd0ebaac 100644 --- a/internal/client/smtpimap/imap/client.go +++ b/internal/client/smtpimap/imap/client.go @@ -81,8 +81,13 @@ func (c *Client) ensureConnected() *errx.MailError { c.lifecycle.Lock() defer c.lifecycle.Unlock() - if c.client != nil && c.client.State() != imap.ConnStateLogout { - return nil + // 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() } @@ -144,6 +149,9 @@ func (c *Client) connectLocked() *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 } @@ -151,6 +159,7 @@ func (c *Client) connectLocked() *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 } @@ -231,6 +240,9 @@ func (c *Client) Folders() ([]models.Mailbox, *errx.MailError) { 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 }