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 --- CHANGELOG.md | 5 ++ .../2026-08-03-post-0.1.0-usability-closed.md | 43 ++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 1 + src/mainwindow.cpp | 29 +++++-- src/mainwindow.h | 10 +++ 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 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 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(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + 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(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, -- cgit v1.2.3