fix: handle SSE errors occurred after stream started#894
fix: handle SSE errors occurred after stream started#894
Conversation
Currently it'd close the connection.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of streaming connections by introducing a comprehensive error handling mechanism for Server-Sent Events (SSE). Previously, errors occurring after a stream had started would lead to an abrupt connection termination. Now, both client and server components are updated to gracefully catch these mid-stream exceptions, serialize them into standardized SSE error events, and allow clients to process these errors without losing the entire connection. This ensures a more resilient and predictable streaming experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/client/transports/http_helpers.py | 94.64% | 93.55% | 🔴 -1.09% |
| src/a2a/client/transports/jsonrpc.py | 86.71% | 86.00% | 🔴 -0.71% |
| src/a2a/client/transports/rest.py | 91.52% | 88.76% | 🔴 -2.75% |
| src/a2a/server/apps/rest/rest_adapter.py | 83.53% | 83.33% | 🔴 -0.20% |
| src/a2a/server/routes/jsonrpc_dispatcher.py | 85.78% | 84.23% | 🔴 -1.55% |
| src/a2a/utils/error_handlers.py | 95.65% | 96.05% | 🟢 +0.40% |
| Total | 91.43% | 91.29% | 🔴 -0.14% |
Generated by coverage-comment.yml
There was a problem hiding this comment.
Code Review
This pull request significantly enhances error handling for streaming HTTP requests, specifically for Server-Sent Events (SSE) across both REST and JSON-RPC transports. The changes introduce a robust mechanism to catch exceptions that occur mid-stream and emit them as structured error events (SSE 'event: error') to the client, preventing stream crashes. This involved modifying the send_http_stream_request function to accept a dedicated SSE error handler, and implementing _handle_sse_error methods in both JsonRpcTransport and RestTransport to parse these incoming error events. On the server side, rest_adapter.py and jsonrpc_dispatcher.py were updated to catch exceptions during stream generation and format them into appropriate SSE error payloads. A new utility function, build_rest_error_payload, was added to standardize REST error responses. Comprehensive integration and unit tests were also added or updated to validate this new error handling behavior. A critical issue was identified where the JsonRpcTransport's _send_stream_request method passes None for a status_error_handler argument that was made mandatory in this PR, which will cause a TypeError at runtime.
| None, | ||
| self._handle_sse_error, |
There was a problem hiding this comment.
The call to send_http_stream_request passes None for the status_error_handler argument. However, the signature for send_http_stream_request in src/a2a/client/transports/http_helpers.py was changed in this PR to make this argument mandatory, no longer allowing None. This will result in a TypeError at runtime.
To fix this, you should provide a valid error handler. RestTransport defines a _handle_http_error method for this purpose. A similar method should be added to JsonRpcTransport and passed here instead of None.
| None, | |
| self._handle_sse_error, | |
| self._handle_http_error, | |
| self._handle_sse_error, |
The spec doesn't defined this behavior: a2aproject/A2A#1262, but currently it'd close the connection.