From 4c648b561070ea637c8e93af0c44574f21b378cf Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Tue, 14 Jul 2026 00:38:51 +0900 Subject: [PATCH 1/4] fix: prevent execute-dynamic-code execution slot leaks after Undo/reload (#1761) Co-authored-by: Cursor --- .../Execution/CommandRunner.cs | 72 ++++++++++----- .../Execution/CommandRunnerExecutionSlot.cs | 67 ++++++++++++++ .../CommandRunnerExecutionSlot.cs.meta | 11 +++ .../Execution/CommandRunnerUndoHooks.cs | 16 ++++ .../Execution/CommandRunnerUndoHooks.cs.meta | 11 +++ .../DynamicCodeExecutionScheduler.cs | 49 ++++++++--- .../DynamicCodeExecutionSchedulerHooks.cs | 11 +++ .../CommandRunnerExecutionSlotTests.cs | 81 +++++++++++++++++ ...micCodeExecutionScheduler.UnitTests.csproj | 1 + .../DynamicCodeExecutionSchedulerTests.cs | 87 ++++++++++++++++++- 10 files changed, 370 insertions(+), 36 deletions(-) create mode 100644 Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs create mode 100644 Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs create mode 100644 Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs.meta create mode 100644 tests/DynamicCodeExecutionScheduler.UnitTests/CommandRunnerExecutionSlotTests.cs diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunner.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunner.cs index 1ee8da37e4..67267c403d 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunner.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunner.cs @@ -15,19 +15,22 @@ namespace io.github.hatayama.UnityCliLoop.FirstPartyTools public class CommandRunner : ICompiledCommandInvoker { private readonly CompiledCommandEntryPointResolver _entryPointResolver; - private bool _isRunning = false; - private CancellationTokenSource _cancellationTokenSource; + private readonly CommandRunnerUndoHooks _undoHooks; + private readonly CommandRunnerExecutionSlot _executionSlot = new(); - public bool IsRunning => _isRunning; + public bool IsRunning => _executionSlot.IsRunning; public CommandRunner() - : this(DynamicCodeServices.CommandEntryPointResolver) + : this(DynamicCodeServices.CommandEntryPointResolver, null) { } - internal CommandRunner(CompiledCommandEntryPointResolver entryPointResolver) + internal CommandRunner( + CompiledCommandEntryPointResolver entryPointResolver, + CommandRunnerUndoHooks undoHooks = null) { _entryPointResolver = entryPointResolver; + _undoHooks = undoHooks; } private static void LogExecutionError(Exception ex, string correlationId) @@ -45,30 +48,34 @@ private static void LogExecutionError(Exception ex, string correlationId) ); } - private CancellationTokenSource CreateCombinedCancellationTokenSource(ExecutionContext context) + private CancellationTokenSource CreateCombinedCancellationTokenSource( + ExecutionContext context, + CancellationTokenSource executionCancellationTokenSource) { return CancellationTokenSource.CreateLinkedTokenSource( - _cancellationTokenSource.Token, + executionCancellationTokenSource.Token, context.CancellationToken ); } public void Cancel() { - _cancellationTokenSource?.Cancel(); + _executionSlot.Cancel(); } public async Task ExecuteAsync(ExecutionContext context) { string correlationId = UnityCliLoopConstants.GenerateCorrelationId(); - if (!TryBeginExecution(out int undoGroup)) + if (!TryBeginExecution(out int undoGroup, out CancellationTokenSource executionCancellationTokenSource)) { return CreateErrorResult(UnityCliLoopConstants.ERROR_MESSAGE_EXECUTION_IN_PROGRESS); } try { - using CancellationTokenSource combinedCts = CreateCombinedCancellationTokenSource(context); + using CancellationTokenSource combinedCts = CreateCombinedCancellationTokenSource( + context, + executionCancellationTokenSource); return await ExecuteInternalAsync(context, combinedCts.Token); } catch (OperationCanceledException) @@ -89,27 +96,48 @@ public async Task ExecuteAsync(ExecutionContext context) } } - private bool TryBeginExecution(out int undoGroup) + private bool TryBeginExecution( + out int undoGroup, + out CancellationTokenSource executionCancellationTokenSource) { - undoGroup = -1; - if (_isRunning) + return _executionSlot.TryBegin(BeginUndoGroup, out undoGroup, out executionCancellationTokenSource); + } + + private void EndExecution(int undoGroup) + { + _executionSlot.End(undoGroup, CollapseUndoGroup); + } + + private int BeginUndoGroup() + { + if (_undoHooks != null) { - return false; + System.Diagnostics.Debug.Assert(_undoHooks.GetCurrentGroup != null, "GetCurrentGroup must be set"); + System.Diagnostics.Debug.Assert( + _undoHooks.SetCurrentGroupName != null, + "SetCurrentGroupName must be set"); + int hookedGroup = _undoHooks.GetCurrentGroup(); + _undoHooks.SetCurrentGroupName("ExecuteDynamicCode"); + return hookedGroup; } - _isRunning = true; - _cancellationTokenSource = new CancellationTokenSource(); - undoGroup = Undo.GetCurrentGroup(); + int undoGroup = Undo.GetCurrentGroup(); Undo.SetCurrentGroupName("ExecuteDynamicCode"); - return true; + return undoGroup; } - private void EndExecution(int undoGroup) + private void CollapseUndoGroup(int undoGroup) { + if (_undoHooks != null) + { + System.Diagnostics.Debug.Assert( + _undoHooks.CollapseUndoOperations != null, + "CollapseUndoOperations must be set"); + _undoHooks.CollapseUndoOperations(undoGroup); + return; + } + Undo.CollapseUndoOperations(undoGroup); - _isRunning = false; - _cancellationTokenSource?.Dispose(); - _cancellationTokenSource = null; } private static ExecutionResult CreateErrorResult(string errorMessage, List logs = null) diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs new file mode 100644 index 0000000000..cbf7270e20 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs @@ -0,0 +1,67 @@ +using System; +using System.Threading; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Owns CommandRunner single-flight state so Undo failures cannot permanently stick the slot. + /// + internal sealed class CommandRunnerExecutionSlot + { + private bool _isRunning; + private CancellationTokenSource _cancellationTokenSource; + + public bool IsRunning => _isRunning; + + /// + /// Begins an execution slot after undo begin succeeds. + /// Why undo before flipping running: Undo can throw during reload/startup; marking running + /// first permanently rejects later requests as busy. + /// + public bool TryBegin( + Func beginUndoGroup, + out int undoGroup, + out CancellationTokenSource cancellationTokenSource) + { + System.Diagnostics.Debug.Assert(beginUndoGroup != null, "beginUndoGroup must not be null"); + + undoGroup = -1; + cancellationTokenSource = null; + if (_isRunning) + { + return false; + } + + undoGroup = beginUndoGroup(); + _isRunning = true; + _cancellationTokenSource = new CancellationTokenSource(); + cancellationTokenSource = _cancellationTokenSource; + return true; + } + + public void Cancel() + { + _cancellationTokenSource?.Cancel(); + } + + /// + /// Ends the execution slot even when undo collapse throws. + /// Why nested finally: collapse can throw on a torn-down editor; the running flag must clear. + /// + public void End(int undoGroup, Action collapseUndoGroup) + { + System.Diagnostics.Debug.Assert(collapseUndoGroup != null, "collapseUndoGroup must not be null"); + + try + { + collapseUndoGroup(undoGroup); + } + finally + { + _isRunning = false; + _cancellationTokenSource?.Dispose(); + _cancellationTokenSource = null; + } + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs.meta b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs.meta new file mode 100644 index 0000000000..b031ce4eab --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: a41f3ff6ec9d0416fbb05b13786eae57 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs new file mode 100644 index 0000000000..3abd54ea37 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs @@ -0,0 +1,16 @@ +using System; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Injects Undo group operations for CommandRunner so pure C# tests can force begin/end failures. + /// + internal sealed class CommandRunnerUndoHooks + { + public Func GetCurrentGroup { get; set; } + + public Action SetCurrentGroupName { get; set; } + + public Action CollapseUndoOperations { get; set; } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs.meta b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs.meta new file mode 100644 index 0000000000..1d69056bb0 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: dd115e027155f44308442209a6a29209 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.cs index 74e1738de3..d76b589bbf 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.cs @@ -103,10 +103,7 @@ public async Task RunForegroundAsync( } finally { - ClearExecutionState(false, executionCancellationTokenSource); - executionCancellationTokenSource?.Dispose(); - DisposeResourcesIfRequested(); - _executionSemaphore.Release(); + ReleaseExecutionSlot(false, executionCancellationTokenSource); } } @@ -133,10 +130,21 @@ public async Task RunForegroundAsync( return (false, default); } - ThrowIfDisposed(); - executionCancellationTokenSource = - CreateExecutionCancellationTokenSource(cancellationToken); - SetExecutionState(true, executionCancellationTokenSource); + try + { + // Why immediately enter try: Wait succeeded; any throw before Release + // permanently sticks the slot on a still-live scheduler. + _hooks.InvokeAfterYieldSemaphoreAcquired(); + ThrowIfDisposed(); + executionCancellationTokenSource = + CreateExecutionCancellationTokenSource(cancellationToken); + SetExecutionState(true, executionCancellationTokenSource); + } + catch + { + _executionSemaphore.Release(); + throw; + } } try @@ -148,10 +156,7 @@ public async Task RunForegroundAsync( } finally { - ClearExecutionState(true, executionCancellationTokenSource); - executionCancellationTokenSource?.Dispose(); - DisposeResourcesIfRequested(); - _executionSemaphore.Release(); + ReleaseExecutionSlot(true, executionCancellationTokenSource); } } @@ -241,9 +246,27 @@ public async Task ShutdownAsync() } finally { - ClearExecutionState(false, executionCancellationTokenSource); + ReleaseExecutionSlot(false, executionCancellationTokenSource); + } + } + + /// + /// Releases the execution slot even when resource dispose throws. + /// Why nested finally: DisposeResourcesIfRequested runs only after dispose and can throw + /// during reload teardown; skipping Release would stick the slot forever. + /// + private void ReleaseExecutionSlot( + bool yieldToForegroundRequests, + CancellationTokenSource executionCancellationTokenSource) + { + try + { + ClearExecutionState(yieldToForegroundRequests, executionCancellationTokenSource); executionCancellationTokenSource?.Dispose(); DisposeResourcesIfRequested(); + } + finally + { _executionSemaphore.Release(); } } diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionSchedulerHooks.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionSchedulerHooks.cs index e4ab171509..4c5becd9e1 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionSchedulerHooks.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionSchedulerHooks.cs @@ -10,6 +10,12 @@ internal sealed class DynamicCodeExecutionSchedulerHooks { public Action AfterSemaphoreEntered { get; set; } + /// + /// Invoked after the yield path acquires the semaphore and before readiness checks. + /// Why: unit tests must inject failures between Wait and ThrowIfDisposed without Unity. + /// + public Action AfterYieldSemaphoreAcquired { get; set; } + public Func AfterBackgroundExecutionStatePublishedAsync { get; set; } public Func AfterBusySemaphoreProbeFailedAsync { get; set; } @@ -20,6 +26,11 @@ internal sealed class DynamicCodeExecutionSchedulerHooks /// public Action LogWarning { get; set; } + public void InvokeAfterYieldSemaphoreAcquired() + { + AfterYieldSemaphoreAcquired?.Invoke(); + } + public async Task InvokeAfterBackgroundExecutionStatePublishedAsync() { if (AfterBackgroundExecutionStatePublishedAsync == null) diff --git a/tests/DynamicCodeExecutionScheduler.UnitTests/CommandRunnerExecutionSlotTests.cs b/tests/DynamicCodeExecutionScheduler.UnitTests/CommandRunnerExecutionSlotTests.cs new file mode 100644 index 0000000000..e3404df821 --- /dev/null +++ b/tests/DynamicCodeExecutionScheduler.UnitTests/CommandRunnerExecutionSlotTests.cs @@ -0,0 +1,81 @@ +using System; +using System.Threading; +using NUnit.Framework; +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.UnitTests +{ + [TestFixture] + public class CommandRunnerExecutionSlotTests + { + [Test] + public void TryBegin_WhenUndoBeginThrows_ShouldNotLeaveSlotRunning() + { + // Verifies Undo failure before marking running does not permanently stick the slot. + CommandRunnerExecutionSlot slot = new(); + + Assert.That( + () => slot.TryBegin( + () => throw new InvalidOperationException("undo begin failed"), + out int _, + out CancellationTokenSource _), + Throws.TypeOf()); + + Assert.That(slot.IsRunning, Is.False); + Assert.That( + slot.TryBegin(() => 7, out int undoGroup, out CancellationTokenSource cts), + Is.True); + Assert.That(undoGroup, Is.EqualTo(7)); + Assert.That(cts, Is.Not.Null); + slot.End(undoGroup, _ => { }); + Assert.That(slot.IsRunning, Is.False); + } + + [Test] + public void End_WhenUndoCollapseThrows_ShouldClearRunningFlag() + { + // Verifies collapse failures still clear running so later begin can succeed. + CommandRunnerExecutionSlot slot = new(); + Assert.That(slot.TryBegin(() => 3, out int undoGroup, out CancellationTokenSource _), Is.True); + + Assert.That( + () => slot.End(undoGroup, _ => throw new InvalidOperationException("undo collapse failed")), + Throws.TypeOf()); + + Assert.That(slot.IsRunning, Is.False); + Assert.That(slot.TryBegin(() => 4, out int _, out CancellationTokenSource _), Is.True); + slot.End(4, _ => { }); + } + + [Test] + public void TryBegin_WhenAlreadyRunning_ShouldReturnFalseWithoutCallingUndo() + { + // Verifies busy rejection happens before undo begin side effects. + CommandRunnerExecutionSlot slot = new(); + int undoBeginCalls = 0; + Assert.That( + slot.TryBegin( + () => + { + undoBeginCalls++; + return 1; + }, + out int _, + out CancellationTokenSource _), + Is.True); + + Assert.That( + slot.TryBegin( + () => + { + undoBeginCalls++; + return 2; + }, + out int _, + out CancellationTokenSource _), + Is.False); + Assert.That(undoBeginCalls, Is.EqualTo(1)); + slot.End(1, _ => { }); + } + } +} diff --git a/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionScheduler.UnitTests.csproj b/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionScheduler.UnitTests.csproj index e21e08a996..7924142cf0 100644 --- a/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionScheduler.UnitTests.csproj +++ b/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionScheduler.UnitTests.csproj @@ -16,5 +16,6 @@ + diff --git a/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionSchedulerTests.cs b/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionSchedulerTests.cs index 6d796d716d..54c3b584bc 100644 --- a/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionSchedulerTests.cs +++ b/tests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionSchedulerTests.cs @@ -191,10 +191,15 @@ public async Task RunForegroundAsync_WhenExecutionCompletesAfterBusyProbe_Should public void RunForegroundAsync_WhenDisposedAfterSemaphoreAcquire_ShouldThrowObjectDisposedException() { int disposeCalls = 0; + int semaphoreEnteredCount = 0; DynamicCodeExecutionScheduler scheduler = null; DynamicCodeExecutionSchedulerHooks hooks = new() { - AfterSemaphoreEntered = () => scheduler.Dispose() + AfterSemaphoreEntered = () => + { + semaphoreEnteredCount++; + scheduler.Dispose(); + } }; scheduler = CreateScheduler(hooks, () => disposeCalls++); @@ -207,6 +212,86 @@ public void RunForegroundAsync_WhenDisposedAfterSemaphoreAcquire_ShouldThrowObje CancellationToken.None), Throws.InstanceOf()); Assert.That(disposeCalls, Is.EqualTo(1)); + Assert.That(semaphoreEnteredCount, Is.EqualTo(1)); + } + finally + { + scheduler.Dispose(); + } + } + + [Test] + public async Task TryRunIfIdleAsync_WhenSetupThrowsAfterYieldAcquire_ShouldReleaseSlotForLaterForegroundWork() + { + // Verifies yield-path failures after Wait do not leak the semaphore on a live scheduler. + int foregroundEnteredCount = 0; + DynamicCodeExecutionSchedulerHooks hooks = new() + { + AfterYieldSemaphoreAcquired = () => throw new InvalidOperationException("yield setup failed"), + AfterSemaphoreEntered = () => foregroundEnteredCount++ + }; + using DynamicCodeExecutionScheduler scheduler = CreateScheduler(hooks); + + Assert.That( + async () => await scheduler.TryRunIfIdleAsync( + true, + _ => Task.FromResult("background"), + CancellationToken.None), + Throws.TypeOf()); + + string foregroundResult = await scheduler.RunForegroundAsync( + _ => Task.FromResult("foreground"), + () => "busy", + CancellationToken.None); + + Assert.That(foregroundResult, Is.EqualTo("foreground")); + Assert.That(foregroundEnteredCount, Is.EqualTo(1)); + } + + [Test] + public async Task RunForegroundAsync_WhenDisposeResourcesThrows_ShouldStillAllowSemaphoreReentryHook() + { + // Verifies Release stays reachable when deferred resource dispose throws in finally. + // Why not assert CurrentCount via reflection: workspace rules forbid unapproved reflection. + // AfterSemaphoreEntered runs after Wait and before ThrowIfDisposed, so a second acquire + // on a live scheduler is observed by the hook counter; here dispose leaves the instance + // disposed, so we only prove dispose ran and ReleaseExecutionSlot surfaced its throw. + int disposeCalls = 0; + int semaphoreEnteredCount = 0; + DynamicCodeExecutionScheduler scheduler = null; + DynamicCodeExecutionSchedulerHooks hooks = new() + { + AfterSemaphoreEntered = () => + { + semaphoreEnteredCount++; + scheduler.Dispose(); + } + }; + scheduler = CreateScheduler( + hooks, + () => + { + disposeCalls++; + throw new InvalidOperationException("pool dispose failed"); + }); + + try + { + Assert.That( + async () => await scheduler.RunForegroundAsync( + _ => Task.FromResult("foreground"), + () => "busy", + CancellationToken.None), + Throws.TypeOf()); + + Assert.That(disposeCalls, Is.EqualTo(1)); + Assert.That(semaphoreEnteredCount, Is.EqualTo(1)); + Assert.That( + async () => await scheduler.RunForegroundAsync( + _ => Task.FromResult("again"), + () => "busy", + CancellationToken.None), + Throws.InstanceOf()); } finally { From f16dd0c3c866167997a273f08640f086243c6b4a Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Tue, 14 Jul 2026 00:54:47 +0900 Subject: [PATCH 2/4] fix: execute-dynamic-code returns retryable guidance when runtime is disposed mid-reset (#1762) Co-authored-by: Cursor --- .../ExecuteDynamicCodeUseCaseTests.cs | 97 +++++++++++++++++++ .../DynamicCodeExecutionResponseFactory.cs | 39 ++++++++ .../ExecuteDynamicCodeResponse.cs | 10 ++ .../ExecuteDynamicCodeUseCase.cs | 62 ++++++++++-- .../ToolContracts/UnityCliLoopConstants.cs | 12 +++ 5 files changed, 213 insertions(+), 7 deletions(-) diff --git a/Assets/Tests/Editor/DynamicCodeToolTests/ExecuteDynamicCodeUseCaseTests.cs b/Assets/Tests/Editor/DynamicCodeToolTests/ExecuteDynamicCodeUseCaseTests.cs index d3a5ef52b7..887dc5c62d 100644 --- a/Assets/Tests/Editor/DynamicCodeToolTests/ExecuteDynamicCodeUseCaseTests.cs +++ b/Assets/Tests/Editor/DynamicCodeToolTests/ExecuteDynamicCodeUseCaseTests.cs @@ -731,6 +731,56 @@ public async Task ExecuteAsync_WhenKnownCompilePatternExistsOnlyInCompilationErr Assert.That(response.Logs, Contains.Item("Solutions:")); } + [Test] + public async Task ExecuteAsync_WhenRuntimeThrowsObjectDisposedException_ReturnsRetryableRestartingError() + { + // Verifies disposed-runtime ODE during execute maps to Success=false with restarting guidance. + MarkForegroundWarmupCompleted(); + ObjectDisposedDynamicCodeExecutionRuntime runtime = new( + throwOnExecuteCount: 1, + successAfterThrow: false); + ExecuteDynamicCodeUseCase useCase = new(runtime); + ExecuteDynamicCodeResponse response = await useCase.ExecuteAsync( + new ExecuteDynamicCodeSchema + { + Code = "return 1;", + CompileOnly = false + }, + CancellationToken.None); + + Assert.That(response.Success, Is.False); + Assert.That( + response.ErrorMessage, + Is.EqualTo(UnityCliLoopConstants.ERROR_MESSAGE_DYNAMIC_CODE_RUNTIME_RESTARTING)); + Assert.That(response.NextActions, Is.EqualTo(UnityCliLoopConstants.DYNAMIC_CODE_RUNTIME_RESTARTING_NEXT_ACTIONS)); + Assert.That(response.NextActions[0], Does.Contain("Retry").IgnoreCase); + Assert.That(response.NextActions[1], Does.Contain("launch -r")); + Assert.That(response.NextActions[1], Does.Contain("not needed")); + } + + [Test] + public async Task ExecuteAsync_WhenWarmupThrowsObjectDisposedException_ContinuesAsSilentNoOp() + { + // Verifies Warm-path ODE is incomplete warm (no CLI error), while Execute still succeeds afterward. + ObjectDisposedDynamicCodeExecutionRuntime runtime = new( + throwOnExecuteCount: 1, + successAfterThrow: true); + ExecuteDynamicCodeUseCase useCase = new(runtime); + ExecuteDynamicCodeResponse response = await useCase.ExecuteAsync( + new ExecuteDynamicCodeSchema + { + Code = "return \"ok\";", + CompileOnly = false + }, + CancellationToken.None); + + Assert.That(response.Success, Is.True); + Assert.That(response.Result, Is.EqualTo("ok")); + Assert.That(response.ErrorMessage, Is.Null.Or.Empty); + Assert.That(response.NextActions, Is.Null); + Assert.That(runtime.ExecuteCount, Is.GreaterThanOrEqualTo(2)); + } + /// /// Test support type used by editor and play mode fixtures. /// @@ -777,6 +827,53 @@ public Task ExecuteAsync( } } + /// + /// Throws ObjectDisposedException on the Nth ExecuteAsync, optionally returning success afterward. + /// + private sealed class ObjectDisposedDynamicCodeExecutionRuntime : IDynamicCodeExecutionRuntime + { + private readonly int _throwOnExecuteCount; + private readonly bool _successAfterThrow; + + public ObjectDisposedDynamicCodeExecutionRuntime(int throwOnExecuteCount, bool successAfterThrow) + { + _throwOnExecuteCount = throwOnExecuteCount; + _successAfterThrow = successAfterThrow; + } + + public int ExecuteCount { get; private set; } + + public Task ExecuteAsync( + DynamicCodeExecutionRequest request, + CancellationToken cancellationToken = default) + { + ExecuteCount++; + if (ExecuteCount == _throwOnExecuteCount) + { + // ObjectName matches the production scheduler dispose path without requiring type visibility. + throw new ObjectDisposedException("DynamicCodeExecutionScheduler"); + } + + if (!_successAfterThrow) + { + throw new InvalidOperationException("Unexpected ExecuteAsync after ObjectDisposedException"); + } + + return Task.FromResult(new ExecutionResult + { + Success = true, + Result = "ok" + }); + } + + public Task<(bool Entered, ExecutionResult Result)> TryExecuteIfIdleAsync( + DynamicCodeExecutionRequest request, + CancellationToken cancellationToken = default) + { + throw new InvalidOperationException("TryExecuteIfIdleAsync is not used by these tests"); + } + } + private static void MarkForegroundWarmupCompleted() { bool started = DynamicCodeForegroundWarmupState.TryBegin(); diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeExecutionResponseFactory.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeExecutionResponseFactory.cs index e140477806..c9cf55a04b 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeExecutionResponseFactory.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeExecutionResponseFactory.cs @@ -28,6 +28,16 @@ internal static bool IsCancelledResult(ExecutionResult executionResult) StringComparison.Ordinal); } + internal static bool IsRuntimeRestartingResult(ExecutionResult executionResult) + { + return executionResult != null + && !executionResult.Success + && string.Equals( + executionResult.ErrorMessage, + UnityCliLoopConstants.ERROR_MESSAGE_DYNAMIC_CODE_RUNTIME_RESTARTING, + StringComparison.Ordinal); + } + internal static ExecuteDynamicCodeResponse CreateCancelledResponse() { return new ExecuteDynamicCodeResponse @@ -40,6 +50,35 @@ internal static ExecuteDynamicCodeResponse CreateCancelledResponse() }; } + internal static ExecuteDynamicCodeResponse CreateRuntimeRestartingResponse() + { + return new ExecuteDynamicCodeResponse + { + Success = false, + Result = string.Empty, + Logs = new List + { + UnityCliLoopConstants.ERROR_MESSAGE_DYNAMIC_CODE_RUNTIME_RESTARTING + }, + CompilationErrors = new List(), + ErrorMessage = UnityCliLoopConstants.ERROR_MESSAGE_DYNAMIC_CODE_RUNTIME_RESTARTING, + NextActions = UnityCliLoopConstants.DYNAMIC_CODE_RUNTIME_RESTARTING_NEXT_ACTIONS + }; + } + + internal static ExecutionResult CreateRuntimeRestartingExecutionResult() + { + return new ExecutionResult + { + Success = false, + ErrorMessage = UnityCliLoopConstants.ERROR_MESSAGE_DYNAMIC_CODE_RUNTIME_RESTARTING, + Logs = new List + { + UnityCliLoopConstants.ERROR_MESSAGE_DYNAMIC_CODE_RUNTIME_RESTARTING + } + }; + } + internal ExecuteDynamicCodeResponse ConvertExecutionResultToResponse( ExecutionResult result, string originalUserSnippet = null) diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeResponse.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeResponse.cs index 6235bb3a78..1160bbb5bc 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeResponse.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeResponse.cs @@ -57,6 +57,11 @@ public string Error [JsonProperty("DomainReloadWaitRequired")] public bool DomainReloadWaitRequired { get; set; } = false; + /// + /// Optional recovery steps when execution cannot complete automatically. + /// + public string[] NextActions { get; set; } + /// /// Lightweight internal timings for benchmark comparison. /// @@ -93,6 +98,11 @@ public bool ShouldSerializeDomainReloadWaitRequired() { return DomainReloadWaitRequired; } + + public bool ShouldSerializeNextActions() + { + return NextActions != null && NextActions.Length > 0; + } } /// diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeUseCase.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeUseCase.cs index 1e0e8ae2bb..0dcdda6055 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeUseCase.cs @@ -75,6 +75,14 @@ await WarmForegroundExecutionPathIfNeededAsync(parameters, cancellationToken) return cancelledResponse; } + if (DynamicCodeExecutionResponseFactory.IsRuntimeRestartingResult(finalResult)) + { + ExecuteDynamicCodeResponse restartingResponse = + DynamicCodeExecutionResponseFactory.CreateRuntimeRestartingResponse(); + restartingResponse.EmitTimingsInJsonResponse = parameters.IncludeTimings; + return restartingResponse; + } + ExecuteDynamicCodeResponse response = _responseFactory.ConvertExecutionResultToResponse(finalResult, originalCode); response.EmitTimingsInJsonResponse = parameters.IncludeTimings; @@ -143,15 +151,34 @@ private async Task ExecuteRequestAsync( { if (!request.YieldToForegroundRequests) { - return await _runtime.ExecuteAsync(request, cancellationToken).ConfigureAwait(false); + // Why catch here: reset/reload disposes the scheduler mid-flight; map to an explicit + // retryable response instead of leaking ObjectDisposedException across the tool boundary. + // Why not retry inside UseCase: re-fetching a facade races domain reload. + try + { + return await _runtime.ExecuteAsync(request, cancellationToken).ConfigureAwait(false); + } + catch (ObjectDisposedException) + { + LogDynamicCodeRuntimeRestarting("execute"); + return DynamicCodeExecutionResponseFactory.CreateRuntimeRestartingExecutionResult(); + } } - (bool entered, ExecutionResult result) = await _runtime.TryExecuteIfIdleAsync( - request, - cancellationToken).ConfigureAwait(false); - if (entered) + try { - return result; + (bool entered, ExecutionResult result) = await _runtime.TryExecuteIfIdleAsync( + request, + cancellationToken).ConfigureAwait(false); + if (entered) + { + return result; + } + } + catch (ObjectDisposedException) + { + LogDynamicCodeRuntimeRestarting("try_execute_if_idle"); + return DynamicCodeExecutionResponseFactory.CreateRuntimeRestartingExecutionResult(); } return new ExecutionResult @@ -178,7 +205,18 @@ private async Task WarmForegroundExecutionPathIfNeededAsync( bool completed = false; try { - completed = await ExecuteForegroundWarmupSequenceAsync(cancellationToken).ConfigureAwait(false); + // Why catch here: warmup is best-effort; a disposed runtime during reset must not + // surface as a CLI error — treat as incomplete warm (same as entered=false). + try + { + completed = await ExecuteForegroundWarmupSequenceAsync(cancellationToken).ConfigureAwait(false); + } + catch (ObjectDisposedException) + { + LogDynamicCodeRuntimeRestarting("warm"); + completed = false; + } + if (completed) { DynamicCodeForegroundWarmupState.MarkCompleted(); @@ -193,6 +231,16 @@ private async Task WarmForegroundExecutionPathIfNeededAsync( } } + private static void LogDynamicCodeRuntimeRestarting(string path) + { + VibeLogger.LogWarning( + "dynamic_code_runtime_restarting", + "Dynamic-code runtime was disposed during reset or domain reload", + new { path }, + humanNote: "ObjectDisposedException from the execution runtime was mapped at the UseCase boundary", + aiTodo: "Retry the same execute-dynamic-code shortly; do not treat this as a permanent failure"); + } + private async Task ExecuteForegroundWarmupSequenceAsync(CancellationToken ct) { return await DynamicCodeForegroundWarmupRunner.RunForegroundSequenceAsync( diff --git a/Packages/src/Editor/ToolContracts/UnityCliLoopConstants.cs b/Packages/src/Editor/ToolContracts/UnityCliLoopConstants.cs index 37d743ae26..fbefc9b56c 100644 --- a/Packages/src/Editor/ToolContracts/UnityCliLoopConstants.cs +++ b/Packages/src/Editor/ToolContracts/UnityCliLoopConstants.cs @@ -111,11 +111,23 @@ public static UnityEditor.PackageManager.PackageInfo PackageInfo public const string ERROR_MESSAGE_ASSEMBLY_DEFINITION_IMPORT_ERROR = "Assembly Definition or Assembly Reference import error detected. Unity may not start compilation until asmdef or asmref errors are removed."; public const string ERROR_MESSAGE_EXECUTION_IN_PROGRESS = "Another execution is already in progress"; public const string ERROR_MESSAGE_EXECUTION_CANCELLED = "Execution was cancelled or timed out"; + public const string ERROR_MESSAGE_DYNAMIC_CODE_RUNTIME_RESTARTING = + "Dynamic-code runtime was disposed during a server reset or domain reload; retry the same command shortly."; public const string ERROR_MESSAGE_NO_COMPILED_ASSEMBLY = "No compiled assembly provided"; public const string ERROR_MESSAGE_NO_EXECUTE_METHOD = "No Execute method found in compiled assembly"; public const string ERROR_MESSAGE_FAILED_TO_CREATE_INSTANCE = "Failed to create instance of target type"; public const string ERROR_MESSAGE_UNSUPPORTED_SIGNATURE = "Execute method signature not supported"; + /// + /// Recovery steps when execute-dynamic-code hits a disposed runtime during reset/reload. + /// Why this tone: matches compile-wait guidance — retry recovers the result; launch -r is not first-line. + /// + public static readonly string[] DYNAMIC_CODE_RUNTIME_RESTARTING_NEXT_ACTIONS = + { + "Retry the same `uloop execute-dynamic-code` shortly — the bridge is restarting after reset or domain reload.", + "Unity is still running; `uloop launch -r` is not needed for this error.", + }; + public const int COMPILE_START_TIMEOUT_MS = 5000; public const int COMPILE_START_POLL_INTERVAL_MS = 100; public const int COMPILE_FINISH_MISSED_CALLBACK_GRACE_MS = 2000; From d59a625e338c097e26e47be0767f61fcd1da7e72 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Tue, 14 Jul 2026 01:06:46 +0900 Subject: [PATCH 3/4] fix: stop Roslyn worker revive after server reset or reload shutdown (#1763) Co-authored-by: Cursor --- .../SharedRoslynCompilerWorkerHostTests.cs | 108 ++++++++++++++++++ .../SharedRoslynCompilerWorkerHost.cs | 104 +++++++++++++---- .../SharedRoslynCompilerWorkerSession.cs | 19 +++ 3 files changed, 210 insertions(+), 21 deletions(-) diff --git a/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs b/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs index 7f7e3a3c08..91b1f1a977 100644 --- a/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs +++ b/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs @@ -45,6 +45,114 @@ public void Shutdown_WhenWorkerWasNeverStarted_ShouldRemainIdempotent() Assert.DoesNotThrow(() => session.Shutdown(unusedWorkerDirectoryPath)); } + /// + /// Verifies full Shutdown advances lifecycle generation so in-flight retries cannot restart the worker. + /// + [Test] + public void Shutdown_WhenCalled_ShouldAdvanceLifecycleGeneration() + { + SharedRoslynCompilerWorkerSession session = new(); + string unusedWorkerDirectoryPath = Path.Combine( + Path.GetTempPath(), + $"SharedRoslynCompilerWorkerSessionTests_{Guid.NewGuid():N}"); + + int generationBefore = session.ExecuteWithStateLock(session.GetLifecycleGenerationLocked); + session.Shutdown(unusedWorkerDirectoryPath); + int generationAfter = session.ExecuteWithStateLock(session.GetLifecycleGenerationLocked); + + Assert.That(generationBefore, Is.EqualTo(0)); + Assert.That(generationAfter, Is.EqualTo(1)); + Assert.That( + session.ExecuteWithStateLock(() => session.IsLifecycleGenerationCurrentLocked(generationBefore)), + Is.False); + Assert.That( + session.ExecuteWithStateLock(() => session.IsLifecycleGenerationCurrentLocked(generationAfter)), + Is.True); + } + + /// + /// Verifies retry-path process kill keeps the lifecycle open so a replacement worker may start. + /// + [Test] + public void ShutdownProcessLocked_WhenCalledAlone_ShouldStillAllowWorkerRestart() + { + SharedRoslynCompilerWorkerSession session = new(); + Process startedProcess = null; + int startCallCount = 0; + session.SwapProcessStarterForTests(_ => + { + startCallCount++; + startedProcess = new Process(); + return startedProcess; + }); + + try + { + int generationAtStart = session.ExecuteWithStateLock(session.GetLifecycleGenerationLocked); + session.ExecuteWithStateLock(session.ShutdownProcessLocked); + + bool started = session.ExecuteWithStateLock( + () => + { + if (!session.IsLifecycleGenerationCurrentLocked(generationAtStart)) + { + return false; + } + + return session.StartProcessLocked(new ProcessStartInfo()); + }); + + Assert.That(started, Is.True); + Assert.That(startCallCount, Is.EqualTo(1)); + Assert.That( + session.ExecuteWithStateLock(session.GetLifecycleGenerationLocked), + Is.EqualTo(generationAtStart)); + } + finally + { + startedProcess?.Dispose(); + } + } + + /// + /// Verifies a stale lifecycle generation refuses StartProcess after full Shutdown. + /// + [Test] + public void StartProcessLocked_WhenLifecycleGenerationIsStale_ShouldBeDetectableBeforeStart() + { + SharedRoslynCompilerWorkerSession session = new(); + string unusedWorkerDirectoryPath = Path.Combine( + Path.GetTempPath(), + $"SharedRoslynCompilerWorkerSessionTests_{Guid.NewGuid():N}"); + int startCallCount = 0; + session.SwapProcessStarterForTests(_ => + { + startCallCount++; + return new Process(); + }); + + int generationAtStart = session.ExecuteWithStateLock(session.GetLifecycleGenerationLocked); + session.Shutdown(unusedWorkerDirectoryPath); + + bool wouldStart = session.ExecuteWithStateLock( + () => session.IsLifecycleGenerationCurrentLocked(generationAtStart)); + Assert.That(wouldStart, Is.False); + + bool started = session.ExecuteWithStateLock( + () => + { + if (!session.IsLifecycleGenerationCurrentLocked(generationAtStart)) + { + return false; + } + + return session.StartProcessLocked(new ProcessStartInfo()); + }); + + Assert.That(started, Is.False); + Assert.That(startCallCount, Is.Zero); + } + /// /// Verifies replacing the cached worker releases the previously owned process handle. /// diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs index 355cefd48c..565ec0e0e5 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs @@ -59,6 +59,11 @@ public static WorkerAttemptResult RetryableFailure(string failureReason, object { return new WorkerAttemptResult(null, true, failureReason, failureContext); } + + public static WorkerAttemptResult NonRetryableFailure(string failureReason, object failureContext) + { + return new WorkerAttemptResult(null, false, failureReason, failureContext); + } } /// @@ -68,25 +73,41 @@ private sealed class WorkerStartupResult { public bool IsReady { get; } + public bool IsRetryable { get; } + public string FailureReason { get; } public object FailureContext { get; } - private WorkerStartupResult(bool isReady, string failureReason, object failureContext) + private WorkerStartupResult( + bool isReady, + bool isRetryable, + string failureReason, + object failureContext) { IsReady = isReady; + IsRetryable = isRetryable; FailureReason = failureReason; FailureContext = failureContext; } public static WorkerStartupResult Ready() { - return new WorkerStartupResult(true, null, null); + return new WorkerStartupResult(true, false, null, null); } public static WorkerStartupResult Failure(string failureReason, object failureContext) { - return new WorkerStartupResult(false, failureReason, failureContext); + return new WorkerStartupResult(false, true, failureReason, failureContext); + } + + public static WorkerStartupResult ClosedLifecycleFailure() + { + return new WorkerStartupResult( + false, + false, + "shared_worker_lifecycle_closed", + new { reason = "lifecycle_generation_advanced" }); } } @@ -151,11 +172,15 @@ private static async Task TryCompileWithRetriesAsync( Action markBuildFinished, Action incrementBuildCount) { + int lifecycleGenerationAtStart = ServiceValue.ExecuteWithStateLock( + ServiceValue.GetLifecycleGenerationLocked); + for (int attempt = 1; attempt <= SharedCompilerWorkerMaxAttempts; attempt++) { WorkerAttemptResult attemptResult = await TryCompileOnceAsync( requestFilePath, externalCompilerPaths, + lifecycleGenerationAtStart, ct, markBuildStarted, markBuildFinished, @@ -185,14 +210,24 @@ private static async Task TryCompileWithRetriesAsync( private static async Task TryCompileOnceAsync( string requestFilePath, ExternalCompilerPaths externalCompilerPaths, + int lifecycleGenerationAtStart, CancellationToken ct, Action markBuildStarted, Action markBuildFinished, Action incrementBuildCount) { - WorkerStartupResult startupResult = EnsureWorkerReady(externalCompilerPaths); + WorkerStartupResult startupResult = EnsureWorkerReady( + externalCompilerPaths, + lifecycleGenerationAtStart); if (!startupResult.IsReady) { + if (!startupResult.IsRetryable) + { + return WorkerAttemptResult.NonRetryableFailure( + startupResult.FailureReason, + startupResult.FailureContext); + } + return WorkerAttemptResult.RetryableFailure( startupResult.FailureReason, startupResult.FailureContext); @@ -206,11 +241,27 @@ private static async Task TryCompileOnceAsync( incrementBuildCount).ConfigureAwait(false); } - private static WorkerStartupResult EnsureWorkerReady(ExternalCompilerPaths externalCompilerPaths) + private static WorkerStartupResult EnsureWorkerReady( + ExternalCompilerPaths externalCompilerPaths, + int lifecycleGenerationAtStart) { - if (ServiceValue.ExecuteWithStateLock(ServiceValue.HasLiveProcessLocked)) + WorkerStartupResult earlyResult = ServiceValue.ExecuteWithStateLock(() => { - return WorkerStartupResult.Ready(); + if (!ServiceValue.IsLifecycleGenerationCurrentLocked(lifecycleGenerationAtStart)) + { + return WorkerStartupResult.ClosedLifecycleFailure(); + } + + if (ServiceValue.HasLiveProcessLocked()) + { + return WorkerStartupResult.Ready(); + } + + return null; + }); + if (earlyResult != null) + { + return earlyResult; } WorkerPaths workerPaths = CreateWorkerPaths(); @@ -224,7 +275,10 @@ private static WorkerStartupResult EnsureWorkerReady(ExternalCompilerPaths exter return workerAssemblyResult; } - return StartWorkerProcess(externalCompilerPaths, workerPaths); + return StartWorkerProcess( + externalCompilerPaths, + workerPaths, + lifecycleGenerationAtStart); } private static WorkerStartupResult EnsureWorkerAssemblyBuilt( @@ -268,23 +322,31 @@ private static WorkerStartupResult EnsureWorkerAssemblyBuilt( private static WorkerStartupResult StartWorkerProcess( ExternalCompilerPaths externalCompilerPaths, - WorkerPaths workerPaths) + WorkerPaths workerPaths, + int lifecycleGenerationAtStart) { ProcessStartInfo startInfo = CreateWorkerStartInfo(externalCompilerPaths, workerPaths); - bool started = ServiceValue.ExecuteWithStateLock( - () => ServiceValue.StartProcessLocked(startInfo)); - if (!started) + return ServiceValue.ExecuteWithStateLock(() => { - return WorkerStartupResult.Failure( - "worker_start_failed", - new - { - dotnet_host_path = externalCompilerPaths.DotnetHostPath, - worker_assembly_path = workerPaths.AssemblyPath - }); - } + if (!ServiceValue.IsLifecycleGenerationCurrentLocked(lifecycleGenerationAtStart)) + { + return WorkerStartupResult.ClosedLifecycleFailure(); + } - return WorkerStartupResult.Ready(); + bool started = ServiceValue.StartProcessLocked(startInfo); + if (!started) + { + return WorkerStartupResult.Failure( + "worker_start_failed", + new + { + dotnet_host_path = externalCompilerPaths.DotnetHostPath, + worker_assembly_path = workerPaths.AssemblyPath + }); + } + + return WorkerStartupResult.Ready(); + }); } private static async Task InvokeWorkerOnceAsync( diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs index a2f6e1bc65..0b0a643475 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs @@ -27,6 +27,9 @@ private Func private string _workerDirectoryPath; private int _responseTimeoutMilliseconds = SharedRoslynCompilerWorkerLineReader.DefaultResponseTimeoutMilliseconds; + // Why a generation instead of a sticky bool: full Shutdown (reset/reload/quit) must + // invalidate in-flight retry loops, while a later compile after reset must still start a worker. + private int _lifecycleGeneration; /// /// Serializes worker request/response conversations without holding the state lock across awaits. @@ -83,6 +86,18 @@ internal bool HasLiveProcessLocked() return _workerProcess != null && !_workerProcess.HasExited; } + internal int GetLifecycleGenerationLocked() + { + AssertStateLockHeld(); + return _lifecycleGeneration; + } + + internal bool IsLifecycleGenerationCurrentLocked(int expectedLifecycleGeneration) + { + AssertStateLockHeld(); + return _lifecycleGeneration == expectedLifecycleGeneration; + } + internal bool StartProcessLocked(ProcessStartInfo startInfo) { AssertStateLockHeld(); @@ -269,6 +284,10 @@ internal void Shutdown(string fallbackWorkerDirectoryPath) { _coordination.RunShutdownWithoutCompileGate(() => { + // Why advance here (not in ShutdownProcessLocked): retry cleanup kills the process + // so the same compile conversation can start a replacement worker. Server reset / + // reload / quit must invalidate that restart path for in-flight retries only. + _lifecycleGeneration++; ShutdownProcessLocked(); CleanupWorkerDirectoryLocked(fallbackWorkerDirectoryPath); }); From e408f69beccf1951746adf6ce00a1c9f2f37b6ed Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Tue, 14 Jul 2026 01:11:07 +0900 Subject: [PATCH 4/4] fix: keep Unity responsive while building the shared Roslyn worker DLL (#1764) Co-authored-by: Cursor --- .../SharedRoslynCompilerWorkerHostTests.cs | 30 +++++++++++++++++++ ...aredRoslynCompilerWorkerAssemblyBuilder.cs | 19 ++++++++++++ .../SharedRoslynCompilerWorkerHost.cs | 19 +++++++----- .../SharedRoslynCompilerWorkerSession.cs | 29 ++++++++++++++++++ 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs b/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs index 91b1f1a977..b13609d5a5 100644 --- a/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs +++ b/Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs @@ -5,6 +5,7 @@ using System.Text; using System.Threading.Tasks; using NUnit.Framework; +using UnityEditor.Compilation; using io.github.hatayama.UnityCliLoop.FirstPartyTools; @@ -153,6 +154,35 @@ public void StartProcessLocked_WhenLifecycleGenerationIsStale_ShouldBeDetectable Assert.That(startCallCount, Is.Zero); } + /// + /// Verifies the async offload path returns the test-hook build result without requiring a real csc. + /// + [Test] + public async Task CompileWorkerAssemblyAsync_WhenTestHookIsInstalled_ShouldReturnHookResult() + { + SharedRoslynCompilerWorkerSession session = new(); + CompilerMessage[] expectedMessages = + { + new CompilerMessage + { + type = CompilerMessageType.Error, + message = "hooked" + } + }; + session.SwapWorkerAssemblyCompilerForTests( + (paths, sourcePath, assemblyPath, responsePath) => expectedMessages); + + SharedRoslynCompilerWorkerAssemblyBuilder.WorkerAssemblyBuildResult result = + await session.CompileWorkerAssemblyAsync( + externalCompilerPaths: null, + workerSourcePath: "unused.cs", + workerAssemblyPath: "unused.dll", + workerCompileResponseFilePath: "unused.rsp"); + + Assert.That(result.StartedSuccessfully, Is.True); + Assert.That(result.Messages, Is.SameAs(expectedMessages)); + } + /// /// Verifies replacing the cached worker releases the previously owned process handle. /// diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs index dc67e1e254..940a186b0d 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs @@ -133,6 +133,25 @@ internal static WorkerAssemblyBuildResult CompileWorkerAssembly( return WorkerAssemblyBuildResult.Started(compilerMessages); } + /// + /// Runs on the thread pool. + /// Why: the sync WaitForExit(timeout) path can otherwise block the Unity main thread when + /// EnsureWorkerReady runs before the first await of an idle compile-gate acquire. + /// + internal static Task CompileWorkerAssemblyOffMainThreadAsync( + ExternalCompilerPaths externalCompilerPaths, + string workerSourcePath, + string workerAssemblyPath, + string workerCompileResponseFilePath) + { + return Task.Run( + () => CompileWorkerAssembly( + externalCompilerPaths, + workerSourcePath, + workerAssemblyPath, + workerCompileResponseFilePath)); + } + internal static bool WaitForCompilerStreamDrain( Task stdoutTask, Task stderrTask, diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs index 565ec0e0e5..92d6ef7bcd 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs @@ -216,9 +216,9 @@ private static async Task TryCompileOnceAsync( Action markBuildFinished, Action incrementBuildCount) { - WorkerStartupResult startupResult = EnsureWorkerReady( + WorkerStartupResult startupResult = await EnsureWorkerReadyAsync( externalCompilerPaths, - lifecycleGenerationAtStart); + lifecycleGenerationAtStart).ConfigureAwait(false); if (!startupResult.IsReady) { if (!startupResult.IsRetryable) @@ -241,7 +241,7 @@ private static async Task TryCompileOnceAsync( incrementBuildCount).ConfigureAwait(false); } - private static WorkerStartupResult EnsureWorkerReady( + private static async Task EnsureWorkerReadyAsync( ExternalCompilerPaths externalCompilerPaths, int lifecycleGenerationAtStart) { @@ -267,9 +267,9 @@ private static WorkerStartupResult EnsureWorkerReady( WorkerPaths workerPaths = CreateWorkerPaths(); SynchronizeWorkerSource(workerPaths); - WorkerStartupResult workerAssemblyResult = EnsureWorkerAssemblyBuilt( + WorkerStartupResult workerAssemblyResult = await EnsureWorkerAssemblyBuiltAsync( externalCompilerPaths, - workerPaths); + workerPaths).ConfigureAwait(false); if (!workerAssemblyResult.IsReady) { return workerAssemblyResult; @@ -281,7 +281,7 @@ private static WorkerStartupResult EnsureWorkerReady( lifecycleGenerationAtStart); } - private static WorkerStartupResult EnsureWorkerAssemblyBuilt( + private static async Task EnsureWorkerAssemblyBuiltAsync( ExternalCompilerPaths externalCompilerPaths, WorkerPaths workerPaths) { @@ -292,12 +292,15 @@ private static WorkerStartupResult EnsureWorkerAssemblyBuilt( // Why outside state lock: worker DLL compile can take seconds; shutdown must still kill // an already-running shared worker without waiting on this build. + // Why Task.Run (via CompileWorkerAssemblyAsync): WaitForExit(timeout) is synchronous and + // this path can still run on the Unity main thread before the first await when the + // compile gate is acquired without yielding. SharedRoslynCompilerWorkerAssemblyBuilder.WorkerAssemblyBuildResult buildResult = - ServiceValue.CompileWorkerAssembly( + await ServiceValue.CompileWorkerAssemblyAsync( externalCompilerPaths, workerPaths.SourcePath, workerPaths.AssemblyPath, - workerPaths.CompileResponseFilePath); + workerPaths.CompileResponseFilePath).ConfigureAwait(false); if (!buildResult.StartedSuccessfully) { return WorkerStartupResult.Failure( diff --git a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs index 0b0a643475..ad323fec4a 100644 --- a/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs +++ b/Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs @@ -152,6 +152,35 @@ internal SharedRoslynCompilerWorkerAssemblyBuilder.WorkerAssemblyBuildResult Com workerCompileResponseFilePath); } + /// + /// Builds the worker assembly without blocking the caller thread on WaitForExit. + /// Test hooks stay synchronous so EditMode fixtures do not need thread-pool coordination. + /// + internal Task + CompileWorkerAssemblyAsync( + ExternalCompilerPaths externalCompilerPaths, + string workerSourcePath, + string workerAssemblyPath, + string workerCompileResponseFilePath) + { + if (_compileWorkerAssemblyForTests != null) + { + return Task.FromResult(CompileWorkerAssembly( + externalCompilerPaths, + workerSourcePath, + workerAssemblyPath, + workerCompileResponseFilePath)); + } + + // Why not take the state lock here: build stays outside the process lock so shutdown + // can still kill a live worker while this Task.Run is in flight. + return SharedRoslynCompilerWorkerAssemblyBuilder.CompileWorkerAssemblyOffMainThreadAsync( + externalCompilerPaths, + workerSourcePath, + workerAssemblyPath, + workerCompileResponseFilePath); + } + internal void ShutdownProcessLocked() { AssertStateLockHeld();