test: pin the two invariants this session left unguarded
Both are silent when broken, which is why they are worth asserting rather than reasoning about. **The cache's reuse decision.** MlsGroupCache exists because quartz drops a secret tree's skipped-generation keys on save, so rebuilding a group between two messages loses any that arrives late. Its safety argument is one comparison: reuse while the stored state is still what the cache last wrote, rebuild when it is not. Get that wrong in either direction and nothing complains -- reuse too eagerly and a group carries on from a ratchet another writer already moved, which corrupts decryption rather than failing it; reuse too rarely and the cache does nothing and the original bug is back with no symptom. That decision is now a generic LiveInstanceCache with MlsGroupCache as a typed facade over it, so it can be tested without standing up an MLS group. Splitting it also made two behaviours explicit that were previously incidental: a failed build no longer leaves the old instance behind, and an instance whose use threw is deliberately not cached -- it is half-advanced and never persisted, so the next caller has to start from disk. **Rumor and row ids agreeing.** MantraDao writes an entity whose id comes from fromXEventTemplate and separately builds the rumor it submits with rumorOf, which hashes the template itself. Both are meant to produce one id and nothing checked it. Diverging would mean submissions naming an event nobody has, deleteByPayloadEventId silently un-queuing nothing so superseded translations go out anyway, and every receiver creating a second row instead of converging on the sender's -- all of it invisible, since the ids are opaque hex either way. Asserted per kind, plus the whole chain out through the submission envelope. Both suites were mutation-checked rather than trusted: inverting the staleness comparison fails one cache test, recording the pre-block state fails another, and hashing the rumor under a different author fails all six id tests. Still uncovered, and not cheaply fixable: FrostSigningManager's and MantraDao's state machines both need a Room harness, and commonTest has none. The FROST crypto path is covered by FrostSigningRoundTest; the message-driven parts around it are not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
@@ -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<String, Entry>()
|
||||
|
||||
/**
|
||||
* 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<String, Mutex>()
|
||||
private val locksGuard = Mutex()
|
||||
|
||||
private suspend fun lockFor(chatRoomId: String): Mutex =
|
||||
locksGuard.withLock { locks.getOrPut(chatRoomId) { Mutex() } }
|
||||
private val cache = LiveInstanceCache<MlsGroup> { 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<T : Any>(
|
||||
/** The persisted form of an instance, for spotting another writer. */
|
||||
private val stateOf: (T) -> String,
|
||||
) {
|
||||
private val logger = Logger.withTag("LiveInstanceCache")
|
||||
|
||||
private class Entry<T>(val instance: T, val state: String)
|
||||
|
||||
private val entries = mutableMapOf<String, Entry<T>>()
|
||||
|
||||
/**
|
||||
* 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<String, Mutex>()
|
||||
private val locksGuard = Mutex()
|
||||
|
||||
private suspend fun lockFor(key: String): Mutex =
|
||||
locksGuard.withLock { locks.getOrPut(key) { Mutex() } }
|
||||
|
||||
suspend fun <R> 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
|
||||
}
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
}
|
||||
@@ -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<Group> { 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<Unit>(
|
||||
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<IllegalStateException> {
|
||||
cache.withInstance<Unit>(
|
||||
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)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user