-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: implement timeout wrapper for OpenAI streams to handle slow models #6574
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
- Add withTimeout wrapper function that monitors async iterables - Wrap OpenAI SDK streams with configurable timeout - Add openAiRequestTimeout configuration option - Include comprehensive tests for timeout scenarios - Fixes issue where local models with long prompt load times would timeout after 5 minutes This approach wraps the OpenAI SDK stream response instead of trying to pass custom fetch options, which the SDK does not properly support.
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.
I wrote a timeout wrapper to fix my own timeout issues. The irony is not lost on me.
| export async function* withTimeout<T>( | ||
| iterable: AsyncIterable<T>, | ||
| timeout: number = DEFAULT_REQUEST_TIMEOUT, | ||
| ): AsyncGenerator<T> { |
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.
Is this intentional? The timeout is cleared in the finally block, but if the iterator throws before entering the try block, we might leak the timeout. Consider wrapping the entire function body in try-finally to ensure cleanup:
| ): AsyncGenerator<T> { | |
| export async function* withTimeout<T>( | |
| iterable: AsyncIterable<T>, | |
| timeout: number = DEFAULT_REQUEST_TIMEOUT, | |
| ): AsyncGenerator<T> { | |
| let timeoutId: NodeJS.Timeout | null = null | |
| let hasTimedOut = false | |
| try { | |
| const resetTimeout = () => { | |
| if (timeoutId) { | |
| clearTimeout(timeoutId) | |
| } | |
| timeoutId = setTimeout(() => { | |
| hasTimedOut = true | |
| }, timeout) | |
| } | |
| // Set initial timeout | |
| resetTimeout() | |
| for await (const value of iterable) { | |
| if (hasTimedOut) { | |
| throw new Error(`Request timeout after ${timeout}ms`) | |
| } | |
| // Reset timeout on each chunk received | |
| resetTimeout() | |
| yield value | |
| } | |
| } catch (error) { | |
| if (hasTimedOut) { | |
| throw new Error(`Request timeout after ${timeout}ms`) | |
| } | |
| // Check if this is a timeout-related error | |
| if (error instanceof Error && (error.message.includes("aborted") || error.message.includes("timeout"))) { | |
| throw new Error(`Request timeout after ${timeout}ms`) | |
| } | |
| throw error | |
| } finally { | |
| if (timeoutId) { | |
| clearTimeout(timeoutId) | |
| } | |
| } | |
| } |
|
|
||
| // Wrap the stream with timeout if configured | ||
| const timeout = this.options.openAiRequestTimeout || DEFAULT_REQUEST_TIMEOUT | ||
| const stream = this.options.openAiRequestTimeout ? withTimeout(baseStream, timeout) : baseStream |
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.
Could we consider always applying the timeout wrapper with the default timeout? Currently, it's only applied when explicitly configured:
| const stream = this.options.openAiRequestTimeout ? withTimeout(baseStream, timeout) : baseStream | |
| // Wrap the stream with timeout (use configured timeout or default) | |
| const timeout = this.options.openAiRequestTimeout || DEFAULT_REQUEST_TIMEOUT | |
| const stream = withTimeout(baseStream, timeout) |
This would ensure consistent timeout behavior across all requests, not just when users explicitly configure it.
| * @param timeout Timeout in milliseconds | ||
| * @returns AbortController instance | ||
| */ | ||
| export function createTimeoutController(timeout: number = DEFAULT_REQUEST_TIMEOUT): AbortController { |
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.
Are these utility functions intended for future use? They're exported but not currently used in the codebase. If they're not needed, we might want to remove them to keep the code lean.
| expect(results).toEqual([{ data: "chunk1" }, { data: "chunk2" }, { data: "chunk3" }]) | ||
| }) | ||
|
|
||
| it.skip("should timeout after specified duration with no chunks", async () => { |
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.
I understand why this test is skipped, but could we explore alternative approaches? Perhaps we could test with a mock that never resolves its next() promise, or use a controlled async generator that we can manually advance?
| openAiStreamingEnabled: z.boolean().optional(), | ||
| openAiHostHeader: z.string().optional(), // Keep temporarily for backward compatibility during migration. | ||
| openAiHeaders: z.record(z.string(), z.string()).optional(), | ||
| openAiRequestTimeout: z.number().min(0).optional(), // Request timeout in milliseconds |
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 more detailed comment explaining when users might want to adjust this value. For example:
| openAiRequestTimeout: z.number().min(0).optional(), // Request timeout in milliseconds | |
| openAiRequestTimeout: z.number().min(0).optional(), // Request timeout in milliseconds. Useful for slow local models or high-latency connections. Default: 300000 (5 minutes) |
|
Closing, the issue needs scoping, this implementation doesn't fix the issue |
This PR fixes the issue where local models with long prompt load times would timeout after 5 minutes due to undici's default bodyTimeout.
Since passing custom fetch or fetchOptions to the OpenAI SDK doesn't work correctly (as confirmed by @pwilkin), this PR implements an alternative approach by wrapping the OpenAI SDK stream response with a configurable timeout.
Users can now configure the timeout in milliseconds in their settings.
Fixes #6570
Important
Introduces a timeout wrapper for OpenAI streams to handle slow models, allowing configurable timeout settings and ensuring only idle time counts towards the timeout.
withTimeoutfunction intimeout-wrapper.tsto wrap async iterables with a configurable timeout.openAiRequestTimeoutoption toProviderSettingsinprovider-settings.tsfor user-configurable timeout.withTimeoutinbase-openai-compatible-provider.tsandopenai.ts.timeout-wrapper.spec.tsfor various timeout scenarios, including no timeout, timeout after delay, and error handling.This description was created by
for 025ea85. You can customize this summary. It will automatically update as commits are pushed.