Croway commented on code in PR #29741:
URL: https://github.com/apache/camel/pull/29741#discussion_r4554344406
##########
components/camel-ai/camel-mcp-server-api/src/test/java/org/apache/camel/component/mcp/server/conformance/McpServerConformanceTestSupport.java:
##########
@@ -102,7 +102,7 @@ public void configure() {
from("ai-tool:slow_tool?tags=" + CONFORMANCE_TAG +
"&description=Exceeds the tool timeout")
.routeId("slow-tool-route")
- .delay(TOOL_TIMEOUT_MILLIS * 3)
+ .delay(TOOL_TIMEOUT_MILLIS + 200)
Review Comment:
`TOOL_TIMEOUT_MILLIS + 200` leaves only 200 ms between the 2 s `future.get`
timeout in `McpServerBridge` and the route completing (it was 4 s). If the
waiting thread wakes more than ~200 ms late, `get` returns `done` and
`testCallToolTimeout` fails. A GC pause or a busy CI runner can cause that, and
with this PR the test classes also run concurrently. I could not make it flake
locally, so this is a risk, not a reproduced failure.
There is a variant that keeps the 4 s margin and is also faster: keep `* 3`
and make the delay synchronous.
```java
.delay(TOOL_TIMEOUT_MILLIS * 3).syncDelayed()
```
On timeout the bridge already calls `future.cancel(true)`. With a
synchronous delay, that interrupt ends the exchange at once
(`DelayProcessorSupport` lines 106-111), so nothing is inflight when the
context stops. With `+ 200` the exchange is still inflight at shutdown.
Graceful shutdown checks for inflight exchanges once per second
(`DefaultShutdownStrategy` line 739), so it still costs about 1 s per class.
Measured on the PR head with `-Dcamel.surefire.parallel=false` (3 runs each,
main 1 run); all tests pass in every variant:
| `slow_tool` / `slow_resource` delay | `VertxMcpServerConformanceTest` |
`VertxMcpServerResourceConformanceTest` |
|---|---|---|
| main: `* 3` | 7.7 s | 6.5 s |
| this PR: `+ 200` | 4.6–4.7 s | 3.5 s |
| `* 3` + `.syncDelayed()` | 3.5–3.7 s | 2.5 s |
The same change applies to `slow_resource` in
`McpServerResourceConformanceTestSupport` line 113.
##########
components/camel-ai/camel-mcp-server/src/test/java/org/apache/camel/component/mcp/server/vertx/VertxMcpSessionEvictionTest.java:
##########
@@ -49,60 +50,68 @@ class VertxMcpSessionEvictionTest {
@Test
void evictsIdleSessionWithoutDelete() throws Exception {
- int port = AvailablePortFinder.getNextAvailable();
- VertxMcpServerEngine engine = startEngine(port, new
McpServerInfo("eviction", "1.0", "/mcp", 0, 500));
- try {
- initializeSession(port);
- assertThat(engine.sessionCount()).isEqualTo(1);
-
- await().atMost(5, TimeUnit.SECONDS).untilAsserted(() ->
assertThat(engine.sessionCount()).isZero());
- } finally {
- engine.stop();
+ try (Port portHolder = AvailablePortFinder.find()) {
+ int port = portHolder.getPort();
+ VertxMcpServerEngine engine = startEngine(port, new
McpServerInfo("eviction", "1.0", "/mcp", 0, 500));
+ try {
+ initializeSession(port);
+ assertThat(engine.sessionCount()).isEqualTo(1);
+
+ await().atMost(5, TimeUnit.SECONDS).untilAsserted(() ->
assertThat(engine.sessionCount()).isZero());
+ } finally {
+ engine.stop();
+ }
}
}
@Test
void postOnlySessionSurvivesKeepAliveUntilIdle() throws Exception {
- int port = AvailablePortFinder.getNextAvailable();
- VertxMcpServerEngine engine = startEngine(port, new
McpServerInfo("eviction", "1.0", "/mcp", 300, 10_000));
- try {
- String sessionId = initializeSession(port);
-
- await().during(2, TimeUnit.SECONDS).atMost(3,
TimeUnit.SECONDS).untilAsserted(() -> {
- assertThat(postWithSession(port, sessionId,
INITIALIZED_NOTIFICATION)).isEqualTo(202);
- assertThat(engine.sessionCount()).isEqualTo(1);
- });
- } finally {
- engine.stop();
+ try (Port portHolder = AvailablePortFinder.find()) {
+ int port = portHolder.getPort();
+ VertxMcpServerEngine engine = startEngine(port, new
McpServerInfo("eviction", "1.0", "/mcp", 300, 10_000));
+ try {
+ String sessionId = initializeSession(port);
+
+ await().during(500, TimeUnit.MILLISECONDS).atMost(1500,
TimeUnit.MILLISECONDS).untilAsserted(() -> {
+ assertThat(postWithSession(port, sessionId,
INITIALIZED_NOTIFICATION)).isEqualTo(202);
+ assertThat(engine.sessionCount()).isEqualTo(1);
+ });
+ } finally {
+ engine.stop();
+ }
}
}
@Test
void activeSessionSurvivesIdleEviction() throws Exception {
- int port = AvailablePortFinder.getNextAvailable();
- VertxMcpServerEngine engine = startEngine(port, new
McpServerInfo("eviction", "1.0", "/mcp", 0, 2_000));
- try {
- String sessionId = initializeSession(port);
-
- await().during(2, TimeUnit.SECONDS).atMost(3,
TimeUnit.SECONDS).untilAsserted(() -> {
- assertThat(postWithSession(port, sessionId,
INITIALIZED_NOTIFICATION)).isEqualTo(202);
- assertThat(engine.sessionCount()).isEqualTo(1);
- });
- } finally {
- engine.stop();
+ try (Port portHolder = AvailablePortFinder.find()) {
+ int port = portHolder.getPort();
+ VertxMcpServerEngine engine = startEngine(port, new
McpServerInfo("eviction", "1.0", "/mcp", 0, 2_000));
+ try {
+ String sessionId = initializeSession(port);
+
+ await().during(500, TimeUnit.MILLISECONDS).atMost(1500,
TimeUnit.MILLISECONDS).untilAsserted(() -> {
Review Comment:
With the 500 ms window this test can no longer fail. The idle TTL is 2 s and
the eviction scan runs every `min(ttl / 2, 1000)` = 1 s, so a session that
nobody touches is still alive when the window closes, about 0.6 s after
`initialize`.
I checked it with a mutation that removes `managed.touch()` from the POST
path (`VertxMcpStreamableServerTransportProvider` line 472), so client activity
no longer keeps the session alive:
| Variant | Without the mutation | With the mutation |
|---|---|---|
| this PR: TTL 2 s, `during(500 ms)` | 5/5 pass (~0.67 s) | **5/5 pass**
(the real test too, 3/3) |
| main: TTL 2 s, `during(2 s)` | 5/5 pass (~2.15 s) | 1/5 fail |
| suggested: TTL 500 ms, `during(1500 ms)` | 5/5 pass (~1.65 s) | **5/5
fail** |
I suggest shrinking the TTL rather than the window. With a 500 ms TTL the
scan runs every 250 ms, so an untouched session is gone within ~750 ms, well
inside the window. The TTL is still 5× the 100 ms polling interval, so active
sessions are safe:
```java
VertxMcpServerEngine engine = startEngine(port, new
McpServerInfo("eviction", "1.0", "/mcp", 0, 500));
// ...
await().during(1500, TimeUnit.MILLISECONDS).atMost(2500,
TimeUnit.MILLISECONDS).untilAsserted(() -> {
```
That is ~1 s slower than this PR but still faster than main, and the test
checks the behaviour again.
##########
components/camel-ai/camel-mcp-server/pom.xml:
##########
@@ -144,6 +148,29 @@
</dependencies>
+ <build>
Review Comment:
The PR description says failsafe does not read the parallel property. In
fact `components/pom.xml` (profile `full`, lines 577-581) already sets
`junit.jupiter.execution.parallel.*` for failsafe from
`camel.failsafe.parallel`, and camel-ftp, camel-jms and camel-mina-sftp only
set that property. `configurationParameters` also take precedence over system
properties, so this block makes `-Dcamel.failsafe.parallel=false` a no-op. That
flag is useful to rule out parallelism when an IT flakes on CI.
I checked it with a probe IT that prints the thread JUnit runs it on. A
`ForkJoinPool-…` thread means parallel and `main` means serial:
| pom | `-Dcamel.failsafe.parallel=false` | Thread | Forked JVM
`junit.jupiter.execution.parallel.enabled` |
|---|---|---|---|
| this PR | – | `ForkJoinPool-1-worker-1` | `false` |
| this PR | yes | `ForkJoinPool-1-worker-1` | `false` |
| suggested (below) | – | `ForkJoinPool-1-worker-1` | `true` |
| suggested (below) | yes | `main` | `false` |
| main | – | `main` | `false` |
I suggest dropping the whole `<build>` block and setting the property next
to the surefire one:
```xml
<camel.surefire.parallel>true</camel.surefire.parallel>
<camel.failsafe.parallel>true</camel.failsafe.parallel>
```
That uses the failsafe default factor of 0.5. Add
`<camel.failsafe.parallel.factor>1</camel.failsafe.parallel.factor>` if you
want 1. Separately, `camel.surefire.parallel.factor=1` on line 43 is already
the default in `parent/pom.xml` (line 44), so that line can go.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
