fix(store): classify insert failures against the database, not the driver's message

`classifyRowError` read SQLite's exception text to decide whether a row that failed to insert was a duplicate, a superseded replaceable, or a genuine write failure. That text is the driver's business, not a contract: the bundled JVM driver spells out `UNIQUE constraint failed: event_headers.id`, while Android's wraps the same failure in an `android.database.SQLException` whose message is **null**. So on the Android host every duplicate fell through to `Failed`, which the relay renders as `OK false "error: SQLException"` — an answer clients retry forever, and a direct violation of STORE-W01/W02 (`OK true` with the `duplicate:` prefix). Four commonTest suites caught it and failed on that target only: NostrServerTest x3 and LiveNegentropyIndexStoreTest. The classifier now asks the connection instead. It already runs after the savepoint rollback, so the connection shows pre-insert state and the two questions have exact answers: is this id already stored, and does a stored version already beat this one at its replaceable/addressable coordinate? The latter is the exact complement of `displacedBy`'s predicate, so a disk error while inserting a *winning* version still reports `Failed` rather than turning into a silent `OK true`. Trigger RAISEs (`blocked:`, `not allowed`) keep being decided by text — they leave no database-visible trace. One behaviour change, noted in the skill's changelog: re-offering a stored replaceable/addressable event byte-for-byte now reports DUPLICATE where the JVM driver previously reported SUPERSEDED. It violates both indexes and which one SQLite names first is up to the driver; the id answer is the one that holds everywhere (and is the truer sentence). Both carry the `duplicate:` prefix, so the wire answer is unchanged. `InsertOutcomeClassificationTest` pins all of this at the store level on both targets, including the guard that a winning version at an occupied coordinate is still Accepted. Also corrects a false claim in the previous commit's message: moving the cordn and contextvm tests to `jvmAndroidTest` did NOT make them run on the Android host. `androidHostTest` has no `dependsOn(jvmAndroidTest)` — only `jvmTest` does — so those suites run on the JVM target alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012BfD4txdnsaPRXmNXbup9n
2026-10-05 19:28:25 +00:00 · 2026-09-18 21:36:33 +00:00
parent 09d356edfb
commit 65c683c9c7
3 changed files with 266 additions and 13 deletions
@@ -37,7 +37,7 @@ Executable spec: the test suites in
 `quartz/src/commonTest/.../store/sqlite/` (`BasicTest`, `ReplaceableTest`, `AddressableTest`,
 `DeletionTest`, `ExpirationTest`, `RightToVanishTest`, `SearchTest`, `SearchRelevanceOrderTest`,
 `MergeQueryCorrectnessTest`, `TagMergeCorrectnessTest`, `QueryAssemblerTest`,
-`SnapshotIdsForNegentropyTest`, `FilterMatcherTest`, …). If a rule here ever contradicts a test,
+`SnapshotIdsForNegentropyTest`, `FilterMatcherTest`, `InsertOutcomeClassificationTest`, …). If a rule here ever contradicts a test,
 the test wins — and this file has a bug to fix.

 ## Kind classes (used throughout)
@@ -189,6 +189,21 @@ back alone and reports `Rejected(reason)`; the rest commit. If the **outer commi
 entry is treated as `Rejected` (the `IEventStore.batchInsert` contract). Outcomes are returned
 in input order; OK frames pair by event id, not order.

+**STORE-W09 — a failed row is classified against the database, not against the driver's
+exception text.** `SQLiteEventStore.classifyRowError` rolls the row's savepoint back and then
+asks the connection (which now shows pre-insert state): id already present → `DUPLICATE`;
+a stored version that beats this one at the replaceable/addressable coordinate (the exact
+complement of the supersession predicate in W01/W02) → `SUPERSEDED`; otherwise `Failed`.
+Message text is only a fast path and a fallback for trigger RAISEs (`blocked:`, `not allowed`),
+which leave no database-visible trace. This matters because the message is driver-specific —
+the bundled JVM driver writes `UNIQUE constraint failed: event_headers.id`, Android's throws an
+`android.database.SQLException` with a **null** message — so a text-only classifier answered
+`OK false` on Android for events the store already held. Corollary: re-offering a stored
+replaceable/addressable event **byte-for-byte** is `DUPLICATE`, not `SUPERSEDED` (it violates
+both indexes and only the id answer is driver-independent); a stale *different* version is
+still `SUPERSEDED`. Both carry the `duplicate:` prefix, so the relay reply is `OK true` either
+way.
+
 ---

 ## Deletion lifecycle — NIP-09 / NIP-62 (STORE-D)
