From 2c555c4640b1a141a4c4448bb1b402a4feaee4ec Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 13 Aug 2026 14:00:23 -0400 Subject: [PATCH] refactor(archive): drop M4 schema-shape validator for fail-closed guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M4 runs its whole body under one BEGIN IMMEDIATE with the marker written last, and SQLite DDL is transactional, so no shipped code path can leave a wrong-shaped object without the marker — a crash rolls everything back. The shape validator (table_shape_matches, scope_age_index_is_correct, validate_retention_schema_shape, and their shape constants) only defended against hand-mutated or foreign-tool DBs, an out-of-scope threat model. Replace it with the reachability-honest minimum: refuse to certify a pre-existing retention_policies/archive_meta table (fail closed, no marker), plain CREATE TABLE otherwise, and an unconditional DROP INDEX IF EXISTS + CREATE INDEX for the scope-age index (carries no data, so a fresh build is always correct). The BEGIN IMMEDIATE + pre-lock guard + in-lock recheck race machinery, seeding, and marker-last ordering are unchanged. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/archive/retention.rs | 186 ++------------- .../src-tauri/src/archive/retention_tests.rs | 214 ++++++------------ .../src-tauri/src/archive/store_migrations.rs | 69 +++--- 3 files changed, 124 insertions(+), 345 deletions(-) diff --git a/desktop/src-tauri/src/archive/retention.rs b/desktop/src-tauri/src/archive/retention.rs index 10108e7b8..bf7d2bdba 100644 --- a/desktop/src-tauri/src/archive/retention.rs +++ b/desktop/src-tauri/src/archive/retention.rs @@ -12,7 +12,7 @@ //! the existing `metric_store.rs` / `pipeline.rs` / `store_migrations.rs` //! precedent. -use rusqlite::{params, Connection, OptionalExtension}; +use rusqlite::{params, Connection}; // ── Constants ──────────────────────────────────────────────────────────────── @@ -33,8 +33,11 @@ pub const MAX_RETENTION_DAYS: i64 = 36_500; /// `retention_policies` + `archive_meta`. Created inside M4 under /// `BEGIN IMMEDIATE` (see `store_migrations::migrate_add_retention_policies`). +/// Plain `CREATE TABLE` (no `IF NOT EXISTS`): M4 creates these once on a DB that +/// provably has neither table yet (the fail-closed guard rejects a pre-existing +/// one), so the clause would be dead weight. pub(super) const RETENTION_SCHEMA: &str = " -CREATE TABLE IF NOT EXISTS retention_policies ( +CREATE TABLE retention_policies ( identity_pubkey TEXT NOT NULL, relay_url TEXT NOT NULL, scope_type TEXT NOT NULL, @@ -45,7 +48,7 @@ CREATE TABLE IF NOT EXISTS retention_policies ( PRIMARY KEY (identity_pubkey, relay_url, scope_type, scope_value, kind) ); -CREATE TABLE IF NOT EXISTS archive_meta ( +CREATE TABLE archive_meta ( key TEXT PRIMARY KEY, value TEXT NOT NULL ); @@ -56,183 +59,20 @@ CREATE TABLE IF NOT EXISTS archive_meta ( /// one-time cost over the existing 1.3M-row archive is paid behind the init /// barrier rather than on the prune hot path). The scope PK /// `(identity, relay, id, scope_type, scope_value)` puts `id` before the -/// scope keys and lacks `archived_at`, so it cannot range-seek by age. +/// scope keys and lacks `archived_at`, so it cannot range-seek by age. M4 +/// `DROP INDEX IF EXISTS`es under this name and recreates it unconditionally, +/// so a fresh build is always correct — plain `CREATE INDEX`, no `IF NOT +/// EXISTS`. pub(super) const SCOPE_AGE_INDEX_DDL: &str = " -CREATE INDEX IF NOT EXISTS idx_archived_event_scopes_age +CREATE INDEX idx_archived_event_scopes_age ON archived_event_scopes (identity_pubkey, relay_url, scope_type, scope_value, archived_at, id); "; -/// Name of the scope-age index, shared by the DDL, the shape check, and the -/// M4 drop-and-rebuild repair path. +/// Name of the scope-age index, shared by the DDL and M4's unconditional +/// drop-and-rebuild. pub(super) const SCOPE_AGE_INDEX_NAME: &str = "idx_archived_event_scopes_age"; -// ── Expected schema shape (the single source of truth M4 validates against) ──── - -/// One column's expected shape: `(name, declared_type, not_null, pk_position)`. -/// `pk_position` is 0 when the column is not part of the primary key, else its -/// 1-based position in the PK — this is exactly what `PRAGMA table_info` reports -/// in its `pk` field, so PK column ORDER is validated, not just membership. -type ColumnShape = (&'static str, &'static str, bool, i64); - -/// Expected `retention_policies` columns in `cid` order. Mirrors -/// [`RETENTION_SCHEMA`]; a drift here or there is caught by the M4 shape check. -const RETENTION_POLICIES_SHAPE: &[ColumnShape] = &[ - ("identity_pubkey", "TEXT", true, 1), - ("relay_url", "TEXT", true, 2), - ("scope_type", "TEXT", true, 3), - ("scope_value", "TEXT", true, 4), - ("kind", "INTEGER", true, 5), - ("days", "INTEGER", false, 0), - ("updated_at", "INTEGER", true, 0), -]; - -/// Expected `archive_meta` columns in `cid` order. A `TEXT PRIMARY KEY` is -/// nullable in SQLite (only `INTEGER PRIMARY KEY` implies NOT NULL), so `key` -/// carries `not_null = false` with `pk_position = 1`. -const ARCHIVE_META_SHAPE: &[ColumnShape] = &[("key", "TEXT", false, 1), ("value", "TEXT", true, 0)]; - -/// One key column's expected shape: `(name, collation, descending)`. The -/// age-range seek needs binary-ordered ascending keys; a differing collation -/// (e.g. `NOCASE`) or sort direction produces an index the prune query planner -/// will not use even when the column names match. -type IndexKeyShape = (&'static str, &'static str, bool); - -/// Expected key columns of [`SCOPE_AGE_INDEX_NAME`] in seqno order — the -/// age-range access path Phase 2 depends on. Order is load-bearing: a covering -/// seek needs the scope keys before `archived_at`. Collation and direction are -/// load-bearing too: [`SCOPE_AGE_INDEX_DDL`] declares every key `BINARY` -/// ascending (SQLite's defaults), and only a matching shape serves the seek. -const SCOPE_AGE_INDEX_SHAPE: &[IndexKeyShape] = &[ - ("identity_pubkey", "BINARY", false), - ("relay_url", "BINARY", false), - ("scope_type", "BINARY", false), - ("scope_value", "BINARY", false), - ("archived_at", "BINARY", false), - ("id", "BINARY", false), -]; - -// ── Shape validation (used by migration M4) ──────────────────────────────────── - -/// Whether `table`'s live columns exactly match `expected` (name, declared -/// type, nullability, and PK position, all in `cid` order). `table` is always a -/// compile-time constant from this module, never user input, so interpolating -/// it into the table-valued `pragma_table_info` call carries no injection risk. -fn table_shape_matches( - conn: &Connection, - table: &str, - expected: &[ColumnShape], -) -> Result { - let mut stmt = conn - .prepare(&format!( - "SELECT name, type, \"notnull\", pk FROM pragma_table_info('{table}') ORDER BY cid" - )) - .map_err(|e| format!("shape check: prepare table_info({table}): {e}"))?; - let actual: Vec<(String, String, i64, i64)> = stmt - .query_map([], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?))) - .map_err(|e| format!("shape check: query table_info({table}): {e}"))? - .collect::, _>>() - .map_err(|e| format!("shape check: read table_info({table}): {e}"))?; - - Ok(actual.len() == expected.len() - && actual.iter().zip(expected).all( - |((name, ty, not_null, pk), (exp_name, exp_ty, exp_not_null, exp_pk))| { - name == exp_name - && ty.eq_ignore_ascii_case(exp_ty) - && (*not_null != 0) == *exp_not_null - && pk == exp_pk - }, - )) -} - -/// Whether the scope-age index exists on `archived_event_scopes` with exactly -/// the expected key columns — name, collation, and sort direction — in order. -/// False when the index is absent, sits on the wrong table, indexes the wrong -/// columns, applies a non-`BINARY` collation or descending order to any key, or -/// is partial (carries a `WHERE` predicate) — all of which M4 repairs by -/// dropping and recreating it (an index carries no data, so a rebuild is safe). -pub(super) fn scope_age_index_is_correct(conn: &Connection) -> Result { - let table: Option = conn - .query_row( - "SELECT tbl_name FROM sqlite_master WHERE type = 'index' AND name = ?1", - params![SCOPE_AGE_INDEX_NAME], - |r| r.get(0), - ) - .optional() - .map_err(|e| format!("shape check: scope-age index table: {e}"))?; - if table.as_deref() != Some("archived_event_scopes") { - return Ok(false); - } - - // A partial index — one carrying a `WHERE` predicate — can hold the exact - // expected key columns yet still fail to serve the unrestricted scope-age - // range scan the prune query needs (SQLite falls back to the PK autoindex). - // `pragma_index_info` reports key columns but NOT partiality, so probe - // `pragma_index_list`'s `partial` flag explicitly and treat a partial index - // like any other wrong shape (drop + rebuild non-partial). Absent from the - // list (should not happen after the table check above) fails closed. - let is_partial = conn - .query_row( - "SELECT partial FROM pragma_index_list('archived_event_scopes') WHERE name = ?1", - params![SCOPE_AGE_INDEX_NAME], - |r| r.get::<_, i64>(0), - ) - .optional() - .map_err(|e| format!("shape check: scope-age index partiality: {e}"))? - .map(|p| p != 0) - .unwrap_or(true); - if is_partial { - return Ok(false); - } - - let mut stmt = conn - .prepare(&format!( - "SELECT name, coll, desc FROM pragma_index_xinfo('{SCOPE_AGE_INDEX_NAME}') \ - WHERE key = 1 ORDER BY seqno" - )) - .map_err(|e| format!("shape check: prepare index_xinfo: {e}"))?; - let keys: Vec<(String, String, i64)> = stmt - .query_map([], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?))) - .map_err(|e| format!("shape check: query index_xinfo: {e}"))? - .collect::, _>>() - .map_err(|e| format!("shape check: read index_xinfo: {e}"))?; - - Ok(keys.len() == SCOPE_AGE_INDEX_SHAPE.len() - && keys.iter().zip(SCOPE_AGE_INDEX_SHAPE).all( - |((name, coll, desc), (exp_name, exp_coll, exp_desc))| { - name == exp_name && coll.eq_ignore_ascii_case(exp_coll) && (*desc != 0) == *exp_desc - }, - )) -} - -/// Validate the COMPLETE expected shape of the M4 objects: both tables' columns -/// (names, types, nullability, PK positions) and the scope-age index's key -/// order. `CREATE ... IF NOT EXISTS` silently preserves a wrong-shaped object -/// that already carries the expected name, so name presence alone cannot -/// certify the schema — a wrong-shaped named table would otherwise let M4 mark -/// itself applied and hand Phase 2 an unusable table or missing access path. -/// Called inside M4's `BEGIN IMMEDIATE`; an `Err` rolls the transaction back -/// with no marker written. -pub(super) fn validate_retention_schema_shape(conn: &Connection) -> Result<(), String> { - if !table_shape_matches(conn, "retention_policies", RETENTION_POLICIES_SHAPE)? { - return Err( - "migration M4: retention_policies has an unexpected column or primary-key shape" - .to_string(), - ); - } - if !table_shape_matches(conn, "archive_meta", ARCHIVE_META_SHAPE)? { - return Err( - "migration M4: archive_meta has an unexpected column or primary-key shape".to_string(), - ); - } - if !scope_age_index_is_correct(conn)? { - return Err(format!( - "migration M4: {SCOPE_AGE_INDEX_NAME} has an unexpected shape after rebuild" - )); - } - Ok(()) -} - // ── Types ───────────────────────────────────────────────────────────────────── /// A retention policy row with its live-subscription status, returned by diff --git a/desktop/src-tauri/src/archive/retention_tests.rs b/desktop/src-tauri/src/archive/retention_tests.rs index c9d661277..3dc425a82 100644 --- a/desktop/src-tauri/src/archive/retention_tests.rs +++ b/desktop/src-tauri/src/archive/retention_tests.rs @@ -366,11 +366,15 @@ fn test_m4_malformed_kinds_json_aborts_and_leaves_no_marker_then_recovers() { } #[test] -fn test_m4_partial_schema_marker_absent_recovers_on_next_open() { +fn test_m4_preexisting_retention_policies_without_marker_fails_closed() { let db = NamedTempFile::new().unwrap(); build_pre_m4_db(db.path()); - // Simulate an interrupted earlier run: retention_policies exists but - // archive_meta, the age index, and the marker do not. + // A `retention_policies` table present without the M4 marker is + // unreachable via shipped code — M4 runs the whole body (create tables, + // build index, seed, marker) in one transactional `BEGIN IMMEDIATE`, so a + // crash rolls back the table too. The only way to reach this state is an + // externally-created table. M4 refuses to certify it: it fails closed and + // rolls back with no marker rather than adopting a table it did not build. { let conn = Connection::open(db.path()).unwrap(); conn.execute_batch( @@ -382,183 +386,107 @@ fn test_m4_partial_schema_marker_absent_recovers_on_next_open() { ) .unwrap(); } - // The idempotent DDL fills in the missing objects and records the marker. - let conn = fresh(&db); - assert_eq!(m4_marker_count(&conn), 1); - let objects: i64 = conn - .query_row( - "SELECT COUNT(*) FROM sqlite_master - WHERE (type = 'table' AND name IN ('retention_policies', 'archive_meta')) - OR (type = 'index' AND name = 'idx_archived_event_scopes_age')", - [], - |r| r.get(0), - ) - .unwrap(); - assert_eq!(objects, 3, "partial schema repaired to the full shape"); -} - -#[test] -fn test_m4_wrong_shaped_named_table_rejected_no_marker() { - let db = NamedTempFile::new().unwrap(); - build_pre_m4_db(db.path()); - // Precreate an `archive_meta` table carrying the expected NAME but the - // wrong shape (its required `value` column is missing). `CREATE ... IF NOT - // EXISTS` preserves it, so only the explicit shape check can catch it. - { - let conn = Connection::open(db.path()).unwrap(); - conn.execute_batch("CREATE TABLE archive_meta (key TEXT PRIMARY KEY);") - .unwrap(); - } - // M4 must reject the incompatible named table and roll back with no marker, - // rather than certify a table Phase 2 could not use. assert!( store::open_archive_db(db.path()).is_err(), - "M4 must reject a wrong-shaped named archive_meta" + "M4 must fail closed on a pre-existing retention_policies" ); let verify = Connection::open(db.path()).unwrap(); assert_eq!( m4_marker_count(&verify), 0, - "no marker may certify the incompatible table" + "no marker may certify an externally-created table" ); - // The rollback left the schema untouched: the bad table is still one-column - // and retention_policies was never committed. - assert!( - !scope_age_index_is_correct(&verify).unwrap(), - "the index build rolled back with the rest of the transaction" - ); - let value_cols: i64 = verify + let archive_meta_exists: bool = verify .query_row( - "SELECT COUNT(*) FROM pragma_table_info('archive_meta') WHERE name = 'value'", + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'archive_meta'", [], - |r| r.get(0), + |r| r.get::<_, i64>(0), ) - .unwrap(); + .unwrap() + > 0; + assert!( + !archive_meta_exists, + "the rollback left no partially-created schema behind" + ); +} + +#[test] +fn test_m4_preexisting_archive_meta_without_marker_fails_closed() { + let db = NamedTempFile::new().unwrap(); + build_pre_m4_db(db.path()); + // Same fail-closed contract, exercised through the second guarded table. + // The table's shape is irrelevant — its mere presence without the marker + // means M4 did not create it, so M4 refuses rather than adopting it. + { + let conn = Connection::open(db.path()).unwrap(); + conn.execute_batch("CREATE TABLE archive_meta (key TEXT PRIMARY KEY);") + .unwrap(); + } + assert!( + store::open_archive_db(db.path()).is_err(), + "M4 must fail closed on a pre-existing archive_meta" + ); + let verify = Connection::open(db.path()).unwrap(); assert_eq!( - value_cols, 0, - "the wrong-shaped table was not silently altered" + m4_marker_count(&verify), + 0, + "no marker may certify an externally-created table" ); -} - -#[test] -fn test_m4_wrong_index_order_dropped_and_rebuilt() { - let db = NamedTempFile::new().unwrap(); - build_pre_m4_db(db.path()); - // Precreate the scope-age index with the expected NAME on the right table - // but the WRONG key order (`archived_at` first). A covering age-range seek - // needs the scope keys before `archived_at`, so this order is unusable. - { - let conn = Connection::open(db.path()).unwrap(); - conn.execute_batch(&format!( - "CREATE INDEX {SCOPE_AGE_INDEX_NAME} - ON archived_event_scopes - (archived_at, identity_pubkey, relay_url, scope_type, scope_value, id);" - )) - .unwrap(); - } - // M4 rebuilds a wrong-shaped index (safe — an index carries no data) rather - // than rejecting, so the open succeeds and the marker lands. - let conn = fresh(&db); - assert_eq!(m4_marker_count(&conn), 1, "M4 completes after the rebuild"); - assert!( - scope_age_index_is_correct(&conn).unwrap(), - "the index was rebuilt into the correct key order" - ); -} - -#[test] -fn test_m4_partial_age_index_dropped_and_rebuilt_non_partial() { - let db = NamedTempFile::new().unwrap(); - build_pre_m4_db(db.path()); - // Precreate the scope-age index with the expected NAME, right table, and - // EXACT key order — but a `WHERE` predicate making it PARTIAL. It holds the - // right columns, so `pragma_index_info` (key columns only) cannot tell it - // apart from the required index; only the partiality probe catches it. A - // partial index cannot serve the unrestricted prune-age range scan, so M4 - // must treat it like any other wrong shape and rebuild it non-partial. - { - let conn = Connection::open(db.path()).unwrap(); - conn.execute_batch(&format!( - "CREATE INDEX {SCOPE_AGE_INDEX_NAME} - ON archived_event_scopes - (identity_pubkey, relay_url, scope_type, scope_value, archived_at, id) - WHERE archived_at > 1000;" - )) - .unwrap(); - // The validator must reject the partial index BEFORE M4 runs — this is - // the exact check that fails without the partiality probe. - assert!( - !scope_age_index_is_correct(&conn).unwrap(), - "a partial index with correct columns must not be certified" - ); - } - // M4 rebuilds the partial index into a non-partial one, then the marker lands. - let conn = fresh(&db); - assert_eq!(m4_marker_count(&conn), 1, "M4 completes after the rebuild"); - assert!( - scope_age_index_is_correct(&conn).unwrap(), - "the index was rebuilt non-partial with the correct key order" - ); - // Prove the rebuilt index carries no `WHERE` predicate. - let partial: i64 = conn + // The rollback did not create retention_policies alongside the bad table. + let retention_policies_exists: bool = verify .query_row( - "SELECT partial FROM pragma_index_list('archived_event_scopes') WHERE name = ?1", - params![SCOPE_AGE_INDEX_NAME], - |r| r.get(0), + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'retention_policies'", + [], + |r| r.get::<_, i64>(0), ) - .unwrap(); - assert_eq!(partial, 0, "the rebuilt index must not be partial"); + .unwrap() + > 0; + assert!( + !retention_policies_exists, + "no policy row was seeded and no sibling table was created" + ); } #[test] -fn test_m4_wrong_collation_age_index_dropped_and_rebuilt_binary() { +fn test_m4_preexisting_index_under_name_is_silently_rebuilt() { let db = NamedTempFile::new().unwrap(); build_pre_m4_db(db.path()); - // Precreate the scope-age index with the expected NAME, right table, exact - // key order, and non-partial — but `COLLATE NOCASE` on the first key. It - // holds the right column names, is non-partial, and passes both the earlier - // name/order and partiality probes; only the xinfo collation check catches - // it. A NOCASE key makes the binary-equality prune-age query fall back to - // the PK autoindex plus a temp B-tree sort, so M4 must rebuild it BINARY. + // Unlike a table (which carries data and is fail-closed), an index carries + // no data, so M4 drops any index sharing the name and recreates it + // unconditionally — no shape inspection. A bogus pre-existing index under + // the name is silently replaced with the correct one and the marker lands. { let conn = Connection::open(db.path()).unwrap(); conn.execute_batch(&format!( - "CREATE INDEX {SCOPE_AGE_INDEX_NAME} - ON archived_event_scopes - (identity_pubkey COLLATE NOCASE, relay_url, scope_type, - scope_value, archived_at, id);" + "CREATE INDEX {SCOPE_AGE_INDEX_NAME} ON archived_event_scopes (id);" )) .unwrap(); - // The validator must reject the NOCASE index BEFORE M4 runs — this is - // the exact check that fails without the xinfo collation probe. - assert!( - !scope_age_index_is_correct(&conn).unwrap(), - "an index with a non-BINARY key collation must not be certified" - ); } - // M4 rebuilds the wrong-collation index BINARY, then the marker lands. let conn = fresh(&db); assert_eq!(m4_marker_count(&conn), 1, "M4 completes after the rebuild"); - assert!( - scope_age_index_is_correct(&conn).unwrap(), - "the index was rebuilt with BINARY collation on every key" - ); - // Prove every rebuilt key carries the default BINARY collation. + // The rebuilt index has the six expected key columns in the covering order. let mut stmt = conn .prepare(&format!( - "SELECT coll FROM pragma_index_xinfo('{SCOPE_AGE_INDEX_NAME}') \ + "SELECT name FROM pragma_index_xinfo('{SCOPE_AGE_INDEX_NAME}') \ WHERE key = 1 ORDER BY seqno" )) .unwrap(); - let colls: Vec = stmt + let keys: Vec = stmt .query_map([], |r| r.get::<_, String>(0)) .unwrap() .collect::, _>>() .unwrap(); - assert_eq!(colls.len(), 6, "all six key columns are indexed"); - assert!( - colls.iter().all(|c| c.eq_ignore_ascii_case("BINARY")), - "every rebuilt key uses BINARY collation, got {colls:?}" + assert_eq!( + keys, + [ + "identity_pubkey", + "relay_url", + "scope_type", + "scope_value", + "archived_at", + "id" + ], + "the index was rebuilt with the covering key order" ); } diff --git a/desktop/src-tauri/src/archive/store_migrations.rs b/desktop/src-tauri/src/archive/store_migrations.rs index e9b9dad15..86bf39267 100644 --- a/desktop/src-tauri/src/archive/store_migrations.rs +++ b/desktop/src-tauri/src/archive/store_migrations.rs @@ -342,10 +342,12 @@ fn migrate_add_cache_write_and_pricing(conn: &Connection) -> Result<(), String> /// committed marker and no-ops. The cheap pre-lock guard keeps steady-state /// opens off the write lock entirely (M4 only takes it until the marker lands). /// -/// All DDL is `CREATE ... IF NOT EXISTS` and seeding is `ON CONFLICT DO -/// NOTHING`, so the whole body is idempotent; the marker is written last inside -/// the same transaction, so a crash before COMMIT rolls back every object and -/// the next open re-runs from scratch. +/// The tables use plain `CREATE TABLE` behind a fail-closed guard (a +/// pre-existing `retention_policies`/`archive_meta` with no marker is an +/// externally-created object M4 refuses to certify) and the index is dropped +/// and recreated unconditionally; seeding is `ON CONFLICT DO NOTHING`. The +/// marker is written last inside the same transaction, so a crash before COMMIT +/// rolls back every object and the next open re-runs from scratch. fn migrate_add_retention_policies(conn: &Connection) -> Result<(), String> { // Cheap pre-lock guard: steady-state opens (marker already present) never // take the write lock. The marker is written last in M4's transaction, so @@ -379,34 +381,43 @@ fn migrate_add_retention_policies_locked(conn: &Connection) -> Result<(), String return Ok(()); } - // Idempotent DDL — creates any missing objects, repairing a partial state - // left by an interrupted earlier run. `IF NOT EXISTS` preserves an object - // that already carries the expected name, so it cannot fix a wrong SHAPE — - // that is what the explicit validation below is for. + // Fail closed on an externally-created table. M4's whole body runs inside + // one `BEGIN IMMEDIATE` transaction with the marker written last, and + // SQLite DDL is transactional — a crash anywhere rolls the whole thing + // back. So no shipped code path can leave either table present without the + // marker; the only way to reach here with one already existing is a + // hand-edited DB or a foreign tool. Rather than certify a table we did not + // create, refuse: roll back with no marker and let a corrected DB re-run. + for table in ["retention_policies", "archive_meta"] { + let exists: bool = conn + .query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = ?1", + params![table], + |r| r.get::<_, i64>(0), + ) + .map_err(|e| format!("migration M4: probe {table}: {e}"))? + > 0; + if exists { + return Err(format!( + "migration M4: {table} already exists without the M4 marker — \ + refusing to certify an externally-created table" + )); + } + } + conn.execute_batch(super::retention::RETENTION_SCHEMA) .map_err(|e| format!("migration M4: create retention schema: {e}"))?; - // Repair a missing or wrong-shaped scope-age index by dropping and - // recreating it. An index carries no data, so a rebuild is always safe — - // unlike a table, whose wrong shape we must reject rather than drop. - if !super::retention::scope_age_index_is_correct(conn)? { - conn.execute_batch(&format!( - "DROP INDEX IF EXISTS {}", - super::retention::SCOPE_AGE_INDEX_NAME - )) - .map_err(|e| format!("migration M4: drop wrong-shaped scope-age index: {e}"))?; - conn.execute_batch(super::retention::SCOPE_AGE_INDEX_DDL) - .map_err(|e| format!("migration M4: create scope-age index: {e}"))?; - } - - // Validate the COMPLETE expected shape (both tables' columns, types, - // nullability, and PK positions, plus the index key order) before seeding - // or writing the marker. A wrong-shaped named TABLE cannot be auto-repaired - // without risking archived data, so it is rejected here and the whole - // transaction rolls back with no marker — the next open re-runs M4 once the - // schema is corrected. This guarantees the marker never certifies a - // half-built or mis-shaped schema that Phase 2 would inherit. - super::retention::validate_retention_schema_shape(conn)?; + // Unconditionally rebuild the scope-age index. It carries no data, so a + // fresh `CREATE` is always correct; dropping any index that happens to + // share the name costs one rebuild and needs no shape inspection. + conn.execute_batch(&format!( + "DROP INDEX IF EXISTS {}", + super::retention::SCOPE_AGE_INDEX_NAME + )) + .map_err(|e| format!("migration M4: drop any pre-existing scope-age index: {e}"))?; + conn.execute_batch(super::retention::SCOPE_AGE_INDEX_DDL) + .map_err(|e| format!("migration M4: create scope-age index: {e}"))?; let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH)