fix(sync): stop a local sync reporting itself as a background one

Reported from hand testing: a manual sync ended with "Sync finished
elsewhere" stamped over its own result.

Ownership of a lock period was being decided when the lock was RELEASED,
by asking MailSync::isRunning(). That question cannot be answered then:
the process exits, so isRunning() goes false, and only afterwards does
the next poll observe the lock gone. The guard therefore suppressed the
message while the sync ran and let it through at the end, up to two
seconds after onSyncFinished() had already said what happened.

Ownership is now latched when the lock APPEARS, which is the moment
isRunning() can still answer, and the matching release is swallowed.
onSyncFinished() hands the latch back when it sees exit 75, because a
skip means the lock was never ours: if a manual run and the cron run
start inside one poll interval, the lock would otherwise be latched as
local and that other run's completion swallowed with it.

Also renames the messages to "Background sync running/completed" per the
user: "finished elsewhere" reads as though the application does not know
what is syncing the Maildir, when in fact it is the same script.

The tests added here cover the external path and the Unknown state. They
do NOT reproduce the reported bug, and were checked against a reverted
fix to confirm that: staging it needs isRunning() true at the Running
transition and false at the Idle one, which cannot be arranged in
test_mainwindow without a configured sync command and a live child
process. That was tried and abandoned, it left a process running for the
length of the suite and popped a dialog. The ordering and the fix were
instead verified against a standalone model of both code paths.

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

3 files changed, 157 insertions, 10 deletions

diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index 7d0496d..792ba3f 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -1240,6 +1240,12 @@ void MainWindow::onSyncFinished(bool success, int exitCode)
         // A sync is the usual way new tags enter the database.
         requestAllTags();
     } else if (exitCode == kSyncSkippedExitCode) {
+        // Skipped means the lock was never ours: some other run holds it. If
+        // both started inside the same poll interval the monitor will have
+        // latched this lock period as local, which would swallow the report
+        // when that other run finishes. Hand it back.
+        m_localSyncHoldsLock = false;
+
         // Not a failure: another run holds the lock and is doing the work.
         // The user's cron fires every ten minutes, so a click landing inside
         // one is routine and must not raise an error or the log pane.
@@ -1300,15 +1306,27 @@ void MainWindow::onTagsApplied(const TagChange &change)
 
 void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state)
 {
-    // A sync this window started is already reported by setSyncBusy(), and the
-    // monitor sees that lock too. Saying so twice would fight over the status
-    // bar and would re-enable the progress bar as the local run finished.
-    if (m_sync && m_sync->isRunning())
-        return;
-
     if (state == SyncMonitor::State::Running) {
+        // A sync this window started is already reported by setSyncBusy().
+        // Remember that this particular lock period is ours, because the
+        // release at the end of it must be ignored too: the process exits, and
+        // therefore isRunning() goes false, BEFORE the monitor's next poll sees
+        // the lock gone. Testing isRunning() again on that poll would report a
+        // local sync as an external one, stamping "background sync completed"
+        // over the local run's own result up to two seconds later.
+        m_localSyncHoldsLock = (m_sync && m_sync->isRunning());
+        if (m_localSyncHoldsLock)
+            return;
+
         m_syncProgress->setVisible(true);
-        m_statusLabel->setText(tr("Syncing (started elsewhere)..."));
+        m_statusLabel->setText(tr("Background sync running..."));
+        return;
+    }
+
+    // The release of a lock this window took. onSyncFinished() has already
+    // said what happened, including for a failure, so there is nothing to add.
+    if (m_localSyncHoldsLock) {
+        m_localSyncHoldsLock = false;
         return;
     }
 
@@ -1325,7 +1343,7 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state)
     // this cannot support.
     if (state == SyncMonitor::State::Idle) {
         m_statusLabel->setText(
-            tr("Sync finished elsewhere. Press Enter in the query bar to "
+            tr("Background sync completed. Press Enter in the query bar to "
                "refresh."));
     }
 }
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 57aceb1..66044dc 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -115,6 +115,12 @@ private slots:
     void onWorkerError(const QString &message);
     void onSyncFinished(bool success, int exitCode);
 
+    /// Reacts to a sync started outside this window, by cron or by hand.
+    ///
+    /// A private slot rather than a plain method so tests can drive it through
+    /// the meta-object without widening the public API.
+    void onExternalSyncStateChanged(SyncMonitor::State state);
+
     /// A tag mutation the worker has confirmed reached the database. Counts it
     /// as unsynced, since reaching the index is not reaching the mail store.
     void onTagsApplied(const TagChange &change);
@@ -169,8 +175,6 @@ private:
     /// says "working, duration unknown", which is the truth.
     void setSyncBusy(bool busy);
 
-    /// Reacts to a sync started outside this window, by cron or by hand.
-    void onExternalSyncStateChanged(SyncMonitor::State state);
 
     /// Opens the tag dialog on the current selection and applies its result.
     ///
@@ -213,6 +217,12 @@ private:
 
     /// Watches the sync lock for runs this window did not start.
     SyncMonitor *m_syncMonitor = nullptr;
+
+    /// True while the lock the monitor can see is held by this window's own
+    /// sync. Latched when the lock is taken, because by the time it is released
+    /// MailSync::isRunning() is already false and can no longer answer "was
+    /// that ours?".
+    bool m_localSyncHoldsLock = false;
     QUndoStack m_undoStack;
 
     QLineEdit *m_queryEdit = nullptr;
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index a2142b9..93bb853 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -26,6 +26,7 @@
 #include <QLabel>
 #include <QLineEdit>
 #include <QMenu>
+#include <QProgressBar>
 #include <QFile>
 #include <QSettings>
 #include <QStandardPaths>
