From f2895b2d297ac689fcd9007b7cc515580adf2d57 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Fri, 31 Jul 2026 09:31:17 -0400 Subject: [PATCH] fix(relays): count filters once per filter, not once per entity it names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Active Subscriptions screen reported two different things as "filters". The header counted filters; a purpose card summed its per-entity rows. Those only agree when every filter names exactly one entity — and the busiest ones name many, because batching is the whole point of them: one `#e` filter per relay carrying every followed chat, one `#d` filter per host relay carrying every joined group. So six chats on six relays read as 144 filters where 24 were on the wire, and "8% of all" divided the inflated number by the real one. The screen exists to answer "why do I have this many subscriptions", and it was overstating its loudest purposes by exactly their batching factor — the opposite of the job. A purpose now tallies each filter once as it is scanned, before the fan-out. The per-entity rows stay, because "which chats is this relay serving" is worth seeing, but they are a breakdown rather than a total: the field is renamed to namedInFilters and says in its docs that it is not summable. The aggregation moves out of the ViewModel into a pure aggregateSubscriptions() so the invariant is testable without a relay pool. AggregateSubscriptionsTest pins it on the reported shape, and was mutation-checked: restoring the old `entityRows.sumOf { … }` fails three of its four cases, the fourth being the relay count, which never depended on it. On device, Public Chat goes from 144 filters over 6 relays to 18 over the same 6, and Relay Groups from 116 to 96. No filter changed; only the arithmetic. Co-Authored-By: Claude Opus 5 (1M context) --- .../ActiveSubscriptionsViewModel.kt | 209 +++++++++++------- .../AggregateSubscriptionsTest.kt | 115 ++++++++++ 2 files changed, 244 insertions(+), 80 deletions(-) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/AggregateSubscriptionsTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/ActiveSubscriptionsViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/ActiveSubscriptionsViewModel.kt index 1074c360a0..3cd5f1fe55 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/ActiveSubscriptionsViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/ActiveSubscriptionsViewModel.kt @@ -28,6 +28,7 @@ import com.vitorpamplona.amethyst.commons.model.topNavFeeds.IFeedTopNavPerRelayF import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.ExplainedFilter import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.SubPurpose import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow @@ -64,7 +65,15 @@ data class SubscriptionEntityRow( val scope: IFeedTopNavPerRelayFilter?, val detail: String?, val relays: List, - val filterCount: Int, + /** + * How many in-flight filters name this entity. + * + * **Not summable across entities.** One batched filter names every chat its relay serves, so it + * counts once for each of them — summing these to get a purpose's total is how "6 chats on 6 + * relays" once read as 144 filters when 24 were on the wire. [SubscriptionPurposeRow.filterCount] + * counts filters, and is the only number that belongs next to a total. + */ + val namedInFilters: Int, ) /** @@ -83,11 +92,25 @@ private data class EntityKey( @Immutable data class SubscriptionPurposeRow( val purpose: SubPurpose, + /** Filters actually in flight for this purpose — the same unit as [ActiveSubscriptionsState.totalFilters]. */ val filterCount: Int, val relays: List, val entities: List, ) +/** + * One purpose's tally for one account, accumulated in a single pass. + * + * [filters] is incremented once per filter; [entities] records the same filter against every entity + * it names. Keeping both means the card can report a real filter count while still breaking down + * which chats or communities that filter is for. + */ +private class PurposeTally { + var filters = 0 + val relays = mutableSetOf() + val entities = mutableMapOf>() +} + @Immutable data class SubscriptionAccountRow( /** Null groups everything not yet attributed to an account. Shown last, never hidden. */ @@ -126,88 +149,114 @@ class ActiveSubscriptionsViewModel : ViewModel() { private suspend fun snapshot(): ActiveSubscriptionsState = withContext(Dispatchers.Default) { val client = Amethyst.instance.client - - // account -> purpose -> entity -> relays / count - val byAccount = mutableMapOf>>>() - val detailOf = mutableMapOf, String?>() - val scopeOf = mutableMapOf, IFeedTopNavPerRelayFilter>() - var total = 0 - var untagged = 0 - val allRelays = mutableSetOf() - - client.connectedRelaysFlow().value.forEach { relay -> - client.activeRequests(relay).values.flatten().forEach { filter -> - total++ - val explained = filter as? ExplainedFilter - if (explained == null) { - untagged++ - return@forEach - } - allRelays.add(relay) - // Discovery filters name no entity — they go looking for things rather than - // serving known ones — but they do carry the selection they search within, which - // is what keeps them from collapsing into one nameless row per purpose. - val scopeKey = explained.scope?.let { it::class.simpleName } - // A batched filter serves several entities at once — relay-group state is one #d - // filter per host relay carrying every joined group on it — so it contributes a - // row to each of them rather than collapsing to "All". - val entities: List = explained.entityIds?.takeIf { it.isNotEmpty() } ?: listOf(null) - entities.forEach { entityId -> - val key = EntityKey(entityId, scopeKey) - byAccount - .getOrPut(explained.accountPubKey) { mutableMapOf() } - .getOrPut(explained.purpose) { mutableMapOf() } - .getOrPut(key) { mutableListOf() } - .add(relay) - detailOf[explained.purpose to key] = explained.purposeDetail - explained.scope?.let { scopeOf.getOrPut(explained.purpose to key) { it } } - } - } - } - - val accounts = - byAccount - .map { (account, purposes) -> - val purposeRows = - purposes - .map { (purpose, entities) -> - val entityRows = - entities - .map { (key, relays) -> - SubscriptionEntityRow( - entityId = key.entityId, - scope = scopeOf[purpose to key], - detail = detailOf[purpose to key], - relays = relays.distinct().sortedBy { it.url }, - filterCount = relays.size, - ) - }.sortedByDescending { it.filterCount } - SubscriptionPurposeRow( - purpose = purpose, - filterCount = entityRows.sumOf { it.filterCount }, - relays = entityRows.flatMap { it.relays }.distinct(), - entities = entityRows, - ) - }.sortedByDescending { it.filterCount } - SubscriptionAccountRow( - accountPubKey = account, - filterCount = purposeRows.sumOf { it.filterCount }, - relays = purposeRows.flatMap { it.relays }.distinct(), - purposes = purposeRows, - ) - } - // unattributed group last, so it reads as a remainder rather than a headline - .sortedWith(compareBy { it.accountPubKey == null }.thenByDescending { it.filterCount }) - - ActiveSubscriptionsState( - accounts = accounts, - totalFilters = total, - totalRelays = allRelays.size, - untaggedFilters = untagged, + aggregateSubscriptions( + client.connectedRelaysFlow().value.associateWith { relay -> + client.activeRequests(relay).values.flatten() + }, ) } companion object { - const val REFRESH_MS = 2_000L + private const val REFRESH_MS = 2000L } } + +/** + * Folds the in-flight filters into the screen's rows. + * + * Pure and separate from the ViewModel so the invariant it exists to keep — every tagged filter + * counted **exactly once**, so a purpose's count shares a unit with the total it is drawn against — + * is testable without a relay pool. It did not hold before: purposes summed their per-entity rows, + * and a batched filter naming six chats counted six times. + */ +fun aggregateSubscriptions(filtersByRelay: Map>): ActiveSubscriptionsState { + // account -> purpose -> tally (real filter count + relays + per-entity breakdown) + val byAccount = mutableMapOf>() + val detailOf = mutableMapOf, String?>() + val scopeOf = mutableMapOf, IFeedTopNavPerRelayFilter>() + var total = 0 + var untagged = 0 + val allRelays = mutableSetOf() + + filtersByRelay.forEach { (relay, filters) -> + filters.forEach { filter -> + total++ + val explained = filter as? ExplainedFilter + if (explained == null) { + untagged++ + return@forEach + } + allRelays.add(relay) + // Discovery filters name no entity — they go looking for things rather than + // serving known ones — but they do carry the selection they search within, which + // is what keeps them from collapsing into one nameless row per purpose. + val scopeKey = explained.scope?.let { it::class.simpleName } + + val tally = + byAccount + .getOrPut(explained.accountPubKey) { mutableMapOf() } + .getOrPut(explained.purpose) { PurposeTally() } + + // The filter counts ONCE, here — before it is fanned out below. This is the + // number that shares a unit with `total`, so a card's share of the whole is a + // comparison of like with like. + tally.filters++ + tally.relays.add(relay) + + // A batched filter serves several entities at once — relay-group state is one #d + // filter per host relay carrying every joined group on it — so it contributes a + // row to each of them rather than collapsing to "All". These rows are a + // breakdown, never a total: see [SubscriptionEntityRow.namedInFilters]. + val entities: List = explained.entityIds?.takeIf { it.isNotEmpty() } ?: listOf(null) + entities.forEach { entityId -> + val key = EntityKey(entityId, scopeKey) + tally.entities.getOrPut(key) { mutableListOf() }.add(relay) + detailOf[explained.purpose to key] = explained.purposeDetail + explained.scope?.let { scopeOf.getOrPut(explained.purpose to key) { it } } + } + } + } + + val accounts = + byAccount + .map { (account, purposes) -> + val purposeRows = + purposes + .map { (purpose, tally) -> + val entityRows = + tally.entities + .map { (key, relays) -> + SubscriptionEntityRow( + entityId = key.entityId, + scope = scopeOf[purpose to key], + detail = detailOf[purpose to key], + relays = relays.distinct().sortedBy { it.url }, + namedInFilters = relays.size, + ) + }.sortedByDescending { it.namedInFilters } + SubscriptionPurposeRow( + purpose = purpose, + // The tally, NOT the sum of the entity rows — a batched filter + // appears in one row per entity it names. + filterCount = tally.filters, + relays = tally.relays.sortedBy { it.url }, + entities = entityRows, + ) + }.sortedByDescending { it.filterCount } + SubscriptionAccountRow( + accountPubKey = account, + filterCount = purposeRows.sumOf { it.filterCount }, + relays = purposeRows.flatMap { it.relays }.distinct(), + purposes = purposeRows, + ) + } + // unattributed group last, so it reads as a remainder rather than a headline + .sortedWith(compareBy { it.accountPubKey == null }.thenByDescending { it.filterCount }) + + return ActiveSubscriptionsState( + accounts = accounts, + totalFilters = total, + totalRelays = allRelays.size, + untaggedFilters = untagged, + ) +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/AggregateSubscriptionsTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/AggregateSubscriptionsTest.kt new file mode 100644 index 0000000000..88f32a424c --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relays/subscriptions/AggregateSubscriptionsTest.kt @@ -0,0 +1,115 @@ +/* + * 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.ui.screen.loggedIn.relays.subscriptions + +import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.ExplainedFilter +import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.SubPurpose +import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * The unit invariant behind the Active Subscriptions screen: a purpose's filter count and the + * screen's total must be the *same thing counted the same way*, because the card draws one as a + * share of the other. + * + * It was broken by batching. A relay-group state filter carries every joined group on its host + * relay, and a public-chat filter carries every followed chat — so summing the per-entity rows + * counted one filter once per entity it named. Six chats over six relays reported 144 filters where + * 24 were on the wire, and the resulting "8% of all" divided an inflated number by a real one. + */ +class AggregateSubscriptionsTest { + private val account = "aa9047325603dacd4f8142093567973566de3b1e20a89557b728c3be4c6a844b" + private val chats = (1..6).map { "cafe$it".padEnd(64, '0') } + + private fun relay(n: Int) = NormalizedRelayUrl("wss://relay$n.example/") + + /** One batched filter naming every chat the relay serves — what the real builders emit. */ + private fun batched(kind: Int) = + ExplainedFilter( + purpose = SubPurpose.PUBLIC_CHATS, + entityIds = chats, + kinds = listOf(kind), + tags = mapOf("e" to chats), + accountPubKey = account, + ) + + /** Six relays, each carrying four batched filters that each name all six chats. */ + private fun sixChatsOnSixRelays(): Map> = (1..6).associate { r -> relay(r) to listOf(batched(40), batched(41), batched(42), batched(43)) } + + @Test + fun `a batched filter counts once, not once per entity it names`() { + val state = aggregateSubscriptions(sixChatsOnSixRelays()) + + // 6 relays x 4 filters = 24 actually in flight. The bug reported 144 (24 x 6 entities). + assertEquals(24, state.totalFilters) + + val purpose = + state.accounts + .single() + .purposes + .single() + assertEquals(24, purpose.filterCount) + + // The per-entity breakdown is still there, and still says each chat is named by 24 filters. + assertEquals(6, purpose.entities.size) + purpose.entities.forEach { assertEquals(24, it.namedInFilters) } + } + + @Test + fun `purpose counts sum to the total, so a share of the whole is a like-for-like ratio`() { + val state = aggregateSubscriptions(sixChatsOnSixRelays()) + + val summed = state.accounts.sumOf { acct -> acct.purposes.sumOf { it.filterCount } } + assertEquals(state.totalFilters, summed) + assertEquals(state.totalFilters, state.accounts.sumOf { it.filterCount }) + } + + @Test + fun `relays are counted distinctly, not once per filter that reaches them`() { + val state = aggregateSubscriptions(sixChatsOnSixRelays()) + + assertEquals(6, state.totalRelays) + assertEquals( + 6, + state.accounts + .single() + .purposes + .single() + .relays.size, + ) + } + + @Test + fun `untagged filters are reported but never attributed to a purpose`() { + val state = + aggregateSubscriptions( + mapOf(relay(1) to listOf(batched(42), Filter(kinds = listOf(1)))), + ) + + assertEquals(2, state.totalFilters) + assertEquals(1, state.untaggedFilters) + // Only the tagged one reaches a card, so the cards no longer add up to the total here — + // which is exactly what the untagged line under the header exists to explain. + assertEquals(1, state.accounts.sumOf { it.filterCount }) + } +}