@@ -323,6 +338,11 @@ non-itemizable cases).

 Add one line per behavior change, newest first: `YYYY-MM-DD <short sha> <rule id> — what changed`.

+- 2026-09-18 (pending) W09, W01/W02 — insert-failure classification now queries the database
+  instead of parsing the driver's exception message (Android's is null, so duplicates were
+  reported as `Failed`/`OK false`). A byte-for-byte re-offer of a stored replaceable/addressable
+  event now reports `DUPLICATE` where the JVM driver previously reported `SUPERSEDED`; both are
+  `duplicate:` → `OK true`, so the wire answer is unchanged.
 - 2026-08-04 (baseline) — rules F01–F13, W01–W08, D01–D08, C01, S01–S06, N01 written from the
  code at the time this skill was introduced. Changes before this date are not itemized;
  archaeology starts at `git log` on `nip01Core/store/`.
@@ -519,7 +519,7 @@ class SQLiteEventStore(
            // ROLLBACK shouldn't mask the original cause.
            runCatching { db.execSQL("ROLLBACK TRANSACTION TO SAVEPOINT $sp") }
            runCatching { db.execSQL("RELEASE SAVEPOINT $sp") }
-            classifyRowError(e)
+            classifyRowError(e, event, db)
        }
    }

@@ -531,8 +531,25 @@ class SQLiteEventStore(
     * I/O error, schema drift) is the store failing to write an acceptable
     * event: `Failed`, so a rising count is loud instead of blending into
     * the duplicate tally.
