🐛 web-api: Fixes handling of unexpected errors (#7939)

Author: pcrespov

packages/service-library/src/servicelib/aiohttp/monitoring.py

@@ -1,6 +1,5 @@
 """Enables monitoring of some quantities needed for diagnostics"""
 
-import asyncio
 import logging
 from collections.abc import Awaitable, Callable
 from time import perf_counter
@@ -61,9 +60,8 @@ async def middleware_handler(request: web.Request, handler: Handler):
         # See https://prometheus.io/docs/concepts/metric_types
 
         log_exception: BaseException | None = None
-        resp: web.StreamResponse = web.HTTPInternalServerError(
-            reason="Unexpected exception"
-        )
+        response: web.StreamResponse = web.HTTPInternalServerError()
+
         canonical_endpoint = request.path
         if request.match_info.route.resource:
             canonical_endpoint = request.match_info.route.resource.canonical
@@ -86,29 +84,25 @@ async def middleware_handler(request: web.Request, handler: Handler):
                 endpoint=canonical_endpoint,
                 user_agent=user_agent,
             ):
-                resp = await handler(request)
+                response = await handler(request)
 
             assert isinstance(  # nosec
-                resp, web.StreamResponse
+                response, web.StreamResponse
             ), "Forgot envelope middleware?"
 
         except web.HTTPServerError as exc:
-            resp = exc
+            response = exc
             log_exception = exc
-            raise resp from exc
+            raise
+
         except web.HTTPException as exc:
-            resp = exc
+            response = exc
             log_exception = None
-            raise resp from exc
-        except asyncio.CancelledError as exc:
-            resp = web.HTTPInternalServerError(text=f"{exc}")
-            log_exception = exc
-            raise resp from exc
+            raise
+
         except Exception as exc:  # pylint: disable=broad-except
-            resp = web.HTTPInternalServerError(text=f"{exc}")
-            resp.__cause__ = exc
             log_exception = exc
-            raise resp from exc
+            raise
 
         finally:
             response_latency_seconds = perf_counter() - start_time
@@ -118,13 +112,13 @@ async def middleware_handler(request: web.Request, handler: Handler):
                 method=request.method,
                 endpoint=canonical_endpoint,
                 user_agent=user_agent,
-                http_status=resp.status,
+                http_status=response.status,
                 response_latency_seconds=response_latency_seconds,
             )
 
             if exit_middleware_cb:
                 with log_catch(logger=log, reraise=False):
-                    await exit_middleware_cb(request, resp)
+                    await exit_middleware_cb(request, response)
 
             if log_exception:
                 log.error(
@@ -135,12 +129,12 @@ async def middleware_handler(request: web.Request, handler: Handler):
                     request.method,
                     request.path,
                     response_latency_seconds,
-                    resp.status,
+                    response.status,
                     exc_info=log_exception,
                     stack_info=True,
                 )
 
-        return resp
+        return response
 
     setattr(  # noqa: B010
         middleware_handler, "__middleware_name__", f"{__name__}.monitor_{app_name}"

packages/service-library/src/servicelib/aiohttp/profiler_middleware.py

@@ -13,7 +13,7 @@ async def profiling_middleware(request: Request, handler):
         try:
             if _profiler.is_running or (_profiler.last_session is not None):
                 raise HTTPInternalServerError(
-                    reason="Profiler is already running. Only a single request can be profiled at any given time.",
+                    text="Profiler is already running. Only a single request can be profiled at any given time.",
                     headers={},
                 )
             _profiler.reset()
@@ -24,7 +24,7 @@ async def profiling_middleware(request: Request, handler):
 
             if response.content_type != MIMETYPE_APPLICATION_JSON:
                 raise HTTPInternalServerError(
-                    reason=f"Profiling middleware is not compatible with {response.content_type=}",
+                    text=f"Profiling middleware is not compatible with {response.content_type=}",
                     headers={},
                 )
 

packages/service-library/src/servicelib/aiohttp/requests_validation.py

@@ -51,7 +51,7 @@ def handle_validation_as_http_error(
             }
             for e in err.errors()
         ]
