diff --git a/crates/persistence/src/backends/postgres/search/query_builder.rs b/crates/persistence/src/backends/postgres/search/query_builder.rs index 00a6838f4..748ff999e 100644 --- a/crates/persistence/src/backends/postgres/search/query_builder.rs +++ b/crates/persistence/src/backends/postgres/search/query_builder.rs @@ -527,6 +527,19 @@ impl PostgresQueryBuilder { "_lastUpdated" => { return Self::build_last_updated_condition(¶m.values, param_offset); } + // Full text over the generated narrative (`_text`) and over the whole + // serialized resource (`_content`), against the same `resource_fts` + // tsvector columns the write path populates. + "_text" => { + return Self::build_fts_condition( + ¶m.values, + "narrative_tsvector", + param_offset, + ); + } + "_content" => { + return Self::build_fts_condition(¶m.values, "content_tsvector", param_offset); + } _ => {} } @@ -563,6 +576,72 @@ impl PostgresQueryBuilder { Some(combined) } + /// Builds the `_text` / `_content` full-text condition against + /// `resource_fts`. + /// + /// Without this, `SearchParamType::Special` fell through to the `None` arm + /// below and the parameter contributed **no** condition. A search whose only + /// parameter was `_text` therefore produced an empty filter and returned + /// every resource of the type — a text query answered with the whole + /// compartment, not a narrower or empty result. SQLite has handled both + /// parameters (via FTS5 `MATCH`) all along; this is the PostgreSQL half. + /// + /// `plainto_tsquery('english', …)` matches `search_text`/`search_content` in + /// `search_impl.rs`, and it parameterises the user's term rather than + /// splicing it, so a term containing tsquery operators is data, not syntax. + /// + /// `tenant_id = $1` is not optional: the sub-select yields a bare + /// `resource_id` set that the outer query intersects with *this* tenant's + /// resources, so omitting it would let tenant B's Patient/123 select tenant + /// A's Patient/123 — a cross-tenant match oracle. `resource_type = $2` + /// likewise keeps an Observation's narrative from selecting a Patient of the + /// same id. Both are already bound as tenant and resource type by every + /// caller of `build_search_query` (`search`, `search_count`), the same + /// invariant `build_missing_condition` and `build_compartment_condition` + /// rely on. + fn build_fts_condition( + values: &[SearchValue], + column: &str, + offset: usize, + ) -> Option { + let mut conditions = Vec::new(); + // Numbered off the running count of *accepted* values, not the loop + // index: a skipped value must not leave a gap, because the caller binds + // this fragment's params consecutively from `offset`. + let mut param_num = offset; + for value in values { + let term = value.value.trim(); + if term.is_empty() { + continue; + } + param_num += 1; + conditions.push(SqlFragment::with_params( + format!( + "id IN (SELECT resource_id FROM resource_fts \ + WHERE tenant_id = $1 AND resource_type = $2 \ + AND {} @@ plainto_tsquery('english', ${}))", + column, param_num + ), + vec![SqlParam::text(term)], + )); + } + if conditions.is_empty() { + // Every value was blank. Returning `None` here would drop the + // parameter and hand back the entire resource type — the defect this + // function exists to fix — so fail closed instead. A term of nothing + // matches nothing, which is also what a stopword-only term already + // does through `plainto_tsquery`. + return Some(SqlFragment::new("FALSE".to_string())); + } + // Repeated values of one parameter are a logical OR, as elsewhere in + // this builder. + let mut combined = conditions.remove(0); + for cond in conditions { + combined = combined.or(cond); + } + Some(combined) + } + fn build_last_updated_condition(values: &[SearchValue], offset: usize) -> Option { let mut conditions = Vec::new(); for (i, value) in values.iter().enumerate() { @@ -1625,6 +1704,105 @@ mod tests { assert_eq!(frag.params.len(), 3); } + fn special_param(name: &str, values: Vec) -> SearchParameter { + SearchParameter { + name: name.to_string(), + param_type: SearchParamType::Special, + modifier: None, + values, + chain: vec![], + components: vec![], + } + } + + #[test] + fn text_search_filters_instead_of_returning_everything() { + // Regression: `SearchParamType::Special` fell through to `None`, so a + // `_text`-only query built no filter at all and `search` answered a + // full-text query with every resource of the type. + let query = SearchQuery::new("Patient").with_parameter(special_param( + "_text", + vec![SearchValue::eq("Zebracrossingdiagnosis")], + )); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("_text must produce a condition, not an empty filter"); + + assert!( + frag.sql.contains("FROM resource_fts"), + "must resolve through the full-text table: {}", + frag.sql + ); + assert!( + frag.sql + .contains("narrative_tsvector @@ plainto_tsquery('english', $3)"), + "_text matches the narrative column: {}", + frag.sql + ); + // Tenant and type scoping keep the sub-select from selecting another + // tenant's — or another resource type's — row of the same id. + assert!(frag.sql.contains("tenant_id = $1"), "{}", frag.sql); + assert!(frag.sql.contains("resource_type = $2"), "{}", frag.sql); + assert_eq!(frag.params.len(), 1); + } + + #[test] + fn content_search_uses_the_content_column() { + let query = SearchQuery::new("Patient").with_parameter(special_param( + "_content", + vec![SearchValue::eq("Quokkaflavoured")], + )); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("_content must produce a condition"); + + assert!( + frag.sql + .contains("content_tsvector @@ plainto_tsquery('english', $3)"), + "{}", + frag.sql + ); + assert_eq!(frag.params.len(), 1); + } + + #[test] + fn text_or_list_placeholders_are_gap_free() { + // Two terms OR together, and a blank one must not consume a placeholder + // number it never binds — the caller binds this fragment's params + // consecutively. + let query = SearchQuery::new("Patient").with_parameter(special_param( + "_text", + vec![ + SearchValue::eq(" "), + SearchValue::eq("fracture"), + SearchValue::eq("sprain"), + ], + )); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("_text OR-list should produce a condition"); + + assert_eq!(frag.params.len(), 2, "the blank term binds nothing"); + assert!(frag.sql.contains("$3"), "{}", frag.sql); + assert!(frag.sql.contains("$4"), "{}", frag.sql); + assert!( + !frag.sql.contains("$5"), + "placeholder numbering must be gap-free: {}", + frag.sql + ); + assert!(frag.sql.contains(" OR "), "{}", frag.sql); + } + + #[test] + fn blank_text_term_fails_closed() { + // Dropping the parameter would return the whole resource type, which is + // the exact failure mode being fixed. + let query = SearchQuery::new("Patient") + .with_parameter(special_param("_text", vec![SearchValue::eq("")])); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("a blank _text must still constrain the query"); + + assert_eq!(frag.sql, "FALSE"); + assert!(frag.params.is_empty()); + } + #[test] fn token_or_list_placeholders_are_gap_free() { // Regression: `status=finished,in-progress` — two code-only token values, diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index 3fa7464a5..317593a27 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -3330,6 +3330,19 @@ impl ReindexTarget for PostgresBackend { .index_contained_resources(&client, tenant_id, resource_type, resource_id, content) .await?; + // Rebuild the full-text row as well. `run_reindex` deletes each + // resource's search entries first (`delete_search_entries` -> + // `delete_search_index`), and that drops the `resource_fts` row; without + // this call nothing put it back, so `$reindex` silently disabled + // `_text`/`_content` on every reindex, with or without `clear_existing`. + // Same defect as the SQLite side — this is not PostgreSQL-specific. + // + // Not counted in `count`, which reports `search_index` entries only. + // `index_fts_content` is DELETE-then-INSERT here, so it is idempotent + // regardless of what ran before it. + self.index_fts_content(&client, tenant_id, resource_type, resource_id, content) + .await?; + Ok(count) } @@ -3397,10 +3410,29 @@ impl SearchableContent { } /// Extracts searchable text content from a FHIR resource. +/// +/// `full_content` backs `_content`, which FHIR defines as a search over *the +/// entire content of the resource* — so it must be a superset of `_text`, the +/// narrative-only search. `collect_strings` skips the `div` key deliberately, to +/// keep raw XHTML markup (`div`, `p`, `xmlns`, attribute values) out of the +/// index; that left the narrative missing from `_content` altogether, so a term +/// that appeared only in `text.div` was findable through `_text` and invisible +/// to `_content`. Appending the already-stripped narrative restores the +/// superset relationship without indexing the markup, and matches SQLite, which +/// has always had the narrative in `_content`. `data` stays excluded — base64 +/// attachment blobs are not text. fn extract_searchable_content(resource: &Value) -> SearchableContent { + let narrative = extract_narrative(resource); + let mut full_content = extract_all_strings(resource); + if !narrative.is_empty() { + if !full_content.is_empty() { + full_content.push(' '); + } + full_content.push_str(&narrative); + } SearchableContent { - narrative: extract_narrative(resource), - full_content: extract_all_strings(resource), + narrative, + full_content, } } @@ -3464,3 +3496,85 @@ fn collect_strings(value: &Value, parts: &mut Vec) { _ => {} } } + +#[cfg(test)] +mod fts_extraction_tests { + use super::*; + use serde_json::json; + + fn patient_with_narrative() -> Value { + json!({ + "resourceType": "Patient", + "name": [{"family": "Purgetest"}], + "text": { + "status": "generated", + "div": "

Assessment: \ + Zebracrossingdiagnosis.

" + }, + "photo": [{"contentType": "image/png", "data": "iVBORw0KGgoAAAANSUhEUg=="}] + }) + } + + #[test] + fn content_includes_the_narrative() { + // Regression: `_content` is "the entire content of the resource", so a + // term that only appears in the narrative must be reachable through it. + // `collect_strings` skips the `div` key, which used to drop the + // narrative from `full_content` entirely — `_text` found the term, + // `_content` did not. + let content = extract_searchable_content(&patient_with_narrative()); + + assert!( + content.narrative.contains("Zebracrossingdiagnosis"), + "narrative: {}", + content.narrative + ); + assert!( + content.full_content.contains("Zebracrossingdiagnosis"), + "_content must be a superset of _text: {}", + content.full_content + ); + assert!( + content.full_content.contains("Purgetest"), + "the non-narrative fields must still be there: {}", + content.full_content + ); + } + + #[test] + fn content_excludes_markup_and_binary_payloads() { + // The narrative goes in stripped, not raw: tag and attribute noise would + // make every resource match `xmlns` or `div`. Base64 attachment data + // stays out for the same reason plus index size. + let content = extract_searchable_content(&patient_with_narrative()); + + assert!( + !content.full_content.contains("xmlns"), + "{}", + content.full_content + ); + assert!( + !content.full_content.contains("

"), + "{}", + content.full_content + ); + assert!( + !content.full_content.contains("iVBORw0KGgo"), + "{}", + content.full_content + ); + } + + #[test] + fn narrative_only_resource_is_not_empty() { + // `index_fts_content` returns early on `is_empty()`; a resource whose + // only text is its narrative must still be indexed. + let content = extract_searchable_content(&json!({ + "resourceType": "Binary", + "text": {"div": "

Solitary

"} + })); + + assert!(!content.is_empty()); + assert!(content.full_content.contains("Solitary")); + } +} diff --git a/crates/persistence/src/backends/sqlite/schema.rs b/crates/persistence/src/backends/sqlite/schema.rs index 86fcaba60..ae3d0dbf1 100644 --- a/crates/persistence/src/backends/sqlite/schema.rs +++ b/crates/persistence/src/backends/sqlite/schema.rs @@ -5,7 +5,7 @@ use rusqlite::Connection; use crate::error::StorageResult; /// Current schema version. -pub const SCHEMA_VERSION: i32 = 14; +pub const SCHEMA_VERSION: i32 = 15; /// Initialize the database schema. pub fn initialize_schema(conn: &Connection) -> StorageResult<()> { @@ -295,6 +295,7 @@ fn migrate_schema(conn: &Connection, from_version: i32) -> StorageResult<()> { 11 => migrate_v11_to_v12(conn)?, 12 => migrate_v12_to_v13(conn)?, 13 => migrate_v13_to_v14(conn)?, + 14 => migrate_v14_to_v15(conn)?, _ => { return Err(crate::error::StorageError::Backend( crate::error::BackendError::Internal { @@ -375,8 +376,13 @@ fn migrate_v2_to_v3(conn: &Connection) -> StorageResult<()> { return Ok(()); } - // Create the FTS5 virtual table for full-text search - // Uses external content mode for smaller index size + // Create the FTS5 virtual table for full-text search. + // + // NOTE: this is a plain fts5 table, NOT external-content mode — there is no + // `content=` clause. It therefore keeps a full second copy of every + // resource's narrative and serialized body in its `resource_fts_content` + // shadow table. That matters when reasoning about where purged PHI lives: + // deleting the `resources` row does not reach this copy (see issue #386). conn.execute( "CREATE VIRTUAL TABLE IF NOT EXISTS resource_fts USING fts5( resource_id UNINDEXED, @@ -1258,6 +1264,61 @@ fn migrate_v13_to_v14(conn: &Connection) -> StorageResult<()> { ensure_tenants_table(conn) } +/// Migrate from schema version 14 to version 15. +/// +/// Sweeps `resource_fts` rows orphaned by the pre-v15 purge paths (issue #386). +/// +/// `resource_fts` is an FTS5 *virtual* table, so it can carry no foreign key and +/// the `ON DELETE CASCADE` from `resources` never reached it — and no purge path +/// deleted from it explicitly. Every database that has ever served `$purge`, +/// type-level `$purge`, or a tenant purge is therefore still holding the +/// narrative text and the complete serialized body of resources an operator was +/// told had been removed. The code fix stops new orphans; this removes the ones +/// already on disk, which is the half that matters to anyone running today. +/// +/// Soft-deleted resources keep their `resources` row, so `NOT EXISTS` correctly +/// preserves their entries — matching current behaviour, where a soft delete +/// does not drop the FTS row. +/// +/// The single statement runs in its own implicit transaction. It scans +/// `resource_fts` once (unavoidable: its key columns are `UNINDEXED`, and an +/// FTS5 virtual table admits no auxiliary index) and probes `resources` by +/// primary key. +fn migrate_v14_to_v15(conn: &Connection) -> StorageResult<()> { + // FTS5 is optional at compile time; a database built without it has no + // table to sweep. + let fts_exists: bool = conn + .query_row( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='resource_fts'", + [], + |_| Ok(true), + ) + .unwrap_or(false); + if !fts_exists { + return Ok(()); + } + + let swept = conn + .execute( + "DELETE FROM resource_fts + WHERE NOT EXISTS ( + SELECT 1 FROM resources r + WHERE r.tenant_id = resource_fts.tenant_id + AND r.resource_type = resource_fts.resource_type + AND r.id = resource_fts.resource_id)", + [], + ) + .map_err(|e| migration_err(format!("v15 resource_fts orphan sweep: {e}")))?; + + if swept > 0 { + tracing::info!( + orphaned_fts_rows = swept, + "Swept full-text rows left behind by pre-v15 purges (issue #386)" + ); + } + Ok(()) +} + fn migration_err(message: String) -> crate::error::StorageError { crate::error::StorageError::Backend(crate::error::BackendError::Internal { backend_name: "sqlite".to_string(), diff --git a/crates/persistence/src/backends/sqlite/search/query_builder.rs b/crates/persistence/src/backends/sqlite/search/query_builder.rs index c64b89036..aa35b4244 100644 --- a/crates/persistence/src/backends/sqlite/search/query_builder.rs +++ b/crates/persistence/src/backends/sqlite/search/query_builder.rs @@ -542,7 +542,15 @@ impl QueryBuilder { let mut conditions = Vec::new(); - for (i, value) in values.iter().enumerate() { + // Numbered off the running count of *accepted* terms rather than the + // loop index: escaping can empty a term (`_text=*` escapes to nothing), + // and the caller binds this fragment's params consecutively from + // `param_offset`, so a skipped value must not leave a placeholder gap. + // `_text=*,fracture` used to emit `?4` while binding only one param, + // which SQLite rejects at prepare time. + let mut param_num = param_offset; + + for value in values { // Escape and prepare the search term let search_term = Fts5Search::escape_fts_query(&value.value); if search_term.is_empty() { @@ -551,7 +559,7 @@ impl QueryBuilder { // Build the FTS match query // Use the column prefix to search only the specified column - let param_num = param_offset + i + 1; + param_num += 1; // `tenant_id = ?1` is not optional. Without it this sub-select // matches *every* tenant's `resource_fts` rows and yields a bare // `resource_id` set, which the outer query then intersects with this @@ -571,7 +579,10 @@ impl QueryBuilder { } if conditions.is_empty() { - return None; + // Every term escaped to nothing. Returning `None` would drop the + // parameter and answer a full-text query with every resource of the + // type; a term that cannot match must match nothing instead. + return Some(SqlFragment::new("0")); } // OR together multiple search terms @@ -901,6 +912,39 @@ mod tests { assert!(combined.sql.contains("OR")); } + #[test] + fn fts_placeholders_are_gap_free() { + // `*` escapes to nothing and binds no parameter, so the term after it + // must still take the next consecutive placeholder. The old index-based + // numbering emitted `?4` while binding a single param, and SQLite + // rejects the prepared statement. + let builder = QueryBuilder::new("tenant1", "Patient"); + let values = vec![SearchValue::eq("*"), SearchValue::eq("fracture")]; + + let frag = builder + .build_fts_condition(&values, "narrative_text", 2) + .expect("a matchable term must produce a condition"); + + assert_eq!(frag.params.len(), 1); + assert!(frag.sql.contains("?3"), "{}", frag.sql); + assert!(!frag.sql.contains("?4"), "{}", frag.sql); + } + + #[test] + fn fts_unmatchable_term_fails_closed() { + // Nothing survives escaping. Dropping the parameter would answer a + // full-text query with every resource of the type. + let builder = QueryBuilder::new("tenant1", "Patient"); + let values = vec![SearchValue::eq("*")]; + + let frag = builder + .build_fts_condition(&values, "narrative_text", 2) + .expect("an unmatchable term must still constrain the query"); + + assert_eq!(frag.sql, "0"); + assert!(frag.params.is_empty()); + } + #[test] fn test_query_builder_basic() { let builder = QueryBuilder::new("tenant1", "Patient"); diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index 7ca510afa..fcfd3ae78 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -42,6 +42,58 @@ fn serialization_error(message: String) -> StorageError { StorageError::Backend(BackendError::SerializationError { message }) } +/// Whether the optional `resource_fts` FTS5 virtual table exists on this +/// database. +/// +/// FTS5 is an optional SQLite compile-time feature. `create_fts_table` +/// (`schema.rs`) succeeds silently when it is absent, so `resource_fts` may +/// legitimately not exist — and a database created by an FTS5-less build keeps +/// no table even once reopened by an FTS5-capable one, because the `v2 -> v3` +/// migration is already recorded as done. +/// +/// Callers therefore probe before touching the table, and treat "table absent" +/// (a determinate fact: there is nothing indexed, so nothing to erase) very +/// differently from "the statement failed" (an unknown, which must never be +/// swallowed on a purge path — see the `DELETE FROM resource_fts` call sites). +/// +/// Cheap: `sqlite_master` is answered from the connection's in-memory schema +/// cache and does not touch disk. +pub(crate) fn fts_table_exists(conn: &rusqlite::Connection) -> StorageResult { + use rusqlite::OptionalExtension; + + conn.query_row( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='resource_fts'", + [], + |_| Ok(()), + ) + .optional() + .map(|found| found.is_some()) + .map_err(|e| internal_error(format!("Failed to probe for resource_fts: {e}"))) +} + +/// Runs a `DELETE FROM resource_fts …` on a purge path, skipping it when FTS5 +/// is unavailable and propagating any other failure. +/// +/// The error handling is the point. Elsewhere in this backend an FTS delete is +/// best-effort (`let _ = …`), which is tolerable on an index-maintenance path +/// where the worst case is a stale entry. On a *purge* path it is not: a +/// swallowed failure means the API answers `200` — "the record is gone" — while +/// the resource's full text is still on disk. That is a false erasure +/// attestation, and it is the same class of defect issue #386 reports, just +/// reached by a different route. So: probe, then be strict. +fn purge_fts_rows( + conn: &rusqlite::Connection, + sql: &str, + params: impl rusqlite::Params, +) -> StorageResult<()> { + if !fts_table_exists(conn)? { + return Ok(()); + } + conn.execute(sql, params) + .map_err(|e| internal_error(format!("purge fts delete: {e}")))?; + Ok(()) +} + /// Extracts the `value[x]` payload from a FHIRPath Patch `Parameters.part` /// entry whose `name` is `"value"`. Returns the value of the first key /// matching `value[A-Z]…` (e.g. `valueString`, `valueQuantity`, @@ -900,8 +952,14 @@ impl ResourceStorage for SqliteBackend { async fn purge_tenant_data(&self, id: &str) -> StorageResult { let mut conn = self.get_connection()?; + // IMMEDIATE, not the DEFERRED default: this transaction reads (the count + // below) before it writes. Under WAL a deferred transaction takes a read + // snapshot on that first read and then fails the read-to-write upgrade + // with SQLITE_BUSY_SNAPSHOT if another connection committed in between — + // and the busy handler is *not* invoked for that code, so the configured + // busy_timeout does not cover it. Taking the write lock up front does. let tx = conn - .transaction() + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) .map_err(|e| internal_error(format!("purge begin: {e}")))?; // Count current-version rows first so we can report what was removed. let removed: i64 = tx @@ -913,6 +971,8 @@ impl ResourceStorage for SqliteBackend { .map_err(|e| internal_error(format!("purge count: {e}")))?; // search_index has ON DELETE CASCADE from resources, but delete it // explicitly too in case foreign keys are not enforced on this handle. + // (That explicit delete is also what fires the `search_index_fts` + // triggers, though a cascade fires them too.) for sql in [ "DELETE FROM search_index WHERE tenant_id = ?1", "DELETE FROM resource_history WHERE tenant_id = ?1", @@ -921,6 +981,22 @@ impl ResourceStorage for SqliteBackend { tx.execute(sql, params![id]) .map_err(|e| internal_error(format!("purge delete: {e}")))?; } + // `resource_fts` is an FTS5 *virtual* table: it can carry no foreign key, + // so the cascade above never reaches it and it must be deleted explicitly + // (issue #386). Left behind, the purged resource's narrative and its + // entire serialized body stay in the database, and are resurrected as a + // match oracle the moment a resource reuses the same logical id. + // + // Deliberately NOT gated on `is_search_offloaded()`: that flag is a + // write-path optimisation ("don't maintain an index nobody reads"), + // whereas this is an erasure guarantee. A deployment that indexed + // locally and later moved search to Elasticsearch still has rows here, + // and a purge is precisely when they must go. + purge_fts_rows( + &tx, + "DELETE FROM resource_fts WHERE tenant_id = ?1", + params![id], + )?; // Per-user settings are keyed by user, not tenant, so they are not swept // by the deletes above — but a client stores PHI-derived query strings in // them, which belong to this tenant (issue #313). Same transaction: this @@ -1117,16 +1193,7 @@ impl SqliteBackend { ) -> StorageResult<()> { use super::search::fts::extract_searchable_content; - // Check if FTS table exists (created in schema v3) - let fts_exists: bool = conn - .query_row( - "SELECT 1 FROM sqlite_master WHERE type='table' AND name='resource_fts'", - [], - |_| Ok(true), - ) - .unwrap_or(false); - - if !fts_exists { + if !fts_table_exists(conn)? { // FTS5 not available - skip silently return Ok(()); } @@ -2357,11 +2424,23 @@ impl PurgableStorage for SqliteBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - let conn = self.get_connection()?; + let mut conn = self.get_connection()?; let tenant_id = tenant.tenant_id().as_str(); + // One transaction for the whole purge. Previously these were four + // independent autocommit statements, so a failure (or a crash) partway + // through left the resource deleted but its full text still in + // `resource_fts` — the exact orphan state this method now exists to + // prevent — and the caller could not tell a partial purge from a failed + // one, because a retry hits the not-found guard below and reports + // `NotFound` while the residue remains. IMMEDIATE for the same + // read-then-write / WAL reason as `purge_tenant_data`. + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("purge begin: {e}")))?; + // Check if resource exists (in any state) - let exists: bool = conn + let exists: bool = tx .query_row( "SELECT 1 FROM resources WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], @@ -2371,7 +2450,7 @@ impl PurgableStorage for SqliteBackend { if !exists { // Also check history in case it was already purged from main table - let history_exists: bool = conn + let history_exists: bool = tx .query_row( "SELECT 1 FROM resource_history WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], @@ -2388,35 +2467,51 @@ impl PurgableStorage for SqliteBackend { } // Delete from resources table - conn.execute( + tx.execute( "DELETE FROM resources WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], ) .map_err(|e| internal_error(format!("Failed to purge resource: {}", e)))?; // Delete from history table - conn.execute( + tx.execute( "DELETE FROM resource_history WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], ) .map_err(|e| internal_error(format!("Failed to purge resource history: {}", e)))?; // Delete from search index - conn.execute( + tx.execute( "DELETE FROM search_index WHERE tenant_id = ?1 AND resource_type = ?2 AND resource_id = ?3", params![tenant_id, resource_type, id], ) .map_err(|e| internal_error(format!("Failed to purge search index: {}", e)))?; + // Delete the full-text rows: no cascade reaches an FTS5 virtual table. + // See `purge_tenant_data` for why this is strict and ungated. + purge_fts_rows( + &tx, + "DELETE FROM resource_fts WHERE tenant_id = ?1 AND resource_type = ?2 AND resource_id = ?3", + params![tenant_id, resource_type, id], + )?; + + tx.commit() + .map_err(|e| internal_error(format!("purge commit: {e}")))?; + Ok(()) } async fn purge_all(&self, tenant: &TenantContext, resource_type: &str) -> StorageResult { - let conn = self.get_connection()?; + let mut conn = self.get_connection()?; let tenant_id = tenant.tenant_id().as_str(); + // Single transaction — see `purge` for the rationale. + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("purge_all begin: {e}")))?; + // Count how many we're about to delete - let count: i64 = conn + let count: i64 = tx .query_row( "SELECT COUNT(DISTINCT id) FROM resources WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], @@ -2425,26 +2520,37 @@ impl PurgableStorage for SqliteBackend { .unwrap_or(0); // Delete from resources table - conn.execute( + tx.execute( "DELETE FROM resources WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], ) .map_err(|e| internal_error(format!("Failed to purge resources: {}", e)))?; // Delete from history table - conn.execute( + tx.execute( "DELETE FROM resource_history WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], ) .map_err(|e| internal_error(format!("Failed to purge resource history: {}", e)))?; // Delete from search index - conn.execute( + tx.execute( "DELETE FROM search_index WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], ) .map_err(|e| internal_error(format!("Failed to purge search index: {}", e)))?; + // Delete the full-text rows: no cascade reaches an FTS5 virtual table. + // See `purge_tenant_data` for why this is strict and ungated. + purge_fts_rows( + &tx, + "DELETE FROM resource_fts WHERE tenant_id = ?1 AND resource_type = ?2", + params![tenant_id, resource_type], + )?; + + tx.commit() + .map_err(|e| internal_error(format!("purge_all commit: {e}")))?; + Ok(count as u64) } } @@ -3699,6 +3805,29 @@ impl ReindexTarget for SqliteBackend { content, )?; + // Rebuild the full-text row as well. Without this, `$reindex` was a + // *destructive* operation for `_text`/`_content`: `run_reindex` deletes + // each resource's search entries via `delete_search_entries` -> + // `delete_search_index`, which does drop the FTS row, and nothing here + // put it back. That happened on every reindex, with or without + // `clear_existing`, so the documented recovery operation silently + // disabled full-text search until each resource was next written. + // + // Not counted in `count`: that value is the number of `search_index` + // entries, which `$reindex-status` reports, and an FTS row is not one. + // + // Safe against duplicates because `run_reindex` always calls + // `delete_search_entries` for the resource immediately before this, and + // SQLite's `index_fts_content` is a bare INSERT with no delete-first. + // If that ordering ever changes, this must become delete-then-insert. + self.index_fts_content( + &conn, + tenant.tenant_id().as_str(), + resource_type, + resource_id, + content, + )?; + Ok(count) } @@ -3713,6 +3842,19 @@ impl ReindexTarget for SqliteBackend { ) .map_err(|e| internal_error(format!("Failed to clear search index: {}", e)))?; + // Clear the full-text rows too, matching PostgreSQL. This is only sound + // because `write_search_entries` above now repopulates them; adding this + // delete on its own would have made `$reindex --clear-existing` wipe + // `_text`/`_content` permanently. + // + // The returned count deliberately stays the `search_index` total — + // callers and tests treat it as the number of index entries cleared. + purge_fts_rows( + &conn, + "DELETE FROM resource_fts WHERE tenant_id = ?1", + params![tenant_id], + )?; + Ok(deleted as u64) } } diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 72674e123..ff52a9dba 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -21,6 +21,20 @@ use helios_persistence::core::BackendKind; #[path = "transactions/if_match_suite.rs"] mod if_match_suite; +/// The backend-agnostic full-text purge-completeness scenarios (issue #386), +/// shared verbatim with the SQLite suite that owns the file. +/// +/// PostgreSQL already deleted `resource_fts` in its purge paths — the defect was +/// SQLite-only — so these lock the *reference* backend's behaviour in place so a +/// future change cannot silently regress it. The `$reindex` scenarios are a +/// different matter: those failed on PostgreSQL too, because +/// `write_search_entries` never rebuilt the full-text row. +/// +/// Declared at the top level for the same `#[path]` resolution reason as +/// `if_match_suite` above. +#[path = "search/fts_purge_suite.rs"] +mod fts_purge_suite; + // ============================================================================ // Backend Configuration Tests (no PostgreSQL instance required) // ============================================================================ @@ -5215,4 +5229,149 @@ mod postgres_integration { postgres_integration_transaction_delete_accepts_matching_if_match, transaction_delete_accepts_matching_if_match ); + + // ======================================================================== + // Full-text purge completeness (issue #386) + // ======================================================================== + + use crate::fts_purge_suite::{self as fts_suite, FtsProbe}; + + /// Reads `resource_fts` directly over the backend's own pool. + /// + /// `PostgresBackend::get_client` is `#[doc(hidden)] pub` precisely so + /// out-of-crate tests can run raw SQL; other tests in this module already + /// do the same. + struct PgFtsProbe(PostgresBackend); + + #[async_trait::async_trait] + impl FtsProbe for PgFtsProbe { + async fn fts_row_count(&self, tenant_id: &str) -> u64 { + let client = self.0.get_client().await.expect("get_client"); + let row = client + .query_one( + "SELECT COUNT(*)::bigint FROM resource_fts WHERE tenant_id = $1", + &[&tenant_id], + ) + .await + .expect("count resource_fts"); + row.get::<_, i64>(0) as u64 + } + + async fn fts_rows_containing(&self, needle: &str) -> u64 { + let client = self.0.get_client().await.expect("get_client"); + let pattern = format!("%{needle}%"); + let row = client + .query_one( + "SELECT COUNT(*)::bigint FROM resource_fts \ + WHERE full_content LIKE $1 OR narrative_text LIKE $1", + &[&pattern], + ) + .await + .expect("count resource_fts by content"); + row.get::<_, i64>(0) as u64 + } + } + + /// One `#[tokio::test]` per shared scenario, each on its own UUID-suffixed + /// tenant so they cannot collide on the shared container. + macro_rules! pg_fts_test { + ($name:ident, $scenario:ident) => { + #[tokio::test] + async fn $name() { + let backend = create_backend().await; + let probe = PgFtsProbe(create_backend().await); + let tenant = create_tenant(stringify!($scenario)); + fts_suite::$scenario(&backend, &probe, &tenant).await; + } + }; + } + + pg_fts_test!( + postgres_integration_purge_removes_fts_rows, + purge_removes_fts_rows + ); + pg_fts_test!( + postgres_integration_purge_all_removes_fts_rows, + purge_all_removes_fts_rows + ); + pg_fts_test!( + postgres_integration_purge_tenant_data_removes_fts_rows, + purge_tenant_data_removes_fts_rows + ); + pg_fts_test!( + postgres_integration_reuse_after_purge_does_not_resurrect_narrative, + reuse_after_purge_does_not_resurrect_narrative + ); + pg_fts_test!( + postgres_integration_tenant_reuse_does_not_resurrect_narrative, + tenant_reuse_does_not_resurrect_narrative + ); + pg_fts_test!( + postgres_integration_repeated_purge_and_recreate_does_not_grow_fts, + repeated_purge_and_recreate_does_not_grow_fts + ); + + #[tokio::test] + async fn postgres_integration_purge_tenant_data_leaves_other_tenants_intact() { + let backend = create_backend().await; + let probe = PgFtsProbe(create_backend().await); + fts_suite::purge_tenant_data_leaves_other_tenants_intact( + &backend, + &probe, + &create_tenant("fts_victim"), + &create_tenant("fts_bystander"), + ) + .await; + } + + /// `$reindex` must rebuild full-text search, not destroy it. + /// + /// This failed on PostgreSQL before the fix, in both modes: `run_reindex` + /// drops each resource's `resource_fts` row via `delete_search_entries`, and + /// `write_search_entries` never put it back. + #[tokio::test] + async fn postgres_integration_reindex_preserves_full_text_search_without_clear() { + pg_reindex_case(false, "fts_reindex_noclear").await; + } + + #[tokio::test] + async fn postgres_integration_reindex_preserves_full_text_search_with_clear() { + pg_reindex_case(true, "fts_reindex_clear").await; + } + + async fn pg_reindex_case(clear_existing: bool, tenant_label: &str) { + use helios_persistence::search::ReindexOperation; + use std::sync::Arc; + + let backend = Arc::new(create_backend().await); + let probe = PgFtsProbe(create_backend().await); + let tenant = create_tenant(tenant_label); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + fts_suite::reindex_preserves_full_text_search( + backend.as_ref(), + &probe, + &tenant, + &reindex, + clear_existing, + ) + .await; + } + + #[tokio::test] + async fn postgres_integration_repeated_reindex_does_not_duplicate_fts_rows() { + use helios_persistence::search::ReindexOperation; + use std::sync::Arc; + + let backend = Arc::new(create_backend().await); + let probe = PgFtsProbe(create_backend().await); + let tenant = create_tenant("fts_reindex_repeat"); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + fts_suite::repeated_reindex_does_not_duplicate_fts_rows( + backend.as_ref(), + &probe, + &tenant, + &reindex, + ) + .await; + } } diff --git a/crates/persistence/tests/search/fts_purge_suite.rs b/crates/persistence/tests/search/fts_purge_suite.rs new file mode 100644 index 000000000..e6577e1a6 --- /dev/null +++ b/crates/persistence/tests/search/fts_purge_suite.rs @@ -0,0 +1,643 @@ +//! Backend-agnostic full-text purge-completeness suite (issue #386). +//! +//! Every scenario is generic over the storage traits, so the *same* assertions +//! run on SQLite and PostgreSQL rather than being retyped per engine. Like +//! `transactions/if_match_suite.rs`, this file is `#[path]`-included by each +//! backend's test binary rather than living in `tests/common/`, which no test +//! target declares and cargo therefore never compiles (issue #306). +//! +//! ## Why these tests assert on raw rows, not on search results +//! +//! The obvious regression test — "purge a resource, then check `_text` finds +//! nothing" — **passes on the unfixed code**, and is therefore worthless. +//! +//! SQLite compiles `_text`/`_content` into a sub-filter: +//! `resource_id IN (SELECT resource_id FROM resource_fts WHERE tenant_id = ?1 +//! AND … MATCH ?n)`, AND-ed onto an outer query that selects from +//! `search_index`. Purge *does* clear `search_index`, so an orphaned +//! `resource_fts` row is unreachable through search while the resource's +//! narrative and complete serialized body are still on disk. +//! +//! So each scenario asserts two independent things: +//! +//! 1. **Residency** — a direct `SELECT COUNT(*) FROM resource_fts` via +//! [`FtsProbe`], which bypasses every query path and answers "is the PHI +//! still there", not "can a search reach it". +//! 2. **Exploitability** — the id-reuse oracle. Purge a resource, create a new +//! one reusing the same logical id, and the *old* resource's text becomes +//! matchable again and is attributed to the new resource. This is the +//! user-visible harm in the issue, and unlike (1) it fails through the +//! public search API. +//! +//! Every scenario also opens with a **positive control** asserting the row was +//! indexed in the first place. Without it, a broken indexer, a typo'd tenant, a +//! missing feature, or a renamed parameter would sail straight through the +//! post-conditions and the test would pass while proving nothing. +//! +//! ## Shared-database backends +//! +//! The PostgreSQL suite runs every scenario against one long-lived container +//! database, so scenarios must not collide. Each takes its own +//! [`TenantContext`]; callers must pass a **distinct tenant per scenario**. +//! +//! A distinct tenant is not enough on its own, because [`FtsProbe:: +//! fts_rows_containing`] is deliberately *not* tenant-scoped — that is the whole +//! point of it. Scenarios run concurrently under `cargo test`, and several end +//! with a live indexed row on purpose (the reindex scenarios, the bystander +//! tenant), so a term shared across scenarios makes the unqualified count +//! non-zero for reasons that have nothing to do with the purge under test. +//! [`planted_term`] therefore derives the token from the tenant id: the probe +//! stays global — it still catches a surviving row under *any* tenant, type or +//! id — while only ever counting rows this scenario planted. + +#![allow(dead_code)] + +use serde_json::{Value, json}; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{PurgableStorage, ResourceStorage, SearchProvider}; +use helios_persistence::tenant::TenantContext; +use helios_persistence::types::{SearchParamType, SearchParameter, SearchQuery, SearchValue}; + +/// Stem of the token planted in narrative text. Rare and unambiguous, so a +/// match can only come from a resource this suite created, never from seeded +/// conformance data or another test's fixtures. +const PLANTED_STEM: &str = "Zebracrossingdiagnosis"; + +/// Stem of the second planted token, for the resource that must survive. +const BYSTANDER_STEM: &str = "Quokkaflavoured"; + +/// The token this scenario plants, unique to `tenant`. +/// +/// See the module docs: the residency probe counts rows across *all* tenants, so +/// the token — not the tenant predicate — is what keeps concurrent scenarios on +/// a shared database from reading as each other's leaks. +pub fn planted_term(tenant: &TenantContext) -> String { + scoped_term(PLANTED_STEM, tenant) +} + +/// The bystander token for `tenant`, unique on the same basis as +/// [`planted_term`]. +pub fn bystander_term(tenant: &TenantContext) -> String { + scoped_term(BYSTANDER_STEM, tenant) +} + +/// Appends the tenant id to `stem` as a single indexable word. +/// +/// Non-alphanumerics are dropped rather than kept: both engines' tokenizers +/// split on `-` and `_`, which would turn one rare token into several common +/// ones and break the exact-match assertions. What remains is one long word that +/// FTS5 and `to_tsvector('english', …)` both keep intact, and that a `LIKE +/// '%…%'` residency probe matches literally. +fn scoped_term(stem: &str, tenant: &TenantContext) -> String { + let suffix: String = tenant + .tenant_id() + .as_str() + .chars() + .filter(|c| c.is_ascii_alphanumeric()) + .collect(); + format!("{stem}{suffix}") +} + +/// Reads physical `resource_fts` rows, bypassing every query path. +/// +/// This is what distinguishes "the PHI is gone" from "a search cannot currently +/// reach the PHI". Both backends can implement it from an integration-test +/// target without any production API change: PostgreSQL exposes +/// `#[doc(hidden)] pub get_client`, and SQLite can be pointed at a file-backed +/// database that the test opens a second `rusqlite` connection onto. +#[async_trait::async_trait] +pub trait FtsProbe { + /// Number of `resource_fts` rows held for `tenant_id`. + async fn fts_row_count(&self, tenant_id: &str) -> u64; + + /// Number of `resource_fts` rows anywhere whose stored text contains + /// `needle`, **with no tenant predicate**. + /// + /// The unqualified form is what catches a `DELETE` whose `WHERE` clause is + /// wrong in a way a tenant-scoped count would hide. + async fn fts_rows_containing(&self, needle: &str) -> u64; +} + +/// A Patient carrying `term` in its generated narrative, which is what +/// `_text` indexes. +pub fn patient_with_narrative(id: &str, term: &str) -> Value { + json!({ + "resourceType": "Patient", + "id": id, + "text": { + "status": "generated", + "div": format!( + "

Assessment: {term}.

" + ) + }, + "name": [{"family": "Purgetest"}] + }) +} + +/// A Patient with no trace of the planted term, for id-reuse scenarios. +pub fn unrelated_patient(id: &str) -> Value { + json!({ + "resourceType": "Patient", + "id": id, + "text": { + "status": "generated", + "div": "

Routine review.

" + }, + "name": [{"family": "Replacement"}] + }) +} + +fn text_query(term: &str) -> SearchQuery { + SearchQuery::new("Patient").with_parameter(SearchParameter { + name: "_text".to_string(), + param_type: SearchParamType::Special, + modifier: None, + values: vec![SearchValue::eq(term)], + chain: vec![], + components: vec![], + }) +} + +fn content_query(term: &str) -> SearchQuery { + SearchQuery::new("Patient").with_parameter(SearchParameter { + name: "_content".to_string(), + param_type: SearchParamType::Special, + modifier: None, + values: vec![SearchValue::eq(term)], + chain: vec![], + components: vec![], + }) +} + +async fn text_hits(backend: &B, tenant: &TenantContext, term: &str) -> Vec +where + B: SearchProvider, +{ + backend + .search(tenant, &text_query(term)) + .await + .expect("_text search should succeed") + .resources + .items + .iter() + .map(|r| r.id().to_string()) + .collect() +} + +async fn content_hits(backend: &B, tenant: &TenantContext, term: &str) -> Vec +where + B: SearchProvider, +{ + backend + .search(tenant, &content_query(term)) + .await + .expect("_content search should succeed") + .resources + .items + .iter() + .map(|r| r.id().to_string()) + .collect() +} + +/// Seeds one Patient carrying this tenant's [`planted_term`] and asserts the +/// full-text pipeline actually indexed it. Every scenario starts here; if this +/// fails, nothing after it would have meant anything. +async fn seed_and_control(backend: &B, probe: &P, tenant: &TenantContext, id: &str) +where + B: ResourceStorage + SearchProvider, + P: FtsProbe, +{ + let term = planted_term(tenant); + backend + .create( + tenant, + "Patient", + patient_with_narrative(id, &term), + FhirVersion::default(), + ) + .await + .expect("seed create should succeed"); + + let rows = probe.fts_row_count(tenant.tenant_id().as_str()).await; + assert_eq!( + rows, 1, + "POSITIVE CONTROL: the seeded resource must be present in resource_fts \ + before the purge, or every assertion below is vacuous" + ); + assert_eq!( + text_hits(backend, tenant, &term).await, + vec![id.to_string()], + "POSITIVE CONTROL: _text must find the seeded narrative before the purge" + ); +} + +// =========================================================================== +// Residency: the purge paths must remove the physical rows +// =========================================================================== + +/// Single-resource `$purge` removes the resource's full-text row. +pub async fn purge_removes_fts_rows(backend: &B, probe: &P, tenant: &TenantContext) +where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge(tenant, "Patient", "p1") + .await + .expect("purge should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "purge must delete the resource's resource_fts row; a surviving row keeps \ + the narrative and the full serialized body on disk after the API \ + reported the resource was removed" + ); + assert_eq!( + probe.fts_rows_containing(&planted_term(tenant)).await, + 0, + "no resource_fts row anywhere may still contain the purged text" + ); +} + +/// Type-level `$purge` removes every matching resource's full-text row. +pub async fn purge_all_removes_fts_rows(backend: &B, probe: &P, tenant: &TenantContext) +where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge_all(tenant, "Patient") + .await + .expect("purge_all should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "purge_all must delete the type's resource_fts rows" + ); + assert_eq!(probe.fts_rows_containing(&planted_term(tenant)).await, 0); +} + +/// Tenant purge removes every one of the tenant's full-text rows, across types. +pub async fn purge_tenant_data_removes_fts_rows( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + let term = planted_term(tenant); + + backend + .create( + tenant, + "Observation", + json!({ + "resourceType": "Observation", + "id": "o1", + "status": "final", + "code": {"text": "vitals"}, + "text": { + "status": "generated", + "div": format!( + "

