From e4897126a1a8417ea19d07aaccc2bf0765c586af Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 3 May 2026 19:22:04 +0000 Subject: [PATCH] fix(okhttp): canonicalize DNS hosts and invalidate on call failure Two follow-ups from the resolver audit: 1. Lowercase the hostname at the public boundary (lookup, invalidate, restore). DNS is case-insensitive; OkHttp normally hands us lowercase but custom callers and persisted records can mix case. This prevents "Example.com" and "example.com" from creating separate cache entries. 2. Drop the cache entry when an OkHttp call to a host fails outright. Without this, a stale-cached IP that no longer works would be served for up to 24h before the soft TTL ran out. Hooking callFailed (final-stage signal after OkHttp tried every address) instead of per-attempt connectFailed avoids over-invalidating multi-A-record hosts where one IP is dead and OkHttp recovers via the next. - Media path: invalidation folded into MediaCallEventListener.finish alongside its existing timing logging. - Relay path: new DnsInvalidatingEventListener wired into OkHttpClientFactoryForRelays via eventListenerFactory. --- .../amethyst/service/okhttp/AmethystDns.kt | 23 ++++++--- .../okhttp/DnsInvalidatingEventListener.kt | 50 +++++++++++++++++++ .../service/okhttp/MediaCallEventListener.kt | 11 +++- .../okhttp/OkHttpClientFactoryForRelays.kt | 1 + .../service/okhttp/AmethystDnsTest.kt | 39 +++++++++++++++ 5 files changed, 115 insertions(+), 9 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/DnsInvalidatingEventListener.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDns.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDns.kt index e67d4c638..fa85d0c37 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDns.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDns.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.service.okhttp import okhttp3.Dns import java.net.InetAddress import java.net.UnknownHostException +import java.util.Locale import java.util.concurrent.CompletableFuture import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.ExecutionException @@ -85,25 +86,29 @@ class AmethystDns( private val dirty = AtomicBoolean(false) override fun lookup(hostname: String): List { - cache[hostname]?.let { entry -> + // DNS hostnames are case-insensitive; canonicalize so "Example.com" and "example.com" + // share a cache entry. OkHttp normally hands us lowercase, but custom callers may not. + val key = hostname.lowercase(Locale.ROOT) + + cache[key]?.let { entry -> val now = System.currentTimeMillis() if (entry.expiresAtMillis > now) { - return entry.unwrap(hostname) + return entry.unwrap(key) } // Soft-expired positive entry: serve stale, refresh in background. Negative // entries fall through to the sync path so transient failures recover quickly. if (entry.addresses.isNotEmpty()) { - triggerBackgroundRefresh(hostname) + triggerBackgroundRefresh(key) return entry.addresses } } val newFuture = CompletableFuture>() - val existing = inflight.putIfAbsent(hostname, newFuture) + val existing = inflight.putIfAbsent(key, newFuture) return if (existing == null) { - resolveAsLeader(hostname, newFuture) + resolveAsLeader(key, newFuture) } else { - awaitFollower(hostname, existing) + awaitFollower(key, existing) } } @@ -212,7 +217,8 @@ class AmethystDns( /** Drop a single host's cached entry. Call when a connection to it fails. */ fun invalidate(hostname: String) { - if (cache.remove(hostname) != null) dirty.set(true) + val key = hostname.lowercase(Locale.ROOT) + if (cache.remove(key) != null) dirty.set(true) } /** @@ -249,7 +255,8 @@ class AmethystDns( runCatching { InetAddress.getByName(literal) }.getOrNull() } if (addresses.isNotEmpty()) { - cache.putIfAbsent(record.hostname, Entry(addresses, record.expiresAtMillis)) + val key = record.hostname.lowercase(Locale.ROOT) + cache.putIfAbsent(key, Entry(addresses, record.expiresAtMillis)) } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/DnsInvalidatingEventListener.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/DnsInvalidatingEventListener.kt new file mode 100644 index 000000000..9feb9884e --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/DnsInvalidatingEventListener.kt @@ -0,0 +1,50 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.service.okhttp + +import okhttp3.Call +import okhttp3.EventListener +import java.io.IOException + +/** + * Drops a host's [AmethystDns] entry whenever an OkHttp call to it fails outright. We hook + * `callFailed` (final-stage signal after OkHttp has tried every address from the DNS lookup) + * rather than per-attempt `connectFailed`: a multi-A-record host with one bad IP fires the + * latter while OkHttp recovers via the next address, and we don't want to invalidate the cache + * just because the first IP was unreachable. + * + * Used by the relay client. The media path uses [MediaCallEventListener], which folds the same + * invalidation into its `finish` method alongside its existing timing logging. + */ +class DnsInvalidatingEventListener private constructor() : EventListener() { + override fun callFailed( + call: Call, + ioe: IOException, + ) { + AmethystDns.shared.invalidate(call.request().url.host) + } + + object Factory : EventListener.Factory { + private val INSTANCE = DnsInvalidatingEventListener() + + override fun create(call: Call): EventListener = INSTANCE + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/MediaCallEventListener.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/MediaCallEventListener.kt index 18875d661..4397112a8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/MediaCallEventListener.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/MediaCallEventListener.kt @@ -122,6 +122,16 @@ class MediaCallEventListener( call: Call, error: IOException?, ) { + val host = call.request().url.host + + // The whole call failed (after OkHttp exhausted every address from the DNS lookup). + // Drop the cached entry so the next attempt re-resolves instead of trying the same + // dead IPs for up to 24h. Per-attempt connectFailed isn't enough — a multi-A-record + // host can have one bad IP and OkHttp will recover by trying the next one. + if (error != null) { + AmethystDns.shared.invalidate(host) + } + val totalMs = (System.nanoTime() - callStartNanos) / 1_000_000 val isSlow = totalMs >= SLOW_CALL_THRESHOLD_MS val wasQueued = queuedAtStart > 0 @@ -129,7 +139,6 @@ class MediaCallEventListener( if (error == null && !isSlow && !wasQueued && !isDebug) return val ttfbMs = if (responseHeadersNanos > 0) (responseHeadersNanos - callStartNanos) / 1_000_000 else -1L - val host = call.request().url.host val reuseTag = if (connectionReused) "reused" else "new" val msg = diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/OkHttpClientFactoryForRelays.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/OkHttpClientFactoryForRelays.kt index fb63b3416..b5dc62b03 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/OkHttpClientFactoryForRelays.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/OkHttpClientFactoryForRelays.kt @@ -56,6 +56,7 @@ class OkHttpClientFactoryForRelays( .Builder() .dispatcher(myDispatcher) .dns(AmethystDns.shared) + .eventListenerFactory(DnsInvalidatingEventListener.Factory) .followRedirects(true) .followSslRedirects(true) .addInterceptor(DefaultContentTypeInterceptor(userAgent)) diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDnsTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDnsTest.kt index fcdcbfb73..45a86b6a6 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDnsTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/AmethystDnsTest.kt @@ -392,6 +392,45 @@ class AmethystDnsTest { assertEquals("In-memory entry wins", listOf(ip("1.1.1.1")), dns.lookup("relay.example")) } + @Test + fun `lookup is case-insensitive`() { + val upstream = CountingDns(mapOf("example.com" to listOf(ip("1.2.3.4")))) + val dns = AmethystDns(delegate = upstream) + + dns.lookup("Example.COM") + dns.lookup("example.com") + dns.lookup("ExAmPlE.cOm") + + assertEquals("Mixed-case lookups should share one cache entry", 1, upstream.calls("example.com")) + } + + @Test + fun `invalidate is case-insensitive`() { + val upstream = CountingDns(mapOf("example.com" to listOf(ip("1.2.3.4")))) + val dns = AmethystDns(delegate = upstream) + + dns.lookup("example.com") + dns.invalidate("EXAMPLE.com") + dns.lookup("example.com") + + assertEquals(2, upstream.calls("example.com")) + } + + @Test + fun `restore lowercases hostnames so subsequent lookups hit`() { + val upstream = CountingDns(mapOf("example.com" to listOf(ip("9.9.9.9")))) + val dns = AmethystDns(delegate = upstream) + + dns.restore( + listOf( + DnsCacheRecord("Example.COM", listOf("9.9.9.9"), System.currentTimeMillis() + 60_000), + ), + ) + + assertEquals(listOf(ip("9.9.9.9")), dns.lookup("example.com")) + assertEquals("Lookup should hit restored entry without upstream", 0, upstream.calls("example.com")) + } + @Test fun `dirty flag tracks positive writes`() { val upstream = CountingDns(mapOf("a.example" to listOf(ip("1.2.3.4"))))