feat: implement Redis-backed trajectory storage and concurrency tests#31
feat: implement Redis-backed trajectory storage and concurrency tests#31Ad1th wants to merge 2 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces distributed Redis deployment capabilities by adding an atomic Redis-backed trajectory store, refactoring trajectory analysis to support pluggable backends, and upgrading existing RedisStore operations to use atomic Lua scripts. A concurrency test suite validates correctness under multi-threaded contention. ChangesDistributed Redis Backend with Atomic Operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
humane_proxy/risk/trajectory.py (1)
145-145: 💤 Low valueAdd
strict=Truetozip()for defensive validation.While
scores[:-1]andtimestamps[:-1]are derived from the same window and should always have matching lengths, addingstrict=Trueprovides a safety net if future changes inadvertently cause length mismatches.♻️ Suggested fix
- history = deque(zip(scores[:-1], timestamps[:-1])) + history = deque(zip(scores[:-1], timestamps[:-1], strict=True))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@humane_proxy/risk/trajectory.py` at line 145, The history deque is built with zip(scores[:-1], timestamps[:-1]) which can silently drop items if lengths diverge; change the construction in trajectory.py (the line assigning history) to use zip(scores[:-1], timestamps[:-1], strict=True) so mismatched lengths raise immediately (TypeError) and surface bugs; update the invocation assigning to history (variable name history and the deque call) to include strict=True.humane_proxy/risk/redis_trajectory.py (1)
34-60: ⚖️ Poor tradeoffConsider adding TTL to trajectory keys for session cleanup.
The Lua script creates three keys per session (
seq,window,payload) but none have an expiration set. For long-running deployments with many unique sessions, this could lead to unbounded memory growth in Redis.Consider either:
- Setting TTL on keys in the Lua script (e.g., after each append, refresh TTL to 24-48 hours)
- Documenting that operators should configure Redis's
maxmemory-policyappropriately♻️ Example: Add TTL refresh to Lua script
local ids = redis.call('ZRANGE', KEYS[2], 0, -1) local response = {} for _, id in ipairs(ids) do local payload_value = redis.call('HGET', KEYS[3], id) table.insert(response, id) table.insert(response, payload_value) end + -- Refresh TTL on all keys (e.g., 48 hours = 172800 seconds) + local ttl = tonumber(ARGV[5]) or 172800 + redis.call('EXPIRE', KEYS[1], ttl) + redis.call('EXPIRE', KEYS[2], ttl) + redis.call('EXPIRE', KEYS[3], ttl) return responseThen pass the TTL as an additional argument in
append().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@humane_proxy/risk/redis_trajectory.py` around lines 34 - 60, The Lua script registered as self._append_script currently creates three per-session keys (the seq key, the ZSET/window key, and the HSET payload key) but never sets expirations; update the script to refresh TTL on all three keys by calling redis.call('EXPIRE', KEYS[1], ARGV[5]), redis.call('EXPIRE', KEYS[2], ARGV[5]), and redis.call('EXPIRE', KEYS[3], ARGV[5]) after the writes (or where appropriate), and change the Python append wrapper (the append method that invokes self._append_script) to accept/pass a TTL seconds argument (e.g., default 24*3600) as ARGV[5]; this ensures every append refreshes the TTL for seq/window/payload and prevents unbounded Redis growth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@humane_proxy/risk/redis_trajectory.py`:
- Around line 34-60: The Lua script registered as self._append_script currently
creates three per-session keys (the seq key, the ZSET/window key, and the HSET
payload key) but never sets expirations; update the script to refresh TTL on all
three keys by calling redis.call('EXPIRE', KEYS[1], ARGV[5]),
redis.call('EXPIRE', KEYS[2], ARGV[5]), and redis.call('EXPIRE', KEYS[3],
ARGV[5]) after the writes (or where appropriate), and change the Python append
wrapper (the append method that invokes self._append_script) to accept/pass a
TTL seconds argument (e.g., default 24*3600) as ARGV[5]; this ensures every
append refreshes the TTL for seq/window/payload and prevents unbounded Redis
growth.
In `@humane_proxy/risk/trajectory.py`:
- Line 145: The history deque is built with zip(scores[:-1], timestamps[:-1])
which can silently drop items if lengths diverge; change the construction in
trajectory.py (the line assigning history) to use zip(scores[:-1],
timestamps[:-1], strict=True) so mismatched lengths raise immediately
(TypeError) and surface bugs; update the invocation assigning to history
(variable name history and the deque call) to include strict=True.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c563a293-96fc-4462-b635-9862889508e8
📒 Files selected for processing (4)
humane_proxy/risk/redis_trajectory.pyhumane_proxy/risk/trajectory.pyhumane_proxy/storage/redis.pytests/test_redis_concurrency.py
|
|
|
@Ad1th please resolve merge conflicts and sign the CLA. Also review CodeRabbit's comments on some files and overall docstring coverage. Didn't do a thorough review yet but seems alright at a high level. You may need to refer to #17 to resolve merge conflicts with full understanding. Feel free to ask me if you have any questions. |
|
Okay, will do so |
|
I guess the failed test is because the pyproject.toml file was not updated with fakeredis, can I add it in that? |
|
Also @Vishisht16 , the it is not letting me type anythign in the version box for the CLA |
|
@Ad1th I had already told you in #5 that you have permission to add For the CLA, you're not supposed to write in the version box. It takes some time to load and you'll be able to sign it once you give it some time. |
|
Also respect CodeRabbit's review from the previous commit. |
|
okay, will work on it |
|
@Ad1th when can I expect the changes? |
3 participants
Description
This PR upgrades the Redis backend for safe multi-process and distributed execution.
Fixes issue #5
It introduces atomic Redis operations for both escalation logging and rate limiting, and adds a dedicated Redis trajectory store so session risk windows are shared consistently across workers. The trajectory flow now switches to Redis-backed storage when the Redis backend is enabled, while preserving the existing in-memory behavior for non-Redis backends.
It also adds concurrency-focused tests using fakeredis and threaded contention to validate:
Type of Change
Checklist
Checklist point 5: marked even though no doc update since it wsa not needed
Some test results:
m pytest tests/test_redis_concurrency.py -q
... [100%]
3 passed in 0.35s
-m pytest tests/test_redis_concurrency.py tests/test_storage_backends.py tests/test_trajectory.py -q
.......................... [100%]
26 passed in 0.79s