aboutsummaryrefslogtreecommitdiffstats
path: root/docs/superpowers/plans
diff options
context:
space:
mode:
Diffstat (limited to 'docs/superpowers/plans')
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md203
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