From 8796ed59d2410f3e47a114b6002fdefb8f259304 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 15 Aug 2026 16:12:07 +0200 Subject: fix(sync): send held edits before the sync-end refresh reads the database An edit made while a sync is running is held rather than sent, because the worker's read-write open blocks on notmuch's exclusive lock. At sync end onExternalSyncStateChanged() refreshed the list first and flushed the held edits afterwards, so the refresh read a database that still carried the old tag, reconciled it into the model, and overwrote the optimistic update the hold had deliberately left applied. The flush then wrote the tag correctly. The database ended up right and the list ended up wrong, with nothing scheduled to re-read it, which is why it looked like the edit had been lost. Reported by hand: a message read during a sync went back to unread when the sync finished. The flush moves ahead of the refresh and keeps both properties it already had. It stays outside the Idle branch, so edits held when /proc/locks becomes unreadable are not stranded waiting for an Idle that never comes, and it stays after the status-bar retire, so its own "N held changes sent" message survives. Both orders leave identical end state, so the first version of the test passed against the defect: after the handler returns the queue is empty and the write has been sent whichever ran first. flushGenerationForTesting() stamps the query generation at flush time, which is what separates them, and the test fails against the old order with Actual: 3, Expected: 2. Co-Authored-By: Claude Opus 5 --- .../2026-08-03-post-0.1.0-usability-closed.md | 43 ++++++++++++++++++++++ 1 file changed, 43 insertions(+) (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md') 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 393279e..c24ba23 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 @@ -5418,3 +5418,46 @@ designs were drafted before asking (keep non-matching rows until an explicit query, defer the refresh while the pointer is over the list, append-only refresh) and none is worth building against a complaint that resolved itself. Reopen only if the churn is reported somewhere it is NOT the view doing its job. + +## 97. An edit made during a sync is reverted in the list when the sync ends + +**Observed (user, 2026-08-15, hand-testing item 89's fix):** "I've waited for +the sync to start, then read an email, noticed the 'unread' tag disappear and +the status bar correctly notifies 'A sync is running, your change will be +applied...'. At the end of the sync process, the read email returned unread." + +**Cause (verified in code).** `onExternalSyncStateChanged()` did two things at +sync end, in this order: `refreshCurrentQuery()` inside the Idle branch, and +`flushHeldEdits()` after it. An edit made during a sync is HELD rather than +sent, because the worker's read-write open blocks on notmuch's exclusive lock +(measured at 9.158s against a 12s hold, returning SUCCESS). So the refresh read +a database that still carried `unread`, reconciled that into the model, and +overwrote the optimistic update the hold had deliberately left applied. The +flush then wrote the tag correctly. + +The database therefore ended up RIGHT and the list ended up WRONG, with nothing +scheduled to re-read it. That is why it read as "the edit was lost" when the +edit had in fact been applied. + +**Fix.** Flush before the refresh. It stays outside the Idle branch, for the +reason it always was: Unknown clears the busy flag, so writes resume from there +and edits already held must not wait for an Idle a broken `/proc/locks` will +never report. It also stays after the status-bar retire, so the flush's own "N +held changes sent" message is not immediately overwritten. Flushing first costs +nothing when nothing is held, since the function returns on an empty queue. + +**Both orders leave IDENTICAL end state, and the first version of the test +passed against the defect.** After the handler returns, the queue is empty and +the write has been sent whichever ran first, so every assertion made afterwards +is blind to the bug. What separates them is the generation at the moment of the +flush: flushing first stamps the generation from before the refresh bumped it. +`flushGenerationForTesting()` exists for that and nothing else; against the old +order the test fails with `Actual: 3, Expected: 2`. + +**Found only by hand.** The suite was green across the reorder, item 89's own +fix was green, and the reversion needed a real cron sync, a real held edit and +someone watching the row. Recorded because it is the second time in one session +that a green suite endorsed a defect a screenshot found. + +**Confirmed by the user:** "one change sent now that the sync has finished. And +the tag I added, sticked to the message." -- cgit v1.2.3