Files
Juan DenisandClaude Opus 5 a7d5a0b919 fix(api): a crash the route caught must still reach /monitoring/errors
Plan 76 milestone B, drift/bug row 4. 43 API handlers answered HTTP 500 from
their own `except Exception`, so the global 500 handler never ran — and that
handler was the ONLY caller of error_log_service.record_error(source='backend').
Every one of those crashes was absent from /monitoring/errors, and the page read
empty precisely because someone had "handled" them.

35 of the 43 also returned str(exc) to the caller. Two returned the full
traceback (templates.py, which also print()ed it), and several logged with a
locally-minted uuid4 'ref' that correlated with nothing but a log line.

The recording side of the 500 handler moves to app/error_reporting.py, so it is
importable rather than reachable only by not-catching:

    record_unexpected(exc)   -> logs, rolls the session back, records, returns
                                the request id
    unexpected_response(exc) -> the above plus the documented crash body

The 500 handler now calls the same pair, so there is one recorder rather than
one recorder and 43 routes that quietly opted out of it.

Caller-visible change on those endpoints: the body is now the documented
{'error': 'Internal server error', 'status': 500, 'code': 'internal_error',
'request_id': ...} instead of the exception text. That is the point — an
unexpected exception's text is written for whoever reads the log and routinely
carries connection strings, absolute paths, and SQL. The request id is the part
a caller can act on, and it matches the row in the error log. No frontend code
read the 'ref' or 'trace' keys this drops (checked).

Two sites keep their own wording and call record_unexpected instead: sso.py's
login failures should not read as a generic crash on the login page.
docker.py's bulk endpoint keeps its per-item `results` — the caller's half-done
work survives, only the exception text is dropped.

Ratchet: tests/unreported_crash_census.py + its test, held at **0**, not a
legacy baseline to shrink. It counts blanket handlers that answer 500 without
calling either reporter, and is scoped to app/api — a service catching an
exception and returning a value is making a domain decision, not declaring an
HTTP crash. Verified non-vacuous: an injected handler fails it, and it passes
again on revert.

Measured and deliberately still open: 56 API handlers swallow a crash and
answer 200. A failure reported as success is worse, but converging it is
milestone C's response-envelope work, not this row.

Gate: tests/test_unexpected_error_reporting.py (11, including the two that pin
why the old shape lost the crash), test_unreported_crash_ratchet.py (9), and
the 66 test files covering every touched module: 1105 passed, 7 skipped, 1
pre-existing Windows-only failure (test_fleet_doctor's socket.gaierror).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-18 20:38:20 -04:00

112 lines
4.0 KiB
Python

"""One place that turns an *unexpected* exception into a recorded, correlated 500.
The global ``@app.errorhandler(500)`` logs the crash, records it into the
centralized error log behind ``/monitoring/errors``, and answers with the
documented JSON body carrying a request id. A route that catches ``Exception``
itself never reaches that handler, so its crash is logged nowhere and is
invisible on the errors page — the failure looks handled precisely because
someone handled it.
That is why this module exists rather than only the handler: the recording side
is importable, so a route that must keep answering JSON in-band can still put
its crash on the record.
Use it only for genuinely unexpected failures. An expected, caller-actionable
failure should raise one of ``app.exceptions``' typed errors instead — those
are not crashes and must not fill the error log.
from app.error_reporting import unexpected_response
try:
return jsonify(SomeService.run())
except Exception as exc: # noqa: BLE001
return unexpected_response(exc)
Prefer deleting the ``try``/``except`` entirely where nothing in it does real
cleanup: letting the exception reach the global handler produces the same
response through the same recorder, with one less thing to keep in step.
"""
import traceback as _traceback
from flask import current_app, request
from app.middleware.request_id import get_request_id
def record_unexpected(exc, *, request_id=None):
"""Log `exc` and record it into the centralized error log. Never raises.
Returns the request id the crash was recorded under so a caller can hand it
to the client — an error the user can quote is worth far more than the
exception text this replaces.
"""
request_id = request_id or get_request_id(create=True)
try:
current_app.logger.exception(
'Unhandled exception on %s %s [request_id=%s]',
request.method, request.path, request_id, exc_info=exc,
)
except Exception: # noqa: BLE001 - reporting must never mask the crash
pass
# Roll back BEFORE recording. This shares the app-context-scoped session
# with the code that just crashed, so a DB-caused failure leaves it in a
# failed transaction and record_error's first query would raise
# PendingRollbackError — swallowed by its own contract, which is how
# database-caused crashes silently never reach the error log.
try:
from app import db
db.session.rollback()
except Exception: # noqa: BLE001
pass
try:
from app.services import error_log_service
from app.middleware.rbac import get_current_user
user = None
try:
user = get_current_user()
except Exception: # noqa: BLE001 - anonymous crashes still count
pass
error_log_service.record_error(
source='backend',
exception_type=type(exc).__name__,
message=str(exc),
traceback=''.join(
_traceback.format_exception(type(exc), exc, exc.__traceback__)
),
endpoint=getattr(request, 'path', None),
method=getattr(request, 'method', None),
user_id=user.id if user else None,
context={'request_id': request_id},
)
except Exception: # noqa: BLE001
pass
return request_id
def unexpected_error_body(request_id):
"""The documented crash body — identical to the global 500 handler's."""
return {
'error': 'Internal server error',
'status': 500,
'code': 'internal_error',
'request_id': request_id,
}
def unexpected_response(exc):
"""Record `exc` and return the standard ``(body, 500)`` for a crash.
The body deliberately does NOT carry ``str(exc)``. An unexpected exception's
text is written for whoever reads the log, not for the caller: it routinely
contains connection strings, absolute paths, and SQL. The request id is the
part the caller can act on.
"""
return unexpected_error_body(record_unexpected(exc)), 500