feat(nacos-mcp): support dynamic MCP server discovery and load balancing from Nacos registry - #3160
Leslie-ZH1023 wants to merge 6 commits into
Conversation
…from Nacos registry Introduce a new agentscope-extensions-nacos-mcp module that subscribes MCP servers from the Nacos MCP registry (Nacos 3.x push-based AiService.subscribeMcpServer) instead of static configuration only: - NacosMcpDiscoveryClient: subscribes registry events and resolves backend endpoints from McpServerDetailInfo - NacosLoadBalancedMcpClientWrapper: an McpClientWrapper that keeps one connection per endpoint, reconciles them on endpoint scale in/out and rebuilds connections when the transport changes (SSE <-> Streamable HTTP) - Pluggable EndpointSelector: round-robin (default) and sticky (session affinity for stateful MCP servers) - NacosMcpClients holder to register discovered clients into the Toolkit an agent actually holds, composing with static tools.json servers Also adds Spring Boot autoconfiguration in agentscope-nacos-spring-boot-starter (agentscope.nacos.mcp.connections.*) with unit tests for the module and the autoconfiguration. Refs agentscope-ai#2938
|
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Well-scoped, genuinely additive integration: a new agentscope-extensions-nacos-mcp module (discovery client + endpoint model + pluggable selector + load-balanced McpClientWrapper), Spring Boot autoconfiguration behind agentscope.nacos.mcp.enabled, BOM/agentscope-all registration and EN/ZH docs. Nothing here touches agentscope-core interfaces, so there is no cascade to harness/distribution/extensions. JavaDoc is thorough and the pure components (NacosMcpEndpoint, both selectors, the NacosMcpClients holder) are covered by unit tests, and build (ubuntu-latest) / build (windows-latest) / validate / Check Module Sync are all green.
The weak spot is the dynamic part — connection reconciliation. The race conditions and blocking calls cluster in NacosLoadBalancedMcpClientWrapper, which is also the only substantial class with no unit test, and one documented behaviour (tool hot-update) does not match what the Toolkit registration path actually does.
Findings
Inline comments carry the details. In priority order:
- [Critical]
NacosMcpClients:106—registerTo()snapshots tools once, so a connection with no endpoints at startup registers zero tools permanently; the "waiting for endpoints to be pushed" / "picked up once endpoints arrive" wording overstates what happens. - [Critical]
NacosLoadBalancedMcpClientWrapper:165—initialize()and thesynchronizedupdateEndpoints()are not on one monitor, so a push that arrives during subscribe can be clobbered by the stale initial snapshot. - [Critical]
NacosLoadBalancedMcpClientWrapper:336— check-then-put inaddEndpoints()can open two connections for one key; the loser is overwritten inendpointClientsand never closed. - [Critical]
NacosLoadBalancedMcpClientWrapper:227— the selected endpoint is not checked for connectivity, so one unhealthy instance fails the wholecallTooleven with healthy connections available (contradicts the wrapper's own and the docs' "one broken connection does not block the others"). - [Critical]
NacosLoadBalancedMcpClientWrapper:321—.block()on the Nacos push callback thread stalls event delivery for the whole client, and throws on Reactor non-blocking threads, which can leave the wrapper with zero connections after the protocol-change rebuild. - [Warning]
NacosLoadBalancedMcpClientWrapper:254—close()vs in-flightinitialize()can re-subscribe a closed wrapper and orphan its connections;subscribedis also notvolatile. - [Warning]
NacosMcpEndpoint:60—key()omits the scheme, so anhttp→httpsflip on the same address/port/path keeps the stale connection;:48—url()needs a normalized leading slash for operator-supplied export paths. - [Warning]
AgentscopeMcpNacosAutoConfiguration:152— the@Beanmethod mutates the config instance's field, so a user-suppliedNacosMcpClientsbean backs off and its wrappers are never closed;:150— missingservice-nameis only caught deep insidesubscribe()and swallowed byregisterTo's catch. - [Warning]
NacosLoadBalancedMcpClientWrapper:69— no unit test for this class, which is whatcodecov/patchis failing on (34.57% of diff hit vs 60% target).
Info
AgentScopeMcpNacosProperties:52has noenabledfield although every other gated properties class here declares one (AgentScopeNacosPromptProperties,A2aCommonProperties,NacosA2aDiscoveryProperties). Unknown fields are ignored by default so nothing breaks, but adding it keeps the flag readable programmatically and consistent.StickyEndpointSelectoris documented as session affinity but pins all calls of one wrapper to one instance — the javadoc already says so, so consider wording the docs table ("Prefers one instance") as "one instance per wrapper" to avoid over-promising per-session stickiness.
Verdict
Not approving in this round: the reconciliation races and the tool-snapshot semantics are worth resolving (or explicitly documenting) first, and codecov/patch is currently red. The overall design is sound and the module layout follows the existing nacos sub-modules, so targeted fixes should be enough — @mention me after the next push for a re-review.
Automated review by github-manager-bot
| McpServerDetailInfo detailInfo = | ||
| discoveryClient.subscribe(serverName, version, listener); | ||
| subscribed = true; | ||
| if (detailInfo == null) { | ||
| logger.warn( | ||
| "No MCP server '{}' found in Nacos during initialization," | ||
| + " waiting for endpoints to be pushed", | ||
| serverName); | ||
| return Collections.<NacosMcpEndpoint>emptyList(); | ||
| } | ||
| currentProtocol = detailInfo.getProtocol(); | ||
| return NacosMcpDiscoveryClient.resolveEndpoints(detailInfo); | ||
| }) | ||
| .flatMap( | ||
| endpoints -> | ||
| addEndpoints(endpoints) | ||
| .doOnSuccess(v -> currentEndpoints = endpoints)) |
There was a problem hiding this comment.
[Critical] subscribe() registers listener before the initial snapshot is reconciled, and initialize() is not on the same monitor as updateEndpoints() (which is synchronized). Nacos can deliver onEvent on the subscribe call, so updateEndpoints() may already have set currentEndpoints to a newer list when doOnSuccess(v -> currentEndpoints = endpoints) here overwrites it with the older initial snapshot — newly pushed endpoints become unreachable until the next push.
Suggest routing the initial result through the same synchronized reconcile path (e.g. Mono.defer(() -> Mono.fromCallable(() -> reconcile(resolveEndpoints(detailInfo), detailInfo.getProtocol())))) and dropping the blind currentEndpoints write here.
| private Mono<Void> addEndpoints(List<NacosMcpEndpoint> endpoints) { | ||
| return Flux.fromIterable(endpoints) | ||
| .filter(endpoint -> !endpointClients.containsKey(endpoint.key())) | ||
| .flatMap( | ||
| endpoint -> | ||
| createEndpointClient(endpoint) | ||
| .flatMap(client -> client.initialize().thenReturn(client)) | ||
| .doOnNext( | ||
| client -> | ||
| endpointClients.put(endpoint.key(), client)) | ||
| .onErrorResume( |
There was a problem hiding this comment.
[Critical] The !endpointClients.containsKey(endpoint.key()) filter and the endpointClients.put(...) in doOnNext are not atomic, and addEndpoints() is reachable both from initialize() (unsynchronized) and from updateEndpoints() (synchronized). Two overlapping passes can both pass the filter for the same key and both open a connection; the second put then overwrites the first entry and the overwritten McpClientWrapper is never closed (closeAllEndpointClients()/close() only see map values) — a leaked MCP connection per collision. Duplicate keys inside one newEndpoints list hit the same path.
Suggest de-duplicating the input by key(), connecting through computeIfAbsent-style per-key single flight, and closing any client that loses the race.
| } | ||
|
|
||
| if (!toAdd.isEmpty()) { | ||
| logger.info( | ||
| "Adding {} new endpoint(s) for MCP server '{}': {}", | ||
| toAdd.size(), | ||
| serverName, | ||
| toAdd); | ||
| addEndpoints(toAdd).block(); |
There was a problem hiding this comment.
[Critical] addEndpoints(...).block() runs on the Nacos push callback thread (onEvent → updateEndpoints), so connection setup with a 30s initialization timeout blocks the subscriber dispatcher: a slow or unreachable instance stalls event delivery for every other MCP server on that Nacos client. And if the push thread is ever a Reactor non-blocking thread, .block() throws IllegalStateException, the reconcile is aborted by the catch in onEvent, and — because the protocol-change branch above has already called closeAllEndpointClients() and cleared currentEndpoints — the wrapper is left with zero connections.
Suggest handing reconciliation to a dedicated single-thread executor / Schedulers.boundedElastic() and letting synchronized serialize it.
| new IllegalStateException( | ||
| "No endpoint available for MCP server '" + serverName + "'")); | ||
| } | ||
|
|
||
| NacosMcpEndpoint endpoint = endpointSelector.select(endpoints); | ||
| McpClientWrapper client = endpointClients.get(endpoint.key()); | ||
| if (client == null || !client.isInitialized()) { | ||
| return Mono.error( | ||
| new IllegalStateException( | ||
| "Selected endpoint '" + endpoint + "' is not connected")); | ||
| } | ||
|
|
||
| logger.debug( | ||
| "Calling MCP tool '{}' on client '{}', endpoint '{}'", toolName, name, endpoint); | ||
| return client.callTool(toolName, arguments, meta); |
There was a problem hiding this comment.
[Critical] A single unhealthy instance breaks the hot path: the selector only checks membership in currentEndpoints, not connectivity, so if RoundRobinEndpointSelector lands on an endpoint whose client is missing / uninitialized / broken, the call fails hard with IllegalStateException even though healthy connections exist. That contradicts the promise in NacosMcpClients#registerTo ("one broken connection does not block the others") and in the docs ("a failing connection does not block the others"), and it makes round-robin unusable during a rolling restart of the MCP server.
Suggest trying the remaining candidates (bounded fan-out over the unconnected selected endpoint) before erroring, and dropping endpoints that fail isInitialized() from the candidate list in select().
| subscribed = false; | ||
| } | ||
| endpointClients.values().forEach(McpClientWrapper::close); | ||
| endpointClients.clear(); | ||
| currentEndpoints = Collections.emptyList(); | ||
| initialized = false; | ||
| cachedTools.clear(); | ||
| logger.info("Closed Nacos load-balanced MCP client '{}'", name); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the endpoints currently connected by this wrapper. | ||
| * | ||
| * @return an unmodifiable view of the current endpoints |
There was a problem hiding this comment.
[Warning] close() is not on the same monitor as initialize(), and neither subscribed nor initialized is guarded. close() racing an in-flight initialize() sets initialized = false and then the still-running doOnSuccess re-subscribes and sets initialized = true, leaving a wrapper that looks closed but holds a live Nacos subscription plus orphaned per-endpoint connections that nothing will ever close.
Suggest a volatile boolean closed checked before subscribe(), close() synchronized on the same lock, and subscribed made volatile (it is written on the callable thread and read from the Spring shutdown thread).
| public NacosMcpClients nacosMcpClients( | ||
| NacosMcpDiscoveryClient discoveryClient, | ||
| AgentScopeMcpNacosProperties mcpNacosProperties) { | ||
| for (Map.Entry<String, NacosMcpConnectionProperties> entry : | ||
| mcpNacosProperties.getConnections().entrySet()) { | ||
| String clientName = entry.getKey(); | ||
| NacosMcpConnectionProperties connection = entry.getValue(); | ||
| NacosMcpConnectionProperties.LoadBalanceStrategy strategy = | ||
| connection.getLoadBalance() != null | ||
| ? connection.getLoadBalance() | ||
| : mcpNacosProperties.getLoadBalance(); | ||
| EndpointSelector selector = | ||
| strategy == NacosMcpConnectionProperties.LoadBalanceStrategy.STICKY | ||
| ? new StickyEndpointSelector() | ||
| : new RoundRobinEndpointSelector(); | ||
| mcpClients.add( | ||
| NacosLoadBalancedMcpClientWrapper.builder(clientName) | ||
| .serverName(connection.getServiceName()) | ||
| .version(connection.getVersion()) | ||
| .discoveryClient(discoveryClient) | ||
| .endpointSelector(selector) | ||
| .build()); | ||
| } | ||
| return new NacosMcpClients(mcpClients); |
There was a problem hiding this comment.
[Warning] The @Bean method mutates the config-instance field mcpClients and hands that same list to the holder, and cleanup relies on @PreDestroy of this class. Two consequences: (1) when a user supplies their own NacosMcpClients (the shouldBackOffWhenUserProvidesCustomNacosMcpClients scenario), this method never runs, mcpClients stays empty, and close() only shuts down the AiService — the wrappers' subscriptions and connections are never closed; (2) any second invocation duplicates entries.
Suggest building the list in a local variable, returning new NacosMcpClients(list), and letting Spring destroy the NacosMcpClients bean itself (it is already AutoCloseable).
| connection.getLoadBalance() != null | ||
| ? connection.getLoadBalance() | ||
| : mcpNacosProperties.getLoadBalance(); | ||
| EndpointSelector selector = | ||
| strategy == NacosMcpConnectionProperties.LoadBalanceStrategy.STICKY | ||
| ? new StickyEndpointSelector() | ||
| : new RoundRobinEndpointSelector(); | ||
| mcpClients.add( | ||
| NacosLoadBalancedMcpClientWrapper.builder(clientName) | ||
| .serverName(connection.getServiceName()) | ||
| .version(connection.getVersion()) | ||
| .discoveryClient(discoveryClient) | ||
| .endpointSelector(selector) | ||
| .build()); |
There was a problem hiding this comment.
[Warning] No validation that connection.getServiceName() is non-blank. A YAML typo (service_name:, or a missing key) yields a wrapper with serverName == null, which fails deep inside subscribe() at first registration, gets swallowed by the catch (Exception e) in registerTo, and is logged as a generic "Failed to register Nacos MCP client" — the operator has no idea which connection key is misconfigured. Builder.build() validates, but this path bypasses it.
Suggest validating each connection here so the context fails fast with the connection key in the message.
|
|
||
| private final NacosMcpDiscoveryClient discoveryClient; |
There was a problem hiding this comment.
[Warning] This class (498 lines) is the heart of the feature but has no test file, which is what the failing codecov/patch check is reporting (34.57% diff hit vs the 60% target). The endpoints/selector tests cover pure functions only.
A NacosLoadBalancedMcpClientWrapperTest with a stubbed NacosMcpDiscoveryClient would also let you pin the behaviours flagged above: init with zero endpoints, failover when the selected endpoint is down, reconcile add/remove, and double-close.
| * }</pre> | ||
| */ | ||
| @ConfigurationProperties(prefix = NacosConstants.NACOS_MCP_PREFIX) | ||
| public class AgentScopeMcpNacosProperties extends BaseNacosProperties { |
There was a problem hiding this comment.
[Info] No enabled field here, although every other enabled-gated properties class in this starter declares one (AgentScopeNacosPromptProperties, NacosA2aDiscoveryProperties, A2aCommonProperties). Unknown fields are ignored by default so binding does not break, but adding private boolean enabled = false; keeps the flag readable programmatically and consistent with the rest of the starter.
| * {@link NacosLoadBalancedMcpClientWrapper}. All tool calls issued through one wrapper stick to | ||
| * one backend instance. | ||
| */ | ||
| public class StickyEndpointSelector implements EndpointSelector { |
There was a problem hiding this comment.
[Info] Worth double-checking the naming against expectations: stickiness is per selector instance (i.e. per wrapper), so this is really "pin one backend per logical MCP client", not per-session affinity — the javadoc says so, but the docs table ("Prefers one instance; suits stateful servers") and the EndpointSelector javadoc ("consecutive calls of one session should stick to one instance") read like per-session affinity. Consider aligning the wording, or keying by an external session id if per-session affinity is the goal.
Adds unit tests for the two uncovered classes of the new Nacos MCP module, which brought patch coverage below the Codecov 60% gate. Endpoint resolution rules (backend/frontend fallback, export path, scheme inference) and the registry subscription overloads are covered directly. The load-balanced wrapper is exercised through a closed loopback port so the MCP transport really fails, covering endpoint bookkeeping, selection, failure handling and the Nacos push listener. New-code coverage of the module moves from 22% to 90% (wrapper 0% -> 93%, discovery client 0% -> 93%).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of 27f7b979..b7643347. The increment is test-only: NacosMcpDiscoveryClientTest (311 lines) and NacosLoadBalancedMcpClientWrapperTest (494 lines), no production changes. That is the right instinct — the wrapper was the one substantial class with no coverage last round — and NacosMcpDiscoveryClientTest in particular is genuinely useful: the version/no-version subscription routing, exception propagation, backend-over-frontend precedence, exportPath precedence and the 443/8443 https inference are all pinned down now. build (ubuntu-latest) / build (windows-latest) / validate / Check Module Sync are green and the CLA is signed.
The four Critical findings from the previous round are still open, since nothing in NacosLoadBalancedMcpClientWrapper / NacosMcpClients changed: the one-shot tool snapshot in registerTo(), initialize() racing the synchronized updateEndpoints() on different monitors, the check-then-put in addEndpoints() that can leak an unclosed connection, and callTool not checking the selected endpoint's connectivity before failing.
The important caveat on this increment: the new wrapper tests don't actually reach the code those findings live in. Every fixture uses an endpoint that fails to connect, so endpointClients stays empty for the whole suite. That makes the reconciliation assertions (shouldAddEndpointsOnPush, shouldDropRemovedEndpoints, shouldRebuildOnProtocolChange) pass on the registry-side bookkeeping alone — they would still pass if closeAllEndpointClients() and addEndpoints() were no-ops — and it means close()-on-scale-in, one-client-per-endpoint, and the failover path are all still unexercised. A stub McpClientWrapper behind a small factory seam is what turns these into tests of the thing that can actually break. Inline comments point at the specific lines, including the loopback-vs-10.0.0.1 mismatch, which is a portability/flakiness risk on routed CI networks rather than a correctness bug.
Not approving on this round: not because of the tests, but because the concurrency findings above are unresolved. Happy to re-review as soon as they land — @Leslie-ZH1023, the test seam suggested here would also be the natural way to prove the initialize()/updateEndpoints() fix.
Automated review by github-manager-bot
| private static McpEndpointInfo unreachableEndpoint(String address) { | ||
| McpEndpointInfo info = new McpEndpointInfo(); | ||
| info.setAddress(address); | ||
| // port 1 on loopback refuses connections immediately |
There was a problem hiding this comment.
// port 1 on loopback refuses connections immediately — but the addresses passed to this helper are 10.0.0.1 / 10.0.0.2, which are not loopback. On a runner where 10.0.0.0/8 is routed (common in CI and Docker networks) the connect attempt can be black-holed by the router instead of refused, so the failure is bounded by initializationTimeout(200ms) rather than being immediate, and on a self-hosted runner where 10.0.0.1 is a live gateway you are making a real connection attempt against infrastructure.
Deterministic alternative: bind an ephemeral ServerSocket, read its port, close it, and connect to 127.0.0.1:<port> — that guarantees ECONNREFUSED on every platform:
private static McpEndpointInfo unreachableEndpoint() throws IOException {
try (ServerSocket probe = new ServerSocket(0)) {
int port = probe.getLocalPort(); // closed as soon as the try-with-resources exits
McpEndpointInfo info = new McpEndpointInfo();
info.setAddress("127.0.0.1");
info.setPort(port);
info.setPath("/mcp");
return info;
}
}Also worth aligning the class-level javadoc at line 52, which says "a closed loopback port".
| unreachableEndpoint("10.0.0.1"), | ||
| unreachableEndpoint("10.0.0.2")))); | ||
|
|
||
| assertEquals(2, wrapper.getCurrentEndpoints().size()); |
There was a problem hiding this comment.
This assertion can't fail for the reason the test name implies. getCurrentEndpoints() returns currentEndpoints, which updateEndpoints() assigns from the registry snapshot — not from endpointClients. So shouldRebuildOnProtocolChange, shouldAddEndpointsOnPush (line 353) and shouldDropRemovedEndpoints (line 397) all pass even if closeAllEndpointClients() and addEndpoints(...) did nothing at all.
The reconciliation logic is precisely the part that was untested before this commit, and it is still the part the tests don't observe. To make these meaningful the test needs visibility into endpointClients, e.g. the smallest seam being a package-private constructor parameter for an endpoint-client factory:
// production
NacosLoadBalancedMcpClientWrapper(Builder builder, Function<NacosMcpEndpoint, Mono<McpClientWrapper>> clientFactory)
// test
AtomicReference<List<McpClientWrapper>> created = new AtomicReference<>();
wrapper = new NacosLoadBalancedMcpClientWrapper(builder, ep -> Mono.just(stubClient(ep)));
...
verify(stubFor("10.0.0.1"), times(1)).close(); // scale-in actually closed the connectionWithout that, a stub/fake McpClientWrapper that reports isInitialized() == true would let you assert: one client per endpoint after a scale-out, close() on scale-in, and all-connections-rebuilt after an SSE ↔ Streamable HTTP switch. Those are the behaviours the previous round flagged as risky.
|
|
||
| @Test | ||
| @DisplayName("Should fail the tool call when the selected endpoint is not connected") | ||
| void shouldFailCallToolWhenSelectedEndpointNotConnected() throws NacosException { |
There was a problem hiding this comment.
shouldFailCallToolWhenSelectedEndpointNotConnected asserts IllegalStateException, which is correct for the fixture as written — both endpoints are unreachable, so there is nothing to fail over to. The problem is that it locks in the contract flagged last round without distinguishing it: the interesting case is one healthy endpoint + one dead endpoint, where a round-robin selection landing on the dead one currently fails the whole callTool instead of retrying on the healthy connection.
Could you add that case (it needs the stub-client seam above)? If the intended contract really is "no failover, the selector must be health-aware", then the wrapper's javadoc should say so explicitly, because "load balanced" plus endpoint is not connected, it will be retried on next Nacos push currently reads as though a healthy instance would absorb the traffic.
| } | ||
|
|
||
| /** | ||
| * Returns the endpoints currently connected by this wrapper. |
There was a problem hiding this comment.
"Returns the endpoints currently connected by this wrapper" — it does not. initialize() sets currentEndpoints = endpoints from resolveEndpoints(detailInfo) after addEndpoints(...) completes, and addEndpoints swallows every per-endpoint connection failure via onErrorResume, so currentEndpoints is the registry view and can be non-empty while endpointClients is empty.
The new shouldTrackPushedEndpoints test asserts exactly that mismatch (2 endpoints returned with zero connections), which is fine as a behavioural pin but makes the javadoc wrong. This is also what makes callTool throw "Selected endpoint ... is not connected" instead of picking a connected one: the selector chooses over currentEndpoints, then the map lookup fails. Suggest either renaming the accessor to getRegisteredEndpoints() / fixing the javadoc, or filtering by endpointClients.containsKey(...) && isInitialized().
| wrapper.initialize().block(); | ||
|
|
||
| List<NacosMcpEndpoint> endpoints = wrapper.getCurrentEndpoints(); | ||
| assertEquals(2, endpoints.size()); |
There was a problem hiding this comment.
Positional assertions on getCurrentEndpoints() couple the test to registry list order. addEndpoints() fans out through Flux.flatMap and endpointClients is a ConcurrentHashMap, so list order is the only thing keeping get(0) == 10.0.0.1 true today. Asserting on endpoint.key() as a set (Set.copyOf(endpoints.stream().map(NacosMcpEndpoint::key).toList())) keeps the same coverage without the order dependency.
| void shouldFallBackToFrontendWhenBackendAbsent() { | ||
| McpServerDetailInfo detail = detail(null, List.of(endpoint("10.0.0.9", 9090))); | ||
|
|
||
| List<NacosMcpEndpoint> endpoints = NacosMcpDiscoveryClient.resolveEndpoints(detail); |
There was a problem hiding this comment.
shouldSkipUnusableEndpointEntries covers null and address-less entries — the remaining branch in resolveEndpoints that is cheap to hit and easy to get wrong is a non-null address with an invalid port (<= 0 or > 65535), which would otherwise produce a URL like http://10.0.0.1:0/mcp and fail later inside the transport. Worth one more entry in the same Arrays.asList(...) if the production code guards it; if it does not guard it, that is a finding on its own.
|
Hi maintainers, this PR is ready for review — CI is all green and unit tests cover the new module and the starter autoconfiguration. It implements the design discussed in #2938 (with the registration-path refinement noted in my latest comment there). One more piece of context: our team at Taikang Insurance Group (泰康保险集团) is already using AgentScope-Java internally, and dynamic MCP server discovery with client-side load balancing from the Nacos registry is exactly the capability we are missing — which is why we built this and are contributing it back. Landing it upstream would let us drop our local snapshot build, so we'd really appreciate a review when you get a chance. Happy to address any feedback. Thanks! |
|
恳请尽快合并。 |
|
Thanks for the ping, and for the patience. Status check so this is not left ambiguous:
Next step: push fixes and resolve the threads (or reply where you disagree — some of these are design tradeoffs, and a "no, intended behaviour" answer is a perfectly good outcome). I will re-review as soon as a new commit lands. Automated review by github-manager-bot |
…calls The load-balanced wrapper mutated its endpoint connections from the Nacos dispatcher thread, the initialization path and the reconcile path without a common monitor, which could race connection setup and leak duplicate clients. Apply every registry snapshot on a single reconcile thread behind one lock, claim endpoint slots atomically, retry endpoints whose handshake failed, and fail over to the remaining connected endpoints when a tool call fails. Drop the one-shot registerTo helper so callers pick the connection they need from NacosMcpClients, and fix endpoint key/path handling, port validation, the service-name check and the Spring shutdown wiring.
…pper awaitConnectedEndpoint and its monitor only existed to serve NacosMcpClients#registerTo, which has been removed in favour of letting callers pick a connection with get(name). Nothing in production calls it anymore, so remove the wait, the monitor and the now-redundant signalEndpointReady calls, and update the docs to say plainly that a Toolkit registration is a snapshot.
|
@oss-maintainer All review threads are addressed. Critical
Warnings / Info
Tests
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of b7643347..41f4a75c (two new commits: 49d4c92b serialize endpoint reconciliation + fail over tool calls, 41f4a75c drop the unused endpoint-ready wait). This closes all five Criticals and the main Warnings from the previous round, and does it structurally rather than with patches: one single-thread reconciler plus a ReconcileLock now owns every mutation of endpointClients/registeredEndpoints, pushApplied orders the initial snapshot against a concurrent push, putIfAbsent + closeQuietly removes the leaked-connection race, .block() is off the Nacos dispatcher thread, callTool dispatches only to connected candidates, key() includes the scheme, and the export path is normalized. The EndpointClientFactory seam is exactly what the tests needed — endpointClients is now asserted through real stub clients (one per endpoint, closed on scale-in, rebuilt on protocol change) instead of through the registry snapshot, and the ephemeral ServerSocket replaces the 10.0.0.1 "loopback" fixture. registerTo is gone, getRegisteredEndpoints() / getConnectedEndpointCount() split the two things getCurrentEndpoints() conflated, and both docs pages say the tool-snapshot truth. license/cla is signed, Check License / Check Module Sync / validate are green; build (ubuntu-latest) and build (windows-latest) were still in_progress on the new head, so I am posting as COMMENT rather than approving until those close.
Findings
- [Warning]
NacosLoadBalancedMcpClientWrapper.java:324— fail-over replays a tool call on the next endpoint for any error, which makes side-effecting tools at-least-once and amplifies deterministic failures. Scope the retry to connection-level errors, or gate it and state the semantics. - [Warning]
:501—reconcile()blocks on per-endpoint connects (create().block(),initialize().block(), 30s default init timeout) while holdingreconcileLock, soclose()and later snapshots wait behind the slowest endpoint; the push queue is also unbounded, so stale snapshots get replayed serially. - [Info]
:128— the previoustry/catcharoundupdateEndpointsis gone and the push-pathCompletableFutureis dropped, so a throwing reconcile now fails silently and the endpoint set just stays stale. - [Info]
:311— the aggregated "All N connected endpoint(s) … failed"IllegalStateExceptioncarries no cause; the reasons live only in WARN lines from earlier in the same call. - [Info]
:536— "retried on the next Nacos push" needs a push to arrive; there is no periodic re-reconcile or on-demand reconnect, so an endpoint unreachable at startup can stay out of rotation indefinitely while its tools remain registered. - [Info]
NacosMcpClients.java:76— withregisterTogone,get(...)is the module's entry point and returnsnull, so a typo'dconnections.<name>key surfaces as an NPE in the documented one-liner.
Verdict
Genuinely good turnaround — the reconciliation rewrite removes the class of bug rather than the instances of it, and the new tests would have caught the original ones. Nothing above is a blocker on correctness of the happy path; the fail-over question is the one I would like settled before merge, because "an unhealthy instance does not break the call" and "a tool call is executed at most once" cannot both be promised without a rule for which errors retry. CLA signed, no merge conflicts, mergeStateStatus=BLOCKED is review/CI gating rather than anything in the branch. @mention me after the next push (or when the two build jobs close) and I will re-check; merging stays with the maintainers.
Automated review by github-manager-bot
| logger.debug( | ||
| "Calling MCP tool '{}' on client '{}', endpoint '{}'", toolName, name, endpoint); | ||
| return client.callTool(toolName, arguments, meta) | ||
| .onErrorResume( |
There was a problem hiding this comment.
[Warning] Fail-over on any error turns every tool call into at-least-once, including deterministic failures.
onErrorResume re-issues the same callTool against the next endpoint for whatever the transport produces: an unwrapped timeout, an unknown-tool error, an argument-validation rejection, a permission denial. Three consequences:
- Duplicated side effects. A
requestTimeout(default 120s) expiry does not mean the server never performed the work — the tool may well have created a resource, sent a message, or run a command before the deadline. Retrying it on another backend instance executes it again, and the caller cannot tell: the second attempt'sCallToolResultis returned as if it were the first, with no "this was a retry" signal. For a load-balanced wrapper whose backends are peers serving the same logical service, that is usually a write to the same downstream system, not an idempotent re-read. - Pointless amplification. A deterministic failure (bad argument name, tool not exposed, unauthorized) is retried on every connected endpoint before the caller sees anything, so a 1-instance-visible mistake becomes N transport round-trips and N WARN lines.
- No overall deadline. Worst case is
candidates.size() × requestTimeout(default 30s init, 120s request) with nothing bounding the aggregate; the caller's own timeout budget is silently multiplied.
Suggested direction:
// opt-in, connection-scoped retry
private static boolean failoverEligible(Throwable e) { ... } // not-initialized / connection reset / transport-level
.onErrorResume(e -> failoverEligible(e) && ++attempt < maxAttempts
? callCandidates(candidates, index + 1, toolName, arguments, meta)
: Mono.error(e))i.e. keep the fail-over (it is exactly what the unhealthy-instance bug needed) but scope it to connection-level failures, or gate it on a builder flag with at-least-once documented on the wrapper and in docs/v2/*/integration/infrastructure/nacos.md, and bound the whole call with an overall deadline instead of per-attempt timeouts only.
| // A previous connect attempt never came up; drop it and retry. | ||
| closeEndpoint(endpoint.key()); | ||
| } | ||
| connectEndpoint(endpoint); |
There was a problem hiding this comment.
[Warning] Blocking connect while holding reconcileLock puts close() and every later snapshot behind the slowest endpoint.
reconcile() is the sole writer and runs on the single-thread reconciler, but it calls connectEndpoint — which does clientFactory.create(...).block() and client.initialize().block() — inside the reconcileLock critical section. Two things follow:
close()has to take the same lock, so shutdown waits for the in-flight connects: up toinitializationTimeout(30s default) per pending endpoint. In a Spring context with several configured connections, a wedged backend turns bean teardown into a multi-minute wait — andNacosMcpClients.close()closes the wrappers sequentially, so the delays add up.- Nacos keeps pushing while a connect is blocked. Each push appends a task to
Executors.newSingleThreadExecutor's unbounded queue and each replayed task blocks on connects again, so the wrapper can fall arbitrarily behind the registry — and stale snapshots are re-applied after newer ones have already been observed. (TheinitialSnapshot && pushAppliedguard only protects the very first snapshot.)
Cheap structural fix: keep the lock for the state mutation only — compute the add/remove sets, update registeredEndpoints/endpointClients bookkeeping under the lock, and run the blocking connects outside it; or coalesce instead of queueing (an AtomicReference<McpServerDetailInfo> holding the latest pending snapshot that the reconciler drains), and in close() do closed = true; reconciler.shutdownNow(); before taking the lock to close the clients. Either shape keeps the single-writer property you want without letting one endpoint's handshake hold everything hostage.
| return; | ||
| } | ||
| // Never reconcile on the Nacos dispatcher thread. | ||
| submitReconcile(detailInfo, false); |
There was a problem hiding this comment.
[Info] The CompletableFuture from the push path is dropped, so a reconcile that throws now disappears silently.
The old listener wrapped updateEndpoints in try/catch with a logger.error. That safety net is gone: submitReconcile(detailInfo, false) returns a future nobody completes-on, so any exception escaping reconcile() (a closeEndpoint throwing, an unexpected deduplicate/resolveEndpoints failure, an interrupt during block()) is captured in an abandoned future — the endpoint set then just stays stale with no trace, which is the hardest failure mode to diagnose in this class.
One line restores the previous behaviour:
submitReconcile(detailInfo, false).whenComplete((v, e) -> {
if (e != null) {
logger.error("Failed to apply endpoint update for MCP server '{}'", serverName, e);
}
});| + " connected endpoint(s) of MCP server '" | ||
| + serverName | ||
| + "' failed to call tool '" | ||
| + toolName |
There was a problem hiding this comment.
[Info] The aggregated error carries no cause, which costs the operator the diagnosis.
When every candidate fails, the caller gets a fresh IllegalStateException whose text says only "All N connected endpoint(s) … failed to call tool 'x'". The actual reasons live in N WARN lines that reference the previous call site, and the exception chain is IllegalStateException -> null. Since this is the one path where a healthy-looking wrapper stops serving, please keep the causes:
IllegalStateException all = new IllegalStateException("All " + candidates.size() + " connected endpoint(s) of MCP server '" + serverName + "' failed to call tool '" + toolName + "'");
causes.forEach(all::addSuppressed); // collected alongside the recursion
return Mono.error(all);This also matters for the fail-over question on the onErrorResume comment: without the cause in hand, a caller cannot distinguish "retry me" from "this tool is broken" once the aggregate surfaces.
| logger.info("Connected endpoint '{}' of MCP server '{}'", endpoint, serverName); | ||
| } | ||
| } catch (Exception e) { | ||
| if (client != null && !registered) { |
There was a problem hiding this comment.
[Info] "Retried on the next Nacos push" needs a push to arrive.
The log line and the getRegisteredEndpoints() javadoc both promise that an endpoint whose connect failed is retried later, but the only trigger is onEvent. A Nacos subscription typically re-pushes on registry changes, so an endpoint that was briefly unreachable at startup can stay out of endpointClients indefinitely: callTool silently loses that candidate (and with the tool snapshot taken at registration, its tools stay registered on a connection that is not there). That is a behaviour difference from the pre-refactor version, which read the registry on every updateEndpoints call with the same weakness but without documenting a retry promise.
Two options, whichever matches the intended contract: reword to "retried the next time Nacos pushes a change for this server", or have the (already existing) reconciler thread also re-apply the last known snapshot on a bounded interval (say every 30–60s, jittered, stopped in close()), which also self-heals the fail-over-masked endpoint case above.
| * @param clientName the logical MCP client name | ||
| * @return the wrapper, or null if no connection with that name is configured | ||
| */ | ||
| public NacosLoadBalancedMcpClientWrapper get(String clientName) { |
There was a problem hiding this comment.
[Info] get(...) returning null now surfaces a typo'd connection key as an NPE in user code.
registerTo is gone, so get("weather") is the entry point of the module — and both this class's javadoc and docs/v2/*/integration/infrastructure/nacos.md show the result being dereferenced straight away (nacosMcpClients.get("weather"); weather.initialize().block();). The key comes from agentscope.nacos.mcp.connections.<name>, so a one-character slip yields NullPointerException with no mention of the configured names.
Since the holder already knows the valid set, either return Optional<NacosLoadBalancedMcpClientWrapper> (then the docs sample shows the checked form) or throw with the available names:
throw new IllegalArgumentException("No Nacos MCP connection '" + clientName + "'; configured: " + names());Either is a better failure than an NPE, and it keeps the one-line wiring in the docs honest.
|
Follow-up on the review above: The one item I would like settled before an approval is the fail-over scope at Re-review on request after the next push; merging stays with the maintainers. Automated review by github-manager-bot |
The wrapper retried any failed call on the next connected endpoint, which turned side-effecting tools into at-least-once operations without saying so. Propagate the error as-is instead and leave retrying to the layer above, which knows whether a tool is idempotent: Toolkit's execution config and the model can both retry, and the class javadoc plus both docs pages now state the at-most-once guarantee. Also log reconcile failures on the push path instead of losing them, correct the wording about an endpoint whose handshake failed, and fail fast when NacosMcpClients#get is asked for a connection name that is not configured.
|
@oss-maintainer Thanks — this is a genuinely good catch, and it is the kind of question that changes the design rather than the code. I thought about it against how AgentScope already works, and I came to the same conclusion as you: both promises cannot hold, so I gave up "an unhealthy instance does not break the call" instead of quietly keeping it. Retry now belongs entirely to the Toolkit layer. That layer's mechanism is already fully documented for users — Concretely, in
The same reasoning applies to write-like tools: we would rather let the caller declare idempotency through |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed 41f4a75c..5bf2aa7a. The fail-over question I said was blocking an approval is resolved, and resolved in the direction I would have argued for: callTool now dispatches to exactly one connected endpoint and propagates the error untouched, so the wrapper's guarantee is at most once and the decision to retry belongs to the Toolkit layer, where ExecutionConfig.maxAttempts / retryOn already live and are already documented. That is the right ownership boundary — this layer cannot know whether a given tool is idempotent, and the model layer already has a documented, caller-visible retry switch. Approving.
The choice is carried through consistently, which is what makes it reviewable rather than a revert: connectedCandidates became connectedEndpoints (no preferred-first reordering needed once there is no sweep), the recursion and the aggregated All N connected endpoint(s) failed error are gone, the two fail-over tests are replaced by shouldNotReplayFailedCall / shouldNotReplayFailedCallWithMeta which assert the untouched message and that the other endpoint is never called, and the at-most-once promise plus the ToolkitConfig/ExecutionConfig example are now stated in the class javadoc and in both the EN and ZH docs pages. NacosMcpClients.get throwing instead of returning null closes last round's other item, and the push-path future is now observed via whenComplete so a failing reconcile can no longer vanish silently — that was the third item I raised.
Findings
- [Warning]
NacosLoadBalancedMcpClientWrapper.java:486— carried over, unchanged by this head:connectEndpointblocks insidereconcileLock, soclose()and later snapshots can wait on up to N connect timeouts during a rolling restart of an MCP server. The fail-over removal makes it a startup/shutdown concern rather than a call-path one, which is why I am not treating it as a blocker; a.timeout(...)on the create, or connecting outside the lock and swapping inside it, caps it. Details in the inline comment. - [Info]
NacosMcpClients.java:83— the new error is a real improvement; only the empty-configuration case reads oddly (configured connections: []) when the likely cause is a wrong prefix that built no connections at all.
Checks
Check License, Check Module Sync, validate, build (ubuntu-latest), build (windows-latest), codecov/patch and license/cla are all green on 5bf2aa7a. I ran no build or test locally, so this approval rests on GitHub's own checks plus reading the diff and the current head blobs — in particular I did not exercise a real Nacos/MCP environment, so the shutdown-latency scenario above is a reading of the code, not an observed failure. Thanks for engaging with the design question rather than the narrow version of the finding; that is what produced the better API. Merging stays with the maintainers.
Automated review by github-manager-bot
| // A previous connect attempt never came up; drop it and retry. | ||
| closeEndpoint(endpoint.key()); | ||
| } | ||
| connectEndpoint(endpoint); |
There was a problem hiding this comment.
[Warning] Carried over and still open: connectEndpoint blocks inside reconcileLock, so one unreachable endpoint still delays close() and every later snapshot.
Nothing about this head changes the shape I raised last round — reconcile takes reconcileLock (line 424), calls connectEndpoint under it (line 486), and connectEndpoint ends in clientFactory.create(...).block() (line 500), while close() takes the same lock (line 354) and submitReconcile feeds a single-thread executor with an unbounded queue.
What the removal of fail-over does change is how visible this becomes. Before, a call could sweep past a half-dead endpoint; now an endpoint that never came up is simply absent from connectedEndpoints(), so a slow or unreachable instance only costs startup/shutdown latency rather than call latency. That is a better place for it, and the Failed to connect endpoint ... tool calls stay on the connected endpoints until Nacos reports this endpoint again message at line 525 is honest about the consequence — my earlier point was that the old wording promised a retry that no code performed, and that is fixed.
Still worth a follow-up: during a rolling restart of a MCP server with N endpoints, a Spring context shutdown can wait on up to N connect timeouts (120s default per endpoint) before close() returns, and queued snapshots replay behind it. Bounding the connect (create(...).timeout(...)) or moving the connect outside the lock and letting the snapshot swap stay inside would cap that.
| return client; | ||
| } | ||
| } | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
[Info] Throwing instead of returning null is the right call; the one thing I would add is the list of configured names being non-empty in the common case.
The change closes last round's point cleanly: a typo in agentscope.nacos.mcp.connections.<name> used to surface as an NPE somewhere inside the starter, and now it names the bad key and the configured set at the call site. Javadoc updated to match (@return the wrapper, never null + @throws), and NacosMcpClientsTest pins it.
Only a nit for a future pass: configuredNames() reports the names that exist, which is exactly what a typo needs — but when the whole block is mis-configured the list is empty and the message reads configured connections: []. Appending "or none at all" for that case (or a separate message) turns the most likely authoring mistake — a wrong prefix, so zero connections were built — into a sentence rather than an empty bracket. Not worth holding the PR for.
4 participants
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
Closes #2938
Background
Nacos is already used by AgentScope as an A2A agent registry, a prompt
listener and a skill repository, but there is no MCP integration: MCP wiring
is static-config only (
McpServerConfigparsed fromtools.jsonmcpServers.<name>and applied byMcpServerRegistrar). There is no way todiscover MCP servers dynamically or to spread calls across their instances.
What this PR does
Adds a new
agentscope-extensions-nacos-mcpmodule that subscribes MCPservers from the Nacos MCP registry (Nacos 3.x push-based
AiService.subscribeMcpServer) instead of relying on static configurationalone:
NacosMcpDiscoveryClient— subscribes registry events and resolvesendpoints from
McpServerDetailInfo(backend endpoints, falling back tofrontend ones).
NacosMcpEndpoint— immutable endpoint view (address,port,path,scheme); infershttpsfor ports 443/8443 when the registry carries noexplicit protocol.
NacosLoadBalancedMcpClientWrapper— anMcpClientWrapperthat keeps oneconnection per endpoint, reconciles them on endpoint scale in/out, and
rebuilds connections when the transport changes (SSE ↔ Streamable HTTP).
A tool call is dispatched only to endpoints whose connection is
established, so an instance that never connected is never selected.
EndpointSelector—RoundRobinEndpointSelector(default) andStickyEndpointSelector(session affinity, for stateful MCP servers).NacosMcpClients— holder that groups the configured wrappers; callers pickthe connection an agent needs with
NacosMcpClients#get(name),initialize()it and register it into the agent'sToolkitthemselves.Spring Boot autoconfiguration is added to the existing
agentscope-nacos-spring-boot-starter:AgentscopeMcpNacosAutoConfigurationplus
agentscope.nacos.mcp.*properties (enabled,load-balance,connections.<name>.service-name/version/load-balance). The MCPAiServiceis created separately from the A2A/prompt clients so a differentNacos cluster can be used per capability. It stays off unless
agentscope.nacos.mcp.enabled=true.The module is registered in
agentscope-bomandagentscope-all, and theNacos integration docs are updated (EN + ZH).
Design decisions
tools.json—additive only, no override. Static servers keep going through
McpServerRegistrar; Nacos-discovered servers are added to the same agentToolkitby the caller, throughToolkit#registerMcpClient(...)on thewrapper returned by
NacosMcpClients#get(name). The two are independentsources and coexist in one Toolkit. Registration publishes the tool list
discovered at that moment, so a server that only appears later, or whose
tool set changes, needs a fresh
registerMcpClientcall; endpoint scalein/out alone does not.
pushed by Nacos and applied at runtime without restarting the agent; a
server that appears after startup is picked up once endpoints arrive.
connected endpoint, and a failure is propagated to the caller as-is: no
wrapping, no replay on another endpoint. This guarantees that this module
executes a tool at most once, which is the safe default for write-like
tools. Retrying belongs to the caller, with a switch AgentScope already
documents:
ExecutionConfig#maxAttemptsplus aretryOnpredicate(
ToolkitConfig.executionConfig(...), default single attempt, i.e. noretry). A caller that retries re-enters
callTooland, underRoundRobinEndpointSelector, normally lands on a different instance, soavailability is still achievable — but the attempt count and the qualifying
error types are owned by the caller. Both
nacos.mdpages carry this as"A failing call is not replayed".
Compatibility
Additive. No change to
agentscope-coreinterfaces, so no cascade to harness,distribution or other extensions. Requires a Nacos 3.x server with the MCP
registry capability enabled;
nacos-clientis already pinned to 3.2.1 inagentscope-dependencies-bom, so no dependency bump is needed.How to test
Manual check: start a Nacos 3.x server and register an MCP server, then:
Register the connection an agent needs into its Toolkit:
Verify that the remote tools are exposed, that calls rotate across instances in
round-robin mode, and that scaling the registered MCP server in/out is
reflected without a restart.
Checklist
mvn spotless:applymvn test)