fix(sqlite): delete resource_fts on every purge path (#386) - #458
Open
mauripunzueta wants to merge 4 commits into
Open
fix(sqlite): delete resource_fts on every purge path (#386)#458mauripunzueta wants to merge 4 commits into
mauripunzueta wants to merge 4 commits into
Conversation
`resource_fts` is an FTS5 *virtual* table, so it can carry no foreign key
and the `ON DELETE CASCADE` from `resources` never reached it — and no
SQLite purge path deleted from it explicitly. Purged resources therefore
left their narrative and their entire serialized body in the database
after the API reported them removed, and a resource reusing the purged
logical id resurrected that text as a match oracle through `_text` /
`_content`. PostgreSQL was unaffected: its `resource_fts` is an ordinary
table with `ON DELETE CASCADE`.
Adds a probe-guarded, strict `DELETE FROM resource_fts` to `purge`,
`purge_all` and `purge_tenant_data`. Strict because a swallowed failure
on a purge path means the API attests to an erasure it did not perform;
probe-guarded because FTS5 is an optional compile-time feature and a
database built without it legitimately has no table. Ungated by
`is_search_offloaded()`: that flag is a write-path optimisation, while
this is an erasure guarantee, and a deployment that indexed locally
before moving search to Elasticsearch still has rows to remove.
`purge` and `purge_all` were four independent autocommit statements, so
a failure partway through produced the very orphan state this prevents,
indistinguishable on retry from a purge that never ran. Both are now
transactional. All three use `TransactionBehavior::Immediate`: they read
before they write, and under WAL a deferred transaction fails that
upgrade with `SQLITE_BUSY_SNAPSHOT`, for which the busy handler — and so
the configured `busy_timeout` — is never consulted.
Also fixes a latent data-loss bug on BOTH backends that blocked the
fourth purge path. `run_reindex` deletes every resource's search entries
via `delete_search_entries`, which drops the `resource_fts` row, while
`write_search_entries` never rebuilt it — so `$reindex` silently
disabled `_text`/`_content` until each resource was next written, with
or without `clear_existing`. `write_search_entries` now repopulates FTS
on SQLite and PostgreSQL, which is what makes adding the FTS delete to
SQLite's `clear_search_index` safe rather than destructive.
Migration v14 -> v15 sweeps orphans already on disk: the code fix alone
stops new ones but leaves every database that has ever served a purge
still holding the content. Soft-deleted resources keep their `resources`
row and are correctly preserved.
`/ui/tenants` discarded `purge_tenant_data`'s error after already
deregistering the tenant, rendering success while the data remained;
it now surfaces the failure.
Tests: a backend-agnostic suite shared by SQLite and PostgreSQL. It
asserts on raw `resource_fts` rows rather than search results, because
purge clears `search_index` and the orphaned row is therefore
unreachable through search — the obvious "search finds nothing after
purge" test passes on the unfixed code. Each scenario opens with a
positive control so it cannot pass vacuously. Verified against unfixed
source: 10 of 13 fail, including the id-reuse oracle. Two existing
assertions that accepted zero results ("FTS unavailable") are tightened,
since they would stay green through a total full-text outage.
The PostgreSQL query builder had no arm for `_text`/`_content`: both are
`SearchParamType::Special`, which fell through to the `None` case, so the
parameter contributed no condition at all. A search whose only parameter
was `_text` therefore built an empty filter and `search`/`search_count`
answered a full-text query with **every** non-deleted resource of that
type. Not a false negative — an unfiltered result set presented as a
match. SQLite, Elasticsearch and MongoDB have all handled both parameters
for as long as they have existed; PostgreSQL alone silently ignored them,
while `resource_fts` was populated on every write and its tsvector
columns and GIN indexes sat unused by the ordinary search path.
`TextSearchProvider::search_text` does implement it correctly, but
nothing calls it: the REST layer goes through `SearchProvider::search`.
Both parameters now resolve through the same `resource_fts` columns the
write path fills, with `plainto_tsquery('english', …)` to match
`search_text`/`search_content`, and the term bound rather than spliced so
tsquery operators in user input stay data. The sub-select is scoped by
`tenant_id = $1` **and** `resource_type = $2`: it yields a bare
`resource_id` set that the outer query intersects with this tenant's
resources, so without the tenant predicate tenant B's Patient/123 would
select tenant A's Patient/123 — a cross-tenant match oracle — and without
the type predicate an Observation's narrative would select a Patient of
the same id. Both binds are already tenant and resource type in every
caller of `build_search_query`, the invariant `build_missing_condition`
and `build_compartment_condition` already rely on.
This is what the two failing PostgreSQL cases in the #386 suite were
reporting. The suite asserts through the public search API precisely so
that a backend which cannot filter cannot pass, and PostgreSQL could not:
the id-reuse oracle re-created a resource under the purged id and `_text`
returned it, not because the purged narrative survived (PostgreSQL's
purge does delete the row) but because the search matched everything.
Its positive controls passed for the same reason they were meant to
catch — one resource in the tenant is indistinguishable from a filter
that matched it.
Both backends' FTS conditions also numbered placeholders off the loop
index while skipping unmatchable terms, so a skipped value left a gap:
on SQLite `_text=*,fracture` emitted `?4` while binding one parameter,
which SQLite rejects when preparing the statement. Numbering now follows
the accepted terms.
And a term that survives neither escaping (SQLite) nor is present at all
(blank) no longer drops the parameter, which would return the whole
resource type — the same fail-open this commit exists to close. It now
matches nothing, which is what a stopword-only term already does through
`plainto_tsquery`.
Conflict in `SqliteBackend::purge_tenant_data`: main (#313) added the per-user settings sweep inside the transaction at the same seam where this branch added the `resource_fts` delete (#386). Both belong there — they clear different stores that the `resources` cascade cannot reach — so both are kept, FTS first, still inside the one IMMEDIATE transaction whose write lock a second connection would deadlock against. No migration renumber needed: main is still at SQLite schema v14, so this branch's v14 -> v15 is unchanged.
…erms Two CI failures on this branch, with two unrelated causes. 1. `_content` never saw the narrative on PostgreSQL. `collect_strings` skips the `div` key to keep raw XHTML markup out of the index, which also dropped the narrative text from `full_content` entirely -- so a term appearing only in `text.div` was findable through `_text` and invisible to `_content`. FHIR defines `_content` as a search over the entire content of the resource, which makes it a superset of `_text`; SQLite has always had the narrative in `_content`. Append the already-stripped narrative to `full_content`, so the markup still stays out (as does base64 `data`), and pin the superset relationship with unit tests. This is what `reindex_preserves_full_text_search` was failing on, not the reindex fix. 2. The residency probe is deliberately not tenant-scoped, so it counted rows planted by *other* scenarios: PostgreSQL runs every scenario concurrently against one shared container database, and several end with a live indexed row on purpose. A distinct tenant per scenario was not enough because the planted token was a shared constant. Derive the token from the tenant id instead -- the probe stays global, and still catches a surviving row under any tenant, type or id, but only counts rows this scenario planted.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
mauripunzueta
marked this pull request as ready for review
July 31, 2026 14:27
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.
Closes #386.
What was wrong
resource_ftsis an FTS5 virtual table, so it can carry no foreign key and theON DELETE CASCADEfromresourcesnever reached it — and no SQLite purge path deleted from it explicitly. A purged resource left its narrative and its entire serialized body in the database after the API reported it removed. PostgreSQL was unaffected: itsresource_ftsis an ordinary table withON DELETE CASCADE(postgres/schema.rs:224-225).The issue's harm model needs one correction, which changed the whole test design. An orphaned row is not directly returned by a search:
_text/_contentcompile toresource_id IN (SELECT … FROM resource_fts …)AND-ed onto an outer query oversearch_index, and purge does clearsearch_index. The two real harms are:$purgeexists to remove the bytes; they were still there.Patient/p1, create a newPatient/p1, and the old resource's text is matchable again and attributed to the new resource. Demonstrated failing on unfixed code (left: ["p1"], right: []). The SQLite FTS sub-select also lacks aresource_typepredicate (sqlite/search/query_builder.rs:566) where Postgres joins on it, so residue leaks across resource types on id collision.What this changes
The three purge paths (
sqlite/storage.rs) get a probe-guarded, strictDELETE FROM resource_fts.let _ =: a swallowed failure on a purge path means the API attests to an erasure it did not perform — the same defect class as the issue, reached another way.is_search_offloaded(): that flag is a write-path optimisation; this is an erasure guarantee. A deployment that indexed locally before moving search to Elasticsearch still has rows to remove.purge/purge_allare now transactional. They were four independent autocommit statements, so a failure partway through produced exactly the orphan state this prevents — and was indistinguishable on retry from a purge that never ran, because the retry hits the not-found guard. All three paths useTransactionBehavior::Immediate: they read before they write, and under WAL a deferred transaction fails that upgrade withSQLITE_BUSY_SNAPSHOT, for which the busy handler — and thereforebusy_timeout— is never consulted.A latent data-loss bug on both backends, which blocked the fourth path.
run_reindexcallsdelete_search_entriesfor every resource unconditionally (search/reindex.rs:832), which drops theresource_ftsrow, whilewrite_search_entriesnever rebuilt it. So$reindexsilently disabled_text/_contentuntil each resource was next written — on SQLite and PostgreSQL, with or withoutclear_existing.write_search_entriesnow repopulates FTS on both backends. That is what makes adding the FTS delete to SQLite'sclear_search_indexsafe rather than destructive; adding it alone would have imported the data loss.Migration v14 → v15 sweeps orphans already on disk. The code fix alone stops new orphans but leaves every database that has ever served a purge still holding the content. Soft-deleted resources keep their
resourcesrow and are correctly preserved./ui/tenantsdiscardedpurge_tenant_data's error after already deregistering the tenant — rendering success while the data remained. It now surfaces the failure.Cross-backend scope
Verified each backend rather than assuming:
resource_fts(FTS5 virtual, no FK possible)resource_fts(ordinary table,ON DELETE CASCADE)write_search_entriesfixed;_text/_contentnever applied at all — fixed, see below_text/_contentreturnTextSearchNotAvailablesearch_index_fts(the other FTS5 table, external-content oversearch_index) is deliberately untouched: it is trigger-maintained, and I verified empirically thatON DELETE CASCADEfires itsAFTER DELETEtrigger withrecursive_triggersboth on and off. A directDELETEagainst an external-content table would be a corruption hazard.Tests
New backend-agnostic suite (
tests/search/fts_purge_suite.rs) shared by SQLite and PostgreSQL via#[path], following theif_match_suite.rsprecedent. It asserts on rawresource_ftsrows, not search results — the obvious "search finds nothing after purge" test passes on the unfixed code and proves nothing. Every scenario opens with a positive control so it cannot pass vacuously.Verified non-vacuous: against unfixed source, 10 of 13 fail. The 3 that pass are exactly the ones that should (FTS5 build pin, table-exists, FTS5-absent tolerance).
Coverage: residency for all three purge paths; over-broad-delete guard (bystander tenant survives); id-reuse and tenant-reuse oracles; repeated purge/recreate cycles;
$reindexpreserves full-text in both modes; repeated reindex doesn't duplicate rows; migration sweeps orphans but preserves soft-deleted; purge tolerates an FTS5-less database.Two existing assertions that accepted zero results ("FTS unavailable") are tightened — they would have stayed green through a total full-text outage, which is how the
$reindexbug survived.Local: full
helios-persistence(sqlite) suite 1,196 passing;helios-rest+helios-uigreen;cargo fmt; clippy clean with CI's exact flags. PostgreSQL scenarios are unverified locally — no Docker here — so they need CI. Note this must not be stacked (CI only triggers forbase: main) and must not come from a fork (DOCKER_HOSTis a secret).Follow-on fix: PostgreSQL never applied
_text/_contentCI reported the two id-reuse oracles failing on PostgreSQL. The cause was not purge — PostgreSQL's purge does delete the row — it was that PostgreSQL had no
_text/_contenthandling at all. Both areSearchParamType::Special, which fell through to theNonearm ofbuild_parameter_condition, so the parameter contributed no condition; a search whose only parameter was_textbuilt an empty filter andsearch/search_countreturned every non-deleted resource of that type. Not a false negative — an unfiltered result set presented as a full-text match.resource_ftswas populated on every write and its tsvector columns and GIN indexes went unused by the ordinary search path.TextSearchProvider::search_textimplements it correctly but has no callers; the REST layer goes throughSearchProvider::search.That is exactly what the suite is built to catch, and why it asserts through the public API as well as on raw rows: a backend that cannot filter cannot pass the id-reuse oracle. Its positive controls passed on the broken code for the reason they exist to expose — with one resource in the tenant, "returned everything" and "matched correctly" are the same answer.
Both parameters now resolve through the same
resource_ftscolumns the write path fills, withplainto_tsquery('english', …)matchingsearch_text/search_content, and the term bound rather than spliced so tsquery operators in user input stay data. The sub-select is scoped bytenant_id = $1andresource_type = $2— it yields a bareresource_idset the outer query intersects with this tenant's resources, so without the tenant predicate tenant B'sPatient/123selects tenant A'sPatient/123, and without the type predicate an Observation's narrative selects a Patient of the same id.Two smaller defects in the same code, one per backend:
_text=*,fractureemitted?4while binding one parameter, which SQLite rejects at prepare time.*) nor is present at all (blank) dropped the parameter, returning the whole resource type — the same fail-open. It now matches nothing, as a stopword-only term already does throughplainto_tsquery.Covered by DB-free unit tests in both builders (so they run on the Windows job too): column selection, tenant/type scoping, gap-free numbering, and fail-closed behaviour.
Deliberately out of scope
Found while doing this; each is filed here rather than as a separate issue, per the follow-ups-in-the-PR-body convention:
PostgresBackend::search_multiignores every search parameter (search_impl.rs:369) — it buildsSELECT … WHERE tenant_id = $1 AND resource_type IN (…)and never calls the query builder, so cross-type search returns the tenant's resources unfiltered. Same fail-open shape as the_textdefect above, wider blast radius, and it needs its own change._filteris unimplemented on PostgreSQL — SQLite handles it, PostgreSQL drops it the way it used to drop_text. Not fixed here because_filterneeds an expression compiler, not a one-arm dispatch; it should fail closed (or 400) rather than return everything in the meantime.Neither backend advertises
_text/_contentinSearchCapabilityProvider, though SQLite has implemented both all along and the CapabilityStatement lists_text. The declaration and the implementation are maintained independently, which is how this gap stayed invisible.PG's
let _ =FTS deletes in the three purge paths (postgres/storage.rs:2178, 2230) are inert — the cascade plus the strictDELETE FROM resourcestwo lines later is the real guarantee. Left alone rather than widening failure modes on the production backend.delete_search_indexearly-returns when search is offloaded, skipping both thesearch_indexandresource_ftsdeletes. On a migrated database that leaves stale rows for live resources forever. Same shape on SQLite, PG and Mongo — a staleness bug, not a purge bug.DELETEis not erasure. Freed pages retain the bytes until overwritten;secure_deleteis off and nothing runsVACUUM. Measured: sweeping 25k of 52k rows took the file from 138 MB to 140 MB. This PR removes the content from the database's logical content, not from the file. If HFS wants to claim regulatory erasure that needs a deliberate decision.resource_ftskey columns areUNINDEXED, so everyDELETE … WHERE tenant_id = ?is a full scan (measured 30.7 ms at 52k rows, 60.9 ms at 102k). Not a new cost class —delete_search_indexalready pays it on every update. Aresource_fts_maprowid table would make both O(1) (measured 32–290× faster).composite/storage.rs:1047purges the primary before secondaries, unlikepurge/purge_all; a primary failure leaves the Elasticsearch copy undeleted.transaction.rshas its own indexing path that never touches FTS, so resources created via a Bundle transaction are absent from_text/_contenton SQLite.Tables surviving
purge_tenant_data:bulk_export_jobs/progress/files,bulk_submissions,bulk_manifests,bulk_entry_results,bulk_submission_changes,bulk_submit_files,user_settings. Most notable isbulk_submission_changes.previous_content, a complete verbatim prior copy of a resource that survives a full tenant purge — worth its own issue.bulk_*_filesadditionally reference NDJSON outside the database, which no DB-level purge can reach.