Skip to content

Conversation

loocor
Copy link
Contributor

@loocor loocor commented Sep 21, 2025

Gracefully handle SSE control frames; refresh endpoint on reconnect; add hooks; reduce noisy logs

  • Gracefully consume non-JSON SSE control frames (e.g., event:endpoint)
  • Auto-refresh POST message endpoint on reconnect (handles new sessionId)
  • Add reconnect/error hooks and lower noisy JSON decode warnings

Resolves repeated warnings like:

sse stream error: body error: error decoding response body
failed to deserialize server message

and reconnection issues with servers that emit endpoint control frames (e.g., git-mcp). Improves compatibility with mixed/extended SSE implementations while preserving strict MCP parsing for actual messages.

Motivation and Context

Certain MCP servers resend control information over SSE when a stream reconnects. Typical examples include event: endpoint frames whose data carries the message POST endpoint (often with a new sessionId). Previously, the client-side SSE path attempted to parse every SSE data as a JSON-RPC message, which produced repeated warnings and interfered with reconnection and message submission.

In particular, GitMCP (idosal/git-mcp) emits event:endpoint with a refreshed endpoint that can include a sessionId. This triggered repeated decode warnings and unstable reconnection in downstream projects (e.g., MCPMate) and motivated this compatibility fix.

This change clearly separates JSON message frames from control frames: control frames are consumed by a reconnect hook and never fed into the JSON parser; valid JSON message frames continue to be parsed strictly per MCP.

How Has This Been Tested?

  • Unit: control frames are skipped; the next JSON message is parsed as expected
  • Unit: event:endpoint updates the shared POST endpoint URI on reconnect
  • Backward compatibility: behavior for spec-compliant servers is unchanged
  • Logging: JSON decode failures now at debug with last_event_id context
  • Downstream integration: to be verified in the referencing backend per project plan

Breaking Changes

None. Changes are internal. Hooks are pub(crate) and do not alter public APIs. Default behavior for compliant servers remains identical.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP documentation and ensured strict parsing for MCP messages
  • My code follows the repository's style guidelines; comments switched to English
  • Appropriate error handling and contextual logging were added
  • Documentation comments and usage notes updated where relevant
  • All tests pass locally (will run full matrix in downstream project)

Additional context

Implementation Details

  • Control-frame split and hook invocation:
    • crates/rmcp/src/transport/common/client_side_sse.rs:214–223 — non-""|"message" SSE events are treated as control frames and passed to handle_control_event(&Sse); only ""|"message" are parsed as JSON.
  • Decode noise reduction:
    • crates/rmcp/src/transport/common/client_side_sse.rs:228–239 — JSON decode failures downgraded to debug and include last_event_id for troubleshooting.
  • Hook definitions and error-logging ergonomics:
    • crates/rmcp/src/transport/common/client_side_sse.rs:101–124SseStreamReconnect provides handle_control_event (no-op by default) and handle_stream_error.
    • handle_stream_error now accepts &(dyn std::error::Error + 'static) so call sites can forward underlying sse_stream::Error directly, avoiding generic constraints and fixing the E0308 type mismatch observed in integration builds.
  • Reconnect helper (private):
    • crates/rmcp/src/transport/sse_client.rs:63–105SseClientReconnect<C> implements the hooks: consumes event:endpoint, resolves the new message endpoint, updates a shared Arc<RwLock<Uri)>, and logs stream errors with uri and last_event_id context. The override of handle_stream_error matches the new signature (&(dyn Error)), see :98–104.
  • Endpoint resolution rules:
    • Absolute http(s):// data → used as-is.
    • ?query only → keep base path, append query.
    • Relative/absolute path (optional query) → replace path_and_query in base.
    • Implementation: message_endpoint(base, endpoint) in crates/rmcp/src/transport/sse_client.rs:300–308.
  • Compile fixes:
    • Addressed E0382 by borrowing sse.id via if let Some(ref event_id) before cloning to last_event_id (crates/rmcp/src/transport/common/client_side_sse.rs:211–213).
    • Addressed E0308 by changing the hook signature to take &(dyn Error) and forwarding the underlying SSE error at the call site (:251).
  • Documentation hygiene:
    • Switched non-English comments to English and normalized //! module docs.

Design Decisions

  • Strict for MCP messages, lenient for control frames: Only frames with event == "" | "message" go through JSON parsing; others are consumed by a hook.
  • Hooks over hard-coded behavior: handle_control_event and handle_stream_error enable future extensions (e.g., ping, other control signals) without API changes; default implementations remain backward compatible.
  • Minimal surface area: Internals only; no public API additions or renames.

@github-actions github-actions bot added T-core Core library changes T-transport Transport layer changes labels Sep 21, 2025
…d reconnect hooks; fmt + doctest/clippy fixes
@loocor loocor marked this pull request as draft September 21, 2025 08:00
@loocor loocor closed this Sep 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-core Core library changes T-transport Transport layer changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant