From fe1894ebd6e4111c029e92a660dc9d41865daa1a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= <Bruebach.Lukas@bdosecurity.de>
Date: Sun, 6 Sep 2026 11:29:40 +0200
Subject: [PATCH 1/2] tests: add manual picture loader benchmark against the
 real card hosts

(cherry picked from commit 588d71d3b9a8278ab98b847802908f322c1035b7)
---
 tests/CMakeLists.txt                          |   6 +
 tests/picture_loader_benchmark/CMakeLists.txt |  26 +
 .../picture_loader_benchmark.cpp              | 479 ++++++++++++++++++
 .../settings_cache_mock.cpp                   | 200 ++++++++
 4 files changed, 711 insertions(+)
 create mode 100644 tests/picture_loader_benchmark/CMakeLists.txt
 create mode 100644 tests/picture_loader_benchmark/picture_loader_benchmark.cpp
 create mode 100644 tests/picture_loader_benchmark/settings_cache_mock.cpp

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 7bb834d7e..d23d9ba04 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -155,3 +155,9 @@ add_subdirectory(loading_from_clipboard)
 add_subdirectory(movecard_tests)
 add_subdirectory(oracle)
 add_subdirectory(settings)
+
+# picture_loader_benchmark links libcockatrice_settings, which only exists when
+# a client/UI-capable target is being built.
+if(WITH_ORACLE OR WITH_CLIENT)
+  add_subdirectory(picture_loader_benchmark)
+endif()
diff --git a/tests/picture_loader_benchmark/CMakeLists.txt b/tests/picture_loader_benchmark/CMakeLists.txt
new file mode 100644
index 000000000..a3b75fe88
--- /dev/null
+++ b/tests/picture_loader_benchmark/CMakeLists.txt
@@ -0,0 +1,26 @@
+# Manual benchmark hitting the real card image hosts. Not registered with
+# add_test(): it requires a cards.xml, touches the network for minutes at a
+# time, and needs network access. Build it explicitly and run by hand.
+add_executable(
+  picture_loader_benchmark_test
+  ${VERSION_STRING_CPP}
+  ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp
+  ../../cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp
+  ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp
+  ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp
+  ../../cockatrice/src/client/settings/cache_settings.h
+  picture_loader_benchmark.cpp
+  settings_cache_mock.cpp
+)
+
+target_include_directories(picture_loader_benchmark_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
+
+target_link_libraries(
+  picture_loader_benchmark_test
+  libcockatrice_card
+  libcockatrice_settings
+  libcockatrice_interfaces
+  libcockatrice_utility
+  Threads::Threads
+  ${TEST_QT_MODULES}
+)
diff --git a/tests/picture_loader_benchmark/picture_loader_benchmark.cpp b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp
new file mode 100644
index 000000000..44d930d37
--- /dev/null
+++ b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp
@@ -0,0 +1,479 @@
+/*
+ * Picture loader benchmark / regression suite against the real card image hosts.
+ *
+ * Deliberately not registered with ctest: it hits live Scryfall / Gatherer
+ * endpoints at ~10 requests per second and takes minutes. Run it by hand.
+ *
+ *   picture_loader_benchmark_test --carddb /path/to/cards.xml [options]
+ *
+ * Modes
+ * -----
+ * default : for every URL template in the configured download list (or for each
+ *           --url given), load #count pictures twice: once cold (network) and
+ *           once cached (served from the QNetworkDiskCache). Both passes must
+ *           load every card with zero failures. The cached pass must complete
+ *           well under the cold time, which is the regression gate for serving
+ *           cached pictures instead of re-fetching them. The cold pass must stay
+ *           above a pacing lower bound, the regression gate for burst-free
+ *           request throttling.
+ * --stress: two CardPictureLoaderWorker instances loading the same cards
+ *           concurrently against one host (~20 req/s aggregate), which forces
+ *           real 429 responses. Both workers must still complete 100% of their
+ *           cards via the shared backoff logic.
+ */
+
+#include "client/settings/cache_settings.h"
+#include "interface/card_picture_loader/card_picture_loader_worker.h"
+#include "interface/card_picture_loader/card_picture_to_load.h"
+
+#include <QCoreApplication>
+#include <QDir>
+#include <QElapsedTimer>
+#include <QEventLoop>
+#include <QFile>
+#include <QImage>
+#include <QList>
+#include <QMessageLogContext>
+#include <QMetaType>
+#include <QMutex>
+#include <QTemporaryDir>
+#include <QTimer>
+#include <QUrl>
+#include <cstdio>
+#include <libcockatrice/card/database/card_database.h>
+#include <libcockatrice/card/printing/exact_card.h>
+#include <libcockatrice/card/printing/printing_info.h>
+#include <libcockatrice/interfaces/interface_card_database_path_provider.h>
+#include <libcockatrice/interfaces/noop_card_preference_provider.h>
+#include <libcockatrice/interfaces/noop_card_set_priority_controller.h>
+#include <libcockatrice/settings/download_settings.h>
+#include <optional>
+
+class BenchmarkCardDatabasePathProvider : public ICardDatabasePathProvider
+{
+public:
+    BenchmarkCardDatabasePathProvider(QString _cardsXml, QString _customSetsDir)
+        : cardsXml(std::move(_cardsXml)), customSetsDir(std::move(_customSetsDir))
+    {
+    }
+
+    QString getCardDatabasePath() const override
+    {
+        return cardsXml;
+    }
+
+    QString getCustomCardDatabasePath() const override
+    {
+        return customSetsDir;
+    }
+
+    QString getTokenDatabasePath() const override
+    {
+        return QString();
+    }
+
+    QString getSpoilerCardDatabasePath() const override
+    {
+        return QString();
+    }
+
+private:
+    QString cardsXml;
+    QString customSetsDir;
+};
+
+struct PassResult
+{
+    int enqueued = 0;
+    int finished = 0;
+    int failed = 0;
+    qint64 elapsedMs = 0;
+    QStringList failedCards;
+};
+
+static QList<ExactCard> selectCardsForTemplate(CardDatabase &db, const QString &urlTemplate, int maxCards)
+{
+    QList<ExactCard> selected;
+    const QList<CardInfoPtr> cards = db.getCardList().values();
+    for (const CardInfoPtr &card : cards) {
+        if (selected.size() >= maxCards) {
+            break;
+        }
+        const SetToPrintingsMap &sets = card->getSets();
+        if (sets.isEmpty()) {
+            continue;
+        }
+        const QList<PrintingInfo> printings = sets.first();
+        if (printings.isEmpty()) {
+            continue;
+        }
+        const ExactCard cardToLoad(card, printings.first());
+        if (CardPictureToLoad(cardToLoad).transformUrl(urlTemplate).isEmpty()) {
+            continue;
+        }
+        selected.append(cardToLoad);
+    }
+    return selected;
+}
+
+static PassResult runPass(CardPictureLoaderWorker *worker, const QList<ExactCard> &cards, int timeoutMs)
+{
+    PassResult result;
+    result.enqueued = cards.size();
+
+    QEventLoop loop;
+    QTimer watchdog;
+    watchdog.setSingleShot(true);
+    watchdog.setInterval(timeoutMs);
+    QObject::connect(&watchdog, &QTimer::timeout, &loop, &QEventLoop::quit);
+
+    QElapsedTimer clock;
+    QObject::connect(worker, &CardPictureLoaderWorker::imageLoaded, &loop,
+                     [&](const ExactCard &card, const QImage &image) {
+                         ++result.finished;
+                         if (image.isNull()) {
+                             ++result.failed;
+                             if (result.failedCards.size() < 10) {
+                                 result.failedCards.append(card.getName());
+                             }
+                         }
+                         if (result.finished >= result.enqueued) {
+                             loop.quit();
+                         }
+                     });
+
+    clock.start();
+    for (const ExactCard &card : cards) {
+        worker->enqueueImageLoad(card);
+    }
+    watchdog.start();
+    loop.exec();
+    result.elapsedMs = clock.elapsed();
+    return result;
+}
+
+struct StressResult
+{
+    PassResult a;
+    PassResult b;
+    int http429Count = 0;
+};
+
+struct LogCounters
+{
+    int http429 = 0;
+    QMutex mutex;
+};
+
+static LogCounters *s_activeCounters = nullptr;
+static QtMessageHandler s_previousMessageHandler = nullptr;
+
+static void stressLogHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg)
+{
+    if (s_activeCounters && msg.contains(QStringLiteral("Too many requests from"))) {
+        QMutexLocker locker(&s_activeCounters->mutex);
+        ++s_activeCounters->http429;
+    }
+    if (s_previousMessageHandler) {
+        s_previousMessageHandler(type, context, msg);
+    }
+}
+
+static StressResult runStress(CardPictureLoaderWorker *workerA,
+                              CardPictureLoaderWorker *workerB,
+                              const QList<ExactCard> &cards,
+                              int timeoutMs)
+{
+    StressResult result;
+    result.a.enqueued = cards.size();
+    result.b.enqueued = cards.size();
+
+    int completed = 0;
+    QMutex completedMutex;
+
+    QEventLoop loop;
+    QTimer watchdog;
+    watchdog.setSingleShot(true);
+    watchdog.setInterval(timeoutMs);
+    QObject::connect(&watchdog, &QTimer::timeout, &loop, &QEventLoop::quit);
+
+    const auto finishOne = [&](PassResult &pass, const ExactCard &card, const QImage &image) {
+        ++pass.finished;
+        if (image.isNull()) {
+            ++pass.failed;
+            if (pass.failedCards.size() < 10) {
+                pass.failedCards.append(card.getName());
+            }
+        }
+        QMutexLocker locker(&completedMutex);
+        ++completed;
+        if (completed >= result.a.enqueued + result.b.enqueued) {
+            loop.quit();
+        }
+    };
+
+    QElapsedTimer clock;
+    QObject::connect(workerA, &CardPictureLoaderWorker::imageLoaded, &loop,
+                     [&](const ExactCard &card, const QImage &image) { finishOne(result.a, card, image); });
+    QObject::connect(workerB, &CardPictureLoaderWorker::imageLoaded, &loop,
+                     [&](const ExactCard &card, const QImage &image) { finishOne(result.b, card, image); });
+
+    // Count 429 responses as seen by the shared rate limiter.
+    LogCounters counters;
+    s_activeCounters = &counters;
+    s_previousMessageHandler = qInstallMessageHandler(stressLogHandler);
+
+    clock.start();
+    for (const ExactCard &card : cards) {
+        workerA->enqueueImageLoad(card);
+        workerB->enqueueImageLoad(card);
+    }
+    watchdog.start();
+    loop.exec();
+    const qint64 elapsedMs = clock.elapsed();
+    result.a.elapsedMs = elapsedMs;
+    result.b.elapsedMs = elapsedMs;
+
+    result.http429Count = counters.http429;
+    s_activeCounters = nullptr;
+    qInstallMessageHandler(s_previousMessageHandler);
+    return result;
+}
+
+static QString formatDuration(qint64 ms)
+{
+    return QStringLiteral("%1.%2 s").arg(ms / 1000).arg((ms % 1000) / 100);
+}
+
+static bool likelyRedirects(const QString &urlTemplate)
+{
+    return urlTemplate.contains(QStringLiteral("api.scryfall.com"));
+}
+
+static QString hostOf(const QString &urlTemplate)
+{
+    return QUrl(urlTemplate).host();
+}
+
+static void printUsage()
+{
+    std::printf("usage: picture_loader_benchmark_test --carddb <path> [options]\n"
+                "\n"
+                "Loads card pictures from the real configured hosts (not a mock server) and\n"
+                "verifies the picture loader's pacing / cache 429 behavior.\n"
+                "\n"
+                "options:\n"
+                "  --carddb <path>       cards.xml to load card data from (required)\n"
+                "  --count <N>           cards to load per template (default 300)\n"
+                "  --url <template>      test only this URL template; repeatable\n"
+                "  --stress              run the two-worker concurrency stress instead\n"
+                "  --stress-url <t>      template used by --stress (default: scryfall uuid)\n"
+                "  --timeout-min <N>     per-pass watchdog in minutes (default auto)\n"
+                "  --cache-dir <dir>     reuse this directory as the sandbox root; non-empty\n"
+                "                        cache skips the cold pacing lower bound\n"
+                "  --help                show this help\n");
+}
+
+int main(int argc, char **argv)
+{
+    QString cardDbArg;
+    QStringList explicitUrls;
+    int count = 300;
+    bool stress = false;
+    QString stressUrl(QStringLiteral("https://api.scryfall.com/cards/!set:uuid!?format=image"));
+    QString cacheDirArg;
+    std::optional<int> timeoutMin;
+
+    QStringList args;
+    for (int i = 1; i < argc; ++i) {
+        args.append(QString::fromLocal8Bit(argv[i]));
+    }
+    for (int i = 0; i < args.size(); ++i) {
+        const QString &arg = args.at(i);
+        const auto value = [&]() -> QString { return i + 1 < args.size() ? args.at(++i) : QString(); };
+        if (arg == QLatin1String("--carddb")) {
+            cardDbArg = value();
+        } else if (arg == QLatin1String("--count")) {
+            count = value().toInt();
+        } else if (arg == QLatin1String("--url")) {
+            explicitUrls.append(value());
+        } else if (arg == QLatin1String("--stress")) {
+            stress = true;
+        } else if (arg == QLatin1String("--stress-url")) {
+            stressUrl = value();
+        } else if (arg == QLatin1String("--timeout-min")) {
+            timeoutMin = value().toInt();
+        } else if (arg == QLatin1String("--cache-dir")) {
+            cacheDirArg = value();
+        } else if (arg == QLatin1String("--help") || arg == QLatin1String("-h")) {
+            printUsage();
+            return 0;
+        } else {
+            std::fprintf(stderr, "unknown argument: %s\n", qPrintable(arg));
+            printUsage();
+            return 2;
+        }
+    }
+
+    if (cardDbArg.isEmpty()) {
+        std::fprintf(stderr, "error: --carddb is required (a cards.xml generated by Oracle)\n");
+        printUsage();
+        return 2;
+    }
+    if (count <= 0) {
+        std::fprintf(stderr, "error: --count must be positive\n");
+        return 2;
+    }
+
+    QCoreApplication app(argc, argv);
+    app.setApplicationName(QStringLiteral("Cockatrice"));
+    app.setOrganizationName(QStringLiteral("Cockatrice"));
+    app.setApplicationVersion(QStringLiteral("9.0.0-benchmark"));
+
+    // The ExactCard argument of imageLoaded crosses threads via a queued connection.
+    qRegisterMetaType<ExactCard>();
+
+    QTemporaryDir sandbox;
+    const QString rootDir = cacheDirArg.isEmpty() ? sandbox.path() : cacheDirArg;
+    QDir().mkpath(rootDir);
+    QDir().mkpath(rootDir + "/config");
+    QDir().mkpath(rootDir + "/data");
+    QDir().mkpath(rootDir + "/cache");
+
+#ifdef Q_OS_UNIX
+    // Redirect every QStandardPaths lookup (and therefore SettingsCache paths)
+    // into the sandbox so the benchmark never touches user config or caches.
+    qputenv("XDG_CONFIG_HOME", (rootDir + "/config").toUtf8());
+    qputenv("XDG_DATA_HOME", (rootDir + "/data").toUtf8());
+    qputenv("XDG_CACHE_HOME", (rootDir + "/cache").toUtf8());
+#endif
+
+    const bool warmStart = QDir(rootDir + "/cache/Cockatrice/downloaded")
+                               .entryList(QDir::Files | QDir::AllDirs | QDir::NoDotAndDotDot, QDir::Name)
+                               .size() > 0;
+
+    // Copied into the sandbox so the loader's binary cache ("cards.xml.cache")
+    // is written next to it instead of next to the user's file, and so a
+    // --cache-dir rerun can pick it up again.
+    const QString dataPath = rootDir + "/data/Cockatrice";
+    QDir().mkpath(dataPath);
+    const QString cardsXml = dataPath + "/cards.xml";
+    if (!QFile::exists(cardsXml)) {
+        if (!QFile::copy(cardDbArg, cardsXml)) {
+            std::fprintf(stderr, "error: could not copy %s to sandbox\n", qPrintable(cardDbArg));
+            return 2;
+        }
+    }
+    const QString customSetsDir = dataPath + "/customsets";
+    QDir().mkpath(customSetsDir);
+
+    auto *prefs = new NoopCardPreferenceProvider();
+    auto *priorityController = new NoopCardSetPriorityController();
+    auto *pathProvider = new BenchmarkCardDatabasePathProvider(cardsXml, customSetsDir);
+    CardDatabase db(nullptr, prefs, pathProvider, priorityController);
+    db.loadCardDatabases();
+    if (db.getLoadStatus() != Ok || db.getCardList().isEmpty()) {
+        std::fprintf(stderr, "error: failed to load card database from %s\n", qPrintable(cardsXml));
+        return 1;
+    }
+    const int availableCards = db.getCardList().size();
+
+    const QStringList urlsToTest =
+        explicitUrls.isEmpty() ? SettingsCache::instance().downloads().getAllURLs() : explicitUrls;
+
+    if (stress) {
+        const QList<ExactCard> cards = selectCardsForTemplate(db, stressUrl, count);
+        if (cards.isEmpty()) {
+            std::fprintf(stderr, "error: no cards satisfy template %s\n", qPrintable(stressUrl));
+            return 1;
+        }
+        SettingsCache::instance().downloads().setDownloadUrls({stressUrl});
+
+        std::printf("=== STRESS: 2 workers x %d cards, host %s ===\n", static_cast<int>(cards.size()),
+                    qPrintable(hostOf(stressUrl)));
+        auto *workerA = new CardPictureLoaderWorker();
+        auto *workerB = new CardPictureLoaderWorker();
+        const int timeoutMs = (timeoutMin.has_value() ? timeoutMin.value() : 15) * 60 * 1000;
+        const StressResult result = runStress(workerA, workerB, cards, timeoutMs);
+
+        const bool completeA = result.a.finished >= result.a.enqueued;
+        const bool completeB = result.b.finished >= result.b.enqueued;
+        const bool zeroFailures = result.a.failed == 0 && result.b.failed == 0;
+        const bool pass = completeA && completeB && zeroFailures;
+
+        std::printf("worker A: %d/%d loaded, %d failed (%.2f images/s)\n", result.a.finished, result.a.enqueued,
+                    result.a.failed,
+                    result.a.elapsedMs > 0
+                        ? static_cast<double>(result.a.finished) * 1000.0 / static_cast<double>(result.a.elapsedMs)
+                        : 0.0);
+        std::printf("worker B: %d/%d loaded, %d failed (%.2f images/s)\n", result.b.finished, result.b.enqueued,
+                    result.b.failed,
+                    result.b.elapsedMs > 0
+                        ? static_cast<double>(result.b.finished) * 1000.0 / static_cast<double>(result.b.elapsedMs)
+                        : 0.0);
+        std::printf("429 responses observed: %d\n", result.http429Count);
+        std::printf("elapsed: %s\n", qPrintable(formatDuration(result.a.elapsedMs)));
+        if (result.a.failed > 0) {
+            std::printf("  worker A failures: %s\n", qPrintable(result.a.failedCards.join(QStringLiteral(", "))));
+        }
+        if (result.b.failed > 0) {
+            std::printf("  worker B failures: %s\n", qPrintable(result.b.failedCards.join(QStringLiteral(", "))));
+        }
+        std::printf("RESULT: %s\n", pass ? "PASS" : "FAIL");
+        return pass ? 0 : 1;
+    }
+
+    bool allPass = true;
+    std::printf("=== PICTURE LOADER BENCHMARK (%d cards available, %d per template)%s ===\n", availableCards, count,
+                warmStart ? ", WARM cache from previous run" : "");
+
+    auto *worker = new CardPictureLoaderWorker();
+    for (const QString &urlTemplate : urlsToTest) {
+        const QList<ExactCard> cards = selectCardsForTemplate(db, urlTemplate, count);
+        if (cards.isEmpty()) {
+            std::printf("SKIP  %-22s %-52s (no cards satisfy the template)\n", qPrintable(hostOf(urlTemplate)),
+                        qPrintable(urlTemplate));
+            continue;
+        }
+        SettingsCache::instance().downloads().setDownloadUrls({urlTemplate});
+
+        const bool redirects = likelyRedirects(urlTemplate);
+        const qint64 perCardMs = redirects ? 200 : 100;
+        const int timeoutMs =
+            (timeoutMin.has_value() ? timeoutMin.value() : (cards.size() * perCardMs * 8 + 60000) / 60000) * 60 * 1000;
+        const qint64 coldLowerMs = static_cast<qint64>(cards.size()) * perCardMs / 2;
+        const qint64 cachedUpperMs = 8000;
+
+        const PassResult cold = runPass(worker, cards, timeoutMs);
+        const PassResult cached = runPass(worker, cards, timeoutMs);
+
+        const bool coldComplete = cold.finished >= cold.enqueued;
+        const bool coldZeroFailures = cold.failed == 0;
+        const bool coldPaced = warmStart || cold.elapsedMs >= coldLowerMs;
+        const bool cachedComplete = cached.finished >= cached.enqueued;
+        const bool cachedZeroFailures = cached.failed == 0;
+        const bool cachedFast = cached.elapsedMs < cachedUpperMs;
+
+        const bool pass =
+            coldComplete && coldZeroFailures && coldPaced && cachedComplete && cachedZeroFailures && cachedFast;
+        allPass = allPass && pass;
+
+        std::printf("%s  %-22s %-52s cold %s (paced>=%s) cached %s (must be <%.0fs) | %d/%d %d/%d loaded, "
+                    "%d/%d failed\n",
+                    pass ? "PASS " : "FAIL ", qPrintable(hostOf(urlTemplate)), qPrintable(urlTemplate),
+                    qPrintable(formatDuration(cold.elapsedMs)), qPrintable(formatDuration(coldLowerMs)),
+                    qPrintable(formatDuration(cached.elapsedMs)), static_cast<double>(cachedUpperMs) / 1000.0,
+                    cold.finished, cold.enqueued, cached.finished, cached.enqueued, cold.failed, cached.failed);
+        if (cold.failed > 0) {
+            std::printf("  cold failures: %s\n", qPrintable(cold.failedCards.join(QStringLiteral(", "))));
+        }
+        if (cached.failed > 0) {
+            std::printf("  cached failures: %s\n", qPrintable(cached.failedCards.join(QStringLiteral(", "))));
+        }
+    }
+
+    std::printf("cache root: %s%s\n", qPrintable(rootDir),
+                cacheDirArg.isEmpty() ? " (reuse with --cache-dir to warm on the next run)" : "");
+    std::printf("RESULT: %s\n", allPass ? "PASS" : "FAIL");
+    return allPass ? 0 : 1;
+}
\ No newline at end of file
diff --git a/tests/picture_loader_benchmark/settings_cache_mock.cpp b/tests/picture_loader_benchmark/settings_cache_mock.cpp
new file mode 100644
index 000000000..9b20db603
--- /dev/null
+++ b/tests/picture_loader_benchmark/settings_cache_mock.cpp
@@ -0,0 +1,200 @@
+/*
+ * Minimal SettingsCache implementation for the picture loader benchmark.
+ *
+ * The picture loader code insists on reading every URL, cache and path via
+ * SettingsCache::instance(). Compiling the real client SettingsCache would drag
+ * in the network update-checker graph, so instead we provide the SettingsCache
+ * member functions it actually uses, backed by the real libcockatrice_settings
+ * manager classes pointed at a sandboxed settings directory.
+ *
+ * The mock must be named SettingsCache: PathsSettings declares
+ * `friend class SettingsCache`, which is the only way to construct it.
+ *
+ * Paths resolved through QStandardPaths are redirected into the sandbox by
+ * setting the XDG_* environment variables before the first instance() call.
+ */
+
+#include "client/settings/cache_settings.h"
+#include "interface/card_picture_loader/card_picture_loader_cache_method.h"
+
+#include <QDir>
+#include <QFile>
+#include <QSettings>
+#include <QStandardPaths>
+#include <libcockatrice/settings/cache_storage_settings.h>
+#include <libcockatrice/settings/cards_display_settings.h>
+#include <libcockatrice/settings/download_settings.h>
+#include <libcockatrice/settings/paths_settings.h>
+#include <utility>
+
+QString SettingsCache::getDataPath()
+{
+    return QStandardPaths::writableLocation(QStandardPaths::AppLocalDataLocation);
+}
+
+QString SettingsCache::getSettingsPath()
+{
+    return getDataPath() + "/settings/";
+}
+
+QString SettingsCache::getCachePath() const
+{
+    return QStandardPaths::writableLocation(QStandardPaths::CacheLocation);
+}
+
+QString SettingsCache::getNetworkCachePath() const
+{
+    return getCachePath() + "/downloaded/";
+}
+
+QString SettingsCache::getCustomCardDatabasePath() const
+{
+    return paths().getCustomCardDatabasePath();
+}
+
+QString SettingsCache::getCardDatabasePath() const
+{
+    return paths().getCardDatabasePath();
+}
+
+QString SettingsCache::getSpoilerCardDatabasePath() const
+{
+    return paths().getSpoilerCardDatabasePath();
+}
+
+QString SettingsCache::getTokenDatabasePath() const
+{
+    return paths().getTokenDatabasePath();
+}
+
+int SettingsCache::getKeepAlive() const
+{
+    return 0;
+}
+
+int SettingsCache::getTimeOut() const
+{
+    return 0;
+}
+
+bool SettingsCache::getNotifyAboutUpdates() const
+{
+    return false;
+}
+
+void SettingsCache::setKnownMissingFeatures(const QString & /*_knownMissingFeatures*/)
+{
+}
+
+QString SettingsCache::getKnownMissingFeatures()
+{
+    return QString();
+}
+
+QString SettingsCache::getClientID()
+{
+    return QString();
+}
+
+DownloadSettings &SettingsCache::downloads() const
+{
+    return *downloadSettings;
+}
+
+CacheStorageSettings &SettingsCache::cacheStorage() const
+{
+    return *cacheStorageSettings;
+}
+
+CardsDisplaySettings &SettingsCache::cardsDisplay() const
+{
+    return *cardsDisplaySettings;
+}
+
+PathsSettings &SettingsCache::paths() const
+{
+    return *pathsSettings;
+}
+
+SettingsCache::SettingsCache()
+{
+    settings = nullptr;
+    isPortableBuild = false;
+
+    const QString settingsPath = getSettingsPath();
+    QDir().mkpath(settingsPath);
+
+    downloadSettings = new DownloadSettings(settingsPath, this);
+    cacheStorageSettings = new CacheStorageSettings(settingsPath, this);
+    cardsDisplaySettings = new CardsDisplaySettings(settingsPath, this);
+    pathsSettings = new PathsSettings(settingsPath, this);
+
+    // Picture downloads enabled, default host list, network disk cache storage.
+    downloadSettings->setPicDownload(true);
+    downloadSettings->resetToDefaultURLs();
+    cacheStorageSettings->setCardImageCacheMethod(
+        static_cast<int>(CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE));
+
+    loadPaths();
+}
+
+void SettingsCache::loadPaths()
+{
+    QString dataPath = getDataPath();
+    QSettings pathsIni(getSettingsPath() + "paths.ini", QSettings::IniFormat);
+
+    auto computePath = [&](const QString &key, const QString &defaultPath) -> QString {
+        QString val = pathsIni.value("paths/" + key).toString();
+        if (val.isEmpty() || !QDir(val).exists()) {
+            if (!QDir().mkpath(defaultPath)) {
+                qCInfo(SettingsCacheLog) << "[SettingsCache] Could not create folder:" << defaultPath;
+            }
+            val = defaultPath;
+            pathsIni.setValue("paths/" + key, val);
+        }
+        return val;
+    };
+
+    auto computeFilePath = [&](const QString &key, const QString &defaultPath) -> QString {
+        QString val = pathsIni.value("paths/" + key).toString();
+        if (!QFile::exists(val) || val.isEmpty()) {
+            val = defaultPath;
+            pathsIni.setValue("paths/" + key, val);
+        }
+        return val;
+    };
+
+    computePath("decks", dataPath + "/decks/");
+    computePath("filters", dataPath + "/filters/");
+    computePath("replays", dataPath + "/replays/");
+    computePath("themes", dataPath + "/themes/");
+    computePath("pics", dataPath + "/pics/");
+    computePath("redirects", getCachePath() + "/redirects/");
+
+    // customPicsPath derived from picsPath
+    QString picsPath = pathsIni.value("paths/pics").toString();
+    if (picsPath.endsWith("/")) {
+        computePath("customPics", picsPath + "CUSTOM/");
+    } else {
+        computePath("customPics", picsPath + "/CUSTOM/");
+    }
+
+    computePath("customSets", dataPath + "/customsets/");
+    computeFilePath("cardDatabase", dataPath + "/cards.xml");
+    computeFilePath("tokenDatabase", dataPath + "/tokens.xml");
+    computeFilePath("spoilerDatabase", dataPath + "/spoiler.xml");
+}
+
+void SettingsCache::setThemeName(const QString &_themeName)
+{
+    if (themeName != _themeName) {
+        themeName = _themeName;
+        emit themeChanged();
+    }
+}
+
+SettingsCache &SettingsCache::instance()
+{
+    static SettingsCache settingsCache;
+    return settingsCache;
+}
\ No newline at end of file