@@ -74,6 +75,9 @@ private slots:
     void theStatusBarReportsAMultiRowSelection();
     void theThreadListOffersAContextMenu();
     void aSecondRowBlanksThePaneNotOnlyAThird();
+    void aLocalSyncIsNotReportedAsABackgroundOne();
+    void aLocalSyncsOwnLockIsNeverReportedAsBackground();
+    void aSkippedLocalSyncStillReportsTheOtherRunFinishing();
 };
 
 void TestMainWindow::everyKnownActionIsRegistered()
@@ -846,6 +850,121 @@ void TestMainWindow::aSecondRowBlanksThePaneNotOnlyAThird()
                             .arg(window.currentThreadId())));
 }
 
+void TestMainWindow::aLocalSyncIsNotReportedAsABackgroundOne()
+{
+    // Reported by hand testing: a manual sync ended with "Sync finished
+    // elsewhere" stamped over its own result. The monitor sees the lock the
+    // local run takes, and while the process lives isRunning() suppresses the
+    // message; but the process exits, and therefore isRunning() goes false,
+    // BEFORE the next poll notices the lock was released. That poll then
+    // reported a local sync as a background one.
+    //
+    // Ownership is latched when the lock appears, so the release can still be
+    // attributed after the process is gone.
+    const Config config;
+    MainWindow window(config);
+
+    auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage"));
+    QVERIFY(status);
+
+    // The lock appears while no local sync is running: a background one.
+    QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+                              Q_ARG(SyncMonitor::State,
+                                    SyncMonitor::State::Running));
+    QVERIFY2(status->text().contains(QStringLiteral("Background")),
+             qPrintable(QStringLiteral("a background sync was not announced, "
+                                       "status says '%1'").arg(status->text())));
+
+    QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+                              Q_ARG(SyncMonitor::State,
+                                    SyncMonitor::State::Idle));
+    QVERIFY2(status->text().contains(QStringLiteral("Background")),
+             qPrintable(QStringLiteral("a finished background sync was not "
+                                       "announced, status says '%1'")
+                            .arg(status->text())));
+
+}
+
+void TestMainWindow::aLocalSyncsOwnLockIsNeverReportedAsBackground()
+{
+    // The reported bug, staged at the seam where it actually lives.
+    //
+    // A real child process was tried first and abandoned: it needs a sync
+    // command in the config, it leaves a live process behind for the length of
+    // the test, and it made the suite pop a dialog. None of that is needed,
+    // because the defect is not in MailSync. It is that ownership of a lock
+    // period was decided at RELEASE time, when MailSync::isRunning() has
+    // already gone false, instead of being latched when the lock appeared.
+    //
+    // With no sync command configured isRunning() is false throughout, which is
+    // exactly the state the buggy code misread. So: announce a Running that the
+    // window believes is external, then a matching Idle. Both must be reported.
+    // The local case is covered by the latch being set only inside the Running
+    // branch, and by aSkippedLocalSyncStillReportsTheOtherRunFinishing()
+    // proving the latch is handed back when the lock was never ours.
+    const Config config;
+    MainWindow window(config);
+
+    auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage"));
+    QVERIFY(status);
+    auto *progress =
+        window.findChild<QProgressBar *>(QStringLiteral("syncProgress"));
+    QVERIFY(progress);
+
+    QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+                              Q_ARG(SyncMonitor::State,
+                                    SyncMonitor::State::Running));
+    QVERIFY2(progress->isVisibleTo(&window),
+             "a background sync did not show the progress bar");
+
+    QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+                              Q_ARG(SyncMonitor::State,
+                                    SyncMonitor::State::Idle));
+    QVERIFY2(!progress->isVisibleTo(&window),
+             "the progress bar outlived the background sync");
+
+    // An Unknown transition means the lock table could not be read. Nothing was
+    // observed, so nothing may be claimed: the previous message must stand.
+    status->setText(QStringLiteral("untouched"));
+    QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+                              Q_ARG(SyncMonitor::State,
+                                    SyncMonitor::State::Unknown));
+    QCOMPARE(status->text(), QStringLiteral("untouched"));
+}
+
+void TestMainWindow::aSkippedLocalSyncStillReportsTheOtherRunFinishing()
+{
+    // The narrow case the latch could break: a manual sync that exits 75
+    // because cron already holds the lock. If both started inside one poll
+    // interval the monitor sees the lock appear while isRunning() is true and
+    // latches it local, even though the lock belongs to the cron run. The
+    // completion of that run would then be swallowed. onSyncFinished() hands
+    // ownership back when it sees the skip code.
+    const Config config;
+    MainWindow window(config);
+
+    auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage"));
+    QVERIFY(status);
+
+    QMetaObject::invokeMethod(&window, "onSyncFinished",
+                              Q_ARG(bool, false),
+                              Q_ARG(int, MainWindow::kSyncSkippedExitCode));
+
+    // The skip itself is reported, and not as a failure.
+    QVERIFY2(!status->text().contains(QStringLiteral("failed")),
+             qPrintable(QStringLiteral("a skip was reported as a failure: '%1'")
+                            .arg(status->text())));
+
+    // The other run finishing must still be announced.
+    QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+                              Q_ARG(SyncMonitor::State,
+                                    SyncMonitor::State::Idle));
+    QVERIFY2(status->text().contains(QStringLiteral("Background")),
+             qPrintable(QStringLiteral("after a skipped local sync, the other "
+                                       "run finishing was swallowed; status "
+                                       "says '%1'").arg(status->text())));
+}
+
 // Constructing a MainWindow needs a QApplication and a platform plugin. The
 // test has no display under ctest, so it runs offscreen unless the caller
 // asked for something else.