Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 27, 2025
Merged

Use monotonic clock for rate limiting #8456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/core/task/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2036,7 +2036,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {

const drainStreamInBackgroundToFindAllUsage = async (apiReqIndex: number) => {
const timeoutMs = DEFAULT_USAGE_COLLECTION_TIMEOUT_MS
const startTime = Date.now()
const startTime = performance.now()
const modelId = getModelId(this.apiConfiguration)

// Local variables to accumulate usage data without affecting the main flow
Expand Down Expand Up @@ -2107,7 +2107,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
// Use the same iterator that the main loop was using
while (!item.done) {
// Check for timeout
if (Date.now() - startTime > timeoutMs) {
if (performance.now() - startTime > timeoutMs) {
console.warn(
`[Background Usage Collection] Timed out after ${timeoutMs}ms for model: ${modelId}, processed ${chunkCount} chunks`,
)
Expand Down Expand Up @@ -2559,10 +2559,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
// Use the shared timestamp so that subtasks respect the same rate-limit
// window as their parent tasks.
if (Task.lastGlobalApiRequestTime) {
const now = Date.now()
const now = performance.now()
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))
Copy link

@roomote roomote bot Oct 2, 2025

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).

}

// Only show rate limiting message if we're not retrying. If retrying, we'll include the delay there.
Expand All @@ -2577,7 +2577,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {

// Update last request time before making the request so that subsequent
// requests — even from new subtasks — will honour the provider's rate-limit.
Task.lastGlobalApiRequestTime = Date.now()
Task.lastGlobalApiRequestTime = performance.now()

const systemPrompt = await this.getSystemPrompt()
this.lastUsedInstructions = systemPrompt
Expand Down
10 changes: 5 additions & 5 deletions src/core/task/__tests__/Task.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,9 +1098,9 @@ describe("Cline", () => {
await parentIterator.next()

// Simulate time passing (more than rate limit)
const originalDateNow = Date.now
const mockTime = Date.now() + (mockApiConfig.rateLimitSeconds + 1) * 1000
Date.now = vi.fn(() => mockTime)
const originalPerformanceNow = performance.now
Copy link

@ellipsis-dev ellipsis-dev bot Oct 2, 2025

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.

const mockTime = performance.now() + (mockApiConfig.rateLimitSeconds + 1) * 1000
performance.now = vi.fn(() => mockTime)

Copy link

@roomote roomote bot Oct 2, 2025

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.

// Create a subtask after time has passed
const child = new Task({
Expand All @@ -1121,8 +1121,8 @@ describe("Cline", () => {
// Verify no rate limiting was applied
expect(mockDelay).not.toHaveBeenCalled()

// Restore Date.now
Date.now = originalDateNow
// Restore performance.now
performance.now = originalPerformanceNow
})

it("should share rate limiting across multiple subtasks", async () => {
Expand Down