From 904eb158f9f648e47c51df68e1adf28b038a4de3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= <Bruebach.Lukas@bdosecurity.de>
Date: Fri, 18 Sep 2026 04:53:03 +0200
Subject: [PATCH 2/2] tests: address review on picture loader benchmark

- Sandbox via unique app/org names plus Linux-only XDG redirection, derive
  the warm-cache probe and data path from SettingsCache, and bail out when the
  temporary sandbox cannot be created.
- Run each pass with a fresh worker and shut workers down before reading the
  429 counters, so the redirect cache is persisted and no worker thread can
  outlive the stack-local counters or the installed message handler.
- Make s_activeCounters atomic and always forward log output when the previous
  handler is the built-in (nullptr) one.
- Validate --timeout-min, scale the cached-pass budget with --count, honour the
  first --url as the stress template, and add the missing trailing newlines.
- Zero-initialise SettingsCache members so the benchmark mock cannot
  dereference an indeterminate pointer.
---
 .../src/client/settings/cache_settings.h      |  54 ++++-----
 .../picture_loader_benchmark.cpp              | 108 ++++++++++++++----
 .../settings_cache_mock.cpp                   |   2 +-
 3 files changed, 116 insertions(+), 48 deletions(-)

diff --git a/cockatrice/src/client/settings/cache_settings.h b/cockatrice/src/client/settings/cache_settings.h
index 23cdb4dbf..7a90d82f1 100644
--- a/cockatrice/src/client/settings/cache_settings.h
+++ b/cockatrice/src/client/settings/cache_settings.h
@@ -58,33 +58,33 @@ signals:
     void themeChanged();
 
 private:
