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

[edelauna]

Related GitHub Issue

Closes: #361

Description

Replaces Task.lastGlobalApiRequestTime (private static) with an injectable RateLimitClock instance, scoped per ClineProvider.

Problem: The static field was shared across all Task instances in the Node.js module. When two subtasks ran concurrently, they stomped on each other's rate-limit timestamps — causing both to see stale values and either over-delay or skip the delay entirely.

Solution:

• New src/core/task/RateLimitClock.ts — small class with getLastRequestTime() and recordRequest()
• Task accepts an optional rateLimitClock via TaskOptions (defaults to a new instance if omitted)
• ClineProvider creates one RateLimitClock per provider instance and passes it to all tasks it creates, so parent and child tasks share rate-limit state within a provider
• Removed Task.lastGlobalApiRequestTime and Task.resetGlobalApiRequestTime()

Behavioral change: Rate-limit state is now per-provider instead of global. Two providers with different API keys no longer share rate-limit timestamps. This is intentionally better — the old global sharing was overly broad.

Test Procedure

• New RateLimitClock.spec.ts — covers initial undefined state, recording, monotonic updates, and cross-instance isolation
• Updated existing Task.spec.ts rate-limit tests to use explicit shared clocks instead of static resets — all pass
• grep -rn "lastGlobalApiRequestTime\|resetGlobalApiRequestTime" src/ returns zero matches
• Full test suite: 398 files pass, 6192 tests pass (1 pre-existing unrelated failure in ShadowCheckpointService.spec.ts)

Pre-Submission Checklist

• Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
• Scope: My changes are focused on the linked issue (one major feature/fix per PR).
• Self-Review: I have performed a thorough self-review of my code.
• Testing: New and/or updated tests have been added to cover my changes (if applicable).
• Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
• Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Documentation Updates

• No documentation updates are required.

Additional Notes

Files changed:

• src/core/task/RateLimitClock.ts (new)
• src/core/task/__tests__/RateLimitClock.spec.ts (new)
• src/core/task/Task.ts (modified — 6 call sites updated)
• src/core/task/__tests__/Task.spec.ts (modified — tests use shared clocks)
• src/core/webview/ClineProvider.ts (modified — creates and injects clock)

Summary by CodeRabbit

• Improvements
  • Refactored API rate limiting to use per-task rate-limit clocks instead of a shared global timestamp, improving control across concurrent operations.
  • Enhanced throttling behavior by tracking the most recent request time per clock instance, reducing unintended waiting between independent tasks.
  • Improved coordination when restoring or creating tasks so related operations share the same rate-limit tracking.

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces a new RateLimitClock class that tracks the last request timestamp via performance.now(). Task replaces its static lastGlobalApiRequestTime field with an injectable RateLimitClock instance (defaulting to a new clock). ClineProvider creates one shared clock per provider and passes it to all task instances.

Changes

RateLimitClock Extraction

Layer / File(s)	Summary
RateLimitClock class and unit tests src/core/task/RateLimitClock.ts, src/core/task/__tests__/RateLimitClock.spec.ts	RateLimitClock class exposes getLastRequestTime(): number | undefined and recordRequest() using performance.now(), with a createRateLimitClock() factory. Four unit tests cover initial undefined state, timestamp recording, monotonicity, and instance isolation.
Task: instance field replaces static timestamp src/core/task/Task.ts	Adds optional rateLimitClock?: RateLimitClock to TaskOptions, removes private static lastGlobalApiRequestTime and resetGlobalApiRequestTime(), adds private rateLimitClock initialized from injection or factory fallback, and replaces six static call sites in recursivelyMakeClineRequests, maybeWaitForProviderRateLimit, attemptApiRequest, and backoffAndAnnounce.
ClineProvider: shared clock injected into Tasks src/core/webview/ClineProvider.ts	Adds a private rateLimitClock field initialized with createRateLimitClock() and passes it into both createTask() and createTaskWithHistoryItem() so all tasks under one provider share the same clock.
Task.spec.ts: clock-based rate-limit test updates src/core/task/__tests__/Task.spec.ts	Removes global-timestamp reset hooks, adds a new retry-backoff test using an explicit clock, and rewires all subtask rate-limiting scenarios (parent/child, multi-subtask, zero-rate-limit, no-rate-limit) to pass a shared rateLimitClock per test.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐇 No more shared clocks on the static shelf,
Each provider now ticks all by itself!
recordRequest() taps performance.now(),
Subtasks won't stomp each other — here's how:
One clock per provider, injected with care,
Rate limits respected, no stomping to spare! 🕐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: extracting RateLimitClock from Task's static state, which is the primary refactoring objective.
Description check	✅ Passed	The PR description is comprehensive and follows the template. It includes the related issue, clear problem statement, solution overview, test procedure, completed pre-submission checklist, and documentation impact assessment.
Linked Issues check	✅ Passed	The PR successfully implements all objectives from issue #361: creates RateLimitClock with required methods, removes static fields/methods from Task, injects clock via TaskOptions, and passes clock from ClineProvider to all tasks.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue #361 objectives. The modifications to RateLimitClock.ts, Task.ts, ClineProvider.ts, and test files are all necessary to implement the required refactoring.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue/361

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/core/task/Task.ts	88.88%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[navedmerchant]

Nit (test strength): This test is named "should respect rate limit window in
retry backoff" and the inline comment explains that with rateLimitSeconds=10
and base backoff requestDelaySeconds=3, the 10s rate-limit window should
dominate the delay. But the assertions only check that a api_req_retry_delayed
message was emitted and that clock.getLastRequestTime() is defined — neither
confirms the delay actually reflected the rate-limit window. The test would
pass even if the backoff used the 3s base value (or any non-zero delay), so it
doesn't really prove the stated behavior.
Consider asserting on the computed delay so a regression in the
window-vs-backoff logic would fail here. Since delay is already mocked as
mockDelay, something like:

// The rate-limit window (10s) should dominate the base exponential backoff
(3s)
expect(mockDelay).toHaveBeenCalledWith(10000)

or assert the countdown ran for ~10 iterations / parse the seconds out of
the api_req_retry_delayed payload. That ties the test to its intent. Not
blocking — current coverage is adequate, just weaker than the name implies.

If you want it tighter/less wordy for an inline review thread, the one-liner
version:

Assertions here only check that a retry-delay message fired and the clock is
set — they don't verify the delay reflected the 10s window vs the 3s base
backoff, so the test would pass even if the window logic regressed. Since
delay is mocked, consider expect(mockDelay).toHaveBeenCalledWith(10000) (or
asserting the countdown length) to match the test's stated intent.
Non-blocking.

[navedmerchant]

A small nit comment, but non blocking, approved

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/core/task/Task.ts`:
- Around line 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.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ce77650f-1e4e-44b6-8899-d7b43d2648d0

📥 Commits

Reviewing files that changed from the base of the PR and between 2074748 and 007689e.

📒 Files selected for processing (5)

• src/core/task/RateLimitClock.ts
• src/core/task/Task.ts
• src/core/task/__tests__/RateLimitClock.spec.ts
• src/core/task/__tests__/Task.spec.ts
• src/core/webview/ClineProvider.ts

🚧 Files skipped from review as they are similar to previous changes (4)

• src/core/task/tests/RateLimitClock.spec.ts
• src/core/task/RateLimitClock.ts
• src/core/webview/ClineProvider.ts
• src/core/task/tests/Task.spec.ts

[coderabbitai]

⚠️ 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.