-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(core): retry streaming transport errors wrapped by ModelHttpException without status code #3058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
521109d
9d6b8b5
826495a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,7 +103,11 @@ private static boolean isRetryableError(Throwable error) { | |
| return hte.isRetryable(); | ||
| } | ||
|
|
||
| if (error instanceof ModelHttpException mhe) { | ||
| // Only treat a ModelHttpException as an HTTP response error when a status code is | ||
| // present. Implementations without a status code (e.g. OpenAIException wrapping a | ||
| // streaming transport failure) must fall through to the transport/IO and cause-chain | ||
| // checks below instead of being classified as a permanent client error (issue #3057). | ||
| if (error instanceof ModelHttpException mhe && mhe.getStatusCode() != null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Please avoid retrying after a streaming response has already emitted any chunks. With this change, a null-status ModelHttpException can fall through to a retryable transport/IO cause regardless of stream progress. ModelUtils applies retryWhen to the entire response Flux, so if the first attempt emits partial text or tool-call deltas and then the connection resets, resubscription reissues the request and downstream observes the first partial output followed by the second response, which can duplicate text or tool calls. AgentScope Python and the OpenAI Python SDK limit automatic retries to establishing the request/stream rather than consuming an already-started stream. Could we guard retries so they are allowed only before the first ChatResponse is emitted, and add tests covering both a reset before the first chunk (retries) and a reset after one chunk (does not retry)?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the careful review — you're right, resubscribing after partial output would duplicate already-delivered chunks downstream. I've pushed a guard in Added the two tests you asked for in
Commit history was rewritten to keep the two changes as separate commits. |
||
| return mhe.isRetryableHttpStatus(); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| package io.agentscope.core.model; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.function.Predicate; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
@@ -110,25 +111,57 @@ public static Flux<ChatResponse> applyTimeoutAndRetry( | |
| if (retryOn == null) { | ||
| retryOn = error -> true; // retry all errors by default | ||
| } | ||
| // Lambda captures below require effectively final variables | ||
| final int retryCount = maxAttempts - 1; | ||
| final Duration effectiveInitialBackoff = initialBackoff; | ||
| final Duration effectiveMaxBackoff = maxBackoff; | ||
| final Predicate<Throwable> effectiveRetryOn = retryOn; | ||
|
|
||
| Retry retrySpec = | ||
| Retry.backoff(maxAttempts - 1, initialBackoff) | ||
| .maxBackoff(maxBackoff) | ||
| .jitter(0.5) | ||
| .filter(retryOn) | ||
| .doBeforeRetry( | ||
| signal -> | ||
| LOG.warn( | ||
| "Retrying model request (attempt {}/{}) due" | ||
| + " to: {}", | ||
| signal.totalRetriesInARow() + 1, | ||
| maxAttempts - 1, | ||
| signal.failure().getMessage(), | ||
| signal.failure())); | ||
|
|
||
| responseFlux = responseFlux.retryWhen(retrySpec); | ||
| // Retrying is only safe before the first response is emitted: resubscribing | ||
| // after partial output would reissue the request and downstream would observe | ||
| // the first partial response followed by the retried one, duplicating content. | ||
| // The emission flag is created inside defer so every subscription (model call) | ||
| // gets a fresh flag, while it persists across retry attempts of the same call. | ||
| final Flux<ChatResponse> source = responseFlux; | ||
| responseFlux = | ||
| Flux.defer( | ||
| () -> { | ||
| AtomicBoolean emittedAnyResponse = new AtomicBoolean(false); | ||
| return source.doOnNext(response -> emittedAnyResponse.set(true)) | ||
| .retryWhen( | ||
| Retry.backoff( | ||
| retryCount, | ||
| effectiveInitialBackoff) | ||
| .maxBackoff(effectiveMaxBackoff) | ||
| .jitter(0.5) | ||
| .filter( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The emission guard is the right call — retrying after partial output would duplicate content. But
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — role-only/usage-only chunks do map to
|
||
| error -> | ||
| !emittedAnyResponse | ||
| .get() | ||
| && effectiveRetryOn | ||
| .test( | ||
| error)) | ||
| .doBeforeRetry( | ||
| signal -> | ||
| LOG.warn( | ||
| "Retrying model" | ||
| + " request" | ||
| + " (attempt" | ||
| + " {}/{})" | ||
| + " due to:" | ||
| + " {}", | ||
| signal | ||
| .totalRetriesInARow() | ||
| + 1, | ||
| retryCount, | ||
| signal.failure() | ||
| .getMessage(), | ||
| signal | ||
| .failure()))); | ||
| }); | ||
| LOG.debug( | ||
| "Applied retry config: maxAttempts={}, initialBackoff={} for model: {}", | ||
| "Applied retry config: maxAttempts={}, initialBackoff={}," | ||
| + " retryBeforeFirstResponse=true for model: {}", | ||
| maxAttempts, | ||
| initialBackoff, | ||
| modelName); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| /* | ||
| * Copyright 2024-2026 the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.agentscope.extensions.model.openai.exception; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import io.agentscope.core.model.ExecutionConfig; | ||
| import io.agentscope.core.model.transport.HttpTransportException; | ||
| import java.net.SocketException; | ||
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Tag; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** | ||
| * Tests that {@link OpenAIException} participates in the {@link ExecutionConfig#RETRYABLE_ERRORS} | ||
| * classification correctly, in particular for streaming transport failures wrapped without an | ||
| * HTTP status code (issue #3057). | ||
| */ | ||
| @Tag("unit") | ||
| @DisplayName("OpenAIException Retry Classification Tests") | ||
| class OpenAIExceptionRetryClassificationTest { | ||
|
|
||
| @Test | ||
| @DisplayName("Should retry streaming transport error wrapped without status code") | ||
| void shouldRetryStreamingTransportErrorWrappedWithoutStatusCode() { | ||
| // Reproduces the production exception chain from issue #3057: | ||
| // OpenAIClient.stream wraps HttpTransportException (no status code) into | ||
| // OpenAIException, which implements ModelHttpException with a null status. | ||
| HttpTransportException transportError = | ||
| new HttpTransportException( | ||
| "SSE/NDJSON stream failed: java.net.SocketException: Connection reset", | ||
| new SocketException("Connection reset")); | ||
| OpenAIException exception = | ||
| new OpenAIException( | ||
| "HTTP transport error during streaming: " + transportError.getMessage(), | ||
| transportError); | ||
|
|
||
| assertTrue(ExecutionConfig.RETRYABLE_ERRORS.test(exception)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Should retry wrapped IO error without status code") | ||
| void shouldRetryWrappedIoErrorWithoutStatusCode() { | ||
| OpenAIException exception = | ||
| new OpenAIException( | ||
| "HTTP transport error during streaming", | ||
| new SocketException("Connection reset")); | ||
|
|
||
| assertTrue(ExecutionConfig.RETRYABLE_ERRORS.test(exception)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Should keep HTTP status based classification for real responses") | ||
| void shouldKeepHttpStatusBasedClassificationForRealResponses() { | ||
| // 429/5xx remain retryable | ||
| assertTrue( | ||
| ExecutionConfig.RETRYABLE_ERRORS.test( | ||
| OpenAIException.create(429, "Rate limited", null, null))); | ||
| assertTrue( | ||
| ExecutionConfig.RETRYABLE_ERRORS.test( | ||
| OpenAIException.create(503, "Service unavailable", null, null))); | ||
| // Other 4xx remain non-retryable | ||
| assertFalse( | ||
| ExecutionConfig.RETRYABLE_ERRORS.test( | ||
| OpenAIException.create(400, "Bad request", null, null))); | ||
| assertFalse( | ||
| ExecutionConfig.RETRYABLE_ERRORS.test( | ||
| OpenAIException.create(401, "Unauthorized", null, null))); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Should not retry unknown error without status code or cause") | ||
| void shouldNotRetryUnknownErrorWithoutStatusCodeOrCause() { | ||
| OpenAIException exception = new OpenAIException("Something went wrong"); | ||
|
|
||
| assertFalse(ExecutionConfig.RETRYABLE_ERRORS.test(exception)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix — the status-code branch no longer short-circuits the cause-chain checks when
getStatusCode()isnull, andshouldNotRetryModelHttpExceptionWithoutStatusCodekeeps the old contract. Since this changes retry behaviour for every provider implementingModelHttpException, please confirm the non-streaming 4xx paths still land inisRetryableHttpStatus()in the extension-module tests (openai/anthropic/dashscope), not just the openai one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the other model extensions: only the openai module has a
ModelHttpExceptionimplementation (OpenAIException). The anthropic and dashscope models surface transport failures as plainHttpTransportException(no status code for connection errors, 4xx/5xx with status codes), so their classification goes through theHttpTransportExceptionbranch ofisRetryableErrorand is unchanged by this fix. To pin that down, 826495a addsAnthropicRetryClassificationTestandDashScopeRetryClassificationTestasserting: connection error without status → retryable, 429/5xx → retryable, 400/401 → not retryable.