[PictureLoader] Schedule exponential backoff on 429 - Too many request handler and call failed on fail (#7053)

Took 16 minutes

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp

@@ -26,6 +26,9 @@
 // never cache more than 300 cards at once for a single deck
 #define CACHED_CARD_PER_DECK_MAX 300
 
+// wait at least this long before retrying a card whose picture failed to load
+static constexpr int RETRY_FAILED_CARDS_SECS = 300;
+
 CardPictureLoader::CardPictureLoader() : QObject(nullptr)
 {
     worker = new CardPictureLoaderWorker;
@@ -135,7 +138,14 @@ void CardPictureLoader::getPixmap(QPixmap &pixmap, const ExactCard &card, QSize
     QPixmap bigPixmap;
     if (QPixmapCache::find(key, &bigPixmap)) {
         if (bigPixmap.isNull()) {
-            qCDebug(CardPictureLoaderLog) << "Cached pixmap for key" << key << "is NULL!";
+            getCardBackLoadingFailedPixmap(pixmap, size);
+            QDateTime failedAtTime = getInstance().failedAt.value(key);
+            if (!failedAtTime.isValid() ||
+                failedAtTime.addSecs(RETRY_FAILED_CARDS_SECS) < QDateTime::currentDateTime()) {
+                getInstance().failedAt.remove(key);
+                QPixmapCache::remove(key);
+                getInstance().worker->enqueueImageLoad(card);
+            }
             return;
         }
 
@@ -159,8 +169,10 @@ void CardPictureLoader::imageLoaded(const ExactCard &card, const QImage &image)
     QPixmap finalPixmap;
 
     if (image.isNull()) {
+        getInstance().failedAt.insert(card.getPixmapCacheKey(), QDateTime::currentDateTime());
         qCDebug(CardPictureLoaderLog) << "Caching NULL pixmap for" << card.getName();
     } else {
+        getInstance().failedAt.remove(card.getPixmapCacheKey());
         if (card.getInfo().getUiAttributes().upsideDownArt) {
 #if (QT_VERSION >= QT_VERSION_CHECK(6, 9, 0))
             QImage mirrorImage = image.flipped(Qt::Horizontal | Qt::Vertical);
@@ -184,8 +196,10 @@ void CardPictureLoader::imageLoaded(const ExactCard &card, const QImage &image)
     // imageLoaded should only be reached if the exactCard isn't already in cache.
     // (plus there's a deduplication mechanism in CardPictureLoaderWorker)
     // It should be safe to connect the CardInfo here without worrying about redundant connections.
-    connect(card.getCardPtr().data(), &QObject::destroyed, this,
-            [cacheKey = card.getPixmapCacheKey()] { QPixmapCache::remove(cacheKey); });
+    connect(card.getCardPtr().data(), &QObject::destroyed, this, [cacheKey = card.getPixmapCacheKey()] {
+        QPixmapCache::remove(cacheKey);
+        getInstance().failedAt.remove(cacheKey);
+    });
 
     card.emitPixmapUpdated();
 }

cockatrice/src/interface/card_picture_loader/card_picture_loader.h

@@ -4,6 +4,8 @@
 #include "card_picture_loader_status_bar.h"
 #include "card_picture_loader_worker.h"
 
+#include <QDateTime>
+#include <QHash>
 #include <QLoggingCategory>
 
 inline Q_LOGGING_CATEGORY(CardPictureLoaderLog, "card_picture_loader");
@@ -56,6 +58,7 @@ private:
 
     CardPictureLoaderWorker *worker;       ///< Worker thread for async image loading
     CardPictureLoaderStatusBar *statusBar; ///< Status bar widget showing load progress
+    QHash<QString, QDateTime> failedAt;    ///< Timestamp of the last failed load attempt per pixmap cache key
 
 public:
     /**

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp

@@ -17,6 +17,8 @@
 #include <version_string.h>
 
 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
 
 CardPictureLoaderWorker::CardPictureLoaderWorker()
     : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()),
@@ -124,6 +126,19 @@ 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) {
+            it.value() = qMin(MAX_REQUESTS_PER_SEC, it.value() + 1);
+        }
+    }
+
+    for (const auto &request : requestLoadQueue) {
+        const QString host = request.first.host();
+        hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC));
+    }
+
     processQueuedRequests();
 }
 
@@ -136,14 +151,26 @@ void CardPictureLoaderWorker::processQueuedRequests()
 
 bool CardPictureLoaderWorker::processSingleRequest()
 {
-    if (!requestLoadQueue.isEmpty()) {
-        auto request = requestLoadQueue.takeFirst();
-        makeRequest(request.first, request.second);
-        return true;
+    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);
+        if (allowance > 0) {
+            hostQuotaRemaining.insert(host, allowance - 1);
+            makeRequest(request.first, request.second);
+            requestLoadQueue.removeAt(i);
+            return true;
+        }
     }
     return false;
 }
 
+void CardPictureLoaderWorker::onHostRateLimited(const QString &host)
+{
+    hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2));
+    hostLast429.insert(host, QDateTime::currentDateTime());
+}
+
 void CardPictureLoaderWorker::enqueueImageLoad(const ExactCard &card)
 {
     // Send call through a connection to ensure the handling is run on the pictureLoader thread

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h

@@ -5,6 +5,8 @@
 #include "card_picture_loader_worker_work.h"
 #include "card_picture_to_load.h"
 
+#include <QDateTime>
+#include <QHash>
 #include <QLoggingCategory>
 #include <QMutex>
 #include <QNetworkAccessManager>
@@ -66,6 +68,12 @@ public:
      */
     void queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker);
 
+    /**
+     * @brief Handles a server returning HTTP 429 by reducing that host's request quota.
+     * @param host The host that returned 429
+     */
+    void onHostRateLimited(const QString &host);
+
     /** @brief Clears the network cache and redirect cache. */
     void clearNetworkCache();
 
@@ -110,8 +118,11 @@ private:
     bool picDownload;                                  ///< Whether downloading images from network is enabled
     QQueue<QPair<QUrl, CardPictureLoaderWorkerWork *>> requestLoadQueue; ///< Queue of pending network requests
 
-    int requestQuota;    ///< Remaining requests allowed per second
-    QTimer requestTimer; ///< Timer to reset the request quota
+    int requestQuota;                       ///< Remaining requests allowed per second
+    QTimer requestTimer;                    ///< Timer to reset the request quota
+    QHash<QString, int> hostRequestQuota;   ///< Sustained per-host request allowance
+    QHash<QString, int> hostQuotaRemaining; ///< Per-host allowance left in the current second
+    QHash<QString, QDateTime> hostLast429;  ///< When each host was last rate limited
 
     CardPictureLoaderLocal *localLoader; ///< Loader for local images
     QSet<QString> currentlyLoading;      ///< Deduplication: contains pixmapCacheKey currently being loaded

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp

@@ -8,8 +8,12 @@
 #include <QLoggingCategory>
 #include <QMovie>
 #include <QNetworkReply>
+#include <QRandomGenerator>
 #include <QThread>
 #include <QThreadPool>
+#include <QTimer>
+
+ServerRateLimiter CardPictureLoaderWorkerWork::s_rateLimiter;
 #include <libcockatrice/settings/download_settings.h>
 
 // Card back returned by gatherer when card is not found
@@ -30,6 +34,7 @@ CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoader
     connect(this, &CardPictureLoaderWorkerWork::imageLoaded, worker, &CardPictureLoaderWorker::handleImageLoaded);
     connect(this, &CardPictureLoaderWorkerWork::requestSucceeded, worker,
             &CardPictureLoaderWorker::imageRequestSucceeded);
+    connect(this, &CardPictureLoaderWorkerWork::rateLimited, worker, &CardPictureLoaderWorker::onHostRateLimited);
 
     // Hook up signals to settings
     connect(&SettingsCache::instance().downloads(), SIGNAL(picDownloadChanged()), this, SLOT(picDownloadChanged()));
@@ -39,10 +44,38 @@ CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoader
 
 void CardPictureLoaderWorkerWork::startNextPicDownload()
 {
+    QDateTime now = QDateTime::currentDateTime();
+    while (!cardToDownload.getCurrentUrl().isEmpty() &&
+           s_rateLimiter.isRateLimited(QUrl(cardToDownload.getCurrentUrl()).host(), now)) {
+        QString host = QUrl(cardToDownload.getCurrentUrl()).host();
+        if (s_rateLimiter.rounds(host) == 1) {
+            // First 429 round for this server: wait out the backoff and give it
+            // one more chance instead of immediately falling through to a worse
+            // source. A second 429 makes us fall through instead.
+            qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
+                << "PictureLoader: [card: " << cardToDownload.getCard().getInfo().getCorrectedName()
+                << " set: " << cardToDownload.getSetName() << "]: Waiting out backoff for " << host << " to retry "
+                << cardToDownload.getCurrentUrl();
+            scheduleDeferredRetry();
+            return;
+        }
+
+        // The server has already 429'd us at least twice, so further retries are
+        // unlikely to succeed: move on to the other configured sources.
+        qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
+            << "PictureLoader: [card: " << cardToDownload.getCard().getInfo().getCorrectedName()
+            << " set: " << cardToDownload.getSetName() << "]: Skipping rate-limited URL "
+            << cardToDownload.getCurrentUrl() << " (server " << host << " still rate limiting)";
+        if (!cardToDownload.nextUrl() && !cardToDownload.nextSet()) {
+            scheduleDeferredRetry();
+            return;
+        }
+    }
+
     QString picUrl = cardToDownload.getCurrentUrl();
 
     if (picUrl.isEmpty()) {
-        picDownloadFailed();
+        scheduleDeferredRetry();
     } else {
         QUrl url(picUrl);
         qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
@@ -108,7 +141,41 @@ static bool imageIsBlackListed(const QByteArray &picData)
 void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply)
 {
     if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 429) {
-        qCWarning(CardPictureLoaderWorkerWorkLog) << "Too many requests.";
+        QString host = reply->url().host();
+        QDateTime now = QDateTime::currentDateTime();
+
+        qint64 retryAfterMs = 0;
+        const QByteArray retryAfterHeader = reply->rawHeader("Retry-After");
+        if (!retryAfterHeader.isEmpty()) {
+            bool ok = false;
+            int seconds = retryAfterHeader.toInt(&ok);
+            if (ok && seconds > 0) {
+                retryAfterMs = static_cast<qint64>(seconds) * 1000;
+            } else {
+                QDateTime retryAfterDate =
+                    QDateTime::fromString(QString::fromLatin1(retryAfterHeader), Qt::RFC2822Date);
+                if (retryAfterDate.isValid()) {
+                    retryAfterMs = qMax<qint64>(0, now.msecsTo(retryAfterDate));
+                }
+            }
+        }
+
+        QDateTime backoffUntil = s_rateLimiter.on429(host, now, retryAfterMs);
+        emit rateLimited(host);
+
+        if (s_rateLimiter.rounds(host) == 1) {
+            qCWarning(CardPictureLoaderWorkerWorkLog).nospace()
+                << "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();
+        } else {
+            qCWarning(CardPictureLoaderWorkerWorkLog).nospace()
+                << "PictureLoader: [card: " << cardToDownload.getCard().getName()
+                << " set: " << cardToDownload.getSetName() << "]: Too many requests from " << host
+                << ", retry already attempted, falling through to other sources";
+            picDownloadFailed();
+        }
     } else {
         bool isFromCache = reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool();
 
@@ -149,6 +216,9 @@ void CardPictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply)
         return;
     }
 
+    // A non-redirect successful response means the server is not rate limiting us anymore.
+    s_rateLimiter.onSuccess(reply->url().host());
+
     // peek is used to keep the data in the buffer for use by QImageReader
     const QByteArray &picData = reply->peek(reply->size());
 
@@ -203,6 +273,42 @@ QImage CardPictureLoaderWorkerWork::tryLoadImageFromReply(QNetworkReply *reply)
     return imgReader.read();
 }
 
+void CardPictureLoaderWorkerWork::scheduleDeferredRetry()
+{
+    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)) {
+        backoffUntil = s_rateLimiter.earliestDeadline(now);
+    }
+
+    if (!backoffUntil.isValid()) {
+        qCWarning(CardPictureLoaderWorkerWorkLog).nospace()
+            << "PictureLoader: [card: " << cardToDownload.getCard().getInfo().getCorrectedName()
+            << " set: " << cardToDownload.getSetName() << "]: All URLs exhausted, no servers in backoff: BAILING OUT";
+        concludeImageLoad(QImage());
+        return;
+    }
+
+    qint64 waitMs = qMax<qint64>(0, now.msecsTo(backoffUntil));
+    // Add some jitter to desynchronize concurrent retries and avoid a thundering herd.
+    waitMs += QRandomGenerator::global()->bounded(5000);
+
+    qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
+        << "PictureLoader: [card: " << cardToDownload.getCard().getInfo().getCorrectedName()
+        << " set: " << cardToDownload.getSetName() << "]: All URLs exhausted, scheduling deferred retry in " << waitMs
+        << "ms";
+
+    QTimer::singleShot(waitMs, this, [this] {
+        s_rateLimiter.clearExpired(QDateTime::currentDateTime());
+
+        cardToDownload.resetIndices();
+        startNextPicDownload();
+    });
+}
+
 void CardPictureLoaderWorkerWork::concludeImageLoad(const QImage &image)
 {
     emit imageLoaded(cardToDownload.getCard(), image);

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h

@@ -4,12 +4,15 @@
 #include "card_picture_loader_worker.h"
 #include "card_picture_to_load.h"
 
+#include <QDateTime>
 #include <QLoggingCategory>
 #include <QMutex>
 #include <QNetworkAccessManager>
 #include <QObject>
+#include <QRandomGenerator>
 #include <QThread>
 #include <libcockatrice/card/database/card_database.h>
+#include <libcockatrice/utility/server_rate_limiter.h>
 
 inline Q_LOGGING_CATEGORY(CardPictureLoaderWorkerWorkLog, "card_picture_loader.worker");
 
@@ -50,6 +53,8 @@ public slots:
 private:
     bool picDownload; ///< Whether network downloading is enabled
 
+    static ServerRateLimiter s_rateLimiter; ///< Shared per-server 429 backoff state
+
     /** @brief Starts downloading the next URL for this card. */
     void startNextPicDownload();
 
@@ -77,6 +82,16 @@ 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();
@@ -100,6 +115,9 @@ signals:
     /** @brief Emitted when a URL has been redirected. */
     void urlRedirected(const QUrl &originalUrl, const QUrl &redirectUrl);
 
+    /** @brief Emitted when a server returned HTTP 429. */
+    void rateLimited(const QString &host);
+
     /** @brief Emitted when a cached URL is invalid and must be removed. */
     void cachedUrlInvalidated(const QUrl &url);
 };

cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp

@@ -17,8 +17,9 @@ CardPictureToLoad::CardPictureToLoad(const ExactCard &_card)
 {
     if (card) {
         sortedSets = extractSetsSorted(card);
-        // The first time called, nextSet will also populate the Urls for the first set.
-        nextSet();
+        currentSetIndex = 0;
+        currentSet = sortedSets.first();
+        populateSetUrls();
     }
 }
 
@@ -101,15 +102,19 @@ void CardPictureToLoad::populateSetUrls()
         }
     }
 
