From 91519166f9de65db162908ca81a7e93667b32bae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 03:55:45 +0200 Subject: [PATCH 1/3] [PictureLoader] Guard dispatch timer restarts and drop dead request quota --- .../card_picture_loader_worker.cpp | 22 +++++++++++-------- .../card_picture_loader_worker.h | 3 +-- 2 files changed, 14 insertions(+), 11 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..b29e838ff 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 @@ -23,8 +23,7 @@ static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between indi 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 @@ -137,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) { @@ -156,27 +153,34 @@ void CardPictureLoaderWorker::resetRequestQuota() void CardPictureLoaderWorker::processQueuedRequests() { + Q_ASSERT(thread() == QThread::currentThread()); + if (requestLoadQueue.isEmpty()) { dispatchTimer.stop(); + requestTimer.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(); + // Restarting an active timer would reset the pacing countdown, so a burst of enqueues could + // keep starving the dispatcher; only start it when it has actually stopped. + if (!dispatchTimer.isActive()) { + dispatchTimer.start(); + } } void CardPictureLoaderWorker::dispatchQueuedRequest() { - if (requestLoadQueue.isEmpty() || requestQuota <= 0) { + if (requestLoadQueue.isEmpty()) { + // All queued requests have been dispatched; stop the pacing and quota-reset timers. dispatchTimer.stop(); + requestTimer.stop(); return; } - if (processSingleRequest()) { - --requestQuota; - } else { + if (!processSingleRequest()) { // 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 9f7fd9437..f0fe17976 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,7 +86,7 @@ public slots: */ QNetworkReply *makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *workThread); - /** @brief Processes all queued requests respecting the request quota. */ + /** @brief Starts the pacing timers if there is queued work, stops them when the queue is empty. */ void processQueuedRequests(); /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ @@ -121,7 +121,6 @@ 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 From 5956dcab8315b832d09c13e8dc312e364fb1d8df 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 2/3] [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 | 23 +++++++++++++------ .../card_picture_loader_worker_work.cpp | 5 ++++ .../card_picture_loader_worker_work.h | 3 +++ 3 files changed, 24 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 b29e838ff..91a984dea 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 @@ -136,6 +136,10 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { + // 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) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { @@ -143,11 +147,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(); } @@ -188,10 +187,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 310caa7dc0694f3cddc4a73f8d53b532440fc827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 04:21:09 +0200 Subject: [PATCH 3/3] [PictureLoader] Hand backed-off requests back to their worker instead of parking them --- .../card_picture_loader_worker.cpp | 25 +++++++++---- .../card_picture_loader_worker_work.cpp | 2 +- .../card_picture_loader_worker_work.h | 35 +++++++++++-------- 3 files changed, 41 insertions(+), 21 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 91a984dea..2925b3af9 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 @@ -108,6 +108,14 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture QUrl cachedRedirect = getCachedRedirect(url); if (!cachedRedirect.isEmpty()) { 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(); + return nullptr; + } return makeRequest(cachedRedirect, worker); } @@ -136,8 +144,9 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { - // Allowances are seeded per host on demand in processSingleRequest(), so a - // rate-limited host never gets a fresh full quota mid-second. + // Allowances are seeded lazily per host in processSingleRequest() when a request is first + // looked at in a new second, so a host that enters the queue mid-second now gets its reduced + // per-host allowance instead of falling through to the full per-second default. hostQuotaRemaining.clear(); QDateTime now = QDateTime::currentDateTime(); @@ -191,12 +200,16 @@ bool CardPictureLoaderWorker::processSingleRequest() for (int i = 0; i < requestLoadQueue.size(); ++i) { const auto &request = requestLoadQueue.at(i); const QString host = request.first.host(); - // Don't dispatch requests to a host that is currently in its 429 backoff. + // 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. if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { - continue; + requestLoadQueue.removeAt(i); + request.second->startNextPicDownload(); + 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. + // 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)); } 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 072a919d7..e4a36ab8c 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,7 +22,7 @@ static const QStringList MD5_BLACKLIST = { "fbc7d763c08771c260b39e2115414eeb" // Current card back hash }; -ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() +const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() { return s_rateLimiter; } 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 8490cb3ac..f05d727de 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 @@ -44,7 +44,27 @@ public: CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading /** @brief Shared per-server 429 backoff state. */ - static ServerRateLimiter &rateLimiter(); + 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. Also used by + * the dispatch machinery to hand an entry back after it was removed from the + * request queue when its host turned out to be backed off. + */ + void startNextPicDownload(); + + /** + * @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(); public slots: /** @@ -58,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(); @@ -85,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();