fix(ratelimit): fail open on a Redis outage, matching login_throttle - #418
Merged
Conversation
slowapi's Limiter re-raises whatever its storage raises, and swallow_errors=True doesn't help either since the header-injection code that runs afterward reads request.state.view_rate_limit unconditionally, which is only set when evaluation finishes without raising. Either way a Redis outage 500'd every rate-limited endpoint, confirmed against a real unreachable Redis. Add FailOpenRedisStorage, a thin Redis storage wrapper that answers "zero hits, resets now" instead of raising when Redis cannot answer. That reads as "not limited" to the strategy above it, so the request goes through with a warning logged rather than a 500 -- the same fail-open policy login_throttle.py already documents and follows for its own Redis calls. Refs #399
haksungjang
force-pushed
the
worktree-agent-a9600db3f0144b100
branch
from
September 6, 2026 23:29
ce7c597 to
95484fa
Compare
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.
Summary
Part 1 of 3 for #399 (policy + tests; the readiness endpoint and runbook updates are separate follow-ups).
core/ratelimit.pybuilt slowapi'sLimiteragainst plainredis://with no documented failure policy. Driven against a real unreachable Redis, the default construction re-raises whatever its storage raises, andswallow_errors=Truedoesn't help either: the header-injection code that runs after a swallowed exception readsrequest.state.view_rate_limitunconditionally, and that attribute is only set once evaluation finishes without raising. Either way, a Redis outage turned every rate-limited endpoint (including/auth/login) into a 500.login_throttle.py's per-address slowdown was already fail-open and already documented as such; this PR does not change its behavior, only proves it with the existing coverage plus a mirrored test.Changes
core/ratelimit.py: addsFailOpenRedisStorage, a thinlimits.storage.redis.RedisStoragesubclass that answers "zero hits, resets now" instead of raising when Redis cannot answer forincr/get/get_expiry. That reads as "not limited" to theFixedWindowRateLimiterstrategy above it, so the request goes through with a warning logged instead of a 500. The module'slimitersingleton is rewired to use it (via aredis+failopen://scheme), with socket timeouts so a hung connection can't stall the fail-open path indefinitely.login_throttle.pyalready has.tests/unit/test_rate_limit.py: storage-level tests (real unreachable-port connection, not just mocks) provingincr/get/get_expirydegrade safely, that a healthy Redis still enforces the limit normally, that the livelimitersingleton is actually wired to the new storage class (not just that the class exists), and an HTTP-level reproduction of the fix end to end.tests/integration/test_ratelimit_fail_open.py(new): breaks only the rate limiter's storage and drives real/auth/registerand/auth/loginrequests through it, proving the limiter fails open independently oflogin_throttle(which stays on the real, healthy Redis in these tests). This is the mirror oftest_login_throttle.py::test_a_redis_outage_leaves_sign_in_working, which breaks onlylogin_throttle's Redis client. Neither test's pass depends on the other control also being down.CHANGELOG.md:[Unreleased] > Fixedentry.Verification
ruff check .andmypy .(full backend, 949 files): clean.tests/unit/test_rate_limit.py(11 tests) andtests/integration/test_ratelimit_fail_open.py(2 tests) pass against real Postgres/Redis. Mutation-checked: reverting theLimiter(...)construction to plainredis://(no fail-open storage) makes the wiring test fail as expected, confirming the tests actually depend on the fix.node tools/em-dash/lint.mjs: clean.Limiterpointed at an unreachable Redis returns HTTP 500 on every request, confirmed with a standalone reproduction against a real closed port, both with and withoutswallow_errors=True.Out of scope
api/v1/health.py/core/readiness.pyand the operator runbook are not touched here; they're planned as separate follow-up PRs per the issue.