Skip to content

test(e2e): keep rate-limit bursts inside one wall-clock window - #813

Merged
jarvis9443 merged 1 commit into
mainfrom
test/ratelimit-e2e-window-boundary
Jul 23, 2026
Merged

test(e2e): keep rate-limit bursts inside one wall-clock window#813
jarvis9443 merged 1 commit into
mainfrom
test/ratelimit-e2e-window-boundary

Conversation

@jarvis9443

Copy link
Copy Markdown
Contributor

The failure

ratelimit-e2e failed on an unrelated PR (#809, an Azure streaming-timeout change):

FAIL src/cases/ratelimit-e2e.test.ts > rate limit e2e: RPM=1 second call gets 429
AssertionError: expected undefined to be an instance of APIError

The second call under rpm: 1 succeeded instead of returning 429. 153 other tests passed.

Why

The limiter buckets on fixed wall-clock windows, not on a sliding window from first use — roll_if_stale in crates/aisix-ratelimit/src/window.rs:

let bucket_start = (now_secs / self.window_secs) * self.window_secs;
if bucket_start != self.window_start { self.window_start = bucket_start; self.count = 0; }

So a burst that straddles a minute boundary lands in two buckets, the later call legitimately gets a fresh allowance, and the assertion flaps purely on when in the minute CI happened to run it. Nothing about the gateway is wrong; the tests just assume a burst is atomic with respect to the window.

Fix

model-group-member-ratelimit-e2e already carried a private awaitWindowHeadroom for exactly this reason (added with #1087). This promotes it to the shared harness and applies it to every sibling that bursts against an RPM cap and asserts a 429:

test headroom
ratelimit-e2e default
team-member-ratelimit-e2e default
passthrough-model-rate-limit-e2e default
videos-e2e default
ratelimit-cluster-e2e (shared-Redis case) default
concurrency-e2e 20s

concurrency-e2e needs more because its readiness probes consume the RPM=1 slots themselves — probe and assertion must land in the same window, and the propagation polling between them takes seconds.

Deliberately not guarded: the second ratelimit-cluster-e2e case (backend=memory, both replicas expected to succeed). Crossing a boundary only loosens the quota there, so it cannot produce a false failure. Guarding it would just add wall-clock time.

Cost

The helper returns immediately unless the current minute has less than the requested headroom left, so a normal run pays nothing. Its two branches are pinned with fake timers (src/harness/window-headroom.test.ts) — the real behaviour only shows up in the last seconds of a minute and would otherwise be exercised on almost no run.

Verification

All six guarded suites pass locally (12 tests). The pre-existing tsc --noEmit warnings in this tree (app possibly undefined, 5 of them) are unchanged — verified identical on pristine main.

`ratelimit-e2e` failed on an unrelated PR with `expected undefined to be
an instance of APIError` — the second call under RPM=1 succeeded instead
of returning 429.

The limiter buckets on fixed wall-clock windows: `roll_if_stale` in
`crates/aisix-ratelimit/src/window.rs` computes
`bucket_start = (now / window_secs) * window_secs`. A burst that
straddles a minute boundary therefore lands in two buckets, the later
call legitimately gets a fresh allowance, and the assertion flaps purely
on when in the minute CI happened to run it.

`model-group-member-ratelimit-e2e` already carried a private
`awaitWindowHeadroom` for exactly this reason (#1087). Promotes it to the
shared harness and applies it to every sibling that bursts against an
RPM cap and asserts a 429:

  ratelimit-e2e, team-member-ratelimit-e2e,
  passthrough-model-rate-limit-e2e, videos-e2e,
  ratelimit-cluster-e2e, concurrency-e2e

`concurrency-e2e` asks for more headroom (20s): its readiness probes
consume the RPM=1 slots themselves, so the probe and the assertion have
to land in the same window, and the propagation polling between them
takes seconds.

Deliberately not guarded: the second `ratelimit-cluster-e2e` case
(`backend=memory`, both replicas expected to succeed) — crossing a
boundary only loosens the quota there, so it cannot produce a false
failure.

The helper only sleeps in the last seconds of a minute, so a normal run
pays nothing. Its two branches are pinned with fake timers, since the
real behaviour is otherwise exercised on almost no run.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2b68657b-6dff-47b3-880c-9a06d71a0cfc

📥 Commits

Reviewing files that changed from the base of the PR and between 6413a77 and 3cf8d2a.

📒 Files selected for processing (10)
  • tests/e2e/src/cases/concurrency-e2e.test.ts
  • tests/e2e/src/cases/model-group-member-ratelimit-e2e.test.ts
  • tests/e2e/src/cases/passthrough-model-rate-limit-e2e.test.ts
  • tests/e2e/src/cases/ratelimit-cluster-e2e.test.ts
  • tests/e2e/src/cases/ratelimit-e2e.test.ts
  • tests/e2e/src/cases/team-member-ratelimit-e2e.test.ts
  • tests/e2e/src/cases/videos-e2e.test.ts
  • tests/e2e/src/harness/admin.ts
  • tests/e2e/src/harness/index.ts
  • tests/e2e/src/harness/window-headroom.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/ratelimit-e2e-window-boundary

Comment @coderabbitai help to get the list of available commands.

@jarvis9443
jarvis9443 merged commit be205ca into main Jul 23, 2026
9 checks passed
@jarvis9443
jarvis9443 deleted the test/ratelimit-e2e-window-boundary branch July 23, 2026 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant