fix(transport): debounce orphan-session teardown so server restarts don't strand sessions#155
Open
dsarno wants to merge 26 commits into
Open
fix(transport): debounce orphan-session teardown so server restarts don't strand sessions#155dsarno wants to merge 26 commits into
dsarno wants to merge 26 commits into
Conversation
Unity 6 exposes MemoryProfiler.TakeSnapshot from UnityEngine.CoreModule with CaptureFlags overloads, so the old Unity.MemoryProfiler.Editor reflection path makes memory_take_snapshot report that com.unity.memoryprofiler is missing even when the package is installed. The profiler list and compare actions only inspect snapshot files, so they no longer depend on MemoryProfiler type discovery. Constraint: Unity 6 moved MemoryProfiler APIs into UnityEngine.CoreModule. Rejected: Require com.unity.memoryprofiler editor assembly type discovery for all memory actions | list and compare only need filesystem metadata. Confidence: high Scope-risk: narrow Tested: uv run --extra dev pytest tests/test_manage_profiler.py -v Tested: Reflected Unity 6 MemoryProfiler.TakeSnapshot overloads in GameClient editor. Not-tested: Full Unity package test suite.
The MemoryProfiler reflection code now verifies the selected overload's trailing parameter type before invoking it. This keeps Unity 6 CaptureFlags overloads and legacy uint overloads from being mixed in environments where both type families can be discovered. Constraint: Reflection overload selection spans Unity versions with different trailing argument types. Confidence: high Scope-risk: narrow Tested: Patched GameClient PackageCache and verified memory_take_snapshot, memory_list_snapshots, memory_compare_snapshots. Tested: Verified profiler_status, get_frame_timing, get_counters, frame_debugger_get_events, and compare error paths. Not-tested: Full Unity package test suite.
…uilder EditMode test assumptions UnityTypeResolver.FindCandidates treated a short name as ambiguous when an internal type shared it (richer assembly sets), so unqualified generics like List<T>/Dictionary<TKey,TValue> failed to resolve while fully-qualified names worked. Add DisambiguateByIdentity: de-dupe by FullName, then prefer a single public type, then a single core/BCL type, narrowing only to a unique survivor so genuine clashes still report ambiguous. Fix EditMode tests that encoded built-in-pipeline/contract assumptions and only failed under URP + ProBuilder (the package test project uses the built-in pipeline and lacks ProBuilder, so these were never exercised): - ManageGraphics negative tests read result["error"] (ErrorResponse has no "message" field); feature_add uses the handler's "type" param, not "feature_type" - ManageMaterialProperties uses the pipeline-appropriate color property (_BaseColor on URP/HDRP) - MCPToolParameterTests asserts pipeline-appropriate shader name and reads _Smoothness on URP/HDRP - ManageProBuilder get_mesh_info test requests include:"faces" Verified in TestbedMCP (URP 17.2 + ProBuilder 6.0.9): full EditMode suite 887 passed / 0 failed / 46 skipped.
… liveness polling
Convert the local MCP HTTP server launch from a visible-terminal model to a
headless background launch and make startup diagnosable and robust.
- Launch windowless via TerminalLauncher.CreateHeadlessProcessStartInfo;
combined stdout/stderr redirected to a per-port log at
Library/MCPForUnity/Logs/server-launch-{port}.log (truncated each launch)
- Replace the per-launch confirmation dialog with a one-time confirm gated on
the new EditorPrefs key HttpServerLaunchConfirmed; the quiet auto-start path
skips it and does not set the flag
- Prepend platform uv/uvx PATH entries so bare uvx/uv resolves under
GUI-launched Unity's minimal non-login PATH (notably macOS)
- Replace the fixed ~30-attempt reachability waits with open-ended polling
tied to the launched process's liveness (5-minute hard cap), emitting a
tail-of-log failure report when the server dies — in both
HttpAutoStartHandler and McpConnectionSection
- Simplify quit-time cleanup to a handshake-scoped StopManagedLocalHttpServer
so headless servers are not left as invisible orphans
- Transport-aware UI button labels (Connect/Disconnect vs Start/End Session),
a transient "Starting…" state, and clearer lifecycle logging
Tests: TerminalLauncherTests covers the headless start-info contract;
ServerManagementServiceCharacterizationTests covers the one-time-confirm gate,
quiet-path bypass, and per-port log redirection.
…ayDev#1120) Kilo Code v7.0.33+ moved MCP config out of the VS Code extension's globalStorage/mcp_settings.json to a CLI-style kilo.jsonc under ~/.config/kilo, with a new schema (https://app.kilo.ai/config.json): an "mcp" container, type:"remote" for HTTP servers (type:"local" for stdio), and an "enabled" flag. Writing the legacy mcpServers / type:"http" / disabled config left the server showing as stdio + disabled. - KiloCodeConfigurator targets ~/.config/kilo/kilo.jsonc on every OS and declares the new format (mcp container, type:remote/local, enabled:true, $schema) - Generalize the per-client HTTP "type" override: replace the UsesStreamableHttpType bool with McpClient.HttpTypeValue (Cline/Roo => "streamableHttp", Kilo => "remote", default "http"); add StdioTypeValue, ServerContainerKey and SchemaUrl fields - ConfigJsonBuilder honors ServerContainerKey ("mcp") and writes a root $schema; JsonFileMcpConfigurator.CheckStatus/Unregister read/remove from the configured container so status detection and teardown work for Kilo - Replace StreamableHttpTypeTests with ClientConfigFormatTests covering Kilo's remote/mcp/enabled format, Cline's streamableHttp, and generic http Verified: 5 new tests + 13 existing config tests pass (EditMode, Unity 2021.3).
A one-command, no-API-key end-to-end gate for the Python<->Unity bridge, runnable locally and in CI. - tools/local_harness.py: boots a headless Hub-licensed Editor (or attaches with --reuse) and runs smoke + EditMode + PlayMode legs over the bridge, aggregating JUnit; exit codes 0-5 (pass / regression / unreachable / no-compile / no-license / no-editor) - Server/tests/e2e/bridge_smoke.py: deterministic no-LLM contract driver over the real wire path; the no-LLM counterpart to claude-nl-suite.yml - .github/workflows/e2e-bridge.yml: PR gate booting headless Unity in CI; self-skips (warns) when Unity license secrets are absent - .github/workflows/python-tests.yml: run the hermetic harness unit tests and add tools/** to the path triggers - tools/tests/test_local_harness.py: 69 hermetic unit tests for the harness's Unity-free decision logic (discovery, version resolution, exit-code mapping) - Document the harness in CLAUDE.md and the contributor docs
- Server/tests/test_tool_test_symmetry.py discovers every module exposing an @mcp_for_unity_tool and fails CI if it is not referenced by any test - A KNOWN_UNTESTED quarantine lets pre-existing gaps (execute_menu_item, manage_shader, manage_tools) skip rather than fail; a second test fails if a quarantine entry is stale, so the list can only shrink Operationalizes the CLAUDE.md rule that every new tool ships with a test.
pyproject.toml is already at 9.7.1; the lockfile lagged at 9.6.6.
…roject runtime artifacts The 8 EditMode/Tools test scripts were tracked without their .meta files, so every contributor's Unity regenerated them with random GUIDs (perpetual untracked churn). Commit the metas so Unity reuses a stable GUID. Also ignore the MCP bridge / UIToolkit runtime artifacts (Assets/UnityMCP, Assets/UI) the test project emits when run.
- local_harness: fail fast (exit 2) on empty --legs and on --ci without UNITY_IMAGE, instead of silently exiting green / crashing with an unhandled CalledProcessError - local_harness: correct the nearest-patch `best` type annotation (key is (patch:int, suffix:str), not a 3-tuple) - test_tool_test_symmetry: fix the module docstring to match behavior (only test_*.py files are scanned; non-test_*.py scripts like tests/e2e/bridge_smoke.py do not count toward coverage) Deferred CodeRabbit's SHA-pin suggestion for e2e-bridge.yml: the repo pins actions by tag (@v4/@v6), not SHA, so pinning one workflow would be inconsistent — left as a repo-wide policy decision.
parse_legs stays a lenient normalizer (test_drops_unknown_legs), but main() now validates the raw --legs input against the shared ALLOWED_LEGS and exits 2 on any unknown value -- so a typo like `smoke,edimode` fails loudly instead of silently dropping the bad leg and running a subset.
…kwins-and-e2e Patch: headless http server launch, Kilo Code (CoplayDev#1120), e2e bridge harness
…eta.12-27528862114 chore: update Unity package to beta version 9.7.2-beta.12
…er-coremodule-api Fix Memory Profiler snapshot actions on Unity 6
…eta.13-27529138874 chore: update Unity package to beta version 9.7.2-beta.13
chore: bump version to 9.7.3
…o-beta-27529986410 chore: sync main (v9.7.3) into beta
…on't strand sessions The HTTP-Local orphan detector (McpConnectionSection.UpdateConnectionStatus) ended the session on a single unreachable server probe. That path also tears down the WebSocket transport's reconnect loop, so a transient blip — e.g. a server restart — stranded the session until a manual Start Session (issue CoplayDev#1207 "trigger 2"). Debounce it: only end the session after the local server has stayed unreachable continuously for OrphanGraceSeconds (10s), leaving the reconnect loop to own transient outages. Adds a pure ShouldEndOrphanedSession helper + a truth-table unit test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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
4 participants
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.
Problem
The HTTP-Local orphan detector in
McpConnectionSection.UpdateConnectionStatus()ended the session the instant a single server-reachability probe failed. That teardown path (EndOrphanedSessionAsync→StopAsync) also tears down the WebSocket transport's reconnect loop — so a transient outage (most commonly a dev-server restart, but also a momentary local-port blip) permanently stranded the session until a manual Start Session.This is the "trigger 2" failure mode behind
CoplayDev/unity-mcp#1207(false orphan teardown at reload/restart boundaries). In the field it took down a live editor with a single console line during a server restart:Fix
Debounce the teardown. Track when the local server first became unreachable and only end the session once it has stayed unreachable continuously for
OrphanGraceSeconds(10s). Any successful reachability check clears the timer, so a blip no longer trips it — the reconnect loop owns transient outages and recovers on its own.ShouldEndOrphanedSession(sessionRunning, localServerReachable, unreachableSinceTime, now, graceSeconds)— all the decision logic, no Unity state, fully unit-testable.serverUnreachableSinceTimerecords the start of a continuous unreachable window; it resets to-1whenever the server is reachable again, the session ends, or the controls aren't shown.Test
Truth-table unit test
McpConnectionSection_ShouldEndOrphanedSession_DebouncesTransientUnreachable(reflection-invoked, inWindows_Characterization.cs): ends only whenrunning && unreachable && elapsed ≥ grace; never on a blip within grace, when the server is reachable, when the session isn't running, or with no recorded unreachable-start.Scope
Standalone fix branched off
beta, independent of the liveness-watchdog work in #151. The substantive change is the singlefix(transport): debounce orphan-session teardowncommit; the remaining commits are the usualbeta-behind-upstream/betadelta (same artifact as #151).Relates to
CoplayDev/unity-mcp#1207.🤖 Generated with Claude Code