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 2 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(). |
| def _redis_key(self, session_id: str | None) -> str: | ||
| """Get the Redis key for a given session's messages.""" | ||
| return f"{self.key_prefix}:{session_id or 'default'}" | ||
| return f"{self.key_prefix}:{self.source_id}:{session_id or 'default'}" |
There was a problem hiding this comment.
Good catch. Colon-joined segments are ambiguous whenever one of them carries a colon, so the separator is now the ASCII unit separator in d309e32. Same migration story as before: keys written with the colon layout stay in place and become unreadable by the new code.
| with patch("agent_framework_redis._history_provider.redis.from_url") as mock_from_url: | ||
| mock_from_url.return_value = mock_redis_client | ||
| audit = RedisHistoryProvider("audit", redis_url="redis://localhost:6379") | ||
| primary = RedisHistoryProvider("primary", redis_url="redis://localhost:6379") |
There was a problem hiding this comment.
Fixed in d309e32 by making the variable load-bearing: the test now also asserts the delete was NOT issued for the other provider's key, so primary is part of the expectation.
…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
| def _redis_key(self, session_id: str | None) -> str: | ||
| """Get the Redis key for a given session's messages.""" | ||
| return f"{self.key_prefix}:{session_id or 'default'}" | ||
| return self._KEY_SEP.join([self.key_prefix, self.source_id, session_id or "default"]) |
There was a problem hiding this comment.
Could _redis_key encode each component injectively instead of joining opaque IDs with \x1f? source_id="audit", session_id="x\x1fy" and source_id="audit\x1fx", session_id="y" produce the same Redis key, so get_messages() can return the other provider's history and clear() can delete it. Length-prefixing or encoding each component would preserve isolation for the identifier domain the base API accepts.
| def _redis_key(self, session_id: str | None) -> str: | ||
| """Get the Redis key for a given session's messages.""" | ||
| return f"{self.key_prefix}:{session_id or 'default'}" | ||
| return self._KEY_SEP.join([self.key_prefix, self.source_id, session_id or "default"]) |
There was a problem hiding this comment.
Also, have we thought about an explicit migration path before changing every existing Redis key? After an upgrade, data written as chat_messages:<session_id> is no longer read, trimmed, or cleared because all operations switch to the new key_prefix\x1fsource_id\x1fsession_id layout, so production conversation history appears lost and a later rollback sees a divergent history. Could we provide an opt-in one-provider migration utility or compatibility mode while keeping unsafe automatic fallback disabled for multi-provider deployments?
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.