Isolate #[throttle] registry tests with a shared TEST_LOCK#1849
Merged
Conversation
Two rate-limit tests intermittently returned 429 instead of 200 because per-principal buckets could leak from concurrent tests through the process-global #[throttle] limiter registry. Add a process-global TEST_LOCK (mirroring circuit_breaker::TEST_LOCK) and have every registry-touching integration test take the lock first, then reset the registry, holding the guard for the whole test so no sibling mutates the registry mid-assertion. Part of #1725.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
madmax983
marked this pull request as ready for review
July 13, 2026 06:49
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk-dev #1849 +/- ##
=============================================
+ Coverage 86.82% 87.02% +0.19%
=============================================
Files 273 276 +3
Lines 202424 207214 +4790
=============================================
+ Hits 175761 180330 +4569
- Misses 26663 26884 +221 🚀 New features to boost your workflow:
|
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.
Before
Two rate-limit integration tests intermittently failed, returning
429instead of200:throttle_route::principal_key_derives_from_session_without_global_principal_middlewarerate_limit_principal::principal_strategy_keys_on_session_user_idUnder the ~1200 parallel integration tests, per-principal token buckets could leak between concurrently-running tests through the process-global
#[throttle]limiter registry (THROTTLE_REGISTRY). Calling__throttle_registry_reset()alone is insufficient: it clears the entire process-wide registry, so a sibling test running concurrently can either repopulate a bucket mid-assertion or have its own limiter dropped out from under it.After
Each test that touches the throttle registry now takes a process-global
TEST_LOCKfirst, then resets the registry, and holds the lock guard for the whole test body. This serializes all registry-touching tests against each other, so no sibling can mutate the registry during another's assertions — the same isolation precedent already used by the circuit-breaker tests (circuit_breaker::TEST_LOCK, PR #1817) andconfig_runtime_drift.How
pub static TEST_LOCK: std::sync::Mutex<()>tosecurity/rate_limit.rs, mirroringcircuit_breaker::TEST_LOCK, and re-exported it (#[doc(hidden)]) fromsecurity/mod.rsnext to__throttle_registry_reset.#[throttle]-registry test inthrottle_route.rs(and the two named flaky tests), take the lock first, then call__throttle_registry_reset(), holding the guard for the whole test. Because the reset clears the whole registry, every registry-touching test must hold the lock — otherwise a locked test's reset could drop an unlocked sibling's limiter — so the guard is applied consistently across all of them. Each also carries#[allow(clippy::await_holding_lock)], matching the circuit-breaker precedent.Notes
rate_limit_principal::principal_strategy_keys_on_session_user_iduses the globalRateLimitLayer, whose token buckets are per-Limiter(per-TestApp) in an in-memoryMemoryStore, not the sharedTHROTTLE_REGISTRY. The guard is applied there as requested for consistency and to serialize it against the throttle tests, but the registry reset is effectively a no-op for it — so if that test's flakiness recurs, the root cause is likely elsewhere (timing / the previously-fixed cookie-jar collision) rather than registry bleed. Flagging for reviewer awareness.Gates
cargo fmt --all -- --check— cleancargo clippy -p autumn-web --all-targets -- -D warnings— cleanthrottle_route+rate_limit_principaltests pass.Generated by Claude Code