fix(persistence): atomic versioned update/delete; MongoDB lost race is 409, not 500 - #1421
Merged
Merged
Conversation
`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
…: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
… 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
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
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
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.
clippy 1.98.1 (needless_borrow): `if_match` is already a reference.
…omic-versioned-writes #1419 and this PR each add a backend-agnostic suite (modifier_parity_suite there, versioned_write_race_suite here) and wire it into the same spots of tests/mongodb_tests.rs and tests/postgres_tests.rs, so whichever merges second conflicts. Resolved here, making the order #1419 then this PR; every test kept unchanged. The resulting tree is byte-identical to the corresponding step of a scratch merge of main with all four open PRs (#1419, #1421, #1422, #1423), which compiled under `cargo test --workspace --all-features --no-run` and passed the full SQLite, PostgreSQL, MongoDB, Elasticsearch and REST suites.
smunini
added a commit
that referenced
this pull request
Sep 21, 2026
…nto feat/1406-shared-conditional-patch #1419, #1421 and this PR each add a backend-agnostic suite and wire it into the same spots of tests/mongodb_tests.rs, tests/postgres_tests.rs and tests/sqlite_tests.rs, so whichever merges later conflicts. Resolved here, making the order #1419, #1421, then this PR; every test kept unchanged. The resulting tree is byte-identical to the corresponding step of a scratch merge of main with all four open PRs (#1419, #1421, #1422, #1423), which compiled under `cargo test --workspace --all-features --no-run` and passed the full SQLite, PostgreSQL, MongoDB, Elasticsearch and REST suites.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR closes two version-aware-write gaps that were found while wiring
If-Matchinto conditional interactions (#1399). Both affect the instance endpoints (PUT,DELETE,PATCHwithIf-Match) as well as the conditional ones.What was broken
#1404: SQLite
updatewas not a compare-and-swap.SELECT version_id, a comparison in Rust, and then anUPDATEwith no version in itsWHERE. Each statement auto-committed on a pooled connection.UPDATEoverwrote the first and committed. Only its historyINSERTthen failed on the primary key.500while its content was already the current row, under a version whose history entry held the winner's content.409.join_allon a current-thread runtime, where synchronous rusqlite calls never interleave.#1404: no backend had an atomic versioned delete.
DELETEwithIf-Matchwas read, compare, then a plaindelete(id), anddelete_with_matchdid the same.DELETEwithIf-Match: W/"1"answered204and deleted v2.#1405: MongoDB answered
500to the loser of a write race.WriteConflict(code 112, labelTransientTransactionError) is raised byupdate_oneinside the transaction.BackendError::Internal, which REST maps to500. It should be409.Fix
SQLite
updatenow carries the expected version in theUPDATEpredicate. Zero rows meansVersionConflict, orNotFoundwhen nothing is live.IMMEDIATEtransaction, so a loser leaves nothing behind.All backends
ResourceStorage::delete_versioned, the delete half of optimistic locking. Each backend implements it as one conditional statement:IMMEDIATEtransaction.delete_with_match, the conditional-delete gate, and the instance and conditional RESTDELETEhandlers all go through it, pinned to the version that was compared. They share a new helper,core::delete_under_precondition.MongoDB
TransientTransactionErrornow map to the sameVersionConflictPostgreSQL returns. REST answers409, or412for an unsatisfiedIf-Match.conditional_if_match_suite.rsis removed.Behaviour changes clients can observe
DELETEwithIf-Matchthat loses a race now answers409. Before, it deleted a version the client never saw.409on a lost update. Before, it answered500and left a corrupted current row and history.409on a lost update or delete race. Before, it answered500.Verification
versioned_write_race_suite.rs, for update and for delete:ConcurrencyError, that the stored state and the history are the winner's, and that there are no gaps or duplicates.if_match_write_race.rs.Run after merging current
main, on real containers:cargo test -p helios-persistence --libwith all five backend features: 1891 passed.sqlite_tests,search_suiteand four composite suites: 268 passed.postgres_tests(full): 271 passed.mongodb_tests(full), run twice to check the race tests are stable: 209 passed both times.cargo test -p helios-rest: 1720 passed.cargo +1.98.1 clippywith the CI flags is clean:helios-persistencewith all four FHIR versions and all five backends,--all-targetshelios-restandhelios-hfs,--all-targetsneedless_borrow, fixed in the last commit.elasticsearch_tests: 140 of 140 on a re-run.es_integration_composite_bad_date_stays_searchable_in_every_sync_mode(no_shard_availableon a fresh index).Not run on this branch alone: the full
--workspace --all-featurescompile, which runs on the combined tree of the open PRs, and the S3 integration suites.Fixes #1404
Fixes #1405