fix(s3): decline transaction Bundles S3 cannot honour, and bound bundle concurrency

[aacruzgon]

Summary

An s3-elasticsearch deployment advertised FHIR transaction support in its CapabilityStatement, accepted transaction Bundles, and could honour neither. When the 30s request timeout fired mid-bundle, 466 of 473 entries were left durably committed with no tombstone or compensating delete, while the client was told the transaction had failed (#489).

This stops claiming the guarantee rather than trying to build it. Two defects are fixed:

1. A capability lie — three places asserted transaction support without consulting the backend.
2. An unbounded fan-out — join_all issued one S3 PUT per entry all at once, which is what blew the timeout in the first place. Refusing transactions removes it outright: that fan-out lived in process_transaction, which this PR deletes.

Fixes #489.

Why not implement atomicity instead

The S3 backend has a compensation log. It cannot run on the failure mode that matters, for two structural reasons:

• TimeoutLayer drops the handler future. Async cancellation stops the task at its await point without propagating an error, so every Err/status >= 400 arm holding rollback logic is skipped by construction.
• The compensation list is only populated after join_all resolves, so at the moment of cancellation it is empty — there is nothing to undo with, even given a cancellation-safe unwind.

More decisively, the architecture already ruled on this. crates/persistence/README.md calls S3 "intentionally storage-focused … archive/history storage", and design discussion #28 places ACID on the relational tier. #28 even specifies the guard:

The trait separation makes this distinction clear: code that requires atomicity takes &dyn TransactionProvider, while code that can tolerate partial failures takes &dyn ResourceStorage.

process_transaction lives on BundleProvider, which every backend implements — so S3 served transactions without ever satisfying the trait meant to gate them. Building a transaction log over an object store would engineer a guarantee this tier was deliberately not given.

Changes

Express and enforce the capability

• BundleProvider::supports_atomic_transactions() — a required method, no default. A new backend must state its position rather than inherit one, because the wrong default is silent corruption in one direction and a needless refusal in the other. SQLite/PostgreSQL → true (real TransactionProvider); MongoDB → true (server-side session, so it unwinds even on a dropped connection); S3 → false; composite → delegates to its primary.
• S3 refuses before the first write with a new TransactionError::AtomicityUnsupported → 501 + not-supported, matching the two sibling capability gaps already mapped that way. A total refusal means a retry finds the server as it left it; the old partial commit would double-write every POST entry, which carries server-assigned ids and so has no idempotency.

Stop advertising what we can't do

BackendCapability::Transactions already encoded the truth per-backend (declared by SQLite/PostgreSQL/MongoDB, absent from S3) — nothing read it. CompositeStorage::capabilities inserted SystemInteraction::Transaction unconditionally, and the CapabilityStatement carried {"code": "transaction"} as a literal. Both now derive it. batch stays unconditional.

The fan-out goes with the path

Corrected. An earlier revision of this description claimed join_all → buffered(16). That is not what landed, and it is corrected here rather than quietly dropped.

origin/main's s3/bundle.rs had two unbounded join_all sites: one in process_transaction (:97) and one in process_batch (:160). Deleting the transaction machinery removes the first outright — there is nothing left to bound. The second was never reachable (BundleProvider::process_batch had no production caller on any backend) and is deleted by #501, which this PR is now stacked on. Neither was ever bounded, and .buffered( appears nowhere in this branch.

Batch entry concurrency — the thing that actually governs an S3 client's wall clock, now that this PR routes those clients to batch — is #501's subject, not this one's.

Keep the Inferno coverage

Every fixture is a transaction Bundle, so the s3-elasticsearch legs would now fail at the load step. Posting them as batches is not a straight swap — uscore_bundle_patient_355.json alone has 473 entries and 2,058 inter-entry urn:uuid references that only a transaction resolves. A naive batch load would leave all of them dangling, manufacturing #459 for that backend.

crates/hfs/tests/inferno/to_batch.jq resolves them client-side, and install.sh probes /metadata — now truthful — to decide whether it's needed. The transform deliberately mirrors resolve_bundle_references, rewriting only an object's reference field and only for urn:uuid: values. Rewriting any matching string would corrupt an Identifier.value that merely looks like a uuid; pdex_bundle_patient_999.json contains exactly that case. POST becomes PUT against the derived id, which also makes loads idempotent.

Remove the now-dead machinery

With the refusal in place, the compensation log, both rollback helpers, the three-phase transaction body and S3's private resolve_bundle_references are unreachable — bundle.rs drops from 337 lines to 53, with no #[allow(dead_code)] left. Deleted rather than kept: if the decision were ever reversed, a durable design would need a persisted intent journal that survives process death, not an in-process Vec a dropped future discards. Meanwhile it reads as a live atomicity guarantee, which is the misreading that produced this bug. SQLite/PostgreSQL/MongoDB keep their own copies of resolve_bundle_references — still live, since they support transactions.

Testing

cargo check --workspace --all-targets clean across sqlite, postgres, mongodb, elasticsearch and s3. cargo test -p helios-persistence -p helios-rest --features R4,sqlite,postgres,s3 — 20 test binaries, 0 failures.

New tests:

• transaction_bundle_is_refused_without_writing_anything — the count == 0 assertion is the point; a refusal that still wrote something would reproduce transaction: s3-elasticsearch advertises and accepts transaction Bundles it cannot honour — a timed-out bundle commits 466 of 473 entries behind a 408 #489 in miniature.

Batch is deliberately not re-asserted at the S3 level. Two such tests were on this branch before the restack; both drove BundleProvider::process_batch, which #501 removed as unreachable, so batch has no S3-level entry point left to test. It is covered over HTTP in helios-rest (batch_conformance.rs, batch_if_match.rs) and end-to-end by the s3-elasticsearch Inferno leg.

• atomicity_unsupported_maps_to_501_not_supported — status, issue code, and that the message names the backend, points at batch, and states nothing was applied.

Removed the three bundle_transaction_* tests, which covered the now-unreachable path, leaving a note in their place.

Verified live against a real s3-elasticsearch deployment

MinIO + Elasticsearch, HFS_STORAGE_BACKEND=s3-elasticsearch:

1. /metadata lists batch, history-system, search-system — transaction is gone.
2. POST / with a transaction Bundle → 501 with the actionable OperationOutcome; Patient?_summary=count confirms nothing was written.
3. install.sh self-detected ("Server does not advertise 'transaction'; rewriting bundles to batch") with no flag or configuration.
4. All 15 fixtures loaded — 8 Patients, 246 Observations, 51 Conditions, 194 Encounters, identical to the transaction-path numbers on every other backend.
5. References resolved — Observation?patient=… → 27, Condition → 2, Encounter → 1. Identical again.

Inferno: us_core_v800

backend	pass	rate	failures
s3-elasticsearch (this branch)	329/544	60.4%	9
postgres (with #495 + #496)	331/544	60.8%	10
sqlite	326/544	59.9%	10

Zero s3-elasticsearch-only failures — its 9 are a strict subset of postgres's 10. This backend has never produced a pass rate before; every nightly died at the load step, so 60.4% is the first number for it.

Now confirmed in CI by run 31016157176 on this branch, once #501 landed beneath it: 30/30 jobs green, s3-elasticsearch 329 pass / 204 skip / 9 fail / 2 error at v8.0.0 — reproducing the 329 above exactly, and identical at the test-id level to sqlite-elasticsearch (0 tests passing only on one side). The two large fixtures that previously 408'd now load in 9.42s (473 entries) and 5.48s (267 entries).

Worth being precise about the earlier evidence: the table above came from a local MinIO deployment, where latency is negligible and the request timeout never fires. The prior CI run on this branch (30973069238) never reached Inferno at all — all six s3-elasticsearch jobs failed at "Load Inferno test data into HFS" and skipped every subsequent step. Sequential batch was the reason, which is why #501 had to land first.

The rewrite was also validated independently by loading both ways into sqlite and diffing: identical across 22 resource types and 7 reference searches.

Notes

• Stacked on batch: entries are processed sequentially and BundleProvider::process_batch is dead on every backend — a batch over ~90 entries times out on S3 #501 (fix(rest): run batch bundle entries with backend-bounded concurrency #505), which must merge first. This PR tells S3 clients to use batch, and that advice only holds once batch can carry the load — sequential batch 408s past ~90 entries on S3. The base branch is fix/501-batch-entry-concurrency; this PR's diff shows only its own changes.
• Merge order is unconstrained relative to fix(search): match bare-id reference searches on Postgres #495/fix(search): index Postgres dates with negative UTC offsets correctly #496/ci(inferno): prune omit entries that suppress nothing, and report new ones #498 — zero file overlap, and no semantic dependency either way.
• The stale s3/tests.rs comment claiming entries "now run concurrently (buffered)" — behaviour that never existed — is gone: it went with the tests it annotated during the restack onto batch: entries are processed sequentially and BundleProvider::process_batch is dead on every backend — a batch over ~90 entries times out on S3 #501.
• Original note, for the record: zero file overlap with fix(search): match bare-id reference searches on Postgres #495/fix(search): index Postgres dates with negative UTC offsets correctly #496/ci(inferno): prune omit entries that suppress nothing, and report new ones #498, and no semantic dependency either way. Worth noting one interaction I checked: ci(inferno): prune omit entries that suppress nothing, and report new ones #498 prunes omit entries verified against sqlite and postgres, and this PR makes the s3-elasticsearch legs produce results for the first time — all three pruned entries (data_absent_reason ×2, practitioner_address_test) also pass on s3-elasticsearch, so the two are safe together in any order.
• This does not reproduce the original timeout. MinIO on localhost finishes the 473-entry bundle in seconds, so the run above validates the new path's correctness, not the old failure. Against real S3 the batch loader is still strictly sequential on this branch — batch: entries are processed sequentially and BundleProvider::process_batch is dead on every backend — a batch over ~90 entries times out on S3 #501 measures it at ~300 ms/entry, with a 408 past ~90 entries.
• Behaviour change for any S3-primary deployment: transaction Bundles now return 501 instead of partially succeeding. That is the intended fix, but it is a visible API change for anyone who was relying on the old behaviour — which could not have been giving them the semantics they asked for.
• The composite's sync_bundle_results still swallows secondary-sync errors with warn! only, so a clean write can leave S3 committed and Elasticsearch unindexed. Separate pre-existing gap, noted in Test suites can pass vacuously: env-gated tests silently skip in CI (sibling of #389) #390's discussion, not addressed here.
• Clippy deferred to CI at the author's request; the CI-style invocation cannot pass on this toolchain because of pre-existing lints in generated and untouched files.

[claude]

Code review

Found 2 issues during review.

1. to_batch.jq relocates resources that already have a stable PUT id, breaking fixture references

This transform relocates resources that already have a stable PUT Type/<id> request onto a uuid-derived id instead, because the then branch only checks whether fullUrl starts with urn:uuid: — it ignores the entry's existing request.method/request.url and resource.id.

Several Inferno fixtures hit this. For example, uscore_bundle_patient_907.json already has resource.id: "907" and request: {method: "PUT", url: "Patient/907"} alongside fullUrl: "urn:uuid:a24eb240-...". After this transform it gets relocated to Patient/a24eb240-9ea3-4fa5-b12d-b798587879d7.

This breaks fixtures that reference the original stable id via literal (non-urn:uuid:) references, which resolve() never rewrites — pdex_bundle_patient_999.json has 21 literal "Patient/999" references left dangling after the transform, and Group fixtures (uscore_group_1a.json, pdex_proxy_group_patient_999.json) that reference Patient/85/Patient/355/Patient/907/Patient/999 by their original stable id become dangling once those Patients relocate.

Suggested fix: when an entry already has a PUT request to a concrete Type/id, preserve that id/url instead of synthesizing a new one from the uuid, and map fullUrl to that preserved Type/id in $map.

hfs/crates/hfs/tests/inferno/to_batch.jq

Lines 31 to 42 in 0e1aa49

	| .entry |= map(
	if ((.fullUrl // "") | startswith("urn:uuid:")) then
	(.fullUrl | uuid_of) as $id
	| (.resource.resourceType // "Unknown") as $rt
	| .resource.id = $id
	| .resource |= resolve($map)
	| .fullUrl = ($rt + "/" + $id)
	| .request = { method: "PUT", url: ($rt + "/" + $id) }
	else
	.resource |= (if . == null then . else resolve($map) end)
	end
	)

2. buffered(16) concurrency bound is unreachable from the actual HTTP batch path

This buffered(BUNDLE_CONCURRENCY) change bounds concurrency inside S3Backend::process_batch, but that method has no caller reachable from an actual HTTP request — a repo-wide search shows its only non-test caller is CompositeStorage::process_batch, which itself is only invoked by its own unit test. The REST batch handler (see batch.rs lines 160-175:

hfs/crates/rest/src/handlers/batch.rs

Lines 160 to 175 in 0e1aa49

	for (index, entry) in entries.iter().enumerate() {
	let result =
	process_batch_entry(state, &tenant, fhir_version, entry, index, principal).await;
	let correlation_details = EntryAuditCorrelation::from_bundle(&correlation, index);
	emit_batch_entry_audit(
	state,
	entry,
	&result,
	principal,
	None,
	Some(&correlation_details),
	);
	response_entries.push(bundle_entry_result_to_json(&result, base_url, prefer));
	}

) processes "batch" bundles with its own fully-sequential for loop over process_batch_entry, never calling into BundleProvider::process_batch.

This means the PR's stated fix for the #489 timeout (bounding rather than unbounding concurrency) doesn't take effect for live batch traffic. It's also the exact path this PR now directs clients to via the 501 refusal + to_batch.jq conversion: a large bundle like the 473-entry one from #489, converted to batch, would still hit the sequential per-entry loop under the same 30s timeout — just serialized instead of unbounded, with no guarantee it completes in time.

hfs/crates/persistence/src/backends/s3/bundle.rs

Lines 91 to 101 in 0e1aa49

	// `BUNDLE_CONCURRENCY` of them at a time.
	use futures::StreamExt;
	let futs: Vec<_> = entries
	.iter()
	.map(|entry| self.process_batch_entry(tenant, entry, fhir_version))
	.collect();
	let results: Vec<_> = futures::stream::iter(futs)
	.buffered(BUNDLE_CONCURRENCY)
	.collect()
	.await;

[codecov]

Codecov Report

❌ Patch coverage is 88.05970% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/persistence/src/backends/mongodb/storage.rs	0.00%	3 Missing ⚠️
...rates/persistence/src/backends/postgres/storage.rs	0.00%	3 Missing ⚠️
crates/persistence/src/composite/storage.rs	87.50%	1 Missing ⚠️
crates/rest/src/routing/fhir_routes.rs	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[aacruzgon]

Draft while blocked on #501 — and two corrections to this PR

Code review on this PR flagged two issues; an overnight full-matrix Inferno run against this branch (30973069238) confirmed both. Both were valid, and the numbers in the original description are no longer accurate.

1. The batch rewrite relocated stable resource ids — fixed (c914754ce)

to_batch.jq branched on fullUrl alone, so entries carrying both a urn:uuid fullUrl and a concrete PUT Type/<id> were moved onto uuid ids. That relocated Patient/85, /355, /907, /908 and /999 — the ids Inferno is configured to exercise, the ids the Group fixtures reference, and the target of 21 literal "Patient/999" references, which are not urn:uuid and so never followed.

It also diverged from the server, which was the premise of the whole transform: phase 1 of process_transaction skips non-POST entries, so a PUT keeps the id it names.

Chasing that turned up a second divergence the review did not mention: SQLite, PostgreSQL and MongoDB build the reference map incrementally, resolving each entry against the map so far, so they never resolve a forward reference. The original rewrite built the map up-front and did, giving s3-elasticsearch three references the other backends lack. Now a single incremental pass.

The consistent rule, applied in all three places: the loader mirrors the server, including where the server does nothing. Same reason references to PUT entries stay unresolved (#459) — resolving them would hand one backend better data and break comparability.

Why the original validation missed it, which matters more than the bug. The equivalence check compared aggregate counts, which cannot see relocation — 8 Patients either way. And the local Inferno run reported 60.4% with "zero backend-specific failures" while four of five test patients did not exist, because tests for a missing patient skip and the gate ignores skips (#491). The pass rate was never evidence the data was correct.

Now: the check compares full sorted id lists per type plus hashed reference values (identical across 23 types), and install.sh fetches all 136 fixture-addressed ids after loading. That guard was verified to fail on the old rewrite — MISSING: Patient/pat015 (HTTP 404) and eight more — not merely to pass on the new one.

The 60.4% figure in the description above is void. It was measured with the buggy transform.

2. buffered(16) was unreachable — commit dropped, real defect filed as #501

BundleProvider::process_batch has no caller reachable from HTTP; the REST handler implements batch itself with a sequential loop. So that commit could not execute, and claiming it fixed the #489 timeout was exactly the kind of unearned capability claim this PR exists to remove. It has been dropped from the branch.

The underlying defect is real and now measured. From the run above, loading fixtures as batch against real S3:

bundle	entries	wall clock	result
uscore_bundle_patient_355.json	473	30.1s	408
uscore_bundle_patient_85.json	267	30.1s	408
all others	≤54	0.2–15.8s	200/201

~300ms per entry, consistently, so ~90 entries is the ceiling inside the 30s budget. patient_85 loaded in 5.5s through the concurrent transaction path — sequential batch is a ~15× regression for the same data.

That is a pre-existing defect, not one this PR introduces; it was invisible because nothing large went through batch until this PR started routing S3 there. Filed as #501.

Status

Draft until #501 lands. This PR declines transaction Bundles on S3 and points callers at batch; that advice is only sound once batch can carry the load. Merging now would turn the six s3-elasticsearch legs red at the load step.

Unaffected and still verified: the capability derivation, the refusal and its 501, the CapabilityStatement fix, the removal of the unreachable transaction machinery, and the tests. Every other leg of the overnight run — sqlite, sqlite-elasticsearch, postgres, mongodb, all six suites — passed.

Once #501 merges, this rebases and the s3-elasticsearch legs should produce a trustworthy pass rate for that backend for the first time.