Surface spoiler removal reload failures and allow disabled-state retry

cockatrice/CMakeLists.txt

@@ -15,6 +15,7 @@ set(cockatrice_SOURCES
     src/client/network/update/client/client_update_checker.cpp
     src/client/network/update/client/release_channel.cpp
     src/client/network/update/card_spoiler/spoiler_background_updater.cpp
+    src/client/network/update/card_spoiler/spoiler_file.cpp
     src/client/crash/crash_reporter.cpp
     src/client/latency_graph_widget.cpp
     src/client/latency_status_widget.cpp

cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp

@@ -2,22 +2,19 @@
 
 #include "../../../../main.h"
 #include "../../../settings/cache_settings.h"
+#include "spoiler_file.h"
 
 #include <QCoreApplication>
-#include <QCryptographicHash>
 #include <QDebug>
-#include <QDir>
 #include <QFile>
 #include <QFileInfo>
 #include <QFutureWatcher>
 #include <QNetworkAccessManager>
 #include <QNetworkReply>
 #include <QNetworkRequest>
-#include <QSaveFile>
 #include <QSystemTrayIcon>
 #include <QTimer>
 #include <QUrl>
-#include <QXmlStreamReader>
 #include <QtConcurrentRun>
 #include <exception>
 #include <libcockatrice/card/database/card_database.h>
@@ -70,7 +67,8 @@ void SpoilerBackgroundUpdater::begin()
         return;
     }
     if (cancelled || operationId != newestOperation ||
-        (!reloadOnly && (!enabledAtStart || !SettingsCache::instance().downloads().getDownloadSpoilersStatus())) ||
+        (reloadOnly ? SettingsCache::instance().downloads().getDownloadSpoilersStatus()
+                    : (!enabledAtStart || !SettingsCache::instance().downloads().getDownloadSpoilersStatus())) ||
         targetPath != SettingsCache::instance().paths().getSpoilerCardDatabasePath()) {
         finish();
         return;
@@ -97,6 +95,11 @@ void SpoilerBackgroundUpdater::begin()
     if (!isCurrent()) {
         finish();
     } else if (reloadOnly) {
+        if (QFileInfo::exists(targetPath) && !removeSpoilerFile(targetPath)) {
+            finish(tr("The spoiler database could not be removed. Check its file permissions and use Reload Card "
+                      "Database to retry."));
+            return;
+        }
         reloadDatabase(false);
     } else {
         downloadFromURL(SPOILERS_STATUS_URL, false);
@@ -106,13 +109,13 @@ void SpoilerBackgroundUpdater::begin()
 bool SpoilerBackgroundUpdater::isCurrent() const
 {
     return !completed && !cancelled && currentUpdater == this && operationId == newestOperation &&
-           (reloadOnly || SettingsCache::instance().downloads().getDownloadSpoilersStatus()) &&
+           (reloadOnly != SettingsCache::instance().downloads().getDownloadSpoilersStatus()) &&
            targetPath == SettingsCache::instance().paths().getSpoilerCardDatabasePath();
 }
 
 void SpoilerBackgroundUpdater::cancelIfSettingsChanged()
 {
-    if ((!reloadOnly && !SettingsCache::instance().downloads().getDownloadSpoilersStatus()) ||
+    if (reloadOnly == SettingsCache::instance().downloads().getDownloadSpoilersStatus() ||
         targetPath != SettingsCache::instance().paths().getSpoilerCardDatabasePath()) {
         cancel();
     }
@@ -150,8 +153,7 @@ void SpoilerBackgroundUpdater::downloadFromURL(const QUrl &url, bool saveResults
         connect(activeReply, &QNetworkReply::finished, this,
                 &SpoilerBackgroundUpdater::actDownloadFinishedSpoilersFile);
     } else {
-        connect(activeReply, &QNetworkReply::finished, this,
+        connect(activeReply, &QNetworkReply::finished, this, &SpoilerBackgroundUpdater::actCheckIfSpoilerSeasonEnabled);
-                &SpoilerBackgroundUpdater::actCheckIfSpoilerSeasonEnabled);
     }
 }
 
@@ -204,84 +206,16 @@ void SpoilerBackgroundUpdater::actDownloadFinishedSpoilersFile()
     }
     spoilerData = reply->readAll();
     QString error;
-    const SaveResult result = saveDownloadedFile(spoilerData, error);
+    const auto result = SpoilerFile::save(targetPath, spoilerData, [this] { return isCurrent(); }, error);
-    if (result == SaveResult::Failed) {
+    if (result == SpoilerFile::SaveResult::Failed) {
         finish(isCurrent() ? error : QString());
-    } else if (result == SaveResult::Written || forceReload) {
+    } else if (result == SpoilerFile::SaveResult::Written || forceReload) {
-        reloadDatabase(result == SaveResult::Written);
+        reloadDatabase(result == SpoilerFile::SaveResult::Written);
     } else {
         finish();
     }
 }
 
-QString SpoilerBackgroundUpdater::validateDownloadedFile(const QByteArray &data) const
-{
-    QXmlStreamReader xml(data);
-    if (!xml.readNextStartElement() || xml.name() != QStringLiteral("cockatrice_carddatabase")) {
-        return tr("The download is not a Cockatrice card database. The previous spoiler file was preserved.");
-    }
-    bool versionOk = false;
-    const int version = xml.attributes().value(QStringLiteral("version")).toInt(&versionOk);
-    if (!versionOk || (version != 3 && version != 4)) {
-        return tr("The spoiler database uses an unsupported format. The previous spoiler file was preserved.");
-    }
-    bool hasCards = false;
-    int depth = 1;
-    while (!xml.atEnd()) {
-        const auto token = xml.readNext();
-        if (token == QXmlStreamReader::StartElement) {
-            ++depth;
-            if (depth == 2 && xml.name() == QStringLiteral("cards")) {
-                hasCards = true;
-            }
-        } else if (token == QXmlStreamReader::EndElement) {
-            --depth;
-        }
-    }
-    if (xml.hasError() || !hasCards) {
-        return tr("The spoiler database is incomplete or invalid. The previous spoiler file was preserved.");
-    }
-    return {};
-}
-
-SpoilerBackgroundUpdater::SaveResult SpoilerBackgroundUpdater::saveDownloadedFile(const QByteArray &data,
-                                                                                 QString &error)
-{
-    error = validateDownloadedFile(data);
-    if (!error.isEmpty()) {
-        return SaveResult::Failed;
-    }
-    if (!isCurrent()) {
-        error = tr("The spoiler update was cancelled because its settings changed.");
-        return SaveResult::Failed;
-    }
-    if (getHash(targetPath) == getHash(data)) {
-        return SaveResult::Unchanged;
-    }
-    const QDir directory = QFileInfo(targetPath).dir();
-    if (!directory.exists() && !directory.mkpath(QStringLiteral("."))) {
-        error = tr("Could not create the spoiler database directory: %1").arg(directory.absolutePath());
-        return SaveResult::Failed;
-    }
-    QSaveFile file(targetPath);
-    file.setDirectWriteFallback(false);
-    if (!file.open(QIODevice::WriteOnly) || file.write(data) != data.size() || !file.flush()) {
-        error = tr("Could not save the spoiler database to %1: %2. The previous file was preserved.")
-                    .arg(targetPath, file.errorString());
-        return SaveResult::Failed;
-    }
-    if (!isCurrent()) {
-        file.cancelWriting();
-        return SaveResult::Failed;
-    }
-    if (!file.commit()) {
-        error = tr("Could not replace the spoiler database at %1: %2. The previous file was preserved.")
-                    .arg(targetPath, file.errorString());
-        return SaveResult::Failed;
-    }
-    return SaveResult::Written;
-}
-
 void SpoilerBackgroundUpdater::reloadDatabase(bool updated)
 {
     reloading = true;
@@ -307,7 +241,10 @@ void SpoilerBackgroundUpdater::reloadDatabase(bool updated)
         if (!isCurrent()) {
             finish();
         } else if (!loaded) {
-            finish(tr("Could not reload card data after updating spoilers. Use Update Spoilers to retry the reload."));
+            finish(reloadOnly
+                       ? tr("Could not reload card data after disabling spoilers. Use Reload Card Database to retry.")
+                       : tr("Could not reload card data after updating spoilers. Use Update Spoilers to retry the "
+                            "reload."));
         } else {
             finish({}, updated);
         }
@@ -356,32 +293,13 @@ void SpoilerBackgroundUpdater::finish(const QString &error, bool updated)
     }
 }
 
-bool SpoilerBackgroundUpdater::deleteSpoilerFile()
+SpoilerBackgroundUpdater *SpoilerBackgroundUpdater::deleteSpoilerFile()
 {
-    const QString path = SettingsCache::instance().paths().getSpoilerCardDatabasePath();
+    // Deletion and the following reload share the operation lifetime. Callers can
-    if (QFileInfo::exists(path) && !removeSpoilerFile(path)) {
+    // observe either failure before the queued operation starts, and an explicit
-        return false;
+    // retry still reloads when the file has already been removed.
-    }
-    // Explicit disable must clear the in-memory view even when another actor has
-    // already removed the file. Queue behind an older reload through the same
-    // updater lifecycle, so its final snapshot cannot restore disabled spoilers.
     auto *updater = new SpoilerBackgroundUpdater(QCoreApplication::instance(), true);
     updater->reloadOnly = true;
     connect(updater, &SpoilerBackgroundUpdater::spoilerCheckerDone, updater, &QObject::deleteLater);
-    return true;
+    return updater;
-}
-
-QByteArray SpoilerBackgroundUpdater::getHash(const QString fileName)
-{
-    QFile file(fileName);
-    if (!file.open(QIODevice::ReadOnly)) {
-        return {};
-    }
-    QCryptographicHash hash(QCryptographicHash::Sha256);
-    return hash.addData(&file) ? hash.result() : QByteArray();
-}
-
-QByteArray SpoilerBackgroundUpdater::getHash(QByteArray data)
-{
-    return QCryptographicHash::hash(data, QCryptographicHash::Sha256);
 }

cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.h

@@ -29,9 +29,9 @@ public:
     {
         return "oracle";
     }
-    QByteArray getHash(const QString fileName);
+    // Queues removal and reload; the application owns the returned updater and
-    QByteArray getHash(QByteArray data);
+    // deletes it on completion. Callers can connect its failure/completion signals.
-    static bool deleteSpoilerFile();
+    static SpoilerBackgroundUpdater *deleteSpoilerFile();
 
 private slots:
     void begin();
@@ -40,13 +40,6 @@ private slots:
     void cancelIfSettingsChanged();
 
 private:
-    enum class SaveResult
-    {
-        Failed,
-        Unchanged,
-        Written
-    };
-
     const bool enabledAtStart;
     const bool forceReload;
     const QString targetPath;
@@ -66,8 +59,6 @@ private:
     void finish(const QString &error = {}, bool updated = false);
     void downloadFromURL(const QUrl &url, bool saveResults);
     void reloadDatabase(bool updated);
-    QString validateDownloadedFile(const QByteArray &data) const;
-    SaveResult saveDownloadedFile(const QByteArray &data, QString &error);
 
 signals:
     void spoilersUpdatedSuccessfully();

cockatrice/src/client/network/update/card_spoiler/spoiler_file.cpp

@@ -0,0 +1,109 @@
+#include "spoiler_file.h"
+
+#include <QCoreApplication>
+#include <QCryptographicHash>
+#include <QDir>
+#include <QFile>
+#include <QFileInfo>
+#include <QSaveFile>
+#include <QXmlStreamReader>
+
+namespace
+{
+QByteArray hashFile(const QString &path)
+{
+    QFile file(path);
+    if (!file.open(QIODevice::ReadOnly)) {
+        return {};
+    }
+    QCryptographicHash hash(QCryptographicHash::Sha256);
+    return hash.addData(&file) ? hash.result() : QByteArray();
+}
+} // namespace
+
+namespace SpoilerFile
+{
+QString validate(const QByteArray &data)
+{
+    QXmlStreamReader xml(data);
+    if (!xml.readNextStartElement() || xml.name() != QStringLiteral("cockatrice_carddatabase")) {
+        return QCoreApplication::translate(
+            "SpoilerBackgroundUpdater",
+            "The download is not a Cockatrice card database. The previous spoiler file was preserved.");
+    }
+    bool versionOk = false;
+    const int version = xml.attributes().value(QStringLiteral("version")).toInt(&versionOk);
+    if (!versionOk || (version != 3 && version != 4)) {
+        return QCoreApplication::translate(
+            "SpoilerBackgroundUpdater",
+            "The spoiler database uses an unsupported format. The previous spoiler file was preserved.");
+    }
+    bool hasCards = false;
+    int depth = 1;
+    while (!xml.atEnd()) {
+        const auto token = xml.readNext();
+        if (token == QXmlStreamReader::StartElement) {
+            ++depth;
+            if (depth == 2 && xml.name() == QStringLiteral("cards")) {
+                hasCards = true;
+            }
+        } else if (token == QXmlStreamReader::EndElement) {
+            --depth;
+        }
+    }
+    if (xml.hasError() || !hasCards) {
+        return QCoreApplication::translate(
+            "SpoilerBackgroundUpdater",
+            "The spoiler database is incomplete or invalid. The previous spoiler file was preserved.");
+    }
+    return {};
+}
+
+SaveResult
+save(const QString &targetPath, const QByteArray &data, const std::function<bool()> &isCurrent, QString &error)
+{
+    error = validate(data);
+    if (!error.isEmpty()) {
+        return SaveResult::Failed;
+    }
+    if (!isCurrent()) {
+        error = QCoreApplication::translate("SpoilerBackgroundUpdater",
+                                            "The spoiler update was cancelled because its settings changed.");
+        return SaveResult::Failed;
+    }
+    if (hashFile(targetPath) == QCryptographicHash::hash(data, QCryptographicHash::Sha256)) {
+        return SaveResult::Unchanged;
+    }
+    const QDir directory = QFileInfo(targetPath).dir();
+    if (!directory.exists() && !directory.mkpath(QStringLiteral("."))) {
+        error = QCoreApplication::translate("SpoilerBackgroundUpdater",
+                                            "Could not create the spoiler database directory: %1")
+                    .arg(directory.absolutePath());
+        return SaveResult::Failed;
+    }
+    QSaveFile file(targetPath);
+    file.setDirectWriteFallback(false);
+    if (!file.open(QIODevice::WriteOnly) || file.write(data) != data.size() || !file.flush()) {
+        error = QCoreApplication::translate(
+                    "SpoilerBackgroundUpdater",
+                    "Could not save the spoiler database to %1: %2. The previous file was preserved.")
+                    .arg(targetPath, file.errorString());
+        return SaveResult::Failed;
+    }
+    if (!isCurrent()) {
+        file.cancelWriting();
+        error = QCoreApplication::translate("SpoilerBackgroundUpdater",
+                                            "The spoiler update was cancelled because its settings changed.");
+        return SaveResult::Failed;
+    }
+    if (!file.commit()) {
+        error = QCoreApplication::translate(
+                    "SpoilerBackgroundUpdater",
+                    "Could not replace the spoiler database at %1: %2. The previous file was preserved.")
+                    .arg(targetPath, file.errorString());
+        return SaveResult::Failed;
+    }
+    return SaveResult::Written;
+}
+
+} // namespace SpoilerFile

cockatrice/src/client/network/update/card_spoiler/spoiler_file.h

@@ -0,0 +1,24 @@
+#ifndef COCKATRICE_SPOILER_FILE_H
+#define COCKATRICE_SPOILER_FILE_H
+
+#include <QByteArray>
+#include <QString>
+#include <functional>
+
+namespace SpoilerFile
+{
+enum class SaveResult
+{
+    Failed,
+    Unchanged,
+    Written
+};
+
+QString validate(const QByteArray &data);
+// Validate before opening the destination, compare complete files, and replace
+// atomically. Check operation ownership both before writing and before committing.
+SaveResult
+save(const QString &targetPath, const QByteArray &data, const std::function<bool()> &isCurrent, QString &error);
+} // namespace SpoilerFile
+
+#endif // COCKATRICE_SPOILER_FILE_H

cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp

@@ -62,7 +62,7 @@ DeckEditorSettingsPage::DeckEditorSettingsPage()
     connect(mpSpoilerPathButton, &QPushButton::clicked, this, &DeckEditorSettingsPage::spoilerPathButtonClicked);
 
     updateNowButton = new QPushButton;
-    updateNowButton->setFixedWidth(150);
+    updateNowButton->setMinimumWidth(150);
     connect(updateNowButton, &QPushButton::clicked, this, &DeckEditorSettingsPage::updateSpoilers);
 
     // Update the GUI depending on if the box is ticked or not
@@ -329,21 +329,28 @@ void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const
 
 void DeckEditorSettingsPage::updateSpoilers()
 {
-    // Disable the button so the user can only press it once at a time
+    if (spoilerUpdater) {
-    updateNowButton->setDisabled(true);
+        return;
-    updateNowButton->setText(tr("Updating..."));
+    }
+    // Application ownership lets either operation finish after settings closes.
+    // When disabled, the same button retries file removal and the local reload.
+    auto *sbu = SettingsCache::instance().downloads().getDownloadSpoilersStatus()
+                    ? new SpoilerBackgroundUpdater(QCoreApplication::instance(), true)
+                    : SpoilerBackgroundUpdater::deleteSpoilerFile();
+    observeSpoilerUpdater(sbu);
+}
 
-    // The explicit update also retries a previous reload of an unchanged file.
+void DeckEditorSettingsPage::observeSpoilerUpdater(SpoilerBackgroundUpdater *sbu)
-    // Application ownership lets it finish if the settings page is closed.
+{
-    auto *sbu = new SpoilerBackgroundUpdater(QCoreApplication::instance(), true);
     spoilerUpdater = sbu;
+    unlockSettings();
     connect(sbu, &SpoilerBackgroundUpdater::spoilerCheckerDone, this, [this, sbu] {
         if (spoilerUpdater == sbu) {
             spoilerUpdater.clear();
             unlockSettings();
         }
     });
-    connect(sbu, &SpoilerBackgroundUpdater::spoilerCheckerDone, sbu, &QObject::deleteLater);
+    connect(sbu, &SpoilerBackgroundUpdater::spoilerCheckerDone, sbu, &QObject::deleteLater, Qt::UniqueConnection);
     connect(sbu, &SpoilerBackgroundUpdater::spoilerUpdateFailed, this, [this, sbu](const QString &message) {
         if (spoilerUpdater != sbu) {
             return;
@@ -357,8 +364,11 @@ void DeckEditorSettingsPage::updateSpoilers()
 
 void DeckEditorSettingsPage::unlockSettings()
 {
-    updateNowButton->setEnabled(SettingsCache::instance().downloads().getDownloadSpoilersStatus());
+    updateNowButton->setEnabled(!spoilerUpdater);
-    updateNowButton->setText(tr("Update Spoilers"));
+    updateNowButton->setText(spoilerUpdater ? tr("Updating...")
+                             : SettingsCache::instance().downloads().getDownloadSpoilersStatus()
+                                 ? tr("Update Spoilers")
+                                 : tr("Reload Card Database"));
     lastUpdatedLabel.setText(tr("Last Change") + ": " + getLastUpdateTime());
 }
 
@@ -397,22 +407,13 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput)
     mpSpoilerSavePathLineEdit->setEnabled(anInput);
     mpSpoilerPathButton->setEnabled(anInput);
     lastUpdatedLabel.setEnabled(anInput);
-    updateNowButton->setEnabled(anInput && !spoilerUpdater);
     infoOnSpoilersLabel.setEnabled(anInput);
 
     if (!anInput && (sender() == &mcDownloadSpoilersCheckBox ||
                      QFileInfo::exists(SettingsCache::instance().paths().getSpoilerCardDatabasePath()))) {
-        if (!SpoilerBackgroundUpdater::deleteSpoilerFile()) {
+        observeSpoilerUpdater(SpoilerBackgroundUpdater::deleteSpoilerFile());
-            auto *notice = new QMessageBox(QMessageBox::Warning, tr("Spoiler update"),
-                                          tr("The spoiler database could not be removed. Check its file permissions "
-                                             "and try disabling spoilers again."),
-                                          QMessageBox::Ok, this);
-            notice->setTextFormat(Qt::PlainText);
-            notice->setAttribute(Qt::WA_DeleteOnClose);
-            notice->open();
-        }
     }
-    lastUpdatedLabel.setText(tr("Last Change") + ": " + getLastUpdateTime());
+    unlockSettings();
 }
 
 void DeckEditorSettingsPage::retranslateUi()
@@ -428,7 +429,7 @@ void DeckEditorSettingsPage::retranslateUi()
     picDownloadCheckBox.setText(tr("Download card pictures on the fly"));
     urlLinkLabel.setText(QString("<a href='%1'>%2</a>").arg(WIKI_CUSTOM_PIC_URL).arg(tr("How to add a custom URL")));
     resetDownloadURLs.setText(tr("Reset Download URLs"));
-    updateNowButton->setText(tr("Update Spoilers"));
+    unlockSettings();
     aAdd->setText(tr("Add New URL"));
     aEdit->setText(tr("Edit URL"));
     aRemove->setText(tr("Remove URL"));

cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h

@@ -69,6 +69,8 @@ private:
 
     /** @brief Refreshes the displayed label of every URL item after limits or settings change. */
     void refreshUrlItems();
+
+    void observeSpoilerUpdater(SpoilerBackgroundUpdater *updater);
 };
 
 #endif // COCKATRICE_DECK_EDITOR_SETTINGS_PAGE_H

tests/CMakeLists.txt

@@ -190,6 +190,15 @@ cockatrice_add_test(
   ${CMAKE_SOURCE_DIR}/cockatrice/src
   ${CMAKE_SOURCE_DIR}/libcockatrice_network
 )
+cockatrice_add_test(
+  NAME
+  spoiler_file_test
+  SOURCES
+  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/card_spoiler/spoiler_file.cpp
+  spoiler_file_test.cpp
+  INCLUDE_DIRS
+  ${CMAKE_SOURCE_DIR}/cockatrice/src
+)
 # The installer's holder probe is Windows-only, but the wording it produces and the
 # question which programs are the payload's own are plain C++ and are what a stuck user
 # has to act on, so they are tested everywhere.

tests/spoiler_file_test.cpp

@@ -0,0 +1,149 @@
+#include "client/network/update/card_spoiler/spoiler_file.h"
+
+#include <QDir>
+#include <QFile>
+#include <QFileInfo>
+#include <QTemporaryDir>
+#include <gtest/gtest.h>
+
+#ifdef Q_OS_UNIX
+#include <csignal>
+#include <sys/resource.h>
+#include <unistd.h>
+#endif
+
+namespace
+{
+class SpoilerFileTest : public ::testing::Test
+{
+protected:
+    QTemporaryDir directory;
+    QString path;
+    QString error;
+    const QByteArray original = "<cockatrice_carddatabase version=\"4\"><cards/></cockatrice_carddatabase>";
+
+    void SetUp() override
+    {
+        ASSERT_TRUE(directory.isValid());
+        path = directory.filePath("spoiler.xml");
+        QFile file(path);
+        ASSERT_TRUE(file.open(QIODevice::WriteOnly));
+        ASSERT_EQ(file.write(original), original.size());
+    }
+
+    QByteArray read()
+    {
+        QFile file(path);
+        EXPECT_TRUE(file.open(QIODevice::ReadOnly));
+        return file.readAll();
+    }
+
+    SpoilerFile::SaveResult save(const QByteArray &data)
+    {
+        return SpoilerFile::save(path, data, [] { return true; }, error);
+    }
+};
+
+TEST_F(SpoilerFileTest, InvalidDocumentsPreserveExistingFile)
+{
+    for (const QByteArray &invalid :
+         {QByteArray(), QByteArray("<html><cards/></html>"),
+          QByteArray("<cockatrice_carddatabase version=\"2\"><cards/></cockatrice_carddatabase>"),
+          QByteArray("<cockatrice_carddatabase><cards/></cockatrice_carddatabase>"),
+          QByteArray("<cockatrice_carddatabase version=\"4\"><sets/></cockatrice_carddatabase>"),
+          QByteArray("<cockatrice_carddatabase version=\"4\"><cards>"),
+          QByteArray("<cockatrice_carddatabase version=\"4\"><cards/></cockatrice_carddatabase><extra/>")}) {
+        EXPECT_EQ(save(invalid), SpoilerFile::SaveResult::Failed);
+        EXPECT_FALSE(error.isEmpty());
+        EXPECT_EQ(read(), original);
+    }
+}
+
+TEST_F(SpoilerFileTest, SupportedVersionsAreAccepted)
+{
+    EXPECT_TRUE(SpoilerFile::validate(original).isEmpty());
+    EXPECT_TRUE(
+        SpoilerFile::validate("<cockatrice_carddatabase version=\"3\"><cards/></cockatrice_carddatabase>").isEmpty());
+}
+
+TEST_F(SpoilerFileTest, IdenticalFileIsUnchanged)
+{
+    EXPECT_EQ(save(original), SpoilerFile::SaveResult::Unchanged);
+    EXPECT_TRUE(error.isEmpty());
+    EXPECT_EQ(read(), original);
+}
+
+TEST_F(SpoilerFileTest, CompleteFileHashDetectsChangesAfterLargeCommonPrefix)
+{
+    QByteArray changed = "<cockatrice_carddatabase version=\"4\"><!--";
+    changed += QByteArray(100000, 'x');
+    changed += "--><cards/></cockatrice_carddatabase>";
+    ASSERT_EQ(save(changed), SpoilerFile::SaveResult::Written);
+    changed.replace("<cards/>", "<cards></cards>");
+    EXPECT_EQ(save(changed), SpoilerFile::SaveResult::Written);
+    EXPECT_EQ(read(), changed);
+    EXPECT_TRUE(error.isEmpty());
+}
+
+TEST_F(SpoilerFileTest, CancellationBeforeWritePreservesExistingFile)
+{
+    EXPECT_EQ(SpoilerFile::save(path, original, [] { return false; }, error), SpoilerFile::SaveResult::Failed);
+    EXPECT_FALSE(error.isEmpty());
+    EXPECT_EQ(read(), original);
+}
+
+TEST_F(SpoilerFileTest, CancellationBeforeCommitPreservesExistingFile)
+{
+    int checks = 0;
+    EXPECT_EQ(SpoilerFile::save(
+                  path, original + "\n", [&checks] { return ++checks == 1; }, error),
+              SpoilerFile::SaveResult::Failed);
+    EXPECT_EQ(checks, 2);
+    EXPECT_FALSE(error.isEmpty());
+    EXPECT_EQ(read(), original);
+    EXPECT_EQ(QDir(directory.path()).entryList(QDir::Files), QStringList{"spoiler.xml"});
+}
+
+TEST_F(SpoilerFileTest, MissingParentIsCreated)
+{
+    path = directory.filePath("nested/spoiler.xml");
+    EXPECT_EQ(save(original), SpoilerFile::SaveResult::Written);
+    EXPECT_EQ(read(), original);
+}
+
+TEST_F(SpoilerFileTest, FileInParentPathReportsFailureWithoutModifyingIt)
+{
+    EXPECT_EQ(SpoilerFile::save(
+                  QDir(path).filePath("spoiler.xml"), original, [] { return true; }, error),
+              SpoilerFile::SaveResult::Failed);
+    EXPECT_FALSE(error.isEmpty());
+    EXPECT_EQ(read(), original);
+}
+
+#ifdef Q_OS_UNIX
+TEST_F(SpoilerFileTest, ShortWriteLeavesExistingFileIntact)
+{
+    // Limit only the child process: a real short write exercises QSaveFile's
+    // buffered flush failure without depending on disk capacity or file ownership.
+    const QByteArray changed = "<cockatrice_carddatabase version=\"4\"><!--" + QByteArray(100000, 'x') +
+                               "--><cards/></cockatrice_carddatabase>";
+    EXPECT_EXIT(
+        {
+            struct rlimit limit;
+            if (getrlimit(RLIMIT_FSIZE, &limit) != 0) {
+                _exit(2);
+            }
+            limit.rlim_cur = 128;
+            if (setrlimit(RLIMIT_FSIZE, &limit) != 0) {
+                _exit(3);
+            }
+            std::signal(SIGXFSZ, SIG_IGN);
+            const auto result = save(changed);
+            _exit(result == SpoilerFile::SaveResult::Failed && !error.isEmpty() ? 0 : 1);
+        },
+        ::testing::ExitedWithCode(0), "");
+    EXPECT_EQ(read(), original);
+    EXPECT_EQ(QDir(directory.path()).entryList(QDir::Files), QStringList{"spoiler.xml"});
+}
+#endif
+} // namespace