-    QSettings *settings;
-    ShortcutsSettings *shortcutsSettings;
-    CardDatabaseSettings *cardDatabaseSettings;
-    ServersSettings *serversSettings;
-    MessageSettings *messageSettings;
-    GameFiltersSettings *gameFiltersSettings;
-    LayoutsSettings *layoutsSettings;
-    DownloadSettings *downloadSettings;
-    RecentsSettings *recentsSettings;
-    CardOverrideSettings *cardOverrideSettings;
-    DebugSettings *debugSettings;
-    CardCounterSettings *cardCounterSettings;
-    TabsSettings *tabsSettings;
-    SoundSettings *soundSettings;
-    GameSettings *gameSettings;
-    ChatSettings *chatSettings;
-    CacheStorageSettings *cacheStorageSettings;
-    UpdatesSettings *updatesSettings;
-    PersonalSettings *personalSettings;
-    CardsDisplaySettings *cardsDisplaySettings;
-    InterfaceSettings *interfaceSettings;
-    DeckEditorSettings *deckEditorSettings;
-    PathsSettings *pathsSettings;
-    VisualDeckStorageSettings *visualDeckStorageSettings;
-    AppearanceSettings *appearanceSettings;
-    NetworkSettings *networkSettings;
-    CommanderBracketSettings *commanderBracketSettings;
+    QSettings *settings = nullptr;
+    ShortcutsSettings *shortcutsSettings = nullptr;
+    CardDatabaseSettings *cardDatabaseSettings = nullptr;
+    ServersSettings *serversSettings = nullptr;
+    MessageSettings *messageSettings = nullptr;
+    GameFiltersSettings *gameFiltersSettings = nullptr;
+    LayoutsSettings *layoutsSettings = nullptr;
+    DownloadSettings *downloadSettings = nullptr;
+    RecentsSettings *recentsSettings = nullptr;
+    CardOverrideSettings *cardOverrideSettings = nullptr;
+    DebugSettings *debugSettings = nullptr;
+    CardCounterSettings *cardCounterSettings = nullptr;
+    TabsSettings *tabsSettings = nullptr;
+    SoundSettings *soundSettings = nullptr;
+    GameSettings *gameSettings = nullptr;
+    ChatSettings *chatSettings = nullptr;
+    CacheStorageSettings *cacheStorageSettings = nullptr;
+    UpdatesSettings *updatesSettings = nullptr;
+    PersonalSettings *personalSettings = nullptr;
+    CardsDisplaySettings *cardsDisplaySettings = nullptr;
+    InterfaceSettings *interfaceSettings = nullptr;
+    DeckEditorSettings *deckEditorSettings = nullptr;
+    PathsSettings *pathsSettings = nullptr;
+    VisualDeckStorageSettings *visualDeckStorageSettings = nullptr;
+    AppearanceSettings *appearanceSettings = nullptr;
+    NetworkSettings *networkSettings = nullptr;
+    CommanderBracketSettings *commanderBracketSettings = nullptr;
 
     QString themeName;
 
