-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve CloudSettingsService error handling for fetch failures #6627
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 retry mechanism with exponential backoff (max 3 retries) - Add detailed network diagnostics for fetch failures - Log proxy configuration, Node.js version, and VSCode version - Provide helpful error messages for common network issues - Add comprehensive tests for retry logic and error handling This should help users debug "fetch failed" errors when using Gemini embedder for codebase indexing by providing more context about the failure and attempting automatic retries. Fixes #6626
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 reviewed my own code and found it surprisingly coherent. The machines are learning.
| vi.mocked(fetch).mockRejectedValue(fetchError) | ||
|
|
||
| // Mock environment variables | ||
| process.env.HTTPS_PROXY = "http://proxy.example.com:8080" |
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.
Potential memory leak concern here. If the test fails before line 461, process.env.HTTPS_PROXY won't be cleaned up and could affect other tests. Consider using a try/finally block:
| process.env.HTTPS_PROXY = "http://proxy.example.com:8080" | |
| // Mock environment variables | |
| const originalProxy = process.env.HTTPS_PROXY | |
| try { | |
| process.env.HTTPS_PROXY = "http://proxy.example.com:8080" | |
| // ... rest of test | |
| } finally { | |
| if (originalProxy) { | |
| process.env.HTTPS_PROXY = originalProxy | |
| } else { | |
| delete process.env.HTTPS_PROXY | |
| } | |
| } |
| import type { SettingsService } from "./SettingsService" | ||
|
|
||
| const ORGANIZATION_SETTINGS_CACHE_KEY = "organization-settings" | ||
| const MAX_FETCH_RETRIES = 3 |
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.
The retry delays are hardcoded (1s, 2s, 4s). Would it be beneficial to make these configurable through constructor options, similar to how RefreshTimer allows configuration? This would give consumers more control over retry behavior in different environments.
| const response = await fetch(url, options) | ||
| return response | ||
| } catch (error) { | ||
| if (retryCount >= MAX_FETCH_RETRIES) { |
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.
When max retries are exceeded, we lose context about how many retry attempts were made. Consider wrapping the error with additional context:
| if (retryCount >= MAX_FETCH_RETRIES) { | |
| if (retryCount >= MAX_FETCH_RETRIES) { | |
| const enhancedError = new Error(`Fetch failed after ${MAX_FETCH_RETRIES} retry attempts: ${error.message}`) | |
| enhancedError.cause = error | |
| throw enhancedError | |
| } |
|
|
||
| try { | ||
| const response = await fetch(`${getRooCodeApiUrl()}/api/organization-settings`, { | ||
| this.log(`[cloud-settings] Attempting to fetch from: ${fullUrl}`) |
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 addition of URL logging for debugging! Consider also logging successful attempts at a debug level - it could help with troubleshooting intermittent issues where some requests succeed and others fail.
| /** | ||
| * Performs network diagnostics to help debug connectivity issues | ||
| */ | ||
| private async performNetworkDiagnostics(url: string): Promise<void> { |
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.
The network diagnostics are helpful! For even more comprehensive debugging, you could consider:
- Checking if DNS resolution works (using
dns.lookupfor the hostname) - Attempting a simple connectivity test to a known endpoint
- Checking if the process has network permissions
Though this might be overkill for the current use case.
| throw error | ||
| } | ||
|
|
||
| const delay = INITIAL_RETRY_DELAY * Math.pow(2, retryCount) |
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.
Minor suggestion: Consider extracting the backoff multiplier as a constant for clarity:
| const delay = INITIAL_RETRY_DELAY * Math.pow(2, retryCount) | |
| const BACKOFF_MULTIPLIER = 2 | |
| const delay = INITIAL_RETRY_DELAY * Math.pow(BACKOFF_MULTIPLIER, retryCount) |
|
This is unrelated to the issue, Closing |
Summary
This PR improves error handling in CloudSettingsService to help users debug "fetch failed" errors when using Gemini embedder for codebase indexing.
Problem
Users were experiencing generic "fetch failed" errors when trying to use the Gemini embedder for codebase indexing, with no clear indication of what was causing the failure. The error occurred in CloudSettingsService when trying to fetch organization settings.
Solution
Added retry mechanism with exponential backoff
Enhanced error logging with network diagnostics
Comprehensive test coverage
Testing
Related Issue
Fixes #6626
Important
Improves
CloudSettingsServiceerror handling with retry logic and enhanced logging, adding comprehensive tests.CloudSettingsServicefor fetch failures, retrying up to 3 times.performNetworkDiagnostics()andfetchWithRetry()toCloudSettingsService.CloudSettingsService.test.ts.This description was created by
for 3879fd4. You can customize this summary. It will automatically update as commits are pushed.