fix(editor): survive domain reloads in HTTP auto-start and reload-resume (#1229)#1234
Open
Scriptwonder wants to merge 2 commits into
Open
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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 update-driven ticks. Adds a managed-launch-handle signal, coalesces TransportManager start calls, updates connection cancellation wiring, 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: update tick
HttpAutoStartHandler->>SessionState: read SessionInitKey / ConnectPendingKey
HttpAutoStartHandler->>HttpBridgeReloadHandler: check IsResumePending
alt resume pending
HttpBridgeReloadHandler->>TransportManager: ResumeHttpWithRetriesAsync
HttpBridgeReloadHandler->>SessionState: erase ResumeSessionKey on completion/failure
else auto-start or reconnect
HttpAutoStartHandler->>TransportManager: StartAsync / ReconnectAsync
HttpAutoStartHandler->>SessionState: set or clear ConnectPendingKey
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
… 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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Unity Editor-side HTTP bridge lifecycle to survive domain reloads during compilation, ensuring auto-start and reload-resume behave reliably across multi-pass compiles and interrupted startup sequences (Fixes #1229).
Changes:
- Reworked HTTP auto-start and reload-resume deferral to use reload-safe
EditorApplication.updateticks and SessionState-backed flags. - Added coalescing in
TransportManager.StartAsyncto prevent concurrent start races from bouncing live connections. - Added EditMode tests covering the new retry/flag semantics and the StartAsync coalescing contract.
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs | Updates characterization text to reflect the new HTTP resume flag storage semantics. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs | Adds a synchronous fake transport client to support transport-lifecycle tests without async test support. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs.meta | Adds Unity meta for the new test script. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs | Adds tests pinning StartAsync coalescing behavior and restart-after-complete behavior. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs.meta | Adds Unity meta for the new test script. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs | Adds tests validating reload-resume flag persistence/consumption across multi-pass reload boundaries. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs.meta | Adds Unity meta for the new test script. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs | Adds tests for tick decision logic, latch semantics, and reconnect-pending behavior. |
| MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs | Removes the legacy HTTP resume EditorPrefs key from the prefs window listing. |
| MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs | Routes user-intent cancel paths through CancelPendingResume / CancelPendingReconnect seams. |
| MCPForUnity/Editor/Services/Transport/TransportManager.cs | Coalesces concurrent StartAsync calls per transport mode onto one in-flight attempt. |
| MCPForUnity/Editor/Services/ServerManagementService.cs | Exposes whether the current domain holds a managed server Process handle. |
| MCPForUnity/Editor/Services/IServerManagementService.cs | Adds HasManagedServerLaunchHandle to support post-reload reconnect wait behavior. |
| MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs | Moves HTTP resume flag to SessionState, adds migration cleanup, and defers resume via update tick + bounded retry semantics. |
| MCPForUnity/Editor/Services/HttpAutoStartHandler.cs | Replaces delayCall-based auto-start with reload-safe update tick + bounded “services not ready” retries and connect-pending reconnect. |
| MCPForUnity/Editor/Services/EditorStateCache.cs | Reworks the “actual compiling” signal to use compilation start/finish events for play-mode false positives (#549). |
| MCPForUnity/Editor/Constants/EditorPrefKeys.cs | Removes the legacy HTTP resume EditorPrefs key constant. |
Files not reviewed (3)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs.meta: Generated file
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs.meta: Generated file
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportManagerTests.cs.meta: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+38
| /// <summary> | ||
| /// True when this domain launched the local HTTP server and still holds its Process | ||
| /// handle. Handles do not survive domain reloads, so false also means "unknown" — | ||
| /// callers should wait rather than fail fast. | ||
| /// </summary> | ||
| bool HasManagedServerLaunchHandle { get; } |
Comment on lines
+40
to
+45
| const string migratedKey = "MCPForUnity.ResumeHttpAfterReload.Migrated"; | ||
| if (!SessionState.GetBool(migratedKey, false)) | ||
| { | ||
| EditorPrefs.DeleteKey(ResumeSessionKey); | ||
| SessionState.SetBool(migratedKey, true); | ||
| } |
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
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