-        reason_msg = error_msg_template.format(
+        user_error_message = error_msg_template.format(
             failed=", ".join(d["loc"] for d in details)
         )
 
@@ -80,15 +80,14 @@ def handle_validation_as_http_error(
             error_str = json_dumps(
                 {
                     "error": {
-                        "msg": reason_msg,
+                        "msg": user_error_message,
                         "resource": resource_name,  # optional
                         "details": details,  # optional
                     }
                 }
             )
 
         raise web.HTTPUnprocessableEntity(  # 422
-            reason=reason_msg,
             text=error_str,
             content_type=MIMETYPE_APPLICATION_JSON,
         ) from err

packages/service-library/src/servicelib/aiohttp/rest_middlewares.py

@@ -11,23 +11,25 @@
 from aiohttp.web_exceptions import HTTPError
 from aiohttp.web_request import Request
 from aiohttp.web_response import StreamResponse
-from common_library.error_codes import create_error_code
+from common_library.error_codes import ErrorCodeStr, create_error_code
 from common_library.json_serialization import json_dumps, json_loads
 from common_library.user_messages import user_message
+from models_library.basic_types import IDStr
 from models_library.rest_error import ErrorGet, ErrorItemType, LogMessageType
+from servicelib.rest_constants import RESPONSE_MODEL_POLICY
+from servicelib.status_codes_utils import is_5xx_server_error
 
 from ..logging_errors import create_troubleshotting_log_kwargs
 from ..mimetype_constants import MIMETYPE_APPLICATION_JSON
 from ..rest_responses import is_enveloped_from_map, is_enveloped_from_text
-from ..utils import is_production_environ
+from ..status_codes_utils import get_code_description
 from . import status
 from .rest_responses import (
     create_data_response,
     create_http_error,
     safe_status_message,
     wrap_as_envelope,
 )
-from .rest_utils import EnvelopeFactory
 from .typing_extension import Handler, Middleware
 from .web_exceptions_extension import get_http_error_class_or_none
 
@@ -46,34 +48,28 @@ def is_api_request(request: web.Request, api_version: str) -> bool:
     return bool(request.path.startswith(base_path))
 
 
-def _handle_unexpected_exception_as_500(
-    request: web.BaseRequest,
-    exception: Exception,
-    *,
-    skip_internal_error_details: bool,
-) -> web.HTTPInternalServerError:
-    """Process unexpected exceptions and return them as HTTP errors with proper formatting.
+def _create_error_context(
+    request: web.BaseRequest, exception: Exception
+) -> tuple[ErrorCodeStr, dict[str, Any]]:
+    """Create error code and context for logging purposes.
 
-    IMPORTANT: this function cannot throw exceptions, as it is called
+    Returns:
+        Tuple of (error_code, error_context)
     """
     error_code = create_error_code(exception)
     error_context: dict[str, Any] = {
         "request.remote": f"{request.remote}",
         "request.method": f"{request.method}",
         "request.path": f"{request.path}",
     }
+    return error_code, error_context
 
-    user_error_msg = _FMSG_INTERNAL_ERROR_USER_FRIENDLY
-
-    http_error = create_http_error(
-        exception,
-        user_error_msg,
-        web.HTTPInternalServerError,
-        skip_internal_error_details=skip_internal_error_details,
-        error_code=error_code,
-    )
 
-    error_context["http_error"] = http_error
+def _log_5xx_server_error(
+    request: web.BaseRequest, exception: Exception, user_error_msg: str
+) -> ErrorCodeStr:
+    """Log 5XX server errors with error code and context."""
+    error_code, error_context = _create_error_context(request, exception)
 
     _logger.exception(
         **create_troubleshotting_log_kwargs(
@@ -83,33 +79,78 @@ def _handle_unexpected_exception_as_500(
             error_code=error_code,
         )
     )
+    return error_code
+
+
+def _handle_unexpected_exception_as_500(
+    request: web.BaseRequest, exception: Exception
+) -> web.HTTPInternalServerError:
+    """Process unexpected exceptions and return them as HTTP errors with proper formatting.
+
+    IMPORTANT: this function cannot throw exceptions, as it is called
+    """
+    error_code, error_context = _create_error_context(request, exception)
+    user_error_msg = _FMSG_INTERNAL_ERROR_USER_FRIENDLY
+
+    error_context["http_error"] = http_error = create_http_error(
+        exception,
+        user_error_msg,
+        web.HTTPInternalServerError,
+        error_code=error_code,
+    )
+
+    _log_5xx_server_error(request, exception, user_error_msg)
+
     return http_error
 
 
 def _handle_http_error(
     request: web.BaseRequest, exception: web.HTTPError
 ) -> web.HTTPError:
-    """Handle standard HTTP errors by ensuring they're properly formatted."""
+    """Handle standard HTTP errors by ensuring they're properly formatted.
+
+    NOTE: this needs further refactoring to avoid code duplication
+    """
     assert request  # nosec
+    assert not exception.empty_body, "HTTPError should not have an empty body"  # nosec
+
     exception.content_type = MIMETYPE_APPLICATION_JSON
     if exception.reason:
         exception.set_status(
             exception.status, safe_status_message(message=exception.reason)
         )
 
     if not exception.text or not is_enveloped_from_text(exception.text):
-        error_message = exception.text or exception.reason or "Unexpected error"
+        # NOTE: aiohttp.HTTPException creates `text = f"{self.status}: {self.reason}"`
+        user_error_msg = exception.text or "Unexpected error"
+
+        error_code: IDStr | None = None
+        if is_5xx_server_error(exception.status):
+            error_code = IDStr(
+                _log_5xx_server_error(request, exception, user_error_msg)
+            )
+
         error_model = ErrorGet(
             errors=[
-                ErrorItemType.from_error(exception),
+                ErrorItemType(
+                    code=exception.__class__.__name__,
+                    message=user_error_msg,
+                    resource=None,
+                    field=None,
+                ),
             ],
             status=exception.status,
             logs=[
-                LogMessageType(message=error_message, level="ERROR"),
+                LogMessageType(message=user_error_msg, level="ERROR"),
             ],
-            message=error_message,
+            message=user_error_msg,
+            support_id=error_code,
+        )
+        exception.text = json_dumps(
+            wrap_as_envelope(
+                error=error_model.model_dump(mode="json", **RESPONSE_MODEL_POLICY)
+            )
         )
-        exception.text = EnvelopeFactory(error=error_model).as_text()
 
     return exception
 
@@ -137,10 +178,8 @@ def _handle_http_successful(
 
 def _handle_exception_as_http_error(
     request: web.Request,
-    exception: Exception,
+    exception: NotImplementedError | TimeoutError,
     status_code: int,
-    *,
-    skip_internal_error_details: bool,
 ) -> HTTPError:
     """
     Generic handler for exceptions that map to specific HTTP status codes.
@@ -155,16 +194,15 @@ def _handle_exception_as_http_error(
         )
         raise ValueError(msg)
 
-    return create_http_error(
-        exception,
-        f"{exception}",
-        http_error_cls,
-        skip_internal_error_details=skip_internal_error_details,
-    )
+    user_error_msg = get_code_description(status_code)
+
+    if is_5xx_server_error(status_code):
+        _log_5xx_server_error(request, exception, user_error_msg)
+
+    return create_http_error(exception, user_error_msg, http_error_cls)
 
 
 def error_middleware_factory(api_version: str) -> Middleware:
-    _is_prod: bool = is_production_environ()
 
     @web.middleware
     async def _middleware_handler(request: web.Request, handler: Handler):
@@ -189,27 +227,19 @@ async def _middleware_handler(request: web.Request, handler: Handler):
 
             except NotImplementedError as exc:
                 result = _handle_exception_as_http_error(
-                    request,
-                    exc,
-                    status.HTTP_501_NOT_IMPLEMENTED,
-                    skip_internal_error_details=_is_prod,
+                    request, exc, status.HTTP_501_NOT_IMPLEMENTED
                 )
 
             except TimeoutError as exc:
                 result = _handle_exception_as_http_error(
-                    request,
-                    exc,
-                    status.HTTP_504_GATEWAY_TIMEOUT,
-                    skip_internal_error_details=_is_prod,
+                    request, exc, status.HTTP_504_GATEWAY_TIMEOUT
                 )
 
         except Exception as exc:  # pylint: disable=broad-except
             #
             # Last resort for unexpected exceptions (including those raise by the exception handlers!)
             #
-            result = _handle_unexpected_exception_as_500(
-                request, exc, skip_internal_error_details=_is_prod
-            )
+            result = _handle_unexpected_exception_as_500(request, exc)
 
         return result
 
@@ -230,7 +260,6 @@ def envelope_middleware_factory(
     api_version: str,
 ) -> Callable[..., Awaitable[StreamResponse]]:
     # FIXME: This data conversion is very error-prone. Use decorators instead!
-    _is_prod: bool = is_production_environ()
 
     @web.middleware
     async def _middleware_handler(

packages/service-library/src/servicelib/aiohttp/rest_responses.py

@@ -80,7 +80,6 @@ def create_http_error(
     ] = web.HTTPInternalServerError,  # type: ignore[assignment]
     *,
     status_reason: str | None = None,
-    skip_internal_error_details: bool = False,
     error_code: ErrorCodeStr | None = None,
 ) -> T_HTTPError:
     """
@@ -99,7 +98,7 @@ def create_http_error(
     # changing the workflows in this function
 
     is_internal_error = bool(http_error_cls == web.HTTPInternalServerError)
-    if is_internal_error and skip_internal_error_details:
+    if is_internal_error:
         error_model = ErrorGet.model_validate(
             {
                 "status": http_error_cls.status_code,

packages/service-library/src/servicelib/logging_errors.py

@@ -39,10 +39,10 @@ def _collect_causes(exc: BaseException) -> str:
     debug_data = json_dumps(
         {
             "exception_type": f"{type(error)}",
-            "exception_details": f"{error}",
+            "exception_string": f"{error}",
+            "exception_causes": _collect_causes(error),
             "error_code": error_code,
             "context": error_context,
-            "causes": _collect_causes(error),
             "tip": tip,
         },
         default=representation_encoder,