Merge 4ee7b416a9 into df74917cb0

cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h

@@ -0,0 +1,69 @@
+#ifndef COCKATRICE_CARD_PICTURE_LOADER_IMAGE_FORMATS_H
+#define COCKATRICE_CARD_PICTURE_LOADER_IMAGE_FORMATS_H
+
+#include <QBuffer>
+#include <QByteArray>
+#include <QByteArrayList>
+#include <QIODevice>
+#include <QImage>
+#include <QImageReader>
+#include <QMovie>
+#include <QString>
+
+namespace CardPictureLoaderImageFormats
+{
+inline QByteArray acceptHeader()
+{
+    // Decoder plugins differ across distributions. Prefer only formats this
+    // runtime can read, then retain valid wildcards for other image endpoints.
+    static const QByteArray header = [] {
+        const auto supported = QImageReader::supportedMimeTypes();
+        QByteArrayList types;
+        for (const QByteArray &preferred :
+             {QByteArray("image/avif"), QByteArray("image/webp"), QByteArray("image/apng"), QByteArray("image/png"),
+              QByteArray("image/jpeg")}) {
+            if (supported.contains(preferred)) {
+                types.append(preferred);
+            }
+        }
+        for (const QByteArray &type : supported) {
+            if (type.startsWith("image/") && !types.contains(type)) {
+                types.append(type);
+            }
+        }
+        types.append("image/*;q=0.5");
+        types.append("*/*;q=0.1");
+        return types.join(',');
+    }();
+    return header;
+}
+
+struct DecodedImage
+{
+    QImage image;
+    QString error;
+};
+
+inline DecodedImage readImage(QIODevice *device)
+{
+    static constexpr int riffHeaderSize = 12; // RIFF_HEADER_SIZE from webp/format_constants.h
+    const auto header = device->peek(riffHeaderSize);
+    if (header.startsWith("RIFF") && header.endsWith("WEBP")) {
+        // Preserve the existing first-frame behavior for animated WebP files.
+        QBuffer buffer;
+        buffer.setData(device->readAll());
+        QMovie movie(&buffer);
+        movie.start();
+        movie.stop();
+        return {movie.currentImage(), movie.lastErrorString()};
+    }
+
+    QImageReader reader;
+    reader.setDecideFormatFromContent(true);
+    reader.setDevice(device);
+    QImage image = reader.read();
+    return {image, image.isNull() ? reader.errorString() : QString()};
+}
+} // namespace CardPictureLoaderImageFormats
+
+#endif // COCKATRICE_CARD_PICTURE_LOADER_IMAGE_FORMATS_H

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp

@@ -2,11 +2,13 @@
 
 #include "../../client/settings/cache_settings.h"
 #include "card_picture_loader_cache_method.h"
+#include "card_picture_loader_image_formats.h"
 #include "card_picture_loader_local.h"
 #include "card_picture_loader_worker_work.h"
 #include "card_picture_to_load.h"
 #include "libcockatrice/card/printing/exact_card.h"
 #include "libcockatrice/utility/server_rate_limiter.h"
+#include "redirect_chain.h"
 
 #include <QAbstractNetworkCache>
 #include <QByteArray>
@@ -100,6 +102,22 @@ CardPictureLoaderWorker::CardPictureLoaderWorker()
 
     connect(&SettingsCache::instance().downloads(), &DownloadSettings::hostRequestLimitsChanged, this,
             [this] { hostRequestLimits = SettingsCache::instance().downloads().getHostRequestLimits(); });
+    connect(&SettingsCache::instance().downloads(), &DownloadSettings::picDownloadChanged, this, [this] {
+        picDownload = SettingsCache::instance().downloads().getPicDownload();
+        if (!picDownload) {
+            // Waiting for network quota or 429 recovery cannot help a cache-only lookup.
+            // Retire cancelled status entries just as the normal dispatcher does.
+            while (!requestLoadQueue.isEmpty()) {
+                const auto entry = requestLoadQueue.takeFirst();
+                if (entry.second) {
+                    makeRequest(entry.first, entry.second);
+                } else {
+                    emit imageRequestSucceeded(entry.first);
+                }
+            }
+            updateTimerState();
+        }
+    });
 }
 
 CardPictureLoaderWorker::~CardPictureLoaderWorker()
