lmi: support TLS Redis (rediss://) in GlobalRateLimiter#452
Merged
Conversation
Previously GlobalRateLimiter hardcoded `async+redis://` for the storage client and opened the SCAN client without `ssl=`. Against a TLS-only Redis (e.g. AWS ElastiCache with in-transit encryption enabled), the TCP handshake succeeds but the server hangs waiting for a ClientHello that never arrives -- typically observed as `get_rate_limit_keys` hanging during startup. Now a shared pair of helpers (`_wants_tls`, `_strip_scheme`) drives both code paths: `rediss://` -> TLS, `redis://` -> plaintext, and a bare `host:port` can be promoted to TLS via `REDIS_USE_TLS` (parsed as a whitelist of `1|true|yes|on`, not `bool()`). Tests parametrize the scan-client kwargs and the storage URL across plain, `redis://`, `rediss://`, and both env-override directions to guard against scheme/transport drift.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds TLS-aware Redis handling to GlobalRateLimiter so it can work reliably with TLS-only Redis endpoints (e.g., AWS ElastiCache in-transit encryption) without hanging during startup key scans.
Changes:
- Introduces helpers to infer TLS from
rediss:///redis://schemes orREDIS_USE_TLSfor barehost:portURLs. - Builds
RedisStorageURLs withasync+rediss://when TLS is required. - Passes
ssl=<tls>to the directcoredis.Redis(...)scan client and adds parametrized tests for both code paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/lmi/src/lmi/rate_limiter.py | Adds scheme/env-based TLS detection and applies it consistently to storage URL construction and the SCAN client. |
| packages/lmi/tests/test_rate_limiter.py | Adds parametrized coverage for TLS selection and expected storage URL scheme across URL formats and env var values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
whitead
approved these changes
Apr 23, 2026
jamesbraza
reviewed
Apr 23, 2026
Collaborator
jamesbraza
left a comment
jamesbraza
left a comment
There was a problem hiding this comment.
Nice work, though I think you clobbered a few things worth keeping around
jamesbraza
approved these changes
Apr 23, 2026
| pytest.param( | ||
| lambda: "rediss://10.58.188.212:6379", | ||
| {"host": "10.58.188.212", "port": 6379, "password": None, "ssl": True}, | ||
| id="rediss-scheme", |
| ) -> None: | ||
| with patch("lmi.rate_limiter.RedisStorage") as mock_redis_storage: | ||
| limiter = GlobalRateLimiter(redis_url=redis_url) | ||
| _ = limiter.storage |
Collaborator
There was a problem hiding this comment.
Why are we doing this? Maybe put an inline comment
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ypicard
reviewed
Apr 24, 2026
| self.redis_url = redis_url | ||
| self._rate_config = RATE_CONFIG if rate_config is None else rate_config | ||
| self._redis_url = redis_url or os.environ.get("REDIS_URL") | ||
| self._use_in_memory = use_in_memory or not self._redis_url |
Collaborator
There was a problem hiding this comment.
@jamesbraza This defaults to in memory when redis isn't configured.
Collaborator
There was a problem hiding this comment.
Oh sounds great.
Can you document that in the Args section of the docstring? Or feel free to skip.
jamesbraza
approved these changes
Apr 24, 2026
| self.redis_url = redis_url | ||
| self._rate_config = RATE_CONFIG if rate_config is None else rate_config | ||
| self._redis_url = redis_url or os.environ.get("REDIS_URL") | ||
| self._use_in_memory = use_in_memory or not self._redis_url |
Collaborator
There was a problem hiding this comment.
Oh sounds great.
Can you document that in the Args section of the docstring? Or feel free to skip.
Document that use_in_memory falls back to in-memory storage when no Redis URL is configured, and surface the redis://, rediss://, and bare host:port URL forms on `redis_url`.
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
GlobalRateLimiterpreviously hardcodedasync+redis://for the storage client (rate_limiter.py:155) and opened theget_rate_limit_keysSCAN client without anyssl=kwarg. Against a TLS-only Redis — e.g. AWS ElastiCache with in-transit encryption enabled — the TCP handshake succeeds but the server then hangs waiting for a ClientHello that never arrives. Symptom in the wild:get_rate_limit_keys()hangs during agent startup, blocking_choose_model_based_on_limitsand every downstream rate-limited call.This PR makes both Redis code paths scheme-aware and consistent:
_wants_tls(redis_url)and_strip_scheme(redis_url)— decide TLS from an explicitrediss:///redis://scheme, or (for barehost:portURLs) from aREDIS_USE_TLSenv var parsed as a whitelist (1|true|yes|on) rather thanbool()(which would treat"0"/"false"as true).async+rediss://{host}:{port}when TLS is required, instead of unconditionally prefixingasync+redis://.get_rate_limit_keys()uses the same helpers and passesssl=<tls>to the directcoredis.Redis(...)client.Behavior matrix
REDIS_URLREDIS_USE_TLSsslhost:6379async+redis://host:6379Falsehost:6379trueasync+rediss://host:6379Truehost:63790/falseasync+redis://host:6379Falseredis://host:6379async+redis://host:6379Falserediss://host:6379async+rediss://host:6379TrueThe bare-host + no-env-var row is the GCP Memorystore path — unchanged.
Test plan
uv run --directory packages/lmi pytest tests/test_rate_limiter.py→ 38 passeduv run mypy packages/lmi/src/lmi/rate_limiter.py→ cleanpre-commit run --files packages/lmi/src/lmi/rate_limiter.py packages/lmi/tests/test_rate_limiter.py→ all hooks pass (ruff, mypy, codespell, detect-secrets, blacken-docs)redis://,rediss://, and bothREDIS_USE_TLSdirections (including the"0"regression case thatbool()would have mishandled)get_rate_limit_keysnow completes startup and proceeds to model selection.Follow-up
A downstream service (paperqa-server) currently carries a local override of
get_rate_limit_keysthat addedssl=Trueto the scan client. Once this lands and is released, that override can be dropped.