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 f9391f7ce..2f46e7941 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/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(); } 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 0c114ae92..5c3ac84a3 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h +++ b/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 +#include #include 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 failedAt; ///< Timestamp of the last failed load attempt per pixmap cache key public: /** 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 8b121d91c..d288236d2 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,6 +17,8 @@ #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 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 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 f927abde6..d1c519b7a 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 @@ -5,6 +5,8 @@ #include "card_picture_loader_worker_work.h" #include "card_picture_to_load.h" +#include +#include #include #include #include @@ -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> 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 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 CardPictureLoaderLocal *localLoader; ///< Loader for local images QSet currentlyLoading; ///< Deduplication: contains pixmapCacheKey currently being loaded 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 bfd46a462..66c56337c 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 @@ -8,8 +8,12 @@ #include #include #include +#include #include #include +#include + +ServerRateLimiter CardPictureLoaderWorkerWork::s_rateLimiter; #include // 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(seconds) * 1000; + } else { + QDateTime retryAfterDate = + QDateTime::fromString(QString::fromLatin1(retryAfterHeader), Qt::RFC2822Date); + if (retryAfterDate.isValid()) { + retryAfterMs = qMax(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(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); 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 cdffc1dff..1e56a4373 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 @@ -4,12 +4,15 @@ #include "card_picture_loader_worker.h" #include "card_picture_to_load.h" +#include #include #include #include #include +#include #include #include +#include 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); }; diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp index 33e4aabdb..5f4ff0bbd 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp +++ b/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, diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_to_load.h b/cockatrice/src/interface/card_picture_loader/card_picture_to_load.h index b57e57644..9e0e7449c 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_to_load.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_to_load.h @@ -25,6 +25,8 @@ private: QList 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 diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index 919199126..cfa1c054e 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -3,6 +3,7 @@ #include "settings_manager.h" const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { + "https://cards.scryfall.io/large/!prop:side!/!set:uuid_substr_0_1!/!set:uuid_substr_1_1!/!set:uuid!.jpg", "https://api.scryfall.com/cards/!set:uuid!?format=image&face=!prop:side!", "https://api.scryfall.com/cards/multiverse/!set:muid!?format=image", "https://gatherer.wizards.com/Handlers/Image.ashx?multiverseid=!set:muid!&type=card", diff --git a/libcockatrice_utility/CMakeLists.txt b/libcockatrice_utility/CMakeLists.txt index 2d34cad31..79f5a11e4 100644 --- a/libcockatrice_utility/CMakeLists.txt +++ b/libcockatrice_utility/CMakeLists.txt @@ -6,7 +6,7 @@ set(CMAKE_AUTOUIC ON) set(CMAKE_AUTORCC ON) set(UTILITY_SOURCES libcockatrice/utility/expression.cpp libcockatrice/utility/levenshtein.cpp - libcockatrice/utility/passwordhasher.cpp + libcockatrice/utility/passwordhasher.cpp libcockatrice/utility/server_rate_limiter.cpp ) set(UTILITY_HEADERS @@ -21,6 +21,7 @@ set(UTILITY_HEADERS libcockatrice/utility/clamped_arithmetic.h libcockatrice/utility/zone_names.h libcockatrice/utility/days_years_between.h + libcockatrice/utility/server_rate_limiter.h ) add_library(libcockatrice_utility STATIC ${UTILITY_SOURCES} ${UTILITY_HEADERS}) diff --git a/libcockatrice_utility/libcockatrice/utility/server_rate_limiter.cpp b/libcockatrice_utility/libcockatrice/utility/server_rate_limiter.cpp new file mode 100644 index 000000000..2a1e3b283 --- /dev/null +++ b/libcockatrice_utility/libcockatrice/utility/server_rate_limiter.cpp @@ -0,0 +1,85 @@ +#include "server_rate_limiter.h" + +bool ServerRateLimiter::isRateLimited(const QString &host, const QDateTime &now) const +{ + auto it = backoffUntil.constFind(host); + return it != backoffUntil.constEnd() && now < it.value(); +} + +int ServerRateLimiter::rounds(const QString &host) const +{ + return retryRounds.value(host, 0); +} + +QDateTime ServerRateLimiter::deadline(const QString &host) const +{ + return backoffUntil.value(host); +} + +QDateTime ServerRateLimiter::earliestDeadline(const QDateTime &now) const +{ + QDateTime earliest; + for (auto it = backoffUntil.cbegin(); it != backoffUntil.cend(); ++it) { + if (now < it.value() && (!earliest.isValid() || it.value() < earliest)) { + earliest = it.value(); + } + } + return earliest; +} + +QDateTime ServerRateLimiter::on429(const QString &host, const QDateTime &now, qint64 retryAfterMs) +{ + QDateTime existing = backoffUntil.value(host); + int round = retryRounds.value(host, 0); + + if (!existing.isValid() || now >= existing) { + if (last429.value(host).isValid() && now >= last429.value(host).addMSecs(RESET_GRACE_MS)) { + round = 0; + } + round = qMin(round + 1, MAX_RETRIES); + retryRounds.insert(host, round); + } + last429.insert(host, now); + + qint64 delay; + if (round >= MAX_RETRIES) { + delay = EXHAUSTED_BACKOFF_MS; + } else { + delay = qMax(MIN_429_BACKOFF_MS, retryAfterMs); + delay = qMin(delay, MAX_BACKOFF_MS); + } + + QDateTime deadline = now.addMSecs(delay); + if (existing.isValid() && existing > deadline) { + deadline = existing; // never shorten an active backoff + } + backoffUntil.insert(host, deadline); + + return deadline; +} + +void ServerRateLimiter::onSuccess(const QString &host) +{ + backoffUntil.remove(host); + retryRounds.remove(host); + last429.remove(host); +} + +void ServerRateLimiter::clearExpired(const QDateTime &now) +{ + auto it = backoffUntil.begin(); + while (it != backoffUntil.end()) { + if (now < it.value()) { + ++it; + continue; + } + + QString host = it.key(); + bool budgetStillFresh = last429.value(host).isValid() && now < last429.value(host).addMSecs(RESET_GRACE_MS); + it = backoffUntil.erase(it); + if (!budgetStillFresh) { + retryRounds.remove(host); + last429.remove(host); + } + } +} diff --git a/libcockatrice_utility/libcockatrice/utility/server_rate_limiter.h b/libcockatrice_utility/libcockatrice/utility/server_rate_limiter.h new file mode 100644 index 000000000..e071b9189 --- /dev/null +++ b/libcockatrice_utility/libcockatrice/utility/server_rate_limiter.h @@ -0,0 +1,73 @@ +#ifndef SERVER_RATE_LIMITER_H +#define SERVER_RATE_LIMITER_H + +#include +#include +#include + +/** + * @class ServerRateLimiter + * @ingroup Utility + * @brief Tracks per-server backoff state triggered by HTTP 429 responses. + * + * Keeps a monotonic backoff deadline per host and a retry-round budget that + * escalates once per backoff window (a burst of concurrent 429s counts as a + * single round). The budget is refreshed after a long period without 429s so a + * server is not permanently blacklisted after a past overload. + */ +class ServerRateLimiter +{ +public: + static constexpr int MAX_RETRIES = 5; ///< Max 429 rounds per host before the budget is exhausted + static constexpr int MIN_429_BACKOFF_MS = 30000; ///< Minimum wait after a 429 (scryfall documented cool-down) + static constexpr int MAX_BACKOFF_MS = 60000; ///< Cap for a single backoff period + static constexpr int EXHAUSTED_BACKOFF_MS = 60000; ///< Cool-down once the retry budget is exhausted + static constexpr qint64 RESET_GRACE_MS = 300000; ///< Idle time after which the retry budget refreshes + + /** + * @brief Checks whether a host is currently in backoff. + * @param host The host to check + * @param now The current time + * @return True if requests to the host should be paused + */ + [[nodiscard]] bool isRateLimited(const QString &host, const QDateTime &now) const; + + /** @return The number of consecutive 429 rounds recorded for the host. */ + [[nodiscard]] int rounds(const QString &host) const; + + /** @return The current backoff deadline for the host, or an invalid QDateTime. */ + [[nodiscard]] QDateTime deadline(const QString &host) const; + + /** @return The earliest active backoff deadline across all hosts, or an invalid QDateTime. */ + [[nodiscard]] QDateTime earliestDeadline(const QDateTime &now) const; + + /** + * @brief Registers a 429 response for the given host. + * @param host The host that returned 429 + * @param now The time the response was received + * @param retryAfterMs A Retry-After hint in milliseconds, or 0 if absent + * @return The new backoff deadline for the host + * + * The round counter only escalates once the previous backoff window has + * fully passed, so concurrent 429s from a burst do not consume the whole + * budget. The deadline is extended monotonically and never shortened. + */ + QDateTime on429(const QString &host, const QDateTime &now, qint64 retryAfterMs = 0); + + /** @brief Clears all penalty state for a host after a successful request. */ + void onSuccess(const QString &host); + + /** + * @brief Removes expired backoffs, refreshing the round budget of hosts + * that have been idle past the reset grace period. + * @param now The current time + */ + void clearExpired(const QDateTime &now); + +private: + QMap backoffUntil; ///< When each host may be contacted again + QMap retryRounds; ///< Consecutive 429 rounds per host + QMap last429; ///< When each host was last rate limited +}; + +#endif // SERVER_RATE_LIMITER_H diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 857e0b041..1c3f4c2c6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -9,6 +9,7 @@ add_test(NAME test_age_formatting COMMAND test_age_formatting) add_test(NAME password_hash_test COMMAND password_hash_test) add_test(NAME server_card_counter_test COMMAND server_card_counter_test) add_test(NAME server_counter_test COMMAND server_counter_test) +add_test(NAME server_rate_limiter_test COMMAND server_rate_limiter_test) add_test(NAME deck_hash_performance_test COMMAND deck_hash_performance_test) set_tests_properties(deck_hash_performance_test PROPERTIES TIMEOUT 5) @@ -23,6 +24,7 @@ add_executable(password_hash_test password_hash_test.cpp) add_executable(deck_hash_performance_test deck_hash_performance_test.cpp) add_executable(server_card_counter_test server_card_counter_test.cpp) add_executable(server_counter_test server_counter_test.cpp) +add_executable(server_rate_limiter_test server_rate_limiter_test.cpp) find_package(GTest) @@ -57,6 +59,7 @@ if(NOT GTEST_FOUND) add_dependencies(deck_hash_performance_test gtest) add_dependencies(server_card_counter_test gtest) add_dependencies(server_counter_test gtest) + add_dependencies(server_rate_limiter_test gtest) endif() include_directories(${GTEST_INCLUDE_DIRS}) @@ -81,6 +84,9 @@ target_link_libraries( target_link_libraries( server_counter_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) +target_link_libraries( + server_rate_limiter_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) add_subdirectory(card_zone_algorithms) add_subdirectory(carddatabase) diff --git a/tests/server_rate_limiter_test.cpp b/tests/server_rate_limiter_test.cpp new file mode 100644 index 000000000..bc15ca421 --- /dev/null +++ b/tests/server_rate_limiter_test.cpp @@ -0,0 +1,151 @@ +#include "gtest/gtest.h" +#include +#include +#include + +namespace +{ + +const QString HOST = "api.scryfall.com"; + +QDateTime timeAt(qint64 secsFromEpoch) +{ + return QDateTime::fromMSecsSinceEpoch(secsFromEpoch * 1000, QTimeZone::UTC); +} + +TEST(ServerRateLimiterTest, First429SetsThirtySecondBackoff) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + QDateTime deadline = limiter.on429(HOST, now); + + EXPECT_TRUE(limiter.isRateLimited(HOST, now)); + EXPECT_EQ(now.addSecs(30), deadline); + EXPECT_EQ(1, limiter.rounds(HOST)); + EXPECT_FALSE(limiter.isRateLimited(HOST, deadline)); +} + +TEST(ServerRateLimiterTest, RetryAfterHeaderIsHonoredWhenLarger) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + QDateTime deadline = limiter.on429(HOST, now, 45 * 1000); + + EXPECT_EQ(now.addSecs(45), deadline); +} + +TEST(ServerRateLimiterTest, RetryAfterSmallerThanFloorIsIgnored) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + QDateTime deadline = limiter.on429(HOST, now, 5 * 1000); + + EXPECT_EQ(now.addSecs(30), deadline); +} + +TEST(ServerRateLimiterTest, Concurrent429sDoNotEscalateOrShorten) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + QDateTime first = limiter.on429(HOST, now); + limiter.on429(HOST, now.addMSecs(100)); + limiter.on429(HOST, now.addMSecs(200)); + + // The burst counts as a single round, but each 429 slides the deadline forward. + EXPECT_EQ(1, limiter.rounds(HOST)); + EXPECT_EQ(now.addMSecs(30200), limiter.deadline(HOST)); + EXPECT_TRUE(limiter.deadline(HOST) >= first); +} + +TEST(ServerRateLimiterTest, EscalatesAfterWindowPasses) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + limiter.on429(HOST, now); + limiter.on429(HOST, now.addSecs(31)); + + EXPECT_EQ(2, limiter.rounds(HOST)); +} + +TEST(ServerRateLimiterTest, BudgetExhaustionAppliesCooldown) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + QDateTime deadline; + for (int i = 0; i < ServerRateLimiter::MAX_RETRIES; ++i) { + deadline = limiter.on429(HOST, now.addSecs(31 * i)); + } + + EXPECT_EQ(ServerRateLimiter::MAX_RETRIES, limiter.rounds(HOST)); + EXPECT_EQ(now.addSecs(31 * (ServerRateLimiter::MAX_RETRIES - 1) + 60), deadline); + + // A 429 inside the exhausted cooldown does not escalate, but keeps sliding the cooldown forward. + limiter.on429(HOST, now.addSecs(31 * (ServerRateLimiter::MAX_RETRIES - 1) + 30)); + EXPECT_EQ(ServerRateLimiter::MAX_RETRIES, limiter.rounds(HOST)); + EXPECT_EQ(now.addSecs(31 * (ServerRateLimiter::MAX_RETRIES - 1) + 30 + 60), limiter.deadline(HOST)); +} + +TEST(ServerRateLimiterTest, SuccessClearsPenalty) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + limiter.on429(HOST, now); + limiter.onSuccess(HOST); + + EXPECT_EQ(0, limiter.rounds(HOST)); + EXPECT_FALSE(limiter.isRateLimited(HOST, now)); +} + +TEST(ServerRateLimiterTest, ClearExpiredRefreshesStaleBudget) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + limiter.on429(HOST, now); + limiter.clearExpired(now.addSecs(400)); + + EXPECT_EQ(0, limiter.rounds(HOST)); + EXPECT_FALSE(limiter.isRateLimited(HOST, now.addSecs(400))); +} + +TEST(ServerRateLimiterTest, ClearExpiredKeepsRecentBudget) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + for (int i = 0; i < ServerRateLimiter::MAX_RETRIES; ++i) { + limiter.on429(HOST, now.addSecs(31 * i)); + } + limiter.clearExpired(now.addSecs(200)); + + EXPECT_EQ(ServerRateLimiter::MAX_RETRIES, limiter.rounds(HOST)); + EXPECT_FALSE(limiter.isRateLimited(HOST, now.addSecs(200))); +} + +TEST(ServerRateLimiterTest, EarliestDeadlineAcrossHosts) +{ + ServerRateLimiter limiter; + QDateTime now = timeAt(1000); + + limiter.on429("api.scryfall.com", now, 40 * 1000); + limiter.on429("gatherer.wizards.com", now.addSecs(5)); + + EXPECT_EQ(now.addSecs(35), limiter.earliestDeadline(now)); + EXPECT_EQ(now.addSecs(40), limiter.deadline("api.scryfall.com")); + EXPECT_EQ(now.addSecs(40), limiter.earliestDeadline(now.addSecs(36))); +} + +} // namespace + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}