@@ -136,52 +154,49 @@ QThread *CardPictureLoaderWorker::workerThread() const
 
 void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker)
 {
-    QUrl cachedRedirect = getCachedRedirect(url);
+    const QUrl resolvedUrl = resolveCachedRedirect(url);
-    if (!cachedRedirect.isEmpty()) {
+    if (!picDownload || (static_cast<CardPictureLoaderCacheMethod::CacheMethod>(
-        queueRequest(cachedRedirect, worker);
+                             SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) ==
-        return;
+                             CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE &&
-    }
+                         cache->metaData(resolvedUrl).isValid() && !worker->shouldBypassCache(resolvedUrl))) {
-    if (static_cast<CardPictureLoaderCacheMethod::CacheMethod>(
-            SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) ==
-            CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE &&
-        cache->metaData(url).isValid()) {
         // A request that will be served from the disk cache never touches the network and therefore
         // doesn't use up any of the rate limit, so it gets to skip the queue.
-        makeRequest(url, worker);
+        // Every reply retires a status, including online cache hits and deferred
+        // 429 retries. Pair that completion with this request before dispatch.
+        emit imageRequestQueued(resolvedUrl, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName());
+        makeRequest(resolvedUrl, worker);
         return;
     }
-    requestLoadQueue.append(qMakePair(url, worker));
+    requestLoadQueue.append(qMakePair(resolvedUrl, QPointer<CardPictureLoaderWorkerWork>(worker)));
-    emit imageRequestQueued(url, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName());
+    emit imageRequestQueued(resolvedUrl, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName());
     processQueuedRequests();
 }
 
 QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker)
 {
-    // Check for cached redirects
+    // Redirect mappings can be learned while a request waits in the queue.
-    QUrl cachedRedirect = getCachedRedirect(url);
+    // Resolve again without recursion, and honor the target host's backoff.
-    if (!cachedRedirect.isEmpty()) {
+    const QUrl resolvedUrl = resolveCachedRedirect(url);
-        // The status bar still needs to reclaim this URL's widget even when we hand the request back
+    if (resolvedUrl != url) {
-        // 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
+        if (requestTouchesNetwork(resolvedUrl, worker) && CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(
-        // entry back to its worker so it waits the backoff out instead of dispatching straight
+                                                              resolvedUrl.host(), QDateTime::currentDateTime())) {
-        // onto the backed-off host.
+            worker->scheduleDeferredRetry(resolvedUrl.host());
-        if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(cachedRedirect.host(),
-                                                                     QDateTime::currentDateTime())) {
-            worker->scheduleDeferredRetry(cachedRedirect.host());
             return nullptr;
         }
-        return makeRequest(cachedRedirect, worker);
+        // The original status was retired above; track the resolved request
+        // separately so its eventual completion has a matching queued entry.
+        emit imageRequestQueued(resolvedUrl, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName());
     }
 
-    QNetworkRequest req(url);
+    QNetworkRequest req(resolvedUrl);
     req.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING));
-    req.setRawHeader("Accept", "image/avif,image/webp,image/apng,image/,/*;q=0.8");
+    req.setRawHeader("Accept", CardPictureLoaderImageFormats::acceptHeader());
 
     // 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 = !requestTouchesNetwork(url);
+    bool useNetworkCache = !requestTouchesNetwork(resolvedUrl, worker);
 
     req.setAttribute(QNetworkRequest::CacheLoadControlAttribute,
                      useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork);
@@ -190,7 +205,7 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture
 
     // Track in-flight replies per host so the unlocked fast path can bound how many requests it
     // issues at once, instead of creating replies that time out before Qt opens a connection.
-    const QString host = url.host();
+    const QString host = resolvedUrl.host();
     hostInFlight.insert(host, hostInFlight.value(host) + 1);
 
     // Release the in-flight slot when the reply is destroyed, not when it emits `finished`, and use
@@ -201,8 +216,22 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture
     connect(reply, &QObject::destroyed, this,
             [this, host] { hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1)); });
 
