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()