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 --- src/mainwindow.cpp | 29 ++++++++++++++++++++++------- src/mainwindow.h | 10 ++++++++++ 2 files changed, 32 insertions(+), 7 deletions(-) (limited to 'src') 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; -- cgit v1.2.3