refactor(Task): extract RateLimitClock from Task static state (#361) (#628)

* refactor(Task): extract RateLimitClock from Task static state

* test(Task): hardening ratelimit spec

Author: edelauna

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()
+}

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<TaskEvents> implements TaskLike {
@@ -284,17 +286,9 @@ export class Task extends EventEmitter<TaskEvents> 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<TaskEvents> implements TaskLike {
 		initialTodos,
 		workspacePath,
 		initialStatus,
+		rateLimitClock,
 	}: TaskOptions) {
 		super()
 
@@ -486,6 +481,7 @@ export class Task extends EventEmitter<TaskEvents> 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<TaskEvents> 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<TaskEvents> 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<TaskEvents> 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<TaskEvents> 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))
 			}
 

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()
+	})
+})