fix: harden MCP extension-point registration against races and stale state by jdneo · Pull Request #164 · microsoft/copilot-for-eclipse

jdneo · 2026-05-09T07:02:53Z

Synchronize all extMcpInfoMap accesses on the manager monitor so the HashMap is never iterated/mutated concurrently by the async doRegistration() worker, the contribution-point-policy event handler, and the UI-thread approval flow.
approveExtMcpRegistration() now snapshots the map under the lock and opens McpApprovalDialog outside the lock so the worker is not stalled while the user interacts; post-dialog reconciliation re-acquires the lock and the LSP-pushing event publish runs outside the lock.
Mark approvedExtMcpServers volatile and make hasExtMcpRegistration() synchronized for consistent visibility from the UI thread.
Avoid pre-populating in-memory approved servers from the persisted cache at startup; let doRegistration() refresh state and fire TOPIC_MCP_EXTENSION_POINT_REGISTRATION_COMPLETED so the LSP receives the verified set instead of stale data (uninstalled plugin, changed port, removed server).
Fix bundle-state guard in loadMcpRegistrationExtensionPoint() (|| -> &&) so already-active or starting bundles are not pointlessly started again.
LanguageServerSettingManager subscribes to the new completion topic and exposes an overload that accepts the approved JSON directly, avoiding the ChatServiceManager lookup from early-startup paths.
Add regression tests covering plugin uninstall, config change (re-approval required), and unchanged config (approval carry-over).

Issue: #153

…state - Synchronize all extMcpInfoMap accesses on the manager monitor so the HashMap is never iterated/mutated concurrently by the async doRegistration() worker, the contribution-point-policy event handler, and the UI-thread approval flow. - approveExtMcpRegistration() now snapshots the map under the lock and opens McpApprovalDialog outside the lock so the worker is not stalled while the user interacts; post-dialog reconciliation re-acquires the lock and the LSP-pushing event publish runs outside the lock. - Mark approvedExtMcpServers volatile and make hasExtMcpRegistration() synchronized for consistent visibility from the UI thread. - Avoid pre-populating in-memory approved servers from the persisted cache at startup; let doRegistration() refresh state and fire TOPIC_MCP_EXTENSION_POINT_REGISTRATION_COMPLETED so the LSP receives the verified set instead of stale data (uninstalled plugin, changed port, removed server). - Fix bundle-state guard in loadMcpRegistrationExtensionPoint() (|| -> &&) so already-active or starting bundles are not pointlessly started again. - LanguageServerSettingManager subscribes to the new completion topic and exposes an overload that accepts the approved JSON directly, avoiding the ChatServiceManager lookup from early-startup paths. - Add regression tests covering plugin uninstall, config change (re-approval required), and unchanged config (approval carry-over). Issue: #153 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Hardens MCP extension-point registration in the Copilot for Eclipse UI layer by improving cross-thread state consistency (async registration worker vs. UI approval flow) and ensuring the language server receives verified (non-stale) extension-contributed MCP server state.

Changes:

Synchronizes extension-contribution state updates and publishes a new completion event so the LSP is updated even when preference-change notifications don’t fire.
Avoids seeding in-memory approved servers from persisted cache at startup to prevent stale configs (e.g., uninstalled plugin / changed port) from being pushed to the LSP first.
Adds regression tests for uninstall/config-change/no-change scenarios.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/McpPreferencePage.java	Removes direct LSP sync after approval; relies on manager event publication.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/LanguageServerSettingManager.java	Subscribes to new “registration completed” topic and adds overload to sync using provided approved JSON.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/McpExtensionPointManager.java	Adds locking/visibility hardening, publishes completion event, avoids stale startup push, fixes bundle state guard.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java	Removes direct LSP sync after approval; relies on manager event publication.
com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/McpExtensionPointManagerTest.java	Adds regression tests for stale-state scenarios (uninstall/config change/unchanged).
com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/events/CopilotEventConstants.java	Introduces new event topic for “MCP extension-point registration completed”.

