fix(inferno): US Core nightly — HFS_BASE_URL and SearchParameter overlay invalidation (#787) - #826
Merged
Merged
Conversation
HFS advertised Bundle.entry.fullUrl using the default http://localhost:8080 base, unreachable from Inferno's separate container. Mirrors the HFS_BASE_URL="http://$RUNNER_IP:$HFS_PORT" pattern already used by inferno-subscription.yml and inferno-bulk-data.yml. Fixes the care_team_role_search_test connection-refused failure in #787. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ransaction-bundle writes sqlite and postgres transaction-bundle processing (BundleProvider::process_transaction) never invalidated the per-tenant SearchParameter registry cache when a SearchParameter was created/updated/deleted inside a Bundle entry, unlike their own non-transactional create/update/delete paths and unlike MongoDB's transaction path. A SearchParameter POSTed inside a transaction Bundle (e.g. US Core's asserted-date param, registered by Inferno's setup bundle) therefore never took effect until the TTL cache refresh, so the search silently dropped the unrecognized parameter under lenient handling and over-returned. sqlite: invalidate() is sufficient — its per-tenant loader queries storage live on next access. postgres needed reload_stored_cache() instead, matching every other SearchParameter write path on that backend: its loader reads a synchronous in-memory cache that only that method refreshes from the database. Adds a regression test (crates/rest/tests/transaction_bundle_search_parameter.rs, sqlite + postgres via testcontainers) that posts a SearchParameter in one transaction Bundle and resources indexed under it in a second — mirroring Inferno's sequential file-load pattern — and asserts the search filters correctly immediately. Confirmed to fail with the exact #787 symptom before this fix. Fixes the asserted-date search failure in #787. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The s3-elasticsearch composite handed Elasticsearch a base_only() TenantSearchRegistries whose loader unconditionally returned an empty overlay for every tenant — S3 had no query capability of its own and nothing populated a real stored-param cache. A SearchParameter POSTed (or PUT/restored) never took effect, not even eventually: there was no TTL refresh for this backend either. S3Backend now owns a real per-tenant registry, refreshed synchronously inside create/update/delete/restore_deleted via scan_live_resources (strongly-consistent LIST+GET), before the write call returns — correct under both Synchronous and Asynchronous composite sync modes, since it never depends on Elasticsearch having received the document yet. This mirrors the reload-cache pattern already used by postgres and mongodb, adapted for S3's lack of a live query and its need to enumerate tenants via list_tenants() for the TTL sweep (reload_stored_cache). start_s3_elasticsearch now populates S3's own registries.base() (the same embedded+spec params the removed build_search_registry() used to load into a throwaway container) and shares that real container with Elasticsearch via with_shared_registry — the same pattern every other composite already uses to give its search backend a live overlay. Regression test confirmed to fail with the exact #787 symptom before this fix and pass after, no regressions across 275 existing s3/ elasticsearch/composite tests. Completes the #787 fix for the 5th (s3-elasticsearch) backend. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
reload_stored_cache_for_tenant (added for #787) fetched every stored SearchParameter object sequentially via scan_live_resources — one GET per key, awaited one at a time. Harmless with a handful of objects, but every seeded spec SearchParameter is a real, individually-keyed S3 object indistinguishable from a stored one, so a tenant with the full ~1.4k-object spec seed turned the very next genuinely new SearchParameter write into a ~30s sequential-GET stall. Inferno's HTTP client timed out mid-write (408) on exactly that path in CI (us_core_v311, s3-elasticsearch) — a real regression from the #787 S3 fix, not a pre-existing issue. scan_live_resources now fans the GETs out concurrently via bulk_write_concurrency(), the same limit (32) the seeder already uses for the matching write fan-out (search/seeder.rs) — the reason that same ~1.4k-object seed only takes seconds, not minutes. Also benefits scan_live_resources's other caller (the SOF in-process resource scan); order was never significant to either caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Format the #787 fix files (sqlite/postgres/s3 storage.rs) — the "Linting" CI check caught these on the first push. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TenantSearchRegistries::for_tenant couldn't tell "the tenant has no stored SearchParameters" apart from "the load itself failed" — both came back as an empty Vec from the loader closure, and either way the empty result was cached for the tenant's process lifetime. This turned any transient loader failure into a permanent one: SQLite's loader (load_tenant_stored_params) already logged and swallowed connection/prepare/query errors into an empty Vec on purpose, but never had a way to signal "retry me" instead of "tenant has nothing". Root- caused via a byte-for-byte local reproduction: the self-hosted Linux CI runner running Test Rust links a SQLite build with SQLITE_OMIT_SHARED_CACHE, silently making our `cache=shared` in-memory URI inert — each pooled connection gets its own empty database. r2d2 is LIFO so almost every operation reuses the one connection that has the data, except begin_transaction, which needs a second connection at the same moment the registry loader does — that second connection sees an empty "resources" table, the loader swallows the resulting query error, and the tenant's overlay is cached as permanently empty. This is what made crates/rest/tests/transaction_bundle_search_parameter.rs's sqlite test fail twice in CI while passing every time locally (a normal SQLite build makes the failure window this narrow to hit almost impossible to reproduce off that runner). StoredParamLoader now returns Option<Vec<SearchParameterDefinition>>: `Some(vec![])` means "loaded fine, tenant has no stored overlay" (the legitimate empty case — S3/postgres/mongodb's cache-backed loaders, which can't fail this way, always return this); `None` means the load could not be attempted or failed, and for_tenant does not cache that outcome, so the next access retries instead of being stuck. A row that individually fails to parse is still skipped without failing the whole load — only the connection/query itself failing returns None. This is a genuine robustness fix independent of the CI-runner-specific trigger: any transient storage hiccup during a registry rebuild would have hit the same permanent-poisoning bug on any backend. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… connection begin_transaction built the transaction's SearchParameterExtractor via tenant_extractor -> for_tenant -> the loader closure -> pool.get(), a *second*, simultaneous connection alongside the one the transaction itself already held. On a SQLite build where cache=shared is silently inert (confirmed on the self-hosted CI runner running Test Rust), that second connection could see a different, empty in-memory database: "no such table: resources". The previous fix (Option<Vec<...>>, d4e5f8f) stopped that failure from being permanently cached, but a transaction's extractor is a snapshot built once and reused by index_resource for every entry in that transaction (transaction.rs's SqliteTransaction.search_extractor) — so a degraded extractor at begin_transaction time meant every resource that transaction created got indexed without the tenant's just-registered SearchParameter, permanently, regardless of the registry healing moments later for subsequent requests. No amount of caching discipline in for_tenant fixes an index row that was never written. begin_transaction now builds the registry from its own held connection on a cache miss (load_tenant_stored_params_with_conn, reusing the same query as the pool-based loader via the new shared parse_stored_search_params helper) instead of requesting a second one — removing the two-simultaneous-connections requirement this bug needed in the first place, on any backend configuration, not just the one CI runner that exposed it. If that read still fails, begin_transaction now fails loudly (a real StorageError) instead of silently proceeding with an incomplete overlay and mis-indexing every resource in the transaction. TenantSearchRegistries gained `cached`/`build_and_cache` to support this pattern generically (peek the cache; build-and-cache from an already-loaded set), factored out of `for_tenant`'s existing logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolve conflict in the SQLite begin_transaction: keep the #787 registry pre-warm on the transaction's own connection, and pass the backend clone main now uses so transactional writes index through the storage path. Claude-Session: https://claude.ai/code/session_01UumPvbC1mPeqrddMFtvgsv
…ter test testcontainers-modules defaults to postgres:11, which rejects the backend's plan_cache_mode startup option, so PostgresBackend::new failed with "db error" in CI. Match the 16-alpine pin used by every other Postgres testcontainer in the repo. Claude-Session: https://claude.ai/code/session_01XJjppFpm6DN4K6MddZ4U5N
…re-nightly # Conflicts: # crates/persistence/src/backends/sqlite/transaction.rs
… into fix/787-inferno-us-core-nightly # Conflicts: # crates/persistence/src/backends/sqlite/transaction.rs
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
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.
Summary
Fixes both confirmed failures in issue #787 (Inferno US Core nightly red since 2026-08-09), across all 5 backends (sqlite, sqlite-elasticsearch, postgres, mongodb, s3-elasticsearch).
Failure 1 —
care_team_role_search_test(connection refused tolocalhost:8080)CI config gap, not an HFS bug:
inferno-us-core.yml's "Start HFS server" step never setHFS_BASE_URL, so search-BundlefullUrls advertised the unreachable default instead of$RUNNER_IP:$HFS_PORT— the exact pattern already used byinferno-subscription.yml/inferno-bulk-data.yml.Failure 2 — asserted-date search doesn't filter
Genuine HFS bug: transaction-bundle writes to
sqlite/postgresnever invalidated the tenant's cachedSearchParameterregistry (unlike their own non-transactionalcreate(), and unlike MongoDB's transaction path), so aSearchParameterPOSTed inside a Bundle (exactly how Inferno's US Core setup registersasserted-date) never took effect until the TTL refresh.s3-elasticsearch (5th backend)
Deeper, separate gap: the composite handed Elasticsearch a
base_only()registry whose loader unconditionally returned an empty overlay for every tenant — S3 never had a real per-tenant registry to invalidate in the first place, and no TTL refresh existed for it either. S3 now owns a real registry, refreshed synchronously (before the write returns) viascan_live_resources— correct under both sync modes, since it never depends on Elasticsearch having received the document yet.Regression caught and fixed during this same PR: the first S3 fix attempt did the reload sequentially (one GET per stored SearchParameter object), and a tenant with the ~1.4k spec-seeded objects turned the very next real write into a ~30s stall that Inferno's client timed out on (visible as a new v3.1.1/s3-elasticsearch failure in an interim CI run). Fixed by fetching concurrently with the same fan-out the seeder already uses for writes.
Root cause detail
See commit messages for full file:line detail. Three independent research passes (including git-blame across the regression window and a live CI-log download) confirmed neither failure is a code regression from a specific commit — both are long-standing latent gaps, only exposed once earlier unrelated validation-test failures (present through 2026-08-02, fixed by 2026-08-09) stopped masking them.
Test plan
fullUrlis wrong withoutHFS_BASE_URL, correct with it.crates/rest/tests/transaction_bundle_search_parameter.rs, sqlite + postgres via testcontainers): posts aSearchParameterin one transaction Bundle and resources indexed under it in a second, asserts the search filters immediately. Confirmed to fail with the exact ci(inferno): US Core nightly red since 2026-08-09 — care_team_role_search hits localhost:8080, asserted-date search does not filter #787 symptom before the fix (git stash+ rerun) and pass after.crates/persistence/src/backends/s3/tests.rs): confirmed same fail-before/pass-after discipline.helios-hfscompiles clean with CI's exact feature set (R4,sqlite,elasticsearch,postgres,mongodb,s3), no warnings.workflow_dispatchrun against this branch on the self-hosted runners — in progress: https://github.com/HeliosSoftware/hfs/actions/runs/33504324228 (sqlite/sqlite-elasticsearch/postgres/mongodb/s3-elasticsearch confirmed passing through v6.1.0 as of PR open; v7.0.0/v8.0.0 still running). Will update this PR with the final result.Out of scope (follow-up)
HeliosSoftware/us-core-test-kitcheckout to a fixed ref (currently floats on its default branch — a CI-hygiene gap unrelated to either fix here).skip— postgres passed 25 of 544 tests and reported success for six weeks #491 (US Core gate ignoresskip), ci(inferno): audit the omit list — 3 entries safe to drop, 5 need version guards, 12 blocked on #490 #492 (omit-list audit) — related but explicitly separate per the original issue.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com