From ddb4ac26b7fb553c9349401dda4510636c67cc9b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 6 Aug 2026 19:12:17 +0200 Subject: fix(sync): hold tag edits made during a background sync A tag edit sent while another process holds notmuch's write lock does not fail: the read-write open blocks and then succeeds. Measured against Slackware's notmuch, 9.158s against a 12s hold, status SUCCESS. Since the worker is a single thread, that blocked open holds up every read queued behind it, so the message pane freezes on whichever thread was selected first and replays the queue when the lock releases. The window now defers instead. While SyncMonitor reports a sync running, a tag change is held rather than sent, and flushed when the sync ends. The optimistic update stands in the meantime, so the row keeps its tag and the edit still counts toward the unsynced indicator, which is what the quit prompt reads. The original diagnosis was that the open fails and the edit is discarded, and a retry was built on it. That was wrong: the error branch in notmuchworker.cpp is unreachable through lock contention. The premise was taken from a plausible-looking error path without provoking the condition, and measurement disproved it. The backlog entry records this rather than quietly correcting it. Verified by hand against a real blocking open, which the tests cannot reach: they drive the deferral through the meta-object and never take a lock. Both locks held for 100s with a tag edit made during the hold. Row kept the tag, status did not expire, indicator rose, window stayed responsive, held edit sent itself on release. The 2s SyncMonitor polling window is knowingly left open: a sync starting between polls is invisible for up to 2s and an edit there still blocks. SyncMonitor::lockHeldIn() would close it at the cost of one file read per tag action, and is recorded as the option to revisit. Also fixes revertPendingTagChange() clearing the entire undo stack after any rejected write, found while working on this. Backlog: item 37 done, and item 46 added for a test that fails only under the offscreen platform, where an 800x800 screen clamps a restored 940px window. Pre-existing and unrelated; the suite is green otherwise. --- src/mainwindow.cpp | 132 +++++++++++++++++++++++++++++++++++++++++++++++++- src/mainwindow.h | 38 +++++++++++++++ src/notmuchworker.cpp | 11 ++++- src/notmuchworker.h | 1 + 4 files changed, 179 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 7cd2769..7c7b801 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1275,9 +1275,15 @@ void MainWindow::revertPendingTagChange() // The undo entry describes a change that never landed, so it would apply a // spurious inverse if the user pressed undo. + // + // undo() alone, deliberately. This used to clear() the whole stack + // afterwards, which threw away every earlier step the user had built up + // because one later write was rejected: undoing an archive of fifty + // threads became impossible if the flag after it happened to land during a + // sync. undo() has already taken the failed command off the redo side of + // the stack, and the commands under it describe changes that did land. if (m_undoStack.canUndo()) m_undoStack.undo(); - m_undoStack.clear(); m_pendingChange = {}; m_pendingThreadIds.clear(); @@ -1287,14 +1293,78 @@ void MainWindow::onWorkerError(const QString &message) { // Spec: the UI updates optimistically and reverts if the write fails. // Without this the list would keep showing a tag the database never got. + // + // A running sync does NOT arrive here. The read-write open blocks on the + // lock and then succeeds rather than failing (measured; see the comment at + // the open in notmuchworker.cpp), so anything reaching this point is a real + // failure that waiting cannot fix. The stall a running sync does cause is + // avoided by not sending the write at all, in sendThreadTagChange(). revertPendingTagChange(); + updatePendingIndicator(); m_statusLabel->setText(message); } +bool MainWindow::aSyncHoldsTheWriteLock() const +{ + // Both sources, exactly as updateSyncControls() reads them. A local sync + // holds the same exclusive lock a cron one does, so an edit made during it + // would block on precisely the same open. + return m_localSyncBusy || m_externalSyncBusy; +} + +void MainWindow::flushHeldEdits() +{ + if (m_heldEdits.isEmpty()) + return; + + // Taken by value and cleared first: sendThreadTagChange() writes + // m_pendingThreadIds, and re-entering partway through the queue must not + // find the same edits still waiting. + const QVector edits = m_heldEdits; + m_heldEdits.clear(); + + for (const HeldEdit &edit : edits) { + // Take the optimistic update back before sending, because + // sendThreadTagChange() applies it again. applyTagChange() is + // idempotent per tag so the rows do not visibly flicker; without this + // the change is applied twice and a later revert undoes only one of + // them, leaving a row showing a tag the database never got. + for (const QString &threadId : edit.threadIds) { + m_model->applyTagChange(threadId, edit.change.removed, + edit.change.added); + } + + sendThreadTagChange(edit.threadIds, edit.change.added, + edit.change.removed, edit.change.description); + } + + // Held edits stop counting as held; what counts now is whatever + // onTagsApplied() confirms. + updatePendingIndicator(); + + showTransientStatus( + tr("%n held change(s) sent now that the sync has finished", "", + int(edits.size()))); +} + void MainWindow::onSyncFinished(bool success, int exitCode) { setSyncBusy(false); + // The local sync no longer holds the write lock, whatever its outcome, so + // edits held during it can go now. + // + // The count below is safe: applyTagsToThreads is a QUEUED call, so the + // onTagsApplied() that records these edits arrives after this function has + // returned, and therefore after the success branch has cleared the map. + // They are counted, not wiped. + // + // These edits reach the index after the sync that would have carried them, + // so they go to the mail store on the NEXT run. That is the same one-run + // delay any edit made mid-sync gets, bounded by the cron interval. + const bool sentHeldEdits = !m_heldEdits.isEmpty(); + flushHeldEdits(); + if (success) { // Only a SUCCESSFUL sync clears the count. Clearing on failure would // assert the edits had reached the mail store when the sync is exactly @@ -1306,6 +1376,21 @@ void MainWindow::onSyncFinished(bool success, int exitCode) showTransientStatus(tr("Sync complete")); if (m_syncingForExit) { + // Edits held during THIS sync were only just sent, on a queued + // connection, so they have not reached the index yet and this sync + // certainly did not carry them. Quitting here would discard exactly + // the work the prompt exists to protect. Tell the user and stay + // open; the indicator shows what is still outstanding. + if (sentHeldEdits) { + m_syncingForExit = false; + QMessageBox::information( + this, tr("Changes still to sync"), + tr("Changes you made while the sync was running have only " + "now been applied, so that sync did not carry them. " + "Sync once more before quitting.")); + return; + } + // The work is safely across, so finish the quit the user asked for. m_syncingForExit = false; m_closeApproved = true; @@ -1430,6 +1515,11 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) m_localSyncHoldsLock = false; m_externalSyncBusy = false; updateSyncControls(); + + // A local sync releases the write lock exactly as a background one + // does, and an edit made during it is held the same way. Without this + // the held edits would wait for the NEXT sync to come and go. + flushHeldEdits(); return; } @@ -1453,6 +1543,14 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) tr("Background sync completed. Press Enter in the query bar to " "refresh.")); } + + // 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) @@ -1513,7 +1611,13 @@ void MainWindow::recordPendingEdit(const QString &messageId, const QString &tag, int MainWindow::pendingEditCount() const { - return m_pendingTagEdits.size() + m_unnettablePendingEdits; + // A held edit has NOT reached the index, so onTagsApplied() never counted + // it. It still has to count here: this is what the exit prompt reads, and + // an edit waiting on a lock is precisely the work quitting would lose. + // Each held edit counts as one whatever its size, since it carries thread + // ids rather than message ids and cannot be netted against the map. + const int held = int(m_heldEdits.size()); + return m_pendingTagEdits.size() + m_unnettablePendingEdits + held; } void MainWindow::updatePendingIndicator() @@ -1679,6 +1783,30 @@ void MainWindow::sendThreadTagChange(const QStringList &threadIds, m_messageView->setTags(m_model->threadAt(current.row()).tags); } + // A sync holds notmuch's exclusive write lock, and the worker's read-write + // open BLOCKS on it rather than failing: measured 9.158s against a 12s + // hold, returning SUCCESS. Sending now would freeze the worker thread for + // the rest of the sync, queueing every later query and thread load behind + // it. Hold the edit and send it when the lock frees. + // + // The rows keep the optimistic update applied above, which is honest: it is + // what the user asked for and it is going to be applied. + if (aSyncHoldsTheWriteLock()) { + m_heldEdits.append(HeldEdit{ + threadIds, TagChange{ {}, add, remove, description } }); + + // NOT transient. This describes state that lasts until the sync ends, + // and a message that expired would leave the user with rows showing a + // tag the database has not got and no explanation of why. + m_statusLabel->setText( + tr("A sync is running; your change will be applied when it " + "finishes.")); + + // A held edit is outstanding work, so the indicator has to show it. + updatePendingIndicator(); + return; + } + m_pendingThreadIds = threadIds; m_pendingChange = TagChange{ {}, add, remove, description }; diff --git a/src/mainwindow.h b/src/mainwindow.h index cf9aca6..22c8187 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -68,6 +68,15 @@ public: /// load is discarded rather than painted, so no thread can reappear. QString currentThreadId() const { return m_currentThreadId; } + /// True while an edit is held back because a sync holds the write lock. + /// Exposed for tests: the deferral is otherwise only observable by watching + /// the worker, which test_mainwindow has no database to drive. + bool hasEditAwaitingSend() const { return !m_heldEdits.isEmpty(); } + + /// Whether the undo stack still holds anything. Exposed so a test can show + /// that a rejected write did not take unrelated history down with it. + bool canUndo() const { return m_undoStack.canUndo(); } + /// The cid: namespace prefix for the nth message of a thread. /// /// MainWindow is the only producer of this value in the application. It @@ -231,6 +240,35 @@ private: /// Undoes the optimistic model update for a write the worker rejected. void revertPendingTagChange(); + /// Whether a write sent now would block the worker on notmuch's write lock. + /// + /// True only for a sync KNOWN to be running. `SyncMonitor::State::Unknown` + /// deliberately does not count: it means `/proc/locks` could not be read, + /// and holding every edit on a platform that cannot observe the lock at all + /// would strand them permanently. + bool aSyncHoldsTheWriteLock() const; + + /// Sends every edit held while the lock was busy, oldest first. + void flushHeldEdits(); + + /// A tag change not yet sent to the worker, because a sync held the write + /// lock when the user made it. + /// + /// Held rather than sent because the read-write open BLOCKS: measured + /// 9.158s against a 12s lock hold, returning SUCCESS, not an error. Sending + /// into that freezes the worker thread, so every later query and thread + /// load queues behind it. The rows show the change meanwhile, which is + /// honest: it is what the user asked for and it is going to be applied. + struct HeldEdit { + QStringList threadIds; + TagChange change; + }; + + /// FIFO, because a sync lasts ~35s and the user can keep tagging through + /// 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; + friend class ThreadTagCommand; Config m_config; diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index a5096d2..3525a2b 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -296,8 +296,17 @@ void NotmuchWorker::applyTags(const TagChange &change) &error); if (status != NOTMUCH_STATUS_SUCCESS) { + // NOT reached by lock contention, despite the wording. Measured + // 2026-08-04: this call BLOCKS on a held write lock and then returns + // SUCCESS (9.158s against a 12s hold), so a running sync never lands + // here. What does land here is a genuinely broken open: bad + // permissions, a corrupt index, a missing database. None of those are + // helped by waiting, so the UI reverts rather than retrying. + // + // The stall a running sync DOES cause is avoided upstream, in + // MainWindow, by not sending the write at all while the lock is held. emit errorOccurred( - QStringLiteral("Cannot open database for writing (is a sync running?): %1") + QStringLiteral("Cannot open database for writing: %1") .arg(QString::fromUtf8(error ? error : notmuch_status_to_string(status)))); free(error); diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 88187ec..96e75f8 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -80,6 +80,7 @@ signals: void threadLoaded(const QVector &messages, quint64 generation); void tagsApplied(const TagChange &change); void allTagsReady(const QStringList &tags, quint64 generation); + void errorOccurred(const QString &message); private: -- cgit v1.2.3