diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt index 6429fe16..efe838a1 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MlsGroupCache.kt @@ -51,43 +51,14 @@ import kotlinx.coroutines.sync.withLock * before this existed, so the fallback is never worse than not caching. */ object MlsGroupCache { - private const val TAG = "MlsGroupCache" - - private val logger = Logger.withTag(TAG) - - private class Entry( - val group: MlsGroup, - /** The state hex this cache last wrote, for spotting another writer. */ - var stateHex: String, - ) - - private val entries = mutableMapOf() - - /** - * Serialises use of one room's group. - * - * The group is mutable and decryption advances it, so two events for the - * same room being decrypted at once would corrupt the ratchet. One lock per - * room rather than one overall, so a busy room cannot hold up a quiet one. - * - * Held across database work, which is safe here because a caller only ever - * takes this lock while it is already running -- it never waits on a - * resource the holder is waiting for. - */ - private val locks = mutableMapOf() - private val locksGuard = Mutex() - - private suspend fun lockFor(chatRoomId: String): Mutex = - locksGuard.withLock { locks.getOrPut(chatRoomId) { Mutex() } } + private val cache = LiveInstanceCache { it.saveState().encodeTls().toHex() } /** * Runs [block] against the room's live group, then stores whatever state it * left behind. * * [storedStateHex] is the room's state as the database currently has it, and - * [build] turns it into a group. [save] is handed the state to persist; it - * runs inside the lock so the stored state and the cached instance cannot - * disagree. + * [build] turns it into a group. [save] is handed the state to persist. * * Returns null without calling [block] when the room has no usable group * state, which is the same thing a failed `toMlsGroup()` meant before. @@ -98,24 +69,84 @@ object MlsGroupCache { build: () -> MlsGroup?, save: suspend (String) -> Unit, block: suspend (MlsGroup) -> T, - ): T? = lockFor(chatRoomId).withLock { - val cached = entries[chatRoomId] + ): T? = cache.withInstance( + key = chatRoomId, + storedState = storedStateHex, + build = build, + save = save, + block = block + ) +} - val group = if (cached != null && cached.stateHex == storedStateHex) { - cached.group +/** + * One live instance per key, reused only while the stored state is still the one + * this cache last wrote. + * + * Split out from [MlsGroupCache] so the decision it makes can be tested without + * standing up an MLS group. That decision is the whole safety argument: reuse + * when nothing else has written, rebuild when something has, and never carry on + * with an instance whose last use failed part-way through. + */ +internal class LiveInstanceCache( + /** The persisted form of an instance, for spotting another writer. */ + private val stateOf: (T) -> String, +) { + private val logger = Logger.withTag("LiveInstanceCache") + + private class Entry(val instance: T, val state: String) + + private val entries = mutableMapOf>() + + /** + * Serialises use of one key's instance. + * + * The instance is mutable and [block] advances it, so two callers running at + * once would corrupt it. One lock per key rather than one overall, so a busy + * key cannot hold up a quiet one. + * + * Held across [block], which may touch the database. Safe here because a + * caller only ever takes this lock while it is already running -- it never + * waits on a resource the holder is itself waiting for. + */ + private val locks = mutableMapOf() + private val locksGuard = Mutex() + + private suspend fun lockFor(key: String): Mutex = + locksGuard.withLock { locks.getOrPut(key) { Mutex() } } + + suspend fun withInstance( + key: String, + storedState: String?, + build: () -> T?, + save: suspend (String) -> Unit, + block: suspend (T) -> R, + ): R? = lockFor(key).withLock { + val cached = entries[key] + + val instance = if (cached != null && cached.state == storedState) { + cached.instance } else { if (cached != null) { - logger.d("Room $chatRoomId was written elsewhere; rebuilding its group") + logger.d("$key was written elsewhere; rebuilding") } + // Dropped before the block runs, so a build that fails does not leave + // the old instance behind to be picked up by the next caller. + entries.remove(key) build() ?: return@withLock null } - val result = block(group) + // Deliberately not in a finally: an instance whose use threw part-way is + // in an unknown state, and the next caller should rebuild from whatever + // was last persisted rather than carry on with it. + val result = block(instance) - val stateHex = group.saveState().encodeTls().toHex() - save(stateHex) - entries[chatRoomId] = Entry(group = group, stateHex = stateHex) + val state = stateOf(instance) + save(state) + entries[key] = Entry(instance = instance, state = state) result } + + /** How many instances are held. For tests. */ + internal fun size(): Int = entries.size } diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/RumorIdAgreementTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/RumorIdAgreementTest.kt new file mode 100644 index 00000000..ea0437ab --- /dev/null +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/database/model/RumorIdAgreementTest.kt @@ -0,0 +1,174 @@ +package press.mantra.compose.database.model + +import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.crypto.EventHasher +import com.vitorpamplona.quartz.nip01Core.signers.EventTemplate +import kotlin.test.Test +import kotlin.test.assertEquals +import press.mantra.compose.nostr.nip30303.ArtifactEvent +import press.mantra.compose.nostr.nip30303.ArtifactVersionEvent +import press.mantra.compose.nostr.nip30303.ChapterEvent +import press.mantra.compose.nostr.nip30303.ChunkEvent +import press.mantra.compose.nostr.nip30303.DialectEvent +import press.mantra.compose.nostr.nip30303.SubmissionEvent +import press.mantra.compose.nostr.nip30303.TranslationArtifactVersionEvent +import press.mantra.compose.nostr.nip30303.tags.ArtifactIdTag + +/** + * The row on disk and the payload on the wire have to be the same event. + * + * `MantraDao` writes an entity whose id comes from `MantraX.fromXEventTemplate`, + * and separately builds the rumor it submits with `rumorOf`, which hashes the + * template itself. Both are supposed to produce one id. Nothing checks that they + * do, and nothing would notice if they stopped: + * + * - the submission would carry a `payloadId` naming an event nobody has, + * - `MarmotInnerEvent.payloadEventId` would stop matching the row it carries, + * so `deleteByPayloadEventId` would silently un-queue nothing and superseded + * translations would go out anyway, + * - and every receiver would create a *second* row rather than converging on + * the sender's, because entity ids are content hashes and the two sides would + * be hashing different things. + * + * All of that is silent. The ids are opaque hex either way. + */ +class RumorIdAgreementTest { + private val author = "a".repeat(64) + private val chatRoomId = "room" + private val other = "b".repeat(64) + + /** Exactly what `MantraDao.rumorOf` does, and it must stay exactly that. */ + private fun rumorIdOf(template: EventTemplate<*>): String = EventHasher.hashId( + pubKey = author, + createdAt = template.createdAt, + kind = template.kind, + tags = template.tags, + content = template.content + ) + + @Test + fun `a dialect's row and its rumor agree`() { + val template = DialectEvent.build(name = "Sesotho", country = "Lesotho", language = "st") + + val entity = MantraDialect.fromDialectEventTemplate( + dialectEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `an artifact's row and its rumor agree`() { + val template = ArtifactEvent.build( + name = "In Detention", + url = "example.com", + visibility = "private", + license = "cc", + dialectId = other + ) + + val entity = MantraArtifact.fromArtifactEventTemplate( + artifactEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `an artifact version's row and its rumor agree`() { + val template = ArtifactVersionEvent.build(content = "1.0") { + addUnique(ArtifactIdTag.assemble(other)) + } + + val entity = MantraArtifactVersion.fromArtifactVersionEventTemplate( + artifactVersionEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `a chapter's and a chunk's rows agree with their rumors`() { + val chapter = ChapterEvent.build( + artifactVersionId = other, + name = "Chapter 1", + originalText = "some text", + index = 0, + wordCount = 2, + characterCount = 9 + ) + val chunk = ChunkEvent.build( + chapterId = other, + text = "some text", + index = 0, + wordCount = 2, + characterCount = 9 + ) + + assertEquals( + rumorIdOf(chapter), + MantraChapter.fromChapterEventTemplate(chapter, chatRoomId, author)?.id + ) + assertEquals( + rumorIdOf(chunk), + MantraChunk.fromChunkEventTemplate(chunk, chatRoomId, author)?.id + ) + } + + @Test + fun `a translation version's row and its rumor agree`() { + val template = TranslationArtifactVersionEvent.build( + artifactVersionId = other, + dialectId = other, + name = "Sesotho", + visibility = "private", + license = "cc" + ) + + val entity = MantraTranslationArtifactVersion.fromTranslationArtifactVersionEventTemplate( + translationArtifactVersionEventTemplate = template, + chatRoomId = chatRoomId, + userPublicKey = author + ) + + assertEquals(rumorIdOf(template), entity?.id) + } + + @Test + fun `the submission names the id the row was written under`() { + // The end of the chain the rest of this file checks a link of: what a + // receiver reads out of the envelope has to be the id the sender stored. + val template = DialectEvent.build(name = "Sesotho", country = "Lesotho", language = "st") + val entity = MantraDialect.fromDialectEventTemplate(template, chatRoomId, author) + + val payload = Event( + id = rumorIdOf(template), + pubKey = author, + createdAt = template.createdAt, + kind = template.kind, + tags = template.tags, + content = template.content, + sig = "" + ) + val submission = SubmissionEvent.build(payload = payload) + + val readBack = SubmissionEvent( + id = "f".repeat(64), + pubKey = author, + createdAt = submission.createdAt, + tags = submission.tags, + content = submission.content, + sig = "" + ) + + assertEquals(entity?.id, readBack.payloadId()) + assertEquals(entity?.id, readBack.payload()?.id) + assertEquals(DialectEvent.KIND, readBack.payloadKind()) + } +} diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/managers/LiveInstanceCacheTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/managers/LiveInstanceCacheTest.kt new file mode 100644 index 00000000..c2931f4d --- /dev/null +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/managers/LiveInstanceCacheTest.kt @@ -0,0 +1,201 @@ +package press.mantra.compose.managers + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlin.test.assertSame +import kotlinx.coroutines.runBlocking + +/** + * The decision behind keeping an MLS group alive between messages. + * + * This cache exists because quartz drops a secret tree's skipped-generation keys + * on save, so rebuilding a group between two messages loses any message that + * arrives late -- permanently, and silently. See `docs/mls-skipped-keys.md`. + * + * Every one of these failures is invisible at runtime. Reuse too eagerly and a + * group carries on from a ratchet another writer has already moved, which + * corrupts decryption rather than failing it. Reuse too rarely and the cache + * does nothing at all, and the bug it was written for comes straight back with + * no symptom to notice. So the rule is asserted rather than reasoned about. + */ +class LiveInstanceCacheTest { + /** Stands in for an MlsGroup: mutable, and its persisted form is its content. */ + private class Group(var state: String) { + /** How many times this particular instance was handed to a caller. */ + var uses: Int = 0 + } + + private fun cache() = LiveInstanceCache { it.state } + + @Test + fun `reuses the instance while nothing else has written`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + val first = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it.uses++; it } + ) + + val second = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it.uses++; it } + ) + + // The same object, not merely an equal one: what has to survive is the + // in-memory skipped-key map, which no amount of rebuilding recovers. + assertSame(first, second) + assertEquals(1, built) + assertEquals(2, second?.uses) + } + + @Test + fun `rebuilds when something else wrote the stored state`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + val first = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it } + ) + + // Sending a message advances the sender ratchet and saves; adding a + // member does too. Carrying on from an instance that has been overtaken + // would diverge the ratchet, which is worse than not caching at all. + stored = "written by someone else" + + val second = cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("written by someone else") }, + save = { stored = it }, + block = { it } + ) + + assertEquals(2, built) + assertEquals(false, first === second) + } + + @Test + fun `persists whatever the block left behind`() = runBlocking { + val cache = cache() + var stored: String? = "start" + + cache.withInstance( + key = "room", + storedState = stored, + build = { Group("start") }, + save = { stored = it }, + block = { it.state = "advanced" } + ) + + assertEquals("advanced", stored) + } + + @Test + fun `the state it records is the one it compares against next time`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + repeat(3) { + cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + // Every use moves the instance on, as decrypting a message does. + block = { group -> group.state = "advanced ${group.uses++}" } + ) + } + + // Recording the pre-block state instead would make every call look like + // somebody else had written, quietly turning the cache off. + assertEquals(1, built) + } + + @Test + fun `does not run the block, or cache anything, when there is nothing to build`() = runBlocking { + val cache = cache() + var ran = false + + val result = cache.withInstance( + key = "room", + storedState = null, + build = { null }, + save = { }, + block = { ran = true } + ) + + assertNull(result) + assertEquals(false, ran) + assertEquals(0, cache.size()) + } + + @Test + fun `an instance whose use threw is not handed to the next caller`() = runBlocking { + val cache = cache() + var stored: String? = "start" + var built = 0 + + assertFailsWith { + cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { error("decryption blew up half way") } + ) + } + + cache.withInstance( + key = "room", + storedState = stored, + build = { built++; Group("start") }, + save = { stored = it }, + block = { it } + ) + + // Half-advanced and never persisted: the next caller has to start from + // what is actually on disk, not from whatever the failure left in memory. + assertEquals(2, built) + } + + @Test + fun `rooms are cached independently`() = runBlocking { + val cache = cache() + var storedA: String? = "a" + var storedB: String? = "b" + + val a = cache.withInstance( + key = "roomA", + storedState = storedA, + build = { Group("a") }, + save = { storedA = it }, + block = { it } + ) + val b = cache.withInstance( + key = "roomB", + storedState = storedB, + build = { Group("b") }, + save = { storedB = it }, + block = { it } + ) + + assertEquals(2, cache.size()) + assertEquals(false, a === b) + } +}