[ISSUE #7384] Harden reactive AI transport for production - #8034

Thanks, this is a well-scoped hardening pass and most of it holds up under scrutiny. I verified the transport side against the pinned runtime rather than trusting the diff, and it behaves as described. Two small items need fixing before merge; everything else is notes.

Verified

• Local: shenyu-plugin-ai-proxy 298/298 and shenyu-spring-boot-starter-plugin-ai-proxy 16/16 green (the starter module needs -pl shenyu-plugin-ai-proxy -DskipTests install first, otherwise it resolves the old SNAPSHOT and fails on the missing Pool/Proxy nested types). checkstyle:check clean on both.
• Rollback: reverting only WebClientAiProxyTransport.java to base fails exactly 6 tests, one per behavior change — REQUEST_TOO_LARGE before exchange, oversized content-length, oversized chunked body, pool-acquire classification, ReadTimeoutException classification, and the "plain TimeoutException must not be reclassified" case. Good discrimination.
• The doOnResponse(... removeHandler(NettyPipeline.ResponseTimeoutHandler)) idiom is correct for reactor-netty 1.1.20: NettyPipeline.ResponseTimeoutHandler resolves to reactor.left.responseTimeoutHandler, and HttpClientOperations re-adds that handler per response (pipeline.get(...) == null guard) and already removes it on terminate for persistent connections. So removing it after headers does not disable the header timeout for later requests on a pooled connection.
• classifiesRealConnectionPoolAcquireTimeout is the right kind of test — a real ConnectionProvider with maxConnections=1 proves the hard-coded shaded class name actually matches. Please keep it if you touch the pool config.
• The switch expressions on Kind are exhaustive, so adding a constant forces a decision at compile time. Nice.

CI is red, but not because of this diff (worth checking before you dig in):

• pr_build fails at shenyu-plugin-ai-token-limiter test compile: cannot find symbol: class ConnectProtocolAdapter. This PR touches neither that module nor that class; it is a stale-master breakage (master has since removed the reference), so a rebase should clear it.
• build (shenyu-integrated-test-http) fails on CachePluginTest.testMemoryModle / testRedisModle, the same pair that fails on other unrelated PRs. Known flake, not yours.
• build is just the aggregate gate reacting to pr_build ([[ "failure" == "success" ]] || exit -1).

Blocking

1. REQUEST_TOO_LARGE is reported to the client as 502 Bad Gateway (inline). It is a client-side error and the plugin already returns 413 for DataBufferLimitException, so the same method is now inconsistent with itself.
2. The retry-policy change for the two new kinds is untested (inline): the three @CsvSource tables still enumerate only the original ten constants, and making both new kinds retryable passes all 41 tests.

Non-blocking

3. @ConditionalOnMissingBean(AiProxyTransport.class) on the resources bean (inline) prevents overriding the resources independently.
4. responseHeaderTimeout is no longer enforced by WebClientAiProxyTransport itself — it now depends on the injected WebClient carrying HttpClient.responseTimeout(...). WebClientAiProxyTransport is public and its constructor accepts any WebClient, so a hand-built transport silently loses the header timeout while the property still looks authoritative. A javadoc note on the constructor would close the trap.
5. Oversized responses changed contract: the exchange now fails with RESPONSE_TOO_LARGE (and is fallback-eligible) instead of returning the upstream status plus headers with an erroring body. That is the better behavior, but callers that inspected the upstream status on oversize will see a different path — worth a RELEASE-NOTES.md line.
6. Optional: a pool-acquire timeout means local saturation, so 503 + Retry-After would describe it better than 502.

Happy to re-review as soon as the two blocking items are addressed.

Review — harden reactive AI transport

Read the full change (18 files, +1753/-87). The overall shape is right: timeouts are separated per phase, resource ownership moves into a dedicated AiProxyHttpClientResources, and failure classification is now shared between the plugin and the retry policy. A few points below, plus the PR currently needs a rebase.

What looks correct

• HttpClient.responseTimeout(...) at the connector plus doOnResponse((response, connection) -> connection.removeHandler(NettyPipeline.ResponseTimeoutHandler)) is the correct way to scope the timeout to the response headers only. Removing the handler as soon as the response starts means a long-lived stream is no longer killed by the header timeout — that is exactly the bug the previous Mono.timeout(responseHeaderTimeout) had.
• Oversized responses now short-circuit with Mono.error(responseTooLarge()) instead of returning the upstream status with a body flux that fails later, and the behaviour change is documented in RELEASE-NOTES.md. Good — a client that used to see 200 + truncated stream now sees 502/RESPONSE_TOO_LARGE, and that is a visible contract change.
• REQUEST_TOO_LARGE is checked before the request is written (exceedsMaxRequestBytes runs first in exchange), maps to 413, and is listed in both retryableTransportFailure and fallbackEligibleTransportFailure false branches. Correct — it is a client error, retrying or falling back would just amplify it.
• AiProxyFailureMapper extracted from AiProxyPlugin. I checked the ordering hazard: AiTransportException and AiProxyException both extend RuntimeException (not ShenyuException), so the ShenyuException -> 400 entry does not shadow them. The old if/else chain had the same order, so no regression.
• hasCauseNamed for PoolAcquireTimeoutException via class-name string is a reasonable compromise for a shaded internal class, and the cause == current guard prevents an infinite loop on a self-referencing cause chain.
• validateProxy fail-fast on blank host / out-of-range port / password-without-username is good; the Tls.verify=false path logs a warning instead of failing silently.

Concerns

1. responseHeaderTimeout is no longer enforced inside the transport. The field was dropped and the constructor javadoc now only asks the caller to supply a connector that enforces it:

public WebClientAiProxyTransport(final WebClient webClient) {
    this(webClient, new AiProxyTransportProperties());
}

The single-argument constructor is still public, so any wiring that does not go through AiProxyPluginConfiguration.aiProxyTransport(...) silently gets no response-header timeout at all. A timeout that quietly disappears is worse than one that is slightly redundant — please either deprecate the 1-arg constructor, or keep a defensive .timeout(...) backstop, or assert at construction that the connector has a response timeout configured.

2. ReadTimeoutException is classified as RESPONSE_HEADER_TIMEOUT, which is retryable and fallback-eligible.

if (hasCause(failure, ReadTimeoutException.class)) {
    return AiTransportException.Kind.RESPONSE_HEADER_TIMEOUT;
}

ReadTimeoutException can also be raised during body reading (a connector-level readTimeout, or an idle handler on a proxied connection). In that case a body-phase timeout is treated as a header timeout, and because RESPONSE_HEADER_TIMEOUT -> retryable = true, a partially streamed response would be replayed upstream. Since the classification now drives retry decisions, it should be unambiguous — either add a distinct kind, or only map ReadTimeoutException when no response has started.

3. Proxy.password is a plain String with a public getter on a configuration-properties object. It will show up in /actuator/env and in any config dump. Please mask it in toString() (the nested class currently has no toString, so add one that masks) and/or document that the value should come from a secret store. Same consideration for a proxy username, which is lower risk.

4. connectTimeoutMillis can throw ArithmeticException at startup.

return Math.toIntExact(properties.getConnectTimeout().toMillis());

connectTimeout is validated as positive but has no upper bound, so a value above ~24.8 days fails with an arithmetic error rather than a validation message. Clamp it, or validate an upper bound in setConnectTimeout.

5. Rebase needed. mergeable_state is dirty, and this PR touches AiProxyPluginConfiguration, AiProxyTransportProperties, WebClientAiProxyTransport and AiProxyPlugin — all of which are also modified by #8110 (AiProxyEngine.prepare now rewrites the payload before prepareRequest and calls providerRequest.setStream(...) after it). Please rebase on top of current master so the transport hardening and the protocol-binding change can be reviewed against a consistent engine.

6. RELEASE-NOTES.md — release notes in this repo are normally assembled by the release manager. The content is accurate, but I would drop that hunk from the PR and keep the note in the description; otherwise every feature PR will conflict on that file.

Nothing here blocks the design. Items 1 and 2 are the ones I would like resolved before merge; 3–6 are nits / process.

Reviewed a835579.

The transport retains a response-header-only timeout even with a custom WebClient connector, while body timeouts remain separately classified. Request-size failures map to 413 and are not replayed; response limits, cancellation and owned client-resource shutdown retain regression coverage. The previous configuration/timeout findings are addressed in this revision. No remaining blocker found in the reviewed hardening delta.

Targeted test report: 147 cases, 0 failures, 0 errors, 0 skipped.

Validation used ./mvnw, Amazon Corretto 21 and project target Java 17. Targeted Checkstyle, separately executed Apache RAT and git diff --check pass. No full-repository or Docker E2E pass is claimed; required CI must succeed before merge.

Re-reviewed the refreshed branch against current master. The merge preserves trace-header sanitation/context propagation and observer wiring alongside owned HTTP resources, request/response bounds and separate header/stream timeouts. No new blocking issue found in the integrated transport delta. Validation on this head: 161 targeted transport/integration/resource lifecycle/failure mapping/retry/plugin/starter tests passed with 0 failures, errors or skips; Maven Checkstyle and separately executed Apache RAT passed. These are scoped module tests, not the full Docker E2E suite. Required CI remains the merge gate.