fix(subscriptions): persist server-driven status transitions (#357)

[mauripunzueta]

Closes #357.

What was wrong

Subscription runtime status lived only in the engine's in-memory DashMap. SubscriptionManager::update_status mutated the entry and returned; nothing was ever written back. Every decision the server made about a subscription was therefore lost on restart, and the stored resource contradicted $status for the life of the resource.

Approach, and why not a side store

The issue offered three directions. This PR takes write-back into the stored Subscription, through the ordinary ResourceStorage::update.

That choice is load-bearing: it needs no new table and no migration, so it applies identically to SQLite, PostgreSQL, MongoDB, S3, and the composite deployments. A separate per-backend projection store would have been ~1,500 lines across four backends for a feature that is opt-in twice (subscriptions is not a default cargo feature, and HFS_SUBSCRIPTIONS_ENABLED defaults to false).

It is also the only option that makes search agree. status is a search parameter served from the index built off stored resources, so a read-time overlay would leave GET /Subscription?status=active answering from stale documents while GET /Subscription/{id} said otherwise. Write-back makes read, search, $status, and _history tell one story.

The main objection to write-back is that the engine's own write would re-enter on_resource_event → register() and erase the counters it just saved. That does not happen: emit_subscription_event is called only from the REST write handlers (create.rs:231, update.rs:224, patch.rs:149, delete.rs:90). A write issued by the engine straight through ResourceStorage emits no event.

Bounding the write volume

SubscriptionEngine::transition_status is the single choke point; the engine no longer calls manager.update_status directly anywhere.

The write is gated on the manager accepting the transition. update_status rejects same-state transitions and transitions out of a terminal state, so when a subscriber outage parks hundreds of concurrent dispatch tasks that all observe consecutive_failures >= off_threshold, exactly one gets Ok and exactly one writes. Over a subscription's whole lifetime that is at most three writes (→ active, → error, → off). A transition whose target already matches the stored status issues no write at all, which also removes the version-bump burst on the first boot after upgrading.

Write-back fails open: the in-memory transition stands and the failure is logged at error (grep Failed to persist subscription status transition). Refusing to mark a subscription off because the database blipped would put the server straight back to hammering the endpoint the breaker just gave up on. It drops rather than retries on a losing race with a concurrent client write — the client's copy is the newer intent. It is bounded by a semaphore (4) and a timeout so it can neither drain the backend pool nor pin a dispatch task.

Three supporting fixes, without which persisting the breaker is worse than not

• Rehydration no longer skips off. Skipping was defensible while the status was volatile. Once it persists, skipping means the subscription is absent from the engine forever, so $status answers 404 for a resource that GET /Subscription/{id} answers 200 for — a regression on the very symptom being fixed. off and entered-in-error are now registered dormant: visible to $status, never matched (the evaluator selects only active), never handshaken.
• error is re-activated on restart. Nothing in the engine performs error → active while a subscription is dormant: it is never dispatched to, so it never succeeds, so nothing resets it. Before this PR the loss of the status on restart is what quietly recovered it. Persisting error without re-running activation would have converted a transient subscriber outage into permanent silent death.
• register no longer zeroes the runtime counters. It runs on every create/update/patch, so an unrelated edit — adding a filter, rotating a header — was already a silent reset of the delivery circuit breaker with no restart involved. Counters now survive re-registration unless the client is deliberately re-arming (writing requested, or moving the subscription out of error/off itself), which is the documented way back from a tripped breaker.

Also fixed

• entered-in-error is parsed. It previously failed from_fhir_str, so register fell back to requested — and rehydration would handshake and activate a subscription the client had explicitly retracted. It is now a status of its own, terminal in both directions.
• $status / $events fall back to storage instead of 404 when the engine does not hold the subscription (rehydration failed for a tenant, HFS_SUBSCRIPTION_REHYDRATE=false, an unparseable topic). Both handlers had the identical miss.

Deliberately not in scope

eventsSinceSubscriptionStart is not persisted. It has no element on Subscription in any FHIR version — it lives on SubscriptionStatus — so persisting it genuinely needs the side store that #269 is building, and doing it naively costs a database write per delivered notification. Today it resets to 0 on restart, which is a rewind; the correct fix is a reservation watermark (persist counter + N, restore from the watermark) so numbering only ever gaps forward, at O(subscriptions / interval) writes rather than O(events). That belongs with the store, not here.

Relationship to #269

PR #269 (cluster-capable HFS) introduces a SubscriptionStateStore carrying exactly this state — but it is PostgreSQL-only and attached only under HFS_CLUSTER=true. After #269 merges, #357 remains open for the default deployment: single node, SQLite, no cluster. This PR closes it there.

The two compose rather than collide: #269 holds runtime status in its shared store and documents that "the FHIR resource's own status field is never written back". If #269 lands after this, the write-back should be gated on !engine.is_cluster_backed() at the one choke point this PR created, so the shared store stays authoritative in cluster mode. That is a one-line boolean at a single site, which is auditable in a 99-file rebase.

This PR touches no persistence schema and adds no migration, specifically so it cannot collide with #269's migration numbering (which must be renumbered on rebase regardless — its ladder claims PG v15–v19 while main has since merged v15 and v16).

Follow-ups noted here rather than filed

• feat(cluster): make HFS cluster-capable behind a load balancer #269's reset_failures fires on every successful delivery and its PG statement is an unconditional UPDATE … SET consecutive_failures = 0, so a healthy subscription writes a row per notification. Making SubscriptionManager::reset_failures return "was non-zero" and skipping the store call would halve cluster-mode per-notification write cost.
• Conditional (conditional_update_handler, conditional_delete_handler) and batch/transaction write paths emit no subscription events at all, so a subscriber silently misses every conditionally- or transactionally-written resource.
• $purge on a Subscription never deregisters it from the engine.
• There is no error → active recovery other than a restart, and no heartbeat loop (heartbeat_check_interval and all_subscriptions() have no production callers).
• A server-driven status change emits no AuditEvent. The write goes straight through ResourceStorage, deliberately bypassing the REST layer (that is what keeps it from re-entering on_resource_event), which also means it bypasses the audit middleware. Arguably a server-authored change to a clinical-workflow resource should be audited; helios_persistence::core::storage::audit already has the shape for it.

Testing

New crates/subscriptions/tests/status_persistence.rs drives the engine through its public on_resource_event against a real SQLite backend and asserts the stored resource, not the in-memory entry: activation persists active; a delivery failure persists error; the breaker persists off; write-back preserves every other field including the version-shape markers rehydration infers the FHIR version from; a no-op transition mints no new version; no store and persist_status = false both leave storage untouched; a deleted subscription is not resurrected.

rehydrate_integration.rs gains the new off / entered-in-error dormant-registration and error re-activation contracts (the old off_subscriptions_are_skipped test asserted the behaviour this PR deliberately changes). Manager unit tests cover counter preservation, the three re-arm paths, tenant scoping, and entered-in-error.

crates/rest/src/handlers/subscriptions.rs gains unit tests for the $status / $events storage fallback: the parsing is split into a pure snapshot_from_stored so it is testable without standing up an app and an engine — native topic vs R4-backport criteria, entered-in-error surviving as itself rather than decaying to requested, and absent/unknown fields.

Verification

Not compiled locally (no C linker in the dev environment), so CI is the verification:

• Linting runs cargo clippy --all-targets --all-features -- -D warnings, so a green tick there means everything including the new tests compiles clean and lint-clean.
• Test Rust runs the suites below. It went green on the commit carrying the write-back and the full suite; see the checks on this PR for the current tip.

Two findings from a self-audit after the first CI run, both fixed here:

• Six of the new tests initially failed with TopicNotFound: the harness seeded the SubscriptionTopic into storage but never into the engine's in-memory topic registry, which is populated only by resource events or by rehydrate. A harness bug, not a production one.
• Two of the eight were passing vacuously. Their negative assertion is "the stored status is still requested", which a setup failure satisfies trivially — so they would have kept passing even with write-back entirely broken. Both now assert the in-memory transition happened first, proving the write was suppressed rather than never attempted.

Two claims in this description were checked against the code rather than assumed:

• Write-back cannot disturb rehydration's cursor paging, because rehydrate defers every activation to activate_pending after the whole tenant scan completes.
• CompositeStorage::update writes the primary and then sync_to_secondaries(SyncEvent::Update), so GET /Subscription?status=active really does become correct on composite deployments — the search-consistency argument for this design holds.

[codecov]

Codecov Report

❌ Patch coverage is 91.28920% with 25 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/subscriptions/src/engine/mod.rs	77.77%	24 Missing ⚠️
crates/subscriptions/src/manager/mod.rs	99.30%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.