Clean MCP sessions and deduplicate tool notifications - #1286
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesTool visibility synchronization now records whether FastMCP’s published tool list changed before notifying MCP clients. Notifications use tracked sessions, bounded concurrent delivery, stale-session cleanup, and retry tracking. Tests cover lifecycle, fingerprinting, inspection, and notification behavior. Tool publication synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Unity
participant PluginHub
participant FastMCP
participant MCPClients
Unity->>PluginHub: register_tools
PluginHub->>FastMCP: sync visibility and inspect tools
FastMCP-->>PluginHub: published tool snapshot
PluginHub->>PluginHub: compare fingerprint
PluginHub->>MCPClients: concurrently send tools/list_changed
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
0217aa5 to
2ccdf40
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: 2ccdf404e8
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 630-671: Update _record_published_tool_list to use only the
supported FastMCP list_tools() API and remove the get_tools() fallback and
related RuntimeError path. Preserve the existing published-tools fallback when
FastMCP is unavailable and continue fingerprinting the resulting list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ea81ae2-a647-4ace-8b23-186c590dc634
📒 Files selected for processing (3)
Server/src/services/tools/__init__.pyServer/src/transport/plugin_hub.pyServer/tests/test_plugin_hub_session_lifecycle.py
2ccdf40 to
dcd568e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Server/src/transport/plugin_hub.py (1)
594-609: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRetry gate and log message can diverge from actual state.
Two related nits in this block:
- Retrying
_pending_tool_list_notifications(line 601) is gated onvisibility_syncedfor this call. If_sync_server_tool_visibilitythrows on this invocation, previously-failed pending sessions won't be retried until the next successful sync, even though their retry is unrelated to the current sync outcome.- If
published_tools_changedisTruebut there are currently no active MCP sessions,_pending_tool_list_notificationsstays empty after the.update(), so execution falls into theelif visibility_synced:branch and logs"Published tool schema unchanged; skipping tools/list_changed"— which is misleading since the schema did change; there simply weren't any sessions to notify.Neither is a functional defect (both self-heal / have no visible client impact), but decoupling the retry from
visibility_syncedand distinguishing the "changed but nobody to notify" case from "unchanged" would make behavior and logs more accurate for future debugging.♻️ Possible refactor
- published_tools_changed = ( - visibility_synced - and await cls._record_published_tool_list(payload.tools) - ) - if published_tools_changed: - cls._pending_tool_list_notifications.update(_active_mcp_sessions) - - if visibility_synced and cls._pending_tool_list_notifications: - # Retry only sessions that have not yet observed the latest - # published schema, avoiding duplicate notifications to peers that - # already succeeded. - await cls._notify_mcp_tool_list_changed( - list(cls._pending_tool_list_notifications) - ) - elif visibility_synced: - logger.debug("Published tool schema unchanged; skipping tools/list_changed") + published_tools_changed = ( + visibility_synced + and await cls._record_published_tool_list(payload.tools) + ) + if published_tools_changed: + cls._pending_tool_list_notifications.update(_active_mcp_sessions) + + if cls._pending_tool_list_notifications: + # Retry sessions that have not yet observed the latest published + # schema, regardless of whether this particular sync succeeded. + await cls._notify_mcp_tool_list_changed( + list(cls._pending_tool_list_notifications) + ) + elif visibility_synced: + logger.debug("Published tool schema unchanged; skipping tools/list_changed")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/src/transport/plugin_hub.py` around lines 594 - 609, Decouple the retry in the published-tool notification flow from the current `visibility_synced` result so `_pending_tool_list_notifications` is retried whenever it is non-empty, including after a failed `_sync_server_tool_visibility` attempt. Update the logging branches around `published_tools_changed` and `_pending_tool_list_notifications` so a changed schema with no active sessions is not logged as unchanged, while preserving the existing unchanged-schema message only when `published_tools_changed` is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 611-639: The _tool_fingerprint method currently calls
tool.to_mcp_tool() without its required overrides argument, causing fingerprint
generation to fail for FastMCP tools. Update the conversion in _tool_fingerprint
to pass an empty overrides mapping, or retain the original tool when no
overrides are needed, while preserving the existing serialization and hashing
flow.
In `@Server/tests/test_plugin_hub_session_lifecycle.py`:
- Around line 104-125: Update
test_patched_session_context_is_tracked_symmetrically to install session
tracking once, preserve the existing MiddlewareServerSession WeakSet, then
emulate a module reload before exiting a session created before reload. Verify
_install_session_tracking reuses the preserved set and that exiting the
pre-reload session removes it from plugin_hub._active_mcp_sessions.
- Around line 128-139: Update test_unchanged_tools_do_not_rebroadcast to invoke
the real publication transaction or caller that combines
_record_published_tool_list with _notify_mcp_tool_list_changed twice using the
same tools. Assert the first run notifies each session once and the second run
leaves both notification counts unchanged, rather than calling
_notify_mcp_tool_list_changed separately before the unchanged result.
---
Nitpick comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 594-609: Decouple the retry in the published-tool notification
flow from the current `visibility_synced` result so
`_pending_tool_list_notifications` is retried whenever it is non-empty,
including after a failed `_sync_server_tool_visibility` attempt. Update the
logging branches around `published_tools_changed` and
`_pending_tool_list_notifications` so a changed schema with no active sessions
is not logged as unchanged, while preserving the existing unchanged-schema
message only when `published_tools_changed` is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a05b9a4-d596-45ba-91a3-0479f0824baa
📒 Files selected for processing (3)
Server/src/services/tools/__init__.pyServer/src/transport/plugin_hub.pyServer/tests/test_plugin_hub_session_lifecycle.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/services/tools/init.py
dcd568e to
a4d7002
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/src/transport/plugin_hub.py (1)
667-682: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
elselog claims "unchanged" even when the schema did change.When
changedisTruebut there are no active MCP sessions, the pending set stays empty and this falls into theelse, logging that the schema was unchanged. Misleading while debugging notification delivery.♻️ Distinguish "unchanged" from "no sessions"
if cls._pending_tool_list_notifications: # Retry only sessions that have not yet observed the latest # published schema, avoiding duplicate notifications to peers that # already succeeded. await cls._notify_mcp_tool_list_changed( list(cls._pending_tool_list_notifications) ) + elif changed: + logger.debug( + "Published tool schema changed but no MCP sessions are active; " + "skipping tools/list_changed" + ) else: logger.debug( "Published tool schema unchanged; skipping tools/list_changed" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/src/transport/plugin_hub.py` around lines 667 - 682, Update the logging branch in the tool-schema publication flow around _pending_tool_list_notifications so it distinguishes an unchanged schema from a changed schema with no active MCP sessions. Preserve notification behavior, but ensure the “unchanged” message is emitted only when changed is false and use an appropriate log message for the no-session case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 667-682: Update the logging branch in the tool-schema publication
flow around _pending_tool_list_notifications so it distinguishes an unchanged
schema from a changed schema with no active MCP sessions. Preserve notification
behavior, but ensure the “unchanged” message is emitted only when changed is
false and use an appropriate log message for the no-session case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aea9f90e-2334-4d67-a136-469baeedcc10
📒 Files selected for processing (3)
Server/src/services/tools/__init__.pyServer/src/transport/plugin_hub.pyServer/tests/test_plugin_hub_session_lifecycle.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/tests/test_plugin_hub_session_lifecycle.py
a4d7002 to
74d3218
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/src/transport/plugin_hub.py (1)
651-670: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFingerprint compare-and-set is unsynchronized across two concurrent call sites.
_record_and_notify_tool_list_changeis reachable from both_handle_register_toolsandservices/tools/__init__.py::sync_tool_visibility_from_unity. The read of_published_tool_fingerprint(Line 651), the write (Line 654), and the pending-set seeding (Line 668) are not atomic, so overlapping publications can both observe "changed" and double-broadcast, or the loser can overwrite a newer digest and suppress a real notification.Worst case today is a redundant
tools/list_changed, but guarding the transaction with a dedicatedasyncio.Lockwould make it deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/src/transport/plugin_hub.py` around lines 651 - 670, Protect the fingerprint compare-and-set and pending-session seeding in _record_and_notify_tool_list_change with a dedicated asyncio.Lock shared by all callers. Hold the lock across _record_published_tool_list and the conditional _pending_tool_list_notifications update so concurrent publications are serialized, while preserving the existing notify=False behavior and return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 651-670: Protect the fingerprint compare-and-set and
pending-session seeding in _record_and_notify_tool_list_change with a dedicated
asyncio.Lock shared by all callers. Hold the lock across
_record_published_tool_list and the conditional _pending_tool_list_notifications
update so concurrent publications are serialized, while preserving the existing
notify=False behavior and return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 09205491-034e-4acc-86ed-455f7f51e73b
📒 Files selected for processing (3)
Server/src/services/tools/__init__.pyServer/src/transport/plugin_hub.pyServer/tests/test_plugin_hub_session_lifecycle.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Server/src/services/tools/init.py
- Server/tests/test_plugin_hub_session_lifecycle.py
74d3218 to
16c901a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/tests/integration/test_stdio_custom_tool_sync.py (1)
134-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring overstates what this test verifies.
_record_and_notify_tool_list_changeis fully mocked, so no retry or pending-notification behavior is exercised — the test only asserts the call path is reached when visibility sync fails. Consider rewording the docstring (retry behavior is covered inServer/tests/test_plugin_hub_session_lifecycle.py).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/tests/integration/test_stdio_custom_tool_sync.py` around lines 134 - 156, The docstring for test_sync_records_published_tools_when_visibility_update_fails overstates coverage by claiming pending notifications retry. Reword it to describe only that the tool-list change notification path is invoked when visibility synchronization fails, leaving the test assertions and mocks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Server/tests/integration/test_stdio_custom_tool_sync.py`:
- Around line 134-156: The docstring for
test_sync_records_published_tools_when_visibility_update_fails overstates
coverage by claiming pending notifications retry. Reword it to describe only
that the tool-list change notification path is invoked when visibility
synchronization fails, leaving the test assertions and mocks unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 861cfba9-5bac-492a-8a71-9de164985382
📒 Files selected for processing (4)
Server/src/services/tools/__init__.pyServer/src/transport/plugin_hub.pyServer/tests/integration/test_stdio_custom_tool_sync.pyServer/tests/test_plugin_hub_session_lifecycle.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/tests/test_plugin_hub_session_lifecycle.py
1 participant
Description
Prevent stale MCP sessions and repeated unchanged tool registrations from causing
tools/list_changedbroadcast growth, while preserving retry delivery for transiently failed sessions.Type of Change
Changes Made
__aenter__/__aexit__, including module reloads.list_tools()wire schema after publication.Compatibility / Package Source
#beta,#main, tag, branch, orfile:): branch derived directly from officialbeta(bd72241ab91c664176091789c9408d676783abec)Packages/packages-lock.json(if using a Git package URL): N/AThe implementation targets the declared
fastmcp>=3.0.2,<4dependency and uses its supportedlist_tools()API.Testing/Screenshots/Recordings
Python tests (
cd Server && uv run pytest tests/ -v)Unity EditMode tests
Unity PlayMode tests
Package import/compile check
Not applicable (explain why in Additional Notes)
Full Python suite: 1342 passed, 3 skipped.
Targeted session/notification/reload/stdio suite: 31 passed.
Direct runtime smoke used the locked FastMCP 3.0.2 dependency.
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)Related Issues
N/A.
Additional Notes
Twenty connect/disconnect cycles return to an empty active-session set. Reordered identical schemas are deduplicated; a real change notifies once, and transient failures retry without rebroadcasting to sessions that already succeeded. Targeted Ruff and
git diff --checkpass.Summary by CodeRabbit
Bug Fixes
Stability
Tests