From e9e994fff476dc505739aed6ace0ecbe4f554012 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 13:31:16 +0000 Subject: [PATCH] fix(quartz/sqlite): Batch A correctness/consistency fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - isExpired (#10): NIP-40 says an event is expired *once* `expiration` is reached, and the SQL trigger uses `<= unixepoch()`. The Kotlin pre-check used `<` (strict) so an event with `expiration == now` passed the Kotlin check then failed in the trigger. Both layers now use `<=`. Also applies to `isExpirationBefore` for consistency. - transaction extension (#7): if the body throws *and* ROLLBACK also throws, we now attach the rollback failure as a suppressed exception instead of letting it mask the original cause. COMMIT is moved outside the catch so a commit failure doesn't trigger a second ROLLBACK on already-finalized transaction state. - SeedModule.hasher (#8): the cache field is now a `kotlin.concurrent. atomics.AtomicReference` (matches the pattern used in BleChunkAssembler and BasicRelayClient) so the hasher publication is visible across threads. The race itself is benign — the seed is stable, so two concurrent computations produce identical hashers — but the prior plain `var` had no visibility guarantee. - delete(Filter()) (#12): documents the intentional asymmetry — `query` on an empty filter returns everything, but `delete` on an empty filter is a no-op (safe-by-default). New test pins the contract. Tests: - testInsertingEventExpiringExactlyNow: events with `expiration == now` are rejected by both Kotlin and the trigger. - testTransactionRollsBackOnException: a user transaction whose body throws leaves the DB unchanged and still accepts new writes. - testDeleteWithEmptyFilterIsSafe: empty-filter delete is a no-op. https://claude.ai/code/session_01X163Nr31vGkvAXoJ3JgMov --- .../nip01Core/store/sqlite/QueryBuilder.kt | 8 +++ .../store/sqlite/SQLiteConnectionExt.kt | 18 +++++-- .../nip01Core/store/sqlite/SeedModule.kt | 16 +++++- .../quartz/nip40Expiration/TagArrayExt.kt | 4 +- .../nip01Core/store/sqlite/BasicTest.kt | 53 +++++++++++++++++++ .../nip01Core/store/sqlite/ExpirationTest.kt | 22 ++++++++ 6 files changed, 113 insertions(+), 8 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/QueryBuilder.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/QueryBuilder.kt index a88a12ceb..fa6ad1ffe 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/QueryBuilder.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/QueryBuilder.kt @@ -318,6 +318,13 @@ class QueryBuilder( // -------------- // Deletes // ------------- + + /** + * Safe-by-default: an empty filter (or a list of only empty filters) + * deletes nothing and returns 0, so a stray `delete(Filter())` cannot + * wipe the entire store. This is asymmetric with `query(Filter())`, + * which intentionally returns every event. + */ fun delete( filter: Filter, db: SQLiteConnection, @@ -331,6 +338,7 @@ class QueryBuilder( } } + /** See [delete] for the empty-filter contract. */ fun delete( filters: List, db: SQLiteConnection, diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SQLiteConnectionExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SQLiteConnectionExt.kt index 0d5a8464c..f56978838 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SQLiteConnectionExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SQLiteConnectionExt.kt @@ -70,14 +70,24 @@ fun SQLiteConnection.changes(): Int = inline fun SQLiteConnection.transaction(body: SQLiteConnection.() -> T): T { execSQL("BEGIN IMMEDIATE TRANSACTION") + val result: T try { - val result = body() - execSQL("END TRANSACTION") - return result + result = body() } catch (e: Throwable) { - execSQL("ROLLBACK TRANSACTION") + // Attempt rollback, but never let a rollback failure mask the + // original cause — attach it as a suppressed exception instead. + try { + execSQL("ROLLBACK TRANSACTION") + } catch (rollbackError: Throwable) { + e.addSuppressed(rollbackError) + } throw e } + // Commit is intentionally outside the catch: if COMMIT fails SQLite + // has already finalized the transaction state, so we must not also + // try to ROLLBACK on top of it. + execSQL("END TRANSACTION") + return result } fun SQLiteStatement.bindAny( diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SeedModule.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SeedModule.kt index a997497a2..d17d65b89 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SeedModule.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/SeedModule.kt @@ -22,7 +22,10 @@ package com.vitorpamplona.quartz.nip01Core.store.sqlite import androidx.sqlite.SQLiteConnection import com.vitorpamplona.quartz.utils.RandomInstance +import kotlin.concurrent.atomics.AtomicReference +import kotlin.concurrent.atomics.ExperimentalAtomicApi +@OptIn(ExperimentalAtomicApi::class) class SeedModule : IModule { override fun create(db: SQLiteConnection) { db.execSQL("CREATE TABLE seeds (seed_value INTEGER)") @@ -78,7 +81,16 @@ class SeedModule : IModule { override fun deleteAll(db: SQLiteConnection) {} - private var hasherCache: TagNameValueHasher? = null + // The hasher is keyed off a stable per-DB seed, so two concurrent + // computations would produce identical hashers — the race is benign. + // We still publish via AtomicReference so the field write is visible + // across threads (commonMain has no @Volatile equivalent). + private val hasherCache = AtomicReference(null) - fun hasher(db: SQLiteConnection): TagNameValueHasher = hasherCache ?: TagNameValueHasher(getSeed(db)).also { hasherCache = it } + fun hasher(db: SQLiteConnection): TagNameValueHasher { + hasherCache.load()?.let { return it } + val fresh = TagNameValueHasher(getSeed(db)) + hasherCache.compareAndSet(null, fresh) + return hasherCache.load() ?: fresh + } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip40Expiration/TagArrayExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip40Expiration/TagArrayExt.kt index 14b0215b7..625708aac 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip40Expiration/TagArrayExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip40Expiration/TagArrayExt.kt @@ -27,10 +27,10 @@ fun TagArray.expiration() = this.firstNotNullOfOrNull(ExpirationTag::parse) fun TagArray.isExpired(): Boolean { val exp = expiration() ?: return false - return exp < TimeUtils.now() + return exp <= TimeUtils.now() } fun TagArray.isExpirationBefore(time: Long): Boolean { val exp = expiration() ?: return false - return exp < time + return exp <= time } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/BasicTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/BasicTest.kt index 0968e4808..dd0e74631 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/BasicTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/BasicTest.kt @@ -200,6 +200,59 @@ class BasicTest : BaseDBTest() { } } + @Test + fun testDeleteWithEmptyFilterIsSafe() = + forEachDB { db -> + val note1 = signer.sign(TextNoteEvent.build("test1", createdAt = 1)) + val note2 = signer.sign(TextNoteEvent.build("test2", createdAt = 2)) + + db.insert(note1) + db.insert(note2) + + // Empty filter: query returns everything, but delete must NOT + // wipe the store. This asymmetry is intentional safe-by-default. + assertEquals(2, db.count(Filter())) + db.delete(Filter()) + + db.assertQuery(note1, Filter(ids = listOf(note1.id))) + db.assertQuery(note2, Filter(ids = listOf(note2.id))) + + // Same applies to a list of empty filters. + db.delete(listOf(Filter(), Filter())) + assertEquals(2, db.count(Filter())) + } + + @Test + fun testTransactionRollsBackOnException() = + forEachDB { db -> + val note1 = signer.sign(TextNoteEvent.build("kept", createdAt = 1)) + val note2 = signer.sign(TextNoteEvent.build("rolled-back", createdAt = 2)) + + // Pre-existing event the test should not disturb. + db.insert(note1) + db.assertQuery(note1, Filter(ids = listOf(note1.id))) + + // A user-level transaction that inserts note2 then throws should + // leave the DB exactly as it was before — note2 must NOT remain. + val sentinel = RuntimeException("boom") + try { + db.transaction { + insert(note2) + throw sentinel + } + error("transaction should have rethrown") + } catch (e: RuntimeException) { + kotlin.test.assertEquals(sentinel, e) + } + + db.assertQuery(note1, Filter(ids = listOf(note1.id))) + db.assertQuery(null, Filter(ids = listOf(note2.id))) + + // After a rollback, the DB must still accept new writes. + db.insert(note2) + db.assertQuery(note2, Filter(ids = listOf(note2.id))) + } + @Test fun testSchemaRecreateIsIdempotent() = forEachDB { db -> diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/ExpirationTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/ExpirationTest.kt index 793470e3b..406b97418 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/ExpirationTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/store/sqlite/ExpirationTest.kt @@ -82,4 +82,26 @@ class ExpirationTest : BaseDBTest() { db.insert(note1) } } + + @Test + fun testInsertingEventExpiringExactlyNow() = + forEachDB { db -> + val time = TimeUtils.now() + + // Per NIP-40 the event is expired once `expiration` is reached; + // the SQL trigger uses `<= unixepoch()` and Kotlin's isExpired + // now agrees, so an event whose expiration equals "now" must be + // rejected by both layers — not silently let through the Kotlin + // pre-check only to fail in the trigger. + val note = + signer.sign( + TextNoteEvent.build("expires-now", createdAt = time - 1) { + expiration(time) + }, + ) + + assertFailsWith { + db.insert(note) + } + } }