fix: Unity tool requests no longer overlap during long-running work#1164
Merged
hatayama merged 12 commits intoMay 18, 2026
Conversation
Reject overlapping Unity tool requests with a machine-readable busy response, propagate request cancellation into tool execution, and teach the Go client to distinguish accepted requests from final responses.
Adopt the UniCli stability pattern more closely by clearing the client deadline after the server accepts a request, while preserving parent cancellation for aborted CLI calls. Track active Unity client tasks during shutdown, cancel accepted requests when the client disconnects, and add editor-state guards for tools that should not run during compile or update transitions.
Keep a bounded final response wait after dispatch acceptance, avoid consuming application bytes while monitoring accepted clients, and surface editor busy guards as retryable server-busy responses.
Regenerate the native CLI launchers after dispatch-ack client changes so repository Go CLI validation and packaged development binaries stay in sync.
Add the required behavior comments to the new guard tests and remove trailing whitespace from generated meta files so repository diff checks stay clean.
Avoid queuing accepted follow-up requests behind Unity's main-thread dispatcher before the single-flight execution gate can reject them. This keeps blocking tools from letting the next command run after the first command releases the gate.
Allow execute-dynamic-code requests to reach their existing scheduler while keeping other Unity tools single-flight. This preserves foreground preemption of background dynamic-code warmup without allowing unrelated tool execution to interleave.
Restore main-thread dispatch for CLI-only bridge commands after moving the generic JSON-RPC hop behind the tool execution gate. Internal commands still touch Unity APIs but do not pass through the regular tool execution service.
Client disconnect cancellation could leave a queued main-thread switch waiting for the next Unity editor tick and turn normal cancellation into a JSON-RPC internal error. Resume queued switches from cancellation as well as the editor queue, and let OperationCanceledException bubble to the bridge shutdown path.
The component writes readiness state during Unity compilation rather than enforcing a lock file. Rename the facade, publisher, and tests so future cleanup can distinguish external readiness state from tool execution serialization.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR threads ChangesCancellation-Aware Tool Execution Protocol
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs (1)
640-654: 💤 Low valueConsider passing cancellation token to
Task.Delayfor faster cleanup.When
StopClientDisconnectMonitorAsynccancels the token, the monitor will complete on the next iteration. However, without a cancellation token onTask.Delay, it must wait up to 100ms for the delay to complete naturally before checkingIsCancellationRequested.♻️ Proposed improvement
private static async Task MonitorClientDisconnectAsync( BridgeClientConnection client, CancellationTokenSource requestCancellationTokenSource) { - while (!requestCancellationTokenSource.IsCancellationRequested) + try { - if (!client.IsConnected) + while (!requestCancellationTokenSource.IsCancellationRequested) { - requestCancellationTokenSource.Cancel(); - return; - } + if (!client.IsConnected) + { + requestCancellationTokenSource.Cancel(); + return; + } - await Task.Delay(ClientDisconnectMonitorPollMilliseconds); + await Task.Delay(ClientDisconnectMonitorPollMilliseconds, requestCancellationTokenSource.Token); + } } + catch (OperationCanceledException) + { + // Expected when request completes normally + } }🤖 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/UnityCliLoopBridgeServer.cs` around lines 640 - 654, MonitorClientDisconnectAsync currently uses Task.Delay(ClientDisconnectMonitorPollMilliseconds) without a cancellation token, so cancelling requestCancellationTokenSource doesn't wake the delay immediately; update MonitorClientDisconnectAsync to pass requestCancellationTokenSource.Token into Task.Delay (Task.Delay(ClientDisconnectMonitorPollMilliseconds, requestCancellationTokenSource.Token)) and handle OperationCanceledException/TaskCanceledException by returning (or wrap the await in a try/catch and return on cancellation). This touches MonitorClientDisconnectAsync and uses the existing ClientDisconnectMonitorPollMilliseconds and requestCancellationTokenSource symbols to locate the change.
🤖 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/UnityCliLoopBridgeServer.cs`:
- Around line 640-654: MonitorClientDisconnectAsync currently uses
Task.Delay(ClientDisconnectMonitorPollMilliseconds) without a cancellation
token, so cancelling requestCancellationTokenSource doesn't wake the delay
immediately; update MonitorClientDisconnectAsync to pass
requestCancellationTokenSource.Token into Task.Delay
(Task.Delay(ClientDisconnectMonitorPollMilliseconds,
requestCancellationTokenSource.Token)) and handle
OperationCanceledException/TaskCanceledException by returning (or wrap the await
in a try/catch and return on cancellation). This touches
MonitorClientDisconnectAsync and uses the existing
ClientDisconnectMonitorPollMilliseconds and requestCancellationTokenSource
symbols to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2580ff2-3a1f-4ba0-a6f4-d3cf6496cf92
⛔ Files ignored due to path filters (9)
Assets/Tests/Editor/CompilationReadinessStatePublisherTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/UnityCliLoopEditorStateGuardTests.cs.metais excluded by none and included by nonePackages/src/Cli~/dist/darwin-amd64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/darwin-arm64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/windows-amd64/uloop.exeis excluded by!**/dist/**,!**/*.exeand included by nonePackages/src/Editor/Application/CompilationReadinessService.cs.metais excluded by none and included by nonePackages/src/Editor/Application/UnityCliLoopEditorStateGuard.cs.metais excluded by none and included by nonePackages/src/Editor/Application/UnityCliLoopToolBusyException.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/Server/CompilationReadinessStatePublisher.cs.metais excluded by none and included by none
📒 Files selected for processing (33)
Assets/Editor/FindGameObjects/FindGameObjectsTestMenu.csAssets/Tests/Editor/CompilationReadinessStatePublisherTests.csAssets/Tests/Editor/FindGameObjectsToolTests.csAssets/Tests/Editor/GetHierarchyToolTests.csAssets/Tests/Editor/JsonRpcProcessorCliVersionGateTests.csAssets/Tests/Editor/ToolSkillSynchronizerTests.csAssets/Tests/Editor/UnityCliLoopEditorStateGuardTests.csAssets/Tests/Editor/UnityCliLoopToolRegistryTests.csAssets/Tests/PlayMode/SimulateKeyboardTests.csAssets/Tests/PlayMode/SimulateMouseInputTests.csAssets/Tests/PlayMode/SimulateMouseUiTests.csPackages/src/Cli~/internal/cli/error_envelope.goPackages/src/Cli~/internal/cli/error_envelope_test.goPackages/src/Cli~/internal/unityipc/client.goPackages/src/Cli~/internal/unityipc/client_test.goPackages/src/Cli~/internal/unityipc/outcome.goPackages/src/Editor/Application/CompilationReadinessService.csPackages/src/Editor/Application/ImprovedErrorHandler.csPackages/src/Editor/Application/MainThreadSwitcher.csPackages/src/Editor/Application/UnityCliLoopEditorStateGuard.csPackages/src/Editor/Application/UnityCliLoopToolBusyException.csPackages/src/Editor/Application/UnityCliLoopToolExecutionService.csPackages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.csPackages/src/Editor/Infrastructure/Api/JsonRpcProcessor.csPackages/src/Editor/Infrastructure/Api/JsonRpcRequest.csPackages/src/Editor/Infrastructure/Api/UnityApiHandler.csPackages/src/Editor/Infrastructure/BridgeTransportListener.csPackages/src/Editor/Infrastructure/InfrastructureEditorStartup.csPackages/src/Editor/Infrastructure/Server/CompilationReadinessStatePublisher.csPackages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.csPackages/src/Editor/ToolContracts/IUnityCliLoopTool.csPackages/src/Editor/ToolContracts/UnityCliLoopConstants.csPackages/src/Editor/ToolContracts/UnityCliLoopTool.cs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 42 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs">
<violation number="1" location="Packages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs:646">
P2: Handle disposed-client checks in the disconnect monitor; `client.IsConnected` can throw during teardown and currently faults the monitor task.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| { | ||
| while (!requestCancellationTokenSource.IsCancellationRequested) | ||
| { | ||
| if (!client.IsConnected) |
There was a problem hiding this comment.
P2: Handle disposed-client checks in the disconnect monitor; client.IsConnected can throw during teardown and currently faults the monitor task.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs, line 646:
<comment>Handle disposed-client checks in the disconnect monitor; `client.IsConnected` can throw during teardown and currently faults the monitor task.</comment>
<file context>
@@ -553,6 +584,88 @@ private string CreateContentLengthFrame(string jsonContent)
+ {
+ while (!requestCancellationTokenSource.IsCancellationRequested)
+ {
+ if (!client.IsConnected)
+ {
+ requestCancellationTokenSource.Cancel();
</file context>
Suggested change
| if (!client.IsConnected) | |
| bool isConnected; | |
| try | |
| { | |
| isConnected = client.IsConnected; | |
| } | |
| catch (ObjectDisposedException) | |
| { | |
| isConnected = false; | |
| } | |
| if (!isConnected) |
Owner
Author
There was a problem hiding this comment.
Fixed in 951098e8.
Changed BridgeClientConnection.IsConnected so a disposed underlying socket or pipe is treated as disconnected instead of faulting the client disconnect monitor task.
Also added an EditMode test covering the disposed connection-state provider case.
Accepted Unity requests can outlive the final response deadline while Unity may still be running the command. Surface that timeout as a retryable response-waiting error instead of a generic internal CLI failure, and refresh the checked-in native CLI binaries.
Client disconnect monitoring can race with transport teardown after an accepted request. Return false from BridgeClientConnection.IsConnected when the underlying socket or pipe has already been disposed, and cover that contract with an EditMode test.
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.
Summary
User Impact
Changes
Verification
go run ./cmd/uloop compile --project-path /Users/a12115/ghq/hatayama/unity-cli-loop --force-recompilego run ./cmd/uloop run-tests --project-path /Users/a12115/ghq/hatayama/unity-cli-loop --test-mode EditMode --filter-type regex --filter-value CompilationReadinessStatePublisherTestsscripts/check-go-cli.sh && git diff --checkcodex-review v3-beta