From b8899ec7ff9f5902acd1b6098f1a8326557d027c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 23:42:43 +0000 Subject: [PATCH] fix: block the relay before dismissing its prompts, and make the cache atomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the Block Relay button, from a review of that change. Dismiss-before-block. The button called blockRelay() fire-and-forget and then dismissed every prompt from the relay. reportSignerErrors swallows a refused or timed-out signature (ManuallyUnauthorizedException, TimedOutException, CouldNotPerformException) with a log line and no toast, so rejecting the signer prompt closed the dialog, left the relay unblocked, and gave the user nothing to tell them so — and the prompts were in the dismissal set for good. The dismissal now runs from a callback that only fires after account.blockRelay() returns; leaving the prompt up is the feedback when it doesn't. This also shrinks the race window, since sendMyPublicAndPrivateOutbox consumes the kind-10006 into LocalCache synchronously before publishing. Non-atomic cache mutations. NOTIFYs are filed from the relay's socket coroutine while dismissals run from the UI, so addPaymentRequestIfNew's `value +=` read-modify-write could drop one of two concurrent edits, and dismissAllFrom read the pending set before updating it — a prompt arriving in between was removed without ever being recorded as dismissed. Both now go through update/getAndUpdate. Also avoids a copy on a hot path in BlockedRelayFilteringClient: every REQ, COUNT and publish went through filterKeys/minus whenever the block list was non-empty, allocating a full copy of the targets just to reproduce them unchanged. A blocked relay is by definition one the app has stopped aiming at, so it now checks whether any target is actually blocked before copying. This matters more now that blocking is one tap from the dialog rather than a trip to the settings screen, so non-empty block lists become the norm. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01U2GsUAheZmAXv6vk4m7m9T --- .../compose/DisplayNotifyMessages.kt | 19 +++++++++----- .../model/NotifyRequestsCache.kt | 25 +++++++++++-------- .../ui/screen/loggedIn/AccountViewModel.kt | 17 ++++++++++++- .../model/NotifyRequestsCacheTest.kt | 17 +++++++++++++ .../BlockedRelayFilteringClient.kt | 17 +++++++++++-- .../BlockedRelayFilteringClientTest.kt | 19 ++++++++++++++ 6 files changed, 95 insertions(+), 19 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/compose/DisplayNotifyMessages.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/compose/DisplayNotifyMessages.kt index 5351632880..304937a73b 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/compose/DisplayNotifyMessages.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/compose/DisplayNotifyMessages.kt @@ -60,12 +60,19 @@ fun DisplayNotifyMessages( onBlockRelay = if (accountViewModel.isWriteable()) { { - accountViewModel.blockRelay(request.relayUrl) - // Every queued prompt from this relay goes with the block, not just the one - // on screen: a paid relay files one NOTIFY per rejected AUTH, so dismissing - // only [request] would immediately re-open the dialog for a relay the user - // just asked us to stop talking to. - requests.dismissAllFrom(request.relayUrl) + accountViewModel.blockRelay(request.relayUrl) { + // Only after the block is signed and published, never before: a refused + // or timed-out signature is swallowed without a toast, so dismissing up + // front would close the dialog on a relay that is still unblocked and + // leave the user no sign that anything failed. Leaving the prompt up is + // the feedback. + // + // Every queued prompt from this relay goes at once, not just the one on + // screen: a paid relay files one NOTIFY per rejected AUTH, so dismissing + // only [request] would immediately re-open the dialog for a relay the + // user just asked us to stop talking to. + requests.dismissAllFrom(request.relayUrl) + } } } else { null diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCache.kt index 720872c55f..1086d7a77a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCache.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.service.relayClient.notifyCommand.model import androidx.compose.runtime.Stable import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.getAndUpdate import kotlinx.coroutines.flow.update @Stable @@ -38,12 +39,12 @@ class NotifyRequestsCache { } fun addPaymentRequestIfNew(paymentRequest: NotifyRequest) { - if ( - !this.transientPaymentRequests.value.contains(paymentRequest) && - !this.transientPaymentRequestDismissals.value.contains(paymentRequest) - ) { - this.transientPaymentRequests.value += paymentRequest - } + if (this.transientPaymentRequestDismissals.value.contains(paymentRequest)) return + + // `update` rather than `value +=`: NOTIFYs are filed from the relay's socket coroutine + // while dismissals run from the UI, and a plain read-modify-write silently drops one of + // two concurrent edits — either losing a prompt or resurrecting a dismissed one. + this.transientPaymentRequests.update { if (paymentRequest in it) it else it + paymentRequest } } fun dismissPaymentRequest(request: NotifyRequest) { @@ -61,10 +62,14 @@ class NotifyRequestsCache { * relay the user just told us never to talk to again. */ fun dismissAllFrom(relayUrl: NormalizedRelayUrl) { - val fromRelay = this.transientPaymentRequests.value.filterTo(mutableSetOf()) { it.relayUrl == relayUrl } - if (fromRelay.isEmpty()) return + // getAndUpdate so the drain and the snapshot of what was drained are one atomic step: a + // NOTIFY filed by the socket coroutine between a separate read and write would otherwise + // be dropped from the pending set without ever being recorded as dismissed. + val before = this.transientPaymentRequests.getAndUpdate { pending -> pending.filterNotTo(mutableSetOf()) { it.relayUrl == relayUrl } } - this.transientPaymentRequests.update { it - fromRelay } - this.transientPaymentRequestDismissals.update { it + fromRelay } + val dismissed = before.filterTo(mutableSetOf()) { it.relayUrl == relayUrl } + if (dismissed.isEmpty()) return + + this.transientPaymentRequestDismissals.update { it + dismissed } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt index 41df89050e..3b9e74dcc9 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt @@ -1919,7 +1919,22 @@ class AccountViewModel( fun unfollowRelayFeed(url: NormalizedRelayUrl) = launchSigner { account.unfollowRelayFeed(url) } - fun blockRelay(url: NormalizedRelayUrl) = launchSigner { account.blockRelay(url) } + /** + * Blocks [url], running [onBlocked] only once the kind-10006 has actually been signed and + * published. + * + * The ordering matters: [reportSignerErrors] swallows a refused or timed-out signature + * (ManuallyUnauthorizedException, TimedOutException, CouldNotPerformException) with nothing but + * a log line, so a caller that cleaned up before the block landed would leave the user with an + * unblocked relay, no feedback, and whatever UI state it tore down already gone. + */ + fun blockRelay( + url: NormalizedRelayUrl, + onBlocked: () -> Unit = {}, + ) = launchSigner { + account.blockRelay(url) + onBlocked() + } fun showWord(word: String) = launchSigner { account.showWord(word) } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCacheTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCacheTest.kt index acf610c3aa..46dce6b5d6 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCacheTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/notifyCommand/model/NotifyRequestsCacheTest.kt @@ -55,6 +55,23 @@ class NotifyRequestsCacheTest { assertTrue(cache.transientPaymentRequests.value.isEmpty()) } + @Test + fun aConcurrentPromptFromTheSameRelayIsNotSilentlySwallowed() { + val cache = NotifyRequestsCache() + cache.addPaymentRequestIfNew("Pay up", paid) + + // Stands in for a NOTIFY landing on the socket coroutine while the UI drains the relay: + // whatever survives the drain must still be reachable, never removed-but-unrecorded. + cache.dismissAllFrom(paid) + cache.addPaymentRequestIfNew("A different demand", paid) + + val pending = cache.transientPaymentRequests.value + val dismissed = cache.transientPaymentRequestDismissals.value + + assertEquals(setOf(NotifyRequest(paid, "Pay up")), dismissed) + assertEquals(setOf(NotifyRequest(paid, "A different demand")), pending) + } + @Test fun dismissingARelayWithNoPromptsChangesNothing() { val cache = NotifyRequestsCache() diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClient.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClient.kt index 80e446bc51..32b7db05e7 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClient.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClient.kt @@ -73,12 +73,25 @@ class BlockedRelayFilteringClient( relayList: Set, ) { val blocked = blockedRelays() - delegate.publish(event, if (blocked.isEmpty()) relayList else relayList - blocked) + delegate.publish(event, if (blocked.hitsNoneOf(relayList)) relayList else relayList - blocked) } private fun Map>.withoutBlocked(): Map> { val blocked = blockedRelays() - if (blocked.isEmpty()) return this + if (blocked.hitsNoneOf(keys)) return this return filterKeys { it !in blocked } } + + /** + * Whether none of the blocked relays appear in [targets] — i.e. whether the caller can be + * handed its own collection back untouched. + * + * Worth the extra scan because this runs on every REQ, COUNT and publish (filter assemblers + * rebuild their per-relay targets constantly) while a blocked relay is, by definition, one the + * app has stopped aiming at — so "the block list is non-empty but irrelevant to this call" is + * the overwhelmingly common case. Without the check, `filterKeys` / `minus` allocate and copy + * the whole collection every time just to reproduce it unchanged. A hash lookup per target and + * no allocation beats an allocation plus a full copy. + */ + private fun Set.hitsNoneOf(targets: Collection): Boolean = isEmpty() || targets.none { it in this } } diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClientTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClientTest.kt index 4965c0cc40..0fe19bde4f 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClientTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/BlockedRelayFilteringClientTest.kt @@ -28,6 +28,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertSame class BlockedRelayFilteringClientTest { private val good = NormalizedRelayUrl("wss://good.example/") @@ -133,6 +134,24 @@ class BlockedRelayFilteringClientTest { assertEquals(emptySet(), inner.publishedRelays) } + @Test + fun aBlockListThatTouchesNothingHereIsPassedThroughWithoutCopying() { + val inner = RecordingClient() + // Non-empty block list, but none of it is aimed at on this call — the common case once the + // user has blocked anything at all. + val client = BlockedRelayFilteringClient(inner) { setOf(blocked) } + + val filters = mapOf(good to listOf(Filter()), alsoGood to listOf(Filter())) + val relays = setOf(good, alsoGood) + client.subscribe("sub", filters, null) + client.publish(event(), relays) + + // Identity, not just equality: reproducing an unchanged map/set costs an allocation and a + // full copy on a path that runs on every REQ and publish. + assertSame(filters, inner.subscribedFilters) + assertSame(relays, inner.publishedRelays) + } + @Test fun blockSetIsReadPerCallSoLaterChangesApply() { val inner = RecordingClient()