Extract server recovery/bind-retry flow into UnityCliLoopServerRecoveryExecutor - #1655
Merged
hatayama merged 2 commits intoJul 9, 2026
Merged
Conversation
…ryExecutor 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.
Contributor
📝 WalkthroughWalkthroughThe recovery-start logic in UnityCliLoopServerController is extracted into a new UnityCliLoopServerRecoveryExecutor class handling coalesced server binding, retries, session-flag updates, and error logging. The controller now delegates to this executor and exposes IsBackgroundUnityProcess as internal static. A test guard allowlist is also updated. ChangesRecovery executor extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Controller as UnityCliLoopServerController
participant Executor as UnityCliLoopServerRecoveryExecutor
participant Server as BridgeServer
Controller->>Executor: StartRecoveryIfNeededAsync(isAfterCompile, ct)
Executor->>Executor: skip if background process or startup protection active
Executor->>Executor: acquire startup semaphore
Executor->>Server: dispose stale server instance
Executor->>Server: TryBindWithWaitAsync (retry loop, 250ms steps, 5s timeout)
alt bind succeeds
Server-->>Executor: server started
Executor->>Executor: mark ready, activate startup protection
else bind fails
Executor->>Executor: clear session flags, throw InvalidOperationException
end
Executor->>Executor: release semaphore
Executor-->>Controller: return
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… 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.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs (1)
92-104: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOnly retry expected bind failures; rethrow unexpected exceptions.
The broad catches turn non-socket bugs into generic
binding_failedretries and can clear the bridge reference after a failed dispose. Keep cleanup, but only handle known bind failures; let other exceptions surface.♻️ Proposed direction
- 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); - } + IUnityCliLoopServerInstance previousServer = _getBridgeServer(); + previousServer.Dispose(); + _setBridgeServer(null); + VibeLogger.LogInfo("server_disposed_before_bind", "disposed previous server instance");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) + + if (!TryGetSocketException(ex, out SocketException sockEx)) { - sockEx = innerSock; + throw; } - 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}"); - } + VibeLogger.LogWarning("binding_failed", $"target=project_ipc code={sockEx.SocketErrorCode} hresult={sockEx.HResult} native={sockEx.ErrorCode}");private static bool TryGetSocketException(Exception ex, out SocketException socketException) { socketException = ex as SocketException; if (socketException != null) { return true; } InvalidOperationException invalidOperationException = ex as InvalidOperationException; socketException = invalidOperationException?.InnerException as SocketException; return socketException != null; }Based on learnings, apply a Fail Fast policy: avoid broad defensive try-catch for unexpected exceptions and only catch expected domain-specific exceptions at the appropriate layer.
Also applies to: 154-166, 178-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs` around lines 92 - 104, The dispose/retry handling in UnityCliLoopServerRecoveryExecutor is too broad and is masking non-bind bugs as recoverable failures. Update the recovery path around the bridge disposal logic to catch only expected bind-related failures (for example via a helper like TryGetSocketException) and rethrow any other exceptions instead of converting them into binding_failed retries. Keep the cleanup behavior that clears the bridge reference, but only do so in the valid failure path and avoid swallowing unexpected exceptions in the affected recovery blocks.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs`:
- Around line 92-104: The dispose/retry handling in
UnityCliLoopServerRecoveryExecutor is too broad and is masking non-bind bugs as
recoverable failures. Update the recovery path around the bridge disposal logic
to catch only expected bind-related failures (for example via a helper like
TryGetSocketException) and rethrow any other exceptions instead of converting
them into binding_failed retries. Keep the cleanup behavior that clears the
bridge reference, but only do so in the valid failure path and avoid swallowing
unexpected exceptions in the affected recovery blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d90782c9-c998-49cb-8cd9-1f0feaecabb1
⛔ Files ignored due to path filters (1)
Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs.metais excluded by none and included by none
📒 Files selected for processing (3)
Assets/Tests/Editor/StaticFacadeStateGuardTests.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerRecoveryExecutor.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs: R3-8 in the Unity CLI Loop Phase 2/3 refactor ToDo
Summary
UnityCliLoopServerControllerServicemixed IPC lifecycle orchestration (start/stop/domain-reload handling) with the coalesced recovery/bind-retry algorithm (StartRecoveryIfNeededAsync+TryBindWithWaitAsync, ~150 lines).internal sealed class UnityCliLoopServerRecoveryExecutor, constructed once in the controller's constructor and wired via a thin one-line delegating wrapper._bridgeServer, is bridged with aFunc<IUnityCliLoopServerInstance> getBridgeServer/Action<IUnityCliLoopServerInstance> setBridgeServerpair — everything else the executor needs was already an injected interface, so no other bridging was required.IsBackgroundUnityProcess()was promoted fromprivatetointernal static(it has zero instance-state dependencies — it only calls the staticAssetDatabase.IsAssetImportWorkerProcess()), letting the executor call it directly without duplication or another delegate.cancellationTokenparameter renamed toctin the moved methods per project convention.Verification
sort -u+comm -23/comm -13) between the pre-move file and the combined post-move files shows only the pre-approved adaptations (ct rename,_bridgeServer→ delegate calls,IsBackgroundUnityProcessstatic-ization, and the new executor's DI wiring/wrapper) — no unrelated logic changes.uloop compile— 0 errors / 0 warnings.uloop run-tests --filter-value 'UnityCliLoopServerControllerRecoveryTests'— 23/23 pass, test file unmodified (public constructor signature ofUnityCliLoopServerControllerServicepreserved, so the existing tests exercise the new code path unchanged).Test plan
uloop compilecleanUnityCliLoopServerControllerRecoveryTests23/23 pass unmodified