From cd56144ba0304247812cf61c8b6e435779df29d0 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 25 Aug 2026 11:35:11 +0200 Subject: docs(backlog): close item 162, record a second site for 163 Item 162 is done and its section moves to the closed file on this commit, with the outcome recorded on it: moveMessages() re-resolves by message id when the recorded path is gone, and the hold candidate was investigated and rejected because aSyncHoldsTheWriteLock() guards notmuch's lock while the window sits between mbsync's rename and that sync's notmuch new. Item 163 gains a second site, found while hand-testing 164 and worse than the one it was filed for. openComposerFor() passes ref.filePath to forDraft(); after mbsync renames the file the parse fails and the reopen is refused BEFORE any composer exists, so composing again starts fresh with no previous path to unlink. The old revision survives, each save mints a new Message-ID, and both revisions reach the server. The unlink machinery is entirely correct and never runs. Its heading and row now name both sites, and the note that 162's fix would cover it is removed: that fix re-resolves inside the worker, while these hold a stale path in the UI. Item 164 gains the reproducer's findings. Seven variants in throwaway databases establish that index_file applies no tags, that the carve-out strips inbox correctly in every filename shape and ordering tried, and that the only reproduction is a pass applying inbox while tag:new is already spent. The trigger is still not established, and the entry says so: the live log shows the hook ran on the affected pass and logged success, which the new match-count instrumentation will disambiguate on the next occurrence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UUQS6n3cmsFrsjCNmwNtf8 --- .../2026-08-03-post-0.1.0-usability-closed.md | 79 ++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 159 ++++++++++++--------- 2 files changed, 174 insertions(+), 64 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 2e941ec..f537385 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 @@ -7751,3 +7751,82 @@ new action and the existing ones gathered under it rather than duplicated. - **Item 160 unblocked this** on 2026-08-25: the status bar exists, so a manual save reports through `refreshDraftStatus()` like an autosave. Route `Ctrl+S` through `saveDraftNow()` and the reporting is already done. + +## 162. Delete fails while a sync is renaming the file underneath it + +**Observed (user, 2026-08-25):** deleting a draft reported `Cannot move + to /Trash`. + +**Cause (verified against the live Maildir, not read):** a stale path, and +neither Delete nor item 158 is at fault. + +1. The composer autosaves a draft as `:2,D` and item 158 indexes it + under exactly that filename. +2. **mbsync uploads it and RENAMES it** to `,U=:2,D`, recording the + server UID in the filename. +3. notmuch still holds the pre-`U=` name until that sync's `notmuch new` runs. +4. `moveMessages()` reads the filename from notmuch and calls + `QFile::rename()` on a path that no longer exists. It fails, the error is + emitted, and the message is skipped. + +Measured, in this order: `notmuch search --output=files` named a file that was +not on disk while `Background sync running...` was up, and the same query was +clean once the sync finished, with the file present under its new `,U=4` name. +That is why it reads as intermittent, and why it heals itself. + +**It is truthful and it loses nothing.** The move is skipped, no wrong folder +is created, no file is destroyed, and the next sync reconciles. The defect is +that the message blames a folder for a timing problem, and that the action +silently does nothing when the user asked for something. + +**This is `CLAUDE.md`'s `,U=` trap from the other side.** `MaildirName::fresh()` +exists because CARRYING that infix across a folder boundary produced +`Maildir error: duplicate UID` on real mail. Here mbsync is ADDING it and the +index lags; the same infix, the opposite direction. + +**Approach, and it needs a decision.** Two candidates: + +- **Refuse the move while a sync holds the lock.** `SyncMonitor` already + reports this, and the held-edit machinery from items 97 and 106 already + exists for exactly this shape: a tag edit made during a sync is held and + flushed when it ends. Delete would join it rather than inventing anything. + This is the likelier right answer, since it matches what every other + mutation already does. +- **Re-resolve the filename** from notmuch immediately before the rename and + re-query the message if the path is gone. Smaller, but it races the same + window it is trying to close, and a second lookup can be stale by the time + it is used. + +**Constraints.** + +- **Delete reaches the real mail server.** Read `CLAUDE.md`'s item 103 notes + before touching `moveMessages()`: a wrong folder name is created, adopted by + mbsync, and propagated to every other client. +- Whatever is built, **the message must say a sync is running**, not name a + folder. The current wording sent the user looking for a broken folder + configuration, which was correct and configured. +- A test cannot see this in the ordinary fixture layout, where nothing renames + a file underneath the index. Driving it means renaming the file between the + index write and the move, which is what the reproducer has to do. + +**Fixed 2026-08-25.** `moveMessages()` re-resolves by MESSAGE ID when the +recorded path is gone: one `reindexFolder()` of that directory, then the +filename from `notmuch_message_get_filenames()` that exists on disk. Bounded +to a single retry, so a genuinely missing file still reports rather than +becoming a silent no-op. + +**The hold candidate above was investigated and rejected**, and the reason +matters more than the fix. `sendMove()` ALREADY refuses while a sync holds the +lock and queues onto `m_heldMoves` (items 97 and 106 built it), yet the defect +still fired. `aSyncHoldsTheWriteLock()` tracks notmuch's write lock, while this +window sits between mbsync's RENAME and that sync's `notmuch new`; mbsync +renames throughout its run without touching that lock, so the damaging window +is open when there is nothing to observe. Refusing on the lock guards the wrong +resource. That refusal is correct for its own purpose and was left alone. + +Covered by `moveMessagesRecoversWhenASyncRenamedTheFile()` and +`moveMessagesStillReportsAMessageThatIsReallyGone()`, the second pinning the +bounded half. The test renames without reindexing, which is the window mbsync +opens, and asserts the database still names the old path so it cannot pass +against a fixture that quietly reindexed. Mutation-checked. + 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 b390bab..f60ee7f 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 @@ -235,8 +235,8 @@ taking that too literally. | 160 | The composer never says a draft was autosaved | feedback | S | **done** 2026-08-25, unreleased. A status bar on the composer: the age line left, the `○ unsaved content` cue beside it. **The fix is a funnel, not a label.** `m_dirty` had SEVEN writers and four of them clear it, only two of which are a save, so a cue hung off the save path silently missed the constructor and the send; `setDirty()` is the one writer now and refreshes both cues plus `setWindowModified()`. Presentation was **reworked after the user looked at it**: it first reused item 151's yellow ribbon treatment, which reads as a misplaced widget on a bare status label, and the cue sat in the permanent (right-hand) tray. Two defects found by probing rather than by reading, see the section | | 161 | The composer has no menu bar | discoverability | S | **done** 2026-08-25, unreleased. File / Edit / Format, to the user's own chosen scope. **Save draft (`Ctrl+S`) is the only NEW action**; everything else is gathered, and the menus show the toolbar's own `QAction` objects rather than copies, per item 140's rule. Two needed hand-building: the HTML toggle is a `QToolButton` and cannot go in a menu, so a checkable twin mirrors it BOTH ways; and the signature entry takes the switch's own `QMenu` pointer, since that menu is rebuilt when the signatures change and copied entries would go stale. Edit's entries follow the editor's own `undoAvailable`/`copyAvailable`. `theMenuBarReachesEveryComposerAction()` is item 132's rule applied to the composer, walking the real menu bar and finding actions by `findChildren`, so a future action added to the toolbar and forgotten in the menus fails without touching the test | -| 162 | Delete fails while a sync is renaming the file underneath it | defect | S | open, 2026-08-25, found by hand. `Cannot move to `. NOT a Delete defect and not item 158's: mbsync renames an uploaded file to add its `,U=` infix, and notmuch keeps the pre-`U=` name until that sync's `notmuch new` runs, so `moveMessages` renames a path that no longer exists. Truthful, harmless and SELF-HEALING, which is why it reads as intermittent: verified a ghost present mid-sync and gone after. The message names a folder as though the folder were the problem. Delete reaches the real mail server, so read `CLAUDE.md` before touching it | -| 163 | The message pane shows a stale path and reports the message unreadable | defect | S | open, 2026-08-25, found by hand. Same root as 162 and a DIFFERENT site: the model keeps the filename a row was loaded with, mbsync renames the file to add `,U=`, and `MimeParser` then opens a path that no longer exists and honestly reports "could not be parsed". notmuch is CORRECT by then; the UI is behind, so 162's fix (refuse the write while a sync runs) does not touch this. The read path needs to recover rather than refuse | +| 162 | Delete fails while a sync is renaming the file underneath it | defect | S | **done, 2026-08-25.** mbsync renames an uploaded file to add its `,U=` infix and notmuch keeps the pre-`U=` name until that sync's `notmuch new` runs, so `moveMessages` renamed a path that no longer existed and Delete silently did nothing while blaming the destination folder. `moveMessages` now re-resolves by MESSAGE ID when the recorded path is gone: one reindex of that directory, then the filename that exists on disk. Bounded to one retry, so a file genuinely gone still reports. Holding the move during a sync was the other candidate and is NOT the fix: `sendMove` already refuses on notmuch's write lock, but this window sits between mbsync's rename and that sync's `notmuch new`, which touches no lock | +| 163 | The message pane shows a stale path, and the composer forks the draft | defect | S | open, 2026-08-25, found by hand. The model keeps the filename a row was loaded with, mbsync renames the file to add `,U=`, and `MimeParser` then opens a path that no longer exists. notmuch is CORRECT by then; the MODEL is behind. **A SECOND site found 2026-08-25 and it is the worse half:** `openComposerFor` passes `ref.filePath` to `forDraft`, the parse fails, and the reopen is REFUSED before any composer exists, so the user composes a fresh draft whose autosave has no previous path to unlink. The old revision survives, each save mints a new Message-ID, and both reach the server. Item 162's fix does not touch either site: that one re-resolves inside the worker, these hold a stale path in the UI | | 164 | A draft this application saved keeps `inbox` | defect | S | open, 2026-08-25, **cause corrected 2026-08-25**. The first diagnosis blamed a missing drafts helper and was WRONG: `NOT_ARRIVALS` in `qtmaildirconf.py` is `("sent", "drafts")`, the folder list includes every account's drafts folder, and `notmuch count` confirms the carve-out query MATCHES the affected draft. The carve-out is scoped to `tag:new`, and the draft carries `inbox` while `tag:new` is 0, so it was never in scope when the hook ran. Measured separately: an mbsync-style rename does NOT re-add `new.tags`, so the retag theory is out too. What remains unestablished is WHICH pass tagged it; establish that before writing code | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1341,64 +1341,7 @@ The 70-second duration recorded above fits a `QTRY_*` waiting for a file that is never going to appear, which is consistent with a wrong destination rather than a slow one. -## 162. Delete fails while a sync is renaming the file underneath it - -**Observed (user, 2026-08-25):** deleting a draft reported `Cannot move - to /Trash`. - -**Cause (verified against the live Maildir, not read):** a stale path, and -neither Delete nor item 158 is at fault. - -1. The composer autosaves a draft as `:2,D` and item 158 indexes it - under exactly that filename. -2. **mbsync uploads it and RENAMES it** to `,U=:2,D`, recording the - server UID in the filename. -3. notmuch still holds the pre-`U=` name until that sync's `notmuch new` runs. -4. `moveMessages()` reads the filename from notmuch and calls - `QFile::rename()` on a path that no longer exists. It fails, the error is - emitted, and the message is skipped. - -Measured, in this order: `notmuch search --output=files` named a file that was -not on disk while `Background sync running...` was up, and the same query was -clean once the sync finished, with the file present under its new `,U=4` name. -That is why it reads as intermittent, and why it heals itself. - -**It is truthful and it loses nothing.** The move is skipped, no wrong folder -is created, no file is destroyed, and the next sync reconciles. The defect is -that the message blames a folder for a timing problem, and that the action -silently does nothing when the user asked for something. - -**This is `CLAUDE.md`'s `,U=` trap from the other side.** `MaildirName::fresh()` -exists because CARRYING that infix across a folder boundary produced -`Maildir error: duplicate UID` on real mail. Here mbsync is ADDING it and the -index lags; the same infix, the opposite direction. - -**Approach, and it needs a decision.** Two candidates: - -- **Refuse the move while a sync holds the lock.** `SyncMonitor` already - reports this, and the held-edit machinery from items 97 and 106 already - exists for exactly this shape: a tag edit made during a sync is held and - flushed when it ends. Delete would join it rather than inventing anything. - This is the likelier right answer, since it matches what every other - mutation already does. -- **Re-resolve the filename** from notmuch immediately before the rename and - re-query the message if the path is gone. Smaller, but it races the same - window it is trying to close, and a second lookup can be stale by the time - it is used. - -**Constraints.** - -- **Delete reaches the real mail server.** Read `CLAUDE.md`'s item 103 notes - before touching `moveMessages()`: a wrong folder name is created, adopted by - mbsync, and propagated to every other client. -- Whatever is built, **the message must say a sync is running**, not name a - folder. The current wording sent the user looking for a broken folder - configuration, which was correct and configured. -- A test cannot see this in the ordinary fixture layout, where nothing renames - a file underneath the index. Driving it means renaming the file between the - index write and the move, which is what the reproducer has to do. - -## 163. The message pane shows a stale path and reports the message unreadable +## 163. The message pane shows a stale path, and the composer forks the draft **Observed (user, 2026-08-25):** selecting a draft filled the pane with `(unreadable message)` and `This message could not be parsed.`, naming a file @@ -1425,6 +1368,38 @@ parse, re-resolve the message id through notmuch and retry once before reporting. `recoverStaleThread()` already exists for the neighbouring problem (item 91 reuses it) and is the shape to follow. +**A SECOND site, found 2026-08-25 while hand-testing item 164, and its +consequence is worse than the pane's.** `MainWindow::openComposerFor()` passes +`ref.filePath` to `ComposeContextBuilder::forDraft()`, and `MessageRef::filePath` +is `notmuch_message_get_filename()` captured when the QUERY ran. After mbsync +renames an uploaded draft, a row loaded before that sync names a file that no +longer exists, `MimeParser::parse` fails, `forDraft` returns an empty context +and the composer reports "That draft could not be read". + +Measured, in order: draft written 10:42, sync at 10:50:43 logged "Detected 2 +file renames" and the file became `,U=5`, the user reopened it at 10:52 and +saw the error. + +**The pane's version is cosmetic and self-repairing. This one silently forks +the draft.** The refusal happens BEFORE `openComposer()`, so the user is left +with no composer for that draft. Composing again opens a FRESH one with +`m_draftPath` empty, so its first autosave has no previous path to unlink: +it writes a new file, `DraftStore` unlinks nothing, and the old revision +stays. Both are then uploaded and both reach the server. + +Two properties make this worse than it first reads. Each save mints a NEW +Message-ID (four saves in the hand test produced four ids), so the revisions +are distinct MESSAGES to notmuch rather than two files of one, and no dedup +anywhere will collapse them. And the unlink machinery is entirely correct +throughout, which is why the code reads fine: `forDraft` sets `draftPath`, +the composer seeds `m_draftPath`, the autosave passes it as `previousPath`, +and `DraftStore::write` unlinks it. The comment at `composecontext.cpp:501` +already names this exact failure ("one message becomes two"). None of it runs, +because the reopen was refused before any of it was reached. + +So the fix below covers this site too, and fixing only the pane would leave +the draft-forking half in place. + **Constraints.** - **A retry must be bounded.** A message that genuinely cannot be parsed @@ -1433,6 +1408,13 @@ reporting. `recoverStaleThread()` already exists for the neighbouring problem - Re-resolving by id is what makes this safe; re-scanning the folder is not, since two files can carry the same id. - The placeholder wording is correct and should stay for the genuine case. +- **The composer site must recover, not merely report better.** A clearer + error still leaves the user composing a second copy of their own draft. +- Whether a draft should keep a STABLE Message-ID across revisions is a + separate question this entry does not decide. It is what turns a stale path + into two server-side messages rather than one replaced file, and it wants + its own item; note that a draft's id is not yet the sent message's id, so + changing it is not obviously free. ## 164. A draft this application saved keeps `inbox` @@ -1469,10 +1451,59 @@ indexes a draft from the application itself, outside `notmuch new`, and a file already known to the database is not a new file on the next pass. But that is a hypothesis and the last two hypotheses here were both wrong. -**Approach.** Reproduce first, in a throwaway database, driving the real -sequence: index the file the way `indexDraftFile()` does, then rename it the -way mbsync does, then `notmuch new`, then the hook. Only then decide whether -the fix belongs in the hook's scope, in item 158's indexing, or in neither. +**The reproducer was built (2026-08-25) and it settles the mechanism.** Seven +variants were driven in throwaway databases, modelling `indexDraftFile()` with +a real `notmuch_database_index_file` call rather than the CLI, because no CLI +command indexes an untracked path without applying `new.tags`. + +What the sweep established, each measured rather than reasoned: + +- `index_file` applies **no tags at all**. A draft the application indexes is + therefore never in `tag:new` scope, and the hook has nothing to carve out. +- Whenever the file IS in `tag:new` scope, the carve-out strips `inbox` + correctly, in every filename shape tried: `:2,DS`, `:2,D`, no info suffix, + in `cur/` and in `new/`, with and without the `,U=4` infix. The real file's + shape (`,U=4:2,D`) is among them. +- It survives the orderings too: `notmuch new` first then the app's index, + the app's index first then the rename, an autosave landing between + `notmuch new` and the hook, and the stale-path `remove_message` that makes + the renamed file arrive as new mail. All six left the draft clean. +- The `D` flag is what puts `draft` on the message (`synchronize_flags`), and + the `S` flag is what removes `unread`. The affected file is `:2,D`, which is + why it carries `unread`, and that matches the reported tag set exactly. + +**The one variant that reproduces it** is the general shape rather than a +filename detail: a pass where `inbox` is applied while `tag:new` has ALREADY +been consumed. Modelled as a file indexed at a path the carve-out does not +cover and moved into the drafts folder afterwards, it ends in precisely the +live end state, `draft inbox unread` in Drafts with `,U=4` and `tag:new` at 0. +Nothing revisits a message once the marker is gone, so the tag is permanent. + +**What is still NOT established, and the next step.** The affected account +writes drafts straight to `/Drafts`, which the carve-out +covers (verified against the live config and the live query, which matches the +message by id today), so the reproducing variant's premise does not hold for +it as written. The live log for the pass that added it reads + + 10:10:52 Added 1 new message to the database. Detected 9 file renames. + 10:10:52 post-new: sent-folder carve-out applied over 9 folder(s) + +so the hook DID run on that pass, over a path the query covers, and logged +success. The remaining candidates are all about what the path or the marker +looked like at that instant, not about the query text: the carve-out logs +"applied" on a `notmuch tag` that matched zero messages, so a successful log +line is not evidence the message was in scope. Instrumenting the hook to log +the carve-out's MATCH COUNT, and leaving it to run until the next draft, is +the cheapest way to close it, and is a log-only change to code that tags real +mail unattended. + +The filename also rules one thing in: `1787645266.M802P16149Q3.` is +exactly `MaildirName::fresh()` output, so the application wrote this file. It +is not a draft another client left behind. + +The reproducer scripts are throwaway and were not kept; `indexfile.c` is +fifteen lines around one `notmuch_database_index_file` call and is trivial to +rebuild from this entry if the instrumentation points back at the hook. **Constraints.** -- cgit v1.2.3