From 0b0abeebbbe594c8afab974943a30ed148d75b86 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 17:23:50 +0000 Subject: [PATCH] fix: prevent duplicate LazyColumn key from observeNotes on addressable updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NoteListMatchingFilter (backing LocalCache.observeNotes) stored notes in a ConcurrentSkipListSet ordered by CreatedAtIdHexComparator. AddressableNotes are mutable: when a newer replaceable event arrives, LocalCache swaps the event on the SAME note instance (consumeBaseReplaceable -> loadEvent), changing its createdAt in place, then re-notifies observers. A sorted set cannot survive a member's sort key mutating underneath it — the moved node is no longer found on the add() search path, so the same note gets inserted a second time and the emitted list carries a duplicate idHex. The App Recommendations screen keys its LazyColumn on note.idHex (an AddressableNote's address, e.g. 31990::nostr-dvm-labeler), so the duplicate crashed with IllegalArgumentException: "Key ... was already used". Dedupe by the immutable idHex instead of a createdAt-ordered set; ordering is computed fresh on each emission. Adds a regression test reproducing the multi-item corruption path. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Ah1aCniyjnzc27x4pwq2Df --- .../observables/NoteListMatchingFilter.kt | 49 +++++-- .../observables/NoteListMatchingFilterTest.kt | 121 ++++++++++++++++++ 2 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt index e938fb2882..07c82c1dc1 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt @@ -24,22 +24,34 @@ import com.vitorpamplona.amethyst.commons.model.AddressableNote import com.vitorpamplona.amethyst.commons.model.Note import com.vitorpamplona.quartz.nip01Core.core.AddressableEvent import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import java.util.SortedSet -import java.util.concurrent.ConcurrentSkipListSet +import java.util.concurrent.ConcurrentHashMap /** * Creates a list of notes (regular and addressable) * that only gets updated when a new note appears. * - * New versions of addressables do not update the list + * New versions of addressables do not update the list. + * + * Membership is keyed by the immutable [Note.idHex] rather than kept in a + * sorted set ordered by createdAt. AddressableNotes are mutable: when a newer + * version of a replaceable event arrives, LocalCache swaps the event on the + * SAME note instance, changing its createdAt in place. A + * ConcurrentSkipListSet ordered on that createdAt cannot survive the change — + * the moved node is no longer found by add()/remove(), so the note ends up + * inserted twice and the emitted list carries a duplicate idHex, crashing any + * LazyColumn keyed on it. Deduping by idHex keeps membership correct + * regardless of createdAt changes; the display order is computed fresh on each + * emission. */ class NoteListMatchingFilter( private val filter: Filter, private val atOnce: (filter: Filter) -> SortedSet, private val update: (List) -> Unit, ) : Observable { - var currentResults: ConcurrentSkipListSet = ConcurrentSkipListSet(CreatedAtIdHexComparator) + val currentResults: ConcurrentHashMap = ConcurrentHashMap() override fun new( event: Event, @@ -47,26 +59,35 @@ class NoteListMatchingFilter( ) { if (event is AddressableEvent && note !is AddressableNote) return - if (filter.match(event)) { - if (currentResults.add(note)) { - val limit = filter.limit - if (limit != null && currentResults.size > limit) { - currentResults.remove(currentResults.last()) - } + // New versions of addressables do not update the list. + if (currentResults.containsKey(note.idHex)) return - update(currentResults.toList()) + if (filter.match(event)) { + currentResults[note.idHex] = note + + val limit = filter.limit + if (limit != null && currentResults.size > limit) { + // Drop the oldest (sorts last under CreatedAtIdHexComparator). + currentResults.values.maxWithOrNull(CreatedAtIdHexComparator)?.let { + currentResults.remove(it.idHex) + } } + + update(snapshot()) } } override fun remove(note: Note) { - if (currentResults.remove(note)) { - update(currentResults.toList()) + if (currentResults.remove(note.idHex) != null) { + update(snapshot()) } } fun init() { - currentResults = ConcurrentSkipListSet(atOnce(filter)) - update(currentResults.toList()) + currentResults.clear() + atOnce(filter).forEach { currentResults[it.idHex] = it } + update(snapshot()) } + + private fun snapshot(): List = currentResults.values.sortedWith(CreatedAtIdHexComparator) } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt new file mode 100644 index 0000000000..cbdfdd6bfc --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt @@ -0,0 +1,121 @@ +/* + * 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.commons.model.observables + +import com.vitorpamplona.amethyst.commons.model.AddressableNote +import com.vitorpamplona.amethyst.commons.model.Note +import com.vitorpamplona.quartz.nip01Core.core.Address +import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter +import com.vitorpamplona.quartz.nip89AppHandlers.definition.AppDefinitionEvent +import com.vitorpamplona.quartz.utils.EventFactory +import java.util.TreeSet +import kotlin.test.Test +import kotlin.test.assertEquals + +class NoteListMatchingFilterTest { + private val author = "d0d0a746b44c9de8422165aef520b1fe041eedf5794f7592505477eeac122c18" + + private val filter = Filter(kinds = listOf(AppDefinitionEvent.KIND)) + + // Amethyst reuses a single AddressableNote instance per address (LocalCache), + // so a newer replaceable event mutates createdAt on the SAME object. + private fun noteFor(dTag: String) = AddressableNote(Address(AppDefinitionEvent.KIND, author, dTag)) + + private fun appDefinition( + dTag: String, + createdAt: Long, + ): Event = + EventFactory.create( + id = "%064x".format(createdAt), + pubKey = author, + createdAt = createdAt, + kind = AppDefinitionEvent.KIND, + tags = arrayOf(arrayOf("d", dTag)), + content = "{}", + sig = "00".repeat(64), + ) + + private fun AddressableNote.load(createdAt: Long) { + event = appDefinition(dTag(), createdAt) + } + + private fun newFilter(sink: (List) -> Unit) = + NoteListMatchingFilter( + filter = filter, + atOnce = { TreeSet(CreatedAtIdHexComparator) }, + update = sink, + ) + + @Test + fun newerVersionOfAnAddressableDoesNotDuplicateTheKey() { + var last: List = emptyList() + val subject = newFilter { last = it } + subject.init() + + // Three app definitions arrive. Their createdAt spread matters: after the + // target moves, the sorted set's search path for the new key must be able + // to bypass the stale node, which is what corrupts a createdAt-ordered set. + val target = noteFor("nostr-dvm-labeler") + val newer = noteFor("other-app") + val newest = noteFor("top-app") + + target.load(1000) + subject.new(target.event!!, target) + newer.load(2000) + subject.new(newer.event!!, newer) + newest.load(4000) + subject.new(newest.event!!, newest) + assertEquals(3, last.size) + + // A newer definition replaces the event on the SAME target instance, + // moving its createdAt from 1000 to 3000 (now between 2000 and 4000). + // LocalCache then notifies the observer again. This must NOT insert the + // note a second time. + target.load(3000) + subject.new(target.event!!, target) + + assertEquals( + listOf(newest.idHex, newer.idHex, target.idHex).sorted(), + last.map { it.idHex }.sorted(), + "each addressable must appear exactly once", + ) + assertEquals(last.size, last.map { it.idHex }.toSet().size, "no duplicate keys") + } + + @Test + fun removeDropsTheNoteEvenAfterCreatedAtChanged() { + var last: List = emptyList() + val subject = newFilter { last = it } + subject.init() + + val target = noteFor("nostr-dvm-labeler") + target.load(1000) + subject.new(target.event!!, target) + assertEquals(1, last.size) + + // The createdAt sort key moves before the delete arrives. + target.load(2000) + subject.remove(target) + + assertEquals(0, last.size, "remove must find the note despite the createdAt change") + } +}