fix: handle every rule answer, and keep its failure out of errorOccurred

A second Apply overwrote the pending token, so the first rule's answer
was dropped as stale although its writes had landed: no undo, no pending
count, no accounts. A write's answer cannot be superseded, so the token
is gone and every ruleApplied() is handled. Pending counts are keyed by
generation for the same reason, since a second Apply before the first
count returned dropped the first Apply entirely.

applyRule() no longer emits errorOccurred(). That path reverted an
unrelated in-flight user write and its message was then overwritten by
"nothing to change". The failure travels in the reply instead:
ruleApplied(description, changes, error). openReadOnly() and writeTags()
take an optional error out-parameter so the rule's path stays quiet.
The window shows the error, still records and offers undo for whatever
landed first, and names the rule from the reply's description.

Item 212.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

4 files changed, 89 insertions, 65 deletions

diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index 918bd7b..3ee31aa 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -2827,10 +2827,8 @@ void MainWindow::onRuleCountsReady(const QVector<int> &counts,
     // Item 212. Apply's count first: it shares this signal and the generation
     // sequence, and needs no dialog, since the test and a closed dialog's
     // last Apply both arrive here without one.
-    if (m_pendingApplyGeneration != 0
-        && generation == m_pendingApplyGeneration) {
-        m_pendingApplyGeneration = 0;
-        confirmRuleApply(counts.value(0, -1));
+    if (m_pendingApplies.contains(generation)) {
+        confirmRuleApply(m_pendingApplies.take(generation), counts.value(0, -1));
         return;
     }
 
@@ -2863,12 +2861,12 @@ void MainWindow::applyRuleToExistingMail(const TagRule &rule)
     const QStringList queries{ SearchTerm::changes(rule.query, rule.add,
                                                    rule.remove) };
 
-    m_pendingApplyRule = rule;
-    m_pendingApplyGeneration = ++m_ruleCountGeneration;
+    const quint64 generation = ++m_ruleCountGeneration;
+    m_pendingApplies.insert(generation, rule);
     QMetaObject::invokeMethod(
         m_worker, "requestMessageCounts", Qt::QueuedConnection,
         Q_ARG(QStringList, queries),
-        Q_ARG(quint64, m_pendingApplyGeneration));
+        Q_ARG(quint64, generation));
 }
 
 TagCommand *MainWindow::armTagCommandForTesting(const QStringList &add,
@@ -2882,9 +2880,8 @@ TagCommand *MainWindow::armTagCommandForTesting(const QStringList &add,
     return command;
 }
 
-void MainWindow::confirmRuleApply(int count)
+void MainWindow::confirmRuleApply(const TagRule &rule, int count)
 {
-    const TagRule rule = m_pendingApplyRule;
     if (count < 0) {
         showTransientStatus(
             tr("Rule \"%1\" could not be counted, so nothing was applied.")
@@ -2929,22 +2926,18 @@ void MainWindow::confirmRuleApply(int count)
     }
 
     const QString description = tr("Apply rule \"%1\"").arg(rule.id);
-    m_pendingRuleToken = ++m_lastRuleToken;
     QMetaObject::invokeMethod(
         m_worker, "applyRule", Qt::QueuedConnection,
         Q_ARG(QString, rule.query), Q_ARG(QStringList, rule.add),
-        Q_ARG(QStringList, rule.remove), Q_ARG(QString, description),
-        Q_ARG(quint64, m_pendingRuleToken));
+        Q_ARG(QStringList, rule.remove), Q_ARG(QString, description));
 
     announceAction(tr("%1: %n message(s)", "", count).arg(description));
 }
 
-void MainWindow::onRuleApplied(quint64 token,
-                               const QVector<TagChange> &changes)
+void MainWindow::onRuleApplied(const QString &description,
+                               const QVector<TagChange> &changes,
+                               const QString &error)
 {
-    if (token == 0 || token != m_pendingRuleToken)
-        return;
-    m_pendingRuleToken = 0;
 
     // One undo entry for the whole rule, one command per operation, each
     // built from the ids that operation MOVED (item 176). The command's first
@@ -2967,15 +2960,22 @@ void MainWindow::onRuleApplied(quint64 token,
         m_undoStack.push(command);
     }
 
-    if (!anyMoved) {
-        // Another writer got there between the count and the write. The
-        // ticket bump stops announceAction() claiming "done" over this.
+    if (anyMoved)
+        m_undoStack.endMacro();
+
+    // The ticket bump stops announceAction() claiming "done" over either
+    // message below.
+    if (!error.isEmpty()) {
         ++m_announceTicket;
-        showTransientStatus(
-            tr("Rule \"%1\" has nothing to change.").arg(m_pendingApplyRule.id));
-        return;
+        // Not transient: a failed write is state the user has to read.
+        m_statusLabel->setText(tr("%1 failed: %2").arg(description, error));
+    } else if (!anyMoved) {
+        // Another writer got there between the count and the write.
+        ++m_announceTicket;
+        showTransientStatus(tr("%1: nothing to change.").arg(description));
     }
-    m_undoStack.endMacro();
+    if (!anyMoved)
+        return;
 
     // Every account, not the ones the model knows: a backfill reaches mail no
     // row shows, so its accounts cannot be resolved through the model the way
diff --git a/src/mainwindow.h b/src/mainwindow.h
index f962089..2766db2 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -864,7 +864,14 @@ private slots:
     /// meanwhile clears that queue, and the backfill's confirmation was then
     /// credited to the user's command, whose undo inverted it over thousands
     /// of messages.
-    void onRuleApplied(quint64 token, const QVector<TagChange> &changes);
+    ///
+    /// Every answer is handled: there is no token to judge one stale by,
+    /// because a write's answer cannot be superseded. Two Applies in flight
+    /// both landed, and each needs its undo and its pending count. Messages
+    /// name the rule from `description`, which the reply carries. A non-empty
+    /// `error` is shown, and whatever landed before it is still recorded.
+    void onRuleApplied(const QString &description,
+                       const QVector<TagChange> &changes, const QString &error);
 
 private:
     /// What onTagsApplied() does for any confirmed write, apart from crediting
@@ -874,7 +881,7 @@ private:
 
     /// The count for applyRuleToExistingMail() arrived: confirm, then send
     /// the rule to the worker's applyRule().
-    void confirmRuleApply(int count);
+    void confirmRuleApply(const TagRule &rule, int count);
 
     void buildUi();
 
@@ -1856,16 +1863,12 @@ private:
     /// counter.
     quint64 m_ruleCountGeneration = 0;
 
-    /// The rule awaiting its change count, and the generation that count
-    /// will carry. Shares m_ruleCountGeneration's sequence, so a pending
-    /// Apply and the dialog's Count matches can never be confused.
-    TagRule m_pendingApplyRule;
-    quint64 m_pendingApplyGeneration = 0;
-
-    /// The token the outstanding applyRule() will answer with, 0 when none.
-    /// A reply carrying any other token is stale and dropped.
-    quint64 m_pendingRuleToken = 0;
-    quint64 m_lastRuleToken = 0;
+    /// The rules awaiting their change count, keyed by the generation each
+    /// count will carry. A map, not one slot: a second Apply before the first
+    /// count returns must not make the first look stale. Shares
+    /// m_ruleCountGeneration's sequence, so an Apply and the dialog's Count
+    /// matches can never be confused.
+    QHash<quint64, TagRule> m_pendingApplies;
 
     /// The generation of a REFRESH query, run after a sync to bring the list
     /// up to date without disturbing it.
diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp
index e5f5afa..ebe4851 100644
--- a/src/notmuchworker.cpp
+++ b/src/notmuchworker.cpp
@@ -390,7 +390,7 @@ QByteArray NotmuchWorker::configPathArg() const
     return m_configPath.isEmpty() ? QByteArray() : m_configPath.toLocal8Bit();
 }
 
-bool NotmuchWorker::openReadOnly()
+bool NotmuchWorker::openReadOnly(QString *error)
 {
     if (m_db) {
         // A read-only handle is a Xapian SNAPSHOT taken when it was opened, so
@@ -407,7 +407,7 @@ bool NotmuchWorker::openReadOnly()
         // results beats refusing to answer at all.
         const notmuch_status_t status =
             notmuch_database_reopen(m_db, NOTMUCH_DATABASE_MODE_READ_ONLY);
-        if (status != NOTMUCH_STATUS_SUCCESS) {
+        if (status != NOTMUCH_STATUS_SUCCESS && !error) {
             emit errorOccurred(
                 QStringLiteral("Cannot refresh notmuch database: %1")
                     .arg(QString::fromUtf8(notmuch_status_to_string(status))));
@@ -416,19 +416,26 @@ bool NotmuchWorker::openReadOnly()
     }
 
     const QByteArray configPath = configPathArg();
-    char *error = nullptr;
+    char *openError = nullptr;
     const notmuch_status_t status = notmuch_database_open_with_config(
         nullptr,                                        // let config decide path
         NOTMUCH_DATABASE_MODE_READ_ONLY,
         configPath.isEmpty() ? nullptr : configPath.constData(),
         nullptr,
         &m_db,
-        &error);
+        &openError);
 
     if (status != NOTMUCH_STATUS_SUCCESS) {
-        emit errorOccurred(QStringLiteral("Cannot open notmuch database: %1")
-            .arg(QString::fromUtf8(error ? error : notmuch_status_to_string(status))));
-        free(error);
+        const QString message =
+            QStringLiteral("Cannot open notmuch database: %1")
+                .arg(QString::fromUtf8(openError
+                                           ? openError
+                                           : notmuch_status_to_string(status)));
+        if (error)
+            *error = message;
+        else
+            emit errorOccurred(message);
+        free(openError);
         m_db = nullptr;
         return false;
     }
@@ -1092,19 +1099,20 @@ void NotmuchWorker::applyTagsToThreads(const QStringList &threadIds,
 
 void NotmuchWorker::applyRule(const QString &query, const QStringList &add,
                               const QStringList &remove,
-                              const QString &description, quint64 token)
+                              const QString &description)
 {
     QVector<TagChange> changes;
-    if (!openReadOnly()) {
-        emit ruleApplied(token, changes);
+    QString error;
+    if (!openReadOnly(&error)) {
+        emit ruleApplied(description, changes, error);
         return;
     }
 
     QStringList messageIds;
     if (!collectMessageIds(m_db, SearchTerm::changes(query, add, remove),
                            &messageIds)) {
-        emit errorOccurred(QStringLiteral("Cannot resolve the rule's query"));
-        emit ruleApplied(token, changes);
+        emit ruleApplied(description, changes,
+                         QStringLiteral("Cannot resolve the rule's query"));
         return;
     }
 
@@ -1117,12 +1125,12 @@ void NotmuchWorker::applyRule(const QString &query, const QStringList &add,
     for (TagChange &op : ops) {
         QStringList changed;
         // An empty list writes nothing; writeTags() is not even asked to open.
-        if (!messageIds.isEmpty() && !writeTags(op, &changed))
-            break;   // errorOccurred() already emitted; report what landed
+        if (!messageIds.isEmpty() && !writeTags(op, &changed, &error))
+            break;   // report what landed before the failure, and the failure
         op.messageIds = changed;
         changes.append(op);
     }
-    emit ruleApplied(token, changes);
+    emit ruleApplied(description, changes, error);
 }
 
 void NotmuchWorker::applyTags(const TagChange &change)
@@ -1146,7 +1154,8 @@ void NotmuchWorker::applyTags(const TagChange &change)
                                 change.description });
 }
 
-bool NotmuchWorker::writeTags(const TagChange &change, QStringList *changedIds)
+bool NotmuchWorker::writeTags(const TagChange &change, QStringList *changedIds,
+                              QString *errorOut)
 {
 
     // The read-only handle must be closed first: notmuch allows only one open
@@ -1174,10 +1183,14 @@ bool NotmuchWorker::writeTags(const TagChange &change, QStringList *changedIds)
         //
         // The stall a running sync DOES cause is avoided upstream, in
         // MainWindow, by not sending the write at all while the lock is held.
-        emit errorOccurred(
+        const QString message =
             QStringLiteral("Cannot open database for writing: %1")
                 .arg(QString::fromUtf8(error ? error
-                                             : notmuch_status_to_string(status))));
+                                             : notmuch_status_to_string(status)));
+        if (errorOut)
+            *errorOut = message;
+        else
+            emit errorOccurred(message);
         free(error);
         return false;
     }
diff --git a/src/notmuchworker.h b/src/notmuchworker.h
index a175d7a..63c5203 100644
--- a/src/notmuchworker.h
+++ b/src/notmuchworker.h
@@ -246,10 +246,11 @@ public slots:
     ///
     /// Answers with ruleApplied() ALWAYS, even when nothing moved, and never
     /// with tagsApplied(): the window builds the undo from the answer rather
-    /// than from the shared confirmation queue.
+    /// than from the shared confirmation queue. Never with errorOccurred()
+    /// either: that path reverts whatever unrelated user write is in flight,
+    /// so a failure travels in the reply.
     void applyRule(const QString &query, const QStringList &add,
-                   const QStringList &remove, const QString &description,
-                   quint64 token);
+                   const QStringList &remove, const QString &description);
 
     /// Resolves whole threads to the message ids and file paths they contain.
     ///
