fix: add jitter to rate-limit backoff

[ndycode]

Problem

The runtime 429 backoff was purely exponential.

That means multiple clients or parallel workers can retry in lockstep after the same upstream rate limit window, creating avoidable bursts.

Fix

Add bounded jitter when a new rate-limit backoff window is created.

Duplicate 429s inside the dedup window continue to reuse the same stored delay so a single retry window stays stable instead of being re-randomized on every duplicate response.

Changes

• lib/request/rate-limit-backoff.ts
  • add bounded jitter for new backoff windows
  • store the most recent jittered delay in state
  • return the stored delay for duplicate 429s inside the dedup window
• test/rate-limit-backoff.test.ts
  • make the suite deterministic by mocking Math.random() to the neutral midpoint
  • add regression coverage for jittered first attempts and stable duplicate delays

Validation

npx vitest run test/rate-limit-backoff.test.ts
Test Files  1 passed (1)
Tests      25 passed (25)

npx vitest run
Test Files  222 passed (222)
Tests      3314 passed (3314)

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr adds bounded ±20% jitter to the exponential rate-limit backoff in lib/request/rate-limit-backoff.ts and stores the jittered delay on state so duplicate 429s within the dedup window return the same stable value instead of recomputing.

key changes:

• addBackoffJitter(baseMs) applies ±20% of baseMs using Math.random() * 2 - 1
• RateLimitState gains lastDelayMs to cache the jittered delay
• duplicate returns now read previous.lastDelayMs instead of recalculating
• Math.random() is globally mocked to 0.5 in tests for determinism; a new regression test covers jittered-first and stable-duplicate paths

issues found:

• negative jitter is silently suppressed for attempt=1: backoffDelay === baseDelay on the first 429, so Math.max(baseDelay, jitteredDelay) clamps any downward jitter to zero. the effective spread for attempt=1 is [baseDelay, baseDelay*1.2], not the symmetric ±20% the constant implies
• missing vitest branch: no test exercises attempt=1 with Math.random()=0 to confirm the floor behaviour is intentional
• mockReturnValueOnce(1) tests an unreachable Math.random() value ([0,1) never includes 1); maximum achievable is ~0.9999
• no test exercises non-neutral jitter through getRateLimitBackoffWithReason, leaving the compounded effect of jitter + calculateBackoffMs untested

Confidence Score: 3/5

safe to merge with low risk — logic change is narrow, 25 tests pass, but asymmetric attempt=1 jitter partially defeats the stated anti-thundering-herd goal and missing branches leave the floor behaviour undocumented

core backoff logic is correct, the dedup path is stable, and no concurrency issues are introduced (getRateLimitBackoff is synchronous, module-level Map is fine under Node.js single-thread model). score is 3 rather than 4 because the one-sided jitter on attempt=1 (the most common 429 case) quietly reduces spreading effectiveness, the missing vitest branches conflict with the project's 80%+ coverage requirement, and the test uses Math.random()=1 which is an unreachable value in production

lib/request/rate-limit-backoff.ts line 82 (Math.max floor asymmetry for attempt=1) and test/rate-limit-backoff.test.ts (missing attempt=1 negative-jitter branch and non-neutral jitter through getRateLimitBackoffWithReason)

Important Files Changed

