diff options
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. |