Unify component storage in LocalProvider#2680
Conversation
Consolidates ToolManager, ResourceManager, and PromptManager into a single LocalProvider class. This aligns with the existing provider architecture where FastMCP iterates through providers for all operations. - LocalProvider stores tools, resources, templates, and prompts - FastMCP.tool/resource/prompt decorators delegate to LocalProvider - Delete redundant manager classes and their tests - Update tests to use public API (get_tools() vs internal dict access)
marvin-context-protocol
Bot
added
breaking change
jlowin
added
feature
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis PR refactors FastMCP to a provider-first architecture: replaces per-type managers (ToolManager, ResourceManager, PromptManager) with a LocalProvider and a providers list, delegates add/get/list/remove operations to LocalProvider or iterates providers (first-wins deduplication), and wires LocalProvider into FastMCP constructor. ComponentService was updated for prompt mounting and to use provider-based lookups. FastMCPProvider's task collection now traverses nested providers. Provider base gained a _notify helper. Several public package all exports were adjusted to remove manager classes. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/fastmcp/contrib/component_manager/component_service.py (2)
196-204: Bug: Using "tool" component_type for prompt lookups.Lines 198 and 224 pass
"tool"to_get_mounted_server_and_keyfor prompt operations. Since prompts use the same namespace transformation as tools (name-based, not URI-based), this happens to work correctly, but it's semantically misleading and could cause issues if prompt transformations diverge from tool transformations in the future.🔎 Proposed fix for semantic clarity
# 2. Check mounted servers via FastMCPProvider/TransformingProvider for provider in self._server._providers: - result = _get_mounted_server_and_key(provider, key, "tool") + result = _get_mounted_server_and_key(provider, key, "prompt") if result is not None: server, unprefixed = result mounted_service = ComponentService(server) prompt = await mounted_service._enable_prompt(unprefixed) return promptNote: This requires updating
_get_mounted_server_and_keyto handle"prompt"the same as"tool"for now:def _get_mounted_server_and_key( provider: Provider, key: str, component_type: str, ) -> tuple[FastMCP, str] | None: ... if isinstance(provider, TransformingProvider): # TransformingProvider - reverse the transformation if component_type == "resource": original = provider._reverse_resource_uri(key) - else: + else: # tool or prompt - both use name-based transformation original = provider._reverse_tool_name(key)
222-229: Same bug: Using "tool" component_type for disable_prompt.Same issue as above - Line 224 uses
"tool"for prompt lookup.🔎 Proposed fix
# 2. Check mounted servers via FastMCPProvider/TransformingProvider for provider in self._server._providers: - result = _get_mounted_server_and_key(provider, key, "tool") + result = _get_mounted_server_and_key(provider, key, "prompt")
🧹 Nitpick comments (3)
examples/mount_example.py (1)
106-106: Prefer public API over private methods in examples.Line 106 calls
_read_resource_mcp(), a private method. Examples should demonstrate only stable, public APIs to avoid coupling examples to implementation details that may change.If
_read_resource_mcp()is internal/experimental, either:
- Replace it with an equivalent public API call if one exists, or
- Add a comment clarifying that this demonstrates internal behavior (not recommended for production)
Can you confirm whether a public alternative exists, or if this line should be removed/updated?
src/fastmcp/server/providers/local.py (2)
252-256: Consider efficiency optimization for get_tool.The current implementation calls
list_tools()which applies all transformations, then searches linearly. For providers with many tools, this could be inefficient. However, since transformations need to be applied and the number of tools is typically small, this is acceptable for now.If performance becomes a concern, consider caching transformed tools or implementing a more direct lookup:
async def get_tool(self, name: str) -> Tool | None: """Get a tool by name, with transformations applied.""" # Check if tool exists before transforming if name not in self._tools and name not in self._tool_transformations: return None tools = await self.list_tools() return next((t for t in tools if t.name == name), None)
458-468: Consider using TypeError for classmethod validation.The static analysis correctly identifies that
TypeErroris more appropriate thanValueErrorwhen the issue is about the type of input (classmethod vs regular function). This is a minor stylistic issue.🔎 Proposed fix
if isinstance(name_or_fn, classmethod): - raise ValueError( + raise TypeError( inspect.cleandoc( """ To decorate a classmethod, first define the method and then call tool() directly on the method instead of using it as a decorator. See https://gofastmcp.com/patterns/decorating-methods for examples and more information. """ ) )Apply similar changes at lines 592-602 and 740-750.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (18)
tests/client/test_client.pyis excluded by none and included by nonetests/contrib/test_component_manager.pyis excluded by none and included by nonetests/deprecated/test_exclude_args.pyis excluded by none and included by nonetests/deprecated/test_import_server.pyis excluded by none and included by nonetests/deprecated/test_settings.pyis excluded by none and included by nonetests/prompts/test_prompt_manager.pyis excluded by none and included by nonetests/resources/test_resource_manager.pyis excluded by none and included by nonetests/server/providers/openapi/test_performance_comparison.pyis excluded by none and included by nonetests/server/providers/test_local_provider.pyis excluded by none and included by nonetests/server/proxy/test_proxy_client.pyis excluded by none and included by nonetests/server/tasks/test_sync_function_task_disabled.pyis excluded by none and included by nonetests/server/tasks/test_task_config.pyis excluded by none and included by nonetests/server/test_mount.pyis excluded by none and included by nonetests/server/test_providers.pyis excluded by none and included by nonetests/server/test_server.pyis excluded by none and included by nonetests/server/test_tool_annotations.pyis excluded by none and included by nonetests/server/test_tool_transformation.pyis excluded by none and included by nonetests/tools/test_tool_manager.pyis excluded by none and included by none
📒 Files selected for processing (12)
examples/mount_example.pysrc/fastmcp/contrib/component_manager/component_service.pysrc/fastmcp/prompts/__init__.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/resources/__init__.pysrc/fastmcp/resources/resource_manager.pysrc/fastmcp/server/providers/__init__.pysrc/fastmcp/server/providers/fastmcp_provider.pysrc/fastmcp/server/providers/local.pysrc/fastmcp/server/server.pysrc/fastmcp/tools/__init__.pysrc/fastmcp/tools/tool_manager.py
💤 Files with no reviewable changes (4)
- src/fastmcp/resources/resource_manager.py
- src/fastmcp/tools/tool_manager.py
- src/fastmcp/prompts/prompt_manager.py
- src/fastmcp/resources/init.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Python ≥ 3.10 with full type annotations
Never use bareexcept- be specific with exception types
Files:
src/fastmcp/prompts/__init__.pysrc/fastmcp/contrib/component_manager/component_service.pysrc/fastmcp/server/providers/__init__.pysrc/fastmcp/server/providers/fastmcp_provider.pyexamples/mount_example.pysrc/fastmcp/tools/__init__.pysrc/fastmcp/server/providers/local.pysrc/fastmcp/server/server.py
src/fastmcp/**/__init__.py
📄 CodeRabbit inference engine (AGENTS.md)
Core types that define a module's purpose should be exported (e.g.,
Middlewarefromfastmcp.server.middleware), while specialized features can live in submodules
Files:
src/fastmcp/prompts/__init__.pysrc/fastmcp/server/providers/__init__.pysrc/fastmcp/tools/__init__.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Review and update related Manager classes (ToolManager, ResourceManager, PromptManager) when modifying MCP object definitions
📚 Learning: 2025-12-21T21:37:55.031Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.031Z
Learning: Applies to src/fastmcp/__init__.py : All module exports should be intentional - only re-export to `fastmcp.*` for fundamental types like `FastMCP` and `Client`, prefer users importing from specific submodules for specialized features
Applied to files:
src/fastmcp/prompts/__init__.pysrc/fastmcp/server/providers/__init__.pysrc/fastmcp/tools/__init__.pysrc/fastmcp/server/server.py
📚 Learning: 2025-12-21T21:37:55.031Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.031Z
Learning: Applies to src/fastmcp/**/__init__.py : Core types that define a module's purpose should be exported (e.g., `Middleware` from `fastmcp.server.middleware`), while specialized features can live in submodules
Applied to files:
src/fastmcp/server/providers/__init__.pysrc/fastmcp/server/server.py
📚 Learning: 2025-11-26T21:51:44.174Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Review and update related Manager classes (ToolManager, ResourceManager, PromptManager) when modifying MCP object definitions
Applied to files:
src/fastmcp/server/providers/local.py
🧬 Code graph analysis (3)
src/fastmcp/prompts/__init__.py (1)
src/fastmcp/prompts/prompt.py (3)
Message(38-46)Prompt(118-322)PromptResult(73-115)
src/fastmcp/server/providers/__init__.py (1)
src/fastmcp/server/providers/local.py (1)
LocalProvider(54-804)
src/fastmcp/server/server.py (3)
src/fastmcp/server/providers/local.py (18)
add_tool_transformation(209-218)list_tools(244-250)tool(357-373)tool(376-392)tool(394-530)list_resources(266-268)resource(532-655)list_resource_templates(284-286)list_prompts(308-310)prompt(658-670)prompt(673-685)prompt(687-804)add_tool(102-115)remove_tool(117-128)remove_tool_transformation(231-238)add_resource(130-140)add_template(155-165)add_prompt(180-190)src/fastmcp/server/providers/base.py (5)
Provider(56-374)list_tools(142-147)list_resources(161-166)list_resource_templates(180-185)list_prompts(203-208)src/fastmcp/resources/resource.py (2)
key(302-304)Resource(137-331)
🪛 Ruff (0.14.10)
src/fastmcp/server/providers/local.py
127-127: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
177-177: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
459-468: Prefer TypeError exception for invalid type
(TRY004)
502-505: Avoid specifying long messages outside the exception class
(TRY003)
511-513: Avoid specifying long messages outside the exception class
(TRY003)
586-589: Avoid specifying long messages outside the exception class
(TRY003)
593-602: Prefer TypeError exception for invalid type
(TRY004)
650-653: Avoid specifying long messages outside the exception class
(TRY003)
741-750: Prefer TypeError exception for invalid type
(TRY004)
780-783: Avoid specifying long messages outside the exception class
(TRY003)
789-791: Avoid specifying long messages outside the exception class
(TRY003)
src/fastmcp/server/server.py
1689-1689: Avoid specifying long messages outside the exception class
(TRY003)
1721-1721: Avoid specifying long messages outside the exception class
(TRY003)
1939-1939: Avoid specifying long messages outside the exception class
(TRY003)
1972-1972: Avoid specifying long messages outside the exception class
(TRY003)
2150-2150: 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). (4)
- GitHub Check: Run tests: Python 3.13 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.10 on ubuntu-latest
🔇 Additional comments (12)
examples/mount_example.py (1)
1-8: ✓ Docstring update aligns with ToolManager removal.The module docstring has been appropriately updated to remove references to ToolManager and now accurately describes the mounting functionality. This aligns with the PR's consolidation of component management into LocalProvider.
src/fastmcp/server/providers/__init__.py (1)
32-32: LGTM! LocalProvider properly exported as a core type.The export aligns with the coding guidelines that core types defining a module's purpose should be exported. LocalProvider is now the central component storage mechanism, making it an appropriate public export.
Also applies to: 41-41
src/fastmcp/prompts/__init__.py (1)
1-8: LGTM! PromptManager correctly removed from public API.The removal of
PromptManageraligns with the PR's objective to consolidate component storage in LocalProvider. Core prompt types remain properly exported.src/fastmcp/server/server.py (5)
196-226: Deprecation handling looks correct, but consider edge case with multiple deprecated params.If a user provides multiple deprecated parameters (e.g.,
on_duplicate_tools="error"andon_duplicate_resources="replace"), only the first one encountered will be used. This behavior is acceptable since the new API consolidates these into a single setting, but users might expect a warning if they provide conflicting values.Consider whether conflicting deprecated parameters should emit an additional warning to help users migrate their code.
236-248: LGTM! LocalProvider correctly initialized as first in provider chain.The initialization ensures local components registered via decorators take precedence over dynamic providers, maintaining backward compatibility with the previous manager-based approach.
1685-1707: LGTM! Duplicate handling logic is correct.The implementation properly handles all four
DuplicateBehaviorvalues. The direct access to_local_provider._toolsis acceptable here since the server owns and manages the local provider.
994-1008: LGTM! Consistent first-provider-wins pattern for component listing.The iteration pattern is applied consistently across tools, resources, templates, and prompts. The
_should_enable_componentcheck combined with duplicate key checking ensures correct filtering behavior.
1501-1523: Verify two-pass resource lookup behavior.The implementation now prioritizes concrete resources from all providers before checking templates from any provider. Previously, each provider was checked for both resources and templates before moving to the next. This could change behavior if:
- Provider A has a template matching
data://foo/123- Provider B has a concrete resource
data://foo/123Previously, Provider A's template might have been used; now Provider B's concrete resource wins.
Confirm this two-pass global lookup (concrete resources first across all providers, then templates) is the intended behavior change.
src/fastmcp/tools/__init__.py (1)
1-4: LGTM! ToolManager correctly removed from public API.The removal aligns with the PR's objective. Core tool types (
Tool,FunctionTool) and transform utilities (forward,forward_raw) remain properly exported.src/fastmcp/server/providers/fastmcp_provider.py (1)
559-584: LGTM! Task aggregation correctly traverses nested providers.The implementation properly delegates to each provider's
get_tasks()method, aggregating components for Docket registration. This aligns with the unified provider architecture and eliminates direct manager access.src/fastmcp/server/providers/local.py (2)
89-239: LGTM! Clean storage interface with appropriate error handling.The storage methods provide a consistent interface for all component types. Using
KeyErrorfor missing items on removal is the correct choice, and the error messages are appropriately descriptive.
1-52: LGTM! Well-documented module with clear examples.The module docstring and class documentation provide excellent examples for both standalone usage and server integration. The type hints and overloads ensure good IDE support.
- Use _reverse_prompt_name for prompts instead of _reverse_tool_name - Remove private method usage from mount example
Server decorators and add_* methods now delegate to LocalProvider. Notifications moved to base Provider class and called by LocalProvider.
Move behavior tests (type serialization, parameters, context injection, decorators) to LocalProvider test files. Keep user-facing feature tests (tags, enabled) in test_server_interactions.py. Add delegation tests to test_server.py.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/fastmcp/server/providers/local_provider.py (1)
470-480: Consider usingTypeErrorfor invalid type errors.When the decorated target is a
classmethod, the error is about an invalid type being passed, which conventionally should be aTypeErrorrather thanValueError. This applies to similar checks inresource(line 605) andprompt(line 753) decorators as well.🔎 Proposed fix for tool decorator
if isinstance(name_or_fn, classmethod): - raise ValueError( + raise TypeError( inspect.cleandoc( """ To decorate a classmethod, first define the method and then call tool() directly on the method instead of using it as a decorator. See https://gofastmcp.com/patterns/decorating-methods for examples and more information. """ ) )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
tests/server/providers/test_local_provider.pyis excluded by none and included by nonetests/server/providers/test_local_provider_prompts.pyis excluded by none and included by nonetests/server/providers/test_local_provider_resources.pyis excluded by none and included by nonetests/server/providers/test_local_provider_tools.pyis excluded by none and included by nonetests/server/test_mount.pyis excluded by none and included by nonetests/server/test_server.pyis excluded by none and included by nonetests/server/test_server_interactions.pyis excluded by none and included by none
📒 Files selected for processing (2)
src/fastmcp/server/providers/__init__.pysrc/fastmcp/server/providers/local_provider.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fastmcp/server/providers/init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Python ≥ 3.10 with full type annotations
Never use bareexcept- be specific with exception types
Files:
src/fastmcp/server/providers/local_provider.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Review and update related Manager classes (ToolManager, ResourceManager, PromptManager) when modifying MCP object definitions
🧬 Code graph analysis (1)
src/fastmcp/server/providers/local_provider.py (5)
src/fastmcp/resources/resource.py (1)
Resource(137-331)src/fastmcp/resources/template.py (1)
ResourceTemplate(97-298)src/fastmcp/server/providers/base.py (2)
Provider(56-394)_notify(74-92)src/fastmcp/server/tasks/config.py (2)
TaskConfig(25-104)supports_tasks(69-75)src/fastmcp/tools/tool_transform.py (2)
ToolTransformConfig(878-921)apply_transformations_to_tools(924-943)
🪛 Ruff (0.14.10)
src/fastmcp/server/providers/local_provider.py
128-128: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
165-165: Avoid specifying long messages outside the exception class
(TRY003)
185-185: Avoid specifying long messages outside the exception class
(TRY003)
201-201: Avoid specifying long messages outside the exception class
(TRY003)
221-221: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
257-257: Avoid specifying long messages outside the exception class
(TRY003)
471-480: Prefer TypeError exception for invalid type
(TRY004)
514-517: Avoid specifying long messages outside the exception class
(TRY003)
523-525: Avoid specifying long messages outside the exception class
(TRY003)
598-601: Avoid specifying long messages outside the exception class
(TRY003)
605-614: Prefer TypeError exception for invalid type
(TRY004)
662-665: Avoid specifying long messages outside the exception class
(TRY003)
753-762: Prefer TypeError exception for invalid type
(TRY004)
792-795: Avoid specifying long messages outside the exception class
(TRY003)
801-803: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
src/fastmcp/server/providers/local_provider.py (6)
1-54: LGTM!Module setup is clean with proper type annotations, conditional imports, and a helpful docstring example.
91-110: LGTM!Clean initialization with well-documented
on_duplicateparameter and properly typed storage dictionaries.
116-151: LGTM!Consistent duplicate-handling pattern with proper notification via
_notify(). The code correctly handles all fourDuplicateBehaviormodes.
300-338: LGTM!Provider interface is correctly implemented.
list_toolsproperly applies transformations, andget_toolcorrectly searches through transformed tools (necessary since transformations can rename tools).
344-362: LGTM!Task filtering correctly uses
task_config.supports_tasks()to identify components eligible for background execution.
699-816: LGTM!The prompt decorator follows the same well-structured pattern as the tool decorator, correctly handling multiple calling conventions via overloads and partial application.
| def add_tool_transformation( | ||
| self, tool_name: str, transformation: ToolTransformConfig | ||
| ) -> None: | ||
| """Add a tool transformation. | ||
|
|
||
| Args: | ||
| tool_name: The name of the tool to transform. | ||
| transformation: The transformation configuration. | ||
| """ | ||
| self._tool_transformations[tool_name] = transformation | ||
|
|
||
| def get_tool_transformation(self, tool_name: str) -> ToolTransformConfig | None: | ||
| """Get a tool transformation. | ||
|
|
||
| Args: | ||
| tool_name: The name of the tool. | ||
|
|
||
| Returns: | ||
| The transformation config, or None if not found. | ||
| """ | ||
| return self._tool_transformations.get(tool_name) | ||
|
|
||
| def remove_tool_transformation(self, tool_name: str) -> None: | ||
| """Remove a tool transformation. | ||
|
|
||
| Args: | ||
| tool_name: The name of the tool. | ||
| """ | ||
| if tool_name in self._tool_transformations: | ||
| del self._tool_transformations[tool_name] |
There was a problem hiding this comment.
Missing _notify("tools") calls in transformation methods.
When a tool transformation is added or removed, the effective tool list changes (tools can be renamed, have descriptions modified, or be disabled). Clients should be notified to refresh their tool lists, similar to how add_tool and remove_tool call self._notify("tools").
🔎 Proposed fix
def add_tool_transformation(
self, tool_name: str, transformation: ToolTransformConfig
) -> None:
"""Add a tool transformation.
Args:
tool_name: The name of the tool to transform.
transformation: The transformation configuration.
"""
self._tool_transformations[tool_name] = transformation
+ self._notify("tools")
def get_tool_transformation(self, tool_name: str) -> ToolTransformConfig | None:
"""Get a tool transformation.
Args:
tool_name: The name of the tool.
Returns:
The transformation config, or None if not found.
"""
return self._tool_transformations.get(tool_name)
def remove_tool_transformation(self, tool_name: str) -> None:
"""Remove a tool transformation.
Args:
tool_name: The name of the tool.
"""
if tool_name in self._tool_transformations:
del self._tool_transformations[tool_name]
+ self._notify("tools")| else: | ||
| raise ValueError( | ||
| "Invalid resource or template definition due to a " | ||
| "mismatch between URI parameters and function parameters." | ||
| ) |
There was a problem hiding this comment.
Unreachable dead code.
This else branch is logically unreachable. The conditions has_uri_params or has_func_params (line 627) and not has_uri_params and not has_func_params (line 644) are mutually exhaustive—every boolean combination is covered by one of these two branches.
🔎 Proposed fix
if has_uri_params or has_func_params:
template = ResourceTemplate.from_function(
fn=fn,
uri_template=uri,
name=name,
title=title,
description=description,
icons=icons,
mime_type=mime_type,
tags=tags,
enabled=enabled,
annotations=annotations,
meta=meta,
task=supports_task,
)
self.add_template(template)
return template
- elif not has_uri_params and not has_func_params:
+ else:
resource_obj = Resource.from_function(
fn=fn,
uri=uri,
name=name,
title=title,
description=description,
icons=icons,
mime_type=mime_type,
tags=tags,
enabled=enabled,
annotations=annotations,
meta=meta,
task=supports_task,
)
self.add_resource(resource_obj)
return resource_obj
- else:
- raise ValueError(
- "Invalid resource or template definition due to a "
- "mismatch between URI parameters and function parameters."
- )🧰 Tools
🪛 Ruff (0.14.10)
662-665: Avoid specifying long messages outside the exception class
(TRY003)
Test Failure AnalysisSummary: The test Root Cause: This is a flaky test that intermittently fails during cleanup. The test itself passes, but the Suggested Solution: This is a test infrastructure issue, not a code bug. The most reliable fix is to mark this specific test to run without parallel execution to avoid timing conflicts:
@pytest.mark.noload # Prevent xdist from scheduling this test in parallel
async def test_nested_streamable_http_server_resolves_correctly(nested_server: str):
"""Test patch for https://github.com/modelcontextprotocol/python-sdk/pull/659"""
async with Client(transport=StreamableHttpTransport(nested_server)) as client:
result = await client.ping()
assert result is TrueAlternatively, increase the fixture's timeout to 5+ seconds and/or increase pytest's global timeout for this test, though this will slow down the test suite. Detailed AnalysisTest Behavior
This pattern indicates a resource contention or timing issue that only manifests under heavy parallel load. Why This HappensThe
In CI with high parallel load (4 workers, 3144 tests), the cancellation sometimes doesn't propagate through uvicorn's shutdown sequence within 3 seconds. The Test File ChangesThe PR shows this test was modified to add the timeout handling: + # Cleanup with timeout to prevent hanging
server_task.cancel()
try:
- await server_task
- except asyncio.CancelledError:
+ await asyncio.wait_for(server_task, timeout=3.0)
+ except (asyncio.CancelledError, asyncio.TimeoutError):
passThis was an attempt to fix the flakiness, but the 3-second fixture timeout combined with pytest's 5-second test timeout creates a narrow 2-second window that's not always sufficient in CI. CI Environment
Not Related to the RefactorThe test works fine locally on the PR branch and only fails intermittently in CI's parallel execution environment. The refactor didn't introduce any server lifecycle changes that would cause this - it's a pre-existing test infrastructure issue. Related Files
Note: This analysis has been updated to reflect the latest test failure (commit 613f913, run #20472384545). |
|
@coderabbitai ignore |
✅ Actions performedReviews paused. |
jlowin
force-pushed
the
local-provider-refactor
branch
from
3910dcf to
613f913
Compare
- Port FastMCPError exception handling from managers to server execute methods - Resolve uvicorn test cleanup conflicts (use graceful shutdown) - Keep manager deletions (execution moved to server)
1 participant
The three manager classes (ToolManager, ResourceManager, PromptManager) duplicated logic that already existed in the provider pattern. This PR consolidates them into a single
LocalProviderthat stores locally-defined components.Before: FastMCP had separate managers for each component type, plus a provider chain. The managers stored decorator-registered components, while providers supplied dynamic components. This created two parallel paths for component lookup.
After: LocalProvider is a proper Provider that stores decorator-registered components. FastMCP's
_providerslist always starts with LocalProvider, so the server just iterates providers for all operations. Server decorators (@tool,@resource,@prompt) andadd_*methods are thin passthroughs to LocalProvider—they inject server defaults and delegate. Single code path, simpler architecture.Breaking:
ToolManager,ResourceManager,PromptManagerare deleted. Users importing these directly will get ImportError. These were internal implementation details, not public API.