Fix: HTTP/Stdio transport routing and middleware session persistence #422
Conversation
- Ensure session persistence for stdio transport by using stable client_id/global fallback. - Fix Nagle algorithm latency issues by setting TCP_NODELAY. - Cleanup duplicate logging and imports. - Resolve C# NullReferenceException in EditorWindow health check.
WalkthroughMultiple refactoring and hardening changes across the codebase: defensive null-checks added to the editor window's health verification; middleware instantiation switched to a singleton pattern with thread-safe lazy initialization; transport detection abstracted into a centralized function; session key retrieval exposed as a public method on middleware; debug context expanded with middleware introspection; and logging improvements added to reduce noise and enhance error tracking. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)Server/src/transport/unity_instance_middleware.py (1)
🪛 Ruff (0.14.7)Server/src/transport/unity_instance_middleware.py106-106: Do not catch blind exception: (BLE001) 🔇 Additional comments (5)
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: 1
🧹 Nitpick comments (2)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
217-231: Defensive checks and exception handling look solid; consider a tiny safety/clarity tweakThe added null-checks and try/catch around
VerifyBridgeConnectionAsynccorrectly address the NRE/crash risk fromEditorApplication.delayCallinvoking this after the window orconnectionSectionhas been torn down. Logging viaMcpLog.Warnis consistent with the rest of the file.If you want to be extra explicit and slightly reduce any theoretical race between the null-check and use of the field, you could capture
connectionSectioninto a local first:EditorApplication.delayCall += async () => { var section = connectionSection; if (this == null || section == null) { return; } try { await section.VerifyBridgeConnectionAsync(); } catch (Exception ex) { McpLog.Warn($"Health check verification failed: {ex.Message}"); } };Not critical in the Unity editor single-threaded context, but a small clarity improvement if you touch this again.
Server/src/services/tools/set_active_instance.py (1)
68-73: Minor: Redundantgetattrcheck.The
getattr(inst, "id", None)check on line 69 is redundant sincematchesis only populated frominstanceswhich were already filtered at line 47 to require anid. However, this is defensive and doesn't harm correctness.for inst in instances: - if not getattr(inst, "id", None): - continue inst_hash = getattr(inst, "hash", "") if inst_hash and inst_hash.lower().startswith(lookup): matches.append(inst)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/github-repo-stats.yml(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(1 hunks)Server/src/main.py(3 hunks)Server/src/services/resources/unity_instances.py(4 hunks)Server/src/services/tools/debug_request_context.py(3 hunks)Server/src/services/tools/set_active_instance.py(3 hunks)Server/src/transport/legacy/unity_connection.py(1 hunks)Server/src/transport/unity_instance_middleware.py(3 hunks)Server/src/transport/unity_transport.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
Server/src/services/tools/debug_request_context.pyServer/src/services/tools/set_active_instance.pyServer/src/services/resources/unity_instances.py
🧬 Code graph analysis (6)
Server/src/services/tools/debug_request_context.py (1)
Server/src/transport/unity_instance_middleware.py (2)
get_session_key(50-63)get_active_instance(71-75)
Server/src/main.py (1)
Server/src/transport/unity_instance_middleware.py (2)
UnityInstanceMiddleware(37-119)get_unity_instance_middleware(21-28)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)
Server/src/services/tools/set_active_instance.py (2)
Server/src/transport/unity_transport.py (1)
_current_transport(22-24)Server/src/transport/unity_instance_middleware.py (3)
get_unity_instance_middleware(21-28)set_active_instance(65-69)get_session_key(50-63)
Server/src/services/resources/unity_instances.py (1)
Server/src/transport/unity_transport.py (1)
_current_transport(22-24)
Server/src/transport/unity_instance_middleware.py (1)
Server/src/services/tools/set_active_instance.py (1)
set_active_instance(15-112)
🪛 Ruff (0.14.7)
Server/src/main.py
57-57: Do not catch blind exception: Exception
(BLE001)
60-60: Do not catch blind exception: Exception
(BLE001)
Server/src/transport/unity_instance_middleware.py
103-103: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (17)
.github/workflows/github-repo-stats.yml (1)
4-4: Intentional disabling of scheduled trigger—LGTM.The schedule trigger has been disabled while keeping
workflow_dispatchactive for manual runs. This aligns with the PR's focus on reducing automated runs and relying on explicit manual execution when needed.Also applies to: 7-7
Server/src/transport/unity_transport.py (1)
22-25: LGTM!Clean abstraction that exposes transport mode as a string identifier. This centralizes transport detection and provides a consistent API for other modules that need to branch on transport type.
Server/src/transport/legacy/unity_connection.py (1)
63-66: LGTM!Good documentation of the current trust model for
config.unity_host. The note about future OS resolver behavior consideration is helpful for maintainability.Server/src/services/resources/unity_instances.py (1)
39-40: LGTM!Clean refactor to use centralized
_current_transport(). Storing the result in a local variable and using it in both the conditional and response payload ensures consistency and avoids redundant function calls.Also applies to: 75-75, 102-102
Server/src/services/tools/debug_request_context.py (2)
41-47: Debug-only exposure of middleware internals is acceptable.Accessing
middleware._active_by_keyunder lock is thread-safe, and exposingall_keys_in_store,plugin_hub_configured, andmiddleware_idis valuable for debugging session persistence issues. Since this is explicitly a debug endpoint, the internal detail exposure is appropriate.
39-39: Good: Using public API instead of private method.The change from
middleware._get_session_key(ctx)tomiddleware.get_session_key(ctx)correctly uses the public API as indicated in the relevant code snippets fromunity_instance_middleware.py.Server/src/services/tools/set_active_instance.py (3)
19-22: LGTM!Clean refactor to use centralized
_current_transport()for transport detection, consistent with the pattern in other files.
57-89: Robust instance resolution with clear error messages.The two-path resolution logic (exact
Name@hashmatch vs. hash prefix matching) with explicit ambiguity handling is well-structured. Error messages guide users tounity://instancesfor correct identifiers.
99-111: LGTM!Including
session_keyin the response is helpful for debugging session persistence issues, which aligns with the PR objective of fixing stdio instance switching.Server/src/main.py (4)
50-50: Good fix for double-logging issue.Setting
propagate = Falseon both the main logger and telemetry logger prevents log messages from bubbling up to the root logger when a file handler is attached, addressing the duplicate logging mentioned in PR objectives.Also applies to: 56-56
57-62: Improved exception handling during logging setup.Replacing bare
passwithlogger.debug(..., exc_info=exc)preserves diagnostic information while maintaining startup robustness. The broadExceptioncatches are intentional here to ensure logging configuration issues never break server startup.
64-68: Good addition of noisy logger suppression.Adding
"mcp.server.lowlevel.server"to the noisy logger list and settingpropagate = Falsehelps reduce clutter during stdio handshake, improving startup log readability.
267-269: LGTM! Correct use of singleton accessor.Using
get_unity_instance_middleware()instead of direct instantiation ensures the same middleware instance is used across the codebase, which is essential for session persistence. This aligns with the changes inunity_instance_middleware.pywhere the singleton pattern was introduced.Server/src/transport/unity_instance_middleware.py (4)
8-14: LGTM: Logging setup is appropriate.The logging configuration follows standard Python practices and will help with debugging session persistence issues.
67-81: LGTM: Consistent use of the renamed session key method.All call sites have been correctly updated to use
get_session_key()with proper locking and session key derivation.
50-63: Code intent is already clear in documentation; no verification needed.The
"global"fallback and single-user local mode assumption are explicitly documented in the method's docstring (lines 52–58) and inline comment (line 62). This is intentional design for local development, not a hidden constraint. The code is sufficiently clear about its scope and limitations.
83-119: Consider more specific exception handling if possible.The defensive approach of not clearing
active_instanceon PluginHub resolution failure (lines 103-114) is reasonable for stdio transport compatibility. However, catching blindException(line 103) is flagged by static analysis and should be more specific if feasible.To determine the appropriate exception types, inspect
PluginHub._resolve_session_id()to identify what it explicitly raises. If specific exceptions can be identified (e.g., custom PluginHub exceptions,ConnectionError, etc.), replace the broadExceptionhandler with those types. If the method can raise unpredictable exceptions or if broad handling is intentional for resilience, document this rationale as a comment to suppress static analysis warnings if necessary.
1 participant
Stdio was having problems switching instances. Should be fixed now.
_resolve_session_idexpected to be called in STDIO mode? #419Summary by CodeRabbit
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.