fix(sync): clear the pending-edit count on a cron sync

Item 54. A sync fired by the user's cron carries tag edits to the mail
store exactly as a local one does, but only onSyncFinished() cleared the
pending state, so the indicator kept reporting work that had already
shipped and the exit prompt asked to sync for it. Verified against a
real cron run: 31 changes, cleared with no manual sync.

The window cannot see an external run's exit status, and /proc/locks
carries no outcome. It does not need to: mailsync.sh already ends every
run with a "RUN END ... status=OK" banner in its log, which outlives the
process that wrote it. MailSync::lastRunOutcome() reads a bounded tail
of that file and takes the last completed marker, so no change to the
script and no optimistic guessing were needed.

Only a definite OK clears anything. A failed run, a missing or
unreadable log, and a State::Unknown lock reading all leave the count
alone: over-reporting costs a redundant sync, under-reporting costs the
user their edits.

m_editedAccounts is drained in the same place, before flushHeldEdits()
and matching the local path's ordering. Item 49 uses it to choose which
mbsync channels a run syncs, and a count that reached zero while the set
stayed full would look correct and still sync the wrong channels.

The log path comes from a new optional [sync] log key, defaulting to
where the script writes, so a test never reads the developer's own log.

Two notes on the verification, both recorded in the backlog:

  - A timing probe endorsed a tail read that was not happening. The
    first version of the huge-log test required the call under 100 ms
    and passed with the seek deleted, because reading 10 MB is fast
    either way. Replaced with an assertion on content.

  - Every fixture was invented and the first batch had the wrong
    timestamp format, since the script uses date -Iseconds. The tests
    passed anyway, because the parser keys on the prefix and the status
    token. One test now builds the banner the way the script does.

The before-flushHeldEdits ordering has no test: without a held lock the
flush is a no-op, so both orderings pass.

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

6 files changed, 139 insertions, 2 deletions

diff --git a/src/config.cpp b/src/config.cpp
index cac4c51..12c5632 100644
--- a/src/config.cpp
+++ b/src/config.cpp
@@ -18,6 +18,8 @@
 
 #include "config.h"
 
+#include "mailsync.h"
+
 #include <QFileInfo>
 #include <QSettings>
 #include <QStandardPaths>
@@ -171,6 +173,14 @@ void Config::load(const QString &path)
         m_syncCommand.clear();
     }
 
+    // Not validated for existence, unlike the command above. The log is written
+    // by the script when it runs, so a fresh install has no file yet, and a
+    // startup problem reported for that would be noise. A missing file simply
+    // reads as SyncOutcome::Unknown when the time comes.
+    m_syncLog = settings.value(QStringLiteral("sync/log")).toString().trimmed();
+    if (m_syncLog.isEmpty())
+        m_syncLog = MailSync::defaultLogPath();
+
     // Account groups are written as [account.work], [account.personal], etc.
     // A dot, not a slash, separates the "account" namespace from the key:
     // QSettings' INI backend treats "/" as its own hierarchical group
diff --git a/src/config.h b/src/config.h
index d3ee767..3174a03 100644
--- a/src/config.h
+++ b/src/config.h
@@ -90,6 +90,15 @@ public:
     /// Empty when unset; the caller disables the Sync button in that case.
     QString syncCommand() const { return m_syncCommand; }
 
+    /// The sync script's log file, read to learn the outcome of a sync this
+    /// process did not start (item 54).
+    ///
+    /// Never empty: an unset key falls back to where assets/mailsync.sh writes
+    /// by default. An empty value would make every background sync report
+    /// SyncOutcome::Unknown, and the pending-edit indicator would then never
+    /// clear on a cron sync, which is exactly the defect this exists to fix.
+    QString syncLog() const { return m_syncLog; }
+
     /// Optional alternate notmuch config file. Empty means "let notmuch decide".
     QString notmuchConfig() const { return m_notmuchConfig; }
 
