zhang-arvin commented on PR #7022:
URL: https://github.com/apache/shenyu/pull/7022#issuecomment-5770396451
@Copilot Thanks for the review — you're right on both points, and the second
one was a real defect I had missed.
**1. Unrelated namespace change removed.** You were correct that the
selector key change (`AbstractNodeDataChangedListener`) had nothing to do with
AI proxy SSE fallback. It also turned out to be a *duplicate*: it is already
carried by its own PR, which I've verified byte-for-byte (identical blob
`b8231212` for the file). I've dropped it from this PR entirely — the branch
now contains only the `AiProxyExecutorService` change plus tests, so this PR is
scoped exclusively to #7021.
**2. The fallback was still invoked after partial output — fixed.** This was
a genuine bug, not just a missing test. `handleDirectFallbackStream`
unconditionally switched to the fallback provider on *any* error, including an
error raised after one or more chunks had already been emitted — which is
exactly the `main-partial + fallback-completion` corruption reported in #7021.
Fix: `executeDirectStream` now tracks whether any chunk has been emitted
(`AtomicBoolean` set in `doOnNext`). `handleDirectFallbackStream` takes that
flag and, if a chunk was already delivered, propagates the error instead of
subscribing to the fallback:
```java
if (chunkAlreadyEmitted) {
LOG.warn("Main direct stream failed after emitting at least one chunk; "
+ "not switching to fallback to avoid mixing partial and
fallback output.", throwable);
return Flux.error(throwable);
}
```
**3. Regression test added**, covering the case you asked for: a main stream
that emits a chunk and then errors, with a fallback configured, must emit only
the main chunk, propagate the error, and **never subscribe to the fallback**
(`verify(fallbackApi, never()).chatCompletionStream(any())`). I also added the
complementary case (error with zero chunks emitted still falls back).
**4. One more defect found while testing:** removing the retry wrapper in
this PR also removed its `NonTransientAiException` repackaging, which
invalidated the existing `testExecuteDirectStreamErrorWithNoFallback` assertion
— the suite failed for real (`expected NonTransientAiException, actual
RuntimeException`). I confirmed `AiProxyPlugin` only logs on error and does not
depend on that exception type, so the production behaviour is fine; the
assertion is now aligned with the propagated error type.
`./mvnw -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy test
-Dtest=AiProxyExecutorServiceTest` → **Tests run: 8, Failures: 0, Errors: 0**.
@Aias00 @dengliming the branch is rebased on current `master` and conflicts
are resolved. Would you mind taking another look?
--
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]
