fix(fileindex): detect .log replacement by inode identity to stop NextUID regression (#644)

helm/Chart.yaml

@@ -2,8 +2,8 @@ apiVersion: v2
 name: yarilo
 description: Production-grade IMAP/POP3/Submission mail server
 type: application
-version: 2.0.16
-appVersion: "2.1.10"
+version: 2.0.17
+appVersion: "2.1.11"
 keywords:
   - imap
   - submission

internal/storage/index/file/file.go

@@ -349,6 +349,18 @@ func (u *userIndex) compactLogIfNeeded(fs *folderState) {
 	if !needCompact {
 		return
 	}
+	// Defence in depth (#644): never flush our in-memory header as ground truth
+	// if the shared log was replaced by another process's compaction since our
+	// last reload — that header could be stale (lower NextUID) and would regress
+	// the folder's UID counter. Under the distributed mailbox lock this must not
+	// happen; if the invariant is ever broken, bail and let the next reload
+	// reconcile from the rewritten base instead. Mirrors Dovecot re-fetching the
+	// index header after a log rotation rather than trusting the earlier one.
+	if fs.logFileReplaced() {
+		slog.Warn("fileindex: skipping compaction, .log replaced since reload", "folder", fs.folder)
+		fs.closeFDs()
+		return
+	}
 	if err := fs.flush(false); err != nil {
 		slog.Warn("fileindex: log compaction flush failed", "folder", fs.folder, "err", err)
 		return
@@ -361,6 +373,38 @@ func (u *userIndex) compactLogIfNeeded(fs *folderState) {
 	fs.logSize = 0
 }
 
+// fdMatchesFile reports whether f still refers to the same on-disk file
+// (device + inode, via os.SameFile) as fi. Returns false when f is nil or
+// either stat is unavailable — an unprovable identity is treated as "not the
+// same file". Single source of the inode-identity comparison used by reload()
+// and logFileReplaced().
+func fdMatchesFile(f *os.File, fi os.FileInfo) bool {
+	if f == nil || fi == nil {
+		return false
+	}
+	st, err := f.Stat()
+	if err != nil || st == nil {
+		return false
+	}
+	return os.SameFile(fi, st)
+}
+
+// logFileReplaced reports whether the on-disk .log is a different file
+// (inode+device) than the one fs.logFD currently holds open — i.e. another
+// process replaced it through truncateLog's rename. Returns false when we hold
+// no fd yet or the path stat fails (treat as "not proven replaced"). Caller
+// must hold fs.mu.
+func (fs *folderState) logFileReplaced() bool {
+	if fs.logFD == nil {
+		return false
+	}
+	logStat, err := os.Stat(fs.indexPath + ".log")
+	if err != nil || logStat == nil {
+		return false
+	}
+	return !fdMatchesFile(fs.logFD, logStat)
+}
+
 // makeOwner builds the owner string passed to yarilo-locks BUSY
 // reports. Format mirrors the maildir / dbox / mdbox backends.
 func makeOwner(u *mailbox.UserInfo) string {

internal/storage/index/file/folder.go

@@ -460,25 +460,40 @@ func (fs *folderState) reload() error {
 	t0 := time.Now()
 	baseStat, baseErr := os.Stat(fs.indexPath)
 
-	// Use fstat on the open FD when available — avoids NFS path resolution.
+	// Stat the .log by PATH so a replacement is detected by file IDENTITY, not
+	// merely mtime+size — mirroring Dovecot's mail_index_should_recreate
+	// inode+device check. Another process's compaction replaces the log via
+	// truncateLog's .tmp+rename (new inode); a cached fs.logFD left pointing at
+	// the old, now-unlinked inode would otherwise fstat a stale size AND keep
+	// absorbing our own appends into a file nobody else sees, so this session
+	// would flush its stale (lower) header as ground truth and regress the
+	// folder's NextUID under concurrent load (#644).
+	logStat, _ := os.Stat(fs.indexPath + ".log")
 	var newLogSize int64
-	if fs.logFD != nil {
-		if st, err := fs.logFD.Stat(); err == nil {
-			newLogSize = st.Size()
-		}
-	} else {
-		if logStat, _ := os.Stat(fs.indexPath + ".log"); logStat != nil {
-			newLogSize = logStat.Size()
-		}
+	if logStat != nil {
+		newLogSize = logStat.Size()
+	}
+	logReplaced := false
+	if fs.logFD != nil && logStat != nil && !fdMatchesFile(fs.logFD, logStat) {
+		logReplaced = true
+		slog.Warn("fileindex: .log replaced under open fd, dropping stale handle",
+			"folder", fs.folder)
+		// closeFDs also drops namesFD by design: the same compaction that
+		// replaced the log rewrote the .names sidecar via saveNames (.tmp+
+		// rename), so our cached namesFD is stale too. Both reopen lazily.
+		fs.closeFDs()
 	}
 
 	var newBaseMod time.Time
 	if baseStat != nil {
 		newBaseMod = baseStat.ModTime()
 	}
 
-	// Fast path: nothing on disk changed.
-	if newBaseMod == fs.baseMod && newLogSize == fs.logSize {
+	// Fast path: nothing on disk changed. Never taken when the log was replaced
+	// out from under us — a replacement means a concurrent compaction rewrote
+	// the base too, and its new mtime may coincide with our cached one under a
+	// coarse mtime resolution or NFS attribute caching.
+	if !logReplaced && newBaseMod == fs.baseMod && newLogSize == fs.logSize {
 		slog.Debug("fileindex: reload fast-path",
 			"folder", fs.folder,
 			"log_size", fs.logSize,
@@ -498,8 +513,9 @@ func (fs *folderState) reload() error {
 		"old_base_mod", fs.baseMod.UnixNano(),
 		"dur_ms", time.Since(t0).Milliseconds())
 
-	// Base file changed (or first open) → full reload.
-	if newBaseMod != fs.baseMod || fs.file == nil {
+	// Base file changed (or first open, or the log was replaced by a concurrent
+	// compaction that also rewrote the base) → full reload.
+	if newBaseMod != fs.baseMod || fs.file == nil || logReplaced {
 		if baseErr != nil {
 			return fmt.Errorf("fileindex/reload: %w", baseErr)
 		}

internal/storage/index/file/log_replace_test.go

@@ -0,0 +1,120 @@
+package file
+
+import (
+	"os"
+	"testing"
+
+	"github.com/0kaba0hub/yarilo/pkg/mailbox"
+)
+
+// TestConcurrentCompactionNoUIDRegression reproduces #644: two pods share the
+// same on-disk index. Pod A holds an open logFD; pod B advances the folder and
+// compacts, which replaces the .log via truncateLog's rename (new inode) and
+// rewrites the base with a higher NextUID. Pod A's cached logFD is now stale
+// (points at the old, unlinked inode). Without the inode-identity check in
+// reload(), pod A would fast-path past the change, keep its low in-memory
+// NextUID, and eventually flush it back — regressing the counter. This test
+// pins that reload() drops the stale fd, reconciles the advanced NextUID, and
+// that pod A's next append lands in the live log (visible to a fresh reader).
+func TestConcurrentCompactionNoUIDRegression(t *testing.T) {
+	dir := t.TempDir()
+
+	// Pod A: create the folder and one message. Leaves a logFD open on inode I1.
+	podA := openIdx(dir, testUser)
+	fa, err := podA.OpenFolder("INBOX", 0)
+	if err != nil {
+		t.Fatalf("podA OpenFolder: %v", err)
+	}
+	ms, _ := podA.NextModSeq(fa.ID)
+	if err := podA.AppendMessage(fa.ID, &mailbox.MessageMeta{UID: 1, ModSeq: ms, Filename: "1.eml", Size: 100}); err != nil {
+		t.Fatalf("podA append UID1: %v", err)
+	}
+
+	fsA := podA.open[fa.ID]
+	if fsA.logFD == nil {
+		t.Fatal("expected podA to hold an open logFD after append")
+	}
+
+	// Pod B (separate backend, same dir): advance the folder to UID 5, then
+	// compact — flush() rewrites the base with NextUID=6 and truncateLog
+	// replaces the .log with a fresh inode I2.
+	podB := openIdx(dir, testUser)
+	fb, err := podB.OpenFolder("INBOX", 0)
+	if err != nil {
+		t.Fatalf("podB OpenFolder: %v", err)
+	}
+	for uid := uint32(2); uid <= 5; uid++ {
+		ms, _ := podB.NextModSeq(fb.ID)
+		if err := podB.AppendMessage(fb.ID, &mailbox.MessageMeta{UID: uid, ModSeq: ms, Filename: "x.eml", Size: 100}); err != nil {
+			t.Fatalf("podB append UID%d: %v", uid, err)
+		}
+	}
+	if err := podB.OptimizeIndex(fb.ID); err != nil {
+		t.Fatalf("podB compact: %v", err)
+	}
+
+	// Simulate the stat coincidence the bug depends on: pretend pod A already
+	// observed the base's current mtime (coarse mtime resolution / NFS attr
+	// cache), so ONLY the log-inode identity can reveal the change. Without the
+	// fix, reload() would fast-path here and keep NextUID=2.
+	if st, _ := os.Stat(fsA.indexPath); st != nil {
+		fsA.mu.Lock()
+		fsA.baseMod = st.ModTime()
+		fsA.mu.Unlock()
+	}
+
+	// Pod A reloads. The stale logFD (I1) must be recognised as replaced,
+	// dropped, and the advanced NextUID picked up from the rewritten base.
+	fsA.mu.Lock()
+	if err := fsA.reload(); err != nil {
+		fsA.mu.Unlock()
+		t.Fatalf("podA reload: %v", err)
+	}
+	got := fsA.file.Header.NextUID
+	stillStaleFD := fsA.logFD != nil
+	fsA.mu.Unlock()
+
+	if got < 6 {
+		t.Fatalf("NextUID regressed/stale after concurrent compaction: got %d, want >= 6", got)
+	}
+	if stillStaleFD {
+		t.Fatal("podA kept its stale logFD after the log was replaced")
+	}
+
+	// Pod A's next append must land in the live log — a fresh reader must see
+	// all six UIDs (1..5 plus the new one), proving the write did not go into
+	// the dead inode.
+	uid, err := podA.AllocateUID(fa.ID)
+	if err != nil {
+		t.Fatalf("podA AllocateUID: %v", err)
+	}
+	if uid < 6 {
+		t.Fatalf("allocated UID regressed: got %d, want >= 6", uid)
+	}
+	ms, _ = podA.NextModSeq(fa.ID)
+	if err := podA.AppendMessage(fa.ID, &mailbox.MessageMeta{UID: uid, ModSeq: ms, Filename: "new.eml", Size: 100}); err != nil {
+		t.Fatalf("podA append new UID: %v", err)
+	}
+
+	reader := openIdx(dir, testUser)
+	fr, err := reader.OpenFolder("INBOX", 0)
+	if err != nil {
+		t.Fatalf("reader OpenFolder: %v", err)
+	}
+	msgs, err := reader.GetMessages(fr.ID, mailbox.SeqSet{})
+	if err != nil {
+		t.Fatalf("reader GetMessages: %v", err)
+	}
+	seen := map[uint32]bool{}
+	for _, m := range msgs {
+		seen[m.UID] = true
+	}
+	for want := uint32(1); want <= 5; want++ {
+		if !seen[want] {
+			t.Errorf("fresh reader missing UID %d after reconcile", want)
+		}
+	}
+	if !seen[uid] {
+		t.Errorf("fresh reader missing podA's post-reconcile UID %d (append went to dead inode)", uid)
+	}
+}