fix: Unity connection stays alive: the server restarts itself after failures and the CLI detects frozen Editors instead of hanging - #1312
Merged
Conversation
…er down A single recovery failure (typically the 30s readiness timeout firing while Unity is still busy with a heavy import right after a domain reload) used to log an error and give up, leaving the IPC server unreachable until the next domain reload or a manual restart. ExecuteTrackedRecoveryAsync now retries the recovery action on a 5s/15s/30s backoff schedule before surfacing the failure. The total backoff stays inside the CLI's 180s readiness polling window, so a late success is still picked up by a waiting uloop command. An explicit Stop Server issued during the backoff abandons the retry so manual intent always wins.
StartServer/StopServer were async void, so an exception such as the InvalidOperationException thrown by MarkServerReadyAsync on readiness probe failure escaped to the Unity synchronization context as an unhandled exception. Routing through Task + Forget() keeps the fire-and-forget call shape while observing and logging failures.
isTransportDisconnectError and isFinalResponseTimeoutError matched substrings like "EOF" and "i/o timeout". Domain reload recovery (compile status polling, execute-dynamic-code reconnect, play-mode wait) depends on this classification, and error strings differ across platforms and locales — Windows named pipe errors in particular do not contain the POSIX phrases, so a reload during compile could surface as a hard failure instead of recovering. Check errors.Is against io.EOF/net.ErrClosed/syscall causes and os.ErrDeadlineExceeded/os.IsTimeout first, keeping the string match only as a fallback for errors without a typed cause. The classifiers move to transport_errors.go because they are shared by several wait paths and compile_wait.go exceeded the 500-line architecture limit.
…r progress ValidateState() returning false (disposed reassembler) was silently ignored in the read loop, so the connection kept reading bytes it could never frame and the CLI hung until its own timeout. Breaking out of the loop closes the connection, which the CLI observes immediately as a disconnect.
… timeout DisposeCancellationSourceAfterServerTaskAsync disposed the source unconditionally after the 5s shutdown wait, even when a client task was still running (e.g. blocked on a main-thread switch during domain reload). Such a task then hit ObjectDisposedException the next time it touched its token — the "already disposed CancellationTokenSource" errors seen in Editor.log. When the wait times out the source is now intentionally leaked instead of disposed: one undisposed CTS is harmless, a disposed-in-use one is not.
…allers Stop() is reachable from three threads: the accept-loop cancellation registration, StopServer on the main thread, and unexpected-exit cleanup on the thread pool. The unguarded close-then-null sequence allowed a double Close on the same socket and left AcceptClient to dereference a nulled _listener, which surfaced as NullReferenceException and made the accept loop spin instead of recovering. Interlocked.Exchange hands the socket to exactly one Stop caller, and AcceptClient on a stopped listener now throws ObjectDisposedException, which ServerLoopAsync already treats as an exit-and-recover signal.
…d running Domain reload on large projects can keep the IPC endpoint down well past the 10s retry window, so commands issued during an externally triggered reload (e.g. the user saving a script) failed with "server not responding" even though Unity was fine. Undispatched dial failures — which are always safe to retry — now keep retrying for base * 6 (60s) as long as a running Unity process is found. Behavior elsewhere is unchanged: no Unity process still fails immediately, busy responses stay bounded by the 10s base window, and each attempt keeps the base accept bound so a dispatched-but-never-acked request still fails at 10s instead of hanging for the extended window.
…command Long-running commands (run-tests and other non-compile tools) waited on a 30-minute absolute read deadline. When the Editor froze with the process alive, the CLI sat silent for the full window with no diagnosis. The server now sends a heartbeat frame every 10 seconds while an accepted request executes, written by a background timer and carrying how long the editor main thread has gone without an EditorApplication.update tick. The CLI distinguishes three cases: - No frame for six intervals: the connection or server stalled; fail with a heartbeat-silence diagnosis instead of waiting out the 30-minute deadline. - Main thread stalled over 5 minutes: the Editor is likely frozen; fail with a diagnosis pointing at 'uloop launch -r'. Stalls over 30 seconds surface as spinner progress before that. - Heartbeats flowing: keep waiting up to the unchanged 30-minute cap. Negotiation is per-request via an acceptsHeartbeat flag answered by heartbeatIntervalSeconds in the dispatch ack, so old CLI/new server and new CLI/old server pairs keep the legacy absolute-deadline behavior and MINIMUM_REQUIRED_CLI_VERSION intentionally stays unchanged. Compile's explicit short response timeout also stays absolute: heartbeat frames are skipped but never extend it, preserving the fallback to status polling. Frame writes now share a per-connection lock because heartbeats and responses are written from different threads.
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 selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds negotiated JSON-RPC heartbeats (processor + server + client), editor main-thread liveness tracking, safer transport stop/accept semantics, recovery retry/backoff and shutdown-timeout behavior, transport error helpers, many new tests, and a CLI version bump with Windows Go test steps. ChangesHeartbeat, retry, and transport hardening
Sequence Diagram(s): sequenceDiagram
participant Client
participant BridgeServer
participant JsonRpcProcessor
participant EditorMainThreadLivenessTracker
Client->>BridgeServer: send request with uloop.acceptsHeartbeat=true
BridgeServer->>JsonRpcProcessor: ProcessRequestWithEarlyResponseAsync(...)
JsonRpcProcessor->>EditorMainThreadLivenessTracker: SecondsSinceLastMainThreadTick()
EditorMainThreadLivenessTracker-->>JsonRpcProcessor: mainThreadStallSeconds
JsonRpcProcessor-->>BridgeServer: dispatch-accepted (+heartbeat factory)
BridgeServer-->>Client: accepted response and then heartbeat frames
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/internal/unityipc/client.go (1)
199-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
ctxcancellation before waiting for the dispatch ack.
watchConnectionCancellationstarts only after the"accepted"frame arrives. If Unity accepts the socket and then never sends that first response, the read on Lines 199-205 sits on the fixed accept deadline, so caller cancellation can hang until the accept timeout expires.Suggested fix
if deadline, ok := acceptCtx.Deadline(); ok { _ = conn.SetDeadline(deadline) } + + stopCancelWatcher := watchConnectionCancellation(ctx, conn) + defer stopCancelWatcher() writeStartedAt := time.Now() if err := Write(conn, payload); err != nil { timing.Write = time.Since(writeStartedAt) timing.Total = time.Since(startedAt) @@ if err := applyPostAcceptDeadline(conn, heartbeatSilence, absoluteDeadline); err != nil { timing.Total = time.Since(startedAt) outcome.Timing = timing return outcome, err } - stopCancelWatcher := watchConnectionCancellation(ctx, conn) - defer stopCancelWatcher() for {Also applies to: 221-223
🤖 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 `@cli/internal/unityipc/client.go` around lines 199 - 205, Start watching ctx cancellation before blocking on readRPCResponse by invoking the same cancellation watcher used by watchConnectionCancellation (or creating a small goroutine) immediately after creating reader and before calling readRPCResponse; on ctx.Done close the conn (or set a short read deadline) so the blocked read returns promptly. Update both the read at the readRPCResponse call (reader := bufio.NewReader(conn); response, err := readRPCResponse(...)) and the analogous block around lines 221-223 to ensure the cancellation watcher runs prior to waiting for the "accepted"/dispatch ack, and preserve existing timing/outcome assignment on error return.
🤖 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.
Inline comments:
In `@cli/internal/unityipc/client_heartbeat_test.go`:
- Around line 205-217: The server writer goroutine is still running when
assertions run, causing spurious broken-pipe failures because defer close(done)
executes after assertions; change the test to stop the writer immediately after
the client times out by replacing defer close(done) with an explicit close(done)
right after SendWithProgressOutcome returns (or alternatively make writeFrame
failures non-fatal within the server goroutine). Update the test around
startHeartbeatTestServer/done/writeFrame/heartbeatAck/heartbeat and ensure
close(done) happens before asserting the timeout result.
---
Outside diff comments:
In `@cli/internal/unityipc/client.go`:
- Around line 199-205: Start watching ctx cancellation before blocking on
readRPCResponse by invoking the same cancellation watcher used by
watchConnectionCancellation (or creating a small goroutine) immediately after
creating reader and before calling readRPCResponse; on ctx.Done close the conn
(or set a short read deadline) so the blocked read returns promptly. Update both
the read at the readRPCResponse call (reader := bufio.NewReader(conn); response,
err := readRPCResponse(...)) and the analogous block around lines 221-223 to
ensure the cancellation watcher runs prior to waiting for the
"accepted"/dispatch ack, and preserve existing timing/outcome assignment on
error return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12677c06-b93e-4c24-825a-74b2db90a32a
⛔ Files ignored due to path filters (4)
Assets/Tests/Editor/BridgeTransportListenerTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/JsonRpcHeartbeatTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/UnityCliLoopBridgeServerShutdownTests.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/Threading/EditorMainThreadLivenessTracker.cs.metais excluded by none and included by none
📒 Files selected for processing (20)
Assets/Tests/Editor/BridgeTransportListenerTests.csAssets/Tests/Editor/JsonRpcHeartbeatTests.csAssets/Tests/Editor/JsonRpcProcessorCliVersionGateTests.csAssets/Tests/Editor/UnityCliLoopBridgeServerShutdownTests.csAssets/Tests/Editor/UnityCliLoopServerControllerStartupLockTests.csPackages/src/Editor/Infrastructure/Api/JsonRpcProcessor.csPackages/src/Editor/Infrastructure/Api/JsonRpcRequest.csPackages/src/Editor/Infrastructure/BridgeTransportListener.csPackages/src/Editor/Infrastructure/InfrastructureEditorStartup.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.csPackages/src/Editor/Infrastructure/Threading/EditorMainThreadLivenessTracker.csPackages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.csPackages/src/Editor/ToolContracts/UnityCliLoopServerConfig.cscli/internal/cli/compile_wait.gocli/internal/cli/connection_retry.gocli/internal/cli/connection_retry_test.gocli/internal/cli/transport_errors.gocli/internal/cli/transport_errors_test.gocli/internal/unityipc/client.gocli/internal/unityipc/client_heartbeat_test.go
💤 Files with no reviewable changes (1)
- cli/internal/cli/compile_wait.go
The Windows listener had the same class of race fixed for the Unix socket listener: Stop() disposed and nulled _activePipe non-atomically while the accept loop, the cancellation registration, and cleanup paths all touch the field. A Stop racing with the next accept could null the reference of a freshly created pipe without disposing it, leaving the accept loop blocked on a pipe nothing can wake. Stop now hands the pipe to exactly one caller via Interlocked.Exchange and marks the listener stopped; accepting on a stopped listener surfaces as ObjectDisposedException so ServerLoopAsync exits and triggers recovery, and a post-publish re-check closes the window between the stopped check and the pipe assignment. The stopped-state paths run before any pipe is created, so their tests execute on all platforms.
… errors Go tests never ran on Windows in CI, so the Windows-only transport code (named pipe dialing, winio deadlines) was effectively unverified. The windows-latest job now runs the Go test suite, and new Windows-only tests exercise a real winio named pipe end to end: heartbeats sliding the silence deadline, silence failing with a timeout-classified diagnosis, pipe close surfacing as typed io.EOF, and a missing pipe failing fast as a retryable connection attempt. Writing those tests surfaced a classification gap: go-winio's deadline error is not os.ErrDeadlineExceeded and os.IsTimeout does not unwrap wrapped errors, so the heartbeat-silence diagnosis and timeout classification missed it. Both classifiers now probe the unwrap chain for a Timeout() cause.
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Running the full Go suite on windows-latest surfaced many pre-existing Windows-incompatible tests outside the transport layer (path separator assumptions, helper binaries built without .exe, TCP-only fixtures in release automation tests). Fixing those is unrelated to this change; the job's purpose is to execute the named pipe transport code, so the run is scoped to internal/unityipc where the Windows-only tests live.
The repository convention moves cli/contract.json and MINIMUM_REQUIRED_CLI_VERSION in lockstep on Go CLI changes, and the CI minimum-version gate enforces it. Strictly speaking the package does not depend on new CLI behavior (heartbeats are negotiated per request and both old/new pairings keep working), but the lockstep bump keeps the shipped CLI and package aligned and lets the gate confirm the decision was deliberate.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@Packages/src/Editor/Infrastructure/BridgeTransportListener.cs`:
- Around line 183-185: _stop flag is set to true in Stop() and never reset,
causing AcceptClient()/ThrowIfStopped() to always fail after first Stop(); make
the Windows listener restartable by clearing _stopped at the start of Start()
(and/or before reinitializing the pipe) using the same synchronization used
around Stop()/Accept—i.e., set _stopped = false in Start() (and update any other
places where a new accept loop or pipe is created such as the blocks referenced
around AcceptClient()/ThrowIfStopped() and the regions noted at 194-196 and
249-271) so subsequent Start/AcceptClient cycles succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d39d8cd6-cce3-4f90-be47-65effb933e40
📒 Files selected for processing (7)
.github/workflows/build-and-test.ymlAssets/Tests/Editor/BridgeTransportListenerTests.csPackages/src/Editor/Infrastructure/BridgeTransportListener.cscli/internal/cli/transport_errors.gocli/internal/cli/transport_errors_test.gocli/internal/unityipc/client.gocli/internal/unityipc/client_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/internal/cli/transport_errors_test.go
- cli/internal/cli/transport_errors.go
- cli/internal/unityipc/client.go
…test The heartbeat frame write used the server token, so StopHeartbeatsAsync could block on an in-flight write to a slow client and stall the final response; the write now uses the heartbeat token so stopping heartbeats cancels it. In the explicit-response-timeout test the server goroutine kept writing through writeFrame (t.Errorf) after the client timed out and closed the connection, failing the test on an expected broken pipe. Expected post-close write failures now end the loop silently.
The IPC client built the "unity editor main thread busy for Ns..." progress message from heartbeat frames, but every progress callback discarded its message argument and rewrote the spinner with a static executing label, so the stall diagnosis never reached the user. Name the connection-stage progress events and map only those tokens to the contextual executing message; any other progress payload is display-ready text and now passes through to the spinner verbatim. Verified on Windows against a real Editor: a delayCall-scheduled 85-second main-thread sleep made get-hierarchy render "unity editor main thread busy for 30s..." through "80s..." and complete without disconnecting.
Merged
This was referenced Jun 16, 2026
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