aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-06 19:12:17 +0200
committerDanilo M. <danix@danix.xyz>2026-08-06 19:12:17 +0200
commitddb4ac26b7fb553c9349401dda4510636c67cc9b (patch)
tree30a5840ad8f1a5081b945b1ac98d5769c9e55532
parent46c82a09903730133b06e28140d29f16b68813f4 (diff)
downloadqtmaildir-ddb4ac26b7fb553c9349401dda4510636c67cc9b.tar.gz
qtmaildir-ddb4ac26b7fb553c9349401dda4510636c67cc9b.zip
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.
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md203
-rw-r--r--src/mainwindow.cpp132
-rw-r--r--src/mainwindow.h38
-rw-r--r--src/notmuchworker.cpp11
-rw-r--r--src/notmuchworker.h1
-rw-r--r--tests/test_mainwindow.cpp199
6 files changed, 581 insertions, 3 deletions
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 99e3b64..0186bfa 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
@@ -84,6 +84,8 @@ taking that too literally.
| 34 | No overview of the Maildir itself | information | M | open |
| 35 | No refresh of the thread list after a sync | workflow | M | open |
| 36 | `test_mainwindow` cannot reach the worker | testing | S | open, on demand |
+| 37 | The worker stalls on a tag edit made during a background sync | correctness | S | **done** |
+| 38 | `test_mainwindow` fails when a real sync holds the lock | testing | XS | open |
| 39 | Thread list cannot be sorted by clicking a column header | workflow | S | open |
| 40 | No live filter over the current view | workflow | M | open |
| 41 | A message whose HTML body carries a `Content-Id` renders blank | correctness | S | open |
@@ -91,6 +93,7 @@ taking that too literally.
| 43 | No "Mark all read" for the current view | workflow | S | open |
| 44 | No way to manage the filters applied at sync time | workflow | ? | open, unspecified |
| 45 | Two Sync buttons on the main window | discoverability | XS | open |
+| 46 | `uiStateSurvivesARestart` fails under the offscreen platform | testing | XS | open |
Sizes are rough: XS under an hour, S a sitting, M a session.
@@ -1753,6 +1756,167 @@ rather than modelled.
`MainWindow` and must keep working. The fixture is per-test, not a suite-wide
`initTestCase`, or every case pays for a `notmuch new`.
+## 37. The worker stalls on a tag edit made during a background sync
+
+**Observed:** the user's note asks whether edits made while a background sync is
+running are carried by that same job or need a manual sync afterwards. The
+answer splits in two, and the second half is a defect rather than a question.
+
+**This entry was rewritten on 2026-08-04 after its original cause was
+disproved by measurement.** It first claimed the read-write open *fails* during
+a sync and the edit is discarded. It does not fail. That claim was written from
+the plausible reading of the error path at `src/notmuchworker.cpp:298-305`
+without ever provoking the condition, and a fix was built on it before anyone
+checked. Recorded here rather than quietly corrected, because the same
+false-cause-from-a-plausible-error-path mistake is cheap to repeat.
+
+**Cause, part one: reaching the disk is not the problem.** `applyTags` calls
+`notmuch_message_tags_to_maildir_flags()` (`src/notmuchworker.cpp:327`)
+immediately after thawing, so a `seen`/`flagged` change renames the file in the
+Maildir at edit time. No manual sync is needed for the change to exist on disk.
+Whether the *running* mbsync carries it is a matter of ordering: mbsync scans
+each mailbox once per run, so an edit landing after that box was scanned goes
+out on the next run. That is expected behaviour, not a bug, and the ten-minute
+cron interval bounds the delay. This half of the note is a question, answered.
+
+**Cause, part two: the write blocks, it does not fail.** Measured 2026-08-04
+against Slackware's notmuch, with the lock held deliberately rather than by
+racing cron:
+
+- `notmuch_database_open_with_config(NOTMUCH_DATABASE_MODE_READ_WRITE, …)`,
+ the exact call `applyTags` makes, **blocks until the lock is free and then
+ returns `NOTMUCH_STATUS_SUCCESS`**. A C probe against a lock held for 12s
+ returned after 9.158s with status 0; the same call with no lock held returns
+ in 0.001s. It was never observed to return an error or to time out.
+- The `notmuch` CLI behaves identically (waits 3.6s and 13.2s against 5s and
+ 15s holds, always exit 0), so this is libnotmuch's behaviour and not a
+ wrapper's retry loop.
+- Therefore the error branch at `src/notmuchworker.cpp:298-305` is **not
+ reachable through lock contention at all**. It fires only for a genuinely
+ broken open: bad permissions, a corrupt index, a missing database.
+
+**The real defect is a stall.** `applyTagsToThreads` is invoked on the worker
+thread through a queued connection (`src/mainwindow.cpp:1788`), so a blocking
+open freezes *the worker*, not the UI. The window keeps painting and the rows
+show the optimistic update, but every later query, thread load and tag write
+sits behind that open in the worker's event queue until the lock frees. Nothing
+is lost and no error appears; the application simply stops responding to
+selections for the duration.
+
+**How bad in practice.** Bounded by how long `notmuch new` holds the lock, which
+on this user's already-indexed Maildir is a fraction of a second at roughly
+T+32s into a ~35s run (measured from `~/.local/state/mailsync.log`: runs start
+at :00 and reach "Processed N total files" 32-40s later). The stall is
+therefore usually invisible, and becomes user-visible only when `notmuch new`
+has real work: a first index, a large delivery, a `notmuch reindex`. That is
+also why it cannot be reproduced by clicking during a normal sync, and why the
+reproduction below holds the lock on purpose.
+
+**Reproducing it.** Racing cron does not work. Hold the lock deliberately:
+`notmuch tag --batch` keeps the write lock for a whole session and releases it
+when stdin closes, so feeding it a slow stream of no-op tag commands holds the
+lock for a controllable time. Verified: a competing writer blocks for exactly
+the remaining hold.
+
+**Approach.** Do not send a write the worker will block on. `SyncMonitor`
+(item 27) already reports whether a sync holds the lock, so `MainWindow` can
+hold the edit while `State::Running` and send it on the transition to `Idle`,
+which is a signal that already exists and already fires. The rows keep showing
+the change meanwhile, which is honest: it is what the user asked for and it is
+going to be applied.
+
+**Explicitly rejected: retrying on error.** That was the first implementation
+and it is dead code against this cause, since the error it keys off never
+arrives from lock contention. Keying on the monitor's state is also strictly
+better: it avoids the stall rather than recovering from it.
+
+**Constraints.** The optimistic-update-then-revert contract must survive: a
+held edit is still unsynced and must keep counting toward the pending indicator,
+or the quit prompt will let the user leave on work that never landed (the
+failure item 28 and the 0.9.0 net-state fix were both about). Do not widen the
+write window by holding the read-write handle open, per the read-only-by-default
+rule in `CLAUDE.md`. A held edit must re-resolve its thread ids when it is
+finally sent: `notmuch new` may have renamed files underneath it. And
+`SyncMonitor::State::Unknown` must not gate writes, or a platform that cannot
+read `/proc/locks` would never send an edit at all.
+
+**What the stall looks like, observed 2026-08-04.** Confirmed by hand with the
+Xapian lock held deliberately: switching between threads left the message pane
+showing the FIRST thread selected, and when the lock released the pane stepped
+through the three or four threads selected in the meantime, in sequence. That
+is the worker's queue draining, and it is the user-visible shape of this defect.
+
+**Reads are NOT blocked by the write lock.** Measured the same day, and it
+bounds how bad this is. A read-only open and a 200-thread query take 0.001s and
+0.015s whether or not another process holds the write lock, identical to
+baseline. So `loadThread` never blocks on the lock itself. The stall is purely
+head-of-line blocking on the single worker thread: one blocked `applyTags` holds
+up every read queued behind it. Do not "fix" this by making reads lock-aware;
+there is nothing there to fix.
+
+**Residual gap: the 2s polling window. Accepted for now (user, 2026-08-06),
+and deliberately left open rather than closed.** `SyncMonitor` polls every two
+seconds, so a sync that starts between polls is invisible to the window for up
+to 2s, and a tag edit in that window is still sent straight into a blocking
+open. The window is 2s wide, `notmuch new` holds the lock for well under a
+second on an already-indexed Maildir, and reads are unaffected either way, so
+the exposure is small and the shipped behaviour is the pre-existing one.
+
+**The option stays on the table, to revisit:** check the lock at send time.
+`SyncMonitor::lockHeldIn()` is already a static, pure function over
+`/proc/locks` content, so `sendThreadTagChange` can call it for the cost of one
+small file read per tag action. That closes the window entirely. It was not
+taken now because it would add untested code to a change that had just been
+verified by hand, which is the wrong order.
+
+### Hand test (2026-08-06): passed
+
+Verified against a real blocking open, which the unit tests cannot reach: they
+drive the deferral through the meta-object and never take a lock. Both locks
+held for 100s by the throwaway scaffold, with a tag edit made during the hold.
+All six expected behaviours confirmed by the user: the row kept the tag, the
+status message did not expire, the unsynced indicator rose, the Sync button was
+disabled, **the window stayed responsive**, and the held edit sent itself on
+release without a click.
+
+The responsiveness check is the one that mattered. The stall observed on
+2026-08-04 left the message pane frozen on the first thread selected and
+replayed the queue on release; that no longer happens.
+
+**Related:** item 35 (refresh after sync) touches the same `Idle` transition,
+and both want a non-destructive path that does not clear the undo stack.
+
+## 38. `test_mainwindow` fails when a real sync holds the lock
+
+**Observed 2026-08-04:** `theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock`
+failed once during a full run and passed on every rerun. The cause is not
+ordering or pollution between tests: the user's cron sync happened to be running
+at that moment.
+
+**Cause (verified in code).** `MainWindow::buildUi()` constructs a real
+`SyncMonitor` on `SyncMonitor::defaultLockPath()` and the live `/proc/locks`
+(`src/mainwindow.cpp:467`) and starts it. Every `MainWindow` a test builds
+therefore observes the machine's actual sync state. The test asserts
+`button->isEnabled()` on a freshly built window, which is false whenever a real
+sync holds `/tmp/mbsync.lock`. With cron firing every ten minutes and a run
+lasting ~35s, roughly 6% of test runs land inside one.
+
+**Not caused by the item 37 work**, though that is when it was noticed. The
+test and the monitor both predate it; confirmed by stashing the item 37 changes
+and seeing the suite pass, then reproducing the failure with a sync live.
+
+**Approach.** The monitor is already injectable: its constructor takes a
+`locksPath` precisely so tests can drive transitions without real locks
+(`src/syncmonitor.h:59-64`), and `test_syncmonitor` uses that. `MainWindow` does
+not expose it. Either let the window take a locks path (config or a setter used
+only by tests), or have the test point `SyncMonitor` at a temporary file. The
+existing tests that drive `onExternalSyncStateChanged` through the meta-object
+are unaffected either way; it is only the construction-time state that leaks in.
+
+**Constraint:** do not simply stop starting the monitor in tests. The
+construction-time state IS the behaviour under test for this case, and a window
+that never polls would pass the assertion for the wrong reason.
+
## 39. Thread list cannot be sorted by clicking a column header
**Observed (user, 2026-08-05):** "left pane columns order by clicking on the
@@ -2004,6 +2168,45 @@ last widget that predates it.
Confirm with the user which of the two survives; the note says redundant, not
which one is wanted.
+## 46. `uiStateSurvivesARestart` fails under the offscreen platform
+
+**Observed 2026-08-06:** the full suite is green on the user's Wayland session
+but `TestMainWindow::uiStateSurvivesARestart` fails under
+`QT_QPA_PLATFORM=offscreen`, which is how the suite is run when a session must
+not open windows on the user's screen. Only the width is wrong:
+
+```
+Actual (reopened.size()): QSize(798x620)
+Expected (resized) : QSize(940x620)
+```
+
+**Cause (verified by probe, not assumed).** The offscreen platform reports an
+800x800 screen. `QMainWindow::restoreGeometry()` clamps a restored window to the
+available screen area, so the test's 940 width comes back as 798 while its 620
+height, which fits, restores untouched. That asymmetry is the tell: the state
+file is written and read correctly, and the zoom factor in the same test
+restores fine. Nothing is broken in item 1's persistence.
+
+**Not a state-file collision.** The test already scopes itself properly with
+`QStandardPaths::setTestModeEnabled(true)` and removes the file at both ends
+(`tests/test_mainwindow.cpp:229-255`), so it never touches
+`~/.local/state/qtmaildir/uistate.conf`. An earlier reading of this failure
+blamed the real state file being held by a running app; that was wrong, and the
+test source disproves it.
+
+**Approach.** Pick a size the smallest plausible test screen can hold, well
+under 800x800, and assert on that. The test is about persistence, not about
+large windows, so the specific number carries no meaning and only needs to
+differ from the default.
+
+**Constraint:** do not "fix" this by widening the assertion to a tolerance or by
+skipping under offscreen. Both would hide a genuine restore failure later, and
+the property under test (the size that went in comes back out) is exact.
+
+**Related:** the same class as item 38. Both are tests that silently depend on
+the machine they run on, and both were found by running the suite in a context
+its author had not tried rather than by reading it.
+
## Deferred, unsized, or split out
Items noted while triaging but not part of the original list. Same numbering
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<HeldEdit> 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<HeldEdit> 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<MessageRef> &messages, quint64 generation);
void tagsApplied(const TagChange &change);
void allTagsReady(const QStringList &tags, quint64 generation);
+
void errorOccurred(const QString &message);
private:
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index d133cfb..c409d1a 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -89,6 +89,11 @@ private slots:
void anEditUndoneNettsBackToZero();
void aDifferentTagOnTheSameMessageStillCounts();
void anEditWithNoMessageIdsStillCounts();
+ void anEditDuringABackgroundSyncIsNotSentYet();
+ void aHeldEditIsSentWhenTheBackgroundSyncEnds();
+ void aHeldEditCountsAsUnsynced();
+ void anUnreadableLockTableStillSendsTheEdit();
+ void aRejectedWriteKeepsEarlierUndoHistory();
};
void TestMainWindow::everyKnownActionIsRegistered()
@@ -1273,6 +1278,200 @@ void TestMainWindow::anEditWithNoMessageIdsStillCounts()
"an edit with no message ids was not counted at all");
}
+// Item 37. A tag edit made while a background sync holds notmuch's write lock
+// used to stall the worker: the read-write open BLOCKS until the lock frees
+// (measured 9.158s against a 12s hold, returning NOTMUCH_STATUS_SUCCESS), so
+// every later query and thread load queued behind it. These cases pin the fix:
+// do not send the write while a sync is running, send it when the sync ends.
+
+void TestMainWindow::anEditDuringABackgroundSyncIsNotSentYet()
+{
+ // The defect. Sending during the sync is what stalls the worker, so the
+ // edit is held instead. The rows still show it: it is what the user asked
+ // for and it is going to be applied.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTableView *>();
+ QVERIFY(view);
+ auto *action = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY2(action, "no flag action registered");
+
+ model->appendBatch({ makeThread(QStringLiteral("t1"), {}) });
+ view->selectRow(0);
+
+ // A cron sync takes the lock.
+ QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+ Q_ARG(SyncMonitor::State,
+ SyncMonitor::State::Running));
+
+ action->trigger();
+
+ QVERIFY2(window.hasEditAwaitingSend(),
+ "the edit was sent straight into a running sync, which is the "
+ "blocking open that stalls the worker");
+ QVERIFY2(model->threadAt(0).tags.contains(QStringLiteral("flagged")),
+ "holding the edit also dropped it from the rows");
+}
+
+void TestMainWindow::aHeldEditIsSentWhenTheBackgroundSyncEnds()
+{
+ // The release. SyncMonitor already reports this transition for item 27, so
+ // the held edit rides a signal that exists rather than a timer.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTableView *>();
+ QVERIFY(view);
+ auto *action = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY(action);
+
+ model->appendBatch({ makeThread(QStringLiteral("t1"), {}) });
+ view->selectRow(0);
+
+ QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+ Q_ARG(SyncMonitor::State,
+ SyncMonitor::State::Running));
+ action->trigger();
+ QVERIFY(window.hasEditAwaitingSend());
+
+ QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+ Q_ARG(SyncMonitor::State,
+ SyncMonitor::State::Idle));
+
+ QVERIFY2(!window.hasEditAwaitingSend(),
+ "the sync ending did not send the held edit");
+ QVERIFY2(model->threadAt(0).tags.contains(QStringLiteral("flagged")),
+ "sending the held edit lost the tag from the rows");
+}
+
+void TestMainWindow::aHeldEditCountsAsUnsynced()
+{
+ // A held edit has not reached the index, so onTagsApplied() never counted
+ // it. It must still count here, because this is what the exit prompt reads:
+ // quitting on a held edit loses it outright, which is the whole failure the
+ // prompt exists to prevent.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTableView *>();
+ QVERIFY(view);
+ auto *action = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY(action);
+ auto *label = window.findChild<QLabel *>(QStringLiteral("pendingEdits"));
+ QVERIFY(label);
+ QVERIFY2(label->isHidden(), "the indicator starts hidden at zero");
+
+ model->appendBatch({ makeThread(QStringLiteral("t1"), {}) });
+ view->selectRow(0);
+
+ QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+ Q_ARG(SyncMonitor::State,
+ SyncMonitor::State::Running));
+ action->trigger();
+
+ QVERIFY2(!label->isHidden(),
+ "an edit held for a running sync was not counted as unsynced, so "
+ "the exit prompt would let the user quit on it");
+}
+
+void TestMainWindow::anUnreadableLockTableStillSendsTheEdit()
+{
+ // State::Unknown means /proc/locks could not be read, so nothing is
+ // observed. Holding writes there would strand every edit forever on a
+ // platform that cannot see the lock at all. Unknown is not "running".
+ //
+ // Driven from Running, not from a fresh window: the guard is that Unknown
+ // CLEARS the busy flag, and a window that was never busy would pass this
+ // whatever Unknown did. Reaching Unknown by way of Running is also the only
+ // way a real monitor gets there, when /proc/locks becomes unreadable
+ // mid-session.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTableView *>();
+ QVERIFY(view);
+ auto *action = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY(action);
+
+ model->appendBatch({ makeThread(QStringLiteral("t1"), {}),
+ makeThread(QStringLiteral("t2"), {}) });
+
+ QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+ Q_ARG(SyncMonitor::State,
+ SyncMonitor::State::Running));
+ view->selectRow(0);
+ action->trigger();
+ QVERIFY2(window.hasEditAwaitingSend(),
+ "the edit was not held during a running sync, so this test is not "
+ "exercising the Unknown transition it claims to");
+
+ // The lock table becomes unreadable. That is not evidence of a sync, so
+ // writing must resume and the held edit must go out.
+ QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+ Q_ARG(SyncMonitor::State,
+ SyncMonitor::State::Unknown));
+ QVERIFY2(!window.hasEditAwaitingSend(),
+ "an unreadable lock table kept the edit held, stranding it on any "
+ "platform without /proc/locks");
+
+ // And a NEW edit is sent rather than held.
+ view->selectRow(1);
+ action->trigger();
+ QVERIFY2(!window.hasEditAwaitingSend(),
+ "an unreadable lock table held a new edit, so writes never resume");
+}
+
+void TestMainWindow::aRejectedWriteKeepsEarlierUndoHistory()
+{
+ // revertPendingTagChange() used to undo the failed command and then CLEAR
+ // the whole stack, so one rejected write threw away every undo step the
+ // user had built up. Undoing the failed command is enough: it is already
+ // off the stack afterwards.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTableView *>();
+ QVERIFY(view);
+ auto *flag = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY(flag);
+ auto *archive = window.findChild<QAction *>(QStringLiteral("archive"));
+ QVERIFY2(archive, "no archive action registered");
+
+ model->appendBatch({ makeThread(QStringLiteral("t1"),
+ { QStringLiteral("inbox") }) });
+ view->selectRow(0);
+
+ // One edit that succeeds, so there is history worth keeping.
+ archive->trigger();
+ TagChange applied;
+ applied.messageIds = { QStringLiteral("m1") };
+ applied.removed = { QStringLiteral("inbox") };
+ applied.description = QStringLiteral("Archive");
+ QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied",
+ Q_ARG(TagChange, applied)));
+
+ // A second edit that the worker rejects outright.
+ flag->trigger();
+ QVERIFY(QMetaObject::invokeMethod(
+ &window, "onWorkerError",
+ Q_ARG(QString, QStringLiteral("Cannot resolve threads"))));
+
+ QVERIFY2(window.canUndo(),
+ "a rejected write cleared the undo history of edits that had "
+ "already succeeded");
+}
+
// Constructing a MainWindow needs a QApplication and a platform plugin. The
// test has no display under ctest, so it runs offscreen unless the caller
// asked for something else.