-
Notifications
You must be signed in to change notification settings - Fork 33
[BR-1794]: Add rate limit retry mechanism with exponential backoff #1834
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
base: master
Are you sure you want to change the base?
Conversation
Implement retry utility to handle 429 rate limit errors with configurable backoff strategy, extract rate limit info from response headers, and apply it to public shared folder content requests to improve reliability under rate limiting conditions.
Deploying drive-web with
|
| Latest commit: |
34d5f3b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82de3e28.drive-web.pages.dev |
| Branch Preview URL: | https://feature-retry-with-backoff.drive-web.pages.dev |
CandelR
left a comment
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 accompanied by some kind of visual information for the user? I don't see the point of this PR from the user's point of view, as they will be waiting at least 60 seconds without knowing what is happening, which is an enormous amount of time.
| return typeof error === 'object' && error !== null; | ||
| } | ||
|
|
||
| function isRateLimitError(error: unknown): boolean { |
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.
Use arrow functions to be consistent with rest of the codebase
| return ( | ||
| error.status === 429 || | ||
| error.statusCode === 429 || | ||
| error.response?.status === 429 || |
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.
If I remember correctly, there are status codes, use them or add this one if it is not there
| return undefined; | ||
| } | ||
|
|
||
| const resetValueMs = Number.parseInt(headers['x-ratelimit-reset'], 10); |
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 unit that return the server is in miliseconds?
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.
yes
| } | ||
| } | ||
|
|
||
| throw new Error('Maximum retries exceeded'); |
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 error reachable? Check it with tests
| maxRetries: 5, | ||
| maxDelay: 60000, | ||
| onRetry: (attempt, delay) => { | ||
| console.log( |
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.
maybe better use console warn if want to log the retry for when user records the console and send to us
| if (!isErrorWithStatus(error) || !error.headers) { | ||
| return undefined; | ||
| } |
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.
this is already checked in isRateLimitError, I think that never will reach this code
| }; | ||
| } | ||
|
|
||
| export async function retryWithBackoff<T>(fn: () => Promise<T>, options: RetryOptions = {}): Promise<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.
add jsdoc to this exported function
- Remove maxDelay option and related fallback logic - Simplify rate limit detection to only check status codes (429) - Update rate limit header from 'x-ratelimit-reset' to 'x-internxt-ratelimit-reset' - Remove limit and remaining fields from RateLimitInfo interface - Add user-facing toast notification on first retry attempt - Convert functions to arrow functions for consistency - Add HTTP_CODES.TOO_MANY_REQUESTS constant - Add JSDoc documentation to retryWithBackoff function - Add internationalization support for retry notification - Update tests to reflect new behavior and validate toast integration - Fix loop condition to prevent unreachable code
52a8dc6 to
cd0f3ad
Compare
|
|
||
| if (!hasShownRateLimitToast) { | ||
| hasShownRateLimitToast = true; | ||
| notificationsService.show({ |
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 would not mix UI related things with pure logic like this function. Return something that allows the function consumer to determine if showing this notification or not proceeds
56215d9 to
6b7760c
Compare
| "link-updated": "Linkeinstellungen aktualisiert", | ||
| "link-deleted": "Link gelöscht", | ||
| "links-deleted": "Link(s) gelöscht" | ||
| "links-deleted": "Link(s) gelöscht", |
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.
Seems that here is missing error-deleting-links translation, can you add it? :)
| hasShownRateLimitNotification = true; | ||
| notificationsService.show({ | ||
| text: t('shared-links.toast.rate-limit-retry'), | ||
| type: ToastType.Warning, | ||
| duration: Infinity, | ||
| }); |
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.
Move this out of the service, return the error, and handle it in the manager component of the UI to decide what to display. This way, we try to keep it clean. Even if there is a function with the notificationService in this service, we will try not to mix it up any further :)
|



Description
Implement retry utility to handle 429 rate limit errors with configurable backoff strategy, extract rate limit info
from response headers, and apply it to public shared folder content requests to improve reliability under rate
limiting conditions.
Related Issues
Related Pull Requests
Checklist
Testing Process
Additional Notes