diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 203 |
1 files changed, 203 insertions, 0 deletions
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 99e3b64..0186bfa 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 @@ -84,6 +84,8 @@ taking that too literally. | 34 | No overview of the Maildir itself | information | M | open | | 35 | No refresh of the thread list after a sync | workflow | M | open | | 36 | `test_mainwindow` cannot reach the worker | testing | S | open, on demand | +| 37 | The worker stalls on a tag edit made during a background sync | correctness | S | **done** | +| 38 | `test_mainwindow` fails when a real sync holds the lock | testing | XS | open | | 39 | Thread list cannot be sorted by clicking a column header | workflow | S | open | | 40 | No live filter over the current view | workflow | M | open | | 41 | A message whose HTML body carries a `Content-Id` renders blank | correctness | S | open | @@ -91,6 +93,7 @@ taking that too literally. | 43 | No "Mark all read" for the current view | workflow | S | open | | 44 | No way to manage the filters applied at sync time | workflow | ? | open, unspecified | | 45 | Two Sync buttons on the main window | discoverability | XS | open | +| 46 | `uiStateSurvivesARestart` fails under the offscreen platform | testing | XS | open | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1753,6 +1756,167 @@ rather than modelled. `MainWindow` and must keep working. The fixture is per-test, not a suite-wide `initTestCase`, or every case pays for a `notmuch new`. +## 37. The worker stalls on a tag edit made during a background sync + +**Observed:** the user's note asks whether edits made while a background sync is +running are carried by that same job or need a manual sync afterwards. The +answer splits in two, and the second half is a defect rather than a question. + +**This entry was rewritten on 2026-08-04 after its original cause was +disproved by measurement.** It first claimed the read-write open *fails* during +a sync and the edit is discarded. It does not fail. That claim was written from +the plausible reading of the error path at `src/notmuchworker.cpp:298-305` +without ever provoking the condition, and a fix was built on it before anyone +checked. Recorded here rather than quietly corrected, because the same +false-cause-from-a-plausible-error-path mistake is cheap to repeat. + +**Cause, part one: reaching the disk is not the problem.** `applyTags` calls +`notmuch_message_tags_to_maildir_flags()` (`src/notmuchworker.cpp:327`) +immediately after thawing, so a `seen`/`flagged` change renames the file in the +Maildir at edit time. No manual sync is needed for the change to exist on disk. +Whether the *running* mbsync carries it is a matter of ordering: mbsync scans +each mailbox once per run, so an edit landing after that box was scanned goes +out on the next run. That is expected behaviour, not a bug, and the ten-minute +cron interval bounds the delay. This half of the note is a question, answered. + +**Cause, part two: the write blocks, it does not fail.** Measured 2026-08-04 +against Slackware's notmuch, with the lock held deliberately rather than by +racing cron: + +- `notmuch_database_open_with_config(NOTMUCH_DATABASE_MODE_READ_WRITE, …)`, + the exact call `applyTags` makes, **blocks until the lock is free and then + returns `NOTMUCH_STATUS_SUCCESS`**. A C probe against a lock held for 12s + returned after 9.158s with status 0; the same call with no lock held returns + in 0.001s. It was never observed to return an error or to time out. +- The `notmuch` CLI behaves identically (waits 3.6s and 13.2s against 5s and + 15s holds, always exit 0), so this is libnotmuch's behaviour and not a + wrapper's retry loop. +- Therefore the error branch at `src/notmuchworker.cpp:298-305` is **not + reachable through lock contention at all**. It fires only for a genuinely + broken open: bad permissions, a corrupt index, a missing database. + +**The real defect is a stall.** `applyTagsToThreads` is invoked on the worker +thread through a queued connection (`src/mainwindow.cpp:1788`), so a blocking +open freezes *the worker*, not the UI. The window keeps painting and the rows +show the optimistic update, but every later query, thread load and tag write +sits behind that open in the worker's event queue until the lock frees. Nothing +is lost and no error appears; the application simply stops responding to +selections for the duration. + +**How bad in practice.** Bounded by how long `notmuch new` holds the lock, which +on this user's already-indexed Maildir is a fraction of a second at roughly +T+32s into a ~35s run (measured from `~/.local/state/mailsync.log`: runs start +at :00 and reach "Processed N total files" 32-40s later). The stall is +therefore usually invisible, and becomes user-visible only when `notmuch new` +has real work: a first index, a large delivery, a `notmuch reindex`. That is +also why it cannot be reproduced by clicking during a normal sync, and why the +reproduction below holds the lock on purpose. + +**Reproducing it.** Racing cron does not work. Hold the lock deliberately: +`notmuch tag --batch` keeps the write lock for a whole session and releases it +when stdin closes, so feeding it a slow stream of no-op tag commands holds the +lock for a controllable time. Verified: a competing writer blocks for exactly +the remaining hold. + +**Approach.** Do not send a write the worker will block on. `SyncMonitor` +(item 27) already reports whether a sync holds the lock, so `MainWindow` can +hold the edit while `State::Running` and send it on the transition to `Idle`, +which is a signal that already exists and already fires. The rows keep showing +the change meanwhile, which is honest: it is what the user asked for and it is +going to be applied. + +**Explicitly rejected: retrying on error.** That was the first implementation +and it is dead code against this cause, since the error it keys off never +arrives from lock contention. Keying on the monitor's state is also strictly +better: it avoids the stall rather than recovering from it. + +**Constraints.** The optimistic-update-then-revert contract must survive: a +held edit is still unsynced and must keep counting toward the pending indicator, +or the quit prompt will let the user leave on work that never landed (the +failure item 28 and the 0.9.0 net-state fix were both about). Do not widen the +write window by holding the read-write handle open, per the read-only-by-default +rule in `CLAUDE.md`. A held edit must re-resolve its thread ids when it is +finally sent: `notmuch new` may have renamed files underneath it. And +`SyncMonitor::State::Unknown` must not gate writes, or a platform that cannot +read `/proc/locks` would never send an edit at all. + +**What the stall looks like, observed 2026-08-04.** Confirmed by hand with the +Xapian lock held deliberately: switching between threads left the message pane +showing the FIRST thread selected, and when the lock released the pane stepped +through the three or four threads selected in the meantime, in sequence. That +is the worker's queue draining, and it is the user-visible shape of this defect. + +**Reads are NOT blocked by the write lock.** Measured the same day, and it +bounds how bad this is. A read-only open and a 200-thread query take 0.001s and +0.015s whether or not another process holds the write lock, identical to +baseline. So `loadThread` never blocks on the lock itself. The stall is purely +head-of-line blocking on the single worker thread: one blocked `applyTags` holds +up every read queued behind it. Do not "fix" this by making reads lock-aware; +there is nothing there to fix. + +**Residual gap: the 2s polling window. Accepted for now (user, 2026-08-06), +and deliberately left open rather than closed.** `SyncMonitor` polls every two +seconds, so a sync that starts between polls is invisible to the window for up +to 2s, and a tag edit in that window is still sent straight into a blocking +open. The window is 2s wide, `notmuch new` holds the lock for well under a +second on an already-indexed Maildir, and reads are unaffected either way, so +the exposure is small and the shipped behaviour is the pre-existing one. + +**The option stays on the table, to revisit:** check the lock at send time. +`SyncMonitor::lockHeldIn()` is already a static, pure function over +`/proc/locks` content, so `sendThreadTagChange` can call it for the cost of one +small file read per tag action. That closes the window entirely. It was not +taken now because it would add untested code to a change that had just been +verified by hand, which is the wrong order. + +### Hand test (2026-08-06): passed + +Verified against a real blocking open, which the unit tests cannot reach: they +drive the deferral through the meta-object and never take a lock. Both locks +held for 100s by the throwaway scaffold, with a tag edit made during the hold. +All six expected behaviours confirmed by the user: the row kept the tag, the +status message did not expire, the unsynced indicator rose, the Sync button was +disabled, **the window stayed responsive**, and the held edit sent itself on +release without a click. + +The responsiveness check is the one that mattered. The stall observed on +2026-08-04 left the message pane frozen on the first thread selected and +replayed the queue on release; that no longer happens. + +**Related:** item 35 (refresh after sync) touches the same `Idle` transition, +and both want a non-destructive path that does not clear the undo stack. + +## 38. `test_mainwindow` fails when a real sync holds the lock + +**Observed 2026-08-04:** `theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock` +failed once during a full run and passed on every rerun. The cause is not +ordering or pollution between tests: the user's cron sync happened to be running +at that moment. + +**Cause (verified in code).** `MainWindow::buildUi()` constructs a real +`SyncMonitor` on `SyncMonitor::defaultLockPath()` and the live `/proc/locks` +(`src/mainwindow.cpp:467`) and starts it. Every `MainWindow` a test builds +therefore observes the machine's actual sync state. The test asserts +`button->isEnabled()` on a freshly built window, which is false whenever a real +sync holds `/tmp/mbsync.lock`. With cron firing every ten minutes and a run +lasting ~35s, roughly 6% of test runs land inside one. + +**Not caused by the item 37 work**, though that is when it was noticed. The +test and the monitor both predate it; confirmed by stashing the item 37 changes +and seeing the suite pass, then reproducing the failure with a sync live. + +**Approach.** The monitor is already injectable: its constructor takes a +`locksPath` precisely so tests can drive transitions without real locks +(`src/syncmonitor.h:59-64`), and `test_syncmonitor` uses that. `MainWindow` does +not expose it. Either let the window take a locks path (config or a setter used +only by tests), or have the test point `SyncMonitor` at a temporary file. The +existing tests that drive `onExternalSyncStateChanged` through the meta-object +are unaffected either way; it is only the construction-time state that leaks in. + +**Constraint:** do not simply stop starting the monitor in tests. The +construction-time state IS the behaviour under test for this case, and a window +that never polls would pass the assertion for the wrong reason. + ## 39. Thread list cannot be sorted by clicking a column header **Observed (user, 2026-08-05):** "left pane columns order by clicking on the @@ -2004,6 +2168,45 @@ last widget that predates it. Confirm with the user which of the two survives; the note says redundant, not which one is wanted. +## 46. `uiStateSurvivesARestart` fails under the offscreen platform + +**Observed 2026-08-06:** the full suite is green on the user's Wayland session +but `TestMainWindow::uiStateSurvivesARestart` fails under +`QT_QPA_PLATFORM=offscreen`, which is how the suite is run when a session must +not open windows on the user's screen. Only the width is wrong: + +``` +Actual (reopened.size()): QSize(798x620) +Expected (resized) : QSize(940x620) +``` + +**Cause (verified by probe, not assumed).** The offscreen platform reports an +800x800 screen. `QMainWindow::restoreGeometry()` clamps a restored window to the +available screen area, so the test's 940 width comes back as 798 while its 620 +height, which fits, restores untouched. That asymmetry is the tell: the state +file is written and read correctly, and the zoom factor in the same test +restores fine. Nothing is broken in item 1's persistence. + +**Not a state-file collision.** The test already scopes itself properly with +`QStandardPaths::setTestModeEnabled(true)` and removes the file at both ends +(`tests/test_mainwindow.cpp:229-255`), so it never touches +`~/.local/state/qtmaildir/uistate.conf`. An earlier reading of this failure +blamed the real state file being held by a running app; that was wrong, and the +test source disproves it. + +**Approach.** Pick a size the smallest plausible test screen can hold, well +under 800x800, and assert on that. The test is about persistence, not about +large windows, so the specific number carries no meaning and only needs to +differ from the default. + +**Constraint:** do not "fix" this by widening the assertion to a tolerance or by +skipping under offscreen. Both would hide a genuine restore failure later, and +the property under test (the size that went in comes back out) is exact. + +**Related:** the same class as item 38. Both are tests that silently depend on +the machine they run on, and both were found by running the suite in a context +its author had not tried rather than by reading it. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering |