Stop the rate-limiting tests racing their own window #203
Loading…
Reference in a new issue
No description provided.
Delete branch "rate-limit-test-windows"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 267. Five test classes gave themselves a two-second rate-limit window and then did work of uncontrolled duration inside it — a password hash per attempt, stub-provider discovery, a container round trip — so the window could roll before the last request, which was then permitted instead of refused.
Enforcement tests now configure a five-minute window. They never wait for it, so the width costs nothing.
PostLogin_AfterTheWindowElapses_TheSameAddressIsPermittedAgainandPostInvite_AfterTheWindowElapses_TheSameCallerIsPermittedAgainkeep two seconds, through a factory of their own — they are the two that need the window to roll. The login one now spends its permits on requests the validation filter refuses with a 400 before the handler hashes anything, so nothing slow happens inside those two seconds; the invitation one warms the host on another caller's budget first. No sleep was lengthened and nothing retries.Beyond the two classes named in the ticket,
AuthenticationRateLimiterBehindATrustedProxyTests,InvitationRateLimiterTestsandInvitationIpRateLimiterTestshad the same shape and are fixed the same way.Also tightened both
Retry-Afterassertions, which a wide window would otherwise have made vacuous: the header must now be what is left of this window, not merely something positive.Proof, all
dotnet test PlaceMark.slnxwith 120 CPU hogs running alongside:exhaustingResponse.StatusCode should be TooManyRequests but was OK.Mutations watched go red: auth paths never matched by the limiter (all twelve tests asserting a 429 on an auth path — six in
AuthenticationRateLimiterTests, five inAuthenticationRateLimiterBehindATrustedProxyTests, one inOidcRateLimitingTests— red); the configured auth window discarded for the default (only the rolling test and theRetry-Afterassertion red);RequireInvitationRateLimit()deleted from the route (all 5 invitation tests red); the configured invitation window discarded (only the rolling test red).Verdict: changes needed
Two things, both small.
The
11 auth tests redin the PR body does not add up: twelve tests assert a 429 on an auth path — six inAuthenticationRateLimiterTests, five inAuthenticationRateLimiterBehindATrustedProxyTests, one inOidcRateLimitingTests— and a limiter that never matches the auth prefix reddens every one of them. Either name the one that stayed green, or say the mutation run was filtered; ifOidcRateLimitingTestswas outside the filter, that is the test whose entire subject is the path matching that mutation breaks, so it is the one worth re-running.Plus the stale class remark noted inline.
The rest holds up.
SpendAPermitAsyncis safe on ordering, not incidentally: the auth limiter isRateLimiterOptions.GlobalLimiteronapp.UseRateLimiter(), which sits above endpoint execution inProgram.cs, so the permit is always taken before any endpoint filter runs —[Required]on bothLoginRequestfields makes the 400 deterministic, and nothing hashes. The tightenedRetry-Afterrange is the specification's, not the observed value's:RateLimitRejectionResponsewrites the lease's ownMetadataName.RetryAfter, whichFixedWindowRateLimitersupplies as the time remaining in the current window, so[window − 1min, window]is exactly "what is left of this window" with a minute of slack for the requests that spent the permits. Both rolling tests still fail if the window does not roll — each ends on a permitted response through the same partition it exhausted — and the invitation one cannot borrow the login one's trick, becauseRequireInvitationRateLimitis a route filter andRequestValidationFiltera group filter, so a refused body would never reach the limiter at all; the warming request is the right answer there. Nosrc/change andcoverage-baseline.jsonuntouched.CI: run #837 for
77baa1bis still running (started 13:59 UTC, seven minutes ago). Not seen green.@ -40,0 +43,4 @@/// the width costs nothing; at two seconds the window could roll between the second invitation and/// the third under load, and the request that should have been refused was permitted instead./// </summary>private static readonly TimeSpan _enforcementWindow = TimeSpan.FromMinutes(5);The class remarks above (line 27) still say "two invitations inside two seconds" — false now, and the identical sentence in
AuthenticationRateLimiterTestswas corrected in this PR.1d798768f442c0f6e13aCI: #837 (
77baa1b, the fix itself) was fully green. #840 (1d79876, that plus a doc comment) failede2eonly —buildgreen — with Playwright 30s navigation timeouts; unrelated PR run #841 failed the same jobs the same way in the same window, on a runner carrying three runs at once (32m vs the usual ~8m). Re-triggered as #844 on42c0f6e; still running, contending with #845.#844 on
42c0f6e:buildsuccess,container-imagessuccess,e2estill running after ~2h — it is sharing the runner with #845'se2e, which is the same contention that produced #840's and #841's Playwright timeouts. Re-check before merging; every test this PR touches runs inbuild.Verdict: mergeable
Re-review at
42c0f6e, superseding review 329 (77baa1b). Nothing to action.The count is right at twelve, and complete. Six
[Fact]s inAuthenticationRateLimiterTests, five inAuthenticationRateLimiterBehindATrustedProxyTests, one inOidcRateLimitingTests, and every one of them asserts a 429 on an auth path. There is no fourth class: a test that asserts a 429 there has to lowerRateLimiting:Auth:PermitLimitfrom the shipped ten a minute, and only those three do. The rest of the folder is the two options classes, which parse configuration with no server, andAuthenticationPathMatchingTests, which assertsIsAuthenticationRequestagainst routing and never a status code — worth knowing that if the mutation was cut atIsAuthenticationRequest, that theory reddens too, so the blast radius is wider than twelve rather than narrower.The remarks now read accurately: ADR-0113's default is
DefaultPermitLimit = 10over_defaultWindow = TimeSpan.FromMinutes(5), and "configured per factory" is exactly whatCreateFactory(window)does withRateLimiting:Invitations:PermitLimit.Nothing drifted. The only change since
77baa1bis those two lines of prose; the five-minute enforcement windows, both rolling factories at two seconds, the warming invitation and theRetry-Afterbounds are byte-identical to what I cleared last round.CI: run #844 for
42c0f6eissuccess, 10m47s.