diff --git a/cmd/seed/main.go b/cmd/seed/main.go index 6368e924..15731fd3 100644 --- a/cmd/seed/main.go +++ b/cmd/seed/main.go @@ -498,8 +498,8 @@ func seedLegacyUnibox(ctx context.Context, pool *pgxpool.Pool, rows []legacyUnib if _, err := pool.Exec(ctx, ` INSERT INTO unibox_mailboxes (email_id, uid_validity, mailbox, attributes, highestmodseq, updated_at) VALUES ($1, $2, 'INBOX', ARRAY['\HasNoChildren'], 1, NOW()) - ON CONFLICT (email_id, uid_validity) DO UPDATE SET - mailbox = EXCLUDED.mailbox, + ON CONFLICT (email_id, mailbox) DO UPDATE SET + uid_validity = EXCLUDED.uid_validity, attributes = EXCLUDED.attributes, highestmodseq = EXCLUDED.highestmodseq, updated_at = NOW() diff --git a/docs/content/docs/development/troubleshooting.mdx b/docs/content/docs/development/troubleshooting.mdx index b5419ea4..b7780c83 100644 --- a/docs/content/docs/development/troubleshooting.mdx +++ b/docs/content/docs/development/troubleshooting.mdx @@ -83,7 +83,7 @@ Newer builds return the invite-only refusal with its own machine code, `registra | 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, 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 | +| 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 lists the same folder name twice, which the Sync card says as well. A folder sharing its `UIDVALIDITY` with another folder is no longer a reason to skip anything: folders are identified by name, which is what IMAP guarantees is unique. 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/internal/app/consumer/event_mailbox_delete.go b/internal/app/consumer/event_mailbox_delete.go index edf63f96..2c4d16e7 100644 --- a/internal/app/consumer/event_mailbox_delete.go +++ b/internal/app/consumer/event_mailbox_delete.go @@ -6,13 +6,20 @@ import ( "github.com/warmbly/warmbly/internal/models" ) +// HandleMailboxDelete retires a folder the last listing no longer had. +// +// A folder is identified by its name. UIDValidity is the fallback for an +// event from a worker deployed before that was true, which names no folder at +// all; deleting by it after the change would be wrong, because the number is +// not unique across folders. func (s *JobsService) HandleMailboxDelete(ctx context.Context, e *models.JobEventMailboxDelete) error { - if err := s.MailboxRepository.DeleteMailbox( - ctx, - e.UserID, - e.EmailID, - e.UIDValidity, - ); err != nil { + var err error + if e.Mailbox != "" { + err = s.MailboxRepository.DeleteMailbox(ctx, e.UserID, e.EmailID, e.Mailbox) + } else { + err = s.MailboxRepository.DeleteMailboxByUIDValidity(ctx, e.UserID, e.EmailID, e.UIDValidity) + } + if err != nil { CaptureError(e.UserID, e.EmailID, err) return err } diff --git a/internal/app/consumer/event_mailbox_rename.go b/internal/app/consumer/event_mailbox_rename.go new file mode 100644 index 00000000..a81bd85b --- /dev/null +++ b/internal/app/consumer/event_mailbox_rename.go @@ -0,0 +1,34 @@ +package jobs + +import ( + "context" + + "github.com/warmbly/warmbly/internal/models" +) + +// HandleMailboxRename follows a folder the server renamed. +// +// A folder is keyed by name, so a rename would otherwise read as one folder +// disappearing and another appearing, which orphans every message filed under +// the old name and re-imports the folder's history under the new one. An IMAP +// RENAME keeps UIDVALIDITY and every UID, so there is nothing to re-import: +// the row and the mail both just move. +// +// The row moves first. If the mail move then fails the event is retried, and +// the second pass finds no row to rename (which is a no-op) and re-runs the +// move, so a partial apply repairs itself rather than leaving the folder +// renamed with its mail behind. +func (s *JobsService) HandleMailboxRename(ctx context.Context, e *models.JobEventMailboxRename) error { + if e.From == "" || e.To == "" || e.From == e.To { + return nil + } + if err := s.MailboxRepository.RenameMailbox(ctx, e.UserID, e.EmailID, e.From, e.To); err != nil { + CaptureError(e.UserID, e.EmailID, err) + return err + } + if err := s.UniboxRepository.MoveFolderPath(ctx, e.EmailID, e.From, e.To); err != nil { + CaptureError(e.UserID, e.EmailID, err) + return err + } + return nil +} diff --git a/internal/app/consumer/event_new_email.go b/internal/app/consumer/event_new_email.go index 5bb8e987..e499f7c4 100644 --- a/internal/app/consumer/event_new_email.go +++ b/internal/app/consumer/event_new_email.go @@ -284,6 +284,7 @@ func (s *JobsService) performWarmupActions(ctx context.Context, e *models.JobEve GmailID: e.Message.GmailID, UID: e.Message.UID, MailboxUIDValidity: e.Message.Mailbox, + MailboxFolder: e.Message.FolderPath, // Stable key so Graph accounts re-resolve the live message id at action // time (Graph ids change on move). RFCMessageID: e.Message.MessageID, diff --git a/internal/app/consumer/event_update_email.go b/internal/app/consumer/event_update_email.go index 2d41dd7b..99255c3f 100644 --- a/internal/app/consumer/event_update_email.go +++ b/internal/app/consumer/event_update_email.go @@ -30,6 +30,11 @@ func (s *JobsService) HandleUpdateEmail(ctx context.Context, e *models.JobEventE if email.ModSeq != e.ModSeq { updateData.ModSeq = &e.ModSeq } + // The source folder's name, which is its identity. Empty on events from + // workers predating the field, which keeps the stored value. + if e.FolderPath != "" && email.FolderPath != e.FolderPath { + updateData.FolderPath = &e.FolderPath + } // A folder move follows the provider. Events from workers predating the // field carry "", which keeps the stored value. if models.ValidFolder(e.Folder) && email.Folder != e.Folder { diff --git a/internal/app/consumer/events.go b/internal/app/consumer/events.go index 03afab6f..bd5afd95 100644 --- a/internal/app/consumer/events.go +++ b/internal/app/consumer/events.go @@ -35,6 +35,7 @@ func (w *JobsService) InitEvents() { Register(w, models.JobEventTypeFlagsRemove, w.HandleFlagsRemove) Register(w, models.JobEventTypeMailboxUpdate, w.HandleMailboxUpdate) Register(w, models.JobEventTypeMailboxDelete, w.HandleMailboxDelete) + Register(w, models.JobEventTypeMailboxRename, w.HandleMailboxRename) Register(w, models.JobEventTypeHistoryIDUpdate, w.HandleHistoryIDUpdate) Register(w, models.JobEventTypeGraphDeltaUpdate, w.HandleGraphDeltaUpdate) Register(w, models.JobEventTypeSyncState, w.HandleSyncState) diff --git a/internal/app/worker/event_warmup_action.go b/internal/app/worker/event_warmup_action.go index 1f8899f7..95c66e5e 100644 --- a/internal/app/worker/event_warmup_action.go +++ b/internal/app/worker/event_warmup_action.go @@ -139,12 +139,13 @@ func (w *WorkerService) runGraphWarmupActions(ctx context.Context, mail *wmail.W } func (w *WorkerService) runImapWarmupActions(ctx context.Context, mail *wmail.WMail, action models.WarmupEmailAction) { - sourceBox := lookupMailboxByUIDValidity(mail.SmtpImapData.Mailboxes, action.MailboxUIDValidity) + sourceBox := lookupWarmupSourceFolder(mail.SmtpImapData.Mailboxes, action) if sourceBox == nil { log.Warn(). + Str("folder", action.MailboxFolder). Uint32("uid_validity", action.MailboxUIDValidity). Str("email_id", action.EmailID.String()). - Msg("Source mailbox for warmup action not found; skipping") + Msg("Source mailbox for warmup action not found or its UIDs have been reissued; skipping") return } @@ -189,6 +190,28 @@ func (w *WorkerService) runImapWarmupActions(ctx context.Context, mail *wmail.WM } } +// lookupWarmupSourceFolder resolves the folder an action's UID lives in. +// +// The folder is found by name, its identity. The UIDVALIDITY still has to +// match: it is the generation the stored UID belongs to, and a server that +// reissued it has given that number to some other message, so acting on it +// would star or file a message nobody asked about. Nothing to act on is the +// right answer there. +// +// An action published before the folder name was carried has only the +// UIDVALIDITY to go on, which is the old behaviour and stays as the fallback. +func lookupWarmupSourceFolder(boxes []*models.Mailbox, action models.WarmupEmailAction) *models.Mailbox { + if action.MailboxFolder == "" { + return lookupMailboxByUIDValidity(boxes, action.MailboxUIDValidity) + } + for _, b := range boxes { + if b != nil && b.Name == action.MailboxFolder && b.UIDValidity == action.MailboxUIDValidity { + return b + } + } + return nil +} + func lookupMailboxByUIDValidity(boxes []*models.Mailbox, uidValidity uint32) *models.Mailbox { for _, b := range boxes { if b != nil && b.UIDValidity == uidValidity { diff --git a/internal/app/worker/warmup_source_folder_test.go b/internal/app/worker/warmup_source_folder_test.go new file mode 100644 index 00000000..ae69b2c5 --- /dev/null +++ b/internal/app/worker/warmup_source_folder_test.go @@ -0,0 +1,63 @@ +package worker + +import ( + "testing" + + "github.com/warmbly/warmbly/internal/models" +) + +// A warmup action addresses a message by UID, and a UID only means something +// inside one folder and one generation of that folder's UIDs. Resolving it by +// UIDVALIDITY alone was both: wrong on a server that gives several folders the +// same number, where the action landed in whichever folder matched first, and +// unable to tell a reissued number from the original. +func TestLookupWarmupSourceFolder(t *testing.T) { + boxes := []*models.Mailbox{ + {Name: "INBOX", UIDValidity: 7}, + // A folder tree created in the same second on a server that stamps + // UIDVALIDITY with the creation time. + {Name: "Clients/Acme", UIDValidity: 42}, + {Name: "Clients/Globex", UIDValidity: 42}, + } + + for _, tc := range []struct { + name string + action models.WarmupEmailAction + want string + }{ + { + "the named folder wins over another with the same UIDVALIDITY", + models.WarmupEmailAction{MailboxFolder: "Clients/Globex", MailboxUIDValidity: 42}, + "Clients/Globex", + }, + { + "a folder whose UIDs were reissued is not acted on", + models.WarmupEmailAction{MailboxFolder: "INBOX", MailboxUIDValidity: 6}, + "", + }, + { + "a folder that is gone is not acted on", + models.WarmupEmailAction{MailboxFolder: "Clients/Initech", MailboxUIDValidity: 42}, + "", + }, + // An action published before the folder name was carried has only the + // number, which is the old behaviour and stays the fallback. + { + "no name falls back to the UIDVALIDITY", + models.WarmupEmailAction{MailboxUIDValidity: 7}, + "INBOX", + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := lookupWarmupSourceFolder(boxes, tc.action) + switch { + case tc.want == "" && got != nil: + t.Fatalf("resolved %q, want nothing to act on", got.Name) + case tc.want != "" && got == nil: + t.Fatalf("resolved nothing, want %q", tc.want) + case tc.want != "" && got.Name != tc.want: + t.Fatalf("resolved %q, want %q", got.Name, tc.want) + } + }) + } +} diff --git a/internal/app/worker/wmail/folder_identity_test.go b/internal/app/worker/wmail/folder_identity_test.go new file mode 100644 index 00000000..eaf48f6b --- /dev/null +++ b/internal/app/worker/wmail/folder_identity_test.go @@ -0,0 +1,202 @@ +package wmail + +import ( + "testing" + + "github.com/warmbly/warmbly/internal/models" +) + +// mailboxUpdates is every folder MAILBOX_UPDATE relayed, by name. +func mailboxUpdates(events []captured) map[string]*models.Mailbox { + out := map[string]*models.Mailbox{} + for _, e := range events { + if e.eventType != models.JobEventTypeMailboxUpdate { + continue + } + box := e.body.(*models.JobEventMailboxUpdate).Data + out[box.Name] = box + } + return out +} + +func mailboxDeletes(events []captured) []string { + var out []string + for _, e := range events { + if e.eventType == models.JobEventTypeMailboxDelete { + out = append(out, e.body.(*models.JobEventMailboxDelete).Mailbox) + } + } + return out +} + +// The bug this file exists for: a server that derives UIDVALIDITY from a +// folder's creation time gives every folder made in the same second the same +// number, and a folder tree made by a mail client, an import or a migration +// is made in one second by definition. Keyed on that number, all but one of +// them were dropped and never synced. Keyed on the name, which is what IMAP +// actually guarantees, every one of them is followed. +func TestSyncFollowsEveryFolderSharingAUIDValidity(t *testing.T) { + conn := &fakeImapConn{folders: []models.Mailbox{ + {Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}, + {Name: "Clients", UIDValidity: 42, HighestModSeq: 100}, + {Name: "Clients/Acme", UIDValidity: 42, HighestModSeq: 100}, + {Name: "Clients/Globex", UIDValidity: 42, HighestModSeq: 100}, + }} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, + &models.Mailbox{Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}) + + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("Sync: %v", err) + } + + updates := mailboxUpdates(*events) + for _, name := range []string{"Clients", "Clients/Acme", "Clients/Globex"} { + if updates[name] == nil { + t.Errorf("%q was never baselined; a shared UIDVALIDITY cost it its whole sync", name) + } + } + if got := mailboxDeletes(*events); len(got) != 0 { + t.Errorf("retired %v; nothing left the listing", got) + } + if len(w.SmtpImapData.Mailboxes) != 4 { + t.Fatalf("tracked %d folders, want 4", len(w.SmtpImapData.Mailboxes)) + } +} + +// A stored message carries both: the folder's name, which is its identity and +// survives a UIDVALIDITY change, and the UIDVALIDITY itself, which is the +// generation its UID belongs to. +func TestSyncStampsTheFolderNameOnStoredMail(t *testing.T) { + conn := &fakeImapConn{ + folders: []models.Mailbox{{Name: "Clients/Acme", UIDValidity: 42, HighestModSeq: 200}}, + changed: uidRange(1), + } + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, + &models.Mailbox{Name: "Clients/Acme", UIDValidity: 42, HighestModSeq: 100}) + + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("Sync: %v", err) + } + + var stored *models.EmailMessageStoreData + for _, e := range *events { + if e.eventType == models.JobEventTypeNewEmail { + stored = e.body.(*models.JobEventNewEmail).Message + } + } + if stored == nil { + t.Fatal("no message was stored") + } + if stored.FolderPath != "Clients/Acme" { + t.Errorf("FolderPath = %q, want the folder's name", stored.FolderPath) + } + if stored.Mailbox != 42 { + t.Errorf("Mailbox = %d, want the UIDVALIDITY the uid belongs to", stored.Mailbox) + } +} + +// An IMAP RENAME keeps UIDVALIDITY and every UID, so a rename is a move, not +// a folder leaving and another arriving. Read the other way it would orphan +// the mail filed under the old name and re-import the folder's history. +func TestSyncFollowsAFolderRename(t *testing.T) { + conn := &fakeImapConn{folders: []models.Mailbox{ + {Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}, + {Name: "Clients/Acme Corp", UIDValidity: 42, HighestModSeq: 100}, + }} + 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: "Clients/Acme", UIDValidity: 42, HighestModSeq: 100}) + w.tracker.setFolder("Clients/Acme", models.SyncFolderCursor{UID: 900}) + + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("Sync: %v", err) + } + + var renamed *models.JobEventMailboxRename + for _, e := range *events { + if e.eventType == models.JobEventTypeMailboxRename { + renamed = e.body.(*models.JobEventMailboxRename) + } + } + if renamed == nil { + t.Fatal("no MAILBOX_RENAME was relayed; the folder's mail would be orphaned") + } + if renamed.From != "Clients/Acme" || renamed.To != "Clients/Acme Corp" { + t.Errorf("renamed %q -> %q, want Clients/Acme -> Clients/Acme Corp", renamed.From, renamed.To) + } + if got := mailboxDeletes(*events); len(got) != 0 { + t.Errorf("retired %v; a rename must not delete the folder", got) + } + if cur := w.tracker.folder("Clients/Acme Corp"); cur.UID != 900 { + t.Errorf("backfill floor under the new name = %d, want 900 to move with it", cur.UID) + } + if cur := w.tracker.folder("Clients/Acme"); cur.UID != 0 { + t.Error("the backfill floor was left behind under the old name") + } + if len(w.SmtpImapData.Mailboxes) != 2 { + t.Fatalf("tracked %d folders, want 2", len(w.SmtpImapData.Mailboxes)) + } +} + +// On a server that stamps UIDVALIDITY from a creation time, several folders +// share a number, so "the folder carrying this UIDVALIDITY" can name more +// than one candidate. Guessing there would move a folder's mail into an +// unrelated folder, so an ambiguous match is not a rename at all. +func TestSyncDoesNotGuessARenameWhenTwoFoldersCouldBeIt(t *testing.T) { + conn := &fakeImapConn{folders: []models.Mailbox{ + {Name: "Clients/Acme Corp", UIDValidity: 42, HighestModSeq: 100}, + {Name: "Clients/Globex", UIDValidity: 42, HighestModSeq: 100}, + }} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, + &models.Mailbox{Name: "Clients/Acme", UIDValidity: 42, HighestModSeq: 100}) + + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("Sync: %v", err) + } + + if hasEvent(*events, models.JobEventTypeMailboxRename) { + t.Fatal("a rename was guessed from an ambiguous UIDVALIDITY") + } + if got := mailboxDeletes(*events); len(got) != 1 || got[0] != "Clients/Acme" { + t.Errorf("retired %v, want the folder that left the listing", got) + } +} + +// A changed UIDVALIDITY is the server saying every UID we hold for the folder +// is void. The folder is re-baselined rather than followed from a cursor that +// now addresses nothing, and an unfinished import walks it again. +func TestSyncRebaselinesAFolderWhoseUIDValidityChanged(t *testing.T) { + conn := &fakeImapConn{ + folders: []models.Mailbox{{Name: "INBOX", UIDValidity: 900, HighestModSeq: 5}}, + changed: uidRange(3), + } + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, + &models.Mailbox{Name: "INBOX", UIDValidity: 7, HighestModSeq: 100}) + w.tracker.setFolder("INBOX", models.SyncFolderCursor{UID: 500, Done: true}) + w.flagScan = map[string]*folderFlagScan{"INBOX": {}} + + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("Sync: %v", err) + } + + box := mailboxUpdates(*events)["INBOX"] + if box == nil { + t.Fatal("the re-baselined folder was never relayed") + } + if box.UIDValidity != 900 || box.HighestModSeq != 5 { + t.Errorf("relayed %+v, want the server's current cursor", box) + } + if hasEvent(*events, models.JobEventTypeNewEmail) { + t.Error("the folder was walked from a cursor its server had already voided") + } + if cur := w.tracker.folder("INBOX"); cur.UID != 0 || cur.Done { + t.Errorf("backfill floor = %+v, want it cleared so the folder is walked again", cur) + } + if _, held := w.flagScan["INBOX"]; held { + t.Error("the flag snapshot survived; it describes UIDs that no longer mean anything") + } + if got := mailboxDeletes(*events); len(got) != 0 { + t.Errorf("retired %v; the folder is still there", got) + } +} diff --git a/internal/app/worker/wmail/folder_test.go b/internal/app/worker/wmail/folder_test.go index 7c3ad2a9..18ff8a4b 100644 --- a/internal/app/worker/wmail/folder_test.go +++ b/internal/app/worker/wmail/folder_test.go @@ -95,7 +95,7 @@ func TestImapSyncSkipsVirtualFolders(t *testing.T) { t.Fatal("All Mail was baselined; virtual folders must be skipped") } case models.JobEventTypeMailboxDelete: - if e.body.(*models.JobEventMailboxDelete).UIDValidity == 11 { + if e.body.(*models.JobEventMailboxDelete).Mailbox == "[Gmail]/Starred" { retired = true } } diff --git a/internal/app/worker/wmail/sync_imap.go b/internal/app/worker/wmail/sync_imap.go index 10d76e5a..05597459 100644 --- a/internal/app/worker/wmail/sync_imap.go +++ b/internal/app/worker/wmail/sync_imap.go @@ -4,7 +4,6 @@ import ( "context" "slices" "sort" - "strconv" "time" goimap "github.com/emersion/go-imap/v2" @@ -45,6 +44,14 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { // that reached it would re-file known mail as archive under a second UID. folders = slices.DeleteFunc(folders, func(b models.Mailbox) bool { return imapVirtualFolder(&b) }) + // Before anything is matched by name, follow the folders whose name + // changed. A rename read as a delete plus a first sighting would orphan + // every message filed under the old name and re-import the folder's + // history under the new one. + if err := w.imapFollowRenames(folders); err != nil { + return nil + } + // condStore decides the incremental strategy for the whole account: // mod-sequences where the server has CONDSTORE, UIDNEXT where it does not // (Outlook.com, Microsoft 365 over IMAP, Yahoo, many hosted servers). @@ -64,11 +71,29 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { continue } + // A folder whose UIDVALIDITY moved is the server telling us every UID + // we hold for it is void: the cursors address nothing and the flag + // snapshot is about messages that may no longer be there. Re-baseline + // it exactly like a first sighting, and drop its backfill floor so an + // import still running walks it again (stored messages are matched by + // Message-ID, so nothing is stored twice). A finished import stays + // finished; the mail that is already here keeps its rows, and its + // stale UIDs are what the warmup path checks the generation against. + if befBox.UIDValidity != box.UIDValidity { + saved := *box + if err := w.mboxEvent(&saved); err != nil { + return nil + } + *befBox = saved + delete(w.flagScan, box.Name) + w.tracker.setFolder(box.Name, models.SyncFolderCursor{}) + continue + } + changed := imapFolderChanged(befBox, box, condStore) fullyProcessed := true if changed && !stats.aborted { - w.SmtpImapData.mailbox = box.UIDValidity - w.SmtpImapData.folder = imapCanonicalFolder(box) + w.setWalking(box) done, err := w.imapIncremental(ctx, box, befBox, condStore, stats) if err != nil { return err @@ -79,7 +104,7 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { fullyProcessed = false } - if changed || befBox.Name != box.Name || !slices.Equal(befBox.Attrs, box.Attrs) { + if changed || !slices.Equal(befBox.Attrs, box.Attrs) { // The stored cursor only moves once every change up to it was // stored; a deferred message keeps the folder re-asked. next := *box @@ -90,22 +115,18 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { if err := w.mboxEvent(&next); err != nil { return nil } - for _, ibox := range w.SmtpImapData.Mailboxes { - if ibox.UIDValidity == box.UIDValidity { - ibox.HighestModSeq = next.HighestModSeq - ibox.UIDNext = next.UIDNext - ibox.Name = next.Name - ibox.Attrs = next.Attrs - } - } + // befBox is the stored copy itself, matched by name, so there is + // nothing else in the list to keep in step with it. + befBox.HighestModSeq = next.HighestModSeq + befBox.UIDNext = next.UIDNext + befBox.Attrs = next.Attrs } // Without CONDSTORE a message marked read elsewhere moves no cursor, // so read state is mirrored by a periodic scan instead. It runs after // the arrivals above so a message stored this pass is already known. if !condStore && !stats.aborted { - w.SmtpImapData.mailbox = box.UIDValidity - w.SmtpImapData.folder = imapCanonicalFolder(box) + w.setWalking(box) if _, err := w.SmtpImapData.ImapClient.SelectForSync(box.Name); err != nil { return err } @@ -115,12 +136,14 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { } } - // Collect deletions first to avoid modifying the slice during iteration - var deleted []uint32 + // Collect deletions first to avoid modifying the slice during iteration. + // Renames were already followed above, so a name missing from the listing + // at this point really is a folder that is gone. + var deleted []string outer: for _, box := range w.SmtpImapData.Mailboxes { for _, f := range folders { - if box.UIDValidity == f.UIDValidity { + if box.Name == f.Name { continue outer } } @@ -128,20 +151,21 @@ outer: if err := w.onEvent(models.JobEventTypeMailboxDelete, &models.JobEventMailboxDelete{ UserID: w.UserID, EmailID: w.ID, + Mailbox: box.Name, UIDValidity: box.UIDValidity, }); err != nil { return nil } - deleted = append(deleted, box.UIDValidity) + deleted = append(deleted, box.Name) } if len(deleted) > 0 { - for _, uidv := range deleted { - delete(w.flagScan, uidv) + for _, name := range deleted { + delete(w.flagScan, name) } filtered := w.SmtpImapData.Mailboxes[:0] for _, b := range w.SmtpImapData.Mailboxes { - if !slices.Contains(deleted, b.UIDValidity) { + if !slices.Contains(deleted, b.Name) { filtered = append(filtered, b) } } @@ -244,14 +268,15 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill continue } if err := w.onEvent(models.JobEventTypeEmailUpdate, &models.JobEventEmailUpdate{ - UserID: w.UserID, - EmailID: w.ID, - ID: internalID, - UID: f.Email.UID, - ModSeq: f.Email.ModSeq, - Mailbox: w.SmtpImapData.mailbox, - Folder: w.SmtpImapData.folder, - Flags: f.Email.Flags, + UserID: w.UserID, + EmailID: w.ID, + ID: internalID, + UID: f.Email.UID, + ModSeq: f.Email.ModSeq, + Mailbox: w.SmtpImapData.mailbox, + FolderPath: w.SmtpImapData.folderPath, + Folder: w.SmtpImapData.folder, + Flags: f.Email.Flags, }); err != nil { return false, w.controlPlaneError(err, stats) } @@ -291,7 +316,7 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill if backfill { w.tracker.state.BackfillSynced++ w.tracker.mark() - w.tracker.setFolder(strconv.FormatUint(uint64(w.SmtpImapData.mailbox), 10), models.SyncFolderCursor{UID: f.Email.UID}) + w.tracker.setFolder(w.SmtpImapData.folderPath, models.SyncFolderCursor{UID: f.Email.UID}) } } return all, nil @@ -332,6 +357,7 @@ func (w *WMail) imapStore(ctx context.Context, msg *models.EmailMessageData) err ID: msg.ID, EmailID: w.ID, Mailbox: w.SmtpImapData.mailbox, + FolderPath: w.SmtpImapData.folderPath, Folder: w.SmtpImapData.folder, ThreadID: threadID, MessageID: msg.MessageID, @@ -382,7 +408,7 @@ func (w *WMail) imapBackfill(ctx context.Context, folders []models.Mailbox, stat if stats.aborted || stats.laneDenied(LaneBackfill) { return nil } - key := strconv.FormatUint(uint64(box.UIDValidity), 10) + key := box.Name cur := w.tracker.folder(key) if cur.Done { continue @@ -391,8 +417,7 @@ func (w *WMail) imapBackfill(ctx context.Context, folders []models.Mailbox, stat w.tracker.completeBackfill(time.Now()) return nil } - w.SmtpImapData.mailbox = box.UIDValidity - w.SmtpImapData.folder = imapCanonicalFolder(box) + w.setWalking(box) count, err := client.SelectForSync(box.Name) if err != nil { @@ -484,11 +509,84 @@ func (w *WMail) mboxEvent(box *models.Mailbox) error { }) } +// FindPair is the stored copy of a listed folder, matched on the folder's +// identity: its name. func (w *SmtpImapData) FindPair(m *models.Mailbox) *models.Mailbox { for _, f := range w.Mailboxes { - if f.UIDValidity == m.UIDValidity { + if f.Name == m.Name { return f } } return nil } + +// setWalking records which folder the pass is inside. Every message stored or +// updated from here is stamped with all three: the folder's name, which is +// its identity, the UIDVALIDITY generation its uid belongs to, and the +// canonical folder the dashboard files it under. +func (w *WMail) setWalking(box *models.Mailbox) { + w.SmtpImapData.mailbox = box.UIDValidity + w.SmtpImapData.folderPath = box.Name + w.SmtpImapData.folder = imapCanonicalFolder(box) +} + +// imapFollowRenames matches a folder that left the listing to one that +// arrived carrying its UIDVALIDITY, and relays the pair as a rename. +// +// That is what an IMAP RENAME looks like from a LIST: RENAME keeps +// UIDVALIDITY and every UID, so the cursor we hold is still good and the +// folder's history does not need re-importing. Read as a delete plus a first +// sighting it would be both, and the mail filed under the old name would be +// left pointing at a folder that no longer exists. +// +// A candidate already known by name is not the target (it is a folder of its +// own), and two candidates mean the guess is not safe to make: on a server +// that stamps UIDVALIDITY from a creation time a whole tree shares one +// number, so an ambiguous match falls back to the delete-and-create path +// rather than moving mail into the wrong folder. +func (w *WMail) imapFollowRenames(folders []models.Mailbox) error { + listed := make(map[string]struct{}, len(folders)) + for i := range folders { + listed[folders[i].Name] = struct{}{} + } + + for _, before := range w.SmtpImapData.Mailboxes { + if _, still := listed[before.Name]; still || before.UIDValidity == 0 { + continue + } + var to *models.Mailbox + for i := range folders { + f := &folders[i] + if f.UIDValidity != before.UIDValidity || w.SmtpImapData.FindPair(f) != nil { + continue + } + if to != nil { + to = nil + break + } + to = f + } + if to == nil { + continue + } + + if err := w.onEvent(models.JobEventTypeMailboxRename, &models.JobEventMailboxRename{ + UserID: w.UserID, + EmailID: w.ID, + From: before.Name, + To: to.Name, + }); err != nil { + return err + } + // The worker's own per-folder state is keyed by name too, so it moves + // with the folder or the backfill restarts and the flag scan + // re-baselines for a change of label. + if scan, ok := w.flagScan[before.Name]; ok { + delete(w.flagScan, before.Name) + w.flagScan[to.Name] = scan + } + w.tracker.renameFolder(before.Name, to.Name) + before.Name = to.Name + } + return nil +} diff --git a/internal/app/worker/wmail/sync_imap_flags.go b/internal/app/worker/wmail/sync_imap_flags.go index 7d760147..253645b1 100644 --- a/internal/app/worker/wmail/sync_imap_flags.go +++ b/internal/app/worker/wmail/sync_imap_flags.go @@ -16,8 +16,8 @@ import ( // 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 +// Both conditions are things the user can fix (get under the folder cap, or +// stop the server listing one name twice), 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. @@ -37,9 +37,9 @@ func (w *WMail) reportFolderOverflow() { // not wait for it; that arrives through UIDNEXT on every pass. func (w *WMail) imapScanFlags(ctx context.Context, box *models.Mailbox, stats *tickStats) *errx.MailError { if w.flagScan == nil { - w.flagScan = map[uint32]*folderFlagScan{} + w.flagScan = map[string]*folderFlagScan{} } - scan := w.flagScan[box.UIDValidity] + scan := w.flagScan[box.Name] now := time.Now() if scan != nil && now.Sub(scan.at) < config.ImapFlagScanInterval { return nil @@ -77,7 +77,7 @@ func (w *WMail) imapScanFlags(ctx context.Context, box *models.Mailbox, stats *t } } } - w.flagScan[box.UIDValidity] = &folderFlagScan{at: now, flags: next} + w.flagScan[box.Name] = &folderFlagScan{at: now, flags: next} return nil } @@ -100,13 +100,14 @@ func (w *WMail) relayFlags(ctx context.Context, box *models.Mailbox, uid uint32, return nil } if err := w.onEvent(models.JobEventTypeEmailUpdate, &models.JobEventEmailUpdate{ - UserID: w.UserID, - EmailID: w.ID, - ID: internalID, - UID: uid, - Mailbox: box.UIDValidity, - Folder: imapCanonicalFolder(box), - Flags: state.Flags, + UserID: w.UserID, + EmailID: w.ID, + ID: internalID, + UID: uid, + Mailbox: box.UIDValidity, + FolderPath: box.Name, + Folder: imapCanonicalFolder(box), + Flags: state.Flags, }); err != nil { return w.controlPlaneError(err, stats) } diff --git a/internal/app/worker/wmail/sync_imap_test.go b/internal/app/worker/wmail/sync_imap_test.go index 4ac37385..90ce163e 100644 --- a/internal/app/worker/wmail/sync_imap_test.go +++ b/internal/app/worker/wmail/sync_imap_test.go @@ -359,7 +359,7 @@ func TestImapBackfillRetriesAFolderAfterATransientFailure(t *testing.T) { if err := w.Sync(t.Context()); err == nil { t.Fatal("a failed folder search was swallowed; the pass must end so the folder is retried") } - if w.tracker.folder("8").Done { + if w.tracker.folder("Archive").Done { t.Fatal("the archive backfill was marked complete by a transient failure") } if st := w.tracker.state.BackfillStatus; st == models.SyncBackfillComplete { @@ -369,7 +369,7 @@ func TestImapBackfillRetriesAFolderAfterATransientFailure(t *testing.T) { if err := w.Sync(t.Context()); err != nil { t.Fatalf("second pass: %v", err.Message) } - if !w.tracker.folder("8").Done { + if !w.tracker.folder("Archive").Done { t.Error("archive is still not done after a successful search") } if err := w.Sync(t.Context()); err != nil { @@ -475,7 +475,7 @@ func TestImapFlagScanBaselinesThenRelaysChanges(t *testing.T) { // The message is marked read in the customer's own mail client. conn.flags[1] = imap.FlagState{MessageID: "", Flags: []string{"\\Seen"}} - w.flagScan[7].at = time.Now().Add(-2 * config.ImapFlagScanInterval) + w.flagScan["INBOX"].at = time.Now().Add(-2 * config.ImapFlagScanInterval) if err := w.Sync(t.Context()); err != nil { t.Fatalf("second Sync: %v", err) } diff --git a/internal/app/worker/wmail/sync_state.go b/internal/app/worker/wmail/sync_state.go index 495344bc..e81c46af 100644 --- a/internal/app/worker/wmail/sync_state.go +++ b/internal/app/worker/wmail/sync_state.go @@ -126,3 +126,15 @@ func (t *syncTracker) setFolder(key string, c models.SyncFolderCursor) { t.state.BackfillCursor.Folders[key] = c t.dirty = true } + +// renameFolder moves a folder's backfill floor to its new name, so a rename +// costs nothing rather than restarting the folder's import from the top. +func (t *syncTracker) renameFolder(from, to string) { + cur, ok := t.state.BackfillCursor.Folders[from] + if !ok { + return + } + delete(t.state.BackfillCursor.Folders, from) + t.state.BackfillCursor.Folders[to] = cur + t.dirty = true +} diff --git a/internal/app/worker/wmail/wmail.go b/internal/app/worker/wmail/wmail.go index e23e2392..c1d3c921 100644 --- a/internal/app/worker/wmail/wmail.go +++ b/internal/app/worker/wmail/wmail.go @@ -41,10 +41,15 @@ type SmtpImapData struct { ImapClient ImapConn SmtpClient *smtp.Client Mailboxes []*models.Mailbox - mailbox uint32 - // folder is the canonical folder of the mailbox currently being walked, - // set alongside mailbox and stamped on every stored/updated message. - folder string + // mailbox is the UIDVALIDITY of the folder currently being walked: the + // generation the UIDs stamped on stored messages belong to, not the + // folder's identity. + mailbox uint32 + // folderPath is that folder's name, which IS its identity, and folder the + // canonical folder it maps to. Both are set alongside mailbox and stamped + // on every stored or updated message. + folderPath string + folder string // overflowReported keeps the "more folders than we follow" warning to // one per worker session; the condition is static until the user // reorganizes their mail. @@ -90,9 +95,9 @@ type WMail struct { laneCache laneCache googleTick *tickStats graphTick *tickStats - // flagScan is the previous flag snapshot per folder, used only on IMAP - // servers without CONDSTORE, which cannot say what changed. - flagScan map[uint32]*folderFlagScan + // flagScan is the previous flag snapshot per folder name, used only on + // IMAP servers without CONDSTORE, which cannot say what changed. + flagScan map[string]*folderFlagScan // transportFailures counts consecutive passes that could not reach the // mail server, which paces the retry and keeps one outage to one warning. transportFailures int diff --git a/internal/client/smtpimap/imap/folders.go b/internal/client/smtpimap/imap/folders.go index f13f2d1b..fe4931c6 100644 --- a/internal/client/smtpimap/imap/folders.go +++ b/internal/client/smtpimap/imap/folders.go @@ -110,36 +110,38 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { resp = append(resp, box) } - resp, conflicts := dedupeByUIDValidity(resp) + resp, conflicts := dedupeByName(resp) c.folderConflicts.Store(int32(conflicts)) return resp, nil } -// dedupeByUIDValidity keeps one folder per UIDVALIDITY. +// dedupeByName keeps one folder per name. // -// Everything downstream identifies a folder by that number, including the -// primary key of the stored folder row, but RFC 3501 only promises UIDs are -// stable within one folder: Dovecot and others derive UIDVALIDITY from the -// creation time, so a folder tree created in the same second shares one. -// Two folders under a single id would advance each other's cursor and delete -// each other's row, which loses mail. Dropping the later one leaves it -// unsynced (and says so) but leaves every other folder correct. Input is -// already ranked, so the inbox and the special folders win any collision. -func dedupeByUIDValidity(boxes []models.Mailbox) ([]models.Mailbox, int) { - seen := make(map[uint32]string, len(boxes)) +// A folder is identified by its name, which is the one thing IMAP does +// guarantee is unique per account, and that is also the primary key of the +// stored folder row. Two rows under one name would advance each other's +// cursor and delete each other's row, which loses mail. +// +// This used to key on UIDVALIDITY, which cost a folder its entire sync +// whenever a server derived that number from a creation time and handed the +// same one to every folder made in the same second. Keyed by name it is a +// guard against a pathological listing rather than an everyday loss, so it +// should stay at zero; it is reported all the same, because a folder silently +// not syncing is the failure that took a release to notice. Input is already +// ranked, so the inbox and the special folders win any collision. +func dedupeByName(boxes []models.Mailbox) ([]models.Mailbox, int) { + seen := make(map[string]struct{}, len(boxes)) kept := boxes[:0] conflicts := 0 for _, box := range boxes { - if other, dup := seen[box.UIDValidity]; dup { + if _, dup := seen[box.Name]; dup { log.Warn(). Str("folder", box.Name). - Str("conflicts_with", other). - Uint32("uid_validity", box.UIDValidity). - Msg("imap: two folders report the same UIDVALIDITY; the second is not synced") + Msg("imap: the server listed one folder name twice; the second is not synced") conflicts++ continue } - seen[box.UIDValidity] = box.Name + seen[box.Name] = struct{}{} kept = append(kept, box) } return kept, conflicts @@ -152,7 +154,7 @@ func (c *Client) FolderOverflow() int { } // FolderConflicts is how many folders the last Folders call left out because -// another folder reported the same UIDVALIDITY. +// the server listed their name more than once. func (c *Client) FolderConflicts() int { return int(c.folderConflicts.Load()) } diff --git a/internal/client/smtpimap/imap/folders_test.go b/internal/client/smtpimap/imap/folders_test.go index b0a77228..dd81bed5 100644 --- a/internal/client/smtpimap/imap/folders_test.go +++ b/internal/client/smtpimap/imap/folders_test.go @@ -152,18 +152,21 @@ func TestIsVirtualFolder(t *testing.T) { } } -// Two folders sharing a UIDVALIDITY is a server doing something RFC 3501 does -// not forbid but everything downstream assumes away: the folder row is keyed -// on it. Following both would advance each other's cursor and delete each -// other's row, so one is dropped and reported. -func TestDedupeByUIDValidity(t *testing.T) { - kept, conflicts := dedupeByUIDValidity([]models.Mailbox{ +// A folder is kept once per name, which is the identity IMAP guarantees. +// Two rows under one name would advance each other's cursor and delete each +// other's row, so the second is dropped and reported. +// +// A shared UIDVALIDITY is explicitly NOT a collision any more: servers that +// derive the number from a folder's creation time give a whole tree the same +// one, and dropping those folders cost them their entire sync. +func TestDedupeByName(t *testing.T) { + kept, conflicts := dedupeByName([]models.Mailbox{ {Name: "INBOX", UIDValidity: 100}, {Name: "Sent", UIDValidity: 200}, // Created in the same second as Sent on a server that stamps - // UIDVALIDITY with the creation time. + // UIDVALIDITY with the creation time. A different folder, kept. {Name: "Projects", UIDValidity: 200}, - {Name: "Notes", UIDValidity: 300}, + {Name: "Sent", UIDValidity: 900}, }) if conflicts != 1 { t.Fatalf("conflicts = %d, want 1", conflicts) @@ -171,26 +174,27 @@ func TestDedupeByUIDValidity(t *testing.T) { if len(kept) != 3 { t.Fatalf("kept %d folders, want 3", len(kept)) } - // The ranked order puts the special folder first, so Sent is the one that - // survives and the plain user folder is the one dropped. - if kept[1].Name != "Sent" { - t.Errorf("kept[1] = %q, want Sent to win the collision", kept[1].Name) + if kept[1].Name != "Sent" || kept[1].UIDValidity != 200 { + t.Errorf("kept[1] = %+v, want the first Sent to win", kept[1]) } - seen := map[uint32]bool{} + if kept[2].Name != "Projects" { + t.Errorf("kept[2] = %q, want the folder that only shares a UIDVALIDITY to survive", kept[2].Name) + } + seen := map[string]bool{} for _, b := range kept { - if seen[b.UIDValidity] { - t.Fatalf("UIDVALIDITY %d survived twice", b.UIDValidity) + if seen[b.Name] { + t.Fatalf("folder %q survived twice", b.Name) } - seen[b.UIDValidity] = true + seen[b.Name] = true } } // The common case must not allocate a conflict or reorder anything. -func TestDedupeByUIDValidityLeavesADistinctListingAlone(t *testing.T) { +func TestDedupeByNameLeavesADistinctListingAlone(t *testing.T) { in := []models.Mailbox{{Name: "INBOX", UIDValidity: 1}, {Name: "Sent", UIDValidity: 2}} - kept, conflicts := dedupeByUIDValidity(in) + kept, conflicts := dedupeByName(in) if conflicts != 0 || len(kept) != 2 || kept[0].Name != "INBOX" || kept[1].Name != "Sent" { - t.Fatalf("a listing with distinct ids was changed: %+v, conflicts %d", kept, conflicts) + t.Fatalf("a listing with distinct names was changed: %+v, conflicts %d", kept, conflicts) } } diff --git a/internal/infrastructure/db/migrations/000134_unibox_folder_identity.down.sql b/internal/infrastructure/db/migrations/000134_unibox_folder_identity.down.sql new file mode 100644 index 00000000..c0e97909 --- /dev/null +++ b/internal/infrastructure/db/migrations/000134_unibox_folder_identity.down.sql @@ -0,0 +1,21 @@ +-- Back to UIDVALIDITY as the folder's identity. +-- +-- The backfill cursor keys are not rewritten back: a folder name that no +-- longer maps to a row would be lost, and the only cost of leaving them is +-- that an unfinished IMAP backfill re-walks its folders once. + +DROP INDEX IF EXISTS idx_unibox_emails_folder_path; + +ALTER TABLE public.unibox_emails DROP COLUMN IF EXISTS folder_path; + +-- Two folders that shared a UIDVALIDITY could both be stored under the new +-- key but not under the old one, so drop the later ones first. +DELETE FROM public.unibox_mailboxes um +USING public.unibox_mailboxes other +WHERE um.email_id = other.email_id + AND um.uid_validity = other.uid_validity + AND (um.updated_at, um.mailbox) < (other.updated_at, other.mailbox); + +ALTER TABLE public.unibox_mailboxes DROP CONSTRAINT unibox_mailboxes_pkey; +ALTER TABLE public.unibox_mailboxes + ADD CONSTRAINT unibox_mailboxes_pkey PRIMARY KEY (email_id, uid_validity); diff --git a/internal/infrastructure/db/migrations/000134_unibox_folder_identity.up.sql b/internal/infrastructure/db/migrations/000134_unibox_folder_identity.up.sql new file mode 100644 index 00000000..e6e56afa --- /dev/null +++ b/internal/infrastructure/db/migrations/000134_unibox_folder_identity.up.sql @@ -0,0 +1,80 @@ +-- A mail folder is identified by its name, not by its UIDVALIDITY. +-- +-- UIDVALIDITY was the folder's identity everywhere: the primary key of this +-- table, the number DELETE_MAILBOX carried, and the stamp on every stored +-- message. RFC 3501 does not support that. It promises only that UIDs are +-- stable WITHIN one folder while its UIDVALIDITY is unchanged, and says +-- nothing about the value being unique ACROSS folders. Servers that derive it +-- from the folder's creation time, Dovecot among them, hand the same number +-- to every folder created in the same second, which is what a folder tree +-- made by a mail client, an import or a server migration is by definition. +-- +-- The sync loop contained that by following one folder per id and reporting +-- the rest, so the others' mail was never synced. Keying by name fixes it at +-- the root: IMAP does guarantee a name is unique per account. +-- +-- UIDVALIDITY keeps the job it actually has. It is the validity marker for +-- the UIDs we hold, so it stays on the folder row (the cursor is void when it +-- changes) and on each message (its stored uid belongs to that generation). +-- +-- Cost note: the backfill below walks unibox_emails, which holds every synced +-- message, so on a long-running instance it is the expensive part. The +-- backend applies migrations at boot inside one transaction and blocks until +-- they finish, so deploy this in a window rather than alongside traffic. +-- ADD COLUMN with a constant DEFAULT is metadata-only on PG 11+ and is not +-- itself a rewrite. + +-- A row with no name cannot be addressed by one. It predates the name being +-- an identity; the next sync pass re-creates it from the listing. +DELETE FROM public.unibox_mailboxes WHERE mailbox = ''; + +-- Collapse names that appear more than once. Under the old primary key a +-- folder whose UIDVALIDITY changed inserted a second row and left the first +-- behind, so the same name can be here twice. Keep the one the sync touched +-- last, breaking a tie on the higher UIDVALIDITY, which is the newer +-- generation on every server that derives it from a clock or a counter. +DELETE FROM public.unibox_mailboxes um +USING public.unibox_mailboxes other +WHERE um.email_id = other.email_id + AND um.mailbox = other.mailbox + AND (um.updated_at, um.uid_validity) < (other.updated_at, other.uid_validity); + +ALTER TABLE public.unibox_mailboxes DROP CONSTRAINT unibox_mailboxes_pkey; +ALTER TABLE public.unibox_mailboxes + ADD CONSTRAINT unibox_mailboxes_pkey PRIMARY KEY (email_id, mailbox); + +-- folder_path is the message's source folder by name: stable across a +-- UIDVALIDITY change, and the thing a rename moves rather than orphans. +-- unibox_emails.mailbox keeps its own meaning as the UIDVALIDITY generation +-- the stored uid belongs to. +ALTER TABLE public.unibox_emails + ADD COLUMN folder_path text NOT NULL DEFAULT ''; + +UPDATE public.unibox_emails ue +SET folder_path = um.mailbox +FROM public.unibox_mailboxes um +WHERE um.email_id = ue.email_id + AND um.uid_validity = ue.mailbox + AND ue.mailbox <> 0; + +CREATE INDEX idx_unibox_emails_folder_path ON public.unibox_emails (email_id, folder_path); + +-- The backfill cursor keys its per-folder floors by folder. On IMAP those +-- keys were UIDVALIDITY rendered as a string; rewrite them to names so a +-- backfill in flight resumes instead of re-walking every folder from the top. +-- Graph accounts already key by folder name and fall through the LEFT JOIN +-- unchanged. +UPDATE public.email_sync_state ess +SET backfill_cursor = jsonb_set( + ess.backfill_cursor, + '{folders}', + ( + SELECT COALESCE(jsonb_object_agg(COALESCE(um.mailbox, f.key), f.value), '{}'::jsonb) + FROM jsonb_each(ess.backfill_cursor -> 'folders') AS f(key, value) + LEFT JOIN public.unibox_mailboxes um + ON um.email_id = ess.email_id + AND um.uid_validity::text = f.key + ) + ) +WHERE jsonb_typeof(ess.backfill_cursor -> 'folders') = 'object' + AND ess.backfill_cursor -> 'folders' <> '{}'::jsonb; diff --git a/internal/models/event.go b/internal/models/event.go index bd22a897..8cb6eca1 100644 --- a/internal/models/event.go +++ b/internal/models/event.go @@ -27,6 +27,10 @@ const ( JobEventTypeEmailUpdate JobEventType = "UPDATE_EMAIL" JobEventTypeMailboxUpdate JobEventType = "UPDATE_MAILBOX" JobEventTypeMailboxDelete JobEventType = "DELETE_MAILBOX" + // JobEventTypeMailboxRename is a folder that kept its UIDVALIDITY under a + // new name. Distinct from a delete plus an insert because the folder's + // stored mail has to move with it rather than be orphaned. + JobEventTypeMailboxRename JobEventType = "RENAME_MAILBOX" JobEventTypeTokenUpdate JobEventType = "TOKEN_UPDATE" JobEventTypeHistoryIDUpdate JobEventType = "HISTORY_ID_UPDATE" diff --git a/internal/models/event_w_emails.go b/internal/models/event_w_emails.go index cb434124..f5a64f48 100644 --- a/internal/models/event_w_emails.go +++ b/internal/models/event_w_emails.go @@ -26,7 +26,11 @@ type JobEventEmailUpdate struct { ID uuid.UUID `json:"id"` UID uint32 `json:"uid"` ModSeq uint64 `json:"mod_seq"` - Mailbox uint32 `json:"mailbox"` + // Mailbox is the folder's UIDVALIDITY, the generation UID belongs to. + Mailbox uint32 `json:"mailbox"` + // FolderPath is the folder's name, its identity. Empty on events from + // workers predating the field (the consumer then keeps the stored value). + FolderPath string `json:"folder_path,omitempty"` // Folder is the canonical folder the message now sits in; empty on events // from workers predating folder tracking (the consumer then keeps the // stored value). diff --git a/internal/models/event_w_mailbox.go b/internal/models/event_w_mailbox.go index 6601ad9d..29495879 100644 --- a/internal/models/event_w_mailbox.go +++ b/internal/models/event_w_mailbox.go @@ -8,8 +8,24 @@ type JobEventMailboxUpdate struct { Data *Mailbox `json:"data"` } +// JobEventMailboxDelete retires a folder that is no longer in the listing. type JobEventMailboxDelete struct { - UserID uuid.UUID `json:"user_id"` - EmailID uuid.UUID `json:"email_id"` - UIDValidity uint32 `json:"uid_validity"` + UserID uuid.UUID `json:"user_id"` + EmailID uuid.UUID `json:"email_id"` + // Mailbox is the folder's name, which is what identifies it. Empty only + // on events from workers that predate the name being the identity; the + // consumer then falls back to UIDValidity. + Mailbox string `json:"mailbox,omitempty"` + // UIDValidity is that legacy fallback and nothing else. + UIDValidity uint32 `json:"uid_validity"` +} + +// JobEventMailboxRename is a folder that kept its UIDVALIDITY under a new +// name, which is what an IMAP RENAME looks like from the listing. The stored +// folder row and the mail filed under the old name both move. +type JobEventMailboxRename struct { + UserID uuid.UUID `json:"user_id"` + EmailID uuid.UUID `json:"email_id"` + From string `json:"from"` + To string `json:"to"` } diff --git a/internal/models/mailbox.go b/internal/models/mailbox.go index 858aa011..637c8240 100644 --- a/internal/models/mailbox.go +++ b/internal/models/mailbox.go @@ -2,11 +2,19 @@ package models import "time" +// Mailbox is one folder of an account. Its identity is Name, which IMAP +// guarantees is unique per account; UIDValidity is not an identity and never +// was, since RFC 3501 only promises UIDs are stable within one folder and +// servers deriving the number from a creation time give a whole folder tree +// the same one. type Mailbox struct { - Name string `json:"name"` - Attrs []string `json:"attributes"` - UIDValidity uint32 `json:"uid_validity"` - HighestModSeq uint64 `json:"highestmodseq"` + Name string `json:"name"` + Attrs []string `json:"attributes"` + // UIDValidity is the validity marker for the UIDs held below: when the + // server changes it, every stored uid for this folder is void and the + // cursors have to be rebuilt. + UIDValidity uint32 `json:"uid_validity"` + HighestModSeq uint64 `json:"highestmodseq"` // UIDNext is the folder's next UID as last seen. It is the incremental // cursor on a server without CONDSTORE, where HighestModSeq stays 0. UIDNext uint32 `json:"uid_next"` diff --git a/internal/models/sync.go b/internal/models/sync.go index 5534f152..d4a2a18c 100644 --- a/internal/models/sync.go +++ b/internal/models/sync.go @@ -36,9 +36,9 @@ const ( ) // SyncFolderCursor is the resumable position inside one folder of a backfill. -// IMAP keys folders by UIDVALIDITY and walks UIDs downward; Graph keys by -// well-known folder name and follows @odata.nextLink; Gmail has no folders and -// uses SyncCursor.PageToken. +// Both IMAP and Graph key folders by name and IMAP walks UIDs downward, Graph +// follows @odata.nextLink; Gmail has no folders and uses +// SyncCursor.PageToken. type SyncFolderCursor struct { // Next is an opaque continuation (Graph nextLink). Next string `json:"next,omitempty" avro:"next"` @@ -83,9 +83,9 @@ type SyncState struct { // 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. + // folders whose name the server listed more than once. 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"` diff --git a/internal/models/unibox.go b/internal/models/unibox.go index e1555a27..1c8f87f1 100644 --- a/internal/models/unibox.go +++ b/internal/models/unibox.go @@ -94,7 +94,14 @@ type EmailMessageData struct { // used when for kafka when an email arrives type EmailMessageStoreData struct { ID uuid.UUID `json:"id"` EmailID uuid.UUID `json:"email_id"` - Mailbox uint32 `json:"mailbox"` + // Mailbox is the source folder's UIDVALIDITY at sync time, which is the + // generation UID belongs to. It is not the folder's identity: see + // FolderPath. + Mailbox uint32 `json:"mailbox"` + // FolderPath is the source folder's name, the identity IMAP actually + // guarantees. Empty on events from workers predating the field and on + // providers with no folders (Gmail). + FolderPath string `json:"folder_path,omitempty"` // Folder is the canonical folder (see the Folder* constants) the message // was in at sync time. Empty on events from workers predating the field; // the consumer normalizes before storing. diff --git a/internal/models/warmup.go b/internal/models/warmup.go index 758445f8..6869dbb8 100644 --- a/internal/models/warmup.go +++ b/internal/models/warmup.go @@ -35,14 +35,20 @@ type WarmupToken struct { // // For Gmail accounts the worker uses GmailID to issue Users.Messages.Modify // requests. For IMAP-backed accounts (Outlook + custom SMTP/IMAP) the worker -// needs UID + the source mailbox's UIDValidity to locate the message; the -// mailbox name is then resolved against the worker's cached folder list. +// needs UID plus the folder to locate the message: MailboxFolder names the +// folder and MailboxUIDValidity says which generation of its UIDs the stored +// UID belongs to, so an action is skipped rather than aimed at whatever +// message inherited the number after a UIDVALIDITY change. type WarmupEmailAction struct { UserID uuid.UUID `json:"user_id"` EmailID uuid.UUID `json:"email_id"` GmailID string `json:"gmail_id"` UID uint32 `json:"uid"` MailboxUIDValidity uint32 `json:"mailbox_uid_validity"` + // MailboxFolder is the source folder's name. Empty on events from + // consumers predating it, where the worker falls back to matching on + // MailboxUIDValidity alone. + MailboxFolder string `json:"mailbox_folder,omitempty"` // RFCMessageID is the immutable RFC 5322 Message-ID. Graph provider ids // change when a message is moved (copy+delete), so the worker re-resolves // the live Graph id from this stable key at action time. diff --git a/internal/repository/folder_identity_live_test.go b/internal/repository/folder_identity_live_test.go new file mode 100644 index 00000000..cc5dcaae --- /dev/null +++ b/internal/repository/folder_identity_live_test.go @@ -0,0 +1,196 @@ +package repository + +import ( + "context" + "os" + "testing" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/warmbly/warmbly/internal/infrastructure/db" + "github.com/warmbly/warmbly/internal/models" +) + +// Issue #356: a folder is keyed by its name, not by its UIDVALIDITY, which +// RFC 3501 never promised was unique across folders. These run the statements +// that changed against a real schema, because the interesting parts of both +// are conditions Postgres evaluates and Go cannot: the upsert's new conflict +// target, and the rename's guard against a name the account already has. +// +// WARMBLY_TEST_DB=postgres://warmbly:warmbly@localhost:15432/warmbly_dev?sslmode=disable \ +// go test ./internal/repository/ -run LiveFolderIdentity -v + +type folderFixture struct { + org uuid.UUID + user uuid.UUID + mailbox uuid.UUID +} + +func newFolderFixture(t *testing.T, pool *pgxpool.Pool) *folderFixture { + t.Helper() + ctx := context.Background() + f := &folderFixture{org: uuid.New(), user: uuid.New(), mailbox: uuid.New()} + tag := "i356-" + f.org.String()[:8] + + exec := func(sql string, args ...any) { + t.Helper() + if _, err := pool.Exec(ctx, sql, args...); err != nil { + t.Fatalf("fixture %q: %v", sql[:min(60, len(sql))], err) + } + } + exec(`INSERT INTO users (id, first_name, last_name, email, password_hash) + VALUES ($1, 'Folder', 'Live', $2, 'x')`, f.user, tag+"@test.local") + exec(`INSERT INTO organizations (id, name, slug, owner_user_id) + VALUES ($1, 'Issue 356', $2, $3)`, f.org, tag, f.user) + exec(`INSERT INTO organization_members (organization_id, user_id, role, accepted_at) + VALUES ($1, $2, 'owner', NOW())`, f.org, f.user) + exec(`INSERT INTO email_accounts (id, user_id, organization_id, email, name, signature_plain, signature_html, provider) + VALUES ($1, $2, $3, $4, 'Folder', '', '', 'smtp_imap')`, f.mailbox, f.user, f.org, tag+"-mb@test.local") + + t.Cleanup(func() { + c := context.Background() + _, _ = pool.Exec(c, `DELETE FROM unibox_emails WHERE email_id = $1`, f.mailbox) + _, _ = pool.Exec(c, `DELETE FROM unibox_mailboxes WHERE email_id = $1`, f.mailbox) + _, _ = pool.Exec(c, `DELETE FROM email_accounts WHERE id = $1`, f.mailbox) + _, _ = pool.Exec(c, `DELETE FROM organization_members WHERE organization_id = $1`, f.org) + _, _ = pool.Exec(c, `DELETE FROM organizations WHERE id = $1`, f.org) + _, _ = pool.Exec(c, `DELETE FROM users WHERE id = $1`, f.user) + }) + return f +} + +func liveFolderDB(t *testing.T) *db.DB { + t.Helper() + dsn := os.Getenv("WARMBLY_TEST_DB") + if dsn == "" { + t.Skip("WARMBLY_TEST_DB not set") + } + handle, err := db.New(context.Background(), dsn) + if err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { handle.Pool.Close() }) + return handle +} + +// Folders that share a UIDVALIDITY are ordinary folders, each with its own +// row and its own cursor. Under the old key the second upsert overwrote the +// first, which is how a folder lost its entire sync. +func TestLiveFolderIdentityKeepsFoldersSharingAUIDValidity(t *testing.T) { + handle := liveFolderDB(t) + f := newFolderFixture(t, handle.Pool) + repo := NewMailboxRepository(handle) + ctx := context.Background() + + for _, name := range []string{"Clients/Acme", "Clients/Globex"} { + if err := repo.CreateEntry(ctx, f.user, f.mailbox, &models.Mailbox{ + Name: name, UIDValidity: 42, HighestModSeq: 7, Attrs: []string{}, + }); err != nil { + t.Fatalf("CreateEntry(%q): %v", name, err) + } + } + + boxes, err := repo.ListMailboxes(ctx, f.user, f.mailbox) + if err != nil { + t.Fatalf("ListMailboxes: %v", err) + } + if len(boxes) != 2 { + t.Fatalf("stored %d folders, want both: %+v", len(boxes), boxes) + } + + // The cursor is per folder, and re-upserting one leaves the other alone. + if err := repo.CreateEntry(ctx, f.user, f.mailbox, &models.Mailbox{ + Name: "Clients/Acme", UIDValidity: 43, HighestModSeq: 99, Attrs: []string{}, + }); err != nil { + t.Fatalf("CreateEntry (second pass): %v", err) + } + acme, err := repo.GetMailbox(ctx, f.user, f.mailbox, "Clients/Acme") + if err != nil || acme == nil { + t.Fatalf("GetMailbox(Clients/Acme) = %+v, %v", acme, err) + } + if acme.UIDValidity != 43 || acme.HighestModSeq != 99 { + t.Errorf("Clients/Acme = %+v, want the new generation and cursor", acme) + } + globex, err := repo.GetMailbox(ctx, f.user, f.mailbox, "Clients/Globex") + if err != nil || globex == nil { + t.Fatalf("GetMailbox(Clients/Globex) = %+v, %v", globex, err) + } + if globex.HighestModSeq != 7 { + t.Errorf("Clients/Globex cursor = %d, want 7 left alone", globex.HighestModSeq) + } + + // Deleting one folder by name does not take its UIDVALIDITY twin with it. + if err := repo.DeleteMailbox(ctx, f.user, f.mailbox, "Clients/Globex"); err != nil { + t.Fatalf("DeleteMailbox: %v", err) + } + boxes, err = repo.ListMailboxes(ctx, f.user, f.mailbox) + if err != nil { + t.Fatalf("ListMailboxes after delete: %v", err) + } + if len(boxes) != 1 || boxes[0].Name != "Clients/Acme" { + t.Fatalf("after deleting one folder: %+v", boxes) + } +} + +// A rename moves the row and the mail. It must not run when the account +// already has a folder under the new name, which is not a rename at all. +func TestLiveFolderIdentityRenameMovesRowAndMail(t *testing.T) { + handle := liveFolderDB(t) + f := newFolderFixture(t, handle.Pool) + mailboxes := NewMailboxRepository(handle) + unibox := NewUniboxRepository(handle) + ctx := context.Background() + + if err := mailboxes.CreateEntry(ctx, f.user, f.mailbox, &models.Mailbox{ + Name: "Clients/Acme", UIDValidity: 42, HighestModSeq: 7, Attrs: []string{}, + }); err != nil { + t.Fatalf("CreateEntry: %v", err) + } + msg := &models.EmailMessageStoreData{ + ID: uuid.New(), EmailID: f.mailbox, Mailbox: 42, FolderPath: "Clients/Acme", + ThreadID: "t-356", MessageID: "<356@test>", UID: 5, Folder: models.FolderInbox, + } + if err := unibox.CreateEntry(ctx, f.user, msg); err != nil { + t.Fatalf("CreateEntry(message): %v", err) + } + + if err := mailboxes.RenameMailbox(ctx, f.user, f.mailbox, "Clients/Acme", "Clients/Acme Corp"); err != nil { + t.Fatalf("RenameMailbox: %v", err) + } + if err := unibox.MoveFolderPath(ctx, f.mailbox, "Clients/Acme", "Clients/Acme Corp"); err != nil { + t.Fatalf("MoveFolderPath: %v", err) + } + + moved, err := mailboxes.GetMailbox(ctx, f.user, f.mailbox, "Clients/Acme Corp") + if err != nil || moved == nil { + t.Fatalf("GetMailbox after rename = %+v, %v", moved, err) + } + // The cursor rides along: an IMAP RENAME changes nothing about the UIDs, + // so re-baselining the folder would re-import its history for a label. + if moved.UIDValidity != 42 || moved.HighestModSeq != 7 { + t.Errorf("renamed folder = %+v, want its cursor intact", moved) + } + stored, err := unibox.GetByID(ctx, f.user, msg.ID) + if err != nil { + t.Fatalf("GetByID: %v", err) + } + if stored.FolderPath != "Clients/Acme Corp" { + t.Errorf("message folder_path = %q, want it to move with the folder", stored.FolderPath) + } + + // A name the account already has is not a rename; the row stays put + // rather than colliding with the folder that is already there. + if err := mailboxes.CreateEntry(ctx, f.user, f.mailbox, &models.Mailbox{ + Name: "Clients/Globex", UIDValidity: 43, Attrs: []string{}, + }); err != nil { + t.Fatalf("CreateEntry(Globex): %v", err) + } + if err := mailboxes.RenameMailbox(ctx, f.user, f.mailbox, "Clients/Globex", "Clients/Acme Corp"); err != nil { + t.Fatalf("RenameMailbox onto an existing name: %v", err) + } + still, err := mailboxes.GetMailbox(ctx, f.user, f.mailbox, "Clients/Globex") + if err != nil || still == nil { + t.Fatalf("Clients/Globex was moved onto an occupied name: %+v, %v", still, err) + } +} diff --git a/internal/repository/pg_mailbox.go b/internal/repository/pg_mailbox.go index faaa78fd..a069bea0 100644 --- a/internal/repository/pg_mailbox.go +++ b/internal/repository/pg_mailbox.go @@ -10,11 +10,21 @@ import ( "github.com/warmbly/warmbly/internal/models" ) +// MailboxRepository stores an account's folder rows. A folder is keyed by its +// name, which IMAP guarantees is unique per account; uid_validity rides along +// as the validity marker for the UIDs the cursor holds. type MailboxRepository interface { CreateEntry(ctx context.Context, userId, emailId uuid.UUID, mb *models.Mailbox) error - GetMailbox(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) (*models.Mailbox, error) + GetMailbox(ctx context.Context, userId, emailId uuid.UUID, name string) (*models.Mailbox, error) ListMailboxes(ctx context.Context, userId, emailId uuid.UUID) ([]models.Mailbox, error) - DeleteMailbox(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) error + DeleteMailbox(ctx context.Context, userId, emailId uuid.UUID, name string) error + // DeleteMailboxByUIDValidity retires a folder a worker named only by its + // UIDVALIDITY, which is what a worker deployed before the name became the + // identity sends. Nothing else should reach for it. + DeleteMailboxByUIDValidity(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) error + // RenameMailbox moves a folder row to a new name. The mail filed under + // the old one is moved by the caller in the same handler. + RenameMailbox(ctx context.Context, userId, emailId uuid.UUID, from, to string) error } type mailboxRepository struct { @@ -31,8 +41,8 @@ func (r *mailboxRepository) CreateEntry(ctx context.Context, userId, emailId uui query := ` INSERT INTO unibox_mailboxes (email_id, uid_validity, mailbox, attributes, highestmodseq, uid_next, updated_at) VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (email_id, uid_validity) DO UPDATE SET - mailbox = EXCLUDED.mailbox, + ON CONFLICT (email_id, mailbox) DO UPDATE SET + uid_validity = EXCLUDED.uid_validity, attributes = EXCLUDED.attributes, highestmodseq = EXCLUDED.highestmodseq, uid_next = EXCLUDED.uid_next, @@ -46,15 +56,15 @@ func (r *mailboxRepository) CreateEntry(ctx context.Context, userId, emailId uui return err } -func (r *mailboxRepository) GetMailbox(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) (*models.Mailbox, error) { +func (r *mailboxRepository) GetMailbox(ctx context.Context, userId, emailId uuid.UUID, name string) (*models.Mailbox, error) { query := ` SELECT mailbox, attributes, uid_validity, highestmodseq, uid_next, updated_at FROM unibox_mailboxes - WHERE email_id = $1 AND uid_validity = $2 + WHERE email_id = $1 AND mailbox = $2 ` var mb models.Mailbox - err := r.db.QueryRow(ctx, query, emailId, uidValidity).Scan( + err := r.db.QueryRow(ctx, query, emailId, name).Scan( &mb.Name, &mb.Attrs, &mb.UIDValidity, &mb.HighestModSeq, &mb.UIDNext, &mb.UpdatedAt, ) if err != nil { @@ -92,10 +102,35 @@ func (r *mailboxRepository) ListMailboxes(ctx context.Context, userId, emailId u return mailboxes, nil } -func (r *mailboxRepository) DeleteMailbox(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) error { +func (r *mailboxRepository) DeleteMailbox(ctx context.Context, userId, emailId uuid.UUID, name string) error { + _, err := r.db.Exec(ctx, + `DELETE FROM unibox_mailboxes WHERE email_id = $1 AND mailbox = $2`, + emailId, name, + ) + return err +} + +func (r *mailboxRepository) DeleteMailboxByUIDValidity(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) error { _, err := r.db.Exec(ctx, `DELETE FROM unibox_mailboxes WHERE email_id = $1 AND uid_validity = $2`, emailId, uidValidity, ) return err } + +// RenameMailbox is an update, not a delete and an insert, so the row keeps the +// cursor it holds: an IMAP RENAME leaves UIDVALIDITY and every UID alone, and +// re-baselining the folder would re-import its history for a change of label. +// +// A name the account already has means the listing showed us both at once, +// which is not a rename. Leave the row alone and let the ordinary insert and +// delete paths sort it out. +func (r *mailboxRepository) RenameMailbox(ctx context.Context, userId, emailId uuid.UUID, from, to string) error { + _, err := r.db.Exec(ctx, + `UPDATE unibox_mailboxes SET mailbox = $3, updated_at = NOW() + WHERE email_id = $1 AND mailbox = $2 + AND NOT EXISTS (SELECT 1 FROM unibox_mailboxes o WHERE o.email_id = $1 AND o.mailbox = $3)`, + emailId, from, to, + ) + return err +} diff --git a/internal/repository/pg_unibox.go b/internal/repository/pg_unibox.go index a288cb7e..467b0637 100644 --- a/internal/repository/pg_unibox.go +++ b/internal/repository/pg_unibox.go @@ -19,12 +19,18 @@ type UpdateUniboxEntry struct { Flags []string `json:"flags"` ModSeq *uint64 `json:"mod_seq"` Mailbox *uint32 `json:"mailbox"` - Folder *string `json:"folder"` + // FolderPath is the source folder's name, the folder's identity. + FolderPath *string `json:"folder_path"` + Folder *string `json:"folder"` } type UniboxRepository interface { CreateEntry(ctx context.Context, userID uuid.UUID, e *models.EmailMessageStoreData) error UpdateEntry(ctx context.Context, userID, emailID, id uuid.UUID, e *UpdateUniboxEntry) error + // MoveFolderPath re-files an account's stored mail when the server renames + // a folder, so the messages travel with it instead of being orphaned under + // a name that no longer exists. + MoveFolderPath(ctx context.Context, emailID uuid.UUID, from, to string) error GetIncoming(ctx context.Context, userID uuid.UUID, limit int, cursor string) (*models.MailSearchResult, error) GetByID(ctx context.Context, userID, id uuid.UUID) (*models.EmailMessageStoreData, error) // GetByIDForOrg is the org-scoped read for the unibox detail view: any @@ -105,7 +111,7 @@ func NewUniboxRepository(db *db.DB) UniboxRepository { } var mailFieldsFull = []string{ - "id", "email_id", "mailbox", "thread_id", "message_id", + "id", "email_id", "mailbox", "folder_path", "thread_id", "message_id", "gmail_id", "parent_id", "uid", "mod_seq", "flags", "bcc", "cc", "from_addr", "in_reply_to", "reply_to", "to_addr", "subject", "size", "internal_date", "sent_date", @@ -120,17 +126,17 @@ var mailFieldsPreview = []string{ func (r *uniboxRepository) CreateEntry(ctx context.Context, userID uuid.UUID, e *models.EmailMessageStoreData) error { query := ` INSERT INTO unibox_emails ( - id, user_id, email_id, mailbox, thread_id, message_id, + id, user_id, email_id, mailbox, folder_path, thread_id, message_id, gmail_id, parent_id, uid, mod_seq, flags, bcc, cc, from_addr, in_reply_to, reply_to, to_addr, subject, size, internal_date, sent_date, snippet, seen, created_at, updated_at, body_text, folder ) VALUES ( - $1, $2, $3, $4, $5, $6, - $7, $8, $9, $10, - $11, $12, $13, $14, $15, $16, - $17, $18, $19, $20, $21, - $22, $23, $24, $25, $26, $27 + $1, $2, $3, $4, $5, $6, $7, + $8, $9, $10, $11, + $12, $13, $14, $15, $16, $17, + $18, $19, $20, $21, $22, + $23, $24, $25, $26, $27, $28 ) ON CONFLICT (id) DO NOTHING ` @@ -138,7 +144,7 @@ func (r *uniboxRepository) CreateEntry(ctx context.Context, userID uuid.UUID, e // The array columns are NOT NULL. A nil Go slice binds as SQL NULL, so a // message with no In-Reply-To (any thread root) would fail the insert. _, err := r.db.Exec(ctx, query, - e.ID, userID, e.EmailID, e.Mailbox, e.ThreadID, e.MessageID, + e.ID, userID, e.EmailID, e.Mailbox, e.FolderPath, e.ThreadID, e.MessageID, e.GmailID, e.ParentID, e.UID, e.ModSeq, textArray(e.Flags), textArray(e.BCC), textArray(e.CC), textArray(e.FromAddr), textArray(e.InReplyTo), textArray(e.ReplyTo), textArray(e.ToAddr), @@ -183,6 +189,11 @@ func (r *uniboxRepository) UpdateEntry(ctx context.Context, userID, emailID, id args = append(args, *e.UID) argPos++ } + if e.FolderPath != nil { + setClauses = append(setClauses, fmt.Sprintf("folder_path = $%d", argPos)) + args = append(args, *e.FolderPath) + argPos++ + } if e.Folder != nil { setClauses = append(setClauses, fmt.Sprintf("folder = $%d", argPos)) args = append(args, *e.Folder) @@ -203,6 +214,18 @@ func (r *uniboxRepository) UpdateEntry(ctx context.Context, userID, emailID, id return err } +func (r *uniboxRepository) MoveFolderPath(ctx context.Context, emailID uuid.UUID, from, to string) error { + if from == "" || to == "" || from == to { + return nil + } + _, err := r.db.Exec(ctx, + `UPDATE unibox_emails SET folder_path = $3, updated_at = NOW() + WHERE email_id = $1 AND folder_path = $2`, + emailID, from, to, + ) + return err +} + func (r *uniboxRepository) GetIncoming(ctx context.Context, userID uuid.UUID, limit int, cursor string) (*models.MailSearchResult, error) { query := fmt.Sprintf(` SELECT %s @@ -240,7 +263,7 @@ func (r *uniboxRepository) GetByID(ctx context.Context, userID, id uuid.UUID) (* var e models.EmailMessageStoreData err := r.db.QueryRow(ctx, query, userID, id).Scan( - &e.ID, &e.EmailID, &e.Mailbox, &e.ThreadID, &e.MessageID, + &e.ID, &e.EmailID, &e.Mailbox, &e.FolderPath, &e.ThreadID, &e.MessageID, &e.GmailID, &e.ParentID, &e.UID, &e.ModSeq, &e.Flags, &e.BCC, &e.CC, &e.FromAddr, &e.InReplyTo, &e.ReplyTo, &e.ToAddr, &e.Subject, &e.Size, &e.InternalDate, &e.SentDate, @@ -279,7 +302,7 @@ func (r *uniboxRepository) GetByIDForOrg(ctx context.Context, orgID, id uuid.UUI var e models.EmailMessageStoreData err := r.db.QueryRow(ctx, query, orgID, id).Scan( &ownerID, - &e.ID, &e.EmailID, &e.Mailbox, &e.ThreadID, &e.MessageID, + &e.ID, &e.EmailID, &e.Mailbox, &e.FolderPath, &e.ThreadID, &e.MessageID, &e.GmailID, &e.ParentID, &e.UID, &e.ModSeq, &e.Flags, &e.BCC, &e.CC, &e.FromAddr, &e.InReplyTo, &e.ReplyTo, &e.ToAddr, &e.Subject, &e.Size, &e.InternalDate, &e.SentDate, diff --git a/internal/sandbox/history.go b/internal/sandbox/history.go index d048d98a..8de0fe7b 100644 --- a/internal/sandbox/history.go +++ b/internal/sandbox/history.go @@ -298,8 +298,8 @@ func seedUniboxHistory(ctx context.Context, pool *pgxpool.Pool) error { if _, err := pool.Exec(ctx, ` INSERT INTO unibox_mailboxes (email_id, uid_validity, mailbox, attributes, highestmodseq, updated_at) VALUES ($1, $2, 'INBOX', '{"\\HasNoChildren"}', 1, NOW()) - ON CONFLICT (email_id, uid_validity) DO UPDATE SET - mailbox = EXCLUDED.mailbox, + ON CONFLICT (email_id, mailbox) DO UPDATE SET + uid_validity = EXCLUDED.uid_validity, attributes = EXCLUDED.attributes, highestmodseq = EXCLUDED.highestmodseq, updated_at = NOW()`, diff --git a/internal/seed/dev_history.go b/internal/seed/dev_history.go index da17da0c..52281932 100644 --- a/internal/seed/dev_history.go +++ b/internal/seed/dev_history.go @@ -196,8 +196,8 @@ func seedDevUnibox(ctx context.Context, pool *pgxpool.Pool) error { if _, err := pool.Exec(ctx, ` INSERT INTO unibox_mailboxes (email_id, uid_validity, mailbox, attributes, highestmodseq, updated_at) VALUES ($1, $2, 'INBOX', ARRAY['\HasNoChildren'], 1, NOW()) - ON CONFLICT (email_id, uid_validity) DO UPDATE SET - mailbox = EXCLUDED.mailbox, + ON CONFLICT (email_id, mailbox) DO UPDATE SET + uid_validity = EXCLUDED.uid_validity, updated_at = NOW() `, mb.emailID, mb.uidValidity); err != nil { return fmt.Errorf("unibox mailbox %s: %w", mb.emailID, err) diff --git a/internal/seed/unibox.go b/internal/seed/unibox.go index f1188906..627f3651 100644 --- a/internal/seed/unibox.go +++ b/internal/seed/unibox.go @@ -149,8 +149,8 @@ func seedUniboxMailboxes(ctx context.Context, pool *pgxpool.Pool) error { _, err := pool.Exec(ctx, ` INSERT INTO unibox_mailboxes (email_id, uid_validity, mailbox, attributes, highestmodseq, updated_at) VALUES ($1, $2, $3, $4, 1, NOW()) - ON CONFLICT (email_id, uid_validity) DO UPDATE SET - mailbox = EXCLUDED.mailbox, + ON CONFLICT (email_id, mailbox) DO UPDATE SET + uid_validity = EXCLUDED.uid_validity, attributes = EXCLUDED.attributes, highestmodseq = EXCLUDED.highestmodseq, updated_at = NOW() @@ -170,13 +170,13 @@ func insertUniboxEmail(ctx context.Context, pool *pgxpool.Pool, row seededUnibox _, err = pool.Exec(ctx, ` INSERT INTO unibox_emails ( - id, user_id, email_id, mailbox, thread_id, message_id, + id, user_id, email_id, mailbox, folder_path, thread_id, message_id, gmail_id, parent_id, uid, mod_seq, flags, bcc, cc, from_addr, in_reply_to, reply_to, to_addr, subject, size, internal_date, sent_date, snippet, seen, created_at, updated_at ) VALUES ( - $1, $2, $3, $4, $5, $6, + $1, $2, $3, $4, 'INBOX', $5, $6, '', $7, $8, 1, $9, '{}', '{}', $10, '{}', '{}', $11, $12, $13, $14, $14, @@ -185,6 +185,7 @@ func insertUniboxEmail(ctx context.Context, pool *pgxpool.Pool, row seededUnibox ON CONFLICT (id) DO UPDATE SET email_id = EXCLUDED.email_id, mailbox = EXCLUDED.mailbox, + folder_path = EXCLUDED.folder_path, thread_id = EXCLUDED.thread_id, message_id = EXCLUDED.message_id, parent_id = EXCLUDED.parent_id, diff --git a/web/src/components/app/emails/SyncStatusCard.tsx b/web/src/components/app/emails/SyncStatusCard.tsx index 2a7c40f4..c8e5a6eb 100644 --- a/web/src/components/app/emails/SyncStatusCard.tsx +++ b/web/src/components/app/emails/SyncStatusCard.tsx @@ -124,8 +124,9 @@ export default function SyncStatusCard({ mailboxId }: { mailboxId: string }) { {(state?.folders_skipped_conflict ?? 0) > 0 && (

- {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. + Your mail server listed {state!.folders_skipped_conflict!.toLocaleString()} folder + {state!.folders_skipped_conflict === 1 ? " name" : " names"} more than once, so only the first of each is + synced. Renaming one of them on your mail server clears this.

)} diff --git a/web/src/lib/api/models/app/emails/SyncState.ts b/web/src/lib/api/models/app/emails/SyncState.ts index 861b9bf7..9bf118d1 100644 --- a/web/src/lib/api/models/app/emails/SyncState.ts +++ b/web/src/lib/api/models/app/emails/SyncState.ts @@ -22,7 +22,7 @@ export interface SyncState { 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 left out because the mail server listed their name more than once. */ folders_skipped_conflict?: number; last_synced_at?: string; }