-    /* Call nextUrl to make sure currentUrl is up-to-date
-       but we don't need the result here. */
-    (void)nextUrl();
+    currentUrlIndex = 0;
+    if (!currentSetUrls.isEmpty()) {
+        currentUrl = currentSetUrls.first();
+    } else {
+        currentUrl = QString();
+    }
 }
 
 bool CardPictureToLoad::nextSet()
 {
-    if (!sortedSets.isEmpty()) {
-        currentSet = sortedSets.takeFirst();
+    currentSetIndex++;
+    if (currentSetIndex < sortedSets.size()) {
+        currentSet = sortedSets.at(currentSetIndex);
         populateSetUrls();
         return true;
     }
@@ -119,8 +124,9 @@ bool CardPictureToLoad::nextSet()
 
 bool CardPictureToLoad::nextUrl()
 {
-    if (!currentSetUrls.isEmpty()) {
-        currentUrl = currentSetUrls.takeFirst();
+    currentUrlIndex++;
+    if (currentUrlIndex < currentSetUrls.size()) {
+        currentUrl = currentSetUrls.at(currentUrlIndex);
         return true;
     }
     currentUrl = QString();
@@ -136,6 +142,28 @@ QString CardPictureToLoad::getSetName() const
     }
 }
 
+QString CardPictureToLoad::peekNextUrl() const
+{
+    int nextIndex = currentUrlIndex + 1;
+    if (nextIndex < currentSetUrls.size()) {
+        return currentSetUrls.at(nextIndex);
+    }
+    return QString();
+}
+
+void CardPictureToLoad::resetIndices()
+{
+    currentSetIndex = 0;
+    if (!sortedSets.isEmpty()) {
+        currentSet = sortedSets.first();
+        populateSetUrls();
+    } else {
+        currentSet = {};
+        currentSetUrls.clear();
+        currentUrl = QString();
+    }
+}
+
 static int parse(const QString &urlTemplate,
                  const QString &propType,
                  const QString &cardName,

cockatrice/src/interface/card_picture_loader/card_picture_to_load.h

@@ -25,6 +25,8 @@ private:
     QList<QString> currentSetUrls; ///< URLs for the current set being attempted
     QString currentUrl;            ///< Currently active URL to download
     CardSetPtr currentSet;         ///< Currently active set
+    int currentSetIndex = 0;       ///< Current position in sortedSets
+    int currentUrlIndex = 0;       ///< Current position in currentSetUrls
 
 public:
     /**
@@ -56,6 +58,9 @@ public:
     /** @return The short name of the current set, or empty string if no set. */
     [[nodiscard]] QString getSetName() const;
 
+    /** @return The next URL in the current set's list without advancing, or empty if at end. */
+    [[nodiscard]] QString peekNextUrl() const;
+
     /**
      * @brief Transforms a URL template into a concrete URL for this card/set.
      * @param urlTemplate The URL template to transform
@@ -88,6 +93,14 @@ public:
      */
     void populateSetUrls();
 
+    /**
+     * @brief Resets iteration indices to the beginning.
+     *
+     * Restarts URL/set iteration from the first set and first URL.
+     * Used for deferred retry after server backoff expires.
+     */
+    void resetIndices();
+
     /**
      * @brief Extract all sets from the card and sort them by priority.
      * @param card The card to extract sets from