Fix HTTP transport timeout defaulting to 5 seconds#2849
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f671d903e
ℹ️ 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.
Prevent test timeout from 6s tool duration
This test exercises a 6‑second tool call, but the repo’s pytest configuration sets a default timeout of 5 seconds (pyproject.toml → timeout = 5), so the test will time out under the normal test run even when the fix works. That makes CI fail for the new test in any environment where pytest-timeout is enabled (as it is here). Consider adding @pytest.mark.timeout(10) or shortening the duration so the test completes within the global timeout.
Useful? React with 👍 / 👎.
|
Warning Rate limit exceeded@jlowin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
WalkthroughThis pull request refactors timeout handling in the Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
loq.tomlis excluded by none and included by nonetests/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 windows-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (2)
src/fastmcp/client/transports.py (2)
256-262: LGTM - timeout handling is straightforward.The conversion from
timedeltatohttpx.Timeoutis correct. Note thathttpx.Timeout(seconds)sets all timeout types (connect, read, write, pool) to the same value, which is a reasonable default when users specify a single timeout duration.
28-28: Verify stability and support status of private module import.The import from
mcp.shared._httpx_utilsuses a private-designated module (underscore prefix), but web search indicates bothMcpHttpClientFactoryandcreate_mcp_http_clientare documented in the MCP SDK source and referenced in official docs. However, confirm whether this is an officially supported public API or internal implementation detail, and whether version pinning or an alternative approach is recommended.
Forward-port of #2848 for FastMCP 3.x.
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.