From 3214095b9900a8dd0e869c6335c7719901d0e6fc Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 18:13:31 +0000 Subject: [PATCH] fix: route stray NIP-29 group content to the host, not a phantom channel A group-scoped content event (kind-9 chat, poll, kind-11 thread) is keyed to its RelayGroupChannel by the relay that served it, because a NIP-29 event doesn't carry its host relay. That's correct for the group's own host-pinned subscriptions, but a message resolved from a NON-host relay -- e.g. a quoted kind-9 fetched by id during missing-event resolution -- was filed under GroupId(groupId, strangerRelay), a channel the group's screens never read, so the message silently vanished (the serving-relay hazard). LocalCache.attachToRelayGroupIfScoped / attachThreadToRelayGroupIfScoped now, when no channel is keyed to the serving relay, redirect the stray to the group's single confirmed host channel via redirectStrayRelayGroupContent, keyed off RelayGroupChannel.hasRelaySignedState(). A phantom channel never has relay-signed state, so the redirect can only ever land on a real host, never on another phantom -- the fix is strictly safe and the common host-pinned arrival stays an untouched O(1) fast path (the scan runs only on the rare no-channel-for-serving-relay miss). Also add the cache-prune gap-fill can't-miss test: drive the production RelayLoadingCursors down a real relay, rewindTo below the window, and confirm the pruned band re-loads with no gap. Tests: RelayGroupContentRoutingTest (pure router + the channel signal); RelayGroupHistoryPagingRelayTest gains the rewind reload case. The LocalCache wiring is unit-covered at the router level but still device- untested end-to-end (flagged in the test plan). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01CDK63toGbE7DQxKxrQnhMU --- .../2026-07-18-nip29-group-chat-test-plan.md | 2 +- .../amethyst/model/LocalCache.kt | 33 +++++- .../model/RelayGroupContentRouting.kt | 52 +++++++++ .../model/RelayGroupContentRoutingTest.kt | 107 ++++++++++++++++++ .../nip29RelayGroups/RelayGroupChannel.kt | 14 +++ .../RelayGroupHistoryPagingRelayTest.kt | 45 ++++++-- 6 files changed, 241 insertions(+), 12 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/model/RelayGroupContentRouting.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/model/RelayGroupContentRoutingTest.kt diff --git a/amethyst/plans/2026-07-18-nip29-group-chat-test-plan.md b/amethyst/plans/2026-07-18-nip29-group-chat-test-plan.md index 87d101c286..a595c78f40 100644 --- a/amethyst/plans/2026-07-18-nip29-group-chat-test-plan.md +++ b/amethyst/plans/2026-07-18-nip29-group-chat-test-plan.md @@ -169,7 +169,7 @@ is buggy** — both are needed before trusting "loads correctly" in the wild. - **No double-download on reconnect** (C9/D8). - **Retirement left no gap:** with `RelayGroupMyJoinedGroups` deleted and `ChannelPublic`/`ChannelFromUser` relay-group content removed, D1/D2/D3 still load — proving the tail+pager replaced them. - **CardWarmup joined-skip** (Tier B #6 / D5): a joined card issues no warmup REQ. -- **Serving-relay hazard (known open item):** quote a group message fetched from a **non-host** relay (`filterMissingEvents`) and confirm whether it appears in the group (`GroupId(G, hostR)`) or is filed under `GroupId(G, otherR)` and lost. Documents the plan's §2 follow-up; expected to **fail** until that fix lands. +- **Serving-relay hazard (FIXED):** a group message fetched from a **non-host** relay (e.g. `filterMissingEvents` quote resolution) used to be filed under `GroupId(G, otherR)` and lost. `LocalCache.attachToRelayGroupIfScoped`/`attachThreadToRelayGroupIfScoped` now redirect a stray (no channel for the serving relay) to the group's single confirmed **host** channel via `redirectStrayRelayGroupContent`, keyed off `RelayGroupChannel.hasRelaySignedState()` (a phantom never has relay-signed state, so the redirect only ever lands on a real host — strictly safe, the common host-pinned path is an untouched O(1) fast path). Covered by `RelayGroupContentRoutingTest`. **Device-untested:** the pure router + channel signal are unit-tested; the LocalCache wiring is a guarded fast-path/slow-path swap that still needs a device pass (Tier D) to confirm end-to-end. ## Exit criteria - Tier A green; Tier B all assertions pass; Tier C1–C9 pass (esp. **C3**). diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt index abddc36e62..3138eee1d8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/LocalCache.kt @@ -2006,9 +2006,20 @@ object LocalCache : ILocalCache, ICacheProvider { if (note.event == null) return if (relay != null) { - // Normal arrival: the group only exists on its host relay and the - // filters are host-pinned, so the serving relay is the group's key. - getOrCreateRelayGroupChannel(GroupId(groupId, relay)).addNote(note, relay) + val exact = GroupId(groupId, relay) + val existing = getRelayGroupChannelIfExists(exact) + if (existing != null) { + // Normal arrival: the group's host-pinned filters served it, so the serving relay IS the + // group's key and its channel already exists. Fast O(1) path — no scan. + existing.addNote(note, relay) + } else { + // No channel keyed to the serving relay: this may be a stray from a NON-host relay (e.g. a + // quoted kind-9 resolved by id). Redirect it to the group's single confirmed host rather + // than mint a phantom channel the group's screens never read (the serving-relay hazard); + // fall back to the serving-relay key when there is no single host (new/ambiguous group). + val target = redirectStrayRelayGroupContent(relayGroupCandidatesFor(groupId)) ?: exact + getOrCreateRelayGroupChannel(target).addNote(note, relay) + } } else { // Our own optimistic send has no provenance relay, so we can't build the (groupId, // relay) key. Attach only when a SINGLE open channel has this group id (the room being @@ -2022,6 +2033,12 @@ object LocalCache : ILocalCache, ICacheProvider { } } + /** Candidate group channels for the [redirectStrayRelayGroupContent] slow path — one scan by group id. */ + private fun relayGroupCandidatesFor(groupId: String): List = + relayGroupChannels + .filter { key, _ -> key.id == groupId } + .map { RelayGroupTargetCandidate(it.groupId, it.hasRelaySignedState()) } + /** * Same routing as [attachToRelayGroupIfScoped] but for kind-11 threads, which * are kept in a separate collection from the chat timeline so the two content @@ -2036,7 +2053,15 @@ object LocalCache : ILocalCache, ICacheProvider { if (note.event == null) return if (relay != null) { - getOrCreateRelayGroupChannel(GroupId(groupId, relay)).addThread(note) + val exact = GroupId(groupId, relay) + val existing = getRelayGroupChannelIfExists(exact) + if (existing != null) { + existing.addThread(note) + } else { + // Same serving-relay hazard as the chat path: prefer the single confirmed host over a phantom. + val target = redirectStrayRelayGroupContent(relayGroupCandidatesFor(groupId)) ?: exact + getOrCreateRelayGroupChannel(target).addThread(note) + } } else { // See attachToRelayGroupIfScoped: only attach when the group id is unambiguous. relayGroupChannels diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/RelayGroupContentRouting.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/RelayGroupContentRouting.kt new file mode 100644 index 0000000000..7444f9000c --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/RelayGroupContentRouting.kt @@ -0,0 +1,52 @@ +/* + * 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.model + +import com.vitorpamplona.quartz.nip29RelayGroups.GroupId + +/** + * A candidate group channel when routing a stray group-scoped content event: its [key] and whether it is a + * confirmed host (has received relay-signed state). See [redirectStrayRelayGroupContent]. + */ +data class RelayGroupTargetCandidate( + val key: GroupId, + val hasRelaySignedState: Boolean, +) + +/** + * Resolves the **serving-relay hazard**. A group-scoped content event (kind-9 chat, poll, kind-11 thread…) + * is keyed to its group channel by the relay that served it, because a NIP-29 event doesn't carry its host + * relay. That is correct for the group's own host-pinned subscriptions, but a message resolved from a + * **non-host** relay — e.g. a quoted kind-9 fetched by id during missing-event resolution — would be filed + * under a channel keyed to that stranger relay, one the group's own screens never read, so the message + * silently vanishes. + * + * Called only when there is **no** channel keyed to the serving relay for this group id (the fast, common + * path attaches directly and never gets here). It picks the group's single confirmed **host** channel — one + * that has received relay-signed state — to attach the stray to instead. Returns that host key, or null when + * there is no single confirmed host (a genuinely new group on the serving relay, or an id ambiguous across + * several hosts), in which case the caller keeps the serving-relay key as today's best effort. + * + * A phantom channel (one minted from an earlier stray) never has relay-signed state, so it can never be + * chosen here — the redirect only ever lands on a real host, never on another phantom. This makes the fix + * strictly safe: it can redirect a stray to a known host, but never divert a message away from one. + */ +fun redirectStrayRelayGroupContent(candidates: List): GroupId? = candidates.filter { it.hasRelaySignedState }.singleOrNull()?.key diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/model/RelayGroupContentRoutingTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/model/RelayGroupContentRoutingTest.kt new file mode 100644 index 0000000000..276572c059 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/model/RelayGroupContentRoutingTest.kt @@ -0,0 +1,107 @@ +/* + * 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.model + +import com.vitorpamplona.amethyst.commons.model.nip29RelayGroups.RelayGroupChannel +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer +import com.vitorpamplona.quartz.nip29RelayGroups.GroupId +import com.vitorpamplona.quartz.nip29RelayGroups.metadata.GroupPinnedEvent +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The serving-relay hazard router: a stray group-scoped content event (no channel keyed to its serving + * relay) must be redirected to the group's single confirmed **host** channel, never to another phantom, and + * never when the host is ambiguous. Also pins that a phantom channel — one that only ever received content — + * reports no relay-signed state, which is what keeps [redirectStrayRelayGroupContent] from ever picking one. + */ +class RelayGroupContentRoutingTest { + private val relayA = RelayUrlNormalizer.normalizeOrNull("wss://relay-a.example/")!! + private val relayB = RelayUrlNormalizer.normalizeOrNull("wss://relay-b.example/")!! + private val relayC = RelayUrlNormalizer.normalizeOrNull("wss://relay-c.example/")!! + + private fun host( + id: String, + relay: NormalizedRelayUrl, + ) = RelayGroupTargetCandidate(GroupId(id, relay), hasRelaySignedState = true) + + private fun phantom( + id: String, + relay: NormalizedRelayUrl, + ) = RelayGroupTargetCandidate(GroupId(id, relay), hasRelaySignedState = false) + + @Test + fun `no candidates leaves the routing to the caller`() { + // A genuinely new group on the serving relay: caller keeps the serving-relay key. + assertNull(redirectStrayRelayGroupContent(emptyList())) + } + + @Test + fun `a single confirmed host is chosen`() { + assertEquals(GroupId("g1", relayA), redirectStrayRelayGroupContent(listOf(host("g1", relayA)))) + } + + @Test + fun `a phantom-only candidate is never chosen`() { + // Redirecting a stray onto another phantom would just move it into a second unread channel. + assertNull(redirectStrayRelayGroupContent(listOf(phantom("g1", relayB)))) + } + + @Test + fun `the confirmed host wins over a phantom for the same id`() { + assertEquals( + GroupId("g1", relayA), + redirectStrayRelayGroupContent(listOf(phantom("g1", relayB), host("g1", relayA))), + ) + } + + @Test + fun `an id claimed by two confirmed hosts is ambiguous and left to the caller`() { + assertNull(redirectStrayRelayGroupContent(listOf(host("g1", relayA), host("g1", relayC)))) + } + + @Test + fun `several phantoms and no host returns null`() { + assertNull(redirectStrayRelayGroupContent(listOf(phantom("g1", relayA), phantom("g1", relayB)))) + } + + @Test + fun `a fresh channel has no relay-signed state until a relay-signed event flips it`() { + val channel = RelayGroupChannel(GroupId("g1", relayA)) + assertFalse("a channel with no relay-signed events is a phantom, not a host", channel.hasRelaySignedState()) + + channel.updatePinned( + GroupPinnedEvent( + id = "d".repeat(64), + pubKey = "b".repeat(64), + createdAt = 1L, + tags = arrayOf(arrayOf("d", "g1"), arrayOf("e", "a".repeat(64))), + content = "", + sig = "0".repeat(128), + ), + ) + assertTrue("a relay-signed pin list makes it a confirmed host", channel.hasRelaySignedState()) + } +} diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip29RelayGroups/RelayGroupChannel.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip29RelayGroups/RelayGroupChannel.kt index 393d276d49..93515eae72 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip29RelayGroups/RelayGroupChannel.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip29RelayGroups/RelayGroupChannel.kt @@ -139,6 +139,20 @@ class RelayGroupChannel( fun threadCount(): Int = threadNotes.size() + /** + * Whether this channel has received any relay-signed state — metadata, roster, roles or pins. Only the + * group's **host** relay signs those (via the `isRelaySignedGroupEvent`-gated consume paths), so this is + * true only for a confirmed host channel and never for a "phantom" one minted from a stray content event + * that arrived from a non-host relay. Used to redirect such strays back to the real host (see + * `LocalCache.attachToRelayGroupIfScoped` / the serving-relay hazard). + */ + fun hasRelaySignedState(): Boolean = + event != null || + members.isNotEmpty() || + admins.isNotEmpty() || + supportedRoles.isNotEmpty() || + pinnedEventIds.isNotEmpty() + /** A relay group lives on exactly one relay: its host. */ override fun relays() = setOf(groupId.relayUrl) diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/paging/RelayGroupHistoryPagingRelayTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/paging/RelayGroupHistoryPagingRelayTest.kt index 0981ea070b..af535d26e5 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/paging/RelayGroupHistoryPagingRelayTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/paging/RelayGroupHistoryPagingRelayTest.kt @@ -152,16 +152,15 @@ class RelayGroupHistoryPagingRelayTest : RelayClientTest() { // tests that feed it synthetic callbacks (RelayLoadingCursorsTest / BackwardRelayPagerTest). /** - * Steps the production [RelayLoadingCursors] backward over one group's `#h` chat until it reports done, - * exactly as the history assembler does: advance → REQ at the requested `until` → feed each event and - * the EOSE back in → advance again. Returns every id delivered. + * One backward drain of the production [cursors] over [groupId] until it reports done — exactly as the + * history assembler steps it: advance → REQ at the requested `until` → feed each event and the EOSE back + * in → advance again. Returns every id delivered. */ - private suspend fun driveCursorsToBottom( + private suspend fun drainFrom( + cursors: RelayLoadingCursors, groupId: String, now: Long, ): Set { - val cursors = RelayLoadingCursors() - cursors.floor = now val relay = defaultRelayUrl val seen = mutableSetOf() cursors.advance(relay, start = now) @@ -175,10 +174,42 @@ class RelayGroupHistoryPagingRelayTest : RelayClientTest() { if (eose) cursors.onEose(relay) cursors.advance(relay, start = now) } - assertTrue(cursors.isDone(relay), "the production cursors must reach the bottom (empty page + EOSE)") return seen } + private suspend fun driveCursorsToBottom( + groupId: String, + now: Long, + ): Set { + val cursors = RelayLoadingCursors().apply { floor = now } + val seen = drainFrom(cursors, groupId, now) + assertTrue(cursors.isDone(defaultRelayUrl), "the production cursors must reach the bottom (empty page + EOSE)") + return seen + } + + @Test + fun aCachePruneBelowTheWindowReloadsTheDroppedBandWithNoGap() = + runBlocking { + // groupChat seeds createdAt == id seed, so hexId(k) is the event at createdAt k. + defaultRelay.preload(groupChat(idBase = 1, count = TOTAL, groupId = "g1")) + val now = 10_000L + val cursors = RelayLoadingCursors().apply { floor = now } + + val firstPass = drainFrom(cursors, "g1", now) + assertEquals(TOTAL, firstPass.size) + assertTrue(cursors.isDone(defaultRelayUrl), "precondition: the first walk reached the bottom") + + // The cache evicts everything at or below createdAt 150 out of the window. rewindTo pulls the + // reached cursor just above the newest pruned event so the next advance re-requests that band — + // without it, a done relay whose cursor already sits below the hole would skip it forever. + cursors.rewindTo(mapOf(defaultRelayUrl to 150L)) + + val reload = drainFrom(cursors, "g1", now) + // The re-walk must re-fetch the entire pruned band (createdAt 1..150) — no gap, no skipped id. + assertEquals((1..150).map { SyntheticEvents.hexId(it) }.toSet(), reload) + assertTrue(cursors.isDone(defaultRelayUrl), "the re-walk terminates on an empty page") + } + @Test fun productionCursorsWalkOneGroupToTheBottomOverTheWire() = runBlocking {