Evaluate cache strategy before enforcing failure response code (#3401)

When a failure response (e.g. 404) was cached to disk via CACHEABLE_STATUS_CODES,
NetworkFetcher threw HttpException before invoking CacheStrategy.read(). This
prevented cache-aware strategies (e.g. CacheControlCacheStrategy) from ever
seeing the cached response, so a cached failure could never expire via
cache-control headers.

Move the failure code check until after CacheStrategy.read() returns a response.
When the strategy opts to reuse the cached response, the check still fires and
preserves existing behavior for the default strategy. When the strategy returns
a fresh request instead, the fetcher now proceeds to the network as expected.

Resolves #3391

Author: leesgit

coil-network-core/src/commonMain/kotlin/coil3/network/NetworkFetcher.kt

@@ -81,10 +81,13 @@ class NetworkFetcher(
                 // Return the image from the disk cache if the cache strategy agrees.
                 cacheResponse = snapshot.toNetworkResponseOrNull()
                 if (cacheResponse != null) {
-                    throwIfFailureResponseCode(cacheResponse)
-
                     readResult = cacheStrategy.value.read(cacheResponse, newRequest(), options)
                     if (readResult.response != null) {
+                        // Only enforce the failure code check when the strategy opts to use the
+                        // cached response. This lets cache-aware strategies (e.g. cache-control)
+                        // issue a fresh request when a cached failure (e.g. 404) has expired.
+                        throwIfFailureResponseCode(readResult.response)
+
                         return SourceFetchResult(
                             source = snapshot.toImageSource(),
                             mimeType = getMimeType(url, readResult.response.headers[CONTENT_TYPE]),

coil-network-core/src/commonTest/kotlin/coil3/network/NetworkFetcherTest.kt

@@ -204,6 +204,76 @@ class NetworkFetcherTest : RobolectricTest() {
         fileSystem.checkNoOpenFiles()
     }
 
+    @Test
+    fun cached404IsRefreshedWhenCacheStrategyRequestsNewRequest() = runTestAsync {
+        val url = "https://example.com/cached-404-refresh.jpg"
+        val expectedSize = 1_000
+
+        val fileSystem = FakeFileSystem()
+        val diskCache = DiskCache.Builder()
+            .directory(fileSystem.workingDirectory)
+            .fileSystem(fileSystem)
+            .maxSizeBytes(Long.MAX_VALUE)
+            .build()
+
+        // Pre-populate the disk cache with a cached 404 response.
+        val editor = diskCache.openEditor(url)!!
+        fileSystem.write(editor.metadata) {
+            CacheNetworkResponse.writeTo(NetworkResponse(code = 404), this)
+        }
+        fileSystem.write(editor.data) {
+            write(ByteArray(32))
+        }
+        editor.commit()
+
+        val networkClient = FakeNetworkClient(
+            respond = {
+                NetworkResponse(
+                    body = NetworkResponseBody(
+                        source = Buffer().apply { write(ByteArray(expectedSize)) },
+                    ),
+                )
+            },
+        )
+
+        // Custom strategy that always asks for a fresh network request, simulating an expired
+        // cached response (similar to what `CacheControlCacheStrategy` does when a cached
+        // failure has become stale).
+        val refreshingStrategy = object : CacheStrategy {
+            override suspend fun read(
+                cacheResponse: NetworkResponse,
+                networkRequest: NetworkRequest,
+                options: Options,
+            ) = CacheStrategy.ReadResult(networkRequest)
+
+            override suspend fun write(
+                cacheResponse: NetworkResponse?,
+                networkRequest: NetworkRequest,
+                networkResponse: NetworkResponse,
+                options: Options,
+            ) = CacheStrategy.WriteResult(networkResponse)
+        }
+
+        val fetcher = NetworkFetcher(
+            url = url,
+            options = Options(context),
+            networkClient = lazyOf(networkClient),
+            diskCache = lazyOf(diskCache),
+            cacheStrategy = lazyOf(refreshingStrategy),
+            connectivityChecker = lazyOf(ConnectivityChecker.ONLINE),
+            concurrentRequestStrategy = lazyOf(ConcurrentRequestStrategy.UNCOORDINATED),
+        )
+
+        // The cached 404 should no longer short-circuit with `HttpException` when the
+        // cache strategy opts to refresh; a network request should be issued instead.
+        val result = fetcher.fetch()
+        assertIs<SourceFetchResult>(result)
+        assertEquals(1, networkClient.requests.size)
+
+        diskCache.shutdown()
+        fileSystem.checkNoOpenFiles()
+    }
+
     class FakeNetworkClient(
         private val respond: suspend (NetworkRequest) -> NetworkResponse,
     ) : NetworkClient {