Fix no way to detect connection closure in non-NO_ERROR cases

[manx98]

Description

When attempting to use SetConnectionLostHandler to handle connection closures, I found that it only handles the NO_ERROR case, which greatly limits its applicability.

However, in situations such as SSE server restarts or connection timeouts, the error usually does not include NO_ERROR. Once the connection is closed, the SSE server destroys the Session ID, causing subsequent requests to fail with an "Invalid Session ID" error.

This means there is no way to actively listen for connection closures caused by errors other than NO_ERROR.

Additionally, the name SetConnectionLostHandler is misleading, as it may give users the false impression that it handles all connection closure cases. I believe that determining the specific error type should be left to the implementer rather than handled by the framework. This would improve the flexibility of error handling.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• MCP spec compatibility implementation
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code refactoring (no functional changes)
• Performance improvement
• Tests only (no functional changes)
• Other (please describe):

Checklist

• My code follows the code style of this project
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• I have updated the documentation accordingly

MCP Spec Compliance

• This PR implements a feature defined in the MCP specification
• Link to relevant spec section: Link text
• Implementation follows the specification exactly

Additional Information

Summary by CodeRabbit

• Bug Fixes

  • Enhanced error handling for Server-Sent Events (SSE) connections—connection loss handlers are now consistently invoked when disconnections occur, improving reliability.

• Documentation

  • Simplified and clarified SSE disconnection handling documentation.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR simplifies SSE (Server-Sent Events) error handling across documentation, implementation, and tests. It removes HTTP/2 NO_ERROR-specific handling from the SSE reader, unifies error paths through a common onConnectionLost handler callback, and updates corresponding test cases and terminology accordingly.

Changes

Cohort / File(s)	Summary
Documentation Updates README.md	Simplified prose describing SSE disconnection handling; removed explicit reference to HTTP/2 idle timeout (NO_ERROR) in favor of generic "disconnections" terminology.
Error Handling Refactor client/transport/sse.go	Removed special-case handling for HTTP/2 NO_ERROR-induced EOF. Now always invokes onConnectionLost handler if present; otherwise logs EOF errors and returns. Unified error termination path.
Test Updates client/transport/sse_test.go	Removed NO_ERROR-specific test cases and blocks. Renamed tests to reflect generic error handling (e.g., WithoutConnectionLostHandler, WithConnectionLost). Updated mocked error scenarios and assertions to align with new behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• fix(client): potential risk of sending on closed channel #194: Modifies SSE reading/control flow around readSSE invocation and responseChan closure, directly intersecting with this PR's callback and termination logic.
• Fix SSE transport not properly handling HTTP/2 NO_ERROR disconnections #509: Reverses or removes NO_ERROR-specific connection-loss handling that was previously added, touching the same readSSE and onConnectionLost callback logic.
• replace bufio.Scanner with bufio.Reader and add error handling #65: Replaces Scanner with Reader and restructures SSE error handling in readSSE; this PR further consolidates error paths through the callback handler.

Suggested reviewers

• caarlos0
• rwjblue-glean

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix no way to detect connection closure in non-NO_ERROR cases' accurately summarizes the main change: removing special-case handling for NO_ERROR and enabling connection loss detection for all error types.
Description check	✅ Passed	The PR description provides a clear problem statement, identifies the limitation of SetConnectionLostHandler for non-NO_ERROR cases, explains the impact, and documents the reasoning. All required template sections are completed with appropriate checkmarks and substantive content.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}