From 1b77744e5bfe6d2a27fb7a22f4e6b912ac6a3313 Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 9 Jul 2026 14:36:13 +0900 Subject: [PATCH 1/2] Extract server recovery/bind-retry flow into UnityCliLoopServerRecoveryExecutor UnityCliLoopServerControllerService mixed IPC lifecycle orchestration with the coalesced recovery/bind-retry algorithm. Moving StartRecoveryIfNeededAsync and TryBindWithWaitAsync into a dedicated executor improves cohesion: the controller now only owns server lifecycle state, while the executor owns the bind-retry loop. The only shared mutable state (_bridgeServer) is bridged with a Func/Action getter-setter pair rather than passing the whole controller, keeping the executor's dependencies to injected interfaces plus the two delegates. --- .../Server/UnityCliLoopServerController.cs | 168 ++------------ .../UnityCliLoopServerRecoveryExecutor.cs | 210 ++++++++++++++++++ ...UnityCliLoopServerRecoveryExecutor.cs.meta | 11 + 3 files changed, 234 insertions(+), 155 deletions(-) create mode 100644 Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs create mode 100644 Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs.meta diff --git a/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.cs b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.cs index 6041a59791..29b1a983b8 100644 --- a/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.cs +++ b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.cs @@ -1,9 +1,7 @@ using System; using System.Threading; using System.Threading.Tasks; -using System.Net.Sockets; using UnityEditor; -using UnityEngine; using io.github.hatayama.UnityCliLoop.Application; using io.github.hatayama.UnityCliLoop.Domain; @@ -38,7 +36,7 @@ private enum ServerStopIntent private readonly UnityCliLoopServerRecoveryTrackingService _recoveryTrackingService; private readonly IUnityCliLoopServerDomainReloadLifecycle _domainReloadLifecycle; private IUnityCliLoopServerInstance _bridgeServer; - private readonly SemaphoreSlim _startupSemaphore = new SemaphoreSlim(1, 1); + private readonly UnityCliLoopServerRecoveryExecutor _recoveryExecutor; internal UnityCliLoopServerControllerService( IUnityCliLoopServerInstanceFactory serverInstanceFactory, @@ -82,9 +80,18 @@ internal UnityCliLoopServerControllerService( _startupProtectionService = startupProtectionService ?? throw new ArgumentNullException(nameof(startupProtectionService)); _recoveryTrackingService = recoveryTrackingService ?? throw new ArgumentNullException(nameof(recoveryTrackingService)); _domainReloadLifecycle = domainReloadLifecycle ?? throw new ArgumentNullException(nameof(domainReloadLifecycle)); + + _recoveryExecutor = new UnityCliLoopServerRecoveryExecutor( + serverInstanceFactory, + readinessService, + startupProtectionService, + sessionFlagsRepository, + toolRegistrarService, + () => _bridgeServer, + server => _bridgeServer = server); } - private bool IsBackgroundUnityProcess() + internal static bool IsBackgroundUnityProcess() { bool isAssetImportWorker = AssetDatabase.IsAssetImportWorkerProcess(); return isAssetImportWorker; @@ -363,158 +370,9 @@ private void OnServerLoopUnexpectedlyExited() /// Centralized, coalesced recovery start. /// Attempts recovery on the project IPC endpoint for up to 5 seconds. /// - public async Task StartRecoveryIfNeededAsync(bool isAfterCompile, CancellationToken cancellationToken) - { - if (IsBackgroundUnityProcess()) - { - VibeLogger.LogInfo("server_start_ignored", "background_process"); - return; - } - - if (_startupProtectionService.IsStartupProtectionActive()) - { - VibeLogger.LogInfo("server_start_ignored", "startup_protection_active"); - if (_bridgeServer?.IsRunning == true) - { - await _readinessService.MarkServerReadyAsync("startup-protection-active", cancellationToken); - return; - } - - return; - } - - VibeLogger.LogInfo("startup_request", "transport=project_ipc"); - - await _startupSemaphore.WaitAsync(cancellationToken); - try - { - // If any server is already running, ignore this request to prevent double-binding - if (_bridgeServer != null && _bridgeServer.IsRunning) - { - VibeLogger.LogInfo("server_start_ignored", $"already_running endpoint={_bridgeServer.Endpoint}"); - await _readinessService.MarkServerReadyAsync("already-running", cancellationToken); - return; - } - - // Ensure previous instance is fully disposed before trying to bind a new one - if (_bridgeServer != null) - { - try - { - _bridgeServer.Dispose(); - VibeLogger.LogInfo("server_disposed_before_bind", "disposed previous server instance"); - } - catch (Exception ex) - { - VibeLogger.LogWarning("server_dispose_failed", ex.Message); - } - finally - { - _bridgeServer = null; - } - } - - bool started = await TryBindWithWaitAsync( - 5000, - 250, - cancellationToken); - - if (!started) - { - // Ensure session reflects stopped state on failure - _sessionFlagsRepository.ClearServerSession(); - _sessionFlagsRepository.ClearReconnectingFlags(); - string message = "Unity CLI Loop server recovery failed because the project IPC endpoint could not be bound within 5000ms."; - Debug.LogError($"[{UnityCliLoopConstants.PROJECT_NAME}] {message}"); - throw new InvalidOperationException(message); - } - - // Mark running and update settings - SaveRunningServerState(); - - // Clear reconnection-related flags on successful recovery - _sessionFlagsRepository.ClearReconnectingFlags(); - _sessionFlagsRepository.ClearPostCompileReconnectingUI(); - _toolRegistrarService.WarmupRegistry(); - await _readinessService.MarkServerReadyAsync("server-recovery", cancellationToken); - - _startupProtectionService.ActivateStartupProtection(5000); - } - finally - { - _startupSemaphore.Release(); - } - } - - private async Task TryBindWithWaitAsync( - int maxWaitMs, - int stepMs, - CancellationToken cancellationToken) + public Task StartRecoveryIfNeededAsync(bool isAfterCompile, CancellationToken cancellationToken) { - int remainingMs = maxWaitMs; - while (true) - { - VibeLogger.LogInfo("binding_attempt", "transport=project_ipc"); - IUnityCliLoopServerInstance server = null; - try - { - // Defensive: dispose any non-running stale instance before creating a new one - if (_bridgeServer != null && !_bridgeServer.IsRunning) - { - try - { - _bridgeServer.Dispose(); - VibeLogger.LogInfo("server_disposed_before_bind", "disposed stale instance"); - } - catch (Exception ex) - { - VibeLogger.LogWarning("server_dispose_failed", ex.Message); - } - finally - { - _bridgeServer = null; - } - } - - server = _serverInstanceFactory.Create(); - server.StartServer(); - _bridgeServer = server; - VibeLogger.LogInfo( - "binding_success", - "Unity CLI Loop server bound the project IPC endpoint.", - new { endpoint = server.Endpoint }); - return true; - } - catch (Exception ex) - { - // Ensure partially created server is cleaned up on failure - server?.Dispose(); - // Unwrap SocketException details if present - SocketException sockEx = ex as SocketException; - if (ex is InvalidOperationException && ex.InnerException is SocketException innerSock) - { - sockEx = innerSock; - } - - if (sockEx != null) - { - VibeLogger.LogWarning("binding_failed", $"target=project_ipc code={sockEx.SocketErrorCode} hresult={sockEx.HResult} native={sockEx.ErrorCode}"); - } - else - { - VibeLogger.LogWarning("binding_failed", $"target=project_ipc code=Unknown hresult={ex.HResult}"); - } - - if (remainingMs <= 0) - { - return false; - } - - int delay = stepMs <= 0 ? remainingMs : Math.Min(stepMs, remainingMs); - await TimerDelay.Wait(delay, cancellationToken); - remainingMs -= delay; - } - } + return _recoveryExecutor.StartRecoveryIfNeededAsync(isAfterCompile, cancellationToken); } private void SaveRunningServerState() diff --git a/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs new file mode 100644 index 0000000000..8f1d904081 --- /dev/null +++ b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs @@ -0,0 +1,210 @@ +using System; +using System.Net.Sockets; +using System.Threading; +using System.Threading.Tasks; + +using io.github.hatayama.UnityCliLoop.Application; +using io.github.hatayama.UnityCliLoop.Domain; +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.Infrastructure +{ + /// + /// Executes the coalesced, bind-retry recovery flow that (re)binds the project IPC endpoint. + /// + internal sealed class UnityCliLoopServerRecoveryExecutor + { + private readonly IUnityCliLoopServerInstanceFactory _serverInstanceFactory; + private readonly UnityCliLoopServerReadinessService _readinessService; + private readonly UnityCliLoopServerStartupProtectionService _startupProtectionService; + private readonly ISessionFlagsRepository _sessionFlagsRepository; + private readonly UnityCliLoopToolRegistrarService _toolRegistrarService; + private readonly Func _getBridgeServer; + private readonly Action _setBridgeServer; + private readonly SemaphoreSlim _startupSemaphore = new SemaphoreSlim(1, 1); + + internal UnityCliLoopServerRecoveryExecutor( + IUnityCliLoopServerInstanceFactory serverInstanceFactory, + UnityCliLoopServerReadinessService readinessService, + UnityCliLoopServerStartupProtectionService startupProtectionService, + ISessionFlagsRepository sessionFlagsRepository, + UnityCliLoopToolRegistrarService toolRegistrarService, + Func getBridgeServer, + Action setBridgeServer) + { + System.Diagnostics.Debug.Assert(serverInstanceFactory != null, "serverInstanceFactory must not be null"); + System.Diagnostics.Debug.Assert(readinessService != null, "readinessService must not be null"); + System.Diagnostics.Debug.Assert(startupProtectionService != null, "startupProtectionService must not be null"); + System.Diagnostics.Debug.Assert(sessionFlagsRepository != null, "sessionFlagsRepository must not be null"); + System.Diagnostics.Debug.Assert(toolRegistrarService != null, "toolRegistrarService must not be null"); + System.Diagnostics.Debug.Assert(getBridgeServer != null, "getBridgeServer must not be null"); + System.Diagnostics.Debug.Assert(setBridgeServer != null, "setBridgeServer must not be null"); + + _serverInstanceFactory = serverInstanceFactory ?? throw new ArgumentNullException(nameof(serverInstanceFactory)); + _readinessService = readinessService ?? throw new ArgumentNullException(nameof(readinessService)); + _startupProtectionService = startupProtectionService ?? throw new ArgumentNullException(nameof(startupProtectionService)); + _sessionFlagsRepository = sessionFlagsRepository ?? throw new ArgumentNullException(nameof(sessionFlagsRepository)); + _toolRegistrarService = toolRegistrarService ?? throw new ArgumentNullException(nameof(toolRegistrarService)); + _getBridgeServer = getBridgeServer ?? throw new ArgumentNullException(nameof(getBridgeServer)); + _setBridgeServer = setBridgeServer ?? throw new ArgumentNullException(nameof(setBridgeServer)); + } + + /// + /// Centralized, coalesced recovery start. + /// Attempts recovery on the project IPC endpoint for up to 5 seconds. + /// + internal async Task StartRecoveryIfNeededAsync(bool isAfterCompile, CancellationToken ct) + { + if (UnityCliLoopServerControllerService.IsBackgroundUnityProcess()) + { + VibeLogger.LogInfo("server_start_ignored", "background_process"); + return; + } + + if (_startupProtectionService.IsStartupProtectionActive()) + { + VibeLogger.LogInfo("server_start_ignored", "startup_protection_active"); + if (_getBridgeServer()?.IsRunning == true) + { + await _readinessService.MarkServerReadyAsync("startup-protection-active", ct); + return; + } + + return; + } + + VibeLogger.LogInfo("startup_request", "transport=project_ipc"); + + await _startupSemaphore.WaitAsync(ct); + try + { + // If any server is already running, ignore this request to prevent double-binding + if (_getBridgeServer() != null && _getBridgeServer().IsRunning) + { + VibeLogger.LogInfo("server_start_ignored", $"already_running endpoint={_getBridgeServer().Endpoint}"); + await _readinessService.MarkServerReadyAsync("already-running", ct); + return; + } + + // Ensure previous instance is fully disposed before trying to bind a new one + if (_getBridgeServer() != null) + { + try + { + _getBridgeServer().Dispose(); + VibeLogger.LogInfo("server_disposed_before_bind", "disposed previous server instance"); + } + catch (Exception ex) + { + VibeLogger.LogWarning("server_dispose_failed", ex.Message); + } + finally + { + _setBridgeServer(null); + } + } + + bool started = await TryBindWithWaitAsync( + 5000, + 250, + ct); + + if (!started) + { + // Ensure session reflects stopped state on failure + _sessionFlagsRepository.ClearServerSession(); + _sessionFlagsRepository.ClearReconnectingFlags(); + string message = "Unity CLI Loop server recovery failed because the project IPC endpoint could not be bound within 5000ms."; + UnityEngine.Debug.LogError($"[{UnityCliLoopConstants.PROJECT_NAME}] {message}"); + throw new InvalidOperationException(message); + } + + // Mark running and update settings + _sessionFlagsRepository.MarkServerStarted(); + + // Clear reconnection-related flags on successful recovery + _sessionFlagsRepository.ClearReconnectingFlags(); + _sessionFlagsRepository.ClearPostCompileReconnectingUI(); + _toolRegistrarService.WarmupRegistry(); + await _readinessService.MarkServerReadyAsync("server-recovery", ct); + + _startupProtectionService.ActivateStartupProtection(5000); + } + finally + { + _startupSemaphore.Release(); + } + } + + private async Task TryBindWithWaitAsync( + int maxWaitMs, + int stepMs, + CancellationToken ct) + { + int remainingMs = maxWaitMs; + while (true) + { + VibeLogger.LogInfo("binding_attempt", "transport=project_ipc"); + IUnityCliLoopServerInstance server = null; + try + { + // Defensive: dispose any non-running stale instance before creating a new one + if (_getBridgeServer() != null && !_getBridgeServer().IsRunning) + { + try + { + _getBridgeServer().Dispose(); + VibeLogger.LogInfo("server_disposed_before_bind", "disposed stale instance"); + } + catch (Exception ex) + { + VibeLogger.LogWarning("server_dispose_failed", ex.Message); + } + finally + { + _setBridgeServer(null); + } + } + + server = _serverInstanceFactory.Create(); + server.StartServer(); + _setBridgeServer(server); + VibeLogger.LogInfo( + "binding_success", + "Unity CLI Loop server bound the project IPC endpoint.", + new { endpoint = server.Endpoint }); + return true; + } + catch (Exception ex) + { + // Ensure partially created server is cleaned up on failure + server?.Dispose(); + // Unwrap SocketException details if present + SocketException sockEx = ex as SocketException; + if (ex is InvalidOperationException && ex.InnerException is SocketException innerSock) + { + sockEx = innerSock; + } + + if (sockEx != null) + { + VibeLogger.LogWarning("binding_failed", $"target=project_ipc code={sockEx.SocketErrorCode} hresult={sockEx.HResult} native={sockEx.ErrorCode}"); + } + else + { + VibeLogger.LogWarning("binding_failed", $"target=project_ipc code=Unknown hresult={ex.HResult}"); + } + + if (remainingMs <= 0) + { + return false; + } + + int delay = stepMs <= 0 ? remainingMs : Math.Min(stepMs, remainingMs); + await TimerDelay.Wait(delay, ct); + remainingMs -= delay; + } + } + } + } +} diff --git a/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs.meta b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs.meta new file mode 100644 index 0000000000..594360a644 --- /dev/null +++ b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: ea3e506a5a0d482baa7f384c2d29c876 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: From 8ae709da93b5bd647e58a60ad4dd6cfba384489e Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 9 Jul 2026 14:52:13 +0900 Subject: [PATCH 2/2] Drop unreachable null-throw chain and register async ct guard for the recovery executor UnityCliLoopServerRecoveryExecutor has exactly one construction site (inside UnityCliLoopServerControllerService's constructor) where every argument is already null-checked or is a compile-time-non-null lambda, so the ArgumentNullException chain could never fire. Following the established internal-helper convention, keep the Debug.Assert preconditions and drop the redundant throw chain. Also add the executor to StaticFacadeStateGuardTests' async cancellation-token guard list, since it owns two async methods with the standard ct parameter and was missing from the tracked path list. --- Assets/Tests/Editor/StaticFacadeStateGuardTests.cs | 3 ++- .../Server/UnityCliLoopServerRecoveryExecutor.cs | 14 +++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/Assets/Tests/Editor/StaticFacadeStateGuardTests.cs b/Assets/Tests/Editor/StaticFacadeStateGuardTests.cs index 88a9d9860b..68f46a0190 100644 --- a/Assets/Tests/Editor/StaticFacadeStateGuardTests.cs +++ b/Assets/Tests/Editor/StaticFacadeStateGuardTests.cs @@ -68,7 +68,8 @@ public sealed class StaticFacadeStateGuardTests "Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs", "Packages/src/Editor/FirstPartyTools/SimulateKeyboard/KeyboardInputMainThreadCleanup.cs", "Packages/src/Editor/FirstPartyTools/SimulateKeyboard/KeyboardInputActionExecutor.cs", - "Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeMissingReturnRetryPolicy.cs" + "Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeMissingReturnRetryPolicy.cs", + "Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs" }; private static readonly Dictionary OverloadGuardMethodsByPath = new Dictionary diff --git a/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs index 8f1d904081..83042a3d7c 100644 --- a/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs +++ b/Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs @@ -40,13 +40,13 @@ internal UnityCliLoopServerRecoveryExecutor( System.Diagnostics.Debug.Assert(getBridgeServer != null, "getBridgeServer must not be null"); System.Diagnostics.Debug.Assert(setBridgeServer != null, "setBridgeServer must not be null"); - _serverInstanceFactory = serverInstanceFactory ?? throw new ArgumentNullException(nameof(serverInstanceFactory)); - _readinessService = readinessService ?? throw new ArgumentNullException(nameof(readinessService)); - _startupProtectionService = startupProtectionService ?? throw new ArgumentNullException(nameof(startupProtectionService)); - _sessionFlagsRepository = sessionFlagsRepository ?? throw new ArgumentNullException(nameof(sessionFlagsRepository)); - _toolRegistrarService = toolRegistrarService ?? throw new ArgumentNullException(nameof(toolRegistrarService)); - _getBridgeServer = getBridgeServer ?? throw new ArgumentNullException(nameof(getBridgeServer)); - _setBridgeServer = setBridgeServer ?? throw new ArgumentNullException(nameof(setBridgeServer)); + _serverInstanceFactory = serverInstanceFactory; + _readinessService = readinessService; + _startupProtectionService = startupProtectionService; + _sessionFlagsRepository = sessionFlagsRepository; + _toolRegistrarService = toolRegistrarService; + _getBridgeServer = getBridgeServer; + _setBridgeServer = setBridgeServer; } ///