diff --git a/tests/picture_loader_benchmark/picture_loader_benchmark.cpp b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp
index 44d930d37..f991f2ec0 100644
--- a/tests/picture_loader_benchmark/picture_loader_benchmark.cpp
+++ b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp
@@ -37,8 +37,11 @@
 #include <QMetaType>
 #include <QMutex>
 #include <QTemporaryDir>
+#include <QThread>
 #include <QTimer>
 #include <QUrl>
+#include <QtLogging>
+#include <atomic>
 #include <cstdio>
 #include <libcockatrice/card/database/card_database.h>
 #include <libcockatrice/card/printing/exact_card.h>
@@ -165,17 +168,38 @@ struct LogCounters
     QMutex mutex;
 };
 
-static LogCounters *s_activeCounters = nullptr;
+static std::atomic<LogCounters *> s_activeCounters{nullptr};
 static QtMessageHandler s_previousMessageHandler = nullptr;
 
 static void stressLogHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg)
 {
-    if (s_activeCounters && msg.contains(QStringLiteral("Too many requests from"))) {
-        QMutexLocker locker(&s_activeCounters->mutex);
-        ++s_activeCounters->http429;
+    if (LogCounters *counters = s_activeCounters.load(std::memory_order_acquire);
+        counters && msg.contains(QStringLiteral("Too many requests from"))) {
+        QMutexLocker locker(&counters->mutex);
+        ++counters->http429;
     }
     if (s_previousMessageHandler) {
         s_previousMessageHandler(type, context, msg);
+    } else {
+        // qInstallMessageHandler() reports the built-in handler as nullptr, so a plain `if` would
+        // swallow every message - including the 429 warnings this run is meant to surface. Fall
+        // back to Qt's message pattern written to stderr instead.
+        std::fprintf(stderr, "%s\n", qPrintable(qFormatLogMessage(type, context, msg)));
+    }
+}
+
+// Stops a worker's thread and frees it. shutdownThread()'s bounded wait guarantees that the worker
+// object was freed by its finished() -> deleteLater chain, so the thread itself can then be deleted
+// safely. A worker whose thread refused to stop is left alone (and leaked) rather than freed while
+// still running.
+static void destroyWorker(CardPictureLoaderWorker *worker)
+{
+    if (!worker) {
+        return;
+    }
+    QThread *thread = worker->workerThread();
+    if (worker->shutdownThread()) {
+        delete thread;
     }
 }
 
@@ -234,8 +258,14 @@ static StressResult runStress(CardPictureLoaderWorker *workerA,
     result.a.elapsedMs = elapsedMs;
     result.b.elapsedMs = elapsedMs;
 
+    // Stop both workers before touching the counters or restoring the message handler: their
+    // threads log from stressLogHandler, and must not outlive the stack-local counters (which is
+    // guaranteed on the watchdog path, where requests and deferred retries are still pending).
+    destroyWorker(workerA);
+    destroyWorker(workerB);
+
     result.http429Count = counters.http429;
-    s_activeCounters = nullptr;
+    s_activeCounters.store(nullptr, std::memory_order_release);
     qInstallMessageHandler(s_previousMessageHandler);
     return result;
 }
@@ -280,6 +310,7 @@ int main(int argc, char **argv)
     QStringList explicitUrls;
     int count = 300;
     bool stress = false;
+    bool stressUrlSet = false;
     QString stressUrl(QStringLiteral("https://api.scryfall.com/cards/!set:uuid!?format=image"));
     QString cacheDirArg;
     std::optional<int> timeoutMin;
@@ -301,8 +332,15 @@ int main(int argc, char **argv)
             stress = true;
         } else if (arg == QLatin1String("--stress-url")) {
             stressUrl = value();
+            stressUrlSet = true;
         } else if (arg == QLatin1String("--timeout-min")) {
-            timeoutMin = value().toInt();
+            bool ok = false;
+            const int parsed = value().toInt(&ok);
+            if (!ok || parsed <= 0) {
+                std::fprintf(stderr, "error: --timeout-min must be a positive integer\n");
+                return 2;
+            }
+            timeoutMin = parsed;
         } else if (arg == QLatin1String("--cache-dir")) {
             cacheDirArg = value();
         } else if (arg == QLatin1String("--help") || arg == QLatin1String("-h")) {
@@ -325,37 +363,53 @@ int main(int argc, char **argv)
         return 2;
     }
 
