wy471x commented on PR #7512:
URL: https://github.com/apache/shenyu/pull/7512#issuecomment-6084600734
## Remaining issues
1. **`reserve()` failure paths bypass the new 503 contract.** `reserve()`
still throws a plain `IllegalStateException` when closed / update-in-progress /
generation budget exhausted, and quarantined cleanup failures hit the same
branch. In all these states `current == null`, so every MCP request — including
purely local tools — still fails with a generic 500 "Internal error". That is
the same blast radius as the earlier fail-closed finding, just via a different
trigger (e.g. 9 concurrent config refreshes against the 8-generation budget, or
one cleanup failure). The README only promises 503 for "no complete current
generation". Please either throw `CatalogUnavailableException` there too, or
document this asymmetry explicitly.
2. **Stale `lastFailure` on fenced updates.** In `applyUpdate`, when `valid`
fails at `reserve()` the `Mono` completes empty, so neither `doOnSuccess` nor
`onErrorResume` runs; `diagnostics().lastFailure` keeps a superseded update's
error class until a later update succeeds. Cosmetic but misleading for
operators.
3. **`build(null, …)` safety is implicit.** `reserve()` may now return null
and relies on `Mono.fromCallable` emptiness short-circuiting `usingWhen` so
`build` never runs; the map lambda `candidate.directory = directory` would NPE
if any future change re-enables a null resource. An explicit guard or assert
would make the invariant local.
4. **FQN cleanup partially regressed.** The main-code cleanup landed
(`PluginServiceImpl` imports, blank-line block removed), but the new hunks
reintroduce inline fully-qualified names: `java.util.Objects.nonNull(...)` /
`java.nio.charset.StandardCharsets.UTF_8` in
`ManagedRemoteMcpCatalog.applyUpdate`, and inline
`java.util.function.BooleanSupplier` in `ManagedRemoteMcpCatalog.reload` and
`RemoteCatalogLifecycle.reserve/refresh`. Test hunks are more pervasive
(`ManagedRemoteMcpCatalogTest`, `RemoteCatalogLifecycleTest`,
`AgentMcpAggregationTest`).
## Nits (non-blocking)
- The `"Remote catalog unavailable"` literal lives in
`CatalogUnavailableException`, the dispatcher's `failure(...)` call, and
docs/tests — extract a constant so renames can't drift.
- `RemoteCatalogLifecycle.call()` / `callRaw()` are near-identical twins
this delta touches — consider extracting the shared `usingWhen` shape.
- `503` / `-32023` are magic numbers in the dispatcher; `diagnostics()`
returns a stringly-typed map, forcing `(long) …get("currentVersion")` casts in
tests.
- The `BooleanSupplier valid` threaded through `reload → refresh → reserve`
doesn't convey intent; `stillCurrent`/`fence` would.
- README claims the debug diagnostics contain exactly "server, method,
generated outbound id, status and byte count", but no test pins
`decodeResponse`'s map shape — key drift would yield nulls silently.
--
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]
