OpenFolder's unlocked existence-check-then-createFresh races (trigger 3 for #644 NextUID regression)

[0kaba0hub]

Summary

Third, distinct trigger for the #644 NextUID-regression class of bug (RFC 3501 UID-monotonicity violation, mdbox/sdbox, report/imap objectid smoketest symptom). #645 fixed the stale-logFD/inode-mismatch trigger; #647/#655 fixed withFolderRO's unlocked reload racing a locked compaction. Neither covers this one — confirmed live on latest (post-#655), reproducing with the identical signature.

Revised understanding (this update): no cross-process activity is required at all. Isolated reproduction (single yarilo-smoketest instance, no imaptest, no other smoketest pods, freshly wiped mailbox) fails identically on both mdbox (u1) and sdbox (u101). In both isolated runs, every createFresh resetting NextUID to 1 WARN has service=imap — yarilo-lmtp never calls createFresh in either run. The race is between two concurrent connections/goroutines within the single yarilo-imap process itself, both racing OpenFolder's unlocked existence-check for the same newly-created folder — not a race between separate yarilo-imap/yarilo-lmtp processes as originally theorized.

Root cause (code-verified)

OpenFolder (internal/storage/index/file/folder.go:43) does its existence check and loadOrInit/createFresh call before any per-folder lock exists:

func (u *userIndex) OpenFolder(folder string, uidValidity uint32) (*mailbox.Folder, error) {
	indexDir := u.indexDir(folder)
	indexPath := indexPathFor(indexDir)

	u.mu.Lock()
	if u.byDir != nil {
		if id, ok := u.byDir[indexDir]; ok {
			// dedup hit — goes through withFolderRO, now correctly locked (#655)
			...
		}
	}
	u.next++
	id := u.next
	u.mu.Unlock()

	if err := os.MkdirAll(indexDir, 0o700); err != nil { ... }
	if err := migrateLegacyFilenames(indexDir); err != nil { ... }

	names, sizes := loadNames(indexDir)
	fs := &folderState{ ... }
	if err := u.loadOrInit(fs, uidValidity); err != nil { ... }  // <- os.Stat + createFresh, UNLOCKED

	u.mu.Lock()
	u.open[id] = fs
	u.byDir[indexDir] = id
	u.mu.Unlock()
	return fs.snapshot(id)
}

u.mu only guards the dedup map (u.byDir/u.open), and it's released before loadOrInit runs. loadOrInit calls os.Stat(fs.indexPath); on ErrNotExist it calls createFresh, which unconditionally sets NextUID = 1 and does an immediate flush(true) (a full base-file write).

For a folder that genuinely does not exist yet on disk (e.g. right after a test-harness wipe, or on first-ever delivery to a new sieve-fileinto target), any two callers reaching OpenFolder for the same (user, folder) before either has created anything will race. This includes two separate processes (yarilo-imap/yarilo-lmtp), but — as now confirmed — it does not require that: two concurrent IMAP connections handled by the same yarilo-imap process (e.g. smoketest's own parallel per-test connections) are sufficient. Both os.Stat calls return ErrNotExist, both call createFresh, both flush(true). Whichever's flush lands last silently overwrites the other's already-progressed state with a fresh NextUID=1 — no error on either side, since both believe they are legitimately creating a brand-new folder.

Unlike #647 (an unlocked read racing a locked write), this is two unlocked writes (both via createFresh) racing each other — #655's fix (locking withFolderRO's reload) does not touch OpenFolder's first-open path at all, so it structurally cannot affect this trigger.

Reproduction — isolated, single smoketest instance, no concurrent load battery

Both runs: mailbox+index fully wiped immediately before, imaptest job deleted, no other smoketest pods running. yarilo:latest (post-#655/#653).

mdbox (u1@d00001.test)

createFresh fired 13 times for distinct folders over the ~57s run, service=imap in every case, caller=userIndex.loadOrInit. Two folders got it more than once:

• INBOX: 09:41:45.877 and again 09:42:37.019 (52s apart — second one landed well after LMTP had been delivering to it)
• sieve-test-mboxid: 09:42:39.178 and 09:42:40.715 (1.4s apart — a tight double-fire, the clearest single-process race window)

No createFresh from service=lmtp anywhere in the run.

sdbox (u101@d00001.test)

Same pattern. Direct UID-regression capture:

09:56:46.750  lmtp  fileindex: uid allocated   folder=INBOX uid=9
09:56:46.916  imap  fileindex: createFresh resetting NextUID to 1   folder=INBOX  caller=loadOrInit
09:56:53.619  lmtp  fileindex: uid allocated   folder=INBOX uid=1   <- regression, 6.7s later

And again a tight double-fire on the same folder from the same service:

09:56:49.092  imap  createFresh resetting NextUID to 1   folder=sieve-test-mboxid
09:56:50.685  imap  createFresh resetting NextUID to 1   folder=sieve-test-mboxid   <- 1.4s later, same folder, same service

No createFresh from service=lmtp in this run either.

smoketest fails identically in both runs:

sieve: FAIL  n=23/24  test=report          err="no valid ARF report delivered back to INBOX within timeout"
sieve: FAIL  n=24/24  test="imap objectid" err="delivered message not found"

Fix

OpenFolder's existence-check-then-createFresh sequence needs the same cross-process (and, as now shown, cross-connection) serialization the write path already has, extended to cover first-ever creation — not just reload of an already-open folder (which #655 covers). Candidates, roughly in order of preference:

1. Acquire the cross-process lock (u.b.locker, locks.MailboxKey(...)) around the os.Stat + loadOrInit + registration sequence in OpenFolder, released only after u.open[id]/u.byDir[indexDir] are populated — mirroring how withFolderLock already serializes established-folder operations. This also naturally fixes the in-process case, since the lock is held regardless of which goroutine/connection is asking.
2. Alternatively, make the initial createFresh → flush(true) sequence detect-and-reconcile a lost race at the filesystem level (e.g. the same inode-identity technique fix(fileindex): detect .log replacement by inode identity to stop NextUID regression (#644) #645 introduced): if the base file already exists by the time this process's own flush(true) is about to write, reload-and-merge instead of blindly overwriting.

Option 1 is preferred: it is a direct structural fix, and since the race turns out to be in-process as often as cross-process, an in-process-only fix (e.g. a per-indexDir in-memory mutex) would only mask the cross-process case and vice versa — the existing cross-process lock is the single mechanism that covers both.

Related

• fileindex: log compaction can persist a stale NextUID, regressing the folder's UID counter under concurrent load #644 — tracking issue for the NextUID-regression symptom class.
• fix(fileindex): detect .log replacement by inode identity to stop NextUID regression (#644) #645 — fixed trigger 1 (stale logFD surviving truncateLog's rename).
• fileindex: withFolderRO reloads without the cross-process lock — can race a locked compaction and poison shared in-memory header (NextUID regression persists after #645) #647 / fix(fileindex): serialize withFolderRO's reload against writers via the cross-process lock (#647) #655 — fixed trigger 2 (withFolderRO's reload unserialized against a locked writer).
• This issue — trigger 3 (unlocked OpenFolder first-creation race), confirmed active after both prior fixes, and confirmed to reproduce with no concurrent-load battery required — a single isolated yarilo-smoketest run against one mailbox backend is sufficient.