fix(editor): qualify server PID EditorPrefs key by port#831
Open
Ramlock wants to merge 3 commits into
Open
Conversation
McpServerManager stored the running server's PID under a fixed EditorPrefs
key ("McpServerManager_ProcessId"). EditorPrefs are machine-global - shared
across every project of the same Editor version - and the key was not
qualified by project or port, so two Editors open at once (different
projects, different ports) clobbered a single slot.
The result was mutual exclusion despite distinct ports: after Editor B
started its server and overwrote the slot with B's PID, Editor A's
CheckExistingProcess (run on domain reload) read B's PID, adopted B's
server as its own _serverProcess, and terminated it on the next stop/quit -
dropping B's connection. Symmetric, so whichever Editor was touched second
killed the other's server.
Qualify the key by the server port via the pure, unit-tested
ProcessIdKeyForPort(int) helper so each project tracks its own server
independently. The per-project binary folder and the orphaned-process kill
were already port-scoped; this closes the last shared-state gap.
Adds EditMode tests for the helper.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a multi-project Unity Editor concurrency issue by making the EditorPrefs key used to persist the MCP server PID port-qualified, preventing two Unity projects (on different ports) from overwriting and later terminating each other’s server process. It also adds EditMode regression tests for the new key derivation helper.
Changes:
- Replace the single global
EditorPrefsPID key with a port-qualified key viaProcessIdKeyForPort(int port). - Update
McpServerManagerto use the derived key for PID persistence across domain reloads. - Add EditMode unit tests validating determinism and uniqueness of the derived key per port.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Editor/Scripts/McpServerManager.cs | Derives the stored PID key from the server port and exposes a pure helper for testing. |
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Tests/Editor/McpServerManagerProcessIdKeyTests.cs | Adds regression coverage for port-qualified PID key behavior. |
| Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Tests/Editor/McpServerManagerProcessIdKeyTests.cs.meta | Unity asset metadata for the new test file. |
Files not reviewed (1)
- Unity-MCP-Plugin/Packages/com.ivanmurzak.unity.mcp/Tests/Editor/McpServerManagerProcessIdKeyTests.cs.meta: Generated file
| // every project of the same Editor version - so an UNqualified key let one project's | ||
| // CheckExistingProcess adopt the OTHER project's server PID and then tear it down on stop, | ||
| // making the two Editors mutually exclusive even though their ports differ. See ProcessIdKeyForPort. | ||
| static string ProcessIdKey => ProcessIdKeyForPort(UnityMcpPluginEditor.Port); |
| const string ProcessIdKey = "McpServerManager_ProcessId"; | ||
| // Qualified by the server port so two Editors (different projects, different ports) on | ||
| // the same machine never share this slot. EditorPrefs are machine-global - shared across | ||
| // every project of the same Editor version - so an UNqualified key let one project's |
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
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.
Fixes #830.
McpServerManagertracked the running server's PID in a fixedEditorPrefskey ("McpServerManager_ProcessId"). SinceEditorPrefsare shared across every project of the same Editor version on a machine and the key wasn't port/project-qualified, two Editors open at once clobbered one slot -- and one project'sCheckExistingProcesswould adopt the other's server PID and terminate it on stop, making the two mutually exclusive despite distinct ports.Change
The key is now derived per port via a pure, unit-tested helper:
The per-project binary folder and orphaned-process kill were already port-scoped; this closes the remaining shared-state gap.
Tests
Adds
McpServerManagerProcessIdKeyTests(EditMode) covering distinct keys per port, stability per port, and port inclusion.Notes
UnityMcpPluginEditor.Portis already used throughout this class; ifInstanceisn't ready during early startup it falls back to the deterministic per-directory port, which is still project-specific -- so the key stays correct.