+     *
+     * Message text is the fast path, not the contract: which exception a
+     * driver throws and what it puts in `getMessage()` is the driver's
+     * business. Android's `SQLiteConnection` wraps constraint failures in
+     * an `android.database.SQLException` carrying a **null** message,
+     * where the bundled JVM driver spells out
+     * `UNIQUE constraint failed: …`. Classifying off text alone therefore
+     * turned every duplicate into a `Failed` on Android — an `OK false`
+     * the client retries forever. So [db] is asked instead: this runs after
+     * the savepoint rollback, so the connection shows the pre-insert state
+     * and the two questions that separate a duplicate from a genuine write
+     * failure ("is this id already here?", "does a stored version already
+     * beat this one?") have exact answers, on every driver.
     */
-    private fun classifyRowError(e: Throwable): IEventStore.InsertOutcome {
+    private fun classifyRowError(
+        e: Throwable,
+        event: Event,
+        db: SQLiteConnection,
+    ): IEventStore.InsertOutcome {
        val message = e.message ?: e::class.simpleName ?: RejectionReason.INSERT_FAILED
        // A second copy of an event the store already holds trips the unique index on
        // event_headers.id. That is not a refusal of the event but a statement that it
@@ -542,25 +559,108 @@ class SQLiteEventStore(
        if (message.contains(DUPLICATE_ID_CONSTRAINT)) {
            return IEventStore.InsertOutcome.Rejected(RejectionReason.DUPLICATE)
        }
-        // The replaceable / addressable unique indexes fire only when the supersession
-        // trigger found nothing older to delete, i.e. the stored version already wins
-        // (STORE-W01/W02). Same shape as a duplicate: nothing to write, `OK true`.
-        if (message.contains(SUPERSEDED_CONSTRAINT)) {
-            return IEventStore.InsertOutcome.Rejected(RejectionReason.SUPERSEDED)
-        }
-        val refusal =
+        // Trigger RAISEs and the immutability guards name themselves, and leave no
+        // database-visible trace to ask about, so they are decided by text alone.
+        val namedRefusal =
            message.contains("blocked:") ||
                message.contains("duplicate:") ||
                message.contains(RejectionReason.PREFIX_REPLACED) ||
-                message.contains("not allowed") ||
-                message.contains("constraint", ignoreCase = true)
-        return if (refusal) {
+                message.contains("not allowed")
+        if (namedRefusal) return IEventStore.InsertOutcome.Rejected(message)
+
+        // Ask the database the two questions the unique indexes answer, in the
+        // order that makes the answer driver-independent. The id question goes
+        // first because *which* index a re-offered replaceable event trips is up
+        // to SQLite: re-inserting a stored replaceable byte-for-byte violates
+        // both `event_headers.id` and `replaceable_idx`, and only the id lookup
+        // says the same thing on every driver ("already have this event" — which
+        // is also the truer sentence). It costs one point lookup on an already
+        // open connection, on the rejected path only.
+        if (isAlreadyStored(event.id, db)) {
+            return IEventStore.InsertOutcome.Rejected(RejectionReason.DUPLICATE)
+        }
+        // The replaceable / addressable unique indexes fire only when the supersession
+        // trigger found nothing older to delete, i.e. the stored version already wins
+        // (STORE-W01/W02). Same shape as a duplicate: nothing to write, `OK true`.
+        if (message.contains(SUPERSEDED_CONSTRAINT) || isSupersededByStored(event, db)) {
+            return IEventStore.InsertOutcome.Rejected(RejectionReason.SUPERSEDED)
+        }
+
+        return if (message.contains("constraint", ignoreCase = true)) {
            IEventStore.InsertOutcome.Rejected(message)
        } else {
            IEventStore.InsertOutcome.Failed(message)
        }
    }

+    /**
+     * Whether [id] is already in `event_headers` — the unique index on
+     * `event_headers.id` restated as a question, for drivers that don't say
+     * which index they tripped. Any failure answers "no": the point is to
+     * recognize a duplicate, and a connection too broken to answer is a
+     * write failure, which is what the caller falls through to.
+     */
+    private fun isAlreadyStored(
+        id: String,
+        db: SQLiteConnection,
+    ): Boolean =
+        runCatching {
+            db.prepare("SELECT 1 FROM event_headers WHERE id = ? LIMIT 1").use { stmt ->
+                stmt.bindText(1, id)
+                stmt.step()
+            }
+        }.getOrDefault(false)
+
+    /**
+     * Whether a stored version already beats [event] at its replaceable /
+     * addressable coordinate (STORE-W01/W02) — the exact complement of
+     * [displacedBy]'s predicate, so a stored row that the supersession
+     * trigger *would* have deleted doesn't count. That precision matters
+     * here: this is the fallback for unrecognized failures, and a disk
+     * error while inserting a winning replaceable event must stay `Failed`
+     * rather than turn into a silent `OK true`. An equal id is the
+     * duplicate case and is answered before this one.
+     */
+    private fun isSupersededByStored(
+        event: Event,
+        db: SQLiteConnection,
+    ): Boolean {
+        val addressable = event.kind.isAddressable() && event is AddressableEvent
+        val sql =
+            when {
+                event.kind.isReplaceable() ->
+                    """
+                    SELECT 1 FROM event_headers
+                    WHERE kind = ? AND pubkey = ?
+                      AND (created_at > ? OR (created_at = ? AND id < ?))
+                    LIMIT 1
+                    """.trimIndent()
+
+                addressable ->
+                    """
+                    SELECT 1 FROM event_headers
+                    WHERE kind = ? AND pubkey = ? AND d_tag = ?
+                      AND kind >= 30000 AND kind < 40000
+                      AND (created_at > ? OR (created_at = ? AND id < ?))
+                    LIMIT 1
+                    """.trimIndent()
+
+                else -> return false
+            }
+        return runCatching {
+            db.prepare(sql).use { stmt ->
+                var i = 1
+                stmt.bindLong(i++, event.kind.toLong())
+                stmt.bindText(i++, event.pubKey)
+                if (addressable) stmt.bindText(i++, (event as AddressableEvent).dTag())
+                stmt.bindLong(i++, event.createdAt)
+                stmt.bindLong(i++, event.createdAt)
+                stmt.bindText(i, event.id)
+                stmt.step()
+            }
+        }.getOrDefault(false)
+    }
+
    inner class Transaction internal constructor(
        val db: SQLiteConnection,
        private val delta: LiveIndexDelta?,
@@ -0,0 +1,133 @@
+/*
+ * Copyright (c) 2025 Vitor Pamplona
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
+ * Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+package com.vitorpamplona.quartz.nip01Core.store.sqlite
+
+import com.vitorpamplona.quartz.nip01Core.core.Event
+import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent
+import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerSync
+import com.vitorpamplona.quartz.nip01Core.store.IEventStore
+import com.vitorpamplona.quartz.nip01Core.store.RejectionReason
+import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent
+import com.vitorpamplona.quartz.nip23LongContent.LongTextNoteEvent
+import com.vitorpamplona.quartz.utils.TimeUtils
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+/**
+ * `batchInsert` must name *why* a row didn't land, because the relay turns
+ * that reason into the NIP-01 answer: [RejectionReason.DUPLICATE] and
+ * [RejectionReason.SUPERSEDED] are `OK true` ("already covered"), everything
+ * else is `OK false`, which clients retry.
+ *
+ * This suite pins the classification **independently of the SQLite driver's
+ * exception text**. The bundled JVM driver raises
+ * `UNIQUE constraint failed: event_headers.id`; Android's raises an
+ * `android.database.SQLException` whose message is `null`. Reading the text
+ * was therefore enough on one target and wrong on the other — a duplicate
+ * came back as `error: SQLException`, an `OK false` the client re-offers
+ * forever. Every case below runs on both targets and so fails on either if
+ * the classifier ever goes back to trusting a message.
+ */
+class InsertOutcomeClassificationTest : BaseDBTest() {
+    val signer = NostrSignerSync()
+
+    private fun assertRejected(
+        expectedReason: String,
+        outcome: IEventStore.InsertOutcome,
+    ) {
+        assertTrue(outcome is IEventStore.InsertOutcome.Rejected, "expected Rejected, got $outcome")
+        assertEquals(expectedReason, outcome.reason)
+    }
+
+    @Test
+    fun duplicateIdIsRejectedAsDuplicate() =
+        forEachDB { db ->
+            val event = signer.sign(TextNoteEvent.build("hello", createdAt = TimeUtils.now()))
+
+            assertEquals(IEventStore.InsertOutcome.Accepted, db.batchInsert(listOf<Event>(event))[0])
+            assertRejected(RejectionReason.DUPLICATE, db.batchInsert(listOf<Event>(event))[0])
+        }
+
+    @Test
+    fun reofferingAStoredReplaceableIsRejectedAsDuplicate() =
+        forEachDB { db ->
+            // Byte-for-byte the stored version, so it violates the id index *and*
+            // replaceable_idx — and which one SQLite reports first is the driver's
+            // choice. "Already have this event" is the answer that holds on all of
+            // them.
+            val event = signer.sign(MetadataEvent.createNew("Vitor", createdAt = TimeUtils.now()))
+
+            assertEquals(IEventStore.InsertOutcome.Accepted, db.batchInsert(listOf<Event>(event))[0])
+            assertRejected(RejectionReason.DUPLICATE, db.batchInsert(listOf<Event>(event))[0])
+        }
+
+    @Test
+    fun olderReplaceableIsRejectedAsSuperseded() =
+        forEachDB { db ->
+            val time = TimeUtils.now()
+            val older = signer.sign(MetadataEvent.createNew("Vitor 1", createdAt = time))
+            val newer = signer.sign(MetadataEvent.createNew("Vitor 2", createdAt = time + 1))
+
+            assertEquals(IEventStore.InsertOutcome.Accepted, db.batchInsert(listOf<Event>(newer))[0])
+            assertRejected(RejectionReason.SUPERSEDED, db.batchInsert(listOf<Event>(older))[0])
+        }
+
+    @Test
+    fun sameSecondReplaceableTieLoserIsRejectedAsSuperseded() =
+        forEachDB { db ->
+            val time = TimeUtils.now()
+            // NIP-01 breaks a created_at tie by lowest id, so the winner is
+            // decided by sorting, not by insertion order.
+            val (winner, loser) =
+                listOf(
+                    signer.sign(MetadataEvent.createNew("Vitor A", createdAt = time)),
+                    signer.sign(MetadataEvent.createNew("Vitor B", createdAt = time)),
+                ).sortedBy { it.id }
+
+            assertEquals(IEventStore.InsertOutcome.Accepted, db.batchInsert(listOf<Event>(winner))[0])
+            assertRejected(RejectionReason.SUPERSEDED, db.batchInsert(listOf<Event>(loser))[0])
+        }
+
+    @Test
+    fun olderAddressableIsRejectedAsSuperseded() =
+        forEachDB { db ->
+            val time = TimeUtils.now()
+            val older = signer.sign(LongTextNoteEvent.build("v1", "title", dTag = "blog", createdAt = time))
+            val newer = signer.sign(LongTextNoteEvent.build("v2", "title", dTag = "blog", createdAt = time + 1))
+
+            assertEquals(IEventStore.InsertOutcome.Accepted, db.batchInsert(listOf<Event>(newer))[0])
+            assertRejected(RejectionReason.SUPERSEDED, db.batchInsert(listOf<Event>(older))[0])
+        }
+
+    @Test
+    fun aNewerVersionStillLandsAtAnOccupiedCoordinate() =
+        forEachDB { db ->
+            // The guard against the classifier over-claiming: a coordinate is
+            // occupied here too, but this version wins, so nothing is rejected.
+            val time = TimeUtils.now()
+            val older = signer.sign(MetadataEvent.createNew("Vitor 1", createdAt = time))
+            val newer = signer.sign(MetadataEvent.createNew("Vitor 2", createdAt = time + 1))
+
+            assertEquals(IEventStore.InsertOutcome.Accepted, db.batchInsert(listOf<Event>(older))[0])
+            assertEquals(IEventStore.InsertOutcome.Accepted, db.batchInsert(listOf<Event>(newer))[0])
+        }
+}