diff --git a/server/src/crate_server/catalog_index_postgres.gleam b/server/src/crate_server/catalog_index_postgres.gleam index 05fc5ba..1420105 100644 --- a/server/src/crate_server/catalog_index_postgres.gleam +++ b/server/src/crate_server/catalog_index_postgres.gleam @@ -67,11 +67,8 @@ pub fn table_store(conn: pog.Connection) -> Result(Store, String) { )) } -/// The tables `migrate` below creates. The readiness probe -/// (`crate_server/readiness.required_tables`) checks these exist rather -/// than mirroring their columns, so an `alter table ... add column` -/// migration here (see the format/label/master/barcode ones below) never -/// needs a matching edit there. +/// The tables `migrate` below creates; the readiness probe checks that +/// they exist rather than mirroring their columns. pub fn required_tables() -> List(String) { ["catalog_releases", "catalog_adoptions", "catalog_edits", "jetstream_cursor"] } diff --git a/server/src/crate_server/follow_index_postgres.gleam b/server/src/crate_server/follow_index_postgres.gleam index 68a157d..e03a45f 100644 --- a/server/src/crate_server/follow_index_postgres.gleam +++ b/server/src/crate_server/follow_index_postgres.gleam @@ -33,10 +33,8 @@ pub fn table_store(conn: pog.Connection) -> Result(Store, String) { )) } -/// The tables `migrate` below creates. The readiness probe -/// (`crate_server/readiness.required_tables`) checks these exist rather -/// than mirroring their columns, so a migration adding a column here never -/// needs a matching edit there. +/// The tables `migrate` below creates; the readiness probe checks that +/// they exist rather than mirroring their columns. pub fn required_tables() -> List(String) { ["follow_edges", "follow_index_seen"] } diff --git a/server/src/crate_server/known_users_postgres.gleam b/server/src/crate_server/known_users_postgres.gleam index 03d1628..991d9cf 100644 --- a/server/src/crate_server/known_users_postgres.gleam +++ b/server/src/crate_server/known_users_postgres.gleam @@ -19,10 +19,8 @@ pub fn table_store(conn: pog.Connection) -> Result(Store, String) { ) } -/// The table `migrate` below creates. The readiness probe -/// (`crate_server/readiness.required_tables`) checks it exists rather than -/// mirroring its columns, so a migration adding a column here never needs a -/// matching edit there. +/// The table `migrate` below creates; the readiness probe checks that it +/// exists rather than mirroring its columns. pub fn required_tables() -> List(String) { ["known_users"] } diff --git a/server/src/crate_server/oauth/sessions_postgres.gleam b/server/src/crate_server/oauth/sessions_postgres.gleam index 91e840c..e09bdea 100644 --- a/server/src/crate_server/oauth/sessions_postgres.gleam +++ b/server/src/crate_server/oauth/sessions_postgres.gleam @@ -69,10 +69,8 @@ pub fn connect(database_url: String) -> Result(Store, String) { table_store(conn, oauth_sessions_table, Some(sessions.ttl_seconds)) } -/// The two tables `crate_server/config_env` actually wires `table_store` -/// against. Named here, not just at the call site, so the readiness probe -/// (`crate_server/readiness.required_tables`) can require the same names -/// `config_env` passes in without a third copy of them. +/// Named here rather than only at the `config_env` call site, so the readiness +/// probe requires the same names without a third copy of them. pub const oauth_sessions_table = "oauth_sessions" pub const discogs_creds_table = "discogs_creds" diff --git a/server/src/crate_server/readiness.gleam b/server/src/crate_server/readiness.gleam index b8e9c61..435ea8e 100644 --- a/server/src/crate_server/readiness.gleam +++ b/server/src/crate_server/readiness.gleam @@ -1,6 +1,6 @@ -//// Readiness is intentionally a capability rather than an environment flag. -//// One actor coalesces concurrent probe requests and caches the result for a -//// short interval, so a public endpoint cannot amplify database work. +//// Readiness is a capability, not an environment flag: one actor coalesces +//// concurrent probes and briefly caches the result, so a public endpoint +//// cannot amplify database work. import crate_server/catalog_index_postgres import crate_server/clock @@ -20,11 +20,8 @@ import pog const cache_ttl_seconds = 5 -/// The tables every store's own `*_postgres` module says it needs (see each -/// module's `required_tables`). This is the one place that list is -/// assembled from every store, so it stays exhaustive; each module is the -/// one place its own table names live, so adding a column to one of their -/// `create table`/`alter table` statements never touches this file. +/// Every store's own required tables, assembled here so the list stays +/// exhaustive while each module stays the only place its table names live. pub fn required_tables() -> List(String) { known_users_postgres.required_tables() |> list.append(catalog_index_postgres.required_tables()) @@ -33,15 +30,11 @@ pub fn required_tables() -> List(String) { |> list.append(sessions_postgres.required_tables()) } -/// Returns the generated readiness query for regression tests. Deliberately -/// narrower than a full column/type/not-null/unique-index audit: it checks -/// that every required table exists and that the connected role holds -/// baseline CRUD privileges on each, which is what actually gates the app -/// from working and is cheap enough to run inside `local_wait_ms`. Table -/// names come from `required_tables()`, are internal constants (never user -/// input), so interpolating them is not injection -- the same reasoning -/// `oauth/sessions_postgres` already relies on for its table-name -/// interpolation. +/// The generated readiness query, exposed for regression tests. Checks that +/// every required table exists and the role holds baseline CRUD on it, rather +/// than auditing columns and indexes: that is what gates the app, and it is +/// cheap enough to finish inside `local_wait_ms`. The interpolated names are +/// internal constants, never user input. pub fn schema_check_sql() -> String { let names = required_tables() @@ -64,10 +57,8 @@ pub type Check { type State { State( last: Option(#(Bool, Int)), - // The generation and start time of the refresh in flight, if any. The - // generation lets a reply be matched to the attempt that asked for it, - // so a late answer from an attempt we have since given up on cannot be - // mistaken for the current one's. + // Generation and start time of the refresh in flight; the generation keeps + // a late reply from an abandoned attempt out of the current one. in_flight: Option(#(Int, Int)), next_generation: Int, worker_replies: Subject(Msg), @@ -79,38 +70,23 @@ type Msg { ProbeResult(Int, Bool) } -/// Bounds how long a `Probe` message may block the actor waiting on a fresh -/// probe result before it gives up and answers from the cache instead. Kept -/// well under `call_budget_ms` so the actor always has room to reply before -/// its caller's own timeout fires, and well under the docker/Caddy -/// healthcheck timeouts below so a slow-but-healthy probe still answers -/// inside one health check even when it can't finish inside this budget. +// How long the actor waits on a fresh result before answering from cache. +// Under `call_budget_ms` and the healthcheck timeouts, so a slow-but-healthy +// probe still answers inside one health check. const local_wait_ms = 500 -/// The budget `cached()` gives callers to hear back from the actor. Deliberately -/// above the docker healthcheck's `--timeout=3s` and below Caddy's -/// `health_timeout 3s`-driven poll cadence: the actor itself never blocks -/// longer than `local_wait_ms`, so this budget is headroom for scheduling, -/// not for the probe itself. See deploy/Caddyfile and the Dockerfile -/// HEALTHCHECK. +// Above the docker healthcheck's --timeout=3s, below Caddy's health_timeout: +// headroom for scheduling, since the actor never blocks past `local_wait_ms`. const call_budget_ms = 4000 -/// How long a stuck refresh is tolerated before a fresh one may start -/// alongside it. Tied to Caddy's `health_interval 10s` (deploy/Caddyfile): -/// once a probe has run longer than one full health-poll cycle without -/// answering, waiting on it any further buys nothing, and it may be a -/// worker that crashed without ever sending a reply rather than one that is -/// merely slow -- either way, a probe that plainly overran must not disable -/// all future probing for the actor's lifetime. +// One full Caddy health_interval (deploy/Caddyfile). Past that the refresh has +// plainly overrun, or its worker died without replying; either way it must not +// wedge probing for the actor's lifetime. const refresh_overrun_seconds = 10 -/// How long a cached answer may be trusted before it must degrade to "not -/// ready", regardless of what it says. Tied to the docker healthcheck's -/// `interval: 30s` (docker-compose.yml), three Caddy poll cycles. Without -/// this ceiling, a database that hangs rather than errors (so `in_flight` -/// never clears) would have every later `Probe` fall back to the last -/// known-good state forever: the exact inverse of the timeout-vs-failure -/// bug this file exists to fix, and one that fails silently. +// Three Caddy poll cycles, the docker healthcheck interval. Without a ceiling +// a database that hangs rather than errors would serve the last known-good +// answer forever, silently. const max_staleness_seconds = 30 pub fn ready() -> Check { @@ -125,21 +101,11 @@ pub fn postgres(conn: pog.Connection) -> Check { cached(fn() { schema_is_ready(conn) }, clock.now_seconds) } -/// Coalesces calls through one actor, so normally only one `probe` runs at a -/// time. The actor never runs `probe` itself: it spawns a worker for it and -/// waits up to `local_wait_ms` for the result, so a slow probe never wedges -/// the actor's mailbox behind it. A probe that doesn't finish in time is -/// "unknown", not "broken": the actor answers with the last known state (or -/// `False` if none exists yet, or if that state has gone stale past -/// `max_staleness_seconds`), and the worker's result folds into state -/// through the actor's own selector whenever it lands -- it must go through -/// that selector rather than a bare manual receive, or the actor's built-in -/// catch-all for unrecognised messages discards it the moment the loop goes -/// back to waiting. If a refresh overruns `refresh_overrun_seconds` -- a -/// hung query, or a worker that crashed without replying at all -- a fresh -/// one is allowed to start rather than leaving probing wedged for good; the -/// generation on each reply keeps a stale answer from either attempt being -/// applied to the wrong one. +/// Coalesces calls through one actor. It spawns a worker for `probe` rather +/// than running it, so a slow probe never wedges its mailbox: an unfinished +/// probe is "unknown" and answered from the last known state, and the worker's +/// reply folds in later. That reply must arrive through the actor's selector; +/// a bare receive loses it to the built-in catch-all. pub fn cached(probe: fn() -> Bool, now: fn() -> Int) -> Check { let assert Ok(started) = actor.new_with_initialiser(1000, fn(subject) { @@ -187,9 +153,8 @@ fn handle( } } -/// Folds a worker's answer into state only if it belongs to the refresh -/// currently in flight; a reply from an attempt already abandoned to -/// overrun is discarded rather than overwriting a newer one. +// A reply from an attempt already abandoned to overrun must not overwrite the +// current one. fn accept( state: State, generation: Int, @@ -229,9 +194,8 @@ fn needs_refresh(last: Option(#(Bool, Int)), now: fn() -> Int) -> Bool { } } -/// The best answer available without waiting on the database: the last -/// known state, or "not ready" if the database has never once answered or -/// that state is older than `max_staleness_seconds` can vouch for. +// The best answer available without waiting on the database, vouching for +// nothing older than `max_staleness_seconds`. fn fallback(last: Option(#(Bool, Int)), now: fn() -> Int) -> Bool { case last { Some(#(available, checked_at)) -> @@ -259,8 +223,7 @@ fn refresh( in_flight: Some(#(generation, now())), next_generation: generation + 1, ) - // Only ProbeResult ever lands on `replies`; the Probe arm below is - // unreachable but keeps this match total without a panic. + // Only ProbeResult lands on `replies`; the catch-all keeps the match total. case process.receive(state.worker_replies, local_wait_ms) { Ok(ProbeResult(reply_generation, available)) if reply_generation == generation diff --git a/server/src/crate_server/shelf_index_postgres.gleam b/server/src/crate_server/shelf_index_postgres.gleam index e887019..4ff9a92 100644 --- a/server/src/crate_server/shelf_index_postgres.gleam +++ b/server/src/crate_server/shelf_index_postgres.gleam @@ -45,10 +45,8 @@ pub fn table_store(conn: pog.Connection) -> Result(shelf_index.Store, String) { )) } -/// The tables `migrate` below creates. The readiness probe -/// (`crate_server/readiness.required_tables`) checks these exist rather -/// than mirroring their columns, so a migration adding a column here never -/// needs a matching edit there. +/// The tables `migrate` below creates; the readiness probe checks that +/// they exist rather than mirroring their columns. pub fn required_tables() -> List(String) { ["shelf_entry_events", "shelf_entries", "shelf_index_seen"] } diff --git a/server/test/health_test.gleam b/server/test/health_test.gleam index a387950..60931ea 100644 --- a/server/test/health_test.gleam +++ b/server/test/health_test.gleam @@ -137,16 +137,14 @@ pub fn probe_timeout_falls_back_to_last_known_state_test() { now, ) - // First probe is fast: it establishes a known-good state. + // Fast probe: a known-good state. assert readiness.is_ready(check) - // The TTL has lapsed, so this call starts a refresh, but the new probe is - // slow. A timeout must fall back to the last known state, not go straight - // to "not ready". + // TTL lapsed, refresh started but slow. Timing out must fall back to the + // last known state rather than report "not ready". assert readiness.is_ready(check) - // Once the slow probe actually completes and fails, that becomes the - // reported state. + // The slow probe finishes and fails, which becomes the reported state. process.sleep(900) assert !readiness.is_ready(check) } @@ -159,9 +157,8 @@ type CountMsg { Bump(process.Subject(Int)) } -/// Returns how many times it has been called so far, starting at 0, via one -/// actor: readiness probes run on a spawned worker, so a plain closed-over -/// variable would race. +// Call count from 0, held in an actor: probes run on a spawned worker, so a +// closed-over variable would race. fn counter() -> fn() -> Int { let assert Ok(started) = actor.new(CountState(n: 0)) @@ -278,17 +275,15 @@ pub fn a_wedged_probe_eventually_reports_not_ready_test() { now, ) - // Establishes a known-good state. + // A known-good state. assert readiness.is_ready(check) - // The TTL lapses and the refresh hangs (the database never returns). The - // last known-good state is still within its staleness ceiling, so it is - // still trusted while the probe is stuck. + // The refresh hangs, but the cached state is inside its staleness ceiling + // and stays trusted. assert readiness.is_ready(check) - // Once that cached state has been trusted longer than its staleness - // ceiling allows, a database that hangs rather than errors must stop - // advertising as ready, even though the original probe never finished. + // Past the ceiling, a database that hangs rather than errors must stop + // advertising as ready even though its probe never finished. assert !readiness.is_ready(check) } @@ -306,10 +301,10 @@ pub fn a_crashing_worker_does_not_permanently_disable_probing_test() { now, ) - // The very first probe's worker crashes before ever answering. + // The first probe's worker crashes before answering. assert !readiness.is_ready(check) - // Probing must recover once the crashed attempt has plainly overrun, - // rather than being wedged forever by a worker that died without a reply. + // Probing must recover once the crashed attempt has overrun, not stay wedged + // behind a worker that died without replying. assert readiness.is_ready(check) }