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.
This commit is contained in:
@@ -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<InetAddress> {
|
||||
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<List<InetAddress>>()
|
||||
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))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
+50
@@ -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
|
||||
}
|
||||
}
|
||||
+10
-1
@@ -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 =
|
||||
|
||||
+1
@@ -56,6 +56,7 @@ class OkHttpClientFactoryForRelays(
|
||||
.Builder()
|
||||
.dispatcher(myDispatcher)
|
||||
.dns(AmethystDns.shared)
|
||||
.eventListenerFactory(DnsInvalidatingEventListener.Factory)
|
||||
.followRedirects(true)
|
||||
.followSslRedirects(true)
|
||||
.addInterceptor(DefaultContentTypeInterceptor(userAgent))
|
||||
|
||||
@@ -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"))))
|
||||
|
||||
Reference in New Issue
Block a user