diff --git a/steward/app.py b/steward/app.py index 14d822f..1c241a1 100644 --- a/steward/app.py +++ b/steward/app.py @@ -3,9 +3,12 @@ from __future__ import annotations import asyncio import logging from pathlib import Path -from quart import Quart, render_template +from quart import Quart, render_template, request from .config import load_bootstrap -from .database import init_db, DB_CONNECT_TIMEOUT_SECONDS +from .database import ( + init_db, ensure_database_reachable, DatabaseUnavailable, + DB_CONNECT_TIMEOUT_SECONDS, +) logger = logging.getLogger(__name__) @@ -245,6 +248,29 @@ def create_app( async def health(): return {"status": "ok"} + # ── 11b. Database availability gate ──────────────────────────────────────── + # Every page in Steward reads the database, so a database that has gone away + # under a running app turns each request into an opaque 500 (this is how a + # login attempt surfaced a bare gaierror). Acquire a connection up front, + # retrying briefly to ride out a restart, and answer honestly if it stays + # down rather than failing deep inside a handler with a generic error. + if not testing: + @app.before_request + async def _database_gate(): + # /health is a liveness probe for the container itself — it must + # stay answerable while the database is down, or a restart loop + # gets triggered by a dependency outage. Static files need no DB. + if request.endpoint in ("health", "static"): + return None + try: + await ensure_database_reachable(app._db_engine) + except DatabaseUnavailable as exc: + logger.error("Database unreachable while serving %s: %s", + request.path, exc) + return await render_template( + "errors/database_unavailable.html"), 503 + return None + # ── 12. Error handlers ───────────────────────────────────────────────────── @app.errorhandler(404) async def not_found(_): diff --git a/steward/database.py b/steward/database.py index af24f8f..eeef900 100644 --- a/steward/database.py +++ b/steward/database.py @@ -4,7 +4,10 @@ import logging import time from typing import TYPE_CHECKING from sqlalchemy import text -from sqlalchemy.ext.asyncio import create_async_engine, async_sessionmaker, AsyncSession +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.ext.asyncio import ( + create_async_engine, async_sessionmaker, AsyncEngine, AsyncSession, +) from sqlalchemy.pool import NullPool if TYPE_CHECKING: @@ -26,6 +29,13 @@ DB_CONNECT_TIMEOUT_SECONDS = 60.0 _BACKOFF_INITIAL_SECONDS = 0.5 _BACKOFF_MAX_SECONDS = 5.0 +# Per-REQUEST retry schedule (seconds between attempts), total ~1.75s across 4 +# tries. Far shorter than the startup budget on purpose: nobody is watching a +# container boot, but somebody is watching this page load. Long enough to ride +# out a pool blip or a fast reconnect, short enough that a genuinely-down +# database gets an honest answer instead of a spinner. +_REQUEST_RETRY_DELAYS = (0.25, 0.5, 1.0) + # Postgres SQLSTATEs that will never resolve by waiting — retrying these just # delays a clear error behind a full timeout budget. _FATAL_SQLSTATES = { @@ -147,6 +157,48 @@ def wait_for_database( ) from last_exc +class DatabaseUnavailable(Exception): + """The database could not be reached while serving a request.""" + + +async def ensure_database_reachable(engine: AsyncEngine) -> None: + """Check out one pooled connection, retrying briefly, or raise. + + Startup readiness (wait_for_database) does not help once the app is already + serving: if the database goes away underneath a running Steward, the next + request needs a connection, the pooled ones are dead, and establishing a new + one fails — which is how a login attempt turned into an opaque 500. + + pool_pre_ping already makes recovery automatic *once the database is back*. + What it cannot do is wait: while the container is genuinely down, its + replacement connect fails too. So retry briefly here to ride out a restart, + then give up and let the caller render an honest "database unavailable" + page rather than a generic error. + + The budget is deliberately short — a person is waiting on this request, and + a page that hangs for half a minute is worse than one that says plainly + what is wrong and offers a retry. + """ + last_exc: BaseException | None = None + for attempt, delay in enumerate(_REQUEST_RETRY_DELAYS + (None,)): + try: + async with engine.connect(): + if attempt: + logger.info( + "Database reachable again after %d retry attempt(s)", attempt) + return + except (SQLAlchemyError, OSError) as exc: + # Bad credentials / missing database will never resolve by waiting, + # and they are not what this guard is for — let them surface. + if _fatal_sqlstate(exc) is not None: + raise + last_exc = exc + if delay is not None: + await asyncio.sleep(delay) + + raise DatabaseUnavailable(_describe(last_exc)) from last_exc + + async def _probe(db_url: str) -> None: """Open one throwaway connection and round-trip a trivial query. diff --git a/steward/templates/errors/database_unavailable.html b/steward/templates/errors/database_unavailable.html new file mode 100644 index 0000000..f664a06 --- /dev/null +++ b/steward/templates/errors/database_unavailable.html @@ -0,0 +1,16 @@ +{% extends "base.html" %} +{% block title %}Database unavailable — Steward{% endblock %} +{% block content %} +
+

503

+

The records are out of reach.

+

+ Steward is running, but cannot reach its database. Nothing has been lost — + this page will work again as soon as the database is back. +

+

+ Steward already retried for a moment before showing this. +

+ Try again +
+{% endblock %} diff --git a/tests/core/test_db_readiness.py b/tests/core/test_db_readiness.py index cedb64c..d5746ba 100644 --- a/tests/core/test_db_readiness.py +++ b/tests/core/test_db_readiness.py @@ -1,8 +1,9 @@ -"""Startup DB readiness retry + connection-pool resilience. +"""DB connection resilience: pool, startup readiness, and request-time outage. -Covers the two recovery gaps that let a database restart take the app down: -the engine handing out stale pooled connections, and startup assuming the DB -is reachable and ready the instant it asks. +Covers the three recovery gaps that let a database restart take the app down: +the engine handing out stale pooled connections (#2626), startup assuming the +DB is reachable and ready the instant it asks (#2627), and a request finding +the database gone and failing with an opaque 500 (#2635). """ import types import pytest @@ -213,3 +214,94 @@ def test_gives_up_after_budget_with_actionable_message(monkeypatch, no_sleep): assert "did not become available" in message # The operator needs the underlying cause, not just "timed out". assert "Name or service not known" in message + + +# ── ensure_database_reachable (issue #2635) ────────────────────────────────── + + +class _FakeEngine: + """Minimal stand-in for AsyncEngine.connect() as an async context manager.""" + + def __init__(self, *errors, then_succeed: bool = True): + self.errors = list(errors) + self.then_succeed = then_succeed + self.calls = 0 + + def connect(self): + engine = self + + class _Ctx: + async def __aenter__(self): + index = engine.calls + engine.calls += 1 + if index < len(engine.errors): + raise engine.errors[index] + if not engine.then_succeed: + raise engine.errors[-1] + return object() + + async def __aexit__(self, *exc_info): + return False + + return _Ctx() + + +@pytest.fixture +def no_async_sleep(monkeypatch): + """Record awaited retry delays without spending them.""" + slept: list[float] = [] + + async def fake_sleep(seconds: float) -> None: + slept.append(seconds) + + monkeypatch.setattr(database.asyncio, "sleep", fake_sleep) + return slept + + +async def test_reachable_on_first_try_costs_no_retries(no_async_sleep): + engine = _FakeEngine() + await database.ensure_database_reachable(engine) + assert engine.calls == 1 + assert no_async_sleep == [] + + +async def test_rides_out_a_brief_outage(no_async_sleep): + # The reported shape: DNS gone while the db container restarts. + engine = _FakeEngine(OSError(-2, "Name or service not known")) + await database.ensure_database_reachable(engine) + assert engine.calls == 2 + assert no_async_sleep == [0.25] + + +async def test_uses_the_full_retry_schedule_before_giving_up(no_async_sleep): + engine = _FakeEngine(OSError("refused"), then_succeed=False) + with pytest.raises(database.DatabaseUnavailable): + await database.ensure_database_reachable(engine) + + # One attempt per delay, plus a final attempt after the last wait. + assert engine.calls == len(database._REQUEST_RETRY_DELAYS) + 1 + assert no_async_sleep == list(database._REQUEST_RETRY_DELAYS) + + +async def test_request_budget_stays_short(no_async_sleep): + # A person is waiting on this; guard against the schedule growing into a hang. + assert sum(database._REQUEST_RETRY_DELAYS) <= 3.0 + + +async def test_unavailable_error_carries_the_underlying_cause(no_async_sleep): + engine = _FakeEngine( + OSError("Name or service not known"), then_succeed=False) + with pytest.raises(database.DatabaseUnavailable) as excinfo: + await database.ensure_database_reachable(engine) + assert "Name or service not known" in str(excinfo.value) + + +async def test_bad_credentials_are_not_masked_as_unavailable(no_async_sleep): + # Retrying a wrong password would be pointless, and reporting it as + # "database unavailable" would send the operator chasing the wrong problem. + engine = _FakeEngine( + _PGError("password authentication failed", "28P01"), then_succeed=False) + with pytest.raises(_PGError): + await database.ensure_database_reachable(engine) + assert engine.calls == 1 + assert no_async_sleep == []