fix: preserve indexing progress when rate limit (429) errors occur #6651
Conversation
- Modified orchestrator to detect 429 errors and skip cache/vector store cleanup - Added new translation key for rate limit error messages - Added comprehensive tests to verify the behavior - Fixes #6650
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| "fileWatcherStarted": "File watcher started.", | ||
| "fileWatcherStopped": "File watcher stopped.", | ||
| "failedDuringInitialScan": "Failed during initial scan: {{errorMessage}}", | ||
| "rateLimitError": "Indexing paused due to rate limit: {{errorMessage}}. Progress has been preserved. Please try again later.", |
There was a problem hiding this comment.
I notice the translation key is only added to the English locale. Should we add this key to all other language files (zh-CN, ja, it, pt-BR, ru, ko, nl, pl, tr, vi, zh-TW) to ensure non-English users see a proper message when encountering rate limits?
| }) | ||
|
|
||
| // Check if this is a rate limit error (429) | ||
| const statusCode = extractStatusCode(error) |
There was a problem hiding this comment.
Consider adding a telemetry event specifically for rate limit errors. This could help track how often users encounter rate limits and inform decisions about rate limit handling:
| location: "startIndexing.cleanup", | ||
| }) | ||
|
|
||
| // Check if this is a rate limit error (429) |
There was a problem hiding this comment.
Would it be helpful to add a comment explaining why 429 errors are handled differently? This could help future maintainers understand the rationale:
| // Verify that the success state was set | ||
| expect(mockStateManager.setSystemState).toHaveBeenCalledWith("Indexed", expect.any(String)) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Consider adding a test case for when returns undefined (no status code found in the error). This would ensure the default error handling path works correctly even when the error doesn't contain status information.
|
Why only 429 check in your PR? Would it be rationally to not clear vector store at all on error? But clear it only by direct user command, such a button press?? |
hannesrudolph
added
the
Issue/PR - Triage
|
The issue needs to be properly scoped first |
This PR fixes issue #6650 where codebase indexing progress was completely erased after receiving a 429 (rate limit) error from the Gemini provider.
Problem
When a 429 error occurred during indexing, the error handling logic would clear both the vector store and cache file, causing all indexing progress to be lost. Users would have to start indexing from scratch after the rate limit was lifted.
Solution
orchestrator.tsto detect 429 errors using theextractStatusCodehelperTesting
orchestrator.spec.tswith tests covering:Fixes #6650
Important
Preserve indexing progress on 429 errors by retaining cache and vector store in
orchestrator.ts, with comprehensive tests added.orchestrator.ts, preserve cache and vector store on 429 errors usingextractStatusCode.rateLimitErrorfor user notification.orchestrator.spec.tswith tests for cache preservation on 429 errors, normal cache clearing on other errors, and error handling during cleanup.extractStatusCodeusage inorchestrator.tsfor error detection.This description was created by
for 6159b31. You can customize this summary. It will automatically update as commits are pushed.