Fix cache misses with content negotiation

ktor-client/ktor-client-core/build.gradle.kts

@@ -34,6 +34,7 @@ kotlin {
             implementation(projects.ktorTestDispatcher)
             implementation(projects.ktorClientMock)
             implementation(projects.ktorServerTestHost)
+            implementation(projects.ktorClientContentNegotiation)
         }
     }
 }

ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt

@@ -393,8 +393,8 @@ internal fun mergedHeadersLookup(
         }
 
         else -> {
-            val value = content.headers.getAll(header) ?: allHeadersExtractor(header) ?: emptyList()
-            value.joinToString(";")
+            val value = content.headers.getAll(header) ?: allHeadersExtractor(header)
+            value.joinHeaderValues()
         }
     }
 }

ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.kt

@@ -50,14 +50,17 @@ public class HttpCacheEntry internal constructor(
     }
 }
 
+// RFC 7230 §3.2.2: multiple values for the same header field are equivalent to a comma-separated list.
+internal fun List<String>?.joinHeaderValues(): String = this?.joinToString(",") ?: ""
+
 internal fun HttpResponse.varyKeys(): Map<String, String> {
     val validationKeys = vary() ?: return emptyMap()
 
     val result = mutableMapOf<String, String>()
     val requestHeaders = call.request.headers
 
     for (key in validationKeys) {
-        result[key.lowercase()] = requestHeaders.getAll(key)?.joinToString(",") ?: ""
+        result[key.lowercase()] = requestHeaders.getAll(key).joinHeaderValues()
     }
 
     return result

ktor-client/ktor-client-core/common/test/HttpCacheTest.kt

@@ -2,17 +2,24 @@
  * Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
  */
 
+import io.ktor.client.*
 import io.ktor.client.call.*
+import io.ktor.client.engine.mock.*
 import io.ktor.client.plugins.api.*
 import io.ktor.client.plugins.cache.*
+import io.ktor.client.plugins.contentnegotiation.*
 import io.ktor.client.request.*
 import io.ktor.client.statement.*
 import io.ktor.http.*
+import io.ktor.http.content.*
+import io.ktor.serialization.*
 import io.ktor.server.response.*
 import io.ktor.server.routing.*
 import io.ktor.server.testing.*
 import io.ktor.util.*
+import io.ktor.util.reflect.*
 import io.ktor.utils.io.*
+import io.ktor.utils.io.charsets.*
 import kotlinx.coroutines.*
 import kotlinx.coroutines.test.TestResult
 import kotlinx.coroutines.test.runTest
@@ -162,6 +169,118 @@ class HttpCacheTest {
         assertEquals(firstResponse, secondResponse)
     }
 
+    // A no-op converter whose only effect is to register application/json as an Accept type.
+    // ContentNegotiation unconditionally appends Accept for every registered codec via
+    // `request.accept(it.contentTypeToSend)`, which creates a second header value when the
+    // caller also sets Accept explicitly. This is sufficient to trigger the varyKey mismatch.
+    private val noOpJsonConverter = object : ContentConverter {
+        override suspend fun serialize(
+            contentType: ContentType,
+            charset: Charset,
+            typeInfo: TypeInfo,
+            value: Any?
+        ): OutgoingContent? = null
+
+        override suspend fun deserialize(
+            charset: Charset,
+            typeInfo: TypeInfo,
+            content: ByteReadChannel
+        ): Any? = null
+    }
+
+    private fun MockRequestHandleScope.cacheableJsonResponse() = respond(
+        content = """{"id":1}""",
+        headers = headersOf(
+            HttpHeaders.ContentType to listOf(ContentType.Application.Json.toString()),
+            HttpHeaders.CacheControl to listOf("max-age=60"),
+            HttpHeaders.Vary to listOf("Accept"),
+            HttpHeaders.ETag to listOf("\"abc123\""),
+        )
+    )
+
+    /**
+     * Regression test: varyKeys() and mergedHeadersLookup() must produce identical strings for
+     * the same multi-value header so that findResponse() can match cached entries.
+     *
+     * Before the fix, varyKeys() joined with "," while mergedHeadersLookup() joined with ";",
+     * so stored "a,b" never equalled looked-up "a;b" and every request was a cache miss.
+     * Both now delegate to joinHeaderValues() (RFC 7230 §3.2.2 comma separator).
+     */
+    @Test
+    fun varyKeysSeparatorMatchesMergedHeadersLookupSeparator() {
+        val requestHeaders = Headers.build {
+            append(HttpHeaders.Accept, "application/vnd.github+json") // set by caller
+            append(HttpHeaders.Accept, "application/json")            // appended by ContentNegotiation
+        }
+
+        // How HttpCacheEntry.varyKeys() stores the value (joinHeaderValues → comma):
+        val stored = requestHeaders.getAll(HttpHeaders.Accept).joinHeaderValues()
+        //  → "application/vnd.github+json,application/json"
+
+        // How mergedHeadersLookup() now computes the lookup value (joinHeaderValues → comma):
+        val lookup = requestHeaders.getAll(HttpHeaders.Accept).joinHeaderValues()
+        //  → "application/vnd.github+json,application/json"
+
+        assertEquals(stored, lookup)
+    }
+
+    /**
+     * Integration test: a fresh cacheable response (max-age=60, Vary: Accept) is never served
+     * from cache when ContentNegotiation is installed, because the comma/semicolon separator
+     * mismatch in findResponse() causes it to always discard the cached entry.
+     */
+    @Test
+    fun cacheHitOccursWithContentNegotiationAndVaryAccept() =
+        runTest {
+            var serverCallCount = 0
+            val client = HttpClient(
+                MockEngine { _ ->
+                    serverCallCount++
+                    cacheableJsonResponse()
+                }
+            ) {
+                install(HttpCache)
+                install(ContentNegotiation) {
+                    register(ContentType.Application.Json, noOpJsonConverter)
+                }
+            }
+
+            val url = "https://example.com/api/resource"
+            client.get(url) { accept(ContentType.parse("application/vnd.github+json")) }
+            client.get(url) { accept(ContentType.parse("application/vnd.github+json")) }
+
+            // First response is cacheable (max-age=60) and the second request is identical.
+            // Expected: second request served from cache → serverCallCount == 1.
+            assertEquals(1, serverCallCount)
+            client.close()
+        }
+
+    /**
+     * Counterpart: without ContentNegotiation each header has a single value, so the
+     * "," vs ";" mismatch doesn't matter and the cache hit works correctly.
+     */
+    @Test
+    fun freshCacheableEntryWithVaryAcceptIsServedFromCacheWhenContentNegotiationIsNotInstalled() =
+        runTest {
+            var serverCallCount = 0
+            val client = HttpClient(
+                MockEngine { _ ->
+                    serverCallCount++
+                    cacheableJsonResponse()
+                }
+            ) {
+                install(HttpCache)
+                // ContentNegotiation intentionally absent: one Accept value → separator irrelevant
+            }
+
+            val url = "https://example.com/api/resource"
+            client.get(url) { accept(ContentType.parse("application/vnd.github+json")) }
+            client.get(url) { accept(ContentType.parse("application/vnd.github+json")) }
+
+            assertEquals(1, serverCallCount)
+            client.close()
+        }
+
     private fun testApplication(block: suspend ApplicationTestBuilder.() -> Unit): TestResult {
         return if (!PlatformUtils.IS_BROWSER) {
             testApplication(EmptyCoroutineContext, block)