+    // In --stress mode an explicit --url selects the template to hammer, matching --url's meaning
+    // in the normal mode; only fall back to the built-in Scryfall template when neither --stress-url
+    // nor --url was supplied.
+    if (stress && !stressUrlSet && !explicitUrls.isEmpty()) {
+        stressUrl = explicitUrls.first();
+    }
+
     QCoreApplication app(argc, argv);
-    app.setApplicationName(QStringLiteral("Cockatrice"));
-    app.setOrganizationName(QStringLiteral("Cockatrice"));
+    // Unique names so a benchmark run can never read or write the real client's settings, cache or
+    // picture URLs on platforms where the XDG redirection below does not apply (macOS, Windows).
+    app.setApplicationName(QStringLiteral("Cockatrice-benchmark"));
+    app.setOrganizationName(QStringLiteral("Cockatrice-benchmark"));
     app.setApplicationVersion(QStringLiteral("9.0.0-benchmark"));
 
     // The ExactCard argument of imageLoaded crosses threads via a queued connection.
     qRegisterMetaType<ExactCard>();
 
     QTemporaryDir sandbox;
+    if (cacheDirArg.isEmpty() && !sandbox.isValid()) {
+        std::fprintf(stderr, "error: could not create a temporary sandbox directory\n");
+        return 2;
+    }
     const QString rootDir = cacheDirArg.isEmpty() ? sandbox.path() : cacheDirArg;
     QDir().mkpath(rootDir);
     QDir().mkpath(rootDir + "/config");
     QDir().mkpath(rootDir + "/data");
     QDir().mkpath(rootDir + "/cache");
 
