From 98914f2c184539c9be9cac7c3ae8def6820868b8 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 20 Aug 2026 09:47:11 +0200 Subject: fix(worker): read the mail root, not the index directory notmuch can be configured with `mail_root` and `path` as separate keys, which puts the Xapian index outside the Maildir. Under that layout notmuch_database_get_path() returns the INDEX directory, and the worker treated it as the mail root at four sites. The consequences are not symmetric. Message paths resolved to `../..` escapes that match no account prefix, which is a display defect. But moveMessages() composes its destination from the same root, so Delete would have written into the Xapian tree: outside the Maildir, invisible to mbsync, and gone from every other client. That is the stranded-mail failure of item 103 with a new cause. notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT) is correct under both layouts, so no conditional is needed. Verified against the live database: with only `path` set it returns the same string as get_path(), making this a no-op for the current configuration. The fixture gains an opt-in splitIndex(). That is load-bearing rather than convenience: in the ordinary layout the index lives inside the mail root and both accessors return the same string, so a test written against it passes whichever one the code uses. All three new tests fail against the old accessor, confirmed by mutation. Also records the finding as backlog item 124, and corrects item 121's timings, which had been copied from item 74 rather than measured. A cold run seven minutes after boot, with the index verifiably unread, gives 2008 ms to the first rows and 38618 ms to a complete list, against the 642 ms and 5714 ms recorded there. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM --- tests/notmuchfixture.h | 33 ++++++++++- tests/test_notmuchworker.cpp | 133 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 6 deletions(-) (limited to 'tests') diff --git a/tests/notmuchfixture.h b/tests/notmuchfixture.h index bb25526..d1bdf62 100644 --- a/tests/notmuchfixture.h +++ b/tests/notmuchfixture.h @@ -44,6 +44,23 @@ public: QString configPath() const { return m_dir.filePath(QStringLiteral("config")); } QString maildirPath() const { return m_dir.filePath(QStringLiteral("mail")); } + /// Where the Xapian index lives. Equal to maildirPath()/.notmuch in the + /// ordinary layout; a directory of its own once splitIndex() is called. + QString indexPath() const + { + return m_splitIndex ? m_dir.filePath(QStringLiteral("index")) + : maildirPath() + QStringLiteral("/.notmuch"); + } + + /// Puts the index OUTSIDE the mail root, as notmuch's `mail_root`/`path` + /// split does (item 124). + /// + /// This is opt-in because it is the only layout that can tell + /// `notmuch_database_get_path()` apart from the mail root: in the ordinary + /// layout the two return the same string, so a test written against it + /// passes whichever accessor the code uses. Call before index(). + void splitIndex() { m_splitIndex = true; } + /// Writes one message into /cur (or new/ when unread). /// /// Returns false if the file could not be written. Call index() afterwards. @@ -103,9 +120,18 @@ public: return false; } QTextStream out(&config); - out << "[database]\n" - << "path=" << maildirPath() << "\n" - << "[new]\n" + out << "[database]\n"; + if (m_splitIndex) { + // Two keys: the mail stays put and only the index moves. notmuch + // reads `path` as the database directory ITSELF here, not as a + // parent to create `.notmuch` in. + QDir().mkpath(indexPath()); + out << "mail_root=" << maildirPath() << "\n" + << "path=" << indexPath() << "\n"; + } else { + out << "path=" << maildirPath() << "\n"; + } + out << "[new]\n" << "tags=unread;inbox;\n"; out.flush(); config.close(); @@ -132,4 +158,5 @@ public: private: QTemporaryDir m_dir; QString m_error; + bool m_splitIndex = false; }; diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index b5da31a..998696f 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -92,6 +92,10 @@ private slots: void moveMessagesReportsOnlyWhatMoved(); void moveMessagesGivesTheFileAFreshMaildirName(); void moveMessagesKeepsTheMaildirFlags(); + + void aSplitIndexStillResolvesTheMailRoot(); + void aSplitIndexMovesIntoTheMaildirNotTheIndex(); + void aSplitIndexListsTheMaildirsFolders(); void twoMessagesMovedTogetherGetDistinctNames(); private: @@ -101,7 +105,8 @@ private: bool addMovableMessage(const QString &folder, const QString &messageId); /// The single file backing `messageId`, or an empty string when the /// database does not know the id. - QString fileOf(const QString &messageId); + QString fileOf(const QString &messageId, + const QString &configPath = QString()); /// Tags of one message, read back through a fresh worker query. QStringList tagsOf(const QString &messageId); @@ -210,9 +215,10 @@ bool TestNotmuchWorker::addMovableMessage(const QString &folder, return m_fixture.index(); } -QString TestNotmuchWorker::fileOf(const QString &messageId) +QString TestNotmuchWorker::fileOf(const QString &messageId, + const QString &configPath) { - NotmuchWorker worker(m_fixture.configPath()); + NotmuchWorker worker(configPath.isEmpty() ? m_fixture.configPath() : configPath); QSignalSpy loaded(&worker, &NotmuchWorker::threadLoaded); worker.loadThread(QStringLiteral("{id:%1}").arg(messageId), QString(), 1); if (loaded.isEmpty()) @@ -1377,5 +1383,126 @@ void TestNotmuchWorker::moveMessagesReportsOnlyWhatMoved() QCOMPARE(inTrash.size(), 1); } + +// Item 124. notmuch can put the Xapian index outside the mail root +// (`mail_root` + `path`), which is how the index moves to faster storage while +// the mail stays put. Under that layout `notmuch_database_get_path()` returns +// the INDEX directory, so any code treating it as the mail root composes paths +// into the wrong tree entirely. +// +// These three need `splitIndex()`, and that is the whole point: in the +// ordinary layout the index lives inside the mail root and both accessors +// return the same string, so a test written against it passes whichever one +// the code uses and a mutation stays green. + +void TestNotmuchWorker::aSplitIndexStillResolvesTheMailRoot() +{ + NotmuchFixture fixture; + QVERIFY(fixture.isValid()); + fixture.splitIndex(); + QVERIFY(fixture.addMessage(QStringLiteral("work/INBOX"), + QStringLiteral("split1@example.org"), + QStringLiteral("Something"), + QStringLiteral("Alice "), + QStringLiteral("Mon, 1 Jun 2026 10:00:00 +0000"), + QStringLiteral("body"), false)); + QVERIFY2(fixture.index(), qPrintable(fixture.error())); + + // The fixture really did split them, or the rest proves nothing. + QVERIFY2(!fixture.indexPath().startsWith(fixture.maildirPath()), + "the fixture did not put the index outside the mail root"); + QVERIFY(QDir(fixture.indexPath() + QStringLiteral("/xapian")).exists()); + + NotmuchWorker worker(fixture.configPath()); + QSignalSpy ready(&worker, &NotmuchWorker::threadsReady); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.runQuery(QStringLiteral("*"), 1, NotmuchWorker::NewestFirst, false); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + QCOMPARE(ready.size(), 1); + + const auto threads = ready.first().at(0).value>(); + QCOMPARE(threads.size(), 1); + + // The path is stored relative to the MAIL ROOT. Resolved against the index + // directory it comes back as a "../.." escape, which matches no account + // prefix and leaves every row belonging to no account at all. + const QString path = threads.first().firstMessagePath; + QVERIFY2(!path.startsWith(QStringLiteral("..")), + qPrintable(QStringLiteral("path escaped the mail root: %1").arg(path))); + QVERIFY2(path.startsWith(QStringLiteral("work/INBOX/")), + qPrintable(QStringLiteral("expected a work/INBOX path, got: %1").arg(path))); +} + +void TestNotmuchWorker::aSplitIndexMovesIntoTheMaildirNotTheIndex() +{ + NotmuchFixture fixture; + QVERIFY(fixture.isValid()); + fixture.splitIndex(); + QVERIFY(fixture.addMessage(QStringLiteral("work/INBOX"), + QStringLiteral("split2@example.org"), + QStringLiteral("Doomed"), + QStringLiteral("Alice "), + QStringLiteral("Mon, 1 Jun 2026 10:00:00 +0000"), + QStringLiteral("body"), false)); + QVERIFY2(fixture.index(), qPrintable(fixture.error())); + + NotmuchWorker worker(fixture.configPath()); + QSignalSpy moved(&worker, &NotmuchWorker::messagesMoved); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.moveMessages({ QStringLiteral("split2@example.org") }, + QStringLiteral("work/Trash")); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + QCOMPARE(moved.size(), 1); + + // The file must land in the MAILDIR's trash. Composed against the index + // directory it lands inside the Xapian tree instead: outside the Maildir, + // invisible to mbsync, and gone from every other client. That is item + // 103's stranded mail with a new cause, which is why this assertion names + // the index directory explicitly rather than only checking the good path. + const QString expected = + fixture.maildirPath() + QStringLiteral("/work/Trash/cur"); + QVERIFY2(!QDir(fixture.indexPath() + QStringLiteral("/work")).exists(), + "the move created a folder inside the INDEX directory"); + + const QString after = + fileOf(QStringLiteral("split2@example.org"), fixture.configPath()); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + QCOMPARE(QFileInfo(after).absolutePath(), expected); + QVERIFY2(QFile::exists(after), qPrintable(after)); +} + +void TestNotmuchWorker::aSplitIndexListsTheMaildirsFolders() +{ + NotmuchFixture fixture; + QVERIFY(fixture.isValid()); + fixture.splitIndex(); + QVERIFY(fixture.addMessage(QStringLiteral("work/INBOX"), + QStringLiteral("split3@example.org"), + QStringLiteral("Something"), + QStringLiteral("Alice "), + QStringLiteral("Mon, 1 Jun 2026 10:00:00 +0000"), + QStringLiteral("body"), false)); + QVERIFY2(fixture.index(), qPrintable(fixture.error())); + + NotmuchWorker worker(fixture.configPath()); + QSignalSpy folders(&worker, &NotmuchWorker::foldersReady); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.requestFolders(); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + QCOMPARE(folders.size(), 1); + + // Scanned from the mail root. Scanned from the index directory the list is + // empty, or worse, names Xapian's own subdirectories as mail folders. + const QStringList found = folders.first().at(0).toStringList(); + QVERIFY2(found.contains(QStringLiteral("work/INBOX")), + qPrintable(QStringLiteral("expected work/INBOX, got: %1") + .arg(found.join(QStringLiteral(", "))))); + QVERIFY2(!found.contains(QStringLiteral("xapian")), + "the index's own directory was listed as a mail folder"); +} + QTEST_MAIN(TestNotmuchWorker) #include "test_notmuchworker.moc" -- cgit v1.2.3