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 3 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.
There was a problem hiding this comment.
Fair point, separator joining was still ambiguous. 22a5adf switches to length-prefixed components (<len>:<value> per segment), so ("audit", "x\x1fy") and ("audit\x1fx", "y") now produce distinct keys by construction. Added test_key_join_is_injective with exactly your pair.
| 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?
There was a problem hiding this comment.
Thought about it and added the lazy path in 22a5adf: reads against the pre-scoping layout (<key_prefix>:<session_id>) migrate with renamenx on first touch (atomic, and a no-op if a concurrent write already landed on the new key), and clear() now deletes both the scoped and the legacy key so old data stays erasable. Two tests cover the migration and the no-migration-when-current case. If you would rather see an eager migration helper or a documented break instead, say the word and I will adjust.
GitHub Actions
Active
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.