| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Connection._run_request catches any unexpected handler exception, converts it to a JSON-RPC -32603 response, and re-raises a fresh RequestError `from None` — discarding the original exception and its traceback. Unlike every other error path in this module (main_loop, the receive loop, stream observers, _on_receive_error, _on_task_error), it is never logged, so server-side handler crashes are invisible: integrators only ever see a contextless "Internal error" on the wire with no stack to diagnose the root cause. Log the original exception before converting it, mirroring the existing logging.exception(..., exc_info=exc) calls in this file. Only the method name and the exception (type + traceback) are logged, not the request params, and str(exc) is already returned to the peer today — so this adds no new data exposure. The protocol response is unchanged. Adds a regression test: a handler that raises returns -32603 and the exception is logged. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
Connection._run_notification wrapped the handler call in contextlib.suppress(Exception), swallowing every notification-handler error silently: no log, and (correctly) no wire response since notifications have none by protocol. It was the only handler-error path in the module that stayed silent — main_loop, the receive loop, stream observers, _on_receive_error, _on_task_error, and (since the previous commit) _run_request all log. Integrators wiring Sentry via the logging integration therefore had to wrap every notification handler (cancel, ext_notification, and on the client side session_update, complete_elicitation) by hand to see failures. Replace the suppress with a try/except that logs the original exception and its traceback, mirroring the logging.exception(..., exc_info=exc) call in _run_request. Behaviour is otherwise unchanged: the exception is still not propagated, and the telemetry span still sees no exception (contextlib.suppress already exited before span_context, so this is not a span-status change). Unlike _run_request we deliberately do not re-raise: notifications have no response and no store bookkeeping, so re-raising would only add a duplicate, contextless "Background task failed" line. Drop the now-unused contextlib import. Adds a regression test: a session/cancel handler that raises is logged (with traceback) instead of silently swallowed. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
Rewrites the request/notification error-logging tests to be deterministic
and precise instead of only asserting that "some record has exc_info":
- Drive Connection._run_request / _run_notification directly with a
recording sender (mirroring test_core's TrackingSender) so assertions
cover the exact wire frame: a request emits one -32603 error carrying
{"details": ...} and re-raises RequestError; a notification emits
nothing (deterministic, no timeout read).
- Assert the logged record is the original RuntimeError (type + message)
under the expected method=..., not merely that a record exists.
Confirmed both tests fail on the pre-fix behaviour (silent suppress / no
request log) and pass with it.
Generated by Mistral Vibe.
Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
|
Hey @PsiACE Tell me if you need me to change anything, should be pretty harmless though |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
What & why
Server-side handler crashes in Connection were converted to a wire error (requests) or silently dropped (notifications) without ever being logged, so integrators wiring an error reporter — e.g. Sentry via its logging integration — couldn't see them, forcing them to wrap every handler by hand. This makes both RPC handler paths log unhandled exceptions with their original traceback, consistent with every other error path in the module.
Changes
Tests
tests/test_request_error_logging.py — deterministic unit tests drive
_run_request/_run_notification with a recording sender (asserting the exact wire
frame + that the original exception is logged). Verified the suite fails on the pre-fix silent-swallow behaviour.
Notes for review