-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| // npx vitest run api/providers/__tests__/anthropic-timeout.spec.ts | ||
|
|
||
| import { describe, it, expect, beforeEach, vitest } from "vitest" | ||
| import { ApiHandlerOptions } from "../../../shared/api" | ||
|
|
||
| // Mock the timeout config utility | ||
| vitest.mock("../utils/timeout-config", () => ({ | ||
| getApiRequestTimeout: vitest.fn(), | ||
| })) | ||
|
|
||
| import { getApiRequestTimeout } from "../utils/timeout-config" | ||
|
|
||
| // Mock the Anthropic SDK | ||
| vitest.mock("@anthropic-ai/sdk", () => { | ||
| const mockAnthropicConstructor = vitest.fn().mockImplementation(() => ({ | ||
| messages: { | ||
| create: vitest.fn(), | ||
| countTokens: vitest.fn(), | ||
| }, | ||
| })) | ||
| return { | ||
| Anthropic: mockAnthropicConstructor, | ||
| } | ||
| }) | ||
|
|
||
| // Import after mocks | ||
| import { Anthropic } from "@anthropic-ai/sdk" | ||
| import { AnthropicHandler } from "../anthropic" | ||
|
|
||
| const mockAnthropicConstructor = Anthropic as any | ||
|
|
||
| describe("AnthropicHandler timeout configuration", () => { | ||
| beforeEach(() => { | ||
| vitest.clearAllMocks() | ||
| }) | ||
|
|
||
| it("should use default timeout of 600 seconds when no configuration is set", () => { | ||
| ;(getApiRequestTimeout as any).mockReturnValue(600000) | ||
|
|
||
| const options: ApiHandlerOptions = { | ||
| apiKey: "test-api-key", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| } | ||
|
|
||
| new AnthropicHandler(options) | ||
|
|
||
| expect(getApiRequestTimeout).toHaveBeenCalled() | ||
| expect(mockAnthropicConstructor).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| apiKey: "test-api-key", | ||
| timeout: 600000, // 600 seconds in milliseconds | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it("should use custom timeout when configuration is set", () => { | ||
| ;(getApiRequestTimeout as any).mockReturnValue(1800000) // 30 minutes | ||
|
|
||
| const options: ApiHandlerOptions = { | ||
| apiKey: "test-api-key", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| } | ||
|
|
||
| new AnthropicHandler(options) | ||
|
|
||
| expect(mockAnthropicConstructor).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| apiKey: "test-api-key", | ||
| timeout: 1800000, // 1800 seconds in milliseconds | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it("should handle zero timeout (no timeout)", () => { | ||
| ;(getApiRequestTimeout as any).mockReturnValue(0) | ||
|
|
||
| const options: ApiHandlerOptions = { | ||
| apiKey: "test-api-key", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| } | ||
|
|
||
| new AnthropicHandler(options) | ||
|
|
||
| expect(mockAnthropicConstructor).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| apiKey: "test-api-key", | ||
| timeout: 0, // No timeout | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it("should use custom base URL when provided", () => { | ||
| ;(getApiRequestTimeout as any).mockReturnValue(600000) | ||
|
|
||
| const options: ApiHandlerOptions = { | ||
| apiKey: "test-api-key", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| anthropicBaseUrl: "https://custom.anthropic.com", | ||
| } | ||
|
|
||
| new AnthropicHandler(options) | ||
|
|
||
| expect(mockAnthropicConstructor).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| baseURL: "https://custom.anthropic.com", | ||
| apiKey: "test-api-key", | ||
| timeout: 600000, | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it("should use authToken when anthropicUseAuthToken is set with custom base URL", () => { | ||
|
Contributor
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. 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', ...)
}) |
||
| ;(getApiRequestTimeout as any).mockReturnValue(900000) // 15 minutes | ||
|
|
||
| const options: ApiHandlerOptions = { | ||
| apiKey: "test-auth-token", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| anthropicBaseUrl: "https://custom.anthropic.com", | ||
| anthropicUseAuthToken: true, | ||
| } | ||
|
|
||
| new AnthropicHandler(options) | ||
|
|
||
| expect(mockAnthropicConstructor).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| baseURL: "https://custom.anthropic.com", | ||
| authToken: "test-auth-token", | ||
| timeout: 900000, | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it("should use apiKey when anthropicUseAuthToken is set but no custom base URL", () => { | ||
| ;(getApiRequestTimeout as any).mockReturnValue(600000) | ||
|
|
||
| const options: ApiHandlerOptions = { | ||
| apiKey: "test-api-key", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| anthropicUseAuthToken: true, | ||
| } | ||
|
|
||
| new AnthropicHandler(options) | ||
|
|
||
| expect(mockAnthropicConstructor).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| apiKey: "test-api-key", | ||
| timeout: 600000, | ||
| }), | ||
| ) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,7 @@ import { getModelParams } from "../transform/model-params" | |||||||||
| import { BaseProvider } from "./base-provider" | ||||||||||
| import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index" | ||||||||||
| import { calculateApiCostAnthropic } from "../../shared/cost" | ||||||||||
| import { getApiRequestTimeout } from "./utils/timeout-config" | ||||||||||
|
|
||||||||||
| export class AnthropicHandler extends BaseProvider implements SingleCompletionHandler { | ||||||||||
| private options: ApiHandlerOptions | ||||||||||
|
|
@@ -33,6 +34,7 @@ export class AnthropicHandler extends BaseProvider implements SingleCompletionHa | |||||||||
| this.client = new Anthropic({ | ||||||||||
| baseURL: this.options.anthropicBaseUrl || undefined, | ||||||||||
| [apiKeyFieldName]: this.options.apiKey, | ||||||||||
| timeout: getApiRequestTimeout(), | ||||||||||
|
Contributor
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. Consider adding a comment here explaining why timeout configuration is important for large file processing. Something like:
Suggested change
This would help future maintainers understand the context. |
||||||||||
| }) | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
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.