fix(db): survive a database restart and a not-yet-ready database
Two connection-recovery gaps, both surfaced by a Postgres restart that left the app throwing tracebacks while the DB itself was healthy. pool_pre_ping + pool_recycle on the app engine [#2626]: when the database restarts, every connection already in the pool is dead at the socket level. SQLAlchemy only discovered that by failing a real query, so the first operation after a restart errored out on whatever triggered it. pre_ping checks liveness on checkout and swaps the dead connection transparently; pool_recycle caps connection age so a socket stranded by a NAT/conntrack timeout or a Docker network rebuild is retired on a timer instead. wait_for_database() gate at startup [#2627]: create_app touches the DB synchronously (migrations, secret re-encryption, settings load) and assumed it was both resolvable and accepting connections on the first try. Neither holds after a host reboot (Docker DNS not yet serving `db` -> gaierror -2) or an unclean shutdown (Postgres still replaying WAL -> "not yet accepting connections"). Both are transient, so retry with capped backoff behind one gate ahead of the first DB touch. Credential and missing-database errors are classified by SQLSTATE and still fail immediately -- waiting cannot fix those. Budget is bootstrap-configurable (STEWARD_DB_CONNECT_TIMEOUT / database.connect_timeout, default 60s) since it governs reaching the DB and so cannot live in the DB-backed settings. Tests drive the retry loop off a fake clock, so backoff and timeout behaviour are deterministic rather than wall-clock dependent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,215 @@
|
||||
"""Startup DB readiness retry + connection-pool resilience.
|
||||
|
||||
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.
|
||||
"""
|
||||
import types
|
||||
import pytest
|
||||
|
||||
from steward import database
|
||||
from steward.database import (
|
||||
POOL_RECYCLE_SECONDS,
|
||||
_describe,
|
||||
_fatal_sqlstate,
|
||||
init_db,
|
||||
wait_for_database,
|
||||
)
|
||||
|
||||
|
||||
class _PGError(Exception):
|
||||
"""Stand-in for an asyncpg error, which carries a SQLSTATE attribute."""
|
||||
|
||||
def __init__(self, message: str, sqlstate: str | None = None):
|
||||
super().__init__(message)
|
||||
self.sqlstate = sqlstate
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def no_sleep(monkeypatch):
|
||||
"""Drive the retry budget off a fake clock instead of wall time.
|
||||
|
||||
sleep() records its duration and advances the clock by exactly that much,
|
||||
so backoff and timeout behaviour are deterministic rather than dependent on
|
||||
how fast the test machine happens to run.
|
||||
"""
|
||||
slept: list[float] = []
|
||||
clock = {"now": 0.0}
|
||||
|
||||
def fake_sleep(seconds: float) -> None:
|
||||
slept.append(seconds)
|
||||
clock["now"] += seconds
|
||||
|
||||
monkeypatch.setattr(database.time, "sleep", fake_sleep)
|
||||
monkeypatch.setattr(database.time, "monotonic", lambda: clock["now"])
|
||||
return slept
|
||||
|
||||
|
||||
def _probe_raising(*errors, then_succeed: bool = True):
|
||||
"""Async probe stub raising the given errors in order.
|
||||
|
||||
Once the list is exhausted it succeeds, unless then_succeed is False — in
|
||||
which case it keeps raising the last error forever (a DB that never comes
|
||||
back).
|
||||
"""
|
||||
calls = {"n": 0}
|
||||
|
||||
async def probe(db_url):
|
||||
i = calls["n"]
|
||||
calls["n"] += 1
|
||||
if i < len(errors):
|
||||
raise errors[i]
|
||||
if not then_succeed:
|
||||
raise errors[-1]
|
||||
|
||||
probe.calls = calls
|
||||
return probe
|
||||
|
||||
|
||||
# ── engine pool configuration (issue #2626) ──────────────────────────────────
|
||||
|
||||
|
||||
def test_engine_enables_pre_ping_and_recycle():
|
||||
app = types.SimpleNamespace(
|
||||
config={"DATABASE_URL": "postgresql+asyncpg://u:p@localhost/db"}
|
||||
)
|
||||
init_db(app)
|
||||
|
||||
# _pre_ping / _recycle are SQLAlchemy pool internals; there is no public
|
||||
# accessor, and these are exactly the settings a DB restart depends on.
|
||||
pool = app._db_engine.sync_engine.pool
|
||||
assert pool._pre_ping is True
|
||||
assert pool._recycle == POOL_RECYCLE_SECONDS
|
||||
|
||||
|
||||
def test_pool_recycle_is_positive():
|
||||
# A non-positive recycle disables age-based retirement entirely.
|
||||
assert POOL_RECYCLE_SECONDS > 0
|
||||
|
||||
|
||||
# ── retry classification ─────────────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_fatal_sqlstate_detects_bad_password():
|
||||
exc = _PGError("password authentication failed", sqlstate="28P01")
|
||||
assert _fatal_sqlstate(exc) == "28P01"
|
||||
|
||||
|
||||
def test_fatal_sqlstate_finds_sqlstate_through_cause_chain():
|
||||
inner = _PGError("database does not exist", sqlstate="3D000")
|
||||
outer = RuntimeError("wrapped by sqlalchemy")
|
||||
outer.__cause__ = inner
|
||||
assert _fatal_sqlstate(outer) == "3D000"
|
||||
|
||||
|
||||
def test_transient_errors_are_not_fatal():
|
||||
assert _fatal_sqlstate(OSError(-2, "Name or service not known")) is None
|
||||
# "the database system is not yet accepting connections" — resolves on its own.
|
||||
assert _fatal_sqlstate(_PGError("not yet accepting", sqlstate="57P03")) is None
|
||||
|
||||
|
||||
def test_describe_unwraps_to_innermost_cause():
|
||||
inner = OSError("Name or service not known")
|
||||
outer = RuntimeError("sqlalchemy boilerplate")
|
||||
outer.__cause__ = inner
|
||||
assert "Name or service not known" in _describe(outer)
|
||||
|
||||
|
||||
def test_describe_survives_self_referential_cause():
|
||||
exc = RuntimeError("loop")
|
||||
exc.__cause__ = exc
|
||||
assert "loop" in _describe(exc)
|
||||
|
||||
|
||||
# ── wait_for_database (issue #2627) ──────────────────────────────────────────
|
||||
|
||||
|
||||
def test_returns_immediately_when_db_is_up(monkeypatch, no_sleep):
|
||||
probe = _probe_raising()
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
wait_for_database("postgresql+asyncpg://u:p@db/steward", timeout_seconds=10)
|
||||
|
||||
assert probe.calls["n"] == 1
|
||||
assert no_sleep == []
|
||||
|
||||
|
||||
def test_retries_dns_failure_then_succeeds(monkeypatch, no_sleep):
|
||||
# The reported failure: gaierror -2 while Docker DNS is not yet serving `db`.
|
||||
probe = _probe_raising(
|
||||
OSError(-2, "Name or service not known"),
|
||||
OSError(-2, "Name or service not known"),
|
||||
)
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
wait_for_database("postgresql+asyncpg://u:p@db/steward", timeout_seconds=30)
|
||||
|
||||
assert probe.calls["n"] == 3
|
||||
assert len(no_sleep) == 2
|
||||
|
||||
|
||||
def test_retries_while_postgres_is_still_recovering(monkeypatch, no_sleep):
|
||||
probe = _probe_raising(
|
||||
_PGError("the database system is not yet accepting connections", "57P03"),
|
||||
)
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
wait_for_database("postgresql+asyncpg://u:p@db/steward", timeout_seconds=30)
|
||||
|
||||
assert probe.calls["n"] == 2
|
||||
|
||||
|
||||
def test_backoff_grows_between_attempts(monkeypatch, no_sleep):
|
||||
probe = _probe_raising(*[OSError("refused")] * 4)
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
wait_for_database("postgresql+asyncpg://u:p@db/steward", timeout_seconds=60)
|
||||
|
||||
assert no_sleep == sorted(no_sleep), "delays should be non-decreasing"
|
||||
assert no_sleep[-1] > no_sleep[0], "backoff should grow, not stay flat"
|
||||
|
||||
|
||||
def test_backoff_is_capped(monkeypatch, no_sleep):
|
||||
probe = _probe_raising(*[OSError("refused")] * 12)
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
wait_for_database("postgresql+asyncpg://u:p@db/steward", timeout_seconds=600)
|
||||
|
||||
assert max(no_sleep) <= database._BACKOFF_MAX_SECONDS
|
||||
|
||||
|
||||
def test_bad_credentials_fail_immediately_without_retrying(monkeypatch, no_sleep):
|
||||
probe = _probe_raising(_PGError("password authentication failed", "28P01"))
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
with pytest.raises(_PGError):
|
||||
wait_for_database("postgresql+asyncpg://u:bad@db/steward", timeout_seconds=30)
|
||||
|
||||
# Waiting cannot fix a wrong password — don't burn the whole budget on it.
|
||||
assert probe.calls["n"] == 1
|
||||
assert no_sleep == []
|
||||
|
||||
|
||||
def test_missing_database_fails_immediately(monkeypatch, no_sleep):
|
||||
probe = _probe_raising(_PGError("database does not exist", "3D000"))
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
with pytest.raises(_PGError):
|
||||
wait_for_database("postgresql+asyncpg://u:p@db/nope", timeout_seconds=30)
|
||||
|
||||
assert probe.calls["n"] == 1
|
||||
|
||||
|
||||
def test_gives_up_after_budget_with_actionable_message(monkeypatch, no_sleep):
|
||||
probe = _probe_raising(
|
||||
OSError("Name or service not known"), then_succeed=False
|
||||
)
|
||||
monkeypatch.setattr(database, "_probe", probe)
|
||||
|
||||
with pytest.raises(RuntimeError) as excinfo:
|
||||
wait_for_database("postgresql+asyncpg://u:p@db/steward", timeout_seconds=2)
|
||||
|
||||
message = str(excinfo.value)
|
||||
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
|
||||
@@ -65,3 +65,50 @@ def test_secret_key_unpersistable_raises_instead_of_ephemeral(tmp_path, monkeypa
|
||||
monkeypatch.delenv("STEWARD_SECRET_KEY", raising=False)
|
||||
with pytest.raises(RuntimeError, match="could not persist"):
|
||||
_resolve_secret_key({})
|
||||
|
||||
|
||||
# ── database connect timeout (bootstrap-only: it governs reaching the DB) ────
|
||||
|
||||
|
||||
def test_db_connect_timeout_defaults_when_unset(tmp_path, monkeypatch):
|
||||
from steward.database import DB_CONNECT_TIMEOUT_SECONDS
|
||||
cfg_file = tmp_path / "config.yaml"
|
||||
cfg_file.write_text("database:\n url: x\nsecret_key: s\n")
|
||||
monkeypatch.delenv("STEWARD_DB_CONNECT_TIMEOUT", raising=False)
|
||||
cfg = load_bootstrap(cfg_file)
|
||||
assert cfg["db_connect_timeout"] == DB_CONNECT_TIMEOUT_SECONDS
|
||||
|
||||
|
||||
def test_db_connect_timeout_from_yaml(tmp_path, monkeypatch):
|
||||
cfg_file = tmp_path / "config.yaml"
|
||||
cfg_file.write_text(
|
||||
"database:\n url: x\n connect_timeout: 120\nsecret_key: s\n")
|
||||
monkeypatch.delenv("STEWARD_DB_CONNECT_TIMEOUT", raising=False)
|
||||
assert load_bootstrap(cfg_file)["db_connect_timeout"] == 120.0
|
||||
|
||||
|
||||
def test_db_connect_timeout_env_overrides_yaml(tmp_path, monkeypatch):
|
||||
cfg_file = tmp_path / "config.yaml"
|
||||
cfg_file.write_text(
|
||||
"database:\n url: x\n connect_timeout: 120\nsecret_key: s\n")
|
||||
monkeypatch.setenv("STEWARD_DB_CONNECT_TIMEOUT", "5")
|
||||
assert load_bootstrap(cfg_file)["db_connect_timeout"] == 5.0
|
||||
|
||||
|
||||
def test_db_connect_timeout_garbage_falls_back_to_default(tmp_path, monkeypatch):
|
||||
from steward.database import DB_CONNECT_TIMEOUT_SECONDS
|
||||
cfg_file = tmp_path / "config.yaml"
|
||||
cfg_file.write_text("database:\n url: x\nsecret_key: s\n")
|
||||
monkeypatch.setenv("STEWARD_DB_CONNECT_TIMEOUT", "not-a-number")
|
||||
cfg = load_bootstrap(cfg_file)
|
||||
assert cfg["db_connect_timeout"] == DB_CONNECT_TIMEOUT_SECONDS
|
||||
|
||||
|
||||
def test_db_connect_timeout_non_positive_falls_back_to_default(tmp_path, monkeypatch):
|
||||
# 0 would mean "never wait", reinstating the crash this setting exists to fix.
|
||||
from steward.database import DB_CONNECT_TIMEOUT_SECONDS
|
||||
cfg_file = tmp_path / "config.yaml"
|
||||
cfg_file.write_text("database:\n url: x\nsecret_key: s\n")
|
||||
monkeypatch.setenv("STEWARD_DB_CONNECT_TIMEOUT", "0")
|
||||
cfg = load_bootstrap(cfg_file)
|
||||
assert cfg["db_connect_timeout"] == DB_CONNECT_TIMEOUT_SECONDS
|
||||
|
||||
Reference in New Issue
Block a user