-#ifdef Q_OS_UNIX
-    // Redirect every QStandardPaths lookup (and therefore SettingsCache paths)
-    // into the sandbox so the benchmark never touches user config or caches.
+#ifdef Q_OS_LINUX
+    // Redirect every QStandardPaths lookup (and therefore SettingsCache paths) into the sandbox so
+    // the benchmark never touches user config or caches. XDG_* only affects Qt's path resolution on
+    // Linux; elsewhere the unique application/organization names above keep the run isolated.
     qputenv("XDG_CONFIG_HOME", (rootDir + "/config").toUtf8());
     qputenv("XDG_DATA_HOME", (rootDir + "/data").toUtf8());
     qputenv("XDG_CACHE_HOME", (rootDir + "/cache").toUtf8());
 #endif
 
-    const bool warmStart = QDir(rootDir + "/cache/Cockatrice/downloaded")
+    // Derive the probe from SettingsCache rather than reconstructing it: Qt appends both the
+    // organization and the application name, so a hand-built path is easy to get wrong.
+    const bool warmStart = QDir(SettingsCache::instance().getNetworkCachePath())
                                .entryList(QDir::Files | QDir::AllDirs | QDir::NoDotAndDotDot, QDir::Name)
                                .size() > 0;
 
     // Copied into the sandbox so the loader's binary cache ("cards.xml.cache")
     // is written next to it instead of next to the user's file, and so a
     // --cache-dir rerun can pick it up again.
