From 18158fa3537b4512d3816065ae9bfd223b5e5daa Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:40:58 -0700 Subject: [PATCH 1/2] fix: stop tearing down healthy HTTP sessions on transient probe misses (#1207) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #1207 (keepalive reload-awareness in WebSocketTransportClient) is untouched here: the resume machinery reworked in #1234 already covers reload boundaries, and the detector debounce removes the dominant churn source dsarno identified. --- .../Services/ServerManagementService.cs | 17 ++++- .../Connection/McpConnectionSection.cs | 36 +++++++++- Server/src/transport/plugin_hub.py | 49 ++++++------- Server/tests/test_plugin_hub_wait_env.py | 35 +++++++++ ...cpConnectionSectionOrphanDetectionTests.cs | 72 +++++++++++++++++++ ...nectionSectionOrphanDetectionTests.cs.meta | 11 +++ 6 files changed, 193 insertions(+), 27 deletions(-) create mode 100644 Server/tests/test_plugin_hub_wait_env.py create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta diff --git a/MCPForUnity/Editor/Services/ServerManagementService.cs b/MCPForUnity/Editor/Services/ServerManagementService.cs index 1d943265d..6bcf6ac17 100644 --- a/MCPForUnity/Editor/Services/ServerManagementService.cs +++ b/MCPForUnity/Editor/Services/ServerManagementService.cs @@ -492,7 +492,10 @@ public bool IsLocalHttpServerReachable() return false; } - return TryConnectToLocalPort(uri.Host, uri.Port, timeoutMs: 50); + // 250ms, not 50ms: on a machine busy with test runs or domain reloads a 50ms + // connect wait produces false "server gone" readings that tore down healthy + // sessions via the orphaned-session detector (#1207). + return TryConnectToLocalPort(uri.Host, uri.Port, timeoutMs: 250); } catch { @@ -504,14 +507,24 @@ private static bool TryConnectToLocalPort(string host, int port, int timeoutMs) { try { + // timeoutMs is an overall budget shared across candidate hosts, so a + // filtered/dropped first candidate cannot multiply the worst-case wait + // (this can run on the editor UI tick). + var elapsed = System.Diagnostics.Stopwatch.StartNew(); foreach (string target in BuildLocalProbeHosts(host)) { + int remainingMs = timeoutMs - (int)elapsed.ElapsedMilliseconds; + if (remainingMs <= 0) + { + break; + } + try { using (var client = new TcpClient()) { var connectTask = client.ConnectAsync(target, port); - if (connectTask.Wait(timeoutMs) && client.Connected) + if (connectTask.Wait(remainingMs) && client.Connected) { return true; } diff --git a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs index 64a0c0212..2873c45ea 100644 --- a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs +++ b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs @@ -61,6 +61,7 @@ private enum TransportProtocol private string lastHealthStatus; private double lastLocalServerRunningPollTime; private bool lastLocalServerRunning; + private int consecutiveServerDownPolls; // Reference to Advanced section for health status updates private Action onHealthStatusUpdate; @@ -318,6 +319,26 @@ private void PersistHttpUrlFromField() RefreshHttpUi(); } + // Consecutive failed reachability polls (0.75s cadence) required before an active + // session is declared orphaned. A single stale reading used to be enough, and one + // missed probe on a machine busy with test runs or reloads tore down healthy + // sessions into reconnect churn (#1207). + internal const int OrphanedSessionDownPollThreshold = 3; + + internal static bool ShouldEndOrphanedSession( + bool httpLocalSelected, + bool sessionRunning, + bool toggleInProgress, + bool editorBusy, + int consecutiveDownPolls) + { + return httpLocalSelected + && sessionRunning + && !toggleInProgress + && !editorBusy + && consecutiveDownPolls >= OrphanedSessionDownPollThreshold; + } + public void UpdateConnectionStatus() { var bridgeService = MCPServiceLocator.Bridge; @@ -335,7 +356,9 @@ public void UpdateConnectionStatus() // Detect orphaned session: if HTTP Local session thinks it's running but the server is gone, // automatically end the session to keep UI in sync with reality. - if (showLocalServerControls && isRunning && !lastLocalServerRunning && !connectionToggleInProgress) + bool editorBusy = EditorApplication.isCompiling || EditorApplication.isUpdating; + if (ShouldEndOrphanedSession(showLocalServerControls, isRunning, connectionToggleInProgress, + editorBusy, consecutiveServerDownPolls)) { McpLog.Info("Server no longer running; ending orphaned session."); _ = EndOrphanedSessionAsync(); @@ -588,6 +611,17 @@ private void UpdateStartHttpButtonState() { lastLocalServerRunningPollTime = now; lastLocalServerRunning = MCPServiceLocator.Server.IsLocalHttpServerReachable(); + + // Probe results taken while the editor is busy are unreliable (main-thread + // stalls starve the connect wait), so they don't count toward teardown. + if (EditorApplication.isCompiling || EditorApplication.isUpdating) + { + consecutiveServerDownPolls = 0; + } + else + { + consecutiveServerDownPolls = lastLocalServerRunning ? 0 : consecutiveServerDownPolls + 1; + } } localServerRunning = lastLocalServerRunning; } diff --git a/Server/src/transport/plugin_hub.py b/Server/src/transport/plugin_hub.py index 1a71f30a7..56c961540 100644 --- a/Server/src/transport/plugin_hub.py +++ b/Server/src/transport/plugin_hub.py @@ -36,6 +36,24 @@ logger = logging.getLogger(__name__) + +def _read_bounded_wait_env(name: str, default_s: float, max_s: float) -> float: + """Read a wait-seconds env override, clamped to [0, max_s]. + + The ceiling exists to keep a typo from stalling every command, but it must sit + well above the default so explicit overrides actually take effect (#1207). + """ + raw = os.environ.get(name) + if raw is None: + return max(0.0, min(default_s, max_s)) + try: + value = float(raw) + except ValueError as e: + logger.warning("Invalid %s=%r, using default %s: %s", name, raw, default_s, e) + value = default_s + return max(0.0, min(value, max_s)) + + # ---------- MCP session tracking ---------- # FastMCP doesn't expose active MCP client sessions. We patch # ``MiddlewareServerSession.__aenter__`` once to register every new @@ -854,19 +872,11 @@ async def _resolve_session_id( # (e.g., via status file, heartbeat, or explicit "reloading" signal from Unity) # rather than blindly waiting up to 20s. See Issue #657. # - # Configurable via: UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S (default: 20.0, max: 20.0) - try: - max_wait_s = float( - os.environ.get("UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S", "20.0")) - except ValueError as e: - raw_val = os.environ.get( - "UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S", "20.0") - logger.warning( - "Invalid UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S=%r, using default 20.0: %s", - raw_val, e) - max_wait_s = 20.0 - # Clamp to [0, 20] to prevent misconfiguration from causing excessive waits - max_wait_s = max(0.0, min(max_wait_s, 20.0)) + # Configurable via: UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S (default: 20.0, max: 120.0). + # The ceiling used to equal the default, which silently neutered the override for + # projects whose reloads/test boundaries legitimately exceed 20s (#1207). + max_wait_s = _read_bounded_wait_env( + "UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S", default_s=20.0, max_s=120.0) if not retry_on_reload: max_wait_s = 0.0 retry_ms = float(getattr(config, "reload_retry_ms", 250)) @@ -1009,17 +1019,8 @@ async def send_command_for_instance( # a main-thread ping command (handled by TransportCommandDispatcher) rather than waiting on # register_tools (which can be delayed by EditorApplication.delayCall). if retry_on_reload and command_type in cls._FAST_FAIL_COMMANDS and command_type != "ping": - try: - max_wait_s = float(os.environ.get( - "UNITY_MCP_SESSION_READY_WAIT_SECONDS", "6")) - except ValueError as e: - raw_val = os.environ.get( - "UNITY_MCP_SESSION_READY_WAIT_SECONDS", "6") - logger.warning( - "Invalid UNITY_MCP_SESSION_READY_WAIT_SECONDS=%r, using default 6.0: %s", - raw_val, e) - max_wait_s = 6.0 - max_wait_s = max(0.0, min(max_wait_s, 20.0)) + max_wait_s = _read_bounded_wait_env( + "UNITY_MCP_SESSION_READY_WAIT_SECONDS", default_s=6.0, max_s=120.0) if max_wait_s > 0: deadline = time.monotonic() + max_wait_s while time.monotonic() < deadline: diff --git a/Server/tests/test_plugin_hub_wait_env.py b/Server/tests/test_plugin_hub_wait_env.py new file mode 100644 index 000000000..ec29f0455 --- /dev/null +++ b/Server/tests/test_plugin_hub_wait_env.py @@ -0,0 +1,35 @@ +"""Env-override clamping for session resolve/ready waits (#1207). + +The ceiling used to equal the default (20s), which silently neutered +UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S for projects whose domain reloads or +test boundaries legitimately exceed 20s. +""" + +from transport.plugin_hub import _read_bounded_wait_env + +ENV = "UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S" + + +def test_default_when_unset(monkeypatch): + monkeypatch.delenv(ENV, raising=False) + assert _read_bounded_wait_env(ENV, default_s=20.0, max_s=120.0) == 20.0 + + +def test_override_above_old_ceiling_is_honored(monkeypatch): + monkeypatch.setenv(ENV, "45") + assert _read_bounded_wait_env(ENV, default_s=20.0, max_s=120.0) == 45.0 + + +def test_override_clamped_to_ceiling(monkeypatch): + monkeypatch.setenv(ENV, "600") + assert _read_bounded_wait_env(ENV, default_s=20.0, max_s=120.0) == 120.0 + + +def test_negative_clamped_to_zero(monkeypatch): + monkeypatch.setenv(ENV, "-5") + assert _read_bounded_wait_env(ENV, default_s=20.0, max_s=120.0) == 0.0 + + +def test_invalid_falls_back_to_default(monkeypatch): + monkeypatch.setenv(ENV, "not-a-number") + assert _read_bounded_wait_env(ENV, default_s=20.0, max_s=120.0) == 20.0 diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs new file mode 100644 index 000000000..af9220945 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs @@ -0,0 +1,72 @@ +using NUnit.Framework; +using MCPForUnity.Editor.Windows.Components.Connection; + +namespace MCPForUnityTests.Editor.Windows +{ + /// + /// Regression tests for #1207: the orphaned-session detector must require several + /// consecutive failed reachability polls (and a quiet editor) before tearing down an + /// active session — a single stale 50ms probe reading used to be enough, so busy + /// machines cycled healthy sessions into reconnect churn. + /// + public class McpConnectionSectionOrphanDetectionTests + { + private static readonly int Threshold = McpConnectionSection.OrphanedSessionDownPollThreshold; + + [Test] + public void EndsSession_AtThreshold_WhenIdleAndRunning() + { + Assert.IsTrue(McpConnectionSection.ShouldEndOrphanedSession( + httpLocalSelected: true, sessionRunning: true, toggleInProgress: false, + editorBusy: false, consecutiveDownPolls: Threshold)); + } + + [Test] + public void SingleFailedPoll_DoesNotEndSession() + { + Assert.IsFalse(McpConnectionSection.ShouldEndOrphanedSession( + httpLocalSelected: true, sessionRunning: true, toggleInProgress: false, + editorBusy: false, consecutiveDownPolls: 1)); + } + + [Test] + public void BelowThreshold_DoesNotEndSession() + { + Assert.IsFalse(McpConnectionSection.ShouldEndOrphanedSession( + httpLocalSelected: true, sessionRunning: true, toggleInProgress: false, + editorBusy: false, consecutiveDownPolls: Threshold - 1)); + } + + [Test] + public void BusyEditor_DefersEvenPastThreshold() + { + Assert.IsFalse(McpConnectionSection.ShouldEndOrphanedSession( + httpLocalSelected: true, sessionRunning: true, toggleInProgress: false, + editorBusy: true, consecutiveDownPolls: Threshold + 2)); + } + + [Test] + public void ToggleInProgress_DoesNotEndSession() + { + Assert.IsFalse(McpConnectionSection.ShouldEndOrphanedSession( + httpLocalSelected: true, sessionRunning: true, toggleInProgress: true, + editorBusy: false, consecutiveDownPolls: Threshold)); + } + + [Test] + public void NonHttpLocalTransport_NeverEndsSession() + { + Assert.IsFalse(McpConnectionSection.ShouldEndOrphanedSession( + httpLocalSelected: false, sessionRunning: true, toggleInProgress: false, + editorBusy: false, consecutiveDownPolls: Threshold + 5)); + } + + [Test] + public void SessionNotRunning_NothingToEnd() + { + Assert.IsFalse(McpConnectionSection.ShouldEndOrphanedSession( + httpLocalSelected: true, sessionRunning: false, toggleInProgress: false, + editorBusy: false, consecutiveDownPolls: Threshold + 5)); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta new file mode 100644 index 000000000..201e07da5 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 2efc49ed806942c79d399d78c69e6302 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: From 2c8ae3ec33327465dba991f3d40cf1c53f3de6b9 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:49:13 -0700 Subject: [PATCH 2/2] fix(editor): reset the orphan down-poll streak when a session starts Review follow-up: a streak accumulated while no session was running (detector inert, counter still counting) survived into a freshly started session and could satisfy orphan detection before the first post-start probe refreshed. Reset on the not-running -> running transition, which covers every start path (manual Connect, auto-start, resume) since UpdateConnectionStatus runs on the UI tick. --- .../Components/Connection/McpConnectionSection.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs index 2873c45ea..1267c15f1 100644 --- a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs +++ b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs @@ -62,6 +62,7 @@ private enum TransportProtocol private double lastLocalServerRunningPollTime; private bool lastLocalServerRunning; private int consecutiveServerDownPolls; + private bool sessionWasRunning; // Reference to Advanced section for health status updates private Action onHealthStatusUpdate; @@ -354,6 +355,15 @@ public void UpdateConnectionStatus() // NOTE: This also updates lastLocalServerRunning which is used below for session toggle visibility. UpdateStartHttpButtonState(); + // A down-poll streak accumulated before/while no session was running must not + // carry into a freshly started session — it would satisfy orphan detection + // before the first post-start probe refreshes (the poll cadence is 0.75s). + if (isRunning && !sessionWasRunning) + { + consecutiveServerDownPolls = 0; + } + sessionWasRunning = isRunning; + // Detect orphaned session: if HTTP Local session thinks it's running but the server is gone, // automatically end the session to keep UI in sync with reality. bool editorBusy = EditorApplication.isCompiling || EditorApplication.isUpdating;