-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent duplicate retry messages when API returns retry info #6542
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
- Check if error message already contains retry-related information - Avoid adding duplicate "Retry attempt X" and "Retrying in Y seconds" messages - Add test coverage for the scenario Fixes #6541
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine. I've reviewed the changes and left some suggestions inline.
|
|
||
| // Check if the error message already contains retry-related information | ||
| // This prevents duplicate retry messages when the API itself returns retry information | ||
| const containsRetryInfo = /retry|retrying|attempt/i.test(errorMsg) |
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 regex pattern intentional? It could match partial words like 'country' containing 'retry'. Consider using word boundaries:
|
|
||
| // Check if the error message already contains retry-related information | ||
| // This prevents duplicate retry messages when the API itself returns retry information | ||
| const containsRetryInfo = /retry|retrying|attempt/i.test(errorMsg) |
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 extract this regex to a constant for better maintainability? Something like:
| const saySpy = vi.spyOn(cline, "say").mockResolvedValue(undefined) | ||
|
|
||
| // Create an error that already contains retry information | ||
| const mockError = new Error("Engine loop is not running. Retry attempt 1\nRetrying now...") |
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.
Great test coverage! Consider adding edge cases to make it even more robust:
- Error messages with 'retry' as part of another word (e.g., 'country')
- Multiple retry-related keywords in the same error
- Case sensitivity verification
This would ensure the regex behaves exactly as intended.
|
Closing, not an issue |
Summary
This PR fixes an issue where Roo Code was displaying duplicate retry messages when using OpenAI Compatible API providers that return errors already containing retry information.
Problem
When using certain API providers (like the Qwen3-235B-A22B model mentioned in the issue), the API itself returns error messages that contain retry-related text such as "Retry attempt 1" and "Retrying now...". Roo Code was then adding its own retry messages on top of these, creating confusing duplicate messages for users.
Solution
Testing
Fixes #6541
Important
Fixes duplicate retry messages in
Task.tsby checking for existing retry info in error messages and adjusts retry message logic accordingly.Task.ts, added regex check for retry-related info in error messages to prevent duplicate retry messages.should not duplicate retry messages when error already contains retry infoinTask.spec.ts.This description was created by
for c74d63d. You can customize this summary. It will automatically update as commits are pushed.