Fix: Unity Editor reload crash + debug-noise reduction

README-DEV.md

@@ -66,6 +66,59 @@ To find it reliably:
 
 Note: In recent builds, the Python server sources are also bundled inside the package under `UnityMcpServer~/src`. This is handy for local testing or pointing MCP clients directly at the packaged server.
 
+## MCP Bridge Stress Test
+
+An on-demand stress utility exercises the MCP bridge with multiple concurrent clients while triggering real script reloads via immediate script edits (no menu calls required).
+
+### Script
+- `tools/stress_mcp.py`
+
+### What it does
+- Starts N TCP clients against the Unity MCP bridge (default port auto-discovered from `~/.unity-mcp/unity-mcp-status-*.json`).
+- Sends lightweight framed `ping` keepalives to maintain concurrency.
+- In parallel, appends a unique marker comment to a target C# file using `manage_script.apply_text_edits` with:
+  - `options.refresh = "immediate"` to force an import/compile immediately (triggers domain reload), and
+  - `precondition_sha256` computed from the current file contents to avoid drift.
+- Uses EOF insertion to avoid header/`using`-guard edits.
+
+### Usage (local)
+```bash
+# Recommended: use the included large script in the test project
+python3 tools/stress_mcp.py \
+  --duration 60 \
+  --clients 8 \
+  --unity-file "TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs"
+```
+
+Flags:
+- `--project` Unity project path (auto-detected to the included test project by default)
+- `--unity-file` C# file to edit (defaults to the long test script)
+- `--clients` number of concurrent clients (default 10)
+- `--duration` seconds to run (default 60)
+
+### Expected outcome
+- No Unity Editor crashes during reload churn
+- Immediate reloads after each applied edit (no `Assets/Refresh` menu calls)
+- Some transient disconnects or a few failed calls may occur during domain reload; the tool retries and continues
+- JSON summary printed at the end, e.g.:
+  - `{"port": 6400, "stats": {"pings": 28566, "applies": 69, "disconnects": 0, "errors": 0}}`
+
+### Notes and troubleshooting
+- Immediate vs debounced:
+  - The tool sets `options.refresh = "immediate"` so changes compile instantly. If you only need churn (not per-edit confirmation), switch to debounced to reduce mid-reload failures.
+- Precondition required:
+  - `apply_text_edits` requires `precondition_sha256` on larger files. The tool reads the file first to compute the SHA.
+- Edit location:
+  - To avoid header guards or complex ranges, the tool appends a one-line marker at EOF each cycle.
+- Read API:
+  - The bridge currently supports `manage_script.read` for file reads. You may see a deprecation warning; it's harmless for this internal tool.
+- Transient failures:
+  - Occasional `apply_errors` often indicate the connection reloaded mid-reply. Edits still typically apply; the loop continues on the next iteration.
+
+### CI guidance
+- Keep this out of default PR CI due to Unity/editor requirements and runtime variability.
+- Optionally run it as a manual workflow or nightly job on a Unity-capable runner.
+
 ## CI Test Workflow (GitHub Actions)
 
 We provide a CI job to run a Natural Language Editing mini-suite against the Unity test project. It spins up a headless Unity container and connects via the MCP bridge.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs

@@ -261,6 +261,31 @@ public void SetComponentProperties_CollectsAllFailuresAndAppliesValidOnes()
             // The collect-and-continue behavior means we should get an error response 
             // that contains info about the failed properties, but valid ones were still applied
             // This proves the collect-and-continue behavior is working
