fix(server): keep the shared HTTP server alive when another editor quits - #1380
Open
KamilDev wants to merge 1 commit into
Open
fix(server): keep the shared HTTP server alive when another editor quits#1380KamilDev wants to merge 1 commit into
KamilDev wants to merge 1 commit into
Conversation
Two editors connected to one Unity-managed HTTP-local server: closing either one, including the one that never launched it, terminated the server and disconnected the other editor and the MCP client. The launch handshake (pidfile path + instance token) lived in a single EditorPrefs slot, which is per user and shared by every running editor, so a quitting editor read the launcher's handshake, validated the real token, and stopped the server. - Key the handshake and PID tracking in EditorPrefs per project hash and per port, so concurrent editors and concurrent servers on different ports never read or clear each other's state - Record the launched port in SessionState as the per-editor-process launch marker; quit cleanup resolves the server to stop from that marker only, so servers launched by other editors or externally are never touched - Before stopping on quit, ask the server (GET /api/instances) how many other Unity instances are still connected and leave it running unless this editor is the last one; a probe that fails or times out (500 ms) also leaves it running - Drop the unused TryGetPortFromPidFilePath and the redundant per-slot port key - Rewrite the misleading "never touches servers launched by other Unity instances" comments - EditMode tests: per-port isolation and launch-marker lifecycle in PidFileManagerTests, last-one-out decision in McpEditorShutdownCleanupTests, /api/instances response parsing in ServerManagementServiceInstanceProbeTests
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesThe local HTTP server lifecycle now uses project-and-port-scoped tracking. The launching editor records its port in Local server tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The per-port tracking and shutdown changes preserve shared-server availability for connected editors without any remaining concrete merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant UnityEditor
participant McpEditorShutdownCleanup
participant ServerManagementService
participant LocalHTTPServer
UnityEditor->>McpEditorShutdownCleanup: editor shutdown
McpEditorShutdownCleanup->>ServerManagementService: get launched port
ServerManagementService-->>McpEditorShutdownCleanup: launched port
McpEditorShutdownCleanup->>ServerManagementService: count other instances
ServerManagementService->>LocalHTTPServer: GET /api/instances
LocalHTTPServer-->>ServerManagementService: instance list
ServerManagementService-->>McpEditorShutdownCleanup: count or probe failure
McpEditorShutdownCleanup->>ServerManagementService: stop server when count is zero
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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.
Description
Closing one Unity editor kills the shared HTTP-local MCP server that other still-open editors are connected to.
Repro: open two Unity projects with the package on HTTP Local, let one of them launch the server, connect both, then quit the editor that did not launch it. The server process exits and the surviving editor's bridge disconnects (the MCP client loses every instance too).
Root cause (v10.2.0): on
EditorApplication.quitting, every editor callsStopManagedLocalHttpServer(), which resolves the server through the launch handshake (pidfile path+instance token).PidFileManagerkept that handshake in a singleEditorPrefsslot.EditorPrefsis per user (registry / plist), so it is shared by every editor the user has open. The quitting editor therefore read the launcher's handshake, validated the real token against the live process command line, and terminated the shared server. The comment inMcpEditorShutdownCleanupsaying the stop "never touches servers launched by other Unity instances" was false for instances on the same machine. Same single slot forStoreTracking, so two Unity-managed servers on different ports also overwrote each other's state.Type of Change
Changes Made
Per-project, per-port state instead of one global slot
PidFileManagersuffixes every handshake/tracking key with.{projectHash}.{port}. Two editors, or two servers on different ports, never read or clear each other's state.StoreHandshake/TryGetHandshake/GetStoredArgsHash/ClearTrackingnow take the port; the redundant stored-port key and the now-unusedTryGetPortFromPidFilePathare gone.Per-editor-process ownership
StoreHandshakealso records the launched port inSessionState(MCPForUnity.LocalHttpServer.LaunchedPort): survives domain reloads, dies with the editor process.StopManagedLocalHttpServer()resolves the server to stop from that marker only, so servers launched by another editor, by an earlier run of this editor, or externally (manualuvx) are never resolved at quit. The EditorPrefs handshake still survives restarts so the manual Stop Server button and the pre-launch stop keep their deterministic, token-validated path.Last-one-out on quit
GET /api/instances(already served in local mode for the CLI) and counts instances whose project hash is not ours (our own session may still be listed while the hub processes the WebSocket close we just issued). Other instances connected → leave the server running and log at debug. Probe fails or exceeds its 500 ms budget → also leave it running; a stray headless process is recoverable, a torn-down shared server is not. The quit handler stays bounded: 750 ms transport stop + 500 ms probe.McpEditorShutdownCleanup.ShouldStopManagedServer(int? otherConnectedInstances)andServerManagementService.TryCountOtherInstances(json, ownHash, out count), mirroring the existingShouldRunCleanupsplit.Unchanged on purpose
UNITY_MCP_ALLOW_BATCHbatch-mode guard.StopLocalHttpServerInternal(they now key off the port they are already operating on). The Python server writes the pidfile before uvicorn binds, so a quit before the port is bound already found nothing to kill; the probe failing in that window changes nothing.EditorPrefsWindowno longer lists the six server keys: they are dynamic per project/port now.Compatibility / Package Source
tools/compile-check.sh) against 6000.3.14f1 reference assemblies, package + EditMode test sources. No#if UNITY_*gates touched.file:(local checkout)Testing/Screenshots/Recordings
Server/changeNew/updated EditMode tests:
PidFileManagerTests: per-port handshake isolation, two-port slots, launch-marker set byStoreHandshake, cleared byClearTrackingfor the same port only,TryGetLaunchedPortfalse when nothing was launched (the regression case). The fixture saves and restores the host editor's SessionState marker so running the suite never drops a real launch.McpEditorShutdownCleanupTests:ShouldStopManagedServerfor 0 / N / probe-failed.ServerManagementServiceInstanceProbeTests:/api/instancesparsing — own session filtered (case-insensitive), other project counted, missing hash counted as other,success:false/ missing array / malformed / null → false.What was actually verified, and how
MCPForUnity.Runtime,MCPForUnity.Editor,TestAsmdefand the EditMode test assembly against 6000.3.14f1 reference assemblies: zero errors. The EditMode tests have not been executed as an NUnit fixture (needs a licensed Editor run; CI's fork-PR EditMode job is a skip). Their assertion bodies were executed against the live Editor viaexecute_code, see below.file:package. It launched the server (pidfile in itsLibrary/MCPForUnity/RunState, token on the process command line).PackageInfo.FindForAssembly(...).resolvedPathconfirmed the patched package was the one loaded.TryGetLaunchedPort→ 8081, SessionState marker 8081, per-project+port handshake present, old global key absent,TryCountOtherConnectedUnityInstances(8081, 500)→ ok, 1 other, 3 ms; against a dead port with a 300 ms budget → false in 312 ms.TryCountOtherInstanceson the live/api/instancespayload → 1 other from either project's point of view;ShouldStopManagedServer1 / 0 / null → keep / stop / keep, NUnit asserts passing.GET /api/instances.TryGetLaunchedPort_NothingLaunched_ReturnsFalse), and the genuinely-last-editor stop path (covered byShouldStopManagedServer(0)plus the unchangedStopLocalHttpServerInternal).Documentation Updates
website/docs/referenceneeded no regeneration.Related Issues
Relates to #1023 (multi-editor support). The reported behaviour is the shutdown half of running several editors against one HTTP-local server.
Additional Notes
MCPForUnity.LocalHttpServer.Last*) from earlier versions are left in place; nothing reads them anymore.Summary by CodeRabbit
Bug Fixes
Tests