[yangjj-iso]

Fixes #6117.

After bootstrap restart, rules arriving before selectors were cached but their tools were never added when the server was later created. Retain snapshots of the latest rules and replay matching rules only after server creation or a selector path move. Unchanged-path selector updates do not re-register tools. Serialize handler changes, remove retained rules and tool cache entries on deletion, and do not replay disabled rules.

Validation: all 21 McpServerPluginDataHandlerTest cases pass on Microsoft OpenJDK 17.0.18, including recovery, deletion, disabled-rule replay, unchanged-path updates, moves to existing servers and caller mutation after receipt. Negative addTool assertions use null-tolerant matchers and pin CACHED_TOOL state. A temporary mutation removing the disabled-rule guard makes its regression fail with NeverWantedButInvoked; the guard is restored in the submitted patch. Checkstyle, affected-reactor Apache RAT and git diff --check pass. This deterministic lifecycle test does not claim a Docker bootstrap/client restart E2E run.

Shared CI fixture corrections: CryptorResponsePluginTest now checks the post-#7903 configuration error for both encrypt/decrypt when a field is absent, including absence of the original user data. SDK tests wait up to 60 seconds for usable registered routes before exercising assertions. ZooKeeper registration tests keep their declared backend ports reachable so the real health checker cannot race registry-removal assertions. Validation: 16 cryptor unit cases and 16 Admin discovery/health cases pass; the HTTP and SDK integration test reactors compile with Checkstyle. Full Docker integration execution is not claimed: the local Linux Docker engine is unavailable. Remote check results are tracked separately.

[Aias00]

Review: replay early rules when the selector arrives

The fix targets the right thing — #6117 is exactly the "rule sync group lands before the selector group after a bootstrap restart, so handlerRule finds no CACHED_SERVER entry and silently drops the tool" case, and retaining the rules to replay them later is the correct shape. addTool also already does removeTool first, so replay is idempotent at the manager level and cannot double-register a tool name. Good.

Two things need fixing before this can merge.

1. The two negative tests cannot fail for the reason they exist

ShenyuMcpServerTool parsed from {"name":"restart-tool","parameters":[]} has a null description and a null requestConfig. But the guards are written with anyString() on those positions:

verify(shenyuMcpServerManager, never()).addTool(anyString(), anyString(), any(), any(), anyString());

anyString() does not match null (Mockito 2+ semantics — it is InstanceOf(String.class)). So if the replay were broken and actually ran for a deleted or disabled rule, the real invocation would be addTool("/restart", "restart-tool", null, null, "<schema>"), the third argument would fail the anyString() matcher, the invocation would not be counted, and never() would still pass. Both deletedRulesAreNotReplayedWhenSelectorsArrive and disabledRulesAreNotReplayedWhenSelectorsArrive are therefore green no-ops — they cannot detect the regression they are named after.

This is easy to demonstrate: temporarily delete the Boolean.FALSE.equals(ruleData.getEnabled()) early return and both tests stay green.

Fix — use null-tolerant matchers on every position, in all three tests:

verify(shenyuMcpServerManager, never()).addTool(anyString(), anyString(), any(), any(), any());

(any() matches nulls, anyString() does not.) The positive assertion in rulesArrivingBeforeSelectorsAreReplayedWithoutToggling should get the same treatment for consistency, and it is worth adding one assertion on CACHED_TOOL state so the tests do not depend solely on manager interaction counting.

2. Replay runs on every selector update, not just on server creation

handlerSelector is invoked for every SELECTOR group UPDATE from admin, not only when a server is first created. The replay block is unconditional, so on any unrelated selector edit (condition tweak, handle change with the same path) this now does, for each rule of that selector:

• registerTool → CACHED_TOOL.cachedHandle(...)
• shenyuMcpServerManager.addTool(...) → which internally removeTool(...) then addTool(...), each .block()-ed with a 10s timeout

For a selector carrying N tools that is N remove+add round trips on the sync thread on every selector change, and it transiently makes every tool disappear from tools/list while the cycle runs. Before this PR handlerSelector did no tool work at all, so this is a new cost on a hot path.

Replay is only actually needed when the server did not exist yet, or when it moved to a different path. The information is already computed two lines above:

boolean serverCreated = StringUtils.isNotBlank(path) && !shenyuMcpServerManager.hasMcpServer(path);
boolean pathMoved = Objects.nonNull(previousServer) && StringUtils.isNotBlank(previousServer.getPath())
        && !previousServer.getPath().equals(path);
...
if (serverCreated || pathMoved) {
    rules.values().stream().filter(rule -> selectorData.getId().equals(rule.getSelectorId()))
            .forEach(this::registerTool);
}

Please also add a test pinning that a second handlerSelector for an unchanged path does not call addTool again — that is the invariant the guard is protecting, and without it the churn can silently come back.

Non-blocking

• rules retains rules whose selector never arrives and is never deleted. Bounded by the rule count, so acceptable, but removeSelector is currently the only pruning path; a rule whose selector was deleted while the gateway was down is only cleaned on the next full refresh.
• rules.put(ruleData.getId(), ruleData) stores the caller's mutable RuleData. If the sync layer reuses instances, the retained snapshot can drift from what was registered. A shallow copy (id + selectorId + enabled + handle) would be safer and is all the replay needs.
• removeSelector performs CACHED_TOOL.get().removeHandle(...) as a side effect inside rules.values().removeIf(...). It works on ConcurrentHashMap, but collecting the matching ids first and removing after is easier to reason about.
• synchronized on all four handlers is fine and the lock order (handler → manager) is consistent, so no deadlock risk today. Worth a comment noting that addTool blocks, since holding the handler monitor across it serializes all selector/rule sync for the duration.

[Aias00]

Review: #7942 fix(mcp): replay early rules when their selector becomes ready (re-review)

Verdict: approved — all requested changes and non-blocking improvements have been cleanly addressed.

What was verified in this update

1. Null-tolerant mock verification & state assertions:
   • Replaced restrictive string matchers with null-tolerant any() in verify(shenyuMcpServerManager, never()).addTool(any(), any(), any(), any(), any()).
   • Added direct state assertions against McpServerPluginDataHandler.CACHED_TOOL in both positive and negative tests to verify cached handle consistency independently of mock invocations.
2. Guarded replay on selector updates:
   • Replay is now explicitly guarded by if (serverCreated || pathMoved), eliminating redundant removeTool + addTool round trips on subsequent unchanged selector updates.
   • Pinned with unchangedSelectorDoesNotRegisterToolsAgain and movedSelectorReplaysRulesToAnExistingServer.
3. Defensive copies and cleaner collections:
   • In handlerRule, RuleData is cloned defensively into a snapshot before caching, ensuring external mutations cannot corrupt the retained rule state (replayUsesSnapshotOfTheReceivedRule).
   • In removeSelector, collected matching rules prior to mutation rather than performing removal as an iteration side-effect.