Fix timeout not propagating to proxy clients in multi-server MCPConfig#2809
Fix timeout not propagating to proxy clients in multi-server MCPConfig#2809
Conversation
marvin-context-protocol
Bot
added
bug
WalkthroughMCPConfigTransport was refactored to support multi-server configurations: it now tracks underlying transports in _transports, adds name_as_prefix, and delays proxy creation until connect_session. A new internal _create_proxy builds per-server transports and ProxyClient-backed proxies (including Transforming* server types). connect_session short-circuits single-server usage or constructs a FastMCPRouter mounting per-server proxies and opens a session via FastMCPTransport. close closes all tracked transports. Separately, two utility functions in src/fastmcp/utilities/mcp_config.py that converted MCP configs into server/transport/proxy tuples were removed. 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. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/utilities/json_schema.py (1)
9-51: Well-designed fallback for circular schema handling.The
dereference_refsfunction properly:
- Uses
jsonrefto inline all$refreferences- Removes
$defsafter successful resolution- Falls back to
resolve_root_reffor self-referencing/circular schemas that can't be fully dereferencedConsider the static analysis suggestion (TRY300) to move the success return into an
elseblock for clearer control flow, though this is a minor style preference.♻️ Optional: Move return to else block per TRY300
try: # Use jsonref to resolve all $ref references # proxies=False returns plain dicts (not proxy objects) # lazy_load=False resolves immediately dereferenced = replace_refs(schema, proxies=False, lazy_load=False) # Remove $defs since all references have been resolved if isinstance(dereferenced, dict) and "$defs" in dereferenced: dereferenced = {k: v for k, v in dereferenced.items() if k != "$defs"} - return dereferenced - except JsonRefError: # Self-referencing/circular schemas can't be fully dereferenced # Fall back to resolving only root-level $ref (for MCP spec compliance) return resolve_root_ref(schema) + else: + return dereferenced
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
pyproject.tomlis excluded by none and included by nonetests/test_mcp_config.pyis excluded by none and included by nonetests/tools/test_tool.pyis excluded by none and included by nonetests/tools/test_tool_transform.pyis excluded by none and included by nonetests/utilities/openapi/test_schemas.pyis excluded by none and included by nonetests/utilities/test_json_schema.pyis excluded by none and included by noneuv.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (5)
src/fastmcp/client/transports.pysrc/fastmcp/tools/tool.pysrc/fastmcp/tools/tool_transform.pysrc/fastmcp/utilities/json_schema.pysrc/fastmcp/utilities/mcp_config.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/client/transports.pysrc/fastmcp/tools/tool.pysrc/fastmcp/utilities/json_schema.pysrc/fastmcp/tools/tool_transform.py
🧠 Learnings (3)
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
Applied to files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/client/transports.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/tools/tool.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Applied to files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/tools/tool_transform.py
🧬 Code graph analysis (4)
src/fastmcp/utilities/mcp_config.py (3)
src/fastmcp/server/server.py (3)
FastMCP(194-2576)name(419-420)as_proxy(2509-2567)src/fastmcp/client/transports.py (3)
ClientTransport(77-119)StreamableHttpTransport(231-332)SSETransport(161-228)src/fastmcp/server/providers/proxy.py (1)
ProxyClient(703-737)
src/fastmcp/client/transports.py (1)
src/fastmcp/utilities/mcp_config.py (1)
mcp_config_to_servers_and_transports(18-31)
src/fastmcp/tools/tool.py (1)
src/fastmcp/utilities/json_schema.py (1)
compress_schema(286-323)
src/fastmcp/tools/tool_transform.py (1)
src/fastmcp/utilities/json_schema.py (1)
compress_schema(286-323)
🪛 Ruff (0.14.10)
src/fastmcp/utilities/json_schema.py
45-45: Consider moving this statement to an else block
(TRY300)
⏰ 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 with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
🔇 Additional comments (9)
src/fastmcp/utilities/mcp_config.py (2)
18-31: LGTM! Clean API extension for proxy client tracking.The return type update to include
Client[Any] | Noneas a fourth tuple element is well-documented and properly typed. This enables the timeout propagation fix for multi-server configurations.
53-72: LGTM! Correct proxy client handling for different server types.The logic correctly distinguishes between:
- Transforming servers that manage their own clients (returning
None)- Non-transforming servers where the
ProxyClientis captured and returned for timeout propagationsrc/fastmcp/tools/tool.py (1)
37-37: LGTM! Import cleanup aligns with schema refactoring.The removal of
resolve_root_refimport is consistent with the changes injson_schema.pywherecompress_schemanow internally handles reference resolution viadereference_refs, which falls back toresolve_root_reffor circular schemas.src/fastmcp/tools/tool_transform.py (2)
683-683: LGTM! Consistent with updatedcompress_schemasignature.The removal of
prune_defs=Truealigns with the refactoredcompress_schemainjson_schema.py, which now handles reference resolution viadereference_refsat the start of the function.
866-866: LGTM! Same pattern as line 683.Consistent removal of the deprecated
prune_defsparameter from thecompress_schemacall.src/fastmcp/client/transports.py (3)
1000-1001: LGTM! Proxy client tracking for timeout propagation.The
_proxy_clientslist enables collecting proxy clients during initialization so their timeouts can be updated inconnect_session().
1017-1028: LGTM! Correct unpacking and conditional collection of proxy clients.The code correctly:
- Unpacks the new 4-tuple return value
- Only appends non-None proxy clients to the tracking list
1036-1041: Core fix: Timeout propagation to proxy clients.This correctly propagates
read_timeout_secondsto all tracked proxy clients before establishing the session. The comment accurately explains why this works withClient.new()'s shallow copy behavior.One observation: this sets
read_timeout_secondseven whentimeoutisNone, which will override any existing value. This behavior seems intentional—if the user doesn't provide a timeout, the proxy clients shouldn't use a stale one.src/fastmcp/utilities/json_schema.py (1)
306-321: LGTM! Correct schema compression flow.The refactored
compress_schema:
- Dereferences all
$refentries first viadereference_refs- Applies parameter pruning
- Runs single-pass optimization with
prune_defs=False(since$defsare already removed bydereference_refs)This ensures MCP client compatibility while maintaining schema optimization.
When MCPConfigTransport has multiple servers, it creates proxy clients for each server. The timeout from the top-level Client wasn't being propagated to these proxy clients, causing HTTP calls to ignore the timeout setting. Store proxy client references and propagate read_timeout_seconds to them in connect_session(). Closes #2802
jlowin
force-pushed
the
fix/mcp-config-timeout-propagation
branch
from
84ed861 to
6b11026
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84ed8618d2
ℹ️ 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".
| def mcp_config_to_servers_and_transports( | ||
| config: MCPConfig, | ||
| ) -> list[tuple[str, FastMCP[Any], ClientTransport]]: | ||
| """A utility function to convert each entry of an MCP Config into a transport and server.""" | ||
| ) -> list[tuple[str, FastMCP[Any], ClientTransport, Client[Any] | None]]: | ||
| """A utility function to convert each entry of an MCP Config into a transport, server, and proxy client. |
There was a problem hiding this comment.
Changing mcp_config_to_servers_and_transports to return a 4‑tuple will raise ValueError: too many values to unpack for any existing callers that do for name, server, transport in .... This helper is part of the documented SDK surface, so the new return shape is a breaking change introduced here. Consider keeping the original 3‑tuple and exposing proxy clients via a new helper or an opt‑in flag to avoid breaking external consumers.
Useful? React with 👍 / 👎.
| This is a regression test for https://github.com/jlowin/fastmcp/issues/2802 | ||
| where timeout was ignored in multi-server configurations. | ||
| """ | ||
| import datetime |
There was a problem hiding this comment.
AGENTS.md (Testing Standards) requires imports to be at the top of the file, not inside test bodies. The inline import datetime violates that rule and will be flagged in review for this repo. Move the import to the module header to align with the project’s mandated testing conventions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/utilities/mcp_config.py (1)
3-3: Consider usingTYPE_CHECKINGfor theClientimport.Since
Clientis only used in type annotations in this file and not at runtime, consider wrapping this import inTYPE_CHECKINGto avoid potential circular import issues and reduce import overhead:+from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from fastmcp.client.client import Client -from fastmcp.client.client import ClientThen use string annotation:
"Client[Any] | None"in the return types.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/test_mcp_config.pyis excluded by none and included by none
📒 Files selected for processing (2)
src/fastmcp/client/transports.pysrc/fastmcp/utilities/mcp_config.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/client/transports.py
🧠 Learnings (3)
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
Applied to files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/client/transports.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Applied to files:
src/fastmcp/utilities/mcp_config.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/utilities/mcp_config.py
🧬 Code graph analysis (2)
src/fastmcp/utilities/mcp_config.py (3)
src/fastmcp/client/transports.py (2)
ClientTransport(77-119)StreamableHttpTransport(231-332)src/fastmcp/mcp_config.py (3)
to_transport(119-123)to_transport(159-168)to_transport(211-233)src/fastmcp/server/providers/proxy.py (1)
ProxyClient(703-737)
src/fastmcp/client/transports.py (1)
src/fastmcp/utilities/mcp_config.py (1)
mcp_config_to_servers_and_transports(18-31)
⏰ 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.13 on ubuntu-latest
- 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
🔇 Additional comments (5)
src/fastmcp/client/transports.py (3)
992-1001: LGTM on proxy client tracking initialization.The local import of
Clientavoids circular dependency issues, and the_proxy_clientslist is properly typed. The initialization alongside_underlying_transportsis consistent with the existing pattern.
1017-1028: LGTM on proxy client collection during multi-server setup.The conditional append correctly filters out
Nonevalues from transforming servers that manage their own clients. The unpacking aligns with the updated 4-tuple return type frommcp_config_to_servers_and_transports.
1036-1044: Timeout propagation approach is correct and properly documented.The code correctly propagates
read_timeout_secondsto all proxy clients via direct mutation of their_session_kwargs. This works becauseClient.new()usescopy.copy(self), creating a shallow copy where all new clients share the same_session_kwargsdictionary reference. The existing comment accurately documents this coupling, so the implementation is clear to future maintainers.src/fastmcp/utilities/mcp_config.py (2)
18-31: LGTM on updated function signature and documentation.The return type correctly includes
Client[Any] | Noneas the fourth tuple element, and the docstring clearly explains thatproxy_clientmay beNonefor transforming server types.
51-72: LGTM on proxy client handling logic.The implementation correctly:
- Initializes
proxy_clienttoNone(line 53)- Leaves it as
Nonefor transforming servers that manage their own clients (line 58-62)- Creates and assigns a
ProxyClientfor the non-transforming path (lines 65-68)- Returns the 4-tuple consistently (line 72)
The type annotation
Client[Any] | Nonematches the actual runtime values.
Refactor MCPConfigTransport to handle all logic inline rather than through helper functions in utilities/mcp_config.py. Proxy clients are now created in connect_session() when session_kwargs (including timeout) are available, fixing timeout propagation to backend servers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/client/transports.py (1)
1023-1037: Consider adding error handling for partial transport creation.While the current implementation is functional, if
_create_proxyraises an exception during the loop (lines 1029-1032), some transports may be left in_transportswithout being properly initialized. Consider wrapping the loop in a try-except block to ensure cleanup on failure:♻️ Optional improvement for robustness
# Multiple servers - create composite with mounted proxies # Clear any previous transports from prior connections self._transports = [] timeout = session_kwargs.get("read_timeout_seconds") composite = FastMCP[Any](name="MCPRouter") +try: for name, server_config in self.config.mcpServers.items(): transport, proxy = self._create_proxy(name, server_config, timeout) self._transports.append(transport) composite.mount(proxy, namespace=name if self.name_as_prefix else None) +except Exception: + # Clean up any transports created before the failure + for transport in self._transports: + await transport.close() + self._transports = [] + raise async with FastMCPTransport(mcp=composite).connect_session( **session_kwargs ) as session: yield sessionNote: This is optional since users should call
close()on exception anyway, but it would make the behavior more defensive.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/client/test_client.pyis excluded by none and included by nonetests/test_mcp_config.pyis excluded by none and included by none
📒 Files selected for processing (2)
src/fastmcp/client/transports.pysrc/fastmcp/utilities/mcp_config.py
💤 Files with no reviewable changes (1)
- src/fastmcp/utilities/mcp_config.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/client/transports.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
Applied to files:
src/fastmcp/client/transports.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Applied to files:
src/fastmcp/client/transports.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/client/transports.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Follow existing patterns and maintain consistency in code implementation
Applied to files:
src/fastmcp/client/transports.py
🧬 Code graph analysis (1)
src/fastmcp/client/transports.py (1)
src/fastmcp/mcp_config.py (9)
MCPConfig(247-298)RemoteMCPServer(175-233)StdioMCPServer(126-168)TransformingRemoteMCPServer(236-237)TransformingStdioMCPServer(171-172)infer_transport_type_from_url(56-73)to_transport(119-123)to_transport(159-168)to_transport(211-233)
🪛 Ruff (0.14.10)
src/fastmcp/client/transports.py
1006-1006: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
src/fastmcp/client/transports.py (5)
36-44: LGTM! Imports support multi-server configuration.The new imports enable handling of different MCP server types including transforming servers, which is essential for the multi-server proxy functionality.
1002-1011: Well-structured initialization for single and multi-server support.The
name_as_prefixflag and_transportslist provide good state management. The single-server optimization (creating transport eagerly in__init__) allows the transport to be inspected before connection, which is a nice ergonomic improvement.
1017-1037: Excellent refactoring that correctly propagates timeout to proxy clients.The key improvement here is extracting
timeoutfromsession_kwargs(line 1026) and passing it to_create_proxy(line 1030), which ensures that the timeout flows through to all backend servers. The single-server optimization (lines 1017-1021) avoids unnecessary overhead, while the multi-server composite approach (lines 1023-1037) elegantly handles multiple backends through mounted proxies.
1039-1076: Core fix correctly implemented - timeout now propagates to proxy clients.This method is the heart of the timeout propagation fix. Line 1067 creates
ProxyClient(transport=transport, timeout=timeout), ensuring that the timeout from session-level configuration flows through to each backend server's proxy client. The handling of transforming servers (lines 1054-1065) is also well-structured, correctly extracting transformation configuration while delegating to base classto_transport()for the underlying transport.
1077-1079: LGTM! Proper cleanup of all underlying transports.The close method correctly iterates through all tracked transports and ensures cleanup, handling both single-server and multi-server configurations appropriately.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9caaff7cac
ℹ️ 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".
| # Multiple servers - create composite with mounted proxies | ||
| # Clear any previous transports from prior connections | ||
| self._transports = [] | ||
| timeout = session_kwargs.get("read_timeout_seconds") |
There was a problem hiding this comment.
When the same Client instance is used across multiple async with client: cycles, connect_session is called again after the previous session ends. In the multi-server path you reset self._transports to an empty list without closing the existing transports, so any prior StdioTransport instances (with keep_alive=True by default) keep their subprocesses running and become unreachable. This leaks processes/connections over repeated connect/disconnect cycles. Consider closing existing transports (or reusing them) before overwriting the list so prior sessions are cleaned up.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fastmcp/client/transports.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/client/transports.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
Applied to files:
src/fastmcp/client/transports.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Applied to files:
src/fastmcp/client/transports.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/client/transports.py
🧬 Code graph analysis (1)
src/fastmcp/client/transports.py (3)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/mcp_config.py (9)
MCPConfig(247-298)RemoteMCPServer(175-233)StdioMCPServer(126-168)TransformingRemoteMCPServer(236-237)TransformingStdioMCPServer(171-172)infer_transport_type_from_url(56-73)to_transport(119-123)to_transport(159-168)to_transport(211-233)src/fastmcp/server/providers/proxy.py (1)
ProxyClient(703-737)
🪛 Ruff (0.14.10)
src/fastmcp/client/transports.py
1006-1006: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (1)
src/fastmcp/client/transports.py (1)
1046-1082: Excellent implementation of timeout propagation fix.This method correctly addresses the core issue described in the PR: timeout now flows from session_kwargs through to the ProxyClient (line 1074), ensuring all backend servers respect the Client-provided timeout.
The logic properly handles:
- Transforming servers by calling base class
to_transport()to get the raw transport, then applying transformations viatool_transformationsparameter- Regular servers via direct
to_transport()call- Tag filtering via
include_tagsandexclude_tags- Circular import prevention by importing ProxyClient locally
| if isinstance(config, dict): | ||
| config = MCPConfig.from_dict(config) | ||
| self.config = config | ||
| self.name_as_prefix = name_as_prefix |
There was a problem hiding this comment.
Add type annotation for name_as_prefix parameter.
The parameter is missing a type annotation. Per coding guidelines, Python ≥ 3.10 requires full type annotations.
🔧 Proposed fix
- def __init__(self, config: MCPConfig | dict, name_as_prefix: bool = True):
+ def __init__(self, config: MCPConfig | dict, name_as_prefix: bool = True) -> None:Also consider adding the bool type annotation if it's not already inferred:
- self.name_as_prefix = name_as_prefix
+ self.name_as_prefix: bool = name_as_prefixBased on coding guidelines requiring full type annotations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.name_as_prefix = name_as_prefix | |
| self.name_as_prefix: bool = name_as_prefix |
1 participant
When using
MCPConfigwith multiple servers, the timeout wasn't propagating to the proxy clients because they were being created in__init__before the timeout was known. The solution was to defer proxy client creation toconnect_session()whensession_kwargsare available.This also simplifies the codebase by moving all logic into
MCPConfigTransportand deleting the helper utilities inutilities/mcp_config.py.Closes #2802