fix(persistence): surface backend failures instead of faking empty results, and assert the contract for every backend

[mauripunzueta]

Closes #212.

Summary

Extends the MongoDB error-handling pattern from #211 to every storage backend — and fixes the five production bugs found along the way: four that the new tests exposed, and one that the fix for Bug 2 introduced and review caught.

Reviewers: despite the test(...)-shaped origin, this changes production error semantics in four backends. Bug 1 is the one to read first — a *-elasticsearch deployment with the cluster down currently answers searches with an empty Bundle and HTTP 200.

The issue scoped this to PostgreSQL / SQLite / S3 and declared Elasticsearch out of scope ("search-only, never a standalone primary"). That turned out to be backwards: Elasticsearch is the one backend that actually violated the contract, and it violated it on the search path, in a shipped topology.

The contract

One new test target, crates/persistence/tests/backend_error_handling.rs, states it once:

An operation against an unreachable or misconfigured store MUST return Err(StorageError::Backend(_)). It MUST NOT return Ok(None), Ok(0), or any other success, and it MUST NOT hang.

The Ok(None) clause is the point. A read that reports "not found" when it never reached the database is indistinguishable from one where the resource is genuinely absent — and "this patient has no recorded allergies" vs "I could not reach the database" are different clinical statements. count returning Ok(0) is the same lie wearing different clothes.

9 tests, all five backends, no server and no Docker — they always run. Each is wrapped in a deadline, so a backend that hangs fails loudly rather than stalling CI.

ResourceStorage is object-safe, so a single &dyn ResourceStorage driver exercises every backend even though their constructors differ wildly:

Backend	How it's broken	Failure surfaces at
SQLite	unmigrated store; unopenable path	operation; construction
PostgreSQL	refused connection; blackholed server	construction (new() verifies connectivity)
MongoDB	dead address	operation
Elasticsearch	dead node, incl. a dedicated search test	operation
S3	dead endpoint, lazy and eager	operation; construction

The MongoDB test moves here from mongodb_tests.rs. It needs no server, so it did not belong in a suite whose tests all skip without one, and its mongodb_integration_* name implied a Docker dependency it never had.

Bug 1 — Elasticsearch silently reported an unreachable cluster as an empty store

Four catch-all arms:

• storage.rs read: Err(_) => return Ok(None) — any transport error became "resource not found"
• search_impl.rs search: a connection failure returned SearchAttempt::EmptyIndex → an empty result set
• count / search_count: _ => Ok(0)
• create_or_update: _ => ("1", true) — a failed existence check meant "new resource, version 1", silently clobbering history

This was live. composite/storage.rs prefers the Search backend for search, and four of the eight supported HFS_STORAGE_BACKEND modes are *-elasticsearch. So in a postgres-elasticsearch deployment with ES down, GET /Patient?identifier=X returned HTTP 200 with an empty Bundle — and update_health(id, result.is_ok()) recorded the dead cluster as healthy.

Transport failures now flow through the existing retry machinery (so a rolling restart or a not-yet-ready cluster is still absorbed) and surface as BackendError::Unavailable once retries are exhausted. The genuinely-empty cases — index_not_found_exception, and doc-not-found → 404 — still return empty, so healthy behaviour is unchanged and no existing ES test changes.

⚠️ Behaviour change worth a reviewer's attention: a search against a down cluster now errors instead of returning an empty 200. The old code did this deliberately ("so searches don't 500 against a backend that isn't ready yet"). Trading a wrong answer for a loud one is the right call for clinical data, but it is a real change and I'd rather surface it than smuggle it in.

Bug 2 — PostgreSQL connect_timeout_secs was dead config

Declared, defaulted, serde-serialized… and never read. create_pool never touched it.

Wiring it alone does not fix the hang: tokio-postgres applies connect_timeout only to TcpStream::connect, leaving DNS, the TLS handshake and the Postgres startup/auth exchange awaited unbounded. A peer that completes the TCP handshake and then stalls — a hung server, or an LB/security group accepting into a blackhole — hung the caller forever.

So this wires cfg.connect_timeout and deadpool's create_timeout, which bounds the whole of connection establishment. wait/recycle are deliberately left unbounded: they govern behaviour under load (queueing for a free slot), not reachability, and capping them would change how a saturated pool sheds traffic.

postgres_backend::blackholed_server_is_bounded_and_surfaces_backend_error is the regression test — it binds a listener and never accepts, so the kernel completes the handshake and the client waits. It hangs on main.

Bug 3 — MongoDB's server-selection timeout was hardcoded (found by CI)

CI flagged the contract test's MongoDB case sitting for the full deadline. Not a hang: SERVER_SELECTION_TIMEOUT was a hardcoded 15s, and it — not connect_timeout_ms — is what bounds an operation against an unreachable server, because the driver keeps re-attempting server selection while its monitor reports no healthy server. The connect timeout only bounds an individual TCP handshake, so lowering it (as the original #211 test did) changed nothing.

An unreachable MongoDB therefore always took 15s to surface an error, with no way for an operator to tune it. Promoted to MongoBackendConfig::server_selection_timeout_ms (default 15000, so existing deployments behave identically) and exposed as HFS_MONGODB_SERVER_SELECTION_TIMEOUT_MS.

Bug 4 — S3: a missing bucket read as an empty store

client.rs collapsed NoSuchBucket into the same S3ClientError::NotFound that get_object/head_object swallow into Ok(None). So a bucket that was typo'd, deleted, or invisible to the credentials made every read answer "this resource does not exist" and count answer zero — the identical failure mode to the Elasticsearch bug, still live in S3.

