Fix HTTP transport timeout defaulting to 5 seconds#2848
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b940dc028
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| transport=StreamableHttpTransport(streamable_http_server) | ||
| ) as client: | ||
| # This should NOT timeout since we fixed the default timeout | ||
| result = await client.call_tool("slow_tool", {"duration": 6}) |
There was a problem hiding this comment.
Keep test runtime under pytest 5s timeout
This test forces a 6-second tool run (duration=6), but pytest-timeout is globally set to 5 seconds in pyproject.toml ([tool.pytest.ini_options].timeout = 5). As written, the test will be terminated by pytest-timeout before the tool completes, so CI will fail even when the HTTP fix works. Use a shorter duration or add a per-test timeout marker to keep it within the configured limit.
Useful? React with 👍 / 👎.
WalkthroughThis PR updates Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/client/transports.py (1)
287-293: Consider preserving MCP's granular timeout defaults when user providesread_timeout_seconds.Using
httpx.Timeout(read_timeout_seconds.total_seconds())sets all timeout components (connect, read, write, pool) to the same value. This means a user settingread_timeout_seconds=timedelta(minutes=5)would also get a 5-minute connect timeout, losing the MCP SDK's sensible 30-second connect default.If preserving granular control is desired:
💡 Suggested refinement
- timeout = httpx.Timeout(read_timeout_seconds.total_seconds()) + timeout = httpx.Timeout( + connect=30.0, # Preserve MCP default connect timeout + read=read_timeout_seconds.total_seconds(), + write=read_timeout_seconds.total_seconds(), + pool=read_timeout_seconds.total_seconds(), + )Alternatively, this may be intentional to give users full control. If so, consider documenting that
read_timeout_secondsoverrides all timeout components.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/integration_tests/test_timeout_fix.pyis excluded by none and included by none
📒 Files selected for processing (1)
src/fastmcp/client/transports.py
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Python source code must use Python ≥3.10 with full type annotations
Never use bareexcept- be specific with exception types
Prioritize readable, understandable code - clarity over cleverness; avoid obfuscated or confusing patterns
Follow existing patterns and maintain consistency across the codebase
Files:
src/fastmcp/client/transports.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
🔇 Additional comments (3)
src/fastmcp/client/transports.py (3)
298-305: Factory path correctly forwards configuration with conditional timeout.The conditional dict unpacking
**({"timeout": timeout} if timeout else {})cleanly avoids passingtimeout=Noneto factories that may not handle it. The explicitfollow_redirects=Trueensures consistent behavior between factory and default paths.
28-28: Private API usage is the official pattern in the MCP SDK—no changes needed.The import from
mcp.shared._httpx_utilsis the recommended approach for HTTP client creation in the MCP Python SDK (also used in official MCP client implementations like SSE and streamable-http). The code correctly usescreate_mcp_http_client()to get MCP-standard timeout defaults (30s connect, 5min read) and follows redirects, as documented in the comment at lines 287-289. This is consistent with the existingMcpHttpClientFactoryimport and represents the intended way to handle HTTP clients in the SDK. While the module path uses the private naming convention, this is the stable interface provided by MCP for this purpose.
306-311: Correct approach using MCP SDK's client factory with appropriate defaults.This properly replaces the raw
httpx.AsyncClient()withcreate_mcp_http_client(), which provides MCP-appropriate defaults (30s connect, 5min read timeout, andfollow_redirects=True). The parameters passed (headers, timeout, auth) are correctly handled—whentimeoutisNone, the factory's defaults apply; when provided, they override the defaults. This addresses the PR objective of fixing premature timeouts from the original 5-second default.
…conds; add test timeout marker
When
StreamableHttpTransportwas updated to use the newstreamable_http_clientAPI, the code path for creating an httpx client when no factory is provided usedhttpx.AsyncClient()directly. Without an explicit timeout, httpx defaults to 5 seconds for all operations.This caused tools taking longer than 5 seconds to fail with a 409 Conflict error as the client's SSE stream would timeout and trigger reconnection logic that conflicts with the existing GET stream.
The fix uses
create_mcp_http_client()from the MCP SDK which sets appropriate defaults (30s connect, 5min read timeout) and enablesfollow_redirects.Fixes #2842, fixes #2845