feat: improve error handling, add timeouts and retry logic for chat c…#79
feat: improve error handling, add timeouts and retry logic for chat c…#79whereisfarukk wants to merge 1 commit intomainfrom
Conversation
…ompletions - Add centralized error handling with handleRequestError() method - Implement model-specific timeouts for chat completions (5min for fast/auto, 3min for pro) - Add retry logic with exponential backoff for transient network errors - Remove console logging statements for production readiness - Simplify error handling while maintaining functionality Fixes timeout issues with vlmrun-orion-1:fast and vlmrun-orion-1:auto models
Summary of ChangesHello @whereisfarukk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and stability of chat completion requests by implementing a comprehensive error handling strategy. It introduces centralized error processing, dynamic timeouts tailored to different models, and a robust retry mechanism for transient network issues. These changes aim to prevent common failures like timeouts and improve the overall user experience by making API interactions more resilient. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to error handling, timeouts, and retry logic, enhancing the robustness of the client. The introduction of a centralized handleRequestError method is a good step towards cleaner code.
My review includes a few suggestions:
- A critical fix for the retry logic to prevent premature termination and ensure it correctly handles retries and throws the final error.
- A suggestion to remove unused parameters from the new error handling function for code clarity.
- A recommendation to replace magic numbers for timeouts with named constants for better readability and maintainability.
Overall, these changes are valuable, and with the suggested adjustments, the implementation will be more robust and maintainable.
| // Add retry logic for transient network errors | ||
| let lastError: any; | ||
| const maxRetries = 3; | ||
|
|
||
| for (let attempt = 1; attempt <= maxRetries; attempt++) { | ||
| try { | ||
| const response = await this.ef.helpers.httpRequestWithAuthentication.call( | ||
| this.ef, | ||
| 'vlmRunApi', | ||
| options, | ||
| ); | ||
| return response; | ||
| } catch (error: any) { | ||
| lastError = error; | ||
|
|
||
| // Check if it's a retryable error (network/timeout errors) | ||
| const errorCode = error.code || error.cause?.code; | ||
| const isRetryable = | ||
| errorCode === 'ETIMEDOUT' || | ||
| errorCode === 'ECONNRESET' || | ||
| errorCode === 'EPIPE' || | ||
| (!error.response && attempt < maxRetries); // Network errors without response | ||
|
|
||
| if (isRetryable && attempt < maxRetries) { | ||
| const delay = RETRY_DELAY * attempt; // Exponential backoff | ||
| await new Promise(resolve => setTimeout(resolve, delay)); | ||
| continue; | ||
| } | ||
|
|
||
| // Not retryable or max retries reached | ||
| this.handleRequestError(error, url, method); | ||
| } | ||
| throw new Error(`HTTP ${error.response?.status || 'Error'}: ${errorDetail}`); | ||
| } | ||
|
|
||
| // All retries exhausted | ||
| this.handleRequestError(lastError, url, method); | ||
| } |
There was a problem hiding this comment.
The current retry logic has a flaw. The call to this.handleRequestError(error, url, method); on line 242 will always throw an exception, causing the function to exit immediately. This makes the code on line 247 (this.handleRequestError(lastError, url, method);) unreachable. Consequently, the function might not wait for all retries to be exhausted and might report an intermediate error instead of the last one.
The retry logic should be refactored to only continue the loop for retryable errors within the retry limit. If an error is not retryable, or if the maximum number of retries has been reached, the loop should be exited, and then the lastError should be thrown.
// Add retry logic for transient network errors
let lastError: any;
const maxRetries = 3;
for (let attempt = 1; attempt <= maxRetries; attempt++) {
try {
const response = await this.ef.helpers.httpRequestWithAuthentication.call(
this.ef,
'vlmRunApi',
options,
);
return response;
} catch (error: any) {
lastError = error;
// Check if it's a retryable error (network/timeout errors)
const errorCode = error.code || error.cause?.code;
const isRetryable =
errorCode === 'ETIMEDOUT' ||
errorCode === 'ECONNRESET' ||
errorCode === 'EPIPE' ||
!error.response; // Network errors without response
if (isRetryable && attempt < maxRetries) {
const delay = RETRY_DELAY * attempt; // Exponential backoff
await new Promise(resolve => setTimeout(resolve, delay));
continue;
}
// Not retryable or max retries reached, so break the loop
break;
}
}
// All retries exhausted or a non-retryable error occurred
this.handleRequestError(lastError, url, method);
}| : config.agentBaseURL; | ||
| } | ||
|
|
||
| private handleRequestError(error: any, url: string, method: string): never { |
There was a problem hiding this comment.
The url and method parameters in handleRequestError are not used within the function. This can be misleading. If they are not needed, they should be removed to simplify the function signature. This would also require updating the call sites in makeRequest and makeAgentRequest.
| private handleRequestError(error: any, url: string, method: string): never { | |
| private handleRequestError(error: any): never { |
| let timeout = 60000; // Default 60 seconds | ||
| if (endpoint === '/openai/chat/completions' && data?.model) { | ||
| // Chat completions need longer timeout | ||
| // "fast" and "auto" models might need more time than "pro" | ||
| if (data.model.includes(':fast') || data.model.includes(':auto')) { | ||
| timeout = 300000; // 5 minutes for fast/auto | ||
| } else { | ||
| timeout = 180000; // 3 minutes for pro | ||
| } |
There was a problem hiding this comment.
The timeout values 60000, 300000, and 180000 are used as magic numbers. It would be better to define them as named constants at the top of the file or in config.ts for better readability and maintainability. This makes it easier to understand their purpose and modify them in one place if needed.
1 participant
…ompletions
Fixes timeout issues with vlmrun-orion-1:fast and vlmrun-orion-1:auto models