diff options
| -rw-r--r-- | src/nmraii.h | 2 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 82 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 78 |
3 files changed, 161 insertions, 1 deletions
diff --git a/src/nmraii.h b/src/nmraii.h index 1c3e8f1..925a8e7 100644 --- a/src/nmraii.h +++ b/src/nmraii.h @@ -67,3 +67,5 @@ using NmMessages = NmHandle<notmuch_messages_t, notmuch_messages_destroy>; using NmThread = NmHandle<notmuch_thread_t, notmuch_thread_destroy>; using NmMessage = NmHandle<notmuch_message_t, notmuch_message_destroy>; using NmTags = NmHandle<notmuch_tags_t, notmuch_tags_destroy>; +using NmFilenames = + NmHandle<notmuch_filenames_t, notmuch_filenames_destroy>; diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 16df4ed..f510fd5 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -184,6 +184,39 @@ void walkReplies(notmuch_messages_t *messages, int depth, } } +/// Teach the database the current filenames in one Maildir directory. +/// +/// Item 162's recovery step. mbsync renames a file to add its `,U=<uid>` infix +/// and notmuch does not learn the new name until a `notmuch new` runs; this +/// indexes just the one directory rather than waiting for that sweep. +/// +/// Deliberately NOT a full `notmuch new`: that walks the entire Maildir and +/// runs the post-new hook, which tags real mail. This must stay a read of one +/// folder with no side effects beyond the filenames it records. +/// +/// Indexing a file already known under another name ADDS a filename to the +/// same message rather than creating a second message, which is what lets the +/// caller pick the surviving path out of get_filenames(). Errors are ignored +/// on purpose: this is a best-effort repair whose caller reports the failure +/// if the path is still missing afterwards. +void reindexFolder(notmuch_database_t *db, const QString &folder) +{ + const QDir dir(folder); + if (!dir.exists()) + return; + + const QFileInfoList entries = + dir.entryInfoList(QDir::Files | QDir::NoDotAndDotDot); + for (const QFileInfo &entry : entries) { + notmuch_message_t *indexed = nullptr; + notmuch_database_index_file( + db, entry.absoluteFilePath().toUtf8().constData(), nullptr, + &indexed); + if (indexed) + notmuch_message_destroy(indexed); + } +} + /// The Maildir FOLDER a message file sits in, relative to the database root. /// /// `<root>/acct/inbox/cur/12345` becomes `acct/inbox`: the `cur`/`new` segment @@ -747,7 +780,54 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, const char *rawName = notmuch_message_get_filename(message.get()); if (!rawName) continue; - const QString from = QString::fromUtf8(rawName); + QString from = QString::fromUtf8(rawName); + + // Item 162. mbsync renames an uploaded file to record the server UID + // (`<name>,U=<uid>:2,<flags>`), and notmuch keeps the pre-`U=` name + // until that sync's `notmuch new` runs. Renaming a path that no longer + // exists fails, and Delete silently does nothing while blaming the + // destination folder for a timing problem. + // + // Refusing while a sync holds the write lock does NOT close this: + // mbsync renames throughout its run without touching notmuch's lock, + // so the damaging window is open when there is nothing to observe. + // Re-resolving is what closes it. + // + // Recovery is by MESSAGE ID, never by scanning the folder: two files + // can carry the same id, and picking the wrong one moves the wrong + // file. notmuch_message_get_filenames() lists every path the database + // holds for this id, so a file that was renamed rather than removed is + // found among them once the folder is reindexed. + if (!QFileInfo::exists(from)) { + // One reindex of the containing folder, which is what teaches + // notmuch the new name. Bounded deliberately: a single attempt, + // and a message that is still missing afterwards falls through to + // the error below, so a genuinely deleted file is still reported + // (item 41's territory) rather than becoming a silent no-op. + const QString folder = QFileInfo(from).absolutePath(); + message.reset(); + reindexFolder(db, folder); + + notmuch_message_t *again = nullptr; + if (notmuch_database_find_message(db, id.toUtf8().constData(), + &again) + == NOTMUCH_STATUS_SUCCESS + && again) { + message.reset(again); + for (NmFilenames names( + notmuch_message_get_filenames(message.get())); + notmuch_filenames_valid(names.get()); + notmuch_filenames_move_to_next(names.get())) { + const QString candidate = QString::fromUtf8( + notmuch_filenames_get(names.get())); + if (QFileInfo::exists(candidate)) { + from = candidate; + break; + } + } + } + } + // The handle is released before the file moves under it. message.reset(); diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index d02f8bd..7574ace 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -92,6 +92,8 @@ private slots: void moveMessagesReportsOnlyWhatMoved(); void moveMessagesGivesTheFileAFreshMaildirName(); void moveMessagesKeepsTheMaildirFlags(); + void moveMessagesRecoversWhenASyncRenamedTheFile(); + void moveMessagesStillReportsAMessageThatIsReallyGone(); void indexDraftFileMakesAFileFindable(); void indexDraftFileRemovesThePreviousFile(); @@ -1365,6 +1367,82 @@ void TestNotmuchWorker::moveMessagesKeepsTheMaildirFlags() QVERIFY(!name.contains(QStringLiteral(",U="))); } +void TestNotmuchWorker::moveMessagesRecoversWhenASyncRenamedTheFile() +{ + // Item 162. mbsync uploads a file and RENAMES it to record the server UID, + // and notmuch keeps the pre-`U=` name until that sync's `notmuch new` + // runs. moveMessages() then renames a path that no longer exists, reports + // "Cannot move <file> to <folder>", and silently does nothing. + // + // The ordinary fixture layout cannot see this: nothing renames a file + // underneath the index. Driving it means renaming the file WITHOUT + // reindexing, which is exactly the window mbsync opens. + const QString id = QStringLiteral("move-stale@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), id), + qPrintable(m_fixture.error())); + + const QString indexed = fileOf(id); + QVERIFY(!indexed.isEmpty()); + + // mbsync's rename, and deliberately NO m_fixture.index() afterwards: the + // database must still name the old path, which is the whole precondition. + const QString renamed = QFileInfo(indexed).absolutePath() + + QStringLiteral("/move-stale.example.org,U=7:2,D"); + QVERIFY2(QFile::rename(indexed, renamed), "could not stage the sync rename"); + + // The guard that proves this test can fail: without it, a fixture that + // quietly reindexed would make the assertions below pass against the bug. + QCOMPARE(fileOf(id), indexed); + QVERIFY2(!QFile::exists(indexed), "the stale path should no longer exist"); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy moved(&worker, &NotmuchWorker::messagesMoved); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.moveMessages({ id }, QStringLiteral("trash")); + + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + QCOMPARE(moved.size(), 1); + QCOMPARE(moved.first().at(0).toStringList(), QStringList{ id }); + + // The file really moved, and the database followed it. + const QString after = fileOf(id); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + QCOMPARE(QFileInfo(after).absolutePath(), + m_fixture.maildirPath() + QStringLiteral("/trash/cur")); + QVERIFY2(QFile::exists(after), qPrintable(after)); + QVERIFY(!QFile::exists(renamed)); + + // The `,U=` infix must not be carried across the folder boundary: that is + // what produced `Maildir error: duplicate UID` on real mail. + QVERIFY(!QFileInfo(after).fileName().contains(QStringLiteral(",U="))); +} + +void TestNotmuchWorker::moveMessagesStillReportsAMessageThatIsReallyGone() +{ + // The bounded half of the recovery above. A file that is genuinely absent, + // rather than merely renamed, must still be REPORTED: recovering silently + // from every missing path would turn a real defect into a move that + // claims success and does nothing. + const QString id = QStringLiteral("move-gone@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), id), + qPrintable(m_fixture.error())); + + const QString indexed = fileOf(id); + QVERIFY(!indexed.isEmpty()); + QVERIFY2(QFile::remove(indexed), "could not remove the file"); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy moved(&worker, &NotmuchWorker::messagesMoved); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.moveMessages({ id }, QStringLiteral("trash")); + + QCOMPARE(errors.size(), 1); + // Nothing is claimed to have moved. + QVERIFY(moved.isEmpty() || moved.first().at(0).toStringList().isEmpty()); +} + void TestNotmuchWorker::twoMessagesMovedTogetherGetDistinctNames() { // The generated name must be unique, since a collision is the entire class |