@@ -398,10 +399,12 @@ signals:
 
     /// applyRule()'s answer: one TagChange per single-tag operation, in the
     /// order applyRule() documents, each carrying the ids that operation
-    /// actually moved (possibly none). `token` is echoed from the request.
-    /// Emitted even after a write error, carrying what was written before it,
-    /// so the window can still offer undo for that part.
-    void ruleApplied(quint64 token, const QVector<TagChange> &changes);
+    /// actually moved (possibly none). `description` is echoed from the
+    /// request, so the window names the right rule. `error` is empty on
+    /// success; on a failure, `changes` holds what was written before it, so
+    /// the window can still offer undo for that part.
+    void ruleApplied(const QString &description,
+                     const QVector<TagChange> &changes, const QString &error);
 
     /// One file entered or left the index outside a query (item 192).
     ///
@@ -497,13 +500,18 @@ signals:
     void errorOccurred(const QString &message);
 
 private:
-    bool openReadOnly();
+    /// With `error` given, a failure is written there instead of emitted, and
+    /// the non-fatal reopen failure is ignored; applyRule() reports in its
+    /// own reply.
+    bool openReadOnly(QString *error = nullptr);
     void close();
 
     /// applyTags() without the signal: writes `change` and fills `changedIds`
-    /// with the messages whose tags really moved. False, with errorOccurred()
-    /// emitted, when the database could not be opened for writing.
-    bool writeTags(const TagChange &change, QStringList *changedIds);
+    /// with the messages whose tags really moved. False when the database
+    /// could not be opened for writing, with the reason in `error` when given
+    /// and through errorOccurred() otherwise.
+    bool writeTags(const TagChange &change, QStringList *changedIds,
+                   QString *error = nullptr);
 
     QByteArray configPathArg() const;