fix(cache): respect max-age before ETag revalidation

bmc08gt · bmc08gt · commit 02c85f64f1c0 · 2026-06-26T09:47:18.000-04:00
ETagCacheStrategy was forcing a network trip on every cold start, causing token icons to flash. Parse Cache-Control max-age from cached response headers and only revalidate once the entry is stale.

Signed-off-by: Brandon McAnsh &lt;git@bmcreations.dev&gt;
diff --git a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/cache/ETagRevalidation.kt b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/cache/ETagRevalidation.kt
@@ -36,16 +36,17 @@ fun ImageRequest.Builder.etagRevalidation() = apply {
 }
 
 /**
- * A [CacheStrategy] that performs conditional revalidation using `ETag` / `Last-Modified`
- * headers stored in Coil's disk-cache metadata.
+ * A [CacheStrategy] that respects `Cache-Control: max-age` and performs conditional
+ * revalidation using `ETag` / `Last-Modified` headers stored in Coil's disk-cache metadata.
  *
  * **Read behaviour:**
  * - If [ETagRevalidationKey] is **not** set on the request, the cached response is returned
  *   immediately (same as [CacheStrategy.DEFAULT]).
- * - If the key **is** set, cached `etag` and `last-modified` headers are copied into
- *   `If-None-Match` / `If-Modified-Since` on the outgoing [NetworkRequest], forcing a
- *   network round-trip. The server can then respond with `304` to avoid re-downloading the
- *   image body.
+ * - If the key **is** set but the cached entry is still fresh (within `max-age`), the cached
+ *   response is returned without a network trip.
+ * - If the entry is stale, cached `etag` and `last-modified` headers are copied into
+ *   `If-None-Match` / `If-Modified-Since` on the outgoing [NetworkRequest]. The server can
+ *   then respond with `304` to avoid re-downloading the image body.
  *
  * **Write behaviour:** delegates entirely to [CacheStrategy.DEFAULT], which merges headers on
  * `304` responses and writes new bodies on `2xx` responses.
@@ -62,6 +63,10 @@ class ETagCacheStrategy : CacheStrategy {
             return CacheStrategy.ReadResult(cacheResponse)
         }
 
+        if (!isStale(cacheResponse)) {
+            return CacheStrategy.ReadResult(cacheResponse)
+        }
+
         val cachedHeaders = cacheResponse.headers
         val etag = cachedHeaders["etag"]
         val lastModified = cachedHeaders["last-modified"]
@@ -91,6 +96,27 @@ class ETagCacheStrategy : CacheStrategy {
     ): CacheStrategy.WriteResult {
         return CacheStrategy.DEFAULT.write(cacheResponse, networkRequest, networkResponse, options)
     }
+
+    companion object {
+        private val MAX_AGE_REGEX = Regex("""max-age\s*=\s*(\d+)""")
+
+        /**
+         * Returns `true` if the cached response has exceeded its `max-age`, or if
+         * `responseMillis` is missing (pre-existing cache entries).
+         */
+        internal fun isStale(cacheResponse: NetworkResponse): Boolean {
+            val responseMillis = cacheResponse.responseMillis
+            if (responseMillis <= 0L) return true
+
+            val cacheControl = cacheResponse.headers["cache-control"] ?: return true
+            val maxAgeSeconds = MAX_AGE_REGEX.find(cacheControl)
+                ?.groupValues?.get(1)?.toLongOrNull()
+                ?: return true
+
+            val ageMillis = System.currentTimeMillis() - responseMillis
+            return ageMillis > maxAgeSeconds * 1000
+        }
+    }
 }
 
 /**
diff --git a/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/cache/ETagCacheStrategyTest.kt b/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/cache/ETagCacheStrategyTest.kt
@@ -13,8 +13,10 @@ import org.robolectric.RuntimeEnvironment
 import org.robolectric.annotation.Config
 import kotlin.test.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
+import kotlin.test.assertTrue
 
 @OptIn(ExperimentalCoilApi::class)
 @RunWith(RobolectricTestRunner::class)
@@ -30,52 +32,121 @@ class ETagCacheStrategyTest {
         return Options(context = RuntimeEnvironment.getApplication(), extras = extras)
     }
 
-    private val baseCacheResponse = NetworkResponse(
-        code = 200,
-        headers = NetworkHeaders.EMPTY,
-    )
+    private fun freshCacheResponse(
+        maxAgeSeconds: Long = 3600,
+        etag: String? = null,
+        lastModified: String? = null,
+    ): NetworkResponse {
+        val headers = NetworkHeaders.Builder()
+            .set("cache-control", "public, max-age=$maxAgeSeconds")
+        if (etag != null) headers.set("etag", etag)
+        if (lastModified != null) headers.set("last-modified", lastModified)
+        return NetworkResponse(
+            code = 200,
+            responseMillis = System.currentTimeMillis() - 60_000, // 1 minute ago
+            headers = headers.build(),
+        )
+    }
+
+    private fun staleCacheResponse(
+        maxAgeSeconds: Long = 3600,
+        etag: String? = null,
+        lastModified: String? = null,
+    ): NetworkResponse {
+        val headers = NetworkHeaders.Builder()
+            .set("cache-control", "public, max-age=$maxAgeSeconds")
+        if (etag != null) headers.set("etag", etag)
+        if (lastModified != null) headers.set("last-modified", lastModified)
+        return NetworkResponse(
+            code = 200,
+            responseMillis = System.currentTimeMillis() - (maxAgeSeconds * 1000 + 60_000),
+            headers = headers.build(),
+        )
+    }
 
     private val baseNetworkRequest = NetworkRequest(url = "https://example.com/icon.png")
 
+    // region key disabled
+
     @Test
     fun `without key returns cache response`() = runTest {
-        val result = strategy.read(baseCacheResponse, baseNetworkRequest, options(false))
+        val result = strategy.read(staleCacheResponse(), baseNetworkRequest, options(false))
         assertNotNull(result.response)
         assertNull(result.request)
     }
 
+    // endregion
+
+    // region freshness
+
     @Test
-    fun `with key and cached etag adds If-None-Match`() = runTest {
-        val cached = baseCacheResponse.copy(
+    fun `with key and fresh cache returns cache response`() = runTest {
+        val cached = freshCacheResponse(etag = "\"abc123\"")
+        val result = strategy.read(cached, baseNetworkRequest, options(true))
+        assertNotNull(result.response)
+        assertNull(result.request)
+    }
+
+    @Test
+    fun `with key and stale cache revalidates`() = runTest {
+        val cached = staleCacheResponse(etag = "\"abc123\"")
+        val result = strategy.read(cached, baseNetworkRequest, options(true))
+        assertNotNull(result.request)
+        assertNull(result.response)
+    }
+
+    @Test
+    fun `with key and no cache-control header treats as stale`() = runTest {
+        val cached = NetworkResponse(
+            code = 200,
+            responseMillis = System.currentTimeMillis(),
             headers = NetworkHeaders.Builder()
                 .set("etag", "\"abc123\"")
-                .build()
+                .build(),
         )
         val result = strategy.read(cached, baseNetworkRequest, options(true))
         assertNotNull(result.request)
-        assertNull(result.response)
-        assertEquals("\"abc123\"", result.request!!.headers["if-none-match"])
     }
 
     @Test
-    fun `with key and cached last-modified adds If-Modified-Since`() = runTest {
-        val cached = baseCacheResponse.copy(
+    fun `with key and zero responseMillis treats as stale`() = runTest {
+        val cached = NetworkResponse(
+            code = 200,
+            responseMillis = 0L,
             headers = NetworkHeaders.Builder()
-                .set("last-modified", "Wed, 21 Oct 2015 07:28:00 GMT")
-                .build()
+                .set("cache-control", "public, max-age=3600")
+                .set("etag", "\"abc123\"")
+                .build(),
         )
         val result = strategy.read(cached, baseNetworkRequest, options(true))
         assertNotNull(result.request)
+    }
+
+    // endregion
+
+    // region conditional headers
+
+    @Test
+    fun `stale cache with etag adds If-None-Match`() = runTest {
+        val cached = staleCacheResponse(etag = "\"abc123\"")
+        val result = strategy.read(cached, baseNetworkRequest, options(true))
+        assertNotNull(result.request)
+        assertEquals("\"abc123\"", result.request!!.headers["if-none-match"])
+    }
+
+    @Test
+    fun `stale cache with last-modified adds If-Modified-Since`() = runTest {
+        val cached = staleCacheResponse(lastModified = "Wed, 21 Oct 2015 07:28:00 GMT")
+        val result = strategy.read(cached, baseNetworkRequest, options(true))
+        assertNotNull(result.request)
         assertEquals("Wed, 21 Oct 2015 07:28:00 GMT", result.request!!.headers["if-modified-since"])
     }
 
     @Test
-    fun `with key and both headers adds both conditional headers`() = runTest {
-        val cached = baseCacheResponse.copy(
-            headers = NetworkHeaders.Builder()
-                .set("etag", "\"abc123\"")
-                .set("last-modified", "Wed, 21 Oct 2015 07:28:00 GMT")
-                .build()
+    fun `stale cache with both headers adds both conditional headers`() = runTest {
+        val cached = staleCacheResponse(
+            etag = "\"abc123\"",
+            lastModified = "Wed, 21 Oct 2015 07:28:00 GMT",
         )
         val result = strategy.read(cached, baseNetworkRequest, options(true))
         assertNotNull(result.request)
@@ -84,11 +155,51 @@ class ETagCacheStrategyTest {
     }
 
     @Test
-    fun `with key and no cached headers sends plain request`() = runTest {
-        val result = strategy.read(baseCacheResponse, baseNetworkRequest, options(true))
+    fun `stale cache with no etag or last-modified sends plain request`() = runTest {
+        val cached = staleCacheResponse()
+        val result = strategy.read(cached, baseNetworkRequest, options(true))
         assertNotNull(result.request)
-        assertNull(result.response)
         assertNull(result.request!!.headers["if-none-match"])
         assertNull(result.request!!.headers["if-modified-since"])
     }
+
+    // endregion
+
+    // region isStale
+
+    @Test
+    fun `isStale returns false when within max-age`() {
+        val response = freshCacheResponse(maxAgeSeconds = 3600)
+        assertFalse(ETagCacheStrategy.isStale(response))
+    }
+
+    @Test
+    fun `isStale returns true when past max-age`() {
+        val response = staleCacheResponse(maxAgeSeconds = 3600)
+        assertTrue(ETagCacheStrategy.isStale(response))
+    }
+
+    @Test
+    fun `isStale returns true when no cache-control`() {
+        val response = NetworkResponse(
+            code = 200,
+            responseMillis = System.currentTimeMillis(),
+            headers = NetworkHeaders.EMPTY,
+        )
+        assertTrue(ETagCacheStrategy.isStale(response))
+    }
+
+    @Test
+    fun `isStale returns true when responseMillis is zero`() {
+        val response = NetworkResponse(
+            code = 200,
+            responseMillis = 0L,
+            headers = NetworkHeaders.Builder()
+                .set("cache-control", "public, max-age=3600")
+                .build(),
+        )
+        assertTrue(ETagCacheStrategy.isStale(response))
+    }
+
+    // endregion
 }
