diff options
Diffstat (limited to 'docs/superpowers/plans')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 100 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 153 |
2 files changed, 185 insertions, 68 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 e7ef7db..4a4b1ff 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 @@ -5040,6 +5040,106 @@ when the write is moved above the guard. The cold-cache cost measured above was not touched and should not be. Nothing about the timing changed. +## 124. The worker reads the index directory as the mail root + +**Observed (measured, 2026-08-20):** not reported from use. Found while +measuring item 121's cold-start cost, which established that the 1.1 GB notmuch +index sits on a 7200rpm platter (`/dev/sda1`, `rotational: 1`) while an NVMe SSD +sits idle in the same machine. Moving the index to the SSD is a notmuch +configuration change and needs no code, but qtmaildir does not survive it. + +**Cause (verified against notmuch 0.39 and a throwaway database).** notmuch +supports splitting the index from the mail with two keys: + +```ini +[database] +mail_root=/data/Mail +path=/home/you/.local/share/notmuch +``` + +Under that layout `notmuch_database_get_path()` returns the **index** +directory, not the mail root. Measured on a split test database: + +| accessor | legacy (`path` only) | split | +|---|---|---| +| `notmuch_database_get_path()` | mail root | **index dir** | +| `notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT)` | mail root | mail root | + +`notmuchworker.cpp` calls `get_path()` at six sites and treats every one as the +mail root: lines 297, 755, 900, 901, 1062, and the `relativeFilePath()` calls +at 364, 380 and 918 that consume them. + +**One of those six moves mail, and it is the dangerous one.** +`moveMessages` composes its destination as +`root + "/" + destFolder + "/cur"` (`src/notmuchworker.cpp:755`). Under a split +config `root` is the index directory, so Delete would move the message into +`<index>/Trash/cur`: outside the Maildir, invisible to mbsync, and gone from +every other client. That is item 103's stranded-mail failure with a new cause, +and this repo has already shipped that class of bug once. + +The rest degrade rather than destroy. `relativeFilePath()` against the wrong +root yields `../../../data/Mail/account/cur/...` instead of `account/cur/...`, +so no path matches an account prefix and every row resolves to no account, +which is exactly what the comment at line 294 already warns about. + +**Approach.** Replace `notmuch_database_get_path()` with +`notmuch_config_get(m_db, NOTMUCH_CONFIG_MAIL_ROOT)` at the six sites. + +**No conditional is needed, and that is the point.** `MAIL_ROOT` returns the +mail root under BOTH layouts, verified above: under a legacy `path`-only config +it equals `get_path()`, so the change is a no-op against the current +configuration and correct against the split one. A fallback to `get_path()` when +`MAIL_ROOT` is NULL is the tempting belt-and-braces addition and should be +resisted unless a NULL is actually observed, since it reintroduces the wrong +answer on the path where it matters. + +**Constraints.** + +- **`moveMessages` is the site to test hardest.** A wrong root there reaches the + mail server, per the "Delete MOVES the file" note in `CLAUDE.md`. +- The test fixture must build a database whose index is NOT inside the mail + root, or it cannot tell the two accessors apart: under the ordinary fixture + layout both return the same string and a mutation stays green. +- `notmuch_config_get` returns a string owned by notmuch (`notmuch.h:2585`); + do not free it. +- Nothing about the legacy layout may change. The migration is the user's to + perform, separately, once this ships. + +**Size: S.** Six call sites and a fixture that can tell them apart. + +**Closed 2026-08-20, unreleased.** `mailRootOf()` in the anonymous namespace of +`notmuchworker.cpp` wraps `notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT)`, and +the four `notmuch_database_get_path()` call sites use it. No conditional: the +accessor is correct under both layouts, verified against the live database where +it returned the same string as `get_path()` before the migration. + +Three tests in `test_notmuchworker.cpp`, all requiring the fixture's new opt-in +`splitIndex()`. That flag is load-bearing rather than convenient: in the +ordinary layout the index sits inside the mail root and both accessors return +the same string, so a test written against it passes whichever one the code +uses. All three fail against the old accessor, confirmed by mutation, with the +move test failing outright rather than subtly. + +**Proven in production the same day.** The developer's own index moved from a +7200rpm platter to NVMe, mail staying at `/data/Mail`: + +| phase | platter, cold | NVMe, cold | +|---|---|---| +| `search_threads` | 673 ms | 12 ms | +| first 200 rows | 2008 ms | 50 ms | +| complete walk, 4634 threads | 38618 ms | 668 ms | + +Counts held at 49174 messages / 5594 inbox / 100 tags at every step, and Delete +then Restore round-tripped through `[Gmail]/Cestino` by hand, which is the +behaviour this item existed to protect. + +**Two things the migration taught that are not in the code.** `database.hook_dir` +defaults to `<database.path>/.notmuch/hooks`, so a split config silently stops +running `post-new`: `notmuch new` reports success and tags nothing. It must be +set explicitly. And the procedure's own first step, holding `/tmp/mbsync.lock`, +conflicts with hand-testing Delete, which auto-syncs; that surfaced item 125. +The procedure lives outside this repo, in the user's own documents. + ## 90. A saved-query button clears the account selection **Observed (user, notes):** "select an account and hit the 'unread' button, the 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 4039753..8129704 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 @@ -191,7 +191,9 @@ taking that too literally. | 122 | The README documents a version of the app that no longer exists | documentation | M | open, 2026-08-20, from the notes. Delete-to-trash is entirely undocumented, including a config key a user must now set | | 123 | Sending mail is not designed | v2 | ? | open, 2026-08-20, from the notes. Brainstorm only, explicitly `#plan-only`; the user places most open UX behind it | -| 124 | The worker reads the index directory as the mail root | defect | S | open, 2026-08-20. Blocks moving the index to an SSD. Under a split `mail_root`/`path` config, Delete would move mail INSIDE the index directory, where mbsync cannot see it | +| 124 | The worker reads the index directory as the mail root | defect | S | **done** 2026-08-20, unreleased. `mailRootOf()` over `NOTMUCH_CONFIG_MAIL_ROOT`, correct under both layouts. Verified by migrating the developer's own index to NVMe the same day: cold start 38.6 s to 0.67 s | + +| 125 | A skipped sync leaves the spinner running for ever | defect | S | open, 2026-08-20, found by hand. `mailsync.sh` exits 75 (EX_TEMPFAIL) when another run holds the lock; the indicator never clears, and a held edit waits for a completion that never comes | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -849,11 +851,27 @@ cold boot on this hardware: So the list is blank for **two seconds**, and keeps growing for **thirty-eight**, on 4% more mail. The user's note said "several seconds" and the note was right. -**The cause is the storage, not the code.** `/data` is `/dev/sda1`, a 7200rpm -platter (`rotational: 1`); warm, the identical walk is 154 ms, a 250x -difference. Item 124 is the prerequisite for moving the index to the NVMe SSD -already in the machine, which would make this gap ~12 ms and reduce this item to -a nicety. +**The cause is the storage, not the code**, and that is the reason to BUILD +this rather than to skip it. `/data` was `/dev/sda1`, a 7200rpm platter +(`rotational: 1`); warm, the identical walk is 154 ms, a 250x difference. + +**The developer's own index moved to NVMe on 2026-08-20** (item 124 was its +prerequisite), which took the cold figures to 12 ms / 50 ms / 668 ms and makes +this invisible *on that machine*. That is precisely why the item stays open. A +mechanical disk is not an exotic configuration, it is the cheap one, and a user +who keeps a large Maildir on spinning rust has nowhere to migrate to. The +measurements above are now the best evidence this project has for what such a +user sees on every cold start, and they were taken on real mail rather than +simulated: + +| storage | first rows | complete walk | +|---|---|---| +| 7200rpm platter, cold | 2008 ms | 38618 ms | +| NVMe, cold | 50 ms | 668 ms | + +Fixing one developer's hardware is not fixing the application. The indicator is +what makes a slow query legible on any disk, and the slower the disk the more it +matters. **Approach.** A busy state on the left pane between `runQuery` and the first `onThreadsReady`, cleared by whichever of the first batch or `queryFinished` @@ -875,10 +893,17 @@ the usual reason a spinner does not spin. ignored that guard would make every cron sync flash the list. That silence is already a test, and it should cover this too. - **Item 74's decision not to address the cold cost was taken on wrong - numbers** and is worth revisiting, though not here. It judged a 5.7 s wait not - worth prefaulting 1.1 GB; the real figure is 38.6 s. The answer is not - prefaulting either way: it is item 124 plus moving the index off the platter. - This item makes the remaining wait legible, nothing more. + numbers**, and its conclusion still holds for a different reason. It judged a + 5.7 s wait not worth prefaulting 1.1 GB; the real figure was 38.6 s. Do not + reopen prefaulting: it trades a large fixed cost at every startup against a + wait that only some users pay, and it is worse on exactly the low-memory + machines most likely to have a slow disk. + +- **Do not treat "move the index to an SSD" as this item's fix.** It is the + right advice for a user who has an SSD, and it is documented, but it is + hardware guidance rather than a change to the application. This item must + stand on its own for a user with one mechanical disk and no migration + available. - Nothing about the query timing may change. **Size: S.** @@ -969,72 +994,64 @@ queued message reaches the sending script. **Size: `?`** until the brainstorm has happened. It is the largest open item by some distance. -## 124. The worker reads the index directory as the mail root +## 125. A skipped sync leaves the spinner running for ever -**Observed (measured, 2026-08-20):** not reported from use. Found while -measuring item 121's cold-start cost, which established that the 1.1 GB notmuch -index sits on a 7200rpm platter (`/dev/sda1`, `rotational: 1`) while an NVMe SSD -sits idle in the same machine. Moving the index to the SSD is a notmuch -configuration change and needs no code, but qtmaildir does not survive it. +**Observed (user, 2026-08-20):** during the item 124 index migration, a Delete +appeared to do nothing: the view did not refresh, the message did not move, and +restarting the application showed it exactly where it had been. The user then +reported "I see a spinner in the bottom right, is going indefinitely. maybe +that's what stopped the move?" That observation is what identified the cause. -**Cause (verified against notmuch 0.39 and a throwaway database).** notmuch -supports splitting the index from the mail with two keys: +**Cause (verified from `/proc` and the sync log).** The migration procedure held +`/tmp/mbsync.lock` to keep cron out of the way. qtmaildir auto-syncs a couple of +seconds after a tag action (item 71), so Delete queued its edit and started a +sync; `mailsync.sh` found the lock held and exited **75**, by design: -```ini -[database] -mail_root=/data/Mail -path=/home/you/.local/share/notmuch +``` +2026-08-20T10:00:01+02:00 === SKIPPED: previous run still in progress === +2026-08-20T10:10:01+02:00 === SKIPPED: previous run still in progress === ``` -Under that layout `notmuch_database_get_path()` returns the **index** -directory, not the mail root. Measured on a split test database: - -| accessor | legacy (`path` only) | split | -|---|---|---| -| `notmuch_database_get_path()` | mail root | **index dir** | -| `notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT)` | mail root | mail root | - -`notmuchworker.cpp` calls `get_path()` at six sites and treats every one as the -mail root: lines 297, 755, 900, 901, 1062, and the `relativeFilePath()` calls -at 364, 380 and 918 that consume them. - -**One of those six moves mail, and it is the dangerous one.** -`moveMessages` composes its destination as -`root + "/" + destFolder + "/cur"` (`src/notmuchworker.cpp:755`). Under a split -config `root` is the index directory, so Delete would move the message into -`<index>/Trash/cur`: outside the Maildir, invisible to mbsync, and gone from -every other client. That is item 103's stranded-mail failure with a new cause, -and this repo has already shipped that class of bug once. - -The rest degrade rather than destroy. `relativeFilePath()` against the wrong -root yields `../../../data/Mail/account/cur/...` instead of `account/cur/...`, -so no path matches an account prefix and every row resolves to no account, -which is exactly what the comment at line 294 already warns about. - -**Approach.** Replace `notmuch_database_get_path()` with -`notmuch_config_get(m_db, NOTMUCH_CONFIG_MAIL_ROOT)` at the six sites. - -**No conditional is needed, and that is the point.** `MAIL_ROOT` returns the -mail root under BOTH layouts, verified above: under a legacy `path`-only config -it equals `get_path()`, so the change is a no-op against the current -configuration and correct against the split one. A fallback to `get_path()` when -`MAIL_ROOT` is NULL is the tempting belt-and-braces addition and should be -resisted unless a NULL is actually observed, since it reintroduces the wrong -answer on the path where it matters. +75 is `EX_TEMPFAIL`, chosen deliberately so a click landing during a cron run is +not reported as a failure (`assets/mailsync.sh`, the comment at the `flock -n` +guard). The application appears to treat it as neither success nor failure: the +sync indicator never cleared, and because an edit made during a sync is HELD +until the sync ends, the delete sat in that queue waiting for a completion that +could never arrive. `notmuch count tag:deleted` on the account confirmed the +write had not reached the database. + +**Nothing was lost**, and that is worth recording separately: held edits are +written to disk (item 106), so the delete survived and applied as soon as a real +sync ran. The defect is that the user cannot tell. + +**Approach.** Handle exit 75 explicitly wherever `MailSync` reports a finish. +It is a third outcome, not a variant of the other two: the work did not happen, +nothing is wrong, and it should be retried rather than reported. Clearing the +indicator is the minimum; re-arming the auto-sync timer is probably right too, +and item 89 already made a skipped auto-sync re-arm rather than give up, so +there is a precedent to follow rather than a policy to invent. **Constraints.** -- **`moveMessages` is the site to test hardest.** A wrong root there reaches the - mail server, per the "Delete MOVES the file" note in `CLAUDE.md`. -- The test fixture must build a database whose index is NOT inside the mail - root, or it cannot tell the two accessors apart: under the ordinary fixture - layout both return the same string and a mutation stays green. -- `notmuch_config_get` returns a string owned by notmuch (`notmuch.h:2585`); - do not free it. -- Nothing about the legacy layout may change. The migration is the user's to - perform, separately, once this ships. - -**Size: S.** Six call sites and a fixture that can tell them apart. +- **Do not turn 75 into an error.** The exit code exists precisely so an + overlapping click is not reported as a failed sync, and item 89 settled that a + skip is routine. Showing the log pane here would be a regression. +- **The held-edit queue must still flush.** Whatever clears the indicator has to + leave the queue in a state where the next successful sync sends it, which is + what happened by luck here rather than by design. +- **A stuck indicator is worse than a wrong one**, because it also blocks the + quit prompt's "unsynced changes" story (items 18, 19, 28, 54). This is the + same class of indicator dishonesty those four items each fixed once. + +**Reproducing it.** Hold the lock in one terminal and act in the application: + +```bash +flock /tmp/mbsync.lock -c 'sleep 300' +``` + +Then Delete a message. Verified by hand on 2026-08-20; this is how it was found. + +**Size: S.** ## Deferred, unsized, or split out |