Handle remote SSE servers closing POST connections early

[aponcedeleonch]

Closes: #255

• Add tolerant HTTP transport that ignores httpx.RemoteProtocolError on POST responses
• Some SSE servers (behind proxies/CDNs) close the POST response connection before the body is sent
• The actual MCP response arrives via the SSE stream, so the POST response body is not needed
• Increase default MCP timeout from 10 to 20 seconds
• Add tests for the tolerant client behavior

[claude]

PR Review: Handle remote SSE servers closing POST connections early

✅ Strengths

1. Well-targeted solution: Addresses a specific real-world issue with SSE servers behind proxies/CDNs
2. Good test coverage: Tests verify both error propagation and tolerant client usage
3. Clear documentation: Comments explain the rationale for the workaround
4. Timeout increase justified: Doubling from 10→20s is reasonable for remote servers

🔍 Issues & Suggestions

Critical: Resource Leak Risk (src/mcp_optimizer/mcp_client.py:31-33)

def __init__(self, original_stream: httpx.AsyncByteStream | httpx.SyncByteStream):
    # In async context, this will always be AsyncByteStream
    self._original: httpx.AsyncByteStream = original_stream

The type annotation forces AsyncByteStream but accepts SyncByteStream in signature. This could cause type errors at runtime. Either:

• Remove | httpx.SyncByteStream from parameter type, OR
• Use union type for self._original with proper type guards

Silent Error Swallowing (src/mcp_optimizer/mcp_client.py:35-42)

async def __aiter__(self):
    try:
        async for chunk in self._original:
            yield chunk
    except httpx.RemoteProtocolError:
        # Server closed connection before body was sent
        pass

Only RemoteProtocolError is caught. Consider:

• Adding logging when error is swallowed (at DEBUG level) for troubleshooting
• Document whether other connection errors should also be tolerated

Missing Test Coverage

The _TolerantStream class has no direct unit tests. Suggest adding:

async def test_tolerant_stream_swallows_remote_protocol_error():
    # Test that RemoteProtocolError is caught and ignored
    
async def test_tolerant_stream_propagates_other_errors():
    # Test that other errors are not swallowed

Minor: Redundant Helper Method (src/mcp_optimizer/mcp_client.py:332-354)

_execute_sse_session_with_client is only called from one place with the factory already known. Consider inlining into _execute_sse_session to reduce indirection:

async def _execute_sse_session(self, operation, url):
    sse_kwargs = {"url": url, "httpx_client_factory": _create_tolerant_httpx_client}
    async with sse_client(**sse_kwargs) as (read_stream, write_stream):
        # ... rest of implementation

This removes ~20 lines while maintaining clarity.

Test Brittleness (tests/test_mcp_client.py:326-334)

The test update uses conditional logic based on client_mock_name:

if client_mock_name == "sse_client":
    mock_client.assert_called_once()
    assert mock_client.call_args.kwargs["url"] == url

This makes the test harder to understand. Consider splitting into separate parametrized test cases for SSE vs non-SSE.

📊 Summary

The PR solves a legitimate problem with a reasonable workaround. Main concerns:

1. Fix type annotation mismatch (critical)
2. Add logging for swallowed errors (recommended)
3. Add unit tests for _TolerantStream (recommended)
4. Consider simplifying by inlining the helper method

Overall: Good work, but needs the type safety issue addressed before merge.

🤖 Generated with Claude Code

[aponcedeleonch]

@claude check if your feedback was addressed

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Feedback Review Complete ✅

All previous review feedback has been addressed:

✅ Critical: Type Annotation Issue - FIXED

Previous issue: Type mismatch in _TolerantStream.__init__ accepting both AsyncByteStream | SyncByteStream but forcing AsyncByteStream annotation.

Resolution: Fixed in src/mcp_optimizer/mcp_client.py:31-32

• Removed | httpx.SyncByteStream from parameter type
• Now correctly accepts only AsyncByteStream
• Type guard added in _TolerantTransport.handle_async_request:66-69 with proper error message

✅ Silent Error Swallowing - FIXED

Previous issue: RemoteProtocolError was caught silently without logging.

Resolution: Added debug logging in src/mcp_optimizer/mcp_client.py:41-44

logger.debug(
    "Ignoring RemoteProtocolError on POST response (expected for some SSE servers)",
    error=str(e),
)

✅ Missing Test Coverage - FIXED

Previous issue: No direct unit tests for _TolerantStream class.

Resolution: Added comprehensive tests in tests/test_mcp_client.py:715-793

• test_tolerant_stream_swallows_remote_protocol_error() - verifies RemoteProtocolError is caught
• test_tolerant_stream_propagates_other_errors() - ensures other errors propagate
• test_tolerant_stream_passes_through_chunks() - validates normal operation
• test_tolerant_stream_aclose() - tests proper cleanup

✅ Redundant Helper Method - ADDRESSED

Previous suggestion: Inline _execute_sse_session_with_client to reduce indirection.

Resolution: Method was simplified and inlined directly into _execute_sse_session:335-338

• No extra helper method exists
• Clean, direct implementation with tolerant client factory

✅ Test Brittleness - ADDRESSED

Previous issue: Conditional logic in test made it harder to understand.

Resolution: Test updated at tests/test_mcp_client.py:330-338

• Conditional now checks for SSE-specific requirements (tolerant client factory)
• Clear assertion for httpx_client_factory parameter
• Still uses parametrization but with clearer SSE-specific verification

📊 Additional Improvements

• Timeout increased from 10→20s (src/mcp_optimizer/config.py:150)
• Comprehensive documentation in code comments
• All tests passing with good coverage

Summary

All critical and recommended issues from the previous review have been properly addressed. The PR is ready to merge from a code review perspective.

---

Job Run | Branch: workaround-remote-sse

[therealnb]

This makes the connection more robust, but it might also hide errors.
I think this is probably a good workaround in the name of usability.