fix(fileindex): detect .log replacement by inode identity to stop NextUID regression (#644)#645
Merged
Merged
Conversation
…tUID regression (#644) Under concurrent load a folder's NextUID could regress mid-life (e.g. 24 -> 1) while UIDValidity stayed unchanged, an RFC 3501 UID-monotonicity violation reproducible only on mdbox/sdbox and the real root cause behind the report/imap-objectid smoketest failures previously misattributed to the FTS checkpoint (#638, a distinct real bug). Root cause: truncateLog replaces the shared .log via .tmp+rename (new inode), but appendMutLog caches fs.logFD and reload()'s fast path fstats that cached fd. After another process compacts, a session holding the old fd keeps writing appends into the now-unlinked inode (invisible to everyone else) and keeps fstatting it in reload's gate, so its in-memory header stays stale. When that session later compacts, flush() persists its stale, lower NextUID as ground truth, discarding the advances other sessions made. Fix, mirroring Dovecot's mail_index_should_recreate inode+device check: - reload() stats the .log by path and compares file identity (os.SameFile) against the cached fd; on replacement it drops the stale handle and forces a full base reload (a replacement implies a concurrent compaction rewrote the base, whose new mtime may coincide with the cached one under coarse mtime resolution / NFS attribute caching). - compactLogIfNeeded bails out (defence in depth) if the log was replaced since the last reload, so a stale header is never flushed as ground truth. Adds TestConcurrentCompactionNoUIDRegression: two backends over one dir, one compacts (inode swap), the other must reconcile the advanced NextUID, drop its stale fd, and land its next append in the live log.
…amesFD drop Address non-blocking review notes on #645: - Extract fdMatchesFile so the path-stat + fd-stat + os.SameFile comparison has a single source, used by both reload() and logFileReplaced(). - Document that dropping namesFD alongside logFD in reload() is intentional: the compaction that replaced the log also rewrote the .names sidecar via saveNames (.tmp+rename), so the cached namesFD is stale too. logFileReplaced()'s guard in compactLogIfNeeded stays as deliberate defense-in-depth (mirrors Dovecot's post-rotation header re-fetch).
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #644.
Problem
Under concurrent load a folder's
NextUIDcounter regressed mid-life (e.g. 24 → 1) whileUIDValiditystayed unchanged — an RFC 3501 UID-monotonicity violation, reproducible only on mdbox/sdbox, and the actual root cause behind thereport/imap objectidsmoketest failures previously misattributed to the FTS checkpoint (#638 was a real but distinct bug).Root cause (verified against code)
truncateLogreplaces the shared.logvia.tmp+os.Rename→ new inode. ButappendMutLogcachesfs.logFD(opens only when nil) andreload()'s fast path fstats that cached fd. After another process compacts:flush()persists its stale, lowerNextUIDas ground truth, discarding the real advances.This is a cross-pod hazard (each pod has its own
folderState+logFD); in-pod sessions share one state and serialise onfs.mu.Fix — mirrors Dovecot's
mail_index_should_recreateinode+device checkreload()stats the.logby path and compares file identity (os.SameFile) against the cached fd. On replacement it drops the stale handle (closeFDs) and forces a full base reload — a replacement implies a concurrent compaction rewrote the base, whose new mtime may coincide with the cached one under coarse mtime resolution or NFS attribute caching.compactLogIfNeededbails out (defence in depth, matching Dovecot re-fetching the header after log rotation) if the log was replaced since the last reload, so a stale header is never flushed as ground truth.Test
TestConcurrentCompactionNoUIDRegression: two backends over one dir; pod B compacts (inode swap + higher NextUID), pod A must reconcile the advanced NextUID, drop its stale fd, and land its next append in the live log (verified by a fresh reader seeing all UIDs).go test ./...+-tags flatcurvebuild +helm lintall green. appVersion → 2.1.11.