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 --- tests/test_notmuchworker.cpp | 143 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 140 insertions(+), 3 deletions(-) (limited to 'tests/test_notmuchworker.cpp') 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