From 6ee4cf529935e17a3937ab16d3d265a1e2d9b05c Mon Sep 17 00:00:00 2001 From: rob thijssen Date: Mon, 17 Aug 2026 13:10:10 +0300 Subject: [PATCH] feat(api): make healthz verify the schema, not just that the process is up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/v1/healthz` returned a static "ok" without touching the database, so the deploy's health probe passed regardless of whether the schema the binary expects had been migrated. An api newer than its schema sailed through the probe and then failed one query at a time on whatever column was missing — the exact failure that job ordering now prevents, with nothing to catch it if that assumption ever breaks again. It now compares the applied migration version against `moments_data::expected_schema_version()`, derived from the migrations compiled into this binary via the same `sqlx::migrate!` MIGRATOR that applies them. There is no second list of expected columns to drift from the real one. schema >= expected -> 200 "ok (schema 6)" schema < expected -> 503, error-level journal line naming both versions no migrations -> 503 cannot read table -> 200 "degraded: schema unverified (...)" A newer schema than expected stays healthy: that is an api rollback under a migrated database, and this binary's queries are still satisfiable. The reverse is not. The degraded case exists because `_sqlx_migrations` is created by moments_rw and reaches moments_ro through the default privileges in asset/sql/bootstrap-moments.sql. A role provisioned before those grants would get 42501, and failing the probe over that would take a working api offline for a permissions detail. `StoreError` gains an `Inaccessible` variant so the two are told apart by SQLSTATE (42501, 42P01) rather than by sniffing message text, and the condition is loud in both the journal and the probe output. Also corrected the startup comment in moments-api: it claimed the api/worker ordering came from systemd dependencies, which cannot be true across two hosts. It comes from deploy.yml. Verified against postgres 16 with the production role split replicated (moments_rw owning the schema, moments_ro granted through bootstrap-moments.sql, plus a legacy role without those grants) and the migrations applied by the real worker binary: current schema -> 200 "ok (schema 6)"; version 6 row deleted -> 503 "schema at 5, this binary expects 6"; table emptied -> 503 "no migrations applied"; legacy role -> 200 degraded, with `curl -fsS` exiting 0 and printing the reason. First confirmed that moments_ro can in fact read `_sqlx_migrations` under the documented grants, so the normal path is the precise one. Refs https://git.lair.cafe/grenade/moments/issues/8 --- CLAUDE.md | 8 ++++ crates/moments-api/src/main.rs | 70 +++++++++++++++++++++++++++++++--- crates/moments-core/src/lib.rs | 11 ++++++ crates/moments-data/src/lib.rs | 39 +++++++++++++++++++ 4 files changed, 123 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9fb4f5b..f35dcf8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,6 +129,14 @@ CI-driven via **Gitea Actions** (`.gitea/workflows/`), the source of infra truth `Type=simple`, so that returns before migrations do. It polls the journal, scoped to the unit's current `InvocationID`, for the `worker started` line the worker only logs after `store.migrate()` returns. + - `/v1/healthz` checks that ordering held rather than assuming it: it compares + the applied migration version against the migrations compiled into the api + binary (`moments_data::expected_schema_version()`), so a schema behind the + binary answers 503 and fails the deploy probe instead of passing and then + erroring one query at a time. A newer schema is healthy (api rollback under a + migrated db). A role that cannot read `_sqlx_migrations` reports *degraded* + at 200 — a missing grant says nothing about whether the api works, so it + warns loudly instead of bricking the deploy. - `build-web` needs both deploy jobs because the prerender fetches `VITE_API_BASE` at build time and bakes whatever the api is serving when it runs. In parallel, whether the crawler snapshot matched the api came down to diff --git a/crates/moments-api/src/main.rs b/crates/moments-api/src/main.rs index f68a4ca..98d02ca 100644 --- a/crates/moments-api/src/main.rs +++ b/crates/moments-api/src/main.rs @@ -9,7 +9,7 @@ use axum::{ }; use chrono::{DateTime, Datelike, NaiveDate, Utc}; use clap::Parser; -use moments_core::{EventReader, reshape}; +use moments_core::{EventReader, StoreError, reshape}; use moments_data::PgStore; use moments_entities::{ BlogPost, BlogPostSummary, DailyCount, Event, EventQuery, HourlyAvg, LanguageDailyCount, @@ -44,8 +44,13 @@ async fn main() -> anyhow::Result<()> { // The api connects as moments_ro and never writes — migrations are owned // by moments-worker, which is the database owner (moments_rw). Running // migrations from here would fail with `permission denied for schema - // public`. The worker must have run at least once before the api accepts - // traffic; in deploy this is ordered via systemd dependencies (§3). + // public`. + // + // The worker must therefore have migrated before this binary serves a + // request. That is not a systemd dependency — the two run on different + // hosts — it is enforced in .gitea/workflows/deploy.yml, where deploy-api + // needs deploy-worker and deploy-worker waits for the migrations to land. + // `/v1/healthz` checks the outcome rather than trusting it. let store = PgStore::connect(&args.database_url).await?; let http = reqwest::Client::builder() .timeout(Duration::from_secs(15)) @@ -90,8 +95,63 @@ fn init_tracing() { } } -async fn healthz() -> &'static str { - "ok" +/// Liveness *and* schema readiness. +/// +/// The api is SELECT-only and cannot migrate; the worker owns that, on another +/// host. This endpoint used to answer a static "ok" without touching the +/// database, so a binary running against a schema older than the one it was +/// built for passed its deploy probe and then failed one query at a time. It +/// now compares the applied migration version against the migrations compiled +/// into this binary. +/// +/// A newer schema than expected is healthy — that is an api rollback under a +/// migrated database, and the queries this binary makes are still satisfiable. +/// The reverse is not. +async fn healthz(State(state): State) -> impl IntoResponse { + let expected = moments_data::expected_schema_version(); + match state.store.schema_version().await { + Ok(Some(applied)) if applied >= expected => { + (StatusCode::OK, format!("ok (schema {applied})\n")) + } + Ok(Some(applied)) => { + tracing::error!( + applied, + expected, + "schema is behind this binary; worker has not migrated yet" + ); + ( + StatusCode::SERVICE_UNAVAILABLE, + format!("unhealthy: schema at {applied}, this binary expects {expected}\n"), + ) + } + Ok(None) => { + tracing::error!(expected, "no migrations applied"); + ( + StatusCode::SERVICE_UNAVAILABLE, + format!("unhealthy: no migrations applied, this binary expects {expected}\n"), + ) + } + // Not being allowed to read the migration table says nothing about + // whether the api works, so this stays passing rather than failing a + // deploy over a missing grant. It is loud in both the journal and the + // probe output, which is where anyone would look. + Err(StoreError::Inaccessible(e)) => { + tracing::warn!( + error = %e, + "cannot read _sqlx_migrations; schema readiness unverified — re-run asset/sql/bootstrap-moments.sql" + ); + ( + StatusCode::OK, + format!( + "degraded: schema unverified ({e}); re-run asset/sql/bootstrap-moments.sql\n" + ), + ) + } + Err(e) => { + tracing::error!(error = %e, "health check could not reach the database"); + (StatusCode::SERVICE_UNAVAILABLE, format!("unhealthy: {e}\n")) + } + } } #[derive(Debug, Deserialize)] diff --git a/crates/moments-core/src/lib.rs b/crates/moments-core/src/lib.rs index bd43a4e..c6b0fed 100644 --- a/crates/moments-core/src/lib.rs +++ b/crates/moments-core/src/lib.rs @@ -15,11 +15,22 @@ use moments_entities::{ pub enum StoreError { #[error("database error: {0}")] Database(String), + /// The role is not permitted to read what was asked for, or the object + /// does not exist. Kept distinct from [`StoreError::Database`] so a caller + /// can tell "the answer is bad" from "I was not allowed to look" — the + /// health check treats the two very differently. + #[error("inaccessible: {0}")] + Inaccessible(String), } /// Read-side port consumed by `moments-api`. #[async_trait] pub trait EventReader: Send + Sync { + /// Highest migration version applied to the database, or `None` if none + /// have been. The api compares this against the migrations compiled into + /// its own binary so that a schema older than the one it was built for is + /// reported as unhealthy rather than discovered one failed query at a time. + async fn schema_version(&self) -> Result, StoreError>; async fn list_events(&self, query: &EventQuery) -> Result, StoreError>; async fn source_summaries( &self, diff --git a/crates/moments-data/src/lib.rs b/crates/moments-data/src/lib.rs index 15f5d95..0bfddf4 100644 --- a/crates/moments-data/src/lib.rs +++ b/crates/moments-data/src/lib.rs @@ -43,6 +43,26 @@ fn map_err(e: E) -> StoreError { StoreError::Database(e.to_string()) } +/// Like [`map_err`], but classifies "you may not look at this" separately from +/// a genuine failure, by SQLSTATE rather than by sniffing the message text. +fn map_access_err(e: sqlx::Error) -> StoreError { + if let sqlx::Error::Database(db) = &e { + match db.code().as_deref() { + // 42501 insufficient_privilege, 42P01 undefined_table + Some("42501") | Some("42P01") => return StoreError::Inaccessible(e.to_string()), + _ => {} + } + } + StoreError::Database(e.to_string()) +} + +/// Highest migration version this binary was built with. Compared against the +/// database's applied version by the api's health check, so the contract is the +/// embedded migration set itself — there is no second list to drift from it. +pub fn expected_schema_version() -> i64 { + MIGRATOR.iter().map(|m| m.version).max().unwrap_or(0) +} + /// What a single per-repo visibility probe concluded, during the /// reconciliation pass that keeps `events.public` honest as repos flip /// visibility upstream. Shared by the github and gitea sources. @@ -79,6 +99,25 @@ pub(crate) fn probe_verdict(status: u16) -> Option { #[async_trait] impl EventReader for PgStore { + async fn schema_version(&self) -> Result, StoreError> { + // `_sqlx_migrations` is created by the worker, so moments_ro reaches it + // through the default privileges in asset/sql/bootstrap-moments.sql. + // A role that predates those grants gets 42501 here, which + // map_access_err reports as Inaccessible rather than a failure — the + // health check degrades instead of declaring a working api broken. + let row = sqlx::query( + r#" + SELECT MAX(version)::bigint AS version + FROM _sqlx_migrations + WHERE success + "#, + ) + .fetch_one(&self.pool) + .await + .map_err(map_access_err)?; + row.try_get("version").map_err(map_err) + } + async fn list_events(&self, query: &EventQuery) -> Result, StoreError> { let sources: Option> = query .sources