From 41d0b94dcd2b4208e5d3e7483c88ce9b664ce565 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Wed, 19 Aug 2026 11:10:51 +0200 Subject: fix(worker): give a moved message a fresh maildir name mbsync's manual, under "the more efficient default UID mapping scheme": "it is important that the MUA renames files when moving them between Maildir folders", and "the general expectation is that a completely new filename is generated as if the message was new". qtmaildir is that MUA and did not rename. moveMessages() kept QFileInfo(from).fileName() verbatim, `,U=` included. That infix is mbsync's per-folder IMAP UID, so carrying it across a folder boundary makes it a claim about a folder the file is no longer in; moving a message out and back then reinserts a UID the server has since reassigned. Reported by the user as `Maildir error: duplicate UID 1`, and measured on the real Maildir: four collisions in one folder, eight distinct messages, none lost. freshMaildirName() regenerates the unique part and keeps ONLY the `:2,` suffix. Keeping the flags is not a contradiction of "as if the message was new": they record seen, flagged and replied, and maildir.synchronize_flags is true, so dropping them would mark every deleted message unread and lose Important on the way to the trash. Two things fell out of the change and both were defects waiting to happen. The already-in-the-destination guard compared full PATHS, which worked only because the name was carried across; with a fresh name it can never be true, so a message already in the destination would be renamed on every move. It compares directories now. And test_mainwindow's folderHasMessageFile() matched on the filename stem, so all fifty-odd assertions using it began reporting "the file is not there" about files that were there. It reads the Message-ID out of each file instead, which is what those assertions always meant. Three mutations fail: the old name carried across, the flags dropped, and the uniqueness counter frozen so two messages moved in one batch collide. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 22 +++++++ src/notmuchworker.cpp | 77 +++++++++++++++++++++-- tests/test_mainwindow.cpp | 40 +++++++++++- tests/test_notmuchworker.cpp | 143 ++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 273 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d17b46c..8ddbe50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,28 @@ point at which they are stable. ## [Unreleased] +### Fixed + +- Moving a message between folders now gives its file a fresh Maildir name. + 0.26.0 carried the old name across, including mbsync's `,U=` UID infix, + which belongs to the folder the file came from. Moving a message out and + back reinserted a UID the server had since reassigned, and mbsync refused + the folder with `Maildir error: duplicate UID`. If you saw that error, see + Upgrading below. + +### Upgrading + +If a sync reported `Maildir error: duplicate UID in ` after +deleting or restoring mail with 0.26.0, that folder holds two files claiming +one UID. No mail is lost; mbsync simply refuses to sync the folder until it is +resolved. Stop any running sync, then strip the `,U=` infix from the newer +of each pair and reindex: + + notmuch new + +mbsync re-derives the UID on the next sync. The code no longer creates this +state. + ### Added - The message pane's right-click menu offers **Select all**. Chromium's own menu diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 2c03226..66b408c 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -20,9 +20,12 @@ #include +#include +#include #include #include #include +#include #include #include @@ -662,6 +665,63 @@ void NotmuchWorker::applyTags(const TagChange &change) emit tagsApplied(change); } +namespace { + +/// A fresh Maildir filename for a message being moved between folders, +/// preserving only its `:2,` suffix. +/// +/// mbsync's manual is explicit about why this exists, under "the more +/// efficient default UID mapping scheme": "it is important that the MUA +/// renames files when moving them between Maildir folders", and "the general +/// expectation is that a completely new filename is generated as if the +/// message was new". +/// +/// The `,U=` infix mbsync writes is its per-folder IMAP UID. Carrying it +/// into another folder makes it a claim about a folder the file is no longer +/// in; moving a message out and back then reinserts a UID the server has +/// since reassigned, and mbsync refuses the folder with `Maildir error: +/// duplicate UID`. Measured on real mail, four collisions in one folder from +/// a single move-and-restore. +/// +/// The FLAGS are kept, deliberately, and that is not a contradiction of +/// "as if the message was new". They record seen, flagged and replied, and +/// `maildir.synchronize_flags` is true, so notmuch reads them back as tags: +/// dropping them would mark every deleted message unread and lose Important +/// on the way to the trash. Only the unique part is regenerated. +QString freshMaildirName(const QString &oldName) +{ + // The `:2,` suffix, when there is one. `info` is everything from the + // separator on, so an empty-flag `:2,` is preserved as faithfully as + // `:2,FS`. + QString info; + const int sep = oldName.indexOf(QStringLiteral(":2,")); + if (sep >= 0) + info = oldName.mid(sep); + + // The conventional left-to-right unique part: time, a per-process counter, + // the pid, the host. The counter is what makes two messages moved in the + // same second distinct, which a timestamp alone does not guarantee. + static quint64 counter = 0; + const qint64 now = QDateTime::currentSecsSinceEpoch(); + const QString host = QHostInfo::localHostName().isEmpty() + ? QStringLiteral("localhost") + : QHostInfo::localHostName(); + + return QStringLiteral("%1.M%2P%3Q%4.%5%6") + .arg(now) + .arg(QDateTime::currentMSecsSinceEpoch() % 1000) + .arg(QCoreApplication::applicationPid()) + .arg(++counter) + // A `/` or a `:` in a hostname would break the path or the flag + // separator. Neither is legal in a hostname, so this is belt and + // braces rather than a known case. + .arg(QString(host).replace(QLatin1Char('/'), QLatin1Char('_')) + .replace(QLatin1Char(':'), QLatin1Char('_'))) + .arg(info); +} + +} // namespace + void NotmuchWorker::moveMessages(const QStringList &messageIds, const QString &destFolder) { @@ -728,15 +788,24 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, continue; } - const QString to = destDir + QLatin1Char('/') + QFileInfo(from).fileName(); - if (from == to) { - // Already where it was asked to go. Reported as moved, since the - // caller's request is satisfied. + // Already where it was asked to go, compared on the DIRECTORY rather + // than on the full path. It used to compare paths, which worked only + // because the filename was carried across unchanged; with a fresh name + // that test can never be true, so a message already in the destination + // would be renamed on every move for no reason, and every rename is a + // new filename mbsync has to reconcile. + if (QFileInfo(from).absolutePath() == QFileInfo(destDir).absoluteFilePath()) { moved.append(id); origins.insert(id, origin); continue; } + // A FRESH name, never the old one. See freshMaildirName(): carrying + // the `,U=` infix across a folder boundary is what produced + // `Maildir error: duplicate UID` on real mail. + const QString to = destDir + QLatin1Char('/') + + freshMaildirName(QFileInfo(from).fileName()); + if (!QFile::rename(from, to)) { emit errorOccurred(QStringLiteral("Cannot move %1 to %2") .arg(QFileInfo(from).fileName(), destFolder)); diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 79e137a..0deaec3 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -8907,15 +8907,51 @@ static bool notmuchTag(const QString &configPath, const QStringList &args) return process.waitForFinished(15000) && process.exitCode() == 0; } +/// Whether `dir` holds the message whose id the fixture wrote as `stem`. +/// +/// Matches on the MESSAGE-ID INSIDE each file, never on the filename. It used +/// to compare filenames, which worked only while a move carried the name +/// across unchanged. It no longer does: mbsync requires an MUA to rename a +/// file when it moves it between folders, so `NotmuchWorker::moveMessages()` +/// generates a fresh name and this helper could never find a moved message +/// again. Every one of these assertions failed at once, each reporting "the +/// file is not there" about a file that was. +/// +/// `stem` stays in the fixture's `.example.org` form so the fifty-odd +/// call sites did not have to change; it is turned back into +/// `` here. static bool folderHasMessageFile(const QString &dir, const QString &stem) { QDir directory(dir); if (!directory.exists()) return false; + + // `del1.example.org` is the fixture's rendering of `del1@example.org`: + // it replaces the `@` to make a filename-safe stem. Only the LAST dot + // before the domain is the substituted one, so the split is on the first + // dot, which is where the local part ends for every id these tests use. + const int dot = stem.indexOf(QLatin1Char('.')); + if (dot < 0) + return false; + const QString messageId = QStringLiteral("<%1@%2>") + .arg(stem.left(dot), stem.mid(dot + 1)); + const QStringList entries = directory.entryList(QDir::Files); for (const QString &entry : entries) { - if (entry == stem || entry.startsWith(stem + QLatin1Char(':'))) - return true; + QFile file(directory.filePath(entry)); + if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) + continue; + // The header block only: a quoted id in a body must not count. + while (!file.atEnd()) { + const QByteArray line = file.readLine(); + if (line.trimmed().isEmpty()) + break; + if (line.startsWith("Message-ID:") || line.startsWith("Message-Id:")) { + if (QString::fromUtf8(line).contains(messageId)) + return true; + break; + } + } } return false; } diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index bf1c23c..b5da31a 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -90,6 +90,9 @@ private slots: void moveMessagesReindexesAtTheNewPath(); void moveMessagesKeepsTheMessagesTags(); void moveMessagesReportsOnlyWhatMoved(); + void moveMessagesGivesTheFileAFreshMaildirName(); + void moveMessagesKeepsTheMaildirFlags(); + void twoMessagesMovedTogetherGetDistinctNames(); private: /// Adds one read message in `folder` and reindexes, for the move tests. @@ -1160,9 +1163,19 @@ void TestNotmuchWorker::moveMessagesRelocatesTheFile() // cur/, never new/: a file in new/ is re-announced as fresh mail by every // reader of the Maildir. - const QString expected = m_fixture.maildirPath() + QStringLiteral("/trash/cur/") - + QFileInfo(before).fileName(); - QVERIFY2(QFile::exists(expected), qPrintable(expected)); + // + // Asserted on the DIRECTORY, not on the full path. The filename is + // deliberately regenerated by the move (see + // moveMessagesGivesTheFileAFreshMaildirName), so an assertion naming the + // old filename here encoded the very bug that item fixes: it required the + // name to be carried across, which is what produced duplicate mbsync UIDs + // on real mail. + const QString expectedDir = + m_fixture.maildirPath() + QStringLiteral("/trash/cur"); + const QString after = fileOf(id); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + QCOMPARE(QFileInfo(after).absolutePath(), expectedDir); + QVERIFY2(QFile::exists(after), qPrintable(after)); QVERIFY(!QFile::exists(before)); } @@ -1217,6 +1230,130 @@ void TestNotmuchWorker::moveMessagesKeepsTheMessagesTags() .arg(after.join(QLatin1Char(' '))))); } +void TestNotmuchWorker::moveMessagesGivesTheFileAFreshMaildirName() +{ + // mbsync's manual, under "the more efficient default UID mapping scheme": + // "it is important that the MUA renames files when moving them between + // Maildir folders", and "the general expectation is that a completely new + // filename is generated as if the message was new". + // + // qtmaildir is that MUA and did not rename. The `,U=` infix is mbsync's + // per-folder IMAP UID, so carrying it into another folder makes it a claim + // about a folder the file is no longer in. Moving a message out and back + // then reinserts a UID the server has since reassigned, and mbsync reports + // `Maildir error: duplicate UID`. + // + // Measured on the user's real Maildir on 2026-08-19: four collisions in one + // folder, eight distinct messages, from a move-out and restore made with + // 0.26.0. + const QString id = QStringLiteral("move5@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), id), + qPrintable(m_fixture.error())); + + // A realistic mbsync name, which the fixture does not produce on its own: + // ,U=:2,. + const QString before = fileOf(id); + QVERIFY(!before.isEmpty()); + const QString uidName = QFileInfo(before).absolutePath() + + QStringLiteral("/move5.example.org,U=42:2,S"); + QVERIFY2(QFile::rename(before, uidName), "could not stage a ,U= filename"); + QVERIFY(m_fixture.index()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.moveMessages({ id }, QStringLiteral("trash")); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + const QString after = fileOf(id); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + QVERIFY(QFile::exists(after)); + + const QString name = QFileInfo(after).fileName(); + // The point of the item: no UID infix survives the move. + QVERIFY2(!name.contains(QStringLiteral(",U=")), + qPrintable(QStringLiteral("the moved file kept a UID infix: %1") + .arg(name))); + // And it is a genuinely new name rather than the old one with the infix + // cut out, which is what mbsync calls the expectation. + QVERIFY2(name != QStringLiteral("move5.example.org:2,S"), + qPrintable(QStringLiteral("the name was only stripped, not " + "regenerated: %1").arg(name))); +} + +void TestNotmuchWorker::moveMessagesKeepsTheMaildirFlags() +{ + // A fresh name must NOT mean fresh state. The `:2,` suffix carries + // seen, flagged and replied, and notmuch's maildir.synchronize_flags is + // true on the user's setup, so dropping it would mark read mail unread and + // lose Important on every message the user deletes. + // + // Asserted on the FLAGS rather than on the whole name, since the unique + // part is expected to change and the flags are expected not to. + const QString id = QStringLiteral("move6@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), id), + qPrintable(m_fixture.error())); + + const QString before = fileOf(id); + QVERIFY(!before.isEmpty()); + // Seen and Flagged, so a suffix that is dropped or truncated shows up. + const QString staged = QFileInfo(before).absolutePath() + + QStringLiteral("/move6.example.org,U=7:2,FS"); + QVERIFY2(QFile::rename(before, staged), "could not stage a flagged filename"); + QVERIFY(m_fixture.index()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.moveMessages({ id }, QStringLiteral("trash")); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + const QString after = fileOf(id); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + const QString name = QFileInfo(after).fileName(); + + QVERIFY2(name.endsWith(QStringLiteral(":2,FS")), + qPrintable(QStringLiteral("the move lost the maildir flags: %1") + .arg(name))); + QVERIFY(!name.contains(QStringLiteral(",U="))); +} + +void TestNotmuchWorker::twoMessagesMovedTogetherGetDistinctNames() +{ + // The generated name must be unique, since a collision is the entire class + // of bug this change exists to remove: two files landing on one name means + // one message silently overwrites the other. + // + // Two messages in ONE batch, which is the case a timestamp alone does not + // cover: both are moved in the same second, so only the per-process counter + // separates them. A generator using time and pid alone passes every other + // test here and fails this one. + const QString first = QStringLiteral("move7@example.org"); + const QString second = QStringLiteral("move8@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), first), + qPrintable(m_fixture.error())); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), second), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.moveMessages({ first, second }, QStringLiteral("trash")); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + const QString a = fileOf(first); + const QString b = fileOf(second); + QVERIFY2(!a.isEmpty() && !b.isEmpty(), + "a message is missing from the database after the move"); + + // Distinct names... + QVERIFY2(QFileInfo(a).fileName() != QFileInfo(b).fileName(), + qPrintable(QStringLiteral("both messages were named %1") + .arg(QFileInfo(a).fileName()))); + // ...and both files really are on disk, which is what a collision would + // have destroyed. The name check alone would pass against one file that + // overwrote the other if the database still named two paths. + QVERIFY(QFile::exists(a)); + QVERIFY(QFile::exists(b)); +} + void TestNotmuchWorker::moveMessagesReportsOnlyWhatMoved() { // A stale id must not abort the batch, and must not be reported as moved -- cgit v1.2.3