diff --git a/src/core/task/RateLimitClock.ts b/src/core/task/RateLimitClock.ts new file mode 100644 index 0000000000..358e2b9216 --- /dev/null +++ b/src/core/task/RateLimitClock.ts @@ -0,0 +1,15 @@ +export class RateLimitClock { + private lastRequestTime?: number + + getLastRequestTime(): number | undefined { + return this.lastRequestTime + } + + recordRequest(): void { + this.lastRequestTime = performance.now() + } +} + +export function createRateLimitClock(): RateLimitClock { + return new RateLimitClock() +} diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4de3e595fc..cbbb87206d 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -6,6 +6,7 @@ import { v7 as uuidv7 } from "uuid" import EventEmitter from "events" import { AskIgnoredError } from "./AskIgnoredError" +import { RateLimitClock, createRateLimitClock } from "./RateLimitClock" import { Anthropic } from "@anthropic-ai/sdk" import OpenAI from "openai" @@ -158,6 +159,7 @@ export interface TaskOptions extends CreateTaskOptions { workspacePath?: string /** Initial status for the task's history item (e.g., "active" for child tasks) */ initialStatus?: "active" | "delegated" | "completed" + rateLimitClock?: RateLimitClock } export class Task extends EventEmitter implements TaskLike { @@ -284,17 +286,9 @@ export class Task extends EventEmitter implements TaskLike { // API apiConfiguration: ProviderSettings api: ApiHandler - private static lastGlobalApiRequestTime?: number + private rateLimitClock: RateLimitClock private autoApprovalHandler: AutoApprovalHandler - /** - * Reset the global API request timestamp. This should only be used for testing. - * @internal - */ - static resetGlobalApiRequestTime(): void { - Task.lastGlobalApiRequestTime = undefined - } - toolRepetitionDetector: ToolRepetitionDetector rooIgnoreController?: RooIgnoreController rooProtectedController?: RooProtectedController @@ -437,6 +431,7 @@ export class Task extends EventEmitter implements TaskLike { initialTodos, workspacePath, initialStatus, + rateLimitClock, }: TaskOptions) { super() @@ -486,6 +481,7 @@ export class Task extends EventEmitter implements TaskLike { this.apiConfiguration = apiConfiguration this.api = buildApiHandler(this.apiConfiguration) + this.rateLimitClock = rateLimitClock ?? createRateLimitClock() this.autoApprovalHandler = new AutoApprovalHandler() this.consecutiveMistakeLimit = consecutiveMistakeLimit ?? DEFAULT_CONSECUTIVE_MISTAKE_LIMIT @@ -2455,12 +2451,12 @@ export class Task extends EventEmitter implements TaskLike { // This prevents the UI from showing an "API Request..." spinner while we are // intentionally waiting due to the rate limit slider. // - // NOTE: We also set Task.lastGlobalApiRequestTime here to reserve this slot - // before we build environment details (which can take time). - // This ensures subsequent requests (including subtasks) still honour the + // NOTE: We also record the request time here to reserve this slot before + // we build environment details (which can take time). This ensures + // subsequent requests (including subtasks) still honour the // provider rate-limit window. await this.maybeWaitForProviderRateLimit(currentItem.retryAttempt ?? 0) - Task.lastGlobalApiRequestTime = performance.now() + this.rateLimitClock.recordRequest() await this.say( "api_req_started", @@ -3854,12 +3850,13 @@ export class Task extends EventEmitter implements TaskLike { const rateLimitSeconds = state?.apiConfiguration?.rateLimitSeconds ?? this.apiConfiguration?.rateLimitSeconds ?? 0 - if (rateLimitSeconds <= 0 || !Task.lastGlobalApiRequestTime) { + const lastRequestTime = this.rateLimitClock.getLastRequestTime() + if (rateLimitSeconds <= 0 || !lastRequestTime) { return } const now = performance.now() - const timeSinceLastRequest = now - Task.lastGlobalApiRequestTime + const timeSinceLastRequest = now - lastRequestTime const rateLimitDelay = Math.ceil( Math.min(rateLimitSeconds, Math.max(0, rateLimitSeconds * 1000 - timeSinceLastRequest) / 1000), ) @@ -3907,7 +3904,7 @@ export class Task extends EventEmitter implements TaskLike { // timestamp earlier to include the environment details build. We still set it // here for direct callers (tests) and for the case where we didn't rate-limit // in the caller. - Task.lastGlobalApiRequestTime = performance.now() + this.rateLimitClock.recordRequest() const systemPrompt = await this.getSystemPrompt() const { contextTokens } = this.getTokenUsage() @@ -4282,8 +4279,9 @@ export class Task extends EventEmitter implements TaskLike { // Respect provider rate limit window let rateLimitDelay = 0 const rateLimit = (state?.apiConfiguration ?? this.apiConfiguration)?.rateLimitSeconds || 0 - if (Task.lastGlobalApiRequestTime && rateLimit > 0) { - const elapsed = performance.now() - Task.lastGlobalApiRequestTime + const lastRequestTime = this.rateLimitClock.getLastRequestTime() + if (lastRequestTime && rateLimit > 0) { + const elapsed = performance.now() - lastRequestTime rateLimitDelay = Math.ceil(Math.min(rateLimit, Math.max(0, rateLimit * 1000 - elapsed) / 1000)) } diff --git a/src/core/task/__tests__/RateLimitClock.spec.ts b/src/core/task/__tests__/RateLimitClock.spec.ts new file mode 100644 index 0000000000..3a70bc75ea --- /dev/null +++ b/src/core/task/__tests__/RateLimitClock.spec.ts @@ -0,0 +1,35 @@ +import { RateLimitClock, createRateLimitClock } from "../RateLimitClock" + +describe("RateLimitClock", () => { + it("returns undefined when no request has been recorded", () => { + const clock = createRateLimitClock() + expect(clock.getLastRequestTime()).toBeUndefined() + }) + + it("records a request and returns a timestamp", () => { + const clock = createRateLimitClock() + clock.recordRequest() + const time = clock.getLastRequestTime() + expect(time).toBeDefined() + expect(time).toBeGreaterThan(0) + }) + + it("updates timestamp on subsequent calls", () => { + const clock = createRateLimitClock() + clock.recordRequest() + const first = clock.getLastRequestTime()! + clock.recordRequest() + const second = clock.getLastRequestTime()! + expect(second).toBeGreaterThanOrEqual(first) + }) + + it("isolates state between different clocks", () => { + const clock1 = createRateLimitClock() + const clock2 = createRateLimitClock() + + clock1.recordRequest() + + expect(clock1.getLastRequestTime()).toBeDefined() + expect(clock2.getLastRequestTime()).toBeUndefined() + }) +}) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 93f1fd1112..dd63135802 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -10,6 +10,7 @@ import type { GlobalState, ProviderSettings, ModelInfo } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { Task } from "../Task" +import { createRateLimitClock } from "../RateLimitClock" import { ClineProvider } from "../../webview/ClineProvider" import { ApiStreamChunk } from "../../../api/transform/stream" import { ContextProxy } from "../../config/ContextProxy" @@ -400,14 +401,6 @@ describe("Cline", () => { describe("getEnvironmentDetails", () => { describe("API conversation handling", () => { - beforeEach(() => { - Task.resetGlobalApiRequestTime() - }) - - afterEach(() => { - Task.resetGlobalApiRequestTime() - }) - it("should strip non-protocol fields from API conversation history before sending to the API", async () => { const cline = new Task({ provider: mockProvider, @@ -669,6 +662,86 @@ describe("Cline", () => { expect(mockDelay).toHaveBeenCalledWith(1000) }) + it("should respect rate limit window in retry backoff", async () => { + const clock = createRateLimitClock() + const rateLimitConfig = { + ...mockApiConfig, + rateLimitSeconds: 10, + } + const cline = new Task({ + provider: mockProvider, + apiConfiguration: rateLimitConfig, + task: "test task", + startTask: false, + rateLimitClock: clock, + }) + vi.spyOn(cline as any, "getSystemPrompt").mockResolvedValue("mock system prompt") + + const mockDelay = vi.fn().mockResolvedValue(undefined) + vi.spyOn(await import("delay"), "default").mockImplementation(mockDelay) + + const saySpy = vi.spyOn(cline, "say") + + const mockError = new Error("API Error") + const mockFailedStream = { + // eslint-disable-next-line require-yield + async *[Symbol.asyncIterator]() { + throw mockError + }, + async next() { + throw mockError + }, + async return() { + return { done: true, value: undefined } + }, + async throw(e: any) { + throw e + }, + async [Symbol.asyncDispose]() {}, + } as AsyncGenerator + + const mockSuccessStream = { + async *[Symbol.asyncIterator]() { + yield { type: "text", text: "Success" } + }, + async next() { + return { done: true, value: { type: "text", text: "Success" } } + }, + async return() { + return { done: true, value: undefined } + }, + async throw(e: any) { + throw e + }, + async [Symbol.asyncDispose]() {}, + } as AsyncGenerator + + let firstAttempt = true + vi.spyOn(cline.api, "createMessage").mockImplementation(() => { + if (firstAttempt) { + firstAttempt = false + return mockFailedStream + } + return mockSuccessStream + }) + const providerState = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + ...providerState, + apiConfiguration: rateLimitConfig, + autoApprovalEnabled: true, + requestDelaySeconds: 3, + }) + + const iterator = cline.attemptApiRequest(0) + await iterator.next() + + // rateLimitSeconds=10 > exponentialDelay=ceil(3*2^0)=3, so + // finalDelay=10 and the countdown loop fires delay(1000) ten times. + expect(mockDelay).toHaveBeenCalledWith(1000) + expect(mockDelay).toHaveBeenCalledTimes(10) + expect(clock.getLastRequestTime()).toBeDefined() + }) + it("should not apply retry delay twice", async () => { const cline = new Task({ provider: mockProvider, @@ -844,9 +917,6 @@ describe("Cline", () => { beforeEach(() => { vi.clearAllMocks() - // Reset the global timestamp before each test - Task.resetGlobalApiRequestTime() - mockApiConfig = { apiProvider: "anthropic", apiKey: "test-key", @@ -880,21 +950,20 @@ describe("Cline", () => { mockDelay.mockClear() }) - afterEach(() => { - // Clean up the global state after each test - Task.resetGlobalApiRequestTime() - }) - it("should enforce rate limiting across parent and subtask", async () => { // Add a spy to track getState calls const getStateSpy = vi.spyOn(mockProvider, "getState") + // Shared clock so parent and child see each other's timestamps + const sharedClock = createRateLimitClock() + // Create parent task const parent = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "parent task", startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(parent as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -924,7 +993,7 @@ describe("Cline", () => { // Verify no delay was applied for the first request expect(mockDelay).not.toHaveBeenCalled() - // Create a subtask immediately after + // Create a subtask immediately after, sharing the same clock const child = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, @@ -932,6 +1001,7 @@ describe("Cline", () => { parentTask: parent, rootTask: parent, startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(child as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -978,12 +1048,15 @@ describe("Cline", () => { }, 10000) // Increase timeout to 10 seconds it("should not apply rate limiting if enough time has passed", async () => { + const sharedClock = createRateLimitClock() + // Create parent task const parent = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "parent task", startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(parent as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1023,6 +1096,7 @@ describe("Cline", () => { parentTask: parent, rootTask: parent, startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(child as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1040,12 +1114,15 @@ describe("Cline", () => { }) it("should share rate limiting across multiple subtasks", async () => { + const sharedClock = createRateLimitClock() + // Create parent task const parent = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "parent task", startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(parent as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1080,6 +1157,7 @@ describe("Cline", () => { parentTask: parent, rootTask: parent, startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(child1 as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1104,6 +1182,7 @@ describe("Cline", () => { parentTask: parent, rootTask: parent, startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(child2 as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1125,12 +1204,15 @@ describe("Cline", () => { mcpEnabled: false, }) + const sharedClock = createRateLimitClock() + // Create parent task const parent = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "parent task", startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(parent as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1165,6 +1247,7 @@ describe("Cline", () => { parentTask: parent, rootTask: parent, startTask: false, + rateLimitClock: sharedClock, }) vi.spyOn(child as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1178,13 +1261,16 @@ describe("Cline", () => { expect(mockDelay).not.toHaveBeenCalled() }) - it("should update global timestamp even when no rate limiting is needed", async () => { + it("should update clock timestamp even when no rate limiting is needed", async () => { + const clock = createRateLimitClock() + // Create task const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "test task", startTask: false, + rateLimitClock: clock, }) vi.spyOn(task as any, "getSystemPrompt").mockResolvedValue("mock system prompt") @@ -1211,10 +1297,9 @@ describe("Cline", () => { const iterator = task.attemptApiRequest(0) await iterator.next() - // Access the private static property via reflection for testing - const globalTimestamp = (Task as any).lastGlobalApiRequestTime - expect(globalTimestamp).toBeDefined() - expect(globalTimestamp).toBeGreaterThan(0) + const lastTime = clock.getLastRequestTime() + expect(lastTime).toBeDefined() + expect(lastTime).toBeGreaterThan(0) }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index dc19a58ce2..414f6c6887 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -47,6 +47,7 @@ import { getModelId, isRetiredProvider, } from "@roo-code/types" +import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock" import { aggregateTaskCostsRecursive, type AggregatedCosts } from "./aggregateTaskCosts" import { TelemetryService } from "@roo-code/telemetry" import { CloudService, getRooCodeApiUrl } from "@roo-code/cloud" @@ -167,6 +168,7 @@ export class ClineProvider private taskEventListeners: WeakMap void>> = new WeakMap() private currentWorkspacePath: string | undefined private _disposed = false + private readonly rateLimitClock: RateLimitClock = createRateLimitClock() private recentTasksCache?: string[] public readonly taskHistoryStore: TaskHistoryStore @@ -1093,6 +1095,7 @@ export class ClineProvider startTask: options?.startTask ?? true, // Preserve the status from the history item to avoid overwriting it when the task saves messages initialStatus: historyItem.status, + rateLimitClock: this.rateLimitClock, }) if (isRehydratingCurrentTask) { @@ -3064,6 +3067,7 @@ export class ClineProvider // its initial state update, so state.currentTaskId is available ASAP. startTask: false, ...options, + rateLimitClock: this.rateLimitClock, }) await this.addClineToStack(task)