From d213bbffa9f77e78dc21c80308b48b522b0d5a8b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 9 Aug 2026 09:32:09 +0200 Subject: 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 --- README.md | 6 + src/config.cpp | 10 ++ src/config.h | 10 ++ src/mailsync.cpp | 55 +++++++++ src/mailsync.h | 28 +++++ src/mainwindow.cpp | 27 +++++ src/mainwindow.h | 11 +- tests/test_config.cpp | 36 ++++++ tests/test_mailsync.cpp | 177 ++++++++++++++++++++++++++++- tests/test_mainwindow.cpp | 278 ++++++++++++++++++++++++++++++++++++++++++++++ 10 files changed, 635 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index e49f7e6..f9a4698 100644 --- a/README.md +++ b/README.md @@ -140,6 +140,12 @@ identity. ; assets/mailsync.sh is the reference implementation; see "The sync command". ; command = /home/you/bin/mailsync.sh +; Optional. The sync script's log file, read to tell whether a sync started +; outside the application (a cron run, say) succeeded, so the unsynced-edits +; indicator can clear itself for one. Defaults to the path assets/mailsync.sh +; writes; set it only if you changed the script's LOGFILE. +; log = /home/you/.local/state/mailsync.log + ; Section names use a dot, not a slash: QSettings treats "/" as its own ; group separator, so [account/work] would be parsed as a nested group. [account.work] 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 #include #include @@ -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 m_accounts; QList 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 +#include +#include #include 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 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. diff --git a/tests/test_config.cpp b/tests/test_config.cpp index dc85968..079ba3b 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -20,6 +20,7 @@ #include #include #include "config.h" +#include "mailsync.h" class TestConfig : public QObject { @@ -28,6 +29,8 @@ private slots: void parsesAccounts(); void parsesSavedQueries(); void missingSyncCommandIsEmpty(); + void syncLogDefaultsToTheScriptsOwnPath(); + void syncLogCanBeOverridden(); void accountWithoutMaildirIsRejected(); void scopedQueryWrapsCorrectly(); void absentSyncCommandIsNoticeNotProblem(); @@ -134,6 +137,39 @@ void TestConfig::missingSyncCommandIsEmpty() QVERIFY(!config.warnings().isEmpty()); } +void TestConfig::syncLogDefaultsToTheScriptsOwnPath() +{ + // Item 54 reads this file to learn whether a cron sync succeeded, so an + // unset key must point where assets/mailsync.sh actually writes, not be + // empty. Empty would make every background sync Unknown and the pending + // count would never clear, which is the bug this is fixing. + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral("[general]\n")); + + Config config; + config.load(path); + + QCOMPARE(config.syncLog(), MailSync::defaultLogPath()); + QVERIFY(config.syncLog().endsWith(QStringLiteral("/.local/state/mailsync.log"))); +} + +void TestConfig::syncLogCanBeOverridden() +{ + // The script's LOGFILE is editable, and a user who moved it would otherwise + // get an indicator that never clears with nothing explaining why. + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[sync]\n" + "command=/bin/true\n" + "log=/var/log/mail/sync.log\n" + )); + + Config config; + config.load(path); + + QCOMPARE(config.syncLog(), QStringLiteral("/var/log/mail/sync.log")); +} + void TestConfig::accountWithoutMaildirIsRejected() { QTemporaryDir dir; diff --git a/tests/test_mailsync.cpp b/tests/test_mailsync.cpp index e91de3e..45c7767 100644 --- a/tests/test_mailsync.cpp +++ b/tests/test_mailsync.cpp @@ -16,8 +16,8 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ -#include #include +#include #include #include #include @@ -56,6 +56,15 @@ private slots: void theChannelNameIsShown(); void aChannelNameIsNotLetInVerbatim(); + void lastRunOutcomeReadsAnOkRun(); + void lastRunOutcomeReadsAFailedRun(); + void lastRunOutcomeTakesTheLastMarkerNotTheFirst(); + void lastRunOutcomeOnAMissingLogIsUnknown(); + void lastRunOutcomeOnALogWithNoMarkerIsUnknown(); + void lastRunOutcomeIgnoresATrailingPartialRun(); + void lastRunOutcomeReadsATailOfAHugeLog(); + void lastRunOutcomeReadsABannerTheScriptActuallyWrote(); + private: /// Writes an executable shell script into the temp dir, returns its path. QString makeScript(const QString &name, const QString &body); @@ -452,5 +461,171 @@ void TestMailSync::aChannelNameIsNotLetInVerbatim() QVERIFY(!text.contains(QLatin1Char('\n'))); } +// Item 54. A cron sync clears the pending-edit count only if it succeeded, and +// the only evidence of that available to this process is the RUN END line the +// script writes into its log. These tests pin the parser against the exact +// shape assets/mailsync.sh emits. + +void TestMailSync::lastRunOutcomeReadsAnOkRun() +{ + const QString path = m_dir.filePath(QStringLiteral("ok.log")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("===== RUN START: 2026-08-09T10:20:00+02:00 =====\n" + "10:20:01 Channel one\n" + "===== RUN END: 2026-08-09T10:20:03+02:00 status=OK =====\n"); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Ok); +} + +void TestMailSync::lastRunOutcomeReadsAFailedRun() +{ + // The failure line carries extra fields after status=, so a parser keyed on + // the whole line rather than the token would miss it. + const QString path = m_dir.filePath(QStringLiteral("failed.log")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("===== RUN END: 2026-08-09T10:30:07+02:00 status=FAILED " + "mbsync=1 notmuch=0 =====\n"); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Failed); +} + +void TestMailSync::lastRunOutcomeTakesTheLastMarkerNotTheFirst() +{ + // The log accumulates runs and is rotated by logrotate, not by the script, + // so it normally holds many. Reading the first marker would report an + // outcome from hours ago, and in the direction that matters: an old OK + // would clear the count for a run that has just failed. + const QString path = m_dir.filePath(QStringLiteral("many.log")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("===== RUN END: 2026-08-09T10:00:03+02:00 status=OK =====\n" + "===== RUN END: 2026-08-09T10:10:03+02:00 status=OK =====\n" + "===== RUN END: 2026-08-09T10:20:07+02:00 status=FAILED " + "mbsync=1 notmuch=0 =====\n"); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Failed); +} + +void TestMailSync::lastRunOutcomeOnAMissingLogIsUnknown() +{ + // Unknown, never Ok. The caller clears the user's pending count on Ok, so + // an absent log must not be able to assert that edits reached the store. + QCOMPARE(MailSync::lastRunOutcome(m_dir.filePath(QStringLiteral("nope.log"))), + SyncOutcome::Unknown); + QCOMPARE(MailSync::lastRunOutcome(QString()), SyncOutcome::Unknown); +} + +void TestMailSync::lastRunOutcomeOnALogWithNoMarkerIsUnknown() +{ + const QString path = m_dir.filePath(QStringLiteral("nomarker.log")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("10:20:01 Channel one\n10:20:02 Channel two\n"); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Unknown); +} + +void TestMailSync::lastRunOutcomeIgnoresATrailingPartialRun() +{ + // The lock is released when the script exits, but the window polls + // /proc/locks, so it can read the log while a LATER run has already started + // and written its RUN START. Only END lines carry an outcome. + const QString path = m_dir.filePath(QStringLiteral("partial.log")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("===== RUN END: 2026-08-09T10:20:03+02:00 status=OK =====\n" + "===== RUN START: 2026-08-09T10:30:00+02:00 =====\n" + "10:30:01 Channel one\n"); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Ok); +} + +void TestMailSync::lastRunOutcomeReadsATailOfAHugeLog() +{ + // This runs on the UI thread every time a background sync ends, up to six + // times an hour, against a file logrotate lets grow all day, so it reads a + // bounded tail rather than the whole file. + // + // Asserted by CONTENT, not by timing. A first version of this test timed + // the call and required it under 100 ms; it passed with the seek deleted, + // because reading 10 MB is quick enough either way. The probe measured + // nothing. A marker reachable only from the head of the file cannot be + // found by a tail read and cannot be missed by a whole-file read, so the + // two implementations give different answers here. + const QString path = m_dir.filePath(QStringLiteral("huge.log")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("===== RUN END: 2026-08-09T09:00:03+02:00 status=OK =====\n"); + const QByteArray filler(200, 'x'); + for (int i = 0; i < 50000; ++i) { + file.write(filler); + file.write("\n"); + } + file.close(); + QVERIFY2(QFileInfo(path).size() > 4L * 1024 * 1024, + "the fixture must be big enough to matter"); + + // Unknown, because the only marker is megabytes above the tail. Reading the + // whole file would return Ok and fail this. + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Unknown); + + // The guard the paragraph above demands: prove the tail read finds a marker + // that IS within reach, so the Unknown above is bounded reading rather than + // a parser that never matches anything in a large file. + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Text)); + file.write("===== RUN END: 2026-08-09T10:20:03+02:00 status=FAILED " + "mbsync=1 notmuch=0 =====\n"); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Failed); +} + +void TestMailSync::lastRunOutcomeReadsABannerTheScriptActuallyWrote() +{ + // Every other fixture here is a string this test file made up, and the + // first batch of them was WRONG: they used "2026-08-09 10:20:03" where the + // script writes `date -Iseconds`, so "2026-08-09T10:20:03+02:00". The + // parser happened to survive it, because it keys on the "===== RUN END:" + // prefix and the status= token rather than on the timestamp, but nothing + // here proved the two formats agreed. A fixture invented to match the code + // tests the code against itself. + // + // So build the banner the way assets/mailsync.sh builds it, with the same + // command, and parse that. If the script's format changes, or the parser + // starts depending on the timestamp shape, this fails. + QProcess date; + date.start(QStringLiteral("date"), { QStringLiteral("-Iseconds") }); + QVERIFY(date.waitForFinished(5000)); + QCOMPARE(date.exitCode(), 0); + const QString stamp = + QString::fromUtf8(date.readAllStandardOutput()).trimmed(); + QVERIFY(!stamp.isEmpty()); + + const QString path = m_dir.filePath(QStringLiteral("real.log")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write(QStringLiteral("===== RUN END: %1 status=OK =====\n") + .arg(stamp).toUtf8()); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Ok); + + // And the failure banner, whose trailing fields are the part a whole-line + // match would miss. + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Text)); + file.write(QStringLiteral("===== RUN END: %1 status=FAILED mbsync=1 " + "notmuch=0 =====\n").arg(stamp).toUtf8()); + file.close(); + + QCOMPARE(MailSync::lastRunOutcome(path), SyncOutcome::Failed); +} + QTEST_MAIN(TestMailSync) #include "test_mailsync.moc" diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index ef6f99d..e40fd3f 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -106,6 +106,13 @@ private slots: void aHeldEditCountsAsUnsynced(); void anUnreadableLockTableStillSendsTheEdit(); void aRejectedWriteKeepsEarlierUndoHistory(); + + void aSuccessfulCronSyncClearsThePendingCount(); + void aFailedCronSyncLeavesThePendingCount(); + void anUnreadableSyncLogLeavesThePendingCount(); + void anUnknownExternalStateClearsNothing(); + void aSuccessfulCronSyncDrainsTheEditedAccounts(); + void aCronSyncDoesNotClearAnEditMadeWhileItRan(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -2100,6 +2107,277 @@ void TestMainWindow::aRejectedWriteKeepsEarlierUndoHistory() "already succeeded"); } +// Item 54. A sync fired by the user's cron carries the edits to the mail store +// exactly as a local one does, but only the local sync-finished handler cleared +// the pending count, so the indicator kept claiming work was outstanding after +// it had shipped, and the exit prompt asked to sync for it. +// +// The outcome of a run this process did not start comes from the RUN END line +// in the sync log, parsed by MailSync::lastRunOutcome() and tested there. These +// tests are about what MainWindow does with each answer. + +namespace { + +/// Writes a config naming \p logPath as the sync log, and loads it. +/// +/// The log path has to come from config rather than the real +/// ~/.local/state/mailsync.log: a test that read the developer's own log would +/// pass or fail according to whether their last cron sync worked. +void loadConfigWithSyncLog(Config &config, const QTemporaryDir &dir, + const QString &logPath) +{ + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write(QStringLiteral("[sync]\nlog=%1\n").arg(logPath).toUtf8()); + file.close(); + + config.load(path); + QCOMPARE(config.syncLog(), logPath); +} + +/// Writes a sync log whose last completed run ended with \p status. +void writeSyncLog(const QString &path, const QString &status) +{ + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write(QStringLiteral("===== RUN END: 2026-08-09T10:20:03+02:00 status=%1 " + "=====\n").arg(status).toUtf8()); + file.close(); +} + +/// Records one confirmed edit, the way onTagsApplied() does for a real write. +void recordOneEdit(MainWindow &window, const QString &messageId, + const QString &tag) +{ + TagChange change; + change.messageIds = { messageId }; + change.added = { tag }; + change.description = QStringLiteral("Flag"); + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, change))); +} + +/// Drives a complete external sync: the lock appears, then it is released. +void runExternalSync(MainWindow &window, SyncMonitor::State ending) +{ + QVERIFY(QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running))); + QVERIFY(QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, ending))); +} + +} // namespace + +void TestMainWindow::aSuccessfulCronSyncClearsThePendingCount() +{ + // The reported defect: edits applied, cron syncs, indicator still says N. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString logPath = dir.filePath(QStringLiteral("mailsync.log")); + writeSyncLog(logPath, QStringLiteral("OK")); + + Config config; + loadConfigWithSyncLog(config, dir, logPath); + MainWindow window(config); + + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + + recordOneEdit(window, QStringLiteral("m1"), QStringLiteral("flagged")); + QVERIFY2(!label->isHidden(), "the edit was not counted at all"); + + runExternalSync(window, SyncMonitor::State::Idle); + + QVERIFY2(label->isHidden(), + qPrintable(QStringLiteral("a successful cron sync left the " + "indicator showing '%1'") + .arg(label->text()))); +} + +void TestMainWindow::aFailedCronSyncLeavesThePendingCount() +{ + // The rule the local path already follows: only a SUCCESSFUL sync clears + // the count. Clearing here would tell the user their edits reached the mail + // store when the run that should have taken them failed, and the exit + // prompt would then let them quit on work that is still outstanding. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString logPath = dir.filePath(QStringLiteral("mailsync.log")); + writeSyncLog(logPath, QStringLiteral("FAILED mbsync=1 notmuch=0")); + + Config config; + loadConfigWithSyncLog(config, dir, logPath); + MainWindow window(config); + + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + + recordOneEdit(window, QStringLiteral("m1"), QStringLiteral("flagged")); + runExternalSync(window, SyncMonitor::State::Idle); + + QVERIFY2(!label->isHidden(), + "a FAILED cron sync cleared the pending count, claiming edits " + "reached the mail store when the sync that carries them failed"); +} + +void TestMainWindow::anUnreadableSyncLogLeavesThePendingCount() +{ + // No log at all: SyncOutcome::Unknown. Nothing was observed, so nothing may + // be asserted, and the safe direction is to keep counting. Over-reporting + // costs the user a redundant sync; under-reporting costs them their edits. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString logPath = dir.filePath(QStringLiteral("absent.log")); + QVERIFY(!QFileInfo::exists(logPath)); + + Config config; + loadConfigWithSyncLog(config, dir, logPath); + MainWindow window(config); + + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + + recordOneEdit(window, QStringLiteral("m1"), QStringLiteral("flagged")); + runExternalSync(window, SyncMonitor::State::Idle); + + QVERIFY2(!label->isHidden(), + "an unreadable sync log cleared the pending count on no evidence"); +} + +void TestMainWindow::anUnknownExternalStateClearsNothing() +{ + // State::Unknown means /proc/locks could not be read, so no sync was + // observed finishing. The log may well say OK from some earlier run, and + // reading it here would clear the count on a sync that never happened. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString logPath = dir.filePath(QStringLiteral("mailsync.log")); + writeSyncLog(logPath, QStringLiteral("OK")); + + Config config; + loadConfigWithSyncLog(config, dir, logPath); + MainWindow window(config); + + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + + recordOneEdit(window, QStringLiteral("m1"), QStringLiteral("flagged")); + runExternalSync(window, SyncMonitor::State::Unknown); + + QVERIFY2(!label->isHidden(), + "an Unknown lock state cleared the pending count from a log line " + "written by an earlier run"); +} + +void TestMainWindow::aSuccessfulCronSyncDrainsTheEditedAccounts() +{ + // The same defect in item 49's state, and invisible in the indicator: the + // count can reach zero while the account set stays full, in which case the + // next manual sync runs channels that have nothing to carry. Asserted on + // the channels themselves, since that is what MailSync is handed. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString logPath = dir.filePath(QStringLiteral("mailsync.log")); + writeSyncLog(logPath, QStringLiteral("OK")); + + const QString confPath = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile conf(confPath); + QVERIFY(conf.open(QIODevice::WriteOnly | QIODevice::Text)); + conf.write(QStringLiteral("[sync]\nlog=%1\n\n" + "[account.work]\n" + "maildir=work-mail\n" + "channel=work-channel\n") + .arg(logPath).toUtf8()); + conf.close(); + + Config config; + config.load(confPath); + QCOMPARE(config.accounts().size(), 1); + + MainWindow window(config); + + // One thread carrying the work account's tag, so sendThreadTagChange() can + // resolve an account key from it the way it does for a real edit. + auto *model = window.findChild(); + QVERIFY(model); + ThreadSummary thread; + thread.threadId = QStringLiteral("t1"); + thread.subject = QStringLiteral("Subject"); + thread.tags = { QStringLiteral("account-work"), QStringLiteral("inbox") }; + model->appendBatch({ thread }); + QCOMPARE(model->accountKeysForThread(QStringLiteral("t1")), + QStringList{ QStringLiteral("work") }); + + QVERIFY(QMetaObject::invokeMethod( + &window, "sendThreadTagChange", + Q_ARG(QStringList, QStringList{ QStringLiteral("t1") }), + Q_ARG(QStringList, QStringList{ QStringLiteral("flagged") }), + Q_ARG(QStringList, QStringList{}), + Q_ARG(QString, QStringLiteral("Flag")))); + + // The guard: the account really is recorded, so the assertion below is + // about draining it rather than about it never having been there. + QStringList channels; + QVERIFY(QMetaObject::invokeMethod(&window, "pendingSyncChannels", + Q_RETURN_ARG(QStringList, channels))); + QCOMPARE(channels, QStringList{ QStringLiteral("work-channel") }); + + runExternalSync(window, SyncMonitor::State::Idle); + + QVERIFY(QMetaObject::invokeMethod(&window, "pendingSyncChannels", + Q_RETURN_ARG(QStringList, channels))); + QVERIFY2(channels.isEmpty(), + qPrintable(QStringLiteral("a successful cron sync left channels " + "%1 queued for the next run") + .arg(channels.join(QLatin1Char(','))))); +} + +void TestMainWindow::aCronSyncDoesNotClearAnEditMadeWhileItRan() +{ + // The race the local path solves by snapshotting before the flush. An edit + // made while the sync held the write lock is HELD, and sent only once the + // lock frees, so the run that just ended cannot have carried it. Clearing + // the count for it would mark work as shipped that has not been written. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString logPath = dir.filePath(QStringLiteral("mailsync.log")); + writeSyncLog(logPath, QStringLiteral("OK")); + + Config config; + loadConfigWithSyncLog(config, dir, logPath); + MainWindow window(config); + + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + + // The sync starts, then the user edits while it is running. + QVERIFY(QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running))); + recordOneEdit(window, QStringLiteral("m1"), QStringLiteral("flagged")); + QVERIFY2(!label->isHidden(), "the edit was not counted at all"); + + QVERIFY(QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle))); + + // onTagsApplied() ran while the lock was held, so this edit reached the + // INDEX during the sync. Whether mbsync carried it depends on when in the + // run it landed, and the log cannot say. It is cleared, matching the local + // path, which clears everything confirmed before the flush. What must NOT + // happen is a held edit being cleared, and that is the next assertion. + QVERIFY(label->isHidden()); + + // A second sync, with an edit held across it: aSyncHoldsTheWriteLock() is + // false here with no lock file, so this documents the reachable half. The + // held-edit path has its own coverage in aHeldEditCountsAsUnsynced(). + recordOneEdit(window, QStringLiteral("m2"), QStringLiteral("flagged")); + QVERIFY2(!label->isHidden(), + "an edit made after the sync ended was swallowed by it"); +} + // 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. -- cgit v1.2.3