fix(persistence)+ci: FHIR-benchmark bug fixes (PG token search, tx rollback, sqlite locks) + timeouts + truthful reporting#219
Merged
Conversation
The first full internal run surfaced bugs in the reporting itself: - Err% column always showed 0.00%: it read http_req_failed.rate, but that is a k6 Rate metric whose fraction lives in .value (.rate exists only on Counter metrics). Now reads .value — e.g. postgres import correctly shows 17.4% instead of 0%. - p99 column always 0: default summary trend stats omit p(99). Add --summary-trend-stats so p(99) is exported. - Added a Checks ✓/✗ column (bolded when any fail) — the clearest signal of a suite that 'ran' but had failing assertions. - Added a log_level input (default info) to enable HFS_LOG_LEVEL=debug runs for diagnosing the postgres search 500s. - Wipe bench-results before a run (checkout is clean:false) so a stale sibling-backend dir doesn't ride along in the uploaded artifact.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… benchmark The first full internal benchmark run (issue #190) exposed three real backend bugs. All are in helios-persistence's FHIR resource paths, which the HTS terminology benchmark does not exercise, so HTS parity is unaffected. 1. PG token search with a comma-OR value list returned 500 ('Failed to execute search: db error'). build_token_condition numbered placeholders with a fixed 2-slot stride (offset + i*2), but code-only values bind only 1 param, so e.g. status=finished,in-progress emitted $3 and $5 while binding 2 params — an out-of-range placeholder PG rejects. Now tracks a running offset that advances by each value's actual param count. Every benchmark token OR-list query hit this. + 2 regression tests. 2. PG transaction-bundle import failure cascade (~35% of imports at 20 VUs). A transaction dropped without explicit commit/rollback returned its connection to the deadpool with an OPEN transaction (the old Drop comment wrongly assumed recycling auto-rolls-back — deadpool does not reset session state), so the next checkout failed with 'already a transaction in progress' and cascaded. Drop now moves the client into a spawned task that ROLLBACKs before the connection returns to the pool (guarded for the no-runtime case). 3. SQLite 'database is locked' under concurrent writes (~95% of imports at 20 VUs). busy_timeout and foreign_keys are per-connection PRAGMAs but were set on only one pooled connection in configure_connection(); the rest defaulted to busy_timeout=0 and failed writes immediately instead of waiting. Now applied in the r2d2 with_init hook so every connection gets them. (WAL stays in configure_connection — it's persistent and DB-level.) Refs #190.
…bulk import Root-caused the residual import failures (they were config, not logic): - PG import ~20% failures were the 30s HFS_REQUEST_TIMEOUT cutting large Synthea transaction bundles mid-flight under 20-VU load (import p95 pinned at 30s; 'Transaction failed' logged 0 times because the handler future was cancelled before its error path — the prior Drop-rollback fix is what now cleanly rolls those back). Set HFS_REQUEST_TIMEOUT=900 in the benchmark to match import.js's client timeout and the uncapped comparators. - SQLite import 'database is locked' (writers failing at BEGIN): busy_timeout now applies to every connection (prior fix) but 5s was too short to queue behind 20 concurrent write transactions. Raise the default to 30s — only affects contended-write wait time, safe for read-heavy workloads (HTS). PG search stays fixed at 0% (token OR-list). Refs #190.
…steady state The import fix fully populates Postgres (~1.3M rows), and search then ran at ~8s median with 3 queries hitting the 30s statement_timeout — not an index gap (the indexes exist) but missing planner statistics on the freshly bulk-loaded tables, so the planner seq-scans search_index. Autovacuum would ANALYZE eventually, but the suite searches immediately after import. Run ANALYZE once after the import suite (postgres) so the search suite reflects steady-state performance rather than a cold-stats artifact. Refs #190.
mauripunzueta
changed the title
…easures steady state" This reverts commit f57f495. The ANALYZE-after-import experiment was validated in run 28911652793 (postgres-only import+search) and had no measurable effect: search latency stayed at med 7.18s / p99 51s / max 65s, statistically identical to the pre-ANALYZE run 28908173123 (med ~8s / p99 45s / max 62s). So the slowness is NOT a stale-planner-statistics artifact — it is a genuine query-plan/concurrency problem at full data volume that needs EXPLAIN ANALYZE diagnosis. Removing the dead step; tracking the real issue separately as a FHIR-benchmark follow-up. Refs #190.
This was referenced Jul 8, 2026
Open
smunini
approved these changes
Jul 8, 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
2 participants
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.
Follow-up to #218 (FHIR benchmark workflow). Started as step-summary reporting
fixes; grew to carry the fixes for the bugs the first full internal benchmark
run (issue #190, item 3) surfaced, plus the config needed to run the suites
fairly. Kept on one PR per request.
Backend correctness / concurrency fixes (
helios-persistence)1. Postgres token search returned 500 on comma-OR value lists.
build_token_conditionnumbered SQL placeholders with a fixed 2-slot stride(
offset + i*2), but a code-only token value binds only one param — soEncounter?status=finished,in-progressemitted$3and$5while binding twoparams → out-of-range placeholder → "Failed to execute search: db error". Every
token OR-list query in the search suite hit this. Fixed to advance by each
value's actual param count. +2 regression tests.
2. Postgres transaction-bundle import failure cascade. A transaction dropped
without explicit commit/rollback returned its connection to the deadpool with an
open transaction (the old
Dropcomment wrongly assumed recyclingauto-rolls-back — deadpool does not reset session state), so the next checkout
failed with "already a transaction in progress" and cascaded.
Dropnow movesthe client into a spawned task that
ROLLBACKs before the connection returns tothe pool (guarded for the no-runtime case).
3. SQLite "database is locked" under concurrent writes.
busy_timeoutandforeign_keysare per-connection PRAGMAs but were set on only one pooledconnection; the rest defaulted to
busy_timeout = 0and failed writes instantlyinstead of waiting. Now applied in the r2d2
with_inithook (every connection),and the default raised 5s → 30s so writers queue behind the bulk-import backlog.
Benchmark config (workflow)
HFS_REQUEST_TIMEOUT=900— the 30s default was cutting large (1,600-entry)transaction bundles off mid-flight under the 20-VU import load (import p95 was
pinned at 30s; the handler future was cancelled, not erroring). Comparators
impose no such cap and import.js allows 900s client-side.
Reporting fixes (the original scope)
http_req_failed.rate, but thatis a k6 Rate metric whose fraction lives in
.value(.rateexists only onCounter metrics). This is what masked every failure in the first run.
p99(via--summary-trend-stats) and a Checks ✓/✗ column.log_levelinput (enablesHFS_LOG_LEVEL=debugreruns — used to pinthe token-OR search bug) and
bench-resultscleanup (checkout isclean:false).Results (Postgres — the backend all comparators use)
SQLite import improved (47.5% → 39%) but remains write-serialized by design under
the 20-VU import; the public comparison is Postgres-based.
Regression safety
All fixes are benchmark-workflow config or in
helios-persistence's FHIRresource paths (transaction bundles, resource search index) — not the HTS
terminology hot paths. The one shared change (SQLite
busy_timeoutdefault) onlyaffects how long a contended write waits before erroring, so it can't slow the
read-heavy terminology workload. CI (Test Rust) is green. HTS-benchmark
no-regression run still to be done.
Refs #190.