fix(api): keep every 500 on the ADR 0031 envelope; narrow the fail-closed handler (#101, #102)

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)

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"