From 334a7439538f57730a70715f0cfe7a4aba03b23d Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:30:02 +0200 Subject: [PATCH 1/8] [PictureLoader] Pace requests and run the throttle timers on the worker thread (#7285) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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. * [PictureLoader] Guard dispatch timer restarts and drop dead request quota * [PictureLoader] Reconcile quota-timer lifecycle with idle 429 recovery --------- Co-authored-by: Lukas Brübach --- .../card_picture_loader_worker.cpp | 80 ++++++++++++++++--- .../card_picture_loader_worker.h | 10 ++- 2 files changed, 77 insertions(+), 13 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..728c5cc6d 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,12 +17,13 @@ #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()), - requestQuota(MAX_REQUESTS_PER_SEC) + : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -60,11 +61,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() @@ -128,8 +136,6 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { - requestQuota = MAX_REQUESTS_PER_SEC; - 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) { @@ -142,13 +148,65 @@ void CardPictureLoaderWorker::resetRequestQuota() hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); } - processQueuedRequests(); + updateTimerState(); } void CardPictureLoaderWorker::processQueuedRequests() { - while (requestQuota > 0 && processSingleRequest()) { - --requestQuota; + // QTimer must be started from the thread it lives in; if this public slot is ever reached from + // another thread, replay it on the worker's event loop instead of letting start() fail silently. + if (thread() != QThread::currentThread()) { + QMetaObject::invokeMethod(this, &CardPictureLoaderWorker::processQueuedRequests, Qt::QueuedConnection); + return; + } + updateTimerState(); +} + +void CardPictureLoaderWorker::dispatchQueuedRequest() +{ + if (requestLoadQueue.isEmpty()) { + // All queued requests have been dispatched; stop the pacing timers. + updateTimerState(); + return; + } + + if (!processSingleRequest()) { + // No queued host currently has allowance left in this second; wait for the quota reset. + dispatchTimer.stop(); + } +} + +void CardPictureLoaderWorker::updateTimerState() +{ + // Never restart an active timer: that would reset the pacing countdown and a burst of enqueues + // could keep starving the dispatcher, so only (re)start a timer that has actually stopped. + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + // Forget per-second allowances once nothing is pending: a stale zero would otherwise delay + // the next single request by a full quota-reset interval. + hostQuotaRemaining.clear(); + } else if (!dispatchTimer.isActive()) { + dispatchTimer.start(); + } + + // The quota timer resets allowances every second and is also the only thing that heals a host + // after a 429 (see resetRequestQuota). It must keep ticking while work is queued or a host is + // still recovering below the ceiling, and only winds down once no host needs recovery anymore. + // Keeping it alive during such idle periods lets reduced quotas recover as intended. + bool hostRecovering = false; + for (auto it = hostRequestQuota.cbegin(); it != hostRequestQuota.cend(); ++it) { + if (it.value() < MAX_REQUESTS_PER_SEC) { + hostRecovering = true; + break; + } + } + + if (!requestLoadQueue.isEmpty() || hostRecovering) { + if (!requestTimer.isActive()) { + requestTimer.start(); + } + } else if (requestTimer.isActive()) { + requestTimer.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..2a13e847c 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 @@ -86,9 +86,12 @@ public slots: */ QNetworkReply *makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *workThread); - /** @brief Processes all queued requests respecting the request quota. */ + /** @brief Ensures the pacing and quota-reset timers reflect the current queue and recovery state. */ 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. @@ -118,8 +121,8 @@ private: bool picDownload; ///< Whether downloading images from network is enabled QQueue> requestLoadQueue; ///< Queue of pending network requests - 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 @@ -139,6 +142,9 @@ private: /** @brief Removes stale redirect entries older than TTL. */ void cleanStaleEntries(); + /** @brief Starts or stops the pacing and quota-reset timers to match the queue and recovery state. */ + void updateTimerState(); + private slots: /** @brief Resets the request quota for rate-limiting. */ void resetRequestQuota(); From 3d5eb84d8110fa42b4d45ec49791b6f3038769de Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:30:03 +0200 Subject: [PATCH 2/8] [PictureLoader] Seed per-host allowances on demand and skip hosts in 429 backoff (#7286) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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, so a server that was just rate limited could 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, handing the entry back to its worker so it can wait the backoff out or fall through to another source instead of parking in the queue with no reply pending. Deferrals wait on the host that is actually blocking the request (cached-redirect targets and the reply host of a 429) rather than the current card URL's host. Rebased onto network-requests/request-pacing, which absorbed the earlier pacing and dispatch-guard commits, and reuses its updateTimerState idle-429 recovery plumbing. * [PictureLoader] Hand backed-off entries back across the whole dispatch tick Review fixes on the 429-backoff skip: - Hand a backed-off queue entry back to its worker via scheduleDeferredRetry(host) instead of startNextPicDownload(), which was looping on the pre-redirect host every 100ms tick whenever the queued URL was a cached-redirect target whose redirect host was the one in backoff. - Keep scanning the queue after a hand-back (--i; continue) so a backed-off host at the head no longer consumes the entire dispatch tick, stalling every healthy host further down. - Emit imageRequestSucceeded(url) before makeRequest()'s cached-redirect backoff return so the status bar reclaims the deferred URL's widget instead of inflating the progress bar forever. - Only spend a per-second allowance when makeRequest() actually issues a request: it returns nullptr when the redirect target is backed off and the work is handed back, so the slot would otherwise be wasted on a no-op. - Gate the dispatch-time backoff skip on requestTouchesNetwork() so entries served straight from the disk cache (e.g. with downloads disabled) are not bounced into a 30-60s deferred retry for a host that 429'd earlier. --------- Co-authored-by: Lukas Brübach --- .../card_picture_loader_worker.cpp | 67 +++++++++++++++---- .../card_picture_loader_worker.h | 4 ++ .../card_picture_loader_worker_work.cpp | 19 ++++-- .../card_picture_loader_worker_work.h | 36 ++++++---- 4 files changed, 94 insertions(+), 32 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 728c5cc6d..93ce6c96f 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 @@ -107,7 +107,17 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture // Check for cached redirects QUrl cachedRedirect = getCachedRedirect(url); if (!cachedRedirect.isEmpty()) { + // The status bar still needs to reclaim this URL's widget even when we hand the request back + // for a deferred retry instead of dispatching it onto the network. emit imageRequestSucceeded(url); + // The redirect target is a different host, which may itself be in 429 backoff; hand the + // entry back to its worker so it waits the backoff out instead of dispatching straight + // onto the backed-off host. + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(cachedRedirect.host(), + QDateTime::currentDateTime())) { + worker->scheduleDeferredRetry(cachedRedirect.host()); + return nullptr; + } return makeRequest(cachedRedirect, worker); } @@ -118,10 +128,7 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture // 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); + bool useNetworkCache = !requestTouchesNetwork(url); req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork); @@ -143,10 +150,10 @@ void CardPictureLoaderWorker::resetRequestQuota() } } - for (const auto &request : requestLoadQueue) { - const QString host = request.first.host(); - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); - } + // Forget the per-second allowances; each host's allowance is re-seeded lazily from its + // reduced sustained quota the first time it is dispatched in the new second, so a host that + // enters the queue mid-second no longer falls through to a fresh full quota. + hostQuotaRemaining.clear(); updateTimerState(); } @@ -212,20 +219,54 @@ void CardPictureLoaderWorker::updateTimerState() 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; hand the entry + // back to its worker so it can wait the backoff out or fall through to another source, + // instead of leaving it parked in the queue with no reply pending. Only applies to + // requests that will actually touch the network: one that will be served from the disk + // cache costs nothing and shouldn't wait out the 429. + if (requestTouchesNetwork(request.first) && + CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + // The queued URL is usually a cached-redirect target whose host differs from + // cardToDownload.getCurrentUrl(), so scheduleDeferredRetry() (which waits out the + // blocked host's deadline) is used instead of startNextPicDownload() looping on the + // original host. Keep scanning so one backed-off entry doesn't monopolize the tick. + auto entry = requestLoadQueue.takeAt(i); + --i; + entry.second->scheduleDeferredRetry(host); + continue; + } + // Seed the allowance now so a host that was rate limited gets its reduced + // allowance instead of a fresh full quota 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); - requestLoadQueue.removeAt(i); + auto entry = requestLoadQueue.takeAt(i); + // The allowance is only spent when a request is actually issued: makeRequest() returns + // nullptr when the cached redirect target is in backoff and it hands the entry back. + if (makeRequest(entry.first, entry.second)) { + hostQuotaRemaining.insert(host, allowance - 1); + } return true; } } return false; } +bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url) const +{ + bool useNetworkCache = static_cast( + SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == + CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && + (cache->metaData(url).isValid() || !picDownload); + return !useNetworkCache; +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); 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 2a13e847c..c6a93cb1c 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 @@ -133,6 +133,10 @@ private: /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; + /** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk + * cache. */ + [[nodiscard]] bool requestTouchesNetwork(const QUrl &url) const; + /** @brief Loads redirect cache from disk. */ void loadRedirectCache(); 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..b70207ff4 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 }; +const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() +{ + return s_rateLimiter; +} + CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) @@ -168,7 +173,7 @@ void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply) << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName() << "]: Too many requests from " << host << ", backing off until " << backoffUntil.toString(Qt::ISODate) << ", retrying the same url"; - scheduleDeferredRetry(); + scheduleDeferredRetry(host); } else { qCWarning(CardPictureLoaderWorkerWorkLog).nospace() << "PictureLoader: [card: " << cardToDownload.getCard().getName() @@ -273,14 +278,16 @@ QImage CardPictureLoaderWorkerWork::tryLoadImageFromReply(QNetworkReply *reply) return imgReader.read(); } -void CardPictureLoaderWorkerWork::scheduleDeferredRetry() +void CardPictureLoaderWorkerWork::scheduleDeferredRetry(const QString &preferredHost) { QDateTime now = QDateTime::currentDateTime(); - // Prefer waiting on the current URL's server so we retry the same source. - QString currentHost = QUrl(cardToDownload.getCurrentUrl()).host(); - QDateTime backoffUntil = s_rateLimiter.deadline(currentHost); - if (!s_rateLimiter.isRateLimited(currentHost, now)) { + // Prefer waiting on the server that is actually blocking the request: callers hand in the + // rate-limited host when it differs from the current URL (e.g. a cached redirect target still + // in backoff), otherwise fall back to the current URL's server so we retry the same source. + QString waitHost = preferredHost.isEmpty() ? QUrl(cardToDownload.getCurrentUrl()).host() : preferredHost; + QDateTime backoffUntil = s_rateLimiter.deadline(waitHost); + if (!s_rateLimiter.isRateLimited(waitHost, now)) { backoffUntil = s_rateLimiter.earliestDeadline(now); } 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..b5cd57af8 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 @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -43,6 +44,28 @@ public: CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading + /** @brief Shared per-server 429 backoff state. */ + static const ServerRateLimiter &rateLimiter(); + + /** + * @brief Starts downloading the next URL for this card. + * + * Skips URLs whose server is currently in 429 backoff, either waiting the + * backoff out or falling through to the other configured sources. + */ + void startNextPicDownload(); + + /** + * @brief Schedules a deferred retry after the relevant server backoff expires. + * @param preferredHost The server that is actually blocking the request, or an empty + * string to use the current URL's server + * + * Waits on the blocking server's backoff deadline, otherwise on the earliest active + * backoff. If no servers are in backoff, concludes with failure. Otherwise resets the + * CardPictureToLoad indices and retries after the backoff period. + */ + void scheduleDeferredRetry(const QString &preferredHost = {}); + public slots: /** * @brief Handles a finished network reply for the card image. @@ -55,9 +78,6 @@ private: static ServerRateLimiter s_rateLimiter; ///< Shared per-server 429 backoff state - /** @brief Starts downloading the next URL for this card. */ - void startNextPicDownload(); - /** @brief Called when all URLs have been exhausted or download failed. */ void picDownloadFailed(); @@ -82,16 +102,6 @@ private: */ void concludeImageLoad(const QImage &image); - /** - * @brief Schedules a deferred retry after the relevant server backoff expires. - * - * Waits on the current URL's server when it is the reason we are blocked, - * otherwise on the earliest active backoff. If no servers are in backoff, - * concludes with failure. Otherwise resets the CardPictureToLoad indices and - * retries after the backoff period. - */ - void scheduleDeferredRetry(); - private slots: /** @brief Updates the picDownload setting when it changes. */ void picDownloadChanged(); From 14acf3bf64f5737e956fe784b4fbc97f30c164f4 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:30:03 +0200 Subject: [PATCH 3/8] [PictureLoader] Add user-configurable per-host request caps (#7287) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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. * [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. * [PictureLoader] Cap unlocked host bursts and adapt them to 429s * [PictureLoader] Store per-host limits readably and show them per URL * [PictureLoader] Make dispatch and rate-limit bookkeeping key on the real host Addresses ZeldaZach's round-4 review nits: - Dispatch now resolves the cached-redirect chain before the in-flight gate, so a redirect learned after a URL was queued can no longer bypass the MAX_IN_FLIGHT_PER_HOST cap and drain the whole queue onto the redirect target, which may carry its own developer cap. processSingleRequest does the same so the allowance math keys on the host that is actually hit. - The per-host in-flight slot is released when the reply is destroyed (with the worker as the connection context) rather than on a 'finished' connection bound to the work object, so an aborted reply or a work object deleted while a reply is pending can never permanently shrink the fast path's concurrency. - storeSettings only prunes limits for hosts with neither a URL nor a developer cap, so throttles on redirect targets (api.scryfall.com -> cards.scryfall.io) survive URL removal. - Unlocked hosts are offered 0..UNLOCKED_HOST_LIMIT_MAX (50) in the rate limit dialog, matching clampHostRequestLimit() and the documented hand-editable range, so values written into downloads.ini are no longer silently rewritten on the next edit. --------- Co-authored-by: Lukas Brübach --- .../card_picture_loader_worker.cpp | 176 +++++++++++++++--- .../card_picture_loader_worker.h | 36 +++- .../deck_editor_settings_page.cpp | 158 +++++++++++++++- .../settings_page/deck_editor_settings_page.h | 15 +- .../settings/download_settings.cpp | 70 +++++++ .../settings/download_settings.h | 43 +++++ tests/settings/settings_defaults_test.cpp | 32 ++++ 7 files changed, 496 insertions(+), 34 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 93ce6c96f..765c40ec1 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,14 +16,15 @@ #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()) + : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), + hostRequestLimits(SettingsCache::instance().downloads().getHostRequestLimits()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -73,6 +74,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() @@ -135,8 +139,21 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture QNetworkReply *reply = networkManager->get(req); - // Connect reply handling - connect(reply, &QNetworkReply::finished, worker, [reply, worker] { worker->handleNetworkReply(reply); }); + // Track in-flight replies per host so the unlocked fast path can bound how many requests it + // issues at once, instead of creating replies that time out before Qt opens a connection. + const QString host = url.host(); + hostInFlight.insert(host, hostInFlight.value(host) + 1); + + // Release the in-flight slot when the reply is destroyed, not when it emits `finished`, and use + // the worker (not the work object) as the context object: a reply can go away without ever + // finishing (aborted, or a work object deleted while a reply is still pending), and a connection + // bound to that work object's lifetime would then never run, permanently shrinking the fast + // path's concurrency until it wedges. This way the slot is released exactly once. + connect(reply, &QObject::destroyed, this, + [this, host] { hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1)); }); + + // Connect reply handling; the work object is the context so its handler dies with it. + connect(reply, &QNetworkReply::finished, worker, [worker, reply] { worker->handleNetworkReply(reply); }); return reply; } @@ -144,10 +161,23 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { QDateTime now = QDateTime::currentDateTime(); - for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { + for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end();) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { - it.value() = qMin(MAX_REQUESTS_PER_SEC, it.value() + 1); + if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // A developer-unlocked host that fell back after a 429 recovers towards the default + // allowance; once it gets there it becomes unlocked (fast-path) again. + if (it.value() + 1 >= DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT) { + it = hostRequestQuota.erase(it); + continue; + } + it.value() += 1; + } else { + // 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; } // Forget the per-second allowances; each host's allowance is re-seeded lazily from its @@ -177,8 +207,53 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() return; } - if (!processSingleRequest()) { - // No queued host currently has allowance left in this second; wait for the quota reset. + QDateTime now = QDateTime::currentDateTime(); + bool dispatched = false; + // Set while an unlocked host still has queued work blocked only by the in-flight cap; the + // timer must keep running so it gets another try as soon as a slot frees. A host blocked by + // its 429 backoff instead waits for the next quota-reset tick to restart the dispatcher. + bool unlockedCapped = false; + + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the pacing and the per-host + // allowance: dispatch their queued requests back-to-back, bounded by their 429 backoff and the + // per-host in-flight cap so a large burst can't queue replies that time out before Qt opens a + // connection for them. + for (int i = 0; i < requestLoadQueue.size();) { + const auto &request = requestLoadQueue.at(i); + // Dispatch decisions must key on the host the request will actually go to, not the URL that + // merely redirects to it: a redirect learned after this URL was queued would otherwise + // bypass the in-flight cap and drain the whole queue onto the target host unchecked. + const QUrl resolvedUrl = resolveCachedRedirect(request.first); + const QString host = resolvedUrl.host(); + if (isUnlockedHost(host)) { + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + ++i; + continue; + } + if (hostInFlight.value(host) < MAX_IN_FLIGHT_PER_HOST) { + makeRequest(resolvedUrl, request.second); + requestLoadQueue.removeAt(i); + dispatched = true; + continue; + } + unlockedCapped = true; + } + ++i; + } + + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + requestTimer.stop(); + return; + } + + if (processSingleRequest()) { + dispatched = true; + } + + // Keep the timer running while there is progress to make or unlocked work waiting on a free + // in-flight slot; otherwise no host has allowance left this second, so wait for the quota reset. + if (!dispatched && !unlockedCapped) { dispatchTimer.stop(); } } @@ -221,8 +296,12 @@ bool CardPictureLoaderWorker::processSingleRequest() { QDateTime now = QDateTime::currentDateTime(); for (int i = 0; i < requestLoadQueue.size(); ++i) { - const auto &request = requestLoadQueue.at(i); - const QString host = request.first.host(); + // Copy the entry: takeAt(i) below erases within the list this reference points into. + const auto request = requestLoadQueue.at(i); + // Resolve cached redirects so the rate-limit and allowance arithmetic keys on the host the + // request will actually hit (see resolveCachedRedirect). + const QUrl resolvedUrl = resolveCachedRedirect(request.first); + const QString host = resolvedUrl.host(); // Don't dispatch requests to a host that is currently in its 429 backoff; hand the entry // back to its worker so it can wait the backoff out or fall through to another source, // instead of leaving it parked in the queue with no reply pending. Only applies to @@ -233,25 +312,39 @@ bool CardPictureLoaderWorker::processSingleRequest() // The queued URL is usually a cached-redirect target whose host differs from // cardToDownload.getCurrentUrl(), so scheduleDeferredRetry() (which waits out the // blocked host's deadline) is used instead of startNextPicDownload() looping on the - // original host. Keep scanning so one backed-off entry doesn't monopolize the tick. + // original host. auto entry = requestLoadQueue.takeAt(i); - --i; - entry.second->scheduleDeferredRetry(host); + if (host != entry.first.host()) { + // A cached redirect target is what is blocked, which the work object would not + // discover from its own URL; wait out that specific host (with jitter) instead. + entry.second->scheduleDeferredRetry(host); + } else { + entry.second->startNextPicDownload(); + } + return true; + } + // Unlocked hosts are handled by dispatchQueuedRequest's fast path, bounded by the in-flight + // cap; they must not fall through to the per-host allowance arithmetic below. + if (isUnlockedHost(host)) { continue; } - // Seed the allowance now so a host that was rate limited gets its reduced - // allowance instead of a fresh full quota mid-second. + int ceiling = hostAllowanceCeiling(host); + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // A 429 dropped this unlocked host out of the fast path and installed a concrete + // allowance; pace it against that allowance until the recovery loop unlocks it again. + ceiling = hostRequestQuota.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + } + // Seed the allowance lazily so a host that enters the queue mid-second gets its reduced + // per-host allowance, clamped against the ceiling so a lowered user cap applies from this + // second onward. if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + hostQuotaRemaining.insert(host, qMin(ceiling, hostRequestQuota.value(host, ceiling))); } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { + hostQuotaRemaining.insert(host, allowance - 1); auto entry = requestLoadQueue.takeAt(i); - // The allowance is only spent when a request is actually issued: makeRequest() returns - // nullptr when the cached redirect target is in backoff and it hands the entry back. - if (makeRequest(entry.first, entry.second)) { - hostQuotaRemaining.insert(host, allowance - 1); - } + makeRequest(resolvedUrl, entry.second); return true; } } @@ -267,9 +360,30 @@ bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url) const return !useNetworkCache; } +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); +} + +bool CardPictureLoaderWorker::isUnlockedHost(const QString &host) const +{ + return hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestQuota.contains(host); +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); + const int ceiling = hostAllowanceCeiling(host); + // An unlocked host has no per-host allowance to halve. Install one instead so it drops out of + // the unlocked fast path and is paced like a throttled host; the recovery loop in + // resetRequestQuota() then walks it back up and unlocks it again. + const int base = + ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : ceiling; + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, base) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } @@ -327,6 +441,22 @@ QUrl CardPictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const return {}; } +QUrl CardPictureLoaderWorker::resolveCachedRedirect(const QUrl &url) const +{ + // Follow the whole cached-redirect chain so dispatch keys on the host that is really hit. The + // depth bound keeps a corrupt or self-referencing cache entry from spinning us forever. + QUrl resolved = url; + int depth = 0; + while (depth++ < MAX_REDIRECT_CHAIN_DEPTH) { + QUrl target = getCachedRedirect(resolved); + if (target.isEmpty() || target == resolved) { + break; + } + resolved = target; + } + return resolved; +} + void CardPictureLoaderWorker::loadRedirectCache() { QSettings settings(cacheFilePath, QSettings::IniFormat); 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 c6a93cb1c..dd551a2dc 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 @@ -124,19 +124,53 @@ 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 + QHash hostInFlight; ///< Network replies currently in flight, per host + + /** @brief Maximum concurrent in-flight network replies per host. */ + static constexpr int MAX_IN_FLIGHT_PER_HOST = 6; + + /** @brief Bound on how many cached-redirect hops dispatch resolution will follow. */ + static constexpr int MAX_REDIRECT_CHAIN_DEPTH = 10; 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 Whether a host may skip dispatch pacing and per-host allowance entirely. + * + * A host is unlocked while it has no user limit and no reduced allowance installed by a 429. + * A 429 drops it out of the fast path until resetRequestQuota() walks the allowance back up. + */ + [[nodiscard]] bool isUnlockedHost(const QString &host) const; + /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; - /** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk +/** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk * cache. */ [[nodiscard]] bool requestTouchesNetwork(const QUrl &url) const; + /** + * @brief Follows the cached-redirect chain to the URL that will actually be requested. + * @param url The URL to resolve + * @return The final URL after chasing cached redirects, or @p url itself if none lead elsewhere + * + * Dispatch decisions (unlocked-host fast path, 429 backoff, in-flight cap) must key on the host + * a request really goes to, not the URL that merely redirects to it. + */ + [[nodiscard]] QUrl resolveCachedRedirect(const QUrl &url) const; + /** @brief Loads redirect cache from disk. */ void loadRedirectCache(); 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..bdbb38c95 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,7 +10,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -51,7 +53,9 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() urlList->setDragDropMode(QAbstractItemView::InternalMove); connect(urlList->model(), &QAbstractItemModel::rowsMoved, this, &DeckEditorSettingsPage::urlListChanged); - urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); + for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { + addUrlItem(url); + } aAdd = new QAction(this); aAdd->setIcon(themePixmap(QStringLiteral("icons/increment"))); @@ -65,11 +69,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; @@ -117,7 +126,9 @@ void DeckEditorSettingsPage::resetDownloadedURLsButtonClicked() { SettingsCache::instance().downloads().resetToDefaultURLs(); urlList->clear(); - urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); + for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { + addUrlItem(url); + } QMessageBox::information(this, tr("Success"), tr("Download URLs have been reset.")); } @@ -126,7 +137,7 @@ void DeckEditorSettingsPage::actAddURL() bool ok; QString msg = QInputDialog::getText(this, tr("Add URL"), tr("URL:"), QLineEdit::Normal, QString(), &ok); if (ok) { - urlList->addItem(msg); + addUrlItem(msg); storeSettings(); } } @@ -141,12 +152,14 @@ void DeckEditorSettingsPage::actRemoveURL() void DeckEditorSettingsPage::actEditURL() { - if (urlList->currentItem()) { - QString oldText = urlList->currentItem()->text(); + QListWidgetItem *item = urlList->currentItem(); + if (item) { + const QString oldText = urlForItem(item); bool ok; QString msg = QInputDialog::getText(this, tr("Edit URL"), tr("URL:"), QLineEdit::Normal, oldText, &ok); if (ok) { - urlList->currentItem()->setText(msg); + item->setData(Qt::UserRole, msg); + item->setText(urlLabel(msg)); storeSettings(); } } @@ -158,10 +171,133 @@ void DeckEditorSettingsPage::storeSettings() QStringList downloadUrls; for (int i = 0; i < urlList->count(); i++) { - qInfo() << "Priority" << i << ":" << urlList->item(i)->text(); - downloadUrls << urlList->item(i)->text(); + const QString url = urlForItem(urlList->item(i)); + qInfo() << "Priority" << i << ":" << url; + downloadUrls << url; } SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); + + // Drop per-host limits whose host is no longer referenced by any configured URL, so removing + // a URL doesn't leave a stale throttle behind that reactivates if the host is re-added. + QSet usedHosts; + for (const QString &url : downloadUrls) { + const QString host = QUrl(url).host(); + if (!host.isEmpty()) { + usedHosts.insert(host); + } + } + QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); + bool limitsChanged = false; + for (auto it = limits.begin(); it != limits.end();) { + // Prune only limits for hosts that are neither referenced by a configured URL nor carry a + // developer cap. Capped hosts are often redirect targets (e.g. api.scryfall.com redirects + // to cards.scryfall.io) that never appear in the URL list, yet they are exactly the hosts + // the throttle applies to, so dropping them when a URL is removed would silently re-enable + // free-running traffic to a rate-sensitive server. + if (!usedHosts.contains(it.key()) && !DownloadSettings::getDeveloperHostCaps().contains(it.key())) { + it = limits.erase(it); + limitsChanged = true; + } else { + ++it; + } + } + if (limitsChanged) { + SettingsCache::instance().downloads().setHostRequestLimits(limits); + } + + refreshUrlItems(); +} + +QListWidgetItem *DeckEditorSettingsPage::addUrlItem(const QString &url) +{ + auto *item = new QListWidgetItem(urlLabel(url)); + item->setData(Qt::UserRole, url); + urlList->addItem(item); + return item; +} + +QString DeckEditorSettingsPage::urlForItem(const QListWidgetItem *item) const +{ + return item->data(Qt::UserRole).toString(); +} + +QString DeckEditorSettingsPage::urlLabel(const QString &url) const +{ + const QString host = QUrl(url).host(); + if (host.isEmpty()) { + return url; + } + + const QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); + const int devCap = + DownloadSettings::getDeveloperHostCaps().value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !limits.contains(host)) { + return tr("%1 (unlimited)").arg(url); + } + + const int requested = limits.value( + host, devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : devCap); + const int effective = SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); + return tr("%1 (%2/s)").arg(url).arg(effective); +} + +void DeckEditorSettingsPage::refreshUrlItems() +{ + for (int i = 0; i < urlList->count(); ++i) { + QListWidgetItem *item = urlList->item(i); + item->setText(urlLabel(urlForItem(item))); + } +} + +void DeckEditorSettingsPage::actAdjustRateLimit() +{ + if (urlList->currentItem() == nullptr) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("Select a URL in the list first.")); + return; + } + + const QString host = QUrl(urlForItem(urlList->currentItem())).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; + QString prompt; + if (unlocked) { + minimum = 0; // 0 means "unlimited" + maximum = DownloadSettings::UNLOCKED_HOST_LIMIT_MAX; + defaultValue = currentLimits.value(host, 0); + prompt = tr("Requests per second (0 = unlimited, fastest; up to %1):").arg(maximum); + } else { + minimum = DownloadSettings::MIN_HOST_REQUEST_LIMIT; + maximum = devCap; + defaultValue = currentLimits.value(host, devCap); + prompt = tr("Requests per second (developer maximum is %1):").arg(maximum); + } + + const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), prompt, 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); + refreshUrlItems(); } void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) @@ -244,4 +380,8 @@ 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")); + + // The per-URL rate limit suffixes are translated, so refresh them when the language changes. + refreshUrlItems(); +} 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..23e33fa20 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 @@ -27,6 +27,7 @@ private slots: void actAddURL(); void actRemoveURL(); void actEditURL(); + void actAdjustRateLimit(); void resetDownloadedURLsButtonClicked(); private: @@ -34,7 +35,7 @@ private: QLabel urlLinkLabel; QCheckBox picDownloadCheckBox; QListWidget *urlList; - QAction *aAdd, *aEdit, *aRemove; + QAction *aAdd, *aEdit, *aRemove, *aRateLimit; QCheckBox mcDownloadSpoilersCheckBox; QLabel msDownloadSpoilersLabel; QGroupBox *mpGeneralGroupBox; @@ -46,6 +47,18 @@ private: QLabel infoOnSpoilersLabel; QPushButton *mpSpoilerPathButton; QPushButton *updateNowButton; + + /** @brief Adds a list item for the given URL, storing the raw URL alongside its displayed label. */ + QListWidgetItem *addUrlItem(const QString &url); + + /** @brief Returns the raw URL stored on a list item. */ + [[nodiscard]] QString urlForItem(const QListWidgetItem *item) const; + + /** @brief Returns the display label for a URL, including its current effective rate limit. */ + [[nodiscard]] QString urlLabel(const QString &url) const; + + /** @brief Refreshes the displayed label of every URL item after limits or settings change. */ + void refreshUrlItems(); }; #endif // COCKATRICE_DECK_EDITOR_SETTINGS_PAGE_H diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index eb73e58ee..6e22a84f4 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -12,6 +12,22 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { const QString DownloadSettings::SCRYFALL_NAMED_LOCALIZED_URL = "https://api.scryfall.com/cards/named?fuzzy=!localizedName!&lang=!sflang!&format=image&face=!prop:side!"; +// 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 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}, + // 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) { @@ -65,3 +81,57 @@ void DownloadSettings::setDownloadSpoilerStatus(bool _spoilerStatus) setValue(_spoilerStatus, "downloadSpoilers"); emit downloadSpoilerStatusChanged(); } + +QHash DownloadSettings::getHostRequestLimits() const +{ + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { + settings.beginGroup(defaultGroup); + } + settings.beginGroup("hostRequestLimits"); + + QHash hostRequestLimits; + const QStringList hosts = settings.childKeys(); + for (const QString &host : hosts) { + hostRequestLimits.insert(host, settings.value(host).toInt()); + } + + settings.endGroup(); + if (!defaultGroup.isEmpty()) { + settings.endGroup(); + } + return hostRequestLimits; +} + +void DownloadSettings::setHostRequestLimits(const QHash &hostRequestLimits) +{ + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { + settings.beginGroup(defaultGroup); + } + + // Drop the legacy single-key form (an opaque @Variant blob) written by earlier builds so each + // host is stored as a plain, hand-editable key in its own subgroup. + settings.remove("hostRequestLimits"); + settings.beginGroup("hostRequestLimits"); + settings.remove(QString()); + for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { + settings.setValue(it.key(), it.value()); + } + settings.endGroup(); + + if (!defaultGroup.isEmpty()) { + settings.endGroup(); + } + settings.sync(); + 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 ae49884f0..7eca77df6 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -9,6 +9,8 @@ #include "settings_manager.h" +#include + class DownloadSettings : public SettingsManager { Q_OBJECT @@ -16,8 +18,35 @@ class DownloadSettings : public SettingsManager static const QStringList DEFAULT_DOWNLOAD_URLS; static const QString SCRYFALL_NAMED_LOCALIZED_URL; + 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 Upper bound offered to the user when lowering an unlocked host's allowance. + * + * Unlocked hosts have no developer cap, so `clampHostRequestLimit` puts no upper bound on + * them; this only bounds what the settings dialog offers, and matches the widest paced + * allowance a user is documented to be able to hand-edit in `downloads.ini`. Choosing 0 + * below this restores the "unlimited" fast path. + */ + static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; + /** @brief Developer cap marking a host as never throttled per host or by the dispatch pacing. */ + 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; @@ -29,9 +58,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 6e674cb86..87b21f015 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 9f66a098ac3ef41ac27e142d1aa6afffbe20d9d5 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:30:03 +0200 Subject: [PATCH 4/8] [PictureLoader] Fix worker thread shutdown and cross-thread cache clearing (#7292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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. * [PictureLoader] Guard cache teardown and stop blocking the UI thread --------- Co-authored-by: Lukas Brübach --- .../card_picture_loader.cpp | 27 +++++++++++++-- .../card_picture_loader/card_picture_loader.h | 7 ++++ .../card_picture_loader_worker.cpp | 33 +++++++++++++++++- .../card_picture_loader_worker.h | 34 +++++++++++++++++-- .../card_picture_loader_worker_work.cpp | 4 +-- .../card_picture_loader_worker_work.h | 5 +-- .../settings_page/storage_settings_page.cpp | 9 +++-- 7 files changed, 108 insertions(+), 11 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 b8a54761a..8e34942e1 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 @@ -43,6 +44,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) qRegisterMetaType("ExactCard"); connect(worker, &CardPictureLoaderWorker::imageLoaded, this, &CardPictureLoader::imageLoaded); + connect(worker, &CardPictureLoaderWorker::networkCacheCleared, this, &CardPictureLoader::networkCacheCleared); statusBar = new CardPictureLoaderStatusBar(nullptr); QMainWindow *mainWindow = qobject_cast(QApplication::activeWindow()); @@ -58,7 +60,18 @@ 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(); + const bool stopped = worker->shutdownThread(); + worker = nullptr; + // Deleting a QThread that is still running is undefined behaviour, so only free it once the + // bounded wait in shutdownThread() confirmed that it stopped. + if (stopped) { + delete pictureLoaderThread; + } + } } void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size) @@ -457,7 +470,17 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - getInstance().worker->clearNetworkCache(); + // During teardown the worker is released before this singleton, so a queued clear may still + // arrive with no worker left to run it. + CardPictureLoaderWorker *worker = getInstance().worker; + if (!worker) { + return; + } + // The disk cache and redirect cache are owned by the worker thread, so the clear has to run + // there. Invoke it asynchronously to keep the GUI responsive while the worker may be walking + // the user's picture directories or recursively deleting the cache directory; callers that + // need to know when it is done can listen for networkCacheCleared(). + QMetaObject::invokeMethod(worker, &CardPictureLoaderWorker::clearNetworkCache, Qt::QueuedConnection); } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h index 0a4934e6d..6224befbf 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h @@ -117,6 +117,9 @@ public: public slots: /** * @brief Clears the network disk cache of the worker. + * + * The clear runs on the worker thread, so this returns before it has completed; connect to + * networkCacheCleared() to act once it is done. */ static void clearNetworkCache(); @@ -131,6 +134,10 @@ public slots: void installPrintingOverride(const ExactCard &originalCard, const ExactCard &overrideCard); void installPrintingOverrideOnLoad(const ExactCard &originalCard, const ExactCard &overrideCard); +signals: + /** @brief Emitted after the worker has finished clearing the network and redirect caches. */ + void networkCacheCleared(); + private slots: /** * @brief Triggered when the user changes the picture download settings. 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 765c40ec1..0fd6a42d5 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 @@ -21,6 +21,7 @@ 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 +static constexpr int THREAD_SHUTDOWN_WAIT_MS = 5000; ///< Bounded wait for the worker thread to stop at exit CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), @@ -59,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); @@ -82,7 +86,33 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() CardPictureLoaderWorker::~CardPictureLoaderWorker() { saveRedirectCache(); - pictureLoaderThread->deleteLater(); +} + +bool 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) { + return true; + } + thread->quit(); + // Only an unbounded wait() would guarantee the thread stops, but this runs from a function-local + // static destructor after main() has returned, with no UI left to interrupt a worker stuck in a + // slow slot or on a stalled filesystem. Bound the wait and leave such a thread to the OS rather + // than hanging the process forever. + if (!thread->wait(THREAD_SHUTDOWN_WAIT_MS)) { + qCWarning(CardPictureLoaderWorkerLog) << "Picture loader worker thread did not stop within" + << THREAD_SHUTDOWN_WAIT_MS << "ms; leaving it to be torn down by the OS"; + return false; + } + return true; +} + +QThread *CardPictureLoaderWorker::workerThread() const +{ + return pictureLoaderThread; } void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) @@ -509,4 +539,5 @@ void CardPictureLoaderWorker::clearNetworkCache() { networkManager->cache()->clear(); redirectCache.clear(); + emit networkCacheCleared(); } 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 dd551a2dc..241194555 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 reports whether it stopped. + * + * 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 (bounded) 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. + * + * @return true if the thread stopped within the timeout, false if it is still running (in + * which case the owner must not delete the QThread). + */ + [[nodiscard]] bool shutdownThread(); + + /** + * @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 @@ -202,6 +229,9 @@ signals: /** @brief Emitted when a network request successfully completes. */ void imageRequestSucceeded(const QUrl &url); + + /** @brief Emitted after clearNetworkCache() has finished clearing both caches. */ + void networkCacheCleared(); }; #endif // PICTURE_LOADER_WORKER_H 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 b70207ff4..19c6be469 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 @@ -27,8 +27,8 @@ const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() return s_rateLimiter; } -CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) - : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), +CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad) + : QObject(worker), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) { // Hook up signals to the orchestrator 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 b5cd57af8..84a7dafdb 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 @@ -37,10 +37,11 @@ class CardPictureLoaderWorkerWork : public QObject public: /** * @brief Constructs a worker for downloading a specific card image. - * @param worker The orchestrating CardPictureLoaderWorker + * @param worker The orchestrating CardPictureLoaderWorker; the work object becomes its child so + * it is destroyed with the worker even if it never reaches concludeImageLoad(). * @param toLoad The ExactCard to download */ - explicit CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad); + explicit CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad); CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading diff --git a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp index 17838e501..4aad91115 100644 --- a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp @@ -181,9 +181,14 @@ StorageSettingsPage::StorageSettingsPage() void StorageSettingsPage::clearDownloadedPicsButtonClicked() { - CardPictureLoader::clearNetworkCache(); + // The network cache is cleared asynchronously on the worker thread, so wait for the completion + // signal before confirming; the in-memory pixmap cache is cleared synchronously right away. + connect( + &CardPictureLoader::getInstance(), &CardPictureLoader::networkCacheCleared, this, + [this] { QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); }, + Qt::SingleShotConnection); CardPictureLoader::clearPixmapCache(); - QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); + CardPictureLoader::clearNetworkCache(); } void StorageSettingsPage::clearImageBackupsButtonClicked() From e01912d3c85685739583dcf39f3464f179bc33ad Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:30:04 +0200 Subject: [PATCH 5/8] [Tests] Add manual picture loader benchmark against the real card hosts (#7288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * tests: add manual picture loader benchmark against the real card hosts (cherry picked from commit 588d71d3b9a8278ab98b847802908f322c1035b7) * 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. --------- Co-authored-by: Lukas Brübach --- .../src/client/settings/cache_settings.h | 54 +- .../card_picture_loader_worker.h | 2 +- tests/CMakeLists.txt | 6 + tests/picture_loader_benchmark/CMakeLists.txt | 26 + .../picture_loader_benchmark.cpp | 547 ++++++++++++++++++ .../settings_cache_mock.cpp | 200 +++++++ 6 files changed, 807 insertions(+), 28 deletions(-) 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/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/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 241194555..8b136f309 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 @@ -184,7 +184,7 @@ private: /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; -/** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk + /** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk * cache. */ [[nodiscard]] bool requestTouchesNetwork(const QUrl &url) const; 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..f991f2ec0 --- /dev/null +++ b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp @@ -0,0 +1,547 @@ +/* + * 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 +#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 std::atomic s_activeCounters{nullptr}; +static QtMessageHandler s_previousMessageHandler = nullptr; + +static void stressLogHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg) +{ + 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; + } +} + +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; + + // 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.store(nullptr, std::memory_order_release); + 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