Fix stdio bridge stalls when Unity is backgrounded during domain reload#787
Merged
dsarno merged 5 commits intoCoplayDev:betafrom Feb 20, 2026
Merged
Conversation
After domain reload, the bridge could accept TCP connections but never process commands. Three defensive fixes address the plausible failure vectors: 1. Stale client cleanup: when a new client connects, close all other active clients (in stdio there is only one server). Forces hung ReadFrameAsUtf8Async to throw and exit cleanly. 2. ProcessCommands self-healing: track consecutive TCS timeouts. After 2+ consecutive, force re-register ProcessCommands on EditorApplication.update and reset the reentrancy guard. 3. Stale command eviction: commands stuck with IsExecuting=true for more than 60s are evicted from the queue with an error result. Also adds always-on diagnostic logging at client connect/disconnect with active client counts. Includes E2E reconnection tests that verify both abrupt disconnect recovery and stale client cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add QueuePlayerLoopUpdate after command enqueue so ProcessCommands fires even when Unity is backgrounded (mirrors HTTP RequestMainThreadPump) - Keep ProcessCommands permanently registered on EditorApplication.update to eliminate the registration gap between Stop and Start during reload - Replace blocking retry loop in Start() (Thread.Sleep x 10) with single attempt + port-switch fallback; async retries handled by reload handler - Replace fire-and-forget TryStartBridgeImmediate with async retry loop (6 attempts with backoff: 0s, 1s, 3s, 5s, 10s, 30s) matching HTTP - Fix macOS port conflict: use ExclusiveAddressUse in both the real listener (CreateConfiguredListener) and the port probe (IsPortAvailable) to prevent AssetImportWorkers from binding the same port via SO_REUSEADDR - Add PortManager unit tests (5 tests including macOS SO_REUSEADDR case) Fixes CoplayDev#785 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a prominent 'Do NOT Guess' section with a URI lookup table near the top of the skill, so LLMs use exact URIs instead of fabricating similar-looking ones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors the stdio transport startup/reload and port management to avoid stalls and macOS port conflicts, adds robust reconnect/retry behavior and stale-command cleanup, and documents resource URIs for the Unity MCP skill, backed by new edit-mode tests. Sequence diagram for stdio bridge resume with retry after domain reloadsequenceDiagram
actor User
participant UnityEditor
participant AssemblyReloadEvents
participant StdioBridgeReloadHandler
participant EditorConfigurationCache
participant MCPServiceLocator
participant TransportManager
participant StdioBridgeHost
User->>UnityEditor: Trigger code change causing domain reload
UnityEditor->>AssemblyReloadEvents: beforeAssemblyReload
AssemblyReloadEvents->>StdioBridgeReloadHandler: OnBeforeAssemblyReload()
StdioBridgeReloadHandler->>UnityEditor: Set ResumeStdioAfterReload flag
StdioBridgeReloadHandler->>StdioBridgeHost: Stop()
UnityEditor->>AssemblyReloadEvents: afterAssemblyReload
AssemblyReloadEvents->>StdioBridgeReloadHandler: OnAfterAssemblyReload()
StdioBridgeReloadHandler->>UnityEditor: Read ResumeStdioAfterReload flag
alt flag not set
StdioBridgeReloadHandler-->>AssemblyReloadEvents: Return (no resume)
else flag set
StdioBridgeReloadHandler->>UnityEditor: Check EditorApplication.isCompiling
alt not compiling
StdioBridgeReloadHandler->>StdioBridgeReloadHandler: ResumeStdioWithRetriesAsync()
else compiling
StdioBridgeReloadHandler->>UnityEditor: EditorApplication.delayCall(ResumeStdioWithRetriesAsync)
end
loop attempts over ResumeRetrySchedule
StdioBridgeReloadHandler->>EditorConfigurationCache: Read Instance.UseHttpTransport
alt UseHttpTransport is true
StdioBridgeReloadHandler->>UnityEditor: Delete ResumeStdioAfterReload flag
StdioBridgeReloadHandler-->>UnityEditor: Abort retries
StdioBridgeReloadHandler-->>StdioBridgeReloadHandler: Break loop
else
StdioBridgeReloadHandler->>StdioBridgeReloadHandler: Wait delay for this attempt
StdioBridgeReloadHandler->>MCPServiceLocator: TransportManager
MCPServiceLocator->>TransportManager: StartAsync(TransportMode.Stdio)
TransportManager->>StdioBridgeHost: Start()
alt listener binds successfully
StdioBridgeHost-->>TransportManager: started = true
TransportManager-->>StdioBridgeReloadHandler: true
StdioBridgeReloadHandler->>UnityEditor: Delete ResumeStdioAfterReload flag
StdioBridgeReloadHandler->>UnityEditor: MCPForUnityEditorWindow.RequestHealthVerification()
StdioBridgeReloadHandler-->>UnityEditor: Resume succeeded
StdioBridgeReloadHandler-->>StdioBridgeReloadHandler: Break loop
else listener fails (port busy or error)
StdioBridgeHost-->>TransportManager: started = false or exception
TransportManager-->>StdioBridgeReloadHandler: false or exception
StdioBridgeReloadHandler->>TransportManager: GetState(TransportMode.Stdio)
StdioBridgeReloadHandler-->>StdioBridgeReloadHandler: Log failure and continue loop
end
end
end
alt all attempts exhausted and not started
StdioBridgeReloadHandler->>UnityEditor: Delete ResumeStdioAfterReload flag
StdioBridgeReloadHandler-->>UnityEditor: Log final failure
end
end
Class diagram for updated stdio bridge, reload handler, and port managerclassDiagram
class StdioBridgeHost {
<<static>>
+bool isRunning
+bool isAutoConnectMode
+ulong MaxFrameBytes
+int FrameIOTimeoutMs
+Stopwatch _uptime
+int _consecutiveTimeouts
+bool _processCommandsHooked
+void Start()
+void Stop()
+TcpListener CreateConfiguredListener(int port)
+Task ListenerLoopAsync(CancellationToken token)
+Task HandleClientAsync(TcpClient client, CancellationToken token)
+void ProcessCommands()
}
class QueuedCommand {
+string CommandJson
+TaskCompletionSource~string~ Tcs
+bool IsExecuting
+long EnqueuedAtMs
}
class PortManager {
<<static>>
+bool IsPortAvailable(int port)
+int DiscoverNewPort()
}
class StdioBridgeReloadHandler {
<<static>>
+TimeSpan[] ResumeRetrySchedule
+void OnBeforeAssemblyReload()
+void OnAfterAssemblyReload()
+Task ResumeStdioWithRetriesAsync()
}
class MCPServiceLocator {
<<static>>
+TransportManager TransportManager
}
class TransportManager {
+Task~bool~ StartAsync(TransportMode mode)
+TransportState GetState(TransportMode mode)
}
class EditorConfigurationCache {
<<singleton>>
+EditorConfigurationCache Instance
+bool UseHttpTransport
}
StdioBridgeHost --> QueuedCommand : uses
StdioBridgeHost --> PortManager : uses for ports
StdioBridgeHost --> MCPServiceLocator : uses to integrate transport
StdioBridgeReloadHandler --> MCPServiceLocator : restarts stdio bridge
StdioBridgeReloadHandler --> EditorConfigurationCache : checks transport mode
StdioBridgeReloadHandler --> StdioBridgeHost : indirectly via TransportManager
PortManager ..> TcpListener : uses for IsPortAvailable
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
HandleClientAsync, you take two separate locks onclientsLockjust to compute the active client count; consider collapsing these into the existing add/remove critical sections to avoid extra locking and reduce the chance of transiently inconsistent counts in logs. - The retry loop in
ResumeStdioWithRetriesAsyncusesTask.Delaywithout any cancellation, so the background task can keep running across editor quit or subsequent reloads; consider threading a cancellation token from assembly reload/quitting events into this method so retries can be aborted cleanly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `HandleClientAsync`, you take two separate locks on `clientsLock` just to compute the active client count; consider collapsing these into the existing add/remove critical sections to avoid extra locking and reduce the chance of transiently inconsistent counts in logs.
- The retry loop in `ResumeStdioWithRetriesAsync` uses `Task.Delay` without any cancellation, so the background task can keep running across editor quit or subsequent reloads; consider threading a cancellation token from assembly reload/quitting events into this method so retries can be aborted cleanly.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs:401` </location>
<code_context>
using (NetworkStream stream = client.GetStream())
{
lock (clientsLock) { activeClients.Add(client); }
+ int clientCount;
+ lock (clientsLock) { clientCount = activeClients.Count; }
try
{
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid locking `clientsLock` twice for add + count; both operations can be done under a single lock.
In `HandleClientAsync`, `activeClients.Add(client)` and `clientCount = activeClients.Count` are guarded by two separate `lock (clientsLock)` blocks. This allows another thread to modify `activeClients` between these operations and adds extra locking overhead. Wrap both statements in a single `lock (clientsLock)` and compute `clientCount` immediately after the add.
```suggestion
try
using (NetworkStream stream = client.GetStream())
{
int clientCount;
lock (clientsLock)
{
activeClients.Add(client);
clientCount = activeClients.Count;
}
try
{
```
</issue_to_address>
### Comment 2
<location> `unity-mcp-skill/SKILL.md:26` </location>
<code_context>
+|----------|-----|
+| Editor state | `mcpforunity://editor/state` |
+| Project info | `mcpforunity://project/info` |
+| Scene gameobject API | `mcpforunity://scene/gameobject-api` |
+| Tags | `mcpforunity://project/tags` |
+| Layers | `mcpforunity://project/layers` |
</code_context>
<issue_to_address>
**suggestion (typo):** Consider using Unity’s canonical spelling "GameObject" in the label.
To stay consistent with Unity terminology and the rest of the docs, please change the label to "Scene GameObject API" while keeping the URI as-is.
```suggestion
| Scene GameObject API | `mcpforunity://scene/gameobject-api` |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 4
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
466-468: Two consecutive lock acquisitions can be combinedLines 467–468 acquire
clientsLocktwice in a row (once to add, once to read count). Same pattern at lines 640–642 (remove + read count). Each pair can be merged into a single lock scope.♻️ Proposed refactor
- lock (clientsLock) { activeClients.Add(client); } - int clientCount; - lock (clientsLock) { clientCount = activeClients.Count; } + int clientCount; + lock (clientsLock) { activeClients.Add(client); clientCount = activeClients.Count; }- lock (clientsLock) { activeClients.Remove(client); } - int remaining; - lock (clientsLock) { remaining = activeClients.Count; } + int remaining; + lock (clientsLock) { activeClients.Remove(client); remaining = activeClients.Count; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs` around lines 466 - 468, The code currently acquires clientsLock twice in a row—once to modify activeClients and again to read its Count—so merge each pair into a single lock scope: inside the same lock(clientsLock) block perform the mutation (Add or Remove) and then immediately read activeClients.Count into clientCount; update both occurrences (the add at methods referencing activeClients.Add and the remove at activeClients.Remove) to use a single lock to reduce contention and avoid redundant locking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs`:
- Around line 114-124: There is no guard preventing multiple concurrent retry
loops from ResumeStdioWithRetriesAsync, so simultaneous domain reloads can start
overlapping loops that both call StdioBridgeHost.StartAsync and flip the
EditorPrefs resume flag; add a static concurrency guard (e.g., a static volatile
bool or an Interlocked-backed static flag named something like
s_resumeLoopRunning) at the top of ResumeStdioWithRetriesAsync to check-and-set
before starting the loop and clear it when the loop completes (including on all
early returns/exceptions), and use that flag to skip starting a second loop;
reference ResumeStdioWithRetriesAsync, StartAsync and the EditorPrefs resume
flag, and keep startStopLock in StdioBridgeHost.Start() unchanged.
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Line 57: The _consecutiveTimeouts counter is incremented but never triggers
the promised self-healing; add a threshold check (e.g., const int
TIMEOUT_THRESHOLD = 3) where timeouts are counted (in HandleClientAsync or the
timeout handler) and when exceeded marshal back to the main thread via
EditorApplication.delayCall to perform corrective actions: remove and re-add the
ProcessCommands delegate from EditorApplication.update and set
processingCommands = 0, then reset _consecutiveTimeouts = 0; ensure this uses
the same ProcessCommands method reference and only mutates
EditorApplication.update on the main thread.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PortManagerTests.cs`:
- Around line 66-106: Tests call PortManager.DiscoverNewPort(), which invokes
SavePort and mutates persistent on-disk port files (unity-mcp-port-<hash>.json
and legacy file), breaking isolation and causing order-dependent failures; fix
by preventing disk writes during tests: either stub/mock SavePort via dependency
injection into PortManager (introduce an ISavePort or IFileSystem interface and
have DiscoverNewPort use it) so tests can inject a no-op saver, or implement
test setup/teardown that snapshots the existing port file contents before
running tests and fully restores them in a [TearDown] method (also delete files
created by the test) to ensure GetPortWithFallback and the running bridge see
the original state.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/StdioBridgeReconnectTests.cs`:
- Around line 22-23: ConnectTimeoutMs is declared but never enforced, so
client1.Connect(...) and client2.Connect(...) can block indefinitely; update the
tests to enforce a connect timeout by replacing blocking Connect calls with
ConnectAsync combined with a Task.Wait/WaitAsync timeout using ConnectTimeoutMs,
or set the socket timeouts (SendTimeout/ReceiveTimeout) prior to connecting and
use the async connect to abort if it exceeds ConnectTimeoutMs; modify all
occurrences of client1.Connect and client2.Connect (and any other Connect calls
in this test) to use this pattern and ensure ConnectTimeoutMs is actually
referenced rather than left unused.
---
Nitpick comments:
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Around line 466-468: The code currently acquires clientsLock twice in a
row—once to modify activeClients and again to read its Count—so merge each pair
into a single lock scope: inside the same lock(clientsLock) block perform the
mutation (Add or Remove) and then immediately read activeClients.Count into
clientCount; update both occurrences (the add at methods referencing
activeClients.Add and the remove at activeClients.Remove) to use a single lock
to reduce contention and avoid redundant locking.
| private const ulong MaxFrameBytes = 64UL * 1024 * 1024; | ||
| private const int FrameIOTimeoutMs = 30000; | ||
| private static readonly Stopwatch _uptime = Stopwatch.StartNew(); | ||
| private static volatile int _consecutiveTimeouts = 0; |
Contributor
There was a problem hiding this comment.
_consecutiveTimeouts tracking has no corrective action — self-healing feature is incomplete
The commit description promises: "force re-registering ProcessCommands after repeated timeouts; reset reentrancy guard." The counter is incremented and logged, but there is no code that:
- Checks
_consecutiveTimeoutsagainst a threshold - Removes and re-adds
ProcessCommandstoEditorApplication.update - Resets the
processingCommandsreentrancy guard (processingCommands = 0)
Without the corrective action, a stuck processingCommands = 1 guard (which could occur if a previous ProcessCommands invocation somehow exited without hitting the finally) will never be cleared by this mechanism, leaving the bridge permanently unable to process commands without a restart.
The intended fix would look like:
🔧 Suggested completion of the self-healing logic
+ private const int ConsecutiveTimeoutThreshold = 3;
// In HandleClientAsync, on TCS timeout:
int timeouts = Interlocked.Increment(ref _consecutiveTimeouts);
McpLog.Warn($"Command TCS timed out ({timeouts} consecutive)");
+ if (timeouts >= ConsecutiveTimeoutThreshold)
+ {
+ McpLog.Warn("[Self-heal] Resetting ProcessCommands hook after repeated timeouts");
+ Interlocked.Exchange(ref processingCommands, 0); // reset stuck reentrancy guard
+ if (_processCommandsHooked)
+ {
+ EditorApplication.update -= ProcessCommands;
+ _processCommandsHooked = false;
+ }
+ if (!_processCommandsHooked)
+ {
+ _processCommandsHooked = true;
+ EditorApplication.update += ProcessCommands;
+ }
+ Interlocked.Exchange(ref _consecutiveTimeouts, 0);
+ }Note: EditorApplication.update must only be modified on the main thread. If the corrective action is triggered from HandleClientAsync (which runs on a thread-pool thread), it must be marshalled to the main thread via EditorApplication.delayCall.
Also applies to: 569-574
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs` at line
57, The _consecutiveTimeouts counter is incremented but never triggers the
promised self-healing; add a threshold check (e.g., const int TIMEOUT_THRESHOLD
= 3) where timeouts are counted (in HandleClientAsync or the timeout handler)
and when exceeded marshal back to the main thread via
EditorApplication.delayCall to perform corrective actions: remove and re-add the
ProcessCommands delegate from EditorApplication.update and set
processingCommands = 0, then reset _consecutiveTimeouts = 0; ensure this uses
the same ProcessCommands method reference and only mutates
EditorApplication.update on the main thread.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PortManagerTests.cs
Show resolved
Hide resolved
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/StdioBridgeReconnectTests.cs
Show resolved
Hide resolved
- Collapse double lock in HandleClientAsync (add + count in one lock) - Add CancellationToken to ResumeStdioWithRetriesAsync so retries abort on editor quit or subsequent domain reloads - Fix 'gameobject' → 'GameObject' capitalization in skill URI table Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs (1)
116-124: DuplicatedIsCompilingreflection logic across three locations.This reflection-based
CompilationPipeline.isCompilingcheck is duplicated here, inHttpBridgeReloadHandler.OnAfterAssemblyReload, and inStdioBridgeHost.IsCompiling(). Consider extracting it into a shared helper (e.g., on a utility class) to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs` around lines 116 - 124, Extract the duplicated reflection logic that checks UnityEditor.Compilation.CompilationPipeline.isCompiling into a single shared static helper method (e.g., EditorCompilationHelper.IsCompilationInProgress) and replace the three in-place implementations in StdioBridgeReloadHandler (the block using Type.GetType and prop.GetValue), HttpBridgeReloadHandler.OnAfterAssemblyReload, and StdioBridgeHost.IsCompiling() to call that helper; the helper should perform the safe Type.GetType("UnityEditor.Compilation.CompilationPipeline, UnityEditor") lookup, reflect the static "isCompiling" property with binding flags, return the combined boolean (including EditorApplication.isCompiling), and swallow/refine exceptions consistently so callers keep the same behavior.MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
642-645: Double lock acquisition onclientsLock— combine into one.The
Removeon line 642 and theCounton line 644 each acquireclientsLockseparately. Since they're adjacent, combining them avoids the redundant lock round-trip and gives an atomic remove-and-count.♻️ Suggested fix
- lock (clientsLock) { activeClients.Remove(client); } - int remaining; - lock (clientsLock) { remaining = activeClients.Count; } + int remaining; + lock (clientsLock) + { + activeClients.Remove(client); + remaining = activeClients.Count; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs` around lines 642 - 645, Combine the two adjacent locks into a single critical section: inside one lock(clientsLock) perform activeClients.Remove(client) and int remaining = activeClients.Count; then call McpLog.Info($"Client handler exited (remaining clients: {remaining})") using that local remaining variable outside the lock; reference the existing clientsLock, activeClients, and McpLog.Info in StdioBridgeHost.cs when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Around line 572-577: The _consecutiveTimeouts counter is only logged but never
causes corrective action; add a threshold check after Interlocked.Increment(ref
_consecutiveTimeouts) (use the same block where McpLog.Warn is called) and, when
exceeded (e.g. 3-5 timeouts), perform a safe self-healing step: reset the
processingCommands reentrancy guard using Interlocked.Exchange(ref
processingCommands, 0) and re-register or invoke ProcessCommands as appropriate,
then log the recovery action via McpLog.Info/Error; ensure the reset is done
atomically and only when the guard is stuck to avoid breaking normal concurrent
behavior.
---
Nitpick comments:
In `@MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs`:
- Around line 116-124: Extract the duplicated reflection logic that checks
UnityEditor.Compilation.CompilationPipeline.isCompiling into a single shared
static helper method (e.g., EditorCompilationHelper.IsCompilationInProgress) and
replace the three in-place implementations in StdioBridgeReloadHandler (the
block using Type.GetType and prop.GetValue),
HttpBridgeReloadHandler.OnAfterAssemblyReload, and StdioBridgeHost.IsCompiling()
to call that helper; the helper should perform the safe
Type.GetType("UnityEditor.Compilation.CompilationPipeline, UnityEditor") lookup,
reflect the static "isCompiling" property with binding flags, return the
combined boolean (including EditorApplication.isCompiling), and swallow/refine
exceptions consistently so callers keep the same behavior.
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Around line 642-645: Combine the two adjacent locks into a single critical
section: inside one lock(clientsLock) perform activeClients.Remove(client) and
int remaining = activeClients.Count; then call McpLog.Info($"Client handler
exited (remaining clients: {remaining})") using that local remaining variable
outside the lock; reference the existing clientsLock, activeClients, and
McpLog.Info in StdioBridgeHost.cs when making the change.
- PortManagerTests: save/restore on-disk port files in SetUp/TearDown to prevent DiscoverNewPort from mutating persistent state - StdioBridgeReconnectTests: replace blocking Connect with ConnectAsync + Wait(ConnectTimeoutMs) so CI tests cannot hang indefinitely Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
unity-mcp-skill/SKILL.md (1)
22-31:mcpforunity://scene/gameobject/{instance_id}is absent from the table but referenced in the doc.Line 181 uses
mcpforunity://scene/gameobject/{instance_id}as the standard way to read full per-object data — a very common operation. Omitting it from the reference table may prompt an LLM to guess the parameterized form, which is precisely the behavior this section aims to suppress.📝 Suggested table addition
| Custom tools | `mcpforunity://custom-tools` | +| Scene GameObject (by ID) | `mcpforunity://scene/gameobject/{instance_id}` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/SKILL.md` around lines 22 - 31, The reference table in SKILL.md is missing the per-object resource URI mcpforunity://scene/gameobject/{instance_id} even though the doc later references it; add a new row to the existing "Resource | URI" table (near the "Scene GameObject API" / "Scene GameObject" entries) with a Resource name like "Scene GameObject (per-instance)" and the URI mcpforunity://scene/gameobject/{instance_id}, so readers and LLMs have the exact parameterized form documented.MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
642-645: Double lock acquisition onclientsLockcan be consolidated.Lines 642 and 644 each acquire
clientsLockindependently. Theremainingcount could become stale between the two acquisitions (another client could connect/disconnect). Consolidate into one block:♻️ Suggested fix
- lock (clientsLock) { activeClients.Remove(client); } - int remaining; - lock (clientsLock) { remaining = activeClients.Count; } + int remaining; + lock (clientsLock) + { + activeClients.Remove(client); + remaining = activeClients.Count; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs` around lines 642 - 645, Consolidate the two separate lock acquisitions on clientsLock into a single critical section: inside one lock(clientsLock) call remove the client from activeClients and read remaining as activeClients.Count (i.e., perform activeClients.Remove(client) and set remaining within the same lock) to avoid the count becoming stale; then call McpLog.Info($"Client handler exited (remaining clients: {remaining})") (you can log outside the lock) — this touches the code manipulating activeClients, clientsLock, and the McpLog.Info call.
29-29: Stale-eviction threshold is measured from enqueue time, not execution start — intentional?
EnqueuedAtMsis captured when the command is queued (line 554), and the eviction check on line 799 comparesnowMs - EnqueuedAtMsagainststaleThresholdMs. This means the threshold covers the entire lifecycle (wait + execution), not just execution time. A command that sat idle in the queue for most of the threshold and only recently began executing would be evicted immediately.Given that
staleThresholdMs= 2 ×FrameIOTimeoutMs= 60 s and there's already a 30 s TCS timeout inHandleClientAsync, this is probably fine in practice — but the comment on line 793 says "stuck with IsExecuting=true for too long", which is slightly misleading since the clock started at enqueue time.If the intent is truly to measure execution duration, consider capturing a separate
ExecutionStartedAtMswhenIsExecutingis set totrue(line 822). Otherwise, updating the comment to clarify the total-lifecycle semantics would prevent future confusion.Also applies to: 553-554, 793-815
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs` at line 29, The eviction check measures from enqueue time (EnqueuedAtMs) rather than execution start, which can evict commands that only recently began executing; to fix, add a new field ExecutionStartedAtMs on the same command type, set ExecutionStartedAtMs when IsExecuting is set to true (in the code path around HandleClientAsync / where IsExecuting is flipped), and change the stale eviction logic (the check that compares nowMs - EnqueuedAtMs against staleThresholdMs) to use nowMs - ExecutionStartedAtMs when IsExecuting==true (fall back to EnqueuedAtMs if ExecutionStartedAtMs is unset); also update the comment that referenced "stuck with IsExecuting=true for too long" to reflect that the eviction now measures execution duration.MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs (2)
38-41:CancellationTokenSourceis cancelled but never disposed.Each call to
ResumeStdioWithRetriesAsynccreates a newCancellationTokenSource(line 143), andCancelRetriesonly callsCancel()on the old one. The old CTS is never disposed. While CTS is lightweight and the GC will eventually collect it, the finalizer may delay cleanup of the internal timer (fromCancelAfter/Task.Delay).Consider disposing the old CTS in
CancelRetries:♻️ Suggested fix
private static void CancelRetries() { - try { _retryCts?.Cancel(); } catch { } + var old = _retryCts; + _retryCts = null; + try { old?.Cancel(); } catch { } + try { old?.Dispose(); } catch { } }Also applies to: 142-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs` around lines 38 - 41, CancelRetries currently only calls _retryCts.Cancel() leaving the old CancellationTokenSource undisposed; update CancelRetries to safely Cancel and Dispose the existing _retryCts (e.g., call _retryCts.Cancel(); _retryCts.Dispose(); _retryCts = null) to avoid leaking the CTS timer, and ensure thread-safety with the same synchronization used around _retryCts in ResumeStdioWithRetriesAsync so you don't race disposing a newly-created CTS; swallow or log ObjectDisposedException if needed.
116-124: Extract shared compilation check helper — duplicated across reload handlers.The compilation state check using
Type.GetType("UnityEditor.Compilation.CompilationPipeline")is duplicated acrossStdioBridgeReloadHandler.cs(lines 118-124),HttpBridgeReloadHandler.cs(lines 90-92), and already encapsulated inStdioBridgeHost.IsCompiling()(lines 234-251). Create a shared utility method (e.g., in aCompilationHelperclass) to centralize this logic and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs` around lines 116 - 124, Extract the duplicated compilation-state logic into a single static helper (e.g., class CompilationHelper with a public static bool IsCompiling()) that implements the current two-step check (EditorApplication.isCompiling plus the reflection lookup of "UnityEditor.Compilation.CompilationPipeline" and its static isCompiling property, with safe exception handling), update StdioBridgeReloadHandler to call CompilationHelper.IsCompiling() instead of the inline reflection block, update HttpBridgeReloadHandler to call the same helper, and remove or refactor StdioBridgeHost.IsCompiling() to delegate to CompilationHelper.IsCompiling() so all three places use the centralized method for consistency.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PortManagerTests.cs (1)
22-45:GetRegistryDirectory()is private, so the test cannot call it directly.The test's
SetUpduplicates the path construction logic fromPortManager.GetRegistryDirectory()(which isprivate static). While this duplication is currently unavoidable due to the access modifier, consider makingGetRegistryDirectory()internal so tests can call it directly. This would ensure the test and production code always target the same directory and prevent silent divergence if the path ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PortManagerTests.cs` around lines 22 - 45, Change PortManager.GetRegistryDirectory() from private static to internal static and update the test to call PortManager.GetRegistryDirectory() instead of duplicating path logic; if the test assembly is separate, add an InternalsVisibleTo attribute for the test assembly name so the internal method is accessible. Locate GetRegistryDirectory() in the PortManager class and modify its access modifier, then replace the duplicated path construction in PortManagerTests.SetUp with a call to PortManager.GetRegistryDirectory() and reuse the returned directory for _legacyFilePath and _portFilePath handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Line 57: _consecutiveTimeouts is incremented on timeout but never acted upon;
either document it as purely diagnostic or implement self-healing: add a
threshold constant (e.g., MAX_CONSECUTIVE_TIMEOUTS), check _consecutiveTimeouts
in the timeout handling path inside ProcessCommands (or the method that
increments it), and when the threshold is exceeded perform corrective actions
such as logging an error with the count, resetting the reentrancy guard,
re-registering or restarting ProcessCommands (or re-initializing the transport),
and resetting _consecutiveTimeouts to 0; alternatively, add a clear comment next
to the _consecutiveTimeouts declaration stating it is only diagnostic if you
choose not to implement the self-healing path.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PortManagerTests.cs`:
- Around line 48-57: TearDown currently only restores pre-existing files when
_savedLegacyFileContent/_savedPortFileContent are non-null, leaving files
created during tests on disk; update the TearDown method (referencing TearDown,
_savedLegacyFileContent, _legacyFilePath, _savedPortFileContent, _portFilePath
and the test flow that calls DiscoverNewPort) so that for each tracked path: if
the saved content is non-null, write it back to the file; otherwise if the path
is non-null and the file exists, delete the file to remove artifacts created
during the test run.
---
Nitpick comments:
In `@MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs`:
- Around line 38-41: CancelRetries currently only calls _retryCts.Cancel()
leaving the old CancellationTokenSource undisposed; update CancelRetries to
safely Cancel and Dispose the existing _retryCts (e.g., call _retryCts.Cancel();
_retryCts.Dispose(); _retryCts = null) to avoid leaking the CTS timer, and
ensure thread-safety with the same synchronization used around _retryCts in
ResumeStdioWithRetriesAsync so you don't race disposing a newly-created CTS;
swallow or log ObjectDisposedException if needed.
- Around line 116-124: Extract the duplicated compilation-state logic into a
single static helper (e.g., class CompilationHelper with a public static bool
IsCompiling()) that implements the current two-step check
(EditorApplication.isCompiling plus the reflection lookup of
"UnityEditor.Compilation.CompilationPipeline" and its static isCompiling
property, with safe exception handling), update StdioBridgeReloadHandler to call
CompilationHelper.IsCompiling() instead of the inline reflection block, update
HttpBridgeReloadHandler to call the same helper, and remove or refactor
StdioBridgeHost.IsCompiling() to delegate to CompilationHelper.IsCompiling() so
all three places use the centralized method for consistency.
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Around line 642-645: Consolidate the two separate lock acquisitions on
clientsLock into a single critical section: inside one lock(clientsLock) call
remove the client from activeClients and read remaining as activeClients.Count
(i.e., perform activeClients.Remove(client) and set remaining within the same
lock) to avoid the count becoming stale; then call McpLog.Info($"Client handler
exited (remaining clients: {remaining})") (you can log outside the lock) — this
touches the code manipulating activeClients, clientsLock, and the McpLog.Info
call.
- Line 29: The eviction check measures from enqueue time (EnqueuedAtMs) rather
than execution start, which can evict commands that only recently began
executing; to fix, add a new field ExecutionStartedAtMs on the same command
type, set ExecutionStartedAtMs when IsExecuting is set to true (in the code path
around HandleClientAsync / where IsExecuting is flipped), and change the stale
eviction logic (the check that compares nowMs - EnqueuedAtMs against
staleThresholdMs) to use nowMs - ExecutionStartedAtMs when IsExecuting==true
(fall back to EnqueuedAtMs if ExecutionStartedAtMs is unset); also update the
comment that referenced "stuck with IsExecuting=true for too long" to reflect
that the eviction now measures execution duration.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PortManagerTests.cs`:
- Around line 22-45: Change PortManager.GetRegistryDirectory() from private
static to internal static and update the test to call
PortManager.GetRegistryDirectory() instead of duplicating path logic; if the
test assembly is separate, add an InternalsVisibleTo attribute for the test
assembly name so the internal method is accessible. Locate
GetRegistryDirectory() in the PortManager class and modify its access modifier,
then replace the duplicated path construction in PortManagerTests.SetUp with a
call to PortManager.GetRegistryDirectory() and reuse the returned directory for
_legacyFilePath and _portFilePath handling.
In `@unity-mcp-skill/SKILL.md`:
- Around line 22-31: The reference table in SKILL.md is missing the per-object
resource URI mcpforunity://scene/gameobject/{instance_id} even though the doc
later references it; add a new row to the existing "Resource | URI" table (near
the "Scene GameObject API" / "Scene GameObject" entries) with a Resource name
like "Scene GameObject (per-instance)" and the URI
mcpforunity://scene/gameobject/{instance_id}, so readers and LLMs have the exact
parameterized form documented.
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
1 participant
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.
Summary
Root Causes
Test plan
Fixes #785
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests
Documentation