When opening an account, check for unexpected message files in the file system, and adjust the next message ID autoincrement sequence in the database to prevent future message delivery failures.

Just to be cautious. This hasn't happened yet in practice that I'm aware of.
But in theory, mox could crash after it has written the message file during
delivery, but before the database transaction was committed. In that case, a
message file for the "next message id" is already present. Any future delivery
attempts will get assigned the same message id by the database, but writing the
file will fail because there already is one, causing delivery to fail (until
the file is moved away).

When opening an account, we now check in the file system if newer files exist
than we expect based on the last existing message in the database. If so, we
adjust the message ID the database will assign next.

store/account.go

@@ -1081,7 +1081,9 @@ func OpenAccountDB(log mlog.Log, accountDir, accountName string) (a *Account, re
 	}
 
 	// Ensure singletons are present. Mailbox counts and total message size, Settings.
-	// Process pending MessageErase records.
+	// Process pending MessageErase records. Check that next the message ID assigned by
+	// the database does not already have a file on disk, or increase the sequence so
+	// it doesn't.
 	var mentioned bool
 	err = db.Write(context.TODO(), func(tx *bstore.Tx) error {
 		if tx.Get(&Settings{ID: 1}) == bstore.ErrAbsent {
@@ -1165,6 +1167,77 @@ func OpenAccountDB(log mlog.Log, accountDir, accountName string) (a *Account, re
 			}
 		}
 
+		// Ensure the message directories don't have a higher message ID than occurs in our
+		// database. If so, increase the next ID used for inserting a message to prevent
+		// clash during delivery.
+		last, err := bstore.QueryTx[Message](tx).SortDesc("ID").Limit(1).Get()
+		if err != nil && err != bstore.ErrAbsent {
+			return fmt.Errorf("querying last message: %v", err)
+		}
+
+		// We look in the directory where the message is stored (the id can be 0, which is fine).
+		maxDBID := last.ID
+		p := acc.MessagePath(maxDBID)
+		dir := filepath.Dir(p)
+		maxFSID := maxDBID
+		// We also try looking for the next directories that would be created for messages,
+		// until one doesn't exist anymore. We never delete these directories.
+		for {
+			np := acc.MessagePath(maxFSID + msgFilesPerDir)
+			ndir := filepath.Dir(np)
+			if _, err := os.Stat(ndir); err == nil {
+				maxFSID = (maxFSID + msgFilesPerDir) &^ (msgFilesPerDir - 1) // First ID for dir.
+				dir = ndir
+			} else if errors.Is(err, fs.ErrNotExist) {
+				break
+			} else {
+				return fmt.Errorf("stat next message directory %q: %v", ndir, err)
+			}
+		}
+		// Find highest numbered file within the directory.
+		entries, err := os.ReadDir(dir)
+		if err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return fmt.Errorf("read message directory %q: %v", dir, err)
+		}
+		dirFirstID := maxFSID &^ (msgFilesPerDir - 1)
+		for _, e := range entries {
+			id, err := strconv.ParseInt(e.Name(), 10, 64)
+			if err == nil && (id < dirFirstID || id >= dirFirstID+msgFilesPerDir) {
+				err = fmt.Errorf("directory %s has message id %d outside of range [%d - %d), ignoring", dir, id, dirFirstID, dirFirstID+msgFilesPerDir)
+			}
+			if err != nil {
+				p := filepath.Join(dir, e.Name())
+				log.Errorx("unrecognized file in message directory, parsing filename as number", err, slog.String("path", p))
+				maxFSID = max(maxFSID, id)
+			}
+		}
+		// Warn if we need to increase the message ID in the database.
+		var mailboxID int64
+		if maxFSID > maxDBID {
+			log.Warn("unexpected message file with higher message id than highest id in database, moving database id sequence forward to prevent clashes during future deliveries", slog.Int64("maxdbmsgid", maxDBID), slog.Int64("maxfilemsgid", maxFSID))
+
+			mb, err := bstore.QueryTx[Mailbox](tx).Limit(1).Get()
+			if err != nil {
+				return fmt.Errorf("get a mailbox: %v", err)
+			}
+			mailboxID = mb.ID
+		}
+		for maxFSID > maxDBID {
+			// Set fields that must be non-zero.
+			m := Message{
+				UID:       ^UID(0),
+				MailboxID: mailboxID,
+			}
+			// Insert and delete to increase the sequence, silly but effective.
+			if err := tx.Insert(&m); err != nil {
+				return fmt.Errorf("inserting message to increase id: %v", err)
+			}
+			if err := tx.Delete(&m); err != nil {
+				return fmt.Errorf("deleting message after increasing id: %v", err)
+			}
+			maxDBID = m.ID
+		}
+
 		return nil
 	})
 	if err != nil {
@@ -3053,6 +3126,7 @@ func OpenEmail(log mlog.Log, email string, checkLoginDisabled bool) (*Account, s
 
 // 64 characters, must be power of 2 for MessagePath
 const msgDirChars = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ-_"
+const msgFilesPerDir = 8 * 1024
 
 // MessagePath returns the filename of the on-disk filename, relative to the
 // containing directory such as <account>/msg or queue.

store/account_test.go

@@ -312,3 +312,90 @@ test
 
 	// todo: test the SMTPMailFrom and VerifiedDomains rule.
 }
+
+// Check that opening an account forwards the Message.ID used for new additions if
+// message files already exist in the file system.
+func TestNextMessageID(t *testing.T) {
+	log := mlog.New("store", nil)
+	os.RemoveAll("../testdata/store/data")
+	mox.ConfigStaticPath = filepath.FromSlash("../testdata/store/mox.conf")
+	mox.MustLoadConfig(true, false)
+
+	// Ensure account exists.
+	acc, err := OpenAccount(log, "mjl", false)
+	tcheck(t, err, "open account")
+	err = acc.Close()
+	tcheck(t, err, "closing account")
+	acc = nil
+
+	// Create file on disk to occupy the first Message.ID that would otherwise be used for deliveries..
+	msgData := []byte("a: b\r\n\r\ntest\r\n")
+	msgDir := filepath.FromSlash("../testdata/store/data/accounts/mjl/msg")
+	os.MkdirAll(filepath.Join(msgDir, "a"), 0700)
+	msgPath := filepath.Join(msgDir, "a", "1")
+	err = os.WriteFile(msgPath, msgData, 0700)
+	tcheck(t, err, "write message file")
+
+	msgPathBogus := filepath.Join(msgDir, "a", "bogus")
+	err = os.WriteFile(msgPathBogus, []byte("test"), 0700)
+	msgPathBadID := filepath.Join(msgDir, "a", "10000") // Out of range.
+	err = os.WriteFile(msgPathBadID, []byte("test"), 0700)
+
+	// Open account. This should increase the next message ID.
+	acc, err = OpenAccount(log, "mjl", false)
+	tcheck(t, err, "open account")
+	defer Switchboard()()
+
+	// Deliver a message. It should get ID 2.
+	mf, err := CreateMessageTemp(log, "account-test")
+	tcheck(t, err, "creating temp message file")
+	_, err = mf.Write(msgData)
+	tcheck(t, err, "write file")
+	defer CloseRemoveTempFile(log, mf, "temp message file")
+	m := Message{
+		Size: int64(len(msgData)),
+	}
+	err = acc.DeliverMailbox(log, "Inbox", &m, mf)
+	tcheck(t, err, "deliver mailbox")
+	if m.ID != 2 {
+		t.Fatalf("got message id %d, expected 2", m.ID)
+	}
+
+	// Ensure account consistency check won't complain.
+	err = os.Remove(msgPath)
+	tcheck(t, err, "removing message path")
+	err = os.Remove(msgPathBogus)
+	tcheck(t, err, "removing message path")
+	err = os.Remove(msgPathBadID)
+	tcheck(t, err, "removing message path")
+
+	err = acc.Close()
+	tcheck(t, err, "closing account")
+
+	// Try again, but also create next message directory, but no file.
+	os.MkdirAll(filepath.Join(msgDir, "b"), 0700)
+	os.MkdirAll(filepath.Join(msgDir, "d"), 0700) // Not used.
+
+	// Open account again, increasing next message ID.
+	acc, err = OpenAccount(log, "mjl", false)
+	tcheck(t, err, "open account")
+
+	// Deliver a message. It should get ID 8*1024+1.
+	mf, err = CreateMessageTemp(log, "account-test")
+	tcheck(t, err, "creating temp message file")
+	_, err = mf.Write(msgData)
+	tcheck(t, err, "write file")
+	defer CloseRemoveTempFile(log, mf, "temp message file")
+	m = Message{
+		Size: int64(len(msgData)),
+	}
+	err = acc.DeliverMailbox(log, "Inbox", &m, mf)
+	tcheck(t, err, "deliver mailbox")
+	if m.ID != 8*1024+1 {
+		t.Fatalf("got message id %d, expected 8*1024+1", m.ID)
+	}
+
+	err = acc.Close()
+	tcheck(t, err, "closing account")
+	acc.WaitClosed()
+}