@@ -161,6 +170,7 @@ private:
     QList<Account> m_accounts;
     QList<SavedQuery> m_savedQueries;
     QString m_syncCommand;
+    QString m_syncLog;
     QString m_notmuchConfig;
     qreal m_messageZoom = 1.0;
     bool m_completionOnFocus = false;
diff --git a/src/mailsync.cpp b/src/mailsync.cpp
index 1d2a99f..10e5ef7 100644
--- a/src/mailsync.cpp
+++ b/src/mailsync.cpp
@@ -19,6 +19,8 @@
 #include "mailsync.h"
 
 #include <QCoreApplication>
+#include <QDir>
+#include <QFile>
 #include <QRegularExpression>
 
 namespace {
@@ -240,3 +242,56 @@ void MailSync::handleError(QProcess::ProcessError error)
     emit outputReceived(message);
     emit finished(false, -1);
 }
+
+QString MailSync::defaultLogPath()
+{
+    // Hardcoded to match assets/mailsync.sh, which builds it the same way from
+    // $HOME. Deriving it from QStandardPaths::GenericStateLocation would append
+    // the application name and point at a file the script never writes.
+    return QDir::homePath() + QStringLiteral("/.local/state/mailsync.log");
+}
+
+SyncOutcome MailSync::lastRunOutcome(const QString &logPath)
+{
+    if (logPath.isEmpty())
+        return SyncOutcome::Unknown;
+
+    QFile file(logPath);
+    if (!file.open(QIODevice::ReadOnly | QIODevice::Text))
+        return SyncOutcome::Unknown;
+
+    // A run's banner is one short line, so a tail this size holds many of them
+    // even when a verbose mbsync run sits between two. Reading the whole file
+    // would be a multi-megabyte read on the UI thread every ten minutes.
+    constexpr qint64 kTailBytes = 64 * 1024;
+    const qint64 size = file.size();
+    if (size > kTailBytes && !file.seek(size - kTailBytes))
+        return SyncOutcome::Unknown;
+
+    const QByteArray tail = file.readAll();
+
+    // Last marker wins: the log accumulates runs, and an older OK must never
+    // speak for a newer failure. RUN START lines are skipped rather than
+    // stopping the scan, since the poll that observes a lock release can land
+    // after the next run has already announced itself.
+    const QList<QByteArray> lines = tail.split('\n');
+    for (auto it = lines.crbegin(); it != lines.crend(); ++it) {
+        const QByteArray line = it->trimmed();
+        if (!line.startsWith("===== RUN END:"))
+            continue;
+
+        // Matched as a token, not as a whole line: the failure banner carries
+        // mbsync= and notmuch= fields after the status.
+        if (line.contains("status=OK"))
+            return SyncOutcome::Ok;
+        if (line.contains("status=FAILED"))
+            return SyncOutcome::Failed;
+
+        // A marker whose status this does not recognise. The script changed, or
+        // the line was truncated by the tail boundary; either way nothing was
+        // observed.
+        return SyncOutcome::Unknown;
+    }
+
+    return SyncOutcome::Unknown;
+}
diff --git a/src/mailsync.h b/src/mailsync.h
index f83b526..a826f43 100644
--- a/src/mailsync.h
+++ b/src/mailsync.h
@@ -65,6 +65,17 @@ private:
     QString m_status;
 };
 
+/// The outcome of a sync run this process did not start.
+///
+/// Unknown is not a failure, it is the absence of evidence: no log, no marker,
+/// an unreadable file. Callers must treat it as "nothing observed" and change
+/// no state on it, exactly as SyncMonitor::State::Unknown is treated.
+enum class SyncOutcome {
+    Unknown,
+    Ok,
+    Failed,
+};
+
 /// Runs the configured external sync command.
 ///
 /// qtmaildir deliberately does not implement sync itself. The existing script
@@ -97,6 +108,23 @@ public:
 
     QString log() const { return m_log; }
 
