Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions src/core/task/RateLimitClock.ts
Original file line number Diff line number Diff line change
@@ -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()
}
34 changes: 16 additions & 18 deletions src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -437,6 +431,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
initialTodos,
workspacePath,
initialStatus,
rateLimitClock,
}: TaskOptions) {
super()

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Comment on lines +3853 to 3855

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use explicit undefined checks for last-request timestamps.

Truthy/falsy checks on lastRequestTime mis-handle a valid 0 timestamp and can incorrectly bypass rate-limit behavior.

Suggested fix
-		const lastRequestTime = this.rateLimitClock.getLastRequestTime()
-		if (rateLimitSeconds <= 0 || !lastRequestTime) {
+		const lastRequestTime = this.rateLimitClock.getLastRequestTime()
+		if (rateLimitSeconds <= 0 || lastRequestTime === undefined) {
 			return
 		}
...
-		const lastRequestTime = this.rateLimitClock.getLastRequestTime()
-		if (lastRequestTime && rateLimit > 0) {
+		const lastRequestTime = this.rateLimitClock.getLastRequestTime()
+		if (lastRequestTime !== undefined && rateLimit > 0) {
 			const elapsed = performance.now() - lastRequestTime
 			rateLimitDelay = Math.ceil(Math.min(rateLimit, Math.max(0, rateLimit * 1000 - elapsed) / 1000))
 		}

Also applies to: 4282-4284

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/task/Task.ts` around lines 3853 - 3855, The condition checking
`!lastRequestTime` uses a falsy comparison which incorrectly treats a valid `0`
timestamp as a missing value and skips rate-limit enforcement. Replace the falsy
check `!lastRequestTime` with an explicit undefined check using `lastRequestTime
=== undefined` (or `lastRequestTime == null` if you want to handle both null and
undefined). Apply this same fix to both the occurrence in the rate-limit check
block around line 3853-3855 and the additional occurrence mentioned at lines
4282-4284 in the same file.

}

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),
)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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))
}

Expand Down
35 changes: 35 additions & 0 deletions src/core/task/__tests__/RateLimitClock.spec.ts
Original file line number Diff line number Diff line change
@@ -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()
})
})
Loading
Loading