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
…ery backend Conditional PATCH was unavailable on MongoDB (unimplemented) and on every composite with a dedicated search backend (refused since #1384; a silent no-match before). Root cause: patch application was not available at the persistence level. REST, SQLite and PostgreSQL each had a private applier, so the composite - which resolves criteria through its search backend and writes through its primary - had nothing to apply a patch with, and MongoDB had none at all. - core/patch.rs: `apply_patch` (JSON Patch, JSON Merge Patch) with typed `PatchError`s, carried as `ValidationError::Patch`. It refuses a patch that changes or removes `resourceType` / `id`, and refuses FHIRPath Patch. - `ConditionalStorage::conditional_patch` is now a provided implementation: `resolve_conditional_matches` -> exactly one -> `read` (the primary's content on a composite; a search copy of another version is a VersionConflict) -> `conditional_if_match_gate` -> `apply_patch` -> `update` (compare-and-swap). - SQLite, PostgreSQL: private appliers and `conditional_patch` deleted; they provide only the resolver. The `apply_fhirpath_patch` stubs, which changed nothing on most paths and still wrote a new version, are gone. - MongoDB and CompositeStorage provide the resolver and declare `ConditionalPatch`; the composite's #1384 refusal is removed. - REST applies instance PATCH with the same applier and maps `PatchError` in one place (a failed `test` op has its own arm, for #1393). Fixes #1406
…: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
… with a search backend The #1384 tests pinned the composite's refusal (501, conditionalPatch: false). With the shared applier the composite serves it, so the same tests now pin that: PATCH [type]?criteria patches the one match (If-Match, no match, identity changes and a failed test op refused), the search backend follows the write, and /metadata advertises it. A patched id is refused on both patch endpoints. Refs #1406
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.
…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
Conditional PATCH (
PATCH [type]?criteria) was unavailable on MongoDB, where it was unimplemented. It was also unavailable on every composite with a dedicated search backend (sqlite-elasticsearch,postgres-elasticsearch,mongodb-elasticsearch,s3-elasticsearch).501since CapabilityStatement advertises conditional interactions the backend lacks (MongoDB patch, S3 all); CompositeStorage has no FHIR version #1384.This PR makes it work on all of them, from one implementation.
Root cause
Change
core/patch.rs: oneapply_patchfor JSON Patch (RFC 6902) and JSON Merge Patch (RFC 7396).PatchErrors, carried asValidationError::Patch.resourceTypeorid.ConditionalStorage::conditional_patchis now a provided implementation. The steps are:readthe current resource.conditional_if_match_gate.apply_patch.update, which is the compare-and-swap.VersionConflict, so a stale search index can't cause a patch to be applied to the wrong content.conditional_patchimplementations are deleted. They now provide only the criteria resolver.apply_fhirpath_patchstubs are gone. On most paths they changed nothing, yet still wrote a new version.CompositeStorageprovide the resolver and declareConditionalPatch. The composite's CapabilityStatement advertises conditional interactions the backend lacks (MongoDB patch, S3 all); CompositeStorage has no FHIR version #1384 refusal is removed./metadataand the501have come from one source since fix: derive conditional interactions in the CapabilityStatement (and their 501) from the storage; FHIR version for CompositeStorage #1401.PatchErrorin one place.testoperation has its own arm, so PATCH skips HFS_VALIDATION_MODE write-path validation; a failed JSON Patch test op returns 400 instead of 422 #1393 (422instead of400) becomes a one-line change.Behaviour changes clients can observe
*-elasticsearchcomposite./metadatareportsconditionalPatch: truefor them on R5 and R6. R4 and R4B have no such element.501.PATCHand conditional PATCH on SQLite and PostgreSQL go through the same applier as before in behaviour: same statuses, same invariants.Verification
All of this ran after merging current
main, on real containers.cargo test -p helios-persistence --lib, all five backend featuressqlite_tests,search_suite, four composite suites,backend_capability_contractpostgres_tests(full)mongodb_tests(full)cargo test -p helios-restcomposite_conditional_capabilitieswith--features R4,R4B,R5,R6,sqlitecomposite_conditional_capabilitiesrun compiles and runs the test gated onall(R4, R5), which a default build skips.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-targetscargo fmt --checkis clean.Not run on this branch alone:
elasticsearch_tests. Elasticsearch has no conditional storage of its own.--workspace --all-featurescompile. That is run on the combined tree of the open PRs.Fixes #1406