+    /// Where assets/mailsync.sh writes its log, unless the config overrides it.
+    static QString defaultLogPath();
+
+    /// Reads the outcome of the last COMPLETED run from \p logPath.
+    ///
+    /// This is how a sync fired by the user's cron is judged: the process that
+    /// ran it is gone and its exit status died with it, but the script writes
+    /// a "RUN END ... status=OK" line before exiting, and that line survives.
+    /// Deriving the outcome from mbsync's own chatter was rejected for the
+    /// reason given on SyncPhaseTracker: a second opinion built from loose text
+    /// matching eventually disagrees with the authoritative one.
+    ///
+    /// Reads a bounded tail, not the file: this runs on the UI thread every
+    /// time a background sync ends, against a file logrotate lets grow all day.
+    /// Anything unreadable, absent or unmarked is Unknown.
+    static SyncOutcome lastRunOutcome(const QString &logPath);
+
 signals:
     void started();
     void outputReceived(const QString &chunk);
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index f830683..96e2a79 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -1902,6 +1902,33 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state)
         showTransientStatus(
             tr("Background sync completed. Press Enter in the query bar to "
                "refresh."));
+
+        // Item 54. A cron sync carries the edits to the mail store exactly as a
+        // local one does, so the count it cleared has to be cleared here too.
+        // Without this the indicator kept reporting work that had already
+        // shipped, and the exit prompt asked to sync for it.
+        //
+        // The outcome comes from the RUN END line the script writes, because
+        // the process that ran this sync is gone and its exit status with it.
+        // Anything other than a definite OK changes nothing: the local path's
+        // rule is that only a SUCCESSFUL sync may clear the count, and Unknown
+        // is the absence of evidence rather than evidence of success.
+        if (MailSync::lastRunOutcome(m_config.syncLog()) == SyncOutcome::Ok) {
+            m_pendingTagEdits.clear();
+            m_unnettablePendingEdits = 0;
+
+            // Cleared HERE, before flushHeldEdits() below, and the ordering is
+            // load-bearing for the reason spelled out on the local path at
+            // onSyncFinished(): the flush calls sendThreadTagChange(), which
+            // writes m_editedAccounts SYNCHRONOUSLY. Clearing after the flush
+            // would discard accounts whose edits this run did not carry, and
+            // those edits would then sync only when some later edit happened to
+            // name the same account. Running first, everything in the set at
+            // this moment is exactly what the finished sync carried, so the
+            // local path's snapshot-and-subtract collapses to a clear.
+            m_editedAccounts.clear();
+            updatePendingIndicator();
+        }
     }
 
     // OUTSIDE the Idle branch, deliberately. Unknown clears the busy flag above,
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 2d41b5e..490467f 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -325,7 +325,11 @@ private:
 
     /// Sends a tag change for a set of threads without touching the undo stack.
     /// Both tagSelected() and ThreadTagCommand route through this.
-    void sendThreadTagChange(const QStringList &threadIds,
+    ///
+    /// Invokable so a test can record an edit against a known account without a
+    /// worker: this is where m_editedAccounts is populated, and item 54's
+    /// draining of it cannot be observed otherwise.
+    Q_INVOKABLE void sendThreadTagChange(const QStringList &threadIds,
                              const QStringList &add,
                              const QStringList &remove,
                              const QString &description);
@@ -543,7 +547,10 @@ private:
     /// The channel names for m_editedAccounts, resolved through the config.
     /// Empty means sync everything, which is what a fetch with nothing pending
     /// has to do.
-    QStringList pendingSyncChannels() const;
+    /// Invokable for the same reason as sendThreadTagChange(): it is the only
+    /// view onto m_editedAccounts, and a count that reaches zero while the set
+    /// stays full looks correct and still syncs the wrong channels.
+    Q_INVOKABLE QStringList pendingSyncChannels() const;
 };
 
 /// Undo entry for a tag change over a set of threads.