fix: comprehensive performance optimizations, claude code config, and stability improvements (issue #577) #595
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Contributor
Reviewer's GuideThis PR focuses on eliminating Unity editor GC hitches and domain reload loops while improving Claude Code CLI integration, HTTP/stdio transport consistency, and server stability, including packaging and logging fixes. Sequence diagram for async Claude CLI configuration from Unity editorsequenceDiagram
actor User
participant UnityWindow as MCPForUnityEditorWindow
participant ConfigSection as McpClientConfigSection
participant Configurator as ClaudeCodeConfigurator
participant CliConfigurator as ClaudeCliMcpConfigurator
participant TaskRunner as Task_Run
participant ClaudeCLI as Claude_CLI_Process
User->>UnityWindow: Click Configure button
UnityWindow->>ConfigSection: OnConfigureClicked()
ConfigSection->>Configurator: get Status
alt Configurator is CLI-based
ConfigSection->>ConfigSection: ConfigureClaudeCliAsync(Configurator)
ConfigSection->>ConfigSection: statusRefreshInFlight.Add(Configurator)
ConfigSection->>ConfigSection: ApplyStatusToUi(showChecking=true, customMessage="Registering..." or "Unregistering...")
ConfigSection->>ConfigSection: Capture projectDir, useHttpTransport, claudePath, httpUrl, uvxPath, gitUrl, packageName, shouldForceRefresh, pathPrepend
ConfigSection->>TaskRunner: Task.Run(lambda ...)
activate TaskRunner
TaskRunner->>CliConfigurator: ConfigureWithCapturedValues(projectDir, claudePath, pathPrepend, useHttpTransport, httpUrl, uvxPath, gitUrl, packageName, shouldForceRefresh)
alt client.status == Configured
CliConfigurator->>CliConfigurator: UnregisterWithCapturedValues(...)
CliConfigurator->>ClaudeCLI: claude mcp remove UnityMCP
CliConfigurator->>ClaudeCLI: claude mcp remove unityMCP
CliConfigurator->>CliConfigurator: client.SetStatus(NotConfigured)
else Register
CliConfigurator->>CliConfigurator: RegisterWithCapturedValues(...)
CliConfigurator->>ClaudeCLI: claude mcp remove UnityMCP
CliConfigurator->>ClaudeCLI: claude mcp remove unityMCP
CliConfigurator->>ClaudeCLI: claude mcp add --transport http|stdio UnityMCP ...
alt CLI success
CliConfigurator->>CliConfigurator: client.SetStatus(Configured)
else CLI failure
CliConfigurator-->>TaskRunner: throw InvalidOperationException
end
end
TaskRunner-->>ConfigSection: ContinueWith(result or error)
deactivate TaskRunner
ConfigSection->>ConfigSection: statusRefreshInFlight.Remove(Configurator)
ConfigSection->>ConfigSection: lastStatusChecks.Remove(Configurator)
alt errorMessage != null
ConfigSection->>Configurator: Client.SetStatus(Error, errorMessage)
ConfigSection->>ConfigSection: RefreshClientStatus(forceImmediate=true)
else success
ConfigSection->>ConfigSection: lastStatusChecks[Configurator] = DateTime.UtcNow
ConfigSection->>ConfigSection: ApplyStatusToUi(showChecking=false)
end
ConfigSection->>ConfigSection: UpdateManualConfiguration()
else Non-CLI configurator
ConfigSection->>ConfigSection: synchronous ConfigureClient
end
Sequence diagram for fixing stuck "Resuming..." state on session endsequenceDiagram
actor User
participant ConnSection as McpConnectionSection
participant Bridge as BridgeService
participant EditorPrefs as EditorPrefs
User->>ConnSection: Click End Session toggle
ConnSection->>ConnSection: OnConnectionToggleClicked()
alt bridgeService.IsRunning is True
ConnSection->>EditorPrefs: DeleteKey(ResumeStdioAfterReload)
ConnSection->>EditorPrefs: DeleteKey(ResumeHttpAfterReload)
ConnSection->>Bridge: StopAsync()
Bridge-->>ConnSection: stopped
else starting or not running
ConnSection->>ConnSection: normal start logic
end
ConnSection->>ConnSection: Update UI (Start Session enabled, no "Resuming..." text)
Note over ConnSection,EditorPrefs: EndOrphanedSessionAsync() also clears resume flags before StopAsync() to avoid stuck "Resuming..." state
Class diagram for updated MCP configurators and Claude Code CLI integrationclassDiagram
class IMcpClientConfigurator {
<<interface>>
+McpClient Client
+McpStatus Status
+bool SupportsAutoConfigure
+void Configure()
+void CheckStatus(bool attemptAutoRewrite)
+string GetConfigureActionLabel()
}
class McpClientConfiguratorBase {
+McpClient Client
+McpStatus status
+bool SupportsAutoConfigure
+McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTransport, bool attemptAutoRewrite)
+void Configure()
+void ConfigureWithCapturedValues(string projectDir, string claudePath, string pathPrepend, bool useHttpTransport, string httpUrl, string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh)
+void Register()
+void RegisterWithCapturedValues(string projectDir, string claudePath, string pathPrepend, bool useHttpTransport, string httpUrl, string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh)
+void Unregister()
+void UnregisterWithCapturedValues(string projectDir, string claudePath, string pathPrepend)
+string GetManualSnippet()
+IList~string~ GetInstallationSteps()
+static string ExtractPackageSourceFromCliOutput(string cliOutput)
}
class ClaudeCliMcpConfigurator {
+bool SupportsHttpTransport
+bool SupportsAutoConfigure
+void ConfigureWithCapturedValues(string projectDir, string claudePath, string pathPrepend, bool useHttpTransport, string httpUrl, string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh)
+void RegisterWithCapturedValues(string projectDir, string claudePath, string pathPrepend, bool useHttpTransport, string httpUrl, string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh)
+void UnregisterWithCapturedValues(string projectDir, string claudePath, string pathPrepend)
}
class ClaudeCodeConfigurator {
+ClaudeCodeConfigurator()
+IList~string~ GetInstallationSteps()
}
class JsonFileMcpConfigurator {
+bool SupportsHttpTransport
+bool SupportsAutoConfigure
+void Configure()
+void CheckStatus(bool attemptAutoRewrite)
+string GetManualSnippet()
}
class McpClient {
+string name
+bool SupportsHttpTransport
}
class StdIoVersionMigration {
+static void RunMigrationIfNeeded()
-static bool ConfigUsesStdIo(McpClient client)
}
IMcpClientConfigurator <|.. McpClientConfiguratorBase
McpClientConfiguratorBase <|-- ClaudeCliMcpConfigurator
McpClientConfiguratorBase <|-- JsonFileMcpConfigurator
ClaudeCliMcpConfigurator <|-- ClaudeCodeConfigurator
McpClientConfiguratorBase o-- McpClient
ClaudeCodeConfigurator o-- McpClient
StdIoVersionMigration ..> IMcpClientConfigurator : uses
StdIoVersionMigration ..> ClaudeCliMcpConfigurator : checks and auto-rewrites
class McpClientConfigSection {
-List~IMcpClientConfigurator~ configurators
-Dictionary~IMcpClientConfigurator, DateTime~ lastStatusChecks
-HashSet~IMcpClientConfigurator~ statusRefreshInFlight
-int selectedClientIndex
+void OnConfigureClicked()
-void ConfigureClaudeCliAsync(IMcpClientConfigurator client)
-void ApplyStatusToUi(IMcpClientConfigurator client, bool showChecking, string customMessage)
-void RefreshClientStatus(IMcpClientConfigurator client, bool forceImmediate)
}
McpClientConfigSection ..> IMcpClientConfigurator : configures
McpClientConfigSection ..> ClaudeCliMcpConfigurator : async configure
McpClientConfigSection ..> McpClientConfiguratorBase : updates Client status
Class diagram for optimized EditorStateCache and transport processingclassDiagram
class EditorStateCache {
<<static>>
-JObject _cached
-long _sequence
-long? _domainReloadAfterUnixMs
-double _lastUpdateTimeSinceStartup
-const double MinUpdateIntervalSeconds
-JObject _cachedClone
-long _cachedCloneSequence
-string _lastTrackedScenePath
-string _lastTrackedSceneName
-bool _lastTrackedIsFocused
-bool _lastTrackedIsPlaying
-bool _lastTrackedIsPaused
-bool _lastTrackedIsUpdating
-bool _lastTrackedTestsRunning
-string _lastTrackedActivityPhase
-bool _domainReloadPending
-bool _lastIsCompiling
+void Initialize()
+void ForceUpdate(string reason)
+JObject GetSnapshot()
-void OnUpdate()
-static bool GetActualIsCompiling()
}
class TransportCommandDispatcher {
<<static>>
-Dictionary~string, PendingCommand~ Pending
-object PendingLock
-int _processingFlag
+void Enqueue(string id, PendingCommand command)
-void UnhookUpdateIfIdle()
-void ProcessQueue()
}
class StdioBridgeHost {
<<static>>
-Dictionary~string, QueuedCommand~ commandQueue
-object lockObj
-float nextHeartbeatAt
+void EnqueueCommand(string id, QueuedCommand command)
-void ProcessCommands()
}
class MCPForUnityEditorWindow {
-double _lastEditorUpdateTime
-const double EditorUpdateIntervalSeconds
+void OnEnable()
+void OnDisable()
+void OnFocus()
-void OnEditorUpdate()
}
EditorStateCache ..> EditorApplication : uses
EditorStateCache ..> EditorSceneManager : uses
EditorStateCache ..> InternalEditorUtility : uses
EditorStateCache ..> TestRunStatus : uses
TransportCommandDispatcher ..> PendingCommand : manages
StdioBridgeHost ..> QueuedCommand : manages
MCPForUnityEditorWindow ..> TransportCommandDispatcher : polls
MCPForUnityEditorWindow ..> StdioBridgeHost : polls
class PendingCommand {
+string Id
+object Payload
+bool IsReady()
}
class QueuedCommand {
+string Id
+object Payload
+bool IsReady()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces JSON-file based Claude configurator with a CLI-driven configurator, adds background-safe captured-value registration/unregistration, extends status checks and auto-rewrite logic, and updates UI to run CLI registration asynchronously. Also includes transport, server, and Editor performance/robustness tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditorUI as "Unity Editor UI"
participant BG as "Background Task"
participant ClaudeCLI as "Claude CLI"
participant Registry as "MCP Registry"
User->>EditorUI: Click Configure (Claude)
EditorUI->>EditorUI: Capture main-thread values (projectDir, claudePath, pathPrepend, transport flags, urls, uvx/git/package)
EditorUI->>BG: Start ConfigureClaudeCliAsync with captured values
BG->>ClaudeCLI: Invoke ConfigureWithCapturedValues / RegisterWithCapturedValues
ClaudeCLI->>ClaudeCLI: Inspect existing registration (list/get), extract package source
ClaudeCLI-->>Registry: Run CLI commands to unregister/register (add/remove MCP entry)
Registry-->>ClaudeCLI: Return registration status/output
ClaudeCLI-->>BG: Return success/error and status
BG->>EditorUI: Update UI state (success or error message)
EditorUI->>User: Show final status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new snapshot caching in EditorStateCache.GetSnapshot() now returns a shared JObject instance instead of a fresh DeepClone each time, which changes semantics and allows callers to mutate the cached object; consider either keeping per-call cloning or returning a read-only/deep-cloned view to avoid subtle shared-state bugs.
- In TransportCommandDispatcher.ProcessQueue and StdioBridgeHost.ProcessCommands you read Pending.Count/commandQueue.Count outside the lock, but these collections are otherwise guarded by a lock; using Count without synchronization can lead to race conditions or undefined behavior, so it would be safer to base the early exit on a separate atomic flag or perform the check inside the lock only.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new snapshot caching in EditorStateCache.GetSnapshot() now returns a shared JObject instance instead of a fresh DeepClone each time, which changes semantics and allows callers to mutate the cached object; consider either keeping per-call cloning or returning a read-only/deep-cloned view to avoid subtle shared-state bugs.
- In TransportCommandDispatcher.ProcessQueue and StdioBridgeHost.ProcessCommands you read Pending.Count/commandQueue.Count outside the lock, but these collections are otherwise guarded by a lock; using Count without synchronization can lead to race conditions or undefined behavior, so it would be safer to base the early exit on a separate atomic flag or perform the check inside the lock only.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/EditorStateCache.cs:508-514` </location>
<code_context>
+ // Cache the cloned version to avoid allocating a new JObject on every GetSnapshot() call.
+ // This fixes the GC spikes that occurred when external tools polled editor state frequently.
+ // Only regenerate the clone when _cached changes (detected via sequence number).
+ if (_cachedClone == null || _cachedCloneSequence != _sequence)
+ {
+ _cachedClone = (JObject)_cached.DeepClone();
+ _cachedCloneSequence = _sequence;
+ }
+
+ return _cachedClone;
}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning a shared cached JObject can leak mutations to all callers and break expectations from the previous DeepClone() behavior.
This changes the contract of GetSnapshot(): it no longer returns an isolated clone but a shared instance. Any caller that mutates the returned JObject will now inadvertently modify the cached snapshot seen by others, risking hard‑to‑trace bugs.
To keep allocations down without changing semantics, either:
- Keep `_cached` as the mutable source of truth and always return `DeepClone()` (optimize how often `_cached` itself changes), or
- Treat the cached snapshot as immutable and restrict mutations to a separate builder object, swapping references only when ready.
Unless you can ensure all consumers treat the returned JObject as strictly read‑only, I’d avoid sharing the cached clone and preserve the original cloning behavior.
</issue_to_address>
### Comment 2
<location> `Server/src/services/tools/refresh_unity.py:103-104` </location>
<code_context>
timeout_s = 60.0
start = time.monotonic()
+ # Blocking reasons that indicate Unity is actually busy (not just stale status)
+ real_blocking_reasons = {"compiling", "domain_reload", "running_tests", "asset_refresh"}
+
while time.monotonic() - start < timeout_s:
</code_context>
<issue_to_address>
**issue (bug_risk):** The "asset_refresh" blocking reason may not match the activityPhase values produced by the Unity editor state.
In editor_state, the activityPhase set when EditorApplication.isUpdating is true is "asset_import". If blocking_reasons are based on that value (or share its terminology), "asset_refresh" here may never appear in advice["blocking_reasons"], so asset import would be treated as non‑blocking and the loop could exit too early.
Please confirm the exact strings used for editor_state blocking_reasons. If they’re aligned with activityPhase, this constant should likely be "asset_import" instead of "asset_refresh" so they match.
</issue_to_address>
### Comment 3
<location> `MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs:226-228` </location>
<code_context>
+ // 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;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reading Pending.Count without holding PendingLock introduces a data race and potential InvalidOperationExceptions.
Because Pending is a mutable collection protected by PendingLock everywhere else, reading Pending.Count without that lock is unsafe. If another thread mutates the dictionary while this method is running, you can both miss newly enqueued commands and hit InvalidOperationExceptions from concurrent reads/writes.
You can keep the early return while staying thread‑safe by taking the lock for the count check:
```csharp
lock (PendingLock)
{
if (Pending.Count == 0)
{
return;
}
}
```
This mirrors the later double‑check inside the lock and removes the unsynchronized access.
</issue_to_address>
### Comment 4
<location> `MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs:823-825` </location>
<code_context>
+ // Early exit BEFORE acquiring lock or allocating. This prevents the per-frame
+ // List allocations that trigger GC every ~1 second (GitHub issue #577).
+ if (commandQueue.Count == 0)
+ {
+ return;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** commandQueue.Count is checked without taking lockObj, which is inconsistent with the locking discipline around commandQueue.
If commandQueue is a non-thread-safe collection, reading Count without the lock while other operations are locked can cause runtime exceptions or data races under concurrent access.
You can preserve the optimization by performing the early exit inside the existing lock:
```csharp
List<(string id, QueuedCommand command)> work;
lock (lockObj)
{
if (commandQueue.Count == 0)
{
return;
}
work = new List<(string, QueuedCommand)>(commandQueue.Count);
...
}
```
This keeps all access to commandQueue serialized while still avoiding unnecessary list allocations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
394-472: Transport and version mismatch detection logic is well-structured.The enhanced status checking with support for both "UnityMCP" and legacy "unityMCP" naming, combined with transport mode and package version verification, is a solid improvement. The auto-rewrite behavior with
attemptAutoRewriteis properly guarded.However, there's a potential issue at line 443: calling
Configure()from withinCheckStatusWithProjectDircan lead to main-thread access violations whenCheckStatusWithProjectDiris invoked from a background thread (e.g., viaRefreshClaudeCliStatusinMcpClientConfigSection.cs).Analysis of the thread-safety concern
Configure()internally callsRegister()which accessesEditorPrefsandApplication.dataPath(lines 585, 602), both of which require main-thread execution. IfCheckStatusWithProjectDiris called from a background thread withattemptAutoRewrite: true, this will throw.Current mitigation: In
McpClientConfigSection.cs,RefreshClaudeCliStatuspassesattemptAutoRewrite: false(line 451), which avoids this path. However, this creates a latent bug if anyone callsCheckStatusWithProjectDir(..., attemptAutoRewrite: true)from a background context.Consider either:
- Documenting that
attemptAutoRewrite: truemust only be called from the main thread, or- Using a captured-values variant for the auto-rewrite path similar to
ConfigureWithCapturedValues.Server/src/services/tools/refresh_unity.py (1)
103-122: Track readiness to avoid false “ready” after timeout.If the wait loop times out,
recovered_from_disconnectstill returns success with “editor is ready”. That can mislead callers and trigger tools while Unity is still busy/unavailable. Please track readiness explicitly and return a timeout failure (or a “not confirmed” flag) when the loop exits without a ready signal.🛠️ Proposed fix
- while time.monotonic() - start < timeout_s: + ready_confirmed = False + while time.monotonic() - start < timeout_s: state_resp = await editor_state.get_editor_state(ctx) state = state_resp.model_dump() if hasattr( state_resp, "model_dump") else state_resp data = (state or {}).get("data") if isinstance( state, dict) else None advice = (data or {}).get( "advice") if isinstance(data, dict) else None 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 break await asyncio.sleep(0.25) + + if wait_for_ready and not ready_confirmed: + return MCPResponse( + success=False, + message="Timed out waiting for Unity to become ready.", + hint="retry", + )
🧹 Nitpick comments (5)
Server/src/main.py (1)
42-52: Consider logging the skipped rollover for observability.The implementation correctly handles Windows file locking by catching
PermissionError. However, silently ignoring the exception provides no visibility into repeated rollover failures, which could cause unbounded log growth if the file remains locked.🔧 Suggested improvement: Add debug logging
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 + # Log at debug level to aid troubleshooting without cluttering output + if self.stream: + try: + self.stream.write( + "[WindowsSafeRotatingFileHandler] Rollover skipped due to PermissionError\n" + ) + except Exception: + passAlternatively, you could use
logging.getLogger(__name__).debug(...)but that risks recursion if this handler is attached to that logger.Server/src/transport/legacy/unity_connection.py (1)
236-243: Fix implicitOptionaltype annotation.Per PEP 484 and the static analysis hint, the
paramsparameter should use explicit union syntax rather than implicitOptional.Suggested fix
- 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]:MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
719-759: ExtractPackageSourceFromCliOutput parsing logic is reasonable but could be more robust.The parsing handles both quoted and unquoted values, which is good. However, there are a few edge cases to consider:
- If the CLI output contains
--fromin a comment or unrelated context, this could produce false matches.- The method doesn't handle escaped quotes within quoted strings.
For the current use case (parsing well-structured CLI output), this is likely sufficient.
💡 Optional: Consider using a regex for clearer intent
private static readonly Regex FromArgRegex = new Regex( @"--from\s+(?:""([^""]+)""|'([^']+)'|(\S+))", RegexOptions.IgnoreCase | RegexOptions.Compiled); private static string ExtractPackageSourceFromCliOutput(string cliOutput) { if (string.IsNullOrEmpty(cliOutput)) return null; var match = FromArgRegex.Match(cliOutput); if (!match.Success) return null; return match.Groups[1].Success ? match.Groups[1].Value : match.Groups[2].Success ? match.Groups[2].Value : match.Groups[3].Value; }MCPForUnity/Editor/Services/EditorStateCache.cs (2)
291-340: Effective optimization pattern - early state change detection.The "cheap checks before expensive operation" pattern is well-implemented. The state comparisons are much cheaper than building a full JSON snapshot.
One minor observation: the string comparison for
_lastTrackedScenePath != scenePathuses reference equality. If Unity'sscene.pathreturns a new string instance with the same value, this could trigger unnecessary rebuilds. Consider usingstring.Equals()for value comparison if spurious updates are observed in practice.Optional: Use value comparison for strings
- bool hasChanges = compilationEdge - || _lastTrackedScenePath != scenePath - || _lastTrackedSceneName != sceneName + bool hasChanges = compilationEdge + || !string.Equals(_lastTrackedScenePath, scenePath, StringComparison.Ordinal) + || !string.Equals(_lastTrackedSceneName, sceneName, StringComparison.Ordinal)
303-323: Consider extracting activity phase computation to a helper method.The activity phase determination logic is duplicated here and in
BuildSnapshot()(lines 389-409). While the duplication is intentional for the optimization pattern, extracting it to a shared helper would improve maintainability if the phase logic evolves.Optional: Extract to helper method
private static string ComputeActivityPhase(bool isCompiling, bool testsRunning, bool isUpdating) { if (testsRunning) return "running_tests"; if (isCompiling) return "compiling"; if (_domainReloadPending) return "domain_reload"; if (isUpdating) return "asset_import"; if (EditorApplication.isPlayingOrWillChangePlaymode) return "playmode_transition"; return "idle"; }
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 <[email protected]>
- 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 <[email protected]>
Contributor
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)
146-153: Don’t claim readiness whenwait_for_ready=False.
If the caller opts out of waiting, the success message should avoid stating that the editor is ready.✅ Proposed fix
- 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: + ready_flag = bool(wait_for_ready and ready_confirmed) + msg = ( + "Refresh recovered after Unity disconnect/retry; editor is ready." + if ready_flag + else "Refresh triggered; Unity may still be reloading." + ) + return MCPResponse( + success=True, + message=msg, + data={"recovered_from_disconnect": True, "ready_confirmed": ready_flag}, + )
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 436-439: Replace the unreliable main-thread check in
McpClientConfiguratorBase (where it currently uses
System.Threading.Thread.CurrentThread.ManagedThreadId == 1) with a cached main
thread id: add a static readonly int MainThreadId field initialized on class
load from System.Threading.Thread.CurrentThread.ManagedThreadId, then change the
check to compare the current thread's ManagedThreadId to MainThreadId before
attempting attemptAutoRewrite/Configure(); alternatively mention using
UnityEditor thread APIs if available for Editor-only validation.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
582-648: Consider consolidating Register/Unregister to reduce duplication.
Register()andRegisterWithCapturedValues()share substantial logic. Similarly for theUnregisterpair. Consider having the main-thread methods capture their values and delegate to the captured-values variants.♻️ Example consolidation for Register
private void Register() { var pathService = MCPServiceLocator.Paths; string claudePath = pathService.GetClaudeCliPath(); if (string.IsNullOrEmpty(claudePath)) { throw new InvalidOperationException("Claude CLI not found. Please install Claude Code first."); } bool useHttpTransport = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); - - string args; - if (useHttpTransport) - { - string httpUrl = HttpEndpointUtility.GetMcpRpcUrl(); - args = $"mcp add --transport http UnityMCP {httpUrl}"; - } - else - { - var (uvxPath, gitUrl, packageName) = AssetPathUtility.GetUvxCommandParts(); - // ... rest of stdio setup - } - string projectDir = Path.GetDirectoryName(Application.dataPath); - string pathPrepend = null; // ... pathPrepend setup ... - - // Remove any existing registrations ... - // Now add the registration ... - // ... rest of implementation + + string httpUrl = useHttpTransport ? HttpEndpointUtility.GetMcpRpcUrl() : null; + var (uvxPath, gitUrl, packageName) = useHttpTransport + ? (null, null, null) + : AssetPathUtility.GetUvxCommandParts(); + bool shouldForceRefresh = !useHttpTransport && AssetPathUtility.ShouldForceUvxRefresh(); + + RegisterWithCapturedValues(projectDir, claudePath, pathPrepend, + useHttpTransport, httpUrl, uvxPath, gitUrl, packageName, shouldForceRefresh); }
Comment on lines
+436
to
+439
| // 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) |
Contributor
There was a problem hiding this comment.
Unreliable main thread detection.
ManagedThreadId == 1 is not a reliable way to detect Unity's main thread. The main thread's ID is not guaranteed to be 1 across all platforms and Unity versions.
Consider using Unity's built-in mechanism or caching the main thread ID at initialization.
🔧 Suggested fix using a cached main thread ID pattern
Add a static field to cache the main thread ID (set during static initialization on the main thread):
// Add near the top of the class
private static readonly int MainThreadId = System.Threading.Thread.CurrentThread.ManagedThreadId;Then replace the check:
-bool isMainThread = System.Threading.Thread.CurrentThread.ManagedThreadId == 1;
+bool isMainThread = System.Threading.Thread.CurrentThread.ManagedThreadId == MainThreadId;Alternatively, if Unity's UnityEditor namespace is available, consider using editor-specific APIs to verify thread context.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs` around lines 436 -
439, Replace the unreliable main-thread check in McpClientConfiguratorBase
(where it currently uses System.Threading.Thread.CurrentThread.ManagedThreadId
== 1) with a cached main thread id: add a static readonly int MainThreadId field
initialized on class load from
System.Threading.Thread.CurrentThread.ManagedThreadId, then change the check to
compare the current thread's ManagedThreadId to MainThreadId before attempting
attemptAutoRewrite/Configure(); alternatively mention using UnityEditor thread
APIs if available for Editor-only validation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
This PR addresses issue #577 with comprehensive performance optimizations and stability improvements across both the Unity Editor integration and Python server.
Related Work
This PR complements #579, which addressed the stdio transport side of issue #577. This PR focuses on the HTTP transport path and broader editor performance optimizations that benefit all transport modes.
Changes
🚀 Performance Optimizations
1. Eliminated Per-Frame GC Allocations in EditorStateCache
Impact: Reduces 28ms GC spikes during editor updates
expensive JSON snapshot builds
DeepClone()result reuse to avoid repeated allocationsisCompilingfalse positives (fix: Filter EditorApplication.isCompiling false positives in Play mode #582)Files:
MCPForUnity/Editor/Services/EditorStateCache.cs2. Fixed Multiple Domain Reload Loops
Impact: Single domain reload instead of 6-7 when
refresh_unitytriggers compilationretry_on_reload=Falseparameter to prevent retrying when Unity disconnects duringcompilation
max_attemptsparameter tosend_command()to control connection-level retrybehavior
real errors
WinError 10053handlingFiles:
Server/src/services/tools/refresh_unity.pyServer/src/transport/legacy/unity_connection.py🐛 Bug Fixes
3. UI "Resuming..." Stuck State
Impact: Prevents UI from getting stuck with disabled "Start Session" button
ResumeStdioAfterReloadandResumeHttpAfterReloadEditorPrefs flags when manuallyending sessions
Files:
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs4. Windows Log Rotation Errors
Impact: Eliminates noisy PermissionError stack traces on Windows
WindowsSafeRotatingFileHandlerthat catches PermissionError during log rotationbehavior)
Files:
Server/src/main.py5. Removed Unsupported
--reinstallFlagImpact: Eliminates uvx warning: "Tools cannot be reinstalled via
uvx"--reinstallfrom all uvx command builders (not supported by uvx)--no-cache --refreshfor dev mode cache busting insteadFiles:
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/Editor/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Services/ServerManagementService.cs6. Python Packaging Fix
Impact: HTTP server can now start correctly
py-modules = ["main"]topyproject.tomlsetuptools.packages.findonly discovers packages (directories with__init__.py), mustexplicitly list standalone modules
Files:
Server/pyproject.toml📝 Documentation & Observability
7. Enhanced Connection Loss Handling Documentation
Impact: Clarifies why connection loss during compile is treated as success (addresses PR
feedback)
Files:
Server/src/services/tools/refresh_unity.py8. Fixed Missing Logger Import
Impact: Prevents NameError at runtime in connection loss paths
import loggingandlogger = logging.getLogger(__name__)Files:
Server/src/services/tools/refresh_unity.pyTesting
infrastructure issues)
transport switching
Breaking Changes
None.
Migration Notes
None required. All changes are backward compatible.
Commit History:
51ec4c6fix: reduce per-frame GC allocations causing editor hitches (issue High performance impact even when MCP server is off #577)802072bfix: prevent multiple domain reloads when calling refresh_unity (issue High performance impact even when MCP server is off #577)dd2132afix: UI and server stability improvements8f5de43docs: improve refresh_unity connection loss handling documentationd3a181bfix: add missing logger import in refresh_unitySummary by Sourcery
Optimize Unity editor integration and Python backend behavior for the Claude Code MCP workflow, reducing GC/connection overhead and improving reliability of CLI-based registration and Unity refresh flows.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.