diff --git a/_primitives/_rust/kei-entity-store/src/verbs/archive.rs b/_primitives/_rust/kei-entity-store/src/verbs/archive.rs index ef657b3..f7ad2d3 100644 --- a/_primitives/_rust/kei-entity-store/src/verbs/archive.rs +++ b/_primitives/_rust/kei-entity-store/src/verbs/archive.rs @@ -9,6 +9,15 @@ //! fall back to legacy `archived` heuristics (those remain in //! `delete.rs` soft-path only). //! +//! FTS semantics: archiving means "hidden from active listing". The verb +//! therefore DELETEs the row from `fts_` inside the same +//! transaction as the UPDATE, so `search` will no longer return the +//! archived row. If a future caller flips the column back to active +//! (unarchive), it MUST reinsert the row into the FTS index — the +//! current contract does not auto-reindex on unarchive. "Keep +//! searchable while archived" is a future feature (`search +//! --include-archived`), NOT today. +//! //! Input: `{ id: }`. //! Output: `{ id, archived_at }` — `archived_at` is the stamped //! timestamp when a `_at` column exists, else `null`. @@ -36,7 +45,7 @@ pub fn run( let has_ts = schema.fields.iter().any(|f| f.name == ts_col); let now: i64 = chrono::Utc::now().timestamp(); - let rows = execute_archive(conn, schema, field_name, &ts_col, has_ts, &id, now)?; + let rows = archive_tx(conn, schema, field_name, &ts_col, has_ts, &id, now)?; if rows == 0 { return Err(VerbError::not_found_text(schema.name, id.as_string())); } @@ -54,7 +63,9 @@ fn guard_enabled(schema: &EntitySchema) -> Result<(), VerbError> { Ok(()) } -fn execute_archive( +/// UPDATE the archived column (+ stamp) and DELETE the row from FTS (if +/// configured) in one transaction. Either both persist or neither does. +fn archive_tx( conn: &Connection, schema: &EntitySchema, field_name: &str, @@ -62,11 +73,35 @@ fn execute_archive( has_ts: bool, id: &PkValue, now: i64, +) -> Result { + let tx = conn.unchecked_transaction()?; + let rows = execute_archive(&tx, schema, field_name, ts_col, has_ts, id, now)?; + if rows > 0 && schema.fts_columns.is_some() { + tx.execute( + &format!( + "DELETE FROM fts_{t} WHERE {t}_id=?1", + t = schema.table + ), + rusqlite::params![id.as_sql()], + )?; + } + tx.commit()?; + Ok(rows) +} + +fn execute_archive( + tx: &rusqlite::Transaction<'_>, + schema: &EntitySchema, + field_name: &str, + ts_col: &str, + has_ts: bool, + id: &PkValue, + now: i64, ) -> Result { let marker = archive_marker(schema, field_name); let pk_name = schema.pk().name; let rows = if has_ts { - conn.execute( + tx.execute( &format!( "UPDATE {t} SET {field_name} = ?1, {ts_col} = ?2 WHERE {pk_name} = ?3", t = schema.table @@ -74,7 +109,7 @@ fn execute_archive( rusqlite::params![marker, now, id.as_sql()], )? } else { - conn.execute( + tx.execute( &format!( "UPDATE {t} SET {field_name} = ?1 WHERE {pk_name} = ?2", t = schema.table diff --git a/_primitives/_rust/kei-entity-store/src/verbs/delete.rs b/_primitives/_rust/kei-entity-store/src/verbs/delete.rs index 92b451b..bdd8f24 100644 --- a/_primitives/_rust/kei-entity-store/src/verbs/delete.rs +++ b/_primitives/_rust/kei-entity-store/src/verbs/delete.rs @@ -1,9 +1,13 @@ //! `delete` verb — hard DELETE by id, OR soft (if schema has an //! `archived` integer field, flips it to 1). +//! +//! The hard-delete path wraps the `fts_
` DELETE and the base-table +//! DELETE in a single `unchecked_transaction`, so a mid-flight FTS failure +//! rolls back the row delete too (mirrors create/update C2 fix). use crate::error::VerbError; use crate::schema::EntitySchema; -use crate::verbs::pk; +use crate::verbs::pk::{self, PkValue}; use rusqlite::Connection; use serde_json::{json, Value}; @@ -22,29 +26,9 @@ pub fn run( let soft = input.get("soft").and_then(|v| v.as_bool()).unwrap_or(false); let rows = if soft && has_archived_field(schema) { - conn.execute( - &format!( - "UPDATE {} SET archived = 1 WHERE {}=?1", - schema.table, - schema.pk().name - ), - rusqlite::params![id.as_sql()], - )? + soft_delete(conn, schema, &id)? } else { - if schema.fts_columns.is_some() { - conn.execute( - &format!("DELETE FROM fts_{} WHERE {}_id=?1", schema.table, schema.table), - rusqlite::params![id.as_sql()], - )?; - } - conn.execute( - &format!( - "DELETE FROM {} WHERE {}=?1", - schema.table, - schema.pk().name - ), - rusqlite::params![id.as_sql()], - )? + hard_delete_tx(conn, schema, &id)? }; if rows == 0 { return Err(VerbError::not_found_text(schema.name, id.as_string())); @@ -52,6 +36,51 @@ pub fn run( Ok(json!({ "ok": true, "id": id.as_json() })) } +fn soft_delete( + conn: &Connection, + schema: &EntitySchema, + id: &PkValue, +) -> Result { + let rows = conn.execute( + &format!( + "UPDATE {} SET archived = 1 WHERE {}=?1", + schema.table, + schema.pk().name + ), + rusqlite::params![id.as_sql()], + )?; + Ok(rows) +} + +/// FTS DELETE + base-table DELETE in one transaction. If either fails, +/// neither persists. +fn hard_delete_tx( + conn: &Connection, + schema: &EntitySchema, + id: &PkValue, +) -> Result { + let tx = conn.unchecked_transaction()?; + if schema.fts_columns.is_some() { + tx.execute( + &format!( + "DELETE FROM fts_{t} WHERE {t}_id=?1", + t = schema.table + ), + rusqlite::params![id.as_sql()], + )?; + } + let rows = tx.execute( + &format!( + "DELETE FROM {} WHERE {}=?1", + schema.table, + schema.pk().name + ), + rusqlite::params![id.as_sql()], + )?; + tx.commit()?; + Ok(rows) +} + fn has_archived_field(schema: &EntitySchema) -> bool { schema.fields.iter().any(|f| f.name == "archived") } diff --git a/_primitives/_rust/kei-entity-store/tests/bug_fixes_smoke.rs b/_primitives/_rust/kei-entity-store/tests/bug_fixes_smoke.rs index f0321cc..f947405 100644 --- a/_primitives/_rust/kei-entity-store/tests/bug_fixes_smoke.rs +++ b/_primitives/_rust/kei-entity-store/tests/bug_fixes_smoke.rs @@ -2,8 +2,8 @@ //! injection/M3/TEXT-cap/M2). Each test names the finding it pins. use kei_entity_store::error::VerbError; -use kei_entity_store::schema::{EntitySchema, FieldDef}; -use kei_entity_store::verbs::{create, search, update}; +use kei_entity_store::schema::{EdgeKeyKind, EntitySchema, FieldDef}; +use kei_entity_store::verbs::{archive, create, delete, search, update}; use kei_entity_store::verbs::validate::MAX_TEXT_BYTES; use kei_entity_store::Store; use rusqlite::Connection; @@ -190,3 +190,172 @@ fn m2_user_version_applied_once_idempotent() { assert_eq!(v, kei_entity_store::engine::CURRENT_USER_VERSION); } } + +// ---------- delete/archive transaction semantics ---------- +// +// Two audit findings pinned here: +// (1) delete.rs hard-path wraps FTS DELETE + table DELETE in one tx +// (was two separate execs — partial failure orphaned FTS rows). +// (2) archive.rs removes the row from FTS inside the same tx as the +// UPDATE (was UPDATE only — archived rows stayed searchable). + +static ARCHIVABLE_FIELDS: &[FieldDef] = &[ + FieldDef::pk("id"), + FieldDef::text_nn("title"), + FieldDef::text("description"), + FieldDef::integer("status"), + FieldDef::integer("status_at"), + FieldDef::created_at(), +]; + +/// FTS + archived_field both configured — required for +/// `archive_removes_from_fts` and `archive_rollback_on_sabotage`. +static ARCHIVABLE_FTS: EntitySchema = EntitySchema { + name: "item", + table: "items", + fields: ARCHIVABLE_FIELDS, + enabled_verbs: &["create", "get", "search", "archive"], + fts_columns: Some(&["title", "description"]), + edge_table: None, + edge_key_kind: EdgeKeyKind::IntegerPair, + archived_field: Some("status"), + custom_migrations: &[], +}; + +static NO_FTS_FIELDS: &[FieldDef] = &[ + FieldDef::pk("id"), + FieldDef::text_nn("title"), + FieldDef::created_at(), +]; + +/// No FTS — required for `delete_succeeds_when_no_fts_configured`. +static NO_FTS_SCHEMA: EntitySchema = EntitySchema { + name: "item", + table: "plain_items", + fields: NO_FTS_FIELDS, + enabled_verbs: &["create", "get", "delete"], + fts_columns: None, + edge_table: None, + edge_key_kind: EdgeKeyKind::IntegerPair, + archived_field: None, + custom_migrations: &[], +}; + +#[test] +fn delete_rollback_on_fts_sabotage() { + // Hard-delete path must roll back when the FTS DELETE fails — the + // row itself must survive. Pins: delete.rs tx wrapping (finding 1). + let s = mk(); + let id = create::run(s.conn(), &SCHEMA, json!({ "title": "keepme" })) + .unwrap()["id"] + .as_i64() + .unwrap(); + + s.conn().execute_batch("DROP TABLE fts_items;").unwrap(); + + let result = delete::run(s.conn(), &SCHEMA, json!({ "id": id })); + assert!(result.is_err(), "delete must fail when FTS is missing"); + + let count: i64 = s + .conn() + .query_row( + "SELECT COUNT(*) FROM items WHERE id=?1", + rusqlite::params![id], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(count, 1, "row must survive FTS-failed delete (rollback)"); +} + +#[test] +fn archive_removes_from_fts() { + // Archive must remove the row from FTS so `search` no longer returns + // it. Pins: archive.rs fts-delete-in-tx (finding 2). + let s = Store::open_memory(&[&ARCHIVABLE_FTS]).unwrap(); + let id = create::run( + s.conn(), + &ARCHIVABLE_FTS, + json!({ "title": "uniqword42", "description": "findme" }), + ) + .unwrap()["id"] + .as_i64() + .unwrap(); + + // Before archive: search finds the row. + let before = + search::run(s.conn(), &ARCHIVABLE_FTS, json!({ "query": "uniqword42" })).unwrap(); + assert_eq!( + before["results"].as_array().unwrap().len(), + 1, + "row must be searchable before archive" + ); + + archive::run(s.conn(), &ARCHIVABLE_FTS, json!({ "id": id })).unwrap(); + + // After archive: search returns zero hits. + let after = + search::run(s.conn(), &ARCHIVABLE_FTS, json!({ "query": "uniqword42" })).unwrap(); + assert_eq!( + after["results"].as_array().unwrap().len(), + 0, + "archived row must not be searchable" + ); +} + +#[test] +fn archive_rollback_on_sabotage() { + // Archive wraps UPDATE + FTS DELETE in one tx. If the FTS delete + // fails, the UPDATE must roll back too. Pins: archive.rs tx + // wrapping (finding 2). + let s = Store::open_memory(&[&ARCHIVABLE_FTS]).unwrap(); + let id = create::run( + s.conn(), + &ARCHIVABLE_FTS, + json!({ "title": "stable", "description": "x" }), + ) + .unwrap()["id"] + .as_i64() + .unwrap(); + + s.conn().execute_batch("DROP TABLE fts_items;").unwrap(); + + let result = archive::run(s.conn(), &ARCHIVABLE_FTS, json!({ "id": id })); + assert!(result.is_err(), "archive must fail when FTS is missing"); + + // Status column must still be 0 (unarchived) — the UPDATE rolled back. + let status: i64 = s + .conn() + .query_row( + "SELECT status FROM items WHERE id=?1", + rusqlite::params![id], + |r| r.get(0), + ) + .unwrap(); + assert_eq!( + status, 0, + "archived_field must be unchanged after FTS-failed archive" + ); +} + +#[test] +fn delete_succeeds_when_no_fts_configured() { + // Delete on a schema with `fts_columns: None` must work — the tx + // wrapping must not introduce a spurious FTS DELETE. + let s = Store::open_memory(&[&NO_FTS_SCHEMA]).unwrap(); + let id = create::run(s.conn(), &NO_FTS_SCHEMA, json!({ "title": "bye" })) + .unwrap()["id"] + .as_i64() + .unwrap(); + + delete::run(s.conn(), &NO_FTS_SCHEMA, json!({ "id": id })).unwrap(); + + let count: i64 = s + .conn() + .query_row( + "SELECT COUNT(*) FROM plain_items WHERE id=?1", + rusqlite::params![id], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(count, 0, "row must be gone after hard delete on no-FTS schema"); +}