-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Use monotonic clock for rate limiting #8456
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
| const originalDateNow = Date.now | ||
| const mockTime = Date.now() + (mockApiConfig.rateLimitSeconds + 1) * 1000 | ||
| Date.now = vi.fn(() => mockTime) | ||
| const originalPerformanceNow = performance.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.
When overriding performance.now here, consider wrapping the override in a try…finally block so that the original function is always restored even if the test fails.
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 found some issues that need attention related to rate-limit delay computation clarity and test robustness.
| const timeSinceLastRequest = now - Task.lastGlobalApiRequestTime | ||
| const rateLimit = apiConfiguration?.rateLimitSeconds || 0 | ||
| rateLimitDelay = Math.ceil(Math.max(0, rateLimit * 1000 - timeSinceLastRequest) / 1000) | ||
| rateLimitDelay = Math.ceil(Math.min(rateLimit, Math.max(0, rateLimit * 1000 - timeSinceLastRequest) / 1000)) |
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.
P2: Now that this uses a monotonic clock, consider extracting this remaining-delay calculation into a small helper (with unit tests) to make the ms→s conversion and clamping intent explicit. For example: min(rateLimitSeconds, ceil(max(0, remainingMs)/1000)). This helps avoid future regressions and clarifies that lastGlobalApiRequestTime and now are performance.now() values (ms, monotonic).
| const originalPerformanceNow = performance.now | ||
| const mockTime = performance.now() + (mockApiConfig.rateLimitSeconds + 1) * 1000 | ||
| performance.now = vi.fn(() => mockTime) | ||
|
|
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.
P3: Instead of reassigning performance.now directly, prefer vi.spyOn(performance, 'now').mockReturnValue(...) and restore with mockRestore(). This avoids mutating globals and plays nicer with parallel tests.
Kilo PR: Kilo-Org/kilocode#2583
Related GitHub Issue
Maybe fixes: #7770
Description
We received reports from users being rate limited for long periods (longer than the maximum 60s allowed in the UI) without ever enabling the rate limiter (one user mentioned using a laptop with a bad battery). This PR aims to rectify this issue by replacing the non-monotonic
Date.nowby the monotonicperformance.now. Additionally, a check was introduced to never rate-limit for longer than the configured time.Pre-Submission Checklist
Screenshots / Videos
before:
Additional Notes
Also changed the background usage collecting to use
performance.nowGet in Touch
Christiaan in shared Slack
Important
Replace
Date.nowwithperformance.nowfor monotonic clock usage in rate limiting and add a check to prevent exceeding configured time.Date.nowwithperformance.nowinTask.tsfor monotonic clock usage in rate limiting.Task.ts.Task.spec.tsto mockperformance.nowfor simulating time passage.Task.spec.ts.This description was created by
for 56c3a7c. You can customize this summary. It will automatically update as commits are pushed.