-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: add timeout configuration to Anthropic provider to prevent hanging on large files #7746
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
Conversation
- Added getApiRequestTimeout() to Anthropic client initialization - This ensures API requests timeout properly instead of hanging indefinitely - Particularly important when dealing with large files that may cause long processing times - Added comprehensive tests for timeout configuration Fixes #7744
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.
Reviewed my own code. Found it adequate. Would merge again.
this.client = new Anthropic({ | ||
baseURL: this.options.anthropicBaseUrl || undefined, | ||
[apiKeyFieldName]: this.options.apiKey, | ||
timeout: getApiRequestTimeout(), |
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.
Consider adding a comment here explaining why timeout configuration is important for large file processing. Something like:
timeout: getApiRequestTimeout(), | |
// Configure timeout to prevent hanging on large files (see issue #7744) | |
// Uses the VSCode setting 'roo-cline.apiRequestTimeout' (default: 600 seconds) | |
timeout: getApiRequestTimeout(), |
This would help future maintainers understand the context.
) | ||
}) | ||
|
||
it("should use authToken when anthropicUseAuthToken is set with custom base URL", () => { |
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.
Nice comprehensive test coverage! Consider grouping related tests together for better organization. For example, all auth-related tests (lines 112-150) could be grouped under a nested describe block:
describe('authentication handling', () => {
it('should use authToken when anthropicUseAuthToken is set with custom base URL', ...)
it('should use apiKey when anthropicUseAuthToken is set but no custom base URL', ...)
})
|
||
const mockAnthropicConstructor = Anthropic as any | ||
|
||
describe("AnthropicHandler timeout configuration", () => { |
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.
While the unit tests are thorough, have we considered adding an integration test that actually verifies timeout behavior with a real (or mocked) slow response? This could help catch regressions where the timeout isn't properly applied to the actual HTTP requests.
Closing in favor of #6319 |
Description
This PR addresses Issue #7744 where users experience API requests getting stuck indefinitely when using the Anthropic provider with large files (~2MB each).
Problem
The Anthropic provider was missing timeout configuration, unlike other providers (OpenAI, Ollama, LM Studio) which already had this implemented. This caused requests to hang indefinitely without providing any feedback to the user.
Solution
getApiRequestTimeout()
to the Anthropic client initializationTesting
Notes
While this PR adds the foundational timeout configuration, future improvements could include:
Fixes #7744
Important
Adds timeout configuration to
AnthropicHandler
to prevent hanging requests, with comprehensive test coverage for various timeout scenarios.getApiRequestTimeout()
toAnthropicHandler
inanthropic.ts
to configure API request timeouts.anthropic-timeout.spec.ts
with 6 test cases for different timeout scenarios, including default, custom, and zero timeout.getApiRequestTimeout
andAnthropic
SDK to test timeout behavior.This description was created by
for d0f95a5. You can customize this summary. It will automatically update as commits are pushed.