{term}

" + ) + } + }), + FhirVersion::default(), + ) + .await + .expect("second seed should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 2, + "POSITIVE CONTROL: both seeded resources must be indexed" + ); + + backend + .purge_tenant_data(tenant.tenant_id().as_str()) + .await + .expect("purge_tenant_data should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "purge_tenant_data must delete every one of the tenant's resource_fts \ + rows, across all resource types" + ); + assert_eq!(probe.fts_rows_containing(&term).await, 0); +} + +/// The purge must not be over-broad: another tenant's rows survive untouched. +/// +/// A `DELETE FROM resource_fts` that lost its tenant predicate would satisfy +/// every other assertion in this suite. This is the only scenario that catches +/// it. +pub async fn purge_tenant_data_leaves_other_tenants_intact( + backend: &B, + probe: &P, + victim: &TenantContext, + bystander: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, victim, "p1").await; + + backend + .create( + bystander, + "Patient", + patient_with_narrative("p1", &bystander_term(bystander)), + FhirVersion::default(), + ) + .await + .expect("bystander seed should succeed"); + assert_eq!( + probe.fts_row_count(bystander.tenant_id().as_str()).await, + 1, + "POSITIVE CONTROL: the bystander tenant must be indexed too" + ); + + backend + .purge_tenant_data(victim.tenant_id().as_str()) + .await + .expect("purge_tenant_data should succeed"); + + assert_eq!( + probe.fts_row_count(victim.tenant_id().as_str()).await, + 0, + "the purged tenant's rows must be gone" + ); + assert_eq!( + probe.fts_row_count(bystander.tenant_id().as_str()).await, + 1, + "purging one tenant must not touch another tenant's resource_fts rows" + ); + assert_eq!( + text_hits(backend, bystander, &bystander_term(bystander)).await, + vec!["p1".to_string()], + "the bystander tenant's full-text search must still work after the purge" + ); +} + +// =========================================================================== +// Exploitability: the id-reuse oracle +// =========================================================================== + +/// Purging a resource and then reusing its logical id must not resurrect the +/// purged narrative. +/// +/// This is the harm the issue describes, and it fails through the public search +/// API on unfixed code: the orphaned `resource_fts` row is matched by +/// `_text`, and because the sub-select intersects on `resource_id` alone, the +/// hit is attributed to the *new* resource. +pub async fn reuse_after_purge_does_not_resurrect_narrative( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge(tenant, "Patient", "p1") + .await + .expect("purge should succeed"); + + // Same logical id — the ordinary case for client-assigned ids, fixtures and + // reload workflows. + backend + .create( + tenant, + "Patient", + unrelated_patient("p1"), + FhirVersion::default(), + ) + .await + .expect("re-create should succeed"); + + assert_eq!( + text_hits(backend, tenant, &planted_term(tenant)).await, + Vec::::new(), + "the purged resource's narrative must not be matchable through a \ + re-created resource that merely reuses its id" + ); + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "only the replacement resource may be indexed; a count of 2 means the \ + purged generation's row is still on disk" + ); +} + +/// Same oracle, reached through a tenant purge and tenant-id reuse — the +/// scenario named in the issue. +pub async fn tenant_reuse_does_not_resurrect_narrative( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge_tenant_data(tenant.tenant_id().as_str()) + .await + .expect("purge_tenant_data should succeed"); + + // A tenant of the same id is provisioned again and stores its own data. + backend + .create( + tenant, + "Patient", + unrelated_patient("p1"), + FhirVersion::default(), + ) + .await + .expect("re-create under the reused tenant id should succeed"); + + assert_eq!( + text_hits(backend, tenant, &planted_term(tenant)).await, + Vec::::new(), + "a re-created tenant must not be able to surface the previous tenant's \ + purged content" + ); + assert_eq!( + content_hits(backend, tenant, &planted_term(tenant)).await, + Vec::::new(), + "_content must be clean too, not only _text" + ); + assert_eq!(probe.fts_row_count(tenant.tenant_id().as_str()).await, 1); +} + +/// Repeated create/purge cycles must not accumulate orphaned rows. +/// +/// Guards against unbounded growth of the FTS table — every purged generation +/// leaving a row behind is both a leak and a slow, permanent index bloat. +pub async fn repeated_purge_and_recreate_does_not_grow_fts( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + for cycle in 0..5 { + backend + .create( + tenant, + "Patient", + patient_with_narrative("p1", &planted_term(tenant)), + FhirVersion::default(), + ) + .await + .unwrap_or_else(|e| panic!("create on cycle {cycle} should succeed: {e}")); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "POSITIVE CONTROL on cycle {cycle}: exactly one row should be indexed" + ); + + backend + .purge(tenant, "Patient", "p1") + .await + .unwrap_or_else(|e| panic!("purge on cycle {cycle} should succeed: {e}")); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "cycle {cycle} left an orphaned resource_fts row behind" + ); + } +} + +// =========================================================================== +// $reindex must REBUILD full-text search, not destroy it +// =========================================================================== + +/// Drives a reindex job to completion, panicking on failure or timeout. +async fn run_reindex_to_completion( + reindex: &helios_persistence::search::ReindexOperation, + tenant: &TenantContext, + request: helios_persistence::search::ReindexRequest, +) { + use helios_persistence::search::ReindexStatus; + + let job_id = reindex + .start(tenant.clone(), request, None) + .await + .expect("reindex should start"); + + for _ in 0..200 { + tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; + let progress = reindex + .get_progress(&job_id) + .await + .expect("progress should be readable"); + match progress.status { + ReindexStatus::Completed => return, + ReindexStatus::Failed => { + panic!("reindex failed: {:?}", progress.error_message) + } + _ => {} + } + } + panic!("reindex timed out"); +} + +/// `$reindex` must leave `_text`/`_content` working. +/// +/// On unfixed code this fails on **both** backends and in **both** modes: +/// `run_reindex` calls `delete_search_entries` for every resource +/// unconditionally — which does drop the `resource_fts` row — while +/// `write_search_entries` never put it back. So the documented recovery +/// operation silently disabled full-text search until each resource happened to +/// be written again. `clear_existing` was never the discriminator. +pub async fn reindex_preserves_full_text_search( + backend: &B, + probe: &P, + tenant: &TenantContext, + reindex: &helios_persistence::search::ReindexOperation, + clear_existing: bool, +) where + B: ResourceStorage + SearchProvider, + P: FtsProbe, +{ + use helios_persistence::search::ReindexRequest; + + seed_and_control(backend, probe, tenant, "p1").await; + + let mut request = ReindexRequest::for_types(vec!["Patient"]); + if clear_existing { + request = request.clear_existing(); + } + run_reindex_to_completion(reindex, tenant, request).await; + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "$reindex (clear_existing={clear_existing}) must rebuild the resource's \ + resource_fts row, not drop it" + ); + assert_eq!( + text_hits(backend, tenant, &planted_term(tenant)).await, + vec!["p1".to_string()], + "_text must still work after $reindex (clear_existing={clear_existing})" + ); + assert_eq!( + content_hits(backend, tenant, &planted_term(tenant)).await, + vec!["p1".to_string()], + "_content must still work after $reindex (clear_existing={clear_existing})" + ); +} + +/// A reindex must not duplicate full-text rows. +/// +/// SQLite's `index_fts_content` is a bare INSERT with no delete-first; it is +/// only safe because `run_reindex` deletes the resource's entries immediately +/// beforehand. This pins that ordering — if it is ever changed, the row count +/// doubles here rather than silently bloating production indexes. +pub async fn repeated_reindex_does_not_duplicate_fts_rows( + backend: &B, + probe: &P, + tenant: &TenantContext, + reindex: &helios_persistence::search::ReindexOperation, +) where + B: ResourceStorage + SearchProvider, + P: FtsProbe, +{ + use helios_persistence::search::ReindexRequest; + + seed_and_control(backend, probe, tenant, "p1").await; + + for pass in 0..3 { + run_reindex_to_completion(reindex, tenant, ReindexRequest::for_types(vec!["Patient"])) + .await; + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "reindex pass {pass} must leave exactly one resource_fts row" + ); + } + + assert_eq!( + text_hits(backend, tenant, &planted_term(tenant)).await, + vec!["p1".to_string()], + "_text must still return exactly one hit after repeated reindexing" + ); +} diff --git a/crates/persistence/tests/search/fts_purge_tests.rs b/crates/persistence/tests/search/fts_purge_tests.rs new file mode 100644 index 000000000..a945d63ec --- /dev/null +++ b/crates/persistence/tests/search/fts_purge_tests.rs @@ -0,0 +1,417 @@ +//! SQLite bindings for the backend-agnostic full-text purge suite (issue #386). +//! +//! The scenarios live in `fts_purge_suite.rs` and are shared verbatim with the +//! PostgreSQL test binary; this file only supplies the SQLite backend, the +//! [`FtsProbe`] implementation, and one `#[tokio::test]` per scenario. +//! +//! ## Why these tests use a file-backed database +//! +//! Proving the fix requires reading `resource_fts` directly — a search-level +//! assertion is vacuous here (see the suite's module docs). `SqliteBackend`'s +//! connection pool is `pub(crate)` and unreachable from an integration test, and +//! the usual `:memory:` harness uses a private per-instance shared-cache name +//! that nothing else can attach to. A temporary file solves both without adding +//! a `#[doc(hidden)] pub` accessor to production code purely for tests. + +#![cfg(feature = "sqlite")] + +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use helios_persistence::backends::sqlite::{SqliteBackend, SqliteBackendConfig}; +use helios_persistence::search::ReindexOperation; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; + +use super::fts_purge_suite::{self as suite, FtsProbe}; + +/// A file-backed SQLite backend plus the path its data lives at. +/// +/// The `TempDir` is returned so the caller keeps it alive: dropping it deletes +/// the database out from under the backend. +fn file_backend() -> (SqliteBackend, tempfile::TempDir, PathBuf) { + let data_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(|p| p.parent()) + .map(|p| p.join("data")) + .unwrap_or_else(|| PathBuf::from("data")); + + let dir = tempfile::tempdir().expect("temp dir"); + let db_path = dir.path().join("fts_purge.db"); + + let config = SqliteBackendConfig { + data_dir: Some(data_dir), + ..Default::default() + }; + let backend = SqliteBackend::with_config(&db_path, config).expect("file-backed SQLite backend"); + backend.init_schema().expect("init schema"); + (backend, dir, db_path) +} + +fn tenant(id: &str) -> TenantContext { + TenantContext::new(TenantId::new(id), TenantPermissions::full_access()) +} + +/// Reads `resource_fts` over a second connection to the same database file. +struct SqliteFtsProbe(PathBuf); + +impl SqliteFtsProbe { + fn open(&self) -> rusqlite::Connection { + rusqlite::Connection::open(&self.0).expect("probe connection") + } +} + +#[async_trait::async_trait] +impl FtsProbe for SqliteFtsProbe { + async fn fts_row_count(&self, tenant_id: &str) -> u64 { + let conn = self.open(); + conn.query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = ?1", + [tenant_id], + |row| row.get::<_, i64>(0), + ) + .expect("resource_fts must exist in a bundled-rusqlite build") as u64 + } + + async fn fts_rows_containing(&self, needle: &str) -> u64 { + let conn = self.open(); + let pattern = format!("%{needle}%"); + conn.query_row( + "SELECT COUNT(*) FROM resource_fts \ + WHERE full_content LIKE ?1 OR narrative_text LIKE ?1", + [&pattern], + |row| row.get::<_, i64>(0), + ) + .expect("resource_fts must exist in a bundled-rusqlite build") as u64 + } +} + +/// FTS5 must be compiled into the test build. +/// +/// Without it `create_fts_table` silently creates nothing and every full-text +/// assertion in this suite would be testing an absent feature. `rusqlite` is +/// pinned with the `bundled` feature, whose amalgamation is built with +/// `-DSQLITE_ENABLE_FTS5`; this fails loudly if that ever changes. +#[test] +fn fts5_is_compiled_into_the_test_build() { + let conn = rusqlite::Connection::open_in_memory().expect("in-memory connection"); + let modules: i64 = conn + .query_row( + "SELECT COUNT(*) FROM pragma_module_list WHERE name = 'fts5'", + [], + |row| row.get(0), + ) + .expect("pragma_module_list should be queryable"); + assert_eq!( + modules, 1, + "FTS5 is absent from this SQLite build, so every _text/_content \ + assertion in this suite would be vacuous. rusqlite must keep its \ + `bundled` feature (crates/persistence/Cargo.toml)." + ); +} + +/// The FTS5 table really is created by `init_schema` on a fresh database. +#[test] +fn resource_fts_table_exists_after_init_schema() { + let (_backend, _dir, path) = file_backend(); + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + let found: i64 = conn + .query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='resource_fts'", + [], + |row| row.get(0), + ) + .expect("sqlite_master query"); + assert_eq!(found, 1, "init_schema must create resource_fts"); +} + +macro_rules! sqlite_fts_test { + ($name:ident, $scenario:ident) => { + #[tokio::test] + async fn $name() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + suite::$scenario(&backend, &probe, &tenant("acme")).await; + } + }; +} + +sqlite_fts_test!(sqlite_purge_removes_fts_rows, purge_removes_fts_rows); +sqlite_fts_test!( + sqlite_purge_all_removes_fts_rows, + purge_all_removes_fts_rows +); +sqlite_fts_test!( + sqlite_purge_tenant_data_removes_fts_rows, + purge_tenant_data_removes_fts_rows +); +sqlite_fts_test!( + sqlite_reuse_after_purge_does_not_resurrect_narrative, + reuse_after_purge_does_not_resurrect_narrative +); +sqlite_fts_test!( + sqlite_tenant_reuse_does_not_resurrect_narrative, + tenant_reuse_does_not_resurrect_narrative +); +sqlite_fts_test!( + sqlite_repeated_purge_and_recreate_does_not_grow_fts, + repeated_purge_and_recreate_does_not_grow_fts +); + +#[tokio::test] +async fn sqlite_purge_tenant_data_leaves_other_tenants_intact() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + suite::purge_tenant_data_leaves_other_tenants_intact( + &backend, + &probe, + &tenant("acme"), + &tenant("globex"), + ) + .await; +} + +#[tokio::test] +async fn sqlite_reindex_preserves_full_text_search_without_clear() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + let backend = Arc::new(backend); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + suite::reindex_preserves_full_text_search( + backend.as_ref(), + &probe, + &tenant("acme"), + &reindex, + false, + ) + .await; +} + +#[tokio::test] +async fn sqlite_reindex_preserves_full_text_search_with_clear() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + let backend = Arc::new(backend); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + suite::reindex_preserves_full_text_search( + backend.as_ref(), + &probe, + &tenant("acme"), + &reindex, + true, + ) + .await; +} + +#[tokio::test] +async fn sqlite_repeated_reindex_does_not_duplicate_fts_rows() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + let backend = Arc::new(backend); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + suite::repeated_reindex_does_not_duplicate_fts_rows( + backend.as_ref(), + &probe, + &tenant("acme"), + &reindex, + ) + .await; +} + +/// A purge must succeed on a database with no FTS5 table. +/// +/// FTS5 is an optional SQLite compile-time feature and `create_fts_table` +/// tolerates its absence, so the purge paths probe before deleting. Dropping the +/// table reproduces that state: purge must return `Ok`, not fail on +/// `no such table: resource_fts`. +#[tokio::test] +async fn sqlite_purge_succeeds_when_fts_table_is_absent() { + let (backend, _dir, path) = file_backend(); + let tenant = tenant("acme"); + + helios_persistence::core::ResourceStorage::create( + &backend, + &tenant, + "Patient", + suite::patient_with_narrative("p1", &suite::planted_term(&tenant)), + helios_fhir::FhirVersion::default(), + ) + .await + .expect("seed create"); + + drop_fts_table(&path); + + use helios_persistence::core::PurgableStorage; + backend + .purge(&tenant, "Patient", "p1") + .await + .expect("purge must tolerate a database without FTS5"); + backend + .purge_all(&tenant, "Patient") + .await + .expect("purge_all must tolerate a database without FTS5"); + helios_persistence::core::ResourceStorage::purge_tenant_data(&backend, "acme") + .await + .expect("purge_tenant_data must tolerate a database without FTS5"); +} + +fn drop_fts_table(path: &Path) { + let conn = rusqlite::Connection::open(path).expect("probe connection"); + conn.execute("DROP TABLE resource_fts", []) + .expect("drop resource_fts"); +} + +/// The `v14 -> v15` migration sweeps orphans left by the pre-fix purge paths. +/// +/// The code fix only stops *new* orphans. Every database that has already served +/// a purge is still holding the narrative and full body of resources its +/// operator was told were removed, so the fix is only half delivered without +/// this sweep. +/// +/// Reproduces that state directly — an FTS row whose `resources` row is gone, +/// which is exactly what the old `purge` left behind — then reopens the database +/// at the pre-migration version and asserts the orphan is swept while a live +/// resource's row is untouched. +#[tokio::test] +async fn migration_sweeps_orphaned_fts_rows_from_existing_databases() { + let (backend, _dir, path) = file_backend(); + let tenant = tenant("acme"); + + // One resource that stays, one that will be orphaned. + for id in ["keeper", "orphan"] { + helios_persistence::core::ResourceStorage::create( + &backend, + &tenant, + "Patient", + suite::patient_with_narrative(id, &suite::planted_term(&tenant)), + helios_fhir::FhirVersion::default(), + ) + .await + .expect("seed create"); + } + drop(backend); + + { + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + // Delete the resource WITHOUT touching resource_fts — precisely what the + // pre-fix purge paths did. + conn.execute( + "DELETE FROM resources WHERE tenant_id = 'acme' AND id = 'orphan'", + [], + ) + .expect("simulate a pre-fix purge"); + + let rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = 'acme'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + rows, 2, + "POSITIVE CONTROL: the simulated pre-fix purge must leave the orphan behind" + ); + + // Wind the recorded schema version back so the migration ladder re-runs. + conn.execute("DELETE FROM schema_version", []) + .expect("clear version"); + conn.execute("INSERT INTO schema_version (version) VALUES (14)", []) + .expect("set version 14"); + } + + // Reopening runs migrate_v14_to_v15. + let config = SqliteBackendConfig { + data_dir: None, + ..Default::default() + }; + let reopened = SqliteBackend::with_config(&path, config).expect("reopen backend"); + reopened.init_schema().expect("migrate to v15"); + + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + let remaining: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = 'acme'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + remaining, 1, + "the v15 migration must sweep the orphaned resource_fts row and keep the live one" + ); + + let orphan_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE resource_id = 'orphan'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!(orphan_rows, 0, "the orphan's row specifically must be gone"); + + let keeper_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE resource_id = 'keeper'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + keeper_rows, 1, + "the migration must not delete rows for resources that still exist" + ); +} + +/// A soft-deleted resource keeps its `resources` row, so the sweep must not +/// touch its full-text entry — matching current behaviour, where a soft delete +/// does not drop the FTS row. +#[tokio::test] +async fn migration_preserves_fts_rows_for_soft_deleted_resources() { + let (backend, _dir, path) = file_backend(); + let tenant = tenant("acme"); + + helios_persistence::core::ResourceStorage::create( + &backend, + &tenant, + "Patient", + suite::patient_with_narrative("p1", &suite::planted_term(&tenant)), + helios_fhir::FhirVersion::default(), + ) + .await + .expect("seed create"); + helios_persistence::core::ResourceStorage::delete(&backend, &tenant, "Patient", "p1") + .await + .expect("soft delete"); + drop(backend); + + { + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + conn.execute("DELETE FROM schema_version", []).unwrap(); + conn.execute("INSERT INTO schema_version (version) VALUES (14)", []) + .unwrap(); + } + + let reopened = SqliteBackend::with_config( + &path, + SqliteBackendConfig { + data_dir: None, + ..Default::default() + }, + ) + .expect("reopen backend"); + reopened.init_schema().expect("migrate to v15"); + + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + let rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = 'acme'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + rows, 1, + "a soft-deleted resource still has a `resources` row, so the sweep must \ + leave its full-text entry alone" + ); +} diff --git a/crates/persistence/tests/search/mod.rs b/crates/persistence/tests/search/mod.rs index 1dbf97dc7..01ed30722 100644 --- a/crates/persistence/tests/search/mod.rs +++ b/crates/persistence/tests/search/mod.rs @@ -40,6 +40,10 @@ pub fn make_sqlite_backend() -> SqliteBackend { pub mod chained_tests; pub mod date_tests; +/// Backend-agnostic scenarios, shared with the PostgreSQL test binary via +/// `#[path]` (issue #386). +pub mod fts_purge_suite; +pub mod fts_purge_tests; pub mod include_tests; pub mod modifier_tests; pub mod number_tests; diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 83251749c..1a9edc8d2 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -3260,13 +3260,15 @@ async fn test_content_search_basic() { let result = backend.search(&tenant, &query).await.unwrap(); - // Should find patients from Illinois (FTS5 must be available) - // If FTS5 is not available, no results will be returned - let count = result.resources.items.len(); - assert!( - count == 0 || count == 2, - "Should find 0 (FTS unavailable) or 2 (FTS available) patients, found {}", - count + // `rusqlite` is pinned with the `bundled` feature, whose amalgamation is + // built with `-DSQLITE_ENABLE_FTS5`, so FTS5 is always available here. + // This used to accept 0 as well ("FTS unavailable"), which meant it stayed + // green through a *total* full-text outage — exactly what happened while + // `$reindex` was silently dropping every `resource_fts` row (issue #386). + assert_eq!( + result.resources.items.len(), + 2, + "Should find both Illinois patients" ); } @@ -3312,16 +3314,16 @@ async fn test_text_search_narrative() { let result = backend.search(&tenant, &query).await.unwrap(); - // FTS5 dependent - either finds 1 or 0 - let count = result.resources.items.len(); - assert!( - count <= 1, - "Should find at most 1 patient with diabetes in narrative" + // Was `count <= 1`, which is satisfied by zero and so could not detect + // full-text search being broken at all (issue #386). FTS5 is always + // compiled in via rusqlite's `bundled` feature, so assert the real result. + assert_eq!( + result.resources.items.len(), + 1, + "Should find the patient with diabetes in the narrative" ); - if count == 1 { - assert_eq!(result.resources.items[0].id(), "text-1"); - } + assert_eq!(result.resources.items[0].id(), "text-1"); } // ============================================================================ diff --git a/crates/ui/src/tenants.rs b/crates/ui/src/tenants.rs index 42fb66b7c..4d79d279a 100644 --- a/crates/ui/src/tenants.rs +++ b/crates/ui/src/tenants.rs @@ -357,13 +357,25 @@ pub async fn delete( }; let _ = storage.deregister_tenant(&id).await; - if query.purge { - let _ = storage.purge_tenant_data(&id).await; - } + // A failed purge must be surfaced, not swallowed. The tenant has already + // been deregistered by the line above, so discarding this error leaves the + // data on disk with nothing in the registry pointing at it, while the page + // renders an ordinary success — an operator told "purged" who still holds + // every resource. `purge_tenant_data` is transactional on the SQL backends, + // so on failure nothing was removed and a retry is safe. + let purge_error = if query.purge { + storage + .purge_tenant_data(&id) + .await + .err() + .map(|e| format!("Tenant '{id}' was deregistered but its data was NOT purged: {e}")) + } else { + None + }; match load_rows(storage, "").await { - Ok(rows) => rows_response(i18n, rows, None), - Err(e) => rows_response(i18n, Vec::new(), Some(e)), + Ok(rows) => rows_response(i18n, rows, purge_error), + Err(e) => rows_response(i18n, Vec::new(), purge_error.or(Some(e))), } }