+
+            // Harden: verify structured error response with failures list contains both invalid fields
+            var successProp = result.GetType().GetProperty("success");
+            Assert.IsNotNull(successProp, "Result should expose 'success' property");
+            Assert.IsFalse((bool)successProp.GetValue(result), "Result.success should be false for partial failure");
+
+            var dataProp = result.GetType().GetProperty("data");
+            Assert.IsNotNull(dataProp, "Result should include 'data' with errors");
+            var dataVal = dataProp.GetValue(result);
+            Assert.IsNotNull(dataVal, "Result.data should not be null");
+            var errorsProp = dataVal.GetType().GetProperty("errors");
+            Assert.IsNotNull(errorsProp, "Result.data should include 'errors' list");
+            var errorsEnum = errorsProp.GetValue(dataVal) as System.Collections.IEnumerable;
+            Assert.IsNotNull(errorsEnum, "errors should be enumerable");
+
+            bool foundRotatoin = false;
+            bool foundInvalidProp = false;
+            foreach (var err in errorsEnum)
+            {
+                string s = err?.ToString() ?? string.Empty;
+                if (s.Contains("rotatoin")) foundRotatoin = true;
+                if (s.Contains("invalidProp")) foundInvalidProp = true;
+            }
+            Assert.IsTrue(foundRotatoin, "errors should mention the misspelled 'rotatoin' property");
+            Assert.IsTrue(foundInvalidProp, "errors should mention the 'invalidProp' property");
         }
 
         [Test] 
@@ -307,6 +332,28 @@ public void SetComponentProperties_ContinuesAfterException()
 
             // The key test: processing continued after the exception and set useGravity
             // This proves the collect-and-continue behavior works even with exceptions
+
+            // Harden: verify structured error response contains velocity failure
+            var successProp2 = result.GetType().GetProperty("success");
+            Assert.IsNotNull(successProp2, "Result should expose 'success' property");
+            Assert.IsFalse((bool)successProp2.GetValue(result), "Result.success should be false when an exception occurs for a property");
+
+            var dataProp2 = result.GetType().GetProperty("data");
+            Assert.IsNotNull(dataProp2, "Result should include 'data' with errors");
+            var dataVal2 = dataProp2.GetValue(result);
+            Assert.IsNotNull(dataVal2, "Result.data should not be null");
+            var errorsProp2 = dataVal2.GetType().GetProperty("errors");
+            Assert.IsNotNull(errorsProp2, "Result.data should include 'errors' list");
+            var errorsEnum2 = errorsProp2.GetValue(dataVal2) as System.Collections.IEnumerable;
+            Assert.IsNotNull(errorsEnum2, "errors should be enumerable");
+
+            bool foundVelocityError = false;
+            foreach (var err in errorsEnum2)
+            {
+                string s = err?.ToString() ?? string.Empty;
+                if (s.Contains("velocity")) { foundVelocityError = true; break; }
+            }
+            Assert.IsTrue(foundVelocityError, "errors should include a message referencing 'velocity'");
         }
     }
 }

UnityMcpBridge/Editor/MCPForUnityBridge.cs

@@ -23,6 +23,11 @@ public static partial class MCPForUnityBridge
         private static bool isRunning = false;
         private static readonly object lockObj = new();
         private static readonly object startStopLock = new();
+        private static readonly object clientsLock = new();
+        private static readonly System.Collections.Generic.HashSet<TcpClient> activeClients = new();
+        private static CancellationTokenSource cts;
+        private static Task listenerTask;
+        private static int processingCommands = 0;
         private static bool initScheduled = false;
         private static bool ensureUpdateHooked = false;
         private static bool isStarting = false;
@@ -193,9 +198,15 @@ private static void EnsureStartedOnEditorIdle()
             }
 
             isStarting = true;
