aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-25 11:44:05 +0200
committerDanilo M. <danix@danix.xyz>2026-08-25 11:44:05 +0200
commit82588d51c4ad0c9e46bbd41658785eb0ba77f48b (patch)
treea4c92bf1a050db260f01897a945b94de22bd6f58
parentcd56144ba0304247812cf61c8b6e435779df29d0 (diff)
downloadqtmaildir-82588d51c4ad0c9e46bbd41658785eb0ba77f48b.tar.gz
qtmaildir-82588d51c4ad0c9e46bbd41658785eb0ba77f48b.zip
fix(compose): resolve a path a sync renamed, at all three read sites
Item 163. mbsync renames an uploaded file to add its `,U=<uid>` infix, and the model's `MessageRef::filePath` was captured when the query ran, so a row loaded before that sync names a file that no longer exists. MimeParser then honestly reports a message it cannot open. MaildirName::resolveRenamed() answers the filesystem question: returns the path unchanged when it still exists, otherwise looks in that one directory for the file whose unique stem matches. mbsync preserves the stem (`<stem>:2,D` becomes `<stem>,U=5:2,D`), which is what makes this safe to do by filename at all. It never recurses, never crosses a folder boundary, and refuses an ambiguous match rather than guessing, since opening or moving the wrong message is worse than reporting none. It lives in MaildirName because that namespace already owns the `,U=` infix and is a pure-value unit testable without a widget. A file that changed FOLDERS is a different question that only the message id can answer, and NotmuchWorker::moveMessages() re-resolves that way already. Three call sites, all of which held a stale path: - The message pane, which reported "(unreadable message)" over a file that was on disk and readable. Cosmetic and self-repairing. - Reply and Forward, refused outright, so the user could not answer a message that was sitting there. - The draft reopen, and this is the half that costs data. The refusal happens BEFORE any composer exists, so the user composes again into a fresh window whose autosave has no previous path to unlink. The old revision survives, each save mints a new Message-ID, and both files reach the server. The unlink machinery was correct throughout and never ran. forDraft() seeds draftPath from the RESOLVED path, never the caller's: seeding the stale one would let the reopen succeed and the unlink still miss, which is the same fork arriving one step later. Covered by five unit tests on the resolver, including the two that keep it honest (a genuinely missing file yields nothing, and a neighbouring message is never matched), and by an integration test that renames the draft the way mbsync does and asserts the file COUNT, which is the shape the fork actually takes. Both mutation-checked; the integration test fails with the reported symptom when the resolution is removed. The stable-Message-ID question is deliberately untouched: it is what turns a stale path into two server-side messages rather than one replaced file, and it wants its own item. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UUQS6n3cmsFrsjCNmwNtf8
-rw-r--r--src/composecontext.cpp23
-rw-r--r--src/maildirname.cpp61
-rw-r--r--src/maildirname.h24
-rw-r--r--src/mainwindow.cpp24
-rw-r--r--tests/test_maildirname.cpp96
-rw-r--r--tests/test_mainwindow.cpp78
6 files changed, 300 insertions, 6 deletions
diff --git a/src/composecontext.cpp b/src/composecontext.cpp
index d0406fc..7233330 100644
--- a/src/composecontext.cpp
+++ b/src/composecontext.cpp
@@ -24,6 +24,7 @@
#include "composecontext.h"
#include "config.h"
+#include "maildirname.h"
#include "mimeparser.h"
#include <QDir>
@@ -491,16 +492,32 @@ ComposeContext ComposeContextBuilder::forDraft(const Config &config,
{
ComposeContext context;
+ // Item 163. The caller's path comes from the model, captured when the
+ // query ran, and mbsync renames an uploaded draft to add its `,U=<uid>`
+ // infix. Resolving first is what stops a rename from refusing the reopen:
+ // the refusal happens BEFORE any composer exists, so the user composes
+ // again into a FRESH window whose autosave has no previous path to unlink,
+ // and the draft is silently forked into two files with two Message-IDs,
+ // both of which reach the server.
+ //
+ // Returns the path unchanged when nothing was renamed, and empty when the
+ // file is genuinely gone, which still fails below exactly as before.
+ const QString resolved = MaildirName::resolveRenamed(path);
+
MimeParser parser;
- const ParsedMessage draft = parser.parse(path);
+ const ParsedMessage draft = parser.parse(resolved);
if (!draft.ok)
return context; // Kind::New and empty: the caller reports the failure.
context.kind = ComposeContext::Kind::Draft;
- context.originalPath = path;
+ context.originalPath = resolved;
// The file this composer OWNS. Without it the first autosave writes a
// second draft and leaves this one behind, so one message becomes two.
- context.draftPath = path;
+ //
+ // The RESOLVED path, never the caller's: seeding the stale one would let
+ // the reopen succeed and the unlink still miss, which is the same fork
+ // arriving one step later.
+ context.draftPath = resolved;
const auto addresses = [](const QString &header) {
QStringList out;
diff --git a/src/maildirname.cpp b/src/maildirname.cpp
index 6263aec..9f827fc 100644
--- a/src/maildirname.cpp
+++ b/src/maildirname.cpp
@@ -20,6 +20,8 @@
#include <QCoreApplication>
#include <QDateTime>
+#include <QDir>
+#include <QFileInfo>
#include <QHostInfo>
namespace MaildirName {
@@ -77,4 +79,63 @@ QString fresh(const QString &oldName)
.arg(info);
}
+QString resolveRenamed(const QString &path)
+{
+ if (path.isEmpty())
+ return QString();
+
+ // The ordinary case, and the overwhelmingly common one: nothing was
+ // renamed. One stat, then out.
+ if (QFileInfo::exists(path))
+ return path;
+
+ const QFileInfo info(path);
+ const QString name = info.fileName();
+
+ // The unique part mbsync preserves. `<stem>:2,D` becomes
+ // `<stem>,U=5:2,D`, so the stem ends at whichever of `,` or `:` comes
+ // first. A name carrying neither is all stem.
+ int cut = name.size();
+ for (const QChar separator : { QLatin1Char(','), QLatin1Char(':') }) {
+ const int at = name.indexOf(separator);
+ if (at >= 0 && at < cut)
+ cut = at;
+ }
+ const QString stem = name.left(cut);
+ if (stem.isEmpty())
+ return QString();
+
+ // One directory, never a recursive walk: a rename keeps the file where it
+ // was, and a file that changed FOLDERS is a different question that only
+ // the message id can answer (see NotmuchWorker::moveMessages(), item 162).
+ const QDir dir(info.absolutePath());
+ if (!dir.exists())
+ return QString();
+
+ QString found;
+ const QFileInfoList entries =
+ dir.entryInfoList(QDir::Files | QDir::NoDotAndDotDot);
+ for (const QFileInfo &entry : entries) {
+ const QString candidate = entry.fileName();
+ // Anchored on the stem AND on what follows it, so `...Q2` cannot match
+ // `...Q23`: the next character must begin the infix or the flags.
+ if (!candidate.startsWith(stem))
+ continue;
+ const QString rest = candidate.mid(stem.size());
+ if (!rest.isEmpty() && !rest.startsWith(QLatin1Char(','))
+ && !rest.startsWith(QLatin1Char(':'))) {
+ continue;
+ }
+
+ // Two files sharing a stem cannot happen in a correct Maildir. Refuse
+ // rather than guess: the caller reports "gone", which is honest, where
+ // a guess could open, move or delete the wrong message.
+ if (!found.isEmpty())
+ return QString();
+ found = entry.absoluteFilePath();
+ }
+
+ return found;
+}
+
} // namespace MaildirName
diff --git a/src/maildirname.h b/src/maildirname.h
index f24bc71..255517d 100644
--- a/src/maildirname.h
+++ b/src/maildirname.h
@@ -38,4 +38,28 @@ namespace MaildirName {
/// what a newly composed draft is.
QString fresh(const QString &oldName);
+/// The file \p path names, or the renamed file that replaced it.
+///
+/// Item 163. mbsync renames an uploaded file to add its `,U=<uid>` infix, and
+/// anything holding the previous name (the model's `MessageRef::filePath`, a
+/// draft's `ComposeContext::draftPath`) then points at a path that no longer
+/// exists. Returns \p path unchanged when it is still there, so the ordinary
+/// case costs one stat and nothing else.
+///
+/// Matched on the UNIQUE STEM, the part before the first `,` or `:`, which
+/// mbsync preserves: `<stem>:2,D` becomes `<stem>,U=5:2,D`. That is what makes
+/// this safe to do by filename at all. The search is confined to the file's
+/// own directory and never recurses, and an ambiguous match (more than one
+/// candidate, which a correct Maildir cannot produce) yields nothing rather
+/// than guessing.
+///
+/// Empty when there is no such file, which every caller must treat as the
+/// genuine "it is gone" it is: recovering silently from a real deletion would
+/// turn a reportable defect into a wrong answer.
+///
+/// This resolves a RENAME, not a MOVE. A file that changed folders is a
+/// different question and belongs to whoever knows the message id;
+/// `NotmuchWorker::moveMessages()` re-resolves that way for item 162.
+QString resolveRenamed(const QString &path);
+
} // namespace MaildirName
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index af3b817..5845922 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -18,6 +18,8 @@
#include "mainwindow.h"
+#include "maildirname.h"
+
#include <QAction>
#include <QApplication>
#include <QCloseEvent>
@@ -1043,8 +1045,13 @@ void MainWindow::openComposerFor(const MessageRef &ref,
return;
}
+ // Item 163, the same stale path the pane and the draft reopen hit. Here it
+ // refuses a Reply or a Forward outright, so the user cannot answer a
+ // message that is sitting on disk and readable.
+ const QString originalPath = MaildirName::resolveRenamed(ref.filePath);
+
MimeParser parser;
- const ParsedMessage original = parser.parse(ref.filePath);
+ const ParsedMessage original = parser.parse(originalPath);
if (!original.ok) {
showTransientStatus(tr("That message could not be read"));
return;
@@ -1052,7 +1059,7 @@ void MainWindow::openComposerFor(const MessageRef &ref,
ComposeContext context;
context.kind = kind;
- context.originalPath = ref.filePath;
+ context.originalPath = originalPath;
const bool replyAll = kind == ComposeContext::Kind::ReplyAll;
const bool forwarding = kind == ComposeContext::Kind::Forward;
@@ -3849,11 +3856,22 @@ void MainWindow::renderMessages(const QVector<MessageRef> &messages)
const MessageRef &ref = messages.at(i);
ThreadRenderItem item;
- item.message = parser.parse(ref.filePath);
+ // Item 163. The model's path was captured when the query ran, and
+ // mbsync renames an uploaded file to add its `,U=<uid>` infix, so a row
+ // loaded before that sync names a file that no longer exists. The pane
+ // then reported the message unreadable while nothing was wrong with it.
+ // Unchanged when nothing was renamed; empty when the file is genuinely
+ // gone, which still reports below.
+ const QString path = MaildirName::resolveRenamed(ref.filePath);
+ item.message = parser.parse(path);
if (!item.message.ok) {
// One unreadable message must not lose the rest of the thread, so
// it becomes an inline note rather than replacing the whole pane.
+ //
+ // Named by the path the model HOLDS, not by the resolved one: when
+ // resolution failed there is no resolved path, and the stale name
+ // is what the user can act on.
item.message = {};
item.message.ok = true;
item.message.from = tr("(unreadable message)");
diff --git a/tests/test_maildirname.cpp b/tests/test_maildirname.cpp
index dcc8fab..a8e4c01 100644
--- a/tests/test_maildirname.cpp
+++ b/tests/test_maildirname.cpp
@@ -18,7 +18,10 @@
#include "maildirname.h"
+#include <QDir>
+#include <QFile>
#include <QSet>
+#include <QTemporaryDir>
#include <QTest>
class TestMaildirName : public QObject
@@ -31,6 +34,11 @@ private slots:
void anEmptyFlagSuffixIsPreserved();
void aNameWithNoSuffixGetsNone();
void theUidInfixIsNotCarriedAcross();
+ void resolveRenamedReturnsAPathThatStillExists();
+ void resolveRenamedFindsTheFileMbsyncRenamed();
+ void resolveRenamedIsEmptyWhenTheFileIsReallyGone();
+ void resolveRenamedDoesNotMatchADifferentMessage();
+ void resolveRenamedRefusesAnAmbiguousMatch();
};
// Two messages written in the same second must not collide, which a
@@ -89,5 +97,93 @@ void TestMaildirName::theUidInfixIsNotCarriedAcross()
.arg(name)));
}
+namespace {
+
+/// One empty file, so a test can assert on which PATH is chosen rather than on
+/// content. resolveRenamed() answers a filesystem question and never opens the
+/// file.
+bool touch(const QString &path)
+{
+ QFile file(path);
+ if (!file.open(QIODevice::WriteOnly))
+ return false;
+ file.close();
+ return true;
+}
+
+} // namespace
+
+void TestMaildirName::resolveRenamedReturnsAPathThatStillExists()
+{
+ // The ordinary case, and the one that must stay cheap: nothing was
+ // renamed, so the answer is the question.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString path = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(touch(path));
+
+ QCOMPARE(MaildirName::resolveRenamed(path), path);
+}
+
+void TestMaildirName::resolveRenamedFindsTheFileMbsyncRenamed()
+{
+ // Item 163. mbsync uploads the file and inserts its `,U=<uid>` infix
+ // before the flag suffix, leaving the unique stem alone.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString stale = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ const QString renamed =
+ dir.filePath(QStringLiteral("1787647354.M369Q2.host,U=5:2,D"));
+ QVERIFY(touch(renamed));
+ QVERIFY2(!QFile::exists(stale), "the stale path must not exist");
+
+ QCOMPARE(MaildirName::resolveRenamed(stale), renamed);
+}
+
+void TestMaildirName::resolveRenamedIsEmptyWhenTheFileIsReallyGone()
+{
+ // The bounded half. A deleted file must NOT be recovered from, or a
+ // reportable defect becomes a wrong answer.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString gone = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(!QFile::exists(gone));
+
+ QVERIFY(MaildirName::resolveRenamed(gone).isEmpty());
+}
+
+void TestMaildirName::resolveRenamedDoesNotMatchADifferentMessage()
+{
+ // A neighbouring file in the same folder is not this message. Matching on
+ // anything looser than the whole stem would return it, and the caller
+ // would then open, display or MOVE the wrong mail.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString stale = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(touch(dir.filePath(QStringLiteral("1787647354.M369Q3.host,U=5:2,D"))));
+ QVERIFY(touch(dir.filePath(QStringLiteral("9999999999.M111Q1.host,U=6:2,D"))));
+
+ QVERIFY(MaildirName::resolveRenamed(stale).isEmpty());
+}
+
+void TestMaildirName::resolveRenamedRefusesAnAmbiguousMatch()
+{
+ // Two files sharing one stem cannot happen in a correct Maildir, so this
+ // is a "the world is not what I assumed" case. Guessing between them could
+ // move or delete the wrong file, and the caller reports honestly instead.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString stale = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(touch(dir.filePath(QStringLiteral("1787647354.M369Q2.host,U=5:2,D"))));
+ QVERIFY(touch(dir.filePath(QStringLiteral("1787647354.M369Q2.host,U=6:2,S"))));
+
+ QVERIFY(MaildirName::resolveRenamed(stale).isEmpty());
+}
+
QTEST_MAIN(TestMaildirName)
#include "test_maildirname.moc"
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index f935cac..21c3661 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -490,6 +490,7 @@ private slots:
void doubleClickingADraftOpensTheComposer();
void aResumedDraftReplacesItsFileRatherThanAddingOne();
void aResumedDraftKeepsItsBlindRecipients();
+ void aDraftRenamedByASyncStillReopensAndReplacesItsFile();
void theComposerSplitsItsToolbarByScope();
void ccAndBccHideBehindADisclosure();
void ccAndBccAreRevealedWhenTheyCarryAValue();
@@ -12861,6 +12862,83 @@ void TestMainWindow::aResumedDraftReplacesItsFileRatherThanAddingOne()
"now exists twice");
}
+void TestMainWindow::aDraftRenamedByASyncStillReopensAndReplacesItsFile()
+{
+ // Item 163, the composer site, and the one that costs data rather than
+ // display. mbsync uploads a draft and renames it to add its `,U=<uid>`
+ // infix; the model's path was captured when the query ran, so the reopen
+ // is handed a name that no longer exists.
+ //
+ // The refusal happens BEFORE any composer exists, so the user composes
+ // again into a FRESH window whose autosave has no previous path to unlink.
+ // The old revision survives, each save mints a new Message-ID, and both
+ // files reach the server. Asserted as the file COUNT, which is the shape
+ // the fork actually takes.
+ ComposeFixture fixture;
+ QVERIFY(fixture.build());
+
+ OutgoingMessage message;
+ message.accountKey = QStringLiteral("acct");
+ message.to = { QStringLiteral("someone@example.org") };
+ message.subject = QStringLiteral("Written before a sync");
+ message.markdownBody = QStringLiteral("The first half.");
+
+ const QString folder = fixture.mailRoot() + QStringLiteral("/acct/Drafts");
+ const QString path = writeDraftFile(folder, message,
+ fixture.config().account(
+ QStringLiteral("acct")));
+ QVERIFY(!path.isEmpty());
+
+ // mbsync's rename: same directory, same unique stem, `,U=<uid>` inserted
+ // before the flag suffix. Nothing reindexes, so the caller below still
+ // holds the pre-rename name, which is the whole precondition.
+ const QFileInfo before(path);
+ const QString base = before.fileName();
+ const int suffix = base.indexOf(QStringLiteral(":2,"));
+ QVERIFY2(suffix > 0, "the draft fixture has no maildir flag suffix");
+ const QString renamed = before.absolutePath() + QLatin1Char('/')
+ + base.left(suffix) + QStringLiteral(",U=7")
+ + base.mid(suffix);
+ QVERIFY2(QFile::rename(path, renamed), "could not stage the sync rename");
+
+ // The guard that proves this test can fail: without it, a fixture that
+ // quietly left the original in place would pass against the bug.
+ QVERIFY2(!QFile::exists(path), "the stale path should no longer exist");
+
+ const auto draftCount = [&folder]() {
+ return QDir(folder + QStringLiteral("/cur"))
+ .entryList(QDir::Files).size();
+ };
+ QCOMPARE(draftCount(), 1);
+
+ // The STALE path, exactly as openComposerFor() passes MessageRef::filePath.
+ const ComposeContext context =
+ ComposeContextBuilder::forDraft(fixture.config(), path);
+ QVERIFY2(context.kind == ComposeContext::Kind::Draft,
+ "the reopen was refused, so the user would compose a second draft");
+ // Resolved, not the caller's: seeding the stale path would let the reopen
+ // succeed and the unlink still miss, forking the draft one step later.
+ QCOMPARE(context.draftPath, renamed);
+
+ ComposeWindow window(context, fixture.config(), fixture.mailRoot());
+ auto *body = window.findChild<QPlainTextEdit *>(QStringLiteral("body"));
+ QVERIFY(body);
+ body->setPlainText(QStringLiteral("The second half."));
+
+ auto *timer = window.findChild<QTimer *>(QStringLiteral("autosave"));
+ QVERIFY2(timer, "the composer has no autosave timer");
+ QVERIFY2(timer->isActive(), "editing the body did not arm the autosave");
+ timer->setInterval(0);
+ QTRY_VERIFY_WITH_TIMEOUT(!timer->isActive(), 5000);
+
+ // Still ONE draft: the autosave replaced the renamed file rather than
+ // leaving it behind beside a new one.
+ QCOMPARE(draftCount(), 1);
+ QVERIFY2(!QFile::exists(renamed),
+ "the renamed draft survived the autosave, so the draft was forked "
+ "into two files and both would reach the server");
+}
+
void TestMainWindow::aResumedDraftKeepsItsBlindRecipients()
{
// MessageBuilder writes Bcc into the draft file deliberately, and says