Fix/577 performance optimizations #131
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…playDev#577) Eliminate memory allocations that occurred every frame, which triggered garbage collection spikes (~28ms) approximately every second. Changes: - EditorStateCache: Skip BuildSnapshot() entirely when state unchanged (check BEFORE building). Increased poll interval from 0.25s to 1.0s. Cache DeepClone() results to avoid allocations on GetSnapshot(). - TransportCommandDispatcher: Early exit before lock/list allocation when Pending.Count == 0, eliminating per-frame allocations when idle. - StdioBridgeHost: Same early exit pattern for commandQueue. - MCPForUnityEditorWindow: Throttle OnEditorUpdate to 2-second intervals instead of every frame, preventing expensive socket checks 60+/sec. Fixes GitHub issue CoplayDev#577: High performance impact even when MCP server is off
…CoplayDev#577) Root Cause: - send_command() had a hardcoded retry loop (min 6 attempts) on connection errors - Each retry resent the refresh_unity command, causing Unity to reload 6 times - retry_on_reload=False only controlled reload-state retries, not connection retries The Fix: 1. Unity C# (MCPForUnity/Editor): - Added --reinstall flag to uvx commands in dev mode - Ensures local development changes are picked up by uvx/Claude Code - Applies to all client configurators (Claude Code, Codex, etc.) 2. Python Server (Server/src): - Added max_attempts parameter to send_command() - Pass max_attempts=0 when retry_on_reload=False - Fixed type handling in refresh_unity.py (handle MCPResponse objects) - Added timeout to connection error recovery conditions - Recovery logic now returns success instead of error to prevent client retries Changes: - MCPForUnity/Editor: Added --reinstall to dev mode uvx commands - Server/refresh_unity.py: Fixed type handling, improved error recovery - Server/unity_connection.py: Added max_attempts param, disable retries when retry_on_reload=False Result: refresh_unity with compile=request now triggers only 1 domain reload instead of 6 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Unity Editor (C#): - Fix "Resuming..." stuck state when manually clicking End Session Clear ResumeStdioAfterReload and ResumeHttpAfterReload flags in OnConnectionToggleClicked and EndOrphanedSessionAsync to prevent UI from getting stuck showing "Resuming..." with disabled button - Remove unsupported --reinstall flag from all uvx command builders uvx does not support --reinstall and shows warning when used Use --no-cache --refresh instead for dev mode cache busting Python Server: - Add "aborted" to connection error patterns in refresh_unity Handle WinError 10053 (connection aborted) gracefully during Unity domain reload, treating it as expected behavior - Add WindowsSafeRotatingFileHandler to suppress log rotation errors Windows file locking prevents log rotation when file is open by another process; catch PermissionError to avoid noisy stack traces - Fix packaging: add py-modules = ["main"] to pyproject.toml setuptools.packages.find only discovers packages (directories with __init__.py), must explicitly list standalone module files Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
WalkthroughSwitches Claude Code configurator to a CLI-based configurator with thread-safe, main-thread-captured async configuration; updates migration and retry behavior for stdio/HTTP and reload cases; adds editor throttling and early-exit queue guards; improves refresh recovery and Windows-safe log rollover; small docs/comments updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Editor UI
participant Main as Main Thread
participant Bg as Background Task
participant CLI as Claude CLI Configurator
participant Cache as Status / EditorPrefs
UI->>Main: OnConfigureClicked()
Main->>Main: Capture projectDir, claudePath, httpUrl, uvxPath, gitUrl, packageName, flags
Main->>UI: Show "Checking..." (custom message)
Main->>Bg: Start ConfigureClaudeCliAsync()
Bg->>CLI: ConfigureWithCapturedValues(...) / UnregisterWithCapturedValues(...)
CLI->>CLI: Build args, remove legacy regs, run Claude CLI, parse output
CLI->>Cache: Read/write registration info
CLI-->>Bg: Success / Error
Bg->>Main: Invoke UI update (refresh status without recheck)
Main->>UI: Update status UI / Show error if any
sequenceDiagram
participant Caller as refresh_unity caller
participant Retry as send_command_with_retry
participant Conn as UnityConnection
participant Unity as Unity Editor
participant State as EditorState (ready checks)
Caller->>Retry: send_command_with_retry(command, retry_on_reload=true)
Retry->>Conn: send_command(..., max_attempts)
Conn->>Unity: Send command
alt Normal response
Unity-->>Conn: Response
Conn-->>Retry: Success
Retry-->>Caller: Return success
else Disconnect / domain reload
Unity->>Unity: Domain reload / disconnected
Conn-->>Retry: Error (disconnected)
Retry->>Caller: Mark recoverable / hint retry
Caller->>State: Poll until ready or timeout
alt Ready before timeout
Caller->>Retry: Retry send_command
Retry->>Conn: Resend
Conn-->>Caller: Success
else Timeout
Retry-->>Caller: Return timeout failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Greptile SummaryThis PR implements comprehensive performance optimizations addressing issue CoplayDev#577 with three main improvements: Performance Optimizations:
Additional Improvements:
The changes are well-structured with clear comments explaining the performance rationale. The optimizations target the root causes: unnecessary allocations, retry loops, and blocking UI operations. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Claude Code
participant Server as Python MCP Server
participant Unity as Unity Editor
Note over Unity: Performance Issue: GC spikes every frame
Client->>Server: refresh_unity(compile="request")
Server->>Unity: send_command("refresh_unity")
Note over Server: retry_on_reload=False (NEW)
Unity->>Unity: Trigger compilation
Unity--xServer: Connection closed (domain reload)
Note over Server: Treats connection loss as SUCCESS<br/>(prevents retry loop)
Server->>Client: Success (recovered_from_disconnect)
Note over Unity: Domain reload completes
loop Wait for ready (if enabled)
Server->>Unity: get_editor_state()
Unity->>Server: editor state
alt ready_for_tools == true
Note over Server: Exit wait loop
else blocking_reasons not in real_blocking_reasons
Note over Server: Exit wait loop (not actually busy)
else still busy
Note over Server: Sleep 0.25s and retry
end
end
Note over Unity: Performance Fix: Reduced GC allocations
loop Editor Update Loop (every frame)
Unity->>Unity: OnUpdate() called
alt Less than 1s since last update
Note over Unity: Skip update (was 0.25s)
else Time for update
Unity->>Unity: Check state variables<br/>(scene, focus, play mode, etc.)
alt No state changes detected
Note over Unity: Skip expensive BuildSnapshot()<br/>(KEY OPTIMIZATION)
else State changed
Unity->>Unity: BuildSnapshot()
Unity->>Unity: Update tracked state
end
end
end
Client->>Server: Poll editor state
Server->>Unity: GetSnapshot()
Note over Unity: Return cached clone<br/>(avoids DeepClone allocations)
Unity->>Server: Cached JObject
Server->>Client: Editor state
Note over Client,Unity: UI Stability Improvements
Client->>Unity: Configure Claude CLI (async)
Note over Unity: Capture main-thread values
Unity->>Unity: Task.Run (background thread)
Unity->>Unity: ConfigureWithCapturedValues()
Note over Unity: No main thread blocking
Unity->>Unity: EditorApplication.delayCall
Unity->>Client: Configuration complete
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
221-246: Avoid lock-free reads ofPending.Count.
Pendingis aDictionary<string, PendingCommand>mutated underPendingLock. ReadingDictionary.Countwithout holding the lock while background threads may modify the dictionary is not thread-safe—this can cause incorrect values, runtime corruption, or exceptions. The code's comment claiming the race is "harmless" does not align with C# thread-safety guarantees forDictionary.Consider removing the lock-free check (the inner check at line 243 already prevents allocations) or switch to
ConcurrentDictionary<string, PendingCommand>which provides thread-safeCountandIsEmptyproperties.🛠️ Minimal safe fix (remove lock-free check)
- // Early exit BEFORE acquiring lock or allocating. This prevents the per-frame - // List allocations that trigger GC every ~1 second (GitHub issue `#577`). - // This check is safe because Pending.Count is only modified under PendingLock, - // and a brief race (seeing 0 when a command was just added) is harmless since - // RequestMainThreadPump() will call ProcessQueue() again immediately. - if (Pending.Count == 0) - { - return; - } if (Interlocked.Exchange(ref _processingFlag, 1) == 1)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
394-466: DefaultattemptAutoRewrite = truecreates design risk despite current safeguardsWhile
CheckStatusWithProjectDiris properly documented as thread-safe and the current only background call site explicitly passesattemptAutoRewrite: falseto avoid calling main-thread-onlyConfigure(), the default parameter value is a risky API design. Future developers might bypass this safeguard without noticing the threading constraint. Consider changing the default toattemptAutoRewrite = falseor renaming the background-safe variant to make the threading contract more explicit at the call site.
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Around line 821-835: Remove the unsafe lock-free read of commandQueue.Count:
delete the outer if (commandQueue.Count == 0) early-return and rely on the inner
double-check inside the lock on lockObj that already prevents allocations;
alternatively, if you intend a lock-free check use a thread-safe structure such
as ConcurrentDictionary and call its IsEmpty, but do not read commandQueue.Count
outside the lock. Locate the code in StdioBridgeHost where commandQueue
(Dictionary<string, QueuedCommand>) and lockObj are used to populate
List<(string id, QueuedCommand)>, and remove or replace the outer check to
ensure all reads of commandQueue.Count occur under lockObj or via a thread-safe
API.
In `@Server/src/main.py`:
- Around line 42-52: The current doRollover in WindowsSafeRotatingFileHandler
silently swallows PermissionError after calling super().doRollover(), which can
leave self.stream closed and the handler unusable; change the handler to catch
PermissionError around the file-operation portion and, when PermissionError
occurs, ensure the handler's stream is restored before returning (e.g., if
self.stream is missing or closed, reopen the log file into self.stream using
self.baseFilename and the handler's mode/encoding/settings so future emit()
calls continue to work), and re-raise other unexpected exceptions instead of
suppressing them; reference WindowsSafeRotatingFileHandler.doRollover,
RotatingFileHandler, self.stream and self.baseFilename when making the fix.
In `@Server/src/transport/legacy/unity_connection.py`:
- Around line 236-249: Update the type annotations in send_command: change the
params type from dict[str, Any] to dict[str, Any] | None and update the return
annotation to reflect it can return either a raw dict or an MCPResponse (e.g.,
dict[str, Any] | MCPResponse or a suitable union/alias used in the codebase).
Adjust any imports/typing aliases if needed and ensure references to MCPResponse
in the function (return statements around the early params None guard and later
return points) are compatible with the new return annotation.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
676-682: Consider extracting resume-flag clearing into a helper to avoid duplication.Both blocks do the same work; a small helper improves maintainability and keeps any future logging consistent.
♻️ Proposed refactor
+ private static void ClearResumeFlags() + { + try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); } catch { } + try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); } catch { } + } ... - try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); } catch { } - try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); } catch { } + ClearResumeFlags(); ... - try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); } catch { } - try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); } catch { } + ClearResumeFlags();Also applies to: 724-730
Server/src/services/tools/refresh_unity.py (1)
60-61: Consider explicit success check instead of defaulting to True.Using
response_dict.get("success", True)means a response without asuccesskey is treated as successful. While this may be intentional for backward compatibility, it could silently skip error handling for malformed responses.Additionally, the
response.__dict__fallback on line 60 may include internal Python attributes ifresponseis neither a dict nor hasmodel_dump(). Consider using a more explicit conversion or raising an error for unexpected response types.💡 Suggested improvement
- response_dict = response if isinstance(response, dict) else (response.model_dump() if hasattr(response, "model_dump") else response.__dict__) - if not response_dict.get("success", True): + if isinstance(response, dict): + response_dict = response + elif hasattr(response, "model_dump"): + response_dict = response.model_dump() + else: + # Unexpected response type - treat as error + return MCPResponse(success=False, error=f"Unexpected response type: {type(response).__name__}") + + if not response_dict.get("success", False):
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+42
to
+52
| class WindowsSafeRotatingFileHandler(RotatingFileHandler): | ||
| """RotatingFileHandler that gracefully handles Windows file locking during rotation.""" | ||
|
|
||
| def doRollover(self): | ||
| """Override to catch PermissionError on Windows when log file is locked.""" | ||
| try: | ||
| super().doRollover() | ||
| except PermissionError: | ||
| # On Windows, another process may have the log file open. | ||
| # Skip rotation this time - we'll try again on the next rollover. | ||
| pass |
There was a problem hiding this comment.
Silent suppression may leave handler in broken state.
The base doRollover() closes the stream before attempting file renames. If PermissionError occurs mid-rollover, the stream may already be closed, leaving subsequent writes to fail silently or raise exceptions.
Consider a more defensive approach that ensures the stream remains usable:
Proposed fix
class WindowsSafeRotatingFileHandler(RotatingFileHandler):
"""RotatingFileHandler that gracefully handles Windows file locking during rotation."""
def doRollover(self):
"""Override to catch PermissionError on Windows when log file is locked."""
try:
super().doRollover()
except PermissionError:
# On Windows, another process may have the log file open.
- # Skip rotation this time - we'll try again on the next rollover.
- pass
+ # Skip rotation this time - we'll try again on the next rollover.
+ # Ensure stream is still open for continued logging.
+ if self.stream is None or self.stream.closed:
+ self.stream = self._open()📝 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.
Suggested change
| class WindowsSafeRotatingFileHandler(RotatingFileHandler): | |
| """RotatingFileHandler that gracefully handles Windows file locking during rotation.""" | |
| def doRollover(self): | |
| """Override to catch PermissionError on Windows when log file is locked.""" | |
| try: | |
| super().doRollover() | |
| except PermissionError: | |
| # On Windows, another process may have the log file open. | |
| # Skip rotation this time - we'll try again on the next rollover. | |
| pass | |
| class WindowsSafeRotatingFileHandler(RotatingFileHandler): | |
| """RotatingFileHandler that gracefully handles Windows file locking during rotation.""" | |
| def doRollover(self): | |
| """Override to catch PermissionError on Windows when log file is locked.""" | |
| try: | |
| super().doRollover() | |
| except PermissionError: | |
| # On Windows, another process may have the log file open. | |
| # Skip rotation this time - we'll try again on the next rollover. | |
| # Ensure stream is still open for continued logging. | |
| if self.stream is None or self.stream.closed: | |
| self.stream = self._open() |
🤖 Prompt for AI Agents
In `@Server/src/main.py` around lines 42 - 52, The current doRollover in
WindowsSafeRotatingFileHandler silently swallows PermissionError after calling
super().doRollover(), which can leave self.stream closed and the handler
unusable; change the handler to catch PermissionError around the file-operation
portion and, when PermissionError occurs, ensure the handler's stream is
restored before returning (e.g., if self.stream is missing or closed, reopen the
log file into self.stream using self.baseFilename and the handler's
mode/encoding/settings so future emit() calls continue to work), and re-raise
other unexpected exceptions instead of suppressing them; reference
WindowsSafeRotatingFileHandler.doRollover, RotatingFileHandler, self.stream and
self.baseFilename when making the fix.
Comment on lines
+236
to
+249
| def send_command(self, command_type: str, params: dict[str, Any] = None, max_attempts: int | None = None) -> dict[str, Any]: | ||
| """Send a command with retry/backoff and port rediscovery. Pings only when requested. | ||
| Args: | ||
| command_type: The Unity command to send | ||
| params: Command parameters | ||
| max_attempts: Maximum retry attempts (None = use config default, 0 = no retries) | ||
| """ | ||
| # Defensive guard: catch empty/placeholder invocations early | ||
| if not command_type: | ||
| raise ValueError("MCP call missing command_type") | ||
| if params is None: | ||
| return MCPResponse(success=False, error="MCP call received with no parameters (client placeholder?)") | ||
| attempts = max(config.max_retries, 5) | ||
| attempts = max(config.max_retries, 5) if max_attempts is None else max_attempts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Server/src/transport/legacy/unity_connection.py | sed -n '230,260p'Repository: dsarno/unity-mcp
Length of output: 1787
🏁 Script executed:
rg "class MCPResponse" --type py -A 5Repository: dsarno/unity-mcp
Length of output: 429
🏁 Script executed:
cat -n Server/src/transport/legacy/unity_connection.py | sed -n '236,350p'Repository: dsarno/unity-mcp
Length of output: 6264
🏁 Script executed:
cat -n Server/src/transport/legacy/unity_connection.py | sed -n '350,420p'Repository: dsarno/unity-mcp
Length of output: 4065
🏁 Script executed:
cat -n Server/src/transport/legacy/unity_connection.py | sed -n '420,460p'Repository: dsarno/unity-mcp
Length of output: 1841
Make params explicitly optional in the type hint.
params defaults to None, so the annotation should be dict[str, Any] | None to satisfy PEP 484 and silence RUF013. Additionally, the function's return type annotation (dict[str, Any]) is inconsistent with actual return statements on lines 248 and 297–301, which return MCPResponse objects. The return type should be updated to reflect both dict[str, Any] and MCPResponse (or a union thereof).
🔧 Proposed fix for params
-def send_command(self, command_type: str, params: dict[str, Any] = None, max_attempts: int | None = None) -> dict[str, Any]:
+def send_command(self, command_type: str, params: dict[str, Any] | None = None, max_attempts: int | None = None) -> dict[str, Any]:📝 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.
Suggested change
| def send_command(self, command_type: str, params: dict[str, Any] = None, max_attempts: int | None = None) -> dict[str, Any]: | |
| """Send a command with retry/backoff and port rediscovery. Pings only when requested. | |
| Args: | |
| command_type: The Unity command to send | |
| params: Command parameters | |
| max_attempts: Maximum retry attempts (None = use config default, 0 = no retries) | |
| """ | |
| # Defensive guard: catch empty/placeholder invocations early | |
| if not command_type: | |
| raise ValueError("MCP call missing command_type") | |
| if params is None: | |
| return MCPResponse(success=False, error="MCP call received with no parameters (client placeholder?)") | |
| attempts = max(config.max_retries, 5) | |
| attempts = max(config.max_retries, 5) if max_attempts is None else max_attempts | |
| def send_command(self, command_type: str, params: dict[str, Any] | None = None, max_attempts: int | None = None) -> dict[str, Any]: | |
| """Send a command with retry/backoff and port rediscovery. Pings only when requested. | |
| Args: | |
| command_type: The Unity command to send | |
| params: Command parameters | |
| max_attempts: Maximum retry attempts (None = use config default, 0 = no retries) | |
| """ | |
| # Defensive guard: catch empty/placeholder invocations early | |
| if not command_type: | |
| raise ValueError("MCP call missing command_type") | |
| if params is None: | |
| return MCPResponse(success=False, error="MCP call received with no parameters (client placeholder?)") | |
| attempts = max(config.max_retries, 5) if max_attempts is None else max_attempts |
🧰 Tools
🪛 Ruff (0.14.13)
236-236: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
246-246: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@Server/src/transport/legacy/unity_connection.py` around lines 236 - 249,
Update the type annotations in send_command: change the params type from
dict[str, Any] to dict[str, Any] | None and update the return annotation to
reflect it can return either a raw dict or an MCPResponse (e.g., dict[str, Any]
| MCPResponse or a suitable union/alias used in the codebase). Adjust any
imports/typing aliases if needed and ensure references to MCPResponse in the
function (return statements around the early params None guard and later return
points) are compatible with the new return annotation.
Add detailed comments and logging to clarify why connection loss during compile is treated as success (expected domain reload behavior, not failure). This addresses PR feedback about potentially masking real connection errors. The logic is intentional and correct: - Connection loss only treated as success when compile='request' - Domain reload causing disconnect is expected Unity behavior - Subsequent wait_for_ready loop validates Unity becomes ready - Prevents multiple domain reload loops (issue CoplayDev#577) Added logging for observability: - Info log when expected disconnect detected - Warning log for non-recoverable errors Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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 (1)
Server/src/services/tools/refresh_unity.py (1)
1-16: Missingloggerimport causes runtimeNameError.The static analysis correctly identified that
loggeris used on lines 82 and 91 but is never imported or defined. This will cause aNameErrorat runtime when the connection-loss handling paths are triggered.🐛 Proposed fix: Add logger import
from __future__ import annotations import asyncio +import logging import time from typing import Annotated, Any, Literal from fastmcp import Context from mcp.types import ToolAnnotations from models import MCPResponse from services.registry import mcp_for_unity_tool from services.tools import get_unity_instance_from_context import transport.unity_transport as unity_transport from transport.legacy.unity_connection import async_send_command_with_retry, _extract_response_reason from services.state.external_changes_scanner import external_changes_scanner import services.resources.editor_state as editor_state + +logger = logging.getLogger(__name__)
Missing logger import causes NameError at runtime when connection loss handling paths are triggered (lines 82 and 91). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/refresh_unity.py (1)
103-122: Avoid exiting the wait loop on unknown blocking reasons.
if not (blocking & real_blocking_reasons)breaks even when Unity reports new busy states not in the set (e.g., future blocking reasons). That can return before readiness is restored. Consider only bypassing when blocking reasons are empty or juststale_status.🔧 Suggested guard
- blocking = set(advice.get("blocking_reasons") or []) - if not (blocking & real_blocking_reasons): - break + blocking = set(advice.get("blocking_reasons") or []) + non_blocking = {"stale_status"} + if not blocking or blocking <= non_blocking: + break
🤖 Fix all issues with AI agents
In `@Server/src/services/tools/refresh_unity.py`:
- Line 140: The function's annotated return type still says "MCPResponse |
dict[str, Any]" but the implementation always converts dict responses into an
MCPResponse (return MCPResponse(**response_dict) if isinstance(response, dict)
else response), so update the function signature to "-> MCPResponse" to match
behavior (replace the union return annotation). Ensure the annotation change
references MCPResponse and leave the body intact; follow the same pattern used
in execute_menu_item.py for consistency.
| ) | ||
|
|
||
| return MCPResponse(**response) if isinstance(response, dict) else response | ||
| return MCPResponse(**response_dict) if isinstance(response, dict) else response |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the context around line 140
cd Server/src/services/tools && wc -l refresh_unity.pyRepository: dsarno/unity-mcp
Length of output: 80
🏁 Script executed:
# Read the file around line 140 to find the function and understand context
cd Server/src/services/tools && sed -n '115,155p' refresh_unity.pyRepository: dsarno/unity-mcp
Length of output: 1217
🏁 Script executed:
# Find the complete function definition - look for def statements before line 140
cd Server/src/services/tools && sed -n '1,140p' refresh_unity.py | tail -50Repository: dsarno/unity-mcp
Length of output: 2417
🏁 Script executed:
# Search for all call sites of refresh_unity function in the repository
rg -nP --type=py 'refresh_unity\s*\(' --max-count=20Repository: dsarno/unity-mcp
Length of output: 578
🏁 Script executed:
# Also check for any type imports and the MCPResponse class definition
rg -nP --type=py 'MCPResponse|from.*import.*MCPResponse' --max-count=20Repository: dsarno/unity-mcp
Length of output: 11011
🏁 Script executed:
# Check what happens with the response in test_refresh_unity_retry_recovery.py
sed -n '30,40p' Server/tests/integration/test_refresh_unity_retry_recovery.pyRepository: dsarno/unity-mcp
Length of output: 567
🏁 Script executed:
# Check what happens with the response in preflight.py
sed -n '65,80p' Server/src/services/tools/preflight.pyRepository: dsarno/unity-mcp
Length of output: 809
🏁 Script executed:
# Get the full function signature and return statement context in refresh_unity.py
sed -n '29,45p' Server/src/services/tools/refresh_unity.pyRepository: dsarno/unity-mcp
Length of output: 793
🏁 Script executed:
# Also check all return statements in the function to understand what types can be returned
rg -nP 'return' Server/src/services/tools/refresh_unity.py | head -20Repository: dsarno/unity-mcp
Length of output: 485
Update return type annotation to match implementation.
The function now always returns MCPResponse (line 140 wraps dict responses), but the annotation at line 38 still declares MCPResponse | dict[str, Any]. This creates a contract violation—type checkers and callers relying on the annotation will expect dict responses that will never arrive.
Either update the annotation to -> MCPResponse: (matching the actual behavior) or revert line 140 to preserve the prior dict return for backward compatibility. For consistency with the rest of the function and similar patterns elsewhere (e.g., execute_menu_item.py), updating the annotation is the correct fix.
🤖 Prompt for AI Agents
In `@Server/src/services/tools/refresh_unity.py` at line 140, The function's
annotated return type still says "MCPResponse | dict[str, Any]" but the
implementation always converts dict responses into an MCPResponse (return
MCPResponse(**response_dict) if isinstance(response, dict) else response), so
update the function signature to "-> MCPResponse" to match behavior (replace the
union return annotation). Ensure the annotation change references MCPResponse
and leave the body intact; follow the same pattern used in execute_menu_item.py
for consistency.
Addresses four issues raised in code review:
1. EditorStateCache.GetSnapshot() - Remove shared cached clone
- Revert to always returning fresh DeepClone() to prevent mutation bugs
- Main GC optimization remains: state-change detection prevents
unnecessary _cached rebuilds (the expensive operation)
- Removed _cachedClone and _cachedCloneSequence fields
2. refresh_unity.py - Fix blocking reason terminology mismatch
- Changed "asset_refresh" to "asset_import" to match activityPhase
values from EditorStateCache.cs
- Ensures asset import is correctly detected as blocking state
3. TransportCommandDispatcher - Fix unsynchronized Count access
- Moved Pending.Count check inside PendingLock
- Prevents data races and InvalidOperationException from concurrent
dictionary access
4. StdioBridgeHost - Fix unsynchronized Count access
- Moved commandQueue.Count check inside lockObj
- Ensures all collection access is properly serialized
All changes maintain the GC allocation optimizations while fixing
thread safety violations and semantic contract changes.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- refresh_unity.py: Track readiness explicitly and return failure on timeout instead of silently returning success when wait loop exits without confirming ready_for_tools=true - McpClientConfiguratorBase.cs: Add thread safety guard for Configure() call in CheckStatusWithProjectDir(). Changed default attemptAutoRewrite to false and added runtime check to prevent calling Configure() from background threads. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/refresh_unity.py (1)
146-151: Avoid asserting readiness whenwait_for_ready=False.
recovered_from_disconnectcan be true even when readiness wasn’t checked, but the response message claims “editor is ready.” Consider tailoring the message (or payload) based onwait_for_ready/ready_confirmed.💡 Suggested tweak
- if recovered_from_disconnect: - return MCPResponse( - success=True, - message="Refresh recovered after Unity disconnect/retry; editor is ready.", - data={"recovered_from_disconnect": True}, - ) + if recovered_from_disconnect: + message = ( + "Refresh recovered after Unity disconnect/retry; editor is ready." + if wait_for_ready + else "Refresh recovered after Unity disconnect/retry; editor readiness not confirmed." + ) + return MCPResponse( + success=True, + message=message, + data={"recovered_from_disconnect": True, "ready_confirmed": bool(wait_for_ready)}, + )
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 526-576: The RegisterWithCapturedValues method currently only
validates claudePath; add fast-fail guard clauses at the start of
RegisterWithCapturedValues to validate projectDir is not null/empty and,
depending on useHttpTransport, that httpUrl (when useHttpTransport==true) or
uvxPath, gitUrl and packageName (when useHttpTransport==false) are not
null/empty, throwing InvalidOperationException with clear messages; similarly
add a simple guard in UnregisterWithCapturedValues to validate projectDir (and
claudePath already checked) before calling ExecPath.TryRun so failures surface
with explicit diagnostics instead of cryptic CLI errors—place these checks
before building args and before any ExecPath.TryRun calls in the respective
methods.
- Around line 436-440: The current main-thread check in
CheckStatusWithProjectDir uses a hardcoded ManagedThreadId == 1 which is
unreliable; change it to compare against a captured main-thread ID stored at
startup (e.g. a static readonly int MainThreadId or similar) instead of the
literal 1, and update the local isMainThread logic (where attemptAutoRewrite and
Configure() are used) to compare
System.Threading.Thread.CurrentThread.ManagedThreadId == MainThreadId; follow
the same capture pattern used in TransportCommandDispatcher.cs /
StdioBridgeHost.cs so auto-rewrite isn't falsely disabled across domain reloads.
In `@Server/src/services/tools/refresh_unity.py`:
- Around line 116-125: The current readiness logic treats any unknown
blocking_reasons as non-blocking because it checks if blocking &
real_blocking_reasons is empty; change this so unknown reasons are considered
blocking and only explicitly known non-blocking conditions short-circuit. Update
the block using the existing names (advice, blocking_reasons, blocking,
real_blocking_reasons, ready_confirmed) to compute blocking =
set(advice.get("blocking_reasons") or []) and then set ready_confirmed True only
when blocking is empty or when blocking is exactly the known non-blocking set
(e.g., {"stale_status"}) — do not treat unknown entries as non-blocking.
♻️ Duplicate comments (1)
Server/src/services/tools/refresh_unity.py (1)
153-153: Return type annotation still looks inconsistent with behavior.The implementation now always returns an
MCPResponse, but the signature still advertisesMCPResponse | dict[str, Any](see Line 38). Consider tightening the annotation to match the current behavior.
Comment on lines
+436
to
+440
| // Configure() requires main thread (accesses EditorPrefs, Application.dataPath) | ||
| // Only attempt auto-rewrite if we're on the main thread | ||
| bool isMainThread = System.Threading.Thread.CurrentThread.ManagedThreadId == 1; | ||
| if (attemptAutoRewrite && isMainThread) | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "IsMainThread|CurrentThreadIsMainThread|SynchronizationContext|MainThread" -g '*.cs'Repository: dsarno/unity-mcp
Length of output: 2079
🏁 Script executed:
# Also search for ManagedThreadId usage to see if this pattern is used elsewhere
rg -n "ManagedThreadId" -g '*.cs'Repository: dsarno/unity-mcp
Length of output: 1037
🏁 Script executed:
# Look at the broader context of the file to understand how threading is handled
fd "McpClientConfiguratorBase.cs" -g '*.cs' --exec head -n 50 {}Repository: dsarno/unity-mcp
Length of output: 151
🏁 Script executed:
cat -n MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs | head -100Repository: dsarno/unity-mcp
Length of output: 4346
🏁 Script executed:
cat -n MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs | sed -n '420,460p'Repository: dsarno/unity-mcp
Length of output: 3253
🏁 Script executed:
# Check the class declaration and any existing field initialization
rg -n "class McpClientConfiguratorBase|private|readonly.*Thread|_mainThread" MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs | head -30Repository: dsarno/unity-mcp
Length of output: 464
🏁 Script executed:
rg -n "CheckStatus" MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs -B 2 -A 2Repository: dsarno/unity-mcp
Length of output: 1913
🏁 Script executed:
# Search for callers of CheckStatus to see if it's called from background threads
rg "\.CheckStatus\(" --type cs -B 2 | head -50Repository: dsarno/unity-mcp
Length of output: 1042
🏁 Script executed:
# Look at the broader context - the entire CheckStatusWithProjectDir method
cat -n MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs | sed -n '359,470p'Repository: dsarno/unity-mcp
Length of output: 8052
Replace hardcoded ManagedThreadId comparison with a captured main-thread ID for reliability.
The check at line 438 uses ManagedThreadId == 1, which is not guaranteed across editor sessions or domain reloads. Since CheckStatusWithProjectDir explicitly supports background-thread execution (lines 352–353) but calls Configure() on the main thread (line 449), a false negative here silently disables auto-rewrite. The codebase already demonstrates the correct pattern elsewhere (TransportCommandDispatcher.cs, StdioBridgeHost.cs): capture the main thread ID at initialization and compare against it. Apply the same approach here to ensure reliable detection.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs` around lines 436 -
440, The current main-thread check in CheckStatusWithProjectDir uses a hardcoded
ManagedThreadId == 1 which is unreliable; change it to compare against a
captured main-thread ID stored at startup (e.g. a static readonly int
MainThreadId or similar) instead of the literal 1, and update the local
isMainThread logic (where attemptAutoRewrite and Configure() are used) to
compare System.Threading.Thread.CurrentThread.ManagedThreadId == MainThreadId;
follow the same capture pattern used in TransportCommandDispatcher.cs /
StdioBridgeHost.cs so auto-rewrite isn't falsely disabled across domain reloads.
Comment on lines
+526
to
+576
| private void RegisterWithCapturedValues( | ||
| string projectDir, string claudePath, string pathPrepend, | ||
| bool useHttpTransport, string httpUrl, | ||
| string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh) | ||
| { | ||
| if (string.IsNullOrEmpty(claudePath)) | ||
| { | ||
| throw new InvalidOperationException("Claude CLI not found. Please install Claude Code first."); | ||
| } | ||
|
|
||
| string args; | ||
| if (useHttpTransport) | ||
| { | ||
| args = $"mcp add --transport http UnityMCP {httpUrl}"; | ||
| } | ||
| else | ||
| { | ||
| // Note: --reinstall is not supported by uvx, use --no-cache --refresh instead | ||
| string devFlags = shouldForceRefresh ? "--no-cache --refresh " : string.Empty; | ||
| args = $"mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}--from \"{gitUrl}\" {packageName}"; | ||
| } | ||
|
|
||
| // Remove any existing registrations - handle both "UnityMCP" and "unityMCP" (legacy) | ||
| McpLog.Info("Removing any existing UnityMCP registrations before adding..."); | ||
| ExecPath.TryRun(claudePath, "mcp remove UnityMCP", projectDir, out _, out _, 7000, pathPrepend); | ||
| ExecPath.TryRun(claudePath, "mcp remove unityMCP", projectDir, out _, out _, 7000, pathPrepend); | ||
|
|
||
| // Now add the registration | ||
| if (!ExecPath.TryRun(claudePath, args, projectDir, out var stdout, out var stderr, 15000, pathPrepend)) | ||
| { | ||
| throw new InvalidOperationException($"Failed to register with Claude Code:\n{stderr}\n{stdout}"); | ||
| } | ||
|
|
||
| McpLog.Info($"Successfully registered with Claude Code using {(useHttpTransport ? "HTTP" : "stdio")} transport."); | ||
| client.SetStatus(McpStatus.Configured); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Thread-safe unregistration using pre-captured values. | ||
| /// </summary> | ||
| private void UnregisterWithCapturedValues(string projectDir, string claudePath, string pathPrepend) | ||
| { | ||
| if (string.IsNullOrEmpty(claudePath)) | ||
| { | ||
| throw new InvalidOperationException("Claude CLI not found. Please install Claude Code first."); | ||
| } | ||
|
|
||
| // Remove both "UnityMCP" and "unityMCP" (legacy naming) | ||
| McpLog.Info("Removing all UnityMCP registrations..."); | ||
| ExecPath.TryRun(claudePath, "mcp remove UnityMCP", projectDir, out _, out _, 7000, pathPrepend); | ||
| ExecPath.TryRun(claudePath, "mcp remove unityMCP", projectDir, out _, out _, 7000, pathPrepend); |
There was a problem hiding this comment.
Validate captured values before building CLI args.
Only claudePath is validated; projectDir, httpUrl, and stdio parts can be null/empty, leading to misleading CLI failures or exceptions. Add fast-fail guards for clearer diagnostics.
✅ Suggested guard clauses
private void RegisterWithCapturedValues(
string projectDir, string claudePath, string pathPrepend,
bool useHttpTransport, string httpUrl,
string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh)
{
if (string.IsNullOrEmpty(claudePath))
{
throw new InvalidOperationException("Claude CLI not found. Please install Claude Code first.");
}
+ if (string.IsNullOrEmpty(projectDir))
+ {
+ throw new ArgumentNullException(nameof(projectDir), "Project directory must be provided.");
+ }
+ if (useHttpTransport && string.IsNullOrEmpty(httpUrl))
+ {
+ throw new ArgumentException("HTTP URL must be provided for HTTP transport.", nameof(httpUrl));
+ }
+ if (!useHttpTransport &&
+ (string.IsNullOrEmpty(uvxPath) || string.IsNullOrEmpty(gitUrl) || string.IsNullOrEmpty(packageName)))
+ {
+ throw new ArgumentException("uvxPath, gitUrl, and packageName are required for stdio transport.");
+ }
string args;
if (useHttpTransport)
{
args = $"mcp add --transport http UnityMCP {httpUrl}";
}
else
{
// Note: --reinstall is not supported by uvx, use --no-cache --refresh instead
string devFlags = shouldForceRefresh ? "--no-cache --refresh " : string.Empty;
args = $"mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}--from \"{gitUrl}\" {packageName}";
}
}
private void UnregisterWithCapturedValues(string projectDir, string claudePath, string pathPrepend)
{
if (string.IsNullOrEmpty(claudePath))
{
throw new InvalidOperationException("Claude CLI not found. Please install Claude Code first.");
}
+ if (string.IsNullOrEmpty(projectDir))
+ {
+ throw new ArgumentNullException(nameof(projectDir), "Project directory must be provided.");
+ }🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs` around lines 526 -
576, The RegisterWithCapturedValues method currently only validates claudePath;
add fast-fail guard clauses at the start of RegisterWithCapturedValues to
validate projectDir is not null/empty and, depending on useHttpTransport, that
httpUrl (when useHttpTransport==true) or uvxPath, gitUrl and packageName (when
useHttpTransport==false) are not null/empty, throwing InvalidOperationException
with clear messages; similarly add a simple guard in
UnregisterWithCapturedValues to validate projectDir (and claudePath already
checked) before calling ExecPath.TryRun so failures surface with explicit
diagnostics instead of cryptic CLI errors—place these checks before building
args and before any ExecPath.TryRun calls in the respective methods.
Comment on lines
+116
to
+125
| if isinstance(advice, dict): | ||
| # Exit if ready_for_tools is True | ||
| if advice.get("ready_for_tools") is True: | ||
| ready_confirmed = True | ||
| break | ||
| # Also exit if the only blocking reason is "stale_status" (Unity in background) | ||
| # Staleness means we can't confirm status, not that Unity is actually busy | ||
| blocking = set(advice.get("blocking_reasons") or []) | ||
| if not (blocking & real_blocking_reasons): | ||
| ready_confirmed = True # No real blocking reasons, consider ready |
There was a problem hiding this comment.
Treat unknown blocking reasons as blocking.
Right now any blocking_reasons not in real_blocking_reasons are treated as “ready,” which can allow tools to proceed while Unity is still busy (e.g., new/unknown reasons). Consider only short‑circuiting when the reasons are explicitly non-blocking (e.g., only stale_status).
🛠️ Safer readiness check
- blocking = set(advice.get("blocking_reasons") or [])
- if not (blocking & real_blocking_reasons):
- ready_confirmed = True # No real blocking reasons, consider ready
- break
+ blocking = set(advice.get("blocking_reasons") or [])
+ allowed_nonblocking = {"stale_status"}
+ if blocking and not blocking.issubset(allowed_nonblocking):
+ continue
+ if blocking.issubset(allowed_nonblocking):
+ ready_confirmed = True
+ break🤖 Prompt for AI Agents
In `@Server/src/services/tools/refresh_unity.py` around lines 116 - 125, The
current readiness logic treats any unknown blocking_reasons as non-blocking
because it checks if blocking & real_blocking_reasons is empty; change this so
unknown reasons are considered blocking and only explicitly known non-blocking
conditions short-circuit. Update the block using the existing names (advice,
blocking_reasons, blocking, real_blocking_reasons, ready_confirmed) to compute
blocking = set(advice.get("blocking_reasons") or []) and then set
ready_confirmed True only when blocking is empty or when blocking is exactly the
known non-blocking set (e.g., {"stale_status"}) — do not treat unknown entries
as non-blocking.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.