Merge remote-tracking branch 'upstream/claude/kind-keller-itzy63' into test/kind-keller

2026-10-05 19:28:25 +00:00 · 2026-09-25 16:51:17 -04:00
parent c4f6469a5f f60d96e00a
commit be2c355589
5 changed files with 226 additions and 14 deletions
@@ -643,6 +643,26 @@ class CordnRuntime(
        }
    }

+    /**
+     * Which identities [coordinatorPubKey] can deliver a Welcome to, or null.
+     *
+     * For annotating people you are *about* to invite. cordn can only add a
+     * member by spending a KeyPackage they published to this coordinator, and
+     * `kp_list` is the only non-destructive way to learn who did -- `kp_take`
+     * by identity consumes one of their single-use packages
+     * (`spec/00.md` §"Coordinator retrieval behavior"), so it can never be
+     * used as a probe. One call answers for everybody, which is why this hands
+     * back the whole set instead of taking a pubkey.
+     *
+     * Null is **not** an empty set. It means the question went unanswered, and
+     * a caller that renders it as "cannot be added" turns a network failure
+     * into a claim about a person. Distinguish them.
+     */
+    suspend fun identitiesWithKeyPackages(coordinatorPubKey: HexKey): Set<HexKey>? {
+        val session = registry.sessionOrNull(coordinatorPubKey) ?: return null
+        return session.keyPackages.identitiesWithKeyPackages()
+    }
+
    /** Publishes one KeyPackage. An attributable act under the account key (§8.4). */
    suspend fun publishKeyPackage(
        coordinatorPubKey: HexKey,
@@ -411,9 +411,18 @@ private fun CordnGroupInfo(
            // everybody would build commits the rest of the group drops on
            // receipt.
            if (canAdminister) {
+                // Asked only once the field is in use. `kp_list` is unpaginated,
+                // so opening this screen must not download the coordinator's
+                // whole table on behalf of somebody who never types.
+                val searching = memberSearch.length > 2
+                val reachable by
+                    produceState<Set<HexKey>?>(null, runtime, coordinatorPubKey, searching) {
+                        if (searching) value = runtime?.identitiesWithKeyPackages(coordinatorPubKey)
+                    }
                CordnAddMember(
                    userSuggestions = userSuggestions,
                    search = memberSearch,
+                    reachable = reachable,
                    onSearchChange = {
                        memberSearch = it
                        adminError = null
@@ -537,16 +546,28 @@ private fun CordnGroupInfo(
 * ## The search finding somebody is not a promise
 *
 * cordn can only add a member by spending a KeyPackage they published **to
- * this coordinator** (`spec/00.md`), and nothing on this device can know
- * whether they did until the coordinator is asked. So the note says so before
- * the attempt, and `CordnGroupManager.invite` says which of the two went wrong
- * after it -- "the coordinator holds no KeyPackage for ..." is a different
- * problem from a name that matched nobody, and they are worth telling apart.
+ * this coordinator** (`spec/00.md`). [reachable] is who the coordinator says
+ * did, so the common disappointment is visible before the tap rather than
+ * after it, and `CordnGroupManager.invite` still says which of the two went
+ * wrong when one does -- "the coordinator holds no KeyPackage for ..." is a
+ * different problem from a name that matched nobody.
+ *
+ * ## Why the badge marks the yes and says nothing about the no
+ *
+ * [reachable] has three states, not two: a set, an empty set, and null for a
+ * lookup that never came back (`CordnRuntime.identitiesWithKeyPackages`). Only
+ * membership in it is something we were told; absence covers both "has not
+ * published here" and "we could not ask". Marking the yes therefore asserts
+ * exactly what we verified, and an unmarked row asserts nothing -- where a
+ * "cannot be added" marker would turn an unreachable coordinator into a claim
+ * about a person. Unmarked rows stay fully tappable for the same reason: the
+ * listing can be up to a minute stale, and the coordinator gets the last word.
 */
@Composable
 private fun CordnAddMember(
    userSuggestions: UserSuggestionState,
    search: String,
+    reachable: Set<HexKey>?,
    onSearchChange: (String) -> Unit,
    busy: Boolean,
    accountViewModel: AccountViewModel,
@@ -587,12 +608,22 @@ private fun CordnAddMember(
                    )
                },
                trailingContent = { user ->
-                    IconButton(onClick = { onInvite(user) }) {
-                        Icon(
-                            symbol = MaterialSymbols.PersonAdd,
-                            contentDescription = stringRes(R.string.cordn_info_add_member),
-                            tint = MaterialTheme.colorScheme.primary,
-                        )
+                    Row(verticalAlignment = Alignment.CenterVertically) {
+                        if (reachable?.contains(user.pubkeyHex) == true) {
+                            Icon(
+                                symbol = MaterialSymbols.Key,
+                                contentDescription = stringRes(R.string.cordn_info_has_key_package),
+                                modifier = Modifier.size(16.dp),
+                                tint = MaterialTheme.colorScheme.primary,
+                            )
+                        }
+                        IconButton(onClick = { onInvite(user) }) {
+                            Icon(
+                                symbol = MaterialSymbols.PersonAdd,
+                                contentDescription = stringRes(R.string.cordn_info_add_member),
+                                tint = MaterialTheme.colorScheme.primary,
+                            )
+                        }
                    }
                },
            )
@@ -414,6 +414,7 @@
    <string name="cordn_coordinators_stale_note">These have not announced in over a month. Creating a group on one that has gone away will fail.</string>
    <string name="cordn_info_add_member">Add someone</string>
    <string name="cordn_info_add_member_placeholder">Name, npub or name@domain</string>
+    <string name="cordn_info_has_key_package">Can be added on this coordinator</string>
    <string name="cordn_info_add_member_note">They can only be added if they published a key package to this coordinator.</string>
    <string name="cordn_exposure_open">What this coordinator can see</string>
    <string name="cordn_exposure_close">Got it</string>
@@ -29,9 +29,13 @@ import com.vitorpamplona.quartz.mls.messages.KeyPackageBundle
 import com.vitorpamplona.quartz.mls.messages.KeyPackageBundleCodec
 import com.vitorpamplona.quartz.nip01Core.core.HexKey
 import com.vitorpamplona.quartz.nip01Core.core.toHexKey
+import com.vitorpamplona.quartz.utils.TimeUtils
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
+import kotlin.coroutines.cancellation.CancellationException
 import kotlin.io.encoding.Base64
 import kotlin.io.encoding.ExperimentalEncodingApi

@@ -122,6 +126,89 @@ class CordnKeyPackages(
        val at: Long,
    )

+    private val snapshotLock = Mutex()
+    private var snapshot: Snapshot? = null
+
+    /**
+     * What `kp_list` reported, the last time we asked.
+     *
+     * `kp_list` takes no arguments and returns every KeyPackage the coordinator
+     * holds for **every** identity (`spec/00.md` leaves retrieval scoping to the
+     * coordinator; the reference implementation answers with the whole table).
+     * So one call answers a question about anybody, and asking it once per
+     * person would be the same download repeated.
+     *
+     * [identities] keeps only pubkeys, not the entries. Another account's
+     * `kp_ref` is useless to us -- a Welcome is addressed to a ref we obtained
+     * by *taking* the package, never to one we read out of a listing -- and on
+     * a busy coordinator those refs are the bulk of the response. Dropping them
+     * bounds what we retain even though nothing bounds what arrives.
+     */
+    class Snapshot(
+        /** Every identity the coordinator holds at least one KeyPackage for. */
+        val identities: Set<HexKey>,
+        /** The full entries for this account, which are the ones we act on. */
+        val mine: List<AvailableKeyPackage>,
+        /** When this was fetched, for [snapshot]'s age check. */
+        val at: Long,
+    )
+
+    /**
+     * Refetches unconditionally and replaces the snapshot.
+     *
+     * Every decision about whether to *publish* reads through here rather than
+     * through [snapshot]. Minting against a stale listing is the one place
+     * staleness does damage: the coordinator keeps a single last-resort package
+     * per identity, so publishing a second silently evicts the first and
+     * strands every invite that referenced it.
+     */
+    suspend fun refresh(): Snapshot = snapshotLock.withLock { fetchLocked() }
+
+    /**
+     * The snapshot, refetched if it is older than [maxAgeSeconds].
+     *
+     * For reads that only *describe* what the coordinator holds. A minute of
+     * staleness costs at most a missing badge for somebody who published within
+     * it, and the act itself still fails loudly at the coordinator if the
+     * listing was wrong -- whereas every refresh is another full-table
+     * download, so asking less often is the cheap side of the trade.
+     */
+    suspend fun snapshot(maxAgeSeconds: Long = SNAPSHOT_TTL_SECONDS): Snapshot =
+        snapshotLock.withLock {
+            snapshot?.takeIf { TimeUtils.now() - it.at < maxAgeSeconds } ?: fetchLocked()
+        }
+
+    /**
+     * Which identities the coordinator can deliver a Welcome to, or null.
+     *
+     * Null means **we do not know** -- the coordinator is unreachable, throttled
+     * us, or answered with more than the transport admits. It does not mean
+     * nobody has published. Callers must keep those apart: rendering a failed
+     * lookup as "this person cannot be added" states something about a person
+     * on the strength of a network error.
+     */
+    suspend fun identitiesWithKeyPackages(): Set<HexKey>? =
+        try {
+            snapshot().identities
+        } catch (e: CancellationException) {
+            throw e
+        } catch (e: Exception) {
+            null
+        }
+
+    /** Must hold [snapshotLock]. */
+    private suspend fun fetchLocked(): Snapshot {
+        val all = coordinator.listKeyPackages()
+        return Snapshot(
+            identities = all.mapTo(mutableSetOf()) { it.pubKey },
+            mine = all.filter { it.pubKey == accountPubKey },
+            at = TimeUtils.now(),
+        ).also { snapshot = it }
+    }
+
+    /** Forgets the snapshot after we changed what the coordinator holds. */
+    private suspend fun invalidate() = snapshotLock.withLock { snapshot = null }
+
    /** Reloads which refs we hold private halves for. Call at startup. */
    suspend fun restore() {
        _published.value = store.list().toSet()
@@ -150,7 +237,7 @@ class CordnKeyPackages(
     * is actually available the moment anyone invites us — and it is what is
     * available that decides whether the next invitation can happen.
     */
-    suspend fun listPublished(): List<AvailableKeyPackage> = coordinator.listKeyPackages().filter { it.pubKey == accountPubKey }
+    suspend fun listPublished(): List<AvailableKeyPackage> = refresh().mine

    /**
     * Generates a KeyPackage, keeps its private half, and publishes it.
@@ -184,6 +271,7 @@ class CordnKeyPackages(
                throw e
            }

+        invalidate()
        return Published(result.keyPackageRef, result.lastResort || lastResort, result.at)
    }

@@ -219,6 +307,7 @@ class CordnKeyPackages(
        val removed = coordinator.removeKeyPackages(keyPackageRefs)
        removed.forEach { store.delete(it) }
        _published.value = _published.value - removed.toSet()
+        invalidate()
        return removed
    }

@@ -233,7 +322,7 @@ class CordnKeyPackages(
     */
    suspend fun topUp(minimum: Int = DEFAULT_POOL): List<String> {
        require(minimum >= 0) { "a KeyPackage pool cannot be negative" }
-        val theirs = coordinator.listKeyPackages().filter { it.pubKey == accountPubKey && !it.lastResort }
+        val theirs = refresh().mine.filter { !it.lastResort }
        val missing = minimum - theirs.size
        if (missing <= 0) return emptyList()
        return List(missing) { publishNew().keyPackageRef }
@@ -248,7 +337,7 @@ class CordnKeyPackages(
     * not the norm.
     */
    suspend fun ensureLastResort(): String? {
-        val existing = coordinator.listKeyPackages().filter { it.pubKey == accountPubKey && it.lastResort }
+        val existing = refresh().mine.filter { it.lastResort }
        // Only one, and only one we can still open: a last-resort package whose
        // private half this device never had is useless to it.
        val usable = existing.firstOrNull { store.load(it.keyPackageRef) != null }
@@ -292,5 +381,16 @@ class CordnKeyPackages(
         * last-resort package is the safety net.
         */
        const val DEFAULT_POOL = 5
+
+        /**
+         * How long a [Snapshot] may be reused by [snapshot].
+         *
+         * Bounded by what a refresh costs rather than by how fast the answer
+         * changes: `kp_list` is unpaginated, so each one downloads the
+         * coordinator's whole table. A minute is long enough that opening a
+         * screen repeatedly costs one call and short enough that somebody who
+         * has just published becomes visible while you are still looking.
+         */
+        const val SNAPSHOT_TTL_SECONDS = 60L
    }
 }
@@ -21,6 +21,8 @@
 package com.vitorpamplona.amethyst.commons.cordn

 import com.vitorpamplona.quartz.cordn.groups.CordnCredential
+import com.vitorpamplona.quartz.cordn.spec00Coordinator.AvailableKeyPackage
+import com.vitorpamplona.quartz.cordn.spec00Coordinator.ICoordinator
 import com.vitorpamplona.quartz.cordn.spec00Coordinator.KeyPackagePublication
 import com.vitorpamplona.quartz.mls.codec.TlsReader
 import com.vitorpamplona.quartz.mls.messages.KeyPackageBundleCodec
@@ -335,4 +337,62 @@ class CordnKeyPackagesTest : CordnTransportHarness() {

            assertTrue(driving { alice.keyPackages.listPublished() }.all { it.pubKey == alice.pubKey })
        }
+
+    @Test
+    fun `the identity set spans everybody, where listPublished is scoped to us`() =
+        runTest {
+            // The two read one `kp_list` response and keep different halves of
+            // it. Scoping the identity set the way listPublished is scoped
+            // would answer "can I add Bob" with our own packages, which is
+            // always no.
+            val alice = Account()
+            val bob = Account()
+            driving { alice.keyPackages.publishNew() }
+            driving { bob.keyPackages.publishNew() }
+
+            val reachable = assertNotNull(driving { alice.keyPackages.identitiesWithKeyPackages() })
+            assertTrue(alice.pubKey in reachable, "ourselves")
+            assertTrue(bob.pubKey in reachable, "somebody else")
+        }
+
+    @Test
+    fun `publishing shows up straight away rather than at the end of the snapshot's life`() =
+        runTest {
+            // The snapshot is reused for a minute, so our own publish has to
+            // drop it. Otherwise the account that just published a package is
+            // told for the next minute that it has none.
+            val alice = Account()
+            assertTrue(driving { alice.keyPackages.identitiesWithKeyPackages() }?.contains(alice.pubKey) == false)
+
+            driving { alice.keyPackages.publishNew() }
+
+            assertTrue(driving { alice.keyPackages.identitiesWithKeyPackages() }?.contains(alice.pubKey) == true)
+        }
+
+    @Test
+    fun `a coordinator that cannot answer is unknown, not nobody`() =
+        runTest {
+            // The distinction the whole return type exists for. An empty set
+            // says the coordinator holds nothing for anyone; null says we never
+            // found out. A caller that folds them together renders a network
+            // failure as a claim that somebody cannot be added.
+            val alice = Account()
+            driving { alice.keyPackages.publishNew() }
+
+            val blind =
+                CordnKeyPackages(
+                    accountPubKey = alice.pubKey,
+                    coordinator = UnreachableList(alice.coordinatorClient),
+                    store = InMemoryCordnKeyPackageStore(),
+                )
+
+            assertNull(driving { blind.identitiesWithKeyPackages() })
+        }
+
+    /** A coordinator whose `kp_list` is the one call that fails. */
+    private class UnreachableList(
+        delegate: ICoordinator,
+    ) : ICoordinator by delegate {
+        override suspend fun listKeyPackages(): List<AvailableKeyPackage> = throw IllegalStateException("kp_list unreachable")
+    }
 }