diff options
| -rw-r--r-- | CHANGELOG.md | 21 | ||||
| -rwxr-xr-x | assets/mailsync.sh | 114 | ||||
| -rwxr-xr-x | assets/test_mailsync.py | 170 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 111 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 113 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-08-29-sync-status-file-design.md | 177 | ||||
| -rw-r--r-- | src/config.cpp | 8 | ||||
| -rw-r--r-- | src/config.h | 6 | ||||
| -rw-r--r-- | src/mailsync.cpp | 90 | ||||
| -rw-r--r-- | src/mailsync.h | 80 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 77 | ||||
| -rw-r--r-- | src/mainwindow.h | 25 | ||||
| -rw-r--r-- | tests/CMakeLists.txt | 20 | ||||
| -rw-r--r-- | tests/test_mailsync.cpp | 190 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 186 |
15 files changed, 1320 insertions, 68 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index d4bdc40..71a0333 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,27 @@ point at which they are stable. ## [Unreleased] +### Added + +- **The sync script reports what it did.** `mailsync.sh` now writes a small + status file at the end of every run, naming the channels it synced, whether + each of mbsync and notmuch succeeded, and whether the run was skipped because + another already held the lock. qtmaildir reads it instead of inferring the + outcome from a line in the log. Set `status` under `[sync]` to move the file; + the default matches what the script writes and no config change is needed. + +### Fixed + +- **A background sync now clears only the accounts it actually carried.** A + sync qtmaildir did not start could only be judged from the log, which cannot + say which accounts a run covered, so a successful run cleared the pending + count for every account, including ones it never touched. Their edits were + then reported as delivered while still sitting locally. A sync started from + inside qtmaildir has always narrowed this correctly; the two paths now agree. +- **A skipped sync no longer looks like a successful one.** When a run exits + because another sync already holds the lock, it carried nothing, and the + pending count stays where it was. + ## [0.28.0] - 2026-08-29 A row in the thread list is now either a conversation or a message, and it diff --git a/assets/mailsync.sh b/assets/mailsync.sh index ea81fa5..abc56c7 100755 --- a/assets/mailsync.sh +++ b/assets/mailsync.sh @@ -43,10 +43,63 @@ export HOME="${HOME:-$(getent passwd "$(id -u)" | cut -d: -f6)}" export GNUPGHOME="${GNUPGHOME:-$HOME/.gnupg}" -LOCKFILE="/tmp/mbsync.lock" +# Overridable for the test suite ONLY. A test must never take the real lock: +# that is the mutex the user's cron sync uses, so a test run holding it would +# block their mail. Production passes nothing and gets the real path. +LOCKFILE="${MAILSYNC_LOCKFILE:-/tmp/mbsync.lock}" LOGFILE="$HOME/.local/state/mailsync.log" +# What qtmaildir READS, as against the log, which is for a human (item 174). +# The application used to infer a finished run from the log's RUN END banner +# and from this lock's inode in /proc/locks. That made a human-readable line +# into wire format, said nothing about WHICH channels a run carried, and left a +# skipped run reporting nothing at all (item 125). This file is the interface; +# the log stays the log. +STATUSFILE="$HOME/.local/state/qtmaildir/syncstatus.json" + mkdir -p "$(dirname "$LOGFILE")" +mkdir -p "$(dirname "$STATUSFILE")" + +# Written atomically, because qtmaildir watches this file and a reader must +# never see a half-written one: mv within a directory is atomic, a redirect +# into the final path is not. +# +# Failure to write it is deliberately NOT fatal. The status file is a +# convenience for the application; the sync itself has already happened, and +# taking the run down over a report of it would turn a cosmetic problem into a +# mail problem. +write_status() { + local state="$1" mbsync_status="$2" notmuch_status="$3" + local started="$4" ended="$5" + shift 5 + + local channels="" sep="" channel + for channel in "$@"; do + # The two characters JSON requires escaping in a string. A channel name + # comes from ~/.mbsyncrc and is an identifier rather than free text, so + # this is belt and braces rather than a real expectation. + channel="${channel//\\/\\\\}" + channel="${channel//\"/\\\"}" + channels="${channels}${sep}\"${channel}\"" + sep=", " + done + + local tmp + tmp="$(mktemp "${STATUSFILE}.XXXXXX")" || return 0 + cat > "$tmp" <<JSON +{ + "version": 1, + "run_id": "$started", + "started": "$started", + "ended": "$ended", + "state": "$state", + "channels": [$channels], + "mbsync_status": $mbsync_status, + "notmuch_status": $notmuch_status +} +JSON + mv -f "$tmp" "$STATUSFILE" 2>/dev/null || rm -f "$tmp" +} # Rotation is NOT this script's job: /etc/logrotate.d/mailsync owns this file, # keeping seven compressed days. An earlier version also rotated by size here, @@ -54,12 +107,31 @@ mkdir -p "$(dirname "$LOGFILE")" # logrotate had just put at .1, losing a day of history and leaving an # uncompressed file where a compressed one belonged. +# Resolved HERE, before the lock and before the pipeline below, for two +# reasons. The block below is piped into tee and so runs in a subshell, where +# a "set --" cannot be seen by the parent that writes the status file; and the +# skip branch needs the list too, to report what the run WOULD have carried. +# +# -a is NOT equivalent to naming every channel: with no arguments at all mbsync +# syncs nothing and exits, which would look like a clean sync that moved no +# mail. It is reported to qtmaildir as "-a" rather than expanded, since only +# ~/.mbsyncrc knows what every channel is, and the application reads "-a" as +# "every account". +[ "$#" -gt 0 ] || set -- -a +CHANNELS=("$@") + exec 200>"$LOCKFILE" if ! flock -n 200; then # Both streams again: a caller that skipped because the cron run holds # the lock needs to be told, not left with silence and an error code. - msg="$(date -Iseconds) === SKIPPED: previous run still in progress ===" + SKIP_TS="$(date -Iseconds)" + msg="$SKIP_TS === SKIPPED: previous run still in progress ===" echo "$msg" | tee -a "$LOGFILE" >&2 + # Item 125. A skip used to report NOTHING a watcher could see: it releases + # a lock it never took, so qtmaildir's spinner waited for a completion that + # never came. A skipped run is a terminal state and says so, with -1 for + # both statuses since neither program ran. + write_status "skipped" -1 -1 "$SKIP_TS" "$SKIP_TS" "${CHANNELS[@]}" # 75 (EX_TEMPFAIL), not 1. A skip is not a failure: the other run is # doing the work. qtmaildir reports 1 as "sync failed" and shows its log # pane, which is wrong for a click that landed during the cron run, and @@ -88,17 +160,12 @@ START_TS="$(date -Iseconds)" # which is both the progress and the account name the status bar shows. # This is not a buffering problem and stdbuf does not help: the output # streams fine, there simply is none to stream. - # "$@" when channels were named, -a otherwise. Quoted and passed as - # separate words, never flattened into a string: a channel name is an - # argument, and mbsync takes an unknown one as a fatal error rather than - # skipping it, which would fail the whole run. - # - # -a is NOT equivalent to naming every channel and cannot be dropped: with - # no arguments at all mbsync syncs nothing and exits, which would look like - # a clean sync that moved no mail. - [ "$#" -gt 0 ] || set -- -a - - mbsync -V "$@" 2>&1 | while IFS= read -r line; do + # "$@" when channels were named, -a otherwise, resolved into CHANNELS + # before the lock above. Quoted and passed as separate words, never + # flattened into a string: a channel name is an argument, and mbsync takes + # an unknown one as a fatal error rather than skipping it, which would fail + # the whole run. + mbsync -V "${CHANNELS[@]}" 2>&1 | while IFS= read -r line; do echo "$(date '+%H:%M:%S') $line" done echo "${PIPESTATUS[0]}" > "$STATUS_DIR/mbsync" @@ -109,6 +176,9 @@ START_TS="$(date -Iseconds)" echo "${PIPESTATUS[0]}" > "$STATUS_DIR/notmuch" END_TS="$(date -Iseconds)" + # Also to a file, for the same subshell reason the statuses are: the parent + # writes the status file and cannot see a variable assigned in here. + echo "$END_TS" > "$STATUS_DIR/end" MBSYNC_STATUS="$(cat "$STATUS_DIR/mbsync")" NOTMUCH_STATUS="$(cat "$STATUS_DIR/notmuch")" if [ "$MBSYNC_STATUS" -eq 0 ] && [ "$NOTMUCH_STATUS" -eq 0 ]; then @@ -123,6 +193,24 @@ START_TS="$(date -Iseconds)" MBSYNC_STATUS="$(cat "$STATUS_DIR/mbsync" 2>/dev/null || echo 1)" NOTMUCH_STATUS="$(cat "$STATUS_DIR/notmuch" 2>/dev/null || echo 1)" +END_TS="$(cat "$STATUS_DIR/end" 2>/dev/null || date -Iseconds)" + +# Item 174. What qtmaildir reads, written before the exit below so it exists +# whichever way this run went. "ok" only when BOTH programs succeeded, matching +# the log banner and the exit status: a caller must not be able to read success +# here and failure there. +# +# The two statuses are reported separately as well as folded into the state, +# because they mean different things to the application: a failed mbsync means +# the edits never reached the server, while a failed notmuch means they did and +# only the local index is behind. +if [ "$MBSYNC_STATUS" -eq 0 ] && [ "$NOTMUCH_STATUS" -eq 0 ]; then + write_status "ok" "$MBSYNC_STATUS" "$NOTMUCH_STATUS" \ + "$START_TS" "$END_TS" "${CHANNELS[@]}" +else + write_status "failed" "$MBSYNC_STATUS" "$NOTMUCH_STATUS" \ + "$START_TS" "$END_TS" "${CHANNELS[@]}" +fi # Report the real outcome. The old unconditional "exit 0" meant a caller # could not distinguish a clean sync from a failed one, so qtmaildir's diff --git a/assets/test_mailsync.py b/assets/test_mailsync.py new file mode 100755 index 0000000..a0bf7ae --- /dev/null +++ b/assets/test_mailsync.py @@ -0,0 +1,170 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0-only +# +# Copyright (C) 2026 Danilo M. <danix@danix.xyz> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 2 as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +"""Checks for the status file mailsync.sh writes (item 174). + +Nothing here touches real mail or the network: mbsync and notmuch are stubs on +PATH, HOME points at a temp directory, and the lock file is redirected there +too, so a run of this suite cannot collide with the user's cron sync. + +The properties worth proving are the ones qtmaildir depends on and a reader of +the script cannot confirm: + + - a status file is written on success, on failure, and on a SKIP, which is + the case item 125 is open for + - it names the channels the run actually synced, which is what lets the + application clear its pending count per account instead of wholesale + - "-a" is reported as such, since a full run carries every account and a + reader that treats it as a channel name clears nothing + - the file is valid JSON at every moment a watcher could read it + +Run: ./test_mailsync.py +""" + +import json +import os +import stat +import subprocess +import tempfile +from pathlib import Path + +SCRIPT = Path(__file__).resolve().parent / "mailsync.sh" + +failures = [] + + +def check(name, condition, detail=""): + if condition: + print(f"ok {name}") + else: + print(f"FAIL {name}{(': ' + detail) if detail else ''}") + failures.append(name) + + +def write_stub(path, exit_code, output="", sleep_for=0): + """A stub standing in for mbsync or notmuch.""" + body = "#!/bin/bash\n" + if output: + body += f"echo {output!r}\n" + if sleep_for: + body += f"sleep {sleep_for}\n" + body += f"exit {exit_code}\n" + path.write_text(body) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +def run(tmp, args=(), mbsync_exit=0, notmuch_exit=0, lockfile=None, + hold_lock=False): + """Runs the script with stubbed binaries, returns (exit code, status dict).""" + bindir = tmp / "bin" + bindir.mkdir(exist_ok=True) + write_stub(bindir / "mbsync", mbsync_exit, "Channel work") + write_stub(bindir / "notmuch", notmuch_exit, "No new mail.") + + env = dict(os.environ) + env["HOME"] = str(tmp) + env["PATH"] = f"{bindir}:{env['PATH']}" + # Never the real /tmp/mbsync.lock: a test must not join the mutex the + # user's cron sync uses, or a run of this suite blocks their mail. + env["MAILSYNC_LOCKFILE"] = str(lockfile or (tmp / "lock")) + + proc = subprocess.run([str(SCRIPT), *args], env=env, capture_output=True, + text=True, timeout=60) + + status_path = tmp / ".local/state/qtmaildir/syncstatus.json" + status = None + if status_path.exists(): + status = json.loads(status_path.read_text()) + return proc.returncode, status + + +def main(): + with tempfile.TemporaryDirectory() as raw: + tmp = Path(raw) + + # --- a successful full run + code, st = run(tmp) + check("a clean run exits 0", code == 0, f"exit {code}") + check("a clean run writes a status file", st is not None) + if st: + check("state is ok", st.get("state") == "ok", str(st.get("state"))) + check("mbsync status recorded", st.get("mbsync_status") == 0) + check("notmuch status recorded", st.get("notmuch_status") == 0) + check("a full run reports -a", st.get("channels") == ["-a"], + str(st.get("channels"))) + check("version is 1", st.get("version") == 1) + check("carries a start and an end", bool(st.get("started")) + and bool(st.get("ended"))) + + # --- named channels are reported, which is the whole point for the + # application: it clears its pending count for these accounts only. + code, st = run(tmp, args=("work", "personal")) + check("named channels exit 0", code == 0) + if st: + check("named channels are listed", + st.get("channels") == ["work", "personal"], + str(st.get("channels"))) + + # --- a failing mbsync + code, st = run(tmp, mbsync_exit=1) + check("a failed mbsync exits nonzero", code != 0, f"exit {code}") + if st: + check("state is failed", st.get("state") == "failed", + str(st.get("state"))) + check("the failing status is recorded", + st.get("mbsync_status") == 1) + + # --- a failing notmuch, with mbsync fine. Distinguished because the + # mail DID reach the server: only the index is behind. + code, st = run(tmp, notmuch_exit=1) + check("a failed notmuch exits nonzero", code != 0) + if st: + check("a notmuch failure is still failed", + st.get("state") == "failed") + check("mbsync is recorded as fine", st.get("mbsync_status") == 0) + + # --- a SKIP, which is item 125: the run exits 75 holding no lock, and + # before this the application had nothing to clear its spinner on. + lock = tmp / "held.lock" + lock.touch() + with open(lock, "w") as handle: + holder = subprocess.Popen( + ["flock", "-x", str(lock), "sleep", "10"]) + try: + # Give flock a moment to actually take it. + subprocess.run(["sleep", "0.3"]) + code, st = run(tmp, lockfile=lock) + finally: + holder.terminate() + holder.wait() + + check("a skipped run exits 75", code == 75, f"exit {code}") + check("a skipped run still writes a status file", st is not None) + if st: + check("state is skipped", st.get("state") == "skipped", + str(st.get("state"))) + + print() + if failures: + print(f"{len(failures)} FAILED: {', '.join(failures)}") + return 1 + print("all checks passed") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index b3cda60..d788573 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -9459,3 +9459,114 @@ non-held branch, which fails with the exact text the user reported. The string is translated, since a user-facing string that misses the Italian translation ships as English inside an otherwise Italian UI: `lupdate` found it with no context warnings and `lrelease` reports 552 finished, 0 unfinished. + +## 174. An external sync's outcome can only be inferred, and never names what it carried + +**Done 2026-08-29**, unreleased. Filed as "an external `notmuch new` reaches the index without the pending count noticing"; the title changed because the premise did, see below. + +**Observed (user, from the notes):** "the statusbar still reads that an +external notmuch run can clear the pending edits without the bar noticing. +Since we own mailsync and the whole process, we should fix that." + +**Cause (verified in the code, 2026-08-28).** Item 54 built exactly one +external path and it is narrower than the note assumes. `SyncMonitor` watches +the inode of `/tmp/mbsync.lock` in `/proc/locks` +(`syncmonitor.cpp:64`), which is the file `assets/mailsync.sh` flocks, and +`onExternalSyncStateChanged()` clears `m_pendingTagEdits` only when +`MailSync::lastRunOutcome()` reads an OK from that script's log +(`mainwindow.cpp:4812-4840`). + +A `notmuch new` that is not that script takes notmuch's own write lock inside +the Xapian directory and never touches `/tmp/mbsync.lock`. Nothing observes +it: no state change, no log line to read an outcome from, so the count keeps +reporting work that has already shipped and the exit prompt offers to sync for +it. This is item 54's symptom surviving through the one route item 54 did not +cover. + +**Approach (not decided).** The note names the lever: the process is ours, so +the honest fix is to stop inferring a sync from a lock file and observe the +INDEX instead. `notmuch_database_get_revision()` gives a monotonic uuid plus +revision that moves whenever anything is committed, whoever committed it; the +worker already holds a handle and reopens it. Watching that would cover the +script, a hand run and a cron entry alike, and would make the outcome question +moot: a revision that moved is evidence the write landed, where a log line is +a report about it. + +**Constraints.** +- Clearing the count on an observed revision bump is NOT sound on its own. A + revision moves for mail arriving as well as for our edits landing, so the + bump has to be read as "the index changed, re-check" rather than "our edits + are in". What settles it is the per-message check the pending map can + already do: every entry names a message and a tag. +- `SyncMonitor` stays whatever this becomes. It answers a different question, + "is a sync running", which drives the spinner and the write hold, and a + revision counter cannot answer it. +- Item 125 is open on the same monitor and should be read alongside: a + `mailsync.sh` that exits 75 leaves the spinner running for ever. + +### What was built, and why it is not what the entry above proposed + +**The premise was corrected before any code.** The note asks for an external +`notmuch new` to clear the pending count. It must not. The count means +"confirmed tag mutations not yet known to have reached the MAIL STORE" +(`mainwindow.h`), which is the server: a tag edit is in notmuch the moment it is +made, and what is outstanding is mbsync pushing the renamed Maildir files. +`notmuch new` re-indexes local files and pushes nothing, so clearing on it would +tell the user their work was safe to quit on while it was still local. That is +item 28's defect in a new costume. + +The entry's own Approach, watching `notmuch_database_get_revision()`, was +rejected for the same reason: a revision moves when mail ARRIVES as well as when +edits land, and in neither case does it say anything about the server. + +**The user reframed the scope, and that is what shaped the fix.** The script was +written for another system and adapted; it is now qtmaildir's only consumer, so +it serves the application rather than the reverse. Where a gap is found here, the +script is reshaped to bridge it. Two facts made this safe to act on: the crontab +runs `mailsync.sh` and nothing else touches mail, and `~/bin/mailsync.sh` is a +SYMLINK into this repo, so an edit is live on the next tick with no deploy step. + +**What was actually wrong** was the reporting channel, not the syncing. The +application inferred a finished run from an inode in `/proc/locks` and from +grepping the log for `RUN END ... status=OK`, which made a human-readable line +into wire format and could not answer the question the count needs answered: +which channels did this run carry? The local path has always narrowed its clear +to the accounts it carried; the external path could not, and cleared everything, +so an edit to an untouched account was reported as delivered. + +**Built:** `assets/mailsync.sh` writes `~/.local/state/qtmaildir/syncstatus.json` +atomically at the end of every run including a skip; `MailSync::readStatus()` +reads it; `MainWindow` prefers it over the log banner, narrows the clear by +channel through `Account::syncChannel()`, and clears nothing on `skipped` or +`failed`. The log banner and `lastRunOutcome()` stay as the fallback for a +missing file, which is what a first run after upgrading looks like. + +Design: `specs/2026-08-29-sync-status-file-design.md`. + +**Item 125 is half closed by this.** A skipped run is now a terminal state the +application can see, so the spinner has something definite to clear on. Whether +`SyncMonitor` should ALSO time out an observation it never saw end is untouched +and stays in that item. + +### Two bugs found while wiring it in, both worth keeping + +**A test read the developer's real sync state, twice.** A `[sync]` section +naming only `log` leaves `syncStatus()` defaulting to the real +`~/.local/state/qtmaildir/syncstatus.json`, so two existing tests asserting that +a FAILED run leaves the count alone read the last real cron run, found `ok`, and +cleared. Pinning only `status` has the mirror problem, and that is how the third +one broke. This is the `/proc/locks` trap of item 61 in a new place, and the +same answer applies: **a sync test must pin BOTH keys inside its own +QTemporaryDir.** `noSyncTestReadsTheRealSyncState()` is the guard, modelled on +`noTestCanSeeTheRealLockTable()`. + +**`Qt::ISODate` carries no milliseconds, and a staleness check on it is wrong by +up to a second.** The status file is preferred only when it describes THIS run, +compared against when the lock appeared, so a stale success cannot outrank a +fresh failure. But the script writes `date -Iseconds`: measured in a standalone +probe, an ISODate round trip of "now" comes back **329 ms behind**, so a fast +sync's own file parsed as older than the lock and was judged stale. The fallback +still worked, so the symptom would have been "the channel narrowing never +happens" with nothing failing. One second of slack matches the precision the +format actually carries, and cannot readmit a genuinely stale file when cron +runs ten minutes apart. diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 8edc6bd..2cc3b5c 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -193,7 +193,7 @@ taking that too literally. | 124 | The worker reads the index directory as the mail root | defect | S | **done** 2026-08-20, unreleased. `mailRootOf()` over `NOTMUCH_CONFIG_MAIL_ROOT`, correct under both layouts. Verified by migrating the developer's own index to NVMe the same day: cold start 38.6 s to 0.67 s | -| 125 | A skipped sync leaves the spinner running for ever | defect | S | open, 2026-08-20, found by hand. `mailsync.sh` exits 75 (EX_TEMPFAIL) when another run holds the lock; the indicator never clears, and a held edit waits for a completion that never comes | +| 125 | A skipped sync leaves the spinner running for ever | defect | S | open, 2026-08-20, **half closed 2026-08-29 by item 174**: a skipped run now writes `state: skipped` to the status file, so the application can see it happened and clears the spinner without clearing the count. What remains is whether `SyncMonitor` should also time out an observation it never saw end, which covers a run that dies without writing anything at all | | 126 | A link with `target="_blank"` does nothing when clicked | defect | S | **done** 2026-08-20, unreleased. `createWindow()` returns a relay page that receives the navigation, hands the URL to the browser and refuses. The URL cannot be read in `createWindow()` itself, which is why a relay rather than a lookup | | 127 | A link's context menu offers four browser actions that cannot work | defect | XS | **done** 2026-08-20, unreleased. Three Open-in actions removed, `CopyLinkToClipboard` kept. Item 126 made them more dangerous rather than less: with a real `createWindow()` they would have started working | @@ -250,13 +250,14 @@ taking that too literally. | 176 | Undoing a thread-scoped action applies its inverse to messages it never changed | defect | S | **done 2026-08-28**, unreleased, on `thread-row-identity`. `NotmuchWorker::applyTags()` reads each message's tags before writing and reports only the ids whose tags actually MOVED; a `TagCommand` base carries that effective set for both `ThreadTagCommand` and `MessageTagCommand`, which had the same defect on a multi-row selection. `tagsApplied` does NOT fire on an empty effective list, since an empty change would push an undo entry whose inverse adds a tag no message ever carried, the same bug one step later. `sendThreadTagChange` gained `onlyMessageIds` so it keeps its thread-scoped REPAINT while restricting the WRITE: the card that changed on screen and the messages that changed on disk are different sets on purpose. **The spec's own plan said item 177 would make a thread undo honest and shrink this to the multi-row case; that was wrong and is corrected in the spec**, an undo inverts an EFFECT, not a scope | | 177 | A thread row means both a message and a conversation, and neither consistently | design | L | **done 2026-08-28**, unreleased, on `thread-row-identity`, eleven commits. Spec: `specs/2026-08-28-thread-row-identity-design.md`. `ThreadListModel::isConversationRow()` is the single predicate and `scopeForSelection()` the single resolver, replacing the `scopeFor()`/`messageScopeFor()` pair that made the CALLER choose. A summary with `totalCount == 1` is unchanged. **Reverses items 108, 110 and 111**, and the user confirmed they are happy to lose the two-tier chips; the `*_thread` submenu and its five action names are deleted with an `### Upgrading` note. Item 112's hiding rule is reversed too: with the absolute entries gone, hiding the toggle on a mixed selection leaves no way to act, so it is a catch-all and the write direction moves with the label. Membership is the union, with two user decisions kept (never evict the current row; an asked-for write evicts at once, an automatic one defers) and one documented lag (a long thread's summary is not updated by a message write, so reading its last unread message waits for the next query). Dashboard from a `ThreadDigest` read by its own worker walk. Two traps found while building: a `QStackedWidget` takes the LARGEST minimum width of its pages and the hidden dashboard was raising the pane's minimum to 395px over MainWindow's 300px floor, caught by an existing resize test; and the pane now holds two `TagStrip`s, so both are named | | 178 | Delete and Restore judge a conversation on one message | defect | XS | **done 2026-08-29**, unreleased, on `thread-row-identity`. `ThreadDigest` carries every message's path, collected by the walk it already makes, so the predicate tests the whole conversation. Known for the SINGLE selected conversation row the digest was requested for; any other selection falls back to the summary's one path, which is the pre-177 answer, deliberately left no worse rather than given a second differently-wrong rule. Section in the closed file | -| 174 | An external `notmuch new` reaches the index without the pending count noticing | defect | S | open, 2026-08-28, from the notes. Item 54 cleared the count for a sync run by `mailsync.sh`, which is what `SyncMonitor` watches; a bare `notmuch new` (a hand run, or a cron entry that is not the script) takes notmuch's own write lock and touches `/tmp/mbsync.lock` not at all, so nothing observes it. The user's framing is the approach: we own `mailsync.sh` and the whole process | +| 174 | An external sync's outcome can only be inferred, and never names what it carried | defect | S | **done 2026-08-29**, unreleased. The premise was corrected first: a bare `notmuch new` must NOT clear the count, since the edits are in the index but not on the server, and the item's own proposal to watch `notmuch_database_get_revision()` was rejected for that reason. `mailsync.sh` writes a JSON status file instead, naming the channels a run carried; the external path now narrows its clear the way the local one always has, and a `skipped` run clears nothing. Log fallback kept. Section in the closed file | | 175 | The send countdown says Undo, and cannot be skipped | presentation | XS | open, 2026-08-28, from the notes. Two changes in one control: the button reads Abort, and a second button sends immediately rather than waiting the countdown out | | 179 | Undo is one level deep in practice, and there is no Redo | workflow | ? | open, 2026-08-29, from the notes. The `QUndoStack` is real and multi-level; what is missing is a `redo` action (absent from `knownActions()`, never called) and an answer to the stack being CLEARED on every new query (`mainwindow.cpp:3458`), which is what makes a deep stack behave like a shallow one. The clear has a correct reason and cannot simply be removed. Redo re-applies a write to real mail, so item 176's rule binds it too | | 180 | The repaint rules are discovered one hole at a time | maintenance | S-L | open, 2026-08-29, from the notes, and a QUESTION rather than a defect. Items 105, 107, 109, 110 and 170 are each one hole in the same surface, all found by hand. Three mechanisms (optimistic repaint, `syncViewMembership()`, revert) agree by documentation rather than by code. Cheapest answer is one invariant test, not a rewrite; the user decides which, and that decides the size | | 181 | The thread dashboard does not follow a write to the conversation it shows | defect | XS | **done 2026-08-29**, unreleased, on `thread-row-identity`, from the notes. The dashboard draws a `ThreadDigest` built by the worker from the INDEX, which arrived only on selection, so a tag write moved the model and the card and left the pane reporting the count the conversation had when it was opened. Reachable from the dashboard's OWN Mark all read button. Re-requested from `onTagsApplied()`, where the write is confirmed: queued beside the write it races it and answers from the state before it, which is how the first fix passed review and failed the test. Section in the closed file | | 182 | An edit made during a sync is announced twice and never says it is waiting | defect | XS | **done 2026-08-29**, unreleased, on `thread-row-identity`, found by hand. The hold branches set a deliberately NON-transient label; all three callers overwrote it a line later with the bare action, so the user was told the write had landed and then told again when it really did. `announceAction()` adds the wait to the action rather than replacing it, since that announcement is what stands in for the confirmation dialog this project rules out. Section in the closed file | | 183 | `undoingAMarkReadRestoresOnlyWhatWasUnread` fails about 1 run in 9 under the full suite | testing | ? | open, 2026-08-29, measured. Item 176's regression test, which guards the undo that rewrote 44 messages of real mail. Nine runs on master: 4 standalone, 3 under `ctest -R mainwindow`, 3 under the FULL parallel suite, and the single failure was in the last group. Not a regression, the base commit behaves the same. Probably the same root cause as item 136 and worth solving with it | +| 184 | New mail waits up to ten minutes, because sync is a fixed cron tick | workflow | ? | open, 2026-08-29, from the user: the 10 minute tick "has always bothered me", and it is already a compromise down from 30. Outgoing edits are immediate (`auto_sync_delay_ms`), so this is the INCOMING half only. Polling faster is not the answer; IMAP IDLE is, and it lives in a watcher that triggers `mailsync.sh`, NOT in qtmaildir, which does no network protocol work. Needs decisions first: which watcher, whether it packages on Slackware, and what the server supports. **Blocked on 174**, whose status file is the reporting channel this needs anyway | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1223,48 +1224,6 @@ rich-text composer with the original inside it, editable. same want ("show me what I am writing"). If this is built, 133 is moot; if this is deferred, 133 is the thing to do instead. Do not build both. -## 174. An external `notmuch new` reaches the index without the pending count noticing - -**Observed (user, from the notes):** "the statusbar still reads that an -external notmuch run can clear the pending edits without the bar noticing. -Since we own mailsync and the whole process, we should fix that." - -**Cause (verified in the code, 2026-08-28).** Item 54 built exactly one -external path and it is narrower than the note assumes. `SyncMonitor` watches -the inode of `/tmp/mbsync.lock` in `/proc/locks` -(`syncmonitor.cpp:64`), which is the file `assets/mailsync.sh` flocks, and -`onExternalSyncStateChanged()` clears `m_pendingTagEdits` only when -`MailSync::lastRunOutcome()` reads an OK from that script's log -(`mainwindow.cpp:4812-4840`). - -A `notmuch new` that is not that script takes notmuch's own write lock inside -the Xapian directory and never touches `/tmp/mbsync.lock`. Nothing observes -it: no state change, no log line to read an outcome from, so the count keeps -reporting work that has already shipped and the exit prompt offers to sync for -it. This is item 54's symptom surviving through the one route item 54 did not -cover. - -**Approach (not decided).** The note names the lever: the process is ours, so -the honest fix is to stop inferring a sync from a lock file and observe the -INDEX instead. `notmuch_database_get_revision()` gives a monotonic uuid plus -revision that moves whenever anything is committed, whoever committed it; the -worker already holds a handle and reopens it. Watching that would cover the -script, a hand run and a cron entry alike, and would make the outcome question -moot: a revision that moved is evidence the write landed, where a log line is -a report about it. - -**Constraints.** -- Clearing the count on an observed revision bump is NOT sound on its own. A - revision moves for mail arriving as well as for our edits landing, so the - bump has to be read as "the index changed, re-check" rather than "our edits - are in". What settles it is the per-message check the pending map can - already do: every entry names a message and a tag. -- `SyncMonitor` stays whatever this becomes. It answers a different question, - "is a sync running", which drives the spinner and the write hold, and a - revision counter cannot answer it. -- Item 125 is open on the same monitor and should be read alongside: a - `mailsync.sh` that exits 75 leaves the spinner running for ever. - ## 175. The send countdown says Undo, and cannot be skipped **Observed (user, from the notes):** "the countdown popup has a 'undo' button @@ -1447,3 +1406,69 @@ same discipline applies here. the suite with a failure that masks the next real one. - The suite baseline is currently ONE known failure. Anything that makes it two intermittently costs the property that a red suite means something. + +## 184. New mail waits up to ten minutes, because sync is a fixed cron tick + +**Observed (user, 2026-08-29):** "the 10 minutes fixed tick has always bothered +me, I want the changes to my mail to be immediate, the 10 minutes mark is a +compromise, it had started at 30 min and was awful." + +**Split the want in two, because only half of it is open.** + +- **Outgoing is already immediate.** An edit arms `auto_sync_delay_ms`, 2 + seconds by default (item 71), so marking a message read reaches the server + without waiting for the tick. +- **Incoming is the gap.** Mail that arrives is invisible until the next cron + run of `mailsync.sh`, so the wait is uniform on [0, 10] minutes with a mean + of five. + +**Why a faster tick is not the fix.** Polling every minute is ten times the +connections and the server load for a mean wait of thirty seconds, and it is +still a poll: the wait is bounded by the interval however small it gets. The +answer to "tell me when something arrives" is IMAP IDLE, where the server holds +the connection and speaks first. + +**Where IDLE may live, and where it must not.** `AGENTS.md` states this +application does NO network protocol work at all: fetching and sending are +external commands, which is what keeps a mail client out of TLS, authentication +and an IMAP state machine. IDLE inside qtmaildir would break that rule outright +and is not on the table. + +It does not need to be inside. mbsync has no IDLE mode, being a batch syncer +that runs and exits; the tools that hold a connection and TRIGGER a sync are +separate programs (`goimapnotify` and the older `imapnotify` are the usual +ones). That shape fits the architecture exactly: the watcher replaces the cron +line, runs `mailsync.sh <channel>` on activity, and qtmaildir stays as ignorant +of IMAP as it is today. The script's flock still serialises a triggered sync +against a manual one. + +**Blocked on item 174**, and not merely sequenced after it. A watcher makes +syncs arrive at unpredictable times rather than on a known tick, which makes +the application's current guesswork about external runs worse. Item 174's +status file is the reporting channel this needs, and building it first means +the watcher has a tested one to write into. + +**Decisions needed before any code, none of which can be made by reading this +repository.** + +- **Which watcher**, or a small one of ours. A third-party daemon means a + SlackBuild in the `my-slackbuilds` repo and a package to maintain. +- **What the mail server supports.** IDLE is optional in IMAP, and a server + without it leaves polling as the only mechanism. +- **How many connections.** One per account per watched folder, held open + indefinitely; some servers cap concurrent connections. + +**Constraints.** + +- **Keep a slow cron tick as a backstop.** A held connection drops on network + sleep, a server restart or a laptop suspend, and a watcher that dies silently + stops mail altogether, which is worse than a ten minute wait. Belt and + braces: the watcher for latency, a slow tick so a dead watcher is survivable. +- **The lock is the shared mutex** and stays so. A triggered sync, a cron sync + and a click in qtmaildir must continue to serialise through + `/tmp/mbsync.lock`, or two mbsync runs corrupt Maildir UID state. +- **This is not a qtmaildir daemon.** A long-running qtmaildir service was + considered and rejected in the same conversation: it answers none of items + 174 or 125 better than a file does, and it adds a process that can wedge and + take mail delivery with it. What the user wants is a watcher, which is a + different thing in a different place. diff --git a/docs/superpowers/specs/2026-08-29-sync-status-file-design.md b/docs/superpowers/specs/2026-08-29-sync-status-file-design.md new file mode 100644 index 0000000..b5c3a40 --- /dev/null +++ b/docs/superpowers/specs/2026-08-29-sync-status-file-design.md @@ -0,0 +1,177 @@ +# A status file, so qtmaildir is told about a sync instead of inferring it + +Item 174, and item 125 with it. 2026-08-29. + +## The premise, which is the user's and which reverses the usual direction + +`assets/mailsync.sh` was written for another system and adapted for qtmaildir. +It is now the only consumer, so the script serves the application rather than +the application accommodating the script. Where a gap is found in qtmaildir, +the script is reshaped to bridge it. mbsync and IMAP stay out of the +application; the script stays a commodity. + +Two facts settle the scope, both verified rather than assumed: + +- The user's crontab runs `mailsync.sh` every ten minutes and nothing else + touches mail. There is no third-party mbsync and no bare `notmuch new`. +- `~/bin/mailsync.sh` is a SYMLINK to `assets/mailsync.sh` in this repo, so an + edit here is live on the next cron tick with no deploy step. + +## What is actually wrong + +qtmaildir learns about a sync it did not start through two indirect channels, +both of which are inferences about a process that has already exited: + +- **An inode in `/proc/locks`** (`syncmonitor.cpp:64`), watching the file the + script flocks. This answers "is a sync running", and it is the reason item + 125 exists: a skipped run (exit 75) releases no lock the monitor ever saw + held, so the spinner runs for ever. +- **A grep of the log** for `RUN END ... status=OK` + (`MailSync::lastRunOutcome`, one production caller at `mainwindow.cpp:5182`). + This makes a human-readable log line into load-bearing wire format: anyone + reformatting that banner breaks the application silently. + +Neither channel carries what the application needs, which is WHICH CHANNELS a +run synced, WHEN, and WITH WHAT RESULT. The consequence is visible in the code: +the local sync path narrows its clear to the accounts the run carried +(`mainwindow.cpp:4623`, `subtract(accountsThisRunCarried)`), while the external +path cannot and does a blanket `m_pendingTagEdits.clear()` at `:5183`. The +external path is coarser than the local one for want of information the script +has and does not report. + +## What is NOT wrong, and must not be "fixed" + +**A bare `notmuch new` cannot clear the pending count, and should not.** The +count means "confirmed tag mutations not yet known to have reached the MAIL +STORE" (`mainwindow.h:1763`), which is the server. A tag edit is in notmuch the +moment it is made; what is outstanding is mbsync pushing the renamed Maildir +files. `notmuch new` re-indexes local files and pushes nothing. + +Item 174's own "Approach" section proposes watching +`notmuch_database_get_revision()`. That is rejected here: a revision moves when +mail ARRIVES as well as when our edits land, and in neither case does it say +anything about the server. Clearing on a revision bump would make the indicator +claim work is safe to quit on when it is still local, which is item 28's defect +returning in a new costume. The item's own constraint gestures at this and then +resolves it with a per-message re-check, but that check reads notmuch, which +also cannot see the server. + +The status file is the correct instrument precisely because the script knows +what notmuch cannot: whether MBSYNC ran and what it returned. + +## The design + +The script writes one JSON file at the end of every run, including a skipped +one. qtmaildir watches that file. + +**Path:** `~/.local/state/qtmaildir/syncstatus.json`, configurable beside +`syncLog` for the same reasons that key exists. Note it goes under the +application's own state directory, not beside `mailsync.log` in +`~/.local/state/`: the log is the script's, the status file is the interface. + +**Shape:** + +```json +{ + "version": 1, + "run_id": "2026-08-29T10:33:07+02:00", + "started": "2026-08-29T10:33:07+02:00", + "ended": "2026-08-29T10:33:41+02:00", + "state": "ok", + "channels": ["work", "personal"], + "mbsync_status": 0, + "notmuch_status": 0 +} +``` + +- `state` is `ok`, `failed` or `skipped`. Three states, not a boolean, because + a skip is neither: item 125 exists because a skip currently reads as neither + success nor failure and so resolves nothing. +- `channels` is the channel list the run actually synced, or `["-a"]` for a + full run. This is what lets the external path narrow its clear the way the + local path already does. +- `version` is refused rather than guessed at by a reader that does not know + it, following the rule the rules file already uses for `kFormatVersion`. + +**Written atomically**, to a temporary file in the same directory and then +`mv`, which is atomic within a filesystem. A reader watching the file must +never see a half-written one, and qtmaildir will be watching it while it is +written. + +**The log keeps its banner.** `RUN END` stays exactly as it is, and +`MailSync::lastRunOutcome()` stays with its eight tests. It becomes the +FALLBACK for a status file that is missing or unreadable, which is what a first +run after upgrading looks like, rather than being deleted in the same change +that adds its replacement. + +## What each side does + +**`assets/mailsync.sh`.** Collect the channel list, write the file on every +exit path. The skip branch at the top exits before the status directory exists, +so it needs its own write. Two properties recorded as load-bearing survive +untouched: printing to stdout as well as the log, and exiting with the real +status. + +**`src/mailsync.{h,cpp}`.** A `SyncStatus` value struct and a static reader, +beside `lastRunOutcome()` and in the same shape: a pure function of a path, +returning a value with an Unknown-equivalent for every failure. No widget, no +process. + +**`src/syncmonitor.{h,cpp}`.** Watch the status file with `QFileSystemWatcher`, +beside the existing `/proc/locks` polling rather than replacing it. The lock +answers "a sync is running now" and the file answers "a run finished and here +is what it did"; these are different questions and a completion record cannot +answer the first. + +**`src/mainwindow.cpp`.** The external path at `:5182` reads the status file +instead of the log, and narrows its clear by channel, resolving channels back +to accounts through `Account::channel`. A `skipped` state clears the spinner +without clearing the count, which is item 125. + +## Constraints + +- **The script is live.** Every edit reaches real mail within ten minutes. + Writing the status file is purely additive and nothing reads it until the + application does, so a partial state is harmless, but the ORDER matters: the + script lands first and is left to run for a few cycles before the reader is + written. +- **`Account::channel` may differ from the account key** and may be empty, + in which case the key is the channel. A reverse lookup must handle both, and + a channel naming no account must be ignored rather than dropped on the floor + silently. +- **`-a` means every account**, and is not the same as a list naming them all. + A reader that treats `["-a"]` as an unknown channel clears nothing on exactly + the run that carried everything. +- **Only a successful run may clear the count**, per the rule the local path + states at `:4614`. `failed` and `skipped` clear no edits. +- **Unknown changes no state**, exactly as `SyncOutcome::Unknown` and + `SyncMonitor::State::Unknown` are treated today. +- **Two readers, one format.** This is the `rules.json` situation again, a bash + writer and a C++ reader agreeing by test rather than by shared code. The + discipline in AGENTS.md under "Changing the rule format" applies: change both + sides together, bump the version only for a breaking change, and run both + suites. +- **No new dependency for JSON.** The script writes it with `printf`, which is + why the shape above has no nesting; qtmaildir reads it with `QJsonDocument`, + which it already uses for `queries.json`. + +## Tests + +- `assets/hooks/`-style Python test for the script, in the shape the two hook + suites already take: run it with a stub `mbsync` and `notmuch` on PATH, + assert the file's contents for ok, failed and skipped. +- `test_mailsync.cpp`: the reader, against files written by hand. Every failure + mode returns the Unknown equivalent, including a truncated file, an unknown + version, and a file that is not JSON at all. +- `test_mainwindow.cpp`: the external path clears the count for a channel it + carried and leaves an account it did not, which is the behaviour the blanket + clear cannot express. And a `skipped` run clears the spinner and no edits. + +## What this deliberately does not do + +- No daemon, no socket, no D-Bus. The file is the interface. +- No change to how a LOCAL sync reports: it has the process and its exit + status, which is better evidence than a file. +- No backfill of item 125's other half. The spinner is cleared by a `skipped` + record here; whether `SyncMonitor` should also time out an observation it + never saw end is a separate question and stays in item 125. diff --git a/src/config.cpp b/src/config.cpp index 8b784ba..0f0caf1 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -475,6 +475,14 @@ void Config::load(const QString &path) if (m_syncLog.isEmpty()) m_syncLog = MailSync::defaultLogPath(); + // The status file the script writes (item 174), beside the log and unvalidated + // for the same reason: a fresh install has none until the first sync runs, + // and a missing one reads as SyncState::Unknown when the time comes. + m_syncStatus = + settings.value(QStringLiteral("sync/status")).toString().trimmed(); + if (m_syncStatus.isEmpty()) + m_syncStatus = MailSync::defaultStatusPath(); + // 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 26fc1c1..1ab923d 100644 --- a/src/config.h +++ b/src/config.h @@ -381,6 +381,11 @@ public: /// clear on a cron sync, which is exactly the defect this exists to fix. QString syncLog() const { return m_syncLog; } + /// The status file assets/mailsync.sh writes, which is what the + /// application READS to learn what a run it did not start actually did + /// (item 174). The log beside it is for a human. + QString syncStatus() const { return m_syncStatus; } + /// Optional alternate notmuch config file. Empty means "let notmuch decide". QString notmuchConfig() const { return m_notmuchConfig; } @@ -548,6 +553,7 @@ private: QString m_syncCommand; QString m_syncLog; + QString m_syncStatus; int m_toolbarIconSize = 24; QString m_notmuchConfig; QString m_dateFormat; diff --git a/src/mailsync.cpp b/src/mailsync.cpp index 10e5ef7..caf61ef 100644 --- a/src/mailsync.cpp +++ b/src/mailsync.cpp @@ -21,6 +21,9 @@ #include <QCoreApplication> #include <QDir> #include <QFile> +#include <QJsonArray> +#include <QJsonDocument> +#include <QJsonObject> #include <QRegularExpression> namespace { @@ -251,6 +254,93 @@ QString MailSync::defaultLogPath() return QDir::homePath() + QStringLiteral("/.local/state/mailsync.log"); } +QString MailSync::defaultStatusPath() +{ + // Hardcoded to match assets/mailsync.sh for the same reason defaultLogPath + // is: the script builds it from $HOME, and QStandardPaths would derive a + // path the script never writes. + // + // Under the application's own state directory rather than beside + // mailsync.log, and the split is deliberate: the log belongs to the script + // and a human reads it, while this file is the interface between the two + // programs. + return QDir::homePath() + + QStringLiteral("/.local/state/qtmaildir/syncstatus.json"); +} + +SyncStatus MailSync::readStatus(const QString &statusPath) +{ + // Every failure below returns this untouched, so Unknown is the default + // rather than something each branch has to remember to set. + SyncStatus status; + + if (statusPath.isEmpty()) + return status; + + QFile file(statusPath); + if (!file.open(QIODevice::ReadOnly)) + return status; + + // The whole file: it holds one run and is a few hundred bytes. The cap is + // against a path that is not the file we think it is, since a reader on the + // UI thread must not swallow something enormous by mistake. + constexpr qint64 kMaxBytes = 64 * 1024; + if (file.size() > kMaxBytes) + return status; + + QJsonParseError error{}; + const QJsonDocument doc = QJsonDocument::fromJson(file.readAll(), &error); + if (error.error != QJsonParseError::NoError || !doc.isObject()) + return status; + + const QJsonObject object = doc.object(); + + // Refused rather than guessed at, the rule the rules file already follows: + // a later version may mean something different by these same field names, + // and acting on it would be worse than observing nothing. The script writes + // 1 and both sides bump together. + if (object.value(QStringLiteral("version")).toInt() != 1) + return status; + + const QString state = object.value(QStringLiteral("state")).toString(); + if (state == QLatin1String("ok")) + status.state = SyncState::Ok; + else if (state == QLatin1String("failed")) + status.state = SyncState::Failed; + else if (state == QLatin1String("skipped")) + status.state = SyncState::Skipped; + else + return status; // An unrecognised state is not a fourth kind of run. + + const QJsonArray channels = + object.value(QStringLiteral("channels")).toArray(); + for (const QJsonValue &value : channels) { + const QString channel = value.toString(); + if (channel.isEmpty()) + continue; + // "-a" is the script's word for "every channel", not the name of one. + // Kept as a flag so a caller cannot match it against configured + // channels, find nothing, and clear nothing on the run that carried + // everything. + if (channel == QLatin1String("-a")) + status.everyChannel = true; + else + status.channels.append(channel); + } + + status.mbsyncStatus = + object.value(QStringLiteral("mbsync_status")).toInt(-1); + status.notmuchStatus = + object.value(QStringLiteral("notmuch_status")).toInt(-1); + + status.started = QDateTime::fromString( + object.value(QStringLiteral("started")).toString(), Qt::ISODate); + status.ended = QDateTime::fromString( + object.value(QStringLiteral("ended")).toString(), Qt::ISODate); + + return status; +} + SyncOutcome MailSync::lastRunOutcome(const QString &logPath) { if (logPath.isEmpty()) diff --git a/src/mailsync.h b/src/mailsync.h index a826f43..d614dbc 100644 --- a/src/mailsync.h +++ b/src/mailsync.h @@ -18,6 +18,7 @@ #pragma once +#include <QDateTime> #include <QObject> #include <QProcess> #include <QString> @@ -76,6 +77,64 @@ enum class SyncOutcome { Failed, }; +/// What a finished run was, from the status file (item 174). +/// +/// Three states rather than SyncOutcome's two, and the third is the point: +/// a run that SKIPPED because another held the lock is neither a success nor a +/// failure, and having no way to say so is why item 125 left the spinner +/// running for ever. +enum class SyncState { + Unknown, + Ok, + Failed, + Skipped, +}; + +/// One finished run of the sync script, as the script itself reported it. +/// +/// This exists because the application used to INFER a finished run, from an +/// inode in /proc/locks and from grepping the log for its RUN END banner. That +/// made a human-readable line into wire format, and it could not answer the +/// question the pending count actually needs answered: which channels did this +/// run carry? The local sync path has always narrowed its clear to the accounts +/// it carried; the external path could not, and cleared everything. +/// +/// Written by assets/mailsync.sh, which is the only producer. The two agree by +/// TEST rather than by shared code, exactly as the two readers of rules.json +/// do: assets/test_mailsync.py pins the writer, test_mailsync.cpp pins the +/// reader, and one test runs the real script and reads what it wrote. +struct SyncStatus +{ + SyncState state = SyncState::Unknown; + + /// The channels the run synced. Empty when `everyChannel` is true. + QStringList channels; + + /// The run covered every account, which the script reports as "-a". + /// + /// Carried as a flag rather than left as the literal string in `channels`, + /// because "-a" is not a channel name: a caller matching it against + /// configured channels finds nothing and clears nothing, on exactly the run + /// that carried everything. + bool everyChannel = false; + + /// Reported separately as well as folded into `state`, because they mean + /// different things: a failed mbsync means the edits never reached the + /// server, while a failed notmuch means they did and only the local index + /// is behind. -1 for a run where neither program ran. + int mbsyncStatus = -1; + int notmuchStatus = -1; + + QDateTime started; + QDateTime ended; + + /// True only for a run that completed with both programs succeeding. + /// Nothing else may clear the pending count, per the rule the local path + /// states: clearing on a failure asserts the edits reached the mail store + /// when the sync is exactly what failed to put them there. + bool carriedEdits() const { return state == SyncState::Ok; } +}; + /// Runs the configured external sync command. /// /// qtmaildir deliberately does not implement sync itself. The existing script @@ -125,6 +184,27 @@ public: /// Anything unreadable, absent or unmarked is Unknown. static SyncOutcome lastRunOutcome(const QString &logPath); + /// Reads the status file assets/mailsync.sh writes (item 174). + /// + /// Preferred over lastRunOutcome(), which stays as the fallback for a file + /// that is missing or unreadable: that is what a first run after upgrading + /// looks like, and deleting a working mechanism in the same change that + /// adds its replacement leaves two broken things instead of one. + /// + /// Anything unreadable, absent, malformed or of an unrecognised version + /// returns a default SyncStatus, whose state is Unknown. Callers must + /// change no state on Unknown, exactly as they must for SyncOutcome and + /// SyncMonitor::State: it is the absence of evidence, not evidence of + /// absence. + /// + /// A whole-file read rather than a tail, unlike lastRunOutcome(): the file + /// holds one run and is a few hundred bytes, where the log holds every run + /// of the day. + static SyncStatus readStatus(const QString &statusPath); + + /// Where the status file lives when the config names none. + static QString defaultStatusPath(); + signals: void started(); void outputReceived(const QString &chunk); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 1da9ac7..01c0251 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -5089,6 +5089,10 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) return; m_externalSyncBusy = true; + // When this run began, so the status file it leaves can be told from + // one an earlier run left (item 174). A stale file must not be read as + // this run's result. + m_externalSyncStartedAt = QDateTime::currentDateTime(); updateSyncControls(); m_statusLabel->setText(tr("Background sync running...")); m_announcedExternalSync = true; @@ -5174,12 +5178,54 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) // 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) { + // The process that ran this sync is gone and its exit status with it, + // so what it did has to be read from what it left behind. + // + // Item 174: the status file the script writes, which says which + // CHANNELS the run carried. The RUN END line in the log remains the + // fallback for a status file that is missing or unreadable, which is + // what a first run after upgrading looks like; it cannot name channels, + // so that path keeps the old blanket clear. + // + // Anything other than a definite success changes nothing, on either + // path: the local rule is that only a SUCCESSFUL sync may clear the + // count, and Unknown is the absence of evidence rather than evidence of + // success. A SKIPPED run is neither, and clears nothing: the other run + // is doing the work and this one carried none of it. + // The status file is preferred, but only when it describes THIS run. + // A stale one outranking a fresh log would be worse than not having it: + // an old success would clear the count for a run that has just failed, + // which is the indicator lying in the direction that loses work. + // + // "This run" is judged on the file being at least as new as the sync + // that just ended. m_externalSyncStartedAt is when the lock appeared, + // and the script writes the file immediately before exiting, so a file + // older than that belongs to an earlier run. + const SyncStatus status = MailSync::readStatus(m_config.syncStatus()); + + // The script writes `date -Iseconds`, which carries no milliseconds, so + // a file written in the same second as the lock appeared parses as up + // to 999ms EARLIER than it. Measured: an ISODate round trip of "now" + // comes back 329ms behind. A plain `>=` therefore judges a fast sync's + // own status file stale and falls back to the log, which is the + // opposite of the intent. + // + // One second of slack, matching the precision the format actually + // carries. This cannot readmit a genuinely stale file: cron runs ten + // minutes apart, and a run whose file is a second old IS this run. + constexpr qint64 kTimestampSlackMs = 1000; + const bool statusIsForThisRun = + status.state != SyncState::Unknown + && (!m_externalSyncStartedAt.isValid() || !status.ended.isValid() + || status.ended.msecsTo(m_externalSyncStartedAt) + <= kTimestampSlackMs); + + const bool carried = + statusIsForThisRun + ? status.carriedEdits() + : MailSync::lastRunOutcome(m_config.syncLog()) == SyncOutcome::Ok; + + if (carried) { m_pendingTagEdits.clear(); // Cleared HERE, before flushHeldEdits() below, and the ordering is @@ -5188,10 +5234,21 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) // 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(); + // name the same account. + // + // WHICH accounts, when the status file said. A run naming channels + // carried those and no others, so clearing the whole set would + // report an untouched account's edits as shipped. `-a` and the log + // fallback both mean every account, where the blanket clear is + // right. + if (!status.everyChannel && !status.channels.isEmpty()) { + for (const Account &account : m_config.accounts()) { + if (status.channels.contains(account.syncChannel())) + m_editedAccounts.remove(account.key); + } + } else { + m_editedAccounts.clear(); + } updatePendingIndicator(); } } diff --git a/src/mainwindow.h b/src/mainwindow.h index 2b217d2..9c93f4f 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -202,6 +202,20 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// Item 174. The set of accounts with edits not yet known to have reached + /// the mail store, so a test can assert that an external sync cleared the + /// accounts it carried and ONLY those. + QSet<QString> editedAccountsForTesting() const { return m_editedAccounts; } + + /// Marks an account edited, standing in for the write funnels: a test + /// asserting which accounts a sync clears needs more than one of them + /// edited, and driving two real writes through a worker to arrange that + /// would test the funnels rather than the clearing. + Q_INVOKABLE void noteEditedAccountForTesting(const QString &accountKey) + { + m_editedAccounts.insert(accountKey); + } + /// Item 178. Stands in for the digest round trip, which a bare window has /// no worker to make. Sets what onThreadDigestLoaded() would have set. void setConversationPathsForTesting(const QString &threadId, @@ -1485,6 +1499,17 @@ private: /// half. Tracked here rather than read back from SyncMonitor so the state /// the UI acted on is the state it was told about. bool m_externalSyncBusy = false; + + /// When the external sync now running began, from the lock appearing. + /// + /// Item 174. The status file the script leaves is preferred over the log's + /// banner, but only when it describes THIS run: a stale file outranking a + /// fresh log would clear the pending count on an old success for a run that + /// has just failed, which is the indicator lying in the direction that + /// loses work. Invalid when no external sync has been observed, where the + /// comparison is skipped rather than failing closed on a file that may well + /// be current. + QDateTime m_externalSyncStartedAt; QUndoStack m_undoStack; QLineEdit *m_queryEdit = nullptr; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e67d7e0..24da10b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -96,6 +96,13 @@ target_compile_definitions(test_translations PRIVATE target_compile_definitions(test_composewindow PRIVATE SOURCE_DIR="${CMAKE_SOURCE_DIR}") +# One test RUNS assets/mailsync.sh with stubbed binaries and reads the status +# file it wrote, which is the guard against the bash writer and the C++ reader +# drifting apart. Every other test in that file writes what it believes the +# script emits, and would go on passing after the script changed. +target_compile_definitions(test_mailsync PRIVATE + SOURCE_DIR="${CMAKE_SOURCE_DIR}") + # The notmuch hooks (assets/hooks/), which are Python rather than C++ and are # therefore registered directly rather than through add_qtmaildir_test(). # @@ -112,6 +119,19 @@ if(Python3_Interpreter_FOUND) COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/assets/hooks/test_${hook_test}.py) endforeach() + + # The sync script, which lives in assets/ rather than assets/hooks/ and so + # is registered on its own rather than through the loop above. + # + # It belongs in the suite for a stronger reason than the hooks do: the + # user's ~/bin/mailsync.sh is a SYMLINK to assets/mailsync.sh, so an edit + # here is live on their next cron tick with no deploy step in between. The + # test stubs mbsync and notmuch, points HOME at a temp directory and + # redirects the lock file, so it can neither reach the network nor take the + # real sync lock, which is the mutex their cron run uses. + add_test(NAME mailsync_script + COMMAND ${Python3_EXECUTABLE} + ${CMAKE_SOURCE_DIR}/assets/test_mailsync.py) else() message(STATUS "Python3 not found: the notmuch hook tests will not run") endif() diff --git a/tests/test_mailsync.cpp b/tests/test_mailsync.cpp index 45c7767..eb4990e 100644 --- a/tests/test_mailsync.cpp +++ b/tests/test_mailsync.cpp @@ -65,6 +65,16 @@ private slots: void lastRunOutcomeReadsATailOfAHugeLog(); void lastRunOutcomeReadsABannerTheScriptActuallyWrote(); + void readStatusReadsAnOkRun(); + void readStatusReadsTheChannelsARunCarried(); + void readStatusReadsAFullRunAsEveryAccount(); + void readStatusReadsASkippedRun(); + void readStatusOnAMissingFileIsUnknown(); + void readStatusOnRubbishIsUnknown(); + void readStatusOnATruncatedFileIsUnknown(); + void readStatusOnAnUnknownVersionIsUnknown(); + void readStatusReadsAFileTheScriptActuallyWrote(); + private: /// Writes an executable shell script into the temp dir, returns its path. QString makeScript(const QString &name, const QString &body); @@ -466,6 +476,186 @@ void TestMailSync::aChannelNameIsNotLetInVerbatim() // script writes into its log. These tests pin the parser against the exact // shape assets/mailsync.sh emits. +// Item 174. The status file is what the application READS, as against the log, +// which is for a human. These pin the reader against the exact shape +// assets/mailsync.sh writes; assets/test_mailsync.py pins the writer against +// the same shape from the other side, and the two agree by test rather than by +// shared code, exactly as the two rules.json readers do. + +static QString writeStatus(const QDir &dir, const QString &name, + const QByteArray &contents) +{ + const QString path = dir.filePath(name); + QFile file(path); + if (!file.open(QIODevice::WriteOnly)) + return QString(); + file.write(contents); + file.close(); + return path; +} + +void TestMailSync::readStatusReadsAnOkRun() +{ + const QString path = writeStatus( + QDir(m_dir.path()), QStringLiteral("ok.json"), + R"({"version": 1, "run_id": "2026-08-29T10:00:00+02:00", + "started": "2026-08-29T10:00:00+02:00", + "ended": "2026-08-29T10:00:12+02:00", + "state": "ok", "channels": ["-a"], + "mbsync_status": 0, "notmuch_status": 0})"); + QVERIFY(!path.isEmpty()); + + const SyncStatus status = MailSync::readStatus(path); + QCOMPARE(status.state, SyncState::Ok); + QVERIFY(status.ended.isValid()); +} + +void TestMailSync::readStatusReadsTheChannelsARunCarried() +{ + // The whole reason this file exists rather than the log's banner: the + // application clears its pending count for the accounts a run carried, and + // the log could never say which those were. + const QString path = writeStatus( + QDir(m_dir.path()), QStringLiteral("channels.json"), + R"({"version": 1, "run_id": "r", "started": "2026-08-29T10:00:00+02:00", + "ended": "2026-08-29T10:00:12+02:00", "state": "ok", + "channels": ["work", "personal"], + "mbsync_status": 0, "notmuch_status": 0})"); + QVERIFY(!path.isEmpty()); + + const SyncStatus status = MailSync::readStatus(path); + QCOMPARE(status.state, SyncState::Ok); + QCOMPARE(status.channels, + (QStringList{ QStringLiteral("work"), QStringLiteral("personal") })); + QVERIFY(!status.everyChannel); +} + +void TestMailSync::readStatusReadsAFullRunAsEveryAccount() +{ + // "-a" is not a channel name and must not be matched against one: a full + // run carries every account, so a reader treating it as an unknown channel + // would clear nothing on exactly the run that carried everything. + const QString path = writeStatus( + QDir(m_dir.path()), QStringLiteral("full.json"), + R"({"version": 1, "run_id": "r", "started": "2026-08-29T10:00:00+02:00", + "ended": "2026-08-29T10:00:12+02:00", "state": "ok", + "channels": ["-a"], "mbsync_status": 0, "notmuch_status": 0})"); + QVERIFY(!path.isEmpty()); + + const SyncStatus status = MailSync::readStatus(path); + QVERIFY2(status.everyChannel, "a -a run was not read as every account"); +} + +void TestMailSync::readStatusReadsASkippedRun() +{ + // Item 125. A skipped run releases a lock it never took, so the spinner had + // nothing to clear on. It is a terminal state, and distinct from a failure: + // the other run is doing the work. + const QString path = writeStatus( + QDir(m_dir.path()), QStringLiteral("skip.json"), + R"({"version": 1, "run_id": "r", "started": "2026-08-29T10:00:00+02:00", + "ended": "2026-08-29T10:00:00+02:00", "state": "skipped", + "channels": ["-a"], "mbsync_status": -1, "notmuch_status": -1})"); + QVERIFY(!path.isEmpty()); + + const SyncStatus status = MailSync::readStatus(path); + QCOMPARE(status.state, SyncState::Skipped); +} + +void TestMailSync::readStatusOnAMissingFileIsUnknown() +{ + QCOMPARE(MailSync::readStatus(m_dir.filePath(QStringLiteral("nope.json"))).state, + SyncState::Unknown); + QCOMPARE(MailSync::readStatus(QString()).state, SyncState::Unknown); +} + +void TestMailSync::readStatusOnRubbishIsUnknown() +{ + const QString path = writeStatus(QDir(m_dir.path()), + QStringLiteral("rubbish.json"), + "this is not json at all\n"); + QVERIFY(!path.isEmpty()); + QCOMPARE(MailSync::readStatus(path).state, SyncState::Unknown); +} + +void TestMailSync::readStatusOnATruncatedFileIsUnknown() +{ + // The script writes atomically through a temp file and mv precisely so this + // cannot happen, but a reader that trusts that is one filesystem away from + // being wrong. Unknown changes no state, so a torn read is harmless. + const QString path = writeStatus(QDir(m_dir.path()), + QStringLiteral("torn.json"), + R"({"version": 1, "state": "o)"); + QVERIFY(!path.isEmpty()); + QCOMPARE(MailSync::readStatus(path).state, SyncState::Unknown); +} + +void TestMailSync::readStatusOnAnUnknownVersionIsUnknown() +{ + // Refused rather than guessed at, the rule the rules file already follows: + // a future version may mean something different by the same field names, + // and acting on it would be worse than observing nothing. + const QString path = writeStatus( + QDir(m_dir.path()), QStringLiteral("future.json"), + R"({"version": 99, "run_id": "r", "started": "2026-08-29T10:00:00+02:00", + "ended": "2026-08-29T10:00:12+02:00", "state": "ok", + "channels": ["-a"], "mbsync_status": 0, "notmuch_status": 0})"); + QVERIFY(!path.isEmpty()); + QCOMPARE(MailSync::readStatus(path).state, SyncState::Unknown); +} + +void TestMailSync::readStatusReadsAFileTheScriptActuallyWrote() +{ + // The guard against the two sides drifting apart. Every test above writes + // what this file BELIEVES the script emits; this one runs the real script + // with stubbed binaries and reads what it actually wrote. + // + // Skipped rather than failed where bash or the script is unavailable: a + // packaging build has no reason to carry either, and a test that cannot run + // has observed nothing. + const QString script = QStringLiteral(SOURCE_DIR "/assets/mailsync.sh"); + if (!QFile::exists(script)) + QSKIP("assets/mailsync.sh not found"); + + QTemporaryDir home; + QVERIFY(home.isValid()); + + // Stubs, so nothing reaches the network and the real lock is never taken. + const QString bin = home.filePath(QStringLiteral("bin")); + QVERIFY(QDir().mkpath(bin)); + for (const QString &name : { QStringLiteral("mbsync"), + QStringLiteral("notmuch") }) { + QFile stub(bin + QLatin1Char('/') + name); + QVERIFY(stub.open(QIODevice::WriteOnly | QIODevice::Text)); + stub.write("#!/bin/bash\nexit 0\n"); + stub.close(); + QVERIFY(stub.setPermissions(QFile::ReadOwner | QFile::WriteOwner + | QFile::ExeOwner)); + } + + QProcessEnvironment env = QProcessEnvironment::systemEnvironment(); + env.insert(QStringLiteral("HOME"), home.path()); + env.insert(QStringLiteral("PATH"), + bin + QLatin1Char(':') + env.value(QStringLiteral("PATH"))); + // Never /tmp/mbsync.lock: that is the mutex the user's cron sync uses, and + // a test that took it would block their mail. + env.insert(QStringLiteral("MAILSYNC_LOCKFILE"), + home.filePath(QStringLiteral("lock"))); + + QProcess proc; + proc.setProcessEnvironment(env); + proc.start(QStringLiteral("bash"), { script, QStringLiteral("work") }); + if (!proc.waitForStarted(5000)) + QSKIP("bash not available"); + QVERIFY(proc.waitForFinished(30000)); + + const SyncStatus status = MailSync::readStatus( + home.filePath(QStringLiteral(".local/state/qtmaildir/syncstatus.json"))); + QCOMPARE(status.state, SyncState::Ok); + QCOMPARE(status.channels, QStringList{ QStringLiteral("work") }); + QVERIFY(!status.everyChannel); +} + void TestMailSync::lastRunOutcomeReadsAnOkRun() { const QString path = m_dir.filePath(QStringLiteral("ok.log")); diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index bbdad19..90cdbc2 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -247,6 +247,7 @@ private slots: void init(); void cleanup(); void noTestCanSeeTheRealLockTable(); + void noSyncTestReadsTheRealSyncState(); void everyKnownActionIsRegistered(); void everyRegisteredActionIsKnown(); void configuredBindingReachesTheAction(); @@ -429,6 +430,8 @@ private slots: void aRejectedWriteKeepsEarlierUndoHistory(); void aSuccessfulCronSyncClearsThePendingCount(); + void anExternalSyncClearsOnlyTheAccountsItCarried(); + void aSkippedExternalSyncClearsNothing(); void aFailedCronSyncLeavesThePendingCount(); void anUnreadableSyncLogLeavesThePendingCount(); void anUnknownExternalStateClearsNothing(); @@ -624,6 +627,53 @@ void TestMainWindow::noTestCanSeeTheRealLockTable() QVERIFY(MainWindow::locksPath().startsWith(QDir::tempPath())); } +void TestMainWindow::noSyncTestReadsTheRealSyncState() +{ + // The same guard as noTestCanSeeTheRealLockTable(), for the two paths a + // Config falls back to when a test does not name them, and it exists + // because that fallback bit twice in one sitting (item 174). + // + // A test writing "[sync]\nlog=..." and nothing else leaves syncStatus() + // pointing at the developer's real ~/.local/state/qtmaildir/syncstatus.json. + // Two tests asserting that a FAILED run leaves the pending count alone + // therefore read the last real cron run, found "ok", and passed against a + // broken clear. Pinning only the status key has the mirror problem: the log + // then defaults to the real mailsync.log. + // + // Asserted on Config rather than on any one test, so a new sync test that + // forgets one key fails here with a message naming the reason rather than + // failing mysteriously whenever the developer's last sync happened to + // succeed. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("[sync]\ncommand=/bin/true\n"); + file.close(); + + Config config; + config.load(path); + + // Both DO default to the real paths, which is correct for the application + // and is exactly the trap for a test. This documents the behaviour so the + // requirement below is obviously about the tests rather than the defaults. + QCOMPARE(config.syncLog(), MailSync::defaultLogPath()); + QCOMPARE(config.syncStatus(), MailSync::defaultStatusPath()); + + QVERIFY2(MailSync::defaultStatusPath().contains( + QStringLiteral(".local/state/qtmaildir/syncstatus.json")), + "the default status path moved: assets/mailsync.sh writes the old " + "one, and the two must agree or every external sync reads as " + "Unknown"); + + // Any test asserting on what a sync did must name BOTH keys in its own + // config, pointing them inside its own QTemporaryDir. There is no fixture + // that can enforce it, since Config is loaded per test, so this is the + // reminder that fails loudly if the defaults ever stop being real paths. +} + void TestMainWindow::everyKnownActionIsRegistered() { // KeyMap::knownActions() is what loadOverrides() validates config bindings @@ -7015,7 +7065,17 @@ void loadConfigWithSyncLog(Config &config, const QTemporaryDir &dir, 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()); + // The status file is pointed at this test's own directory even though these + // tests are about the LOG, and the omission cost two false greens: without + // it Config falls back to the real ~/.local/state/qtmaildir/syncstatus.json, + // so a test asserting that a FAILED log leaves the count alone read the + // developer's own last cron run, found "ok" and cleared. Same rule as the + // lock table: no test may observe the machine's real sync state. Pointing + // it at a file that does not exist makes readStatus() return Unknown, which + // is exactly the fallback-to-log case these tests mean to exercise. + file.write(QStringLiteral("[sync]\nlog=%1\nstatus=%2\n") + .arg(logPath, dir.filePath(QStringLiteral("no-status.json"))) + .toUtf8()); file.close(); config.load(path); @@ -7056,6 +7116,130 @@ void runExternalSync(MainWindow &window, SyncMonitor::State ending) } // namespace +/// Item 174. A run this process did not start now reports what it DID, in the +/// status file assets/mailsync.sh writes, instead of being inferred from the +/// log's RUN END banner. +/// +/// The property that banner could never express: WHICH channels the run +/// carried. The local sync path has always narrowed its clear to the accounts +/// it carried (onSyncFinished's snapshot-and-subtract); the external path had +/// no way to and cleared everything, so an edit to an account the run did not +/// touch was reported as shipped when it had not been. +void TestMainWindow::anExternalSyncClearsOnlyTheAccountsItCarried() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + + const QString statusPath = dir.filePath(QStringLiteral("syncstatus.json")); + QFile status(statusPath); + QVERIFY(status.open(QIODevice::WriteOnly)); + // Timestamped NOW rather than with a fixed date: the status file is only + // read as this run's result when it is at least as new as the sync that + // just ended, so a fixture dated in the past is correctly ignored as stale + // and the test would exercise the log fallback instead. + const QString now = + QDateTime::currentDateTime().toString(Qt::ISODate); + // A run that carried ONE of the two accounts. + status.write(QStringLiteral(R"({"version": 1, "run_id": "r", + "started": "%1", "ended": "%1", + "state": "ok", "channels": ["work"], + "mbsync_status": 0, "notmuch_status": 0})") + .arg(now).toUtf8()); + status.close(); + + const QString confPath = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile conf(confPath); + QVERIFY(conf.open(QIODevice::WriteOnly | QIODevice::Text)); + // BOTH keys, always. Pinning only one leaves the other defaulting to the + // developer's real ~/.local/state file, and a test then reads their last + // cron run instead of its own fixture: that is how two tests in this group + // went green against a broken clear before this was noticed. + conf.write(QStringLiteral("[sync]\nstatus=%1\nlog=%2\n" + "[account.work]\nmaildir=work\ntrash=trash\n" + "[account.personal]\nmaildir=personal\ntrash=trash\n") + .arg(statusPath, + dir.filePath(QStringLiteral("no-log.log"))) + .toUtf8()); + conf.close(); + + Config config; + config.load(confPath); + QCOMPARE(config.syncStatus(), statusPath); + + MainWindow window(config); + auto *label = window.findChild<QLabel *>(QStringLiteral("pendingEdits")); + QVERIFY(label); + + // An edit on each account. Only the first is carried by the run above. + QVERIFY(QMetaObject::invokeMethod(&window, "noteEditedAccountForTesting", + Q_ARG(QString, QStringLiteral("work")))); + QVERIFY(QMetaObject::invokeMethod(&window, "noteEditedAccountForTesting", + Q_ARG(QString, + QStringLiteral("personal")))); + recordOneEdit(window, QStringLiteral("m1"), QStringLiteral("flagged")); + QVERIFY2(!label->isHidden(), "the edit was not counted at all"); + + runExternalSync(window, SyncMonitor::State::Idle); + + // The account the run carried is gone; the one it did not is still waiting. + // Asserting only that something cleared would pass against the old blanket + // clear, which is the behaviour this replaces. + QVERIFY2(!window.editedAccountsForTesting().contains( + QStringLiteral("work")), + "the account the sync carried is still marked as edited"); + QVERIFY2(window.editedAccountsForTesting().contains( + QStringLiteral("personal")), + "an account the sync never carried was cleared anyway, which is " + "the blanket clear this replaces"); +} + +/// Item 125, the half this closes. A run that SKIPPED because another held the +/// lock did the work of neither: it must clear no edits, and before the status +/// file there was nothing to tell the application it had happened at all. +void TestMainWindow::aSkippedExternalSyncClearsNothing() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + + const QString statusPath = dir.filePath(QStringLiteral("syncstatus.json")); + QFile status(statusPath); + QVERIFY(status.open(QIODevice::WriteOnly)); + // NOW, for the staleness reason the other test records. + const QString now = + QDateTime::currentDateTime().toString(Qt::ISODate); + status.write(QStringLiteral(R"({"version": 1, "run_id": "r", + "started": "%1", "ended": "%1", + "state": "skipped", "channels": ["-a"], + "mbsync_status": -1, "notmuch_status": -1})") + .arg(now).toUtf8()); + status.close(); + + const QString confPath = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile conf(confPath); + QVERIFY(conf.open(QIODevice::WriteOnly | QIODevice::Text)); + conf.write(QStringLiteral("[sync]\nstatus=%1\nlog=%2\n") + .arg(statusPath, + dir.filePath(QStringLiteral("no-log.log"))) + .toUtf8()); + conf.close(); + + Config config; + config.load(confPath); + MainWindow window(config); + + auto *label = window.findChild<QLabel *>(QStringLiteral("pendingEdits")); + QVERIFY(label); + + recordOneEdit(window, QStringLiteral("m1"), QStringLiteral("flagged")); + QVERIFY(!label->isHidden()); + + runExternalSync(window, SyncMonitor::State::Idle); + + QVERIFY2(!label->isHidden(), + "a SKIPPED run cleared the pending count: it synced nothing, so " + "the edits are still only local"); +} + void TestMainWindow::aSuccessfulCronSyncClearsThePendingCount() { // The reported defect: edits applied, cron syncs, indicator still says N. |