From b1b6190d2fe1f9e861f01cb1d53cebb296734b73 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sat, 18 Jul 2026 18:32:54 -0400 Subject: [PATCH] fix(relay): forgive reconnect backoff when the network or transport changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A relay's reconnect backoff was process-global and network-blind: the delay earned on one network was still being served out on the next. The only reset signal was OkHttpClient reference identity, which is rebuilt off the metered bit, so it fired for wifi<->cellular and nothing else. Wifi A -> wifi B, a VPN coming up, a captive portal clearing, and metered-wifi -> cellular all left every relay parked on a penalty earned against a network the device had left — up to five minutes of silence on a network that might reach the relay instantly. The transient Off that would have reset things is swallowed by the 200ms debounce in ConnectivityFlow, so it never rescued those cases. Key the decision on ConnectivityStatus.Active.networkId instead, which is the same signal SurgeDns already uses to stale its cache, and treat a genuine network change as a full pool rebuild: every socket is bound to an interface that no longer carries traffic, and needsToReconnect() cannot see that because it only compares the proxy and the timeouts. Also treat a Tor policy flip as a transport change. Flipping a Tor toggle while Tor is already up leaves both OkHttpClient references identical, so a relay whose transport just changed kept waiting out a backoff earned on the other transport. Only TorRelaySettings is compared, not the relay sets that TorRelayEvaluation also carries — those churn while an account's relay lists load (observed firing three times in one cold start), and forgiving the whole pool every time any list updates is far more damage than it repairs. Adds IRelayClient.resetBackoff() (default no-op, so the existing fakes and BleNostrClient are unaffected) rather than reusing ignoreRetryDelays, which only skips the gate for a single attempt and still doubles the stored delay — a relay that failed that one dial came back worse off than before. INostrClient.resetBackoff() is deliberately separate from reconnect(): the latter debounces, so folding this into a coalescing command would let a later request silently drop the reset. The decision table moves into RelayProxyClientConnector.apply() so it can be exercised directly, without a debounce and a shared StateFlow in the way. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../relayClient/RelayProxyClientConnector.kt | 166 +++++++++---- .../RelayProxyClientConnectorTest.kt | 233 ++++++++++++++++++ .../nip01Core/relay/client/INostrClient.kt | 11 + .../nip01Core/relay/client/NostrClient.kt | 6 + .../nip01Core/relay/client/pool/RelayPool.kt | 7 + .../relay/client/single/IRelayClient.kt | 18 ++ .../client/single/basic/BasicRelayClient.kt | 8 + .../basic/BasicRelayClientBackoffTest.kt | 100 ++++++++ 8 files changed, 504 insertions(+), 45 deletions(-) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnectorTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnector.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnector.kt index f1f2eee12b..5e93a4dc0b 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnector.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnector.kt @@ -20,6 +20,7 @@ */ package com.vitorpamplona.amethyst.service.relayClient +import com.vitorpamplona.amethyst.commons.tor.TorRelaySettings import com.vitorpamplona.amethyst.model.torState.TorRelayEvaluation import com.vitorpamplona.amethyst.service.connectivity.ConnectivityStatus import com.vitorpamplona.amethyst.ui.tor.TorServiceStatus @@ -63,6 +64,27 @@ class RelayProxyClientConnector( private var lastTorConnection: OkHttpClient? = null private var lastClearConnection: OkHttpClient? = null + // The network we were last on. The OkHttp clients above are rebuilt off the metered + // bit, so they only tell us about wifi<->cellular; they say nothing about wifi A -> + // wifi B, a VPN coming up, or a captive portal clearing. Those all mint a new + // networkHandle, and after them every existing socket is bound to an interface that + // is gone and every accumulated backoff was earned against a network we have left. + private var lastNetworkId: Long? = null + + // The user's Tor preferences. TorRelayEvaluation has no equals() and a fresh instance is + // emitted on unrelated churn, so we track the settings by value. Without this, flipping a + // Tor toggle while Tor is already up leaves both OkHttpClient references identical -> + // transportChanged=false -> a relay parked on a 5-minute backoff keeps waiting it out on + // a transport it is no longer using. + // + // Deliberately only [TorRelaySettings], not the trusted/DM/money relay sets that + // TorRelayEvaluation also carries: those churn constantly while an account's relay lists + // load from the network (observed firing three times during a single cold start). A relay + // moving between classifications can flip its transport too, but forgiving the whole + // pool's backoff every time any relay list updates is far more damage than making that + // one relay serve out its delay. + private var lastTorSettings: TorRelaySettings? = null + @OptIn(FlowPreview::class) val relayServices = combine( @@ -74,51 +96,8 @@ class RelayProxyClientConnector( ) { torSettings, torConnection, clearConnection, connectivity, torStatus -> RelayServiceInfra(torSettings, torConnection, clearConnection, connectivity, torStatus) }.debounce(100) - .onEach { - when { - it.connectivity is ConnectivityStatus.StartingService -> { - // ignore - } - - it.connectivity is ConnectivityStatus.Off -> { - Log.d("ManageRelayServices") { "Connectivity Off: Pausing Relay Services ${it.connectivity}" } - if (client.isActive()) { - client.disconnect() - } - if (it.torStatus is TorServiceStatus.Active) { - Log.d("ManageRelayServices", "Connectivity off, Tor idle") - } - } - - it.connectivity is ConnectivityStatus.Active && !client.isActive() -> { - Log.d("ManageRelayServices", "Connectivity On: Resuming Relay Services") - - if (it.torStatus is TorServiceStatus.Active) { - Log.d("ManageRelayServices", "Connectivity resumed, Tor active") - } - - // only calls this if the client is not active. Otherwise goes to the else below - client.connect() - } - - else -> { - // Only skip the per-relay exponential backoff when the actual HTTP - // transport changed. Otherwise (e.g. Tor still bootstrapping, the SOCKS - // port not yet listening) honor each relay's backoff so we don't - // reconnect-fail-reconnect on every unrelated infrastructure event. - val transportChanged = - it.torConnection !== lastTorConnection || it.clearConnection !== lastClearConnection - lastTorConnection = it.torConnection - lastClearConnection = it.clearConnection - - Log.d("ManageRelayServices") { "Relay Services have changed, reconnecting relays that need to (transportChanged=$transportChanged)" } - client.reconnect( - onlyIfChanged = true, - ignoreRetryDelays = transportChanged, - ) - } - } - }.onStart { + .onEach { apply(it) } + .onStart { Log.d("ManageRelayServices", "Resuming Relay Services") client.connect() }.onCompletion { @@ -130,4 +109,101 @@ class RelayProxyClientConnector( SharingStarted.WhileSubscribed(30000), null, ) + + /** + * Decides what a change in the relay infrastructure means for the pool. Split out of the + * flow so the decision table can be exercised directly, without a debounce and a shared + * StateFlow in the way. + */ + fun apply(infra: RelayServiceInfra) { + val networkId = (infra.connectivity as? ConnectivityStatus.Active)?.networkId + val torSettings = infra.evaluator.torSettings + + when { + infra.connectivity is ConnectivityStatus.StartingService -> { + // ignore + } + + infra.connectivity is ConnectivityStatus.Off -> { + Log.d("ManageRelayServices") { "Connectivity Off: Pausing Relay Services ${infra.connectivity}" } + if (client.isActive()) { + client.disconnect() + } + if (infra.torStatus is TorServiceStatus.Active) { + Log.d("ManageRelayServices", "Connectivity off, Tor idle") + } + // disconnect() already cleared every relay's backoff. Forget the network + // so the next Active is treated as a fresh start rather than a change. + lastNetworkId = null + } + + infra.connectivity is ConnectivityStatus.Active && !client.isActive() -> { + Log.d("ManageRelayServices", "Connectivity On: Resuming Relay Services") + + if (infra.torStatus is TorServiceStatus.Active) { + Log.d("ManageRelayServices", "Connectivity resumed, Tor active") + } + + // only calls this if the client is not active. Otherwise goes to the else below + client.connect() + lastNetworkId = networkId + lastTorSettings = torSettings + lastTorConnection = infra.torConnection + lastClearConnection = infra.clearConnection + } + + else -> { + // Only skip the per-relay exponential backoff when the actual HTTP + // transport changed. Otherwise (e.g. Tor still bootstrapping, the SOCKS + // port not yet listening) honor each relay's backoff so we don't + // reconnect-fail-reconnect on every unrelated infrastructure event. + val transportChanged = + infra.torConnection !== lastTorConnection || infra.clearConnection !== lastClearConnection + + // A different network entirely. Every socket is bound to an interface that + // no longer carries traffic, and needsToReconnect() cannot see that (it only + // compares the proxy and the timeouts), so those sockets would otherwise sit + // there until OkHttp's 120s ping finally fails. + val networkChanged = networkId != null && lastNetworkId != null && networkId != lastNetworkId + + // Same network and same OkHttp clients, but the user re-classified which + // relays go through Tor. The relays whose transport flipped must re-dial now. + val torPolicyChanged = lastTorSettings != null && torSettings != lastTorSettings + + val previousNetworkId = lastNetworkId + + lastTorConnection = infra.torConnection + lastClearConnection = infra.clearConnection + lastNetworkId = networkId ?: lastNetworkId + lastTorSettings = torSettings + + if (networkChanged) { + Log.d("ManageRelayServices") { + "Network identity changed ($previousNetworkId -> $networkId), rebuilding every relay connection" + } + // Full teardown: disconnect() drops the dead sockets AND clears each + // relay's backoff, so the new network starts from a clean slate. + client.reconnect(onlyIfChanged = false, ignoreRetryDelays = true) + } else { + val freshStart = transportChanged || torPolicyChanged + if (freshStart) { + // The failures behind the current backoffs were measured against a + // transport we are no longer using. ignoreRetryDelays alone only + // skips the gate once and still doubles the stored delay, so a relay + // that fails this one dial would come back worse off than before. + client.resetBackoff() + } + + Log.d("ManageRelayServices") { + "Relay Services have changed, reconnecting relays that need to " + + "(transportChanged=$transportChanged torPolicyChanged=$torPolicyChanged)" + } + client.reconnect( + onlyIfChanged = true, + ignoreRetryDelays = freshStart, + ) + } + } + } + } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnectorTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnectorTest.kt new file mode 100644 index 0000000000..1b3f8b9da8 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/RelayProxyClientConnectorTest.kt @@ -0,0 +1,233 @@ +/* + * 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.relayClient + +import com.vitorpamplona.amethyst.commons.tor.TorRelaySettings +import com.vitorpamplona.amethyst.commons.tor.TorType +import com.vitorpamplona.amethyst.model.torState.TorRelayEvaluation +import com.vitorpamplona.amethyst.service.connectivity.ConnectivityStatus +import com.vitorpamplona.amethyst.service.relayClient.RelayProxyClientConnector.RelayServiceInfra +import com.vitorpamplona.amethyst.ui.tor.TorServiceStatus +import com.vitorpamplona.quartz.nip01Core.relay.client.EmptyNostrClient +import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient +import io.mockk.mockk +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.MutableStateFlow +import okhttp3.OkHttpClient +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Guards the decision table in [RelayProxyClientConnector.apply]: which environment changes + * are allowed to forgive a relay's accumulated reconnect backoff. + * + * Before this was keyed on network identity, the only signal was OkHttpClient reference + * identity, which is rebuilt off the metered bit. That misses every transition where + * metered-ness does not flip — wifi A -> wifi B, a VPN coming up, a captive portal clearing, + * metered wifi -> cellular — leaving relays parked on a backoff earned against a network the + * device already left. It also missed a Tor policy flip while Tor was already up, where both + * client references stay identical. + */ +class RelayProxyClientConnectorTest { + /** Records what the connector asked the relay pool to do. */ + class RecordingClient : INostrClient by EmptyNostrClient() { + var backoffResets = 0 + val reconnects = mutableListOf>() // onlyIfChanged to ignoreRetryDelays + var disconnects = 0 + private var active = false + + override fun isActive() = active + + override fun connect() { + active = true + } + + override fun disconnect() { + active = false + disconnects++ + } + + override fun resetBackoff() { + backoffResets++ + } + + override fun reconnect( + onlyIfChanged: Boolean, + ignoreRetryDelays: Boolean, + ) { + reconnects.add(onlyIfChanged to ignoreRetryDelays) + } + + /** True when the pool was told to tear everything down and rebuild. */ + fun sawFullRebuild() = reconnects.any { !it.first } + + fun sawBackoffForgiven() = backoffResets > 0 || sawFullRebuild() + + fun clear() { + backoffResets = 0 + disconnects = 0 + reconnects.clear() + } + } + + // Only reference identity matters, and building a real OkHttpClient pulls in + // android.util.Log, which is not usable in JVM unit tests. + private val torClient = mockk() + private val clearClient = mockk() + + private val client = RecordingClient() + + private val connector = + RelayProxyClientConnector( + torEvaluator = MutableStateFlow(evaluation()), + torConnection = MutableStateFlow(torClient), + clearConnection = MutableStateFlow(clearClient), + connectivityStatus = MutableStateFlow(ConnectivityStatus.Off), + torStatus = MutableStateFlow(TorServiceStatus.Off), + client = client, + // The flow itself is never collected here; apply() is driven directly. + scope = CoroutineScope(Dispatchers.Unconfined), + ) + + private fun evaluation(settings: TorRelaySettings = TorRelaySettings()) = + TorRelayEvaluation( + torSettings = settings, + trustedRelayList = emptySet(), + dmRelayList = emptySet(), + ) + + private fun infra( + networkId: Long = 1L, + isMobile: Boolean = false, + tor: OkHttpClient = torClient, + clear: OkHttpClient = clearClient, + evaluation: TorRelayEvaluation = evaluation(), + torStatus: TorServiceStatus = TorServiceStatus.Off, + ) = RelayServiceInfra( + evaluator = evaluation, + torConnection = tor, + clearConnection = clear, + connectivity = ConnectivityStatus.Active(networkId, isMobile), + torStatus = torStatus, + ) + + /** + * Runs the first Active event — which activates the client and records the baseline + * network and transport — then clears the recording so each test observes only its + * own transition. + */ + private fun settleOnFirstNetwork() { + connector.apply(infra(networkId = 1L)) + client.clear() + } + + @Test + fun `a new network forgives the backoff even when metered-ness does not change`() { + settleOnFirstNetwork() + + // metered wifi -> cellular: same isMobile, so the OkHttp clients are NOT rebuilt. + connector.apply(infra(networkId = 2L)) + + assertTrue( + "A different network must forgive backoffs earned on the previous one", + client.sawBackoffForgiven(), + ) + assertTrue( + "Sockets bound to the old interface are dead; the pool must be rebuilt, not " + + "merely asked to reconnect what needsToReconnect() can see", + client.sawFullRebuild(), + ) + } + + @Test + fun `flipping a tor toggle while tor is already up forgives the backoff`() { + settleOnFirstNetwork() + + // Same network, same OkHttpClient instances: only the routing policy changed. + connector.apply( + infra( + networkId = 1L, + evaluation = evaluation(TorRelaySettings(torType = TorType.INTERNAL, dmRelaysViaTor = true)), + ), + ) + + assertEquals( + "A relay whose transport just flipped must not wait out a backoff earned on the other transport", + 1, + client.backoffResets, + ) + assertEquals(listOf(true to true), client.reconnects) + } + + @Test + fun `unrelated churn on the same network leaves the backoff alone`() { + settleOnFirstNetwork() + + // Tor bootstrap progress: nothing about the transport or the network changed. + connector.apply(infra(networkId = 1L, torStatus = TorServiceStatus.Active(9050))) + + assertEquals( + "Backoff must survive unrelated infrastructure events, or dead relays get hammered", + 0, + client.backoffResets, + ) + assertTrue("No full rebuild for noise", !client.sawFullRebuild()) + assertEquals(listOf(true to false), client.reconnects) + } + + @Test + fun `a rebuilt transport on the same network forgives the backoff`() { + settleOnFirstNetwork() + + // Tor's SOCKS port came up: the Tor-routed OkHttpClient is a new instance. + connector.apply(infra(networkId = 1L, tor = mockk())) + + assertEquals(1, client.backoffResets) + assertEquals(listOf(true to true), client.reconnects) + } + + /** + * Losing connectivity already resets every relay through disconnect(), and coming back + * dials the whole pool through connect(). Treating the new network as a change on top of + * that would tear down a pool that was just rebuilt. + */ + @Test + fun `regaining connectivity on a different network does not double-rebuild`() { + settleOnFirstNetwork() + + connector.apply( + RelayServiceInfra(evaluation(), torClient, clearClient, ConnectivityStatus.Off, TorServiceStatus.Off), + ) + assertEquals("Losing the network must pause the pool", 1, client.disconnects) + client.clear() + + // back online, on a different network than the one we lost. + connector.apply(infra(networkId = 7L)) + + assertTrue( + "connect() already dials every relay from a clean backoff; no rebuild needed", + !client.sawFullRebuild(), + ) + assertEquals(0, client.backoffResets) + } +} diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/INostrClient.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/INostrClient.kt index 35a09a9748..f57e48c462 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/INostrClient.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/INostrClient.kt @@ -45,6 +45,17 @@ interface INostrClient : AutoCloseable { ignoreRetryDelays: Boolean = false, ) + /** + * Clears the accumulated reconnect backoff of every relay in the pool, so the next + * [reconnect] dials immediately instead of serving out a penalty earned on a network + * or transport that is no longer in use. See [IRelayClient.resetBackoff]. + * + * Kept separate from [reconnect] on purpose: implementations debounce reconnect + * requests, and folding this into a coalescing command would let a later request + * silently drop the reset. + */ + fun resetBackoff() { } + fun isActive(): Boolean /** diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/NostrClient.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/NostrClient.kt index 2e98749f34..8378d8140a 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/NostrClient.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/NostrClient.kt @@ -208,6 +208,12 @@ class NostrClient( refreshConnection.tryEmit(Reconnect(onlyIfChanged, ignoreRetryDelays)) } + override fun resetBackoff() { + // Applied eagerly rather than through refreshConnection: that flow debounces, + // so a reconnect emitted within 200ms would replace this one and lose the reset. + relayPool.resetBackoff() + } + override fun subscribe( subId: String, filters: Map>, diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/pool/RelayPool.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/pool/RelayPool.kt index 8b6bd87516..22c2e0003b 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/pool/RelayPool.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/pool/RelayPool.kt @@ -79,6 +79,13 @@ class RelayPool( decoder = decoder, ) + /** Clears every relay's reconnect backoff. See [IRelayClient.resetBackoff]. */ + fun resetBackoff() { + relays.forEach { url, relay -> + relay.resetBackoff() + } + } + fun reconnectIfNeedsTo(ignoreRetryDelays: Boolean = false) { relays.forEach { url, relay -> if (relay.isConnected()) { diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/IRelayClient.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/IRelayClient.kt index 06e2ac4f58..5a8e936714 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/IRelayClient.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/IRelayClient.kt @@ -32,6 +32,24 @@ interface IRelayClient { fun connectAndSyncFiltersIfDisconnected(ignoreRetryDelays: Boolean = false) + /** + * Forgets the accumulated reconnect backoff without touching the socket. + * + * Call when the conditions that produced the failures no longer apply — the + * device moved to a different network, or the relay's transport changed (Tor + * came up, or the user re-classified this relay). Past failures were measured + * against an environment that no longer exists, so holding a relay at a 5-minute + * delay would keep it dark for minutes on a network that might reach it instantly. + * + * Unlike [disconnect] this leaves a live connection alone: it only clears the + * penalty a *disconnected* relay would otherwise have to wait out. + * + * No-op by default: only transports that actually throttle reconnects (see + * [com.vitorpamplona.quartz.nip01Core.relay.client.single.basic.BasicRelayClient]) + * have anything to forget. + */ + fun resetBackoff() { } + fun isConnected(): Boolean fun sendOrConnectAndSync(cmd: Command) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClient.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClient.kt index b495d42dbb..7a45428db5 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClient.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClient.kt @@ -289,6 +289,14 @@ open class BasicRelayClient( } } + override fun resetBackoff() { + // Same two fields disconnect() clears, but deliberately not the socket: a relay + // that is currently connected must keep its session. This only forgives the wait + // a disconnected relay would otherwise serve out. + delayToConnectInSeconds = DELAY_TO_RECONNECT_IN_SECS + lastConnectTentativeInSeconds = 0L + } + fun upRelayDelayToConnect() { if (delayToConnectInSeconds < TimeUtils.FIVE_MINUTES) { delayToConnectInSeconds = delayToConnectInSeconds * 2 diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClientBackoffTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClientBackoffTest.kt index b9c2cd279c..0ba77832c5 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClientBackoffTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/single/basic/BasicRelayClientBackoffTest.kt @@ -24,6 +24,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.EmptyConnection import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocketListener import kotlin.test.Test +import kotlin.test.assertEquals import kotlin.test.assertTrue /** @@ -65,6 +66,33 @@ class BasicRelayClientBackoffTest { } } + /** + * Ticks until the relay dials again, then stops immediately — leaving the clock + * parked right after a failed attempt. Anchors tests that need to reason about the + * remaining backoff window instead of landing at an arbitrary point in the cycle. + * Returns false if no attempt happened within [limitSeconds]. + */ + private fun tickUntilNextAttempt( + client: BasicRelayClient, + builder: FakeWebsocketBuilder, + clock: MutableClock, + limitSeconds: Long, + tickSeconds: Long = 3, + onConnectAttempt: (WebSocketListener) -> Unit, + ): Boolean { + val end = clock.now + limitSeconds + while (clock.now < end) { + clock.now += tickSeconds + val before = builder.connectAttempts + client.connectAndSyncFiltersIfDisconnected() + if (builder.connectAttempts > before) { + onConnectAttempt(builder.lastListener) + return true + } + } + return false + } + private fun newClient( builder: FakeWebsocketBuilder, clock: MutableClock, @@ -154,4 +182,76 @@ class BasicRelayClientBackoffTest { "Expected backoff to reset after a stable session: no reconnect within 15s of a clean close", ) } + + /** + * A relay that failed its way up to the 5-minute ceiling on one network must dial + * immediately once the device moves to another network — the failures were measured + * against an environment that no longer exists. Without [BasicRelayClient.resetBackoff] + * the relay stays dark for up to 5 more minutes on a network that may reach it instantly. + */ + @Test + fun resetBackoffDialsImmediatelyAfterAMaxedOutBackoff() { + val builder = FakeWebsocketBuilder() + val clock = MutableClock() + val client = newClient(builder, clock) + + // drive the backoff to its ceiling with an unreachable host. + client.connect() + builder.lastListener.onFailure(RuntimeException("Unable to resolve host"), null, null) + runTicks(client, builder, clock, totalSeconds = 60 * 60) { listener -> + listener.onFailure(RuntimeException("Unable to resolve host"), null, null) + } + + // Tick until the next attempt actually fires, so the clock sits at a known point + // in the backoff cycle (right after a failure) rather than wherever the hour ended. + // Without this anchor the probe window below can straddle a legitimate retry. + val anchored = + tickUntilNextAttempt(client, builder, clock, limitSeconds = 20 * 60) { listener -> + listener.onFailure(RuntimeException("Unable to resolve host"), null, null) + } + assertTrue(anchored, "Test setup: expected a retry within 20 min to anchor the cycle") + + // confirm it really is parked: a full minute of ticks buys no attempt. + val parkedAt = builder.connectAttempts + runTicks(client, builder, clock, totalSeconds = 60) { listener -> + listener.onFailure(RuntimeException("Unable to resolve host"), null, null) + } + assertEquals( + parkedAt, + builder.connectAttempts, + "Test setup: relay should be sitting on a long backoff", + ) + + // the network changed underneath us. + client.resetBackoff() + + client.connectAndSyncFiltersIfDisconnected() + assertEquals( + parkedAt + 1, + builder.connectAttempts, + "Expected resetBackoff to let the relay dial at once on the new network", + ) + } + + /** resetBackoff is not a disconnect: a healthy session must survive it. */ + @Test + fun resetBackoffLeavesALiveConnectionAlone() { + val builder = FakeWebsocketBuilder() + val clock = MutableClock() + val client = newClient(builder, clock) + + client.connect() + builder.lastListener.onOpen(50, false) + assertTrue(client.isConnected(), "Test setup: relay should be connected") + + val attemptsBefore = builder.connectAttempts + client.resetBackoff() + + assertTrue(client.isConnected(), "Expected resetBackoff to leave the live socket untouched") + assertEquals( + attemptsBefore, + builder.connectAttempts, + "Expected resetBackoff not to dial a second socket for an already-connected relay", + ) + } }