fix: stop tearing down healthy HTTP sessions on transient probe misses (#1207)#1237
fix: stop tearing down healthy HTTP sessions on transient probe misses (#1207)#1237Scriptwonder wants to merge 2 commits into
Conversation
CoplayDev#1207) The orphaned-session detector ended an active session on a SINGLE stale reachability reading, and the reading came from a lone 50ms TCP connect cached for 0.75s — trivially false-negative on a machine busy with test runs or domain reloads. Evidence bundles in CoplayDev#1207 show 13 teardowns and 147 socket closures in one session from exactly this loop, wedging the bridge in no_unity_session churn until manual recovery. - Require 3 consecutive failed polls (0.75s cadence) before declaring a session orphaned; probe readings taken while the editor is compiling or importing don't count toward teardown, and detection is skipped entirely while busy. - Raise the probe's connect wait 50ms -> 250ms, as an overall budget shared across candidate hosts so the worst-case main-thread wait cannot multiply. - Honor UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S above 20s (ceiling now 120s; default unchanged): the old ceiling equalled the default, silently neutering the documented escape hatch for projects whose reloads or test boundaries legitimately exceed 20s. Same treatment for UNITY_MCP_SESSION_READY_WAIT_SECONDS, and both now share one bounded env-read helper. The remaining piece of CoplayDev#1207 (keepalive reload-awareness in WebSocketTransportClient) is untouched here: the resume machinery reworked in CoplayDev#1234 already covers reload boundaries, and the detector debounce removes the dominant churn source dsarno identified.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdjusts Unity editor local HTTP server reachability probing to use a longer overall timeout with a shared time budget across hosts, and introduces a consecutive-down-poll threshold to defer orphaned-session teardown. Server-side, centralizes environment-variable wait-time parsing/clamping in plugin_hub.py with an expanded ceiling, plus new tests. ChangesUnity Editor: Reachability probing and orphan-session teardown
Estimated code review effort: 3 (Moderate) | ~25 minutes Server-side bounded wait-time environment handling
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Poller as UpdateStartHttpButtonState
participant Status as UpdateConnectionStatus
participant Policy as ShouldEndOrphanedSession
Poller->>Poller: probe local HTTP server reachability
Poller->>Poller: increment/reset consecutiveServerDownPolls
Status->>Policy: check editorBusy, toggleInProgress, downPollCount
Policy-->>Status: return true/false
Status->>Status: end session if true
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
342-366: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset the orphaned-poll counter when the session starts
consecutiveServerDownPollsonly clears afterUpdateStartHttpButtonState()sees the server up again, so a stale count can survive a successfulStartAsync()and immediately satisfy orphan detection before the next fresh probe. Clear it when the session transitions to running.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs` around lines 342 - 366, The orphaned-session detection in UpdateConnectionStatus can be triggered by a stale consecutiveServerDownPolls count after the session transitions to running. Reset consecutiveServerDownPolls when the connection successfully starts, and make sure the reset happens in the start-path tied to StartAsync/UpdateStartHttpButtonState so ShouldEndOrphanedSession only uses fresh probes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs`:
- Around line 342-366: The orphaned-session detection in UpdateConnectionStatus
can be triggered by a stale consecutiveServerDownPolls count after the session
transitions to running. Reset consecutiveServerDownPolls when the connection
successfully starts, and make sure the reset happens in the start-path tied to
StartAsync/UpdateStartHttpButtonState so ShouldEndOrphanedSession only uses
fresh probes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39ada2ce-19ef-4b3b-955c-1851ab24acdd
📒 Files selected for processing (6)
MCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/transport/plugin_hub.pyServer/tests/test_plugin_hub_wait_env.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta
Review follow-up: a streak accumulated while no session was running (detector inert, counter still counting) survived into a freshly started session and could satisfy orphan detection before the first post-start probe refreshed. Reset on the not-running -> running transition, which covers every start path (manual Connect, auto-start, resume) since UpdateConnectionStatus runs on the UI tick.
2 participants
Addresses the dominant churn source in #1207, following the fix order @dsarno proposed from the evidence bundle.
Problem
Two amplifiers turned ordinary reload/test boundaries into
no_unity_sessionwedges:McpConnectionSection.UpdateConnectionStatusends an active session the moment one cached reachability reading is false — and that reading is a lone TCP connect with a 50 ms wait, cached 0.75 s. On a machine busy with a test run or domain reload the probe times out against a perfectly healthy server; the detector then force-stops the bridge (13 teardowns / 147 socket closures in one session in the macOS / HTTP transport: bridge WebSocket repeatedly drops (close 1005) on reload/test boundaries → no_unity_session #1207 evidence bundle), feeding the reconnect churn that outruns the server's session-resolve window.UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_Sis documented as configurable but was clamped to a ceiling equal to its default (20 s), so users whose reloads legitimately exceed 20 s could not actually extend the window.Fix
internal staticpredicate with tests.UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_Sceiling raised to 120 s (default unchanged at 20 s);UNITY_MCP_SESSION_READY_WAIT_SECONDSlikewise (default 6 s). Both now share one bounded env-read helper with tests.What this deliberately does not do
WebSocketTransportClient— fix(editor): survive domain reloads in HTTP auto-start and reload-resume (#1229) #1234's resume rework already owns reload boundaries, and per the maintainer analysis the detector debounce removes the dominant churn source. If churn persists after both land, that's the next lever.Verification
cd Server && uv run pytest tests/ -q→ 1313 passed, 3 skipped (5 new env-clamp tests).tools/check-unity-versions.sh(2021.3 floor passes locally).Summary by CodeRabbit