fix(hts): incremental sqlite concept FTS (stop full rebuild on every boot) (#295)#338
Merged
Merged
Conversation
…n every boot The sqlite HTS server rebuilt the entire concepts FTS index (concepts_fts, concepts_word_fts, concepts_search_fts) over all concepts on every startup, synchronously before binding. With a full terminology corpus this exceeds the readiness window in a debug build, so the server never binds and /health never answers — breaking the sqlite HTS benchmark leg (#295). Give FTS the same lifecycle concept closures already have: - write_code_system invalidates the re-imported system's FTS content rows and its concepts_fts_built tracker row, in the same transaction as the concept upserts, so the tracker is the authoritative "FTS current" flag. - prebuild_concepts_fts is now incremental: guarded INSERTs build only systems missing from concepts_fts_built, it skips the whole transaction when nothing is stale, and returns the count built. finalize_after_bootstrap and new_inner drop the unconditional DELETE wipe and only ANALYZE when a build actually ran. - CLI `hts import` finalizes the DB (closures + FTS) so a served DB ships index-ready and the server-start finalize is a sub-second no-op. - delete_code_system clears the deleted system's FTS rows + tracker (the FTS5 tables have no FK to cascade from), preventing orphans now that boot no longer wipes. Postgres needs no change: it searches a live pg_trgm GIN index maintained by the engine on every write (already incremental); a parity test pins that invariant. Fixes #295
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
mauripunzueta
marked this pull request as ready for review
July 21, 2026 23:04
smunini
approved these changes
Jul 22, 2026
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
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
Fixes #295. The sqlite HTS server rebuilt the entire concept FTS index (
concepts_fts,concepts_word_fts,concepts_search_fts) over all concepts on every startup, synchronously beforeTcpListener::bind().finalize_after_bootstrap()did an unconditionalDELETEof all FTS +prebuild_concepts_ftsover the full corpus. With a full terminology corpus this exceeds the readiness window in a debug build, so the server never binds and/health(which just returns 200 once bound) never answers — breaking the sqlite HTS benchmark leg.concepts_search_fts(added in #273) pushed it over the edge.Root cause is architectural: FTS was the one index without the invalidate-on-import → incremental-startup-rebuild → prebuild-in-CLI-import lifecycle that concept closures already have. This PR gives FTS that same lifecycle, so the per-boot rebuild is removed, not deferred or hidden.
What changed (SQLite)
write_code_systemnow deletes the re-imported system's rows fromconcepts_fts,concepts_word_fts,concepts_search_ftsand itsconcepts_fts_builttracker row, in the same transaction as the concept upserts (mirrors the adjacent closure/implicit invalidation). This makesconcepts_fts_builtthe authoritative "FTS current for this system" flag.prebuild_concepts_ftsbuilds only systems missing fromconcepts_fts_built(aNOT IN (…)guard on every INSERT — mandatory now the wipe is gone, since FTS5 has no row uniqueness), skips the whole transaction via a cheap up-front gate when nothing is stale, and returns the count built.finalize_after_bootstrapandnew_innerdrop the unconditionalDELETE FROM concepts_fts…and only runANALYZEwhen a build actually ran (stats persist across restarts).hts importships an index-ready DB — it now finalizes (closures + FTS) at the end of the untimed import, so the server-start finalize is a sub-second no-op. Previously only closures were pre-built here, leaving the full FTS rebuild to block startup.delete_code_systemclears the deleted system's FTS rows + tracker (the FTS5 tables have no FK to cascade from), preventing orphans now that boot no longer wipes.Net effect: a warm restart, and the benchmark's
hts import→ serve flow, do zero FTS work before the server binds. Filtered$expandcorrectness is unchanged (and strengthened — a live$importthat changes a display is now reflected without a restart), via the existing lazyensure_concepts_ftsfallback.All-DB consistency
Postgres needs no code change. It searches a live
pg_trgmGIN index onconcepts.display(postgres/schema.rs) that the engine maintains transactionally on every write — already incremental, no per-boot rebuild, no tracker. This PR adds a Postgres parity test to pin that invariant as executable.Not a masking workaround
The banned pattern hides a cost while leaving the work unchanged (e.g. widening the readiness window, which #292 tried and the issue explicitly rejects). This PR does the opposite: it removes the per-boot rebuild and moves the genuinely-once cost onto the once event (import), exactly as closures already do. No
sleep, no timeout bump, no backgrounding-past-the-probe. The correctness contract is strengthened, and the new tests fail loudly if a future change reintroduces a per-boot rebuild or drops the invalidation.Design provenance
Approach vetted by a 5-persona review (SQLite internals, SRE/readiness, performance/benchmark, FHIR-terminology correctness, architecture) — all five converged on this incremental + prebuild-on-import shape and rejected background-after-bind (write-lock contention would pollute the first EX0x benchmark iterations, plus a half-built
concepts_search_ftshazard via the non-trackerEXISTSprobe).Tests
value_set.rs— incremental build is idempotent / no duplicate rows; rebuilds only the invalidated system.fhir_bundle.rs— re-import clears the FTS tracker + content (direct Leg A guard).terminology_import.rs— end-to-end: re-import changes a display → filtered$expandreflects the new display, stale one gone.postgres_http_tests.rs— Postgres parity for the same re-import → filtered$expandbehavior.Verification note
Built and formatted with
cargo fmt; the local environment has no C linker socargo check/testcan't link here — relying on CI to compile and run the suite (incl. the Postgres testcontainer parity test). Draft until CI (and ideally a benchmark preflight run on the sqlite leg) is green.