fix: A command that reached Unity after a busy retry no longer reports the earlier busy answer when its connection drops or times out by hatayama · Pull Request #3196 · hatayama/unity-cli-loop

hatayama · 2026-10-06T15:52:04Z

Summary

When a command first gets a busy answer from Unity and the retry then reaches Unity, a dropped connection, a timeout, or a frozen-editor report on that retry is now reported as what it is, instead of as the earlier busy answer.
uloop compile sent while another tool is running no longer exits 1 with UNITY_SERVER_BUSY while the compile is in fact running in Unity: it goes on to ask Unity for the compile status and returns the real result.

User Impact

Before: uloop compile sent while another tool was running got a busy answer, retried, and reached Unity once the tool finished. Unity then ran the compile, but the compile took longer than the 2-second final response wait (or the domain reload dropped the connection). The CLI then printed UNITY_SERVER_BUSY ("'compile' was not executed because Unity is busy running …") and exited 1. It did not ask Unity for the compile status, and it did not reach the compile resend added in fix: Send the compile again when Unity lost the request or rejected it as already compiling #3192.
After: the same sequence follows the same recovery as a compile that never saw a busy answer. The CLI asks Unity for the status of the request that reached it and prints the compile result.
Other commands no longer say a command "was not executed" when it had in fact started in Unity (details per caller below).

Cause

finishNonRetryableConnectionAttempt returned the earlier busy answer whenever the current attempt ended in an error that was not an RPC answer. It did not check whether that attempt's request had reached Unity. The rule dates from when the retry window and the connection deadline ended at the same moment. Each attempt now has its own deadline, so that reasoning no longer holds. A busy answer still means Unity ran nothing, but only for the attempt it answered.

Changes

finishNonRetryableConnectionAttempt returns the earlier busy answer only when the current attempt's request never reached Unity (a failed connect or write). An attempt that reached Unity (RequestDispatched) keeps its own error and outcome and takes the same path as a first attempt, including the Unity focus handling.
A caller's cancellation still wins over both, unchanged.
The busy preference on the undispatched-dial side (finishUndispatchedRetryProbe) is unchanged.
Rewrote the comment above the branch. The sentence about the connection deadline firing before the context reports expiry is gone, because it described the structure that no longer exists.

Input space

Previous attempt × current attempt × caller cancellation. When the current attempt succeeds, gets a busy answer, or is an undispatched transient dial failure, it never reaches this branch, so nothing changes for those cases.

