From d2ac376dfc92e3d78f003093505f873fb07345c5 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sun, 27 Sep 2026 22:56:22 -0400 Subject: [PATCH] fix(marmot): amy catches up after being offline and honors White Noise admin removals Two of the three harness failures against MDK 0.10.4 were real bugs: Test 12 (offline catch-up). amy's sync applied kind-445 events in the order the relays returned them, newest first. A message or rename sent after a commit failed to open (UndecryptableOuter / 'no canonical epoch') when it was tried before that commit. The cursor then moved past it, so it was never fetched again: an offline member came back missing the rename and every message after a membership change. Sync now handles Welcomes first and group events oldest first, and re-ingests events that failed that way whenever a commit lands in the same pass. Test 12 now passes. Test 27 (deletion wn->amy). White Noise deletes with kind 4891 ('removal', {"v":1,"action":"remove"}) instead of kind 5 whenever the deleter is a group admin, their own messages included. Neither amy nor the app knew the kind, so an admin's deletion from White Noise never applied (the test passed alone and failed after test 21 made wn an admin). A 4891 now retracts its targets when its author is a current group admin: in amy's message list, and in the app live and on restore. It never renders as a row. Test 27 now passes. Test 29 (disband amy->wn) still fails: wn stays at epoch 1 and never applies the disband. Our commit matches MDK's disband validation rules (inline lifecycle + admin-policy updates, every other leaf removed), and a plain removal of the last other member works (new harness test 34 passes), so it points to the post-join window MDK uses for its own rotation. It is left open. Test 29 now logs wn's full group view, and test 27 prints both epochs when it fails. Co-Authored-By: Claude Opus 5.5 --- .../vitorpamplona/amethyst/model/Account.kt | 1 + .../amethyst/model/AccountMarmotActions.kt | 15 +++++++ .../loggedIn/DecryptAndIndexProcessor.kt | 1 + .../amethyst/cli/commands/MessageCommands.kt | 9 +++- cli/tests/marmot/marmot-interop-headless.sh | 1 + cli/tests/marmot/tests-manage.sh | 38 +++++++++++++++++ cli/tests/marmot/tests-media.sh | 17 +++++++- .../amethyst/commons/marmot/MarmotManager.kt | 30 ++++++++++++- .../commons/marmot/MarmotSyncPolicy.kt | 36 +++++++++++++++- .../model/marmotGroups/MarmotGroupList.kt | 2 + .../marmot/MarmotEditsAndSystemRowsTest.kt | 42 +++++++++++++++++++ .../foundation/appEvents/MarmotAppEvent.kt | 7 ++++ 12 files changed, 193 insertions(+), 6 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt index 192e9523c1..eb0d4e75f4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt @@ -4048,6 +4048,7 @@ class Account( // kind:1009 edit is re-linked to its message too. val innerNote = marmot.indexMarmotInnerEvent(innerEvent).note marmotGroupList.addMessage(groupId, innerNote) + marmot.applyMarmotAdminRemoval(groupId, innerEvent) } catch (e: Exception) { Log.w( "Account", diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountMarmotActions.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountMarmotActions.kt index 3ba76a367c..d1b3a20cad 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountMarmotActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountMarmotActions.kt @@ -298,6 +298,21 @@ class AccountMarmotActions( return IndexedInnerEvent(innerNote, isNew) } + /** + * Apply a kind-4891 admin removal: drop the messages it names from the conversation + * when its author is an admin of the group (see [MarmotManager.adminRemovalTargets]). + * Runs on live delivery and on the restart replay, which re-adds every stored message. + */ + fun applyMarmotAdminRemoval( + nostrGroupId: HexKey, + innerEvent: Event, + ) { + val manager = account.marmotManager ?: return + manager.adminRemovalTargets(nostrGroupId, innerEvent).forEach { targetId -> + account.cache.getNoteIfExists(targetId)?.let { account.marmotGroupList.removeMessage(nostrGroupId, it) } + } + } + /** [note] holds the inner event; [isNew] is true the first time this client indexed it. */ class IndexedInnerEvent( val note: Note, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/DecryptAndIndexProcessor.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/DecryptAndIndexProcessor.kt index b22771664f..71178e26dc 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/DecryptAndIndexProcessor.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/DecryptAndIndexProcessor.kt @@ -771,6 +771,7 @@ class GroupEventHandler( // peer-sent kind:1210 is dropped inside addMessage — see // `MarmotGroupList.isDisplayableFeedMessage`. account.marmotGroupList.addMessage(result.groupId, innerNote) + account.marmot.applyMarmotAdminRemoval(result.groupId, innerEvent) // Traffic is the natural clock for disappearing messages: a // group being read is a group whose expired messages should diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt index 8c66e70686..3b56df46df 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt @@ -113,7 +113,14 @@ object MessageCommands { // A deletion is not its own row either. The retracted body is // blanked rather than the row dropped, so a harness (or a reader // paging back) can tell "retracted" from "never arrived". - val deleted = ctx.marmot.deletedIds(parsed) + val deleted = + ctx.marmot.deletedIds( + parsed, + ctx.marmot + .groupView(gid) + ?.adminPubkeys + ?.toSet() ?: emptySet(), + ) // Pinned at persist time from the retention of the epoch that // DELIVERED each message, so it is the message's own expiry and not // a recomputation against whatever the group's setting is now. diff --git a/cli/tests/marmot/marmot-interop-headless.sh b/cli/tests/marmot/marmot-interop-headless.sh index 966303d14a..37b233460e 100755 --- a/cli/tests/marmot/marmot-interop-headless.sh +++ b/cli/tests/marmot/marmot-interop-headless.sh @@ -216,6 +216,7 @@ ALL_TESTS=( test_31_reaction_materializes_on_wn test_32_amy_message_after_wn_commit test_33_wn_leaves_amy_admin_group + test_34_amy_removes_last_other_member ) # --tests runs a subset in the order given. Most tests read state a previous diff --git a/cli/tests/marmot/tests-manage.sh b/cli/tests/marmot/tests-manage.sh index 912eeefbc7..84a653b289 100644 --- a/cli/tests/marmot/tests-manage.sh +++ b/cli/tests/marmot/tests-manage.sh @@ -398,3 +398,41 @@ test_33_wn_leaves_amy_admin_group() { record_result "$id" fail "amy never committed wn's SelfRemove; wn is still in the tree" fi } + +test_34_amy_removes_last_other_member() { + banner "Test 34 — amy removes the only other member; wn processes its own removal" + local id="34 amy removes wn from a 2-member group" + + # Test 06 removes one of three members. Removing the only other member leaves + # the committer alone in the tree, which is the shape a device removal hit: + # White Noise never applied it and kept showing itself as a member. + local out gid mls_gid b_gid + out=$(amy_json marmot group create --name "Interop-34") || { record_result "$id" fail "amy group create failed"; return; } + gid=$(printf '%s' "$out" | jq -r '.group_id') + mls_gid=$(printf '%s' "$out" | jq -r '.mls_group_id') + amy_json marmot group add "$gid" "$B_NPUB" >/dev/null || { record_result "$id" fail "amy could not invite wn"; return; } + b_gid=$(wait_for_invite B 60) || { record_result "$id" fail "wn never received the Welcome"; return; } + wn_b groups accept "$b_gid" >/dev/null 2>&1 || true + wn_group_field_becomes "$mls_gid" '.group.group_id // empty' "$mls_gid" 120 || { record_result "$id" fail "wn never surfaced the group"; return; } + + wn_b messages send "$mls_gid" "34 before removal" >/dev/null 2>&1 || true + amy_json marmot await message "$gid" --match "34 before removal" --timeout 90 >/dev/null || { record_result "$id" fail "amy never got wn's message"; return; } + + amy_json marmot group remove "$gid" "$B_NPUB" >/dev/null || { record_result "$id" fail "amy remove failed"; return; } + + local deadline=$(( $(date +%s) + 120 )) gone=0 view + while [[ $(date +%s) -lt $deadline ]]; do + wn_b sync >/dev/null 2>&1 || true + view=$(wn_b_json groups show "$mls_gid" 2>/dev/null || true) + if ! printf '%s' "$view" | jq -e --arg p "$B_HEX" '.result.members[]? | select((.member_id // .pubkey // .public_key) == $p)' >/dev/null 2>&1; then + gone=1; break + fi + sleep 3 + done + printf 'test34 wn view after removal: %s\n' "$(printf '%s' "$view" | head -c 2000)" >>"$LOG_FILE" + if [[ "$gone" -eq 1 ]]; then + record_result "$id" pass + else + record_result "$id" fail "wn still lists itself as a member after amy removed it" + fi +} diff --git a/cli/tests/marmot/tests-media.sh b/cli/tests/marmot/tests-media.sh index 1779cc4ee4..73a285687b 100644 --- a/cli/tests/marmot/tests-media.sh +++ b/cli/tests/marmot/tests-media.sh @@ -439,8 +439,15 @@ test_27_deletion_wn_to_amy() { record_result "$id" fail "amy has no event id for wn's message"; return fi - if ! wn_b messages delete "$mls_gid" "$target" >/dev/null 2>&1; then + # Keep wn's answer: "the command ran" and "the tombstone reached a relay" are + # different, and only the second one can reach amy. + local del + del=$(wn_b --json messages delete "$mls_gid" "$target" 2>>"$LOG_FILE") || { record_result "$id" fail "wn messages delete failed"; return + } + printf 'wn messages delete %s -> %s\n' "$target" "$del" >>"$LOG_FILE" + if ! printf '%s' "$del" | jq -e '.result.published != false' >/dev/null 2>&1; then + record_result "$id" fail "wn did not publish the delete tombstone: $del"; return fi # The row stays, blanked and flagged: "retracted" and "never arrived" are @@ -460,7 +467,12 @@ test_27_deletion_wn_to_amy() { done if [[ "$gone" -ne 1 ]]; then - record_result "$id" fail "amy never marked wn's message deleted"; return + # Tell a lost tombstone from a split group: if the two sides sit on different + # epochs, the delete was sent where amy cannot follow. + local wn_epoch amy_epoch + wn_epoch=$(wn_b_json groups show "$mls_gid" 2>/dev/null | jq -r '.result.mls.epoch // "?"') + amy_epoch=$(amy_json marmot group show "$gid" 2>/dev/null | jq -r '.epoch // "?"') + record_result "$id" fail "amy never marked wn's message deleted (wn epoch $wn_epoch, amy epoch $amy_epoch)"; return fi if [[ -n "$body" ]]; then record_result "$id" fail "amy flagged the message deleted but still shows '$body'"; return @@ -695,6 +707,7 @@ test_29_disband_amy_to_wn() { wn_b sync >/dev/null 2>&1 || true sleep 5 done + printf 'disband29 wn view: %s\n' "$(wn_b_json groups show "$mls_gid" 2>&1 | head -c 3000)" >>"$LOG_FILE" printf 'disband29 epoch %s -> %s (amy now %s), wn at %s\n' \ "$before_epoch" "$after_epoch" "${amy_now:-?}" "${saw:-}" >>"$LOG_FILE" diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt index 5796ed9978..7c6da904cf 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt @@ -1598,25 +1598,51 @@ class MarmotManager( * targets together, since a deletion is only authorized against the message * it names. */ - fun deletedIds(messages: List): Set { + fun deletedIds( + messages: List, + /** The group's admins: their kind-4891 removals apply to anyone's message. */ + admins: Set = emptySet(), + ): Set { val authorOf = HashMap(messages.size) val claims = ArrayList>() + val removed = HashSet() for (event in messages) { if (event.kind == DeletionRequestEvent.KIND) { for (tag in event.tags) { if (tag.size >= 2 && tag[0] == "e") claims.add(tag[1] to event.pubKey) } + } else if (event.kind == MarmotAppEvent.KIND_REMOVE) { + if (event.pubKey in admins) removed.addAll(adminRemovalTargets(event)) } else { authorOf[event.id] = event.pubKey } } - val deleted = HashSet(claims.size) + val deleted = HashSet(claims.size + removed.size) for ((targetId, deleter) in claims) { if (authorOf[targetId] == deleter) deleted.add(targetId) } + deleted.addAll(removed.filter { it in authorOf }) return deleted } + /** + * The messages a kind-4891 removal names, if its author may remove them: a current + * admin of [nostrGroupId]. Empty for anything else. White Noise sends 4891 instead of + * kind 5 whenever the deleter is an admin (their own messages included), so without + * this an admin's deletion from White Noise never reached us. + */ + fun adminRemovalTargets( + nostrGroupId: HexKey, + event: Event, + ): List { + if (event.kind != MarmotAppEvent.KIND_REMOVE) return emptyList() + val admins = groupView(nostrGroupId)?.adminPubkeys ?: return emptyList() + if (event.pubKey !in admins) return emptyList() + return adminRemovalTargets(event) + } + + private fun adminRemovalTargets(event: Event): List = event.tags.mapNotNull { tag -> if (tag.size >= 2 && tag[0] == "e") tag[1] else null } + /** * The slice of canonical group state that kind:1210 rows are derived from, * or null when this client is not in the group. diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotSyncPolicy.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotSyncPolicy.kt index 99bafa11b1..3fc71f2066 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotSyncPolicy.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotSyncPolicy.kt @@ -143,13 +143,24 @@ class MarmotSyncPolicy( } if (filterMap.isEmpty()) return - val events = drain(filterMap, timeoutMs) + // Relays answer newest-first and several relays interleave, but a kind:445 can only + // be opened at the epoch its predecessors built: a message sent after a commit fails + // (UndecryptableOuter, "no canonical epoch") if it is tried before that commit. The + // cursor then moves past it and it is never fetched again, which is how an offline + // member came back missing a rename and every message after a membership change. + // Welcomes first (they create the groups), then group events oldest first. + val events = + drain(filterMap, timeoutMs) + .distinctBy { it.second.id } + .sortedWith(compareBy({ if (it.second.kind == GiftWrapEvent.KIND) 0 else 1 }, { it.second.createdAt })) var maxGwSeen = gwSince ?: 0L val maxGroupSeen = perGroupFilters.keys.associateWith { cursors.groupSince(it) ?: 0L }.toMutableMap() var sawGiftWrap = false val sawGroupEvent = mutableSetOf() val stagedProposals = mutableSetOf() + val waitingOnEpoch = mutableListOf() + var advancedEpoch = false for ((relay, event) in events) { // All the MLS/NIP-59 decryption + persistence lives in MarmotIngest — @@ -163,6 +174,8 @@ class MarmotSyncPolicy( } log("ingest ${event.kind}/${event.id.take(8)} via $relay → ${result::class.simpleName}$detail") if (result is MarmotIngestResult.ProposalStaged) stagedProposals.add(result.groupId) + if (event.kind == GroupEvent.KIND && result.couldOpenAfterACommit()) waitingOnEpoch.add(event) + if (result is MarmotIngestResult.Commit) advancedEpoch = true when (event.kind) { GiftWrapEvent.KIND -> { @@ -179,6 +192,22 @@ class MarmotSyncPolicy( } } + // Same-second ties and cross-relay stragglers can still put an event ahead of the + // commit it needs; once a commit landed, give those another go, until a pass + // opens nothing new. + while (advancedEpoch && waitingOnEpoch.isNotEmpty()) { + advancedEpoch = false + val retry = waitingOnEpoch.toList() + waitingOnEpoch.clear() + for (event in retry) { + val result = marmot.ingest(event) + log("retry ${event.kind}/${event.id.take(8)} → ${result::class.simpleName}") + if (result is MarmotIngestResult.Commit) advancedEpoch = true + if (result is MarmotIngestResult.ProposalStaged) stagedProposals.add(result.groupId) + if (result.couldOpenAfterACommit()) waitingOnEpoch.add(event) + } + } + // A member's SelfRemove only takes effect once an admin commits it. for (gid in stagedProposals) { marmot.commitStagedProposalsIfAdmin(gid)?.let { log("committed staged proposals for ${gid.take(8)} → ${it.signedEvent.id.take(8)}") } @@ -224,3 +253,8 @@ class MarmotSyncPolicy( const val GIFT_WRAP_LOOKBACK_SECS: Long = 2L * 24 * 60 * 60 } } + +/** Failed only because the epoch it was sent at isn't reached yet; a later commit may open it. */ +private fun MarmotIngestResult.couldOpenAfterACommit(): Boolean = + this is MarmotIngestResult.UndecryptableOuter || + (this is MarmotIngestResult.Failure && message.startsWith(MarmotManager.NO_CANONICAL_EPOCH_ERROR)) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/marmotGroups/MarmotGroupList.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/marmotGroups/MarmotGroupList.kt index 2d767a6aa8..59059005c9 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/marmotGroups/MarmotGroupList.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/marmotGroups/MarmotGroupList.kt @@ -177,6 +177,7 @@ class MarmotGroupList( companion object { private const val MARMOT_INNER_KIND_DELETION = 5 + private const val MARMOT_INNER_KIND_ADMIN_REMOVAL = 4891 private const val MARMOT_INNER_KIND_REACTION = 7 private const val MARMOT_INNER_KIND_EDIT = 1009 private const val MARMOT_INNER_KIND_STREAM_START = 1200 @@ -192,6 +193,7 @@ class MarmotGroupList( private val NON_CHAT_INNER_KINDS = setOf( MARMOT_INNER_KIND_DELETION, + MARMOT_INNER_KIND_ADMIN_REMOVAL, MARMOT_INNER_KIND_REACTION, MARMOT_INNER_KIND_EDIT, MARMOT_INNER_KIND_STREAM_START, diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotEditsAndSystemRowsTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotEditsAndSystemRowsTest.kt index e2ea1de62e..0dd1ca3c03 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotEditsAndSystemRowsTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotEditsAndSystemRowsTest.kt @@ -140,6 +140,48 @@ class MarmotEditsAndSystemRowsTest { assertTrue(mine.innerEvent.id !in f.manager.deletedIds(f.manager.storedEvents())) } + @Test + fun `an admin's kind 4891 removal retracts a message, a non-admin's does not`() = + runBlocking { + // White Noise sends 4891 instead of kind 5 whenever the deleter is an admin, + // for their own messages too; missing it meant those deletions never applied. + val f = Fixture() + f.manager.createGroup( + nostrGroupId, + MarmotGroupData( + nostrGroupId = nostrGroupId, + name = "moderated", + relays = listOf("wss://relay.invalid"), + adminPubkeys = listOf(f.signer.pubKey), + ), + ) + val target = f.manager.buildTextMessage(nostrGroupId, "moderate me") + val admin = f.signer.pubKey + val stranger = "f".repeat(64) + + fun removal(by: String) = + MarmotAppEvent.build( + pubKey = by, + kind = MarmotAppEvent.KIND_REMOVE, + content = """{"v":1,"action":"remove"}""", + createdAt = 1_800_000_000L, + tags = arrayOf(arrayOf("e", target.innerEvent.id)), + ) + val byStranger = removal(stranger) + f.manager.persistDecryptedMessage(nostrGroupId, byStranger.toJson().dropLast(1) + ",\"sig\":\"\"}") + val admins = setOf(admin) + assertTrue(target.innerEvent.id !in f.manager.deletedIds(f.manager.storedEvents(), admins)) + assertTrue(f.manager.adminRemovalTargets(nostrGroupId, Event.fromJson(byStranger.toJson().dropLast(1) + ",\"sig\":\"\"}")).isEmpty()) + + val byAdmin = removal(admin) + f.manager.persistDecryptedMessage(nostrGroupId, byAdmin.toJson().dropLast(1) + ",\"sig\":\"\"}") + assertTrue(target.innerEvent.id in f.manager.deletedIds(f.manager.storedEvents(), admins)) + assertEquals( + listOf(target.innerEvent.id), + f.manager.adminRemovalTargets(nostrGroupId, Event.fromJson(byAdmin.toJson().dropLast(1) + ",\"sig\":\"\"}")), + ) + } + @Test fun `one kind 5 retracts every message it names`() = runBlocking { diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/foundation/appEvents/MarmotAppEvent.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/foundation/appEvents/MarmotAppEvent.kt index e63c2f7d2f..b23f7f1e65 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/foundation/appEvents/MarmotAppEvent.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/foundation/appEvents/MarmotAppEvent.kt @@ -83,6 +83,13 @@ class MarmotAppEvent( /** A durable group system row, synthesized from canonical state. */ const val KIND_SYSTEM = 1210 + /** + * An admin's removal of a message (anyone's, their own included), with an `e` tag + * naming the target and `{"v":1,"action":"remove"}` as content. White Noise sends + * it instead of a kind-5 deletion whenever the deleter is a group admin. + */ + const val KIND_REMOVE = 4891 + private val ALLOWED_MEMBERS = setOf("id", "pubkey", "created_at", "kind", "tags", "content") val EMPTY_TAGS: TagArray = emptyArray()