From 5d025ca0bde16f8ab88ffffa3c37e20d0e200b8f Mon Sep 17 00:00:00 2001 From: tooomm Date: Sat, 12 Sep 2026 17:30:46 +0200 Subject: [PATCH 1/8] Use capitalized app names (#7255) * use capitalized app name * update urls * app description * Update main.cpp --- cockatrice/src/client/sound_engine.cpp | 4 ++-- cockatrice/src/interface/deck_loader/deck_file_format.h | 2 +- cockatrice/src/interface/deck_loader/deck_loader.cpp | 2 +- cockatrice/src/interface/deck_loader/deck_loader.h | 2 +- cockatrice/src/interface/theme_manager.cpp | 2 +- cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp | 4 ++-- cockatrice/src/interface/window_main.cpp | 6 +++--- cockatrice/src/main.cpp | 8 ++++---- oracle/src/main.cpp | 6 +++--- oracle/src/pages.cpp | 2 +- oracle/src/parsehelpers.cpp | 2 +- oracle/src/raw_json_scanner.h | 2 +- servatrice/src/servatrice_database_interface.cpp | 7 ++++--- 13 files changed, 25 insertions(+), 24 deletions(-) diff --git a/cockatrice/src/client/sound_engine.cpp b/cockatrice/src/client/sound_engine.cpp index 18de2264d..96cafa3d3 100644 --- a/cockatrice/src/client/sound_engine.cpp +++ b/cockatrice/src/client/sound_engine.cpp @@ -94,7 +94,7 @@ QStringMap &SoundEngine::getAvailableThemes() QDir dir; availableThemes.clear(); - // load themes from user profile dir + // Load themes from user profile dir dir.setPath(SettingsCache::instance().getDataPath() + "/sounds"); @@ -104,7 +104,7 @@ QStringMap &SoundEngine::getAvailableThemes() } } - // load themes from cockatrice system dir + // Load themes from Cockatrice system dir dir.setPath(qApp->applicationDirPath() + #ifdef Q_OS_MAC "/../Resources/sounds" diff --git a/cockatrice/src/interface/deck_loader/deck_file_format.h b/cockatrice/src/interface/deck_loader/deck_file_format.h index 995de32c0..3a25797ec 100644 --- a/cockatrice/src/interface/deck_loader/deck_file_format.h +++ b/cockatrice/src/interface/deck_loader/deck_file_format.h @@ -17,7 +17,7 @@ enum Format PlainText, /** - * This is cockatrice's native deck file format, and supports deck metadata such as banner cards and tags. + * This is Cockatrice's native deck file format, and supports deck metadata such as banner cards and tags. * Stored as .cod files. */ Cockatrice diff --git a/cockatrice/src/interface/deck_loader/deck_loader.cpp b/cockatrice/src/interface/deck_loader/deck_loader.cpp index f03339da8..f29b4eed2 100644 --- a/cockatrice/src/interface/deck_loader/deck_loader.cpp +++ b/cockatrice/src/interface/deck_loader/deck_loader.cpp @@ -50,7 +50,7 @@ DeckLoader::loadFromFile(const QString &fileName, DeckFileFormat::Format fmt, bo result = deckList.loadFromFile_Native(&file); if (!result) { qCInfo(DeckLoaderLog) << "Failed to load " << fileName - << "as cockatrice format; retrying as plain format"; + << "as Cockatrice format; retrying as plain format"; file.seek(0); result = deckList.loadFromFile_Plain(&file, CardNameNormalizer()); fmt = DeckFileFormat::PlainText; diff --git a/cockatrice/src/interface/deck_loader/deck_loader.h b/cockatrice/src/interface/deck_loader/deck_loader.h index b851c6895..be0df311d 100644 --- a/cockatrice/src/interface/deck_loader/deck_loader.h +++ b/cockatrice/src/interface/deck_loader/deck_loader.h @@ -131,7 +131,7 @@ public: static void printDeckList(QPrinter *printer, const DeckList &deckList); /** - * Converts the given deck's file to the cockatrice file format. + * Converts the given deck's file to the Cockatrice file format. * Uses the lastLoadInfo in the LoadedDeck to determine the current name of the file and where to save to. * @param deck The deck to convert. Should have valid lastLoadInfo. Will update the lastLoadInfo. * @return Whether the conversion succeeded. diff --git a/cockatrice/src/interface/theme_manager.cpp b/cockatrice/src/interface/theme_manager.cpp index cc6be175a..12c8fad2c 100644 --- a/cockatrice/src/interface/theme_manager.cpp +++ b/cockatrice/src/interface/theme_manager.cpp @@ -224,7 +224,7 @@ QStringMap &ThemeManager::getAvailableThemes() } } - // load themes from cockatrice system dir + // Load themes from Cockatrice system dir dir.setPath(systemThemesBasePath()); for (QString themeName : dir.entryList(QDir::AllDirs | QDir::NoDotAndDotDot, QDir::Name)) { diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp index de7dd3e97..fb559fc4b 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp @@ -478,13 +478,13 @@ void DlgSettings::closeEvent(QCloseEvent *event) case Invalid: loadErrorMessage = tr("Your card database is invalid.\n\n" "Cockatrice may not function correctly with an invalid database\n\n" - "You may need to rerun oracle to update your card database.\n\n" + "You may need to rerun Oracle to update your card database.\n\n" "Would you like to change your database location setting?"); break; case VersionTooOld: loadErrorMessage = tr("Your card database version is too old.\n\n" "This can cause problems loading card information or images\n\n" - "Usually this can be fixed by rerunning oracle to to update your card database.\n\n" + "Usually this can be fixed by rerunning Oracle to to update your card database.\n\n" "Would you like to change your database location setting?"); break; case NotLoaded: diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index ce28fc24a..3daaeb8d3 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -92,8 +92,8 @@ #include #define GITHUB_PAGES_URL "https://cockatrice.github.io" -#define GITHUB_CONTRIBUTORS_URL "https://github.com/Cockatrice/Cockatrice/graphs/contributors?type=c" -#define GITHUB_CONTRIBUTE_URL "https://github.com/Cockatrice/Cockatrice#cockatrice" +#define GITHUB_CONTRIBUTORS_URL "https://github.com/Cockatrice/Cockatrice/graphs/contributors" +#define GITHUB_CONTRIBUTE_URL "https://github.com/Cockatrice/Cockatrice#" #define GITHUB_TRANSIFEX_TRANSLATORS_URL "https://github.com/Cockatrice/Cockatrice/wiki/Translator-Hall-of-Fame" #define GITHUB_TRANSLATOR_FAQ_URL "https://github.com/Cockatrice/Cockatrice/wiki/Translation-FAQ" #define GITHUB_ISSUES_URL "https://github.com/Cockatrice/Cockatrice/issues" @@ -1050,7 +1050,7 @@ void MainWindow::createCardUpdateProcess(bool background) if (dir.exists(binaryName)) { updaterCmd = dir.absoluteFilePath(binaryName); - } else { // try and find the directory oracle is stored in the build directory + } else { // try and find the directory Oracle is stored in the build directory QDir findLocalDir(dir); findLocalDir.cdUp(); findLocalDir.cd(getCardUpdaterBinaryName()); diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index ac77b4241..829e08742 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -231,7 +231,7 @@ int main(int argc, char *argv[]) // These values are only used by the settings loader/saver // Wrong or outdated values are kept to not break things QCoreApplication::setOrganizationName("Cockatrice"); - QCoreApplication::setOrganizationDomain("cockatrice.de"); + QCoreApplication::setOrganizationDomain("cockatrice.github.io"); QCoreApplication::setApplicationName("Cockatrice"); QCoreApplication::setApplicationVersion(VERSION_STRING); @@ -250,7 +250,7 @@ int main(int argc, char *argv[]) // Command-line parser QCommandLineParser parser; - parser.setApplicationDescription("Cockatrice"); + parser.setApplicationDescription("Cockatrice Client"); parser.addHelpOption(); parser.addVersionOption(); @@ -350,8 +350,8 @@ int main(int argc, char *argv[]) qCInfo(MainLog) << "MainWindow constructor finished"; ui.setWindowIcon(themePixmap(QStringLiteral("cockatrice"))); - // set name of the app desktop file; used by wayland to load the window icon - QGuiApplication::setDesktopFileName("cockatrice"); + // Set name of the app desktop file; used by wayland to load the window icon + QGuiApplication::setDesktopFileName("Cockatrice"); SettingsCache::instance().network().setClientID(generateClientID()); diff --git a/oracle/src/main.cpp b/oracle/src/main.cpp index bb88153b1..0f962f9be 100644 --- a/oracle/src/main.cpp +++ b/oracle/src/main.cpp @@ -50,8 +50,8 @@ int main(int argc, char *argv[]) QApplication app(argc, argv); QCoreApplication::setOrganizationName("Cockatrice"); - QCoreApplication::setOrganizationDomain("cockatrice"); - // this can't be changed, as it influences the default save path for cards.xml + QCoreApplication::setOrganizationDomain("Cockatrice"); + // This can't be changed, as it influences the default save path for cards.xml QCoreApplication::setApplicationName("Cockatrice"); // If the program is opened with the -s flag, it will only do spoilers. Otherwise it will do MTGJSON/Tokens @@ -83,7 +83,7 @@ int main(int argc, char *argv[]) QIcon icon("theme:appicon.svg"); wizard.setWindowIcon(icon); // set name of the app desktop file; used by wayland to load the window icon - QGuiApplication::setDesktopFileName("oracle"); + QGuiApplication::setDesktopFileName("Oracle"); wizard.show(); diff --git a/oracle/src/pages.cpp b/oracle/src/pages.cpp index a10daadb3..df0c3f51d 100644 --- a/oracle/src/pages.cpp +++ b/oracle/src/pages.cpp @@ -796,7 +796,7 @@ void SaveSetsPage::retranslateUi() { setTitle(tr("Sets imported")); if (wizard()->downloadedPlainXml) { - setSubTitle(tr("A cockatrice database file of %1 MB has been downloaded.") + setSubTitle(tr("A Cockatrice card database file of %1 MB has been downloaded.") .arg(qRound(wizard()->xmlData.size() / 1000000.0))); } else { setSubTitle(tr("The following sets have been found:")); diff --git a/oracle/src/parsehelpers.cpp b/oracle/src/parsehelpers.cpp index a97bf67c9..ee7c4eb2e 100644 --- a/oracle/src/parsehelpers.cpp +++ b/oracle/src/parsehelpers.cpp @@ -20,7 +20,7 @@ * Note that "...enters tapped unless..." returns false. * * @param name The name of the card - * @param text The oracle text of the card + * @param text The Oracle text of the card */ bool parseCipt(const QString &name, const QString &text) { diff --git a/oracle/src/raw_json_scanner.h b/oracle/src/raw_json_scanner.h index 086f2b3e3..04fcc52e4 100644 --- a/oracle/src/raw_json_scanner.h +++ b/oracle/src/raw_json_scanner.h @@ -54,7 +54,7 @@ using ScanProgressCallback = std::function expectedversion) { - qCCritical(DatabaseInterfaceLog) << poolStr << "Error opening database: the database schema version" - << dbversion << "is too new, you need to update servatrice" - << "(this servatrice actually uses version" << expectedversion << ")"; + qCCritical(DatabaseInterfaceLog) + << poolStr << "Error opening database: the database schema version" << dbversion + << "is too new, you need to update Servatrice" << "(Currently running Servatrice actually uses version" + << expectedversion << ")"; return false; } } else { From fd82b140a85a38145c1a1c59eaf510e329dcc503 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 13 Sep 2026 03:36:29 +0200 Subject: [PATCH 2/8] [PictureLoader] Serve cached pictures from the disk cache instead of re-fetching them (#7284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With picture downloads enabled, requests were issued with AlwaysNetwork cache control, which per Qt never consults the disk cache. A picture that had already been downloaded was therefore fetched from the network again on every session start, with the queue bypass letting those re-fetches skip the rate limit entirely. Treat the network cache as the intent of the 'Network Cache' storage method suggests: if the URL is already cached, serve it with AlwaysCache (no network, no quota); only a genuine miss goes to the network, and only when downloads are enabled. Cache hits skip the queue for free since they never consume the per-second request allowance. Co-authored-by: Lukas Brübach --- .../card_picture_loader_worker.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index d288236d2..34092f361 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -84,8 +84,8 @@ void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWor SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && cache->metaData(url).isValid()) { - // If we hit a cached url, we get to make the request for free, since it won't contribute towards the - // rate-limit + // A request that will be served from the disk cache never touches the network and therefore + // doesn't use up any of the rate limit, so it gets to skip the queue. makeRequest(url, worker); return; } @@ -107,10 +107,13 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture req.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING)); req.setRawHeader("Accept", "image/avif,image/webp,image/apng,image/,/*;q=0.8"); - bool useNetworkCache = - !picDownload && static_cast( - SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == - CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE; + // Cached entries are served straight from the disk cache even when picture downloads are + // enabled: re-fetching an already-cached image would burn the rate limit for nothing. Only a + // genuine cache miss goes to the network, and only when downloads are enabled. + bool useNetworkCache = static_cast( + SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == + CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && + (cache->metaData(url).isValid() || !picDownload); req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork); From 1c6ee62393d7495343c09a501ecdd86d19877369 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:51:07 +0200 Subject: [PATCH 3/8] [PictureLoader] Pace requests and run the throttle timers on the worker thread Previously the whole backed-up queue was drained in a burst as soon as a request was enqueued, sending up to 10 requests back-to-back and then immediately re-filling the quota one second later. That hard-bursts a rate-limited API like Scryfall's (10 requests/second) into a 30 second lockout. Introduce a pacing timer that dispatches a single queue entry every 100 ms, so the per-second allowance is used smoothly instead of in spikes, and keep the quota timer at 1 second. Also fix both timers' thread affinity: they are QTimer value members and so are not QObject children, meaning moveToThread() on the worker left them on the main thread while the slot code started them from the picture thread, which was a no-op that also warned. They are moved to the worker thread explicitly and started lazily from there. --- .../card_picture_loader_worker.cpp | 40 ++++++++++++++++--- .../card_picture_loader_worker.h | 4 ++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 34092f361..4a2caaab4 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -17,8 +17,10 @@ #include static constexpr int MAX_REQUESTS_PER_SEC = 10; -static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance -static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering +static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance +static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering +static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between individual network requests +static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), @@ -60,11 +62,18 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); + // QTimer value members are not QObject children, so moveToThread on the worker doesn't move + // them. They must live in the worker's thread to be started from the slot code that runs there. + requestTimer.moveToThread(pictureLoaderThread); + dispatchTimer.moveToThread(pictureLoaderThread); + connect(this, &CardPictureLoaderWorker::imageLoadEnqueued, this, &CardPictureLoaderWorker::handleImageLoadEnqueued); connect(&requestTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::resetRequestQuota); - requestTimer.setInterval(1000); - requestTimer.start(); + requestTimer.setInterval(static_cast(QUOTA_RESET_INTERVAL_MS)); + + connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest); + dispatchTimer.setInterval(DISPATCH_INTERVAL_MS); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -147,8 +156,29 @@ void CardPictureLoaderWorker::resetRequestQuota() void CardPictureLoaderWorker::processQueuedRequests() { - while (requestQuota > 0 && processSingleRequest()) { + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + return; + } + // Start lazily from the worker's own thread: QTimer must be started in the thread it lives in. + if (!requestTimer.isActive()) { + requestTimer.start(); + } + dispatchTimer.start(); +} + +void CardPictureLoaderWorker::dispatchQueuedRequest() +{ + if (requestLoadQueue.isEmpty() || requestQuota <= 0) { + dispatchTimer.stop(); + return; + } + + if (processSingleRequest()) { --requestQuota; + } else { + // No queued host currently has allowance left in this second; wait for the quota reset. + dispatchTimer.stop(); } } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index d1c519b7a..9f7fd9437 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -89,6 +89,9 @@ public slots: /** @brief Processes all queued requests respecting the request quota. */ void processQueuedRequests(); + /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ + void dispatchQueuedRequest(); + /** * @brief Processes a single queued request. * @return true if a request was processed, false if queue is empty. @@ -120,6 +123,7 @@ private: int requestQuota; ///< Remaining requests allowed per second QTimer requestTimer; ///< Timer to reset the request quota + QTimer dispatchTimer; ///< Timer pacing individual network requests QHash hostRequestQuota; ///< Sustained per-host request allowance QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited From e3820c3f34ad7aaf62e4aa27fa9995ca8aea4980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:52:25 +0200 Subject: [PATCH 4/8] [PictureLoader] Seed per-host allowances on demand and skip hosts in 429 backoff The quota reset re-filled every host's remaining allowance to a full MAX_REQUESTS_PER_SEC as soon as the queue had a request for it. A server that was just rate limited could therefore be hammered again at full speed immediately after (or even during) recovery. Only seed a host's allowance the first time it is dispatched in the current second, seeded from its reduced sustained quota, and skip hosts still inside their 429 backoff window entirely. This makes the pacing commit's burst-free behavior hold per host too, instead of just smoothing the global aggregate. --- .../card_picture_loader_worker.cpp | 22 +++++++++++++------ .../card_picture_loader_worker_work.cpp | 5 +++++ .../card_picture_loader_worker_work.h | 3 +++ 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 4a2caaab4..3add23bfb 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -138,6 +138,9 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { requestQuota = MAX_REQUESTS_PER_SEC; + // Allowances are seeded per host on demand in processSingleRequest(), so a + // rate-limited host never gets a fresh full quota mid-second. + hostQuotaRemaining.clear(); QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { @@ -146,11 +149,6 @@ void CardPictureLoaderWorker::resetRequestQuota() } } - for (const auto &request : requestLoadQueue) { - const QString host = request.first.host(); - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); - } - processQueuedRequests(); } @@ -184,10 +182,20 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() bool CardPictureLoaderWorker::processSingleRequest() { + QDateTime now = QDateTime::currentDateTime(); for (int i = 0; i < requestLoadQueue.size(); ++i) { const auto &request = requestLoadQueue.at(i); - QString host = request.first.host(); - int allowance = hostQuotaRemaining.value(host, MAX_REQUESTS_PER_SEC); + const QString host = request.first.host(); + // Don't dispatch requests to a host that is currently in its 429 backoff. + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + continue; + } + // Seed the allowance only now, so a host that was rate limited last second + // doesn't get a fresh full quota the moment it is queried mid-second. + if (!hostQuotaRemaining.contains(host)) { + hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + } + int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { hostQuotaRemaining.insert(host, allowance - 1); makeRequest(request.first, request.second); diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index 66c56337c..072a919d7 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp @@ -22,6 +22,11 @@ static const QStringList MD5_BLACKLIST = { "fbc7d763c08771c260b39e2115414eeb" // Current card back hash }; +ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() +{ + return s_rateLimiter; +} + CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index 1e56a4373..8490cb3ac 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -43,6 +43,9 @@ public: CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading + /** @brief Shared per-server 429 backoff state. */ + static ServerRateLimiter &rateLimiter(); + public slots: /** * @brief Handles a finished network reply for the card image. From da307a82b3c63e0b90cef4197d8203fb79133db6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 16:58:10 +0200 Subject: [PATCH 5/8] [PictureLoader] Add user-configurable per-host request caps Picture downloads were throttled to a uniform 10 requests/second per host with no way to tune a specific server. A rate-limited API host (Scryfall caps at 10 req/s) can trip 429s during bursts, and CDN hosts with no rate limit were throttled needlessly. Introduce developer-owned per-host caps that users can only ever lower, never raise, exposed in the download settings page: - DownloadSettings::DEVELOPER_HOST_CAPS sets the ceiling per host (api.scryfall.com 9, cards.scryfall.io unlimited, others 10). - A new hostRequestLimits setting stores user overrides in downloads.ini; clampHostRequestLimit() bounds them to [1, devCap] so a user can reduce api.scryfall.com to 5 but never raise it above 9. - The picture worker seeds, halves on 429, and recovers its sustained per-host allowance against the effective ceiling instead of the global maximum, and skips per-host accounting entirely for unlocked hosts (cards.scryfall.io) while global pacing and 429 backoff still apply. - The deck editor settings page gains one spinbox per known host, each clamped to its developer cap. --- .../card_picture_loader_worker.cpp | 41 ++++++++-- .../card_picture_loader_worker.h | 9 +++ .../deck_editor_settings_page.cpp | 74 +++++++++++++++++++ .../settings_page/deck_editor_settings_page.h | 7 ++ .../settings/download_settings.cpp | 45 +++++++++++ .../settings/download_settings.h | 34 +++++++++ tests/settings/settings_defaults_test.cpp | 32 ++++++++ 7 files changed, 236 insertions(+), 6 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 3add23bfb..84c5a02c5 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -16,15 +16,16 @@ #include #include -static constexpr int MAX_REQUESTS_PER_SEC = 10; -static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance +static constexpr int MAX_REQUESTS_PER_SEC = DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT; +static constexpr int MIN_HOST_QUOTA = DownloadSettings::MIN_HOST_REQUEST_LIMIT; static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between individual network requests static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), - requestQuota(MAX_REQUESTS_PER_SEC) + requestQuota(MAX_REQUESTS_PER_SEC), + hostRequestLimits(SettingsCache::instance().downloads().getHostRequestLimits()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -74,6 +75,9 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest); dispatchTimer.setInterval(DISPATCH_INTERVAL_MS); + + connect(&SettingsCache::instance().downloads(), &DownloadSettings::hostRequestLimitsChanged, this, + [this] { hostRequestLimits = SettingsCache::instance().downloads().getHostRequestLimits(); }); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -145,7 +149,9 @@ void CardPictureLoaderWorker::resetRequestQuota() QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { - it.value() = qMin(MAX_REQUESTS_PER_SEC, it.value() + 1); + // Recover towards the host's effective allowance ceiling, which may be + // lowered by the user's per-host request limits. + it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); } } @@ -190,10 +196,18 @@ bool CardPictureLoaderWorker::processSingleRequest() if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { continue; } + const int ceiling = hostAllowanceCeiling(host); + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the per-host allowance + // entirely; only the global quota and request pacing still apply. + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); + return true; + } // Seed the allowance only now, so a host that was rate limited last second // doesn't get a fresh full quota the moment it is queried mid-second. if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + hostQuotaRemaining.insert(host, hostRequestQuota.value(host, ceiling)); } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { @@ -206,9 +220,24 @@ bool CardPictureLoaderWorker::processSingleRequest() return false; } +int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const +{ + const int devCap = DownloadSettings::getDeveloperHostCaps().value(host, MAX_REQUESTS_PER_SEC); + if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestLimits.contains(host)) { + return DownloadSettings::UNLIMITED_HOST_QUOTA; + } + const int requested = hostRequestLimits.value(host, devCap); + return SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); + if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // Unlocked hosts have no per-host allowance to halve; the shared backoff + // window tracked by the rate limiter still paces them. + return; + } + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, hostAllowanceCeiling(host)) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 9f7fd9437..70bc36419 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -125,12 +125,21 @@ private: QTimer requestTimer; ///< Timer to reset the request quota QTimer dispatchTimer; ///< Timer pacing individual network requests QHash hostRequestQuota; ///< Sustained per-host request allowance + QHash hostRequestLimits; ///< User-set per-host request allowances QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited CardPictureLoaderLocal *localLoader; ///< Loader for local images QSet currentlyLoading; ///< Deduplication: contains pixmapCacheKey currently being loaded + /** + * @brief Effective per-host allowance ceiling for a host. + * @param host The host to look up + * @return The allowance ceiling in requests/second, or DownloadSettings::UNLIMITED_HOST_QUOTA + * when the developer unlocked the host and no user limit is set for it. + */ + [[nodiscard]] int hostAllowanceCeiling(const QString &host) const; + /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index f3eac05b8..4337bf0f3 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,12 +10,17 @@ #include #include #include +#include #include +#include +#include #include #include #include #include +static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for hosts unlocked by the developer + DeckEditorSettingsPage::DeckEditorSettingsPage() { picDownloadCheckBox.setChecked(SettingsCache::instance().downloads().getPicDownload()); @@ -96,6 +101,53 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() &DownloadSettings::setDownloadSpoilerStatus); connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, this, &DeckEditorSettingsPage::setSpoilersEnabled); + // Per-host request limit group: one spinbox per known picture host. A spinbox at its + // lower bound (0 for unlocked hosts, the developer cap for capped hosts) means "follow + // the developer default"; the worker clamps any explicit value against the developer cap. + mpRequestLimitGroupBox = new QGroupBox; + auto *requestLimitLayout = new QGridLayout; + + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + const QHash userLimits = SettingsCache::instance().downloads().getHostRequestLimits(); + + QSet hosts; + for (const QString &urlTemplate : SettingsCache::instance().downloads().getAllURLs()) { + hosts.insert(QUrl(urlTemplate).host()); + } + const QList devHosts = devCaps.keys(); + for (const QString &devHost : devHosts) { + hosts.insert(devHost); + } + + QList sortedHosts(hosts.cbegin(), hosts.cend()); + std::sort(sortedHosts.begin(), sortedHosts.end(), + [](const QString &a, const QString &b) { return a.localeAwareCompare(b) < 0; }); + + int hostRow = 1; + for (const QString &host : sortedHosts) { + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; + + auto *hostLabel = new QLabel(host); + auto *spinBox = new QSpinBox; + if (unlocked) { + spinBox->setRange(0, UNLOCKED_HOST_LIMIT_MAX); // 0 means "unlimited" + spinBox->setValue(userLimits.value(host, 0)); + } else { + spinBox->setRange(DownloadSettings::MIN_HOST_REQUEST_LIMIT, devCap); + spinBox->setValue(userLimits.value(host, devCap)); + } + connect(spinBox, &QSpinBox::valueChanged, this, &DeckEditorSettingsPage::storeRequestLimits); + + requestLimitLayout->addWidget(hostLabel, hostRow, 0); + requestLimitLayout->addWidget(spinBox, hostRow, 1); + requestLimitSpinBoxes.insert(host, spinBox); + ++hostRow; + } + + requestLimitLayout->addWidget(&requestLimitHelpLabel, hostRow, 0, 1, 2); + mpRequestLimitGroupBox->setLayout(requestLimitLayout); + mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -104,6 +156,7 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() auto *lpMainLayout = new QVBoxLayout; lpMainLayout->addWidget(mpGeneralGroupBox); + lpMainLayout->addWidget(mpRequestLimitGroupBox); lpMainLayout->addWidget(mpSpoilerGroupBox); setLayout(lpMainLayout); @@ -164,6 +217,24 @@ void DeckEditorSettingsPage::storeSettings() SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); } +void DeckEditorSettingsPage::storeRequestLimits() +{ + QHash stored; + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + for (auto it = requestLimitSpinBoxes.cbegin(); it != requestLimitSpinBoxes.cend(); ++it) { + const QString host = it.key(); + const int value = it.value()->value(); + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + // Only values that differ from the developer default are persisted; the worker + // treats a missing entry as "follow the developer default". + const int developerDefault = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? 0 : devCap; + if (value != developerDefault) { + stored.insert(host, value); + } + } + SettingsCache::instance().downloads().setHostRequestLimits(stored); +} + void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) { storeSettings(); @@ -230,6 +301,9 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput) void DeckEditorSettingsPage::retranslateUi() { mpGeneralGroupBox->setTitle(tr("URL Download Priority")); + mpRequestLimitGroupBox->setTitle(tr("Per-Host Request Limit")); + requestLimitHelpLabel.setText(tr("Pictures per second per host. Hosts can be lowered below their developer " + "limit but never raised above it; 0 means the host is not throttled per host.")); mpSpoilerGroupBox->setTitle(tr("Spoilers")); mcDownloadSpoilersCheckBox.setText(tr("Download Spoilers Automatically")); mcSpoilerSaveLabel.setText(tr("Spoiler Location:")); diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index 5db009c8a..b3745def4 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -5,9 +5,11 @@ #include #include +#include #include #include #include +#include class DeckEditorSettingsPage : public AbstractSettingsPage { @@ -19,6 +21,7 @@ public: private slots: void storeSettings(); + void storeRequestLimits(); void urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int); void setSpoilersEnabled(bool); void spoilerPathButtonClicked(); @@ -40,6 +43,10 @@ private: QGroupBox *mpGeneralGroupBox; QGroupBox *mpSpoilerGroupBox; + QGroupBox *mpRequestLimitGroupBox; + QLabel requestLimitHelpLabel; + QHash requestLimitSpinBoxes; + QLineEdit *mpSpoilerSavePathLineEdit; QLabel mcSpoilerSaveLabel; QLabel lastUpdatedLabel; diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index cfa1c054e..c510d8863 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -9,6 +9,22 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { "https://gatherer.wizards.com/Handlers/Image.ashx?multiverseid=!set:muid!&type=card", "https://gatherer.wizards.com/Handlers/Image.ashx?name=!name!&type=card"}; +// Developer-set ceilings for the per-host request allowance. Users may lower a host's +// allowance via the download settings, but can never raise it above these values. Hosts +// not listed default to DEFAULT_HOST_REQUEST_LIMIT. A cap of UNLIMITED_HOST_QUOTA marks a +// host that is never throttled per host (request pacing and 429 backoff still apply). +const QHash DownloadSettings::DEVELOPER_HOST_CAPS = { + // The Scryfall API enforces 10 requests/second; stay one under so a burst can't trip 429s. + {"api.scryfall.com", 9}, + // The Scryfall image CDN has no documented per-client rate limit. + {"cards.scryfall.io", UNLIMITED_HOST_QUOTA}, +}; + +const QHash &DownloadSettings::getDeveloperHostCaps() +{ + return DEVELOPER_HOST_CAPS; +} + DownloadSettings::DownloadSettings(const QString &settingPath, QObject *parent = nullptr) : SettingsManager(settingPath + "downloads.ini", "downloads", QString(), parent) { @@ -50,3 +66,32 @@ void DownloadSettings::setDownloadSpoilerStatus(bool _spoilerStatus) setValue(_spoilerStatus, "downloadSpoilers"); emit downloadSpoilerStatusChanged(); } + +QHash DownloadSettings::getHostRequestLimits() const +{ + const QVariantMap stored = getValue("hostRequestLimits").toMap(); + QHash hostRequestLimits; + for (auto it = stored.cbegin(); it != stored.cend(); ++it) { + hostRequestLimits.insert(it.key(), it.value().toInt()); + } + return hostRequestLimits; +} + +void DownloadSettings::setHostRequestLimits(const QHash &hostRequestLimits) +{ + QVariantMap stored; + for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { + stored.insert(it.key(), it.value()); + } + setValue(stored, "hostRequestLimits"); + emit hostRequestLimitsChanged(); +} + +int DownloadSettings::clampHostRequestLimit(const QString &host, int requested) const +{ + const int devCap = DEVELOPER_HOST_CAPS.value(host, DEFAULT_HOST_REQUEST_LIMIT); + if (devCap == UNLIMITED_HOST_QUOTA) { + return qMax(MIN_HOST_REQUEST_LIMIT, requested); + } + return qBound(MIN_HOST_REQUEST_LIMIT, requested, devCap); +} diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index a3a6f4ca9..e075a3c07 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -9,14 +9,34 @@ #include "settings_manager.h" +#include + class DownloadSettings : public SettingsManager { Q_OBJECT friend class SettingsCache; static const QStringList DEFAULT_DOWNLOAD_URLS; + static const QHash DEVELOPER_HOST_CAPS; public: + /** @brief Per-host request allowance (requests/second) when no developer cap applies. */ + static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; + /** @brief Floor for any per-host request allowance. */ + static constexpr int MIN_HOST_REQUEST_LIMIT = 1; + /** @brief Developer cap marking a host as never throttled per host (pacing still applies). */ + static constexpr int UNLIMITED_HOST_QUOTA = -1; + + /** + * @brief Developer-set per-host allowance ceilings (requests/second), keyed by host. + * + * Hosts not present default to DEFAULT_HOST_REQUEST_LIMIT. An entry of + * UNLIMITED_HOST_QUOTA marks a host that users may still lower, but that is never + * throttled per host by default. Users can never raise a host's allowance above its + * developer cap. + */ + static const QHash &getDeveloperHostCaps(); + explicit DownloadSettings(const QString &, QObject *); QStringList getAllURLs() const; @@ -27,9 +47,23 @@ public: [[nodiscard]] bool getDownloadSpoilersStatus() const; void setDownloadSpoilerStatus(bool _spoilerStatus); + /** @brief User-set per-host request allowances (requests/second). Missing hosts use the developer default. */ + QHash getHostRequestLimits() const; + void setHostRequestLimits(const QHash &hostRequestLimits); + + /** + * @brief Clamps the user's requested allowance for a host against its developer cap. + * @param host The host to clamp for + * @param requested The user-requested allowance in requests/second + * @return The effective allowance. Users may lower a host's allowance but never raise it + * above the developer cap; hosts with UNLIMITED_HOST_QUOTA have no upper bound. + */ + [[nodiscard]] int clampHostRequestLimit(const QString &host, int requested) const; + signals: void picDownloadChanged(); void downloadSpoilerStatusChanged(); + void hostRequestLimitsChanged(); }; #endif // COCKATRICE_DOWNLOADSETTINGS_H diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index eda778ae9..932ddb99b 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -334,6 +334,38 @@ TEST_F(SettingsDefaultsTest, Download_DownloadSpoilersStatus_Default) ASSERT_EQ(s.getDownloadSpoilersStatus(), false); } +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_Default) +{ + DownloadSettings s(settingsPath, nullptr); + ASSERT_TRUE(s.getHostRequestLimits().isEmpty()); +} + +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_SetAndGet) +{ + DownloadSettings s(settingsPath, nullptr); + s.setHostRequestLimits({{"api.scryfall.com", 5}}); + const QHash limits = s.getHostRequestLimits(); + ASSERT_EQ(limits.size(), 1); + ASSERT_EQ(limits.value("api.scryfall.com"), 5); +} + +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_StackedHostCaps) +{ + DownloadSettings s(settingsPath, nullptr); + // The developer cap for the Scryfall API lowers the ceiling to 9; a user can + // reduce it further but can never raise it above the cap. + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 9), 9); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 20), 9); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 5), 5); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 0), 1); + // Hosts without a developer cap fall back to the global default ceiling. + ASSERT_EQ(s.clampHostRequestLimit("gatherer.wizards.com", 10), 10); + ASSERT_EQ(s.clampHostRequestLimit("gatherer.wizards.com", 20), 10); + // The Scryfall CDN is unlocked: no upper bound (values are only floored). + ASSERT_EQ(s.clampHostRequestLimit("cards.scryfall.io", 20), 20); + ASSERT_EQ(s.clampHostRequestLimit("cards.scryfall.io", 0), 1); +} + // --- AppearanceSettings --- TEST_F(SettingsDefaultsTest, Appearance_ThemeName_Default) From 7b82ca08daa18fa5d188c4570c69db2b2f6706e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 17:32:30 +0200 Subject: [PATCH 6/8] [PictureLoader] Let unlocked hosts skip dispatch pacing; adjust limits per URL Two refinements to the per-host request caps: - Unlocked hosts (UNLIMITED_HOST_QUOTA, e.g. cards.scryfall.io) no longer wait on the 100ms dispatch pacing or consume the global per-second quota. dispatchQueuedRequest fires their queued requests back-to-back, bounded only by their 429 backoff window and Qt's per-host connection pool, so an unthrottled CDN is not artificially slowed. - The deck editor download settings page replaces the static grid of one spinbox per known host with an "Adjust Rate Limit" toolbar action on the URL list. It picks the host out of the selected URL and clamps the entry against the developer cap table (including for user-added URLs). Also fixes a review finding: resetRequestQuota could write the UNLIMITED_HOST_QUOTA sentinel (-1) into the sustained per-host quota when a host became unlocked mid-run, permanently poisoning its allowance. Stale entries for unlocked hosts are now dropped, and the per-second seed is clamped against the effective ceiling so a lowered limit applies immediately. --- .../card_picture_loader_worker.cpp | 47 +++++-- .../deck_editor_settings_page.cpp | 121 ++++++++---------- .../settings_page/deck_editor_settings_page.h | 10 +- .../settings/download_settings.cpp | 2 +- .../settings/download_settings.h | 2 +- 5 files changed, 91 insertions(+), 91 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 84c5a02c5..fab651d72 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -147,12 +147,19 @@ void CardPictureLoaderWorker::resetRequestQuota() hostQuotaRemaining.clear(); QDateTime now = QDateTime::currentDateTime(); - for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { + for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end();) { + // An unlocked host has no per-host allowance; drop any stale entry instead of + // recovering it towards the UNLIMITED_HOST_QUOTA sentinel, which would poison it. + if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + it = hostRequestQuota.erase(it); + continue; + } if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { // Recover towards the host's effective allowance ceiling, which may be // lowered by the user's per-host request limits. it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); } + ++it; } processQueuedRequests(); @@ -173,6 +180,27 @@ void CardPictureLoaderWorker::processQueuedRequests() void CardPictureLoaderWorker::dispatchQueuedRequest() { + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + return; + } + + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the dispatch pacing and the + // global per-second quota: dispatch every queued request for them back-to-back, bounded + // only by their 429 backoff window and Qt's per-host connection pool. + QDateTime now = QDateTime::currentDateTime(); + for (int i = 0; i < requestLoadQueue.size();) { + const auto &request = requestLoadQueue.at(i); + const QString host = request.first.host(); + if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && + !CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); + } else { + ++i; + } + } + if (requestLoadQueue.isEmpty() || requestQuota <= 0) { dispatchTimer.stop(); return; @@ -197,17 +225,11 @@ bool CardPictureLoaderWorker::processSingleRequest() continue; } const int ceiling = hostAllowanceCeiling(host); - // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the per-host allowance - // entirely; only the global quota and request pacing still apply. - if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { - makeRequest(request.first, request.second); - requestLoadQueue.removeAt(i); - return true; - } // Seed the allowance only now, so a host that was rate limited last second - // doesn't get a fresh full quota the moment it is queried mid-second. + // doesn't get a fresh full quota the moment it is queried mid-second. Clamp + // against the ceiling so a lowered user cap applies from this second onward. if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, ceiling)); + hostQuotaRemaining.insert(host, qMin(ceiling, hostRequestQuota.value(host, ceiling))); } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { @@ -232,12 +254,13 @@ int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + const int ceiling = hostAllowanceCeiling(host); + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { // Unlocked hosts have no per-host allowance to halve; the shared backoff // window tracked by the rate limiter still paces them. return; } - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, hostAllowanceCeiling(host)) / 2)); + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, ceiling) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index 4337bf0f3..6223cd480 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,16 +10,14 @@ #include #include #include -#include #include #include -#include #include #include #include #include -static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for hosts unlocked by the developer +static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for rate limits on hosts unlocked by the developer DeckEditorSettingsPage::DeckEditorSettingsPage() { @@ -70,11 +68,16 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() aRemove->setIcon(themePixmap(QStringLiteral("icons/decrement"))); connect(aRemove, &QAction::triggered, this, &DeckEditorSettingsPage::actRemoveURL); + aRateLimit = new QAction(this); + aRateLimit->setIcon(themePixmap(QStringLiteral("icons/cogwheel"))); + connect(aRateLimit, &QAction::triggered, this, &DeckEditorSettingsPage::actAdjustRateLimit); + auto *urlToolBar = new QToolBar; urlToolBar->setOrientation(Qt::Vertical); urlToolBar->addAction(aAdd); urlToolBar->addAction(aRemove); urlToolBar->addAction(aEdit); + urlToolBar->addAction(aRateLimit); urlToolBar->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::MinimumExpanding); auto *urlListLayout = new QHBoxLayout; @@ -101,53 +104,6 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() &DownloadSettings::setDownloadSpoilerStatus); connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, this, &DeckEditorSettingsPage::setSpoilersEnabled); - // Per-host request limit group: one spinbox per known picture host. A spinbox at its - // lower bound (0 for unlocked hosts, the developer cap for capped hosts) means "follow - // the developer default"; the worker clamps any explicit value against the developer cap. - mpRequestLimitGroupBox = new QGroupBox; - auto *requestLimitLayout = new QGridLayout; - - const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); - const QHash userLimits = SettingsCache::instance().downloads().getHostRequestLimits(); - - QSet hosts; - for (const QString &urlTemplate : SettingsCache::instance().downloads().getAllURLs()) { - hosts.insert(QUrl(urlTemplate).host()); - } - const QList devHosts = devCaps.keys(); - for (const QString &devHost : devHosts) { - hosts.insert(devHost); - } - - QList sortedHosts(hosts.cbegin(), hosts.cend()); - std::sort(sortedHosts.begin(), sortedHosts.end(), - [](const QString &a, const QString &b) { return a.localeAwareCompare(b) < 0; }); - - int hostRow = 1; - for (const QString &host : sortedHosts) { - const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; - - auto *hostLabel = new QLabel(host); - auto *spinBox = new QSpinBox; - if (unlocked) { - spinBox->setRange(0, UNLOCKED_HOST_LIMIT_MAX); // 0 means "unlimited" - spinBox->setValue(userLimits.value(host, 0)); - } else { - spinBox->setRange(DownloadSettings::MIN_HOST_REQUEST_LIMIT, devCap); - spinBox->setValue(userLimits.value(host, devCap)); - } - connect(spinBox, &QSpinBox::valueChanged, this, &DeckEditorSettingsPage::storeRequestLimits); - - requestLimitLayout->addWidget(hostLabel, hostRow, 0); - requestLimitLayout->addWidget(spinBox, hostRow, 1); - requestLimitSpinBoxes.insert(host, spinBox); - ++hostRow; - } - - requestLimitLayout->addWidget(&requestLimitHelpLabel, hostRow, 0, 1, 2); - mpRequestLimitGroupBox->setLayout(requestLimitLayout); - mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -156,7 +112,6 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() auto *lpMainLayout = new QVBoxLayout; lpMainLayout->addWidget(mpGeneralGroupBox); - lpMainLayout->addWidget(mpRequestLimitGroupBox); lpMainLayout->addWidget(mpSpoilerGroupBox); setLayout(lpMainLayout); @@ -217,22 +172,52 @@ void DeckEditorSettingsPage::storeSettings() SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); } -void DeckEditorSettingsPage::storeRequestLimits() +void DeckEditorSettingsPage::actAdjustRateLimit() { - QHash stored; - const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); - for (auto it = requestLimitSpinBoxes.cbegin(); it != requestLimitSpinBoxes.cend(); ++it) { - const QString host = it.key(); - const int value = it.value()->value(); - const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - // Only values that differ from the developer default are persisted; the worker - // treats a missing entry as "follow the developer default". - const int developerDefault = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? 0 : devCap; - if (value != developerDefault) { - stored.insert(host, value); - } + if (urlList->currentItem() == nullptr) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("Select a URL in the list first.")); + return; } - SettingsCache::instance().downloads().setHostRequestLimits(stored); + + const QString host = QUrl(urlList->currentItem()->text()).host(); + if (host.isEmpty()) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("The selected URL does not have a valid host.")); + return; + } + + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + const QHash currentLimits = SettingsCache::instance().downloads().getHostRequestLimits(); + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; + + bool ok = false; + int minimum; + int maximum; + int defaultValue; + if (unlocked) { + minimum = 0; // 0 means "unlimited" + maximum = UNLOCKED_HOST_LIMIT_MAX; + defaultValue = currentLimits.value(host, 0); + } else { + minimum = DownloadSettings::MIN_HOST_REQUEST_LIMIT; + maximum = devCap; + defaultValue = currentLimits.value(host, devCap); + } + + const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), + tr("Requests per second (developer maximum is %1):").arg(maximum), + defaultValue, minimum, maximum, 1, &ok); + if (!ok) { + return; + } + + QHash limits = currentLimits; + if (unlocked ? value == 0 : value == devCap) { + limits.remove(host); + } else { + limits.insert(host, value); + } + SettingsCache::instance().downloads().setHostRequestLimits(limits); } void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) @@ -301,9 +286,6 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput) void DeckEditorSettingsPage::retranslateUi() { mpGeneralGroupBox->setTitle(tr("URL Download Priority")); - mpRequestLimitGroupBox->setTitle(tr("Per-Host Request Limit")); - requestLimitHelpLabel.setText(tr("Pictures per second per host. Hosts can be lowered below their developer " - "limit but never raised above it; 0 means the host is not throttled per host.")); mpSpoilerGroupBox->setTitle(tr("Spoilers")); mcDownloadSpoilersCheckBox.setText(tr("Download Spoilers Automatically")); mcSpoilerSaveLabel.setText(tr("Spoiler Location:")); @@ -318,4 +300,5 @@ void DeckEditorSettingsPage::retranslateUi() aAdd->setText(tr("Add New URL")); aEdit->setText(tr("Edit URL")); aRemove->setText(tr("Remove URL")); -} \ No newline at end of file + aRateLimit->setText(tr("Adjust Rate Limit")); +} diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index b3745def4..57de5699e 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -5,11 +5,9 @@ #include #include -#include #include #include #include -#include class DeckEditorSettingsPage : public AbstractSettingsPage { @@ -21,7 +19,6 @@ public: private slots: void storeSettings(); - void storeRequestLimits(); void urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int); void setSpoilersEnabled(bool); void spoilerPathButtonClicked(); @@ -30,6 +27,7 @@ private slots: void actAddURL(); void actRemoveURL(); void actEditURL(); + void actAdjustRateLimit(); void resetDownloadedURLsButtonClicked(); private: @@ -37,16 +35,12 @@ private: QLabel urlLinkLabel; QCheckBox picDownloadCheckBox; QListWidget *urlList; - QAction *aAdd, *aEdit, *aRemove; + QAction *aAdd, *aEdit, *aRemove, *aRateLimit; QCheckBox mcDownloadSpoilersCheckBox; QLabel msDownloadSpoilersLabel; QGroupBox *mpGeneralGroupBox; QGroupBox *mpSpoilerGroupBox; - QGroupBox *mpRequestLimitGroupBox; - QLabel requestLimitHelpLabel; - QHash requestLimitSpinBoxes; - QLineEdit *mpSpoilerSavePathLineEdit; QLabel mcSpoilerSaveLabel; QLabel lastUpdatedLabel; diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index c510d8863..5293e390b 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -12,7 +12,7 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { // Developer-set ceilings for the per-host request allowance. Users may lower a host's // allowance via the download settings, but can never raise it above these values. Hosts // not listed default to DEFAULT_HOST_REQUEST_LIMIT. A cap of UNLIMITED_HOST_QUOTA marks a -// host that is never throttled per host (request pacing and 429 backoff still apply). +// host that is never throttled per host and skips the dispatch pacing (429 backoff still applies). const QHash DownloadSettings::DEVELOPER_HOST_CAPS = { // The Scryfall API enforces 10 requests/second; stay one under so a burst can't trip 429s. {"api.scryfall.com", 9}, diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index e075a3c07..9fcf9e61d 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -24,7 +24,7 @@ public: static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; /** @brief Floor for any per-host request allowance. */ static constexpr int MIN_HOST_REQUEST_LIMIT = 1; - /** @brief Developer cap marking a host as never throttled per host (pacing still applies). */ + /** @brief Developer cap marking a host as never throttled per host or by the dispatch pacing. */ static constexpr int UNLIMITED_HOST_QUOTA = -1; /** From 77ded1da97adb24b81eb73778e54e8cda393b0bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 22:50:19 +0200 Subject: [PATCH 7/8] [PictureLoader] Fix worker thread shutdown and cross-thread cache clearing clearNetworkCache() ran directly on the UI thread while the worker thread owned the disk cache and redirect cache, racing cache reads/writes. Make it a worker-thread slot invoked via a blocking queued call when the thread is running, so the 'Cached card pictures have been reset.' message is truthful. The worker thread was also never quit()/wait()ed: both destructors only deleteLater'd their objects, so Qt warned 'QThread: Destroyed while thread is still running' and leaked a running loop at exit. Wire the worker's finished() signal to its own deleteLater() (canonical worker-object pattern), add shutdownThread() to stop the loop, and let CardPictureLoader destroy the QThread only after wait() has returned. --- .../card_picture_loader.cpp | 20 ++++++++++-- .../card_picture_loader_worker.cpp | 26 +++++++++++++++- .../card_picture_loader_worker.h | 31 +++++++++++++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index 8c81d641d..e7e31f5a2 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -55,7 +56,14 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) CardPictureLoader::~CardPictureLoader() { - worker->deleteLater(); + if (worker) { + // Capture the thread first: shutdownThread() blocks until the worker has been freed by the + // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. + QThread *pictureLoaderThread = worker->workerThread(); + worker->shutdownThread(); + worker = nullptr; + delete pictureLoaderThread; + } } void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size) @@ -295,7 +303,15 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - getInstance().worker->clearNetworkCache(); + auto &worker = *getInstance().worker; + // The disk cache and redirect cache are owned by the worker thread; clearing them from the + // UI thread would race with the worker's cache reads/writes. Block until the worker thread + // has executed the clear so the "Cached card pictures have been reset." message is truthful. + if (worker.isRunning()) { + QMetaObject::invokeMethod(&worker, "clearNetworkCache", Qt::BlockingQueuedConnection); + } else { + worker.clearNetworkCache(); + } } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index fab651d72..daac1bff5 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -60,6 +60,9 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() localLoader = new CardPictureLoaderLocal(this); pictureLoaderThread = new QThread; + // The worker object frees itself once its thread finishes, so no event loop is left + // running and the QThread is never destroyed while still executing. + connect(pictureLoaderThread, &QThread::finished, this, &QObject::deleteLater); pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); @@ -83,7 +86,28 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() CardPictureLoaderWorker::~CardPictureLoaderWorker() { saveRedirectCache(); - pictureLoaderThread->deleteLater(); +} + +void CardPictureLoaderWorker::shutdownThread() +{ + // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as + // its event loop exits, so nothing - not even a member read - may run once wait() returns. + // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. + QThread *thread = pictureLoaderThread; + if (thread) { + thread->quit(); + thread->wait(); + } +} + +QThread *CardPictureLoaderWorker::workerThread() const +{ + return pictureLoaderThread; +} + +bool CardPictureLoaderWorker::isRunning() const +{ + return pictureLoaderThread != nullptr && pictureLoaderThread->isRunning(); } void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 70bc36419..96405252f 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -74,10 +74,37 @@ public: */ void onHostRateLimited(const QString &host); - /** @brief Clears the network cache and redirect cache. */ - void clearNetworkCache(); + /** + * @brief Stops the worker thread and releases it. + * + * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an + * exit request to the worker's event loop and QThread::wait() blocks until the loop has + * returned and the thread finished. Only QThread members are touched here, so this method is + * safe to call from the owning thread. The worker object itself is freed by the finished() -> + * deleteLater chain (see the constructor); the QThread object is deleted afterwards by the + * owner (CardPictureLoader::~CardPictureLoader), not by this method. + */ + void shutdownThread(); + + /** @return Whether the worker's thread is currently running. */ + bool isRunning() const; + + /** + * @brief Returns the worker's QThread. + * @return The worker thread + * + * Only meaningful while the worker object is alive; capture it before calling shutdownThread(). + */ + QThread *workerThread() const; public slots: + /** + * @brief Clears the network cache and redirect cache. + * + * Runs on the worker thread; invoke it via a queued call when coming from another thread, + * since both caches are owned by the worker thread. + */ + void clearNetworkCache(); /** * @brief Makes a network request for the given URL using the specified worker. * @param url URL to load From 8052676b292aed57f47a85fb2d3e64ef49afae0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 11:29:40 +0200 Subject: [PATCH 8/8] 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 4f5dc88eb..c950827b2 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -132,3 +132,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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +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 selectCardsForTemplate(CardDatabase &db, const QString &urlTemplate, int maxCards) +{ + QList selected; + const QList 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 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 &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 &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 [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 cards.xml to load card data from (required)\n" + " --count cards to load per template (default 300)\n" + " --url