From 301511700ce0e54937f34069791a5cee6d722d64 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 3 Jul 2026 22:48:28 -0700 Subject: [PATCH 1/2] fix(editor): survive domain reloads in HTTP auto-start and reload-resume (#1229) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Auto-start died for the whole session whenever startup included a compile: the ctor latched SessionState before the delayCall ran, and the reload wiped the delayCall. Reload-resume died at multi-pass compiles: the one-shot flag was consumed before the deferred (delayCall) resume ran, and the next boundary deleted it again. - Replace delayCall with EditorApplication.update ticks that the [InitializeOnLoad] ctor re-arms on every domain load; latch only when the deferred work actually dispatches, retry (bounded per domain) while editor services are still initializing, and skip the subscription entirely in the common case where auto-start is off and nothing is pending. - Move the resume flag from EditorPrefs (per-user machine-global, survives crashes, leaks across concurrently open editors) to SessionState; keep it until the resume succeeds, is cancelled, or exhausts its retries, instead of consuming it at boundaries where the bridge is down. Manual Connect, End Session, transport switch, and orphan cleanup cancel a pending resume through a named seam (CancelPendingResume), which also aborts an in-flight retry loop; exhaustion erases the flag so later reloads don't replay 49s failure loops. - Serialize TransportManager.StartAsync per mode: concurrent starts coalesce onto one in-flight attempt, so a manual Connect can no longer race the resume/auto-start loops into bouncing a just-established session (WebSocketTransportClient.StartAsync tears down a live connection first). - A SessionState connect-pending marker lets the next domain load finish an auto-start whose connect phase a reload killed — connect-only, never re-spawning (StartLocalHttpServer stops a still-booting server first). Whether a launch-process handle exists is now answered live by ServerManagementService.HasManagedServerLaunchHandle; without one (post- reload, or an externally started server) the wait polls to the 5-minute hard cap instead of fail-fasting. - Busy gate uses EditorStateCache.GetActualIsCompiling (now internal, with the CompilationPipeline reflection bound once as a delegate): raw isCompiling stays true all play session under Recompile-After-Finished-Playing (#549). - One-time (per session) migration deletes the legacy EditorPrefs flag. The stdio sibling has the same defect class (StdioBridgeReloadHandler.cs:65 delete-when-not-running, :133 delayCall) — follow-up, kept out of scope here, along with the remaining stdio copies of the isCompiling probe. --- .../Editor/Constants/EditorPrefKeys.cs | 1 - .../Editor/Services/EditorStateCache.cs | 36 ++- .../Editor/Services/HttpAutoStartHandler.cs | 224 ++++++++++++++++-- .../Services/HttpBridgeReloadHandler.cs | 166 ++++++++----- .../Services/IServerManagementService.cs | 7 + .../Services/ServerManagementService.cs | 2 + .../Services/Transport/TransportManager.cs | 22 +- .../Connection/McpConnectionSection.cs | 15 +- .../Windows/EditorPrefs/EditorPrefsWindow.cs | 1 - .../Services/HttpAutoStartHandlerTests.cs | 194 +++++++++++++++ .../HttpAutoStartHandlerTests.cs.meta | 11 + .../Services/HttpBridgeReloadHandlerTests.cs | 196 +++++++++++++++ .../HttpBridgeReloadHandlerTests.cs.meta | 11 + .../Services/TransportManagerTests.cs | 66 ++++++ .../Services/TransportManagerTests.cs.meta | 11 + .../Assets/Tests/EditMode/TestUtilities.cs | 44 +++- .../Windows_Characterization.cs | 2 +- 17 files changed, 909 insertions(+), 100 deletions(-) create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs.meta create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs.meta create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs.meta diff --git a/MCPForUnity/Editor/Constants/EditorPrefKeys.cs b/MCPForUnity/Editor/Constants/EditorPrefKeys.cs index def02c795..5fdfeb0c9 100644 --- a/MCPForUnity/Editor/Constants/EditorPrefKeys.cs +++ b/MCPForUnity/Editor/Constants/EditorPrefKeys.cs @@ -17,7 +17,6 @@ internal static class EditorPrefKeys internal const string DebugLogs = "MCPForUnity.DebugLogs"; internal const string ValidationLevel = "MCPForUnity.ValidationLevel"; internal const string UnitySocketPort = "MCPForUnity.UnitySocketPort"; - internal const string ResumeHttpAfterReload = "MCPForUnity.ResumeHttpAfterReload"; internal const string ResumeStdioAfterReload = "MCPForUnity.ResumeStdioAfterReload"; internal const string UvxPathOverride = "MCPForUnity.UvxPath"; diff --git a/MCPForUnity/Editor/Services/EditorStateCache.cs b/MCPForUnity/Editor/Services/EditorStateCache.cs index 579f5043b..52a6e237e 100644 --- a/MCPForUnity/Editor/Services/EditorStateCache.cs +++ b/MCPForUnity/Editor/Services/EditorStateCache.cs @@ -529,12 +529,32 @@ public static JObject GetSnapshot() } } + // CompilationPipeline.isCompiling is not available on the 2021.3 package floor, hence + // reflection; bound once as a delegate so per-frame callers avoid the reflection invoke. + private static readonly Func PipelineIsCompiling = CreatePipelineIsCompilingDelegate(); + + private static Func CreatePipelineIsCompilingDelegate() + { + try + { + var prop = Type.GetType("UnityEditor.Compilation.CompilationPipeline, UnityEditor") + ?.GetProperty("isCompiling", BindingFlags.Public | BindingFlags.Static); + return prop?.GetMethod != null + ? (Func)prop.GetMethod.CreateDelegate(typeof(Func)) + : null; + } + catch + { + return null; + } + } + /// /// Returns the actual compilation state, working around a known Unity quirk where /// EditorApplication.isCompiling can return false positives in Play mode. /// See: https://github.com/CoplayDev/unity-mcp/issues/549 /// - private static bool GetActualIsCompiling() + internal static bool GetActualIsCompiling() { // If EditorApplication.isCompiling is false, Unity is definitely not compiling if (!EditorApplication.isCompiling) @@ -543,18 +563,10 @@ private static bool GetActualIsCompiling() } // In Play mode, EditorApplication.isCompiling can have false positives. - // Double-check with CompilationPipeline.isCompiling via reflection. - if (EditorApplication.isPlaying) + // Double-check with CompilationPipeline.isCompiling. + if (EditorApplication.isPlaying && PipelineIsCompiling != null) { - try - { - Type pipeline = Type.GetType("UnityEditor.Compilation.CompilationPipeline, UnityEditor"); - var prop = pipeline?.GetProperty("isCompiling", BindingFlags.Public | BindingFlags.Static); - if (prop != null) - { - return (bool)prop.GetValue(null); - } - } + try { return PipelineIsCompiling(); } catch { // If reflection fails, fall back to EditorApplication.isCompiling diff --git a/MCPForUnity/Editor/Services/HttpAutoStartHandler.cs b/MCPForUnity/Editor/Services/HttpAutoStartHandler.cs index ebc445ffb..ba1dc85d7 100644 --- a/MCPForUnity/Editor/Services/HttpAutoStartHandler.cs +++ b/MCPForUnity/Editor/Services/HttpAutoStartHandler.cs @@ -17,54 +17,193 @@ namespace MCPForUnity.Editor.Services [InitializeOnLoad] internal static class HttpAutoStartHandler { - private const string SessionInitKey = "HttpAutoStartHandler.SessionInitialized"; + internal const string SessionInitKey = "HttpAutoStartHandler.SessionInitialized"; - static HttpAutoStartHandler() + // Set while AutoStartAsync is in flight. A domain reload kills the in-flight task but + // leaves this set, so the next domain load can finish the connect phase without + // re-spawning the server (StartLocalHttpServer would first stop a still-booting process). + internal const string ConnectPendingKey = "HttpAutoStartHandler.ConnectPending"; + + // Bounds the per-frame retry when editor services keep throwing on a fresh launch. + // Plain static, so every domain reload grants a fresh budget. + private const int MaxServiceNotReadyRetries = 300; + private static int _serviceNotReadyRetries; + + internal enum TickDecision { - // SessionState resets on editor process start but persists across domain reloads. - // Only run once per session — let HttpBridgeReloadHandler handle reload-resume cases. - if (SessionState.GetBool(SessionInitKey, false)) return; + DeferBusy, + DeferToResume, + Skip, + ShouldStart, + ShouldReconnect, + } + static HttpAutoStartHandler() + { if (Application.isBatchMode && string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable("UNITY_MCP_ALLOW_BATCH"))) { return; } - // Only check lightweight EditorPrefs here — services like EditorConfigurationCache - // and MCPServiceLocator may not be initialized yet on fresh editor launch. - bool autoStartEnabled = EditorPrefs.GetBool(EditorPrefKeys.AutoStartOnLoad, false); - if (!autoStartEnabled) return; + bool latched = SessionState.GetBool(SessionInitKey, false); + bool connectPending = SessionState.GetBool(ConnectPendingKey, false); - SessionState.SetBool(SessionInitKey, true); + // Cheap pre-check so the common case (auto-start off, nothing pending) costs one + // EditorPrefs read per domain load instead of an update subscription. The pref is + // re-read every domain load, so enabling it takes effect at the next reload. + if (!latched && !connectPending && + !EditorPrefs.GetBool(EditorPrefKeys.AutoStartOnLoad, false)) + { + return; + } + + // Latched with nothing pending: this session already auto-started. + if (latched && !connectPending) + { + return; + } - // Delay to let the editor and services finish initialization. - EditorApplication.delayCall += OnEditorReady; + // Pending EditorApplication.delayCall/update delegates are wiped by domain reloads, + // so the deferred work must NOT latch up front — latching eagerly killed auto-start + // for the whole session whenever startup included a compile (#1229). An update tick + // is reload-safe because this ctor re-arms it on every domain load. + EditorApplication.update += WaitForEditorReady; } - private static void OnEditorReady() + /// + /// Drops a reload-interrupted auto-start connect. Called when the user takes manual + /// control of the bridge lifecycle, so no later domain load revives the connect. + /// + internal static void CancelPendingReconnect() => SessionState.EraseBool(ConnectPendingKey); + + private static void WaitForEditorReady() { - try + switch (TickCore(HttpBridgeReloadHandler.IsEditorBusy())) { - bool autoStartEnabled = EditorPrefs.GetBool(EditorPrefKeys.AutoStartOnLoad, false); - if (!autoStartEnabled) return; + case TickDecision.DeferBusy: + case TickDecision.DeferToResume: + return; // stay registered, try again next tick + + case TickDecision.Skip: + EditorApplication.update -= WaitForEditorReady; + return; + + case TickDecision.ShouldStart: + if (!TryBeginAutoStart()) + { + DeferOrGiveUp(); + return; + } + SessionState.SetBool(SessionInitKey, true); + EditorApplication.update -= WaitForEditorReady; + return; + + case TickDecision.ShouldReconnect: + if (!TryBeginReconnect()) + { + DeferOrGiveUp(); + return; + } + EditorApplication.update -= WaitForEditorReady; + return; + } + } + + // Services may not be initialized on the first frames of a fresh launch; retry next + // tick, but not forever — a persistently broken environment shouldn't churn exceptions + // every frame for the whole session. A later domain reload retries with a fresh budget. + private static void DeferOrGiveUp() + { + if (++_serviceNotReadyRetries < MaxServiceNotReadyRetries) return; + EditorApplication.update -= WaitForEditorReady; + McpLog.Warn("[HTTP Auto-Start] Editor services unavailable; giving up until the next domain reload"); + } + + /// + /// Decision core for the editor-ready tick, separated so EditMode tests can drive it + /// without spawning servers. Never writes the session latch — the caller latches only + /// after the start work actually dispatches. + /// + internal static TickDecision TickCore(bool editorBusy) + { + if (editorBusy) return TickDecision.DeferBusy; + + bool connectPending = SessionState.GetBool(ConnectPendingKey, false); + if (!connectPending) + { + if (SessionState.GetBool(SessionInitKey, false)) return TickDecision.Skip; + + // Only check lightweight EditorPrefs here — heavier services are touched in + // TryBeginAutoStart once the editor is idle. No latch when disabled: the pref + // is re-read on the next domain load. + if (!EditorPrefs.GetBool(EditorPrefKeys.AutoStartOnLoad, false)) return TickDecision.Skip; + } + + // A pending reload-resume owns bridge revival — checked only when we would + // otherwise act, so a plain Skip never waits out the resume window. + if (HttpBridgeReloadHandler.IsResumePending) return TickDecision.DeferToResume; - bool useHttp = EditorConfigurationCache.Instance.UseHttpTransport; - if (!useHttp) return; + return connectPending ? TickDecision.ShouldReconnect : TickDecision.ShouldStart; + } + + /// + /// Returns true when the auto-start decision was actually made (including deliberate + /// early-outs). Returns false when services were not ready yet, so the caller leaves + /// the session latch unset and retries instead of consuming it. + /// + private static bool TryBeginAutoStart() + { + try + { + if (!EditorConfigurationCache.Instance.UseHttpTransport) return true; // Don't auto-start if bridge is already running. - if (MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http)) return; + if (MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http)) return true; _ = AutoStartAsync(); + return true; } catch (Exception ex) { - McpLog.Debug($"[HTTP Auto-Start] Deferred check failed: {ex.Message}"); + McpLog.Debug($"[HTTP Auto-Start] Services not ready: {ex.Message}"); + return false; } } + /// + /// Returns false when services were not ready yet (caller retries). On true the + /// pending reconnect was either dispatched or deliberately dropped (auto-start + /// disabled, transport switched, bridge already running). + /// + internal static bool TryBeginReconnect() + { + bool proceed; + try + { + proceed = EditorPrefs.GetBool(EditorPrefKeys.AutoStartOnLoad, false) + && EditorConfigurationCache.Instance.UseHttpTransport + && !MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http); + } + catch (Exception ex) + { + McpLog.Debug($"[HTTP Auto-Start] Services not ready: {ex.Message}"); + return false; + } + + if (!proceed) + { + SessionState.EraseBool(ConnectPendingKey); + return true; + } + + _ = ReconnectAsync(); + return true; + } + private static async Task AutoStartAsync() { + SessionState.SetBool(ConnectPendingKey, true); try { bool isLocal = !HttpEndpointUtility.IsRemoteScope(); @@ -104,13 +243,46 @@ private static async Task AutoStartAsync() { McpLog.Warn($"[HTTP Auto-Start] Failed: {ex.Message}"); } + finally + { + // Reached on every terminal outcome. A domain reload that kills the task + // mid-flight skips this, leaving the key set for the reconnect path. + SessionState.EraseBool(ConnectPendingKey); + } + } + + /// + /// Finishes an auto-start whose connect phase was killed by a domain reload. + /// Connect-only: never spawns a server — the previous domain already did. + /// + private static async Task ReconnectAsync() + { + try + { + if (HttpEndpointUtility.IsRemoteScope()) + { + await ConnectBridgeAsync(); + return; + } + + await WaitForServerAndConnectAsync(); + } + catch (Exception ex) + { + McpLog.Warn($"[HTTP Auto-Start] Post-reload reconnect failed: {ex.Message}"); + } + finally + { + SessionState.EraseBool(ConnectPendingKey); + } } /// /// Waits for the local HTTP server to accept connections, then connects the bridge. - /// Mirrors TryAutoStartSessionAsync in McpConnectionSection: keep polling reachability while - /// the launched process is alive; declare failure only when it exits without the port coming - /// up, or a generous hard cap is reached. + /// Mirrors TryAutoStartSessionAsync in McpConnectionSection: while a managed launch + /// process is alive, keep polling reachability and declare failure only when it exits + /// without the port coming up. Without a launch handle (post-reload reconnect, or a + /// server started externally) there is nothing to watch die, so poll to the hard cap. /// private static async Task WaitForServerAndConnectAsync() { @@ -139,10 +311,12 @@ private static async Task WaitForServerAndConnectAsync() } } - bool processAlive = server.IsManagedServerLaunchProcessAlive(); double elapsed = EditorApplication.timeSinceStartup - startTime; + bool launchProcessDied = server.HasManagedServerLaunchHandle + && !server.IsManagedServerLaunchProcessAlive() + && elapsed > 1.0; - if ((!processAlive && elapsed > 1.0) || elapsed > hardCap.TotalSeconds) + if (launchProcessDied || elapsed > hardCap.TotalSeconds) { // Last-resort connect attempt in case reachability detection missed a live server. if (await MCPServiceLocator.Bridge.StartAsync()) diff --git a/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs b/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs index 58bba54fa..4ce38688e 100644 --- a/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs +++ b/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs @@ -1,6 +1,5 @@ using System; using System.Threading.Tasks; -using MCPForUnity.Editor.Constants; using MCPForUnity.Editor.Helpers; using MCPForUnity.Editor.Services.Transport; using MCPForUnity.Editor.Windows; @@ -14,6 +13,13 @@ namespace MCPForUnity.Editor.Services [InitializeOnLoad] internal static class HttpBridgeReloadHandler { + // SessionState, not EditorPrefs: it survives domain reloads but dies with the editor + // process and is per-editor-instance. EditorPrefs is per-user machine-global, so a + // second open editor could consume or delete this editor's pending resume, and a + // crash mid-compile would leave a stale flag that resurrects the bridge on the next + // launch (#1229). + internal const string ResumeSessionKey = "MCPForUnity.ResumeHttpAfterReload"; + private static readonly TimeSpan[] ResumeRetrySchedule = { TimeSpan.Zero, @@ -26,32 +32,36 @@ internal static class HttpBridgeReloadHandler static HttpBridgeReloadHandler() { + // Migration: the flag lived in EditorPrefs before it moved to SessionState — the + // key STRING is shared, so renaming ResumeSessionKey would silently break this + // cleanup. Once per session, not per reload: the EditorPrefs key is machine-global, + // and an older-version editor open concurrently still uses it for its own resume. + // Safe to delete this block a few releases after v10. + const string migratedKey = "MCPForUnity.ResumeHttpAfterReload.Migrated"; + if (!SessionState.GetBool(migratedKey, false)) + { + EditorPrefs.DeleteKey(ResumeSessionKey); + SessionState.SetBool(migratedKey, true); + } + AssemblyReloadEvents.beforeAssemblyReload += OnBeforeAssemblyReload; AssemblyReloadEvents.afterAssemblyReload += OnAfterAssemblyReload; } + internal static bool IsResumePending => SessionState.GetBool(ResumeSessionKey, false); + + /// + /// Drops a pending reload-resume. Called when the user takes manual control of the + /// bridge lifecycle (Connect, End Session, transport switch, orphan cleanup); the + /// retry loop re-checks the flag per attempt, so this also aborts an in-flight loop. + /// + internal static void CancelPendingResume() => SessionState.EraseBool(ResumeSessionKey); + private static void OnBeforeAssemblyReload() { try { - var transport = MCPServiceLocator.TransportManager; - bool shouldResume = transport.IsRunning(TransportMode.Http); - - if (shouldResume) - { - EditorPrefs.SetBool(EditorPrefKeys.ResumeHttpAfterReload, true); - } - else - { - EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); - } - - if (shouldResume) - { - // beforeAssemblyReload is synchronous; force a synchronous teardown so we do not - // leave an orphaned socket due to an unfinished async close handshake. - transport.ForceStop(TransportMode.Http); - } + OnBeforeAssemblyReloadCore(MCPServiceLocator.TransportManager); } catch (Exception ex) { @@ -59,63 +69,91 @@ private static void OnBeforeAssemblyReload() } } - private static void OnAfterAssemblyReload() + internal static void OnBeforeAssemblyReloadCore(TransportManager transport) { - bool resume = false; - try - { - // Only resume HTTP if it is still the selected transport. - bool useHttp = EditorConfigurationCache.Instance.UseHttpTransport; - resume = useHttp && EditorPrefs.GetBool(EditorPrefKeys.ResumeHttpAfterReload, false); - if (resume) - { - EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); - } - } - catch (Exception ex) + if (transport.IsRunning(TransportMode.Http)) { - McpLog.Warn($"Failed to read HTTP bridge reload flag: {ex.Message}"); - resume = false; + SessionState.SetBool(ResumeSessionKey, true); + + // beforeAssemblyReload is synchronous; force a synchronous teardown so we do not + // leave an orphaned socket due to an unfinished async close handshake. + transport.ForceStop(TransportMode.Http); } + // When the bridge is not running, leave any pending flag alone: during a multi-pass + // compile the next reload lands before the deferred resume ran, and deleting the + // flag here is what used to lose the resume permanently (#1229). Explicit cancel + // paths (End Session, transport switch, orphan cleanup) erase the flag instead. + } - if (!resume) + private static void OnAfterAssemblyReload() + { + if (OnAfterAssemblyReloadCore()) { - return; + EditorApplication.update += ResumeTick; } + } - // If the editor is not compiling, attempt an immediate restart without relying on editor focus. - bool isCompiling = EditorApplication.isCompiling; + /// + /// Decision core, separated so EditMode tests can drive it. Returns true when a resume + /// should be scheduled. Does not consume the flag — it survives until the resume + /// succeeds, is cancelled, or exhausts its retries, so a further reload in the middle + /// of any deferral re-enters here instead of losing the resume. + /// + internal static bool OnAfterAssemblyReloadCore() + { try { - var pipeline = Type.GetType("UnityEditor.Compilation.CompilationPipeline, UnityEditor"); - var prop = pipeline?.GetProperty("isCompiling", System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Static); - if (prop != null) isCompiling |= (bool)prop.GetValue(null); - } - catch { } + if (!SessionState.GetBool(ResumeSessionKey, false)) return false; - if (!isCompiling) + // Only resume HTTP if it is still the selected transport. + if (!EditorConfigurationCache.Instance.UseHttpTransport) + { + SessionState.EraseBool(ResumeSessionKey); + return false; + } + + return true; + } + catch (Exception ex) { - _ = ResumeHttpWithRetriesAsync(); - return; + // Transport-config read failed (services racing the reload boundary): schedule + // the resume anyway rather than dropping it — the retry loop re-checks the + // transport per attempt and erases the flag itself on cancel/switch/exhaustion, + // so a stale flag cannot wedge the session. + McpLog.Warn($"Failed to read HTTP bridge reload flag: {ex.Message}"); + return true; } + } - // Fallback when compiling: schedule on the editor loop - EditorApplication.delayCall += () => - { - _ = ResumeHttpWithRetriesAsync(); - }; + private static void ResumeTick() + { + if (IsEditorBusy()) return; + EditorApplication.update -= ResumeTick; + _ = ResumeHttpWithRetriesAsync(); } - private static async Task ResumeHttpWithRetriesAsync() + /// + /// Busy gate for the deferral ticks. Uses the #549-aware compiling check because raw + /// EditorApplication.isCompiling stays true for a whole play session under the + /// "Recompile After Finished Playing" preference, which would block resume until + /// play mode exits. + /// + internal static bool IsEditorBusy() + => EditorStateCache.GetActualIsCompiling() || EditorApplication.isUpdating; + + // scheduleOverride lets EditMode tests pass an all-zero schedule so the loop + // completes synchronously (the test framework floor cannot run async tests). + internal static async Task ResumeHttpWithRetriesAsync(TimeSpan[] scheduleOverride = null) { + TimeSpan[] schedule = scheduleOverride ?? ResumeRetrySchedule; Exception lastException = null; - for (int i = 0; i < ResumeRetrySchedule.Length; i++) + for (int i = 0; i < schedule.Length; i++) { int attempt = i + 1; - McpLog.Debug($"[HTTP Reload] Resume attempt {attempt}/{ResumeRetrySchedule.Length}"); + McpLog.Debug($"[HTTP Reload] Resume attempt {attempt}/{schedule.Length}"); - TimeSpan delay = ResumeRetrySchedule[i]; + TimeSpan delay = schedule[i]; if (delay > TimeSpan.Zero) { McpLog.Debug($"[HTTP Reload] Waiting {delay.TotalSeconds:0.#}s before resume attempt {attempt}"); @@ -123,9 +161,21 @@ private static async Task ResumeHttpWithRetriesAsync() catch { return; } } + // The flag doubles as the cancel signal (see CancelPendingResume). + if (!IsResumePending) return; + // Abort retries if the user switched transports while we were waiting. if (!EditorConfigurationCache.Instance.UseHttpTransport) { + SessionState.EraseBool(ResumeSessionKey); + return; + } + + // Never bounce a session someone else established while we were waiting + // (WebSocketTransportClient.StartAsync tears down a live connection first). + if (MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http)) + { + SessionState.EraseBool(ResumeSessionKey); return; } @@ -134,6 +184,7 @@ private static async Task ResumeHttpWithRetriesAsync() bool started = await MCPServiceLocator.TransportManager.StartAsync(TransportMode.Http); if (started) { + SessionState.EraseBool(ResumeSessionKey); McpLog.Debug($"[HTTP Reload] Resume succeeded on attempt {attempt}"); MCPForUnityEditorWindow.RequestHealthVerification(); return; @@ -150,6 +201,11 @@ private static async Task ResumeHttpWithRetriesAsync() } } + // Exhausted: erase the flag so later reload boundaries don't replay this failure + // loop for the rest of the session. This cannot swallow a multi-pass resume — a + // reload mid-loop kills the task before this line, leaving the flag set. + SessionState.EraseBool(ResumeSessionKey); + if (lastException != null) { McpLog.Warn($"Failed to resume HTTP MCP bridge after domain reload: {lastException.Message}"); diff --git a/MCPForUnity/Editor/Services/IServerManagementService.cs b/MCPForUnity/Editor/Services/IServerManagementService.cs index 990ab323f..60f90b04d 100644 --- a/MCPForUnity/Editor/Services/IServerManagementService.cs +++ b/MCPForUnity/Editor/Services/IServerManagementService.cs @@ -30,6 +30,13 @@ public interface IServerManagementService /// bool IsManagedServerLaunchProcessAlive(); + /// + /// True when this domain launched the local HTTP server and still holds its Process + /// handle. Handles do not survive domain reloads, so false also means "unknown" — + /// callers should wait rather than fail fast. + /// + bool HasManagedServerLaunchHandle { get; } + /// /// Writes a launch-failure report to the Console (Error): the tail of the launch log, /// the log path, and a copy-command hint pointing at the Manual Server Launch foldout. diff --git a/MCPForUnity/Editor/Services/ServerManagementService.cs b/MCPForUnity/Editor/Services/ServerManagementService.cs index 1d943265d..da627ae63 100644 --- a/MCPForUnity/Editor/Services/ServerManagementService.cs +++ b/MCPForUnity/Editor/Services/ServerManagementService.cs @@ -1002,6 +1002,8 @@ private string GetLocalHttpServerLaunchLogPath(int port) return Path.Combine(dir, $"server-launch-{port}.log"); } + public bool HasManagedServerLaunchHandle => _lastLaunchedProcess != null; + public bool IsManagedServerLaunchProcessAlive() { try diff --git a/MCPForUnity/Editor/Services/Transport/TransportManager.cs b/MCPForUnity/Editor/Services/Transport/TransportManager.cs index dd6ccf872..f7761420f 100644 --- a/MCPForUnity/Editor/Services/Transport/TransportManager.cs +++ b/MCPForUnity/Editor/Services/Transport/TransportManager.cs @@ -16,6 +16,8 @@ public class TransportManager private TransportState _stdioState = TransportState.Disconnected("stdio"); private Func _webSocketFactory; private Func _stdioFactory; + private Task _httpStartTask; + private Task _stdioStartTask; public TransportManager() { @@ -42,7 +44,25 @@ private IMcpTransportClient GetOrCreateClient(TransportMode mode) }; } - public async Task StartAsync(TransportMode mode) + public Task StartAsync(TransportMode mode) + { + // Editor-main-thread only (no locking needed). Coalesce concurrent starts for the + // same mode: manual Connect, reload-resume, and auto-start can otherwise race, and + // WebSocketTransportClient.StartAsync tears down a live connection first — two + // interleaved starts bounce each other's session. + Task inFlight = mode == TransportMode.Http ? _httpStartTask : _stdioStartTask; + if (inFlight != null && !inFlight.IsCompleted) + { + return inFlight; + } + + Task started = StartCoreAsync(mode); + if (mode == TransportMode.Http) _httpStartTask = started; + else _stdioStartTask = started; + return started; + } + + private async Task StartCoreAsync(TransportMode mode) { IMcpTransportClient client = GetOrCreateClient(mode); diff --git a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs index 64a0c0212..be0cebc0f 100644 --- a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs +++ b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs @@ -189,7 +189,8 @@ private void RegisterCallbacks() // Clear any stale resume flags when user manually changes transport try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); } catch { } - try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); } catch { } + HttpBridgeReloadHandler.CancelPendingResume(); + HttpAutoStartHandler.CancelPendingReconnect(); if (useHttp) { @@ -799,7 +800,8 @@ private async void OnConnectionToggleClicked() // 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 { } + HttpBridgeReloadHandler.CancelPendingResume(); + HttpAutoStartHandler.CancelPendingReconnect(); await bridgeService.StopAsync(); if (httpRemoteForLog) @@ -834,6 +836,12 @@ private async void OnConnectionToggleClicked() McpLog.Info($"Connecting to {HttpEndpointUtility.GetRemoteBaseUrl()}…"); } + // The user is taking over bridge lifecycle: drop any pending reload-resume + // or interrupted auto-start reconnect so neither can bounce the session + // we are about to establish. + HttpBridgeReloadHandler.CancelPendingResume(); + HttpAutoStartHandler.CancelPendingReconnect(); + bool started = await bridgeService.StartAsync(); if (started) { @@ -888,7 +896,8 @@ private async Task EndOrphanedSessionAsync() // Clear resume flags to prevent getting stuck in "Resuming..." state try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); } catch { } - try { EditorPrefs.DeleteKey(EditorPrefKeys.ResumeHttpAfterReload); } catch { } + HttpBridgeReloadHandler.CancelPendingResume(); + HttpAutoStartHandler.CancelPendingReconnect(); await MCPServiceLocator.Bridge.StopAsync(); } diff --git a/MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs b/MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs index 0d858bdba..3d011362a 100644 --- a/MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs +++ b/MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs @@ -32,7 +32,6 @@ public class EditorPrefsWindow : EditorWindow // Boolean prefs { EditorPrefKeys.DebugLogs, EditorPrefType.Bool }, { EditorPrefKeys.UseHttpTransport, EditorPrefType.Bool }, - { EditorPrefKeys.ResumeHttpAfterReload, EditorPrefType.Bool }, { EditorPrefKeys.ResumeStdioAfterReload, EditorPrefType.Bool }, { EditorPrefKeys.UseEmbeddedServer, EditorPrefType.Bool }, { EditorPrefKeys.LockCursorConfig, EditorPrefType.Bool }, diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs new file mode 100644 index 000000000..f4229bf2c --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs @@ -0,0 +1,194 @@ +using NUnit.Framework; +using MCPForUnity.Editor.Constants; +using MCPForUnity.Editor.Services; +using MCPForUnity.Editor.Services.Transport; +using UnityEditor; + +namespace MCPForUnityTests.Editor.Services +{ + /// + /// Tests for the auto-start tick decisions (#1229): the session latch must only be + /// written when the deferred work actually dispatches, so a domain reload that wipes + /// the pending tick can no longer consume the once-per-session auto-start. + /// TickCore is a pure decision — it never writes the latch and never spawns servers. + /// The TryBeginReconnect tests only exercise its deliberate-drop paths, which never + /// dispatch the async connect. + /// + public class HttpAutoStartHandlerTests + { + private FakeTransportClient _fakeClient; + private TransportManager _savedManager; + private bool _savedLatch; + private bool _savedConnectPending; + private bool _savedResumeFlag; + private bool _savedAutoStart; + private bool _savedUseHttpTransport; + + [SetUp] + public void SetUp() + { + _savedLatch = SessionState.GetBool(HttpAutoStartHandler.SessionInitKey, false); + _savedConnectPending = SessionState.GetBool(HttpAutoStartHandler.ConnectPendingKey, false); + _savedResumeFlag = SessionState.GetBool(HttpBridgeReloadHandler.ResumeSessionKey, false); + _savedAutoStart = EditorPrefs.GetBool(EditorPrefKeys.AutoStartOnLoad, false); + _savedUseHttpTransport = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); + _savedManager = MCPServiceLocator.TransportManager; + + SessionState.EraseBool(HttpAutoStartHandler.SessionInitKey); + SessionState.EraseBool(HttpAutoStartHandler.ConnectPendingKey); + SessionState.EraseBool(HttpBridgeReloadHandler.ResumeSessionKey); + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, false); + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, true); + EditorConfigurationCache.Instance.Refresh(); + + _fakeClient = new FakeTransportClient(); + var manager = new TransportManager(); + manager.Configure(() => _fakeClient, () => _fakeClient); + MCPServiceLocator.Register(manager); + } + + [TearDown] + public void TearDown() + { + // Stored-false and absent are indistinguishable: every read uses GetBool(key, false). + SessionState.SetBool(HttpAutoStartHandler.SessionInitKey, _savedLatch); + SessionState.SetBool(HttpAutoStartHandler.ConnectPendingKey, _savedConnectPending); + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, _savedResumeFlag); + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, _savedAutoStart); + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, _savedUseHttpTransport); + EditorConfigurationCache.Instance.Refresh(); + MCPServiceLocator.Register(_savedManager); + } + + private static bool LatchSet => + SessionState.GetBool(HttpAutoStartHandler.SessionInitKey, false); + + private static bool ConnectPendingSet => + SessionState.GetBool(HttpAutoStartHandler.ConnectPendingKey, false); + + [Test] + public void TickCore_EditorBusy_Defers() + { + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, true); + + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.DeferBusy, + HttpAutoStartHandler.TickCore(editorBusy: true)); + Assert.IsFalse(LatchSet); + } + + [Test] + public void TickCore_ResumePending_YieldsToReloadHandler() + { + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, true); + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.DeferToResume, + HttpAutoStartHandler.TickCore(editorBusy: false)); + Assert.IsFalse(LatchSet); + } + + [Test] + public void TickCore_ResumePendingButNothingToDo_SkipsWithoutWaiting() + { + // Auto-start disabled and not latched: a pending resume must not keep the + // tick alive when the only possible outcome is Skip. + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.Skip, + HttpAutoStartHandler.TickCore(editorBusy: false)); + } + + [Test] + public void TickCore_AutoStartDisabled_SkipsWithoutLatch() + { + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.Skip, + HttpAutoStartHandler.TickCore(editorBusy: false)); + Assert.IsFalse(LatchSet, "no latch when disabled — the pref is re-read on the next domain load"); + } + + [Test] + public void TickCore_AutoStartEnabled_ShouldStartWithoutWritingLatch() + { + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, true); + + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.ShouldStart, + HttpAutoStartHandler.TickCore(editorBusy: false)); + Assert.IsFalse(LatchSet, "the caller latches only after the start work actually dispatches"); + } + + [Test] + public void TickCore_Latched_Skips() + { + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, true); + SessionState.SetBool(HttpAutoStartHandler.SessionInitKey, true); + + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.Skip, + HttpAutoStartHandler.TickCore(editorBusy: false)); + } + + [Test] + public void TickCore_LatchedWithConnectPending_Reconnects() + { + SessionState.SetBool(HttpAutoStartHandler.SessionInitKey, true); + SessionState.SetBool(HttpAutoStartHandler.ConnectPendingKey, true); + + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.ShouldReconnect, + HttpAutoStartHandler.TickCore(editorBusy: false), + "a reload that killed the in-flight connect should finish connect-only, never re-spawn"); + } + + [Test] + public void TickCore_ResumePendingBeatsReconnect() + { + SessionState.SetBool(HttpAutoStartHandler.SessionInitKey, true); + SessionState.SetBool(HttpAutoStartHandler.ConnectPendingKey, true); + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Assert.AreEqual( + HttpAutoStartHandler.TickDecision.DeferToResume, + HttpAutoStartHandler.TickCore(editorBusy: false), + "the reload handler owns bridge revival while a resume is pending"); + } + + [Test] + public void TryBeginReconnect_AutoStartDisabled_DropsPendingReconnect() + { + SessionState.SetBool(HttpAutoStartHandler.ConnectPendingKey, true); + + Assert.IsTrue(HttpAutoStartHandler.TryBeginReconnect()); + Assert.IsFalse(ConnectPendingSet, "a deliberate drop must consume the pending marker"); + } + + [Test] + public void TryBeginReconnect_StdioSelected_DropsPendingReconnect() + { + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, true); + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, false); + EditorConfigurationCache.Instance.Refresh(); + SessionState.SetBool(HttpAutoStartHandler.ConnectPendingKey, true); + + Assert.IsTrue(HttpAutoStartHandler.TryBeginReconnect()); + Assert.IsFalse(ConnectPendingSet); + } + + [Test] + public void TryBeginReconnect_BridgeAlreadyRunning_DropsPendingReconnect() + { + EditorPrefs.SetBool(EditorPrefKeys.AutoStartOnLoad, true); + var start = MCPServiceLocator.TransportManager.StartAsync(TransportMode.Http); + Assert.IsTrue(start.IsCompleted && start.Result, "fake bridge should start synchronously"); + SessionState.SetBool(HttpAutoStartHandler.ConnectPendingKey, true); + + Assert.IsTrue(HttpAutoStartHandler.TryBeginReconnect()); + Assert.IsFalse(ConnectPendingSet); + Assert.AreEqual(1, _fakeClient.StartCalls, "an already-running bridge must not be restarted"); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs.meta new file mode 100644 index 000000000..be7fe6905 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b295bffe6b1f4006bd4aa5061932ec0f +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs new file mode 100644 index 000000000..83e042d71 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs @@ -0,0 +1,196 @@ +using System; +using System.Threading.Tasks; +using NUnit.Framework; +using MCPForUnity.Editor.Constants; +using MCPForUnity.Editor.Services; +using MCPForUnity.Editor.Services.Transport; +using UnityEditor; + +namespace MCPForUnityTests.Editor.Services +{ + /// + /// Tests for the HTTP reload-resume flag semantics (#1229): the flag must survive + /// multi-pass compile boundaries and only be consumed on success, cancel, or exhaustion. + /// Uses fake transports and a zero-delay retry schedule so every path completes + /// synchronously (UTF 1.1 cannot run async tests). + /// + public class HttpBridgeReloadHandlerTests + { + private static readonly TimeSpan[] ZeroSchedule = + { + TimeSpan.Zero, TimeSpan.Zero, TimeSpan.Zero + }; + + private FakeTransportClient _fakeClient; + private TransportManager _manager; + private TransportManager _savedManager; + private bool _savedResumeFlag; + private bool _savedUseHttpTransport; + + [SetUp] + public void SetUp() + { + _savedResumeFlag = SessionState.GetBool(HttpBridgeReloadHandler.ResumeSessionKey, false); + _savedUseHttpTransport = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); + _savedManager = MCPServiceLocator.TransportManager; + + SessionState.EraseBool(HttpBridgeReloadHandler.ResumeSessionKey); + + _fakeClient = new FakeTransportClient(); + _manager = new TransportManager(); + _manager.Configure(() => _fakeClient, () => _fakeClient); + MCPServiceLocator.Register(_manager); + + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, true); + EditorConfigurationCache.Instance.Refresh(); + } + + [TearDown] + public void TearDown() + { + // Stored-false and absent are indistinguishable: every read uses GetBool(key, false). + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, _savedResumeFlag); + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, _savedUseHttpTransport); + EditorConfigurationCache.Instance.Refresh(); + MCPServiceLocator.Register(_savedManager); + } + + private static bool ResumeFlagSet => + SessionState.GetBool(HttpBridgeReloadHandler.ResumeSessionKey, false); + + private void StartBridge() + { + Task start = _manager.StartAsync(TransportMode.Http); + Assert.IsTrue(start.IsCompleted && start.Result, "fake bridge should start synchronously"); + } + + [Test] + public void BeforeReloadCore_BridgeRunning_SetsFlagAndForceStops() + { + StartBridge(); + + HttpBridgeReloadHandler.OnBeforeAssemblyReloadCore(_manager); + + Assert.IsTrue(ResumeFlagSet, "flag should be set when the bridge was running"); + Assert.IsFalse(_manager.IsRunning(TransportMode.Http), "bridge should be force-stopped before reload"); + } + + [Test] + public void BeforeReloadCore_BridgeNotRunning_PreservesPendingFlag() + { + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + HttpBridgeReloadHandler.OnBeforeAssemblyReloadCore(_manager); + + Assert.IsTrue(ResumeFlagSet, + "a pending resume must survive a reload boundary where the bridge is down (#1229 multi-pass compile)"); + } + + [Test] + public void AfterReloadCore_NoFlag_DoesNotResume() + { + Assert.IsFalse(HttpBridgeReloadHandler.OnAfterAssemblyReloadCore()); + } + + [Test] + public void AfterReloadCore_FlagSetHttpSelected_ResumesAndKeepsFlag() + { + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Assert.IsTrue(HttpBridgeReloadHandler.OnAfterAssemblyReloadCore()); + Assert.IsTrue(ResumeFlagSet, "flag is only consumed when the resume actually succeeds"); + } + + [Test] + public void AfterReloadCore_FlagSetStdioSelected_ClearsFlagAndSkips() + { + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, false); + EditorConfigurationCache.Instance.Refresh(); + + Assert.IsFalse(HttpBridgeReloadHandler.OnAfterAssemblyReloadCore()); + Assert.IsFalse(ResumeFlagSet, "switching transports cancels the pending resume"); + } + + [Test] + public void Resume_Success_ConnectsAndClearsFlag() + { + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Task resume = HttpBridgeReloadHandler.ResumeHttpWithRetriesAsync(ZeroSchedule); + + Assert.IsTrue(resume.IsCompleted, "resume should complete synchronously with fakes"); + Assert.IsTrue(_manager.IsRunning(TransportMode.Http)); + Assert.AreEqual(1, _fakeClient.StartCalls); + Assert.IsFalse(ResumeFlagSet); + } + + [Test] + public void Resume_Exhaustion_ClearsFlagAfterAllAttempts() + { + _fakeClient.StartResult = false; + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Task resume = HttpBridgeReloadHandler.ResumeHttpWithRetriesAsync(ZeroSchedule); + + Assert.IsTrue(resume.IsCompleted, "resume should complete synchronously with fakes"); + Assert.AreEqual(ZeroSchedule.Length, _fakeClient.StartCalls); + Assert.IsFalse(ResumeFlagSet, + "exhaustion erases the flag so later reload boundaries don't replay the failure loop"); + } + + [Test] + public void Resume_FlagErasedMidLoop_StopsRetrying() + { + _fakeClient.StartResult = false; + // Simulates End Session / transport switch cancelling while a retry is in flight. + _fakeClient.OnStart = () => SessionState.EraseBool(HttpBridgeReloadHandler.ResumeSessionKey); + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Task resume = HttpBridgeReloadHandler.ResumeHttpWithRetriesAsync(ZeroSchedule); + + Assert.IsTrue(resume.IsCompleted, "resume should complete synchronously with fakes"); + Assert.AreEqual(1, _fakeClient.StartCalls, "erasing the flag must abort the retry loop"); + } + + [Test] + public void Resume_BridgeAlreadyRunning_ClearsFlagWithoutRestart() + { + StartBridge(); + SessionState.SetBool(HttpBridgeReloadHandler.ResumeSessionKey, true); + + Task resume = HttpBridgeReloadHandler.ResumeHttpWithRetriesAsync(ZeroSchedule); + + Assert.IsTrue(resume.IsCompleted, "resume should complete synchronously with fakes"); + Assert.AreEqual(1, _fakeClient.StartCalls, + "a session established while the resume waited must not be bounced"); + Assert.IsFalse(ResumeFlagSet); + } + + [Test] + public void Scenario_MultiPassCompile_ResumesAtSecondBoundary() + { + // Pass 1 begins: bridge running when the first reload hits. + StartBridge(); + HttpBridgeReloadHandler.OnBeforeAssemblyReloadCore(_manager); + Assert.IsTrue(ResumeFlagSet); + Assert.IsFalse(_manager.IsRunning(TransportMode.Http)); + + // After pass 1: still compiling, so the resume tick defers — flag must not be consumed. + Assert.IsTrue(HttpBridgeReloadHandler.OnAfterAssemblyReloadCore()); + Assert.IsTrue(ResumeFlagSet); + + // Pass 2 begins with the bridge down. The old code deleted the flag at this + // boundary, which is exactly how #1229 lost the resume permanently. + HttpBridgeReloadHandler.OnBeforeAssemblyReloadCore(_manager); + Assert.IsTrue(ResumeFlagSet); + + // After pass 2: editor idle, resume runs and reconnects. + Assert.IsTrue(HttpBridgeReloadHandler.OnAfterAssemblyReloadCore()); + Task resume = HttpBridgeReloadHandler.ResumeHttpWithRetriesAsync(ZeroSchedule); + Assert.IsTrue(resume.IsCompleted, "resume should complete synchronously with fakes"); + Assert.IsTrue(_manager.IsRunning(TransportMode.Http)); + Assert.IsFalse(ResumeFlagSet); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs.meta new file mode 100644 index 000000000..03460efc1 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 45fcbd8bbcd2411fbef0b27859c77465 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs new file mode 100644 index 000000000..d253f7b43 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs @@ -0,0 +1,66 @@ +using System.Threading.Tasks; +using NUnit.Framework; +using MCPForUnity.Editor.Services.Transport; + +namespace MCPForUnityTests.Editor.Services +{ + /// + /// Pins TransportManager.StartAsync's coalescing contract: concurrent starts for the + /// same mode share one in-flight attempt instead of racing — a second StartAsync would + /// otherwise tear down the first connection mid-handshake (manual Connect vs the + /// reload-resume/auto-start loops). + /// + public class TransportManagerTests + { + private sealed class PendingTransportClient : IMcpTransportClient + { + public readonly TaskCompletionSource Pending = new TaskCompletionSource(); + public int StartCalls; + + public bool IsConnected => false; + public string TransportName => "http"; + public TransportState State { get; } = TransportState.Disconnected("http"); + + public Task StartAsync() + { + StartCalls++; + return Pending.Task; + } + + public Task StopAsync() => Task.CompletedTask; + public Task VerifyAsync() => Task.FromResult(false); + public Task ReregisterToolsAsync() => Task.CompletedTask; + } + + [Test] + public void StartAsync_ConcurrentCallsSameMode_CoalesceIntoOneAttempt() + { + var client = new PendingTransportClient(); + var manager = new TransportManager(); + manager.Configure(() => client, () => client); + + Task first = manager.StartAsync(TransportMode.Http); + Task second = manager.StartAsync(TransportMode.Http); + + Assert.AreEqual(1, client.StartCalls, "concurrent starts must share one client attempt"); + Assert.AreSame(first, second, "the in-flight task is returned to concurrent callers"); + + client.Pending.SetResult(true); // let the shared attempt finish + } + + [Test] + public void StartAsync_AfterCompletedStart_StartsFresh() + { + var client = new FakeTransportClient(); + var manager = new TransportManager(); + manager.Configure(() => client, () => client); + + Task first = manager.StartAsync(TransportMode.Http); + Assert.IsTrue(first.IsCompleted && first.Result, "fake start should complete synchronously"); + + Task second = manager.StartAsync(TransportMode.Http); + Assert.AreEqual(2, client.StartCalls, "a completed start must not block later restarts"); + Assert.IsTrue(second.IsCompleted && second.Result); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs.meta new file mode 100644 index 000000000..9df17b776 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b019ec9ec1df4b6eb345e6f2a2c13915 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs index f1d76c2e7..8abb32a8e 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs @@ -1,6 +1,8 @@ using System; using System.Collections; using System.IO; +using System.Threading.Tasks; +using MCPForUnity.Editor.Services.Transport; using Newtonsoft.Json.Linq; using NUnit.Framework; using UnityEditor; @@ -136,5 +138,45 @@ public static void CleanupEmptyParentFolders(string folderPath) } } } -} + /// + /// Synchronous IMcpTransportClient fake for transport-lifecycle tests: StartAsync + /// completes immediately with StartResult so retry loops run without awaits + /// (the test framework floor cannot run async tests). + /// + public sealed class FakeTransportClient : IMcpTransportClient + { + public bool StartResult = true; + public int StartCalls; + public Action OnStart; + + public bool IsConnected { get; private set; } + public string TransportName => "http"; + public TransportState State { get; private set; } + = TransportState.Disconnected("http"); + + public Task StartAsync() + { + StartCalls++; + OnStart?.Invoke(); + IsConnected = StartResult; + State = StartResult + ? TransportState.Connected("http") + : TransportState.Disconnected("http", "fake start failure"); + return Task.FromResult(StartResult); + } + + public Task StopAsync() + { + IsConnected = false; + State = TransportState.Disconnected("http"); + return Task.CompletedTask; + } + + public Task VerifyAsync() + => Task.FromResult(IsConnected); + + public Task ReregisterToolsAsync() + => Task.CompletedTask; + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs index d5c45113c..817b31f8e 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs @@ -245,7 +245,7 @@ public void McpConnectionSection_UsesEnumFieldValueChangedCallback_ForTransport( "1. Get previous and new transport values", "2. Persist UseHttpTransport to EditorPrefs", "3. Persist HttpTransportScope if HTTP", - "4. Clear resume flags (ResumeStdioAfterReload, ResumeHttpAfterReload)", + "4. Clear resume flags (ResumeStdioAfterReload in EditorPrefs, HTTP resume key in SessionState)", "5. Update UI visibility", "6. Invoke OnManualConfigUpdateRequested event", "7. Invoke OnTransportChanged event", From d42c7d352d7a64d97a860462f04d2e4280d22112 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:48:07 -0700 Subject: [PATCH 2/2] fix(editor): track compilation via pipeline events; harden StartAsync mode + retry-loop races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups: - CompilationPipeline.isCompiling does not exist on the supported Unity range (reflection probe on 2021.3 and 6000.4: neither public nor non-public), so the reflected play-mode double-check never resolved and GetActualIsCompiling silently fell back to the raw signal — the #549 false positive was never actually mitigated. Track compilationStarted/compilationFinished events instead (public across the range); in Play mode trust the event-tracked state, outside it the raw signal is reliable. - TransportManager.StartAsync validates the mode before touching the coalescing slots, matching the class's unsupported-mode contract instead of routing unknown values to the stdio slot. - The resume retry loop's transport/IsRunning pre-checks moved inside the attempt try: a service read racing the reload boundary now burns a retry instead of killing the fire-and-forget task with the flag still set. --- .../Editor/Services/EditorStateCache.cs | 51 ++++++++----------- .../Services/HttpBridgeReloadHandler.cs | 32 +++++++----- .../Services/Transport/TransportManager.cs | 7 ++- 3 files changed, 45 insertions(+), 45 deletions(-) diff --git a/MCPForUnity/Editor/Services/EditorStateCache.cs b/MCPForUnity/Editor/Services/EditorStateCache.cs index 52a6e237e..54625c2e4 100644 --- a/MCPForUnity/Editor/Services/EditorStateCache.cs +++ b/MCPForUnity/Editor/Services/EditorStateCache.cs @@ -259,6 +259,12 @@ static EditorStateCache() EditorApplication.update += OnUpdate; EditorApplication.playModeStateChanged += _ => ForceUpdate("playmode"); + // Tracks whether an assembly compilation is actually running, for + // GetActualIsCompiling's Play-mode check. Statics reset on domain reload + // and this [InitializeOnLoad] ctor re-subscribes, so the flag is per-domain. + UnityEditor.Compilation.CompilationPipeline.compilationStarted += _ => _pipelineCompilationRunning = true; + UnityEditor.Compilation.CompilationPipeline.compilationFinished += _ => _pipelineCompilationRunning = false; + AssemblyReloadEvents.beforeAssemblyReload += () => { _domainReloadPending = true; @@ -529,30 +535,18 @@ public static JObject GetSnapshot() } } - // CompilationPipeline.isCompiling is not available on the 2021.3 package floor, hence - // reflection; bound once as a delegate so per-frame callers avoid the reflection invoke. - private static readonly Func PipelineIsCompiling = CreatePipelineIsCompilingDelegate(); - - private static Func CreatePipelineIsCompilingDelegate() - { - try - { - var prop = Type.GetType("UnityEditor.Compilation.CompilationPipeline, UnityEditor") - ?.GetProperty("isCompiling", BindingFlags.Public | BindingFlags.Static); - return prop?.GetMethod != null - ? (Func)prop.GetMethod.CreateDelegate(typeof(Func)) - : null; - } - catch - { - return null; - } - } + // Set/cleared by the CompilationPipeline.compilationStarted/Finished events + // subscribed in the static ctor. NOTE: CompilationPipeline.isCompiling does not + // exist on the supported Unity range (verified by reflection probe on 2021.3 and + // 6000.4 — neither public nor non-public), so the reflection this replaced never + // resolved and always fell back to the raw signal. + private static bool _pipelineCompilationRunning; /// /// Returns the actual compilation state, working around a known Unity quirk where - /// EditorApplication.isCompiling can return false positives in Play mode. - /// See: https://github.com/CoplayDev/unity-mcp/issues/549 + /// EditorApplication.isCompiling can return false positives in Play mode (e.g. a + /// recompile deferred by Recompile-After-Finished-Playing keeps it true for the + /// whole play session). See: https://github.com/CoplayDev/unity-mcp/issues/549 /// internal static bool GetActualIsCompiling() { @@ -562,18 +556,15 @@ internal static bool GetActualIsCompiling() return false; } - // In Play mode, EditorApplication.isCompiling can have false positives. - // Double-check with CompilationPipeline.isCompiling. - if (EditorApplication.isPlaying && PipelineIsCompiling != null) + // In Play mode, trust the event-tracked pipeline state instead: a deferred + // recompile keeps EditorApplication.isCompiling true without any compilation + // actually running. + if (EditorApplication.isPlaying) { - try { return PipelineIsCompiling(); } - catch - { - // If reflection fails, fall back to EditorApplication.isCompiling - } + return _pipelineCompilationRunning; } - // Outside Play mode or if reflection failed, trust EditorApplication.isCompiling + // Outside Play mode the raw signal is reliable. return true; } } diff --git a/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs b/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs index 4ce38688e..937483ee4 100644 --- a/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs +++ b/MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs @@ -164,23 +164,27 @@ internal static async Task ResumeHttpWithRetriesAsync(TimeSpan[] scheduleOverrid // The flag doubles as the cancel signal (see CancelPendingResume). if (!IsResumePending) return; - // Abort retries if the user switched transports while we were waiting. - if (!EditorConfigurationCache.Instance.UseHttpTransport) + try { - SessionState.EraseBool(ResumeSessionKey); - return; - } + // Inside the attempt try: a service read racing the reload boundary must + // burn a retry, not kill this fire-and-forget task with the flag still set + // (which would leave nothing scheduled to consume it until the next reload). - // Never bounce a session someone else established while we were waiting - // (WebSocketTransportClient.StartAsync tears down a live connection first). - if (MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http)) - { - SessionState.EraseBool(ResumeSessionKey); - return; - } + // Abort retries if the user switched transports while we were waiting. + if (!EditorConfigurationCache.Instance.UseHttpTransport) + { + SessionState.EraseBool(ResumeSessionKey); + return; + } + + // Never bounce a session someone else established while we were waiting + // (WebSocketTransportClient.StartAsync tears down a live connection first). + if (MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http)) + { + SessionState.EraseBool(ResumeSessionKey); + return; + } - try - { bool started = await MCPServiceLocator.TransportManager.StartAsync(TransportMode.Http); if (started) { diff --git a/MCPForUnity/Editor/Services/Transport/TransportManager.cs b/MCPForUnity/Editor/Services/Transport/TransportManager.cs index f7761420f..bc4d6814c 100644 --- a/MCPForUnity/Editor/Services/Transport/TransportManager.cs +++ b/MCPForUnity/Editor/Services/Transport/TransportManager.cs @@ -50,7 +50,12 @@ public Task StartAsync(TransportMode mode) // same mode: manual Connect, reload-resume, and auto-start can otherwise race, and // WebSocketTransportClient.StartAsync tears down a live connection first — two // interleaved starts bounce each other's session. - Task inFlight = mode == TransportMode.Http ? _httpStartTask : _stdioStartTask; + Task inFlight = mode switch + { + TransportMode.Http => _httpStartTask, + TransportMode.Stdio => _stdioStartTask, + _ => throw new ArgumentOutOfRangeException(nameof(mode), mode, "Unsupported transport mode"), + }; if (inFlight != null && !inFlight.IsCompleted) { return inFlight;