Skip to content

Commit 328ff49

Browse files
sakurachanclaude
andcommitted
Address CodeRabbit feedback: refactor connection pool and fix critical issues
- Fix timezone-aware vs naive datetime comparison in port discovery and instance sorting - Update type annotations to modern Python style (Optional[X] -> X | None, Dict -> dict, List -> list) - Extract UnityConnectionPool to separate connection_pool.py module - Convert list_unity_instances from tool to resource (mcpforunity://unity-instances) - Fix server.py imports after refactoring 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 9d5b9c6 commit 328ff49

File tree

10 files changed

+540
-535
lines changed

10 files changed

+540
-535
lines changed
Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
"""
2+
Connection pool for managing multiple Unity Editor instances.
3+
"""
4+
import logging
5+
import os
6+
import threading
7+
import time
8+
9+
from models import UnityInstanceInfo
10+
from port_discovery import PortDiscovery
11+
12+
logger = logging.getLogger(__name__)
13+
14+
15+
class UnityConnectionPool:
16+
"""Manages connections to multiple Unity Editor instances"""
17+
18+
def __init__(self):
19+
# Import here to avoid circular dependency
20+
from unity_connection import UnityConnection
21+
self._UnityConnection = UnityConnection
22+
23+
self._connections: dict[str, "UnityConnection"] = {}
24+
self._known_instances: dict[str, UnityInstanceInfo] = {}
25+
self._last_full_scan: float = 0
26+
self._scan_interval: float = 5.0 # Cache for 5 seconds
27+
self._pool_lock = threading.Lock()
28+
self._default_instance_id: str | None = None
29+
30+
# Check for default instance from environment
31+
env_default = os.environ.get("UNITY_MCP_DEFAULT_INSTANCE", "").strip()
32+
if env_default:
33+
self._default_instance_id = env_default
34+
logger.info(f"Default Unity instance set from environment: {env_default}")
35+
36+
def discover_all_instances(self, force_refresh: bool = False) -> list[UnityInstanceInfo]:
37+
"""
38+
Discover all running Unity Editor instances.
39+
40+
Args:
41+
force_refresh: If True, bypass cache and scan immediately
42+
43+
Returns:
44+
List of UnityInstanceInfo objects
45+
"""
46+
now = time.time()
47+
48+
# Return cached results if valid
49+
if not force_refresh and (now - self._last_full_scan) < self._scan_interval:
50+
logger.debug(f"Returning cached Unity instances (age: {now - self._last_full_scan:.1f}s)")
51+
return list(self._known_instances.values())
52+
53+
# Scan for instances
54+
logger.debug("Scanning for Unity instances...")
55+
instances = PortDiscovery.discover_all_unity_instances()
56+
57+
# Update cache
58+
with self._pool_lock:
59+
self._known_instances = {inst.id: inst for inst in instances}
60+
self._last_full_scan = now
61+
62+
logger.info(f"Found {len(instances)} Unity instances: {[inst.id for inst in instances]}")
63+
return instances
64+
65+
def _resolve_instance_id(self, instance_identifier: str | None, instances: list[UnityInstanceInfo]) -> UnityInstanceInfo:
66+
"""
67+
Resolve an instance identifier to a specific Unity instance.
68+
69+
Args:
70+
instance_identifier: User-provided identifier (name, hash, name@hash, path, port, or None)
71+
instances: List of available instances
72+
73+
Returns:
74+
Resolved UnityInstanceInfo
75+
76+
Raises:
77+
ConnectionError: If instance cannot be resolved
78+
"""
79+
if not instances:
80+
raise ConnectionError(
81+
"No Unity Editor instances found. Please ensure Unity is running with MCP for Unity bridge."
82+
)
83+
84+
# Use default instance if no identifier provided
85+
if instance_identifier is None:
86+
if self._default_instance_id:
87+
instance_identifier = self._default_instance_id
88+
logger.debug(f"Using default instance: {instance_identifier}")
89+
else:
90+
# Use the most recently active instance
91+
# Instances with no heartbeat (None) should be sorted last (use 0.0 as sentinel)
92+
sorted_instances = sorted(
93+
instances,
94+
key=lambda inst: inst.last_heartbeat.timestamp() if inst.last_heartbeat else 0.0,
95+
reverse=True,
96+
)
97+
logger.info(f"No instance specified, using most recent: {sorted_instances[0].id}")
98+
return sorted_instances[0]
99+
100+
identifier = instance_identifier.strip()
101+
102+
# Try exact ID match first
103+
for inst in instances:
104+
if inst.id == identifier:
105+
return inst
106+
107+
# Try project name match
108+
name_matches = [inst for inst in instances if inst.name == identifier]
109+
if len(name_matches) == 1:
110+
return name_matches[0]
111+
elif len(name_matches) > 1:
112+
# Multiple projects with same name - return helpful error
113+
suggestions = [
114+
{
115+
"id": inst.id,
116+
"path": inst.path,
117+
"port": inst.port,
118+
"suggest": f"Use unity_instance='{inst.id}'"
119+
}
120+
for inst in name_matches
121+
]
122+
raise ConnectionError(
123+
f"Project name '{identifier}' matches {len(name_matches)} instances. "
124+
f"Please use the full format (e.g., '{name_matches[0].id}'). "
125+
f"Available instances: {suggestions}"
126+
)
127+
128+
# Try hash match
129+
hash_matches = [inst for inst in instances if inst.hash == identifier or inst.hash.startswith(identifier)]
130+
if len(hash_matches) == 1:
131+
return hash_matches[0]
132+
elif len(hash_matches) > 1:
133+
raise ConnectionError(
134+
f"Hash '{identifier}' matches multiple instances: {[inst.id for inst in hash_matches]}"
135+
)
136+
137+
# Try composite format: Name@Hash or Name@Port
138+
if "@" in identifier:
139+
name_part, hint_part = identifier.split("@", 1)
140+
composite_matches = [
141+
inst for inst in instances
142+
if inst.name == name_part and (
143+
inst.hash.startswith(hint_part) or str(inst.port) == hint_part
144+
)
145+
]
146+
if len(composite_matches) == 1:
147+
return composite_matches[0]
148+
149+
# Try port match (as string)
150+
try:
151+
port_num = int(identifier)
152+
port_matches = [inst for inst in instances if inst.port == port_num]
153+
if len(port_matches) == 1:
154+
return port_matches[0]
155+
except ValueError:
156+
pass
157+
158+
# Try path match
159+
path_matches = [inst for inst in instances if inst.path == identifier]
160+
if len(path_matches) == 1:
161+
return path_matches[0]
162+
163+
# Nothing matched
164+
available_ids = [inst.id for inst in instances]
165+
raise ConnectionError(
166+
f"Unity instance '{identifier}' not found. "
167+
f"Available instances: {available_ids}. "
168+
f"Use the unity_instances resource to see all instances."
169+
)
170+
171+
def get_connection(self, instance_identifier: str | None = None):
172+
"""
173+
Get or create a connection to a Unity instance.
174+
175+
Args:
176+
instance_identifier: Optional identifier (name, hash, name@hash, etc.)
177+
If None, uses default or most recent instance
178+
179+
Returns:
180+
UnityConnection to the specified instance
181+
182+
Raises:
183+
ConnectionError: If instance cannot be found or connected
184+
"""
185+
# Refresh instance list if cache expired
186+
instances = self.discover_all_instances()
187+
188+
# Resolve identifier to specific instance
189+
target = self._resolve_instance_id(instance_identifier, instances)
190+
191+
# Return existing connection or create new one
192+
with self._pool_lock:
193+
if target.id not in self._connections:
194+
logger.info(f"Creating new connection to Unity instance: {target.id} (port {target.port})")
195+
conn = self._UnityConnection(port=target.port, instance_id=target.id)
196+
if not conn.connect():
197+
raise ConnectionError(
198+
f"Failed to connect to Unity instance '{target.id}' on port {target.port}. "
199+
f"Ensure the Unity Editor is running."
200+
)
201+
self._connections[target.id] = conn
202+
else:
203+
# Update existing connection with instance_id and port if changed
204+
conn = self._connections[target.id]
205+
conn.instance_id = target.id
206+
if conn.port != target.port:
207+
logger.info(f"Updating cached port for {target.id}: {conn.port} -> {target.port}")
208+
conn.port = target.port
209+
logger.debug(f"Reusing existing connection to: {target.id}")
210+
211+
return self._connections[target.id]
212+
213+
def disconnect_all(self):
214+
"""Disconnect all active connections"""
215+
with self._pool_lock:
216+
for instance_id, conn in self._connections.items():
217+
try:
218+
logger.info(f"Disconnecting from Unity instance: {instance_id}")
219+
conn.disconnect()
220+
except Exception:
221+
logger.exception(f"Error disconnecting from {instance_id}")
222+
self._connections.clear()
223+
224+
225+
# Global Unity connection pool
226+
_unity_connection_pool: UnityConnectionPool | None = None
227+
_pool_init_lock = threading.Lock()
228+
229+
230+
def get_unity_connection_pool() -> UnityConnectionPool:
231+
"""Get or create the global Unity connection pool."""
232+
global _unity_connection_pool
233+
if _unity_connection_pool is None:
234+
with _pool_init_lock:
235+
if _unity_connection_pool is None:
236+
_unity_connection_pool = UnityConnectionPool()
237+
return _unity_connection_pool

MCPForUnity/UnityMcpServer~/src/port_discovery.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ def discover_all_unity_instances() -> List[UnityInstanceInfo]:
228228
Returns:
229229
List of UnityInstanceInfo objects for all discovered instances
230230
"""
231-
instances_by_port: Dict[int, tuple[UnityInstanceInfo, datetime]] = {}
231+
instances_by_port: Dict[int, tuple[UnityInstanceInfo, float]] = {}
232232
base = PortDiscovery.get_registry_dir()
233233

234234
# Scan all status files
@@ -238,7 +238,7 @@ def discover_all_unity_instances() -> List[UnityInstanceInfo]:
238238
for status_file_path in status_files:
239239
try:
240240
status_path = Path(status_file_path)
241-
file_mtime = datetime.fromtimestamp(status_path.stat().st_mtime)
241+
file_mtime = status_path.stat().st_mtime
242242

243243
with status_path.open('r') as f:
244244
data = json.load(f)
@@ -269,12 +269,12 @@ def discover_all_unity_instances() -> List[UnityInstanceInfo]:
269269
logger.debug(f"Instance {project_name}@{hash_value} has heartbeat but port {port} not responding")
270270
continue
271271

272-
freshness = last_heartbeat or file_mtime
272+
freshness = last_heartbeat.timestamp() if last_heartbeat else file_mtime
273273

274274
existing = instances_by_port.get(port)
275275
if existing:
276-
_, existing_time = existing
277-
if existing_time >= freshness:
276+
_, existing_freshness = existing
277+
if existing_freshness >= freshness:
278278
logger.debug(
279279
"Skipping stale status entry %s in favor of more recent data for port %s",
280280
status_path.name,

Server/tools/list_unity_instances.py renamed to MCPForUnity/UnityMcpServer~/src/resources/unity_instances.py

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
"""
2-
Tool to list all available Unity Editor instances.
2+
Resource for listing all available Unity Editor instances.
33
"""
4-
from typing import Annotated, Any
4+
from typing import Any
55

6-
from fastmcp import Context
7-
from registry import mcp_for_unity_tool
6+
from registry import mcp_for_unity_resource
87
from unity_connection import get_unity_connection_pool
98

109

11-
@mcp_for_unity_tool(description="List all running Unity Editor instances with their details.")
12-
def list_unity_instances(
13-
ctx: Context,
14-
force_refresh: Annotated[bool, "Force refresh the instance list, bypassing cache"] = False
15-
) -> dict[str, Any]:
10+
@mcp_for_unity_resource(
11+
uri="mcpforunity://unity-instances",
12+
name="unity_instances",
13+
description="Provides a list of all running Unity Editor instances with their details."
14+
)
15+
def get_unity_instances() -> dict[str, Any]:
1616
"""
1717
List all available Unity Editor instances.
1818
@@ -26,17 +26,12 @@ def list_unity_instances(
2626
- last_heartbeat: Last heartbeat timestamp
2727
- unity_version: Unity version (if available)
2828
29-
Args:
30-
force_refresh: If True, bypass cache and scan immediately
31-
3229
Returns:
3330
Dictionary containing list of instances and metadata
3431
"""
35-
ctx.info(f"Listing Unity instances (force_refresh={force_refresh})")
36-
3732
try:
3833
pool = get_unity_connection_pool()
39-
instances = pool.discover_all_instances(force_refresh=force_refresh)
34+
instances = pool.discover_all_instances(force_refresh=True)
4035

4136
# Check for duplicate project names
4237
name_counts = {}
@@ -60,7 +55,6 @@ def list_unity_instances(
6055
return result
6156

6257
except Exception as e:
63-
ctx.error(f"Error listing Unity instances: {e}")
6458
return {
6559
"success": False,
6660
"error": f"Failed to list Unity instances: {str(e)}",

MCPForUnity/UnityMcpServer~/src/server.py

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from config import config
1010
from tools import register_all_tools
1111
from resources import register_all_resources
12-
from unity_connection import get_unity_connection_pool, UnityConnectionPool
12+
from connection_pool import get_unity_connection_pool
1313
import time
1414

1515
# Configure logging using settings from config
@@ -62,16 +62,14 @@
6262
except Exception:
6363
pass
6464

65-
# Global connection pool
66-
_unity_connection_pool: UnityConnectionPool = None
67-
68-
6965
@asynccontextmanager
7066
async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]:
7167
"""Handle server startup and shutdown."""
72-
global _unity_connection_pool
7368
logger.info("MCP for Unity Server starting up")
7469

70+
# Initialize connection pool variable
71+
pool = None
72+
7573
# Record server startup telemetry
7674
start_time = time.time()
7775
start_clk = time.perf_counter()
@@ -103,15 +101,15 @@ def _emit_startup():
103101
"Skipping Unity connection on startup (UNITY_MCP_SKIP_STARTUP_CONNECT=1)")
104102
else:
105103
# Initialize connection pool and discover instances
106-
_unity_connection_pool = get_unity_connection_pool()
107-
instances = _unity_connection_pool.discover_all_instances()
104+
pool = get_unity_connection_pool()
105+
instances = pool.discover_all_instances()
108106

109107
if instances:
110108
logger.info(f"Discovered {len(instances)} Unity instance(s): {[i.id for i in instances]}")
111109

112110
# Try to connect to default instance
113111
try:
114-
_unity_connection_pool.get_connection()
112+
pool.get_connection()
115113
logger.info("Connected to default Unity instance on startup")
116114

117115
# Record successful Unity connection (deferred)
@@ -160,10 +158,10 @@ def _emit_startup():
160158
try:
161159
# Yield the connection pool so it can be attached to the context
162160
# Note: Tools will use get_unity_connection_pool() directly
163-
yield {"pool": _unity_connection_pool}
161+
yield {"pool": pool}
164162
finally:
165-
if _unity_connection_pool:
166-
_unity_connection_pool.disconnect_all()
163+
if pool:
164+
pool.disconnect_all()
167165
logger.info("MCP for Unity Server shut down")
168166

169167
# Initialize MCP server

0 commit comments

Comments
 (0)