+    // Retire the status even if the work object is destroyed before the reply
+    // finishes. Connect this before the work callback, which may enqueue a retry.
+    connect(reply, &QNetworkReply::finished, this, [this, reply] { emit imageRequestSucceeded(reply->url()); });
+
     // 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); });
+    const QPointer<CardPictureLoaderWorkerWork> guardedWorker(worker);
+    connect(reply, &QNetworkReply::finished, worker, [guardedWorker, reply] {
+        if (guardedWorker) {
+            guardedWorker->handleNetworkReply(reply);
+        }
+    });
+    // A work item may be cancelled before the reply finishes. In that case its
+    // context-bound handler is disconnected, but the reply must still release
+    // the host's in-flight slot and any network resources.
+    connect(worker, &QObject::destroyed, reply, [reply] { reply->abort(); });
+    connect(reply, &QNetworkReply::finished, reply, &QObject::deleteLater);
 
     return reply;
 }
@@ -269,19 +298,25 @@ void CardPictureLoaderWorker::dispatchQueuedRequest()
     // connection for them.
     for (int i = 0; i < requestLoadQueue.size();) {
         const auto &request = requestLoadQueue.at(i);
+        if (!request.second) {
+            const auto cancelled = requestLoadQueue.takeAt(i);
+            emit imageRequestSucceeded(cancelled.first);
+            continue;
+        }
         // 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)) {
+            if (requestTouchesNetwork(resolvedUrl, request.second) &&
+                CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) {
                 ++i;
                 continue;
             }
             if (hostInFlight.value(host) < MAX_IN_FLIGHT_PER_HOST) {
-                makeRequest(resolvedUrl, request.second);
+                const auto entry = requestLoadQueue.takeAt(i);
-                requestLoadQueue.removeAt(i);
+                makeRequest(entry.first, entry.second);
                 dispatched = true;
                 continue;
             }
@@ -347,6 +382,11 @@ bool CardPictureLoaderWorker::processSingleRequest()
     for (int i = 0; i < requestLoadQueue.size(); ++i) {
         // Copy the entry: takeAt(i) below erases within the list this reference points into.
         const auto request = requestLoadQueue.at(i);
+        if (!request.second) {
+            requestLoadQueue.removeAt(i--);
+            emit imageRequestSucceeded(request.first);
+            continue;
+        }
         // 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);
@@ -356,13 +396,14 @@ bool CardPictureLoaderWorker::processSingleRequest()
         // instead of leaving it parked in the queue with no reply pending. Only applies to
         // 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) &&
+        if (requestTouchesNetwork(resolvedUrl, request.second) &&
             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.
             auto entry = requestLoadQueue.takeAt(i);
+            emit imageRequestSucceeded(entry.first);
             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.
@@ -393,15 +434,21 @@ bool CardPictureLoaderWorker::processSingleRequest()
         if (allowance > 0) {
             hostQuotaRemaining.insert(host, allowance - 1);
             auto entry = requestLoadQueue.takeAt(i);
-            makeRequest(resolvedUrl, entry.second);
+            makeRequest(entry.first, entry.second);
             return true;
         }
     }
     return false;
 }
 
-bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url) const
+bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url, const CardPictureLoaderWorkerWork *worker) const
 {
+    if (!picDownload) {
+        return false;
+    }
+    if (worker->shouldBypassCache(url)) {
+        return true;
+    }
     bool useNetworkCache = static_cast<CardPictureLoaderCacheMethod::CacheMethod>(
                                SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) ==
                                CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE &&
@@ -477,33 +524,19 @@ void CardPictureLoaderWorker::cacheRedirect(const QUrl &originalUrl, const QUrl
     // saveRedirectCache();
 }
 
+void CardPictureLoaderWorker::invalidateRedirect(const QUrl &url)
+{
+    redirectCache.remove(url);
+}
+
 void CardPictureLoaderWorker::removedCachedUrl(const QUrl &url)
 {
     networkManager->cache()->remove(url);
 }
 
-QUrl CardPictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const
+QUrl CardPictureLoaderWorker::resolveCachedRedirect(const QUrl &url)
 {
-    if (redirectCache.contains(originalUrl)) {
+    return resolveImageRedirects(url, redirectCache, MAX_REDIRECT_CHAIN_DEPTH);
-        return redirectCache[originalUrl].first;
-    }
-    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()

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h

@@ -5,6 +5,7 @@
 #include <QLoggingCategory>
 #include <QObject>
 #include <QPair>
+#include <QPointer>
 #include <QQueue>
 #include <QSet>
 #include <QString>
@@ -139,6 +140,9 @@ public slots:
     /** @brief Caches a redirect mapping between original and redirected URL. */
     void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
 
+    /** @brief Removes a stale redirect mapping. */
+    void invalidateRedirect(const QUrl &url);
+
     /** @brief Removes a URL from the network cache. */
     void removedCachedUrl(const QUrl &url);
 
@@ -150,7 +154,7 @@ private:
     QString cacheFilePath;                             ///< Path to persistent redirect cache file
     static constexpr int CacheTTLInDays = 30;          ///< Time-to-live for redirect cache entries (days)
     bool picDownload;                                  ///< Whether downloading images from network is enabled
-    QQueue<QPair<QUrl, CardPictureLoaderWorkerWork *>> requestLoadQueue; ///< Queue of pending network requests
+    QQueue<QPair<QUrl, QPointer<CardPictureLoaderWorkerWork>>> requestLoadQueue; ///< Queue of pending network requests
 
     QTimer requestTimer;                    ///< Timer to reset the request quota
     QTimer dispatchTimer;                   ///< Timer pacing individual network requests
@@ -185,12 +189,9 @@ private:
      */
     [[nodiscard]] bool isUnlockedHost(const QString &host) const;
 
-    /** @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;
+    [[nodiscard]] bool requestTouchesNetwork(const QUrl &url, const CardPictureLoaderWorkerWork *worker) const;
 
     /**
      * @brief Follows the cached-redirect chain to the URL that will actually be requested.
@@ -200,7 +201,7 @@ private:
      * 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;
+    [[nodiscard]] QUrl resolveCachedRedirect(const QUrl &url);
 
     /** @brief Loads redirect cache from disk. */
     void loadRedirectCache();

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp

@@ -1,22 +1,20 @@
 #include "card_picture_loader_worker_work.h"
 
 #include "../../client/settings/cache_settings.h"
+#include "card_picture_loader_image_formats.h"
 #include "card_picture_loader_worker.h"
 #include "card_picture_to_load.h"
 #include "libcockatrice/card/card_info.h"
 #include "libcockatrice/card/printing/exact_card.h"
 #include "libcockatrice/utility/server_rate_limiter.h"
 
-#include <QBuffer>
 #include <QByteArray>
 #include <QCryptographicHash>
 #include <QDateTime>
 #include <QDebug>
-#include <QImageReader>
 #include <QList>
 #include <QLoggingCategory>
 #include <QMessageLogger>
-#include <QMovie>
 #include <QNetworkReply>
 #include <QNetworkRequest>
 #include <QRandomGenerator>
@@ -25,6 +23,8 @@
 #include <QTimer>
 #include <QUrl>
 #include <QVariant>
+#include <limits>
+#include <utility>
 
 ServerRateLimiter CardPictureLoaderWorkerWork::s_rateLimiter;
 #include <QtGlobal>
@@ -50,23 +50,32 @@ CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker
     // Hook up signals to the orchestrator
     connect(this, &CardPictureLoaderWorkerWork::requestImageDownload, worker, &CardPictureLoaderWorker::queueRequest);
     connect(this, &CardPictureLoaderWorkerWork::urlRedirected, worker, &CardPictureLoaderWorker::cacheRedirect);
+    connect(this, &CardPictureLoaderWorkerWork::redirectInvalidated, worker,
+            &CardPictureLoaderWorker::invalidateRedirect);
     connect(this, &CardPictureLoaderWorkerWork::cachedUrlInvalidated, worker,
             &CardPictureLoaderWorker::removedCachedUrl);
     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()));
 
