A down backend answers 500, not 503 — and /_readiness hardcodes "storage": "ok"

[mauripunzueta]

Summary

When a storage backend is unreachable, HFS tells the world two wrong things:

1. Every request returns 500 Internal Server Error — "the server has a bug" — instead of 503 Service Unavailable with Retry-After, which is what "the database is down, try again shortly" actually means.
2. The readiness probe reports "storage": "ok" unconditionally, so an orchestrator keeps routing traffic to a pod that cannot serve any of it.

Split out of #275 (#212), which makes this newly reachable and therefore newly worth fixing.

Why now

Before #275, an unreachable Elasticsearch cluster returned an empty Bundle with HTTP 200, and an unreachable S3 bucket read as an empty store. #275 fixes that: transport failures now surface as BackendError::Unavailable rather than as fabricated empty successes.

That is a strict improvement — a 500 is an honest answer where an empty 200 was a false one. But Unavailable is a precise signal about a transient, retryable, not-our-fault condition, and the REST layer immediately throws that precision away.

1. BackendError::Unavailable → 500

crates/rest/src/error.rs:

BackendError::UnsupportedCapability { capability, .. } => RestError::NotImplemented {
    feature: capability,
},
_ => RestError::InternalError { … },   // ← everything else, including Unavailable

RestError has no ServiceUnavailable variant at all. (SERVICE_UNAVAILABLE appears only as ad-hoc responses in handlers/sof/export.rs for "export not configured", never via the error type.)

Consequences of answering 500 instead of 503:

• Clients must not retry a 500; they should retry a 503. We are telling them the opposite of the truth.
• No Retry-After, so a client that does retry has no guidance and hammers a struggling backend.
• Load balancers and service meshes shed and route on 503, not 500.
• Alerting: a 500 spike means "we shipped a bug", a 503 spike means "a dependency is down". Conflating them costs on-call the first ten minutes of every incident.
• FHIR-wise, the OperationOutcome also can't carry the right issue.code (transient/exception vs a generic error).

2. The readiness probe hardcodes "ok"

crates/rest/src/handlers/health.rs:

pub async fn readiness_handler<S>(State(_state): State<AppState<S>>) -> RestResult<Response>
where S: ResourceStorage + Send + Sync,
{
    // Try a simple operation to verify storage is working
    // In a real implementation, we might try a count or read operation
    let response = serde_json::json!({
        "status": "ready",
        "checks": { "storage": "ok" }     // ← unconditional
    });

Note State(_state): the storage handle is bound and deliberately unused, the trait bound S: ResourceStorage is carried for a check that was never written, and the comment says as much.

So /_readiness is a constant. A pod whose database is unreachable reports itself ready, Kubernetes keeps it in the rotation, and every request it receives 500s. The probe is worse than absent — it is actively asserting a fact it never checked. This is the same class of defect #275 is about (a success-shaped answer to a question we never asked), one layer up.

Suggested fix

• Add RestError::ServiceUnavailable { retry_after: Option<Duration>, .. } → 503 + Retry-After, with an OperationOutcome carrying issue.code = transient.
• Map BackendError::Unavailable (and ConnectionFailed) to it; leave the _ => catch-all as 500 for genuine internal faults. Enumerate the BackendError variants explicitly so a new variant is a compile error rather than a silent 500.
• Make readiness_handler actually drive storage — a cheap count against a known tenant, or a dedicated ping/health hook on the trait — and return 503 when it fails. Keep /_liveness independent of storage: liveness must not fail on a dependency outage or the orchestrator will restart-loop a healthy process.
• Consider a short cache/timeout on the readiness check so probe traffic cannot amplify an outage.

Acceptance

• A request against a down backend returns 503 with Retry-After, not 500.
• /_readiness returns 503 when storage is unreachable, and its response reflects a check that actually ran.
• /_liveness still returns 200 when only the backend is down.
• A test covers each, ideally reusing the unreachable-backend construction from crates/persistence/tests/backend_error_handling.rs.