Filename	Overview
lib/request/rate-limit-backoff.ts	adds bounded ±20% jitter to new backoff windows and stores last delay for stable duplicate returns; jitter is asymmetrically suppressed for attempt=1 due to Math.max(baseDelay, jitteredDelay) floor — only upward jitter applies on first 429
test/rate-limit-backoff.test.ts	adds global Math.random mock (0.5 neutral) and a new jitter regression case, but misses attempt=1 with negative jitter, uses an unreachable Math.random()=1 value, and lacks non-neutral jitter coverage through getRateLimitBackoffWithReason

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getRateLimitBackoff called] --> B[pruneStaleRateLimitState]
    B --> C{previous state\nexists?}
    C -->|yes| D{now - lastAt\n< DEDUP_WINDOW 2s?}
    D -->|yes - isDuplicate| E[return previous.lastDelayMs\nisDuplicate: true]
    D -->|no| F{now - lastAt\n< RESET_MS 120s?}
    F -->|yes| G[attempt = consecutive429 + 1]
    F -->|no| H[attempt = 1]
    C -->|no| H
    G --> I[backoffDelay = min baseDelay × 2^attempt-1, MAX_60s]
    H --> I
    I --> J[addBackoffJitter\nbaseMs ± 20% via Math.random]
    J --> K[jitteredDelay = min jittered, MAX_60s]
    K --> L{jitteredDelay\n< baseDelay?}
    L -->|yes — suppressed on attempt=1| M[delayMs = baseDelay]
    L -->|no| N[delayMs = jitteredDelay]
    M --> O[store lastDelayMs in state]
    N --> O
    O --> P[return delayMs\nisDuplicate: false]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/request/rate-limit-backoff.ts
Line: 80-82

Comment:
**negative jitter silently discarded on attempt=1**

for attempt=1, `backoffDelay === baseDelay`. if `Math.random()` returns a value below `0.5`, `jitteredDelay < baseDelay`, and the `Math.max` floor restores it to `baseDelay` — so downward jitter has no effect on the first 429. the effective spread is `[baseDelay, baseDelay×1.2]` (one-sided upward), not the symmetric `±20%` the constant implies.

this is the most common 429 case, so the anti-thundering-herd goal stated in the PR description is only half-realised for first retries. attempt=2+ are unaffected because `backoffDelay >> baseDelay` and symmetric jitter applies.

if the `Math.max` floor is intentional (to honour the server-sent `Retry-After` as a hard minimum), add a comment:

```suggestion
	const backoffDelay = Math.min(baseDelay * Math.pow(2, attempt - 1), MAX_BACKOFF_MS);
	const jitteredDelay = Math.min(addBackoffJitter(backoffDelay), MAX_BACKOFF_MS);
	// Math.max honours server Retry-After as a hard floor; on attempt=1 this makes jitter one-sided (upward only)
	const delayMs = Math.max(baseDelay, jitteredDelay);
```

or remove the floor to get symmetric spread:

```typescript
const delayMs = Math.max(0, jitteredDelay);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/rate-limit-backoff.test.ts
Line: 44-60

Comment:
**missing vitest branch: attempt=1 with maximum negative jitter**

the new test only mocks `Math.random()` to `1` for attempt=1 (maximum positive jitter → `1200ms`). two gaps:

1. `mockReturnValueOnce(1)` is unreachable in production — `Math.random()` returns `[0, 1)`, so 1 is never produced. the assertion `1200` is therefore a value that can never actually occur. use `0.999` or similar to stay within the real range.

2. there is no case for attempt=1 with `Math.random()=0` (maximum negative jitter). this is the branch where `jitteredDelay = 800 < baseDelay = 1000` and the `Math.max` floor activates. the project requires 80%+ branch coverage — add a test to document whether this floor is intentional:

```typescript
it("clamps negative jitter to baseDelay on attempt=1", () => {
	vi.mocked(Math.random).mockReturnValueOnce(0);
	const result = getRateLimitBackoff(5, "floor-test", 1000);
	// jitteredDelay = 800, but Math.max(1000, 800) = 1000
	expect(result.delayMs).toBe(1000);
	expect(result.attempt).toBe(1);
});
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/rate-limit-backoff.test.ts
Line: 150-188

Comment:
**no vitest coverage for jitter × `getRateLimitBackoffWithReason`**

all tests in this block run with `Math.random()` globally mocked to `0.5` (neutral jitter = 0). `getRateLimitBackoffWithReason` wraps `getRateLimitBackoff` and then calls `calculateBackoffMs(result.delayMs, result.attempt, reason)`, which multiplies the already-jittered delay by `2^(attempt-1) × multiplier`. with neutral jitter the compounded output is always deterministic, but in production a non-zero jitter flows through the multiplier.

add at least one test with a non-neutral mock to pin the expected output and document intent:

