fix(editor): survive domain reloads in HTTP auto-start and reload-resume (#1229)#1234
Draft
Scriptwonder wants to merge 2 commits into
Draft
fix(editor): survive domain reloads in HTTP auto-start and reload-resume (#1229)#1234Scriptwonder wants to merge 2 commits into
Scriptwonder wants to merge 2 commits into
Conversation
…ume (CoplayDev#1229) Auto-start died for the whole session whenever startup included a compile: the ctor latched SessionState before the delayCall ran, and the reload wiped the delayCall. Reload-resume died at multi-pass compiles: the one-shot flag was consumed before the deferred (delayCall) resume ran, and the next boundary deleted it again. - Replace delayCall with EditorApplication.update ticks that the [InitializeOnLoad] ctor re-arms on every domain load; latch only when the deferred work actually dispatches, retry (bounded per domain) while editor services are still initializing, and skip the subscription entirely in the common case where auto-start is off and nothing is pending. - Move the resume flag from EditorPrefs (per-user machine-global, survives crashes, leaks across concurrently open editors) to SessionState; keep it until the resume succeeds, is cancelled, or exhausts its retries, instead of consuming it at boundaries where the bridge is down. Manual Connect, End Session, transport switch, and orphan cleanup cancel a pending resume through a named seam (CancelPendingResume), which also aborts an in-flight retry loop; exhaustion erases the flag so later reloads don't replay 49s failure loops. - Serialize TransportManager.StartAsync per mode: concurrent starts coalesce onto one in-flight attempt, so a manual Connect can no longer race the resume/auto-start loops into bouncing a just-established session (WebSocketTransportClient.StartAsync tears down a live connection first). - A SessionState connect-pending marker lets the next domain load finish an auto-start whose connect phase a reload killed — connect-only, never re-spawning (StartLocalHttpServer stops a still-booting server first). Whether a launch-process handle exists is now answered live by ServerManagementService.HasManagedServerLaunchHandle; without one (post- reload, or an externally started server) the wait polls to the 5-minute hard cap instead of fail-fasting. - Busy gate uses EditorStateCache.GetActualIsCompiling (now internal, with the CompilationPipeline reflection bound once as a delegate): raw isCompiling stays true all play session under Recompile-After-Finished-Playing (CoplayDev#549). - One-time (per session) migration deletes the legacy EditorPrefs flag. The stdio sibling has the same defect class (StdioBridgeReloadHandler.cs:65 delete-when-not-running, :133 delayCall) — follow-up, kept out of scope here, along with the remaining stdio copies of the isCompiling probe.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReworks HTTP auto-start and reload-resume handling to survive Unity domain reloads by moving state from EditorPrefs to SessionState and replacing delayCall scheduling with an EditorApplication.update tick state machine. Adds a managed-launch-handle signal, coalesces TransportManager start calls, updates connection UI cancellation, and adds tests. ChangesReload-safe auto-start and resume
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EditorApplication
participant HttpAutoStartHandler
participant SessionState
participant HttpBridgeReloadHandler
participant TransportManager
EditorApplication->>HttpAutoStartHandler: domain load / update tick
HttpAutoStartHandler->>SessionState: check SessionInitKey / ConnectPendingKey
HttpAutoStartHandler->>HttpBridgeReloadHandler: check IsResumePending
alt Resume pending
HttpBridgeReloadHandler->>HttpBridgeReloadHandler: ResumeTick (editor idle)
HttpBridgeReloadHandler->>TransportManager: ResumeHttpWithRetriesAsync -> StartAsync(Http)
HttpBridgeReloadHandler->>SessionState: erase ResumeSessionKey on success/exhaustion
else Auto-start decision
HttpAutoStartHandler->>HttpAutoStartHandler: TickCore -> ShouldStart or ShouldReconnect
HttpAutoStartHandler->>TransportManager: AutoStartAsync or ReconnectAsync
HttpAutoStartHandler->>SessionState: set/erase ConnectPendingKey
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Scriptwonder
added a commit
to Scriptwonder/unity-mcp
that referenced
this pull request
Jul 4, 2026
CoplayDev#1207) The orphaned-session detector ended an active session on a SINGLE stale reachability reading, and the reading came from a lone 50ms TCP connect cached for 0.75s — trivially false-negative on a machine busy with test runs or domain reloads. Evidence bundles in CoplayDev#1207 show 13 teardowns and 147 socket closures in one session from exactly this loop, wedging the bridge in no_unity_session churn until manual recovery. - Require 3 consecutive failed polls (0.75s cadence) before declaring a session orphaned; probe readings taken while the editor is compiling or importing don't count toward teardown, and detection is skipped entirely while busy. - Raise the probe's connect wait 50ms -> 250ms, as an overall budget shared across candidate hosts so the worst-case main-thread wait cannot multiply. - Honor UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S above 20s (ceiling now 120s; default unchanged): the old ceiling equalled the default, silently neutering the documented escape hatch for projects whose reloads or test boundaries legitimately exceed 20s. Same treatment for UNITY_MCP_SESSION_READY_WAIT_SECONDS, and both now share one bounded env-read helper. The remaining piece of CoplayDev#1207 (keepalive reload-awareness in WebSocketTransportClient) is untouched here: the resume machinery reworked in CoplayDev#1234 already covers reload boundaries, and the detector debounce removes the dominant churn source dsarno identified.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs (1)
164-187: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep service-race failures inside the retry loop.
OnAfterAssemblyReloadCore()intentionally schedules resume when config reads race reload, but these pre-start checks can still throw before the attempttry/catch. That can kill the fire-and-forget task and leaveResumeSessionKeyset with no retry scheduled.Suggested fix
- // Abort retries if the user switched transports while we were waiting. - if (!EditorConfigurationCache.Instance.UseHttpTransport) - { - SessionState.EraseBool(ResumeSessionKey); - return; - } - - // Never bounce a session someone else established while we were waiting - // (WebSocketTransportClient.StartAsync tears down a live connection first). - if (MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http)) - { - SessionState.EraseBool(ResumeSessionKey); - return; - } - try { + // Abort retries if the user switched transports while we were waiting. + if (!EditorConfigurationCache.Instance.UseHttpTransport) + { + SessionState.EraseBool(ResumeSessionKey); + return; + } + + // Never bounce a session someone else established while we were waiting. + if (MCPServiceLocator.TransportManager.IsRunning(TransportMode.Http)) + { + SessionState.EraseBool(ResumeSessionKey); + return; + } + bool started = await MCPServiceLocator.TransportManager.StartAsync(TransportMode.Http);🤖 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 `@MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs` around lines 164 - 187, Keep service-race failures inside the retry loop: in OnAfterAssemblyReloadCore, move the early transport/state checks that can throw (such as UseHttpTransport and TransportManager.IsRunning) into the existing try/catch around TransportManager.StartAsync, or wrap the whole resume attempt block so any transient service lookup/race exception is caught and triggers the retry scheduling path. Make sure SessionState.EraseBool(ResumeSessionKey) only happens on confirmed success or intentional abort, so a thrown pre-start check doesn’t cancel the fire-and-forget resume task without rescheduling.MCPForUnity/Editor/Services/EditorStateCache.cs (1)
532-569: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t reflect
CompilationPipeline.isCompilingas a public property. Unity 6000.3 doesn’t expose that member publicly, so this delegate will stay null and Play mode will fall back toEditorApplication.isCompiling, bringing back the false-positive busy state. Use the compilation started/finished events, or switch to a non-public lookup only if that member is present on the supported Unity range.🤖 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 `@MCPForUnity/Editor/Services/EditorStateCache.cs` around lines 532 - 569, The GetActualIsCompiling/PipelineIsCompiling path is reflecting CompilationPipeline.isCompiling as a public property, but that member isn’t publicly available on the newer Unity range so the delegate never binds. Update CreatePipelineIsCompilingDelegate to use a supported signal instead, such as the compilation started/finished events, or only fall back to a non-public lookup when the member exists on the target Unity versions. Keep the change localized to EditorStateCache.GetActualIsCompiling and its helper delegate setup.
🤖 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 `@MCPForUnity/Editor/Services/Transport/TransportManager.cs`:
- Around line 53-61: Validate the transport mode before touching the coalescing
tasks in `TransportManager.StartAsync`; the current ternary treats every
non-`Http` value as `Stdio`, which can incorrectly reuse `_stdioStartTask` for
unsupported enum values. Refactor the lookup and assignment logic into a
`switch` on `mode` so only `TransportMode.Http` and `TransportMode.Stdio` map to
`_httpStartTask` and `_stdioStartTask`, and let invalid modes preserve the
unsupported-mode contract instead of returning an unrelated task.
---
Outside diff comments:
In `@MCPForUnity/Editor/Services/EditorStateCache.cs`:
- Around line 532-569: The GetActualIsCompiling/PipelineIsCompiling path is
reflecting CompilationPipeline.isCompiling as a public property, but that member
isn’t publicly available on the newer Unity range so the delegate never binds.
Update CreatePipelineIsCompilingDelegate to use a supported signal instead, such
as the compilation started/finished events, or only fall back to a non-public
lookup when the member exists on the target Unity versions. Keep the change
localized to EditorStateCache.GetActualIsCompiling and its helper delegate
setup.
In `@MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs`:
- Around line 164-187: Keep service-race failures inside the retry loop: in
OnAfterAssemblyReloadCore, move the early transport/state checks that can throw
(such as UseHttpTransport and TransportManager.IsRunning) into the existing
try/catch around TransportManager.StartAsync, or wrap the whole resume attempt
block so any transient service lookup/race exception is caught and triggers the
retry scheduling path. Make sure SessionState.EraseBool(ResumeSessionKey) only
happens on confirmed success or intentional abort, so a thrown pre-start check
doesn’t cancel the fire-and-forget resume task without rescheduling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f277cbf6-2f05-45fe-89e0-1ebe64b90915
📒 Files selected for processing (17)
MCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Services/EditorStateCache.csMCPForUnity/Editor/Services/HttpAutoStartHandler.csMCPForUnity/Editor/Services/HttpBridgeReloadHandler.csMCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Services/Transport/TransportManager.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs
💤 Files with no reviewable changes (2)
- MCPForUnity/Editor/Constants/EditorPrefKeys.cs
- MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs
Comment on lines
+53
to
+61
| Task<bool> inFlight = mode == TransportMode.Http ? _httpStartTask : _stdioStartTask; | ||
| if (inFlight != null && !inFlight.IsCompleted) | ||
| { | ||
| return inFlight; | ||
| } | ||
|
|
||
| Task<bool> started = StartCoreAsync(mode); | ||
| if (mode == TransportMode.Http) _httpStartTask = started; | ||
| else _stdioStartTask = started; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate mode before reading or writing the coalescing slot.
Line 53 treats any non-Http value as Stdio; if an invalid enum reaches StartAsync while a Stdio start is pending, it returns that unrelated task instead of preserving the unsupported-mode contract. Use a switch for both lookup and assignment.
Suggested fix
- Task<bool> inFlight = mode == TransportMode.Http ? _httpStartTask : _stdioStartTask;
+ Task<bool> inFlight;
+ switch (mode)
+ {
+ case TransportMode.Http:
+ inFlight = _httpStartTask;
+ break;
+ case TransportMode.Stdio:
+ inFlight = _stdioStartTask;
+ break;
+ default:
+ return Task.FromException<bool>(
+ new ArgumentOutOfRangeException(nameof(mode), mode, "Unsupported transport mode"));
+ }
if (inFlight != null && !inFlight.IsCompleted)
{
return inFlight;
}
Task<bool> started = StartCoreAsync(mode);
- if (mode == TransportMode.Http) _httpStartTask = started;
- else _stdioStartTask = started;
+ switch (mode)
+ {
+ case TransportMode.Http:
+ _httpStartTask = started;
+ break;
+ case TransportMode.Stdio:
+ _stdioStartTask = started;
+ break;
+ }
return started;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Task<bool> inFlight = mode == TransportMode.Http ? _httpStartTask : _stdioStartTask; | |
| if (inFlight != null && !inFlight.IsCompleted) | |
| { | |
| return inFlight; | |
| } | |
| Task<bool> started = StartCoreAsync(mode); | |
| if (mode == TransportMode.Http) _httpStartTask = started; | |
| else _stdioStartTask = started; | |
| Task<bool> inFlight; | |
| switch (mode) | |
| { | |
| case TransportMode.Http: | |
| inFlight = _httpStartTask; | |
| break; | |
| case TransportMode.Stdio: | |
| inFlight = _stdioStartTask; | |
| break; | |
| default: | |
| return Task.FromException<bool>( | |
| new ArgumentOutOfRangeException(nameof(mode), mode, "Unsupported transport mode")); | |
| } | |
| if (inFlight != null && !inFlight.IsCompleted) | |
| { | |
| return inFlight; | |
| } | |
| Task<bool> started = StartCoreAsync(mode); | |
| switch (mode) | |
| { | |
| case TransportMode.Http: | |
| _httpStartTask = started; | |
| break; | |
| case TransportMode.Stdio: | |
| _stdioStartTask = started; | |
| break; | |
| } | |
| return started; |
🤖 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 `@MCPForUnity/Editor/Services/Transport/TransportManager.cs` around lines 53 -
61, Validate the transport mode before touching the coalescing tasks in
`TransportManager.StartAsync`; the current ternary treats every non-`Http` value
as `Stdio`, which can incorrectly reuse `_stdioStartTask` for unsupported enum
values. Refactor the lookup and assignment logic into a `switch` on `mode` so
only `TransportMode.Http` and `TransportMode.Stdio` map to `_httpStartTask` and
`_stdioStartTask`, and let invalid modes preserve the unsupported-mode contract
instead of returning an unrelated task.
… mode + retry-loop races Review follow-ups: - CompilationPipeline.isCompiling does not exist on the supported Unity range (reflection probe on 2021.3 and 6000.4: neither public nor non-public), so the reflected play-mode double-check never resolved and GetActualIsCompiling silently fell back to the raw signal — the CoplayDev#549 false positive was never actually mitigated. Track compilationStarted/compilationFinished events instead (public across the range); in Play mode trust the event-tracked state, outside it the raw signal is reliable. - TransportManager.StartAsync validates the mode before touching the coalescing slots, matching the class's unsupported-mode contract instead of routing unknown values to the stdio slot. - The resume retry loop's transport/IsRunning pre-checks moved inside the attempt try: a service read racing the reload boundary now burns a retry instead of killing the fire-and-forget task with the flag still set.
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.
Fixes #1229. Full credit to @jtpistella for the precise two-part root-cause diagnosis in the issue — both mechanisms verified line-for-line before writing this.
Root causes (verified at beta HEAD)
HttpAutoStartHandler's cctor set the reload-survivingSessionStatelatch before registering theEditorApplication.delayCall— a domain reload wipes the pending delegate, and the latch then short-circuits every later domain load.HttpBridgeReloadHandlerconsumed the one-shot resume flag before the deferred (delayCall) resume ran, and the nextbeforeAssemblyReloaddeleted it again because the bridge wasn't running.Fix
delayCall: the[InitializeOnLoad]cctor re-arms anEditorApplication.updatetick on every domain load, and the latch is written only when the deferred work actually dispatches. Services-not-ready frames retry (bounded per domain); the common case (auto-start off, nothing pending) skips the subscription entirely.TransportManager.StartAsyncserialized per mode: concurrent starts coalesce onto one in-flight attempt. Manual Connect, reload-resume, and auto-start all flow through it, andWebSocketTransportClient.StartAsynctears down a live connection first — previously two interleaved starts could bounce a just-established session.StartLocalHttpServerstops a still-booting server first).IServerManagementService.HasManagedServerLaunchHandleanswers the "can we watch the launch process die?" question live; without a handle the wait polls to the 5-minute hard cap instead of fail-fasting.EditorStateCache.GetActualIsCompiling(nowinternal, reflection bound once as a delegate): rawEditorApplication.isCompilingstays true for a whole play session under Recompile-After-Finished-Playing (EditorApplication.isCompilingFalse Positive in Play Mode #549), which would otherwise block resume until play exits.CancelPendingResume/CancelPendingReconnect) that also abort an in-flight retry loop.Intended behavior changes
Verification
tools/check-unity-versions.sh(2021.3.45f2 floor passes locally; CI covers the rest — the floor caught thatCompilationPipeline.isCompilingneeds reflection there).HttpBridgeReloadHandlerTests,HttpAutoStartHandlerTests,TransportManagerTests), including a scenario test replaying the exact multi-pass-compile sequence from the issue, and a pin on theStartAsynccoalescing contract. Full EditMode suite: 1002 passed / 0 failed locally.Relationship to other open PRs (checked before filing)
HttpAutoStartHandlerfor a different claimed race (EditorPrefs unavailable during[InitializeOnLoad]) — it does not fix either root cause here, and it removes the once-per-session latch (a manually stopped bridge would resurrect after every reload). If that EditorPrefs issue is real (we could not corroborate it — the [Bug] v10.0.0: HTTP auto-start and reload-resume both lost to delayCall/domain-reload race (Windows) #1229 repro itself shows EditorPrefs working in the cctor), it can layer on top of this change.AutoStartAsync— trivial textual conflict with this PR, easy rebase for whichever lands second.Known follow-ups (deliberately out of scope)
StdioBridgeReloadHandler.cs:65(flag deleted at a bridge-down boundary — primary) and:133(delayCalldeferral — secondary). Kept out to respect fix: harden localhost resolution and reload transport resilience on Windows #688's centralized stdio teardown sequencing; same pattern applies.isCompilingreflection probe can now consolidate ontoEditorStateCache.GetActualIsCompiling.McpConnectionSection.TryAutoStartSessionAsync) lacks the same reload survival; candidate for the same connect-pending pattern.Summary by CodeRabbit
New Features
Bug Fixes
Tests