Skip to content

fix(ratelimit): fail open on a Redis outage, matching login_throttle - #418

Merged
haksungjang merged 1 commit into
mainfrom
worktree-agent-a9600db3f0144b100
Sep 6, 2026
Merged

fix(ratelimit): fail open on a Redis outage, matching login_throttle#418
haksungjang merged 1 commit into
mainfrom
worktree-agent-a9600db3f0144b100

Conversation

@haksungjang

Copy link
Copy Markdown
Contributor

Summary

Part 1 of 3 for #399 (policy + tests; the readiness endpoint and runbook updates are separate follow-ups).

core/ratelimit.py built slowapi's Limiter against plain redis:// with no documented failure policy. Driven against a real unreachable Redis, the default construction re-raises whatever its storage raises, and swallow_errors=True doesn't help either: the header-injection code that runs after a swallowed exception reads request.state.view_rate_limit unconditionally, 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: adds FailOpenRedisStorage, a thin limits.storage.redis.RedisStorage subclass that answers "zero hits, resets now" instead of raising when Redis cannot answer for incr/get/get_expiry. That reads as "not limited" to the FixedWindowRateLimiter strategy above it, so the request goes through with a warning logged instead of a 500. The module's limiter singleton is rewired to use it (via a redis+failopen:// scheme), with socket timeouts so a hung connection can't stall the fail-open path indefinitely.
  • Module docstring documents the observed failure mode and the fix, mirroring the level of detail login_throttle.py already has.
  • tests/unit/test_rate_limit.py: storage-level tests (real unreachable-port connection, not just mocks) proving incr/get/get_expiry degrade safely, that a healthy Redis still enforces the limit normally, that the live limiter singleton 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/register and /auth/login requests through it, proving the limiter fails open independently of login_throttle (which stays on the real, healthy Redis in these tests). This is the mirror of test_login_throttle.py::test_a_redis_outage_leaves_sign_in_working, which breaks only login_throttle's Redis client. Neither test's pass depends on the other control also being down.
  • CHANGELOG.md: [Unreleased] > Fixed entry.

Verification

  • ruff check . and mypy . (full backend, 949 files): clean.
  • Targeted pytest: tests/unit/test_rate_limit.py (11 tests) and tests/integration/test_ratelimit_fail_open.py (2 tests) pass against real Postgres/Redis. Mutation-checked: reverting the Limiter(...) construction to plain redis:// (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.
  • Observed behavior (documented in the module docstring): a plain slowapi Limiter pointed at an unreachable Redis returns HTTP 500 on every request, confirmed with a standalone reproduction against a real closed port, both with and without swallow_errors=True.

Out of scope

api/v1/health.py / core/readiness.py and the operator runbook are not touched here; they're planned as separate follow-up PRs per the issue.

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
haksungjang force-pushed the worktree-agent-a9600db3f0144b100 branch from ce7c597 to 95484fa Compare September 6, 2026 23:29
@haksungjang
haksungjang merged commit 76aeda5 into main Sep 6, 2026
25 checks passed
@haksungjang
haksungjang deleted the worktree-agent-a9600db3f0144b100 branch September 6, 2026 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant