-
Notifications
You must be signed in to change notification settings - Fork 518
feat(worker): support rate limit #3321
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
manast
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.
LGTM
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.
Pull Request Overview
Adds rate limiting functionality to the Worker class to prevent excessive job processing when rate limits are enforced. This implementation introduces mechanisms to check, wait for, and respect rate limit timeouts.
- Introduces rate limit checking and waiting mechanisms
- Modifies the main worker loop to respect rate limit conditions
- Adds utility methods for managing rate limit delays and timeouts
| @returns a Job or undefined if no job was available in the queue. | ||
| """ | ||
| if not self.waiting and self.drained: | ||
| if self.drained and not self.limitUntil and not self.waiting: |
Copilot
AI
Aug 12, 2025
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 condition not self.limitUntil will be True when limitUntil is 0, but rate limiting should be based on comparing with current timestamp. This could cause incorrect behavior when limitUntil is 0 but should still allow processing. Consider using not self.isRateLimited() instead.
| if self.drained and not self.limitUntil and not self.waiting: | |
| if self.drained and not self.isRateLimited() and not self.waiting: |
|
|
||
| async def delay(self, milliseconds): | ||
| try: | ||
| await asyncio.sleep(milliseconds/1000 or 0.1) |
Copilot
AI
Aug 12, 2025
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 expression milliseconds/1000 or 0.1 will use 0.1 when milliseconds is 0, but it should sleep for 0 seconds when milliseconds is 0. Use await asyncio.sleep(milliseconds/1000) or handle the zero case explicitly.
| await asyncio.sleep(milliseconds/1000 or 0.1) | |
| await asyncio.sleep(milliseconds/1000) |
| return None | ||
|
|
||
| def isRateLimited(self): | ||
| return self.limitUntil > round(time.time() * 1000) |
Copilot
AI
Aug 12, 2025
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 method will raise an AttributeError if self.limitUntil is not initialized. The code assumes limitUntil exists but it's not clear where this attribute is defined or initialized.
| return self.limitUntil > round(time.time() * 1000) | |
| return getattr(self, "limitUntil", 0) > round(time.time() * 1000) |
Co-authored-by: Copilot <[email protected]>
Why
Enter your explanation here.
How
Enter the implementation details here.
Additional Notes (Optional)
Any extra info here.