From b9fa144d1cf6d34b57b8a535abece04a68bc4101 Mon Sep 17 00:00:00 2001 From: SUMAN JANA Date: Mon, 7 Sep 2026 07:15:20 +0000 Subject: [PATCH] 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) }