From 51ec4c64240fb4cc84149c529cc8000c37e5ff64 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 19 Jan 2026 18:09:19 -0800 Subject: [PATCH 1/7] fix: reduce per-frame GC allocations causing editor hitches (issue #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 #577: High performance impact even when MCP server is off --- .../Editor/Services/EditorStateCache.cs | 104 ++++++++++++++++-- .../Transport/TransportCommandDispatcher.cs | 16 +++ .../Transport/Transports/StdioBridgeHost.cs | 13 +++ .../Editor/Windows/MCPForUnityEditorWindow.cs | 15 +++ 4 files changed, 139 insertions(+), 9 deletions(-) diff --git a/MCPForUnity/Editor/Services/EditorStateCache.cs b/MCPForUnity/Editor/Services/EditorStateCache.cs index 838e966de..23c43ced5 100644 --- a/MCPForUnity/Editor/Services/EditorStateCache.cs +++ b/MCPForUnity/Editor/Services/EditorStateCache.cs @@ -30,7 +30,21 @@ internal static class EditorStateCache private static long? _domainReloadAfterUnixMs; private static double _lastUpdateTimeSinceStartup; - private const double MinUpdateIntervalSeconds = 0.25; + private const double MinUpdateIntervalSeconds = 1.0; // Reduced frequency: 1s instead of 0.25s + + // Cached clone to avoid DeepClone() allocations on every GetSnapshot() call + private static JObject _cachedClone; + private static long _cachedCloneSequence; + + // State tracking to detect when snapshot actually changes (checked BEFORE building) + private static string _lastTrackedScenePath; + private static string _lastTrackedSceneName; + private static bool _lastTrackedIsFocused; + private static bool _lastTrackedIsPlaying; + private static bool _lastTrackedIsPaused; + private static bool _lastTrackedIsUpdating; + private static bool _lastTrackedTestsRunning; + private static string _lastTrackedActivityPhase; private static JObject _cached; @@ -263,16 +277,78 @@ private static void OnUpdate() { // Throttle to reduce overhead while keeping the snapshot fresh enough for polling clients. double now = EditorApplication.timeSinceStartup; - if (now - _lastUpdateTimeSinceStartup < MinUpdateIntervalSeconds) + // Use GetActualIsCompiling() to avoid Play mode false positives (issue #582) + bool isCompiling = GetActualIsCompiling(); + + // Check for compilation edge transitions (always update on these) + bool compilationEdge = isCompiling != _lastIsCompiling; + + if (!compilationEdge && now - _lastUpdateTimeSinceStartup < MinUpdateIntervalSeconds) { - // Still update on compilation edge transitions to keep timestamps meaningful. - bool isCompiling = GetActualIsCompiling(); - if (isCompiling == _lastIsCompiling) - { - return; - } + return; } + // Fast state-change detection BEFORE building snapshot. + // This avoids the expensive BuildSnapshot() call entirely when nothing changed. + // These checks are much cheaper than building a full JSON snapshot. + var scene = EditorSceneManager.GetActiveScene(); + string scenePath = string.IsNullOrEmpty(scene.path) ? null : scene.path; + string sceneName = scene.name ?? string.Empty; + bool isFocused = InternalEditorUtility.isApplicationActive; + bool isPlaying = EditorApplication.isPlaying; + bool isPaused = EditorApplication.isPaused; + bool isUpdating = EditorApplication.isUpdating; + bool testsRunning = TestRunStatus.IsRunning; + + var activityPhase = "idle"; + if (testsRunning) + { + activityPhase = "running_tests"; + } + else if (isCompiling) + { + activityPhase = "compiling"; + } + else if (_domainReloadPending) + { + activityPhase = "domain_reload"; + } + else if (isUpdating) + { + activityPhase = "asset_import"; + } + else if (EditorApplication.isPlayingOrWillChangePlaymode) + { + activityPhase = "playmode_transition"; + } + + bool hasChanges = compilationEdge + || _lastTrackedScenePath != scenePath + || _lastTrackedSceneName != sceneName + || _lastTrackedIsFocused != isFocused + || _lastTrackedIsPlaying != isPlaying + || _lastTrackedIsPaused != isPaused + || _lastTrackedIsUpdating != isUpdating + || _lastTrackedTestsRunning != testsRunning + || _lastTrackedActivityPhase != activityPhase; + + if (!hasChanges) + { + // No state change - skip the expensive BuildSnapshot entirely. + // This is the key optimization that prevents the 28ms GC spikes. + return; + } + + // Update tracked state + _lastTrackedScenePath = scenePath; + _lastTrackedSceneName = sceneName; + _lastTrackedIsFocused = isFocused; + _lastTrackedIsPlaying = isPlaying; + _lastTrackedIsPaused = isPaused; + _lastTrackedIsUpdating = isUpdating; + _lastTrackedTestsRunning = testsRunning; + _lastTrackedActivityPhase = activityPhase; + _lastUpdateTimeSinceStartup = now; ForceUpdate("tick"); } @@ -425,7 +501,17 @@ public static JObject GetSnapshot() { _cached = BuildSnapshot("rebuild"); } - return (JObject)_cached.DeepClone(); + + // 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; } } diff --git a/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs b/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs index b525be2d2..0c5d8c832 100644 --- a/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs +++ b/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs @@ -218,6 +218,16 @@ private static void UnhookUpdateIfIdle() private static void ProcessQueue() { + // 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) { return; @@ -229,6 +239,12 @@ private static void ProcessQueue() lock (PendingLock) { + // Double-check inside lock to handle race condition + if (Pending.Count == 0) + { + return; + } + ready = new List<(string, PendingCommand)>(Pending.Count); foreach (var kvp in Pending) { diff --git a/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs b/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs index 30e31958e..d33366fc3 100644 --- a/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs +++ b/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs @@ -818,9 +818,22 @@ private static void ProcessCommands() nextHeartbeatAt = now + 0.5f; } + // 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; + } + List<(string id, QueuedCommand command)> work; lock (lockObj) { + // Double-check inside lock to handle race condition + if (commandQueue.Count == 0) + { + return; + } + work = new List<(string, QueuedCommand)>(commandQueue.Count); foreach (var kvp in commandQueue) { diff --git a/MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs b/MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs index d818edd05..43b1a549b 100644 --- a/MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs +++ b/MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs @@ -232,6 +232,11 @@ private void EnsureToolsLoaded() toolsSection.Refresh(); } + // Throttle OnEditorUpdate to avoid per-frame overhead (GitHub issue #577). + // Connection status polling every frame caused expensive network checks 60+ times/sec. + private double _lastEditorUpdateTime; + private const double EditorUpdateIntervalSeconds = 2.0; + private void OnEnable() { EditorApplication.update += OnEditorUpdate; @@ -257,6 +262,16 @@ private void OnFocus() private void OnEditorUpdate() { + // Throttle to 2-second intervals instead of every frame. + // This prevents the expensive IsLocalHttpServerReachable() socket checks from running + // 60+ times per second, which caused main thread blocking and GC pressure. + double now = EditorApplication.timeSinceStartup; + if (now - _lastEditorUpdateTime < EditorUpdateIntervalSeconds) + { + return; + } + _lastEditorUpdateTime = now; + if (rootVisualElement == null || rootVisualElement.childCount == 0) return; From 802072b294f23b119e0503c35775652d89562fe3 Mon Sep 17 00:00:00 2001 From: dsarno Date: Tue, 20 Jan 2026 13:20:01 -0800 Subject: [PATCH 2/7] fix: prevent multiple domain reloads when calling refresh_unity (issue #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 --- .../Configurators/ClaudeCodeConfigurator.cs | 21 +- .../Clients/McpClientConfiguratorBase.cs | 235 ++++++++++++++---- .../Editor/Helpers/AssetPathUtility.cs | 2 +- .../Editor/Helpers/CodexConfigHelper.cs | 1 + .../Editor/Helpers/ConfigJsonBuilder.cs | 3 +- .../Migrations/StdIoVersionMigration.cs | 18 +- .../Services/ServerManagementService.cs | 4 +- .../ClientConfig/McpClientConfigSection.cs | 97 +++++++- Server/pyproject.toml | 6 + Server/src/services/tools/refresh_unity.py | 60 +++-- .../src/transport/legacy/unity_connection.py | 32 ++- 11 files changed, 393 insertions(+), 86 deletions(-) diff --git a/MCPForUnity/Editor/Clients/Configurators/ClaudeCodeConfigurator.cs b/MCPForUnity/Editor/Clients/Configurators/ClaudeCodeConfigurator.cs index 06c7f703d..c890d7c46 100644 --- a/MCPForUnity/Editor/Clients/Configurators/ClaudeCodeConfigurator.cs +++ b/MCPForUnity/Editor/Clients/Configurators/ClaudeCodeConfigurator.cs @@ -1,30 +1,27 @@ -using System; using System.Collections.Generic; -using System.IO; using MCPForUnity.Editor.Models; namespace MCPForUnity.Editor.Clients.Configurators { - public class ClaudeCodeConfigurator : JsonFileMcpConfigurator + /// + /// Claude Code configurator using the CLI-based registration (claude mcp add/remove). + /// This integrates with Claude Code's native MCP management. + /// + public class ClaudeCodeConfigurator : ClaudeCliMcpConfigurator { public ClaudeCodeConfigurator() : base(new McpClient { name = "Claude Code", - windowsConfigPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".claude.json"), - macConfigPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".claude.json"), - linuxConfigPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".claude.json"), SupportsHttpTransport = true, - HttpUrlProperty = "url", // Claude Code uses "url" for HTTP servers - IsVsCodeLayout = false, // Claude Code uses standard mcpServers layout }) { } public override IList GetInstallationSteps() => new List { - "Open your project in Claude Code", - "Click Configure in MCP for Unity (or manually edit ~/.claude.json)", - "The MCP server will be added to the global mcpServers section", - "Restart Claude Code to apply changes" + "Ensure Claude CLI is installed (comes with Claude Code)", + "Click Register to add UnityMCP via 'claude mcp add'", + "The server will be automatically available in Claude Code", + "Use Unregister to remove via 'claude mcp remove'" }; } } diff --git a/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs b/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs index 2cff1ab60..2678ad012 100644 --- a/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs +++ b/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs @@ -391,7 +391,7 @@ internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTran } catch { } - // Check if UnityMCP exists + // Check if UnityMCP exists (handles both "UnityMCP" and legacy "unityMCP") if (ExecPath.TryRun(claudePath, "mcp list", projectDir, out var listStdout, out var listStderr, 10000, pathPrepend)) { if (!string.IsNullOrEmpty(listStdout) && listStdout.IndexOf("UnityMCP", StringComparison.OrdinalIgnoreCase) >= 0) @@ -401,7 +401,11 @@ internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTran bool currentUseHttp = useHttpTransport; // Get detailed info about the registration to check transport type - if (ExecPath.TryRun(claudePath, "mcp get UnityMCP", projectDir, out var getStdout, out var getStderr, 7000, pathPrepend)) + // Try both "UnityMCP" and "unityMCP" (legacy naming) + string getStdout = null, getStderr = null; + bool gotInfo = ExecPath.TryRun(claudePath, "mcp get UnityMCP", projectDir, out getStdout, out getStderr, 7000, pathPrepend) + || ExecPath.TryRun(claudePath, "mcp get unityMCP", projectDir, out getStdout, out getStderr, 7000, pathPrepend); + if (gotInfo) { // Parse the output to determine registered transport mode // The CLI output format contains "Type: http" or "Type: stdio" @@ -409,14 +413,57 @@ internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTran bool registeredWithStdio = getStdout.Contains("Type: stdio", StringComparison.OrdinalIgnoreCase); // Check for transport mismatch - if ((currentUseHttp && registeredWithStdio) || (!currentUseHttp && registeredWithHttp)) + bool hasTransportMismatch = (currentUseHttp && registeredWithStdio) || (!currentUseHttp && registeredWithHttp); + + // For stdio transport, also check package version + bool hasVersionMismatch = false; + string configuredPackageSource = null; + string expectedPackageSource = null; + if (registeredWithStdio) + { + expectedPackageSource = AssetPathUtility.GetMcpServerPackageSource(); + configuredPackageSource = ExtractPackageSourceFromCliOutput(getStdout); + hasVersionMismatch = !string.IsNullOrEmpty(configuredPackageSource) && + !string.Equals(configuredPackageSource, expectedPackageSource, StringComparison.OrdinalIgnoreCase); + } + + // If there's any mismatch and auto-rewrite is enabled, re-register + if (hasTransportMismatch || hasVersionMismatch) { - string registeredTransport = registeredWithHttp ? "HTTP" : "stdio"; - string currentTransport = currentUseHttp ? "HTTP" : "stdio"; - string errorMsg = $"Transport mismatch: Claude Code is registered with {registeredTransport} but current setting is {currentTransport}. Click Configure to re-register."; - client.SetStatus(McpStatus.Error, errorMsg); - McpLog.Warn(errorMsg); - return client.status; + if (attemptAutoRewrite) + { + string reason = hasTransportMismatch + ? $"Transport mismatch (registered: {(registeredWithHttp ? "HTTP" : "stdio")}, expected: {(currentUseHttp ? "HTTP" : "stdio")})" + : $"Package version mismatch (registered: {configuredPackageSource}, expected: {expectedPackageSource})"; + McpLog.Info($"{reason}. Re-registering..."); + try + { + // Force re-register by ensuring status is not Configured (which would toggle to Unregister) + client.SetStatus(McpStatus.IncorrectPath); + Configure(); + return client.status; + } + catch (Exception ex) + { + McpLog.Warn($"Auto-reregister failed: {ex.Message}"); + client.SetStatus(McpStatus.IncorrectPath, $"Configuration mismatch. Click Configure to re-register."); + return client.status; + } + } + else + { + if (hasTransportMismatch) + { + string errorMsg = $"Transport mismatch: Claude Code is registered with {(registeredWithHttp ? "HTTP" : "stdio")} but current setting is {(currentUseHttp ? "HTTP" : "stdio")}. Click Configure to re-register."; + client.SetStatus(McpStatus.Error, errorMsg); + McpLog.Warn(errorMsg); + } + else + { + client.SetStatus(McpStatus.IncorrectPath, $"Package version mismatch: registered with '{configuredPackageSource}' but current version is '{expectedPackageSource}'."); + } + return client.status; + } } } @@ -447,6 +494,84 @@ public override void Configure() } } + /// + /// Thread-safe version of Configure that uses pre-captured main-thread values. + /// All parameters must be captured on the main thread before calling this method. + /// + public void ConfigureWithCapturedValues( + string projectDir, string claudePath, string pathPrepend, + bool useHttpTransport, string httpUrl, + string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh) + { + if (client.status == McpStatus.Configured) + { + UnregisterWithCapturedValues(projectDir, claudePath, pathPrepend); + } + else + { + RegisterWithCapturedValues(projectDir, claudePath, pathPrepend, + useHttpTransport, httpUrl, uvxPath, gitUrl, packageName, shouldForceRefresh); + } + } + + /// + /// Thread-safe registration using pre-captured values. + /// + 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 + { + string devFlags = shouldForceRefresh ? "--no-cache --refresh --reinstall " : 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); + } + + /// + /// Thread-safe unregistration using pre-captured values. + /// + 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); + + McpLog.Info("MCP server successfully unregistered from Claude Code."); + client.SetStatus(McpStatus.NotConfigured); + } + private void Register() { var pathService = MCPServiceLocator.Paths; @@ -468,7 +593,7 @@ private void Register() { var (uvxPath, gitUrl, packageName) = AssetPathUtility.GetUvxCommandParts(); // Use central helper that checks both DevModeForceServerRefresh AND local path detection. - string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh " : string.Empty; + string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh --reinstall " : string.Empty; args = $"mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}--from \"{gitUrl}\" {packageName}"; } @@ -496,17 +621,10 @@ private void Register() } catch { } - // Check if UnityMCP already exists and remove it first to ensure clean registration - // This ensures we always use the current transport mode setting - bool serverExists = ExecPath.TryRun(claudePath, "mcp get UnityMCP", projectDir, out _, out _, 7000, pathPrepend); - if (serverExists) - { - McpLog.Info("Existing UnityMCP registration found - removing to ensure transport mode is up-to-date"); - if (!ExecPath.TryRun(claudePath, "mcp remove UnityMCP", projectDir, out var removeStdout, out var removeStderr, 10000, pathPrepend)) - { - McpLog.Warn($"Failed to remove existing UnityMCP registration: {removeStderr}. Attempting to register anyway..."); - } - } + // 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 with the current transport mode if (!ExecPath.TryRun(claudePath, args, projectDir, out var stdout, out var stderr, 15000, pathPrepend)) @@ -542,26 +660,13 @@ private void Unregister() pathPrepend = "/usr/local/bin:/usr/bin:/bin"; } - bool serverExists = ExecPath.TryRun(claudePath, "mcp get UnityMCP", projectDir, out _, out _, 7000, pathPrepend); - - if (!serverExists) - { - client.SetStatus(McpStatus.NotConfigured); - McpLog.Info("No MCP for Unity server found - already unregistered."); - return; - } - - if (ExecPath.TryRun(claudePath, "mcp remove UnityMCP", projectDir, out var stdout, out var stderr, 10000, pathPrepend)) - { - McpLog.Info("MCP server successfully unregistered from Claude Code."); - } - else - { - throw new InvalidOperationException($"Failed to unregister: {stderr}"); - } + // 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); + McpLog.Info("MCP server successfully unregistered from Claude Code."); client.SetStatus(McpStatus.NotConfigured); - // Status is already set - no need for blocking CheckStatus() call } public override string GetManualSnippet() @@ -577,7 +682,7 @@ public override string GetManualSnippet() "# Unregister the MCP server:\n" + "claude mcp remove UnityMCP\n\n" + "# List registered servers:\n" + - "claude mcp list # Only works when claude is run in the project's directory"; + "claude mcp list"; } if (string.IsNullOrEmpty(uvxPath)) @@ -587,14 +692,14 @@ public override string GetManualSnippet() string packageSource = AssetPathUtility.GetMcpServerPackageSource(); // Use central helper that checks both DevModeForceServerRefresh AND local path detection. - string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh " : string.Empty; + string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh --reinstall " : string.Empty; return "# Register the MCP server with Claude Code:\n" + $"claude mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}--from \"{packageSource}\" mcp-for-unity\n\n" + "# Unregister the MCP server:\n" + "claude mcp remove UnityMCP\n\n" + "# List registered servers:\n" + - "claude mcp list # Only works when claude is run in the project's directory"; + "claude mcp list"; } public override IList GetInstallationSteps() => new List @@ -603,5 +708,51 @@ public override string GetManualSnippet() "Use Register to add UnityMCP (or run claude mcp add UnityMCP)", "Restart Claude Code" }; + + /// + /// Extracts the package source (--from argument value) from claude mcp get output. + /// The output format includes args like: --from "mcpforunityserver==9.0.1" + /// + private static string ExtractPackageSourceFromCliOutput(string cliOutput) + { + if (string.IsNullOrEmpty(cliOutput)) + return null; + + // Look for --from followed by the package source + // The CLI output may have it quoted or unquoted + int fromIndex = cliOutput.IndexOf("--from", StringComparison.OrdinalIgnoreCase); + if (fromIndex < 0) + return null; + + // Move past "--from" and any whitespace + int startIndex = fromIndex + 6; + while (startIndex < cliOutput.Length && char.IsWhiteSpace(cliOutput[startIndex])) + startIndex++; + + if (startIndex >= cliOutput.Length) + return null; + + // Check if value is quoted + char quoteChar = cliOutput[startIndex]; + if (quoteChar == '"' || quoteChar == '\'') + { + startIndex++; + int endIndex = cliOutput.IndexOf(quoteChar, startIndex); + if (endIndex > startIndex) + return cliOutput.Substring(startIndex, endIndex - startIndex); + } + else + { + // Unquoted - read until whitespace or end of line + int endIndex = startIndex; + while (endIndex < cliOutput.Length && !char.IsWhiteSpace(cliOutput[endIndex])) + endIndex++; + + if (endIndex > startIndex) + return cliOutput.Substring(startIndex, endIndex - startIndex); + } + + return null; + } } } diff --git a/MCPForUnity/Editor/Helpers/AssetPathUtility.cs b/MCPForUnity/Editor/Helpers/AssetPathUtility.cs index 34c9ea49d..af57dde8c 100644 --- a/MCPForUnity/Editor/Helpers/AssetPathUtility.cs +++ b/MCPForUnity/Editor/Helpers/AssetPathUtility.cs @@ -195,7 +195,7 @@ public static (string uvxPath, string fromUrl, string packageName) GetUvxCommand } /// - /// Determines whether uvx should use --no-cache --refresh flags. + /// Determines whether uvx should use --no-cache --refresh --reinstall flags. /// Returns true if DevModeForceServerRefresh is enabled OR if the server URL is a local path. /// Local paths (file:// or absolute) always need fresh builds to avoid stale uvx cache. /// diff --git a/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs b/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs index 22a78cdd2..2b5d1451f 100644 --- a/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs +++ b/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs @@ -24,6 +24,7 @@ private static void AddDevModeArgs(TomlArray args) if (!AssetPathUtility.ShouldForceUvxRefresh()) return; args.Add(new TomlString { Value = "--no-cache" }); args.Add(new TomlString { Value = "--refresh" }); + args.Add(new TomlString { Value = "--reinstall" }); } public static string BuildCodexServerBlock(string uvPath) diff --git a/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs b/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs index 676d25677..6c3dcca3c 100644 --- a/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs +++ b/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs @@ -159,7 +159,7 @@ private static JObject EnsureObject(JObject parent, string name) private static IList BuildUvxArgs(string fromUrl, string packageName) { // Dev mode: force a fresh install/resolution (avoids stale cached builds while iterating). - // `--no-cache` is the key flag; `--refresh` ensures metadata is revalidated. + // `--no-cache` avoids reading from cache; `--refresh` ensures metadata is revalidated; `--reinstall` forces rebuild. // Keep ordering consistent with other uvx builders: dev flags first, then --from , then package name. var args = new List(); @@ -168,6 +168,7 @@ private static IList BuildUvxArgs(string fromUrl, string packageName) { args.Add("--no-cache"); args.Add("--refresh"); + args.Add("--reinstall"); } if (!string.IsNullOrEmpty(fromUrl)) { diff --git a/MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs b/MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs index 84b63f328..850e27337 100644 --- a/MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs +++ b/MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs @@ -54,10 +54,24 @@ private static void RunMigrationIfNeeded() { try { - if (!ConfigUsesStdIo(configurator.Client)) + if (!configurator.SupportsAutoConfigure) continue; - if (!configurator.SupportsAutoConfigure) + // Handle CLI-based configurators (e.g., Claude Code CLI) + // CheckStatus with attemptAutoRewrite=true will auto-reregister if version mismatch + if (configurator is ClaudeCliMcpConfigurator cliConfigurator) + { + var previousStatus = configurator.Status; + configurator.CheckStatus(attemptAutoRewrite: true); + if (configurator.Status != previousStatus) + { + touchedAny = true; + } + continue; + } + + // Handle JSON file-based configurators + if (!ConfigUsesStdIo(configurator.Client)) continue; MCPServiceLocator.Client.ConfigureClient(configurator); diff --git a/MCPForUnity/Editor/Services/ServerManagementService.cs b/MCPForUnity/Editor/Services/ServerManagementService.cs index cd62258d3..e9c60d9a7 100644 --- a/MCPForUnity/Editor/Services/ServerManagementService.cs +++ b/MCPForUnity/Editor/Services/ServerManagementService.cs @@ -443,7 +443,7 @@ public bool StartLocalHttpServer() } catch { } - // Note: Dev mode cache-busting is handled by `uvx --no-cache --refresh` in the generated command. + // Note: Dev mode cache-busting is handled by `uvx --no-cache --refresh --reinstall` in the generated command. // Create a per-launch token + pidfile path so Stop can be deterministic without relying on port/PID heuristics. string baseUrlForPid = HttpEndpointUtility.GetBaseUrl(); @@ -1310,7 +1310,7 @@ private bool TryGetLocalHttpServerCommandParts(out string fileName, out string a } // Use central helper that checks both DevModeForceServerRefresh AND local path detection. - string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh " : string.Empty; + string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh --reinstall " : string.Empty; string args = string.IsNullOrEmpty(fromUrl) ? $"{devFlags}{packageName} --transport http --http-url {httpUrl}" : $"{devFlags}--from {fromUrl} {packageName} --transport http --http-url {httpUrl}"; diff --git a/MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs b/MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs index c606460fb..4f153e4c0 100644 --- a/MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs +++ b/MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs @@ -221,6 +221,13 @@ private void OnConfigureClicked() var client = configurators[selectedClientIndex]; + // Handle CLI configurators asynchronously + if (client is ClaudeCliMcpConfigurator) + { + ConfigureClaudeCliAsync(client); + return; + } + try { MCPServiceLocator.Client.ConfigureClient(client); @@ -237,6 +244,92 @@ private void OnConfigureClicked() } } + private void ConfigureClaudeCliAsync(IMcpClientConfigurator client) + { + if (statusRefreshInFlight.Contains(client)) + return; + + statusRefreshInFlight.Add(client); + bool isCurrentlyConfigured = client.Status == McpStatus.Configured; + ApplyStatusToUi(client, showChecking: true, customMessage: isCurrentlyConfigured ? "Unregistering..." : "Registering..."); + + // Capture ALL main-thread-only values before async task + string projectDir = Path.GetDirectoryName(Application.dataPath); + bool useHttpTransport = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); + string claudePath = MCPServiceLocator.Paths.GetClaudeCliPath(); + string httpUrl = HttpEndpointUtility.GetMcpRpcUrl(); + var (uvxPath, gitUrl, packageName) = AssetPathUtility.GetUvxCommandParts(); + bool shouldForceRefresh = AssetPathUtility.ShouldForceUvxRefresh(); + + // Compute pathPrepend on main thread + string pathPrepend = null; + if (Application.platform == RuntimePlatform.OSXEditor) + pathPrepend = "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin"; + else if (Application.platform == RuntimePlatform.LinuxEditor) + pathPrepend = "/usr/local/bin:/usr/bin:/bin"; + try + { + string claudeDir = Path.GetDirectoryName(claudePath); + if (!string.IsNullOrEmpty(claudeDir)) + pathPrepend = string.IsNullOrEmpty(pathPrepend) ? claudeDir : $"{claudeDir}:{pathPrepend}"; + } + catch { } + + Task.Run(() => + { + try + { + if (client is ClaudeCliMcpConfigurator cliConfigurator) + { + cliConfigurator.ConfigureWithCapturedValues( + projectDir, claudePath, pathPrepend, + useHttpTransport, httpUrl, + uvxPath, gitUrl, packageName, shouldForceRefresh); + } + return (success: true, error: (string)null); + } + catch (Exception ex) + { + return (success: false, error: ex.Message); + } + }).ContinueWith(t => + { + string errorMessage = null; + if (t.IsFaulted && t.Exception != null) + { + errorMessage = t.Exception.GetBaseException()?.Message ?? "Configuration failed"; + } + else if (!t.Result.success) + { + errorMessage = t.Result.error; + } + + EditorApplication.delayCall += () => + { + statusRefreshInFlight.Remove(client); + lastStatusChecks.Remove(client); + + if (errorMessage != null) + { + if (client is McpClientConfiguratorBase baseConfigurator) + { + baseConfigurator.Client.SetStatus(McpStatus.Error, errorMessage); + } + McpLog.Error($"Configuration failed: {errorMessage}"); + RefreshClientStatus(client, forceImmediate: true); + } + else + { + // Registration succeeded - trust the status set by RegisterWithCapturedValues + // and update UI without re-verifying (which could fail due to CLI timing/scope issues) + lastStatusChecks[client] = DateTime.UtcNow; + ApplyStatusToUi(client); + } + UpdateManualConfiguration(); + }; + }); + } + private void OnBrowseClaudeClicked() { string suggested = RuntimeInformation.IsOSPlatform(OSPlatform.OSX) @@ -396,7 +489,7 @@ private bool ShouldRefreshClient(IMcpClientConfigurator client) return (DateTime.UtcNow - last) > StatusRefreshInterval; } - private void ApplyStatusToUi(IMcpClientConfigurator client, bool showChecking = false) + private void ApplyStatusToUi(IMcpClientConfigurator client, bool showChecking = false, string customMessage = null) { if (selectedClientIndex < 0 || selectedClientIndex >= configurators.Count) return; @@ -410,7 +503,7 @@ private void ApplyStatusToUi(IMcpClientConfigurator client, bool showChecking = if (showChecking) { - clientStatusLabel.text = "Checking..."; + clientStatusLabel.text = customMessage ?? "Checking..."; clientStatusLabel.style.color = StyleKeyword.Null; clientStatusIndicator.AddToClassList("warning"); configureButton.text = client.GetConfigureActionLabel(); diff --git a/Server/pyproject.toml b/Server/pyproject.toml index 3af175b94..cf7c854fd 100644 --- a/Server/pyproject.toml +++ b/Server/pyproject.toml @@ -56,6 +56,12 @@ mcp-for-unity = "main:main" requires = ["setuptools>=64.0.0", "wheel"] build-backend = "setuptools.build_meta" +[tool.setuptools.packages.find] +where = ["src"] + +[tool.setuptools.package-dir] +"" = "src" + [tool.coverage.run] source = ["src"] omit = [ diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index 80f805f1f..dfcb32c0c 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -43,29 +43,47 @@ async def refresh_unity( } recovered_from_disconnect = False + # Don't retry on reload - refresh_unity triggers compilation/reload, + # so retrying would cause multiple reloads (issue #577) response = await unity_transport.send_with_unity_instance( async_send_command_with_retry, unity_instance, "refresh_unity", params, + retry_on_reload=False, ) - # Option A: treat disconnects / retry hints as recoverable when wait_for_ready is true. - # Unity can legitimately disconnect during refresh/compile/domain reload, so callers should not - # interpret that as a hard failure (#503-style loops). - if isinstance(response, dict) and not response.get("success", True): - hint = response.get("hint") - err = (response.get("error") or response.get("message") or "").lower() - reason = _extract_response_reason(response) - is_retryable = ( - hint == "retry" + # Handle connection errors during refresh/compile gracefully. + # Unity disconnects during domain reload, which is expected behavior - not a failure. + # If we sent the command and connection closed, the refresh was likely triggered successfully. + # Convert MCPResponse to dict if needed + 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): + hint = response_dict.get("hint") + err = (response_dict.get("error") or response_dict.get("message") or "").lower() + reason = _extract_response_reason(response_dict) + + # Connection closed/timeout during compile = refresh was triggered, Unity is reloading + # This is SUCCESS, not failure - don't return error to prevent Claude Code from retrying + is_connection_lost = ( + "connection closed" in err or "disconnected" in err - or "could not connect" in err # Connection failed during domain reload + or "timeout" in err + or reason == "reloading" ) - if (not wait_for_ready) or (not is_retryable): - return MCPResponse(**response) - if reason not in {"reloading", "no_unity_session"}: + + if is_connection_lost and compile == "request": + # Refresh with compile was triggered; Unity disconnected during domain reload + # Return success to prevent retry loops recovered_from_disconnect = True + elif hint == "retry" or "could not connect" in err: + # Retryable error - proceed to wait loop if wait_for_ready + if not wait_for_ready: + return MCPResponse(**response_dict) + recovered_from_disconnect = True + else: + # Non-recoverable error + return MCPResponse(**response_dict) # Optional server-side wait loop (defensive): if Unity tool doesn't wait or returns quickly, # poll the canonical editor_state resource until ready or timeout. @@ -73,6 +91,9 @@ async def refresh_unity( 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: state_resp = await editor_state.get_editor_state(ctx) state = state_resp.model_dump() if hasattr( @@ -81,8 +102,15 @@ async def refresh_unity( state, dict) else None advice = (data or {}).get( "advice") if isinstance(data, dict) else None - if isinstance(advice, dict) and advice.get("ready_for_tools") is True: - break + if isinstance(advice, dict): + # Exit if ready_for_tools is True + if advice.get("ready_for_tools") is 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): + break await asyncio.sleep(0.25) # After readiness is restored, clear any external-dirty flag for this instance so future tools can proceed cleanly. @@ -100,4 +128,4 @@ async def refresh_unity( data={"recovered_from_disconnect": True}, ) - return MCPResponse(**response) if isinstance(response, dict) else response + return MCPResponse(**response_dict) if isinstance(response, dict) else response diff --git a/Server/src/transport/legacy/unity_connection.py b/Server/src/transport/legacy/unity_connection.py index 7e40c89d6..951c7ec5f 100644 --- a/Server/src/transport/legacy/unity_connection.py +++ b/Server/src/transport/legacy/unity_connection.py @@ -233,14 +233,20 @@ def receive_full_response(self, sock, buffer_size=config.buffer_size) -> bytes: logger.error(f"Error during receive: {str(e)}") raise - def send_command(self, command_type: str, params: dict[str, Any] = None) -> dict[str, Any]: - """Send a command with retry/backoff and port rediscovery. Pings only when requested.""" + 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 base_backoff = max(0.5, config.retry_delay) def read_status_file(target_hash: str | None = None) -> dict | None: @@ -734,7 +740,8 @@ def send_command_with_retry( *, instance_id: str | None = None, max_retries: int | None = None, - retry_ms: int | None = None + retry_ms: int | None = None, + retry_on_reload: bool = True ) -> dict[str, Any] | MCPResponse: """Send a command to a Unity instance, waiting politely through Unity reloads. @@ -744,6 +751,8 @@ def send_command_with_retry( instance_id: Optional Unity instance identifier (name, hash, name@hash, etc.) max_retries: Maximum number of retries for reload states retry_ms: Delay between retries in milliseconds + retry_on_reload: If False, don't retry when Unity is reloading (for commands + that trigger compilation/reload and shouldn't be re-sent) Returns: Response dictionary or MCPResponse from Unity @@ -768,11 +777,15 @@ def send_command_with_retry( # Clamp to [0, 30] to prevent misconfiguration from causing excessive waits max_wait_s = max(0.0, min(max_wait_s, 30.0)) - response = conn.send_command(command_type, params) + # If retry_on_reload=False, disable connection-level retries too (issue #577) + # Commands that trigger compilation/reload shouldn't retry on disconnect + send_max_attempts = None if retry_on_reload else 0 + + response = conn.send_command(command_type, params, max_attempts=send_max_attempts) retries = 0 wait_started = None reason = _extract_response_reason(response) - while _is_reloading_response(response) and retries < max_retries: + while retry_on_reload and _is_reloading_response(response) and retries < max_retries: if wait_started is None: wait_started = time.monotonic() logger.debug( @@ -842,7 +855,8 @@ async def async_send_command_with_retry( instance_id: str | None = None, loop=None, max_retries: int | None = None, - retry_ms: int | None = None + retry_ms: int | None = None, + retry_on_reload: bool = True ) -> dict[str, Any] | MCPResponse: """Async wrapper that runs the blocking retry helper in a thread pool. @@ -853,6 +867,7 @@ async def async_send_command_with_retry( loop: Optional asyncio event loop max_retries: Maximum number of retries for reload states retry_ms: Delay between retries in milliseconds + retry_on_reload: If False, don't retry when Unity is reloading Returns: Response dictionary or MCPResponse on error @@ -864,7 +879,8 @@ async def async_send_command_with_retry( return await loop.run_in_executor( None, lambda: send_command_with_retry( - command_type, params, instance_id=instance_id, max_retries=max_retries, retry_ms=retry_ms), + command_type, params, instance_id=instance_id, max_retries=max_retries, + retry_ms=retry_ms, retry_on_reload=retry_on_reload), ) except Exception as e: return MCPResponse(success=False, error=str(e)) From 488dadc961f3890da474cbe726621c103c5c1399 Mon Sep 17 00:00:00 2001 From: dsarno Date: Tue, 20 Jan 2026 14:05:22 -0800 Subject: [PATCH 3/7] fix: UI and server stability improvements 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 --- .../Editor/Clients/McpClientConfiguratorBase.cs | 9 ++++++--- MCPForUnity/Editor/Helpers/AssetPathUtility.cs | 3 ++- MCPForUnity/Editor/Helpers/CodexConfigHelper.cs | 2 +- MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs | 6 +++--- .../Editor/Services/ServerManagementService.cs | 5 +++-- .../Components/Connection/McpConnectionSection.cs | 11 +++++++++++ Server/pyproject.toml | 3 +++ Server/src/main.py | 15 ++++++++++++++- Server/src/services/tools/refresh_unity.py | 1 + 9 files changed, 44 insertions(+), 11 deletions(-) diff --git a/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs b/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs index 2678ad012..1a608ce64 100644 --- a/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs +++ b/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs @@ -534,7 +534,8 @@ private void RegisterWithCapturedValues( } else { - string devFlags = shouldForceRefresh ? "--no-cache --refresh --reinstall " : string.Empty; + // 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}"; } @@ -593,7 +594,8 @@ private void Register() { var (uvxPath, gitUrl, packageName) = AssetPathUtility.GetUvxCommandParts(); // Use central helper that checks both DevModeForceServerRefresh AND local path detection. - string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh --reinstall " : string.Empty; + // Note: --reinstall is not supported by uvx, use --no-cache --refresh instead + string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh " : string.Empty; args = $"mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}--from \"{gitUrl}\" {packageName}"; } @@ -692,7 +694,8 @@ public override string GetManualSnippet() string packageSource = AssetPathUtility.GetMcpServerPackageSource(); // Use central helper that checks both DevModeForceServerRefresh AND local path detection. - string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh --reinstall " : string.Empty; + // Note: --reinstall is not supported by uvx, use --no-cache --refresh instead + string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh " : string.Empty; return "# Register the MCP server with Claude Code:\n" + $"claude mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}--from \"{packageSource}\" mcp-for-unity\n\n" + diff --git a/MCPForUnity/Editor/Helpers/AssetPathUtility.cs b/MCPForUnity/Editor/Helpers/AssetPathUtility.cs index af57dde8c..b65cf21ea 100644 --- a/MCPForUnity/Editor/Helpers/AssetPathUtility.cs +++ b/MCPForUnity/Editor/Helpers/AssetPathUtility.cs @@ -195,9 +195,10 @@ public static (string uvxPath, string fromUrl, string packageName) GetUvxCommand } /// - /// Determines whether uvx should use --no-cache --refresh --reinstall flags. + /// Determines whether uvx should use --no-cache --refresh flags. /// Returns true if DevModeForceServerRefresh is enabled OR if the server URL is a local path. /// Local paths (file:// or absolute) always need fresh builds to avoid stale uvx cache. + /// Note: --reinstall is not supported by uvx and will cause a warning. /// public static bool ShouldForceUvxRefresh() { diff --git a/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs b/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs index 2b5d1451f..10f2bacb3 100644 --- a/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs +++ b/MCPForUnity/Editor/Helpers/CodexConfigHelper.cs @@ -21,10 +21,10 @@ private static void AddDevModeArgs(TomlArray args) { if (args == null) return; // Use central helper that checks both DevModeForceServerRefresh AND local path detection. + // Note: --reinstall is not supported by uvx, use --no-cache --refresh instead if (!AssetPathUtility.ShouldForceUvxRefresh()) return; args.Add(new TomlString { Value = "--no-cache" }); args.Add(new TomlString { Value = "--refresh" }); - args.Add(new TomlString { Value = "--reinstall" }); } public static string BuildCodexServerBlock(string uvPath) diff --git a/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs b/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs index 6c3dcca3c..dd00d73d0 100644 --- a/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs +++ b/MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs @@ -159,16 +159,16 @@ private static JObject EnsureObject(JObject parent, string name) private static IList BuildUvxArgs(string fromUrl, string packageName) { // Dev mode: force a fresh install/resolution (avoids stale cached builds while iterating). - // `--no-cache` avoids reading from cache; `--refresh` ensures metadata is revalidated; `--reinstall` forces rebuild. + // `--no-cache` avoids reading from cache; `--refresh` ensures metadata is revalidated. + // Note: --reinstall is not supported by uvx and will cause a warning. // Keep ordering consistent with other uvx builders: dev flags first, then --from , then package name. var args = new List(); - + // Use central helper that checks both DevModeForceServerRefresh AND local path detection. if (AssetPathUtility.ShouldForceUvxRefresh()) { args.Add("--no-cache"); args.Add("--refresh"); - args.Add("--reinstall"); } if (!string.IsNullOrEmpty(fromUrl)) { diff --git a/MCPForUnity/Editor/Services/ServerManagementService.cs b/MCPForUnity/Editor/Services/ServerManagementService.cs index e9c60d9a7..69ca6d5ea 100644 --- a/MCPForUnity/Editor/Services/ServerManagementService.cs +++ b/MCPForUnity/Editor/Services/ServerManagementService.cs @@ -443,7 +443,7 @@ public bool StartLocalHttpServer() } catch { } - // Note: Dev mode cache-busting is handled by `uvx --no-cache --refresh --reinstall` in the generated command. + // Note: Dev mode cache-busting is handled by `uvx --no-cache --refresh` in the generated command. // Create a per-launch token + pidfile path so Stop can be deterministic without relying on port/PID heuristics. string baseUrlForPid = HttpEndpointUtility.GetBaseUrl(); @@ -1310,7 +1310,8 @@ private bool TryGetLocalHttpServerCommandParts(out string fileName, out string a } // Use central helper that checks both DevModeForceServerRefresh AND local path detection. - string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh --reinstall " : string.Empty; + // Note: --reinstall is not supported by uvx, use --no-cache --refresh instead + string devFlags = AssetPathUtility.ShouldForceUvxRefresh() ? "--no-cache --refresh " : string.Empty; string args = string.IsNullOrEmpty(fromUrl) ? $"{devFlags}{packageName} --transport http --http-url {httpUrl}" : $"{devFlags}--from {fromUrl} {packageName} --transport http --http-url {httpUrl}"; diff --git a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs index ab5c9036a..7d70d4749 100644 --- a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs +++ b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs @@ -674,6 +674,12 @@ private async void OnConnectionToggleClicked() { if (bridgeService.IsRunning) { + // Clear any resume flags when user manually ends the session to prevent + // getting stuck in "Resuming..." state (the flag may have been set by a + // domain reload that started just before the user clicked End Session) + try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); } catch { } + try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); } catch { } + await bridgeService.StopAsync(); } else @@ -717,6 +723,11 @@ private async Task EndOrphanedSessionAsync() { connectionToggleInProgress = true; connectionToggleButton?.SetEnabled(false); + + // Clear resume flags to prevent getting stuck in "Resuming..." state + try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); } catch { } + try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); } catch { } + await MCPServiceLocator.Bridge.StopAsync(); } catch (Exception ex) diff --git a/Server/pyproject.toml b/Server/pyproject.toml index cf7c854fd..87f686cb7 100644 --- a/Server/pyproject.toml +++ b/Server/pyproject.toml @@ -62,6 +62,9 @@ where = ["src"] [tool.setuptools.package-dir] "" = "src" +[tool.setuptools] +py-modules = ["main"] + [tool.coverage.run] source = ["src"] omit = [ diff --git a/Server/src/main.py b/Server/src/main.py index c46a4765c..bbd4e14be 100644 --- a/Server/src/main.py +++ b/Server/src/main.py @@ -37,6 +37,19 @@ from fastmcp import FastMCP from logging.handlers import RotatingFileHandler + + +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 from starlette.requests import Request from starlette.responses import JSONResponse from starlette.routing import WebSocketRoute @@ -69,7 +82,7 @@ "~/Library/Application Support/UnityMCP"), "Logs") os.makedirs(_log_dir, exist_ok=True) _file_path = os.path.join(_log_dir, "unity_mcp_server.log") - _fh = RotatingFileHandler( + _fh = WindowsSafeRotatingFileHandler( _file_path, maxBytes=512*1024, backupCount=2, encoding="utf-8") _fh.setFormatter(logging.Formatter(config.log_format)) _fh.setLevel(getattr(logging, config.log_level)) diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index dfcb32c0c..c2f20fb05 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -68,6 +68,7 @@ async def refresh_unity( is_connection_lost = ( "connection closed" in err or "disconnected" in err + or "aborted" in err # WinError 10053: connection aborted or "timeout" in err or reason == "reloading" ) From 8f5de4319f01e398cae3b7cb99122afafcb2f176 Mon Sep 17 00:00:00 2001 From: dsarno Date: Tue, 20 Jan 2026 17:27:53 -0800 Subject: [PATCH 4/7] docs: improve refresh_unity connection loss handling documentation 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 #577) Added logging for observability: - Info log when expected disconnect detected - Warning log for non-recoverable errors Co-Authored-By: Claude Sonnet 4.5 --- Server/src/services/tools/refresh_unity.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index c2f20fb05..330daf940 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -74,8 +74,12 @@ async def refresh_unity( ) if is_connection_lost and compile == "request": - # Refresh with compile was triggered; Unity disconnected during domain reload - # Return success to prevent retry loops + # EXPECTED BEHAVIOR: When compile="request", Unity triggers domain reload which + # causes connection to close mid-command. This is NOT a failure - the refresh + # was successfully triggered. Treating this as success prevents Claude Code from + # retrying unnecessarily (which would cause multiple domain reloads - issue #577). + # The subsequent wait_for_ready loop (below) will verify Unity becomes ready. + logger.info("refresh_unity: Connection lost during compile (expected - domain reload triggered)") recovered_from_disconnect = True elif hint == "retry" or "could not connect" in err: # Retryable error - proceed to wait loop if wait_for_ready @@ -83,7 +87,8 @@ async def refresh_unity( return MCPResponse(**response_dict) recovered_from_disconnect = True else: - # Non-recoverable error + # Non-recoverable error - connection issue unrelated to domain reload + logger.warning(f"refresh_unity: Non-recoverable error (compile={compile}): {err[:100]}") return MCPResponse(**response_dict) # Optional server-side wait loop (defensive): if Unity tool doesn't wait or returns quickly, From d3a181b0e608c436e0c9195940ccc62e8499feb4 Mon Sep 17 00:00:00 2001 From: dsarno Date: Tue, 20 Jan 2026 17:44:40 -0800 Subject: [PATCH 5/7] fix: add missing logger import in refresh_unity 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 --- Server/src/services/tools/refresh_unity.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index 330daf940..52a370dc9 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -1,6 +1,7 @@ from __future__ import annotations import asyncio +import logging import time from typing import Annotated, Any, Literal @@ -15,6 +16,8 @@ from services.state.external_changes_scanner import external_changes_scanner import services.resources.editor_state as editor_state +logger = logging.getLogger(__name__) + @mcp_for_unity_tool( description="Request a Unity asset database refresh and optionally a script compilation. Can optionally wait for readiness.", From 776bf26b24eab1b1b49a2ac3b0d345eb10cbbe31 Mon Sep 17 00:00:00 2001 From: dsarno Date: Tue, 20 Jan 2026 17:58:54 -0800 Subject: [PATCH 6/7] fix: address code review feedback on thread safety and semantics 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 --- .../Editor/Services/EditorStateCache.cs | 18 ++++-------------- .../Transport/TransportCommandDispatcher.cs | 12 +----------- .../Transport/Transports/StdioBridgeHost.cs | 9 +-------- Server/src/services/tools/refresh_unity.py | 3 ++- 4 files changed, 8 insertions(+), 34 deletions(-) diff --git a/MCPForUnity/Editor/Services/EditorStateCache.cs b/MCPForUnity/Editor/Services/EditorStateCache.cs index 23c43ced5..24fec0f14 100644 --- a/MCPForUnity/Editor/Services/EditorStateCache.cs +++ b/MCPForUnity/Editor/Services/EditorStateCache.cs @@ -32,10 +32,6 @@ internal static class EditorStateCache private static double _lastUpdateTimeSinceStartup; private const double MinUpdateIntervalSeconds = 1.0; // Reduced frequency: 1s instead of 0.25s - // Cached clone to avoid DeepClone() allocations on every GetSnapshot() call - private static JObject _cachedClone; - private static long _cachedCloneSequence; - // State tracking to detect when snapshot actually changes (checked BEFORE building) private static string _lastTrackedScenePath; private static string _lastTrackedSceneName; @@ -502,16 +498,10 @@ public static JObject GetSnapshot() _cached = BuildSnapshot("rebuild"); } - // 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; + // Always return a fresh clone to prevent mutation bugs. + // The main GC optimization comes from state-change detection (OnUpdate) + // which prevents unnecessary _cached rebuilds, not from caching the clone. + return (JObject)_cached.DeepClone(); } } diff --git a/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs b/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs index 0c5d8c832..999c7bf11 100644 --- a/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs +++ b/MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs @@ -218,16 +218,6 @@ private static void UnhookUpdateIfIdle() private static void ProcessQueue() { - // 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) { return; @@ -239,7 +229,7 @@ private static void ProcessQueue() lock (PendingLock) { - // Double-check inside lock to handle race condition + // Early exit inside lock to prevent per-frame List allocations (GitHub issue #577) if (Pending.Count == 0) { return; diff --git a/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs b/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs index d33366fc3..68ab1b64c 100644 --- a/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs +++ b/MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs @@ -818,17 +818,10 @@ private static void ProcessCommands() nextHeartbeatAt = now + 0.5f; } - // 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; - } - List<(string id, QueuedCommand command)> work; lock (lockObj) { - // Double-check inside lock to handle race condition + // Early exit inside lock to prevent per-frame List allocations (GitHub issue #577) if (commandQueue.Count == 0) { return; diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index 52a370dc9..cd2b8610f 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -101,7 +101,8 @@ async def refresh_unity( 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"} + # Must match activityPhase values from EditorStateCache.cs + real_blocking_reasons = {"compiling", "domain_reload", "running_tests", "asset_import"} while time.monotonic() - start < timeout_s: state_resp = await editor_state.get_editor_state(ctx) From 365c3cd2642bab5d10014eeb000a218706cdd4d5 Mon Sep 17 00:00:00 2001 From: dsarno Date: Tue, 20 Jan 2026 18:04:49 -0800 Subject: [PATCH 7/7] fix: address code review feedback on thread safety and timeout handling - 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 --- .../Editor/Clients/McpClientConfiguratorBase.cs | 10 ++++++++-- Server/src/services/tools/refresh_unity.py | 12 ++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs b/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs index 1a608ce64..897e7dfd8 100644 --- a/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs +++ b/MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs @@ -352,8 +352,11 @@ public override McpStatus CheckStatus(bool attemptAutoRewrite = true) /// Internal thread-safe version of CheckStatus. /// Can be called from background threads because all main-thread-only values are passed as parameters. /// projectDir, useHttpTransport, and claudePath are REQUIRED (non-nullable) to enforce thread safety at compile time. + /// NOTE: attemptAutoRewrite is NOT fully thread-safe because Configure() requires the main thread. + /// When called from a background thread, pass attemptAutoRewrite=false and handle re-registration + /// on the main thread based on the returned status. /// - internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTransport, string claudePath, bool attemptAutoRewrite = true) + internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTransport, string claudePath, bool attemptAutoRewrite = false) { try { @@ -430,7 +433,10 @@ internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTran // If there's any mismatch and auto-rewrite is enabled, re-register if (hasTransportMismatch || hasVersionMismatch) { - if (attemptAutoRewrite) + // 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) { string reason = hasTransportMismatch ? $"Transport mismatch (registered: {(registeredWithHttp ? "HTTP" : "stdio")}, expected: {(currentUseHttp ? "HTTP" : "stdio")})" diff --git a/Server/src/services/tools/refresh_unity.py b/Server/src/services/tools/refresh_unity.py index cd2b8610f..28de80300 100644 --- a/Server/src/services/tools/refresh_unity.py +++ b/Server/src/services/tools/refresh_unity.py @@ -96,6 +96,7 @@ async def refresh_unity( # Optional server-side wait loop (defensive): if Unity tool doesn't wait or returns quickly, # poll the canonical editor_state resource until ready or timeout. + ready_confirmed = False if wait_for_ready: timeout_s = 60.0 start = time.monotonic() @@ -115,14 +116,25 @@ async def refresh_unity( 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 break await asyncio.sleep(0.25) + # If we timed out without confirming readiness, log and return failure + if not ready_confirmed: + logger.warning(f"refresh_unity: Timed out after {timeout_s}s waiting for editor to become ready") + return MCPResponse( + success=False, + message=f"Refresh triggered but timed out after {timeout_s}s waiting for editor readiness.", + data={"timeout": True, "wait_seconds": timeout_s}, + ) + # After readiness is restored, clear any external-dirty flag for this instance so future tools can proceed cleanly. try: inst = unity_instance or await editor_state.infer_single_instance_id(ctx)