Skip to content

test: give the redis counter poll five seconds instead of one - #13892

Open
nic-6443 wants to merge 2 commits into
apache:masterfrom
nic-6443:test/redis-counter-poll-bound
Open

test: give the redis counter poll five seconds instead of one#13892
nic-6443 wants to merge 2 commits into
apache:masterfrom
nic-6443:test/redis-counter-poll-bound

Conversation

@nic-6443

@nic-6443 nic-6443 commented Aug 27, 2026

Copy link
Copy Markdown
Member

Description

t/lib/test_redis.lua's wait_counters_above() gave up after about one second, and one second is not always enough for the counter it waits on to show up. This bounds the wait by a five second deadline instead.

Some context on why the wait exists at all, since that is where the flakiness comes from. ai-rate-limiting commits token usage in its log phase, which APISIX runs from log_by_lua_block — after the response has already gone out to the client. Cosockets are not available there, so limit-count-redis's log_phase_incoming() defers the actual write with ngx.timer.at(0, ...). That means when the test client holds request N's response, the counter for N is still unwritten: the log phase has to run, the zero-delay timer has to be picked up by the event loop, and the redis roundtrip has to complete. Fire request N+1 immediately and its access phase reads a budget that still looks unspent — which is the off-by-one-request symptom these tests used to show, wrong status codes and X-AI-RateLimit-Remaining values lagging by one request. wait_counters_above() bridges that gap: snapshot with sum_counters() before the request, then poll until the total grows.

The flaky part is the ceiling on that poll, not the logic. 100 iterations of ngx.sleep(0.01) is about one second of wall clock, and that second has to cover all three hops above plus the poller's own KEYS + GET against the same redis. On a loaded runner — parallel jobs sharing one redis container, a worker that doesn't get scheduled promptly — the write is still in flight when the poll runs out, so the helper returns an error and the caller's assert fails even though the counter lands a moment later. Nothing about it is deterministic, which is why it turns up on PRs that touch nothing in this area.

I bumped the ceiling to five seconds, but as a wall-clock deadline rather than a bigger iteration count. An iteration count bounds only the sleeps: each pass also does a full sum_counters() round trip, so the real ceiling stretches with redis latency — precisely the condition the wait exists to absorb. At 100ms per poll, 500 iterations would run for the better part of a minute while the comment above it claimed five seconds. A deadline says what it means.

The loop still returns as soon as the counter moves, so a healthy run costs exactly what it did before.

The only caller today is t/plugin/ai-rate-limiting.t TEST 37.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

This is a test-only change to a test helper, so no new tests or documentation.

wait_counters_above() polled 100 times at 10ms, so it gave up after one
second. That single second has to cover the whole asynchronous path from
"client has the response" to "counter is visible in redis", which is not
always enough on a loaded CI runner.
Copilot AI lite review requested due to automatic review settings August 27, 2026 09:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Aug 27, 2026
An iteration count only bounds the sleeps. Every pass also does a full
sum_counters() round trip, so the effective ceiling grows with redis
latency -- exactly when the wait matters. Use a five second deadline.

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

注册 for free to join this conversation on GitHub. Already have an account? 登录 to comment

标签

size:XS This PR changes 0-9 lines, ignoring generated files.

项目

None yet

Development

Successfully merging this pull request may close these issues.

3 participants