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 /docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | |
| 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 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 111 |
1 files changed, 111 insertions, 0 deletions
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. |