-            // Attempt start; if it succeeds, remove the hook to avoid overhead
-            Start();
-            isStarting = false;
+            try
+            {
+                // Attempt start; if it succeeds, remove the hook to avoid overhead
+                Start();
+            }
+            finally
+            {
+                isStarting = false;
+            }
             if (isRunning)
             {
                 EditorApplication.update -= EnsureStartedOnEditorIdle;
@@ -319,8 +330,17 @@ public static void Start()
                     string platform = Application.platform.ToString();
                     string serverVer = ReadInstalledServerVersionSafe();
                     Debug.Log($"<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: MCPForUnityBridge started on port {currentUnityPort}. (OS={platform}, server={serverVer})");
-                    Task.Run(ListenerLoop);
+                    // Start background listener with cooperative cancellation
+                    cts = new CancellationTokenSource();
+                    listenerTask = Task.Run(() => ListenerLoopAsync(cts.Token));
                     EditorApplication.update += ProcessCommands;
+                    // Ensure lifecycle events are (re)subscribed in case Stop() removed them earlier in-domain
+                    try { AssemblyReloadEvents.beforeAssemblyReload -= OnBeforeAssemblyReload; } catch { }
+                    try { AssemblyReloadEvents.beforeAssemblyReload += OnBeforeAssemblyReload; } catch { }
+                    try { AssemblyReloadEvents.afterAssemblyReload -= OnAfterAssemblyReload; } catch { }
+                    try { AssemblyReloadEvents.afterAssemblyReload += OnAfterAssemblyReload; } catch { }
+                    try { EditorApplication.quitting -= Stop; } catch { }
+                    try { EditorApplication.quitting += Stop; } catch { }
                     // Write initial heartbeat immediately
                     heartbeatSeq++;
                     WriteHeartbeat(false, "ready");
@@ -335,6 +355,7 @@ public static void Start()
 
         public static void Stop()
         {
+            Task toWait = null;
             lock (startStopLock)
             {
                 if (!isRunning)
@@ -346,23 +367,55 @@ public static void Stop()
                 {
                     // Mark as stopping early to avoid accept logging during disposal
                     isRunning = false;
-                    // Mark heartbeat one last time before stopping
-                    WriteHeartbeat(false, "stopped");
-                    listener?.Stop();
+
+                    // Quiesce background listener quickly
+                    var cancel = cts;
+                    cts = null;
+                    try { cancel?.Cancel(); } catch { }
+
+                    try { listener?.Stop(); } catch { }
                     listener = null;
-                    EditorApplication.update -= ProcessCommands;
-                    if (IsDebugEnabled()) Debug.Log("<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: MCPForUnityBridge stopped.");
+
+                    // Capture background task to wait briefly outside the lock
+                    toWait = listenerTask;
+                    listenerTask = null;
                 }
                 catch (Exception ex)
                 {
                     Debug.LogError($"Error stopping MCPForUnityBridge: {ex.Message}");
                 }
             }
+
+            // Proactively close all active client sockets to unblock any pending reads
+            TcpClient[] toClose;
+            lock (clientsLock)
+            {
+                toClose = activeClients.ToArray();
+                activeClients.Clear();
+            }
+            foreach (var c in toClose)
+            {
+                try { c.Close(); } catch { }
+            }
+
+            // Give the background loop a short window to exit without blocking the editor
+            if (toWait != null)
+            {
+                try { toWait.Wait(100); } catch { }
+            }
+
+            // Now unhook editor events safely
+            try { EditorApplication.update -= ProcessCommands; } catch { }
+            try { AssemblyReloadEvents.beforeAssemblyReload -= OnBeforeAssemblyReload; } catch { }
+            try { AssemblyReloadEvents.afterAssemblyReload -= OnAfterAssemblyReload; } catch { }
+            try { EditorApplication.quitting -= Stop; } catch { }
+
+            if (IsDebugEnabled()) Debug.Log("<b><color=#2EA3FF>MCP-FOR-UNITY</color></b>: MCPForUnityBridge stopped.");
         }
 
-        private static async Task ListenerLoop()
+        private static async Task ListenerLoopAsync(CancellationToken token)
         {
-            while (isRunning)
+            while (isRunning && !token.IsCancellationRequested)
             {
                 try
                 {
@@ -378,31 +431,38 @@ private static async Task ListenerLoop()
                     client.ReceiveTimeout = 60000; // 60 seconds
 
                     // Fire and forget each client connection
-                    _ = HandleClientAsync(client);
+                    _ = Task.Run(() => HandleClientAsync(client, token), token);
                 }
                 catch (ObjectDisposedException)
                 {
                     // Listener was disposed during stop/reload; exit quietly
-                    if (!isRunning)
+                    if (!isRunning || token.IsCancellationRequested)
                     {
                         break;
                     }
                 }
+                catch (OperationCanceledException)
+                {
+                    break;
+                }
                 catch (Exception ex)
                 {
-                    if (isRunning)
+                    if (isRunning && !token.IsCancellationRequested)
                     {
                         if (IsDebugEnabled()) Debug.LogError($"Listener error: {ex.Message}");
                     }
                 }
             }
         }
 
-        private static async Task HandleClientAsync(TcpClient client)
+        private static async Task HandleClientAsync(TcpClient client, CancellationToken token)
         {
             using (client)
             using (NetworkStream stream = client.GetStream())
             {
+                lock (clientsLock) { activeClients.Add(client); }
+                try
+                {
                 // Framed I/O only; legacy mode removed
                 try
                 {
@@ -437,12 +497,12 @@ private static async Task HandleClientAsync(TcpClient client)
                     return; // abort this client
                 }
 
-                while (isRunning)
+                while (isRunning && !token.IsCancellationRequested)
                 {
                     try
                     {
                         // Strict framed mode only: enforced framed I/O for this connection
-                        string commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs);
+                        string commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs, token).ConfigureAwait(false);
 
                         try
                         {
@@ -454,7 +514,7 @@ private static async Task HandleClientAsync(TcpClient client)
                         }
                         catch { }
                         string commandId = Guid.NewGuid().ToString();
-                        TaskCompletionSource<string> tcs = new();
+                        var tcs = new TaskCompletionSource<string>(TaskCreationOptions.RunContinuationsAsynchronously);
 
                         // Special handling for ping command to avoid JSON parsing
                         if (commandText.Trim() == "ping")
@@ -473,7 +533,7 @@ private static async Task HandleClientAsync(TcpClient client)
                             commandQueue[commandId] = (commandText, tcs);
                         }
 
-                        string response = await tcs.Task;
+                        string response = await tcs.Task.ConfigureAwait(false);
                         byte[] responseBytes = System.Text.Encoding.UTF8.GetBytes(response);
                         await WriteFrameAsync(stream, responseBytes);
                     }
@@ -496,6 +556,11 @@ private static async Task HandleClientAsync(TcpClient client)
                         break;
                     }
                 }
