Skip to content

Python: fix(redis): scope RedisHistoryProvider keys by source_id #7494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Yufeng He (he-yufeng) wants to merge 2 commits into
base: main
Choose a base branch
from
+29 −5
Open

Python: fix(redis): scope RedisHistoryProvider keys by source_id #7494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47c0a0a
fix(redis): scope RedisHistoryProvider keys by source_id
he-yufeng Aug 3, 2026
d309e32
fix(redis): make the provider key separator collision-proof and use i…
he-yufeng Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion python/packages/redis/agent_framework_redis/_history_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,13 @@ def __init__(
else:
self._redis_client = redis.from_url(redis_url, decode_responses=True) # type: ignore[no-untyped-call]

# Unit separator: source ids and session ids are opaque strings and can
# legitimately contain ':', which would make colon-joined keys ambiguous.
_KEY_SEP = "\x1f"

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"])

@moonbox3 Evan Mattson (moonbox3) Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@moonbox3 Evan Mattson (moonbox3) Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


async def get_messages(
self,
Expand Down
28 changes: 24 additions & 4 deletions python/packages/redis/tests/test_providers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,8 +420,16 @@ def test_key_format(self, mock_redis_client: MagicMock):
mock_from_url.return_value = mock_redis_client
provider = RedisHistoryProvider("mem", redis_url="redis://localhost:6379", key_prefix="msgs")

assert provider._redis_key("session-123") == "msgs:session-123"
assert provider._redis_key(None) == "msgs:default"
assert provider._redis_key("session-123") == "msgs\x1fmem\x1fsession-123"
assert provider._redis_key(None) == "msgs\x1fmem\x1fdefault"

def test_keys_isolated_per_source_id(self, mock_redis_client: MagicMock):
with patch("agent_framework_redis._history_provider.redis.from_url") as mock_from_url:
mock_from_url.return_value = mock_redis_client
first = RedisHistoryProvider("audit", redis_url="redis://localhost:6379", key_prefix="msgs")
second = RedisHistoryProvider("primary", redis_url="redis://localhost:6379", key_prefix="msgs")

assert first._redis_key("s1") != second._redis_key("s1")


class TestRedisHistoryProviderGetMessages:
Expand Down Expand Up @@ -482,7 +490,7 @@ async def test_max_messages_trimming(self, mock_redis_client: MagicMock):

await provider.save_messages("s1", [Message(role="user", contents=["msg"])])

mock_redis_client.ltrim.assert_called_once_with("chat_messages:s1", -10, -1)
mock_redis_client.ltrim.assert_called_once_with("chat_messages\x1fmem\x1fs1", -10, -1)

async def test_no_trim_when_under_limit(self, mock_redis_client: MagicMock):
mock_redis_client.llen = AsyncMock(return_value=3)
Expand All @@ -503,7 +511,19 @@ async def test_clear_calls_delete(self, mock_redis_client: MagicMock):
provider = RedisHistoryProvider("mem", redis_url="redis://localhost:6379")

await provider.clear("session-1")
mock_redis_client.delete.assert_called_once_with("chat_messages:session-1")
mock_redis_client.delete.assert_called_once_with("chat_messages\x1fmem\x1fsession-1")

async def test_clear_leaves_other_source_ids_untouched(self, mock_redis_client: MagicMock):
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")

await audit.clear("session-1")
# the destructive case from #7471: clearing one provider must not
# delete the shared session's messages belonging to another provider
mock_redis_client.delete.assert_called_once_with("chat_messages\x1faudit\x1fsession-1")
assert primary._redis_key("session-1") not in mock_redis_client.delete.call_args.args


class TestRedisHistoryProviderBeforeAfterRun:
Expand Down
Loading