-    const QString dataPath = rootDir + "/data/Cockatrice";
+    const QString dataPath = SettingsCache::instance().getDataPath();
     QDir().mkpath(dataPath);
     const QString cardsXml = dataPath + "/cards.xml";
     if (!QFile::exists(cardsXml)) {
@@ -427,7 +481,6 @@ int main(int argc, char **argv)
     std::printf("=== PICTURE LOADER BENCHMARK (%d cards available, %d per template)%s ===\n", availableCards, count,
                 warmStart ? ", WARM cache from previous run" : "");
 
-    auto *worker = new CardPictureLoaderWorker();
     for (const QString &urlTemplate : urlsToTest) {
         const QList<ExactCard> cards = selectCardsForTemplate(db, urlTemplate, count);
         if (cards.isEmpty()) {
@@ -442,10 +495,21 @@ int main(int argc, char **argv)
         const int timeoutMs =
             (timeoutMin.has_value() ? timeoutMin.value() : (cards.size() * perCardMs * 8 + 60000) / 60000) * 60 * 1000;
         const qint64 coldLowerMs = static_cast<qint64>(cards.size()) * perCardMs / 2;
-        const qint64 cachedUpperMs = 8000;
+        // Decoding and cache-reading scale with the card count, so a flat budget would spuriously
+        // fail larger --count runs served entirely from a healthy cache.
+        const qint64 cachedUpperMs = qMax<qint64>(2000, static_cast<qint64>(cards.size()) * 10);
 
-        const PassResult cold = runPass(worker, cards, timeoutMs);
-        const PassResult cached = runPass(worker, cards, timeoutMs);
+        // A fresh worker per pass: if the cold pass hits the watchdog, its outstanding cards stay
+        // in the worker's currentlyLoading set, which would make the cached pass silently skip them
+        // and burn its own watchdog; late cold replies would also be misattributed to the cached
+        // pass.
+        auto *coldWorker = new CardPictureLoaderWorker();
+        const PassResult cold = runPass(coldWorker, cards, timeoutMs);
+        destroyWorker(coldWorker);
+
+        auto *cachedWorker = new CardPictureLoaderWorker();
+        const PassResult cached = runPass(cachedWorker, cards, timeoutMs);
+        destroyWorker(cachedWorker);
 
         const bool coldComplete = cold.finished >= cold.enqueued;
         const bool coldZeroFailures = cold.failed == 0;
@@ -472,8 +536,12 @@ int main(int argc, char **argv)
         }
     }
 
-    std::printf("cache root: %s%s\n", qPrintable(rootDir),
-                cacheDirArg.isEmpty() ? " (reuse with --cache-dir to warm on the next run)" : "");
+    if (cacheDirArg.isEmpty()) {
+        std::printf("cache root: %s (temporary; pass --cache-dir <dir> to persist it for a warm rerun)\n",
+                    qPrintable(rootDir));
+    } else {
+        std::printf("cache root: %s\n", qPrintable(rootDir));
+    }
     std::printf("RESULT: %s\n", allPass ? "PASS" : "FAIL");
     return allPass ? 0 : 1;
-}
\ No newline at end of file
+}
diff --git a/tests/picture_loader_benchmark/settings_cache_mock.cpp b/tests/picture_loader_benchmark/settings_cache_mock.cpp
index 9b20db603..d93c7e002 100644
--- a/tests/picture_loader_benchmark/settings_cache_mock.cpp
+++ b/tests/picture_loader_benchmark/settings_cache_mock.cpp
@@ -197,4 +197,4 @@ SettingsCache &SettingsCache::instance()
 {
     static SettingsCache settingsCache;
     return settingsCache;
-}
\ No newline at end of file
+}