feat: add Redis session support for scalable distributed memory #1785
Conversation
- Add RedisSession class with full Session protocol implementation - Support Redis URL connection strings and direct client injection - Include TTL (time-to-live) support for automatic session expiration - Add key prefixes for multi-tenancy and namespace isolation - Implement atomic operations using Redis pipelines for data integrity - Add comprehensive test suite with in-memory fakeredis support - Include detailed example demonstrating Redis session features - Update documentation with Redis session reference - Add redis extra dependency group for easy installation The RedisSession enables production-grade, distributed session memory that scales across multiple application instances while maintaining full compatibility with the existing Session interface.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
…s compatibility - Add Redis client ownership tracking with _owns_client attribute - Implement conditional close() method to prevent closing shared clients - Fix decode_responses compatibility in get_items() and pop_item() methods - Handle both bytes (default) and string (decode_responses=True) Redis responses - Add comprehensive test coverage for client ownership scenarios - Add tests for decode_responses=True client compatibility with both fakeredis and real Redis - Ensure proper resource management and thread-safe operations - Maintain backward compatibility while fixing production edge cases This addresses Redis client lifecycle management issues and ensures compatibility with common Redis client configurations used in production.
- Rename test_injection_like_content to test_data_integrity_with_problematic_strings - Update test documentation to accurately describe what it validates - Remove misleading security claims about SQL injection testing - Add additional test cases for JSON-like strings and escape sequences - Focus on actual technical challenges: JSON parsing, serialization, and string escaping - Improve code clarity with better comments explaining each test case - Fix line length issues to meet project style standards This test now honestly represents what it validates: data integrity with strings that could potentially break parsers, rather than making false claims about injection vulnerability testing.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
Add comprehensive tests for previously uncovered code paths: - test_get_next_id_method: Test atomic counter functionality for message IDs - test_corrupted_data_handling: Test graceful handling of malformed JSON data - test_ping_connection_failure: Test network failure scenarios with mocked exceptions - test_close_method_coverage: Test client ownership edge cases in close() method
Replace type ignores with proper type casting and safe content access: - Use cast() for FakeRedis Redis type compatibility - Replace direct dict access with .get() methods - Add _safe_rpush helper for async/sync operation handling - Maintain mypy effectiveness without defeating type checking
|
@seratch I've raised the coverage to 87%, but this is still failing the coverage check. It will be difficult to add any more coverage for this code. There is other memory code, not related to this PR, with much less coverage, e.g. src/agents/memory/openai_conversations_session.py has 27%, this might be better targets. Also, I wasnt aware there was a coverage issue when developing, the make check target runs make tests, not make coverage, might be good to update this? |
- Add test cases covering constructor, lifecycle, CRUD operations, error handling, and runner integration - Increase openai_conversations_session.py coverage from 27% to 79% - Brings overall project coverage to 95%, meeting required threshold Note: Tests assert expected behavior based on code analysis. OpenAI conversations session domain expert should review for accuracy. Coverage improvement is critical for CI/CD pipeline requirements.
|
@seratch I've added a test file for openai_conversations_session.py even though this is not directly related to this PR. This has increased the coverage for this code from 27% to 79%, this bumps the overall coverage to 95%. Someone who understand this capability better should review this test file, I've just asserted logic based on the implementation, I dont understand this feature and therefore do not know if the assertions are sensible. |
- Add session.clear_session() at start of redis_session_example.py for clean demonstrations - Update docstring to explain session clearing behavior and production considerations - Prevents confusion from accumulated data across multiple example runs - Ensures consistent, predictable example behavior This addresses potential user confusion when running examples multiple times, as Redis sessions persist data between runs by design.
| @@ -0,0 +1,445 @@ | |||
| """Tests for OpenAI Conversations Session functionality.""" | |||
The RedisSession enables production-grade, distributed session memory that scales across multiple application instances while maintaining full compatibility with the existing Session interface.