```typescript
it("jitter propagates through calculateBackoffMs", () => {
	vi.mocked(Math.random).mockReturnValueOnce(1); // +20% jitter
	const result = getRateLimitBackoffWithReason(6, "jitter-reason", 1000, "tokens");
	// getRateLimitBackoff: delayMs = max(1000, 1200) = 1200
	// calculateBackoffMs(1200, 1, "tokens") = floor(1200 * 1 * 1.5) = 1800
	expect(result.delayMs).toBe(1800);
	expect(result.reason).toBe("tokens");
});
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix: add jitter to rate-limit backoff" | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 35 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 35 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dcded8b5-0aef-4076-8b19-4a216c8f78df

📥 Commits

Reviewing files that changed from the base of the PR and between 478f44c and 6969815.

📒 Files selected for processing (2)

• lib/request/rate-limit-backoff.ts
• test/rate-limit-backoff.test.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/jitter-rate-limit-backoff

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/jitter-rate-limit-backoff

---

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

[greptile-apps]

negative jitter silently discarded on attempt=1

for attempt=1, backoffDelay === baseDelay. if Math.random() returns a value below 0.5, jitteredDelay < baseDelay, and the Math.max floor restores it to baseDelay — so downward jitter has no effect on the first 429. the effective spread is [baseDelay, baseDelay×1.2] (one-sided upward), not the symmetric ±20% the constant implies.

this is the most common 429 case, so the anti-thundering-herd goal stated in the PR description is only half-realised for first retries. attempt=2+ are unaffected because backoffDelay >> baseDelay and symmetric jitter applies.

if the Math.max floor is intentional (to honour the server-sent Retry-After as a hard minimum), add a comment:

Suggested change

	const backoffDelay = Math.min(baseDelay * Math.pow(2, attempt - 1), MAX_BACKOFF_MS);
	const jitteredDelay = Math.min(addBackoffJitter(backoffDelay), MAX_BACKOFF_MS);
	const delayMs = Math.max(baseDelay, jitteredDelay);
	const backoffDelay = Math.min(baseDelay * Math.pow(2, attempt - 1), MAX_BACKOFF_MS);
	const jitteredDelay = Math.min(addBackoffJitter(backoffDelay), MAX_BACKOFF_MS);
	// Math.max honours server Retry-After as a hard floor; on attempt=1 this makes jitter one-sided (upward only)
	const delayMs = Math.max(baseDelay, jitteredDelay);

or remove the floor to get symmetric spread:

const delayMs = Math.max(0, jitteredDelay);

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/request/rate-limit-backoff.ts
Line: 80-82

Comment:
**negative jitter silently discarded on attempt=1**

for attempt=1, `backoffDelay === baseDelay`. if `Math.random()` returns a value below `0.5`, `jitteredDelay < baseDelay`, and the `Math.max` floor restores it to `baseDelay` — so downward jitter has no effect on the first 429. the effective spread is `[baseDelay, baseDelay×1.2]` (one-sided upward), not the symmetric `±20%` the constant implies.

this is the most common 429 case, so the anti-thundering-herd goal stated in the PR description is only half-realised for first retries. attempt=2+ are unaffected because `backoffDelay >> baseDelay` and symmetric jitter applies.

if the `Math.max` floor is intentional (to honour the server-sent `Retry-After` as a hard minimum), add a comment:

```suggestion
	const backoffDelay = Math.min(baseDelay * Math.pow(2, attempt - 1), MAX_BACKOFF_MS);
	const jitteredDelay = Math.min(addBackoffJitter(backoffDelay), MAX_BACKOFF_MS);
	// Math.max honours server Retry-After as a hard floor; on attempt=1 this makes jitter one-sided (upward only)
	const delayMs = Math.max(baseDelay, jitteredDelay);
```

or remove the floor to get symmetric spread:

```typescript
const delayMs = Math.max(0, jitteredDelay);
```

How can I resolve this? If you propose a fix, please make it concise.

[ndycode]

Superseded by #387, which rebuilds the full open PR stack onto one reviewed integration branch.

[ndycode]

Closing in favor of #387.