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 84b9c8421..5bb513809 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 @@ -145,11 +145,16 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture const QString host = url.host(); hostInFlight.insert(host, hostInFlight.value(host) + 1); - // Connect reply handling - connect(reply, &QNetworkReply::finished, worker, [this, reply, worker, host] { - hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1)); - worker->handleNetworkReply(reply); - }); + // 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; } @@ -216,14 +221,18 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() // connection for them. for (int i = 0; i < requestLoadQueue.size();) { const auto &request = requestLoadQueue.at(i); - const QString host = request.first.host(); + // 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(request.first, request.second); + makeRequest(resolvedUrl, request.second); requestLoadQueue.removeAt(i); dispatched = true; continue; @@ -288,14 +297,24 @@ 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. if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { auto entry = requestLoadQueue.takeAt(i); - entry.second->startNextPicDownload(); + 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 @@ -319,7 +338,7 @@ bool CardPictureLoaderWorker::processSingleRequest() if (allowance > 0) { hostQuotaRemaining.insert(host, allowance - 1); auto entry = requestLoadQueue.takeAt(i); - makeRequest(entry.first, entry.second); + makeRequest(resolvedUrl, entry.second); return true; } } @@ -407,6 +426,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 ae49d1fe5..b720247cb 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 @@ -132,6 +132,9 @@ private: /** @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 @@ -154,6 +157,16 @@ private: /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) 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 8628aa5a8..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 @@ -189,7 +189,12 @@ void DeckEditorSettingsPage::storeSettings() QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); bool limitsChanged = false; for (auto it = limits.begin(); it != limits.end();) { - if (!usedHosts.contains(it.key())) { + // 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 { @@ -269,7 +274,7 @@ void DeckEditorSettingsPage::actAdjustRateLimit() QString prompt; if (unlocked) { minimum = 0; // 0 means "unlimited" - maximum = DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT; + 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 { diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index 2df51448a..7eca77df6 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -25,6 +25,15 @@ public: static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; /** @brief Floor for any per-host request allowance. */ static constexpr int MIN_HOST_REQUEST_LIMIT = 1; + /** + * @brief 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;