diff options
| -rw-r--r-- | CHANGELOG.md | 5 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 43 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 1 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 29 | ||||
| -rw-r--r-- | src/mainwindow.h | 10 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 95 |
6 files changed, 176 insertions, 7 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 816f03b..61a96bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,11 @@ point at which they are stable. pending until a manual sync or the next cron run. It now re-arms at the configured delay. Skipping a concurrent run is unchanged: two mbsync runs cannot share the lock. +- A tag change made while a sync was running reappeared undone in the thread + list when that sync finished. The change was held until the sync released + notmuch's write lock, but the list was refreshed from the database before the + held change was written to it, so the refresh painted the old tag back. The + change itself was never lost, only the list was wrong. ## [0.23.0] - 2026-08-15 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." 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 14a242a..4a68f0b 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 @@ -161,6 +161,7 @@ taking that too literally. | 95 | A query in the overflow menu cannot be run | defect | XS | **done** 2026-08-15, unreleased. Pre-existing and not caused by 93: the entry's action owned a submenu, and Qt emits no `triggered` for those, so the connection had never fired. Surfaced because 93 moved every query into the menu | | 94 | `pinned` has nothing left to decide once the buttons are built-in | maintenance | S | open; **blocked on 93**, and deliberately not part of it. A user-visible removal: the row becomes built-ins only and every saved query lives in the menu | | 96 | A query returning the thread already on display opens onto the placeholder | defect | S | **done** 2026-08-15, unreleased. Split from 66's unverified half, which had a different cause. Reproduced from two screenshots after four measured eliminations | +| 97 | An edit made during a sync is reverted in the list when the sync ends | defect | S | **done** 2026-08-15, unreleased. Found by hand-testing item 89's fix. The sync-end refresh ran BEFORE the held-edit flush, so it read a database that still carried the old tag | Sizes are rough: XS under an hour, S a sitting, M a session. diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 3c2dca6..ba803bc 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -2798,6 +2798,9 @@ void MainWindow::flushHeldEdits() const QVector<HeldEdit> edits = m_heldEdits; m_heldEdits.clear(); + // Stamped for the ordering test. See flushGenerationForTesting(). + m_flushGeneration = m_generation; + for (const HeldEdit &edit : edits) { // Take the optimistic update back before sending, because // sendThreadTagChange() applies it again. applyTagChange() is @@ -3248,6 +3251,25 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) m_statusLabel->setText(m_defaultStatus); } + // BEFORE the refresh below, and the order is the whole of a defect. An edit + // made during a sync is held, because the worker's read-write open blocks + // on notmuch's exclusive lock. Refreshing first meant reading a database + // that still carried the old tag and reconciling that into the model, which + // overwrote the optimistic update; the flush then wrote the tag correctly, + // leaving the database right and the list wrong with nothing scheduled to + // re-read it. Reported by hand as a message going back to unread at the end + // of the sync it was read during. + // + // Flushing first also costs nothing when there is nothing held: the + // function returns immediately on an empty queue. + // + // OUTSIDE the Idle branch, deliberately, and this predates the reordering. + // Unknown clears the busy flag above, so writes resume from here on; + // leaving the flush inside Idle would let a new edit go straight out while + // the ones already held sat waiting for an Idle that a broken /proc/locks + // will never report. + flushHeldEdits(); + if (state == SyncMonitor::State::Idle) { refreshCurrentQuery(); @@ -3279,13 +3301,6 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) } } - // OUTSIDE the Idle branch, deliberately. Unknown clears the busy flag above, - // so writes resume from here on; leaving the flush inside Idle would let a - // new edit go straight out while the ones already held sat waiting for an - // Idle that a broken /proc/locks will never report. After the status - // message, which flushHeldEdits() overwrites with its own when it sent - // something. - flushHeldEdits(); } void MainWindow::showTransientStatus(const QString &text) diff --git a/src/mainwindow.h b/src/mainwindow.h index e2d1861..2a86d12 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -207,6 +207,15 @@ public: return m_refreshGeneration; } + /// The query generation as it stood when the held edits were last flushed, + /// or 0 if they never have been. + /// + /// Recorded because the ORDER of the flush and the sync-end refresh is the + /// whole of one defect and both leave identical end states: a test that + /// looks afterwards passes whichever ran first. Compared against the + /// generation the refresh bumps, this says which came first. + quint64 flushGenerationForTesting() const { return m_flushGeneration; } + /// The generation a database-stats reply must carry to be accepted. /// /// A test seam, for the same reason as the one above: onDatabaseStatsReady @@ -700,6 +709,7 @@ private: /// it. Order matters: two edits touching one thread must reach the database /// in the order they were made, or the later one does not win. QVector<HeldEdit> m_heldEdits; + quint64 m_flushGeneration = 0; friend class ThreadTagCommand; friend class MessageTagCommand; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 08b2f84..253d3e6 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -202,6 +202,7 @@ private slots: void autoSyncIsNotArmedWhenDisabledOrWithNothingPending(); void autoSyncSkipsWhileABackgroundSyncIsRunning(); void aSkippedAutoSyncRearmsRatherThanGivingUp(); + void aHeldEditIsSentBeforeTheSyncEndRefreshReadsTheDatabase(); void aSuccessfulSyncRefreshesRatherThanRerunningTheQuery(); void markReadCanBeDisabled(); void pendingEditCountSurvivesAQuery(); @@ -4106,6 +4107,100 @@ void TestMainWindow::aSkippedAutoSyncRearmsRatherThanGivingUp() "the re-armed sync cleared the pending indicator"); } +void TestMainWindow::aHeldEditIsSentBeforeTheSyncEndRefreshReadsTheDatabase() +{ + // Reported by hand, 2026-08-15: read a message while a cron sync is + // running, watch the unread tag go and the status bar say the change will + // be applied when the sync finishes, and when it does the message is unread + // again. + // + // The edit is held during a sync because the worker's read-write open + // BLOCKS on notmuch's exclusive lock. At sync end the handler refreshed the + // list FIRST and flushed the held edits afterwards, so the refresh read a + // database that still carried `unread`, reconciled it into the model, and + // overwrote the optimistic update. 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 was lost. + // + // Asserted on the ORDER, not on the tag: this window has no worker to + // answer either the refresh or the write, so the rows cannot show the + // outcome. What decides the bug is whether the edit had been sent by the + // time the refresh was issued. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString locks = dir.filePath(QStringLiteral("locks")); + { + QFile f(locks); + QVERIFY(f.open(QIODevice::WriteOnly)); + // A held lock, so the edit below is held rather than sent. + f.write("1: FLOCK ADVISORY WRITE 1 00:00:0 0\n"); + } + MainWindow::setLocksPathForTesting(locks); + + Config config; + config.load(writeSyncConfig(dir)); + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + selectThreadRow(view, 0); + + // The sync is observed as running, which is what makes the edit held. + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + + auto *toggleUnread = + window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(toggleUnread); + toggleUnread->trigger(); + + QVERIFY2(window.hasEditAwaitingSend(), + "the edit was not held, so this test proves nothing about the " + "order the sync-end handler does things in"); + + const quint64 before = window.currentGenerationForTesting(); + + // The sync ends. Both the refresh and the flush happen in this one call. + { + QFile f(locks); + QVERIFY(f.open(QIODevice::WriteOnly | QIODevice::Truncate)); + } + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + + QVERIFY2(!window.hasEditAwaitingSend(), + "the sync ended without ever flushing the held edit"); + + // The write went out. Nothing else in this handler sends one, so its + // presence is what proves the flush ran, and the refresh below is what it + // has to have run BEFORE. + QVERIFY2(!window.pendingThreadIdsForTesting().isEmpty(), + "the held edit was dropped rather than sent"); + + const quint64 after = window.currentGenerationForTesting(); + QVERIFY2(after > before, "the sync end did not refresh the list at all"); + + // THE ASSERTION THIS TEST EXISTS FOR. Both orders leave identical end + // state, so everything above passes against the defect; only the + // generation stamped at flush time separates them. + // + // Flushing first means the stamp is the generation from BEFORE the refresh + // bumped it. Refreshing first means the flush sees the bumped one, and the + // refresh has already read a database that still carries the old tag. + QCOMPARE(window.flushGenerationForTesting(), before); +} + void TestMainWindow::aSuccessfulSyncRefreshesRatherThanRerunningTheQuery() { // Reported by hand against item 71: reading a message in the Unread view, |