[PictureLoader] Hand backed-off entries back across the whole dispatch tick

Review fixes on the 429-backoff skip:

- Hand a backed-off queue entry back to its worker via
  scheduleDeferredRetry(host) instead of startNextPicDownload(), which was
  looping on the pre-redirect host every 100ms tick whenever the queued URL
  was a cached-redirect target whose redirect host was the one in backoff.
- Keep scanning the queue after a hand-back (--i; continue) so a backed-off
  host at the head no longer consumes the entire dispatch tick, stalling
  every healthy host further down.
- Emit imageRequestSucceeded(url) before makeRequest()'s cached-redirect
  backoff return so the status bar reclaims the deferred URL's widget instead
  of inflating the progress bar forever.
- Only spend a per-second allowance when makeRequest() actually issues a
  request: it returns nullptr when the redirect target is backed off and the
  work is handed back, so the slot would otherwise be wasted on a no-op.
- Gate the dispatch-time backoff skip on requestTouchesNetwork() so entries
  served straight from the disk cache (e.g. with downloads disabled) are not
  bounced into a 30-60s deferred retry for a host that 429'd earlier.

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp

@@ -107,6 +107,9 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture
     // Check for cached redirects
     QUrl cachedRedirect = getCachedRedirect(url);
     if (!cachedRedirect.isEmpty()) {
+        // The status bar still needs to reclaim this URL's widget even when we hand the request back
+        // for a deferred retry instead of dispatching it onto the network.
+        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.
@@ -115,7 +118,6 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture
             worker->scheduleDeferredRetry(cachedRedirect.host());
             return nullptr;
         }
-        emit imageRequestSucceeded(url);
         return makeRequest(cachedRedirect, worker);
     }
 
@@ -126,10 +128,7 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture
     // Cached entries are served straight from the disk cache even when picture downloads are
     // enabled: re-fetching an already-cached image would burn the rate limit for nothing. Only a
     // genuine cache miss goes to the network, and only when downloads are enabled.
-    bool useNetworkCache = static_cast<CardPictureLoaderCacheMethod::CacheMethod>(
+    bool useNetworkCache = !requestTouchesNetwork(url);
-                               SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) ==
-                               CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE &&
-                           (cache->metaData(url).isValid() || !picDownload);
 
     req.setAttribute(QNetworkRequest::CacheLoadControlAttribute,
                      useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork);
@@ -226,11 +225,19 @@ bool CardPictureLoaderWorker::processSingleRequest()
         const QString host = request.first.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.
+        // instead of leaving it parked in the queue with no reply pending. Only applies to
-        if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) {
+        // requests that will actually touch the network: one that will be served from the disk
+        // cache costs nothing and shouldn't wait out the 429.
+        if (requestTouchesNetwork(request.first) &&
+            CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) {
+            // The queued URL is usually a cached-redirect target whose host differs from
+            // cardToDownload.getCurrentUrl(), so scheduleDeferredRetry() (which waits out the
+            // blocked host's deadline) is used instead of startNextPicDownload() looping on the
+            // original host. Keep scanning so one backed-off entry doesn't monopolize the tick.
             auto entry = requestLoadQueue.takeAt(i);
-            entry.second->startNextPicDownload();
+            --i;
-            return true;
+            entry.second->scheduleDeferredRetry(host);
+            continue;
         }
         // Seed the allowance now so a host that was rate limited gets its reduced
         // allowance instead of a fresh full quota mid-second.
@@ -239,15 +246,27 @@ bool CardPictureLoaderWorker::processSingleRequest()
         }
         int allowance = hostQuotaRemaining.value(host);
         if (allowance > 0) {
-            hostQuotaRemaining.insert(host, allowance - 1);
             auto entry = requestLoadQueue.takeAt(i);
-            makeRequest(entry.first, entry.second);
+            // The allowance is only spent when a request is actually issued: makeRequest() returns
+            // nullptr when the cached redirect target is in backoff and it hands the entry back.
+            if (makeRequest(entry.first, entry.second)) {
+                hostQuotaRemaining.insert(host, allowance - 1);
+            }
             return true;
         }
     }
     return false;
 }
 
+bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url) const
+{
+    bool useNetworkCache = static_cast<CardPictureLoaderCacheMethod::CacheMethod>(
+                               SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) ==
+                               CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE &&
+                           (cache->metaData(url).isValid() || !picDownload);
+    return !useNetworkCache;
+}
+
 void CardPictureLoaderWorker::onHostRateLimited(const QString &host)
 {
     hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2));

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h

@@ -133,6 +133,10 @@ private:
     /** @brief Returns cached redirect URL for the given original URL, if available. */
     [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const;
 
+    /** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk
+     * cache. */
+    [[nodiscard]] bool requestTouchesNetwork(const QUrl &url) const;
+
     /** @brief Loads redirect cache from disk. */
     void loadRedirectCache();
 

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h

@@ -51,9 +51,7 @@ public:
      * @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
+     * backoff out or falling through to the other configured sources.
-     * 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();