From 4ef55d8b98a3ea476d2151f0fb1bbef75d7cda47 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 08:53:51 -0400 Subject: [PATCH 1/7] fix(sqlite): make update and delete atomic compare-and-swaps `SqliteBackend::update` was a `SELECT version_id`, a comparison in Rust and an `UPDATE` with no version in its `WHERE`, each statement auto-committed on a pooled connection. With the default pool (10 connections, WAL) on a multi-threaded runtime, two writers holding the same version both pass the comparison. The second `UPDATE` overwrites the first and commits; only its history `INSERT` then fails on `PRIMARY KEY (.., version_id)`. So the loser gets a 500 while its content is already the current row, under a version whose history entry holds the winner's content: a lost update plus a current/history divergence, not just a missed 409. The #1399 race test did not see this because it drives the writers with `join_all` on a current-thread runtime, where synchronous rusqlite calls never interleave. `update` now carries the expected version in the `UPDATE` predicate (zero rows -> `VersionConflict`, or `NotFound` when nothing is live) and runs the history row and search index in the same IMMEDIATE transaction, so a loser leaves nothing behind. Index extraction moves ahead of the write lock. Adds `ResourceStorage::delete_versioned`, the delete half of optimistic locking. The default implementation is read-compare-delete and documented as not atomic; SQLite implements it (and plain `delete`) in one IMMEDIATE transaction, and `delete_with_match` now deletes exactly the version its `If-Match` list was evaluated against instead of calling plain `delete`. New cross-backend suite `versioned_write_race_suite.rs`: 8 tasks on a multi-threaded runtime, released by a barrier, over many resources; asserts one winner, every loser a `ConcurrencyError`, stored state and history are the winner's with no gap or duplicate. Refs #1404 --- .../src/backends/sqlite/storage.rs | 400 +++++++++++------- crates/persistence/src/core/storage.rs | 64 ++- .../search/versioned_write_race_suite.rs | 344 +++++++++++++++ crates/persistence/tests/sqlite_tests.rs | 51 +++ 4 files changed, 694 insertions(+), 165 deletions(-) create mode 100644 crates/persistence/tests/search/versioned_write_race_suite.rs diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index d589926be..2b7d41f69 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -461,48 +461,15 @@ impl ResourceStorage for SqliteBackend { let resource_type = current.resource_type(); tenant.check_permission(Operation::Update, resource_type)?; - let conn = self.get_connection()?; + let mut conn = self.get_connection()?; let tenant_id = tenant.tenant_id().as_str(); let id = current.id(); - // Check that the resource still exists with the expected version - let actual_version: Result = conn.query_row( - "SELECT version_id FROM resources - WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", - params![tenant_id, resource_type, id], - |row| row.get(0), - ); - - let actual_version = match actual_version { - Ok(v) => v, - Err(rusqlite::Error::QueryReturnedNoRows) => { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - Err(e) => { - return Err(internal_error(format!( - "Failed to get current version: {}", - e - ))); - } - }; - - // Check version match - if actual_version != current.version_id() { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_version: current.version_id().to_string(), - actual_version, - }, - )); - } - - // Calculate new version - let new_version: u64 = actual_version.parse().unwrap_or(0) + 1; + // The expected version is `current`'s, and the UPDATE below only matches + // a row that still carries it — so the new version follows from what the + // caller already read. + let expected_version = current.version_id(); + let new_version: u64 = expected_version.parse().unwrap_or(0) + 1; let new_version_str = new_version.to_string(); // Ensure the resource has correct type and id @@ -519,27 +486,85 @@ impl ResourceStorage for SqliteBackend { let data = serde_json::to_vec(&resource) .map_err(|e| serialization_error(format!("Failed to serialize resource: {}", e)))?; + // Extract the search values before taking the write lock: it is pure + // CPU, and SQLite has one writer. + let prepared = (!self.is_search_offloaded()) + .then(|| self.prepare_index(tenant_id, resource_type, id, &resource)); + let now = Utc::now(); let last_updated = now.to_rfc3339(); + let fhir_version_str = current.fhir_version().as_mime_param(); - // Update the resource - conn.execute( - "UPDATE resources SET version_id = ?1, data = ?2, last_updated = ?3 - WHERE tenant_id = ?4 AND resource_type = ?5 AND id = ?6", - params![ - new_version_str, - data, - last_updated, - tenant_id, - resource_type, - id - ], - ) - .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))?; + // Compare-and-swap, history row and search index in ONE transaction. + // + // This used to be a `SELECT version_id`, a comparison in Rust, and then + // an `UPDATE` with no version in its `WHERE`, each statement + // auto-committed on a pooled connection. Two writers holding the same + // version on two connections both passed the comparison; the second + // `UPDATE` then overwrote the first and committed, and only its history + // `INSERT` failed — on `PRIMARY KEY (…, version_id)` — so that writer got + // a 500 while its content was already the current row, under a version + // whose history entry holds the *winner's* content (#1404). + // + // The version now rides in the `UPDATE`'s predicate, so the comparison + // and the write are one statement; and everything that follows shares + // its transaction, so a writer that loses leaves nothing behind. + // IMMEDIATE takes the write lock up front, where the busy handler + // applies (see `purge_tenant_data`). + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("Failed to begin update: {}", e)))?; + + let updated = tx + .execute( + "UPDATE resources SET version_id = ?1, data = ?2, last_updated = ?3 + WHERE tenant_id = ?4 AND resource_type = ?5 AND id = ?6 + AND version_id = ?7 AND is_deleted = 0", + params![ + new_version_str, + data, + last_updated, + tenant_id, + resource_type, + id, + expected_version + ], + ) + .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))?; + + if updated == 0 { + // Matched nothing; which of the two reasons it was costs a query, + // but only on the path that is already failing. + let actual: Result = tx.query_row( + "SELECT version_id FROM resources + WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", + params![tenant_id, resource_type, id], + |row| row.get(0), + ); + return match actual { + Ok(actual_version) => Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version, + }, + )), + Err(rusqlite::Error::QueryReturnedNoRows) => { + Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })) + } + Err(e) => Err(internal_error(format!( + "Failed to get current version: {}", + e + ))), + }; + } // Insert into history (preserve the original FHIR version) - let fhir_version_str = current.fhir_version().as_mime_param(); - conn.execute( + tx.execute( "INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) VALUES (?1, ?2, ?3, ?4, ?5, ?6, 0, ?7)", params![tenant_id, resource_type, id, new_version_str, data, last_updated, fhir_version_str], @@ -547,8 +572,13 @@ impl ResourceStorage for SqliteBackend { .map_err(|e| internal_error(format!("Failed to insert history: {}", e)))?; // Re-index the resource (delete old entries, add new) - self.delete_search_index(&conn, tenant_id, resource_type, id)?; - self.index_resource(&conn, tenant_id, resource_type, id, &resource)?; + if let Some(prepared) = prepared { + self.delete_search_index(&tx, tenant_id, resource_type, id)?; + self.write_prepared_index(&tx, tenant_id, resource_type, id, &resource, prepared)?; + } + + tx.commit() + .map_err(|e| internal_error(format!("Failed to commit update: {}", e)))?; // A SearchParameter write invalidates this tenant's cached registry. if resource_type == "SearchParameter" { @@ -574,112 +604,17 @@ impl ResourceStorage for SqliteBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let conn = self.get_connection()?; - let tenant_id = tenant.tenant_id().as_str(); - - // Check if resource exists and get its fhir_version - let result: Result<(String, Vec, String), _> = conn.query_row( - "SELECT version_id, data, fhir_version FROM resources - WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", - params![tenant_id, resource_type, id], - |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), - ); - - let (current_version, data, fhir_version_str) = match result { - Ok(v) => v, - Err(rusqlite::Error::QueryReturnedNoRows) => { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - Err(e) => { - return Err(internal_error(format!("Failed to check resource: {}", e))); - } - }; - - let now = Utc::now(); - let deleted_at = now.to_rfc3339(); - - // Calculate new version for the deletion record - let new_version: u64 = current_version.parse().unwrap_or(0) + 1; - let new_version_str = new_version.to_string(); - - // Soft delete the resource, guarded by the version we just read. - // - // The `version_id`/`is_deleted` predicates make this a compare-and-swap - // rather than a blind overwrite. Without them a concurrent writer that - // lands between the SELECT above and this UPDATE is silently clobbered, - // and worse: `new_version` was computed from the stale read, so the - // history INSERT below then collides with the row that writer already - // wrote and trips `PRIMARY KEY (tenant_id, resource_type, id, - // version_id)`. Because neither statement runs in a transaction, the - // UPDATE is already committed at that point — the caller gets a 500 and - // the current row now points at a version whose history entry holds - // someone else's content. - // - // MongoDB and S3 already guarded their equivalent writes (a - // `version_id` term in the update filter, and a conditional PUT - // respectively); this brings SQLite to parity. Losing the race is - // reported as `NotFound`, which is what a caller racing a concurrent - // delete would have seen anyway. - let updated = conn - .execute( - "UPDATE resources SET is_deleted = 1, deleted_at = ?1, version_id = ?2, last_updated = ?1 - WHERE tenant_id = ?3 AND resource_type = ?4 AND id = ?5 - AND version_id = ?6 AND is_deleted = 0", - params![ - deleted_at, - new_version_str, - tenant_id, - resource_type, - id, - current_version - ], - ) - .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; - - if updated == 0 { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - - // Insert deletion record into history (preserve fhir_version) - conn.execute( - "INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, 1, ?7)", - params![tenant_id, resource_type, id, new_version_str, data, deleted_at, fhir_version_str], - ) - .map_err(|e| internal_error(format!("Failed to insert deletion history: {}", e)))?; - - // Delete search index entries (skip when search is offloaded). Keyed on - // resource_key. The tenant_id/resource_type prefix is required for the - // delete to seek idx_search_composite instead of full-scanning - // search_index (see delete_search_index, #1197); the soft-delete keeps - // the resources row, so the subquery resolves. - if !self.is_search_offloaded() { - conn.execute( - "DELETE FROM search_index - WHERE tenant_id = ?1 AND resource_type = ?2 - AND resource_key = ( - SELECT rowid 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 delete search index: {}", e)))?; - } - - // A SearchParameter delete invalidates this tenant's cached registry. - if resource_type == "SearchParameter" { - self.tenant_registries().invalidate(tenant_id); - } + self.soft_delete(tenant, resource_type, id, None) + } - Ok(()) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) } async fn count( @@ -1279,6 +1214,138 @@ impl ResourceStorage for SqliteBackend { // Search Index Helpers impl SqliteBackend { + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + /// + /// The read of the current row, the tombstone `UPDATE`, the deletion history + /// row and the search-index cleanup share one `IMMEDIATE` transaction. They + /// used to be auto-committed statements: a failure after the `UPDATE` left a + /// tombstone with no history entry, and a writer landing between the read + /// and the `UPDATE` turned a plain delete into a spurious `NotFound`. Inside + /// the write lock neither can happen, and `expected_version` is compared + /// against the very row the `UPDATE` then tombstones — the comparison and + /// the delete cannot be separated (#1404). + fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let mut conn = self.get_connection()?; + let tenant_id = tenant.tenant_id().as_str(); + + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("Failed to begin delete: {}", e)))?; + + // Check if resource exists and get its fhir_version + let result: Result<(String, Vec, String), _> = tx.query_row( + "SELECT version_id, data, fhir_version FROM resources + WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", + params![tenant_id, resource_type, id], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + ); + + let (current_version, data, fhir_version_str) = match result { + Ok(v) => v, + Err(rusqlite::Error::QueryReturnedNoRows) => { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + Err(e) => { + return Err(internal_error(format!("Failed to check resource: {}", e))); + } + }; + + if let Some(expected) = expected_version + && expected != current_version + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: current_version, + }, + )); + } + + let now = Utc::now(); + let deleted_at = now.to_rfc3339(); + + // Calculate new version for the deletion record + let new_version: u64 = current_version.parse().unwrap_or(0) + 1; + let new_version_str = new_version.to_string(); + + // Soft delete the resource. The `version_id`/`is_deleted` predicates + // keep the statement a compare-and-swap in its own right: the write + // lock already guarantees the row is the one read above, and the + // predicate is what would say so if that ever stopped being true. + let updated = tx + .execute( + "UPDATE resources SET is_deleted = 1, deleted_at = ?1, version_id = ?2, last_updated = ?1 + WHERE tenant_id = ?3 AND resource_type = ?4 AND id = ?5 + AND version_id = ?6 AND is_deleted = 0", + params![ + deleted_at, + new_version_str, + tenant_id, + resource_type, + id, + current_version + ], + ) + .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; + + if updated == 0 { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + + // Insert deletion record into history (preserve fhir_version) + tx.execute( + "INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, 1, ?7)", + params![tenant_id, resource_type, id, new_version_str, data, deleted_at, fhir_version_str], + ) + .map_err(|e| internal_error(format!("Failed to insert deletion history: {}", e)))?; + + // Delete search index entries (skip when search is offloaded). Keyed on + // resource_key. The tenant_id/resource_type prefix is required for the + // delete to seek idx_search_composite instead of full-scanning + // search_index (see delete_search_index, #1197); the soft-delete keeps + // the resources row, so the subquery resolves. + if !self.is_search_offloaded() { + tx.execute( + "DELETE FROM search_index + WHERE tenant_id = ?1 AND resource_type = ?2 + AND resource_key = ( + SELECT rowid 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 delete search index: {}", e)))?; + } + + tx.commit() + .map_err(|e| internal_error(format!("Failed to commit delete: {}", e)))?; + + // A SearchParameter delete invalidates this tenant's cached registry. + if resource_type == "SearchParameter" { + self.tenant_registries().invalidate(tenant_id); + } + + Ok(()) + } + /// Brings a soft-deleted resource back to life with new content. /// /// FHIR permits a deleted resource to be restored by a subsequent update @@ -2118,9 +2185,14 @@ impl VersionedStorage for SqliteBackend { }, )); } + drop(conn); - // Perform delete - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against. + // A plain `delete` here was check-then-act: a writer landing after the + // read above was deleted along with the version the client named + // (#1404). + self.delete_versioned(tenant, resource_type, id, ¤t_version) + .await } async fn list_versions( diff --git a/crates/persistence/src/core/storage.rs b/crates/persistence/src/core/storage.rs index fbea93526..1d4a49566 100644 --- a/crates/persistence/src/core/storage.rs +++ b/crates/persistence/src/core/storage.rs @@ -13,7 +13,7 @@ use serde_json::Value; use crate::core::preconditions::EntityTagPrecondition; use crate::core::sof_runner::SofRunner; -use crate::error::{BackendError, ResourceError, StorageError, StorageResult}; +use crate::error::{BackendError, ConcurrencyError, ResourceError, StorageError, StorageResult}; use crate::tenant::TenantContext; /// A cheap per-tenant change detector for committed writes (#1078). @@ -546,6 +546,68 @@ pub trait ResourceStorage: Send + Sync { id: &str, ) -> StorageResult<()>; + /// Deletes a resource (soft delete) only if `expected_version` is still its + /// current version — the delete half of optimistic locking, as + /// [`update`](Self::update) is the update half. + /// + /// This is what a `DELETE` carrying `If-Match` must go through. Evaluating + /// the precondition against one read and then calling + /// [`delete`](Self::delete) is check-then-act: a writer landing in between + /// is deleted along with the version the client named, a version the + /// client never saw (#1404). + /// + /// `expected_version` is a bare version id (`3`), not an `If-Match` field + /// value; [`VersionedStorage::delete_with_match`] takes the latter. + /// + /// # Atomicity + /// + /// SQLite, PostgreSQL and MongoDB implement this as ONE conditional write + /// carrying the version in its predicate, so the comparison and the delete + /// cannot be separated. S3 makes the tombstone write conditional on the + /// object it compared. The **default implementation is not atomic**: it + /// reads, compares and calls [`delete`](Self::delete), which narrows the + /// window to this call but does not close it. It exists so that stores + /// which are never the system of record for a version (search secondaries, + /// test doubles) need not invent a guarantee they cannot give; a wrapper + /// around a real backend MUST delegate rather than inherit it. + /// + /// # Errors + /// + /// * `StorageError::Resource(NotFound)` - If no live resource exists + /// (never created, or already deleted) + /// * `StorageError::Concurrency(VersionConflict)` - If the current version + /// is not `expected_version`; nothing is deleted + /// * `StorageError::Tenant` - If the tenant doesn't have delete permission + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + let current = match self.read(tenant, resource_type, id).await { + Ok(Some(current)) => current, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) => { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + Err(e) => return Err(e), + }; + if current.version_id() != expected_version { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version: current.version_id().to_string(), + }, + )); + } + self.delete(tenant, resource_type, id).await + } + /// Checks if a resource exists. /// /// This is more efficient than `read` when you only need to check existence. diff --git a/crates/persistence/tests/search/versioned_write_race_suite.rs b/crates/persistence/tests/search/versioned_write_race_suite.rs new file mode 100644 index 000000000..956db52a6 --- /dev/null +++ b/crates/persistence/tests/search/versioned_write_race_suite.rs @@ -0,0 +1,344 @@ +//! Backend-agnostic race suite for version-aware writes (issues #1404, #1405). +//! +//! `ResourceStorage::update` and `ResourceStorage::delete_versioned` are +//! compare-and-swaps on the version the caller holds: of any number of writers +//! holding the same version, exactly one writes, and every other one is told +//! it lost with a `ConcurrencyError` — not with a backend `Internal` error, and +//! never with a success that silently overwrote the winner. +//! +//! The writers are real tasks on a multi-threaded runtime, released together +//! by a barrier and repeated over many resources, so the interleaving these +//! tests need happens by volume rather than by timing. Each test's caller must +//! therefore run it with `#[tokio::test(flavor = "multi_thread")]`; on a +//! current-thread runtime a synchronous backend (SQLite) never interleaves and +//! the assertions hold vacuously. +//! +//! Included by `#[path]` into each backend's test binary, the same arrangement +//! as `conditional_if_match_suite.rs`. + +#![allow(dead_code)] + +use std::sync::Arc; + +use serde_json::{Value, json}; +use tokio::sync::Barrier; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{ResourceStorage, VersionedStorage}; +use helios_persistence::error::{ResourceError, StorageError}; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; +use helios_persistence::types::StoredResource; + +const WRITERS: usize = 8; + +fn tenant(base: &str, label: &str) -> TenantContext { + TenantContext::new( + TenantId::new(format!("{base}-{label}")), + TenantPermissions::full_access(), + ) +} + +fn patient(id: &str, family: &str) -> Value { + json!({ + "resourceType": "Patient", + "id": id, + "name": [{"family": family}] + }) +} + +fn family(stored: &StoredResource) -> String { + stored.content()["name"][0]["family"] + .as_str() + .unwrap_or("?") + .to_string() +} + +/// Creates `Patient/{id}` and proves it reads back at version 1 — the positive +/// control: every writer below holds exactly this `StoredResource`. +async fn seed(backend: &S, tenant: &TenantContext, id: &str) -> StoredResource { + backend + .create( + tenant, + "Patient", + patient(id, "Seed"), + FhirVersion::default(), + ) + .await + .expect("seed patient"); + let current = backend + .read(tenant, "Patient", id) + .await + .expect("read seeded patient") + .expect("seeded patient exists"); + assert_eq!(current.version_id(), "1"); + assert_eq!(family(¤t), "Seed"); + current +} + +/// What one racing writer did. +#[derive(Debug)] +enum Outcome { + Updated(StoredResource), + Deleted, + Failed(StorageError), +} + +fn assert_losers_are_concurrency_errors(outcomes: &[Outcome], deleted: bool, context: &str) { + for outcome in outcomes { + match outcome { + Outcome::Updated(_) | Outcome::Deleted => {} + Outcome::Failed(StorageError::Concurrency(_)) => {} + // A writer that lost to a *delete* finds no live resource at all. + Outcome::Failed(StorageError::Resource(ResourceError::NotFound { .. })) if deleted => {} + Outcome::Failed(StorageError::Resource(ResourceError::Gone { .. })) if deleted => {} + other => panic!("{context}: a loser is a concurrency refusal, got {other:?}"), + } + } +} + +/// `WRITERS` tasks all hold version 1 of the same resource and `update` it at +/// once, `rounds` times over. Exactly one writes version 2; the rest are +/// concurrency errors; the stored resource and the history are the winner's. +pub async fn concurrent_updates_from_the_same_version_admit_one( + backend: Arc, + base: &str, + rounds: usize, +) where + S: ResourceStorage + VersionedStorage + Send + Sync + 'static, +{ + let t = tenant(base, "update-race"); + + for round in 0..rounds { + let id = format!("race-{round}"); + let current = seed(backend.as_ref(), &t, &id).await; + let barrier = Arc::new(Barrier::new(WRITERS)); + + let mut tasks = Vec::new(); + for n in 0..WRITERS { + let (backend, t, current, barrier, id) = ( + backend.clone(), + t.clone(), + current.clone(), + barrier.clone(), + id.clone(), + ); + tasks.push(tokio::spawn(async move { + barrier.wait().await; + match backend + .update(&t, ¤t, patient(&id, &format!("Writer{n}"))) + .await + { + Ok(stored) => Outcome::Updated(stored), + Err(e) => Outcome::Failed(e), + } + })); + } + let mut outcomes = Vec::new(); + for task in tasks { + outcomes.push(task.await.expect("writer task")); + } + let context = format!("round {round}"); + + let winners: Vec<&StoredResource> = outcomes + .iter() + .filter_map(|o| match o { + Outcome::Updated(stored) => Some(stored), + _ => None, + }) + .collect(); + assert_eq!( + winners.len(), + 1, + "{context}: exactly one of {WRITERS} writers holding version 1 may write: {outcomes:?}" + ); + assert_losers_are_concurrency_errors(&outcomes, false, &context); + assert_eq!(winners[0].version_id(), "2", "{context}"); + + let stored = backend + .read(&t, "Patient", &id) + .await + .expect("read after race") + .expect("still live"); + assert_eq!(stored.version_id(), "2", "{context}"); + assert_eq!(family(&stored), family(winners[0]), "{context}"); + + assert_eq!( + backend + .list_versions(&t, "Patient", &id) + .await + .expect("list versions"), + vec!["1".to_string(), "2".to_string()], + "{context}: history has no gap and no duplicate" + ); + let v2 = backend + .vread(&t, "Patient", &id, "2") + .await + .expect("vread 2") + .expect("version 2 exists"); + assert_eq!( + family(&v2), + family(winners[0]), + "{context}: history's version 2 is the winner's content" + ); + } +} + +/// Half the tasks `update` from version 1 and half `delete_versioned` version +/// 1, all at once. Exactly one of them wins: either version 2 is an update and +/// the resource is live, or version 2 is the tombstone — never an update *and* +/// a delete of "version 1", which is the delete removing a version its caller +/// never saw. +pub async fn concurrent_update_and_versioned_delete_admit_one( + backend: Arc, + base: &str, + rounds: usize, +) where + S: ResourceStorage + VersionedStorage + Send + Sync + 'static, +{ + let t = tenant(base, "delete-race"); + + for round in 0..rounds { + let id = format!("race-{round}"); + let current = seed(backend.as_ref(), &t, &id).await; + let barrier = Arc::new(Barrier::new(WRITERS)); + + let mut tasks = Vec::new(); + for n in 0..WRITERS { + let (backend, t, current, barrier, id) = ( + backend.clone(), + t.clone(), + current.clone(), + barrier.clone(), + id.clone(), + ); + tasks.push(tokio::spawn(async move { + barrier.wait().await; + if n % 2 == 0 { + match backend + .update(&t, ¤t, patient(&id, &format!("Writer{n}"))) + .await + { + Ok(stored) => Outcome::Updated(stored), + Err(e) => Outcome::Failed(e), + } + } else { + match backend.delete_versioned(&t, "Patient", &id, "1").await { + Ok(()) => Outcome::Deleted, + Err(e) => Outcome::Failed(e), + } + } + })); + } + let mut outcomes = Vec::new(); + for task in tasks { + outcomes.push(task.await.expect("writer task")); + } + let context = format!("round {round}"); + + let winners: Vec<&Outcome> = outcomes + .iter() + .filter(|o| matches!(o, Outcome::Updated(_) | Outcome::Deleted)) + .collect(); + assert_eq!( + winners.len(), + 1, + "{context}: exactly one of {WRITERS} writers holding version 1 may write: {outcomes:?}" + ); + let deleted = matches!(winners[0], Outcome::Deleted); + assert_losers_are_concurrency_errors(&outcomes, deleted, &context); + + let after = backend.read(&t, "Patient", &id).await; + match winners[0] { + Outcome::Updated(winner) => { + let stored = after.expect("read after race").expect("still live"); + assert_eq!(stored.version_id(), "2", "{context}"); + assert_eq!(family(&stored), family(winner), "{context}"); + } + _ => assert!( + matches!( + after, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) + ), + "{context}: the delete won, so nothing is live: {after:?}" + ), + } + + assert_eq!( + backend + .list_versions(&t, "Patient", &id) + .await + .expect("list versions"), + vec!["1".to_string(), "2".to_string()], + "{context}: one write landed, as version 2" + ); + } +} + +/// `delete_versioned` is a compare-and-swap on the *current* version: a stale +/// version is refused and deletes nothing, the current one deletes, and a +/// second delete of the same version finds nothing live. +pub async fn versioned_delete_is_a_compare_and_swap(backend: &S, base: &str) +where + S: ResourceStorage + VersionedStorage, +{ + let t = tenant(base, "delete-cas"); + let v1 = seed(backend, &t, "cas").await; + let v2 = backend + .update(&t, &v1, patient("cas", "Updated")) + .await + .expect("update to v2"); + assert_eq!(v2.version_id(), "2"); + + // The version the client saw is gone: refuse, and leave v2 live. + let stale = backend.delete_versioned(&t, "Patient", "cas", "1").await; + assert!( + matches!(stale, Err(StorageError::Concurrency(_))), + "a stale versioned delete is a concurrency error, got {stale:?}" + ); + let live = backend + .read(&t, "Patient", "cas") + .await + .expect("read") + .expect("a refused delete deletes nothing"); + assert_eq!(live.version_id(), "2"); + assert_eq!(family(&live), "Updated"); + + // `delete_with_match` takes an `If-Match` field value and goes the same way. + let stale = backend + .delete_with_match(&t, "Patient", "cas", "W/\"1\"") + .await; + assert!( + matches!(stale, Err(StorageError::Concurrency(_))), + "{stale:?}" + ); + + backend + .delete_versioned(&t, "Patient", "cas", "2") + .await + .expect("the current version deletes"); + let after = backend.read(&t, "Patient", "cas").await; + assert!( + matches!( + after, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) + ), + "{after:?}" + ); + assert_eq!( + backend + .list_versions(&t, "Patient", "cas") + .await + .expect("list versions"), + vec!["1".to_string(), "2".to_string(), "3".to_string()], + "the tombstone is version 3" + ); + + let again = backend.delete_versioned(&t, "Patient", "cas", "2").await; + assert!( + matches!( + again, + Err(StorageError::Resource(ResourceError::NotFound { .. })) + ), + "nothing live is left to delete: {again:?}" + ); +} diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 33ddd74c7..6168f3709 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -98,6 +98,57 @@ async fn sqlite_conditional_writers_with_the_same_if_match_admit_one() { .await; } +/// The backend-agnostic race suite for version-aware writes (#1404, #1405). +/// Same `#[path]` arrangement. +#[path = "search/versioned_write_race_suite.rs"] +mod versioned_write_race_suite; + +/// A file-backed (WAL) backend with the default pool — what `hfs` runs in +/// production. Several pooled connections on several runtime threads is the +/// configuration in which SQLite writers really interleave; the shared-cache +/// `:memory:` database serialises them at the table lock instead. +fn create_file_backend(dir: &tempfile::TempDir) -> SqliteBackend { + let backend = + SqliteBackend::with_config(dir.path().join("race.db"), SqliteBackendConfig::default()) + .expect("Failed to create SQLite backend"); + backend.init_schema().expect("Failed to initialize schema"); + backend +} + +/// #1404: of several writers holding the same version, one `update` writes. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn sqlite_concurrent_updates_from_the_same_version_admit_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let backend = std::sync::Arc::new(create_file_backend(&dir)); + versioned_write_race_suite::concurrent_updates_from_the_same_version_admit_one( + backend, + "update-race-1404", + 40, + ) + .await; +} + +/// #1404: an update and a versioned delete of the same version: one wins. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn sqlite_concurrent_update_and_versioned_delete_admit_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let backend = std::sync::Arc::new(create_file_backend(&dir)); + versioned_write_race_suite::concurrent_update_and_versioned_delete_admit_one( + backend, + "delete-race-1404", + 40, + ) + .await; +} + +/// #1404: `delete_versioned` compares and deletes in one step. +#[tokio::test] +async fn sqlite_versioned_delete_is_a_compare_and_swap() { + let backend = create_backend(); + versioned_write_race_suite::versioned_delete_is_a_compare_and_swap(&backend, "delete-cas-1404") + .await; +} + fn create_backend() -> SqliteBackend { // Configure with data directory to load spec SearchParameters // CARGO_MANIFEST_DIR for tests is crates/persistence From 1c989d2fcdf7000cd7726f24b74c95f1acc7f7df Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 08:59:32 -0400 Subject: [PATCH 2/7] fix(mongodb): serve :of-type, reference :identifier, :[type], :above/:below MongoDB refused three standard search modifiers that SQLite, PostgreSQL and Elasticsearch serve, so the same request succeeded or failed with HFS_STORAGE_BACKEND: token `:of-type`, reference `:identifier` and the reference `:[type]` qualifier all fell into the `UnsupportedModifier` catch-all of their filter builder. Reference `:above`/`:below` were refused one step earlier, by `validate_query_support`. Root cause: the builders were never written. Nothing was missing from the index - the writer has always stored `value_identifier_type_system` / `value_identifier_type_code` on identifier rows (and the partial index `idx_search_identifier_type_v2` exists for them), so no reindex is needed. - `:of-type` compares type system, type code and value; an empty part is not compared (as SQLite/PostgreSQL); anything but three parts matches nothing rather than dropping the condition. - `:[type]` is the qualified reference: `subject:Patient=1` builds the very filter `subject=Patient/1` builds, so it cannot match `Group/1`. A value naming another type matches nothing. Qualified reference values are now version-agnostic (`strip_reference_version`), like every other backend. - `:identifier` has the SQLite/PostgreSQL meaning (the reference's target carries the identifier). A filter document cannot join, so `matching_resource_ids` resolves the targets first - tenant-scoped, and bounded to the parameter's declared target types so the token index serves it - and turns the parameter into one index-bounded `$in` of `Type/id` plus an anchored `_history` regex each. More than 10 000 targets is refused (`TooManyResults`), never truncated. - reference `:above`/`:below` reuse the uri shapes. `modifiers_for_type` advertises what is now served, and the new cross-backend `modifier_parity_suite` walks every modifier `SearchModifier::is_valid_for` allows on every parameter type on all four backends, with each backend's known differences stated explicitly so no backend can silently fall behind again. Fixes #1408 --- .../src/backends/mongodb/backend.rs | 72 +- .../src/backends/mongodb/search_impl.rs | 661 ++++++++++++- .../persistence/tests/elasticsearch_tests.rs | 87 ++ crates/persistence/tests/mongodb_tests.rs | 21 + crates/persistence/tests/postgres_tests.rs | 54 ++ crates/persistence/tests/search/mod.rs | 3 + .../tests/search/modifier_parity_suite.rs | 904 ++++++++++++++++++ .../tests/search/modifier_tests.rs | 52 + 8 files changed, 1810 insertions(+), 44 deletions(-) create mode 100644 crates/persistence/tests/search/modifier_parity_suite.rs diff --git a/crates/persistence/src/backends/mongodb/backend.rs b/crates/persistence/src/backends/mongodb/backend.rs index 52b0e495b..548839b15 100644 --- a/crates/persistence/src/backends/mongodb/backend.rs +++ b/crates/persistence/src/backends/mongodb/backend.rs @@ -872,21 +872,31 @@ impl MongoBackend { /// rejected is a visible 400 rather than the old silent wrong answer; /// narrowing it would need a name-aware capability path. /// - /// `:above`/`:below` are now implemented for `uri` (#1002): - /// `build_uri_filter` resolves them segment-aware, mirroring SQLite and - /// Elasticsearch, so they are advertised there. `:in`/`:not-in` stay + /// `:above`/`:below` are implemented for `uri` (#1002) and `reference` + /// (#1408): `build_uri_filter` / `build_reference_filter` resolve them + /// segment-aware, mirroring SQLite and Elasticsearch, so they are + /// advertised there. Token `:of-type` and reference `:identifier` are + /// served too (#1408) — the latter by `resolve_reference_identifier` in + /// `matching_resource_ids`, ahead of the filter builders — as is the + /// reference `:[type]` qualifier, which like on the other backends is not + /// a named modifier and so is not listed. `:in`/`:not-in` stay /// unimplemented and unadvertised for every type (rejected outright by - /// `validate_query_support`), as do token `:above`/`:below`/`:of-type`/ - /// `:text-advanced` and reference `:identifier`/`:above`/`:below`/ - /// `:text-advanced` — token/reference `:above`/`:below` need terminology - /// subsumption and hierarchy resolution respectively, neither of which is - /// implemented — each still hits an `UnsupportedModifier` catch-all in - /// its type-specific builder. + /// `validate_query_support`), as do token `:above`/`:below` (terminology + /// subsumption) and `:text-advanced` on token and reference — each still + /// hits an `UnsupportedModifier` catch-all in its type-specific builder. pub(super) fn modifiers_for_type(param_type: SearchParamType) -> Vec<&'static str> { match param_type { SearchParamType::String => vec!["exact", "contains", "text", "missing"], - SearchParamType::Token => vec!["text", "code-text", "not", "missing"], - SearchParamType::Reference => vec!["contains", "text", "code-text", "missing"], + SearchParamType::Token => vec!["text", "code-text", "of-type", "not", "missing"], + SearchParamType::Reference => vec![ + "identifier", + "contains", + "text", + "code-text", + "below", + "above", + "missing", + ], SearchParamType::Uri => vec!["exact", "contains", "below", "above", "missing"], SearchParamType::Date | SearchParamType::Number | SearchParamType::Quantity => { vec!["missing"] @@ -971,20 +981,25 @@ mod capability_tests { // Token honors text/code-text but not the non-spec :code; :not and // :missing ARE honored (#881, generically in `matching_resource_ids`), - // but :of-type is still rejected by `build_token_filter`. + // and so is :of-type (#1408). Terminology-backed modifiers are not. let t = MongoBackend::modifiers_for_type(SearchParamType::Token); assert!(!t.contains(&"code")); assert!(t.contains(&"code-text")); assert!(t.contains(&"not")); assert!(t.contains(&"missing")); - assert!(!t.contains(&"of-type")); + assert!(t.contains(&"of-type")); + assert!(!t.contains(&"in")); + assert!(!t.contains(&"above")); - // Reference honors contains/text/code-text/missing but not :identifier. + // Reference honors contains/text/code-text/missing, and :identifier + // and :above/:below (#1408). let r = MongoBackend::modifiers_for_type(SearchParamType::Reference); assert!(r.contains(&"contains")); assert!(r.contains(&"text")); assert!(r.contains(&"missing")); - assert!(!r.contains(&"identifier")); + assert!(r.contains(&"identifier")); + assert!(r.contains(&"above")); + assert!(r.contains(&"below")); // Uri honors exact/contains/missing and :above/:below (#1002, // segment-aware in build_uri_filter). @@ -1090,8 +1105,8 @@ mod capability_tests { // `:not-in` outright for every parameter type; advertising // either would be a straightforward regression back to // over-promising. `:above`/`:below` are rejected for every - // type EXCEPT uri (#1002: build_uri_filter resolves them - // there, segment-aware). + // type EXCEPT uri (#1002) and reference (#1408), whose + // builders resolve them segment-aware. assert!( !matches!(modifier_str, "in" | "not-in"), "{param_type} advertises `{modifier_str}`, which \ @@ -1099,9 +1114,12 @@ mod capability_tests { ); assert!( !matches!(modifier_str, "above" | "below") - || param_type == SearchParamType::Uri, + || matches!( + param_type, + SearchParamType::Uri | SearchParamType::Reference + ), "{param_type} advertises `{modifier_str}`, which \ - validate_query_support rejects for every type except uri" + validate_query_support rejects for every type except uri and reference" ); // The advertised string must be a real, round-trippable @@ -1112,12 +1130,19 @@ mod capability_tests { SearchModifier::parse does not recognize" ) }); + // `Display` writes `of-type` in its legacy camelCase, which + // `parse` reads back; every other spelling is its own. assert_eq!( - parsed.to_string(), - modifier_str, + SearchModifier::parse(&parsed.to_string()), + Some(parsed.clone()), "{param_type}'s advertised `{modifier_str}` does not round-trip \ through SearchModifier::parse/Display" ); + assert!( + parsed.to_string() == modifier_str || modifier_str == "of-type", + "{param_type} advertises `{modifier_str}`, which is not the \ + spelling SearchModifier writes" + ); // `:missing` and `:not` are resolved generically in // `matching_resource_ids` *before* any type-specific value @@ -1126,8 +1151,11 @@ mod capability_tests { // than setting it — setting it would wrongly fail (e.g. token's // `build_token_filter` has no `Missing`/`Not` arm and would hit // its `Some(other) => Err(UnsupportedModifier)` catch-all). + // Reference `:identifier` is resolved there too (#1408), by + // `resolve_reference_identifier`: it needs the database, which + // a filter builder does not have, so the builder never sees it. let probe_modifier = match modifier_str { - "missing" | "not" => None, + "missing" | "not" | "identifier" => None, other => Some(SearchModifier::parse(other).unwrap()), }; diff --git a/crates/persistence/src/backends/mongodb/search_impl.rs b/crates/persistence/src/backends/mongodb/search_impl.rs index e67b5a3e8..9a1c0838d 100644 --- a/crates/persistence/src/backends/mongodb/search_impl.rs +++ b/crates/persistence/src/backends/mongodb/search_impl.rs @@ -128,6 +128,11 @@ const PROBE_ROW_LIMIT: u64 = 100_000; // 300k × ~45 bytes/UUID ≈ 13.5 MB — safely under the 16 MB BSON document cap. const MAX_RESULT_ID_SET: usize = 300_000; +/// Targets a reference `:identifier` search may resolve to (#1408). They +/// travel in one `$in`, two entries each; 10 000 stays near 1 MB, well under +/// the 16 MB BSON limit. Past it the search is refused, never truncated. +const MAX_IDENTIFIER_TARGETS: usize = 10_000; + async fn collect_documents(mut cursor: Cursor) -> StorageResult> { let mut docs = Vec::new(); while cursor @@ -1371,10 +1376,11 @@ impl MongoBackend { for param in &query.parameters { // `:in`/`:not-in` are unsupported for every parameter type. // `:above`/`:below` are served for `uri` by `build_uri_filter` - // (segment-aware, mirroring SQLite/Elasticsearch, #1002) but stay - // rejected for token/reference: token `:above`/`:below` need - // terminology subsumption and reference `:above`/`:below` need - // hierarchy resolution, neither of which is implemented here. + // (segment-aware, mirroring SQLite/Elasticsearch, #1002) and for + // `reference` by `build_reference_filter` (the same URL/path + // hierarchy on the stored reference, #1408) but stay rejected for + // token: token `:above`/`:below` need terminology subsumption, + // which is not implemented here. // Every modifier on a composite (#1206) is rejected here except // `:missing`: `composite_search::component_param` hardcodes // `modifier: None` when building each component's filter, so any @@ -1395,12 +1401,14 @@ impl MongoBackend { ) || (matches!( param.modifier, Some(SearchModifier::Above) | Some(SearchModifier::Below) - ) && param.param_type != SearchParamType::Uri) - || (param.param_type == SearchParamType::Composite - && param - .modifier - .as_ref() - .is_some_and(|m| !matches!(m, SearchModifier::Missing))); + ) && !matches!( + param.param_type, + SearchParamType::Uri | SearchParamType::Reference + )) || (param.param_type == SearchParamType::Composite + && param + .modifier + .as_ref() + .is_some_and(|m| !matches!(m, SearchModifier::Missing))); if modifier_unsupported { return Err(StorageError::Search(SearchError::UnsupportedModifier { modifier: param @@ -1859,6 +1867,31 @@ impl MongoBackend { .await; } + // Reference `:identifier` (#1408) names its targets by their + // identifier, which a filter document cannot join on: each such + // parameter is resolved to its complete filter here, once, and no + // target at all empties the search (parameters are ANDed). + let mut identifier_filters: HashMap = HashMap::new(); + for (i, param) in normal.iter().enumerate() { + if param.param_type == SearchParamType::Reference + && matches!(param.modifier, Some(SearchModifier::Identifier)) + { + match self + .resolve_reference_identifier(&search_index, tenant_id, resource_type, param) + .await? + { + Some(filter) => identifier_filters.insert(i, filter), + None => return Ok(Some(HashSet::new())), + }; + } + } + let normal_filter = |i: usize| -> StorageResult { + match identifier_filters.get(&i) { + Some(filter) => Ok(filter.clone()), + None => self.build_search_index_filter(tenant_id, resource_type, normal[i]), + } + }; + // #1206: a composite's probe must run over its component filters // regardless of how many normal params there are — unlike a plain // param, its own filter isn't a single document to count, and the @@ -1892,7 +1925,7 @@ impl MongoBackend { } } } else { - let filter = self.build_search_index_filter(tenant_id, resource_type, param)?; + let filter = normal_filter(i)?; let count = search_index .count_documents(filter) .limit(PROBE_ROW_LIMIT) @@ -1917,7 +1950,7 @@ impl MongoBackend { let driver_filter = if let Some((filter, _)) = composite_probes.remove(&driver_idx) { filter } else { - self.build_search_index_filter(tenant_id, resource_type, normal[driver_idx])? + normal_filter(driver_idx)? }; let mut driver_cursor = search_index @@ -1969,8 +2002,7 @@ impl MongoBackend { if i == driver_idx { continue; } - let param_filter = - self.build_search_index_filter(tenant_id, resource_type, param)?; + let param_filter = normal_filter(i)?; let bounded = doc! { "$and": [ param_filter, @@ -2787,6 +2819,7 @@ impl MongoBackend { "value_token_display": { "$regex": regex, "$options": "i" } }); } + Some(SearchModifier::OfType) => return Ok(Self::build_of_type_filter(&value.value)), Some(other) => { return Err(StorageError::Search(SearchError::UnsupportedModifier { modifier: other.to_string(), @@ -2815,6 +2848,178 @@ impl MongoBackend { } } + /// Builds the `:of-type` predicate (#1408): `type-system|type-code|value` + /// against the identifier row's `value_identifier_type_system` / + /// `value_identifier_type_code` / `value_token_code`. + /// + /// An empty part is not compared, as on SQLite and PostgreSQL + /// (`|MR|12345` is "typed MR, in any system"). Anything but three parts + /// matches nothing, as on PostgreSQL and Elasticsearch: the spec requires + /// all three, and guessing which one is absent would over-match. A row + /// written without the type fields — an identifier with no `type`, or one + /// indexed before they were stored — simply fails the equality. + fn build_of_type_filter(value: &str) -> Document { + let parts: Vec<&str> = value.splitn(3, '|').collect(); + let [type_system, type_code, identifier_value] = parts[..] else { + return Self::match_nothing(); + }; + + let mut filter = Document::new(); + if !identifier_value.is_empty() { + filter.insert("value_token_code", identifier_value); + } + if !type_system.is_empty() { + filter.insert("value_identifier_type_system", type_system); + } + if !type_code.is_empty() { + filter.insert("value_identifier_type_code", type_code); + } + if filter.is_empty() { + return Self::match_nothing(); + } + filter + } + + /// A `search_index` predicate no row satisfies. + fn match_nothing() -> Document { + doc! { "resource_id": { "$in": Bson::Array(Vec::new()) } } + } + + /// The `Type/id` (or absolute URL) a `:[type]` reference search names, or + /// `None` when the value names another type (`subject:Patient=Group/1`), + /// which no reference can satisfy. + fn typed_reference(type_name: &str, value: &str) -> Option { + let base = strip_reference_version(value); + if !base.contains('/') { + return Some(format!("{type_name}/{base}")); + } + let mut segments = base.rsplit('/'); + let _id = segments.next(); + (segments.next() == Some(type_name)).then(|| base.to_string()) + } + + /// The identifier-row predicate of one `:identifier` value, in the token + /// grammar SQLite and PostgreSQL use for it: `system|value`, `system|`, + /// `|value` (no system) or a bare value. + fn identifier_predicate(value: &str) -> Document { + match value.split_once('|') { + Some(("", code)) => doc! { + "value_token_system": { "$in": [Bson::Null, Bson::String(String::new())] }, + "value_token_code": code, + }, + Some((system, "")) => doc! { "value_token_system": system }, + Some((system, code)) => doc! { + "value_token_system": system, + "value_token_code": code, + }, + None => doc! { "value_token_code": value }, + } + } + + /// Resolves a reference `:identifier` parameter (#1408) into its complete + /// `search_index` filter, or `None` when no resource carries the + /// identifier — and so nothing can match. + /// + /// Same meaning as SQLite and PostgreSQL give it: the reference's *target* + /// has the identifier. Those backends express it as a sub-select on the + /// target's `identifier` rows; MongoDB has no join a filter document can + /// carry, so the targets are read first and the parameter becomes one + /// `$in` over their `Type/id` — each with an anchored `_history` regex, so + /// a versioned reference matches too and every entry stays index-bounded. + /// + /// The lookup is scoped to the tenant, which is load-bearing (another + /// tenant's identifiers must not decide this tenant's matches), and to the + /// parameter's declared target types, which is what lets it use the token + /// index: every value index leads with `resource_type`. + async fn resolve_reference_identifier( + &self, + search_index: &mongodb::Collection, + tenant_id: &str, + resource_type: &str, + param: &SearchParameter, + ) -> StorageResult> { + let predicates: Vec = param + .values + .iter() + .map(|value| Bson::Document(Self::identifier_predicate(&value.value))) + .collect(); + + let targets: Vec = { + let registry = self.tenant_registry(tenant_id); + let registry = registry.read(); + crate::search::resolve_param_targets(®istry, resource_type, ¶m.name) + }; + + let mut lookup = doc! { "tenant_id": tenant_id }; + if !targets.is_empty() { + lookup.insert("resource_type", doc! { "$in": targets }); + } + lookup.insert("param_name", "identifier"); + lookup.insert("$or", Bson::Array(predicates)); + + let mut cursor = search_index + .find(lookup) + .projection(doc! { "resource_type": 1, "resource_id": 1, "_id": 0 }) + .await + .or_query_error("Failed to resolve :identifier targets")?; + + let mut found: std::collections::BTreeSet = std::collections::BTreeSet::new(); + loop { + let batch = read_cursor_batch(&mut cursor, CANDIDATE_BATCH_SIZE).await?; + let read = batch.len(); + for row in &batch { + if let (Ok(target_type), Ok(target_id)) = + (row.get_str("resource_type"), row.get_str("resource_id")) + { + found.insert(format!("{target_type}/{target_id}")); + } + } + if found.len() > MAX_IDENTIFIER_TARGETS { + return Err(StorageError::Search(SearchError::TooManyResults { + count: found.len(), + max: MAX_IDENTIFIER_TARGETS, + })); + } + if read < CANDIDATE_BATCH_SIZE { + break; + } + } + if found.is_empty() { + return Ok(None); + } + + Ok(Some(Self::identifier_targets_filter( + tenant_id, + resource_type, + ¶m.name, + found, + ))) + } + + /// The filter a resolved `:identifier` parameter becomes: the referencing + /// rows whose `value_reference` is one of `targets`, at any version. + fn identifier_targets_filter( + tenant_id: &str, + resource_type: &str, + param_name: &str, + targets: impl IntoIterator, + ) -> Document { + let mut references: Vec = Vec::new(); + for target in targets { + references.push(Bson::RegularExpression(bson::Regex { + pattern: format!("^{}/_history/", regex_escape(&target)), + options: String::new(), + })); + references.push(Bson::String(target)); + } + doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "param_name": param_name, + "value_reference": { "$in": references }, + } + } + /// Builds the `search_index` filter for a `Reference`-typed parameter. /// /// #1083: bare-id search (`subject=123`) used to emit a single @@ -2875,20 +3080,63 @@ impl MongoBackend { }); } - if let Some(modifier) = ¶m.modifier { - return Err(StorageError::Search(SearchError::UnsupportedModifier { - modifier: modifier.to_string(), - param_type: "reference".to_string(), - })); + // `:below` / `:above` - URL/path hierarchy on the stored reference, + // the shapes `build_uri_filter` uses (#1408). Canonical `|version` + // comparison is not implemented, as on the other backends. + if matches!(param.modifier.as_ref(), Some(SearchModifier::Below)) { + return Ok(doc! { + "value_reference": { + "$regex": format!( + "^{}(/|$)", + regex_escape(value.value.trim_end_matches('/')) + ) + } + }); } + if matches!(param.modifier.as_ref(), Some(SearchModifier::Above)) { + return Ok(doc! { + "value_reference": { + "$in": crate::search::compute_parent_uris(&value.value) + } + }); + } + + // `:[type]` (#1408): `subject:Patient=123` is `subject=Patient/123`, + // so it takes the qualified branch below and can never match + // `Group/123`. + let typed; + let reference = match param.modifier.as_ref() { + None => value.value.as_str(), + Some(SearchModifier::Type(type_name)) => { + match Self::typed_reference(type_name, &value.value) { + Some(reference) => { + typed = reference; + typed.as_str() + } + None => return Ok(Self::match_nothing()), + } + } + // `:identifier` is resolved by `resolve_reference_identifier` + // before any filter is built; a path that does not do that has + // no way to honour it. + Some(modifier) => { + return Err(StorageError::Search(SearchError::UnsupportedModifier { + modifier: modifier.to_string(), + param_type: "reference".to_string(), + })); + } + }; - if value.value.contains('/') { + if reference.contains('/') { + // Version-agnostic, like every other backend: a versioned search + // value names the resource, not one version of it. + let base = strip_reference_version(reference); return Ok(doc! { "$or": [ - { "value_reference": &value.value }, + { "value_reference": base }, { "value_reference": { - "$regex": format!("^{}/_history/", regex_escape(&value.value)) + "$regex": format!("^{}/_history/", regex_escape(base)) } } ] @@ -5557,3 +5805,372 @@ mod cursor_keyset_tests { )); } } + +/// #1408: token `:of-type`, reference `:[type]`, `:identifier`, `:above` and +/// `:below` — refused as unsupported modifiers before. +#[cfg(test)] +mod modifier_parity_filter_tests { + use super::*; + use crate::backends::mongodb::MongoBackendConfig; + + const V2_0203: &str = "http://terminology.hl7.org/CodeSystem/v2-0203"; + + fn backend() -> MongoBackend { + MongoBackend::new(MongoBackendConfig::default()).unwrap() + } + + fn param( + name: &str, + param_type: SearchParamType, + modifier: SearchModifier, + values: &[&str], + ) -> SearchParameter { + SearchParameter { + name: name.to_string(), + param_type, + modifier: Some(modifier), + values: values.iter().map(|v| SearchValue::eq(*v)).collect(), + chain: vec![], + components: vec![], + } + } + + fn filter(param: &SearchParameter) -> Document { + backend() + .build_search_index_filter("t1", "Observation", param) + .unwrap() + } + + fn subject(modifier: SearchModifier, value: &str) -> Document { + filter(¶m( + "subject", + SearchParamType::Reference, + modifier, + &[value], + )) + } + + fn patient() -> SearchModifier { + SearchModifier::Type("Patient".to_string()) + } + + /// The envelope every filter carries, so a predicate is never evaluated + /// against another tenant's, type's or parameter's rows. + fn envelope(name: &str) -> Document { + doc! { "tenant_id": "t1", "resource_type": "Observation", "param_name": name } + } + + fn with(mut envelope: Document, predicate: Document) -> Document { + envelope.extend(predicate); + envelope + } + + #[test] + fn the_gate_admits_the_new_modifiers_and_still_refuses_terminology() { + let backend = backend(); + let admitted = [ + param( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &["s|c|v"], + ), + param("subject", SearchParamType::Reference, patient(), &["1"]), + param( + "subject", + SearchParamType::Reference, + SearchModifier::Identifier, + &["s|v"], + ), + param( + "subject", + SearchParamType::Reference, + SearchModifier::Above, + &["http://example.org/fhir/Patient/1"], + ), + param( + "subject", + SearchParamType::Reference, + SearchModifier::Below, + &["http://example.org/fhir"], + ), + ]; + for param in admitted { + let shown = format!("{param:?}"); + let query = SearchQuery::new("Observation").with_parameter(param); + assert!(backend.validate_query_support(&query).is_ok(), "{shown}"); + } + + for modifier in [ + SearchModifier::In, + SearchModifier::NotIn, + SearchModifier::Above, + SearchModifier::Below, + ] { + let query = SearchQuery::new("Observation").with_parameter(param( + "code", + SearchParamType::Token, + modifier.clone(), + &["http://loinc.org|1234-5"], + )); + assert!( + matches!( + backend.validate_query_support(&query), + Err(StorageError::Search( + SearchError::UnsupportedModifier { .. } + )) + ), + "token :{modifier} must stay refused" + ); + } + } + + #[test] + fn of_type_compares_type_system_type_code_and_value() { + let of_type = |value: &str| { + filter(¶m( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &[value], + )) + }; + + assert_eq!( + of_type(&format!("{V2_0203}|MR|12345")), + with( + envelope("identifier"), + doc! { + "value_token_code": "12345", + "value_identifier_type_system": V2_0203, + "value_identifier_type_code": "MR", + } + ) + ); + // An empty part is not compared. + assert_eq!( + of_type("|MR|12345"), + with( + envelope("identifier"), + doc! { "value_token_code": "12345", "value_identifier_type_code": "MR" } + ) + ); + // The identifier value may itself contain a pipe. + assert_eq!( + of_type("|MR|a|b"), + with( + envelope("identifier"), + doc! { "value_token_code": "a|b", "value_identifier_type_code": "MR" } + ) + ); + } + + /// Fewer than three parts — or three empty ones — must match nothing: + /// dropping the condition instead would return every resource of the type. + #[test] + fn a_malformed_of_type_value_matches_nothing() { + for value in ["12345", "MR|12345", "||"] { + let built = filter(¶m( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &[value], + )); + assert_eq!( + built, + with(envelope("identifier"), MongoBackend::match_nothing()), + "{value}" + ); + } + } + + #[test] + fn of_type_values_are_ored() { + let built = filter(¶m( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &["|MR|1", "|SS|2"], + )); + assert_eq!( + built.get_array("$or").unwrap(), + &vec![ + Bson::Document( + doc! { "value_token_code": "1", "value_identifier_type_code": "MR" } + ), + Bson::Document( + doc! { "value_token_code": "2", "value_identifier_type_code": "SS" } + ), + ] + ); + } + + /// `subject:Patient=123` is `subject=Patient/123`: the same filter, and so + /// never `Group/123`. + #[test] + fn type_modifier_is_the_qualified_reference() { + let plain = filter(&SearchParameter { + name: "subject".to_string(), + param_type: SearchParamType::Reference, + modifier: None, + values: vec![SearchValue::eq("Patient/123")], + chain: vec![], + components: vec![], + }); + assert_eq!( + plain, + with( + envelope("subject"), + doc! { "$or": [ + { "value_reference": "Patient/123" }, + { "value_reference": { "$regex": "^Patient/123/_history/" } }, + ]} + ) + ); + assert_eq!(subject(patient(), "123"), plain); + assert_eq!(subject(patient(), "Patient/123"), plain); + // Version-agnostic, with or without the modifier. + assert_eq!(subject(patient(), "123/_history/2"), plain); + assert_eq!(subject(patient(), "Patient/123/_history/2"), plain); + assert_eq!( + filter(&SearchParameter { + name: "subject".to_string(), + param_type: SearchParamType::Reference, + modifier: None, + values: vec![SearchValue::eq("Patient/123/_history/2")], + chain: vec![], + components: vec![], + }), + plain + ); + } + + #[test] + fn type_modifier_keeps_an_absolute_url_of_that_type() { + assert_eq!( + subject(patient(), "http://example.org/fhir/Patient/123"), + with( + envelope("subject"), + doc! { "$or": [ + { "value_reference": "http://example.org/fhir/Patient/123" }, + { "value_reference": { + "$regex": "^http://example\\.org/fhir/Patient/123/_history/" + } }, + ]} + ) + ); + } + + #[test] + fn type_modifier_and_a_value_of_another_type_match_nothing() { + for value in ["Group/123", "http://example.org/fhir/Group/123"] { + assert_eq!( + subject(patient(), value), + with(envelope("subject"), MongoBackend::match_nothing()), + "{value}" + ); + } + } + + #[test] + fn type_modifier_escapes_regex_metacharacters() { + let built = subject(patient(), "1.5"); + let arms = built.get_array("$or").unwrap(); + assert_eq!( + arms[1].as_document().unwrap(), + &doc! { "value_reference": { "$regex": "^Patient/1\\.5/_history/" } } + ); + } + + #[test] + fn reference_below_and_above_mirror_the_uri_shapes() { + assert_eq!( + subject(SearchModifier::Below, "http://example.org/fhir/"), + with( + envelope("subject"), + doc! { "value_reference": { "$regex": "^http://example\\.org/fhir(/|$)" } } + ) + ); + let above = subject(SearchModifier::Above, "http://example.org/fhir/Patient/1"); + let parents = above + .get_document("value_reference") + .unwrap() + .get_array("$in") + .unwrap(); + assert!(parents.contains(&Bson::String( + "http://example.org/fhir/Patient/1".to_string() + ))); + assert!(parents.contains(&Bson::String("http://example.org/fhir".to_string()))); + } + + /// `:identifier` needs the database; a path that did not resolve it must + /// refuse rather than treat the identifier as a reference. + #[test] + fn an_unresolved_identifier_modifier_is_refused_by_the_builder() { + let error = backend() + .build_search_index_filter( + "t1", + "Observation", + ¶m( + "subject", + SearchParamType::Reference, + SearchModifier::Identifier, + &["http://example.org/mrn|12345"], + ), + ) + .unwrap_err(); + assert!(matches!( + error, + StorageError::Search(SearchError::UnsupportedModifier { ref modifier, .. }) + if modifier == "identifier" + )); + } + + #[test] + fn identifier_predicates_follow_the_token_grammar() { + assert_eq!( + MongoBackend::identifier_predicate("http://example.org/mrn|12345"), + doc! { "value_token_system": "http://example.org/mrn", "value_token_code": "12345" } + ); + assert_eq!( + MongoBackend::identifier_predicate("http://example.org/mrn|"), + doc! { "value_token_system": "http://example.org/mrn" } + ); + assert_eq!( + MongoBackend::identifier_predicate("12345"), + doc! { "value_token_code": "12345" } + ); + assert_eq!( + MongoBackend::identifier_predicate("|12345"), + doc! { + "value_token_system": { "$in": [Bson::Null, Bson::String(String::new())] }, + "value_token_code": "12345", + } + ); + } + + /// Each resolved target is matched exactly or at any version — never as a + /// prefix of another id (`Patient/p1` must not admit `Patient/p10`). + #[test] + fn resolved_identifier_targets_are_one_bounded_in() { + let built = MongoBackend::identifier_targets_filter( + "t1", + "Observation", + "subject", + vec!["Patient/p.1".to_string()], + ); + assert_eq!( + built, + with( + envelope("subject"), + doc! { "value_reference": { "$in": [ + Bson::RegularExpression(bson::Regex { + pattern: "^Patient/p\\.1/_history/".to_string(), + options: String::new(), + }), + Bson::String("Patient/p.1".to_string()), + ]}} + ) + ); + } +} diff --git a/crates/persistence/tests/elasticsearch_tests.rs b/crates/persistence/tests/elasticsearch_tests.rs index e4085e06d..cb4088bfe 100644 --- a/crates/persistence/tests/elasticsearch_tests.rs +++ b/crates/persistence/tests/elasticsearch_tests.rs @@ -689,6 +689,11 @@ mod token_code_system_suite; #[path = "search/empty_value_suite.rs"] mod empty_value_suite; +/// The backend-agnostic modifier parity suite (#1408). Same `#[path]` +/// arrangement. +#[path = "search/modifier_parity_suite.rs"] +mod modifier_parity_suite; + #[path = "common/container_cleanup.rs"] mod container_cleanup; @@ -1068,6 +1073,88 @@ mod es_integration { .await; } + /// #1408: every modifier `SearchModifier::is_valid_for` allows, on every + /// parameter type. + #[tokio::test] + async fn es_modifier_parity() { + use super::modifier_parity_suite::{Divergence, Expect}; + + let backend = create_backend().await; + super::modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + "modifier-parity-1408", + &[ + // A short `:of-type` value adds no condition at all: every Patient. + Divergence { + label: "Patient?identifier:ofType=MR|12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + Divergence { + label: "Patient?identifier:ofType=12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + // `:code-text` is a word-prefix match (`match_phrase_prefix`), not a + // starts-with on the whole display. + Divergence { + label: "Observation?code:code-text=rate", + expect: Expect::Ids(&["ob-pat"]), + }, + // Terminology-backed token modifiers are not refused but degraded: + // `:in` / `:not-in` match nothing, `:above` / `:below` match the code + // itself. Unreachable over REST, which expands them or answers 501 first. + Divergence { + label: "Observation?code:in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:not-in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:above=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?code:below=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + // `:[type]` filters on the indexed `resource_type`, which a versioned + // reference does not carry, and a bare id also matches an absolute URL. + Divergence { + label: "Observation?subject:Patient=p1", + expect: Expect::Ids(&["ob-abs", "ob-pat"]), + }, + Divergence { + label: "Observation?subject:Patient=Patient/p1", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?subject:Patient=Patient/p1/_history/2", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?subject:Patient=p1,nobody", + expect: Expect::Ids(&["ob-abs", "ob-pat"]), + }, + // `:identifier` looks for token rows under the reference parameter's own + // name, which nothing writes: it never matches. + Divergence { + label: "Observation?subject:identifier=http://example.org/mrn|12345", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?subject:identifier=http://example.org/mrn|", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?subject:identifier=12345", + expect: Expect::Ids(&[]), + }, + ], + ) + .await; + } + // ======================================================================== // Index-side date handling (#1314) // ======================================================================== diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 3aa39e373..a375895b5 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -759,6 +759,27 @@ async fn mongodb_empty_values_are_rejected_on_every_path() { empty_value_suite::empty_values_are_rejected_on_every_path(&backend, "empty-value-1380").await; } +/// The backend-agnostic modifier parity suite (#1408). Same `#[path]` +/// arrangement. +#[path = "search/modifier_parity_suite.rs"] +mod modifier_parity_suite; + +/// #1408: `:of-type`, reference `:identifier` and reference `:[type]` were +/// refused as unsupported modifiers. Needs the full registry. +#[tokio::test] +async fn mongodb_modifier_parity() { + let Some(backend) = create_backend_with_full_registry("modifier_parity").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + "modifier-parity-1408", + &[], + ) + .await; +} + /// #1062: a comma-separated value list on one `SearchParameter` is OR per /// FHIR (https://build.fhir.org/search.html#combining) — for date same as /// every other type. Drives the real `SearchProvider::search` / diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 3db141d04..2b4f32cf6 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -108,6 +108,11 @@ mod conditional_if_match_suite; #[path = "search/empty_value_suite.rs"] mod empty_value_suite; +/// The backend-agnostic modifier parity suite (#1408). Same `#[path]` +/// arrangement. +#[path = "search/modifier_parity_suite.rs"] +mod modifier_parity_suite; + #[path = "common/container_cleanup.rs"] mod container_cleanup; @@ -19101,6 +19106,55 @@ mod postgres_integration { .await; } + /// #1408: every modifier `SearchModifier::is_valid_for` allows, on every + /// parameter type. + #[tokio::test] + async fn postgres_integration_modifier_parity() { + use super::modifier_parity_suite::{Divergence, Expect}; + + let backend = create_backend().await; + super::modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + &unique_base("modifier_parity"), + &[ + // A short `:of-type` value adds no condition at all: every Patient. + Divergence { + label: "Patient?identifier:ofType=MR|12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + Divergence { + label: "Patient?identifier:ofType=12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + // Terminology-backed token modifiers are not refused but degraded: + // `:in` / `:not-in` match nothing, `:above` / `:below` match the code + // itself. Unreachable over REST, which expands them or answers 501 first. + Divergence { + label: "Observation?code:in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:not-in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:above=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?code:below=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + // A value naming another type wins over the `:[type]` modifier. + Divergence { + label: "Observation?subject:Patient=Group/p1", + expect: Expect::Ids(&["ob-grp"]), + }, + ], + ) + .await; + } + #[tokio::test] async fn postgres_integration_distinct_tenant_ids_never_share_data() { let backend = create_backend().await; diff --git a/crates/persistence/tests/search/mod.rs b/crates/persistence/tests/search/mod.rs index 2c1bde65c..92f148d5f 100644 --- a/crates/persistence/tests/search/mod.rs +++ b/crates/persistence/tests/search/mod.rs @@ -70,6 +70,9 @@ pub mod include_tests; pub mod meta_params_suite; pub mod meta_params_suite_tests; pub mod meta_params_tests; +/// Backend-agnostic scenarios, shared with the PostgreSQL, MongoDB and +/// Elasticsearch test binaries via `#[path]` (#1408). +pub mod modifier_parity_suite; pub mod modifier_tests; /// Backend-agnostic scenarios, shared with the PostgreSQL, MongoDB and /// Elasticsearch test binaries via `#[path]` (#1337). diff --git a/crates/persistence/tests/search/modifier_parity_suite.rs b/crates/persistence/tests/search/modifier_parity_suite.rs new file mode 100644 index 000000000..350a034b4 --- /dev/null +++ b/crates/persistence/tests/search/modifier_parity_suite.rs @@ -0,0 +1,904 @@ +//! Backend-agnostic modifier parity suite (issue #1408). +//! +//! MongoDB answered `identifier:of-type=…`, `subject:identifier=…` and +//! `subject:Patient=…` with "unsupported modifier" while SQLite, PostgreSQL +//! and Elasticsearch served them, and nothing noticed: every backend had its +//! own modifier tests, none shared. This suite is the shared one. It walks +//! every modifier [`SearchModifier::is_valid_for`] allows on each parameter +//! type over one seeded data set and states, per cell, the ids that must +//! match. +//! +//! A backend that really does differ passes its differences in as +//! [`Divergence`]s, each keyed by the cell's label (`Type?param:modifier=value`) +//! with what that backend returns instead. A divergence that stops being one +//! fails the suite too, so the list cannot rot, and an unknown label is an +//! error. Anything not listed must agree with the table. +//! +//! Semantics worth knowing before reading the table: +//! +//! - `:identifier` on a reference is implemented by SQLite, PostgreSQL and +//! MongoDB as "the reference's *target* has this identifier" (a join on the +//! target's `identifier` rows). `Reference.identifier` itself — a logical +//! reference — is not indexed by the shared extractor, so `ob-logical` is +//! matched by no backend. +//! - `:[type]` with a bare id is `Type/id`, version-agnostic, and never the +//! same id under another type (`Group/p1`). +//! - `:in` / `:not-in` need a terminology server and `:above` / `:below` on a +//! token need subsumption; neither exists at this layer. The cells are here +//! so that what each backend does with them is on record. +//! +//! Included by `#[path]` into each backend's test binary, like +//! `token_code_system_suite.rs`. The backend must be built with the spec +//! search parameters loaded; the positive controls fail loudly if it was not. + +#![allow(dead_code)] + +use std::collections::BTreeSet; + +use serde_json::json; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{ResourceStorage, SearchProvider}; +use helios_persistence::error::{SearchError, StorageError}; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; +use helios_persistence::types::{ + SearchModifier, SearchParamType, SearchParameter, SearchQuery, SearchValue, +}; + +const V2_0203: &str = "http://terminology.hl7.org/CodeSystem/v2-0203"; +const MRN: &str = "http://example.org/mrn"; +const SSN: &str = "http://example.org/ssn"; +const LOINC: &str = "http://loinc.org"; +const VS: &str = "http://example.org/fhir/ValueSet/a"; +const ABS_PATIENT: &str = "http://example.org/fhir/Patient/p1"; + +/// What a cell must produce. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Expect { + /// The search succeeds and returns exactly these ids. + Ids(&'static [&'static str]), + /// The search is refused as an unsupported modifier. + Rejected, +} + +/// A cell on which one backend knowingly differs from the table. +pub struct Divergence { + /// The cell's label, `Type?param:modifier=value`. + pub label: &'static str, + /// What this backend produces instead. + pub expect: Expect, +} + +struct Case { + resource_type: &'static str, + param: &'static str, + param_type: SearchParamType, + modifier: SearchModifier, + value: String, + expect: Expect, +} + +impl Case { + fn label(&self) -> String { + format!( + "{}?{}:{}={}", + self.resource_type, self.param, self.modifier, self.value + ) + } +} + +fn case( + resource_type: &'static str, + param: &'static str, + param_type: SearchParamType, + modifier: SearchModifier, + value: impl Into, + expect: Expect, +) -> Case { + Case { + resource_type, + param, + param_type, + modifier, + value: value.into(), + expect, + } +} + +/// The matrix. See [`seed`] for the resources the ids name. +fn cases() -> Vec { + use Expect::{Ids, Rejected}; + use SearchModifier as M; + use SearchParamType as T; + + let patient = || M::Type("Patient".to_string()); + let group = || M::Type("Group".to_string()); + + vec![ + // ---- token --------------------------------------------------- + // `:of-type` is `type-system|type-code|identifier-value`. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|MR|12345"), + Ids(&["p1"]), + ), + // Same value, other type: the type really is compared. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|SS|12345"), + Ids(&["p2"]), + ), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|MR|99999"), + Ids(&[]), + ), + // An empty part is not compared. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + "|MR|12345", + Ids(&["p1"]), + ), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}||12345"), + Ids(&["p1", "p2"]), + ), + // The grammar has three parts; fewer name nothing. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + "MR|12345", + Ids(&[]), + ), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + "12345", + Ids(&[]), + ), + // An OR-list of `:of-type` values. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|MR|12345,{V2_0203}|SS|12345"), + Ids(&["p1", "p2"]), + ), + case( + "Observation", + "code", + T::Token, + M::Text, + "heart", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "code", + T::Token, + M::CodeText, + "heart", + Ids(&["ob-pat"]), + ), + // `:code-text` is starts-with, `:text` is not. + case( + "Observation", + "code", + T::Token, + M::CodeText, + "rate", + Ids(&[]), + ), + case( + "Observation", + "code", + T::Token, + M::Not, + format!("{LOINC}|1234-5"), + Ids(&["ob-abs", "ob-grp", "ob-logical", "ob-nosubj", "ob-ver"]), + ), + case( + "Observation", + "code", + T::Token, + M::Missing, + "true", + Ids(&["ob-nosubj"]), + ), + case( + "Observation", + "code", + T::Token, + M::Missing, + "false", + Ids(&["ob-abs", "ob-grp", "ob-logical", "ob-pat", "ob-ver"]), + ), + // Terminology-backed: not available at this layer. + case( + "Observation", + "code", + T::Token, + M::In, + "http://example.org/fhir/ValueSet/a", + Rejected, + ), + case( + "Observation", + "code", + T::Token, + M::NotIn, + "http://example.org/fhir/ValueSet/a", + Rejected, + ), + case( + "Observation", + "code", + T::Token, + M::Above, + format!("{LOINC}|1234-5"), + Rejected, + ), + case( + "Observation", + "code", + T::Token, + M::Below, + format!("{LOINC}|1234-5"), + Rejected, + ), + // ---- string -------------------------------------------------- + case( + "Patient", + "family", + T::String, + M::Exact, + "Smith", + Ids(&["p1"]), + ), + case( + "Patient", + "family", + T::String, + M::Exact, + "smith", + Ids(&["p3"]), + ), + case( + "Patient", + "family", + T::String, + M::Contains, + "MIT", + Ids(&["p1", "p2", "p3"]), + ), + case( + "Patient", + "family", + T::String, + M::Contains, + "thso", + Ids(&["p2"]), + ), + case( + "Patient", + "family", + T::String, + M::Text, + "thso", + Ids(&["p2"]), + ), + case( + "Patient", + "family", + T::String, + M::Missing, + "true", + Ids(&["p4"]), + ), + case( + "Patient", + "family", + T::String, + M::Missing, + "false", + Ids(&["p1", "p2", "p3"]), + ), + // ---- reference ----------------------------------------------- + // `subject:Patient=p1` is `subject=Patient/p1`: not `Group/p1`. + case( + "Observation", + "subject", + T::Reference, + patient(), + "p1", + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + group(), + "p1", + Ids(&["ob-grp"]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + "Patient/p1", + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + "Patient/p1/_history/2", + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + "nobody", + Ids(&[]), + ), + // The modifier and the value disagree on the type: nothing can be + // both. + case( + "Observation", + "subject", + T::Reference, + patient(), + "Group/p1", + Ids(&[]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + ABS_PATIENT, + Ids(&["ob-abs"]), + ), + // Two ids under one type modifier. + case( + "Observation", + "subject", + T::Reference, + patient(), + "p1,nobody", + Ids(&["ob-pat", "ob-ver"]), + ), + // `:identifier`: the reference's target has the identifier. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|12345"), + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|"), + Ids(&["ob-pat", "ob-ver"]), + ), + // Value alone: `p1` (a Patient) and the Group `p1` both carry 12345. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + "12345", + Ids(&["ob-grp", "ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|99999"), + Ids(&[]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Missing, + "true", + Ids(&["ob-logical", "ob-nosubj"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Missing, + "false", + Ids(&["ob-abs", "ob-grp", "ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Contains, + "example.org", + Ids(&["ob-abs"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Text, + "smith", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::CodeText, + "john", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Below, + "http://example.org/fhir/Patient", + Ids(&["ob-abs"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Above, + format!("{ABS_PATIENT}/_history/1"), + Ids(&["ob-abs"]), + ), + // ---- uri ----------------------------------------------------- + case( + "ValueSet", + "url", + T::Uri, + M::Below, + VS, + Ids(&["vs-a", "vs-sub"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Above, + format!("{VS}/sub"), + Ids(&["vs-a", "vs-sub"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Contains, + "ValueSet/a/s", + Ids(&["vs-sub"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Missing, + "true", + Ids(&["vs-none"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Missing, + "false", + Ids(&["vs-a", "vs-sub"]), + ), + // ---- date / number / quantity ---------------------------------- + case( + "Patient", + "birthdate", + T::Date, + M::Missing, + "true", + Ids(&["p2", "p3", "p4"]), + ), + case( + "Patient", + "birthdate", + T::Date, + M::Missing, + "false", + Ids(&["p1"]), + ), + case( + "RiskAssessment", + "probability", + T::Number, + M::Missing, + "true", + Ids(&["ra-none"]), + ), + case( + "RiskAssessment", + "probability", + T::Number, + M::Missing, + "false", + Ids(&["ra-half"]), + ), + case( + "Observation", + "value-quantity", + T::Quantity, + M::Missing, + "false", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "value-quantity", + T::Quantity, + M::Missing, + "true", + Ids(&["ob-abs", "ob-grp", "ob-logical", "ob-nosubj", "ob-ver"]), + ), + ] +} + +fn query( + resource_type: &str, + param: &str, + param_type: SearchParamType, + modifier: Option, + value: &str, +) -> SearchQuery { + SearchQuery::new(resource_type) + .with_parameter(SearchParameter { + name: param.to_string(), + param_type, + modifier, + // Comma-separated values are an OR-list, as the REST layer parses them. + values: value.split(',').map(SearchValue::eq).collect(), + ..Default::default() + }) + .with_count(100) +} + +/// Runs one query and reduces the result to what the table compares. +async fn outcome( + backend: &S, + tenant: &TenantContext, + query: &SearchQuery, +) -> Result, String> +where + S: ResourceStorage + SearchProvider, +{ + match backend.search(tenant, query).await { + Ok(result) => Ok(result + .resources + .items + .iter() + .map(|r| r.id().to_string()) + .collect()), + Err(StorageError::Search(SearchError::UnsupportedModifier { .. })) => { + Err("REJECTED".to_string()) + } + Err(other) => Err(format!("ERROR {other}")), + } +} + +fn render(outcome: &Result, String>) -> String { + match outcome { + Ok(ids) => format!("{:?}", ids.iter().collect::>()), + Err(e) => e.clone(), + } +} + +fn render_expect(expect: &Expect) -> String { + match expect { + Expect::Ids(ids) => { + let sorted: BTreeSet<&str> = ids.iter().copied().collect(); + format!("{:?}", sorted.iter().collect::>()) + } + Expect::Rejected => "REJECTED".to_string(), + } +} + +/// The seeded resources: +/// +/// - Patients: `p1` Smith, born 1980, MRN 12345 typed `MR`; `p2` Smithson, +/// SSN 12345 typed `SS`; `p3` smith (lower case), no identifier; `p4` has +/// neither name nor identifier. +/// - Group `p1`: the same id as the Patient, with an untyped identifier +/// `http://example.org/grp|12345`. +/// - Observations, by `subject`: `ob-pat` `Patient/p1` (display "John Smith", +/// coded LOINC 1234-5 "Heart rate", 5 mg); `ob-ver` `Patient/p1/_history/2`; +/// `ob-abs` the absolute URL; `ob-grp` `Group/p1`; `ob-logical` only a +/// `Reference.identifier`; `ob-nosubj` neither subject nor code. +/// - ValueSets `vs-a`, `vs-sub` (a URL below `vs-a`'s), `vs-none` (no URL). +/// - RiskAssessments `ra-half` (probability 0.5) and `ra-none`. +async fn seed(backend: &S, tenant: &TenantContext) +where + S: ResourceStorage + SearchProvider, +{ + let other_code = json!({"coding": [{"system": LOINC, "code": "9999-9"}]}); + let resources = [ + ( + "Patient", + json!({ + "id": "p1", + "identifier": [{ + "type": {"coding": [{"system": V2_0203, "code": "MR"}]}, + "system": MRN, + "value": "12345", + }], + "name": [{"family": "Smith", "given": ["John"]}], + "birthDate": "1980-01-01", + }), + ), + ( + "Patient", + json!({ + "id": "p2", + "identifier": [{ + "type": {"coding": [{"system": V2_0203, "code": "SS"}]}, + "system": SSN, + "value": "12345", + }], + "name": [{"family": "Smithson"}], + }), + ), + ( + "Patient", + json!({"id": "p3", "name": [{"family": "smith"}]}), + ), + ("Patient", json!({"id": "p4", "active": true})), + ( + "Group", + json!({ + "id": "p1", + "type": "person", + "actual": true, + "identifier": [{"system": "http://example.org/grp", "value": "12345"}], + }), + ), + ( + "Observation", + json!({ + "id": "ob-pat", + "status": "final", + "code": {"coding": [{"system": LOINC, "code": "1234-5", "display": "Heart rate"}]}, + "subject": {"reference": "Patient/p1", "display": "John Smith"}, + "valueQuantity": { + "value": 5, "unit": "mg", + "system": "http://unitsofmeasure.org", "code": "mg", + }, + }), + ), + ( + "Observation", + json!({ + "id": "ob-ver", + "status": "final", + "code": other_code.clone(), + "subject": {"reference": "Patient/p1/_history/2"}, + }), + ), + ( + "Observation", + json!({ + "id": "ob-abs", + "status": "final", + "code": other_code.clone(), + "subject": {"reference": ABS_PATIENT}, + }), + ), + ( + "Observation", + json!({ + "id": "ob-grp", + "status": "final", + "code": other_code.clone(), + "subject": {"reference": "Group/p1"}, + }), + ), + ( + "Observation", + json!({ + "id": "ob-logical", + "status": "final", + "code": other_code.clone(), + "subject": {"identifier": {"system": MRN, "value": "12345"}}, + }), + ), + ("Observation", json!({"id": "ob-nosubj", "status": "final"})), + ( + "ValueSet", + json!({"id": "vs-a", "status": "active", "url": VS}), + ), + ( + "ValueSet", + json!({"id": "vs-sub", "status": "active", "url": format!("{VS}/sub")}), + ), + ("ValueSet", json!({"id": "vs-none", "status": "active"})), + ( + "RiskAssessment", + json!({ + "id": "ra-half", + "status": "final", + "subject": {"reference": "Patient/p1"}, + "prediction": [{"probabilityDecimal": 0.5}], + }), + ), + ( + "RiskAssessment", + json!({ + "id": "ra-none", + "status": "final", + "subject": {"reference": "Patient/p1"}, + }), + ), + ]; + for (resource_type, resource) in resources { + let id = resource["id"].as_str().unwrap_or_default().to_string(); + backend + .create(tenant, resource_type, resource, FhirVersion::default()) + .await + .unwrap_or_else(|e| panic!("create {resource_type}/{id} failed: {e}")); + } +} + +/// Seeds the data under a caller-unique tenant and asserts the matrix, with +/// the caller's [`Divergence`]s applied. +pub async fn every_valid_modifier_agrees_across_backends( + backend: &S, + tenant_base: &str, + divergences: &[Divergence], +) where + S: ResourceStorage + SearchProvider, +{ + use SearchParamType as T; + + let tenant = TenantContext::new(TenantId::new(tenant_base), TenantPermissions::full_access()); + seed(backend, &tenant).await; + + // Positive controls: the unmodified search on every parameter the matrix + // uses. Polled because Elasticsearch is near-real-time. A failure here + // means the parameter did not index — a backend built without the spec + // search parameters — not that a modifier is wrong. + let controls: [(&str, &str, SearchParamType, &str, &[&str]); 9] = [ + ("Patient", "identifier", T::Token, "12345", &["p1", "p2"]), + ("Patient", "family", T::String, "smiths", &["p2"]), + ("Patient", "birthdate", T::Date, "1980-01-01", &["p1"]), + ("Observation", "code", T::Token, "1234-5", &["ob-pat"]), + ( + "Observation", + "subject", + T::Reference, + "Patient/p1", + &["ob-pat", "ob-ver"], + ), + ( + "Observation", + "subject", + T::Reference, + "Group/p1", + &["ob-grp"], + ), + ( + "Observation", + "value-quantity", + T::Quantity, + "5", + &["ob-pat"], + ), + ("ValueSet", "url", T::Uri, VS, &["vs-a"]), + ( + "RiskAssessment", + "probability", + T::Number, + "0.5", + &["ra-half"], + ), + ]; + for (resource_type, param, param_type, value, expected) in controls { + let control = query(resource_type, param, param_type, None, value); + let want: BTreeSet = expected.iter().map(|id| id.to_string()).collect(); + let mut got = Ok(BTreeSet::new()); + for _ in 0..60 { + got = outcome(backend, &tenant, &control).await; + if got.as_ref() == Ok(&want) { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(250)).await; + } + assert_eq!( + got, + Ok(want), + "positive control {resource_type}?{param}={value}" + ); + } + + let table = cases(); + let labels: BTreeSet = table.iter().map(Case::label).collect(); + for divergence in divergences { + assert!( + labels.contains(divergence.label), + "divergence names no cell of the matrix: {}", + divergence.label + ); + } + + let mut failures = Vec::new(); + for case in &table { + let label = case.label(); + let got = outcome( + backend, + &tenant, + &query( + case.resource_type, + case.param, + case.param_type, + Some(case.modifier.clone()), + &case.value, + ), + ) + .await; + let shown = render(&got); + println!("{label} -> {shown}"); + + let agreed = render_expect(&case.expect); + match divergences.iter().find(|d| d.label == label) { + Some(divergence) => { + let diverged = render_expect(&divergence.expect); + if shown == agreed { + failures.push(format!( + "{label}: now agrees with the matrix ({agreed}); remove the divergence" + )); + } else if shown != diverged { + failures.push(format!( + "{label}: got {shown}, expected the known divergence {diverged} \ + (matrix: {agreed})" + )); + } + } + None => { + if shown != agreed { + failures.push(format!("{label}: got {shown}, expected {agreed}")); + } + } + } + } + assert!(failures.is_empty(), "\n{}", failures.join("\n")); +} diff --git a/crates/persistence/tests/search/modifier_tests.rs b/crates/persistence/tests/search/modifier_tests.rs index 0bcce63d5..bb2ebc38c 100644 --- a/crates/persistence/tests/search/modifier_tests.rs +++ b/crates/persistence/tests/search/modifier_tests.rs @@ -1375,3 +1375,55 @@ async fn test_of_type_modifier_type_discrimination() { let _dl_result = backend.search(&tenant, &dl_query.with_count(100)).await; let _pp_result = backend.search(&tenant, &pp_query.with_count(100)).await; } + +/// #1408: every modifier `SearchModifier::is_valid_for` allows, on every +/// parameter type, over one data set. The scenarios are backend-agnostic +/// (`modifier_parity_suite.rs`): PostgreSQL, MongoDB and Elasticsearch run the +/// same ones. +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_modifier_parity_suite() { + use super::modifier_parity_suite::{Divergence, Expect}; + + let backend = create_sqlite_backend(); + super::modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + "modifier-parity", + &[ + // A short `:of-type` value is read as `type-code|value`, or `value`. + Divergence { + label: "Patient?identifier:ofType=MR|12345", + expect: Expect::Ids(&["p1"]), + }, + Divergence { + label: "Patient?identifier:ofType=12345", + expect: Expect::Ids(&["p1", "p2"]), + }, + // Terminology-backed token modifiers are not refused but degraded: + // `:in` / `:not-in` match nothing, `:above` / `:below` match the code + // itself. Unreachable over REST, which expands them or answers 501 first. + Divergence { + label: "Observation?code:in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:not-in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:above=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?code:below=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + // A value naming another type wins over the `:[type]` modifier. + Divergence { + label: "Observation?subject:Patient=Group/p1", + expect: Expect::Ids(&["ob-grp"]), + }, + ], + ) + .await; +} From edf6afafff5a78ce91741db33d75d43565c512f0 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:01:24 -0400 Subject: [PATCH 3/7] test(persistence): modifier parity suite covers AND, search_count and tenants The suite's `:identifier` lookup is a second query; a cell now proves it is tenant-scoped, three cells AND a modified parameter with a plain one so it is also exercised as the non-driving filter, and every cell checks that `search_count` counts what `search` returns. Refs #1408 --- .../persistence/tests/elasticsearch_tests.rs | 8 ++ .../tests/search/modifier_parity_suite.rs | 122 ++++++++++++++++-- 2 files changed, 117 insertions(+), 13 deletions(-) diff --git a/crates/persistence/tests/elasticsearch_tests.rs b/crates/persistence/tests/elasticsearch_tests.rs index cb4088bfe..a52f638bc 100644 --- a/crates/persistence/tests/elasticsearch_tests.rs +++ b/crates/persistence/tests/elasticsearch_tests.rs @@ -1136,6 +1136,10 @@ mod es_integration { label: "Observation?subject:Patient=p1,nobody", expect: Expect::Ids(&["ob-abs", "ob-pat"]), }, + Divergence { + label: "Observation?subject:Patient=p1&code=9999-9", + expect: Expect::Ids(&["ob-abs"]), + }, // `:identifier` looks for token rows under the reference parameter's own // name, which nothing writes: it never matches. Divergence { @@ -1150,6 +1154,10 @@ mod es_integration { label: "Observation?subject:identifier=12345", expect: Expect::Ids(&[]), }, + Divergence { + label: "Observation?subject:identifier=http://example.org/mrn|12345&code=1234-5", + expect: Expect::Ids(&[]), + }, ], ) .await; diff --git a/crates/persistence/tests/search/modifier_parity_suite.rs b/crates/persistence/tests/search/modifier_parity_suite.rs index 350a034b4..965744738 100644 --- a/crates/persistence/tests/search/modifier_parity_suite.rs +++ b/crates/persistence/tests/search/modifier_parity_suite.rs @@ -75,16 +75,28 @@ struct Case { param_type: SearchParamType, modifier: SearchModifier, value: String, + /// A second, unmodified token parameter ANDed with the first, so the + /// modified one is also exercised where it does not drive the search. + and: Option<(&'static str, &'static str)>, expect: Expect, } impl Case { fn label(&self) -> String { + let and = self + .and + .map(|(param, value)| format!("&{param}={value}")) + .unwrap_or_default(); format!( - "{}?{}:{}={}", + "{}?{}:{}={}{and}", self.resource_type, self.param, self.modifier, self.value ) } + + fn and(mut self, param: &'static str, value: &'static str) -> Self { + self.and = Some((param, value)); + self + } } fn case( @@ -101,6 +113,7 @@ fn case( param_type, modifier, value: value.into(), + and: None, expect, } } @@ -483,6 +496,44 @@ fn cases() -> Vec { format!("{ABS_PATIENT}/_history/1"), Ids(&["ob-abs"]), ), + // Another tenant's Patient `p1` carries MRN 77777; this tenant's does + // not, and its references must not match on the strength of it. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|77777"), + Ids(&[]), + ), + // ANDed with a plain parameter, in both directions of selectivity. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|12345"), + Ids(&["ob-pat"]), + ) + .and("code", "1234-5"), + case( + "Observation", + "subject", + T::Reference, + patient(), + "p1", + Ids(&["ob-ver"]), + ) + .and("code", "9999-9"), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}||12345"), + Ids(&["p2"]), + ) + .and("family", "smithson"), // ---- uri ----------------------------------------------------- case( "ValueSet", @@ -595,6 +646,32 @@ fn query( .with_count(100) } +/// The query of one cell: the modified parameter, plus its companion if any. +fn case_query(case: &Case) -> SearchQuery { + let query = query( + case.resource_type, + case.param, + case.param_type, + Some(case.modifier.clone()), + &case.value, + ); + match case.and { + Some((param, value)) => query.with_parameter(SearchParameter { + name: param.to_string(), + // `family` is the one string companion; the rest are tokens. + param_type: if param == "family" { + SearchParamType::String + } else { + SearchParamType::Token + }, + modifier: None, + values: vec![SearchValue::eq(value)], + ..Default::default() + }), + None => query, + } +} + /// Runs one query and reduces the result to what the table compares. async fn outcome( backend: &S, @@ -793,6 +870,22 @@ pub async fn every_valid_modifier_agrees_across_backends( let tenant = TenantContext::new(TenantId::new(tenant_base), TenantPermissions::full_access()); seed(backend, &tenant).await; + // The same Patient id under another tenant, with an identifier this + // tenant's `p1` does not have. + let other = TenantContext::new( + TenantId::new(format!("{tenant_base}-other")), + TenantPermissions::full_access(), + ); + backend + .create( + &other, + "Patient", + json!({"id": "p1", "identifier": [{"system": MRN, "value": "77777"}]}), + FhirVersion::default(), + ) + .await + .unwrap_or_else(|e| panic!("create the other tenant's Patient/p1 failed: {e}")); + // Positive controls: the unmodified search on every parameter the matrix // uses. Polled because Elasticsearch is near-real-time. A failure here // means the parameter did not index — a backend built without the spec @@ -863,21 +956,24 @@ pub async fn every_valid_modifier_agrees_across_backends( let mut failures = Vec::new(); for case in &table { let label = case.label(); - let got = outcome( - backend, - &tenant, - &query( - case.resource_type, - case.param, - case.param_type, - Some(case.modifier.clone()), - &case.value, - ), - ) - .await; + let query = case_query(case); + let got = outcome(backend, &tenant, &query).await; let shown = render(&got); println!("{label} -> {shown}"); + // `search_count` takes its own route through some backends; it must + // count what `search` returns. + if let Ok(ids) = &got { + match backend.search_count(&tenant, &query).await { + Ok(count) if count == ids.len() as u64 => {} + Ok(count) => failures.push(format!( + "{label}: search_count is {count}, search returned {}", + ids.len() + )), + Err(e) => failures.push(format!("{label}: search_count failed: {e}")), + } + } + let agreed = render_expect(&case.expect); match divergences.iter().find(|d| d.label == label) { Some(divergence) => { From 36741b73ecffdab7b20092cb7091d64c0a6c3739 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:05:11 -0400 Subject: [PATCH 4/7] fix(persistence): atomic versioned delete on every backend A `DELETE` carrying `If-Match` was check-then-act everywhere: the REST handler (and each backend's `delete_with_match`, and the conditional-delete gate from #1399) evaluated the precondition against one read and then called the unconditional `delete(id)`. A writer landing in between was deleted along with the version the client named - a version the client never saw - and the client was told 204. Reproduced deterministically over REST (a primary whose `read` is followed by another writer's update: `DELETE` + `If-Match: W/"1"` answered 204 and removed version 2) and by an 8-task race on SQLite and PostgreSQL (an `update` from version 1 and a delete "of version 1" both succeeded). `ResourceStorage::delete_versioned` is now one conditional write per backend: - PostgreSQL: the existing single-statement soft delete gains `AND ($5 IS NULL OR version_id = $5)`, evaluated on the locked row. - SQLite: version compare, tombstone UPDATE (version in its predicate), history row and index cleanup in one IMMEDIATE transaction. - MongoDB: the tombstone `update_one` already filtered on the version it read; the expected version is now compared against that same document. - S3: compared on the object whose ETag the conditional PUT is tied to. - Elasticsearch keeps the documented non-atomic default: it is a search secondary and never the system of record for a version. - CompositeStorage, CompositeSubmitJobs and IndexingSubmitJobs delegate to the primary instead of inheriting the default, and a refused precondition no longer counts against the primary's health. `core::delete_under_precondition` routes the instance `DELETE`, every `conditional_delete` and `delete_with_match` through it when `If-Match` is present; a delete without a precondition is unchanged. Losing the race is `VersionConflict` -> 409, what `PUT` + `If-Match` already answers. Fixes #1404 --- .../src/backends/mongodb/search_impl.rs | 2 +- .../src/backends/mongodb/storage.rs | 378 +++++++++++------- .../src/backends/postgres/storage.rs | 377 +++++++++-------- crates/persistence/src/backends/s3/storage.rs | 151 ++++--- .../src/backends/sqlite/storage.rs | 2 +- .../persistence/src/composite/bulk_submit.rs | 12 + .../src/composite/indexing_submit_jobs.rs | 12 + crates/persistence/src/composite/storage.rs | 64 ++- crates/persistence/src/core/mod.rs | 5 +- crates/persistence/src/core/preconditions.rs | 38 ++ crates/persistence/src/core/storage.rs | 6 +- crates/persistence/tests/mongodb_tests.rs | 16 + crates/persistence/tests/postgres_tests.rs | 40 ++ crates/rest/src/handlers/delete.rs | 32 +- crates/rest/tests/if_match_write_race.rs | 316 +++++++++++++++ 15 files changed, 1078 insertions(+), 373 deletions(-) create mode 100644 crates/rest/tests/if_match_write_race.rs diff --git a/crates/persistence/src/backends/mongodb/search_impl.rs b/crates/persistence/src/backends/mongodb/search_impl.rs index e67b5a3e8..14de7fe1d 100644 --- a/crates/persistence/src/backends/mongodb/search_impl.rs +++ b/crates/persistence/src/backends/mongodb/search_impl.rs @@ -873,7 +873,7 @@ impl ConditionalStorage for MongoBackend { 1 => { let current = matches.into_iter().next().expect("single match must exist"); crate::core::conditional_if_match_gate(if_match, resource_type, Some(¤t))?; - self.delete(tenant, resource_type, current.id()).await?; + crate::core::delete_under_precondition(self, tenant, if_match, ¤t).await?; Ok(ConditionalDeleteResult::Deleted(current)) } n => Ok(ConditionalDeleteResult::MultipleMatches(n)), diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index e9fed97bd..0237f69b1 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -1139,152 +1139,18 @@ impl ResourceStorage for MongoBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let db = self.get_database().await?; - let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); - let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); - let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; - let tenant_id = tenant.tenant_id().as_str(); - - let delete_lookup_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "is_deleted": false, - }; - - let maybe_existing = if let Some(active_session) = session.as_mut() { - resources - .find_one(delete_lookup_filter.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!( - "Failed to check resource before delete (session): {}", - e - )) - })? - } else { - resources - .find_one(delete_lookup_filter) - .await - .map_err(|e| { - internal_error(format!("Failed to check resource before delete: {}", e)) - })? - }; - - let Some(existing_doc) = maybe_existing else { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - }; - - let current_version = existing_doc - .get_str("version_id") - .map_err(|e| internal_error(format!("Missing current version: {}", e)))? - .to_string(); - let new_version = next_version(¤t_version)?; - - let payload = existing_doc - .get_document("data") - .map_err(|e| internal_error(format!("Missing resource payload: {}", e)))? - .clone(); - let fhir_version = existing_doc - .get_str("fhir_version") - .unwrap_or("4.0") - .to_string(); - let created_at = extract_created_at(&existing_doc, Utc::now()); - - let now = Utc::now(); - let now_bson = chrono_to_bson(now); - - let delete_update_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": ¤t_version, - "is_deleted": false, - }; - let delete_update_doc = doc! { - "$set": { - "version_id": &new_version, - "is_deleted": true, - "deleted_at": now_bson, - "last_updated": now_bson, - } - }; - - let update_result = if let Some(active_session) = session.as_mut() { - resources - .update_one(delete_update_filter.clone(), delete_update_doc.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!("Failed to soft-delete resource (session): {}", e)) - })? - } else { - resources - .update_one(delete_update_filter, delete_update_doc) - .await - .map_err(|e| internal_error(format!("Failed to soft-delete resource: {}", e)))? - }; - - if update_result.matched_count == 0 { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - - let history_doc = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": &new_version, - "data": Bson::Document(payload), - "created_at": chrono_to_bson(created_at), - "last_updated": now_bson, - "is_deleted": true, - "deleted_at": now_bson, - "fhir_version": fhir_version, - }; - - if let Some(active_session) = session.as_mut() { - history - .insert_one(history_doc) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!( - "Failed to insert deletion history row (session): {}", - e - )) - })?; - } else { - history.insert_one(history_doc).await.map_err(|e| { - internal_error(format!("Failed to insert deletion history row: {}", e)) - })?; - } - - self.delete_search_index(&db, tenant_id, resource_type, id, &mut session) - .await?; - - commit_best_effort_multi_write_session(&mut session, transaction_active, "delete").await?; - - // A SearchParameter delete may remove a tenant's overlay entry: refresh - // the stored-param cache and drop registries. This must run after the - // commit above: `reload_stored_cache` reads the `resources` collection - // without the session, so it cannot observe the delete while the - // transaction is still open. - if resource_type == "SearchParameter" { - if let Err(e) = self.reload_stored_cache().await { - tracing::warn!("SearchParameter cache reload failed: {e}"); - } - } + self.soft_delete(tenant, resource_type, id, None).await + } - Ok(()) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) + .await } async fn exists( @@ -1953,6 +1819,220 @@ impl SearchIndexDocuments { } impl MongoBackend { + /// The version of the live (not deleted) resource, read outside any + /// session — what a write that just lost a race reports as the version it + /// lost to. + async fn live_version( + &self, + resources: &Collection, + tenant_id: &str, + resource_type: &str, + id: &str, + ) -> StorageResult> { + let live = resources + .find_one(doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "is_deleted": false, + }) + .await + .map_err(|e| internal_error(format!("Failed to reload current version: {}", e)))?; + Ok(live.and_then(|d| d.get_str("version_id").ok().map(str::to_string))) + } + + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + /// + /// The tombstone `update_one` carries the version in its filter, so the + /// comparison and the delete are one conditional write whether or not the + /// deployment supports transactions. `expected_version` is checked against + /// the document that filter is then built from: a `DELETE` with `If-Match` + /// used to be evaluated above this layer against an earlier read and then + /// deleted whatever version was current by the time it got here (#1404). + async fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let db = self.get_database().await?; + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); + let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; + let tenant_id = tenant.tenant_id().as_str(); + + let delete_lookup_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "is_deleted": false, + }; + + let maybe_existing = if let Some(active_session) = session.as_mut() { + resources + .find_one(delete_lookup_filter.clone()) + .session(active_session) + .await + .map_err(|e| { + internal_error(format!( + "Failed to check resource before delete (session): {}", + e + )) + })? + } else { + resources + .find_one(delete_lookup_filter) + .await + .map_err(|e| { + internal_error(format!("Failed to check resource before delete: {}", e)) + })? + }; + + let Some(existing_doc) = maybe_existing else { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + }; + + let current_version = existing_doc + .get_str("version_id") + .map_err(|e| internal_error(format!("Missing current version: {}", e)))? + .to_string(); + if let Some(expected) = expected_version + && expected != current_version + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: current_version, + }, + )); + } + let new_version = next_version(¤t_version)?; + + let payload = existing_doc + .get_document("data") + .map_err(|e| internal_error(format!("Missing resource payload: {}", e)))? + .clone(); + let fhir_version = existing_doc + .get_str("fhir_version") + .unwrap_or("4.0") + .to_string(); + let created_at = extract_created_at(&existing_doc, Utc::now()); + + let now = Utc::now(); + let now_bson = chrono_to_bson(now); + + let delete_update_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": ¤t_version, + "is_deleted": false, + }; + let delete_update_doc = doc! { + "$set": { + "version_id": &new_version, + "is_deleted": true, + "deleted_at": now_bson, + "last_updated": now_bson, + } + }; + + let update_result = if let Some(active_session) = session.as_mut() { + resources + .update_one(delete_update_filter.clone(), delete_update_doc.clone()) + .session(active_session) + .await + .map_err(|e| { + internal_error(format!("Failed to soft-delete resource (session): {}", e)) + })? + } else { + resources + .update_one(delete_update_filter, delete_update_doc) + .await + .map_err(|e| internal_error(format!("Failed to soft-delete resource: {}", e)))? + }; + + if update_result.matched_count == 0 { + // A writer got in after the read above (only possible without a + // transaction). A versioned delete says which way it lost. + if let Some(expected) = expected_version + && let Some(actual) = self + .live_version(&resources, tenant_id, resource_type, id) + .await? + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: actual, + }, + )); + } + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + + let history_doc = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": &new_version, + "data": Bson::Document(payload), + "created_at": chrono_to_bson(created_at), + "last_updated": now_bson, + "is_deleted": true, + "deleted_at": now_bson, + "fhir_version": fhir_version, + }; + + if let Some(active_session) = session.as_mut() { + history + .insert_one(history_doc) + .session(active_session) + .await + .map_err(|e| { + internal_error(format!( + "Failed to insert deletion history row (session): {}", + e + )) + })?; + } else { + history.insert_one(history_doc).await.map_err(|e| { + internal_error(format!("Failed to insert deletion history row: {}", e)) + })?; + } + + self.delete_search_index(&db, tenant_id, resource_type, id, &mut session) + .await?; + + commit_best_effort_multi_write_session(&mut session, transaction_active, "delete").await?; + + // A SearchParameter delete may remove a tenant's overlay entry: refresh + // the stored-param cache and drop registries. This must run after the + // commit above: `reload_stored_cache` reads the `resources` collection + // without the session, so it cannot observe the delete while the + // transaction is still open. + if resource_type == "SearchParameter" { + if let Err(e) = self.reload_stored_cache().await { + tracing::warn!("SearchParameter cache reload failed: {e}"); + } + } + + Ok(()) + } + /// Brings a soft-deleted resource back to life with new content. /// /// FHIR permits a deleted resource to be restored by a subsequent update @@ -2687,7 +2767,13 @@ impl VersionedStorage for MongoBackend { )); } - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against. + // A plain `delete` here was check-then-act: a writer landing after the + // read above was deleted along with the version the client named + // (#1404). + let actual = actual.to_string(); + self.delete_versioned(tenant, resource_type, id, &actual) + .await } async fn list_versions( diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index 4c66e6515..dd13bb2e2 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -601,167 +601,18 @@ impl ResourceStorage for PostgresBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let client = self.get_client().await?; - let tenant_id = tenant.tenant_id().as_str(); - - let now = Utc::now(); + self.soft_delete(tenant, resource_type, id, None).await + } - // Soft delete the resource and write its deletion history row, in one - // statement, deriving the tombstone's version from the row itself. - // - // Three things used to be separate here: a `SELECT version_id`, an - // `UPDATE` compare-and-swapping against it, and an `INSERT` of the - // history row. The `INSERT` was folded into the `UPDATE` first; this - // folds in the `SELECT` too, so a delete is one round trip where it was - // three. On the crud suite that is 275,382 statements and 275,382 - // occupied-connection round trips removed from a workload that already - // demands ~36 cores' worth of PostgreSQL execution on a 4-core host — - // the round trip, not the 0.06 ms of execution behind it, is what is - // being bought back. - // - // ## Why this is *more* atomic, not less - // - // The read-then-CAS it replaces was correct but pessimistic. Under READ - // COMMITTED, a writer landing between the `SELECT` and the `UPDATE` - // meant the `version_id = ` predicate matched nothing, and this - // returned `NotFound` — a 404 for a resource that plainly existed and - // was live. Computing `version_id + 1` inside the `UPDATE`'s target list - // removes the window rather than detecting it: PostgreSQL takes the row - // lock, and if the row was concurrently updated it re-evaluates both the - // qualifier and the target list against the *committed new* version of - // the tuple (EvalPlanQual). So the tombstone's version is always exactly - // one more than whatever version is current at the instant the row is - // locked, never one more than a version that has since moved on. - // - // That is what preserves the primary-key fix the CAS was introduced for. - // `resource_history` is keyed `PRIMARY KEY (tenant_id, resource_type, - // id, version_id)` (schema.rs). The failure the CAS prevented was a - // history row computed from a stale read colliding with one a concurrent - // writer had already inserted. A version derived from the locked row - // cannot be stale, so it cannot collide — the invariant is enforced by - // construction instead of by a guard that has to lose a race to notice. - // - // A concurrent *delete* is still resolved correctly and still costs - // nothing extra: the loser re-evaluates `is_deleted = FALSE` against the - // committed tombstone, matches no row, and reports `NotFound`, which is - // exactly what it reported before. - // - // ## What changes, stated plainly - // - // An unconditional `DELETE` that races a concurrent `UPDATE` now - // succeeds — deleting the version that writer just committed — where it - // used to fail with `NotFound`. That is a deliberate correction: FHIR's - // delete interaction (https://hl7.org/fhir/http.html#delete) carries no - // precondition of its own, so "delete the current state" is the right - // reading and the 404 was spurious. Callers that *do* want a - // precondition use `If-Match`, which is evaluated above this layer. - // - // What this does NOT change: that `If-Match` on `DELETE` is evaluated by - // the REST handler (and by `delete_with_match`) against its own earlier - // read and is therefore still check-then-act. It was check-then-act - // before this change too — the CAS removed here guarded the version - // *this function* had read a microsecond earlier, never the version the - // caller's precondition was evaluated against — so no precondition - // guarantee moves in either direction. Making `If-Match` on `DELETE` - // atomic needs the expected version threaded into this statement, which - // is a signature change and a separate piece of work. - // - // ## The version arithmetic - // - // `version_id` is `TEXT`, so the increment is guarded rather than a bare - // cast: a non-numeric value would make `::bigint` raise 22P02 and turn a - // delete into a 500. The `CASE` reproduces the Rust it replaces — - // `current_version.parse::().unwrap_or(0) + 1` — for every value - // this server can have written (`'7'` -> 8, `'007'` -> 8, `''` and - // `'abc'` -> 1, matching `unwrap_or(0)`). `CASE` does not evaluate the - // branch it did not select, so the cast never runs on a value the regex - // rejected. Version ids are server-issued decimal integers on every - // write path in this backend, so the fallback is unreachable in - // practice and is here only so that it degrades the same way the Rust - // did rather than differently. - // - // `RETURNING` feeds the history row from the tuple just written, so the - // deletion entry carries the resource's own `fhir_version` without - // making a round trip through the client. As in `create` and `update`, - // no matching row means the CTE yields nothing, the insert selects - // nothing, and the statement reports zero rows affected — one signal for - // both writes. One statement is also one implicit transaction: the - // tombstone lands with the delete or neither does. - // - // ## The tombstone stores `'null'::jsonb`, not the resource - // - // A deletion entry is the record that the resource was deleted, not a - // version of the resource: FHIR gives it `request.method = DELETE` and - // no `resource` in a history Bundle, and `410 Gone` on a vread of that - // version. `history_entry_to_json` has always omitted the body, and the - // vread handler now answers `410`, so nothing can ask for these bytes. - // - // Storing them was not free. `data` is a JSONB body of a few kilobytes; - // the `UPDATE` above does not touch that column, so `resources` keeps - // its existing TOAST datum untouched, but a TOAST pointer cannot be - // shared across tables — inserting it into `resource_history` detoasts - // the value, re-compresses it, writes it, and puts the whole body in the - // WAL a second time. That was 10.6% of the crud suite's Postgres - // execution time on run 33213565802 for a row no reader can reach. - // - // `'null'::jsonb` rather than `NULL` because `resource_history.data` is - // `NOT NULL` (schema v1) and both `vread` and the history readers deserialise - // the column into a `serde_json::Value` with a non-nullable `FromSql`; - // a SQL `NULL` would panic in `row.get`, and widening the column would - // put a migration and six read sites in the way of a write-path change. - // `Value::Null` reaches the same readers as a well-formed value that - // renders as `null`, and they already discard it for a deleted version. - // - // Rows written by an older build keep their bodies and are read back - // exactly as before; nothing needs backfilling, because the only reader - // was already dropping the value on the floor. - let updated = execute_cached( - &client, - "WITH del AS ( - UPDATE resources - SET is_deleted = TRUE, - deleted_at = $1, - last_updated = $1, - version_id = ((CASE WHEN version_id ~ '^[0-9]+$' THEN version_id::bigint ELSE 0 END) + 1)::text - WHERE tenant_id = $2 AND resource_type = $3 AND id = $4 - AND is_deleted = FALSE - RETURNING tenant_id, resource_type, id, version_id, last_updated, is_deleted, fhir_version - ) - INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) - SELECT tenant_id, resource_type, id, version_id, 'null'::jsonb, last_updated, is_deleted, fhir_version FROM del", - &[&now, &tenant_id, &resource_type, &id], - ) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) .await - .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; - - if updated == 0 { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - - // Delete search index entries (skip when search is offloaded) - if !self.is_search_offloaded() { - execute_cached( - &client, - "DELETE FROM search_index WHERE tenant_id = $1 AND resource_type = $2 AND resource_id = $3", - &[&tenant_id, &resource_type, &id], - ) - .await - .map_err(|e| internal_error(format!("Failed to delete search index: {}", e)))?; - } - - // A SearchParameter delete invalidates the tenant overlays. - if resource_type == "SearchParameter" { - if let Err(e) = self.reload_stored_cache().await { - tracing::warn!("SearchParameter cache reload failed: {e}"); - } - } - - Ok(()) } async fn count( @@ -1276,6 +1127,204 @@ impl ResourceStorage for PostgresBackend { // ============================================================================ impl PostgresBackend { + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + async fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let client = self.get_client().await?; + let tenant_id = tenant.tenant_id().as_str(); + + let now = Utc::now(); + + // Soft delete the resource and write its deletion history row, in one + // statement, deriving the tombstone's version from the row itself. + // + // Three things used to be separate here: a `SELECT version_id`, an + // `UPDATE` compare-and-swapping against it, and an `INSERT` of the + // history row. The `INSERT` was folded into the `UPDATE` first; this + // folds in the `SELECT` too, so a delete is one round trip where it was + // three. On the crud suite that is 275,382 statements and 275,382 + // occupied-connection round trips removed from a workload that already + // demands ~36 cores' worth of PostgreSQL execution on a 4-core host — + // the round trip, not the 0.06 ms of execution behind it, is what is + // being bought back. + // + // ## Why this is *more* atomic, not less + // + // The read-then-CAS it replaces was correct but pessimistic. Under READ + // COMMITTED, a writer landing between the `SELECT` and the `UPDATE` + // meant the `version_id = ` predicate matched nothing, and this + // returned `NotFound` — a 404 for a resource that plainly existed and + // was live. Computing `version_id + 1` inside the `UPDATE`'s target list + // removes the window rather than detecting it: PostgreSQL takes the row + // lock, and if the row was concurrently updated it re-evaluates both the + // qualifier and the target list against the *committed new* version of + // the tuple (EvalPlanQual). So the tombstone's version is always exactly + // one more than whatever version is current at the instant the row is + // locked, never one more than a version that has since moved on. + // + // That is what preserves the primary-key fix the CAS was introduced for. + // `resource_history` is keyed `PRIMARY KEY (tenant_id, resource_type, + // id, version_id)` (schema.rs). The failure the CAS prevented was a + // history row computed from a stale read colliding with one a concurrent + // writer had already inserted. A version derived from the locked row + // cannot be stale, so it cannot collide — the invariant is enforced by + // construction instead of by a guard that has to lose a race to notice. + // + // A concurrent *delete* is still resolved correctly and still costs + // nothing extra: the loser re-evaluates `is_deleted = FALSE` against the + // committed tombstone, matches no row, and reports `NotFound`, which is + // exactly what it reported before. + // + // ## What changes, stated plainly + // + // An unconditional `DELETE` that races a concurrent `UPDATE` now + // succeeds — deleting the version that writer just committed — where it + // used to fail with `NotFound`. That is a deliberate correction: FHIR's + // delete interaction (https://hl7.org/fhir/http.html#delete) carries no + // precondition of its own, so "delete the current state" is the right + // reading and the 404 was spurious. Callers that *do* want a + // precondition use `If-Match`, which is evaluated above this layer. + // + // ## The versioned form (#1404) + // + // `If-Match` on `DELETE` used to be evaluated above this layer against + // an earlier read and followed by this unconditional statement — + // check-then-act, so a writer landing in between was deleted along with + // the version the client named. `expected_version` threads the + // precondition into the statement: `$5 IS NULL OR version_id = $5` is + // evaluated on the locked row, and re-evaluated by EvalPlanQual against + // the committed tuple if a writer got there first, so the comparison and + // the delete cannot be separated. With no expected version the + // predicate is constant-true and the statement is the one above. + // + // ## The version arithmetic + // + // `version_id` is `TEXT`, so the increment is guarded rather than a bare + // cast: a non-numeric value would make `::bigint` raise 22P02 and turn a + // delete into a 500. The `CASE` reproduces the Rust it replaces — + // `current_version.parse::().unwrap_or(0) + 1` — for every value + // this server can have written (`'7'` -> 8, `'007'` -> 8, `''` and + // `'abc'` -> 1, matching `unwrap_or(0)`). `CASE` does not evaluate the + // branch it did not select, so the cast never runs on a value the regex + // rejected. Version ids are server-issued decimal integers on every + // write path in this backend, so the fallback is unreachable in + // practice and is here only so that it degrades the same way the Rust + // did rather than differently. + // + // `RETURNING` feeds the history row from the tuple just written, so the + // deletion entry carries the resource's own `fhir_version` without + // making a round trip through the client. As in `create` and `update`, + // no matching row means the CTE yields nothing, the insert selects + // nothing, and the statement reports zero rows affected — one signal for + // both writes. One statement is also one implicit transaction: the + // tombstone lands with the delete or neither does. + // + // ## The tombstone stores `'null'::jsonb`, not the resource + // + // A deletion entry is the record that the resource was deleted, not a + // version of the resource: FHIR gives it `request.method = DELETE` and + // no `resource` in a history Bundle, and `410 Gone` on a vread of that + // version. `history_entry_to_json` has always omitted the body, and the + // vread handler now answers `410`, so nothing can ask for these bytes. + // + // Storing them was not free. `data` is a JSONB body of a few kilobytes; + // the `UPDATE` above does not touch that column, so `resources` keeps + // its existing TOAST datum untouched, but a TOAST pointer cannot be + // shared across tables — inserting it into `resource_history` detoasts + // the value, re-compresses it, writes it, and puts the whole body in the + // WAL a second time. That was 10.6% of the crud suite's Postgres + // execution time on run 33213565802 for a row no reader can reach. + // + // `'null'::jsonb` rather than `NULL` because `resource_history.data` is + // `NOT NULL` (schema v1) and both `vread` and the history readers deserialise + // the column into a `serde_json::Value` with a non-nullable `FromSql`; + // a SQL `NULL` would panic in `row.get`, and widening the column would + // put a migration and six read sites in the way of a write-path change. + // `Value::Null` reaches the same readers as a well-formed value that + // renders as `null`, and they already discard it for a deleted version. + // + // Rows written by an older build keep their bodies and are read back + // exactly as before; nothing needs backfilling, because the only reader + // was already dropping the value on the floor. + let updated = execute_cached( + &client, + "WITH del AS ( + UPDATE resources + SET is_deleted = TRUE, + deleted_at = $1, + last_updated = $1, + version_id = ((CASE WHEN version_id ~ '^[0-9]+$' THEN version_id::bigint ELSE 0 END) + 1)::text + WHERE tenant_id = $2 AND resource_type = $3 AND id = $4 + AND is_deleted = FALSE + AND ($5::text IS NULL OR version_id = $5::text) + RETURNING tenant_id, resource_type, id, version_id, last_updated, is_deleted, fhir_version + ) + INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) + SELECT tenant_id, resource_type, id, version_id, 'null'::jsonb, last_updated, is_deleted, fhir_version FROM del", + &[&now, &tenant_id, &resource_type, &id, &expected_version], + ) + .await + .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; + + if updated == 0 { + // Matched nothing. For a versioned delete that is either "nothing + // live" or "live at another version"; telling them apart costs a + // query, but only on the path that is already failing. + if let Some(expected) = expected_version { + let actual = client + .query_opt( + "SELECT version_id FROM resources + WHERE tenant_id = $1 AND resource_type = $2 AND id = $3 AND is_deleted = FALSE", + &[&tenant_id, &resource_type, &id], + ) + .await + .map_err(|e| internal_error(format!("Failed to get current version: {}", e)))?; + if let Some(row) = actual { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: row.get::<_, String>(0), + }, + )); + } + } + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + + // Delete search index entries (skip when search is offloaded) + if !self.is_search_offloaded() { + execute_cached( + &client, + "DELETE FROM search_index WHERE tenant_id = $1 AND resource_type = $2 AND resource_id = $3", + &[&tenant_id, &resource_type, &id], + ) + .await + .map_err(|e| internal_error(format!("Failed to delete search index: {}", e)))?; + } + + // A SearchParameter delete invalidates the tenant overlays. + if resource_type == "SearchParameter" { + if let Err(e) = self.reload_stored_cache().await { + tracing::warn!("SearchParameter cache reload failed: {e}"); + } + } + + Ok(()) + } + /// Brings a soft-deleted resource back to life with new content. /// /// FHIR permits a deleted resource to be restored by a subsequent update @@ -2136,8 +2185,12 @@ impl VersionedStorage for PostgresBackend { )); } - // Perform delete - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against. + // A plain `delete` here was check-then-act: a writer landing after the + // read above was deleted along with the version the client named + // (#1404). + self.delete_versioned(tenant, resource_type, id, ¤t_version) + .await } async fn list_versions( @@ -3163,7 +3216,7 @@ impl ConditionalStorage for PostgresBackend { // Exactly one match - delete it let existing = matches.into_iter().next().unwrap(); crate::core::conditional_if_match_gate(if_match, resource_type, Some(&existing))?; - self.delete(tenant, resource_type, existing.id()).await?; + crate::core::delete_under_precondition(self, tenant, if_match, &existing).await?; Ok(ConditionalDeleteResult::Deleted(existing)) } n => { diff --git a/crates/persistence/src/backends/s3/storage.rs b/crates/persistence/src/backends/s3/storage.rs index 9c4c3736b..f2a31fed1 100644 --- a/crates/persistence/src/backends/s3/storage.rs +++ b/crates/persistence/src/backends/s3/storage.rs @@ -44,6 +44,90 @@ pub(crate) struct CurrentResourceWithMeta { } impl S3Backend { + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + /// + /// The tombstone is a conditional PUT on the ETag of the object loaded + /// here, and `expected_version` is compared on that same object — so a + /// writer landing after the comparison changes the ETag and the PUT is + /// refused (`OptimisticLockFailure`) rather than deleting a version the + /// caller never saw (#1404). That guarantee is the object store's + /// conditional write: a store that ignores `If-Match` on PUT gives none. + async fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let location = self.tenant_location(tenant)?; + let current_key = location.keyspace.current_resource_key(resource_type, id); + + let Some(actual) = self + .load_current_with_meta(tenant, resource_type, id) + .await? + else { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + }; + + if actual.resource.is_deleted() { + return Err(StorageError::Resource(ResourceError::Gone { + resource_type: resource_type.to_string(), + id: id.to_string(), + deleted_at: actual.resource.deleted_at(), + })); + } + + if let Some(expected) = expected_version + && expected != actual.resource.version_id() + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: actual.resource.version_id().to_string(), + }, + )); + } + + let deleted = actual.resource.mark_deleted(); + let payload = self.serialize_json(&deleted)?; + + match self + .put_json_object( + &location.bucket, + ¤t_key, + &payload, + actual.etag.as_deref(), + None, + ) + .await + { + Ok(_) => { + self.put_history_and_indexes(&location, &deleted, HistoryMethod::Delete) + .await?; + self.maybe_reload_search_param_cache(tenant, resource_type, None) + .await; + Ok(()) + } + Err(StorageError::Backend(BackendError::QueryError { .. })) => Err( + StorageError::Concurrency(ConcurrencyError::OptimisticLockFailure { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_etag: actual.etag.unwrap_or_default(), + actual_etag: None, + }), + ), + Err(err) => Err(err), + } + } + /// Serialises `value` to a JSON byte vector. pub(crate) fn serialize_json(&self, value: &T) -> StorageResult> { serde_json::to_vec(value).map_err(|e| { @@ -939,59 +1023,18 @@ impl ResourceStorage for S3Backend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let location = self.tenant_location(tenant)?; - let current_key = location.keyspace.current_resource_key(resource_type, id); - - let Some(actual) = self - .load_current_with_meta(tenant, resource_type, id) - .await? - else { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - }; - - if actual.resource.is_deleted() { - return Err(StorageError::Resource(ResourceError::Gone { - resource_type: resource_type.to_string(), - id: id.to_string(), - deleted_at: actual.resource.deleted_at(), - })); - } - - let deleted = actual.resource.mark_deleted(); - let payload = self.serialize_json(&deleted)?; + self.soft_delete(tenant, resource_type, id, None).await + } - match self - .put_json_object( - &location.bucket, - ¤t_key, - &payload, - actual.etag.as_deref(), - None, - ) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) .await - { - Ok(_) => { - self.put_history_and_indexes(&location, &deleted, HistoryMethod::Delete) - .await?; - self.maybe_reload_search_param_cache(tenant, resource_type, None) - .await; - Ok(()) - } - Err(StorageError::Backend(BackendError::QueryError { .. })) => Err( - StorageError::Concurrency(ConcurrencyError::OptimisticLockFailure { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_etag: actual.etag.unwrap_or_default(), - actual_etag: None, - }), - ), - Err(err) => Err(err), - } } async fn count( @@ -1406,7 +1449,11 @@ impl VersionedStorage for S3Backend { )); } - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against, + // not whatever is current by the time `delete` loads it again (#1404). + let actual_version = actual_version.to_string(); + self.delete_versioned(tenant, resource_type, id, &actual_version) + .await } async fn list_versions( diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index 2b7d41f69..c571bbf0d 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -3301,7 +3301,7 @@ impl ConditionalStorage for SqliteBackend { // Exactly one match - delete it let existing = matches.into_iter().next().unwrap(); crate::core::conditional_if_match_gate(if_match, resource_type, Some(&existing))?; - self.delete(tenant, resource_type, existing.id()).await?; + crate::core::delete_under_precondition(self, tenant, if_match, &existing).await?; Ok(ConditionalDeleteResult::Deleted(existing)) } n => { diff --git a/crates/persistence/src/composite/bulk_submit.rs b/crates/persistence/src/composite/bulk_submit.rs index 7110bfad7..de91bc782 100644 --- a/crates/persistence/src/composite/bulk_submit.rs +++ b/crates/persistence/src/composite/bulk_submit.rs @@ -470,6 +470,18 @@ impl ResourceStorage for CompositeSubmitJobs { self.composite.delete(tenant, resource_type, id).await } + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.composite + .delete_versioned(tenant, resource_type, id, expected_version) + .await + } + async fn exists( &self, tenant: &TenantContext, diff --git a/crates/persistence/src/composite/indexing_submit_jobs.rs b/crates/persistence/src/composite/indexing_submit_jobs.rs index ff794e327..918eccfe4 100644 --- a/crates/persistence/src/composite/indexing_submit_jobs.rs +++ b/crates/persistence/src/composite/indexing_submit_jobs.rs @@ -239,6 +239,18 @@ impl ResourceStorage for IndexingSubmitJobs { self.inner.delete(tenant, resource_type, id).await } + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.inner + .delete_versioned(tenant, resource_type, id, expected_version) + .await + } + async fn exists( &self, tenant: &TenantContext, diff --git a/crates/persistence/src/composite/storage.rs b/crates/persistence/src/composite/storage.rs index 1eab608e6..cd60bc584 100644 --- a/crates/persistence/src/composite/storage.rs +++ b/crates/persistence/src/composite/storage.rs @@ -1080,6 +1080,57 @@ impl ResourceStorage for CompositeStorage { Ok(()) } + /// The primary is the system of record for versions, so the compare-and- + /// swap is its alone; secondaries are told about the delete only once it + /// has won. Inheriting the trait's default here would turn the primary's + /// atomic delete back into read-compare-delete (#1404). + #[instrument(skip(self, tenant), fields(resource_type = %resource_type, id = %id))] + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + let result = self + .primary + .delete_versioned(tenant, resource_type, id, expected_version) + .await; + + // A refused precondition is the primary working, not the primary + // failing: racing clients must not be able to mark it unhealthy. + let refused = matches!( + result, + Err(StorageError::Concurrency(_) | StorageError::Resource(_)) + ); + let primary_id = self.config.primary_id().unwrap_or("primary"); + self.update_health( + primary_id, + result.is_ok() || refused, + result + .as_ref() + .err() + .filter(|_| !refused) + .map(|e| e.to_string()), + ); + + result?; + + // Sync to secondaries + if let Err(e) = self + .sync_to_secondaries(SyncEvent::Delete { + resource_type: resource_type.to_string(), + resource_id: id.to_string(), + tenant_id: tenant.tenant_id().clone(), + }) + .await + { + warn!(error = %e, "Failed to sync delete to secondaries"); + } + + Ok(()) + } + async fn count( &self, tenant: &TenantContext, @@ -1609,9 +1660,16 @@ impl ConditionalStorage for CompositeStorage { resource_type, Some(¤t), )?; - self.primary - .delete(tenant, resource_type, current.id()) - .await?; + // `current` is the search backend's copy; the primary's + // compare-and-swap runs on its version, so a stale copy + // is a 409, not a delete (#1404). + crate::core::delete_under_precondition( + self.primary.as_ref(), + tenant, + if_match, + ¤t, + ) + .await?; if let Err(e) = self .sync_to_secondaries(SyncEvent::Delete { diff --git a/crates/persistence/src/core/mod.rs b/crates/persistence/src/core/mod.rs index 7c17469c7..ed035fb5a 100644 --- a/crates/persistence/src/core/mod.rs +++ b/crates/persistence/src/core/mod.rs @@ -160,8 +160,9 @@ pub use history::{ }; pub use preconditions::{ EntityTag, EntityTagPrecondition, MalformedPrecondition, bundle_if_match_gate, - bundle_if_none_exist_gate, conditional_if_match_gate, if_match_field_satisfied, - multiple_matches_entry, not_supported_entry, precondition_failed_entry, + bundle_if_none_exist_gate, conditional_if_match_gate, delete_under_precondition, + if_match_field_satisfied, multiple_matches_entry, not_supported_entry, + precondition_failed_entry, }; pub use search::{ ChainedSearchProvider, FullSearchProvider, INCLUDE_TRUNCATION_OUTCOME_ID, IncludeProvider, diff --git a/crates/persistence/src/core/preconditions.rs b/crates/persistence/src/core/preconditions.rs index aff395673..8f967d909 100644 --- a/crates/persistence/src/core/preconditions.rs +++ b/crates/persistence/src/core/preconditions.rs @@ -496,6 +496,44 @@ pub fn conditional_if_match_gate( )) } +/// Deletes `current` — the resource an `If-Match` precondition has just been +/// evaluated against — so that the evaluation and the delete are one step. +/// +/// With a precondition the delete goes through +/// [`ResourceStorage::delete_versioned`](super::ResourceStorage::delete_versioned), +/// pinned to `current`'s version: a writer landing after the evaluation ends +/// in `VersionConflict` instead of being deleted along with the version the +/// client named (#1404). Without one it is the plain, unconditional +/// [`delete`](super::ResourceStorage::delete) it always was — FHIR's delete +/// carries no precondition of its own. +/// +/// Shared by `DELETE [type]/[id]` and every +/// [`ConditionalStorage::conditional_delete`](super::ConditionalStorage::conditional_delete). +pub async fn delete_under_precondition( + storage: &S, + tenant: &crate::tenant::TenantContext, + if_match: &EntityTagPrecondition, + current: &StoredResource, +) -> crate::error::StorageResult<()> +where + S: super::ResourceStorage + ?Sized, +{ + if if_match.is_present() { + storage + .delete_versioned( + tenant, + current.resource_type(), + current.id(), + current.version_id(), + ) + .await + } else { + storage + .delete(tenant, current.resource_type(), current.id()) + .await + } +} + /// Builds the `412` bundle entry result used by every backend. pub fn precondition_failed_entry(diagnostics: &str) -> BundleEntryResult { BundleEntryResult::error( diff --git a/crates/persistence/src/core/storage.rs b/crates/persistence/src/core/storage.rs index 1d4a49566..ac210860c 100644 --- a/crates/persistence/src/core/storage.rs +++ b/crates/persistence/src/core/storage.rs @@ -574,9 +574,11 @@ pub trait ResourceStorage: Send + Sync { /// # Errors /// /// * `StorageError::Resource(NotFound)` - If no live resource exists - /// (never created, or already deleted) + /// (never created, or already deleted; S3 reports the latter as `Gone`, + /// as its `delete` does) /// * `StorageError::Concurrency(VersionConflict)` - If the current version - /// is not `expected_version`; nothing is deleted + /// is not `expected_version`; nothing is deleted. S3 reports a writer + /// that lands after its comparison as `OptimisticLockFailure`. /// * `StorageError::Tenant` - If the tenant doesn't have delete permission async fn delete_versioned( &self, diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 3aa39e373..088995591 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -759,6 +759,22 @@ async fn mongodb_empty_values_are_rejected_on_every_path() { empty_value_suite::empty_values_are_rejected_on_every_path(&backend, "empty-value-1380").await; } +/// The backend-agnostic race suite for version-aware writes (#1404, #1405). +/// Same `#[path]` arrangement. +#[path = "search/versioned_write_race_suite.rs"] +mod versioned_write_race_suite; + +/// #1404: `delete_versioned` compares and deletes in one step. +#[tokio::test] +async fn mongodb_versioned_delete_is_a_compare_and_swap() { + let Some(backend) = create_backend("delete_cas_1404").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::versioned_delete_is_a_compare_and_swap(&backend, "delete-cas-1404") + .await; +} + /// #1062: a comma-separated value list on one `SearchParameter` is OR per /// FHIR (https://build.fhir.org/search.html#combining) — for date same as /// every other type. Drives the real `SearchProvider::search` / diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 3db141d04..ecf702851 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -108,6 +108,11 @@ mod conditional_if_match_suite; #[path = "search/empty_value_suite.rs"] mod empty_value_suite; +/// The backend-agnostic race suite for version-aware writes (#1404, #1405). +/// Same `#[path]` arrangement. +#[path = "search/versioned_write_race_suite.rs"] +mod versioned_write_race_suite; + #[path = "common/container_cleanup.rs"] mod container_cleanup; @@ -19190,4 +19195,39 @@ mod postgres_integration { assert!(included.is_empty()); } + + /// #1404: of several writers holding the same version, one `update` writes. + #[tokio::test(flavor = "multi_thread", worker_threads = 8)] + async fn postgres_integration_concurrent_updates_from_the_same_version_admit_one() { + let backend = create_backend().await; + super::versioned_write_race_suite::concurrent_updates_from_the_same_version_admit_one( + std::sync::Arc::new(backend), + &unique_base("update_race_1404"), + 10, + ) + .await; + } + + /// #1404: an update and a versioned delete of the same version: one wins. + #[tokio::test(flavor = "multi_thread", worker_threads = 8)] + async fn postgres_integration_concurrent_update_and_versioned_delete_admit_one() { + let backend = create_backend().await; + super::versioned_write_race_suite::concurrent_update_and_versioned_delete_admit_one( + std::sync::Arc::new(backend), + &unique_base("delete_race_1404"), + 10, + ) + .await; + } + + /// #1404: `delete_versioned` compares and deletes in one step. + #[tokio::test] + async fn postgres_integration_versioned_delete_is_a_compare_and_swap() { + let backend = create_backend().await; + super::versioned_write_race_suite::versioned_delete_is_a_compare_and_swap( + &backend, + &unique_base("delete_cas_1404"), + ) + .await; + } } diff --git a/crates/rest/src/handlers/delete.rs b/crates/rest/src/handlers/delete.rs index 726c84fb3..1f6c6e17b 100644 --- a/crates/rest/src/handlers/delete.rs +++ b/crates/rest/src/handlers/delete.rs @@ -37,6 +37,8 @@ use crate::state::AppState; /// - `200 OK` - Resource deleted, returning OperationOutcome /// - `404 Not Found` - Resource does not exist, or is already deleted /// - `412 Precondition Failed` - `If-Match` was supplied and is not satisfied +/// - `409 Conflict` - `If-Match` was satisfied, but another writer changed the +/// resource before the delete landed (#1404); nothing is deleted /// - `405 Method Not Allowed` - `AuditEvent` resources are immutable /// /// # The `If-Match` precondition @@ -169,10 +171,32 @@ where // Perform the delete. Everything above this line is a refusal path; nothing // below it may run for a request that failed its precondition. - state - .storage() - .delete(tenant.context(), &resource_type, &id) - .await?; + // + // With `If-Match` the delete is pinned to the version the precondition was + // just evaluated against, inside storage's own compare-and-swap. A plain + // `delete` here was check-then-act: a writer landing after the read above + // was deleted along with the version the client named, one it never saw + // (#1404). Losing that race is `VersionConflict` -> 409, what `PUT` with + // `If-Match` answers for the same race. A satisfied precondition implies a + // current resource, so the `None` arm is the precondition-less delete of + // something absent: storage's own `NotFound` -> 404, as before. + match existing_resource.as_ref() { + Some(current) => { + helios_persistence::core::delete_under_precondition( + state.storage(), + tenant.context(), + &if_match, + current, + ) + .await? + } + None => { + state + .storage() + .delete(tenant.context(), &resource_type, &id) + .await? + } + } debug!( resource_type = %resource_type, diff --git a/crates/rest/tests/if_match_write_race.rs b/crates/rest/tests/if_match_write_race.rs new file mode 100644 index 000000000..fbe5639bc --- /dev/null +++ b/crates/rest/tests/if_match_write_race.rs @@ -0,0 +1,316 @@ +//! #1404: `If-Match` on the instance `PUT`, `PATCH` and `DELETE` is a +//! compare-and-swap in storage, not a check in the handler followed by an +//! unconditional write. +//! +//! The handlers read the current resource to evaluate the precondition and +//! then write. These tests land a second writer *between* those two steps, +//! deterministically: the primary backend is wrapped so that a `read` can be +//! armed to update the resource right after it has produced its answer. The +//! handler therefore sees version 1, the precondition `W/"1"` is satisfied, +//! and by the time the handler writes, version 2 — which the client never saw +//! — is current. The write must be refused (409) and version 2 must survive. +//! +//! Before the fix `DELETE` answered `204` and deleted version 2. + +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::{Arc, Mutex}; + +use async_trait::async_trait; +use axum::http::StatusCode; +use axum_test::TestServer; +use helios_fhir::FhirVersion; +use helios_persistence::backends::sqlite::{SqliteBackend, SqliteBackendConfig}; +use helios_persistence::composite::{CompositeConfig, CompositeStorage, DynStorage}; +use helios_persistence::core::{BackendKind, ResourceStorage}; +use helios_persistence::error::StorageResult; +use helios_persistence::tenant::TenantContext; +use helios_persistence::types::StoredResource; +use helios_rest::ServerConfig; +use serde_json::{Value, json}; + +fn sqlite() -> SqliteBackend { + let data_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../../data") + .canonicalize() + .expect("repo data dir"); + let backend = SqliteBackend::with_config( + ":memory:", + SqliteBackendConfig { + data_dir: Some(data_dir), + ..Default::default() + }, + ) + .expect("in-memory SQLite"); + backend.init_schema().expect("init schema"); + backend +} + +/// A primary whose next `read` is followed — before the caller can act on what +/// it read — by another writer's update. +struct RacingPrimary { + inner: SqliteBackend, + /// Content the interloper writes after the next `read`; taken when used. + interloper: Mutex>, +} + +#[async_trait] +impl ResourceStorage for RacingPrimary { + fn backend_name(&self) -> &'static str { + "racing-primary" + } + + async fn create( + &self, + tenant: &TenantContext, + resource_type: &str, + resource: Value, + fhir_version: FhirVersion, + ) -> StorageResult { + self.inner + .create(tenant, resource_type, resource, fhir_version) + .await + } + + async fn create_or_update( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + resource: Value, + fhir_version: FhirVersion, + ) -> StorageResult<(StoredResource, bool)> { + self.inner + .create_or_update(tenant, resource_type, id, resource, fhir_version) + .await + } + + async fn read( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + ) -> StorageResult> { + let seen = self.inner.read(tenant, resource_type, id).await?; + let interloper = self.interloper.lock().expect("interloper lock").take(); + if let (Some(seen), Some(content)) = (seen.as_ref(), interloper) { + self.inner + .update(tenant, seen, content) + .await + .expect("the interloper's update lands"); + } + Ok(seen) + } + + async fn update( + &self, + tenant: &TenantContext, + current: &StoredResource, + resource: Value, + ) -> StorageResult { + self.inner.update(tenant, current, resource).await + } + + async fn delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + ) -> StorageResult<()> { + self.inner.delete(tenant, resource_type, id).await + } + + // A wrapper around a real backend delegates; the trait's default is + // read-compare-delete and would reopen the window. + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.inner + .delete_versioned(tenant, resource_type, id, expected_version) + .await + } + + async fn count( + &self, + tenant: &TenantContext, + resource_type: Option<&str>, + ) -> StorageResult { + self.inner.count(tenant, resource_type).await + } +} + +fn server() -> (TestServer, Arc) { + let primary = Arc::new(RacingPrimary { + inner: sqlite(), + interloper: Mutex::new(None), + }); + + let config = CompositeConfig::builder() + .primary("sqlite", BackendKind::Sqlite) + .build() + .expect("composite config"); + let mut backends: HashMap = HashMap::new(); + backends.insert("sqlite".to_string(), primary.clone() as DynStorage); + let composite = Arc::new(CompositeStorage::new(config, backends).expect("composite")); + + let state = helios_rest::AppState::new( + composite, + ServerConfig { + base_url: "http://localhost:8080".to_string(), + ..ServerConfig::for_testing() + }, + ); + let server = TestServer::new(helios_rest::routing::fhir_routes::create_routes(state)) + .expect("create test server"); + (server, primary) +} + +fn patient(id: &str, family: &str) -> Value { + json!({"resourceType": "Patient", "id": id, "name": [{"family": family}]}) +} + +/// Creates `Patient/{id}` and proves it is live at version 1 with `If-Match` +/// honoured at all — the positive control for every refusal below. +async fn seed(server: &TestServer, id: &str) { + let created = server + .put(&format!("/Patient/{id}")) + .json(&patient(id, "Seed")) + .await; + assert_eq!( + created.status_code(), + StatusCode::CREATED, + "{}", + created.text() + ); + + let read = server.get(&format!("/Patient/{id}")).await; + assert_eq!(read.status_code(), StatusCode::OK); + assert_eq!(read.json::()["meta"]["versionId"], "1"); + + let stale = server + .delete(&format!("/Patient/{id}")) + .add_header("If-Match", "W/\"7\"") + .await; + assert_eq!( + stale.status_code(), + StatusCode::PRECONDITION_FAILED, + "an unsatisfied If-Match is refused by the handler's own check" + ); +} + +/// The interloper's version 2 is current and intact. +async fn assert_interloper_survives(server: &TestServer, id: &str) { + let read = server.get(&format!("/Patient/{id}")).await; + assert_eq!( + read.status_code(), + StatusCode::OK, + "version 2 was never seen by the client and must not have been removed: {}", + read.text() + ); + let body = read.json::(); + assert_eq!(body["meta"]["versionId"], "2"); + assert_eq!(body["name"][0]["family"], "Interloper"); +} + +#[tokio::test] +async fn delete_with_if_match_does_not_delete_a_version_written_after_the_check() { + let (server, primary) = server(); + seed(&server, "del").await; + + *primary.interloper.lock().unwrap() = Some(patient("del", "Interloper")); + let response = server + .delete("/Patient/del") + .add_header("If-Match", "W/\"1\"") + .await; + + assert_eq!( + response.status_code(), + StatusCode::CONFLICT, + "the delete lost to a concurrent writer: {}", + response.text() + ); + assert_interloper_survives(&server, "del").await; + + // The client re-reads, sees version 2, and may delete that. + let retry = server + .delete("/Patient/del") + .add_header("If-Match", "W/\"2\"") + .await; + assert_eq!( + retry.status_code(), + StatusCode::NO_CONTENT, + "{}", + retry.text() + ); + assert_eq!( + server.get("/Patient/del").await.status_code(), + StatusCode::GONE + ); +} + +#[tokio::test] +async fn delete_without_if_match_still_deletes_whatever_is_current() { + let (server, primary) = server(); + seed(&server, "plain").await; + + *primary.interloper.lock().unwrap() = Some(patient("plain", "Interloper")); + let response = server.delete("/Patient/plain").await; + + assert_eq!( + response.status_code(), + StatusCode::NO_CONTENT, + "FHIR's delete carries no precondition of its own: {}", + response.text() + ); + assert_eq!( + server.get("/Patient/plain").await.status_code(), + StatusCode::GONE + ); +} + +#[tokio::test] +async fn put_with_if_match_does_not_overwrite_a_version_written_after_the_check() { + let (server, primary) = server(); + seed(&server, "put").await; + + *primary.interloper.lock().unwrap() = Some(patient("put", "Interloper")); + let response = server + .put("/Patient/put") + .add_header("If-Match", "W/\"1\"") + .json(&patient("put", "Overwriter")) + .await; + + assert_eq!( + response.status_code(), + StatusCode::CONFLICT, + "{}", + response.text() + ); + assert_interloper_survives(&server, "put").await; +} + +#[tokio::test] +async fn patch_with_if_match_does_not_overwrite_a_version_written_after_the_check() { + let (server, primary) = server(); + seed(&server, "patch").await; + + *primary.interloper.lock().unwrap() = Some(patient("patch", "Interloper")); + let response = server + .patch("/Patient/patch") + .add_header("If-Match", "W/\"1\"") + .text(r#"[{"op":"replace","path":"/name/0/family","value":"Patcher"}]"#) + .content_type("application/json-patch+json") + .await; + + assert_eq!( + response.status_code(), + StatusCode::CONFLICT, + "{}", + response.text() + ); + assert_interloper_survives(&server, "patch").await; +} From 073ed60a52079412fdc370878eb84f5d4614c14d Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:11:19 -0400 Subject: [PATCH 5/7] fix(mongodb): report a lost write race as a version conflict, not a 500 MongoDB's `update` and `delete` run in a multi-document transaction on a replica set. MongoDB does not queue a conflicting writer behind the first one the way PostgreSQL queues it behind a row lock: the loser's `update_one` fails immediately with Error code 112 (WriteConflict) ... labels: {"TransientTransactionError"} (captured from the replica-set test container with eight tasks holding the same version; it is raised by the write inside the transaction, not at commit, and no duplicate-key error on the history collection was seen). Every driver error on that path was wrapped as `BackendError::Internal`, so the client got `500 Internal Server Error` for "you lost a race, read and retry". The loser wrote nothing; only the classification was wrong. Code 112 and the `TransientTransactionError` label (the transaction did not and will not commit) now surface as `ConcurrencyError::VersionConflict` against whatever is live afterwards - `NotFound` when the winner was a delete - which REST already renders as 409, the answer PostgreSQL gives for the same race. `UnknownTransactionCommitResult` is deliberately not classified: there the write may have landed. No retry where the write carries a precondition: `update` always names a version and so does `delete_versioned`, and the writer they lost to has moved the resource on. The unconditional `delete` - "delete whatever is current" - gets ONE retry after a short pause, the driver's recommended handling of a transient transaction error. Removes the MongoDB exemption from #1399's eight-writer test, and adds the MongoDB runs of `versioned_write_race_suite.rs` plus a race between updates and unconditional deletes (the retried path) on all three backends. Fixes #1405 --- .../src/backends/mongodb/storage.rs | 631 ++++++++++++------ crates/persistence/tests/mongodb_tests.rs | 50 +- crates/persistence/tests/postgres_tests.rs | 14 +- .../search/conditional_if_match_suite.rs | 21 +- .../search/versioned_write_race_suite.rs | 101 +++ crates/persistence/tests/sqlite_tests.rs | 14 +- 6 files changed, 606 insertions(+), 225 deletions(-) diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index 0237f69b1..a8ac8ec0e 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -61,6 +61,67 @@ pub(super) fn is_duplicate_key_error(err: &MongoError) -> bool { err.to_string().contains("E11000") } +/// The server's `WriteConflict` code. +const WRITE_CONFLICT_CODE: i32 = 112; + +/// Pause before the single retry of an unconditional delete that hit a write +/// conflict: long enough for the winner's transaction to commit, so the retry +/// does not just collide with it again. +const WRITE_CONFLICT_RETRY_DELAY: std::time::Duration = std::time::Duration::from_millis(25); + +/// Why one attempt at a versioned write failed. +enum WriteAttemptError { + /// The server refused the write because a concurrent writer got there + /// first ([`is_write_conflict`]); nothing was written. + Conflict(MongoError), + /// Anything else, already in its final form. + Storage(StorageError), +} + +impl WriteAttemptError { + /// Classifies a driver error raised inside the attempt's session. + fn driver(context: &str, err: MongoError) -> Self { + if is_write_conflict(&err) { + Self::Conflict(err) + } else { + Self::Storage(internal_error(format!("{}: {}", context, err))) + } + } +} + +impl> From for WriteAttemptError { + fn from(err: E) -> Self { + Self::Storage(err.into()) + } +} + +/// True when the server refused (and rolled back) a write because another +/// operation got to the document first. +/// +/// Inside a multi-document transaction MongoDB does not queue behind a +/// conflicting writer the way PostgreSQL queues behind a row lock: the loser's +/// write fails at once with `WriteConflict` (112), labelled +/// `TransientTransactionError`, and its transaction is aborted. The label on +/// its own means the same thing for our purposes — the transaction did not and +/// will not commit, and running it again is safe — so both are classified. +/// `UnknownTransactionCommitResult` is deliberately not: there the write may +/// have landed. +/// +/// This used to reach the client as `BackendError::Internal` -> 500 (#1405): +/// "the server failed", for what is "you lost a race, read and retry". +pub(super) fn is_write_conflict(err: &MongoError) -> bool { + if err.contains_label(mongodb::error::TRANSIENT_TRANSACTION_ERROR) { + return true; + } + match err.kind.as_ref() { + MongoErrorKind::Command(command) => command.code == WRITE_CONFLICT_CODE, + MongoErrorKind::Write(mongodb::error::WriteFailure::WriteError(write)) => { + write.code == WRITE_CONFLICT_CODE + } + _ => false, + } +} + pub(super) fn ensure_resource_identity(resource_type: &str, id: &str, resource: &mut Value) { if let Some(obj) = resource.as_object_mut() { obj.insert( @@ -616,17 +677,28 @@ async fn commit_best_effort_multi_write_session( transaction_active: bool, operation: &str, ) -> StorageResult<()> { + try_commit_best_effort_multi_write_session(session, transaction_active) + .await + .map_err(|e| { + internal_error(format!( + "Failed to commit MongoDB transaction after {}: {}", + operation, e + )) + }) +} + +/// [`commit_best_effort_multi_write_session`] with the driver's error intact, +/// for the writes that classify a commit-time `WriteConflict`. +async fn try_commit_best_effort_multi_write_session( + session: &mut Option, + transaction_active: bool, +) -> Result<(), MongoError> { if !transaction_active { return Ok(()); } if let Some(active_session) = session.as_mut() { - active_session.commit_transaction().await.map_err(|e| { - internal_error(format!( - "Failed to commit MongoDB transaction after {}: {}", - operation, e - )) - })?; + active_session.commit_transaction().await?; } Ok(()) @@ -945,192 +1017,23 @@ impl ResourceStorage for MongoBackend { current: &StoredResource, resource: Value, ) -> StorageResult { - let resource_type = current.resource_type(); - tenant.check_permission(Operation::Update, resource_type)?; - - let db = self.get_database().await?; - let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); - let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); - let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; - let tenant_id = tenant.tenant_id().as_str(); - let id = current.id(); - - let current_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "is_deleted": false, - }; - - let maybe_existing = if let Some(active_session) = session.as_mut() { - resources - .find_one(current_filter.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!("Failed to load current resource (session): {}", e)) - })? - } else { - resources - .find_one(current_filter) - .await - .map_err(|e| internal_error(format!("Failed to load current resource: {}", e)))? - }; - - let Some(existing_doc) = maybe_existing else { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - }; - - let actual_version = existing_doc - .get_str("version_id") - .map_err(|e| internal_error(format!("Missing current version: {}", e)))? - .to_string(); - - if actual_version != current.version_id() { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_version: current.version_id().to_string(), - actual_version, - }, - )); - } - - let new_version = next_version(current.version_id())?; - - let mut resource = resource; - ensure_resource_identity(resource_type, id, &mut resource); - let payload = value_to_document(&resource)?; - - let now = Utc::now(); - let now_bson = chrono_to_bson(now); - let fhir_version = current.fhir_version(); - let fhir_version_str = fhir_version.as_mime_param().to_string(); - - let update_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": current.version_id(), - "is_deleted": false, - }; - let update_doc = doc! { - "$set": { - "version_id": &new_version, - "data": Bson::Document(payload.clone()), - "last_updated": now_bson, - "is_deleted": false, - "deleted_at": Bson::Null, - "fhir_version": &fhir_version_str, - } - }; - - let update_result = if let Some(active_session) = session.as_mut() { - resources - .update_one(update_filter.clone(), update_doc.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!("Failed to update resource (session): {}", e)) - })? - } else { - resources - .update_one(update_filter, update_doc) - .await - .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))? - }; - - if update_result.matched_count == 0 { - let latest = resources - .find_one(doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - }) - .await - .map_err(|e| { - internal_error(format!("Failed to reload version conflict state: {}", e)) - })?; - - let actual = latest - .as_ref() - .and_then(|d| d.get_str("version_id").ok()) - .unwrap_or("unknown") - .to_string(); - - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_version: current.version_id().to_string(), - actual_version: actual, - }, - )); - } - - let created_at = extract_created_at(&existing_doc, now); - - let history_doc = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": &new_version, - "data": Bson::Document(payload), - "created_at": chrono_to_bson(created_at), - "last_updated": now_bson, - "is_deleted": false, - "deleted_at": Bson::Null, - "fhir_version": fhir_version_str, - }; - - if let Some(active_session) = session.as_mut() { - history - .insert_one(history_doc) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!( - "Failed to insert updated history row (session): {}", - e - )) - })?; - } else { - history.insert_one(history_doc).await.map_err(|e| { - internal_error(format!("Failed to insert updated history row: {}", e)) - })?; - } - - self.index_resource(&db, tenant_id, resource_type, id, &resource, &mut session) - .await?; - - commit_best_effort_multi_write_session(&mut session, transaction_active, "update").await?; - - // A SearchParameter update may change a tenant's overlay (status flips, - // expression edits): refresh the stored-param cache and drop registries. - // This must run after the commit above: `reload_stored_cache` reads the - // `resources` collection without the session, so it cannot observe the - // update while the transaction is still open. - if resource_type == "SearchParameter" { - if let Err(e) = self.reload_stored_cache().await { - tracing::warn!("SearchParameter cache reload failed: {e}"); - } + // No retry: `update` always carries a precondition (`current`'s + // version), and a writer that beat us to the document has, or is about + // to have, moved it on. The honest answer is the one PostgreSQL gives + // for the same race — `VersionConflict` -> 409 — not a second attempt. + match self.update_attempt(tenant, current, resource).await { + Ok(stored) => Ok(stored), + Err(WriteAttemptError::Storage(e)) => Err(e), + Err(WriteAttemptError::Conflict(e)) => Err(self + .lost_race( + tenant, + current.resource_type(), + current.id(), + current.version_id(), + &e, + ) + .await), } - - Ok(StoredResource::from_storage( - resource_type, - id, - new_version, - tenant.tenant_id().clone(), - resource, - created_at, - now, - None, - fhir_version, - )) } async fn delete( @@ -1819,6 +1722,207 @@ impl SearchIndexDocuments { } impl MongoBackend { + /// One attempt at [`ResourceStorage::update`]: the compare-and-swap on + /// `current`'s version, the history row and the search index, in one + /// transaction where the deployment has them. A write the server refused + /// because another writer holds the document comes back as + /// [`WriteAttemptError::Conflict`] rather than as an `Internal` error. + async fn update_attempt( + &self, + tenant: &TenantContext, + current: &StoredResource, + resource: Value, + ) -> Result { + let resource_type = current.resource_type(); + tenant.check_permission(Operation::Update, resource_type)?; + + let db = self.get_database().await?; + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); + let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; + let tenant_id = tenant.tenant_id().as_str(); + let id = current.id(); + + let current_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "is_deleted": false, + }; + + let maybe_existing = if let Some(active_session) = session.as_mut() { + resources + .find_one(current_filter.clone()) + .session(active_session) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to load current resource (session)", e) + })? + } else { + resources + .find_one(current_filter) + .await + .map_err(|e| internal_error(format!("Failed to load current resource: {}", e)))? + }; + + let Some(existing_doc) = maybe_existing else { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + }) + .into()); + }; + + let actual_version = existing_doc + .get_str("version_id") + .map_err(|e| internal_error(format!("Missing current version: {}", e)))? + .to_string(); + + if actual_version != current.version_id() { + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: current.version_id().to_string(), + actual_version, + }) + .into(), + ); + } + + let new_version = next_version(current.version_id())?; + + let mut resource = resource; + ensure_resource_identity(resource_type, id, &mut resource); + let payload = value_to_document(&resource)?; + + let now = Utc::now(); + let now_bson = chrono_to_bson(now); + let fhir_version = current.fhir_version(); + let fhir_version_str = fhir_version.as_mime_param().to_string(); + + let update_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": current.version_id(), + "is_deleted": false, + }; + let update_doc = doc! { + "$set": { + "version_id": &new_version, + "data": Bson::Document(payload.clone()), + "last_updated": now_bson, + "is_deleted": false, + "deleted_at": Bson::Null, + "fhir_version": &fhir_version_str, + } + }; + + let update_result = if let Some(active_session) = session.as_mut() { + resources + .update_one(update_filter.clone(), update_doc.clone()) + .session(active_session) + .await + .map_err(|e| WriteAttemptError::driver("Failed to update resource (session)", e))? + } else { + resources + .update_one(update_filter, update_doc) + .await + .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))? + }; + + if update_result.matched_count == 0 { + let latest = resources + .find_one(doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + }) + .await + .map_err(|e| { + internal_error(format!("Failed to reload version conflict state: {}", e)) + })?; + + let actual = latest + .as_ref() + .and_then(|d| d.get_str("version_id").ok()) + .unwrap_or("unknown") + .to_string(); + + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: current.version_id().to_string(), + actual_version: actual, + }) + .into(), + ); + } + + let created_at = extract_created_at(&existing_doc, now); + + let history_doc = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": &new_version, + "data": Bson::Document(payload), + "created_at": chrono_to_bson(created_at), + "last_updated": now_bson, + "is_deleted": false, + "deleted_at": Bson::Null, + "fhir_version": fhir_version_str, + }; + + if let Some(active_session) = session.as_mut() { + history + .insert_one(history_doc) + .session(active_session) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to insert updated history row (session)", e) + })?; + } else { + history.insert_one(history_doc).await.map_err(|e| { + internal_error(format!("Failed to insert updated history row: {}", e)) + })?; + } + + self.index_resource(&db, tenant_id, resource_type, id, &resource, &mut session) + .await?; + + try_commit_best_effort_multi_write_session(&mut session, transaction_active) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to commit MongoDB transaction after update", e) + })?; + + // A SearchParameter update may change a tenant's overlay (status flips, + // expression edits): refresh the stored-param cache and drop registries. + // This must run after the commit above: `reload_stored_cache` reads the + // `resources` collection without the session, so it cannot observe the + // update while the transaction is still open. + if resource_type == "SearchParameter" { + if let Err(e) = self.reload_stored_cache().await { + tracing::warn!("SearchParameter cache reload failed: {e}"); + } + } + + Ok(StoredResource::from_storage( + resource_type, + id, + new_version, + tenant.tenant_id().clone(), + resource, + created_at, + now, + None, + fhir_version, + )) + } + /// The version of the live (not deleted) resource, read outside any /// session — what a write that just lost a race reports as the version it /// lost to. @@ -1857,6 +1961,109 @@ impl MongoBackend { id: &str, expected_version: Option<&str>, ) -> StorageResult<()> { + let mut retried = false; + loop { + let conflict = match self + .soft_delete_attempt(tenant, resource_type, id, expected_version) + .await + { + Ok(()) => return Ok(()), + Err(WriteAttemptError::Storage(e)) => return Err(e), + Err(WriteAttemptError::Conflict(e)) => e, + }; + + // ONE more attempt, and only for a delete with no precondition: + // "delete whatever is current" means the same thing after the + // other writer commits, which is the driver's recommended handling + // of a transient transaction error. A delete that names a version + // is never run again — the writer it lost to has moved the resource + // on, and the client must see that (`VersionConflict` -> 409). + if expected_version.is_none() && !retried { + retried = true; + tracing::debug!( + resource_type, + id, + error = %conflict, + "MongoDB write conflict on an unconditional delete; retrying once" + ); + tokio::time::sleep(WRITE_CONFLICT_RETRY_DELAY).await; + continue; + } + + return Err(self + .lost_race( + tenant, + resource_type, + id, + expected_version.unwrap_or("unknown"), + &conflict, + ) + .await); + } + } + + /// Reports a write the server refused with `WriteConflict` as the + /// concurrency error it is: `VersionConflict` against whatever is live now, + /// or `NotFound` when the winner was a delete. The winner may not have + /// committed yet, in which case the live version still reads as the + /// expected one and is reported as `unknown` rather than as a conflict of a + /// version with itself. A failure of this read must not turn a 409 back + /// into a 500, so it degrades to `unknown` too. + async fn lost_race( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + conflict: &MongoError, + ) -> StorageError { + tracing::debug!( + resource_type, + id, + expected_version, + error = %conflict, + "MongoDB write conflict: a concurrent writer won" + ); + + let live = match self.get_database().await { + Ok(db) => { + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + self.live_version(&resources, tenant.tenant_id().as_str(), resource_type, id) + .await + } + Err(e) => Err(e), + }; + + match live { + Ok(None) => StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + }), + Ok(Some(actual)) if actual != expected_version => { + StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version: actual, + }) + } + Ok(Some(_)) | Err(_) => StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version: "unknown".to_string(), + }), + } + } + + /// One attempt at [`Self::soft_delete`]; see [`Self::update_attempt`]. + async fn soft_delete_attempt( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> Result<(), WriteAttemptError> { tenant.check_permission(Operation::Delete, resource_type)?; let db = self.get_database().await?; @@ -1878,10 +2085,7 @@ impl MongoBackend { .session(active_session) .await .map_err(|e| { - internal_error(format!( - "Failed to check resource before delete (session): {}", - e - )) + WriteAttemptError::driver("Failed to check resource before delete (session)", e) })? } else { resources @@ -1896,7 +2100,8 @@ impl MongoBackend { return Err(StorageError::Resource(ResourceError::NotFound { resource_type: resource_type.to_string(), id: id.to_string(), - })); + }) + .into()); }; let current_version = existing_doc @@ -1906,14 +2111,15 @@ impl MongoBackend { if let Some(expected) = expected_version && expected != current_version { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { resource_type: resource_type.to_string(), id: id.to_string(), expected_version: expected.to_string(), actual_version: current_version, - }, - )); + }) + .into(), + ); } let new_version = next_version(¤t_version)?; @@ -1952,7 +2158,7 @@ impl MongoBackend { .session(active_session) .await .map_err(|e| { - internal_error(format!("Failed to soft-delete resource (session): {}", e)) + WriteAttemptError::driver("Failed to soft-delete resource (session)", e) })? } else { resources @@ -1969,19 +2175,21 @@ impl MongoBackend { .live_version(&resources, tenant_id, resource_type, id) .await? { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { resource_type: resource_type.to_string(), id: id.to_string(), expected_version: expected.to_string(), actual_version: actual, - }, - )); + }) + .into(), + ); } return Err(StorageError::Resource(ResourceError::NotFound { resource_type: resource_type.to_string(), id: id.to_string(), - })); + }) + .into()); } let history_doc = doc! { @@ -2003,10 +2211,7 @@ impl MongoBackend { .session(active_session) .await .map_err(|e| { - internal_error(format!( - "Failed to insert deletion history row (session): {}", - e - )) + WriteAttemptError::driver("Failed to insert deletion history row (session)", e) })?; } else { history.insert_one(history_doc).await.map_err(|e| { @@ -2017,7 +2222,11 @@ impl MongoBackend { self.delete_search_index(&db, tenant_id, resource_type, id, &mut session) .await?; - commit_best_effort_multi_write_session(&mut session, transaction_active, "delete").await?; + try_commit_best_effort_multi_write_session(&mut session, transaction_active) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to commit MongoDB transaction after delete", e) + })?; // A SearchParameter delete may remove a tenant's overlay entry: refresh // the stored-param cache and drop registries. This must run after the diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 088995591..995a4e475 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -742,7 +742,6 @@ async fn mongodb_conditional_writers_with_the_same_if_match_admit_one() { conditional_if_match_suite::concurrent_writers_with_the_same_if_match_admit_one( &backend, "cond-if-match-race-1381", - false, ) .await; } @@ -764,6 +763,55 @@ async fn mongodb_empty_values_are_rejected_on_every_path() { #[path = "search/versioned_write_race_suite.rs"] mod versioned_write_race_suite; +/// #1405: of several writers holding the same version, one `update` writes and +/// every loser is a `ConcurrencyError` — the server's `WriteConflict` used to +/// reach them as `BackendError::Internal`. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn mongodb_concurrent_updates_from_the_same_version_admit_one() { + let Some(backend) = create_backend("update_race_1405").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::concurrent_updates_from_the_same_version_admit_one( + std::sync::Arc::new(backend), + "update-race-1405", + 10, + ) + .await; +} + +/// #1404: an update and a versioned delete of the same version: one wins. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn mongodb_concurrent_update_and_versioned_delete_admit_one() { + let Some(backend) = create_backend("delete_race_1404").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::concurrent_update_and_versioned_delete_admit_one( + std::sync::Arc::new(backend), + "delete-race-1404", + 10, + ) + .await; +} + +/// #1405: an update racing an unconditional delete — the one write that is +/// retried after a `WriteConflict` — leaves a contiguous history and no +/// `Internal` error. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn mongodb_concurrent_update_and_plain_delete_stay_consistent() { + let Some(backend) = create_backend("plain_delete_race_1405").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::concurrent_update_and_plain_delete_stay_consistent( + std::sync::Arc::new(backend), + "plain-delete-race-1405", + 10, + ) + .await; +} + /// #1404: `delete_versioned` compares and deletes in one step. #[tokio::test] async fn mongodb_versioned_delete_is_a_compare_and_swap() { diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index ecf702851..099e34a56 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -19089,7 +19089,6 @@ mod postgres_integration { super::conditional_if_match_suite::concurrent_writers_with_the_same_if_match_admit_one( &backend, &unique_base("cond_if_match_race_1381"), - true, ) .await; } @@ -19220,6 +19219,19 @@ mod postgres_integration { .await; } + /// #1404: an update racing an unconditional delete leaves a contiguous + /// history. + #[tokio::test(flavor = "multi_thread", worker_threads = 8)] + async fn postgres_integration_concurrent_update_and_plain_delete_stay_consistent() { + let backend = create_backend().await; + super::versioned_write_race_suite::concurrent_update_and_plain_delete_stay_consistent( + std::sync::Arc::new(backend), + &unique_base("plain_delete_race_1404"), + 10, + ) + .await; + } + /// #1404: `delete_versioned` compares and deletes in one step. #[tokio::test] async fn postgres_integration_versioned_delete_is_a_compare_and_swap() { diff --git a/crates/persistence/tests/search/conditional_if_match_suite.rs b/crates/persistence/tests/search/conditional_if_match_suite.rs index 2017c7e90..63b6994dc 100644 --- a/crates/persistence/tests/search/conditional_if_match_suite.rs +++ b/crates/persistence/tests/search/conditional_if_match_suite.rs @@ -328,16 +328,16 @@ pub async fn if_match_is_evaluated_against_the_resolved_match( /// resolved after the winner, or by the swap (`VersionConflict`) if it resolved /// before. None of them may write version 3. /// -/// `losers_are_concurrency_errors` is `false` for MongoDB: its `update` runs in -/// a multi-document transaction, and a racing loser there surfaces the server's -/// `WriteConflict` as `BackendError::Internal` rather than as a -/// `ConcurrencyError`. The loser still writes nothing — which is what this -/// asserts for every backend — but its error is not classified. -pub async fn concurrent_writers_with_the_same_if_match_admit_one( - backend: &S, - base: &str, - losers_are_concurrency_errors: bool, -) where +/// That holds on every backend. MongoDB used to be exempt from the second +/// half: its `update` runs in a multi-document transaction, and a racing loser +/// there surfaced the server's `WriteConflict` as `BackendError::Internal` +/// (#1405). +/// +/// These writers are futures on one task, so a backend whose calls never yield +/// (SQLite) runs them one after another; `versioned_write_race_suite.rs` is the +/// test with real parallelism. +pub async fn concurrent_writers_with_the_same_if_match_admit_one(backend: &S, base: &str) +where S: ResourceStorage + ConditionalStorage + SearchProvider, { let t = tenant(base, "race"); @@ -374,7 +374,6 @@ pub async fn concurrent_writers_with_the_same_if_match_admit_one( for result in &results { match result { Ok(ConditionalUpdateResult::Updated(_)) | Err(StorageError::Concurrency(_)) => {} - Err(_) if !losers_are_concurrency_errors => {} other => panic!("a loser is a concurrency refusal, nothing else: {other:?}"), } } diff --git a/crates/persistence/tests/search/versioned_write_race_suite.rs b/crates/persistence/tests/search/versioned_write_race_suite.rs index 956db52a6..b8626626e 100644 --- a/crates/persistence/tests/search/versioned_write_race_suite.rs +++ b/crates/persistence/tests/search/versioned_write_race_suite.rs @@ -274,6 +274,107 @@ pub async fn concurrent_update_and_versioned_delete_admit_one( } } +/// Half the tasks `update` from version 1 and half `delete` with no +/// precondition. A plain delete removes whatever is current, so an update and +/// a delete may both land (as versions 2 and 3) — but only one of each, every +/// other task is refused cleanly, and the history is contiguous. On MongoDB +/// this is the path with the one bounded retry of a write conflict (#1405). +pub async fn concurrent_update_and_plain_delete_stay_consistent( + backend: Arc, + base: &str, + rounds: usize, +) where + S: ResourceStorage + VersionedStorage + Send + Sync + 'static, +{ + let t = tenant(base, "plain-delete-race"); + + for round in 0..rounds { + let id = format!("race-{round}"); + let current = seed(backend.as_ref(), &t, &id).await; + let barrier = Arc::new(Barrier::new(WRITERS)); + + let mut tasks = Vec::new(); + for n in 0..WRITERS { + let (backend, t, current, barrier, id) = ( + backend.clone(), + t.clone(), + current.clone(), + barrier.clone(), + id.clone(), + ); + tasks.push(tokio::spawn(async move { + barrier.wait().await; + if n % 2 == 0 { + match backend + .update(&t, ¤t, patient(&id, &format!("Writer{n}"))) + .await + { + Ok(stored) => Outcome::Updated(stored), + Err(e) => Outcome::Failed(e), + } + } else { + match backend.delete(&t, "Patient", &id).await { + Ok(()) => Outcome::Deleted, + Err(e) => Outcome::Failed(e), + } + } + })); + } + let mut outcomes = Vec::new(); + for task in tasks { + outcomes.push(task.await.expect("writer task")); + } + let context = format!("round {round}"); + + let updates = outcomes + .iter() + .filter(|o| matches!(o, Outcome::Updated(_))) + .count(); + let deletes = outcomes + .iter() + .filter(|o| matches!(o, Outcome::Deleted)) + .count(); + assert!( + updates <= 1, + "{context}: one update from version 1: {outcomes:?}" + ); + assert!( + deletes <= 1, + "{context}: a resource is deleted once: {outcomes:?}" + ); + assert!( + updates + deletes >= 1, + "{context}: somebody wins: {outcomes:?}" + ); + assert_losers_are_concurrency_errors(&outcomes, true, &context); + + let expected_versions: Vec = + (1..=1 + updates + deletes).map(|v| v.to_string()).collect(); + assert_eq!( + backend + .list_versions(&t, "Patient", &id) + .await + .expect("list versions"), + expected_versions, + "{context}: one version per successful write, no gap, no duplicate: {outcomes:?}" + ); + + let after = backend.read(&t, "Patient", &id).await; + if deletes == 1 { + assert!( + matches!( + after, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) + ), + "{context}: a delete landed, so nothing is live: {after:?}" + ); + } else { + let stored = after.expect("read after race").expect("still live"); + assert_eq!(stored.version_id(), "2", "{context}"); + } + } +} + /// `delete_versioned` is a compare-and-swap on the *current* version: a stale /// version is refused and deletes nothing, the current one deletes, and a /// second delete of the same version finds nothing live. diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 6168f3709..756c26b51 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -93,7 +93,6 @@ async fn sqlite_conditional_writers_with_the_same_if_match_admit_one() { conditional_if_match_suite::concurrent_writers_with_the_same_if_match_admit_one( &backend, "cond-if-match-race-1381", - true, ) .await; } @@ -141,6 +140,19 @@ async fn sqlite_concurrent_update_and_versioned_delete_admit_one() { .await; } +/// #1404: an update racing an unconditional delete leaves a contiguous history. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn sqlite_concurrent_update_and_plain_delete_stay_consistent() { + let dir = tempfile::tempdir().expect("tempdir"); + let backend = std::sync::Arc::new(create_file_backend(&dir)); + versioned_write_race_suite::concurrent_update_and_plain_delete_stay_consistent( + backend, + "plain-delete-race-1404", + 40, + ) + .await; +} + /// #1404: `delete_versioned` compares and deletes in one step. #[tokio::test] async fn sqlite_versioned_delete_is_a_compare_and_swap() { From e325b0bcaace2ba211104d44c3e290c9a03393a6 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 13:11:30 -0400 Subject: [PATCH 6/7] docs(persistence): the If-Match check and the delete are now one step Doc comments on conditional_if_match_gate, VersionedStorage::delete_with_match and the conditional DELETE handler still described the check-then-act window that #1404 closed: a delete under a precondition now goes through ResourceStorage::delete_versioned, pinned to the version that was compared. --- crates/persistence/src/core/preconditions.rs | 3 ++- crates/persistence/src/core/versioned.rs | 5 ++++- crates/rest/src/handlers/delete.rs | 9 +++++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/persistence/src/core/preconditions.rs b/crates/persistence/src/core/preconditions.rs index 8f967d909..184a74a9d 100644 --- a/crates/persistence/src/core/preconditions.rs +++ b/crates/persistence/src/core/preconditions.rs @@ -460,7 +460,8 @@ pub fn if_match_field_satisfied(raw: &str, current_version_id: &str) -> bool { /// this between resolving the match and writing, and then hands *that* row to /// `update`, whose compare-and-swap is keyed on the version evaluated here. A /// writer landing in between therefore ends in `VersionConflict`, never in a -/// write over a version the client did not name. +/// write over a version the client did not name. A delete gets the same +/// guarantee from [`delete_under_precondition`]. /// /// The failure is [`ConcurrencyError::OptimisticLockFailure`], which the REST /// layer already renders as `412`. `id` is empty when nothing matched. diff --git a/crates/persistence/src/core/versioned.rs b/crates/persistence/src/core/versioned.rs index 12bddda07..35dee5594 100644 --- a/crates/persistence/src/core/versioned.rs +++ b/crates/persistence/src/core/versioned.rs @@ -135,7 +135,10 @@ pub trait VersionedStorage: ResourceStorage { /// # Errors /// /// * `StorageError::Resource(NotFound)` - If the resource doesn't exist - /// * `StorageError::Concurrency(VersionConflict)` - If versions don't match + /// * `StorageError::Concurrency(VersionConflict)` - If versions don't match, + /// including when a writer lands after the comparison: implementations + /// delete through [`ResourceStorage::delete_versioned`], pinned to the + /// version they compared (#1404) /// * `StorageError::Tenant` - If the tenant doesn't have delete permission async fn delete_with_match( &self, diff --git a/crates/rest/src/handlers/delete.rs b/crates/rest/src/handlers/delete.rs index 1f6c6e17b..3bc3c6fe7 100644 --- a/crates/rest/src/handlers/delete.rs +++ b/crates/rest/src/handlers/delete.rs @@ -266,10 +266,11 @@ where /// below — as it does on `DELETE [type]/[id]` for a resource that does not /// exist: no current representation satisfies `If-Match` (RFC 9110 §13.1.1). /// -/// The check and the delete are not one atomic step. No backend's `delete` -/// compares-and-swaps on a version (`update` does), so a writer landing between -/// the two is deleted along with the version the client named — the same -/// window [`delete_handler`] has, not a wider one. +/// The check and the delete are one step (#1404): with `If-Match` the backend +/// deletes through `ResourceStorage::delete_versioned`, pinned to the version +/// it evaluated, so a writer landing between the two is answered `409` instead +/// of being deleted along with the version the client named — as on +/// [`delete_handler`]. /// /// # No match /// From ec4587bebc3ba06bf93a74f564d491a16a9a5d9f Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 13:32:01 -0400 Subject: [PATCH 7/7] style(rest): drop a needless borrow in delete_handler clippy 1.98.1 (needless_borrow): `if_match` is already a reference. --- crates/rest/src/handlers/delete.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rest/src/handlers/delete.rs b/crates/rest/src/handlers/delete.rs index a0aba2756..87575d288 100644 --- a/crates/rest/src/handlers/delete.rs +++ b/crates/rest/src/handlers/delete.rs @@ -185,7 +185,7 @@ where helios_persistence::core::delete_under_precondition( state.storage(), tenant.context(), - &if_match, + if_match, current, ) .await?