fix(connection): log unhandled RPC handler exceptions#116
Merged
Conversation
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>
michelTho
force-pushed
the
micheltho/log-unhandled-handler-exceptions
branch
from
3d82436 to
1f0b15d
Compare
Contributor
Author
|
Hey @PsiACE Tell me if you need me to change anything, should be pretty harmless though |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Server-side handler crashes in
Connectionwere converted to a wire error (requests) or silently dropped (notifications) without ever being logged, so integrators wiring an error reporter — e.g. Sentry via itsloggingintegration — 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
_run_request— log the original exception (with traceback) before converting to JSON-RPC-32603; it previously didraise err from None, discarding the root-cause traceback._run_notification— replacecontextlib.suppress(Exception)(swallowed every notification error, no log, no response) with atry/exceptthat logs. Drops the now-unusedcontextlibimport.Tests
tests/test_request_error_logging.py— deterministic unit tests drive_run_request/_run_notificationwith a recording sender (asserting the exact wireframe + that the original exception is logged). Verified the suite fails on the pre-fix silent-swallow behaviour.
Notes for review
suppressused to exit beforespan_context, so the span never saw the exception; catching insidetrypreserves that.