Fix AnthropicChatOptions.timeout(Duration) being ignored outside model construction#6605
Conversation
062338d to
2e063b9
Compare
…l construction AnthropicChatModel.internalCall()/internalStream() never forwarded the effective (default or per-request) AnthropicChatOptions.getTimeout() into a com.anthropic.core.RequestOptions for the underlying SDK call, so only the timeout baked in once at AnthropicChatModel construction (via spring.ai.anthropic.timeout) ever had any effect — contradicting the documented per-call usage. Add a requestOptionsFor(Prompt) helper that resolves the merged AnthropicChatOptions timeout into an explicit RequestOptions and pass it to both the sync create() and async createStreaming() calls. Fixes spring-projects#6604 Signed-off-by: seeun0210 <cocobell3@naver.com>
2e063b9 to
e9f8848
Compare
|
While working on this, I noticed the same pattern likely exists in other provider modules — e.g. If this fix direction looks right to maintainers, I'm happy to apply the same pattern to OpenAI (and check other provider modules for the same issue) in follow-up PRs. |
|
Hi @sobychacko — thanks for triaging #6604 and assigning it to the 2.0.1 milestone! Just wanted to flag that this PR already has a working fix + regression tests ready for review whenever you get a chance. Happy to adjust the approach if you'd prefer a different direction (e.g. bundling it with other providers as mentioned in my follow-up comment on this PR). No rush — just wanted to make sure it's on your radar alongside the issue. Thanks! |
sobychacko
left a comment
There was a problem hiding this comment.
Looks good. I added some minor nit-picks. You can also add your name (full real name) as an author to all the classes you modified in the PR.
| * @return request options carrying the resolved timeout, or | ||
| * {@link RequestOptions#none()} if none is set | ||
| */ | ||
| private static RequestOptions requestOptionsFor(Prompt prompt) { |
There was a problem hiding this comment.
Too much javadoc on a private method. Can you cut it down to what is absolutely necessary? We usually prefer this much detail only on the public API's.
| * @param prompt the prompt with message history and options | ||
| * @return the effective Anthropic chat options | ||
| */ | ||
| private static AnthropicChatOptions resolveAnthropicOptions(Prompt prompt) { |
There was a problem hiding this comment.
In-line these comments instead of javadoc. This level of comment details are not required for private methods.
| * {@link ChatClient} default option, or as a per-request {@code ChatClient} override — | ||
| * not just at construction time. | ||
| * | ||
| * @author seeun0210 |
There was a problem hiding this comment.
Use your full real name.
Reduce the Javadoc on the private resolveAnthropicOptions and requestOptionsFor helpers to brief inline comments, and add the author tag to the classes touched by this PR, as requested in review. Signed-off-by: seeun0210 <cocobell3@naver.com>
|
Thanks for the review @sobychacko — addressed all three points: trimmed the private-method Javadoc on On the follow-up I raised earlier: I went and confirmed the same pattern in Happy to fix it — would you prefer it bundled into this PR or as a separate follow-up PR (with its own issue)? Whatever is easier to review on your end. |
|
Quick follow-up on my question above: I see #6648 has already been opened to propagate the timeout for the OpenAI models, so no need for a separate OpenAI PR from me — this one can stay scoped to Anthropic. 👍 I left a note over there linking the two since they share the same root cause. |
Thank you for taking time to contribute this pull request!
You might have already read the contributor guide, but as a reminder, please make sure to:
git commit -s) per the DCOmainbranch and squash your commitsFor more details, please check the contributor guide.
Thank you upfront!
Summary
Fixes #6604.
AnthropicChatModel.internalCall()/internalStream()never forwarded the effectiveAnthropicChatOptions.getTimeout()(whether set viaChatClient.Builder.defaultOptions()or a per-requestChatClient.prompt().options(...)override) into acom.anthropic.core.RequestOptionsfor the underlying Anthropic Java SDK call. Both call sites always used the SDK's defaultRequestOptions, so the only timeout that ever had any effect was the one baked into the sharedAnthropicChatModel/AnthropicClientonce, at construction time (viaspring.ai.anthropic.timeout). This contradicts the documented usage, which shows.timeout(Duration)as a per-callAnthropicChatOptionssetting.Why this gap existed: The shared
AnthropicClient/AnthropicChatModelbean is (correctly) built once at startup, so the timeout baked into its HTTP client is fixed at that point. When per-callAnthropicChatOptions.timeout(Duration)was documented as an override, the wiring to actually forward it into the SDK's per-callRequestOptions(which the SDK already supports without rebuilding the client) was never added — the two call sites just kept using the SDK's single-argcreate()/createStreaming()overloads.Why per-call overrides above 60s matter: Some calls legitimately need longer than the SDK's 60s default:
web_searchtool use, where the model can issue multiple search round-trips (query → fetch → read → continue generating) within a single API call.maxTokens, since output tokens are produced sequentially, so wall-clock time scales with output length.AnthropicSkillContainer) invoking code execution to generate/edit complexxlsx/pptx/docx/pdfdocuments.Without this fix, an app has no way to grant just those calls more time — raising
spring.ai.anthropic.timeoutglobally is the only lever, and even then, an unconfigured/short construction-time timeout compounds silently across the SDK's automatic retries (defaultmaxRetries: 2→ 3 attempts), so a call can fail after ~3× the configured timeout with no indication why.requestOptionsFor(Prompt)helper that resolves the merged (default + per-request)AnthropicChatOptions.getTimeout()into an explicitRequestOptions, passed to bothmessageService.create(params, requestOptions)andcreateStreaming(params, requestOptions).AnthropicChatModelTests' Mockito stubs to match the new two-arg SDK call signature (mechanical change, no behavior change to the tests themselves).AnthropicChatModelTimeoutTestswith 3 cases: construction-time timeout bounds a call; a per-requestChatClienttimeout overrides a too-short construction-time default; aChatClientdefault-options timeout overrides a too-short construction-time default. All 3 fail without the fix (verified) and pass with it.Test plan
./mvnw -pl models/spring-ai-anthropic test— full module suite (123 tests) passesAnthropicChatModelTimeoutTestsfail without the fix (confirmed viagit stashon the production change) and pass with itMockWebServerwithsetHeadersDelay(...)for deterministic timing