Repository navigation
test(ci): move pinned rate-limit windows off the wall-clock boundary - #895
Merged
Merged
Conversation
The login/client-errors limiter bucket boundary is Redis server wall-clock inside the Lua script (#543/#544, deliberate - replicas must agree without trusting host clocks). A boundary crossing inside a test's status loop resets the count mid-loop, so the PermitLimit+1-th request lands in a fresh bucket and the expected 429 arrives as a 401. That is the exact signature of the two 2026-09-16 CI specimens on #840 (MultiInstanceRateLimitTests on feat/829-dashboard, UntrustedPeerForwardedHeaderTests on feat/834-link-colour, both 'Expected: TooManyRequests, Actual: Unauthorized' on branches whose re-runs were green). The #775 census dismissed rollover using a 218 ms test as the interval; the actually-failing loops are multi-second under CI load, and ~10 limiter tests run per assembly, which puts the per-run crossing probability at 1-3% - matching the observed 2/62 PR-branch rate. Pinning the test window at 24h drops a mid-loop crossing to ~0.003%. The Retry-After range assertion in RateLimitingTests moves with the window: it asserts the Retry-After-within-window semantic, not the 900 s value.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Owner
Author
|
@coderabbitai review |
|
Action performedReview triggered.
|
The 24h test rate-limit window avoids a Redis TIME boundary crossing inside a status loop, but it also retains a host's counter for the whole test job. Give every WebApplicationFactory a per-instance Redis key namespace so it cannot carry another host's attempts into that window. MultiInstanceRateLimitTests deliberately overrides that default: its migration process and both serving processes receive one generated namespace, preserving its acceptance proof that two instances share the same Redis budget.
Program registered shared state from one bound SharedStateOptions instance, then bound the same config a second time only to configure the report cap. If the reads ever diverged, auth counters and report leases would occupy different Redis namespaces. Return the bound options from AddCluckworkSharedState and use its namespace for the report-concurrency registration.
Boot a Redis-backed test host with a known namespace, consume an auth counter and acquire a report-concurrency permit, then assert both real Redis keys use that namespace. The guard fails if Program restores an independent report-cap namespace binding. Mutation verified by routing only the report cap to a -wrong namespace: the new test went red, then passed after restoration.
The factory's IAsyncLifetime disposal hides WebApplicationFactory's IAsyncDisposable method, so await using did not stop its Postgres container. Dispose through the factory's Task-returning member explicitly. Use a 24h counter window too: the Redis TTL reaches the next aligned boundary, and a one-minute window could expire between increment and key scan.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the observed 429→401 rate-limit flakes. Does not close #840 — leader-gate and StealLoss remain.
What broke
The limiter obtains its window from Redis server time inside
RedisFixedWindowCounter's Lua script. When the 900-second boundary falls inside a test's login/status loop, the counter starts a fresh bucket partway through the loop. The request expected to be 429 then gets the normal 401 login rejection.That is the signature in both September 16 specimens:
MultiInstanceRateLimitTests.Login_budget_is_shared_across_two_instances_over_one_redisUntrustedPeerForwardedHeaderTests.Forwarded_for_from_an_untrusted_real_peer_is_ignored_so_forged_headers_share_one_bucketThe earlier 218ms probability calculation was too small a denominator: the actual request loops are multi-second under CI load and several run in one assembly.
The fix
CluckworkWebApplicationFactorygets a generatedSharedState:Redis:KeyNamespace. A long window retains counters for the job, so this prevents another factory sharing Redis from spending the same bucket.MultiInstanceRateLimitTestsdeliberately sends one generated namespace to its migration process and both serving processes, preserving the thing it proves: two instances share one Redis budget.RateLimitingTestsupdates itsRetry-Afterassertion to follow the configured window; it tests that the value is within the window, not that tests use the production 900-second value.Shared-state registration hardening
The review found that
Program.csboundSharedStateOptionstwice: once to register shared state and again to configure the report-concurrency cap. #895 now reuses the object returned byAddCluckworkSharedState, so auth counters and report leases cannot silently receive different Redis namespaces if the two reads ever diverge.SharedStateNamespaceWiringTestsboots a Redis-backed host with a known namespace, consumes an auth counter and acquires a report permit, then asserts both actual Redis keys use it. The guard was mutation-tested by routing only the report cap to a-wrongnamespace; it failed, then passed after restoration. It proves the two subsystems receive the configured namespace, not that two identical configuration binds would diverge. This is a small production wiring change; production configuration and behavior stay the same.Verification
dotnet build tests/Cluckwork.Api.IntegrationTests/Cluckwork.Api.IntegrationTests.csprojMultiInstanceRateLimitTests,ForwardedHeaderTrustKestrelTests, andRateLimitingTests: all passed-wrongreport-cap namespace mutationSharedStateNamespaceWiringTestspasses