+    deferredRetry = new QTimer(this);
+    deferredRetry->setSingleShot(true);
+    connect(deferredRetry, &QTimer::timeout, this, [this] {
+        s_rateLimiter.clearExpired(QDateTime::currentDateTime());
+        cardToDownload.resetIndices();
+        startNextPicDownload();
+    });
+
     startNextPicDownload();
 }
 
 void CardPictureLoaderWorkerWork::startNextPicDownload()
 {
+    redirectVisited.clear();
     QDateTime now = QDateTime::currentDateTime();
-    while (!cardToDownload.getCurrentUrl().isEmpty() &&
+    while (picDownload && !cardToDownload.getCurrentUrl().isEmpty() &&
            s_rateLimiter.isRateLimited(QUrl(cardToDownload.getCurrentUrl()).host(), now)) {
         QString host = QUrl(cardToDownload.getCurrentUrl()).host();
         if (s_rateLimiter.rounds(host) == 1) {
@@ -96,7 +105,11 @@ void CardPictureLoaderWorkerWork::startNextPicDownload()
     QString picUrl = cardToDownload.getCurrentUrl();
 
     if (picUrl.isEmpty()) {
-        scheduleDeferredRetry();
+        if (picDownload) {
+            scheduleDeferredRetry();
+        } else {
+            concludeImageLoad(QImage());
+        }
     } else {
         QUrl url(picUrl);
         qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
@@ -127,27 +140,38 @@ void CardPictureLoaderWorkerWork::picDownloadFailed()
 
 void CardPictureLoaderWorkerWork::handleNetworkReply(QNetworkReply *reply)
 {
-    QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
+    const QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
-    bool redirectFailure = false;
+    const int statusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+    const bool isRedirect =
+        statusCode == 301 || statusCode == 302 || statusCode == 303 || statusCode == 307 || statusCode == 308;
+    bool redirectFailure = isRedirect && !redirectTarget.isValid();
     if (redirectTarget.isValid()) {
-        QUrl url = reply->request().url();
+        const QUrl url = reply->url();
-        QUrl redirectUrl = redirectTarget.toUrl();
+        const QUrl redirectUrl = url.resolved(redirectTarget.toUrl());
-        if (redirectUrl.isRelative()) {
+        redirectVisited.insert(url);
-            redirectUrl = url.resolved(redirectUrl);
+        if (!redirectUrl.isValid() || redirectUrl.isEmpty() || redirectVisited.contains(redirectUrl) ||
-        }
+            redirectVisited.size() > 10) {
-        if (url == redirectUrl) {
+            qCWarning(CardPictureLoaderWorkerWorkLog) << "Invalid, cyclic or overlong redirect from" << url;
-            qCWarning(CardPictureLoaderWorkerWorkLog) << "recursive redirect detected!";
             redirectFailure = true;
         } else {
             emit urlRedirected(url, redirectUrl);
         }
     }
 
-    if (redirectFailure || reply->error()) {
+    if (redirectFailure) {
+        for (const QUrl &visited : std::as_const(redirectVisited)) {
+            emit redirectInvalidated(visited);
+        }
+        // Do not retry a cached cyclic redirect as a corrupt image: advance to
+        // the next configured source, just as for an ordinary failed response.
+        if (reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool()) {
+            emit cachedUrlInvalidated(reply->url());
+        }
+        picDownloadFailed();
+    } else if (reply->error()) {
         handleFailedReply(reply);
     } else {
         handleSuccessfulReply(reply);
-        emit requestSucceeded(reply->url());
     }
 
     reply->deleteLater();
@@ -184,6 +208,15 @@ void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply)
         QDateTime backoffUntil = s_rateLimiter.on429(host, now, retryAfterMs);
         emit rateLimited(host);
 
+        // Downloads may have been disabled while this request was in flight.
+        // Record the server's limit for future online requests, but resume the
+        // cache-only search now instead of scheduling a new network backoff.
+        if (!picDownload) {
+            cardToDownload.resetIndices();
+            startNextPicDownload();
+            return;
+        }
+
         if (s_rateLimiter.rounds(host) == 1) {
             qCWarning(CardPictureLoaderWorkerWorkLog).nospace()
                 << "PictureLoader: [card: " << cardToDownload.getCard().getName()
@@ -208,7 +241,9 @@ void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply)
 
             emit cachedUrlInvalidated(reply->url());
 
-            emit requestImageDownload(reply->url(), this);
+            if (!retryCachedUrl(reply->url())) {
+                picDownloadFailed();
+            }
         } else {
             qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
                 << "PictureLoader: [card: " << cardToDownload.getCard().getName()
@@ -226,9 +261,8 @@ void CardPictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply)
 
     // List of status codes from https://doc.qt.io/qt-6/qnetworkreply.html#redirected
     int statusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
-    if (statusCode == 301 || statusCode == 302 || statusCode == 303 || statusCode == 305 || statusCode == 307 ||
+    if (statusCode == 301 || statusCode == 302 || statusCode == 303 || statusCode == 307 || statusCode == 308) {
-        statusCode == 308) {
+        QUrl redirectUrl = reply->url().resolved(reply->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl());
-        QUrl redirectUrl = reply->header(QNetworkRequest::LocationHeader).toUrl();
         qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
             << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName()
             << "]: following " << (isFromCache ? "cached redirect" : "redirect") << " to "
@@ -252,15 +286,23 @@ void CardPictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply)
         return;
     }
 
-    QImage image = tryLoadImageFromReply(reply);
+    const auto decoded = CardPictureLoaderImageFormats::readImage(reply);
+    const QImage &image = decoded.image;
 
     if (image.isNull()) {
         qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
             << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName()
             << "]: Possible " << (isFromCache ? "cached" : "downloaded") << " picture at "
-            << reply->url().toDisplayString() << " could not be loaded: " << reply->errorString();
+            << reply->url().toDisplayString() << " could not be loaded: " << decoded.error;
 
-        picDownloadFailed();
+        // HTTP success can still contain an invalid or unsupported image. Remove
+        // that cache entry so future loads do not keep selecting the same bytes.
+        emit cachedUrlInvalidated(reply->url());
+        if (!isFromCache || !retryCachedUrl(reply->url())) {
+            // Fresh bad responses advance to the next source; cached-only mode
+            // must never turn a decoder failure into a network download.
+            picDownloadFailed();
+        }
     } else {
         qCDebug(CardPictureLoaderWorkerWorkLog).nospace()
             << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName()
@@ -271,27 +313,21 @@ void CardPictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply)
     }
 }
 
-QImage CardPictureLoaderWorkerWork::tryLoadImageFromReply(QNetworkReply *reply)
+bool CardPictureLoaderWorkerWork::shouldBypassCache(const QUrl &url) const
 {
-    static constexpr int riffHeaderSize = 12; // RIFF_HEADER_SIZE from webp/format_constants.h
+    return picDownload && retriedCacheUrls.contains(url);
-    auto replyHeader = reply->peek(riffHeaderSize);
+}
 
-    if (replyHeader.startsWith("RIFF") && replyHeader.endsWith("WEBP")) {
+bool CardPictureLoaderWorkerWork::retryCachedUrl(const QUrl &url)
-        auto imgBuf = QBuffer(this);
+{
-        imgBuf.setData(reply->readAll());
+    if (!picDownload || retriedCacheUrls.contains(url)) {
-
+        return false;
-        auto movie = QMovie(&imgBuf);
-        movie.start();
-        movie.stop();
-
-        return movie.currentImage();
     }
-
+    // Record before emitting: the queue can dispatch synchronously, and removing
-    QImageReader imgReader;
+    // an unreadable cache entry can fail. The retry must bypass that entry.
-    imgReader.setDecideFormatFromContent(true);
+    retriedCacheUrls.insert(url);
-    imgReader.setDevice(reply);
+    emit requestImageDownload(url, this);
-
+    return true;
-    return imgReader.read();
 }
 
 void CardPictureLoaderWorkerWork::scheduleDeferredRetry(const QString &preferredHost)
@@ -324,12 +360,7 @@ void CardPictureLoaderWorkerWork::scheduleDeferredRetry(const QString &preferred
         << " set: " << cardToDownload.getSetName() << "]: All URLs exhausted, scheduling deferred retry in " << waitMs
         << "ms";
 
-    QTimer::singleShot(waitMs, this, [this] {
+    deferredRetry->start(static_cast<int>(qMin<qint64>(waitMs, std::numeric_limits<int>::max())));
-        s_rateLimiter.clearExpired(QDateTime::currentDateTime());
-
-        cardToDownload.resetIndices();
-        startNextPicDownload();
-    });
 }
 
 void CardPictureLoaderWorkerWork::concludeImageLoad(const QImage &image)
@@ -341,4 +372,9 @@ void CardPictureLoaderWorkerWork::concludeImageLoad(const QImage &image)
 void CardPictureLoaderWorkerWork::picDownloadChanged()
 {
     picDownload = SettingsCache::instance().downloads().getPicDownload();
+    if (!picDownload && deferredRetry->isActive()) {
+        deferredRetry->stop();
+        cardToDownload.resetIndices();
+        startNextPicDownload();
+    }
 }

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h

@@ -6,7 +6,10 @@
 #include <QImage>
 #include <QLoggingCategory>
 #include <QObject>
+#include <QSet>
 #include <QString>
+#include <QTimer>
+#include <QUrl>
 #include <qtmetamacros.h>
 
 class ExactCard;
@@ -47,6 +50,9 @@ public:
     /** @brief Shared per-server 429 backoff state. */
     static const ServerRateLimiter &rateLimiter();
 
+    /** @brief A corrupt cached response gets one network retry per URL and work item. */
+    [[nodiscard]] bool shouldBypassCache(const QUrl &url) const;
+
     /**
      * @brief Starts downloading the next URL for this card.
      *
@@ -74,7 +80,12 @@ public slots:
     void handleNetworkReply(QNetworkReply *reply);
 
 private:
-    bool picDownload; ///< Whether network downloading is enabled
+    bool picDownload;                ///< Whether network downloading is enabled
+    QTimer *deferredRetry = nullptr; ///< Owned retry, cancellable when downloads are disabled
+    QSet<QUrl> retriedCacheUrls;
+
+    bool retryCachedUrl(const QUrl &url);
+    QSet<QUrl> redirectVisited; ///< Redirect chain for the current source URL
 
     static ServerRateLimiter s_rateLimiter; ///< Shared per-server 429 backoff state
 
@@ -87,13 +98,6 @@ private:
     /** @brief Processes a successful network reply. */
     void handleSuccessfulReply(QNetworkReply *reply);
 
-    /**
-     * @brief Attempts to read an image from a network reply.
-     * @param reply The network reply
-     * @return The loaded QImage or an empty image if reading failed
-     */
-    QImage tryLoadImageFromReply(QNetworkReply *reply);
-
     /**
      * @brief Finalizes the image loading process.
      * @param image The loaded image (empty if failed)
@@ -116,15 +120,15 @@ signals:
      */
     void imageLoaded(const ExactCard &card, const QImage &image);
 
-    /** @brief Emitted when a network request completes successfully. */
-    void requestSucceeded(const QUrl &url);
-
     /** @brief Request that a URL be downloaded. */
     void requestImageDownload(const QUrl &url, CardPictureLoaderWorkerWork *instance);
 
     /** @brief Emitted when a URL has been redirected. */
     void urlRedirected(const QUrl &originalUrl, const QUrl &redirectUrl);
 
+    /** @brief A redirect mapping is unsafe and should be removed. */
+    void redirectInvalidated(const QUrl &url);
+
     /** @brief Emitted when a server returned HTTP 429. */
     void rateLimited(const QString &host);
 

cockatrice/src/interface/card_picture_loader/redirect_chain.h

@@ -0,0 +1,37 @@
+#ifndef COCKATRICE_REDIRECT_CHAIN_H
+#define COCKATRICE_REDIRECT_CHAIN_H
+
+#include <QDateTime>
+#include <QHash>
+#include <QPair>
+#include <QSet>
+#include <QUrl>
+#include <utility>
+
+// Resolve previously accepted redirects without recursion. Invalid chains are
+// removed so subsequent requests can rediscover them from the source server.
+inline QUrl resolveImageRedirects(const QUrl &url, QHash<QUrl, QPair<QUrl, QDateTime>> &cache, int maxHops)
+{
+    QUrl resolved = url;
+    QSet<QUrl> visited;
+    for (int depth = 0; depth <= maxHops; ++depth) {
+        if (visited.contains(resolved)) {
+            break;
+        }
+        const auto it = cache.constFind(resolved);
+        if (it == cache.cend() || it.value().first.isEmpty()) {
+            return resolved;
+        }
+        if (depth == maxHops) {
+            break;
+        }
+        visited.insert(resolved);
+        resolved = it.value().first;
+    }
+    for (const QUrl &entry : std::as_const(visited)) {
+        cache.remove(entry);
+    }
+    return url;
+}
+
+#endif

tests/CMakeLists.txt

@@ -171,6 +171,7 @@ endfunction()
 
 # --- Unit tests ------------------------------------------------------------------
 
+cockatrice_add_test(NAME image_redirect_chain_test SOURCES image_redirect_chain_test.cpp)
 cockatrice_add_test(NAME expression_test SOURCES expression_test.cpp LIBS libcockatrice_utility)
 cockatrice_add_test(NAME clamped_arithmetic_test SOURCES clamped_arithmetic_test.cpp LIBS libcockatrice_utility)
 cockatrice_add_test(NAME test_age_formatting SOURCES test_age_formatting.cpp LIBS libcockatrice_utility)

tests/image_redirect_chain_test.cpp

@@ -0,0 +1,82 @@
+#include "../cockatrice/src/interface/card_picture_loader/redirect_chain.h"
+
+#include <gtest/gtest.h>
+
+namespace
+{
+using RedirectCache = QHash<QUrl, QPair<QUrl, QDateTime>>;
+
+QUrl url(int index)
+{
+    return QUrl(QStringLiteral("https://cards.example/%1").arg(index));
+}
+
+RedirectCache chain(int hops)
+{
+    RedirectCache cache;
+    for (int index = 0; index < hops; ++index) {
+        cache.insert(url(index), {url(index + 1), QDateTime()});
+    }
+    return cache;
+}
+} // namespace
+
+TEST(ImageRedirectChain, MissingHopReturnsLastKnownUrlWithoutRemovingMappings)
+{
+    auto cache = chain(2);
+    const auto original = cache;
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(2));
+    EXPECT_EQ(cache, original);
+    EXPECT_EQ(resolveImageRedirects(url(9), cache, 10), url(9));
+}
+
+TEST(ImageRedirectChain, ExactlyMaxHopsResolvesAndKeepsCache)
+{
+    auto cache = chain(10);
+    const auto original = cache;
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(10));
+    EXPECT_EQ(cache, original);
+}
+
+TEST(ImageRedirectChain, CycleReturnsOriginalAndPurgesVisitedMappings)
+{
+    auto cache = chain(2);
+    cache.insert(url(2), {url(1), QDateTime()});
+    cache.insert(url(9), {url(10), QDateTime()});
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(0));
+    EXPECT_FALSE(cache.contains(url(0)));
+    EXPECT_FALSE(cache.contains(url(1)));
+    EXPECT_FALSE(cache.contains(url(2)));
+    EXPECT_TRUE(cache.contains(url(9)));
+}
+
+TEST(ImageRedirectChain, OverlongChainReturnsOriginalAndPurgesVisitedMappings)
+{
+    auto cache = chain(12);
+    cache.insert(url(20), {url(21), QDateTime()});
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(0));
+    for (int index = 0; index < 10; ++index) {
+        EXPECT_FALSE(cache.contains(url(index)));
+    }
+    EXPECT_TRUE(cache.contains(url(10)));
+    EXPECT_TRUE(cache.contains(url(11)));
+    EXPECT_TRUE(cache.contains(url(20)));
+}
+
+TEST(ImageRedirectChain, EmptyTargetStopsAtItsSource)
+{
+    auto cache = chain(1);
+    cache.insert(url(1), {QUrl(), QDateTime()});
+    const auto original = cache;
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(1));
+    EXPECT_EQ(cache, original);
+}
+
+TEST(ImageRedirectChain, ZeroHopsDoesNotFollowOrRemoveMappings)
+{
+    auto cache = chain(1);
+    EXPECT_EQ(resolveImageRedirects(url(5), cache, 0), url(5));
+    EXPECT_TRUE(cache.contains(url(0)));
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 0), url(0));
+    EXPECT_TRUE(cache.contains(url(0)));
+}