Python: fix(redis): scope RedisHistoryProvider keys by source_id - #7494
Python: fix(redis): scope RedisHistoryProvider keys by source_id#7494Yufeng He (he-yufeng) wants to merge 5 commits into
Conversation
Two providers with different source_ids but the same key_prefix shared one Redis list per session, so a write-only audit sink contaminated the primary provider's loaded history, and clear() on one deleted the other's conversation. The key now includes source_id, matching the Cosmos provider's scoping. Existing keys written under the old layout are left in place; deleting them would risk removing a sibling provider's data, and they simply become unreadable by the new code.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes a Redis key-collision bug in the Python RedisHistoryProvider by scoping stored message lists by source_id, preventing multiple providers with the same key_prefix from contaminating or deleting each other’s session history (as described in #7471).
Changes:
- Updated
RedisHistoryProviderRedis key layout to includesource_id({key_prefix}:{source_id}:{session_id|default}). - Updated and extended Redis provider tests to reflect the new key format and to verify per-
source_idisolation and safeclear()behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/redis/agent_framework_redis/_history_provider.py | Includes source_id in the Redis storage key to isolate histories across provider instances. |
| python/packages/redis/tests/test_providers.py | Updates key-format assertions and adds coverage proving source_id isolation and non-destructive clear(). |
…t in tests Colon-joined keys were ambiguous for source ids or session ids containing a colon (a:b + c vs a + b:c). Join with the ASCII unit separator instead. The clear-isolation test now asserts on the other provider's key too, so the unused variable lint is gone as well.
GitHub Actions
Inactive
GitHub Actions
Inactive
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
|
Yufeng He (@he-yufeng) checks are failing here |
…rovider-source-id-key
GitHub Actions
Inactive
|
Merged current main in so the checks re-run fresh. Same story as the sibling PR: the failing pre-commit step was flagging a file this PR never touches. |
GitHub Actions
Inactive
|
Package Checks red was mine too: pyright on current main rejects the two type: ignore[misc] on the exists/renamenx calls as unnecessary. Dropped both in 154cd8a. |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (5 commit(s)): 47c0a0ac9715, d309e32691ce, 22a5adff3892, 042eae044cba, 154cd8a1aadd
Model: gpt-5.6-sol
Overview
The length-prefixed key format correctly isolates new Redis history by source and prevents delimiter-based collisions, with focused tests covering both properties. However, the automatic legacy-key compatibility path cannot determine which source owns previously shared data, and its destructive migration and cleanup behavior can expose, lose, or fork conversation history. Error suppression around that migration also turns Redis authorization and operational failures into silent missing history.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
4 verified findings remained after source verification (3 high, 1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/redis/agent_framework_redis/_history_provider.py
|
Yufeng He (@he-yufeng) please re-open when all comments are addressed, and threads are resolved with the proper message(s). |
|
All four review threads are now addressed in the code and resolved with notes above. The migration is gone in favor of an in-place read merge, which is what threads 1, 2 and 4 were really after, and the blanket suppress left with the rename. One mechanical note: GitHub will not reopen this PR now that the branch has moved, so the rework continues as #7845 (same branch, same scope, Fixes #7471). Tests: 43 passed in the redis package, ruff and pyright clean. |
Closes #7471.
_redis_keynow readskey_prefix:source_id:session_id, so two providers sharing a key_prefix stop sharing a Redis list: the audit sink's copies no longer load back into the primary provider's context, andclear()on one can no longer wipe the other's session. This matches howCosmosHistoryProviderscopes everything by source_id.On compatibility: keys written under the old layout stay in Redis but become unreadable by the new code. I deliberately did not make
clear()delete the old shared key, since that key can hold a sibling provider's history and deleting it would reproduce the exact cross-provider destruction this fixes. A leftover key per session is harmless beyond the storage; admins can expire it manually.Tests: the key-format and trim/clear assertions moved to the new layout, plus two new cases proving keys differ per source_id and that clearing one provider leaves the other provider's list untouched. 57/57 in the redis package suite pass locally.