A missing object is a legitimate Ok(None). A missing bucket means we learned nothing at all. It now gets its own S3ClientError::BucketNotFound variant that the object callers cannot swallow, mapped to BackendError::Unavailable.

Scope, precisely: this covers GetObject / PutObject / ListObjectsV2, whose error responses carry a body and can therefore say NoSuchBucket. The two HEAD paths cannot, and are not fixed here — see #284, and the Not in this PR section below. The mock is written to match: it reports BucketNotFound on get/put/list, and on the HEAD operations it mirrors the real client exactly (a bare 404 → NotFound → Ok(None)).

That last part is deliberate and worth a reviewer's eye. An earlier revision had the mock answer BucketNotFound on HEAD too — a distinction the real client provably cannot make — which made s3_missing_bucket_create_and_count_are_errors look like it proved create's existence check notices a missing bucket. It does not; the check concludes "new resource" and the following PutObject is what fails. Green either way, but the green meant something other than what it appeared to. The mock is now honest, the test says so in a comment, and the real gap is filed rather than papered over.

Bug 5 — the Postgres timeout fix needed a knob (found in review)

Bug 2 above is right, but on its own it introduced a regression. connect_timeout_secs went from a field nothing read to a hard 5s ceiling on connection establishment — with no way to raise it. from_env() reads six HFS_PG_* variables and not that one, and parse_connection_string starts from Default, so every deployment path was pinned to the default. A Postgres that legitimately needs longer than 5s to answer a fresh connection (a cold or loaded cluster, a cross-region failover, TLS through a distant proxy) would have started failing with nothing an operator could do about it.

That is the same defect Bug 3 fixes for MongoDB, so Postgres gets the symmetric treatment: HFS_PG_CONNECT_TIMEOUT_SECS, read via a helper that both construction paths use. from_connection_string is the dominant one — it backs HFS_DATABASE_URL and HTS's database_url — so wiring only from_env would have left the ceiling unreachable for most deployments. The URL never carries a timeout (parse_connection_string parses no query parameters), so applying the env value on top of it clobbers nothing. Documented in the README next to the other HFS_PG_* variables.

Closing the last coverage gap: PostgreSQL per-operation arms

Postgres was the one backend whose per-operation error arms stayed uncovered. new() connects eagerly, so an unreachable server fails at construction and the caller never obtains a backend to drive — leaving all 56 internal_error(..) arms in postgres/storage.rs unexercised. That is exactly the "unexercised .map_err arms" this issue names as its motivation.

Reaching them needs a server that answers but cannot serve the query, so postgres_integration_unmigrated_store_surfaces_backend_error creates an empty database, skips init_schema(), and drives read/count/create against it — mirroring the SQLite unmigrated-store test. It needs Docker, so it lives with the other container tests.

Not in this PR (real findings, each filed as its own issue)

• S3's HEAD paths still conflate a missing bucket with a missing object — filed as S3: a missing bucket is indistinguishable from a missing object on the HEAD paths #284. A HEAD response has no body, so real S3 cannot return NoSuchBucket there; the SDK reports a bare 404 and head_object swallows it into Ok(None). Latent rather than live: the create and bulk-submit existence checks are each followed by a write that does fail correctly, and validate_buckets still returns the right Unavailable variant (only its message misleads). Safe by coincidence, so it is worth fixing — but not here.
• Postgres: statement_timeout is applied to one pooled connection, so ~9 of 10 connections run uncapped #285 — Postgres SET statement_timeout is applied to exactly one pooled connection, then returned to the pool — every other connection gets the server default, so statement_timeout_ms is effectively unenforced. Fixing it via a deadpool post_create hook would newly enforce a 30s cap on queries that have never been capped, which could break long-running bulk-export work. Deserves its own PR and its own load validation.
  PostgresConfig::schema_name is a third declared-but-unhonoured field in the same struct; folded into Postgres: statement_timeout is applied to one pooled connection, so ~9 of 10 connections run uncapped #285.
• A down backend answers 500, not 503 — and /_readiness hardcodes "storage": "ok" #286 — crates/rest/src/error.rs maps every BackendError except UnsupportedCapability to a 500, and RestError has no ServiceUnavailable variant — so "the database is down" is indistinguishable from "the server has a bug". No 503, no Retry-After, and /_readiness hardcodes "storage": "ok" (its State(_state) is literally unused), so an orchestrator keeps routing traffic to a pod that cannot serve it. This PR raises the stakes on it: the Elasticsearch fix converts a silent empty-200 into a real error, and that error now lands as a 500. A 500 is the honest answer where an empty 200 was a wrong one, but 503 + Retry-After is the right answer, and it should follow soon.

Testing

cargo test -p helios-persistence --all-features --test backend_error_handling — needs no Docker.

CI is green, and the new tests are confirmed to have actually run rather than silently filtering to zero: backend_error_handling reports 9 passed; 0 failed, and the Docker-backed postgres_integration_unmigrated_store_surfaces_backend_error passes alongside it. (Authored without a local C linker, so CI was the first compile; every API used was verified against the vendored crate sources.)

Issue checklist

• PostgreSQL — refused connection and blackholed server (construction), plus unmigrated store (operation)
• SQLite — unopenable path (construction) and unmigrated store (operation)
• S3 — dead endpoint, lazy and eager; plus missing-bucket-is-not-an-empty-store
• MongoDB — the test(mongodb): assert graceful backend errors on an unreachable server #211 test moves here from mongodb_tests.rs; its hardcoded server-selection timeout is now tunable (Bug 3)
• Elasticsearch — declared out of scope by the issue; it turned out to be the backend most flagrantly violating the contract, on the search path, in a shipped topology (Bug 1)

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rates/persistence/src/backends/postgres/backend.rs	91.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!