diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-29 11:25:02 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-29 11:25:02 +0200 |
| commit | 3fd999907ae8344f76ee4e5be1ac278a26f452ca (patch) | |
| tree | 17c01d44daee0147ecaca6cccd393471e6379cf1 /assets | |
| parent | 8c78dd139a77e896c72b7fc8b799af3c0df34344 (diff) | |
| download | qtmaildir-3fd999907ae8344f76ee4e5be1ac278a26f452ca.tar.gz qtmaildir-3fd999907ae8344f76ee4e5be1ac278a26f452ca.zip | |
feat: have the sync script report what it did
Item 174, and half of item 125.
The premise was corrected before any code. The note asks for an external
`notmuch new` to clear the pending count; it must not. That count means
tag mutations not yet known to have reached the MAIL STORE, which is the
server: an 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. The
entry's own proposal to watch notmuch_database_get_revision() was
rejected for the same reason: a revision moves when mail ARRIVES too, and
in neither case does it say anything about the server.
What was actually wrong was the reporting channel. The application
inferred a finished run from an inode in /proc/locks and from grepping
the log for its RUN END banner, which made a human-readable line into
wire format and could not say WHICH channels a run carried. The local
sync path has always narrowed its clear to the accounts it carried; the
external path could not, and cleared everything, so an edit to an
account a run never touched was reported as delivered.
So the script reports instead of leaving evidence to be inferred. It
writes ~/.local/state/qtmaildir/syncstatus.json atomically at the end of
every run, including a skip, naming the channels, both exit statuses and
a state of ok, failed or skipped. MailSync::readStatus() reads it,
MainWindow prefers it over the log banner and narrows the clear through
Account::syncChannel(). A skipped run clears nothing, which is item 125's
first half: the application can now see that a run happened and carried
nothing. The log banner and lastRunOutcome() stay as the fallback for a
missing file, which is what a first run after upgrading looks like.
This is the user's own framing of the scope: the script was written for
another system and adapted, and is now qtmaildir's only consumer, so it
serves the application rather than the reverse. Two facts made it safe to
act on: their 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.
Two bugs found while wiring it in, both recorded in the closed item.
A test read the developer's real sync state, twice: a [sync] section
naming only `log` leaves syncStatus() defaulting to the real file, so two
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. noSyncTestReadsTheRealSyncState() is the guard, modelled
on noTestCanSeeTheRealLockTable().
And Qt::ISODate carries no milliseconds. 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, and a round trip of "now" comes back 329 ms behind,
measured. A fast sync's own file therefore parsed as stale and fell back
to the log, with nothing failing to say so. One second of slack matches
the precision the format carries.
Design: docs/superpowers/specs/2026-08-29-sync-status-file-design.md
Suite: 43 of 44, with undoMovesTheMessageBack failing as it does on
master (item 136).
Diffstat (limited to 'assets')
| -rwxr-xr-x | assets/mailsync.sh | 114 | ||||
| -rwxr-xr-x | assets/test_mailsync.py | 170 |
2 files changed, 271 insertions, 13 deletions
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()) |
