diff --git a/server/core/errors.py b/server/core/errors.py index 608fed8..47cf3af 100644 --- a/server/core/errors.py +++ b/server/core/errors.py @@ -9,6 +9,7 @@ from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse from pydantic import BaseModel +from sqlalchemy.exc import OperationalError from starlette.exceptions import HTTPException as StarletteHTTPException logger = structlog.stdlib.get_logger() @@ -39,7 +40,14 @@ def _request_id(request: Request) -> str | None: return getattr(request.state, "request_id", None) -def _error_json(code: str, message: str, request: Request, details: Any = None, status: int = 500): +def _error_json( + code: str, + message: str, + request: Request, + details: Any = None, + status: int = 500, + headers: dict[str, str] | None = None, +): body = ErrorResponse( error=ErrorDetail( code=code, @@ -48,7 +56,9 @@ def _error_json(code: str, message: str, request: Request, details: Any = None, request_id=_request_id(request), ) ) - return JSONResponse(status_code=status, content=body.model_dump(exclude_none=True)) + return JSONResponse( + status_code=status, content=body.model_dump(exclude_none=True), headers=headers + ) # --------------------------------------------------------------------------- @@ -99,6 +109,29 @@ async def http_exception_handler(request: Request, exc: StarletteHTTPException): status=exc.status_code, ) + @app.exception_handler(OperationalError) + async def operational_error_handler(request: Request, exc: OperationalError): + # A DB outage is not a bug in the handling endpoint; /readyz already + # reports 503 for the identical condition (ReadinessResult.http_status), + # so this keeps both surfaces agreeing on what a DB outage means. + # Starlette picks the most specific handler by exception class (MRO), + # so this wins over the Exception catch-all regardless of registration + # order. server/db/engine.py normalizes the asyncpg outage shapes that + # would otherwise bypass this handler (raw OSError at connect time, + # bare DBAPIError on a mid-query disconnect) into OperationalError. + logger.error( + "database_unavailable", + request_id=_request_id(request), + exc_msg=str(exc)[:200], + ) + return _error_json( + code="service_unavailable", + message="The database is temporarily unavailable. Please retry shortly.", + request=request, + status=503, + headers={"Retry-After": "5"}, + ) + @app.exception_handler(Exception) async def unhandled_exception_handler(request: Request, exc: Exception): logger.error( diff --git a/server/db/engine.py b/server/db/engine.py index 7b664dd..1937d4c 100644 --- a/server/db/engine.py +++ b/server/db/engine.py @@ -2,6 +2,7 @@ from typing import AsyncGenerator +from sqlalchemy import event, exc from sqlalchemy.ext.asyncio import AsyncEngine, AsyncSession, async_sessionmaker, create_async_engine from server.core.config import settings @@ -11,6 +12,43 @@ _async_session_factory: async_sessionmaker[AsyncSession] | None = None +def _install_outage_normalization(engine: AsyncEngine) -> None: + """Surface database outages uniformly as ``OperationalError``. + + The asyncpg dialect leaks two outage shapes that bypass SQLAlchemy's usual + error translation, so the app-level ``OperationalError`` handler + (``server/core/errors.py``) would never see them: + + - connect-time network failures (DB down/unreachable/DNS) escape as raw + ``OSError`` subclasses (``ConnectionRefusedError``, ``socket.gaierror``); + - a connection dying mid-query surfaces as a bare ``DBAPIError`` with + ``connection_invalidated=True`` instead of ``OperationalError``. + + Both are re-wrapped here so callers see one exception type for "the + database is temporarily unavailable". Auth/config mistakes (bad password, + unknown database) and genuine SQL bugs take other exception types and are + deliberately left alone. + """ + + @event.listens_for(engine.sync_engine, "do_connect") + def _wrap_connect_oserror(dialect, conn_rec, cargs, cparams): + try: + return dialect.connect(*cargs, **cparams) + except OSError as exc_: # ConnectionRefusedError, gaierror, timeout… + raise exc.OperationalError( + "database connection failed", None, exc_ + ) from exc_ + + @event.listens_for(engine.sync_engine, "handle_error") + def _wrap_disconnect(context): + if context.is_disconnect and not isinstance( + context.sqlalchemy_exception, exc.OperationalError + ): + return exc.OperationalError( + "database connection lost", None, context.original_exception + ) + + def get_engine() -> AsyncEngine: """Get or create the async engine (lazy initialization).""" global _engine @@ -24,6 +62,7 @@ def get_engine() -> AsyncEngine: pool_timeout=30, pool_recycle=300, ) + _install_outage_normalization(_engine) return _engine diff --git a/tests/test_db_outage_error_handling.py b/tests/test_db_outage_error_handling.py new file mode 100644 index 0000000..f548407 --- /dev/null +++ b/tests/test_db_outage_error_handling.py @@ -0,0 +1,118 @@ +"""Regression test for issue #354. + +A database outage (e.g. Postgres unreachable) raises `sqlalchemy.exc.OperationalError` +from inside a route handler. `register_exception_handlers`'s catch-all `Exception` +handler caught it and returned 500/`internal_error`, indistinguishable from a genuine +endpoint bug and not a signal most retry policies back off on. `/readyz` already +reports 503 for the identical condition (`ReadinessResult.http_status`), so the two +surfaces disagreed about what a DB outage means. +""" + +from __future__ import annotations + +import pytest +from httpx import ASGITransport, AsyncClient +from sqlalchemy.exc import OperationalError + +from server.app import create_app +from server.db.engine import dispose_engine, get_engine, get_session + +pytestmark = pytest.mark.asyncio + + +async def _raise_operational_error(): + raise OperationalError("SELECT 1", {}, Exception("connection refused")) + yield # pragma: no cover - unreachable, keeps this an async generator + + +async def test_db_outage_returns_503_not_500(): + app = create_app() + app.dependency_overrides[get_session] = _raise_operational_error + + # No raise_app_exceptions=False here: the OperationalError handler lives in + # ExceptionMiddleware, which sends its response WITHOUT re-raising — so this + # also pins that a DB outage produces no propagated exception at all. + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://test") as ac: + resp = await ac.get("/v1/subjects/does-not-matter/health") + + assert resp.status_code == 503 + body = resp.json() + assert body["error"]["code"] == "service_unavailable" + assert "Retry-After" in resp.headers + + +async def test_non_db_exceptions_still_return_500(): + """The catch-all handler must still cover unrelated bugs: only + `OperationalError` gets the 503 treatment.""" + app = create_app() + + async def _raise_value_error(): + raise ValueError("not a database problem") + yield # pragma: no cover + + app.dependency_overrides[get_session] = _raise_value_error + + # The Exception catch-all lives in ServerErrorMiddleware, which sends its + # 500 response and then re-raises the original exception (so real servers + # log it); raise_app_exceptions=False lets the test client see the response + # instead of the propagated ValueError. + transport = ASGITransport(app=app, raise_app_exceptions=False) + async with AsyncClient(transport=transport, base_url="http://test") as ac: + resp = await ac.get("/v1/subjects/does-not-matter/health") + + assert resp.status_code == 500 + assert resp.json()["error"]["code"] == "internal_error" + + +# --------------------------------------------------------------------------- +# The asyncpg dialect does not raise OperationalError for real outages on its +# own: connect-time failures escape as raw OSError subclasses and a mid-query +# disconnect surfaces as a bare DBAPIError. server/db/engine.py normalizes +# both; these tests pin that normalization end to end (issue #354's actual +# repro is the app-level test below — no dependency override, a real engine +# pointed at a dead port). +# --------------------------------------------------------------------------- + + +async def test_engine_wraps_connect_failure_as_operational_error(monkeypatch): + from sqlalchemy import text + from sqlalchemy.exc import OperationalError + + from server.core.config import settings + + monkeypatch.setattr( + settings, "database_url", "postgresql+asyncpg://u:p@127.0.0.1:1/statewave" + ) + await dispose_engine() + try: + engine = get_engine() + with pytest.raises(OperationalError) as excinfo: + async with engine.connect() as conn: + await conn.execute(text("SELECT 1")) + assert isinstance(excinfo.value.orig, OSError) + finally: + await dispose_engine() + + +async def test_db_outage_returns_503_without_dependency_override(monkeypatch): + """Issue #354's reproduction: the database is unreachable, a normal request + comes in, and the real get_session/engine path (no override) must yield the + 503 payload — this fails if OperationalError never surfaces from the engine.""" + from server.core.config import settings + + monkeypatch.setattr( + settings, "database_url", "postgresql+asyncpg://u:p@127.0.0.1:1/statewave" + ) + await dispose_engine() + try: + app = create_app() + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://test") as ac: + resp = await ac.get("/v1/subjects/does-not-matter/health") + + assert resp.status_code == 503 + assert resp.json()["error"]["code"] == "service_unavailable" + assert "Retry-After" in resp.headers + finally: + await dispose_engine()