fix(projects): let a membership's foreign key violation out, so the sweep retries
A project's memberships are foreign keys into both the project and the version they name, so a chronicle page carrying a membership whose project is in a *later* page cannot apply it yet. `ChronicleManager.applyPage` handles exactly this: it catches the violation, counts the payload `failed`, and `sweep` re-runs every page until failures stop falling. Phase 3 broke that for projects by catching the violation inside the apply arm. `applyInnerEvent` then returned normally, `applyPage` counted the payload applied, the sweep saw nothing left to do, and the membership was dropped -- silently, permanently, on the device a chronicle exists for. The catch was added so that `FrostSigningManager.applySignedEvent` would not report a succeeded session as failed. That reasoning was already satisfied: `applySignedEvent` wraps the call in its own try/catch. And no sibling arm catches -- a translation chapter naming an absent chapter throws today -- so the catch made projects the only kind a chronicle could not retry. It is gone. Pages are 128 events or 64 KiB, and projects sort last in APPLY_ORDER, so a boundary falling between a project and its memberships is the ordinary case for any room with a chapter's worth of chunks in it. `pages arriving out of order still converge` already modelled this -- one page per payload, delivered backwards, comparing every row count on both devices -- and passed anyway, because `rowCounts` did not look at the project tables. It does now, which is what makes that test the one that would have caught this. `ProjectApplyJvmTest` asserts the throw rather than the swallow, and adds the retry in miniature: refused while the project is missing, accepted once it is there, same event both times. 1404 tests, no failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
aceef313ed
commit
2fb8d6921b
4 files changed
+109
-51
No files matched your search
+27
-36
@@ -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
|
||||
}
|
||||
|
||||
+44
-8
@@ -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()
|
||||
|
||||
+24
-2
@@ -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"
|
||||
|
||||
+14
-5
@@ -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
|
||||
|
||||
Reference in new issue
Block a user