diff --git a/helm/Chart.yaml b/helm/Chart.yaml index dd675ecc..77436ff9 100644 --- a/helm/Chart.yaml +++ b/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 diff --git a/internal/storage/index/file/file.go b/internal/storage/index/file/file.go index 60598003..2e178822 100644 --- a/internal/storage/index/file/file.go +++ b/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 { diff --git a/internal/storage/index/file/folder.go b/internal/storage/index/file/folder.go index 26ad8608..ef8be3e1 100644 --- a/internal/storage/index/file/folder.go +++ b/internal/storage/index/file/folder.go @@ -460,16 +460,28 @@ 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 @@ -477,8 +489,11 @@ func (fs *folderState) reload() error { 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) } diff --git a/internal/storage/index/file/log_replace_test.go b/internal/storage/index/file/log_replace_test.go new file mode 100644 index 00000000..3c67e8cd --- /dev/null +++ b/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) + } +}