Replace sorted sets with Redis Search index for session listing (fixes #121)#140
Replace sorted sets with Redis Search index for session listing (fixes #121)#140fcenedes merged 7 commits intoredis:mainfrom
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
There was a problem hiding this comment.
Thanks @fcenedes! This is awesome and exactly what we need!
Before we can merge, we need one change. In our project, using RedisVL query types instead of raw execute_command() calls would keep things standardized. This keeps our abstraction layer consistent and makes the code easier to maintain.
Required changes:
- Index creation: Use redisvl.index.SearchIndex with schema instead of FT.CREATE
- Search queries: Use redisvl.query.FilterQuery instead of FT.SEARCH
- Import placement: Move the import in main.py to the top of the file
- Type hint: Double check all typehints
- Some other comments regarding WORKING_MEMORY_INDEX_NAME/PREFIX and exceptions
Finally, would you mind adding warnings.warn() to the deprecated sessions_key() method?
|
is this better ? |
attempts to fix linting
- remove dead code - imports on top
| except Exception as e: | ||
| logger.error(f"Error listing sessions: {e}") | ||
| # Return empty results on error | ||
| return 0, [] |
There was a problem hiding this comment.
Would prefer not to have silent errors. This can mask real issues, consider using logger.exception() to include the traceback, or re-raise the exception after logging.
| """Get the sessions key for a namespace. | ||
|
|
||
| DEPRECATED: This method is deprecated. Session listing now uses | ||
| Redis Search index on working memory JSON documents instead of | ||
| sorted sets. The index automatically handles TTL expiration. | ||
| """ | ||
| return f"sessions:{namespace}" if namespace else "sessions" |
There was a problem hiding this comment.
nit: raise a deprecated warning
| WORKING_MEMORY_INDEX_NAME = settings.working_memory_index_name | ||
| WORKING_MEMORY_INDEX_PREFIX = settings.working_memory_index_prefix |
There was a problem hiding this comment.
It would be better to just import it directly something like:
from agent_memory_server.confi.settings import working_memory_index_name
And then use it later on
| Returns: | ||
| True if index was dropped, False if it didn't exist | ||
| """ | ||
| index_name = WORKING_MEMORY_INDEX_NAME |
There was a problem hiding this comment.
Remove WORKING_MEMORY_INDEX_NAME and use imported variable
| # Calculate start and end indices (0-indexed start, inclusive end) | ||
| start = offset | ||
| end = offset + limit - 1 | ||
| from agent_memory_server.working_memory_index import get_working_memory_index |
There was a problem hiding this comment.
Would move this to the top
2 participants
Replace sorted sets with Redis Search index for session listing (fixes #121)
Description
Problem
PR #119 fixed a regression where list_sessions() was reading from a sorted set that was never populated. However, using a sorted set for session indexing has a fundamental flaw: when working memory expires via TTL, the session entry remains in the sorted set as a stale/orphaned entry.
This means list_sessions() could return session IDs for sessions that no longer exist.
Solution
Replace the sorted set approach with a Redis Search index on the working memory JSON documents. This ensures that when TTL expires and the key is deleted, Redis Search automatically removes the document from the index - eliminating stale entries.
Changes
New Files:
Modified Files:
Benefits
✅ No more stale sessions - Expired sessions are automatically removed from the index
✅ Better filtering - Can now filter by user_id in addition to namespace
✅ Consistent behavior - Index is automatically maintained by Redis
Migration
The old sorted set data will naturally age out. No manual migration required.
Testing
All 647 tests pass
Added new tests for search index behavior and user_id filtering