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 944536ce..3552e6ca 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 @@ -1330,13 +1330,23 @@ data class ChatMessage( // writes "The group signed ..." when the session completes, and // three more lines here would be a second account of it. // - // The row may be refused: both references are foreign keys, so a - // membership naming a version this device does not hold cannot be - // written. That is the one-room scope decision doing its job - // rather than an error to propagate -- `applySignedEvent` - // swallows throwables because the signature is valid either way, - // and letting this one reach it would report a session that - // succeeded as a session that failed. + // **The foreign key violation is allowed out, and has to be.** + // Both references are keys, so a membership naming a version or + // a project this device does not hold cannot be written -- and + // `ChronicleManager.applyPage` learns that only by catching it. + // It counts the payload failed, and `sweep` runs the pages again + // until failures stop falling, which is the whole mechanism that + // delivers a membership whose project landed in a *later* page. + // Swallowing it here counts the payload as applied, the sweep + // terminates, and the membership is lost silently on the one + // device that has nothing else to fall back on. + // + // It was caught here for one commit, on the reasoning that + // `applySignedEvent` must not report a succeeded session as + // failed. That reasoning was already satisfied: it has its own + // try/catch. And no sibling arm catches -- a translation chapter + // naming an absent chapter throws today -- so catching made this + // the only kind the chronicle could not retry. ProjectArtifactVersionEvent.KIND -> { MantraProjectArtifactVersion.fromProjectArtifactVersionEvent( projectArtifactVersionEvent = ProjectArtifactVersionEvent( @@ -1349,21 +1359,12 @@ data class ChatMessage( ), chatRoomId = groupId, )?.let { membership -> - try { - database.mantraProjectArtifactVersionDao().upsert( - membership.copy( - marmotGroupEventId = marmotGroupEventId, - groupSignedEventId = groupSignedEventId, - ) + database.mantraProjectArtifactVersionDao().upsert( + membership.copy( + marmotGroupEventId = marmotGroupEventId, + groupSignedEventId = groupSignedEventId, ) - } catch (error: Throwable) { - logger.w( - "Project ${membership.projectId.take(8)} names artifact " + - "version ${membership.artifactVersionId.take(8)}, which " + - "this device does not hold", - error - ) - } + ) } null } @@ -1383,22 +1384,12 @@ data class ChatMessage( ), chatRoomId = groupId, )?.let { membership -> - try { - database.mantraProjectTranslationArtifactVersionDao().upsert( - membership.copy( - marmotGroupEventId = marmotGroupEventId, - groupSignedEventId = groupSignedEventId, - ) + database.mantraProjectTranslationArtifactVersionDao().upsert( + membership.copy( + marmotGroupEventId = marmotGroupEventId, + groupSignedEventId = groupSignedEventId, ) - } catch (error: Throwable) { - logger.w( - "Project ${membership.projectId.take(8)} names " + - "translation " + - membership.translationArtifactVersionId.take(8) + - ", which this device does not hold", - error - ) - } + ) } null } diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/model/ProjectApplyJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/model/ProjectApplyJvmTest.kt index 1d90b48a..fbdf9af4 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/model/ProjectApplyJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/database/model/ProjectApplyJvmTest.kt @@ -19,6 +19,7 @@ import press.mantra.compose.nostr.nip30303.tags.ProjectIdTag import kotlin.test.AfterTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFails import kotlin.test.assertNull import kotlin.test.assertTrue import kotlin.test.fail @@ -295,8 +296,25 @@ class ProjectApplyJvmTest { ) } + /** + * The refusal is **allowed out**, and that is the whole of why it matters. + * + * `ChronicleManager.applyPage` learns that a payload could not be applied + * only by catching the exception: it counts the payload failed, and `sweep` + * runs the pages again until failures stop falling. That loop is what + * delivers a membership whose project arrived in a *later* page. Swallow the + * violation here and the payload counts as applied, the sweep terminates, + * and the membership is lost silently -- on the one device that has nothing + * to fall back on. + * + * It was swallowed here for one commit, on the reasoning that + * `FrostSigningManager.applySignedEvent` must not report a succeeded session + * as failed. That reasoning was already satisfied -- it has its own + * try/catch -- and no sibling arm catches, so catching made this the only + * kind a chronicle could not retry. + */ @Test - fun `a membership naming a version this device lacks leaves the project standing`() = + fun `a membership naming a version this device lacks throws, and spares the project`() = runBlocking { seed() val project = signed(projectTemplate()) @@ -307,11 +325,7 @@ class ProjectApplyJvmTest { ProjectArtifactVersionEvent.membershipsOf(project, listOf(absent)).single() ) - // The foreign key refuses it. What must not happen is the refusal - // escaping: `applySignedEvent` would report a session that succeeded - // as one that failed, and the group would be told to abandon a - // ceremony whose signature is perfectly good. - assertNull(apply(source)) + assertFails { apply(source) } assertTrue( db.mantraProjectArtifactVersionDao().getByProjectId(project.id).isEmpty() @@ -323,7 +337,7 @@ class ProjectApplyJvmTest { } @Test - fun `a membership naming a project this device lacks is refused the same way`() = + fun `a membership naming a project this device lacks throws the same way`() = runBlocking { seed() val absentProject = "8".repeat(64) @@ -334,12 +348,34 @@ class ProjectApplyJvmTest { createdAt = 1_000, ) - assertNull(apply(signed(template))) + assertFails { apply(signed(template)) } assertTrue( db.mantraProjectArtifactVersionDao().getByProjectId(absentProject).isEmpty() ) } + @Test + fun `a membership that arrives after its project applies cleanly`() = runBlocking { + seed() + val project = signed(projectTemplate()) + val source = signed( + ProjectArtifactVersionEvent.membershipsOf(project, listOf(artifactVersionId)).single() + ) + + // The retry the sweep performs, in miniature: refused while the project + // is missing, accepted once it is there. Same event both times. + assertFails { apply(source) } + apply(project) + apply(source) + + assertEquals( + listOf("1.0"), + db.mantraProjectArtifactVersionDao() + .getArtifactVersionsByProjectId(project.id) + .map { it.versionLabel } + ) + } + @Test fun `a membership missing the project it belongs to writes nothing`() = runBlocking { seed() diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/ChronicleApplyJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/ChronicleApplyJvmTest.kt index cd0eb5de..8036a603 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/ChronicleApplyJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/ChronicleApplyJvmTest.kt @@ -414,6 +414,23 @@ class ChronicleApplyJvmTest { db.mantraTranslationChunkDao().getTranslationChunksByTranslationChapterId(it.id) } + // The projects, and what each one holds. + // + // Counted here rather than in a test of their own, because the test that + // matters most for them is `pages arriving out of order still converge` + // -- and that test compares exactly this map. A membership is a foreign + // key into both a project and a version, so delivering backwards is the + // scenario where one gets refused and has to be retried, and this map not + // looking at the project tables is why that test once passed against an + // apply arm that swallowed the refusal and lost the row. + val projects = db.mantraProjectDao().getProjectsByChatRoomId(chatRoomId) + val projectSources = projects.flatMap { + db.mantraProjectArtifactVersionDao().getByProjectId(it.id) + } + val projectTranslations = projects.flatMap { + db.mantraProjectTranslationArtifactVersionDao().getByProjectId(it.id) + } + return mapOf( "dialects" to dialects.size, "artifacts" to artifacts.size, @@ -423,6 +440,9 @@ class ChronicleApplyJvmTest { "translationVersions" to translationVersions.size, "translationChapters" to translationChapters.size, "translationChunks" to translationChunks.size, + "projects" to projects.size, + "projectSources" to projectSources.size, + "projectTranslations" to projectTranslations.size, ) } @@ -473,7 +493,8 @@ class ChronicleApplyJvmTest { mapOf( "dialects" to 0, "artifacts" to 0, "versions" to 0, "chapters" to 0, "chunks" to 0, "translationVersions" to 0, "translationChapters" to 0, - "translationChunks" to 0, + "translationChunks" to 0, "projects" to 0, "projectSources" to 0, + "projectTranslations" to 0, ), rowCounts(receiver), "the new member starts with nothing, which is the whole problem" @@ -731,7 +752,8 @@ class ChronicleApplyJvmTest { mapOf( "dialects" to 0, "artifacts" to 0, "versions" to 0, "chapters" to 0, "chunks" to 0, "translationVersions" to 0, "translationChapters" to 0, - "translationChunks" to 0, + "translationChunks" to 0, "projects" to 0, "projectSources" to 0, + "projectTranslations" to 0, ), rowCounts(receiver), "a bystander already holds the work; re-applying would only rewrite its provenance" diff --git a/docs/project-feature.md b/docs/project-feature.md index 45b5807a..5e73a85e 100644 --- a/docs/project-feature.md +++ b/docs/project-feature.md @@ -14,6 +14,7 @@ it belongs to: | the `summarize` resolver is Phase 4 | the seam is Phase 4, the resolution is Phase 7c | two of its four call sites are composables and cannot await a database, so the names are pre-resolved in `FrostSigningViewModel` | | three chronicle tests to add | three to add and four to fix | the existing suite compares a seeded room against `CHRONICLABLE_KINDS` itself, and one test asserts every chroniclable kind can also be *rebuilt* -- which projects deliberately cannot | | around twenty new strings | eighteen, three of them already there | `projects`, `no_projects_exists_in_this_group` and `create_project` came with the stub | +| the apply arms catch the foreign key violation | they let it out | `ChronicleManager.sweep` retries a page only on a *counted* failure, so catching it loses a membership that crosses a page boundary — and `applySignedEvent`, the reason for catching, already has its own try/catch | The one thing found while building that the plan did not anticipate: a project reaches a chronicle only off the `GroupSignedEvent` record, because there is no @@ -725,11 +726,19 @@ already writes "The group signed …" when a session completes, which is the lin that covers it. Three new strings for a transcript the signing session is already narrating would be three places for the same event to be described differently. -Both membership arms must tolerate the version not being here. The FK will refuse -the insert; catch it, log it, and return null rather than letting it escape — -`applySignedEvent` swallows throwables for a reason (the signature is valid -whether or not this device could write the row down) but a refusal that reaches it -is reported as a failed apply on a session that succeeded. +**Both membership arms let the foreign key violation out, and must.** The +temptation is to catch it — `applySignedEvent` swallows throwables so a succeeded +session is not reported as failed — and that is what was built first. It was wrong +twice over. `applySignedEvent` already has its own try/catch, so the reasoning was +satisfied without doing anything; and `ChronicleManager.applyPage` learns a +payload could not be applied **only** by catching the exception, counts it +`failed`, and `sweep` re-runs the pages until failures stop falling. That loop is +the entire mechanism for a membership whose project arrived in a later page. +Swallowing it counts the payload as applied, the sweep terminates, and the row is +lost silently on the one device with nothing to fall back on. + +No sibling arm catches either — a translation chapter naming an absent chapter +throws today — so catching made this the only kind a chronicle could not retry. **Tests** (`jvmTest`): a `ProjectApplyJvmTest` in the shape of `TranslationChunkApplyJvmTest` — apply a project with three owners and assert