feat: address the review on the folder identity change by retiring a deleted folder's backfill floor with it, since a name is reusable and an inherited floor silently skips the next folder's history, by claiming a rename only when a UIDVALIDITY has exactly one missing folder and one new one, because two missing folders and one arrival cannot say which was renamed and guessing moves the wrong folder's mail, by deduplicating names before the folder cap rather than after so a name listed twice cannot spend a real folder's slot, by deleting on a legacy UIDVALIDITY-only event only when that number still names exactly one folder, and by moving the folder row and its mail in one transaction so a refused rename cannot leave the messages in a folder nothing renamed

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
 }

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
 }

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

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
 }

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

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
 }
 

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

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
 }

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