From f40d5267dd4ec23a86886a2f92668f91fc6148ca Mon Sep 17 00:00:00 2001 From: Arik Levinsky Date: Mon, 20 Jul 2026 11:52:09 -0600 Subject: [PATCH] fix(api): keep every 500 on the ADR 0031 envelope; narrow the fail-closed handler (#101, #102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #101 (two gaps in the error envelope): - An unmapped TollgateError fell to the 500 fallback but still echoed str(exc), so an internal invariant string (e.g. "idempotency replay is missing its stored response") surfaced in the body. Unmapped errors now use the generic fallback message; mapped errors keep their controlled domain message. - A non-TollgateError/non-datastore exception (a genuine bug) was handled by Starlette's default middleware as plain-text "Internal Server Error", off the documented envelope. A catch-all Exception handler now returns the enveloped generic 500 and logs the traceback so the defect is not lost. #102: the fail-closed handler was registered for bare OSError, a very broad base — any incidental non-datastore OSError/TimeoutError on the request path became a retryable 503 enforcement_unavailable, telling the SDK the datastore is down and masking a real defect. Narrowed to ConnectionError + the builtin TimeoutError plus the SQLAlchemy connectivity types (OperationalError, InterfaceError, TimeoutError); driver-wrapped connect/DNS failures still fail closed, while a bare OSError now surfaces as a 500 via the catch-all. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BjJ4QScUKbrMzF1jujL5Bg --- src/tollgate/api/errors.py | 57 ++++++++++++++++++++++++++--------- tests/unit/test_api_errors.py | 30 ++++++++++++++++++ 2 files changed, 73 insertions(+), 14 deletions(-) diff --git a/src/tollgate/api/errors.py b/src/tollgate/api/errors.py index 804f145..08e3e97 100644 --- a/src/tollgate/api/errors.py +++ b/src/tollgate/api/errors.py @@ -9,6 +9,7 @@ from __future__ import annotations +import logging from typing import Final from fastapi import FastAPI, Request @@ -32,18 +33,22 @@ UnknownModel, ) +logger = logging.getLogger(__name__) + #: Datastore connectivity/timeout failures that mean "no enforcement decision was made". #: They are translated to :class:`EnforcementUnavailable` so a client SDK sees the documented -#: fail-closed 503 envelope (ADR 0031) instead of an off-contract 500. On the request path the -#: only I/O is the datastore, so a raw ``OSError`` (connection refused/reset, host unreachable, -#: DNS failure, and the ``ETIMEDOUT`` builtin ``TimeoutError``, which subclasses ``OSError``) -#: is a connect-time outage. ``OperationalError``/``InterfaceError`` cover failures on an -#: established connection (server-side statement timeout, dropped socket); the SQLAlchemy -#: ``TimeoutError`` covers pool-checkout exhaustion. Statement/constraint errors -#: (``IntegrityError``, ``ProgrammingError``, ...) are deliberately excluded: those are bugs, -#: not outages, and must keep failing loudly as 500s. +#: fail-closed 503 envelope (ADR 0031) instead of an off-contract 500. A connect-time outage +#: surfaces as ``ConnectionError`` (refused/reset/aborted) or the builtin ``TimeoutError`` +#: (``ETIMEDOUT``); ``OperationalError``/``InterfaceError`` cover failures on an established or +#: attempted connection through the driver (server-side statement timeout, dropped socket, and the +#: connect/DNS failures SQLAlchemy wraps); the SQLAlchemy ``TimeoutError`` covers pool-checkout +#: exhaustion. A *bare* ``OSError`` is deliberately excluded (#102): it is too broad — an incidental +#: filesystem/socket error or a plain bug would otherwise be mislabeled a retryable datastore +#: outage and invite fail-closed-with-grace, masking a defect that must surface loudly as a 500. +#: Statement/constraint errors (``IntegrityError``, ``ProgrammingError``) are likewise excluded. _UNAVAILABLE_ERRORS: Final = ( - OSError, + ConnectionError, + TimeoutError, OperationalError, InterfaceError, SQLAlchemyTimeoutError, @@ -74,18 +79,33 @@ } +def _envelope( + status: int, code: str, message: str, *, headers: dict[str, str] | None = None +) -> JSONResponse: + body = ErrorEnvelope(error=ErrorBody(code=code, message=message)) + return JSONResponse(status_code=status, content=body.model_dump(), headers=headers) + + def _error_response(exc: TollgateError) -> JSONResponse: - status, code, default_message = _MAPPING.get(type(exc), _FALLBACK) - envelope = ErrorEnvelope(error=ErrorBody(code=code, message=str(exc) or default_message)) + mapped = _MAPPING.get(type(exc)) + if mapped is None: + # An unmapped TollgateError is an internal invariant breach, not a client-facing outcome; + # surface the generic 500 message so an internal invariant string (e.g. "idempotency replay + # is missing its stored response") never leaks into the response body (#101). + status, code, message = _FALLBACK + else: + status, code, default_message = mapped + message = str(exc) or default_message headers = {"WWW-Authenticate": "Bearer"} if status == 401 else None - return JSONResponse(status_code=status, content=envelope.model_dump(), headers=headers) + return _envelope(status, code, message, headers=headers) def register_error_handlers(app: FastAPI) -> None: - """Install the domain-error and fail-closed datastore handlers on ``app``. + """Install the domain-error, fail-closed datastore, and catch-all handlers on ``app``. The ``TollgateError`` handler covers every domain subtype; the connectivity handler - translates a datastore outage into the 503 ``EnforcementUnavailable`` envelope (#62). + translates a datastore outage into the 503 ``EnforcementUnavailable`` envelope (#62); the + catch-all keeps every unexpected exception on the ADR 0031 envelope (#101). """ async def handle(_: Request, exc: Exception) -> JSONResponse: @@ -98,6 +118,15 @@ async def handle_unavailable(_: Request, exc: Exception) -> JSONResponse: # the driver's message (it can carry the DSN); use the envelope's default text. return _error_response(EnforcementUnavailable()) + async def handle_unexpected(_: Request, exc: Exception) -> JSONResponse: + # A genuine bug (any exception not otherwise mapped) still returns the ADR 0031 envelope + # rather than Starlette's plain-text "Internal Server Error"; the generic message never + # leaks internals, and the traceback is logged so the defect is not lost (#101). + logger.exception("unhandled exception on the request path") + status, code, message = _FALLBACK + return _envelope(status, code, message) + app.add_exception_handler(TollgateError, handle) for exc_type in _UNAVAILABLE_ERRORS: app.add_exception_handler(exc_type, handle_unavailable) + app.add_exception_handler(Exception, handle_unexpected) diff --git a/tests/unit/test_api_errors.py b/tests/unit/test_api_errors.py index e923de3..31b236a 100644 --- a/tests/unit/test_api_errors.py +++ b/tests/unit/test_api_errors.py @@ -105,3 +105,33 @@ class _Novel(TollgateError): response = _client_raising(_Novel()).get("/boom") assert response.status_code == 500 assert response.json()["error"]["code"] == "internal_error" + + +def test_unmapped_error_does_not_echo_its_internal_message() -> None: + # An unmapped TollgateError is an internal invariant breach; the 500 body must carry the + # generic message, never the raw internal string (#101). + leak = "idempotency replay is missing its stored response" + response = _client_raising(TollgateError(leak)).get("/boom") + assert response.status_code == 500 + body = response.json() + assert body["error"]["code"] == "internal_error" + assert body["error"]["message"] == "internal error" + assert leak not in body["error"]["message"] + + +def test_unexpected_exception_is_enveloped_not_plain_text() -> None: + # A non-Tollgate, non-datastore exception (a genuine bug) must still return the ADR 0031 + # envelope, not Starlette's plain-text "Internal Server Error" (#101). + response = _client_raising(ValueError("some internal detail")).get("/boom") + assert response.status_code == 500 + body = response.json() + assert body["error"]["code"] == "internal_error" + assert body["error"]["message"] == "internal error" + + +def test_incidental_oserror_is_not_a_datastore_outage() -> None: + # A bare OSError that is not a connection/timeout is not a datastore outage: it must surface as + # a 500 (a bug to fix), not a retryable 503 that tells the SDK to fail-closed-with-grace (#102). + response = _client_raising(OSError("some incidental os error")).get("/boom") + assert response.status_code == 500 + assert response.json()["error"]["code"] == "internal_error"