+      synchronized (this) {
+        FeatureFlags flags = CopilotCore.getPlugin().getFeatureFlags();
+        if (flags != null && !flags.isMcpEnabled()) {
+          return;
+        }
+        loadMcpRegistrationExtensionPoint();
+        detectChangesInMcpContribs(persistedMcpContribs);


+    Map<String, McpRegistrationInfo> dialogInput;
+    synchronized (this) {
+      if (extMcpInfoMap.isEmpty()) {
+        MessageDialog.openInformation(shell, "", "No MCP server registration found");
+        return null;
+      }
+      // Take a shallow snapshot so the dialog can iterate the map safely even if doRegistration()
+      // mutates extMcpInfoMap on the async worker. Approval flips happen on the McpRegistrationInfo
+      // value objects, which are shared between the snapshot and the live map by reference, so the
+      // user's choices are reflected in extMcpInfoMap once the dialog returns.
+      dialogInput = new HashMap<>(extMcpInfoMap);
    }



+    updateApprovedMcpServerString(extMcpInfoMap);
+    persistExtMcpInfo(extMcpInfoMap);


jdneo · 2026-05-09T07:10:20Z

@martinlippert Would you mind helping to verify if this patch fixes the issue?

Clone the code and switch to the branch cs/issue-153

Then run ./mvnw clean verify -DskipTests, the zip can be found at com.microsoft.copilot.eclipse.repository/target/com.microsoft.copilot.eclipse.repository-0.17.0-SNAPSHOT.zip

martinlippert · 2026-05-10T14:37:26Z

@martinlippert Would you mind helping to verify if this patch fixes the issue?

Clone the code and switch to the branch cs/issue-153

Then run ./mvnw clean verify -DskipTests, the zip can be found at com.microsoft.copilot.eclipse.repository/target/com.microsoft.copilot.eclipse.repository-0.17.0-SNAPSHOT.zip

@jdneo Happy to it a try, but running the build doesn't work when you checkout the repo and go from there.
First, you run into the issue that copilot-agent/dist folder is empty, so you need to figure out how to populate that with the appropriate content. After that, the build fails with:

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:4.0.13:validate-classpath (default-validate-classpath) on project com.microsoft.copilot.eclipse.core: Execution default-validate-classpath of goal org.eclipse.tycho:tycho-compiler-plugin:4.0.13:validate-classpath failed: bundleLocation not found: <user-home>/.m2/repository/p2/osgi/bundle/com.ibm.icu.source/76.1.0/com.ibm.icu.source-76.1.0.jar for artifact eclipse-plugin:com.ibm.icu.source:76.1.0: <user-home>.m2/repository/p2/osgi/bundle/com.ibm.icu.source/76.1.0/com.ibm.icu.source-76.1.0.jar -> [Help 1]

Would be great if you could have a look or provide a snapshot repo location to be used instead.

jdneo · 2026-05-11T01:56:27Z

@martinlippert Have you tried to run npm i -f first?

See: https://github.com/microsoft/copilot-for-eclipse/blob/main/CONTRIBUTING.md#building-the-project

martinlippert · 2026-05-11T06:32:19Z

@martinlippert Have you tried to run npm i -f first?

See: https://github.com/microsoft/copilot-for-eclipse/blob/main/CONTRIBUTING.md#building-the-project

Yes, I did that, but the build still failed for me locally. Switching Tycho to 5.0.2 solved the issue for me locally. Probably worth to update the project to that version of Tycho.

martinlippert · 2026-05-11T06:38:28Z

@martinlippert Would you mind helping to verify if this patch fixes the issue?

Clone the code and switch to the branch cs/issue-153

Then run ./mvnw clean verify -DskipTests, the zip can be found at com.microsoft.copilot.eclipse.repository/target/com.microsoft.copilot.eclipse.repository-0.17.0-SNAPSHOT.zip

Since I can build the project locally now using Tycho 5.0.2, I updated the test environment and did some manual testing again. As a first step, I am running the test environment and went to the preferences for MCP servers.

When adding an MCP server config that works, everything seems to be fine.
When I change the existing MCP server config to something that doesn't work (e.g. by changing the port of the local MCP server) and click "Apply", this change doesn't have any effect. The MCP server list below still looks like the server works fine and there is no error reported.

When I remove the entire server definition from the preferences, click "Apply", and re-enter the server config, the preferences dialog behaves fine (reports an error if the config is wrong). However, changing the existing config doesn't seem to trigger the re-evaluation of the server config (even after clicking "Apply").

I haven't tried the same scenario with the extension point yet (again), but I can take a look at the extension point variant again once the preference dialog works. My assumption is that the underlying issue affects both (preference dialog and extension point).

Copilot AI review requested due to automatic review settings May 9, 2026 07:02

jdneo requested review from duzitong, ethanyhou and xinyi-gong as code owners May 9, 2026 07:02

Copilot started reviewing on behalf of jdneo May 9, 2026 07:03 View session

Copilot AI reviewed May 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: harden MCP extension-point registration against races and stale state#164

fix: harden MCP extension-point registration against races and stale state#164
jdneo wants to merge 1 commit into
mainfrom
cs/issue-153

jdneo commented May 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jdneo commented May 9, 2026

Uh oh!

martinlippert commented May 10, 2026

Uh oh!

jdneo commented May 11, 2026

Uh oh!

martinlippert commented May 11, 2026

Uh oh!

martinlippert commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		updateApprovedMcpServerString(extMcpInfoMap);
		persistExtMcpInfo(extMcpInfoMap);

Conversation

jdneo commented May 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

jdneo commented May 9, 2026

Uh oh!

martinlippert commented May 10, 2026

Uh oh!

jdneo commented May 11, 2026

Uh oh!

martinlippert commented May 11, 2026

Uh oh!

martinlippert commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants