Merge pull request #342 from rocker1166/fix/imap-dead-session-reconnect

fix(worker): reconnect dropped IMAP sessions and sync nested Gmail folders

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) {

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

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.

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)
 }
 

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
 }

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")
+	}
+}

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)
 }