diff --git a/MCPForUnity/Editor/Services/EditorStateCache.cs b/MCPForUnity/Editor/Services/EditorStateCache.cs index d02b26528..1efb8b9a6 100644 --- a/MCPForUnity/Editor/Services/EditorStateCache.cs +++ b/MCPForUnity/Editor/Services/EditorStateCache.cs @@ -1,4 +1,5 @@ using System; +using System.Globalization; using System.Reflection; using MCPForUnity.Editor.Helpers; using Newtonsoft.Json; @@ -22,8 +23,24 @@ internal static class EditorStateCache private static long _observedUnixMs; private static bool _lastIsCompiling; - private static long? _lastCompileStartedUnixMs; - private static long? _lastCompileFinishedUnixMs; + + // Compile edges live in SessionState, recorded from the CompilationPipeline + // events, rather than in statics sampled off the update tick. Two reasons, + // both load-bearing: + // + // - A successful compile ends in a domain reload that wipes every static in + // this class, including the "was compiling" flag the falling edge was + // derived from. The finish of the very compile a client is waiting on was + // therefore unobservable: both timestamps read null afterwards, so nothing + // downstream could tell "finished" from "never started" (issue #814). + // - The events fire at the true edges. Sampling quantised them to the 1s + // update throttle and dropped any compile shorter than one tick entirely. + // + // SessionState survives domain reloads and dies with the editor session, + // which is exactly the lifetime these values describe. + private const string CompileStartedKey = "MCPForUnity.EditorState.CompileStartedUnixMs"; + private const string CompileFinishedKey = "MCPForUnity.EditorState.CompileFinishedUnixMs"; + private const string CompileCountKey = "MCPForUnity.EditorState.CompileCount"; private static bool _domainReloadPending; private static long? _domainReloadBeforeUnixMs; @@ -262,8 +279,24 @@ static EditorStateCache() // Tracks whether an assembly compilation is actually running, for // GetActualIsCompiling. 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; + // The timestamps beside it are not per-domain — see the SessionState + // note on CompileStartedKey. + UnityEditor.Compilation.CompilationPipeline.compilationStarted += _ => + { + _pipelineCompilationRunning = true; + SetSessionUnixMs(CompileStartedKey, DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()); + SessionState.SetInt(CompileCountKey, SessionState.GetInt(CompileCountKey, 0) + 1); + ForceUpdate("compilation_started"); + }; + UnityEditor.Compilation.CompilationPipeline.compilationFinished += _ => + { + _pipelineCompilationRunning = false; + // Fires before the domain reload, which is what makes the finish + // observable at all: the write lands while this domain is alive and + // is read back by the next one. + SetSessionUnixMs(CompileFinishedKey, DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()); + ForceUpdate("compilation_finished"); + }; AssemblyReloadEvents.beforeAssemblyReload += () => { @@ -378,14 +411,6 @@ private static JObject BuildSnapshot(string reason) _observedUnixMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); bool isCompiling = GetActualIsCompiling(); - if (isCompiling && !_lastIsCompiling) - { - _lastCompileStartedUnixMs = _observedUnixMs; - } - else if (!isCompiling && _lastIsCompiling) - { - _lastCompileFinishedUnixMs = _observedUnixMs; - } _lastIsCompiling = isCompiling; var scene = EditorSceneManager.GetActiveScene(); @@ -458,8 +483,8 @@ private static JObject BuildSnapshot(string reason) { IsCompiling = isCompiling, IsDomainReloadPending = _domainReloadPending, - LastCompileStartedUnixMs = _lastCompileStartedUnixMs, - LastCompileFinishedUnixMs = _lastCompileFinishedUnixMs, + LastCompileStartedUnixMs = GetSessionUnixMs(CompileStartedKey), + LastCompileFinishedUnixMs = GetSessionUnixMs(CompileFinishedKey), LastDomainReloadBeforeUnixMs = _domainReloadBeforeUnixMs, LastDomainReloadAfterUnixMs = _domainReloadAfterUnixMs }, @@ -535,6 +560,27 @@ public static JObject GetSnapshot() } } + /// + /// Compilations begun this editor session, surviving domain reloads. Callers + /// that trigger a compile snapshot this first, then wait for it to move — the + /// only signal that separates "a compile ran" from "one never started", which + /// reads as idle either way. + /// + internal static int CompileCount => SessionState.GetInt(CompileCountKey, 0); + + internal static long? GetSessionUnixMs(string key) + { + string raw = SessionState.GetString(key, string.Empty); + return long.TryParse(raw, NumberStyles.Integer, CultureInfo.InvariantCulture, out long value) + ? value + : (long?)null; + } + + // SessionState has no long overload, so these round-trip through an + // invariant string rather than losing precision through int or float. + internal static void SetSessionUnixMs(string key, long value) + => SessionState.SetString(key, value.ToString(CultureInfo.InvariantCulture)); + // 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 diff --git a/MCPForUnity/Editor/Tools/RefreshUnity.cs b/MCPForUnity/Editor/Tools/RefreshUnity.cs index e35237d80..01fbff001 100644 --- a/MCPForUnity/Editor/Tools/RefreshUnity.cs +++ b/MCPForUnity/Editor/Tools/RefreshUnity.cs @@ -18,6 +18,18 @@ public static class RefreshUnity { private const int DefaultWaitTimeoutSeconds = 60; + /// Backstop on the wait for compilation to begin. Not the normal + /// exit: RequestScriptCompilation records a pending request that the + /// editor drains into a pipeline run on a later tick whether or not any + /// source changed — "recompiles those scripts which require it" in the docs + /// describes per-assembly skipping inside that run, not a run that is skipped. + /// With nothing changed, 6000.3 still raises compilationStarted/Finished + /// (~100 ms, cached) and reloads the domain. The grace only bounds the cases + /// where the run never begins: the pipeline refusing to start on a setup + /// error, or play mode with "Recompile After Finished Playing" deferring it + /// until exit. Both are reported as compile_started = false. + private const int CompileStartGraceSeconds = 10; + public static async Task HandleCommand(JObject @params) { string mode = @params?["mode"]?.ToString() ?? "if_dirty"; @@ -36,6 +48,7 @@ public static async Task HandleCommand(JObject @params) bool refreshTriggered = false; bool compileRequested = false; + int compileCountBefore = EditorStateCache.CompileCount; try { @@ -77,6 +90,34 @@ public static async Task HandleCommand(JObject @params) return new ErrorResponse($"refresh_failed: {ex.Message}"); } + // RequestScriptCompilation only queues; the pipeline starts on a later + // editor tick. Sampling the state here therefore reported "idle" for a + // compile that was about to run, and the caller's readiness poll — which + // begins the moment this returns — saw a ready editor and returned + // immediately, so wait_for_ready silently did nothing for exactly the call + // it exists for (issue #814). Waiting for the start edge first makes + // resulting_state, and every readiness decision downstream of it, truthful. + // + // Unlike WaitForUnityReadyAsync this cannot span a domain reload: it + // resolves the moment compilation *starts*, long before assemblies swap. + // That is why it is safe on Unity 6+ where waiting for readiness is not. + // + // Gated on wait_for_ready: that flag is documented as the non-blocking + // switch, and this wait is a wait — cheap when the pipeline starts on the + // next tick, but a full grace when it never does. A caller who opted out + // of waiting gets the immediate return and the poll hint. + // + // compile_started is null when nothing was waited for (no compile + // requested, or wait_for_ready=false), so "not observed" never reads as + // "did not start". + bool? compileStarted = null; + if (compileRequested && waitForReady) + { + compileStarted = await WaitForCompilationToStartAsync( + compileCountBefore, + TimeSpan.FromSeconds(CompileStartGraceSeconds)).ConfigureAwait(true); + } + // Unity 6+ fix: Skip wait_for_ready when compile was requested. // The EditorApplication.update polling in WaitForUnityReadyAsync doesn't survive // domain reloads properly in Unity 6+, causing infinite compilation loops. @@ -100,6 +141,7 @@ await WaitForUnityReadyAsync( { refresh_triggered = refreshTriggered, compile_requested = compileRequested, + compile_started = compileStarted, resulting_state = "unknown", }); } @@ -117,6 +159,7 @@ await WaitForUnityReadyAsync( { refresh_triggered = refreshTriggered, compile_requested = compileRequested, + compile_started = compileStarted, resulting_state = resultingState, hint = shouldWaitForReady ? "Unity refresh completed; editor should be ready." @@ -124,6 +167,92 @@ await WaitForUnityReadyAsync( }); } + /// + /// Resolves true once a compilation is under way, or false once + /// the grace elapsed without one. Two of the three exits are a start: + /// + /// the pipeline is running; + /// moved past + /// — a short compile can begin and end + /// inside AssetDatabase.Refresh, before this is even armed, and the counter is + /// the only thing that still sees it; + /// the grace elapsed with neither — the pipeline declined or deferred + /// the request (see ). + /// + /// The first two are also tested synchronously on entry, so the case where a + /// reload is already imminent never leaves this command queued as a + /// continuation — see the note on the fast path below. The running case is + /// observed from and + /// completed so that the caller's continuations run inline in that handler, + /// for the reason given at the completion source. + /// + internal static Task WaitForCompilationToStartAsync(int compileCountBefore, TimeSpan grace) + { + // Synchronous fast path, and the reason it matters: the counter check is + // there for a compile that began *and ended* inside AssetDatabase.Refresh + // above, and in that state the domain reload is already imminent. Resolving + // it from a later tick would hand the rest of this command to the + // synchronization context as a queued continuation, which the reload + // discards along with the rest of the domain — losing the response. An + // already-completed task resumes the await inline instead, so nothing is + // left queued. + if (EditorStateCache.CompileCount != compileCountBefore + || EditorStateCache.GetActualIsCompiling()) + { + return Task.FromResult(true); + } + + // Resolved from the compilationStarted event itself, not from a poll, and + // deliberately *without* RunContinuationsAsynchronously. Both matter for + // the same reason: the response has to be on the wire before the compile + // finishes, because the domain reload follows compilationFinished directly + // and a cached no-change compile lasts ~110 ms. Every await between here + // and the socket captures Unity's synchronization context; a continuation + // posted to it runs one editor frame later, and there are three of them + // (this method's caller, the CommandRegistry async wrapper, its + // AwaitHandler). Completing the task on the main thread with inlining + // allowed lets the awaiter see the captured context as the current one and + // run all three inline, inside this event handler, so the only hop left + // is the dispatcher's thread-pool send. A poll would also quantise the + // edge to the update tick, which in an unfocused Editor is most of that + // window on its own. + // + // EditorStateCache subscribed to the same event at domain load, so its + // handler has already flipped GetActualIsCompiling() by the time this one + // runs; the caller reads resulting_state = "compiling" inline. + var tcs = new TaskCompletionSource(); + var start = DateTime.UtcNow; + Action onStarted = null; + EditorApplication.CallbackFunction tick = null; + + onStarted = _ => + { + CompilationPipeline.compilationStarted -= onStarted; + EditorApplication.update -= tick; + tcs.TrySetResult(true); + }; + + // The update hook only carries the grace: the pipeline declined or deferred + // the request, so no reload is coming and inlining is harmless there too. + tick = () => + { + if ((DateTime.UtcNow - start) <= grace) + { + return; + } + + CompilationPipeline.compilationStarted -= onStarted; + EditorApplication.update -= tick; + tcs.TrySetResult(false); + }; + + CompilationPipeline.compilationStarted += onStarted; + EditorApplication.update += tick; + // Nudge Unity to pump once in case update is throttled. + try { EditorApplication.QueuePlayerLoopUpdate(); } catch { } + return tcs.Task; + } + private static Task WaitForUnityReadyAsync(TimeSpan timeout) { var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorStateCacheSessionValuesTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorStateCacheSessionValuesTests.cs new file mode 100644 index 000000000..7f4b26d5c --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorStateCacheSessionValuesTests.cs @@ -0,0 +1,47 @@ +using NUnit.Framework; +using UnityEditor; +using MCPForUnity.Editor.Services; + +namespace MCPForUnityTests.Editor.Services +{ + [TestFixture] + public class EditorStateCacheSessionValuesTests + { + private const string Key = "MCPForUnityTests.EditorStateCache.SessionUnixMs"; + + [SetUp] + public void SetUp() => SessionState.EraseString(Key); + + [TearDown] + public void TearDown() => SessionState.EraseString(Key); + + [Test] + public void SessionUnixMs_RoundTripsValueBeyondInt32() + { + // A unix-ms timestamp does not fit an int; SessionState has no long + // overload, so the value goes through a string and must come back exact. + const long value = 1788652878150L; + Assert.Greater(value, int.MaxValue, "test value must exceed what SessionState.SetInt could hold"); + + EditorStateCache.SetSessionUnixMs(Key, value); + + Assert.AreEqual(value, EditorStateCache.GetSessionUnixMs(Key)); + } + + [Test] + public void GetSessionUnixMs_UnsetKey_ReturnsNull() + { + Assert.IsNull(EditorStateCache.GetSessionUnixMs(Key)); + } + + [TestCase("not-a-number")] + [TestCase("1788652878150.5")] + [TestCase("1,788,652,878,150")] + public void GetSessionUnixMs_MalformedValue_ReturnsNull(string raw) + { + SessionState.SetString(Key, raw); + + Assert.IsNull(EditorStateCache.GetSessionUnixMs(Key)); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorStateCacheSessionValuesTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorStateCacheSessionValuesTests.cs.meta new file mode 100644 index 000000000..7c50d50a8 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorStateCacheSessionValuesTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 8d06aeec365345728e1e5d94458257e3 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RefreshUnityTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RefreshUnityTests.cs new file mode 100644 index 000000000..c2d27cc1e --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RefreshUnityTests.cs @@ -0,0 +1,87 @@ +using System; +using System.Collections; +using Newtonsoft.Json.Linq; +using NUnit.Framework; +using UnityEditor; +using UnityEngine.TestTools; +using MCPForUnity.Editor.Services; +using MCPForUnity.Editor.Tools; +using static MCPForUnityTests.Editor.TestUtilities; + +namespace MCPForUnityTests.Editor.Tools +{ + public class RefreshUnityTests + { + [Test] + public void HandleCommand_CompileNone_NoWait_CompletesSynchronously() + { + // scope=scripts skips AssetDatabase.Refresh, compile=none skips the + // request, wait_for_ready=false skips both waits: nothing on this path + // yields, so the task must already be complete when it is handed back. + var task = RefreshUnity.HandleCommand(new JObject + { + ["mode"] = "if_dirty", + ["scope"] = "scripts", + ["compile"] = "none", + ["wait_for_ready"] = false, + }); + + Assert.IsTrue(task.IsCompleted, "compile=none with wait_for_ready=false must not defer"); + + var result = ToJObject(task.Result); + if (TestRunStatus.IsRunning) + { + // Under the bridge's run_tests the handler short-circuits before the + // refresh logic; that exit is synchronous too, and is all we can check. + Assert.IsFalse(result.Value("success"), result.ToString()); + Assert.AreEqual("tests_running", result["data"]?["reason"]?.ToString(), result.ToString()); + return; + } + + Assert.IsTrue(result.Value("success"), result.ToString()); + var data = result["data"]; + Assert.IsFalse(data.Value("refresh_triggered"), result.ToString()); + Assert.IsFalse(data.Value("compile_requested"), result.ToString()); + Assert.AreEqual(JTokenType.Null, data["compile_started"].Type, + "compile_started must be null when no compile was waited for"); + } + + [Test] + public void WaitForCompilationToStart_CounterAlreadyMoved_CompletesSynchronouslyTrue() + { + // A compile that began and ended inside AssetDatabase.Refresh leaves only + // the counter behind. Presenting a stale "before" value reproduces that + // state without triggering a compile. + var task = RefreshUnity.WaitForCompilationToStartAsync( + EditorStateCache.CompileCount - 1, + TimeSpan.FromSeconds(10)); + + Assert.IsTrue(task.IsCompleted, "counter already moved must resolve without a tick"); + Assert.IsTrue(task.Result, "a moved counter is a start, not a grace expiry"); + } + + [UnityTest] + public IEnumerator WaitForCompilationToStart_GraceElapsed_ResolvesFalse() + { + // No compile is requested here, so with the counter current the only way + // out is the grace. A zero grace expires on the first update tick. + var task = RefreshUnity.WaitForCompilationToStartAsync( + EditorStateCache.CompileCount, + TimeSpan.Zero); + + Assert.IsFalse(task.IsCompleted, "nothing has started, so the wait must actually wait"); + + double deadline = EditorApplication.timeSinceStartup + 5.0; + while (!task.IsCompleted) + { + if (EditorApplication.timeSinceStartup > deadline) + { + Assert.Fail("grace expiry never resolved the wait"); + } + yield return null; + } + + Assert.IsFalse(task.Result, "grace expiry must report that no compile started"); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RefreshUnityTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RefreshUnityTests.cs.meta new file mode 100644 index 000000000..b0f8494e5 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RefreshUnityTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 36e5277cc5884e4da2560eb78ccb1ff9 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: