[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.

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
+    // Release the in-flight slot when the reply is destroyed, not when it emits `finished`, and use
-    connect(reply, &QNetworkReply::finished, worker, [this, reply, worker, host] {
+    // the worker (not the work object) as the context object: a reply can go away without ever
-        hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1));
+    // finishing (aborted, or a work object deleted while a reply is still pending), and a connection
-        worker->handleNetworkReply(reply);
+    // 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);
+        // Copy the entry: takeAt(i) below erases within the list this reference points into.
-        const QString host = request.first.host();
+        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);
+            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);

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<QString> 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();
 

cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp

@@ -189,7 +189,12 @@ void DeckEditorSettingsPage::storeSettings()
     QHash<QString, int> 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 {

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;