Fix stdio reloads #402
Fix stdio reloads #402
Conversation
Well most of them, I modified a few
It's less clusterd
Works like Windsurf, but it sucks ass
We mirror what we've done with the HTTP/websocket connection We also ensure the states from the stdio/HTTP connections are handled separately. Things now work as expected
WalkthroughRefactors transport management to per-transport (HTTP and Stdio) state and mode-aware APIs, adds a persistent ResumeStdioAfterReload preference, introduces a StdioBridgeReloadHandler to manage stdio across assembly reloads, and updates callers to use TransportManager with explicit TransportMode. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Editor as Unity Editor
participant Before as BeforeReload (AssemblyReloadEvents.before)
participant StdioHandler as StdioBridgeReloadHandler
participant EditorPrefs as EditorPrefs
participant TransportMgr as TransportManager
participant Stdio as Stdio Bridge
participant After as AfterReload (AssemblyReloadEvents.after)
participant Window as MCPForUnityEditorWindow
Editor->>Before: beforeAssemblyReload
Before->>StdioHandler: onBeforeAssemblyReload()
StdioHandler->>TransportMgr: Query IsRunning(Stdio) & IsHttpInUse()
alt stdio running && http not in use
StdioHandler->>EditorPrefs: Set ResumeStdioAfterReload = true
StdioHandler->>TransportMgr: StopAsync(Stdio)
TransportMgr->>Stdio: Stop transport client
else
StdioHandler->>EditorPrefs: Clear ResumeStdioAfterReload
end
Editor->>After: afterAssemblyReload
After->>StdioHandler: onAfterAssemblyReload()
StdioHandler->>EditorPrefs: Read ResumeStdioAfterReload
StdioHandler->>TransportMgr: Query IsHttpInUse()
alt resume conditions met
StdioHandler->>EditorPrefs: Clear ResumeStdioAfterReload
StdioHandler->>TransportMgr: StartAsync(Stdio)
TransportMgr->>Stdio: Start transport client
StdioHandler->>Window: Request health verification
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-13T13:27:23.040ZApplied to files:
📚 Learning: 2025-09-03T16:00:55.839ZApplied to files:
🧬 Code graph analysis (2)MCPForUnity/Editor/Services/BridgeControlService.cs (2)
MCPForUnity/Editor/Services/Transport/TransportManager.cs (1)
🔇 Additional comments (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs (3)
29-42: Consolidate duplicate condition check.The condition
!useHttp && isRunningis evaluated twice (lines 29 and 38). Consider consolidating into a single block.- if (!useHttp && isRunning) - { - EditorPrefs.SetBool(EditorPrefKeys.ResumeStdioAfterReload, true); - } - else - { - EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); - } - - if (!useHttp && isRunning) - { - // Stop only the stdio bridge; leave HTTP untouched if it is running concurrently. - MCPServiceLocator.TransportManager.StopAsync(TransportMode.Stdio); - } + bool shouldResume = !useHttp && isRunning; + if (shouldResume) + { + EditorPrefs.SetBool(EditorPrefKeys.ResumeStdioAfterReload, true); + // Stop only the stdio bridge; leave HTTP untouched if it is running concurrently. + MCPServiceLocator.TransportManager.StopAsync(TransportMode.Stdio); + } + else + { + EditorPrefs.DeleteKey(EditorPrefKeys.ResumeStdioAfterReload); + }
38-42: Consider adding error handling for the async stop operation.
StopAsyncis called fire-and-forget, so exceptions won't be caught by the surroundingtry-catch. For consistency withHttpBridgeReloadHandler(lines 41-48), consider usingContinueWithto log errors.if (!useHttp && isRunning) { // Stop only the stdio bridge; leave HTTP untouched if it is running concurrently. - MCPServiceLocator.TransportManager.StopAsync(TransportMode.Stdio); + var stopTask = MCPServiceLocator.TransportManager.StopAsync(TransportMode.Stdio); + stopTask.ContinueWith(t => + { + if (t.IsFaulted && t.Exception != null) + { + McpLog.Warn($"Error stopping stdio bridge before reload: {t.Exception.GetBaseException().Message}"); + } + }, System.Threading.Tasks.TaskScheduler.Default); }
77-88: Async start exceptions may be silently swallowed.
StartAsyncis called fire-and-forget, so thecatchblock won't capture async failures. For consistency withHttpBridgeReloadHandler(lines 95-113), consider usingContinueWithto handle the result and log failures.private static void TryStartBridgeImmediate() { - try - { - MCPServiceLocator.TransportManager.StartAsync(TransportMode.Stdio); - MCPForUnity.Editor.Windows.MCPForUnityEditorWindow.RequestHealthVerification(); - } - catch (Exception ex) - { - McpLog.Warn($"Failed to resume stdio bridge after reload: {ex.Message}"); - } + var startTask = MCPServiceLocator.TransportManager.StartAsync(TransportMode.Stdio); + startTask.ContinueWith(t => + { + if (t.IsFaulted) + { + var baseEx = t.Exception?.GetBaseException(); + McpLog.Warn($"Failed to resume stdio bridge after reload: {baseEx?.Message}"); + return; + } + if (!t.Result) + { + McpLog.Warn("Failed to resume stdio bridge after domain reload"); + } + else + { + MCPForUnity.Editor.Windows.MCPForUnityEditorWindow.RequestHealthVerification(); + } + }, System.Threading.Tasks.TaskScheduler.Default); }MCPForUnity/Editor/Services/BridgeControlService.cs (1)
77-78: Consider ensuringActiveModereflects current preference.
ActiveModereturns_preferredModewhich is only updated whenResolvePreferredMode()is called. If accessed before other methods, it may return the stale default (Http). Consider callingResolvePreferredMode()in the getter for consistency.- public TransportMode? ActiveMode => _preferredMode; + public TransportMode? ActiveMode => ResolvePreferredMode();MCPForUnity/Editor/Services/Transport/TransportManager.cs (3)
38-46: Mode-aware client creation andStartAsyncflow are sound; minor robustness tweaks possibleThe
GetOrCreateClient(mode)switch andStartAsync(TransportMode mode)logic look correct and align with the new per-mode model:
- Correct factory is used per mode.
- On failure (
started == false) you clean up viaclient.StopAsync()and mark the mode as disconnected.- On success you prefer
client.Statebut fall back to a sensibleConnecteddefault.Two optional robustness nits:
- In the failure path,
await client.StopAsync();can still throw and bypass yourUpdateState(...)call. Mirroring the try/catch you use inStopAsync’sStopClienthelper (log & still update state) would make this more resilient.- For the failure state, you currently use
mode.ToString().ToLowerInvariant()as the transport name, whereas success usesclient.TransportName. Usingclient.TransportNamein both paths would keep naming consistent and avoid subtle discrepancies if the enum name and transport name ever diverge.These are not blockers, just cleanups to consider.
Also applies to: 48-62
64-88:StopAsyncsemantics and implementation look reasonable; ensure “stop all” default is intendedThe refactored
StopAsync(TransportMode? mode = null)looks solid:
- Local
StopClientdefensively handles null clients, catches/logs exceptions, and always updates the per-mode state.mode == nullstopping both transports is a sensible generalization now that you support multiple modes.- Mode-specific paths delegate correctly to HTTP vs stdio.
One thing to double‑check is API semantics: if any existing callers relied on previous “stop current active transport only” behavior,
StopAsync()now stopping both could be a behavior change. If that’s not desired, you may want an explicitStopAllAsync()helper and keepStopAsync()delegating to “preferred”/“active” mode instead.If the new “stop all” default is intentional, the current implementation is fine as-is.
91-103: Prefer explicitswitchonTransportModeinstead ofmode == Http ? ... : ...for future-proofingIn
VerifyAsync,GetState,IsRunning, andUpdateStateyou use patterns like:
mode == TransportMode.Http ? _httpClient : _stdioClientmode == TransportMode.Http ? _httpState : _stdioStateif (mode == TransportMode.Http) { ... } else { ... }With the current enum (
Http,Stdio), this works, but it silently treats any futureTransportModevalue asStdio. That’s inconsistent withGetOrCreateClient, which throws for unknown modes, and could cause subtle bugs if new modes are added later.Consider refactoring these to explicit
switches, mirroringGetOrCreateClient:private IMcpTransportClient GetClient(TransportMode mode) => mode switch { TransportMode.Http => _httpClient, TransportMode.Stdio => _stdioClient, _ => throw new ArgumentOutOfRangeException(nameof(mode), mode, "Unsupported transport mode"), }; public TransportState GetState(TransportMode mode) => mode switch { TransportMode.Http => _httpState, TransportMode.Stdio => _stdioState, _ => throw new ArgumentOutOfRangeException(nameof(mode), mode, "Unsupported transport mode"), };…and similarly in
VerifyAsync,IsRunning, andUpdateState. This keeps behavior well-defined and consistent if/when additional modes are introduced.Also applies to: 105-108, 110-110, 112-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs(1 hunks)MCPForUnity/Editor/Services/BridgeControlService.cs(4 hunks)MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs(5 hunks)MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs(1 hunks)MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs.meta(1 hunks)MCPForUnity/Editor/Services/Transport/TransportManager.cs(3 hunks)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs(0 hunks)
💤 Files with no reviewable changes (1)
- MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Services/StdioBridgeReloadHandler.csMCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs.metaMCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/StdioBridgeReloadHandler.csMCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs.meta
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs.meta
🧬 Code graph analysis (4)
MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs (4)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)MCPForUnity/Editor/Services/Transport/TransportManager.cs (3)
TransportManager(11-123)TransportManager(20-25)IsRunning(110-110)MCPForUnity/Editor/Services/BridgeControlService.cs (1)
TransportMode(25-30)MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)
MCPForUnity/Editor/Services/BridgeControlService.cs (2)
MCPForUnity/Editor/Services/Transport/TransportManager.cs (1)
IsRunning(110-110)MCPForUnity/Editor/Services/IBridgeControlService.cs (2)
BridgeVerificationResult(47-47)BridgeVerificationResult(60-81)
MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs (4)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Services/Transport/TransportManager.cs (3)
TransportManager(11-123)TransportManager(20-25)IsRunning(110-110)MCPForUnity/Editor/Services/BridgeControlService.cs (1)
TransportMode(25-30)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)
MCPForUnity/Editor/Services/Transport/TransportManager.cs (1)
MCPForUnity/Editor/Services/BridgeControlService.cs (4)
TransportMode(25-30)Task(80-97)Task(99-110)Task(112-118)
🔇 Additional comments (12)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
13-14: LGTM!The new
ResumeStdioAfterReloadconstant follows the established naming convention and is logically grouped with the relatedResumeHttpAfterReloadkey.MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs.meta (1)
1-11: LGTM!Standard Unity metadata file with a unique GUID for the new
StdioBridgeReloadHandler.csscript.MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs (1)
10-20: Good implementation of the stdio reload handler.The structure correctly mirrors
HttpBridgeReloadHandlerby subscribing toAssemblyReloadEventsand managing the resume flag viaEditorPrefs. This aligns well with the PR objective of bringing stdio behavior in line with websocket connection behavior.MCPForUnity/Editor/Services/BridgeControlService.cs (3)
52-59: LGTM!The
IsRunningproperty now correctly resolves the preferred mode and queries the transport manager for mode-specific state. This aligns with the per-mode transport management approach.
99-110: LGTM!
StopAsynccorrectly resolves the preferred mode and stops only that specific transport, preserving the existing error handling pattern.
120-136: LGTM with note on blocking call.The
Verify(int port)method usesGetAwaiter().GetResult()to meet the synchronous interface contract. This is acceptable in the Unity Editor context, though be mindful that it blocks the calling thread.MCPForUnity/Editor/Services/HttpBridgeReloadHandler.cs (4)
27-28: LGTM!The update correctly uses
TransportManager.IsRunning(TransportMode.Http)for explicit mode-aware state checking.
62-64: Good defensive check.The addition of
useHttpverification ensures HTTP transport only resumes if it remains the preferred transport, preventing incorrect behavior if the user switched preferences during reload.
39-49: LGTM!The stop operation correctly targets
TransportMode.Httpand maintains proper error handling viaContinueWith.
95-113: LGTM!The start operation correctly uses
TransportMode.Httpwith proper error handling and health verification on success.MCPForUnity/Editor/Services/Transport/TransportManager.cs (2)
13-16: Per-transport clients and state look consistent with multi-mode designSplitting
_httpClient/_stdioClientand_httpState/_stdioStatecleanly supports concurrent tracking of both transports and aligns with the PR objective (HTTP/websocket vs stdio). No correctness issues spotted here.
27-28: Mark deprecatedActiveTransportandActiveModeproperties with[Obsolete]attribute and provide clear migration pathBoth properties now always return
null, creating risk of silent behavioral regressions for existing callers. To improve discoverability and safety:
- Add
[Obsolete("Use per-mode APIs (StartAsync(mode), GetState(mode), IsRunning(mode), etc.) instead.", error: false)]to both properties- Add XML doc comments indicating they will always return
nulland will be removed in a future version
… mode The ActiveMode property now calls ResolvePreferredMode() to return the actual active transport mode rather than just the preferred mode setting.
- Consolidated the !useHttp && isRunning checks into a single shouldResume flag. - Wrapped the fire-and-forget StopAsync in a continuation that logs faults (matching the HTTP handler pattern). - Wrapped StartAsync in a continuation that logs failures and only triggers the health check on success.
… handling - Replace if-else chains with switch expressions for better readability and exhaustiveness checking - Add GetClient() helper method to centralize client retrieval logic - Wrap StopAsync in try-catch to log failures when stopping a failed transport - Use client.TransportName instead of mode.ToString() for consistent naming in error messages
1 participant
We fix the auto-reconnect logic for stdio. That is, if you're connected via stdio, and you reload the domain, then you'll still be connected via stdio. Similar to our websocket connection
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.