fix(delete): recover when a sync renamed the file underneath a move

Item 162. mbsync uploads a file and renames it to record the server UID
(`<name>,U=<uid>:2,<flags>`), and notmuch keeps the pre-`U=` name until
that sync's `notmuch new` runs. moveMessages() renamed a path that no
longer existed, reported "Cannot move <file> to <folder>", and skipped
the message: Delete silently did nothing while blaming the destination
folder for a timing problem.

Before renaming, check whether the recorded path still exists. If it
does not, reindex that one Maildir directory and re-read the message's
filenames by id, taking the one that is on disk.

Recovery is by MESSAGE ID rather than by scanning the folder, because
two files can carry the same id and scanning could move the wrong one.
reindexFolder() indexes a single directory and is deliberately not a
`notmuch new`, which would walk the whole Maildir and run the post-new
hook that tags real mail.

Bounded to one reindex and one retry, so a file that is genuinely gone
still reports rather than becoming a silent no-op. The second test pins
that half.

Holding the move while a sync runs was the other candidate and is not
the fix: sendMove() already refuses on the write lock (items 97 and
106), but aSyncHoldsTheWriteLock() tracks notmuch's lock, while this
window sits between mbsync's rename and that sync's `notmuch new`.
mbsync renames without touching that lock, so the damaging window is
open when there is nothing to observe. That refusal is left alone; it
does its own job.

The ordinary fixture layout cannot see this, since nothing renames a
file underneath the index. The test renames without reindexing, which
is exactly the window mbsync opens, and guards that the database still
names the old path so it cannot pass against a fixture that quietly
reindexed. Mutation-checked: disabling the recovery reproduces the
original error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UUQS6n3cmsFrsjCNmwNtf8

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