diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 203 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 132 | ||||
| -rw-r--r-- | src/mainwindow.h | 38 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 11 | ||||
| -rw-r--r-- | src/notmuchworker.h | 1 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 199 |
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. |