fix bug in imapserver with rename of inbox, and add consistency checks

renaming inbox is special. the mailbox isn't renamed, but its messages moved to
a new mailbox. we weren't updating the destination mailbox uidnext with the new
messages. the fix not only sets the uidnext correctly, but also renumbers the
uids, starting at 1.

this also adds a consistency check for message uids and mailbox uidnexts, and
for mailbox uidvalidity account nextuidvalidity in "mox verifydata".

this also adds command "mox fixuidmeta" (not listed) that fixes up mailbox uidnext
and account uidvalidity. and command "mox reassignuids" that will renumber the
uids for either one or all mailboxes in an account.

doc.go

@@ -390,6 +390,8 @@ on-disk message file and there are no unrecognized files. If option -fix is
 specified, unrecognized message files are moved away. This may be needed after
 a restore, because messages enqueued or delivered in the future may get those
 message sequence numbers assigned and writing the message file would fail.
+Consistency of message/mailbox UID, UIDNEXT and UIDVALIDITY is verified as
+well.
 
 Because verifydata opens the database files, schema upgrades may automatically
 be applied. This can happen if you use a new mox release. It is useful to run

imapserver/rename_test.go

@@ -66,11 +66,21 @@ func TestRename(t *testing.T) {
 	tc.transactf("ok", `list "" "k*" return (subscribed)`)
 	tc.xuntagged(imapclient.UntaggedList{Separator: '/', Mailbox: "k"}, imapclient.UntaggedList{Flags: []string{"\\Subscribed"}, Separator: '/', Mailbox: "k/l"}, imapclient.UntaggedList{Separator: '/', Mailbox: "k/l/m"})
 
-	// Renaming inbox keeps inbox in existence and does not rename children.
+	// Renaming inbox keeps inbox in existence, moves messages, and does not rename children.
 	tc.transactf("ok", "create inbox/a")
+	// To check if UIDs are renumbered properly, we add UIDs 1 and 2. Expunge 1,
+	// keeping only 2. Then rename the inbox, which should renumber UID 2 in the old
+	// inbox to UID 1 in the newly created mailbox.
+	tc.transactf("ok", "append inbox (\\deleted) \" 1-Jan-2022 10:10:00 +0100\" {1+}\r\nx")
+	tc.transactf("ok", "append inbox (label1) \" 1-Jan-2022 10:10:00 +0100\" {1+}\r\nx")
+	tc.transactf("ok", `select inbox`)
+	tc.transactf("ok", "expunge")
 	tc.transactf("ok", "rename inbox minbox")
 	tc.transactf("ok", `list "" (inbox inbox/a minbox)`)
 	tc.xuntagged(imapclient.UntaggedList{Separator: '/', Mailbox: "Inbox"}, imapclient.UntaggedList{Separator: '/', Mailbox: "Inbox/a"}, imapclient.UntaggedList{Separator: '/', Mailbox: "minbox"})
+	tc.transactf("ok", `select minbox`)
+	tc.transactf("ok", `uid fetch 1:* flags`)
+	tc.xuntagged(imapclient.UntaggedFetch{Seq: 1, Attrs: []imapclient.FetchAttr{imapclient.FetchUID(1), imapclient.FetchFlags{"label1"}}})
 
 	// Renaming to new hiearchy that does not have any subscribes.
 	tc.transactf("ok", "rename minbox w/w")

imapserver/server.go

@@ -2060,9 +2060,9 @@ func (c *conn) cmdRename(tag, cmd string, p *parser) {
 			uidval, err := c.account.NextUIDValidity(tx)
 			xcheckf(err, "next uid validity")
 
-			// Inbox is very special case. Unlike other mailboxes, its children are not moved. And
+			// Inbox is very special. Unlike other mailboxes, its children are not moved. And
-			// unlike a regular move, its messages are moved to a newly created mailbox.
+			// unlike a regular move, its messages are moved to a newly created mailbox. We do
-			// We do indeed create a new destination mailbox and actually move the messages.
+			// indeed create a new destination mailbox and actually move the messages.
 			// ../rfc/9051:2101
 			if src == "Inbox" {
 				exists, err := c.account.MailboxExists(tx, dst)
@@ -2087,23 +2087,33 @@ func (c *conn) cmdRename(tag, cmd string, p *parser) {
 				err = tx.Insert(&dstMB)
 				xcheckf(err, "create new destination mailbox")
 
-				var messages []store.Message
+				// Move existing messages, with their ID's and on-disk files intact, to the new
+				// mailbox.
+				var oldUIDs []store.UID
 				q := bstore.QueryTx[store.Message](tx)
 				q.FilterNonzero(store.Message{MailboxID: srcMB.ID})
-				q.Gather(&messages)
+				q.SortAsc("UID")
-				_, err = q.UpdateNonzero(store.Message{MailboxID: dstMB.ID})
+				err = q.ForEach(func(m store.Message) error {
+					oldUIDs = append(oldUIDs, m.UID)
+					m.MailboxID = dstMB.ID
+					m.UID = dstMB.UIDNext
+					dstMB.UIDNext++
+					if err := tx.Update(&m); err != nil {
+						return fmt.Errorf("updating message to move to new mailbox: %w", err)
+					}
+					return nil
+				})
 				xcheckf(err, "moving messages from inbox to destination mailbox")
 
-				uids := make([]store.UID, len(messages))
+				err = tx.Update(&dstMB)
-				for i, m := range messages {
+				xcheckf(err, "updating uidnext in destination mailbox")
-					uids[i] = m.UID
+
-				}
 				var dstFlags []string
 				if tx.Get(&store.Subscription{Name: dstMB.Name}) == nil {
 					dstFlags = []string{`\Subscribed`}
 				}
 				changes = []store.Change{
-					store.ChangeRemoveUIDs{MailboxID: srcMB.ID, UIDs: uids},
+					store.ChangeRemoveUIDs{MailboxID: srcMB.ID, UIDs: oldUIDs},
 					store.ChangeAddMailbox{Name: dstMB.Name, Flags: dstFlags},
 					// todo: in future, we could announce all messages. no one is listening now though.
 				}

main.go

@@ -16,6 +16,7 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"time"
 
@@ -139,6 +140,8 @@ var commands = []struct {
 	{"junk test", cmdJunkTest},
 	{"junk train", cmdJunkTrain},
 	{"bumpuidvalidity", cmdBumpUIDValidity},
+	{"reassignuids", cmdReassignUIDs},
+	{"fixuidmeta", cmdFixUIDMeta},
 	{"dmarcdb addreport", cmdDMARCDBAddReport},
 	{"ensureparsed", cmdEnsureParsed},
 	{"message parse", cmdMessageParse},
@@ -2047,3 +2050,165 @@ func cmdBumpUIDValidity(c *cmd) {
 	})
 	xcheckf(err, "updating database")
 }
+
+func cmdReassignUIDs(c *cmd) {
+	c.unlisted = true
+	c.params = "account [mailboxid]"
+	c.help = `Reassign UIDs in one mailbox or all mailboxes in an account and bump UID validity, causing IMAP clients to refetch messages.
+
+Opens account database file directly. Ensure mox does not have the account
+open, or is not running.
+`
+	args := c.Parse()
+	if len(args) != 1 && len(args) != 2 {
+		c.Usage()
+	}
+
+	var mailboxID int64
+	if len(args) == 2 {
+		var err error
+		mailboxID, err = strconv.ParseInt(args[1], 10, 64)
+		xcheckf(err, "parsing mailbox id")
+	}
+
+	mustLoadConfig()
+	a, err := store.OpenAccount(args[0])
+	xcheckf(err, "open account")
+	defer func() {
+		if err := a.Close(); err != nil {
+			log.Printf("closing account: %v", err)
+		}
+	}()
+
+	// Gather the last-assigned UIDs per mailbox.
+	uidlasts := map[int64]store.UID{}
+
+	err = a.DB.Write(context.Background(), func(tx *bstore.Tx) error {
+		// Reassign UIDs, going per mailbox. We assign starting at 1, only changing the
+		// message if it isn't already at the intended UID. Doing it in this order ensures
+		// we don't get into trouble with duplicate UIDs for a mailbox.
+		q := bstore.QueryTx[store.Message](tx)
+		if len(args) == 2 {
+			q.FilterNonzero(store.Message{MailboxID: mailboxID})
+		}
+		q.SortAsc("MailboxID", "UID")
+		err := q.ForEach(func(m store.Message) error {
+			uidlasts[m.MailboxID]++
+			uid := uidlasts[m.MailboxID]
+			if m.UID != uid {
+				m.UID = uid
+				if err := tx.Update(&m); err != nil {
+					return fmt.Errorf("updating uid for message: %v", err)
+				}
+			}
+			return nil
+		})
+		if err != nil {
+			return fmt.Errorf("reading through messages: %v", err)
+		}
+
+		// Now update the uidnext and uidvalidity for each mailbox.
+		err = bstore.QueryTx[store.Mailbox](tx).ForEach(func(mb store.Mailbox) error {
+			// Assign each mailbox a completely new uidvalidity.
+			uidvalidity, err := a.NextUIDValidity(tx)
+			if err != nil {
+				return fmt.Errorf("assigning next uid validity: %v", err)
+			}
+
+			if mb.UIDValidity >= uidvalidity {
+				// This should not happen, but since we're fixing things up after a hypothetical
+				// mishap, might as well account for inconsistent uidvalidity.
+				next := store.NextUIDValidity{ID: 1, Next: mb.UIDValidity + 2}
+				if err := tx.Update(&next); err != nil {
+					log.Printf("updating nextuidvalidity: %v, continuing", err)
+				}
+				mb.UIDValidity++
+			} else {
+				mb.UIDValidity = uidvalidity
+			}
+			mb.UIDNext = uidlasts[mb.ID] + 1
+			if err := tx.Update(&mb); err != nil {
+				return fmt.Errorf("updating uidvalidity and uidnext for mailbox: %v", err)
+			}
+			return nil
+		})
+		if err != nil {
+			return fmt.Errorf("updating mailboxes: %v", err)
+		}
+		return nil
+	})
+	xcheckf(err, "updating database")
+}
+
+func cmdFixUIDMeta(c *cmd) {
+	c.unlisted = true
+	c.params = "account"
+	c.help = `Fix inconsistent UIDVALIDITY and UIDNEXT in messages/mailboxes/account.
+
+The next UID to use for a message in a mailbox should always be higher than any
+existing message UID in the mailbox. If it is not, the mailbox UIDNEXT is
+updated.
+
+Each mailbox has a UIDVALIDITY sequence number, which should always be lower
+than the per-account next UIDVALIDITY to use. If it is not, the account next
+UIDVALIDITY is updated.
+
+Opens account database file directly. Ensure mox does not have the account
+open, or is not running.
+`
+	args := c.Parse()
+	if len(args) != 1 {
+		c.Usage()
+	}
+
+	mustLoadConfig()
+	a, err := store.OpenAccount(args[0])
+	xcheckf(err, "open account")
+	defer func() {
+		if err := a.Close(); err != nil {
+			log.Printf("closing account: %v", err)
+		}
+	}()
+
+	var maxUIDValidity uint32
+
+	err = a.DB.Write(context.Background(), func(tx *bstore.Tx) error {
+		// We look at each mailbox, retrieve its max UID and compare against the mailbox
+		// UIDNEXT.
+		err := bstore.QueryTx[store.Mailbox](tx).ForEach(func(mb store.Mailbox) error {
+			if mb.UIDValidity > maxUIDValidity {
+				maxUIDValidity = mb.UIDValidity
+			}
+			m, err := bstore.QueryTx[store.Message](tx).FilterNonzero(store.Message{MailboxID: mb.ID}).SortDesc("UID").Limit(1).Get()
+			if err == bstore.ErrAbsent || err == nil && m.UID < mb.UIDNext {
+				return nil
+			} else if err != nil {
+				return fmt.Errorf("finding message with max uid in mailbox: %w", err)
+			}
+			mb.UIDNext = m.UID + 1
+			if err := tx.Update(&mb); err != nil {
+				log.Printf("fixing uidnext to %d (max uid is %d) for mailbox id %d", mb.UIDNext, m.UID, mb.ID)
+				return fmt.Errorf("updating mailbox uidnext: %v", err)
+			}
+			return nil
+		})
+		if err != nil {
+			return fmt.Errorf("processing mailboxes: %v", err)
+		}
+
+		uidvalidity := store.NextUIDValidity{ID: 1}
+		if err := tx.Get(&uidvalidity); err != nil {
+			return fmt.Errorf("reading account next uidvalidity: %v", err)
+		}
+		if maxUIDValidity >= uidvalidity.Next {
+			log.Printf("account next uidvalidity %d <= highest uidvalidity %d found in mailbox, resetting account next uidvalidity to %d", uidvalidity.Next, maxUIDValidity, maxUIDValidity+1)
+			uidvalidity.Next = maxUIDValidity + 1
+			if err := tx.Update(&uidvalidity); err != nil {
+				return fmt.Errorf("updating account next uidvalidity: %v", err)
+			}
+		}
+
+		return nil
+	})
+	xcheckf(err, "updating database")
+}

verifydata.go

@@ -34,6 +34,8 @@ on-disk message file and there are no unrecognized files. If option -fix is
 specified, unrecognized message files are moved away. This may be needed after
 a restore, because messages enqueued or delivered in the future may get those
 message sequence numbers assigned and writing the message file would fail.
+Consistency of message/mailbox UID, UIDNEXT and UIDVALIDITY is verified as
+well.
 
 Because verifydata opens the database files, schema upgrades may automatically
 be applied. This can happen if you use a new mox release. It is useful to run
@@ -218,12 +220,32 @@ possibly making them potentially no longer readable by the previous version.
 		// todo: add some kind of check for the bloom filter?
 
 		// Check that all messages in the database have a message file on disk.
+		// And check consistency of UIDs with the mailbox UIDNext, and check UIDValidity.
 		seen := map[string]struct{}{}
 		dbpath := filepath.Join(accdir, "index.db")
 		db, err := bstore.Open(ctxbg, dbpath, &bstore.Options{MustExist: true}, store.DBTypes...)
 		checkf(err, dbpath, "opening account database to check messages")
 		if err == nil {
-			err := bstore.QueryDB[store.Message](ctxbg, db).ForEach(func(m store.Message) error {
+			uidvalidity := store.NextUIDValidity{ID: 1}
+			if err := db.Get(ctxbg, &uidvalidity); err != nil {
+				checkf(err, dbpath, "missing nextuidvalidity")
+			}
+
+			mailboxUIDNexts := map[int64]store.UID{}
+			err := bstore.QueryDB[store.Mailbox](ctxbg, db).ForEach(func(mb store.Mailbox) error {
+				mailboxUIDNexts[mb.ID] = mb.UIDNext
+
+				if mb.UIDValidity >= uidvalidity.Next {
+					checkf(errors.New(`inconsistent uidvalidity for mailbox/account, see "mox fixuidmeta"`), dbpath, "mailbox id %d has uidvalidity %d >= account nextuidvalidity %d", mb.ID, mb.UIDValidity, uidvalidity.Next)
+				}
+				return nil
+			})
+			checkf(err, dbpath, "reading mailboxes to check uidnext consistency")
+
+			err = bstore.QueryDB[store.Message](ctxbg, db).ForEach(func(m store.Message) error {
+				if uidnext := mailboxUIDNexts[m.MailboxID]; m.UID >= uidnext {
+					checkf(errors.New(`inconsistent uidnext for message/mailbox, see "mox fixuidmeta"`), dbpath, "message id %d in mailbox id %d has uid %d >= mailbox uidnext %d", m.ID, m.MailboxID, m.UID, uidnext)
+				}
 				mp := store.MessagePath(m.ID)
 				seen[mp] = struct{}{}
 				p := filepath.Join(accdir, "msg", mp)