From 98f766fcd320a7c7c455dfc82c078efc4fe06a0f Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sun, 6 Sep 2026 18:16:26 +0200 Subject: [PATCH] fix: put the invite in the room, so a stuck one can be seen Inviting a member to a group that already had members put nothing whatsoever in the transcript. Not "put it in late" -- nothing, and nothing ever if the invite did not complete. So the one failure the user is best placed to notice, an invite that never reached the person it was made for, was the one the app kept to itself. The line existed. It was written by `MarmotOutboundDao.deliveryWelcome`, which is the wrong place for it, and the reason is the two paths through `inviteMember` that docs/marmot-membership.md already describes. A group that is still only its creator has nobody to inform, so its Welcome goes out immediately and `deliveryWelcome` runs inside the invite. A group that has members must broadcast a commit first, and its Welcome waits for a relay to acknowledge it -- `DatabaseNostrRepository.broadcastProcessed` picks the stored `MarmotCommitResult` back up and delivers then. Every invite after a group's first therefore wrote its transcript line a relay round trip away from the invite, if at all. **Four separate silences, not one.** Worth listing because only the first is about the deferral, and fixing that alone would have left the other three: 1. The deferred path wrote nothing until the ack, and nothing ever without one. 2. The write hung off `getMarmotKeyPackageById(...)?.let { getProfileByPublicKey(...)?.let { ... } }`. Those two lookups were there to *name* the invitee, and a miss on either cost the whole line rather than just the name. 3. `deliveryWelcome` wraps its body in `catch (e: Throwable) { logger.e(...) }` and returned Unit, so a Welcome that could not be built reached the log and no further. 4. `inviteMemberToChatRoom` is `@Transaction`. An invite that threw -- no MLS state for the room, a credential identity that does not match the peer -- rolled its line back with everything else, which is right, and left no account of the refusal anywhere durable. And the line it did write was `messageType = "message"`, `isUserMessage = true`, so it rendered as a chat bubble: "Invited Bob to chat", attributed to the inviter as something they said. **Three membership types, and the line moves to invite time.** `ChatMessage.MEMBERSHIP_TYPES` -- `memberInvited`, `memberInviteSent`, `memberInviteFailed` -- rendered by the transcript as system notices through `RitualNotice`, the way the ceremony, signing and chronicle lines already are. `memberInvited` is written by `inviteMember` and by `addMembersToChatRoom`'s batch path, *when the invite is made*, and deliberately **inside** the caller's transaction. Both halves of that matter and they pull opposite ways: written any later and an invite waiting on an ack that never comes shows nothing, which is the bug; written outside the transaction and an invite that does not survive `addMember` leaves the room claiming one was made. `memberInviteSent` is written by `DatabaseNostrRepository` alone. It is not written on the immediate path, and that is not an oversight: there the Welcome goes out in the same breath as the invite, so one line is the whole truth. It would also be a line the transcript could not order -- `MantraConverters` stores `Instant` as `epochSeconds`, the room query is `ORDER BY createdAt DESC`, and two rows written in the same second tie. Only the deferred path separates the two events in time, so only it owes a second line. `memberInviteFailed` carries the reason, because it is the only copy the user gets. `deliveryWelcome` now returns `Boolean` and files this line from its own catch before returning false -- its callers had no other way to see a failure it had already swallowed, and on the deferred path there is no invite screen left to fail back to. `addMembersToChatRoom` reads that answer instead of a `runCatching` that could never catch anything. **The refusal is written from outside the transaction that rolled it back.** `DatabaseChatRepository.inviteMember` catches, calls `announceInviteFailed`, and rethrows. The throw is what puts a message on the invite screen now; the line is what is still there tomorrow. Swallowing it instead would have popped the user back to the chat as though the invite had gone out, which is the bug the existing `runCatching` in `AddMemberToChatRoomConfirmationViewModel` was added to stop. **No schema change.** `messageType` is a free-form string column with a default, so new values need no migration -- unlike the chronicle rename, which had to rewrite the ones already stored. Nothing reindexes these either: they carry no `marmotGroupEventId`, so `getResolvedMarmotGroupEventIds` cannot see them and `UNRESOLVED_MARMOT_TYPES` does not name them. Six new tests. Four on the DAO: the immediate path leaves a line naming the invitee where the old code left none, the deferred path leaves one *and* claims no Welcome sent before any ack, everything an invite writes is a membership type rather than something the transcript would render as a bubble, and a refused invite leaves no claim that one was made. Two new ones on `DatabaseChatRepository`, which had no test file: a refused invite is written into the room, and the caller still gets the throw. Still open, and now said plainly in the doc rather than implied: a Participant row carries no state saying where its invite got to. The transcript narrates it; the `TODO: Update status of participant Invitation.PENDING -> Invitation.SENT` is untouched. Nor does an invitee with no published key package reach the room at all -- that fails in the view model, before there is an invite to write a line about. 806 tests pass -- 509 jvm, 297 android. Co-Authored-By: Claude Opus 5 --- .../compose/database/dao/MarmotOutboundDao.kt | 213 +++++++++++++++--- .../compose/database/model/ChatMessage.kt | 43 ++++ .../repository/DatabaseChatRepository.kt | 29 ++- .../repository/DatabaseNostrRepository.kt | 44 +++- .../ui/view/model/ChatMessageListViewModel.kt | 28 ++- .../database/dao/MarmotOutboundDaoJvmTest.kt | 143 ++++++++++-- .../DatabaseChatRepositoryJvmTest.kt | 126 +++++++++++ docs/marmot-membership.md | 40 +++- 8 files changed, 605 insertions(+), 61 deletions(-) create mode 100644 composeApp/src/jvmTest/kotlin/press/mantra/compose/database/repository/DatabaseChatRepositoryJvmTest.kt diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt index 6b82cd43..2539c488 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/dao/MarmotOutboundDao.kt @@ -21,6 +21,7 @@ import press.mantra.compose.database.model.Participant import press.mantra.compose.exceptions.MarmotMissingChatGroupException import press.mantra.compose.database.model.intermdiate.LocalChatRoom import press.mantra.compose.extensions.exporterSecret +import press.mantra.compose.extensions.shortened import press.mantra.compose.extensions.toHex import press.mantra.compose.managers.MarmotInboundManager.EPOCH_RETENTION_WINDOW import press.mantra.compose.nostr.MarmotDelivery @@ -231,6 +232,12 @@ abstract class MarmotOutboundDao( }.onFailure { logger.e("Failed to invite $peerPublicKey to ${localChatRoom.chatRoom.id}", it) notAdded.add(peerPublicKey) + announceInviteFailed( + chatRoomId = localChatRoom.chatRoom.id, + userPublicKey = localChatRoom.chatRoom.userPublicKey, + peerPublicKey = peerPublicKey, + reason = it.message, + ) } } @@ -287,9 +294,30 @@ abstract class MarmotOutboundDao( ) ) + // One line each, as `inviteMember` writes for the invites it makes -- this + // path does its own commit and never goes through it. Written before the + // Welcomes rather than after, so a delivery that fails has an invite to be + // read against instead of a failure on its own. + peers.forEach { (peerPublicKey, _) -> + announceMembership( + chatRoomId = localChatRoom.chatRoom.id, + userPublicKey = localChatRoom.chatRoom.userPublicKey, + messageType = ChatMessage.TYPE_MEMBER_INVITED, + content = "Invited ${memberName(peerPublicKey)} to the group", + ) + } + val welcomeBytes = commitResult.welcomeBytes if (welcomeBytes == null) { logger.e("Batched commit for ${localChatRoom.chatRoom.id} produced no welcome") + peers.forEach { (peerPublicKey, _) -> + announceInviteFailed( + chatRoomId = localChatRoom.chatRoom.id, + userPublicKey = localChatRoom.chatRoom.userPublicKey, + peerPublicKey = peerPublicKey, + reason = "the group produced no invitation to send", + ) + } return peers.map { it.first } } @@ -300,17 +328,20 @@ abstract class MarmotOutboundDao( val notAdded = mutableListOf() peers.forEach { (peerPublicKey, peerKeyPackage) -> - runCatching { - deliveryWelcome( - nostrGroupId = localChatRoom.chatRoom.id, - userPublicKey = localChatRoom.chatRoom.userPublicKey, - welcomeBytes = welcomeBytes, - peerKeyPackageEventId = peerKeyPackage.id, - relays = relays, - createdAt = Clock.System.now() - ) - }.onFailure { - logger.e("Failed to deliver the welcome to $peerPublicKey", it) + // `deliveryWelcome` swallows what it catches and reports it as a + // transcript line instead, so its answer is what says whether this peer + // was reached -- a runCatching here would see nothing to catch. + val delivered = deliveryWelcome( + nostrGroupId = localChatRoom.chatRoom.id, + userPublicKey = localChatRoom.chatRoom.userPublicKey, + welcomeBytes = welcomeBytes, + peerKeyPackageEventId = peerKeyPackage.id, + relays = relays, + createdAt = Clock.System.now() + ) + + if (!delivered) { + logger.e("Failed to deliver the welcome to $peerPublicKey") notAdded.add(peerPublicKey) } } @@ -318,6 +349,100 @@ abstract class MarmotOutboundDao( return notAdded } + /** + * One line in the room's transcript, about a membership change. + * + * The same shape as `ChronicleManager.announce`, and for the same reason: this + * is the device saying what it just did, not an event anybody sent. Content is + * a whole sentence, so nothing prefixes a name to it -- see + * [ChatMessage.MEMBERSHIP_TYPES]. + */ + private suspend fun announceMembership( + chatRoomId: HexKey, + userPublicKey: HexKey, + messageType: String, + content: String, + ) { + database.chatMessageDao().upsert( + ChatMessage( + content = content, + messageType = messageType, + chatRoomId = chatRoomId, + senderPublicKey = userPublicKey, + isUserMessage = true, + giftWrapPayloadId = null, + marmotGroupEventId = null, + marmotInnerEventId = null, + ) + ) + } + + /** + * What to call an invitee on a membership line. + * + * Written into the content rather than resolved at render time, because these + * lines are about something that happened once and the AUTHORED-set machinery + * that follows a rename is for lines with an actor. A member being invited has + * a Profile row by construction -- Participant.participantPublicKey is a + * foreign key onto it -- so the fallback is for the delivery path, which runs + * long after the invite and reads its peer back out of a key package. + */ + private suspend fun memberName(publicKey: HexKey): String = + database.profileDao().getProfileByPublicKey(publicKey)?.humanReadableNameOrPubkey() + ?: publicKey.shortened() + + /** + * The Welcome for an invite made earlier has gone out. + * + * Written by the deferred delivery site only -- `DatabaseNostrRepository`, once + * a relay has acknowledged the commit. An invite into a group that was still + * just its creator sends its Welcome in the same breath as the invite, so its + * [ChatMessage.TYPE_MEMBER_INVITED] line already says this and a second one + * would only be the same second told twice. See [ChatMessage.MEMBERSHIP_TYPES]. + */ + suspend fun announceInviteSent( + chatRoomId: HexKey, + userPublicKey: HexKey, + peerKeyPackageEventId: HexKey, + ) { + val peerPublicKey = database.marmotKeyPackageDao() + .getMarmotKeyPackageById(peerKeyPackageEventId)?.publicKey + + announceMembership( + chatRoomId = chatRoomId, + userPublicKey = userPublicKey, + messageType = ChatMessage.TYPE_MEMBER_INVITE_SENT, + content = peerPublicKey?.let { "Sent ${memberName(it)} their invitation" } + ?: "Sent the invitation", + ) + } + + /** + * An invite did not make it, and nothing else will say so. + * + * The reason is put in the line because it is the only copy the user gets: the + * screen that asked for the invite is gone by the time the Welcome is + * delivered, and the failures that happen before it closes take the whole + * transaction -- and the invite line inside it -- down with them. See + * `DatabaseChatRepository.inviteMember`. + */ + suspend fun announceInviteFailed( + chatRoomId: HexKey, + userPublicKey: HexKey, + peerPublicKey: HexKey?, + reason: String?, + ) { + val who = peerPublicKey?.let { "${memberName(it)}'s invitation" } ?: "the invitation" + val why = reason?.takeIf { it.isNotBlank() }?.let { ": $it" } ?: "" + + announceMembership( + chatRoomId = chatRoomId, + userPublicKey = userPublicKey, + messageType = ChatMessage.TYPE_MEMBER_INVITE_FAILED, + content = "Couldn't send $who$why", + ) + } + private suspend fun inviteMember( nostrGroupId: HexKey, mlsGroup: MlsGroup, @@ -357,6 +482,20 @@ abstract class MarmotOutboundDao( "KeyPackage credential identity does not match memberPubKey" } + // Say so now, not when the Welcome eventually goes out. On the deferred path + // that is a relay round trip away and may never happen at all, and until this + // line existed the room showed nothing whatsoever in the meantime -- an invite + // that was queued, one that failed to reach the wire and one that was never + // made all looked identical from the transcript. Inside the caller's + // transaction, so an invite that does not survive `addMember` leaves no claim + // that it did. + announceMembership( + chatRoomId = nostrGroupId, + userPublicKey = userPublicKey, + messageType = ChatMessage.TYPE_MEMBER_INVITED, + content = "Invited ${memberName(peerPublicKey)} to the group", + ) + val retainedBefore = mlsGroup.retainedSecrets() val commitResult = mlsGroup.addMember( peerKeyPackage.tlsEncodedMarmotKeyPackage @@ -462,6 +601,16 @@ abstract class MarmotOutboundDao( } } + /** + * Put the invitee's Welcome on the outbound queue. + * + * Returns whether it got there. The caller cannot see that any other way -- the + * body swallows what it catches, deliberately, because on the deferred path this + * runs from a relay acknowledgement with no invite screen left to fail back to. + * What replaced reporting nothing at all is the [ChatMessage.TYPE_MEMBER_INVITE_FAILED] + * line written below, which outlives the screen and is the only account of a + * Welcome that never went out. + */ suspend fun deliveryWelcome( nostrGroupId: HexKey, userPublicKey: HexKey, @@ -469,7 +618,7 @@ abstract class MarmotOutboundDao( peerKeyPackageEventId: HexKey, relays: List, createdAt: Instant - ) { + ): Boolean { try { val welcomeBase64 = Base64.encode( source = welcomeBytes @@ -507,22 +656,14 @@ abstract class MarmotOutboundDao( ) ) + // The invite's own transcript line is not written here. It used to be, and + // that put nothing in the room for an invite into an established group -- + // where this runs a relay round trip later, if at all, and never at all if + // the ack does not come. It also hung off the two lookups below, so a + // missing key package or profile cost the line and not just the name. + // `inviteMember` writes it when the invite is made instead; what this + // function still owes the room is the failure below. database.marmotKeyPackageDao().getMarmotKeyPackageById(peerKeyPackageEventId)?.let { marmotKeyPackage -> - database.profileDao().getProfileByPublicKey(marmotKeyPackage.publicKey)?.let { profile -> - // Save chatMessage for the invite... - database.chatMessageDao().upsert( - ChatMessage( - content = "Invited ${profile.humanReadableNameOrPubkey() ?: "participant"} to chat", // use profile.humanReadable... - chatRoomId = nostrGroupId, - senderPublicKey = userPublicKey, - isUserMessage = true, // TODO: This is information message... - giftWrapPayloadId = welcomeEventId, - marmotGroupEventId = null, - marmotInnerEventId = null - ) - ) - } - // The group's signed work, offered to the member being invited. // Nothing else will ever show it to them: MLS gives a joiner no // history, and a group-signed event never travels -- every device @@ -550,8 +691,26 @@ abstract class MarmotOutboundDao( recipient = marmotKeyPackage.publicKey, ) } + + return true } catch (e: Throwable) { logger.e("Failed to deliver welcome:", e) + + // The room's only account of this. Its own runCatching because the + // failure being reported may well be the database, and a throw from + // here would be one the callers on the immediate path do not expect -- + // taking the invite's transaction down over a line about it. + runCatching { + announceInviteFailed( + chatRoomId = nostrGroupId, + userPublicKey = userPublicKey, + peerPublicKey = database.marmotKeyPackageDao() + .getMarmotKeyPackageById(peerKeyPackageEventId)?.publicKey, + reason = e.message, + ) + }.onFailure { logger.e("Failed to record the undelivered welcome:", it) } + + return false } } diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/ChatMessage.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/ChatMessage.kt index e657249c..cab0c0d6 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/ChatMessage.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/model/ChatMessage.kt @@ -357,6 +357,49 @@ data class ChatMessage( TYPE_CHRONICLE_RECEIVED, ) + /** + * Adding a member to the group, as lines in the group's chat. + * + * An invite is two separate things, and in an established group they can be + * minutes apart: the membership change this device commits, and the Welcome + * that lets the invitee in going on the wire. The second waits on a relay + * acknowledging the commit -- see docs/marmot-membership.md -- and it is the + * one that can fail after the screen that asked for it has closed, which + * leaves the transcript as the only place able to say so. + * + * So [TYPE_MEMBER_INVITED] is written the moment the invite is made, and + * [TYPE_MEMBER_INVITE_SENT] only where the Welcome is delivered later than + * that. Where the two happen together -- a group that is still only its + * creator has nobody to inform, so its Welcome goes out immediately -- there + * is one line, because there was one event. + * + * Written by the inviter's device, for itself. None of these travels: what + * the group receives is the commit, and each member's transcript is written + * from what it made of that. A member watching from the side sees nothing + * here, correctly. + * + * Content is a whole sentence rather than a predicate, so these stay out of + * the AUTHORED sets and nothing prefixes a name to them. The invitee's name + * is written into the content the way the chronicle's is, which does not + * follow a rename and is the accepted cost for a line about a thing that + * happened once. + */ + const val TYPE_MEMBER_INVITED = "memberInvited" + const val TYPE_MEMBER_INVITE_SENT = "memberInviteSent" + const val TYPE_MEMBER_INVITE_FAILED = "memberInviteFailed" + + /** + * Every membership line, for the one check the transcript dispatches on. + * + * A type missing from here renders as a chat bubble -- silently, and looking + * exactly like the inviter having said "Invited Bob to the group". + */ + val MEMBERSHIP_TYPES = setOf( + TYPE_MEMBER_INVITED, + TYPE_MEMBER_INVITE_SENT, + TYPE_MEMBER_INVITE_FAILED, + ) + const val TYPE_UNDECRYPTABLE_OUTER_LAYER = "undecryptableOuterLayer" const val TYPE_PENDING_COMMIT = "pendingCommit" diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt index 80b04c00..51f0087d 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt @@ -168,11 +168,30 @@ class DatabaseChatRepository( peerPublicKey: HexKey, peerKeyPackage: MarmotKeyPackage ) { - database.marmotOutboundDao().inviteMemberToChatRoom( - localChatRoom = localChatRoom, - peerPublicKey = peerPublicKey, - peerKeyPackage = peerKeyPackage - ) + try { + database.marmotOutboundDao().inviteMemberToChatRoom( + localChatRoom = localChatRoom, + peerPublicKey = peerPublicKey, + peerKeyPackage = peerKeyPackage + ) + } catch (e: Throwable) { + // Outside the transaction that just went down, which is the point: the + // invite line `inviteMember` writes went with it, so an invite refused + // for want of MLS state or over a mismatched credential left the room + // with nothing at all to show. The caller still gets the throw and + // still puts its own message on the invite screen -- this is the copy + // that is still there tomorrow. + logger.e("Failed to invite $peerPublicKey to ${localChatRoom.chatRoom.id}", e) + runCatching { + database.marmotOutboundDao().announceInviteFailed( + chatRoomId = localChatRoom.chatRoom.id, + userPublicKey = localChatRoom.chatRoom.userPublicKey, + peerPublicKey = peerPublicKey, + reason = e.message, + ) + }.onFailure { logger.e("Failed to record the refused invite:", it) } + throw e + } } override suspend fun addMembers( localChatRoom: LocalChatRoom, diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseNostrRepository.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseNostrRepository.kt index 54ec9490..a827c7eb 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseNostrRepository.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseNostrRepository.kt @@ -439,15 +439,45 @@ class DatabaseNostrRepository( database.chatRoomDao().findChatRoomById(marmotCommitResult.chatRoomId)?.let { localChatRoom -> // TODO: Update status of participant Invitation.PENDING -> Invitation.SENT logger.d("localChatRoom: $localChatRoom") - marmotCommitResult.welcomeBytes?.let { welcomeBytes -> - logger.d("welcomeBytes: ${welcomeBytes.toHex()}") - database.marmotOutboundDao().deliveryWelcome( - nostrGroupId = marmotCommitResult.chatRoomId, + val welcomeBytes = marmotCommitResult.welcomeBytes + if (welcomeBytes == null) { + // The commit was acknowledged and there is nothing to let + // the invitee in with, so this invite is over. Nobody is + // waiting on the answer by now -- the screen that asked + // closed when the commit was made -- which is why it goes + // in the room rather than to a caller. + database.marmotOutboundDao().announceInviteFailed( + chatRoomId = marmotCommitResult.chatRoomId, + userPublicKey = marmotCommitResult.userPublicKey, + peerPublicKey = database.marmotKeyPackageDao() + .getMarmotKeyPackageById(marmotCommitResult.peerKeyPackageEventId) + ?.publicKey, + reason = "the commit carried no invitation", + ) + return@let + } + + logger.d("welcomeBytes: ${welcomeBytes.toHex()}") + val delivered = database.marmotOutboundDao().deliveryWelcome( + nostrGroupId = marmotCommitResult.chatRoomId, + userPublicKey = marmotCommitResult.userPublicKey, + welcomeBytes = welcomeBytes, + peerKeyPackageEventId = marmotCommitResult.peerKeyPackageEventId, + createdAt = marmotCommitResult.createdAt, + relays = Relays.DefaultDMRelayList.map { it.url } // TODO: Get localChatRoom relays... + ) + + // This is the deferred delivery site, and the only one where + // the Welcome goes out later than the invite that made it -- + // so it is the only one that owes the room a second line. An + // invite whose room was still just its creator sent its + // Welcome in the same breath and said so once already. A + // failure needs nothing here: `deliveryWelcome` files its own. + if (delivered) { + database.marmotOutboundDao().announceInviteSent( + chatRoomId = marmotCommitResult.chatRoomId, userPublicKey = marmotCommitResult.userPublicKey, - welcomeBytes = welcomeBytes, peerKeyPackageEventId = marmotCommitResult.peerKeyPackageEventId, - createdAt = marmotCommitResult.createdAt, - relays = Relays.DefaultDMRelayList.map { it.url } // TODO: Get localChatRoom relays... ) } } diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt index 66067b1b..4a72aabf 100755 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt @@ -424,6 +424,24 @@ class ChatMessageListViewModel( return@items } + // Inviting somebody is nobody's words either, and + // for a while it was not in the room at all: the + // line was written when the Welcome went out, which + // on the deferred path is a relay round trip away + // and may never happen. Passed as answered and + // settled for the same reason the chronicle's are + // -- these report rather than ask, so they stay in + // the quiet tint and offer nothing to review. + if (localChatMessage.chatMessage.messageType in ChatMessage.MEMBERSHIP_TYPES) { + RitualNotice( + localChatMessage = localChatMessage, + isAnswered = true, + isSettled = true, + onClick = {} + ) + return@items + } + // Signing lines are the same kind of thing and get // the same treatment -- nobody said them either -- // but they lead somewhere else, because what a @@ -745,6 +763,13 @@ private fun RitualNotice( ChatMessage.TYPE_CHRONICLE_SENT -> Icons.Default.Upload ChatMessage.TYPE_CHRONICLE_RECEIVED -> Icons.Default.Download + // An invite made and an invite sent are two separate steps on the deferred + // path, so they get separate icons -- the whole reason both lines exist is + // to be able to see that the first happened and the second did not. + ChatMessage.TYPE_MEMBER_INVITED -> Icons.Default.PersonAdd + ChatMessage.TYPE_MEMBER_INVITE_SENT -> Icons.Default.Upload + ChatMessage.TYPE_MEMBER_INVITE_FAILED -> Icons.Default.ErrorOutline + else -> Icons.Default.PanTool } @@ -763,7 +788,8 @@ private fun RitualNotice( val tint = when { chatMessage.messageType == ChatMessage.TYPE_DKG_FAILED || - chatMessage.messageType == ChatMessage.TYPE_FROST_FAILED -> + chatMessage.messageType == ChatMessage.TYPE_FROST_FAILED || + chatMessage.messageType == ChatMessage.TYPE_MEMBER_INVITE_FAILED -> MaterialTheme.colorScheme.error isRequest -> MaterialTheme.colorScheme.primary else -> MaterialTheme.colorScheme.onSurfaceVariant diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/dao/MarmotOutboundDaoJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/dao/MarmotOutboundDaoJvmTest.kt index 37693973..f8698d79 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/dao/MarmotOutboundDaoJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/dao/MarmotOutboundDaoJvmTest.kt @@ -6,6 +6,7 @@ import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair import kotlinx.coroutines.runBlocking import press.mantra.compose.database.MantraDatabase import press.mantra.compose.database.builder.getRoomDatabase +import press.mantra.compose.database.model.ChatMessage import press.mantra.compose.database.model.ChatRoom import press.mantra.compose.database.model.MarmotKeyPackage import press.mantra.compose.database.model.NostrEvent @@ -51,25 +52,36 @@ class MarmotOutboundDaoJvmTest { private val user = KeyPair().pubKey.toHexKey() private val peer = KeyPair().pubKey.toHexKey() + private val secondPeer = KeyPair().pubKey.toHexKey() private val roomId = "b".repeat(64) /** - * A room with `mlsGroupState = null` -- exactly the shape a room restored from an inbound - * gift wrap has, which is the case the guard exists for. + * A member the database knows by name. Every invitee needs one -- + * Participant.participantPublicKey is a foreign key onto Profile -- and the membership + * lines read the name back out of it. */ - private suspend fun seedStatelessRoom(): LocalChatRoom { - val nostrEventId = "c".repeat(64) + private suspend fun seedProfile(publicKey: String, name: String, nostrEventId: String) { db.nostrEventDao().upsert( NostrEvent( id = nostrEventId, - pubKey = user, + pubKey = publicKey, kind = 0, tags = emptyArray(), content = "{}", sig = "0".repeat(128), ) ) - db.profileDao().upsert(Profile(publicKey = user, userName = "user", nostrEventId = nostrEventId)) + db.profileDao().upsert( + Profile(publicKey = publicKey, userName = name, nostrEventId = nostrEventId) + ) + } + + /** + * A room with `mlsGroupState = null` -- exactly the shape a room restored from an inbound + * gift wrap has, which is the case the guard exists for. + */ + private suspend fun seedStatelessRoom(): LocalChatRoom { + seedProfile(user, "user", "c".repeat(64)) val chatRoom = ChatRoom( id = roomId, userPublicKey = user, @@ -100,18 +112,7 @@ class MarmotOutboundDaoJvmTest { */ private suspend fun seedMlsRoom(): LocalChatRoom { val stateless = seedStatelessRoom() - val peerEventId = "e".repeat(64) - db.nostrEventDao().upsert( - NostrEvent( - id = peerEventId, - pubKey = peer, - kind = 0, - tags = emptyArray(), - content = "{}", - sig = "0".repeat(128), - ) - ) - db.profileDao().upsert(Profile(publicKey = peer, userName = "peer", nostrEventId = peerEventId)) + seedProfile(peer, "peer", "e".repeat(64)) val mlsGroup = MlsGroup.create( identity = user.hexToByteArray(), initialExtensions = listOf( @@ -265,4 +266,110 @@ class MarmotOutboundDaoJvmTest { assertEquals(1, retained.size, "the pre-commit epoch was not retained") assertEquals(0L, retained.single().epoch, "a freshly created group is at epoch 0") } + + // ---- What the room is told about it ----------------------------------- + + private suspend fun transcript() = db.chatMessageDao().getChatMessagesByChatRoomId(roomId) + .map { it.chatMessage } + + /** + * The invite has to reach the transcript, and as a membership line rather than a chat + * bubble. It used to be written by `deliveryWelcome` -- which meant it depended on the + * invitee's key package and profile both being found, and neither is here, so the room + * was told nothing at all about an invite that succeeded. + */ + @Test + fun `inviting a member puts a line in the room`() = runBlocking { + val localChatRoom = seedMlsRoom() + + db.marmotOutboundDao().inviteMemberToChatRoom( + localChatRoom = localChatRoom, + peerPublicKey = peer, + peerKeyPackage = marmotKeyPackageFor(peer), + ) + + val line = transcript().singleOrNull { it.messageType == ChatMessage.TYPE_MEMBER_INVITED } + assertNotNull(line, "the invite left no line in the room") + assertTrue(line.content.contains("peer"), "the line does not name the invitee: ${line.content}") + } + + /** + * The case the line exists for. A group that already has members defers its Welcome until + * a relay acknowledges the commit -- see docs/marmot-membership.md -- so an invite made + * here is on nothing but a promise, and until the line moved to invite time the room said + * nothing whatsoever in the meantime and nothing ever if the ack never came. + */ + @Test + fun `an invite into an established group is in the room before its welcome goes out`() = runBlocking { + db.marmotOutboundDao().inviteMemberToChatRoom( + localChatRoom = seedMlsRoom(), + peerPublicKey = peer, + peerKeyPackage = marmotKeyPackageFor(peer), + ) + seedProfile(secondPeer, "second", "f".repeat(64)) + + // Re-read: the first invite advanced the epoch and persisted new state. + val established = assertNotNull(db.chatRoomDao().findChatRoomById(roomId)) + db.marmotOutboundDao().inviteMemberToChatRoom( + localChatRoom = established, + peerPublicKey = secondPeer, + peerKeyPackage = marmotKeyPackageFor(secondPeer), + ) + + val invited = transcript().filter { it.messageType == ChatMessage.TYPE_MEMBER_INVITED } + assertEquals(2, invited.size, "the deferred invite left no line in the room") + assertTrue( + invited.any { it.content.contains("second") }, + "the second invitee is not named: ${invited.map { it.content }}", + ) + + // Nothing has acknowledged the commit, so the Welcome has not gone out and nothing + // may claim it has. That absence is what makes a stuck invite visible. + assertTrue( + transcript().none { it.messageType == ChatMessage.TYPE_MEMBER_INVITE_SENT }, + "a welcome was reported sent before any relay acknowledged the commit", + ) + } + + /** + * Membership lines are not somebody's words, and the transcript decides that from the + * type alone. One missing from [ChatMessage.MEMBERSHIP_TYPES] renders as a chat bubble, + * looking exactly like the inviter having said "Invited peer to the group". + */ + @Test + fun `the invite line is a membership line and not a bubble`() = runBlocking { + db.marmotOutboundDao().inviteMemberToChatRoom( + localChatRoom = seedMlsRoom(), + peerPublicKey = peer, + peerKeyPackage = marmotKeyPackageFor(peer), + ) + + assertTrue( + transcript().all { it.messageType in ChatMessage.MEMBERSHIP_TYPES }, + "an invite wrote something the transcript would render as a bubble: " + + transcript().map { it.messageType }, + ) + } + + /** + * A refused invite must not leave the room claiming one was made. The line is written + * inside the caller's transaction precisely so that it goes down with everything else the + * refused invite touched -- what tells the user instead is + * `DatabaseChatRepository.inviteMember`, from outside it. + */ + @Test + fun `a refused invite leaves no claim that one was made`() = runBlocking { + runCatching { + db.marmotOutboundDao().inviteMemberToChatRoom( + localChatRoom = seedStatelessRoom(), + peerPublicKey = peer, + peerKeyPackage = keyPackage(), + ) + } + + assertTrue( + transcript().none { it.messageType == ChatMessage.TYPE_MEMBER_INVITED }, + "the room was told about an invite that was refused", + ) + } } diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/repository/DatabaseChatRepositoryJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/repository/DatabaseChatRepositoryJvmTest.kt new file mode 100644 index 00000000..4bfc224c --- /dev/null +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/repository/DatabaseChatRepositoryJvmTest.kt @@ -0,0 +1,126 @@ +package press.mantra.compose.database.repository + +import androidx.room3.Room +import com.vitorpamplona.quartz.nip01Core.core.toHexKey +import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.runBlocking +import press.mantra.compose.database.MantraDatabase +import press.mantra.compose.database.builder.getRoomDatabase +import press.mantra.compose.database.model.ChatMessage +import press.mantra.compose.database.model.ChatRoom +import press.mantra.compose.database.model.MarmotKeyPackage +import press.mantra.compose.database.model.NostrEvent +import press.mantra.compose.database.model.Profile +import press.mantra.compose.database.model.intermdiate.LocalChatRoom +import press.mantra.compose.exceptions.MarmotMissingChatGroupException +import kotlin.test.AfterTest +import kotlin.test.Test +import kotlin.test.assertFailsWith +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +/** + * What the room is told when an invite does not happen. + * + * `MarmotOutboundDao.inviteMemberToChatRoom` writes the invite's transcript line inside its + * own transaction, which is right: an invite that does not survive `addMember` must not leave + * the room claiming one was made. But it means a refusal takes the only account of itself + * down with it, and the screen that asked has closed by the time anybody looks -- so the + * repository is where a failure has to be written from, outside the transaction that rolled + * back. + */ +class DatabaseChatRepositoryJvmTest { + + private val db: MantraDatabase = getRoomDatabase( + Room.inMemoryDatabaseBuilder() + ) + + @AfterTest + fun closeDb() = db.close() + + private val user = KeyPair().pubKey.toHexKey() + private val peer = KeyPair().pubKey.toHexKey() + private val roomId = "b".repeat(64) + + private val repository = DatabaseChatRepository( + database = db, + scope = CoroutineScope(Dispatchers.Unconfined), + ) + + private suspend fun seedProfile(publicKey: String, name: String, nostrEventId: String) { + db.nostrEventDao().upsert( + NostrEvent( + id = nostrEventId, + pubKey = publicKey, + kind = 0, + tags = emptyArray(), + content = "{}", + sig = "0".repeat(128), + ) + ) + db.profileDao().upsert( + Profile(publicKey = publicKey, userName = name, nostrEventId = nostrEventId) + ) + } + + /** A room restored from an inbound gift wrap: no MLS state, so nothing to invite into. */ + private suspend fun seedStatelessRoom(): LocalChatRoom { + seedProfile(user, "user", "c".repeat(64)) + seedProfile(peer, "peer", "e".repeat(64)) + val chatRoom = ChatRoom( + id = roomId, + userPublicKey = user, + subject = "a restored room", + description = null, + mlsGroupState = null, + ) + db.chatRoomDao().upsert(chatRoom) + return LocalChatRoom(chatRoom = chatRoom) + } + + private fun keyPackage() = MarmotKeyPackage( + id = "d".repeat(64), + publicKey = peer, + tlsEncodedMarmotKeyPackage = ByteArray(0), + ) + + @Test + fun `a refused invite is written into the room it was refused for`() = runBlocking { + val localChatRoom = seedStatelessRoom() + + assertFailsWith { + repository.inviteMember( + localChatRoom = localChatRoom, + peerPublicKey = peer, + peerKeyPackage = keyPackage(), + ) + } + + val line = db.chatMessageDao().getChatMessagesByChatRoomId(roomId) + .map { it.chatMessage } + .singleOrNull { it.messageType == ChatMessage.TYPE_MEMBER_INVITE_FAILED } + + assertNotNull(line, "the refused invite left no line in the room") + assertTrue(line.content.contains("peer"), "the line does not name the invitee: ${line.content}") + } + + /** + * The caller still has to hear about it. The transcript line is the copy that is there + * tomorrow; the throw is what puts a message on the invite screen today, and swallowing it + * would pop the user back to the chat as though the invite had gone out. + */ + @Test + fun `a refused invite still reaches the caller`() = runBlocking { + val localChatRoom = seedStatelessRoom() + + assertFailsWith { + repository.inviteMember( + localChatRoom = localChatRoom, + peerPublicKey = peer, + peerKeyPackage = keyPackage(), + ) + } + } +} diff --git a/docs/marmot-membership.md b/docs/marmot-membership.md index c6feeb21..46a783b3 100644 --- a/docs/marmot-membership.md +++ b/docs/marmot-membership.md @@ -129,6 +129,37 @@ The group state is persisted after `commit()` and before any Welcome is delivere so a crash between the two leaves the group at the epoch the Welcomes describe rather than one behind it. +## What the room is told + +An invite writes three kinds of line into the room's transcript, none of which +travels — see `ChatMessage.MEMBERSHIP_TYPES`. They exist because the deferral +above is invisible from anywhere else: the screen that asks for an invite closes +the moment the commit is made, and everything that can still go wrong goes wrong +after that. + +| line | written by | says | +|-----------------------------|-----------------------------------------|---------------------------------------| +| `TYPE_MEMBER_INVITED` | `inviteMember`, `addMembersToChatRoom` | the membership change was made | +| `TYPE_MEMBER_INVITE_SENT` | `DatabaseNostrRepository` | the deferred Welcome went on the wire | +| `TYPE_MEMBER_INVITE_FAILED` | `deliveryWelcome`, `DatabaseChatRepository` | it did not, and why | + +The invite line is written **when the invite is made**, inside the caller's +transaction. Both halves of that matter: written any later and an invite waiting +on an ack that never comes leaves the room showing nothing, which is what this +looked like before; written outside the transaction and an invite that does not +survive `addMember` leaves the room claiming one was made. + +Only the immediate branch's Welcome goes out in the same breath as the invite, so +only the deferred branch owes a second line. Where the two are one event, one line +is the whole truth — and `createdAt` is stored to the second, so a second line +would be one the transcript could not reliably order after the first anyway. + +A refusal is the awkward case, because rolling the transaction back is right and +takes the account of it with it. `DatabaseChatRepository.inviteMember` catches, +writes the failed line from outside the transaction, and rethrows — the throw is +what puts a message on the invite screen now, the line is what is still there +tomorrow. + ## Other things that bite **Sequential invites each advance the epoch.** Where they still happen — the @@ -140,9 +171,12 @@ error. **A member with no published key package cannot be added.** A Marmot invite needs the invitee's `MarmotKeyPackage`. Both call sites look it up with a timeout and -collect the ones that failed. Today that only reaches the log — the -`TODO: Update status of participant Invitation.PENDING -> Invitation.SENT` at the -Welcome delivery site is the same gap seen from the other end. +collect the ones that failed. That is still only a log and a returned list — +nothing puts it in the room, because it happens before there is an invite to write +a line about. The `TODO: Update status of participant Invitation.PENDING -> +Invitation.SENT` at the Welcome delivery site is the same gap seen from the other +end: the transcript lines above say what happened, but a Participant row still +carries no state saying where its invite got to. **`deliveryWelcome` uses `Relays.DefaultDMRelayList`, not the room's relays.** There is a `TODO: Get localChatRoom relays...` on the ack-triggered call site.