Repository navigation
test(ci): probe the shared login counter key behind the 429/401 flake - #866
Merged
Merged
Conversation
#840's census reproduced the rate-limit flake once in 36 runs and got 0 failures in 80 instrumented runs, because the instrumentation recorded what the counter was asked to count and never who asked. The login bucket key is auth-login:<client-ip> alone, so any concurrently running class that presents the loopback address spends the budget MultiInstanceRateLimitTests measures. The probe reads the shared key directly and reports the statuses it saw, the keys present before and after, and what was already spent when it started. It emits rather than asserts, so a green run is information too. Also records the four-lane findings, including the refutation of the thread-pool-starvation story for DurableJobWorkerLeaderGateTests and the server-side read of the Npgsql Authenticate timeout.
|
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: 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 |
Unused SubprocessExitTimeout, comments restating the code, and a qualifier-per-call instead of one alias for the TestHarness name that StackExchange.Redis also exports.
The probe booted a serving child pointed at an unreachable database and
waited on /health/ready. DatabaseReadyHealthCheck 503s that endpoint
whenever the database is unreachable, so readiness never arrived and the
wait ran to its 60s timeout - locally and on CI, identically.
The limiter runs before the endpoint, so the probe now boots against a
private Postgres it never queries, waits by polling /api/v1/auth/login for
401 or 429, and reads the shared key around a fourteen-request burst.
First real run: the bucket already held one spend from elsewhere in the
suite before the probe's first request, and moved 19 while the probe sent
14 - the collision is real. It also showed the live key shape is
{cluckwork:win:auth-login:<ip>}:<bucket>, not the namespace-only form.
CI is green on the probe. Trim the comments that restated the mechanism twice, record the bucket-rollover confounder the first CI run exposed (a 900s window turns over mid-burst, so a delta larger than the burst is not by itself proof of a foreign writer), and assert the burst actually crossed the budget.
The mechanism claim needs both runs, not one: the CI line carries bucketCountAfter and spendOnKeysThisProbeDidNotCreate, which is what rules out a mid-burst window rollover as the reason the counter moved by 19 on a 14-request burst.
The probe waited for the login route to answer 401 or 429 and called that ready. 429 means the opposite: the bucket is keyed on the loopback address alone, so a concurrent class can empty it before this child binds its port. The probe saw a foreign 429, burst into a dead bucket, and reported 14 refusals and 40 increments as its own result. Waiting for 401 alone does not fix it - a 900s window spent by a neighbour stays spent for the whole wait. The budget is now cleared before the burst, which is legitimate only because the foreign-spend reading is taken first, before the child exists. That reading is the finding; the clear removes the confound. The Postgres container stays. Program.cs migrates before the web host starts, so a serving process with no reachable database never reaches its request pipeline. The limiter needs no database; the boot does. Also: the probe no longer asserts an exact count. The bucket is shared, so a concurrent login racing the clear adds spend it did not cause.
This was referenced Sep 14, 2026
mforce
added a commit
that referenced
this pull request
Sep 15, 2026
## What Gives the login rate-limit counter a bucket the probe can measure without the suite's own traffic in it. Follow-up to #866, which proved the collision but did not remove it from the way. `IsolatedLoginBucket` makes loopback a trusted proxy for one test host, so `ForwardedHeadersMiddleware` honours `X-Forwarded-For` and the limiter keys on an address the test names instead of on the socket peer. ## Two findings from building it **A first cut minted buckets as `198.51.100.<a>.<b>`.** Five octets is not a valid IPv4 literal. `ForwardedHeadersMiddleware` rejects it and falls back to the socket peer, so every "independent" bucket silently collapsed onto loopback — where the test's own exhaustion turned the next client's first request into a 429. It looked like isolation not working, not like a malformed address. Buckets are TEST-NET-3 `/32`s now, and the helper says why that matters. **The probe's own fixture spends against the probe's own bucket.** The burst moved the counter by 18 on 14 requests, in a Redis container private to that class — nothing external could have written it. `CluckworkWebApplicationFactory` builds its host in the constructor, before any test body runs, and the suite logs in constantly from loopback. So #840's collision reproduces one level down, inside a single class. That is why the assertion is `>=` and not `==`: an exact assertion here would have made the file red on a working mechanism. ## Why configuration and not `PostConfigure<ForwardedHeadersOptions>` `Program.cs:63` reads `RateLimiting:TrustedProxies` eagerly into a registration record and hands that snapshot to `AddCluckworkEdgeSecurity`, which clears `KnownIPNetworks` and rebuilds them from it. A `PostConfigure` runs afterwards and is silently overwritten — a helper written that way looks correct and does nothing at runtime. ## Guard A host that already sets `RateLimiting:TrustedProxies:0` is refused, not merged. Such a host is exercising #143's anti-spoof boundary; overwriting its proxy list would delete the thing under test while leaving the test green. ## Verification Executed, 32/32 across every class that touches the login limiter or the forwarded-header trust decision: - `RateLimitingTests`, `AuthRateLimitLoggingTests`, `DistributedRateLimiterWiringTests`, `MultiInstanceRateLimitTests`, `LoginCounterKeyProbeTests`, `SchemaDocsTests` — 20 - `ForwardedHeaderTrustKestrelTests`, `SecurityHeadersTests`, `KestrelRequestBodyLimitTests` — 12 The second group matters most: those are the anti-spoof tests, and they still pass because they configure their own proxies and therefore never enter this helper. ## Not in this PR The collision is still there for the ~102 classes that log in through the shared factory. This isolates the probe. Extending it to the suite is the next step and should be its own PR, because it means every factory opting in and each one is a place to get the trust decision wrong. Does not close #840. No flake has been shown to stop flaking yet — that needs repetition, not reasoning. --------- Co-authored-by: mforce <mforce@users.noreply.github.com>
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
1 participant
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.
What
The instrument the census on #775 was missing, plus the written findings from a four-lane investigation of the remaining flakes in #840.
tests/Cluckwork.Api.IntegrationTests/LoginCounterKeyProbeTests.cs— boots a real serving child, reads the sharedauth-login:127.0.0.1counter straight out of Redis around a controlled burst, and reports what was already spent before it started.docs/plans/840-integration-flakes/01-swarm-findings.md— the four lanes, with file:line evidence, including one refuted hypothesis and three corrections to this PR's own earlier claims.The finding
The probe's first trustworthy run moved the counter by 40 on 14 requests.
That is the flake, with arithmetic. The login bucket key is the client address alone (
DistributedIpFixedWindowPolicy.cs:44,RateLimitKey.cs:21-38),MultiInstanceRateLimitTestscarries no[Collection](:24) so it runs concurrently with other collections, andFakeRemoteIpStartupFilterrewritesConnection.RemoteIpAddressonly whenX-Test-Remoteis present (:17-19) — whichAuthBodyLimitTestsnever sets (:73-78). Several classes independently empty one bucket, and whichever runs last sees 429 where it expected 401.The census's instrumentation almost certainly did see these foreign increments and attributed them to nothing, which is why it got 0 failures in 80 instrumented runs.
What the probe had to get right first
Its first result was a bug in the probe, and the record says so:
Program.cs:170runsMigrateAsyncbefore the web host starts, so a serving process with no reachable database never reaches its request pipeline. The limiter needs no database; the boot does.Verification
Executed, not assumed:
MultiInstanceRateLimitTests,AuthBodyLimitTests,DistributedRateLimiterWiringTests.SchemaDocsTests5/5 — the image-pin guard walks every tracked file, and this PR adds a tracked document naming a Postgres image.CLUCKWORK_840_PROBEline present in the log and identical to the local run.Not in this PR
Deliberately, from the same findings:
DurableJobWorkerLeaderGateTests. The thread-pool-starvation story a lane proposed is refuted —BackgroundService.StartAsyncrunsExecuteAsyncsynchronously to its first incomplete await — so either change would have no mechanism behind it, which is exactly what CI: root-cause the four remaining integration flakes #840 forbids.X-Test-Remoteper class, or a shared collection) is the follow-up, and it should be its own PR with its own mutation check.StealLossConnectionReleaseTests— server-side starvation on a 2-core runner, owned by the CI: measure and cut the integration suite's wall clock (container reuse, parallelism) #839/CI: measure where Build and test's 10 minutes go, and what the backend suite covers #775 wall-clock work.Does not close #840. This proves the mechanism behind the rate-limit flake and narrows the leader-gate one. Both fixes land separately, and #840 stays open until a flake stops flaking.