+                }
+                finally
+                {
+                    lock (clientsLock) { activeClients.Remove(client); }
+                }
             }
         }
 
@@ -574,9 +639,9 @@ private static async System.Threading.Tasks.Task WriteFrameAsync(NetworkStream s
 #endif
         }
 
-        private static async System.Threading.Tasks.Task<string> ReadFrameAsUtf8Async(NetworkStream stream, int timeoutMs)
+        private static async System.Threading.Tasks.Task<string> ReadFrameAsUtf8Async(NetworkStream stream, int timeoutMs, CancellationToken cancel)
         {
-            byte[] header = await ReadExactAsync(stream, 8, timeoutMs);
+            byte[] header = await ReadExactAsync(stream, 8, timeoutMs, cancel).ConfigureAwait(false);
             ulong payloadLen = ReadUInt64BigEndian(header);
              if (payloadLen > MaxFrameBytes)
             {
@@ -589,7 +654,7 @@ private static async System.Threading.Tasks.Task<string> ReadFrameAsUtf8Async(Ne
                 throw new System.IO.IOException("Frame too large for buffer");
             }
             int count = (int)payloadLen;
-            byte[] payload = await ReadExactAsync(stream, count, timeoutMs);
+            byte[] payload = await ReadExactAsync(stream, count, timeoutMs, cancel).ConfigureAwait(false);
             return System.Text.Encoding.UTF8.GetString(payload);
         }
 
@@ -624,6 +689,10 @@ private static void WriteUInt64BigEndian(byte[] dest, ulong value)
 
         private static void ProcessCommands()
         {
+            if (!isRunning) return;
+            if (Interlocked.Exchange(ref processingCommands, 1) == 1) return; // reentrancy guard
+            try
+            {
             // Heartbeat without holding the queue lock
             double now = EditorApplication.timeSinceStartup;
             if (now >= nextHeartbeatAt)
@@ -734,6 +803,11 @@ private static void ProcessCommands()
                 // Remove quickly under lock
                 lock (lockObj) { commandQueue.Remove(id); }
             }
+            }
+            finally
+            {
+                Interlocked.Exchange(ref processingCommands, 0);
+            }
         }
 
         // Helper method to check if a string is valid JSON
@@ -865,8 +939,7 @@ private static void OnBeforeAssemblyReload()
         {
             // Stop cleanly before reload so sockets close and clients see 'reloading'
             try { Stop(); } catch { }
-            WriteHeartbeat(true, "reloading");
-            LogBreadcrumb("Reload");
+            // Avoid file I/O or heavy work here
         }
 
         private static void OnAfterAssemblyReload()