You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "matteominin (via GitHub)" <gi...@apache.org> on 2026/07/10 13:43:21 UTC
[PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
matteominin opened a new pull request, #24587:
URL: https://github.com/apache/camel/pull/24587
# Description
`OpenAIEndpoint` kept its MCP tool state (`cachedMcpTools`, `toolClientMap`,
`returnDirectTools`, `toolToServerName`) in unsynchronized collections shared by all
concurrent exchanges. With `mcpReconnect=true` (default), `reconnectMcpServer` mutated
them in place while other exchanges read them — reachable in any multi-threaded route
hitting an MCP transport error. Three failures, each fixed:
1. **Reader corruption** (`ConcurrentModificationException` / half-rebuilt tool list) →
reconnect now publishes immutable snapshots (`List/Map/Set.copyOf`) swapped via
`volatile` references; readers stay lock-free.
2. **Duplicate reconnect** (concurrent reconnects close each other's clients, leak
transports) → serialized with a per-server `ReentrantLock` + re-check; only the first
thread reconnects, old client closed once.
3. **Stale entries** (`returnDirectTools` / `toolToServerName` kept vanished tools) →
pruned from all four collections on reconnect.
**Tests:** `OpenAIEndpointMcpReconnectConcurrencyTest`, one per issue. No user-visible
change → no upgrade-guide entry.
This description was drafted by Claude Code
# Target
- [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch)
# Tracking
- [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).
<!--
# *Note*: trivial changes like, typos, minor documentation fixes and other small items do not require a JIRA issue. In this case your pull request should address just this issue, without pulling in other changes.
-->
# Apache Camel coding standards and style
- [x] I checked that each commit in the pull request has a meaningful subject line and body.
<!--
If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
-->
- [x] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes.
<!--
You can run the aforementioned command in your module so that the build auto-formats your code. This will also be verified as part of the checks and your PR may be rejected if if there are uncommited changes after running `mvn clean install -DskipTests`.
You can learn more about the contribution guidelines at https://github.com/apache/camel/blob/main/CONTRIBUTING.md
-->
# AI-assisted contributions
- [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway commented on code in PR #24587:
URL: https://github.com/apache/camel/pull/24587#discussion_r3559450538
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIEndpoint.java:
##########
@@ -86,11 +88,12 @@ public class OpenAIEndpoint extends DefaultEndpoint {
private OpenAIClient client;
- private List<ChatCompletionFunctionTool> cachedMcpTools;
- private Map<String, McpSyncClient> toolClientMap;
- private Set<String> returnDirectTools;
+ private Map<String, ReentrantLock> mcpClientLocks;
Review Comment:
Minor: `mcpClientLocks` (and `serverConfigs`) are written in `doStart` and read from exchange threads but are not `volatile`, while the other four fields now are. Camel's service lifecycle provides the happens-before in practice, but since this PR is explicitly about visibility, making these `volatile` too keeps the reasoning uniform.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway merged PR #24587:
URL: https://github.com/apache/camel/pull/24587
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway commented on code in PR #24587:
URL: https://github.com/apache/camel/pull/24587#discussion_r3559450514
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIEndpoint.java:
##########
@@ -311,37 +351,37 @@ McpSyncClient reconnectMcpServer(String toolName) {
}
try {
- String transportType = props.get("transportType");
- McpClientTransport transport = createMcpTransport(serverName, transportType, props);
- Duration timeout = Duration.ofSeconds(configuration.getMcpTimeout());
- McpSyncClient newClient = McpClient.sync(transport)
- .requestTimeout(timeout)
- .initializationTimeout(timeout)
- .build();
- newClient.initialize();
-
- // Re-list tools and update mappings for all tools from this server
- McpSchema.ListToolsResult toolsResult = newClient.listTools();
- List<McpSchema.Tool> tools = toolsResult.tools();
+ McpSyncClient newClient = createMcpClient(serverName, props);
- // Remove old mappings for this server
- toolClientMap.entrySet().removeIf(e -> e.getValue() == oldClient);
- cachedMcpTools.removeIf(t -> {
- String name = t.function().name();
- return serverName.equals(toolToServerName.get(name));
- });
+ List<McpSchema.Tool> tools = newClient.listTools().tools();
- // Add new mappings
- cachedMcpTools.addAll(McpToolConverter.convert(tools));
- for (McpSchema.Tool tool : tools) {
- toolClientMap.put(tool.name(), newClient);
- toolToServerName.put(tool.name(), serverName);
+ // Rebuild shared tool state, replacing this server's old tools with the freshly listed ones
+ Set<String> oldServerTools = toolsForServer(serverName);
+ List<ChatCompletionFunctionTool> newTools = new ArrayList<>(cachedMcpTools);
Review Comment:
**Lost update across servers**: this read-copy-write of all four global collections runs under only the *per-server* lock. With two MCP servers configured, servers A and B can reconnect concurrently under different locks, both snapshot the same base state, and the last publish silently erases the other's update — leaving that server's tools mapped to its already-closed old client. Suggest one short global lock (or `synchronized` block) around this rebuild-and-publish section, keeping the per-server lock for the expensive close/connect/`listTools()` part.
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIEndpoint.java:
##########
@@ -311,37 +351,37 @@ McpSyncClient reconnectMcpServer(String toolName) {
}
try {
- String transportType = props.get("transportType");
- McpClientTransport transport = createMcpTransport(serverName, transportType, props);
- Duration timeout = Duration.ofSeconds(configuration.getMcpTimeout());
- McpSyncClient newClient = McpClient.sync(transport)
- .requestTimeout(timeout)
- .initializationTimeout(timeout)
- .build();
- newClient.initialize();
-
- // Re-list tools and update mappings for all tools from this server
- McpSchema.ListToolsResult toolsResult = newClient.listTools();
- List<McpSchema.Tool> tools = toolsResult.tools();
+ McpSyncClient newClient = createMcpClient(serverName, props);
- // Remove old mappings for this server
- toolClientMap.entrySet().removeIf(e -> e.getValue() == oldClient);
- cachedMcpTools.removeIf(t -> {
- String name = t.function().name();
- return serverName.equals(toolToServerName.get(name));
- });
+ List<McpSchema.Tool> tools = newClient.listTools().tools();
- // Add new mappings
- cachedMcpTools.addAll(McpToolConverter.convert(tools));
- for (McpSchema.Tool tool : tools) {
- toolClientMap.put(tool.name(), newClient);
- toolToServerName.put(tool.name(), serverName);
+ // Rebuild shared tool state, replacing this server's old tools with the freshly listed ones
+ Set<String> oldServerTools = toolsForServer(serverName);
+ List<ChatCompletionFunctionTool> newTools = new ArrayList<>(cachedMcpTools);
+ Map<String, McpSyncClient> newClientMap = new HashMap<>(toolClientMap);
+ Map<String, String> newToolToServer = new HashMap<>(toolToServerName);
+ Set<String> newReturnDirect = new HashSet<>(returnDirectTools);
- if (tool.annotations() != null && Boolean.TRUE.equals(tool.annotations().returnDirect())) {
- returnDirectTools.add(tool.name());
+ newTools.removeIf(t -> oldServerTools.contains(t.function().name()));
+ newClientMap.keySet().removeIf(oldServerTools::contains);
+ newToolToServer.keySet().removeIf(oldServerTools::contains);
+ newReturnDirect.removeIf(oldServerTools::contains);
+
+ newTools.addAll(McpToolConverter.convert(tools));
+ for (McpSchema.Tool tool : tools) {
+ newClientMap.put(tool.name(), newClient);
+ newToolToServer.put(tool.name(), serverName);
+ if (isReturnDirect(tool)) {
+ newReturnDirect.add(tool.name());
}
}
+ // Publish immutable snapshots for lock-free readers
+ cachedMcpTools = List.copyOf(newTools);
Review Comment:
**Torn reads**: these four volatile writes are not atomic as a group — a reader can observe the new `cachedMcpTools` with the old `toolClientMap` and hit `IllegalStateException("Tool ... not found")` in `OpenAIProducer` for a tool the model was legitimately offered. Consolidating the four collections into one immutable holder swapped via a single volatile reference eliminates this (and the lost-update issue above) in one refactor.
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIEndpoint.java:
##########
@@ -129,14 +132,13 @@ protected void doStop() throws Exception {
LOG.warn("Error closing MCP client: {}", e.getMessage(), e);
}
}
- toolClientMap.clear();
- }
- if (cachedMcpTools != null) {
- cachedMcpTools.clear();
- }
- if (returnDirectTools != null) {
- returnDirectTools.clear();
+ toolClientMap = null;
}
+
+ cachedMcpTools = null;
+ returnDirectTools = null;
+ toolToServerName = null;
Review Comment:
**Stop/reconnect race**: nulling `toolToServerName` means a reconnect racing with endpoint stop now NPEs at the first line of `reconnectMcpServer` (the old code degraded to the "cannot reconnect" warning). A reconnect already past the null checks can also publish fresh state and a new initialized client *after* `doStop` closed everything — a leaked transport. Consider acquiring the per-server locks here, or checking `isStopping()/isStopped()` inside the locked section. Minor: `mcpClientLocks` and `serverConfigs` are left non-null while everything else is nulled.
##########
components/camel-ai/camel-openai/src/test/java/org/apache/camel/component/openai/OpenAIEndpointMcpReconnectConcurrencyTest.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.openai;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
+
+import com.openai.models.chat.completions.ChatCompletionFunctionTool;
+import io.modelcontextprotocol.client.McpSyncClient;
+import io.modelcontextprotocol.spec.McpSchema;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Concurrency regression tests for CAMEL-23957: MCP reconnect must not corrupt the shared endpoint tool state.
+ */
+class OpenAIEndpointMcpReconnectConcurrencyTest {
+
+ private static final String SERVER = "test-server";
+ private static final List<String> TOOL_NAMES = List.of("get_weather", "find_location");
+
+ private final AtomicInteger clientsCreated = new AtomicInteger();
+
+ private volatile List<String> serverTools = TOOL_NAMES;
+
+ /**
+ * Overrides the client-creation seam so reconnects return mocks instead of opening real transports.
+ */
+ private class TestEndpoint extends OpenAIEndpoint {
+ TestEndpoint(OpenAIComponent component) {
+ super("openai:chat-completion", component, new OpenAIConfiguration());
+ }
+
+ @Override
+ McpSyncClient createMcpClient(String serverName, Map<String, String> props) {
+ clientsCreated.incrementAndGet();
+ return newMockClient();
+ }
+ }
+
+ private McpSyncClient newMockClient() {
+ McpSyncClient client = mock(McpSyncClient.class);
+ when(client.listTools()).thenReturn(McpSchema.ListToolsResult.builder(mockTools()).build());
+ return client;
+ }
+
+ private List<McpSchema.Tool> mockTools() {
+ return serverTools.stream()
+ .map(n -> McpSchema.Tool.builder(n, Map.of("type", "object")).description("mock " + n).build())
+ .toList();
+ }
+
+ private TestEndpoint newEndpoint(McpSyncClient initialClient) {
+ DefaultCamelContext ctx = new DefaultCamelContext();
+ OpenAIComponent component = new OpenAIComponent();
+ component.setCamelContext(ctx);
+ TestEndpoint endpoint = new TestEndpoint(component);
+ endpoint.setCamelContext(ctx);
+
+ endpoint.setMcpTools(new ArrayList<>(McpToolConverter.convert(mockTools())));
+
+ Map<String, McpSyncClient> clientMap = new ConcurrentHashMap<>();
+ Map<String, String> toolToServer = new ConcurrentHashMap<>();
+ for (String n : serverTools) {
+ clientMap.put(n, initialClient);
+ toolToServer.put(n, SERVER);
+ }
+ endpoint.setToolClientMap(clientMap);
+ endpoint.setToolToServerName(toolToServer);
+ endpoint.setReturnDirectTools(ConcurrentHashMap.newKeySet());
+
+ Map<String, Map<String, String>> serverConfigs = new ConcurrentHashMap<>();
+ serverConfigs.put(SERVER, Map.of("transportType", "stdio"));
+ endpoint.setServerConfigs(serverConfigs);
+
+ Map<String, ReentrantLock> locks = new ConcurrentHashMap<>();
+ locks.put(SERVER, new ReentrantLock());
+ endpoint.setMcpClientLocks(locks);
+
+ return endpoint;
+ }
+
+ @Test
+ void concurrentReadersAndReconnectsDoNotThrow() throws Exception {
Review Comment:
Coverage gap: all tests use a single server, so the cross-server lost-update race (see comment in `doReconnectMcpServer`) isn't exercised. A two-server variant of this test — reconnecting both servers concurrently and asserting each server's tools still map to a live client afterwards — would catch it. Nit: `assertEquals(reconnected, c)` on mocks relies on identity equality; `assertSame` states the intent directly.
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIEndpoint.java:
##########
@@ -86,11 +88,12 @@ public class OpenAIEndpoint extends DefaultEndpoint {
private OpenAIClient client;
- private List<ChatCompletionFunctionTool> cachedMcpTools;
- private Map<String, McpSyncClient> toolClientMap;
- private Set<String> returnDirectTools;
+ private Map<String, ReentrantLock> mcpClientLocks;
Review Comment:
Minor: `mcpClientLocks` (and `serverConfigs`) are written in `doStart` and read from exchange threads but are not `volatile`, while the other four fields now are. Camel's service lifecycle provides the happens-before in practice, but since this PR is explicitly about visibility, making these `volatile` too keeps the reasoning uniform.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #24587:
URL: https://github.com/apache/camel/pull/24587#discussion_r3559462024
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIEndpoint.java:
##########
@@ -129,14 +132,13 @@ protected void doStop() throws Exception {
LOG.warn("Error closing MCP client: {}", e.getMessage(), e);
}
}
- toolClientMap.clear();
- }
- if (cachedMcpTools != null) {
- cachedMcpTools.clear();
- }
- if (returnDirectTools != null) {
- returnDirectTools.clear();
+ toolClientMap = null;
Review Comment:
**Race condition during shutdown:** `doStop()` nulls `toolClientMap`, `cachedMcpTools`, `returnDirectTools`, and `toolToServerName` without acquiring the per-server locks. A concurrent `reconnectMcpServer` that has already acquired the lock and enters `doReconnectMcpServer` will NPE on `toolClientMap.get(toolName)` or `new HashMap<>(toolClientMap)` when the field has been nulled.
The `BaseService` lifecycle lock and the per-server `ReentrantLock` are independent — no ordering is enforced between `doStop` and the locked reconnect section. This can happen during Camel context shutdown if an in-flight exchange encounters a transport error and triggers reconnect while the context is stopping.
Consider either:
- Acquiring all per-server locks before nulling the fields in `doStop()`
- Adding null-guards inside `doReconnectMcpServer` for each volatile field read
- Using a `volatile boolean stopped` flag checked under the lock
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "matteominin (via GitHub)" <gi...@apache.org>.
matteominin commented on PR #24587:
URL: https://github.com/apache/camel/pull/24587#issuecomment-4938152612
Thanks @gnodet and @Croway for detailed review.
I addressed the reported issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24587:
URL: https://github.com/apache/camel/pull/24587#issuecomment-4938190254
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24587:
URL: https://github.com/apache/camel/pull/24587#issuecomment-4938424107
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `components/camel-ai/camel-openai`
---
<details><summary>:microscope: Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **38 affected modules** (current approach: 9).
<details><summary>:warning: Modules only in Scalpel (29)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-lucene`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-csimple-maven-plugin`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
**Skip-tests mode would test 9 modules** (1 direct + 8 downstream), **skip tests for 29** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (9)</summary>
- `camel-jbang-mcp`
- `camel-jbang-plugin-mcp`
- `camel-jbang-plugin-route-parser`
- `camel-jbang-plugin-tui`
- `camel-jbang-plugin-validate`
- `camel-launcher-container`
- `camel-openai`
- `camel-yaml-dsl-validator`
- `camel-yaml-dsl-validator-maven-plugin`
</details>
<details><summary>Modules with tests skipped (29)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-lucene`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-csimple-maven-plugin`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
> :information_source: Shadow mode — Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
:warning: **Some tests are disabled on GitHub Actions** (`@DisabledIfSystemProperty(named = "ci.env.name")`) and require manual verification:
- `components/camel-ai/camel-openai`: 6 test(s) disabled on GitHub Actions
<details><summary>All tested modules (38 modules)</summary>
- Camel :: AI :: OpenAI
- Camel :: All Components Sync point
- Camel :: Assembly
- Camel :: Catalog :: CSimple Maven Plugin (deprecated)
- Camel :: Catalog :: Camel Catalog
- Camel :: Catalog :: Camel Report Maven Plugin
- Camel :: Catalog :: Camel Route Parser
- Camel :: Catalog :: Console
- Camel :: Catalog :: Dummy Component
- Camel :: Catalog :: Lucene (deprecated)
- Camel :: Catalog :: Maven
- Camel :: Catalog :: Suggest
- Camel :: Component DSL
- Camel :: Coverage
- Camel :: Docs
- Camel :: Endpoint DSL
- Camel :: Endpoint DSL :: Support
- Camel :: Integration Tests
- Camel :: JBang :: Core
- Camel :: JBang :: Integration tests
- Camel :: JBang :: MCP
- Camel :: JBang :: Main
- Camel :: JBang :: Plugin :: Edit
- Camel :: JBang :: Plugin :: Generate
- Camel :: JBang :: Plugin :: Kubernetes
- Camel :: JBang :: Plugin :: MCP
- Camel :: JBang :: Plugin :: Route Parser
- Camel :: JBang :: Plugin :: TUI
- Camel :: JBang :: Plugin :: Testing
- Camel :: JBang :: Plugin :: Validate
- Camel :: Kamelet Main
- Camel :: Launcher
- Camel :: Launcher :: Container
- Camel :: YAML DSL
- Camel :: YAML DSL :: Deserializers
- Camel :: YAML DSL :: Maven Plugins
- Camel :: YAML DSL :: Validator
- Camel :: YAML DSL :: Validator Maven Plugin
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/29112973781)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-23957: Synchronize MCP reconnect to protect shared endpoint tool state [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway commented on code in PR #24587:
URL: https://github.com/apache/camel/pull/24587#discussion_r3559450546
##########
components/camel-ai/camel-openai/src/test/java/org/apache/camel/component/openai/OpenAIEndpointMcpReconnectConcurrencyTest.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.openai;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
+
+import com.openai.models.chat.completions.ChatCompletionFunctionTool;
+import io.modelcontextprotocol.client.McpSyncClient;
+import io.modelcontextprotocol.spec.McpSchema;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Concurrency regression tests for CAMEL-23957: MCP reconnect must not corrupt the shared endpoint tool state.
+ */
+class OpenAIEndpointMcpReconnectConcurrencyTest {
+
+ private static final String SERVER = "test-server";
+ private static final List<String> TOOL_NAMES = List.of("get_weather", "find_location");
+
+ private final AtomicInteger clientsCreated = new AtomicInteger();
+
+ private volatile List<String> serverTools = TOOL_NAMES;
+
+ /**
+ * Overrides the client-creation seam so reconnects return mocks instead of opening real transports.
+ */
+ private class TestEndpoint extends OpenAIEndpoint {
+ TestEndpoint(OpenAIComponent component) {
+ super("openai:chat-completion", component, new OpenAIConfiguration());
+ }
+
+ @Override
+ McpSyncClient createMcpClient(String serverName, Map<String, String> props) {
+ clientsCreated.incrementAndGet();
+ return newMockClient();
+ }
+ }
+
+ private McpSyncClient newMockClient() {
+ McpSyncClient client = mock(McpSyncClient.class);
+ when(client.listTools()).thenReturn(McpSchema.ListToolsResult.builder(mockTools()).build());
+ return client;
+ }
+
+ private List<McpSchema.Tool> mockTools() {
+ return serverTools.stream()
+ .map(n -> McpSchema.Tool.builder(n, Map.of("type", "object")).description("mock " + n).build())
+ .toList();
+ }
+
+ private TestEndpoint newEndpoint(McpSyncClient initialClient) {
+ DefaultCamelContext ctx = new DefaultCamelContext();
+ OpenAIComponent component = new OpenAIComponent();
+ component.setCamelContext(ctx);
+ TestEndpoint endpoint = new TestEndpoint(component);
+ endpoint.setCamelContext(ctx);
+
+ endpoint.setMcpTools(new ArrayList<>(McpToolConverter.convert(mockTools())));
+
+ Map<String, McpSyncClient> clientMap = new ConcurrentHashMap<>();
+ Map<String, String> toolToServer = new ConcurrentHashMap<>();
+ for (String n : serverTools) {
+ clientMap.put(n, initialClient);
+ toolToServer.put(n, SERVER);
+ }
+ endpoint.setToolClientMap(clientMap);
+ endpoint.setToolToServerName(toolToServer);
+ endpoint.setReturnDirectTools(ConcurrentHashMap.newKeySet());
+
+ Map<String, Map<String, String>> serverConfigs = new ConcurrentHashMap<>();
+ serverConfigs.put(SERVER, Map.of("transportType", "stdio"));
+ endpoint.setServerConfigs(serverConfigs);
+
+ Map<String, ReentrantLock> locks = new ConcurrentHashMap<>();
+ locks.put(SERVER, new ReentrantLock());
+ endpoint.setMcpClientLocks(locks);
+
+ return endpoint;
+ }
+
+ @Test
+ void concurrentReadersAndReconnectsDoNotThrow() throws Exception {
Review Comment:
Coverage gap: all tests use a single server, so the cross-server lost-update race (see comment in `doReconnectMcpServer`) isn't exercised. A two-server variant of this test — reconnecting both servers concurrently and asserting each server's tools still map to a live client afterwards — would catch it. Nit: `assertEquals(reconnected, c)` on mocks relies on identity equality; `assertSame` states the intent directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org