diff --git a/MCPForUnity/Editor/Services/Transport/Transports/StdioTransportClient.cs b/MCPForUnity/Editor/Services/Transport/Transports/StdioTransportClient.cs index cf07087f1..afe77d912 100644 --- a/MCPForUnity/Editor/Services/Transport/Transports/StdioTransportClient.cs +++ b/MCPForUnity/Editor/Services/Transport/Transports/StdioTransportClient.cs @@ -1,6 +1,7 @@ using System; using System.Threading.Tasks; using MCPForUnity.Editor.Helpers; +using UnityEditor; namespace MCPForUnity.Editor.Services.Transport.Transports { @@ -15,19 +16,50 @@ public class StdioTransportClient : IMcpTransportClient public string TransportName => "stdio"; public TransportState State => _state; - public Task StartAsync() + // Bounded window to wait for the bridge to actually bind after StartAutoConnect. Covers the + // OS port-release delay after a domain reload (the same port can stay held for a few hundred + // ms, longer on Windows/macOS), during which Start() defers binding to an editor-idle retry + // or falls back to a new port once BusyPortFallbackWindowSeconds elapses. + internal const double ReadyWaitTimeoutSeconds = 5.0; + private const int ReadyPollIntervalMs = 100; + + // Pure predicate (unit-testable): keep polling while the bridge is not yet ready and the + // bounded window has not elapsed. + internal static bool ShouldKeepWaitingForReady(bool bridgeReady, double secondsWaited) + => !bridgeReady && secondsWaited < ReadyWaitTimeoutSeconds; + + public async Task StartAsync() { try { StdioBridgeHost.StartAutoConnect(); - _state = TransportState.Connected("stdio", port: StdioBridgeHost.GetCurrentPort()); - return Task.FromResult(true); + + // StartAutoConnect triggers the bind, but when the previous port is still held after a + // domain reload it defers binding to an editor-idle retry — so IsRunning can still be + // false right here. Wait (bounded) for the bridge to actually become ready before + // reporting success; otherwise callers immediately verify a bool that was never + // awaited and get a spurious "Bridge not running" (the Start Session race). + bool ready = await WaitForBridgeReadyAsync(); + _state = ready + ? TransportState.Connected("stdio", port: StdioBridgeHost.GetCurrentPort()) + : TransportState.Disconnected("stdio", "Bridge not ready yet (port still releasing after reload)."); + return ready; } catch (Exception ex) { _state = TransportState.Disconnected("stdio", ex.Message); - return Task.FromResult(false); + return false; + } + } + + private static async Task WaitForBridgeReadyAsync() + { + double start = EditorApplication.timeSinceStartup; + while (ShouldKeepWaitingForReady(StdioBridgeHost.IsRunning, EditorApplication.timeSinceStartup - start)) + { + await Task.Delay(ReadyPollIntervalMs); } + return StdioBridgeHost.IsRunning; } public Task StopAsync() diff --git a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs index 64a0c0212..6c8b4a389 100644 --- a/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs +++ b/MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs @@ -59,6 +59,7 @@ private enum TransportProtocol private bool httpServerToggleInProgress; private Task verificationTask; private string lastHealthStatus; + private int consecutiveVerifyFailures; private double lastLocalServerRunningPollTime; private bool lastLocalServerRunning; @@ -318,6 +319,17 @@ private void PersistHttpUrlFromField() RefreshHttpUi(); } + // Consecutive failed bridge verifications required before the health indicator is + // shown as Unhealthy ("broken"). A stdio domain reload briefly rebinds the listener + // — the port can even hop (e.g. 6402 -> 6403) — during which a single VerifyAsync() + // ping transiently fails with "Bridge not running" even though the bridge recovers on + // its own. Flashing broken on that lone miss is misleading, so debounce: require + // repeated failures before surfacing it. + internal const int UnhealthyVerificationThreshold = 2; + + internal static bool ShouldReportUnhealthy(int consecutiveVerifyFailures) + => consecutiveVerifyFailures >= UnhealthyVerificationThreshold; + public void UpdateConnectionStatus() { var bridgeService = MCPServiceLocator.Bridge; @@ -1044,6 +1056,7 @@ private async Task VerifyBridgeConnectionInternalAsync() { newStatus = HealthStatus.Healthy; isHealthy = true; + consecutiveVerifyFailures = 0; // Only log if state changed if (lastHealthStatus != newStatus) @@ -1054,8 +1067,11 @@ private async Task VerifyBridgeConnectionInternalAsync() } else if (result.HandshakeValid) { + // Handshake succeeded, so the socket is reachable — not the transient + // rebind case. Ping failing is a genuine warning; reset the miss counter. newStatus = HealthStatus.PingFailed; isHealthy = false; + consecutiveVerifyFailures = 0; // Log once per distinct warning state if (lastHealthStatus != newStatus) @@ -1066,6 +1082,19 @@ private async Task VerifyBridgeConnectionInternalAsync() } else { + // Could not reach the bridge at all. A stdio reload rebinds the listener + // (the port can hop), so one miss does not mean it is down. Debounce: only + // surface "broken" after repeated failures (mirrors #1207). Until then leave + // the indicator in its current state; the next verification resolves it. + consecutiveVerifyFailures++; + if (!ShouldReportUnhealthy(consecutiveVerifyFailures)) + { + McpLog.Debug( + $"Connection verification miss {consecutiveVerifyFailures}/{UnhealthyVerificationThreshold} " + + $"(transient, not surfacing): {result.Message}"); + return; + } + newStatus = HealthStatus.Unhealthy; isHealthy = false; diff --git a/Server/src/services/tools/debug_request_context.py b/Server/src/services/tools/debug_request_context.py index ddef0fd1b..f997a42f2 100644 --- a/Server/src/services/tools/debug_request_context.py +++ b/Server/src/services/tools/debug_request_context.py @@ -46,16 +46,13 @@ async def debug_request_context(ctx: Context) -> dict[str, Any]: # List all ctx attributes for debugging ctx_attrs = [attr for attr in dir(ctx) if not attr.startswith("_")] - # Get session state info via middleware + # Get session state info via middleware. Active-instance storage now lives + # in FastMCP's session-scoped state store, keyed by ctx.session_id, so + # there is no global dict to enumerate — that snapshot was a footgun + # anyway (it exposed every connected client's selection). middleware = get_unity_instance_middleware() - derived_key = await middleware.get_session_key(ctx) active_instance = await middleware.get_active_instance(ctx) - # Debugging middleware internals - # NOTE: These fields expose internal implementation details and may change between versions. - with middleware._lock: - all_keys = list(middleware._active_by_key.keys()) - # Debugging PluginHub state plugin_hub_configured = PluginHub.is_configured() @@ -77,9 +74,7 @@ async def debug_request_context(ctx: Context) -> dict[str, Any]: "client_id": ctx_client_id, }, "session_state": { - "derived_key": derived_key, "active_instance": active_instance, - "all_keys_in_store": all_keys, "plugin_hub_configured": plugin_hub_configured, "middleware_id": id(middleware), }, diff --git a/Server/src/services/tools/set_active_instance.py b/Server/src/services/tools/set_active_instance.py index 6b90b351d..586724f09 100644 --- a/Server/src/services/tools/set_active_instance.py +++ b/Server/src/services/tools/set_active_instance.py @@ -54,7 +54,7 @@ async def set_active_instance( "message": f"Active instance set to {resolved_id}", "data": { "instance": resolved_id, - "session_key": await middleware.get_session_key(ctx), + "session_id": getattr(ctx, "session_id", None), }, } @@ -138,18 +138,15 @@ async def set_active_instance( "error": "Internal error: Instance resolution failed." } - # Store selection in middleware (session-scoped) + # Store selection in middleware (session-scoped via FastMCP context state). middleware = get_unity_instance_middleware() - # We use middleware.set_active_instance to persist the selection. - # The session key is an internal detail but useful for debugging response. await middleware.set_active_instance(ctx, resolved.id) - session_key = await middleware.get_session_key(ctx) return { "success": True, "message": f"Active instance set to {resolved.id}", "data": { "instance": resolved.id, - "session_key": session_key, + "session_id": getattr(ctx, "session_id", None), }, } diff --git a/Server/src/transport/legacy/unity_connection.py b/Server/src/transport/legacy/unity_connection.py index 1d703a862..aefa72304 100644 --- a/Server/src/transport/legacy/unity_connection.py +++ b/Server/src/transport/legacy/unity_connection.py @@ -550,17 +550,21 @@ def _resolve_instance_id(self, instance_identifier: str | None, instances: list[ if self._default_instance_id: instance_identifier = self._default_instance_id logger.debug(f"Using default instance: {instance_identifier}") + elif len(instances) == 1: + # Sole instance: unambiguous, select it without requiring a hint. + return instances[0] else: - # Use the most recently active instance - # Instances with no heartbeat (None) should be sorted last (use 0 as sentinel) - sorted_instances = sorted( - instances, - key=lambda inst: inst.last_heartbeat.timestamp() if inst.last_heartbeat else 0.0, - reverse=True, + # 2+ instances connected and nothing pinned. Refuse to guess — + # silently routing to the most-recently-heartbeated editor lets + # an unbound session retarget another project's Unity (#1023). + # Mirror the HTTP "multiple connected, no active set" guard. + available_ids = [inst.id for inst in instances] + raise ConnectionError( + "Multiple Unity instances are connected and none is selected. " + "Pass unity_instance on the call or use set_active_instance " + f"with one of: {available_ids}. " + "Read mcpforunity://instances for current sessions." ) - logger.info( - f"No instance specified, using most recent: {sorted_instances[0].id}") - return sorted_instances[0] identifier = instance_identifier.strip() diff --git a/Server/src/transport/plugin_hub.py b/Server/src/transport/plugin_hub.py index 1a71f30a7..7753c931c 100644 --- a/Server/src/transport/plugin_hub.py +++ b/Server/src/transport/plugin_hub.py @@ -83,8 +83,16 @@ class InstanceSelectionRequiredError(RuntimeError): "Call set_active_instance with Name@hash from mcpforunity://instances." ) - def __init__(self, message: str | None = None): - super().__init__(message or self._SELECTION_REQUIRED) + def __init__(self, message: str | None = None, + available_instances: list[str] | None = None): + # Carried structurally so the transport layer can surface the ids without + # parsing the message; also appended to the text for parity with the stdio + # guard, which lists the ids inline. + self.available_instances = available_instances or [] + text = message or self._SELECTION_REQUIRED + if self.available_instances: + text = f"{text} Available instances: {self.available_instances}." + super().__init__(text) class PluginHub(WebSocketEndpoint): @@ -907,9 +915,19 @@ async def _try_once() -> tuple[str | None, int, bool]: # Multiple sessions but no explicit target is ambiguous return None, count, explicit_required + async def _available_instance_ids() -> list[str]: + # Error path only; one extra registry read keeps the refusal actionable. + try: + sessions = await cls._registry.list_sessions(user_id=user_id) + return sorted( + f"{s.project_name}@{s.project_hash}" for s in sessions.values()) + except Exception: + return [] + session_id, session_count, explicit_required = await _try_once() if session_id is None and explicit_required and not target_hash and session_count > 0: - raise InstanceSelectionRequiredError() + raise InstanceSelectionRequiredError( + available_instances=await _available_instance_ids()) deadline = time.monotonic() + max_wait_s wait_started = None @@ -918,9 +936,11 @@ async def _try_once() -> tuple[str | None, int, bool]: while session_id is None and time.monotonic() < deadline: if not target_hash and session_count > 1: raise InstanceSelectionRequiredError( - InstanceSelectionRequiredError._MULTIPLE_INSTANCES) + InstanceSelectionRequiredError._MULTIPLE_INSTANCES, + available_instances=await _available_instance_ids()) if session_id is None and explicit_required and not target_hash and session_count > 0: - raise InstanceSelectionRequiredError() + raise InstanceSelectionRequiredError( + available_instances=await _available_instance_ids()) if wait_started is None: wait_started = time.monotonic() logger.debug( diff --git a/Server/src/transport/unity_instance_middleware.py b/Server/src/transport/unity_instance_middleware.py index 9c367ef46..82ba47fd7 100644 --- a/Server/src/transport/unity_instance_middleware.py +++ b/Server/src/transport/unity_instance_middleware.py @@ -55,10 +55,11 @@ class UnityInstanceMiddleware(Middleware): for all tool and resource calls. """ + # Key used in FastMCP's session-scoped state store for the active instance. + _ACTIVE_INSTANCE_STATE_KEY = "mcpforunity.active_instance" + def __init__(self): super().__init__() - self._active_by_key: dict[str, str] = {} - self._lock = RLock() self._metadata_lock = RLock() self._unity_managed_tool_names: set[str] = set() self._tool_alias_to_unity_target: dict[str, str] = {} @@ -68,43 +69,30 @@ def __init__(self): self._tool_visibility_refresh_interval_seconds = 0.5 self._has_logged_empty_registry_warning = False - async def get_session_key(self, ctx) -> str: - """ - Derive a stable key for the calling session. + async def set_active_instance(self, ctx, instance_id: str) -> None: + """Store the active instance for this MCP session. - Prioritizes client_id for stability. - In remote-hosted mode, falls back to user_id for session isolation. - Otherwise falls back to 'global' (assuming single-user local mode). + Persisted via FastMCP's session-scoped state store, which keys by + ``ctx.session_id`` (the MCP-Session-Id header on HTTP, a per-subprocess + UUID on stdio). Two MCP sessions cannot share state — see #1023 for the + bug this replaces, which keyed on the peer-supplied ``client_id`` and + collapsed multiple clients onto the same record. """ - client_id = getattr(ctx, "client_id", None) - if isinstance(client_id, str) and client_id: - return client_id - - # In remote-hosted mode, use user_id so different users get isolated instance selections - user_id = await ctx.get_state("user_id") - if isinstance(user_id, str) and user_id: - return f"user:{user_id}" - - # Fallback to global for local dev stability - return "global" - - async def set_active_instance(self, ctx, instance_id: str) -> None: - """Store the active instance for this session.""" - key = await self.get_session_key(ctx) - with self._lock: - self._active_by_key[key] = instance_id + await ctx.set_state(self._ACTIVE_INSTANCE_STATE_KEY, instance_id) async def get_active_instance(self, ctx) -> str | None: - """Retrieve the active instance for this session.""" - key = await self.get_session_key(ctx) - with self._lock: - return self._active_by_key.get(key) + """Retrieve the active instance for this MCP session.""" + return await ctx.get_state(self._ACTIVE_INSTANCE_STATE_KEY) async def clear_active_instance(self, ctx) -> None: - """Clear the stored instance for this session.""" - key = await self.get_session_key(ctx) - with self._lock: - self._active_by_key.pop(key, None) + """Clear the stored instance for this MCP session. + + Overwrites with None rather than calling ``delete_state``: the read + path already treats None as "no active instance", and this keeps the + method usable from minimal context shims that don't implement + ``delete_state``. + """ + await ctx.set_state(self._ACTIVE_INSTANCE_STATE_KEY, None) async def _discover_instances(self, ctx) -> list: """ diff --git a/Server/src/transport/unity_transport.py b/Server/src/transport/unity_transport.py index f3b1d3d3f..9d07f4714 100644 --- a/Server/src/transport/unity_transport.py +++ b/Server/src/transport/unity_transport.py @@ -4,7 +4,7 @@ import logging from typing import Awaitable, Callable, TypeVar -from transport.plugin_hub import PluginHub +from transport.plugin_hub import InstanceSelectionRequiredError, PluginHub from core.config import config from core.constants import API_KEY_HEADER from services.api_key_service import ApiKeyService @@ -84,6 +84,20 @@ async def send_with_unity_instance( retry_on_reload=retry_on_reload, ) return normalize_unity_response(raw) + except InstanceSelectionRequiredError as exc: + # Not retryable: a blind retry fails identically. The client must pick + # an instance, so hint at selection and hand over the ids structurally. + return normalize_unity_response( + MCPResponse( + success=False, + error=str(exc), + hint="select_instance", + data={ + "reason": "instance_selection_required", + "available_instances": exc.available_instances, + }, + ).model_dump() + ) except Exception as exc: # NOTE: asyncio.TimeoutError has an empty str() by default, which is confusing for clients. err = str(exc) or f"{type(exc).__name__}" diff --git a/Server/tests/integration/test_instance_routing_comprehensive.py b/Server/tests/integration/test_instance_routing_comprehensive.py index 4a8ac3d68..ecfceb7ec 100644 --- a/Server/tests/integration/test_instance_routing_comprehensive.py +++ b/Server/tests/integration/test_instance_routing_comprehensive.py @@ -21,6 +21,22 @@ from transport.models import SessionList, SessionDetails +def _make_stateful_ctx(session_id: str) -> Mock: + """Build a Context shim whose set/get/delete_state share one dict. + + The middleware now defers persistence to FastMCP's session-scoped state + store, so a useful unit-test mock must actually round-trip values rather + than returning fresh Mocks. This helper does that minimally. + """ + state: dict[str, object] = {} + ctx = Mock(spec=Context) + ctx.session_id = session_id + ctx.set_state = AsyncMock(side_effect=lambda k, v: state.__setitem__(k, v)) + ctx.get_state = AsyncMock(side_effect=lambda k: state.get(k)) + ctx.delete_state = AsyncMock(side_effect=lambda k: state.pop(k, None)) + return ctx + + class TestInstanceRoutingBasics: """Test basic middleware functionality.""" @@ -28,96 +44,63 @@ class TestInstanceRoutingBasics: async def test_middleware_stores_and_retrieves_instance(self): """Middleware should store and retrieve instance per session.""" middleware = UnityInstanceMiddleware() - ctx = Mock(spec=Context) - ctx.session_id = "test-session-1" - ctx.client_id = "test-client-1" + ctx = _make_stateful_ctx("test-session-1") - # Set active instance await middleware.set_active_instance(ctx, "TestProject@abc123") - # Retrieve should return same instance assert await middleware.get_active_instance(ctx) == "TestProject@abc123" @pytest.mark.asyncio async def test_middleware_isolates_sessions(self): - """Different sessions should have independent instance selections.""" - middleware = UnityInstanceMiddleware() + """Two MCP sessions must not see each other's active instance. - ctx1 = Mock(spec=Context) - ctx1.session_id = "session-1" - ctx1.client_id = "client-1" + Regression test for #1023: the prior implementation keyed on the + peer-supplied client_id and collapsed multiple clients onto a shared + record. Each ctx in this test holds its own private state dict, so + leakage would surface as a cross-read. + """ + middleware = UnityInstanceMiddleware() - ctx2 = Mock(spec=Context) - ctx2.session_id = "session-2" - ctx2.client_id = "client-2" + ctx1 = _make_stateful_ctx("session-1") + ctx2 = _make_stateful_ctx("session-2") - # Set different instances for different sessions await middleware.set_active_instance(ctx1, "Project1@aaa") await middleware.set_active_instance(ctx2, "Project2@bbb") - # Each session should retrieve its own instance assert await middleware.get_active_instance(ctx1) == "Project1@aaa" assert await middleware.get_active_instance(ctx2) == "Project2@bbb" - @pytest.mark.asyncio - async def test_middleware_fallback_to_client_id(self): - """When session_id unavailable, should use client_id.""" - middleware = UnityInstanceMiddleware() - - ctx = Mock(spec=Context) - ctx.session_id = None - ctx.client_id = "client-123" - - await middleware.set_active_instance(ctx, "Project@xyz") - assert await middleware.get_active_instance(ctx) == "Project@xyz" - - @pytest.mark.asyncio - async def test_middleware_fallback_to_global(self): - """When no session/client id, should use 'global' key.""" - middleware = UnityInstanceMiddleware() - - ctx = Mock(spec=Context) - ctx.session_id = None - ctx.client_id = None - ctx.get_state = AsyncMock(return_value=None) - - await middleware.set_active_instance(ctx, "Project@global") - assert await middleware.get_active_instance(ctx) == "Project@global" - class TestInstanceRoutingIntegration: """Test that instance routing works end-to-end for all tool categories.""" @pytest.mark.asyncio async def test_middleware_injects_state_into_context(self): - """Middleware on_call_tool should inject instance into ctx state.""" + """Middleware on_call_tool should inject instance into ctx state. + + After this PR the middleware writes two distinct keys: a persistence + key (``mcpforunity.active_instance``) when ``set_active_instance`` is + called, and a per-request injection key (``unity_instance``) inside + ``on_call_tool``. Tools downstream read ``unity_instance``. + """ middleware = UnityInstanceMiddleware() - # Create mock context with state management - ctx = Mock(spec=Context) - ctx.session_id = "test-session" - state_storage = {} - ctx.set_state = AsyncMock(side_effect=lambda k, - v: state_storage.__setitem__(k, v)) - ctx.get_state = AsyncMock(side_effect=lambda k: state_storage.get(k)) + ctx = _make_stateful_ctx("test-session") - # Create middleware context middleware_ctx = Mock() middleware_ctx.fastmcp_context = ctx - # Set active instance await middleware.set_active_instance(ctx, "TestProject@abc123") - # Mock call_next async def mock_call_next(ctx): return {"success": True} - # Execute middleware await middleware.on_call_tool(middleware_ctx, mock_call_next) - # Verify state was injected - ctx.set_state.assert_called_once_with( - "unity_instance", "TestProject@abc123") + # The per-request injection must have happened so tools downstream + # can read it; the persistence write is verified by the round-trip + # test in TestInstanceRoutingBasics above. + ctx.set_state.assert_any_call("unity_instance", "TestProject@abc123") @pytest.mark.asyncio async def test_get_unity_instance_from_context_checks_state(self): diff --git a/Server/tests/integration/test_middleware_auth_integration.py b/Server/tests/integration/test_middleware_auth_integration.py index 95c1880d3..75213b4eb 100644 --- a/Server/tests/integration/test_middleware_auth_integration.py +++ b/Server/tests/integration/test_middleware_auth_integration.py @@ -66,38 +66,6 @@ async def test_sets_user_id_in_context_state(self, monkeypatch): assert await ctx.get_state("user_id") == "user-55" -class TestMiddlewareSessionKey: - @pytest.mark.asyncio - async def test_get_session_key_uses_user_id_fallback(self): - """When no client_id, middleware should use user:$user_id as session key.""" - from transport.unity_instance_middleware import UnityInstanceMiddleware - - middleware = UnityInstanceMiddleware() - - ctx = DummyContext() - # Simulate no client_id attribute - if hasattr(ctx, "client_id"): - delattr(ctx, "client_id") - await ctx.set_state("user_id", "user-77") - - key = await middleware.get_session_key(ctx) - assert key == "user:user-77" - - @pytest.mark.asyncio - async def test_get_session_key_prefers_client_id(self): - """client_id should take precedence over user_id.""" - from transport.unity_instance_middleware import UnityInstanceMiddleware - - middleware = UnityInstanceMiddleware() - - ctx = DummyContext() - ctx.client_id = "client-abc" - await ctx.set_state("user_id", "user-77") - - key = await middleware.get_session_key(ctx) - assert key == "client-abc" - - class TestAutoSelectDisabledRemoteHosted: @pytest.mark.asyncio async def test_auto_select_returns_none_in_remote_hosted(self, monkeypatch): diff --git a/Server/tests/test_stdio_instance_resolution.py b/Server/tests/test_stdio_instance_resolution.py new file mode 100644 index 000000000..99523ca72 --- /dev/null +++ b/Server/tests/test_stdio_instance_resolution.py @@ -0,0 +1,100 @@ +"""Stdio instance-resolution guard tests. + +Regression coverage for #1023: the stdio connection pool used to silently +route an unbound session (no unity_instance, no default) to the most-recently +heartbeated editor, letting one project's session retarget another. The pool +now refuses to guess when 2+ instances are connected, mirroring the HTTP +"multiple connected, no active set" guard. +""" + +from datetime import datetime, timedelta + +import pytest + +from models.models import UnityInstanceInfo +from transport.legacy.unity_connection import UnityConnectionPool + + +def _instance(name: str, port: int, heartbeat: datetime | None) -> UnityInstanceInfo: + return UnityInstanceInfo( + id=f"{name}@{name.lower()}hash", + name=name, + path=f"/path/{name}", + hash=f"{name.lower()}hash", + port=port, + status="running", + last_heartbeat=heartbeat, + ) + + +def test_resolve_none_with_single_instance_selects_it(): + """Sole connected instance is unambiguous and selected without a hint.""" + pool = UnityConnectionPool() + only = _instance("Solo", 6400, datetime.now()) + + resolved = pool._resolve_instance_id(None, [only]) + + assert resolved.id == only.id + + +def test_resolve_none_with_multiple_instances_raises(): + """2+ instances and nothing pinned must error instead of guessing (#1023).""" + pool = UnityConnectionPool() + now = datetime.now() + newer = _instance("Newer", 6400, now) + older = _instance("Older", 6401, now - timedelta(seconds=30)) + + with pytest.raises(ConnectionError, match="Multiple Unity instances"): + pool._resolve_instance_id(None, [newer, older]) + + +def test_resolve_none_does_not_fall_back_to_most_recent_heartbeat(): + """The guard must not leak the most-recently-heartbeated id into the error. + + Proves we no longer auto-pick sorted_instances[0]; both ids are offered as + explicit choices rather than one being silently returned. + """ + pool = UnityConnectionPool() + now = datetime.now() + newer = _instance("Newer", 6400, now) + older = _instance("Older", 6401, now - timedelta(seconds=30)) + + with pytest.raises(ConnectionError) as exc_info: + pool._resolve_instance_id(None, [newer, older]) + + message = str(exc_info.value) + assert newer.id in message + assert older.id in message + + +def test_resolve_none_with_default_instance_still_works(): + """An explicit default short-circuits the multi-instance guard.""" + pool = UnityConnectionPool() + pool._default_instance_id = "Newer@newerhash" + now = datetime.now() + newer = _instance("Newer", 6400, now) + older = _instance("Older", 6401, now - timedelta(seconds=30)) + + resolved = pool._resolve_instance_id(None, [newer, older]) + + assert resolved.id == "Newer@newerhash" + + +def test_resolve_explicit_identifier_unaffected_by_guard(): + """Explicit selection still resolves even with multiple instances.""" + pool = UnityConnectionPool() + now = datetime.now() + newer = _instance("Newer", 6400, now) + older = _instance("Older", 6401, now - timedelta(seconds=30)) + + resolved = pool._resolve_instance_id("Older@olderhash", [newer, older]) + + assert resolved.id == "Older@olderhash" + + +def test_resolve_no_instances_raises_distinct_error(): + """No instances at all still raises the "none found" error, not the guard.""" + pool = UnityConnectionPool() + + with pytest.raises(ConnectionError, match="No Unity Editor instances found"): + pool._resolve_instance_id(None, []) diff --git a/Server/tests/test_transport_characterization.py b/Server/tests/test_transport_characterization.py index 45e35e6a8..11fef31f9 100644 --- a/Server/tests/test_transport_characterization.py +++ b/Server/tests/test_transport_characterization.py @@ -102,15 +102,35 @@ async def configured_plugin_hub(plugin_registry): # SESSION MANAGEMENT & ROUTING TESTS # ============================================================================ +def _make_ctx(session_id: str | None = None) -> Mock: + """Build a minimal Context shim with FastMCP-compatible session state. + + Each ctx has its own private ``state`` dict, so isolation tests can prove + that two ctxs cannot read each other's writes — which is the FastMCP + invariant we now rely on (state keyed by ``ctx.session_id`` in production). + """ + state: dict[str, object] = {} + ctx = Mock() + ctx.session_id = session_id or "test-session" + ctx.set_state = AsyncMock(side_effect=lambda k, v: state.__setitem__(k, v)) + ctx.get_state = AsyncMock(side_effect=lambda k: state.get(k)) + ctx.delete_state = AsyncMock(side_effect=lambda k: state.pop(k, None)) + return ctx + + class TestUnityInstanceMiddlewareSessionManagement: - """Test instance routing and per-session state management.""" + """Test instance routing and per-session state management. + + The middleware now delegates persistence to FastMCP's session-scoped + state store (``ctx.set_state`` / ``ctx.get_state``), which is keyed by + ``ctx.session_id`` (the MCP-Session-Id header on HTTP, a per-subprocess + UUID on stdio). The tests below validate that contract from the + middleware's perspective. + """ @pytest.mark.asyncio async def test_middleware_stores_instance_per_session(self, mock_context): - """ - Current behavior: Middleware maintains independent instance selection - per session using get_session_key() derivation. - """ + """A single ctx round-trips set/get correctly via session state.""" middleware = UnityInstanceMiddleware() instance_id = "TestProject@abc123def456" @@ -120,52 +140,21 @@ async def test_middleware_stores_instance_per_session(self, mock_context): assert retrieved == instance_id, \ "Middleware must store and retrieve instance per session" - @pytest.mark.asyncio - async def test_middleware_uses_client_id_over_session_id(self): - """ - Current behavior: get_session_key() prioritizes client_id for stability, - falling back to 'global' when unavailable. - """ - middleware = UnityInstanceMiddleware() - - ctx = Mock() - ctx.client_id = "stable-client-id" - ctx.session_id = "unstable-session-id" - - key = await middleware.get_session_key(ctx) - assert key == "stable-client-id" - - @pytest.mark.asyncio - async def test_middleware_falls_back_to_global_key(self): - """ - Current behavior: When client_id is None/missing, use 'global' key. - This allows single-user local mode to work without session tracking. - """ - middleware = UnityInstanceMiddleware() - - ctx = Mock() - ctx.client_id = None - ctx.session_id = "session-id" - ctx.get_state = AsyncMock(return_value=None) - - key = await middleware.get_session_key(ctx) - assert key == "global" - @pytest.mark.asyncio async def test_middleware_isolates_multiple_sessions(self): """ - Current behavior: Different sessions (different client_ids) maintain - separate instance selections. + Two independent ctxs must not see each other's selection. + + This is the regression test for #1023: previously the middleware keyed + on the peer-supplied client_id and collapsed multiple clients onto the + same record. The new implementation defers to FastMCP session state, + which is isolated per ``ctx.session_id`` — modelled here as two ctxs + each holding their own private state dict. """ middleware = UnityInstanceMiddleware() - ctx1 = Mock() - ctx1.client_id = "client-1" - ctx1.session_id = "session-1" - - ctx2 = Mock() - ctx2.client_id = "client-2" - ctx2.session_id = "session-2" + ctx1 = _make_ctx("session-1") + ctx2 = _make_ctx("session-2") await middleware.set_active_instance(ctx1, "Project1@hash1") await middleware.set_active_instance(ctx2, "Project2@hash2") @@ -175,10 +164,7 @@ async def test_middleware_isolates_multiple_sessions(self): @pytest.mark.asyncio async def test_middleware_clear_instance(self, mock_context): - """ - Current behavior: clear_active_instance() removes stored instance - for the session, allowing reset to None. - """ + """clear_active_instance() resets the per-session selection to None.""" middleware = UnityInstanceMiddleware() instance_id = "TestProject@xyz" @@ -189,22 +175,14 @@ async def test_middleware_clear_instance(self, mock_context): assert await middleware.get_active_instance(mock_context) is None @pytest.mark.asyncio - async def test_middleware_thread_safe_updates(self): - """ - Current behavior: Middleware uses RLock to serialize access to - _active_by_key dictionary. - """ + async def test_middleware_repeated_updates_settle_to_latest(self): + """Sequential writes within one session leave the latest value in place.""" middleware = UnityInstanceMiddleware() - ctx = Mock() - ctx.client_id = "client-123" - ctx.session_id = "session-123" + ctx = _make_ctx("session-123") - # Rapidly update instances (would race without locking) for i in range(10): - instance = f"Project{i}@hash{i}" - await middleware.set_active_instance(ctx, instance) + await middleware.set_active_instance(ctx, f"Project{i}@hash{i}") - # Final state should be consistent assert await middleware.get_active_instance(ctx) == "Project9@hash9" @@ -1232,6 +1210,71 @@ async def test_resolve_session_id_rejects_ambiguous_selection(self, plugin_regis PluginHub._lock = None PluginHub._loop = None + @pytest.mark.asyncio + async def test_resolve_session_id_ambiguity_lists_available_instances(self, plugin_registry): + """The refusal carries the instance ids (parity with the stdio guard) so + agents can select without a second lookup.""" + loop = asyncio.get_event_loop() + PluginHub.configure(plugin_registry, loop) + + await plugin_registry.register( + session_id="sess-1", + project_name="Project1", + project_hash="hash-1", + unity_version="2022.3" + ) + await plugin_registry.register( + session_id="sess-2", + project_name="Project2", + project_hash="hash-2", + unity_version="2023.2" + ) + + with pytest.raises(InstanceSelectionRequiredError) as excinfo: + await PluginHub._resolve_session_id(None) + + assert excinfo.value.available_instances == [ + "Project1@hash-1", "Project2@hash-2"] + assert "Project1@hash-1" in str(excinfo.value) + assert "Project2@hash-2" in str(excinfo.value) + + # Cleanup + PluginHub._registry = None + PluginHub._lock = None + PluginHub._loop = None + + @pytest.mark.asyncio + async def test_http_selection_error_hints_selection_not_retry(self, monkeypatch): + """A blind retry fails identically, so the HTTP wrapper must hint at + selection and surface the ids structurally instead of the blanket + retry hint.""" + from transport import unity_transport + + async def _no_user(): + return None + + async def _raise_selection(*_args, **_kwargs): + raise InstanceSelectionRequiredError( + InstanceSelectionRequiredError._MULTIPLE_INSTANCES, + available_instances=["A@hash-a", "B@hash-b"]) + + monkeypatch.setattr(unity_transport, "_is_http_transport", lambda: True) + monkeypatch.setattr( + unity_transport, "_resolve_user_id_from_request", _no_user) + monkeypatch.setattr( + unity_transport.PluginHub, "send_command_for_instance", _raise_selection) + + async def _send_fn(*_a, **_k): + raise AssertionError("stdio path should not be used on HTTP transport") + + resp = await unity_transport.send_with_unity_instance( + _send_fn, None, "manage_scene", {}) + + assert resp["success"] is False + assert resp["hint"] == "select_instance" + assert resp["data"]["reason"] == "instance_selection_required" + assert resp["data"]["available_instances"] == ["A@hash-a", "B@hash-b"] + @pytest.mark.asyncio async def test_resolve_session_id_parses_instance_format(self, plugin_registry): """ @@ -1459,22 +1502,6 @@ async def test_middleware_handles_exception_during_autoselect(self, mock_context assert instance is None - @pytest.mark.asyncio - async def test_middleware_handles_client_id_false_but_not_none(self): - """ - Current behavior: get_session_key checks isinstance(client_id, str) AND len, - so falsy non-string values fall through to 'global'. - """ - middleware = UnityInstanceMiddleware() - - ctx = Mock() - ctx.client_id = "" # Empty string - ctx.session_id = "session-id" - ctx.get_state = AsyncMock(return_value=None) - - key = await middleware.get_session_key(ctx) - assert key == "global" # Empty string doesn't pass isinstance+truthy check - def test_plugin_hub_encoding_is_json(self): """ Current behavior: PluginHub WebSocketEndpoint uses JSON encoding. diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport.meta new file mode 100644 index 000000000..bbe5f7ea1 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport.meta @@ -0,0 +1,8 @@ +fileFormatVersion: 2 +guid: 4963343217cdb4937835086fabddc98a +folderAsset: yes +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport/StdioTransportClientReadinessTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport/StdioTransportClientReadinessTests.cs new file mode 100644 index 000000000..b436667d2 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport/StdioTransportClientReadinessTests.cs @@ -0,0 +1,44 @@ +using NUnit.Framework; +using MCPForUnity.Editor.Services.Transport.Transports; + +namespace MCPForUnityTests.Editor.Transport +{ + /// + /// Regression tests for the Start Session readiness race: stdio StartAsync used to return true + /// unconditionally and callers immediately verified StdioBridgeHost.IsRunning — which is still + /// false while the previous port releases after a domain reload, producing a spurious + /// "Bridge not running" until the user clicked Start Session several times. StartAsync now waits + /// (bounded) for the bridge to actually bind; this covers the wait predicate. + /// + public class StdioTransportClientReadinessTests + { + private static readonly double Timeout = StdioTransportClient.ReadyWaitTimeoutSeconds; + + [Test] + public void KeepsWaiting_WhenNotReady_AndWithinWindow() + { + Assert.IsTrue(StdioTransportClient.ShouldKeepWaitingForReady(bridgeReady: false, secondsWaited: 0.0)); + Assert.IsTrue(StdioTransportClient.ShouldKeepWaitingForReady(bridgeReady: false, secondsWaited: Timeout - 0.1)); + } + + [Test] + public void StopsWaiting_AsSoonAsReady() + { + Assert.IsFalse(StdioTransportClient.ShouldKeepWaitingForReady(bridgeReady: true, secondsWaited: 0.0)); + } + + [Test] + public void StopsWaiting_WhenWindowElapses_EvenIfNotReady() + { + Assert.IsFalse(StdioTransportClient.ShouldKeepWaitingForReady(bridgeReady: false, secondsWaited: Timeout)); + Assert.IsFalse(StdioTransportClient.ShouldKeepWaitingForReady(bridgeReady: false, secondsWaited: Timeout + 1.0)); + } + + [Test] + public void ReadyWindow_IsGenerousEnoughForPortRelease() + { + // Must cover the OS port-release delay + the BusyPortFallbackWindowSeconds (3s) fallback. + Assert.GreaterOrEqual(Timeout, 3.0); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport/StdioTransportClientReadinessTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport/StdioTransportClientReadinessTests.cs.meta new file mode 100644 index 000000000..dbc3a6a17 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Transport/StdioTransportClientReadinessTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 322603f43226040548e602c8bb6cc204 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionHealthDebounceTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionHealthDebounceTests.cs new file mode 100644 index 000000000..9702feefb --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionHealthDebounceTests.cs @@ -0,0 +1,49 @@ +using NUnit.Framework; +using MCPForUnity.Editor.Windows.Components.Connection; + +namespace MCPForUnityTests.Editor.Windows +{ + /// + /// Regression tests for the stdio health-verification debounce: a domain reload briefly + /// rebinds the bridge listener (the port can hop, e.g. 6402 -> 6403), during which a single + /// VerifyAsync() ping transiently fails with "Bridge not running" even though the bridge + /// recovers on its own. The health indicator must not flash "broken" on that lone miss — + /// mirroring the #1207 orphaned-session debounce. + /// + public class McpConnectionSectionHealthDebounceTests + { + private static readonly int Threshold = McpConnectionSection.UnhealthyVerificationThreshold; + + [Test] + public void SingleVerifyMiss_DoesNotReportUnhealthy() + { + // The lone transient miss during a reload rebind must be tolerated. + Assert.IsFalse(McpConnectionSection.ShouldReportUnhealthy(1)); + } + + [Test] + public void BelowThreshold_DoesNotReportUnhealthy() + { + Assert.IsFalse(McpConnectionSection.ShouldReportUnhealthy(Threshold - 1)); + } + + [Test] + public void AtThreshold_ReportsUnhealthy() + { + Assert.IsTrue(McpConnectionSection.ShouldReportUnhealthy(Threshold)); + } + + [Test] + public void PastThreshold_ReportsUnhealthy() + { + Assert.IsTrue(McpConnectionSection.ShouldReportUnhealthy(Threshold + 3)); + } + + [Test] + public void Threshold_RequiresMoreThanOneFailure() + { + // The whole point of the debounce: a single miss must never be enough. + Assert.GreaterOrEqual(Threshold, 2); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionHealthDebounceTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionHealthDebounceTests.cs.meta new file mode 100644 index 000000000..ab1305c20 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionHealthDebounceTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b1a4438b3752b44c49e980187025c2bf +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: