diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-02-qtmaildir-v1.md | 8 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-08-02-qtmaildir-design.md | 4 | ||||
| -rw-r--r-- | src/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | src/mailsync.cpp | 83 | ||||
| -rw-r--r-- | src/mailsync.h | 44 | ||||
| -rw-r--r-- | tests/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | tests/test_mailsync.cpp | 239 |
7 files changed, 374 insertions, 6 deletions
diff --git a/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md b/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md index 1e14105..41bf1bb 100644 --- a/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md +++ b/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md @@ -3200,10 +3200,10 @@ Expected: FAIL, `mailsync.h: No such file or directory`. /// Runs the configured external sync command. /// /// qtmaildir deliberately does not implement sync itself. The existing script -/// holds a flock that is the shared mutex between the user's hourly cron sync -/// and any manual sync; running the script joins that mutex, whereas a built-in -/// implementation would sit outside it and could run mbsync concurrently with -/// cron, corrupting Maildir UID state. +/// holds a flock that is the shared mutex between the user's cron sync, which +/// runs every 10 minutes, and any manual sync; running the script joins that +/// mutex, whereas a built-in implementation would sit outside it and could run +/// mbsync concurrently with cron, corrupting Maildir UID state. class MailSync : public QObject { Q_OBJECT diff --git a/docs/superpowers/specs/2026-08-02-qtmaildir-design.md b/docs/superpowers/specs/2026-08-02-qtmaildir-design.md index 3089341..02c969e 100644 --- a/docs/superpowers/specs/2026-08-02-qtmaildir-design.md +++ b/docs/superpowers/specs/2026-08-02-qtmaildir-design.md @@ -94,7 +94,7 @@ notmuch access happens on that thread; the UI never blocks. **Sync.** qtmaildir runs a configured external command rather than reimplementing `mbsync` orchestration. The decisive reason is the `flock` guard in the existing script: it is the shared mutex between the user's -hourly cron sync and any manual sync. Reimplementing the sync internally +cron sync (every 10 minutes) and any manual sync. Reimplementing it internally would place qtmaildir outside that mutex, and two concurrent `mbsync -a` runs on one Maildir corrupt UID state. Calling the script joins the mutex for free. Reimplementing would also not remove the dependency, since `mbsync` @@ -379,7 +379,7 @@ query re-runs so new mail appears. On non-zero, the status bar shows the last stderr lines with a "Show log" link. Sync never runs automatically in v1: no timer, no sync on startup. The user's -cron already syncs hourly, and a second scheduler competing with the first is +cron already syncs every 10 minutes, and a second scheduler competing with it is exactly what the script's `flock` guard exists to prevent. The button means "now". diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 5f01128..6a2b18e 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -7,6 +7,7 @@ add_library(qtmaildir_lib STATIC cidschemehandler.cpp notmuchworker.cpp threadlistmodel.cpp + mailsync.cpp ) target_include_directories(qtmaildir_lib diff --git a/src/mailsync.cpp b/src/mailsync.cpp new file mode 100644 index 0000000..fdc433f --- /dev/null +++ b/src/mailsync.cpp @@ -0,0 +1,83 @@ +#include "mailsync.h" + +MailSync::MailSync(const QString &command, QObject *parent) + : QObject(parent), m_command(command) +{ + // mbsync reports failures on stderr, so both channels go into one log: + // splitting them would leave the pane empty for the runs worth reading. + m_process.setProcessChannelMode(QProcess::MergedChannels); + + connect(&m_process, &QProcess::readyRead, + this, &MailSync::handleReadyRead); + connect(&m_process, &QProcess::finished, + this, &MailSync::handleFinished); + connect(&m_process, &QProcess::errorOccurred, + this, &MailSync::handleError); +} + +bool MailSync::isRunning() const +{ + return m_process.state() != QProcess::NotRunning; +} + +bool MailSync::start() +{ + if (!isAvailable() || isRunning()) + return false; + + // splitCommand handles quoted arguments; running through a shell would make + // a config value into an injection point. + const QStringList parts = QProcess::splitCommand(m_command); + if (parts.isEmpty()) + return false; + + m_log.clear(); + + m_process.setProgram(parts.first()); + m_process.setArguments(parts.mid(1)); + + // Deliberately no waitForStarted(): the spec requires the UI stay usable + // during sync, and a failed launch arrives via errorOccurred() instead. + m_process.start(); + + emit started(); + return true; +} + +void MailSync::handleReadyRead() +{ + const QByteArray data = m_process.readAll(); + if (data.isEmpty()) + return; + + const QString chunk = QString::fromUtf8(data); + m_log += chunk; + emit outputReceived(chunk); +} + +void MailSync::handleFinished(int exitCode, QProcess::ExitStatus status) +{ + // Drain anything buffered at exit. + handleReadyRead(); + + // No guard against a preceding launch failure is needed: verified that + // QProcess emits errorOccurred(FailedToStart) *instead of* finished(), + // not before it. + const bool success = status == QProcess::NormalExit && exitCode == 0; + emit finished(success, exitCode); +} + +void MailSync::handleError(QProcess::ProcessError error) +{ + // Config validates the path at load time, but the script can be deleted or + // its filesystem unmounted afterwards. Without this the spinner would stay + // up forever with nothing explaining why. + if (error != QProcess::FailedToStart) + return; + + const QString message = + QStringLiteral("Failed to start sync command: %1\n").arg(m_command); + m_log += message; + emit outputReceived(message); + emit finished(false, -1); +} diff --git a/src/mailsync.h b/src/mailsync.h new file mode 100644 index 0000000..af4acbc --- /dev/null +++ b/src/mailsync.h @@ -0,0 +1,44 @@ +#pragma once + +#include <QObject> +#include <QProcess> +#include <QString> + +/// Runs the configured external sync command. +/// +/// qtmaildir deliberately does not implement sync itself. The existing script +/// holds a flock that is the shared mutex between the user's cron sync, which +/// runs every 10 minutes, and any manual sync; running the script joins that +/// mutex, whereas a built-in implementation would sit outside it and could run +/// mbsync concurrently with cron, corrupting Maildir UID state. +class MailSync : public QObject +{ + Q_OBJECT +public: + explicit MailSync(const QString &command, QObject *parent = nullptr); + + /// False when no command is configured; the UI disables its Sync button. + bool isAvailable() const { return !m_command.isEmpty(); } + bool isRunning() const; + + /// Returns false if unavailable or already running. A true return means the + /// process was handed to the event loop, not that it launched successfully: + /// a missing binary surfaces asynchronously through finished(false, ...). + bool start(); + + QString log() const { return m_log; } + +signals: + void started(); + void outputReceived(const QString &chunk); + void finished(bool success, int exitCode); + +private: + void handleReadyRead(); + void handleFinished(int exitCode, QProcess::ExitStatus status); + void handleError(QProcess::ProcessError error); + + QString m_command; + QProcess m_process; + QString m_log; +}; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f970385..c2bfc8a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -14,3 +14,4 @@ add_qtmaildir_test(interceptor) add_qtmaildir_test(htmlbuilder) add_qtmaildir_test(notmuchworker) add_qtmaildir_test(threadlistmodel) +add_qtmaildir_test(mailsync) diff --git a/tests/test_mailsync.cpp b/tests/test_mailsync.cpp new file mode 100644 index 0000000..cb8d454 --- /dev/null +++ b/tests/test_mailsync.cpp @@ -0,0 +1,239 @@ +#include <QElapsedTimer> +#include <QFile> +#include <QSignalSpy> +#include <QTemporaryDir> +#include <QtTest> + +#include "mailsync.h" + +class TestMailSync : public QObject +{ + Q_OBJECT +private slots: + void initTestCase(); + + void unavailableWhenCommandEmpty(); + void unavailableWhenCommandIsOnlyWhitespace(); + void successfulRunEmitsFinished(); + void failedRunReportsExitCode(); + void capturesOutput(); + void capturesStderrToo(); + void emitsStartedSignal(); + void logIsClearedBetweenRuns(); + void refusesConcurrentRuns(); + void canRunAgainAfterFinishing(); + void missingBinaryReportsFailureNotSilence(); + void startDoesNotBlock(); + void argumentsAreNotShellInterpreted(); + +private: + /// Writes an executable shell script into the temp dir, returns its path. + QString makeScript(const QString &name, const QString &body); + + QTemporaryDir m_dir; +}; + +void TestMailSync::initTestCase() +{ + QVERIFY(m_dir.isValid()); +} + +QString TestMailSync::makeScript(const QString &name, const QString &body) +{ + const QString path = m_dir.filePath(name); + QFile file(path); + if (!file.open(QIODevice::WriteOnly | QIODevice::Text)) + return QString(); + file.write("#!/bin/sh\n"); + file.write(body.toUtf8()); + file.write("\n"); + file.close(); + file.setPermissions(QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + return path; +} + +void TestMailSync::unavailableWhenCommandEmpty() +{ + // Braces, not parens: MailSync sync(QString()) is a function declaration. + MailSync sync{ QString() }; + QVERIFY(!sync.isAvailable()); + QVERIFY(!sync.start()); +} + +void TestMailSync::unavailableWhenCommandIsOnlyWhitespace() +{ + // A config line like `command = ` reaches here as spaces, not as empty. + // splitCommand yields nothing for it, so start() must refuse rather than + // try to launch an empty program name. + MailSync sync(QStringLiteral(" ")); + QVERIFY(!sync.start()); + QVERIFY(!sync.isRunning()); +} + +void TestMailSync::successfulRunEmitsFinished() +{ + MailSync sync(makeScript(QStringLiteral("ok.sh"), QStringLiteral("exit 0"))); + QVERIFY(sync.isAvailable()); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.first().at(0).toBool(), true); + QCOMPARE(spy.first().at(1).toInt(), 0); +} + +void TestMailSync::failedRunReportsExitCode() +{ + MailSync sync(makeScript(QStringLiteral("fail.sh"), QStringLiteral("exit 3"))); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + + QCOMPARE(spy.first().at(0).toBool(), false); + // The exit code reaches the UI: the status bar shows why sync failed. + QCOMPARE(spy.first().at(1).toInt(), 3); +} + +void TestMailSync::capturesOutput() +{ + MailSync sync(makeScript(QStringLiteral("talk.sh"), + QStringLiteral("echo syncing"))); + + QSignalSpy spy(&sync, &MailSync::finished); + QSignalSpy output(&sync, &MailSync::outputReceived); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + + QVERIFY(sync.log().contains(QStringLiteral("syncing"))); + QVERIFY(!output.isEmpty()); +} + +void TestMailSync::capturesStderrToo() +{ + // mbsync reports failures on stderr. If only stdout were captured, the log + // pane would be empty for exactly the runs the user needs to read. + MailSync sync(makeScript(QStringLiteral("noisy.sh"), + QStringLiteral("echo boom >&2\nexit 1"))); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + + QVERIFY(sync.log().contains(QStringLiteral("boom"))); +} + +void TestMailSync::emitsStartedSignal() +{ + MailSync sync(makeScript(QStringLiteral("started.sh"), QStringLiteral("exit 0"))); + + QSignalSpy started(&sync, &MailSync::started); + QSignalSpy finished(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(finished.wait(5000)); + + QCOMPARE(started.count(), 1); +} + +void TestMailSync::logIsClearedBetweenRuns() +{ + MailSync sync(makeScript(QStringLiteral("once.sh"), QStringLiteral("echo first"))); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + QVERIFY(sync.log().contains(QStringLiteral("first"))); + + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + // Stale output from the previous run must not accumulate: the log pane is + // meant to show what this sync did. + QCOMPARE(sync.log().count(QStringLiteral("first")), 1); +} + +void TestMailSync::refusesConcurrentRuns() +{ + MailSync sync(makeScript(QStringLiteral("slow.sh"), QStringLiteral("sleep 2"))); + QVERIFY(sync.start()); + // The cron sync and this one share a flock; starting twice from the GUI is + // still refused locally so the button cannot queue runs. + QVERIFY(!sync.start()); + QVERIFY(sync.isRunning()); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(spy.wait(10000)); +} + +void TestMailSync::canRunAgainAfterFinishing() +{ + MailSync sync(makeScript(QStringLiteral("again.sh"), QStringLiteral("exit 0"))); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + QVERIFY(!sync.isRunning()); + + // A refusal must be about "currently running", not a one-shot latch. + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.count(), 2); +} + +void TestMailSync::missingBinaryReportsFailureNotSilence() +{ + // Config checks the path at load time, but the script can be deleted or + // unmounted afterwards. Failing silently would leave the spinner up + // forever with no explanation. + MailSync sync(m_dir.filePath(QStringLiteral("definitely-not-here.sh"))); + QVERIFY(sync.isAvailable()); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + + QCOMPARE(spy.first().at(0).toBool(), false); + QVERIFY(!sync.log().isEmpty()); + QVERIFY(!sync.isRunning()); +} + +void TestMailSync::startDoesNotBlock() +{ + // Spec: "The UI stays usable during sync." start() must hand off to the + // event loop rather than waiting for the process. + MailSync sync(makeScript(QStringLiteral("blocker.sh"), QStringLiteral("sleep 2"))); + + QElapsedTimer timer; + timer.start(); + QVERIFY(sync.start()); + const qint64 elapsed = timer.elapsed(); + + QVERIFY2(elapsed < 500, + qPrintable(QStringLiteral("start() blocked for %1ms").arg(elapsed))); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(spy.wait(10000)); +} + +void TestMailSync::argumentsAreNotShellInterpreted() +{ + // The command comes from a config file. Running it through a shell would + // turn that value into an injection point, so the metacharacters below must + // arrive at the script as literal arguments. + const QString script = makeScript(QStringLiteral("args.sh"), + QStringLiteral("echo \"$1\"")); + + MailSync sync(QStringLiteral("%1 \"; touch %2/pwned\"") + .arg(script, m_dir.path())); + + QSignalSpy spy(&sync, &MailSync::finished); + QVERIFY(sync.start()); + QVERIFY(spy.wait(5000)); + + QVERIFY(!QFile::exists(m_dir.filePath(QStringLiteral("pwned")))); + QVERIFY(sync.log().contains(QStringLiteral("; touch"))); +} + +QTEST_MAIN(TestMailSync) +#include "test_mailsync.moc" |