fix(quartz/sqlite): Batch A correctness/consistency fixes

- 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
This commit is contained in:
Claude
2026-04-25 13:31:16 +00:00
parent 28b23b5e63
commit e9e994fff4
6 changed files with 113 additions and 8 deletions
@@ -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<Filter>,
db: SQLiteConnection,
@@ -70,14 +70,24 @@ fun SQLiteConnection.changes(): Int =
inline fun <T> 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(
@@ -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<TagNameValueHasher?>(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
}
}
@@ -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
}
@@ -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 ->
@@ -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<SQLiteException> {
db.insert(note)
}
}
}