diff --git a/internal/app/consumer/event_mailbox_delete.go b/internal/app/consumer/event_mailbox_delete.go index 2c4d16e7..3c35f0e0 100644 --- a/internal/app/consumer/event_mailbox_delete.go +++ b/internal/app/consumer/event_mailbox_delete.go @@ -3,6 +3,7 @@ package jobs import ( "context" + "github.com/rs/zerolog/log" "github.com/warmbly/warmbly/internal/models" ) @@ -10,19 +11,30 @@ import ( // // 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. +// all; it only deletes when the number matches exactly one folder, because it +// no longer identifies one on a server that stamps it from a creation time. +// An ambiguous one is left alone rather than retried: nothing about it will +// change, and the next pass from an updated worker retires the folder by +// name. func (s *JobsService) HandleMailboxDelete(ctx context.Context, e *models.JobEventMailboxDelete) error { - 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 := s.MailboxRepository.DeleteMailbox(ctx, e.UserID, e.EmailID, e.Mailbox); err != nil { + CaptureError(e.UserID, e.EmailID, err) + return err + } + return nil } + + removed, err := s.MailboxRepository.DeleteMailboxByUIDValidity(ctx, e.UserID, e.EmailID, e.UIDValidity) if err != nil { CaptureError(e.UserID, e.EmailID, err) return err } - + if removed == 0 { + log.Info(). + Str("email_id", e.EmailID.String()). + Uint32("uid_validity", e.UIDValidity). + Msg("legacy mailbox delete skipped: that UIDVALIDITY does not name exactly one folder") + } return nil } diff --git a/internal/app/consumer/event_mailbox_rename.go b/internal/app/consumer/event_mailbox_rename.go index a81bd85b..3f6bc734 100644 --- a/internal/app/consumer/event_mailbox_rename.go +++ b/internal/app/consumer/event_mailbox_rename.go @@ -3,6 +3,7 @@ package jobs import ( "context" + "github.com/rs/zerolog/log" "github.com/warmbly/warmbly/internal/models" ) @@ -12,23 +13,26 @@ import ( // 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 and the mail both just move, and they move together or not at all. // -// 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. +// A rename that finds nothing to move is not an error. The destination name +// already existing means the listing showed both folders at once, which is +// not a rename, and the ordinary insert and delete paths handle it. 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 { + renamed, err := s.MailboxRepository.RenameMailbox(ctx, e.UserID, e.EmailID, e.From, e.To) + if 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 + if !renamed { + log.Info(). + Str("email_id", e.EmailID.String()). + Str("from", e.From). + Str("to", e.To). + Msg("mailbox rename had nothing to move; the destination name is already taken or the source is gone") } return nil } diff --git a/internal/app/worker/wmail/folder_identity_test.go b/internal/app/worker/wmail/folder_identity_test.go index eaf48f6b..318604d3 100644 --- a/internal/app/worker/wmail/folder_identity_test.go +++ b/internal/app/worker/wmail/folder_identity_test.go @@ -200,3 +200,55 @@ func TestSyncRebaselinesAFolderWhoseUIDValidityChanged(t *testing.T) { t.Errorf("retired %v; the folder is still there", got) } } + +// A folder's backfill floor goes with the folder. A name is reusable, so a +// floor left behind is inherited by whatever is created under that name next: +// a "done" cursor skips the new folder's history entirely, and the messages it +// skips are not new mail either, so nothing reports them missing. +func TestSyncForgetsTheBackfillFloorOfADeletedFolder(t *testing.T) { + conn := &fakeImapConn{folders: []models.Mailbox{{Name: "INBOX", UIDValidity: 7, 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, Done: true}) + + if err := w.Sync(t.Context()); err != nil { + t.Fatalf("Sync: %v", err) + } + + if got := mailboxDeletes(*events); len(got) != 1 || got[0] != "Clients/Acme" { + t.Fatalf("retired %v, want the folder that left the listing", got) + } + if cur := w.tracker.folder("Clients/Acme"); cur.Done || cur.UID != 0 { + t.Errorf("backfill floor = %+v, want it gone with the folder", cur) + } +} + +// A UIDVALIDITY that names two missing folders and one new one cannot say +// which was renamed. Picking either moves a folder's mail into a folder it has +// nothing to do with, so neither is claimed. +func TestSyncDoesNotGuessARenameWhenTwoStoredFoldersAreMissing(t *testing.T) { + conn := &fakeImapConn{folders: []models.Mailbox{ + {Name: "Clients/Initech", UIDValidity: 42, HighestModSeq: 100}, + }} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, + &models.Mailbox{Name: "Clients/Acme", UIDValidity: 42, HighestModSeq: 100}) + w.SmtpImapData.Mailboxes = append(w.SmtpImapData.Mailboxes, + &models.Mailbox{Name: "Clients/Globex", 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 a UIDVALIDITY that two missing folders share") + } + deletes := mailboxDeletes(*events) + if len(deletes) != 2 { + t.Fatalf("retired %v, want both folders that left the listing", deletes) + } + if mailboxUpdates(*events)["Clients/Initech"] == nil { + t.Error("the new folder was not baselined") + } +} diff --git a/internal/app/worker/wmail/sync_imap.go b/internal/app/worker/wmail/sync_imap.go index 05597459..5ea0d00b 100644 --- a/internal/app/worker/wmail/sync_imap.go +++ b/internal/app/worker/wmail/sync_imap.go @@ -162,6 +162,10 @@ outer: if len(deleted) > 0 { for _, name := range deleted { delete(w.flagScan, name) + // The backfill floor goes with the folder. A name is reusable, + // and a floor left behind would be inherited by whatever is + // created under it next. + w.tracker.clearFolder(name) } filtered := w.SmtpImapData.Mailboxes[:0] for _, b := range w.SmtpImapData.Mailboxes { @@ -539,54 +543,65 @@ func (w *WMail) setWalking(box *models.Mailbox) { // 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. +// A rename is only claimed when the UIDVALIDITY has exactly one folder on +// each side of it: one stored folder that is gone, and one listed folder that +// is new. Anything else is a guess. On a server that stamps UIDVALIDITY from +// a creation time a whole tree shares one number, so two stored folders can +// go missing while one arrives, and picking either would move the wrong +// folder's mail into it. Those fall through to the ordinary delete and +// first-sight paths, which lose nothing that was not already gone. func (w *WMail) imapFollowRenames(folders []models.Mailbox) error { listed := make(map[string]struct{}, len(folders)) for i := range folders { listed[folders[i].Name] = struct{}{} } + // Group both sides by UIDVALIDITY: the stored folders that are no longer + // listed, and the listed folders that are not stored. + gone := map[uint32][]*models.Mailbox{} 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 { + gone[before.UIDValidity] = append(gone[before.UIDValidity], before) + } + if len(gone) == 0 { + return nil + } + + arrived := map[uint32][]*models.Mailbox{} + for i := range folders { + f := &folders[i] + if f.UIDValidity == 0 || w.SmtpImapData.FindPair(f) != nil { continue } + arrived[f.UIDValidity] = append(arrived[f.UIDValidity], f) + } + + for uidValidity, before := range gone { + to := arrived[uidValidity] + if len(before) != 1 || len(to) != 1 { + continue + } + from := before[0] if err := w.onEvent(models.JobEventTypeMailboxRename, &models.JobEventMailboxRename{ UserID: w.UserID, EmailID: w.ID, - From: before.Name, - To: to.Name, + From: from.Name, + To: to[0].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 + if scan, ok := w.flagScan[from.Name]; ok { + delete(w.flagScan, from.Name) + w.flagScan[to[0].Name] = scan } - w.tracker.renameFolder(before.Name, to.Name) - before.Name = to.Name + w.tracker.renameFolder(from.Name, to[0].Name) + from.Name = to[0].Name } return nil } diff --git a/internal/app/worker/wmail/sync_state.go b/internal/app/worker/wmail/sync_state.go index e81c46af..32c3d069 100644 --- a/internal/app/worker/wmail/sync_state.go +++ b/internal/app/worker/wmail/sync_state.go @@ -127,6 +127,21 @@ func (t *syncTracker) setFolder(key string, c models.SyncFolderCursor) { t.dirty = true } +// clearFolder forgets a folder's backfill floor. +// +// It has to go when the folder does. A name is reusable, so a floor left +// behind is inherited by whatever is created under that name next: a "done" +// cursor skips the new folder's history entirely, and a UID floor skips +// everything below it. Neither shows up as an error, because the messages are +// not new mail either; they are simply never imported. +func (t *syncTracker) clearFolder(name string) { + if _, ok := t.state.BackfillCursor.Folders[name]; !ok { + return + } + delete(t.state.BackfillCursor.Folders, name) + 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) { diff --git a/internal/client/smtpimap/imap/folders.go b/internal/client/smtpimap/imap/folders.go index fe4931c6..57509a4a 100644 --- a/internal/client/smtpimap/imap/folders.go +++ b/internal/client/smtpimap/imap/folders.go @@ -79,6 +79,12 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { return nil, c.handleError(err) } + // Before the cap, not after: a name the server listed twice would + // otherwise spend one of the slots the cap allows and cost a real folder + // its sync, which is the same failure this whole change is about. + all, conflicts := dedupeByName(all) + c.folderConflicts.Store(int32(conflicts)) + kept, overflow := rankFolders(all, limit) c.folderOverflow.Store(int32(overflow)) @@ -110,8 +116,6 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { resp = append(resp, box) } - resp, conflicts := dedupeByName(resp) - c.folderConflicts.Store(int32(conflicts)) return resp, nil } diff --git a/internal/repository/folder_identity_live_test.go b/internal/repository/folder_identity_live_test.go index cc5dcaae..d0ea0c4d 100644 --- a/internal/repository/folder_identity_live_test.go +++ b/internal/repository/folder_identity_live_test.go @@ -155,11 +155,12 @@ func TestLiveFolderIdentityRenameMovesRowAndMail(t *testing.T) { t.Fatalf("CreateEntry(message): %v", err) } - if err := mailboxes.RenameMailbox(ctx, f.user, f.mailbox, "Clients/Acme", "Clients/Acme Corp"); err != nil { + renamed, err := mailboxes.RenameMailbox(ctx, f.user, f.mailbox, "Clients/Acme", "Clients/Acme Corp") + if 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) + if !renamed { + t.Fatal("RenameMailbox reported nothing moved") } moved, err := mailboxes.GetMailbox(ctx, f.user, f.mailbox, "Clients/Acme Corp") @@ -186,11 +187,24 @@ func TestLiveFolderIdentityRenameMovesRowAndMail(t *testing.T) { }); err != nil { t.Fatalf("CreateEntry(Globex): %v", err) } - if err := mailboxes.RenameMailbox(ctx, f.user, f.mailbox, "Clients/Globex", "Clients/Acme Corp"); err != nil { + onto, err := mailboxes.RenameMailbox(ctx, f.user, f.mailbox, "Clients/Globex", "Clients/Acme Corp") + if err != nil { t.Fatalf("RenameMailbox onto an existing name: %v", err) } + if onto { + t.Fatal("a rename onto an occupied name reported success") + } 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) } + // The mail must not have moved either: the two halves of a rename travel + // together or the messages end up in a folder nothing renamed. + stayed, err := unibox.GetByID(ctx, f.user, msg.ID) + if err != nil { + t.Fatalf("GetByID after the refused rename: %v", err) + } + if stayed.FolderPath != "Clients/Acme Corp" { + t.Errorf("message folder_path = %q; the refused rename moved mail", stayed.FolderPath) + } } diff --git a/internal/repository/pg_mailbox.go b/internal/repository/pg_mailbox.go index a069bea0..3c67cc57 100644 --- a/internal/repository/pg_mailbox.go +++ b/internal/repository/pg_mailbox.go @@ -20,11 +20,13 @@ type MailboxRepository interface { 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 + // identity sends. It reports how many rows it removed, because that + // number can now be zero for a reason worth saying out loud. Nothing else + // should reach for it. + DeleteMailboxByUIDValidity(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) (int64, error) + // RenameMailbox moves a folder and the mail filed under it to a new name, + // in one transaction. It reports whether the rename happened. + RenameMailbox(ctx context.Context, userId, emailId uuid.UUID, from, to string) (bool, error) } type mailboxRepository struct { @@ -110,27 +112,79 @@ func (r *mailboxRepository) DeleteMailbox(ctx context.Context, userId, emailId u 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`, +// DeleteMailboxByUIDValidity deletes only when the number names exactly one +// folder. +// +// It used to name exactly one by construction, because UIDVALIDITY was the +// key. It is not any more, and that is the whole point: a server that stamps +// the number from a creation time gives a folder tree one number, so an +// unguarded delete here would retire every folder in that tree over one that +// went away. The count is checked inside the statement so nothing can be +// created between counting and deleting. +func (r *mailboxRepository) DeleteMailboxByUIDValidity(ctx context.Context, userId, emailId uuid.UUID, uidValidity uint32) (int64, error) { + tag, err := r.db.Exec(ctx, + `DELETE FROM unibox_mailboxes um + WHERE um.email_id = $1 AND um.uid_validity = $2 + AND (SELECT count(*) FROM unibox_mailboxes o + WHERE o.email_id = $1 AND o.uid_validity = $2) = 1`, emailId, uidValidity, ) - return err + if err != nil { + return 0, err + } + return tag.RowsAffected(), nil } // 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. // +// The folder row and the mail filed under it move in one transaction. Apart +// they are two ways to end up half-renamed: a destination that already exists +// leaves the row where it is while the messages walk over to a folder that +// never moved, and a failure between the two strands every message under a +// name nothing points at. Neither is visible afterwards, because the result +// still looks like a folder and its mail, just not the same folder. +// // 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 +// which is not a rename. Nothing is written, and 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, +func (r *mailboxRepository) RenameMailbox(ctx context.Context, userId, emailId uuid.UUID, from, to string) (bool, error) { + if from == "" || to == "" || from == to { + return false, nil + } + + tx, err := r.db.Begin(ctx) + if err != nil { + return false, err + } + defer tx.Rollback(ctx) //nolint:errcheck + + tag, err := tx.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 + if err != nil { + return false, err + } + if tag.RowsAffected() == 0 { + // Nothing under the old name, or the new one is taken. Either way the + // mail must not move: it would land in a folder this did not rename. + return false, nil + } + + if _, err := tx.Exec(ctx, + `UPDATE unibox_emails SET folder_path = $3, updated_at = NOW() + WHERE email_id = $1 AND folder_path = $2`, + emailId, from, to, + ); err != nil { + return false, err + } + + if err := tx.Commit(ctx); err != nil { + return false, err + } + return true, nil } diff --git a/internal/repository/pg_unibox.go b/internal/repository/pg_unibox.go index 467b0637..6935c65f 100644 --- a/internal/repository/pg_unibox.go +++ b/internal/repository/pg_unibox.go @@ -27,10 +27,6 @@ type UpdateUniboxEntry struct { 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 @@ -214,18 +210,6 @@ 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