#	Previous	Current attempt	ctx	Before	After	Test
1	busy	not dispatched, not a dial failure (failed write, permanently refused connect)	live	busy + busy outcome	same	`…PrefersBusyOverAnUndispatchedTransportError`
2	busy	same	cancelled	`ctx.Err()`	same	same test (second half)
3	busy	dispatched, accepted, dropped	live	busy	current disconnect + current outcome	`…KeepsADispatchedFailureAfterBusy` (dropped after the accept), `…SurfacesADroppedConnectionAfterBusy`
4	busy	dispatched, not accepted, dropped	live	busy	current disconnect + current outcome	`…KeepsADispatchedFailureAfterBusy` (dropped before the accept)
5	busy	dispatched, accepted, final response timeout (a compile longer than 2 s)	live	busy (focus handling skipped)	current timeout + current outcome (focus handling as on a first attempt)	`…KeepsADispatchedFailureAfterBusy` (final response timed out), `…SurfacesAFinalResponseTimeoutAfterBusy`
6	busy	dispatched, not accepted, timeout	live	busy (focus handling skipped)	current timeout + current outcome (enters the focus handling)	`…SurfacesAnUnansweredRequestAfterBusy`
7	busy	dispatched, accepted, editor unresponsive (the heartbeat reported the main-thread stall limit; commands without a response timeout)	live	busy (focus handling skipped)	the unresponsive error + current outcome (main-thread-stall focus handling, as on a first attempt)	`…KeepsAnEditorUnresponsiveErrorAfterBusy`
8	busy	dispatched, accepted, any other non-RPC error (for example, a final response that fails to decode)	live	busy	that error + current outcome (no focus handling, before or after: only timeouts and the unresponsive error enter it)	`…KeepsADispatchedFailureAfterBusy` (failed another way after the accept)
9	busy	dispatched failure (#3–#8)	cancelled	`ctx.Err()`	same	cancellation subtests of `…KeepsADispatchedFailureAfterBusy`
10	busy	RPC error other than busy	either	current RPC error	same	existing `…SurfacesDispatchedFailureAfterBusy`
11	not busy (none / dial failure)	any	either	current error + outcome	same (branch not entered)	existing retry tests

Not used as axes: the command (this function does not look at it; the caller differences are listed below), responseTimeout (it only affects the focus reason after the branch, which is unchanged), and how long ago the busy answer came (never looked at).

Behaviour change by caller

In general, an attempt that reached Unity after a busy answer now gets the same recovery as a first attempt, including that recovery's limits. A caller that waits for the result of a request Unity never registered now waits, instead of ending at once with the busy answer.

Each caller below was checked by reading its code. Only compile was also exercised on a device.

compile: a dispatched drop, or an accepted final response timeout, now reaches shouldWaitForCompileStatus and the compile status polling (and the fix: Send the compile again when Unity lost the request or rejected it as already compiling #3192 resend), instead of exiting 1 with UNITY_SERVER_BUSY.
pause-point recovery compile: it sends through the same retry loop. Its entry runs with freshCompileAttemptOptions{}, so the fix: Send the compile again when Unity lost the request or rejected it as already compiling #3192 resend never applies (canResendCompile is false). Its own busy retry (sendCompileWithBusyRetry) resends only on a busy answer, and no longer sees one for a request that reached Unity.
- Better: when the compile tool registered the request that reached Unity, the recovery now gets that compile's result. Before, the busy answer made it send the compile again 2 seconds later with the same request ID, which cost an extra compile.
- Worse: when the request reached Unity but the connection dropped before the compile tool registered it (for example, a domain reload started while the accepted request waited for the main thread), the recovery now polls the status. Unity has no record of the request, so the recovery waits until the compile wait timeout (10 minutes by default) and exits 1. Before, the busy answer made it send the compile again after 2 seconds.
- The worse case is how a first attempt that drops the same way already ends today (the fix: Send the compile again when Unity lost the request or rejected it as already compiling #3192 tests pin that). It also needs another command to hold the execution slot at the same moment, because that is the only way compile gets a busy answer (the editor-state busy check covers only control-play-mode and execute-dynamic-code).
run-tests (PlayMode runs that keep the project's Enter Play Mode settings, the only mode that waits across the domain reload; other modes are on the plain path below): a dispatched drop, or an accepted final response timeout, now goes on to wait for the test result (shouldWaitForCompileStatus), instead of exiting 1 with UNITY_SERVER_BUSY.
- Better: when the test run that reached Unity started, the wait gets its result and exits by it. Before, the busy answer made it exit 1 while the tests ran.
- Worse: when the request reached Unity but the connection dropped before the run started, Unity never stores a result for that request ID. The wait polls until TimeoutSeconds + 60 seconds (600 + 60 = 660 seconds by default) and exits 1 with RUN_TESTS_WAIT_TIMEOUT. Before, it exited 1 at once with UNITY_SERVER_BUSY. A first attempt that drops the same way already ends like this today.
- run-tests gets a busy answer only while another command holds the execution slot (the editor-state check does not cover it), so the worse case needs another command at the same moment.
control-play-mode (Play, Stop, Pause, and Resume, which use the state wait): a dispatched drop now goes on to the play mode state wait, instead of exiting 1 with UNITY_SERVER_BUSY.
- Better: when the request that reached Unity ran (for example, entering play mode, whose domain reload drops the connection), the wait sees the new state and reports success. Before, the busy answer made it exit 1 although the state had changed.
- Worse: when the request reached Unity but the connection dropped before it ran, the state the action asks for never comes (unless it already held). The wait polls until TimeoutSeconds (180 seconds by default) and exits 1 with CONTROL_PLAY_MODE_WAIT_TIMEOUT. Before, it exited 1 at once with UNITY_SERVER_BUSY. A first attempt that drops the same way already ends like this today.
- This command also gets busy answers from the editor-state check, which runs on the main thread after the accept and answers busy while Unity compiles or updates assets. So the worse case does not need another command: busy answers during a compile, then a retry that reaches Unity just before the domain reload that follows the compile.
execute-dynamic-code (--wait-for-domain-reload): a dispatched drop waits for the domain reload before it reports the failure. Exit stays 1.
hot-reload and the other tools on the plain path (also list, sync, enable-pause-point): stderr reports how the attempt that reached Unity ended, instead of UNITY_SERVER_BUSY. That is UNITY_DISCONNECTED_AFTER_ACCEPT, UNITY_DISCONNECTED_AFTER_DISPATCH, UNITY_RESPONSE_TIMEOUT_AFTER_ACCEPT, the classified timeout, or the classified form of any other error the attempt ended with. Exit stays 1.
Commands without a response timeout (every caller above except the two compile paths): a request accepted after a busy answer can also end with the heartbeat reporting that the Editor main thread stalled past the limit. The CLI now reports UNITY_EDITOR_UNRESPONSIVE ("Unity accepted the request, but the Editor main thread stopped responding.") instead of UNITY_SERVER_BUSY, and the main-thread-stall focus handling brings Unity to the front. Exit stays 1. The compile paths never get here: Unity sends its first heartbeat 10 seconds after the accept, and their 2-second response timeout has ended the wait by then.
Unity focus: a timeout after a busy answer now gets the same focus handling as a timeout on a first attempt, which brings Unity to the front. This covers a pre-accept timeout, and an accepted request's final response timeout when the caller set no response timeout. Before, these went straight back as the busy answer.

Verification

CI for Go (build-cli) does not run on pull requests to this integration branch, so everything below was run locally on macOS (darwin-arm64). On this PR, Complexity Report and File Length Report ran and passed.

New and changed tests

Unit, table-driven (TestFinishNonRetryableConnectionAttemptKeepsADispatchedFailureAfterBusy): dropped after the accept / dropped before the accept / final response timed out after the accept / failed another way after the accept (a plain error standing for one that is neither a disconnect nor a timeout, such as a final response that fails to decode). Each case checks the attempt's own error and outcome, and that a cancelled caller still gets context.Canceled.
Unit (TestFinishNonRetryableConnectionAttemptKeepsAnEditorUnresponsiveErrorAfterBusy): an editor-unresponsive error from an accepted attempt after a busy answer. It runs with a real focus controller whose process lookup finds no Unity. It checks the attempt's own error and outcome, and that the lookup ran once, which is the main-thread-stall focus handling a first attempt gets.
The busy attempt in these unit tests carries a distinct timing, so handing back its outcome instead of the current one fails in every case, including those where the flags match.
Unit, changed: the existing busy-over-transport test now uses an undispatched attempt, and is renamed …PrefersBusyOverAnUndispatchedTransportError. It pinned the dispatched shape that this PR fixes.
Retry loop with a TCP stand-in for Unity (busy on the first connection, then the case under test). These follow the existing …SurfacesDispatchedFailureAfterBusy test and skip on Windows for the same reason; the unit tests cover the branch on every platform.
- …SurfacesADroppedConnectionAfterBusy: transport disconnect, dispatched + accepted, shouldWaitForCompileStatus true.
- …SurfacesAFinalResponseTimeoutAfterBusy: 50 ms response timeout, IsFinalResponseTimeoutError true, accepted, shouldWaitForCompileStatus true.
- …SurfacesAnUnansweredRequestAfterBusy: no ack, response timeout, dispatched but not accepted.
- In the two timeout tests the stand-in stays silent until the client hangs up, so the client's own deadline is always what ends the wait, however slow the runner.

Red / Green

Red (tests written before the fix): the three non-cancellation subtests of …KeepsADispatchedFailureAfterBusy and the three new retry tests failed, all because the busy answer came back. The three cancellation subtests, …PrefersBusyOverAnUndispatchedTransportError, and the existing …SurfacesDispatchedFailureAfterBusy passed.
Two tests were added during review, after the fix. Under m1 below (the code before this PR) both fail:
- the editor-unresponsive test fails on all three checks: the busy answer comes back, the busy attempt's outcome comes back, and the Unity process lookup runs 0 times instead of once;
- the "failed another way after the accept" case fails because the busy answer comes back instead of its own error.
Green: all of them pass.

Mutations (4 of 4 caught)

#	Mutation	Failing tests
m1	drop the `RequestDispatched` check (the code before this PR)	the four non-cancellation subtests, `…KeepsAnEditorUnresponsiveErrorAfterBusy`, and the three new retry tests
m2	delete the line that returns the busy answer	`…PrefersBusyOverAnUndispatchedTransportError`
m3	move the cancellation check inside the undispatched branch	the four cancellation subtests
m4	keep the current error but hand back the busy attempt's outcome	the four non-cancellation subtests, `…KeepsAnEditorUnresponsiveErrorAfterBusy`, `…SurfacesADroppedConnectionAfterBusy`, `…SurfacesAFinalResponseTimeoutAfterBusy`

Suites and checks

go test ./... -count=1 in cli/project-runner: 938 passed, 1 failed. The failure is TestSendWithTransientConnectionRetryAbortsOnRefusedConnect, which cannot bind its Unix socket in the sandbox (bind: operation not permitted). On the base commit the same run gives 921 passed and the same 1 failure. The difference of 17 is the new tests and subtests.
New tests with -race -count=20, rerun after the review changes: pass.
scripts/check-go-cli.sh:
- It stops at the cli/common tests, because the ipcendpoint test cannot create its directory under the system temp directory in the sandbox. cli/common is not touched by this PR; its fmt, vet, and lint passed before the stop.
- Replayed the remaining stages by hand for cli/dispatcher, cli/release-automation, and cli/project-runner. fmt diff, vet, and golangci-lint report 0 issues (cli/project-runner again after the review changes). Tests pass except the one Unix-socket test above.
- The binary rebuild stage stops because the sandbox blocks the go-winres download. Built darwin-arm64 directly with the same flags.
Complexity (scripts/check-code-complexity.sh with fail-on-exceeded): cyclop 0 issues in all four modules. No C# finding above 15.
File length (scripts/check-file-length.sh with fail-on-exceeded): no file over 500 SLOC.
Coverage (profiles for all four modules, coverage-report --mode report):
- project-runner 95.26% (baseline 95.2), the same after the review changes. finishNonRetryableConnectionAttempt is 100% covered.
- dispatcher 94.2 (94.2) and release-automation 96.4 (96.4).
- common shows 91.9 locally only because its ipcendpoint tests fail in the sandbox, as above.
No document describes the old replacement: searching docs and Packages/src markdown for the busy wording found nothing.

Real Editor check (this branch's Unity project, dev binary, `ULOOP_DEBUG=1`)

Each run started uloop execute-dynamic-code running Thread.Sleep(1500) in the background and sent uloop compile about 0.1 s later. A one-line comment in an EditMode test source made the compile and domain reload real. Times are seconds after the Editor logged execute_dynamic_code_start.

Run	Runner	Compile sent	Tool finished	Compile request received by Unity	CLI result
before	this PR with m1 applied	+0.14	+1.90	+2.24	exit 1, `UNITY_SERVER_BUSY` at +4.17. The compile finished successfully in Unity at +10.2.
after 1	this PR	+0.02	+2.60	+3.04	final attempt ended in the 2 s response timeout (send 5006 ms in total, last attempt 2004 ms) → status polling → exit 0, `Success: true`
after 2	this PR	+0.16	+1.56	+2.21	final attempt ended in the 2 s response timeout (send 4013 ms in total, last attempt 2013 ms) → status polling → exit 0, `Success: true`

The Editor applies the busy check before the tool switches to the main thread. A compile sent while the sleeping tool held the execution slot was therefore answered busy until the tool finished. The send total minus the last attempt is the time spent on those busy answers.
The test source change was reverted and compiled again afterwards.
These runs used the fix commit; the later review commits change tests only.

Not verified

The Windows-only parts (named pipes). The retry tests skip on Windows like the existing one. The unit tests are platform independent.
The run-tests, control-play-mode, execute-dynamic-code, pause-point recovery, plain-tool, and editor-unresponsive changes were checked by reading their code, not on a device.

Not covered

Only compile recognizes a request that reached Unity but was never registered, and sends it again (fix: Send the compile again when Unity lost the request or rejected it as already compiling #3192). control-play-mode, run-tests, and the pause-point recovery compile (which runs without the fix: Send the compile again when Unity lost the request or rejected it as already compiling #3192 resend) wait until their timeout instead, whether the request was a first attempt or came after a busy answer. Fixing that belongs in a separate PR.

Notes

Ships with the next project runner release. Protocol version not bumped: the wire format is unchanged.

…usy answer After a busy answer, the retry that reached Unity can still end in a dropped connection or a timeout. Today the retry loop reports the earlier busy answer instead, so compile never asks Unity for its status even though the compile is running. These tests pin the attempt's own error and outcome, keep the busy answer for an attempt that never reached Unity, and keep a caller's cancellation winning in both cases.

…nswer A busy answer means Unity ran nothing, so it was a fair diagnosis for any later transport error back when the retry window and the connection deadline ended together. Each attempt now has its own deadline, and a retry that reached Unity may already be running: replacing its dropped connection or timeout with the busy answer stopped compile from asking Unity for its status and reported a running command as not executed. The busy answer now wins only over an attempt that never reached Unity, and a caller's cancellation still wins over both.

coderabbitai · 2026-10-06T15:52:25Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: hatayama/unity-cli-loop/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 2883fcea-485d-4b95-9358-e2c3e29cc8d1

📥 Commits

Reviewing files that changed from the base of the PR and between 2454ad7 and c1d1256.

📒 Files selected for processing (2)

cli/project-runner/internal/projectrunner/connection_retry_flow_test.go
cli/project-runner/internal/projectrunner/connection_retry_test.go

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The connection retry flow now returns the current attempt’s outcome and error when a non-RPC failure occurs after dispatch. Tests cover undispatched failures, dispatched failures, and caller cancellation.

Changes

Connection retry outcomes

Layer / File(s)	Summary
Failure outcome selection and unit tests `cli/project-runner/internal/projectrunner/connection_retry_flow.go`, `cli/project-runner/internal/projectrunner/connection_retry_flow_test.go`	After cancellation handling, the flow returns the earlier busy outcome only when the current request was not dispatched. Unit tests check dispatched failures, cancellation, and the undispatched case.
Retried TCP request tests `cli/project-runner/internal/projectrunner/connection_retry_test.go`	TCP tests simulate a busy response on the first connection. They check the error and outcome flags for a retried request that disconnects, times out after acceptance, or times out without an acceptance acknowledgment.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c1d12

The retry outcome change is ready to merge after normal checks; no specific user-facing regression remains identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c1d12

The change corrects misleading failure reporting without adding a new command or permission boundary. Existing recovery preserves request identity and limits resends. Some uncertainty remains because the editor-side guarantees that prevent resending a still-live compile were not independently verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The effective behavioral scope is commands already using the projectrunner retry path, with compile gaining access to existing status recovery after a post-busy dispatched failure. The reviewed diff does not add an attacker-controlled input, connection target, dependency, or authority transition.

Trust Boundaries and Controls

observed — The existing IPC client distinguishes successful request dispatch from server acceptance. Cancellation and socket-close paths remain in place, while the changed selector preserves cancellation precedence rather than substituting the earlier busy error.

Resilience and Maintainability Implications

observed — Transport retry does not automatically resend after the dispatched failure. Existing compile recovery limits total sends to three and requires remaining time plus missing-request or busy-rejection evidence. Missing-request detection requires repeated ready-without-result responses after restart evidence, but a transport disconnect itself can supply that evidence. The editor-side guarantee that this cannot duplicate a still-live request remains unverified.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly describes the main change: dispatched attempts after a busy response keep their own connection or timeout error. It is longer than preferred, but remains specific and relevant.
Description check	✅ Passed	The description explains the change, its user impact, caller-specific behavior, tests, and verification. It is directly related to the changeset.

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…y tests The final response timeout and unanswered request tests let the stand-in close the connection 150 ms after the client's deadline. On a slow runner the client can fall that far behind, and the close then arrives first as a disconnect. Reading until the client hangs up makes the client's own deadline the only thing that can end the wait.

An accepted request can also end with the heartbeat reporting a stalled main thread. That error is not an RPC answer either, so it used to be replaced by the earlier busy answer too. The new test pins its own error and outcome and the main-thread-stall focus handling a first attempt gets. The busy attempt in these tests now carries a distinct timing, so returning its outcome instead of the current one fails in every case.

The rule keeps the error of an attempt that reached Unity whatever its kind, not only disconnects, timeouts, and the editor-unresponsive error. A final response that fails to decode, for one, reaches the same branch. A table case with a plain error pins that the rule does not depend on the kind of error, and that a cancelled caller still gets the cancellation.

hatayama added 2 commits October 7, 2026 00:28

hatayama added 3 commits October 7, 2026 01:06

hatayama merged commit 3ee9b85 into feature/hot-reload-large-project-feedback Oct 6, 2026
3 of 4 checks passed

hatayama deleted the fix/cli-dispatched-failure-masked-by-earlier-busy branch October 6, 2026 16:55

hatayama mentioned this pull request Oct 6, 2026

feat: Hot reload now applies edits on partial types and in global-using assemblies, and the response says whether an edit is live #3198

Merged

Navigation Menu

Repository navigation

fix: A command that reached Unity after a busy retry no longer reports the earlier busy answer when its connection drops or times out - #3196

fix: A command that reached Unity after a busy retry no longer reports the earlier busy answer when its connection drops or times out#3196
hatayama merged 5 commits into
feature/hot-reload-large-project-feedbackfrom
fix/cli-dispatched-failure-masked-by-earlier-busy

hatayama commented Oct 6, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 6, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

hatayama commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

User Impact

Cause

Changes

Input space

Behaviour change by caller

Verification

New and changed tests

Red / Green

Mutations (4 of 4 caught)

Suites and checks

Real Editor check (this branch's Unity project, dev binary, ULOOP_DEBUG=1)

Not verified

Not covered

Notes

Uh oh!

coderabbitai Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Security Architecture Review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

hatayama commented Oct 6, 2026 •

edited

Loading

Real Editor check (this branch's Unity project, dev binary, `ULOOP_DEBUG=1`)

coderabbitai Bot commented Oct 6, 2026 •

edited

Loading