Skip to content

Python: fix(redis): deduplicate RedisContextProvider writes - #10179

Closed
pratik wayase (PratikWayase) wants to merge 2 commits into
microsoft:mainfrom
PratikWayase:fix/redis-context-provider-duplicate-save
Closed

pratik wayase (PratikWayase) wants to merge 2 commits into
microsoft:mainfrom
PratikWayase:fix/redis-context-provider-duplicate-save

Conversation

@PratikWayase

Copy link
Copy Markdown
Contributor

Motivation & Context

RedisContextProvider.after_run saves every message in context.input_messages on each run. Behind the AG-UI endpoint, the TypeScript client (@ag-ui/client HttpAgent) resends the whole conversation on every request, so each turn re-saves everything that came before. In a 15-message chat I ended up with 240 documents in Redis; "We deploy to westus3." was stored 14 times. Search results got flooded with copies of the earliest messages, so the deployment fact never reached the model the agent answered "I don't have the deployment location" in a new chat.

The same chat, sent one message at a time, stored 30 documents and answered correctly.

_add made it worse: it called redis_index.load(prepared) without keys, so RedisVL handed every document a brand-new random key. Nothing checked whether a message was already stored.

History providers were fixed for this same replay pattern in #7242 and #8269. Memory providers were not.

Description & Review Guide

  • What are the major changes?

    • Each memory document now gets a deterministic key derived from sha256(scope_fields + role + content). I deliberately did not use message_id the assistant reply comes back with a different id than the one stored (the stored id is the chatcmpl id; the AG-UI stream uses its own id), so message_id-based keys would never match.
    • _add pipelines a single EXISTS check to find which candidate keys are already in Redis, and skips them. They are neither re-embedded nor re-stored.
    • Survivors are loaded with explicit keys via redis_index.key(suffix). This reads the real index prefix and separator, so borrowed indexes with non-default prefixes still produce searchable keys.
    • Whitespace-only content (tool-call messages, etc.) is skipped before keying, and content is .strip()-ed so whitespace differences between the stored model text and the replayed AG-UI text don't produce a second copy.
    • Refactored the scope-field assembly into a small _scope_values(session_id) helper, used by _add, _document_redis_key, and _redis_search. They can no longer drift apart.
  • What is the impact of these changes?

    • Storage and embedding calls grow linearly with conversation length, not quadratically.
    • Search results stay distinct, so facts actually reach the model.
    • Pre-existing conversations get one extra copy of each message on the first post-fix run, then stay stable. Old documents keep their random keys.
    • Same text from the same role in one conversation is stored once. "yes" / "thanks" repeats collapse to one entry this is the tradeoff the issue accepted.
  • What do you want reviewers to focus on?

    • The key derivation in _document_key_suffix and the dedup flow in _add.
    • The choice to key on content (not message_id) and the .strip() normalization these are intentional and called out in the issue.
    • The pipeline mock additions to mock_index in test_context_provider_edges.py and test_providers.py both files had separate fixtures that needed the same update.

Related Issue

Fixes #10127

AI Assistance

  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: AI assisted with issue investigation, the reproduction script, and PR text error handling. I verified everything against a live Redis and went through multiple review rounds myself.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI balanced review requested due to automatic review settings November 8, 2026 14:44
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Nov 8, 2026
@github-actions github-actions Bot changed the title fix(redis): deduplicate RedisContextProvider writes Python: fix(redis): deduplicate RedisContextProvider writes Nov 8, 2026

Copilot AI left a comment

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.

🟡 Changes recommended

Reused pipeline mocks accumulate EXISTS calls and break multi-run replay tests, while regression coverage does not verify that existing documents are skipped during embedding and loading.

2 open issues
What changed in this PR

This PR prevents replayed conversations from creating duplicate Redis memory documents, extending replay protection to Python’s RedisContextProvider.

Changes:

  • Derives deterministic keys from scope, role, and trimmed content, while respecting the index’s configured prefix.
  • Checks existing keys before embedding or storing documents and deduplicates messages within each batch.
  • Adds replay, key-stability, and whitespace-handling tests, plus updated pipeline mocks.
File Description
python/​packages/​redis/​tests/​test_providers.py Adds pipeline and deterministic-key mocks.
python/​packages/​redis/​tests/​test_context_provider_sdk.py Tests replay filtering, embedding skips, and key identity.
python/​packages/​redis/​tests/​test_context_provider_integration.py Adds live Redis replay and stable-key checks.
python/​packages/​redis/​tests/​test_context_provider_edges.py Updates mocks for existence checks and explicit keys.
python/​packages/​redis/​agent_framework_redis/​_context_provider.py Implements deterministic keys, deduplicated writes, and shared scope assembly.

Review effort: Balanced

@PratikWayase

Copy link
Copy Markdown
Contributor Author

The failing test test_file_checkpoint_storage_concurrent_saves_across_instances is in packages/core and tests FileCheckpointStorage on the filesystem. this PR only touches packages/redis. The failure is a Windows PermissionError on os.replace(), which is a known flaky file-rename race on Windows CI. Re-running the failed job should clear it; nothing in this PR could have caused it

@eavanvalkenburg

Copy link
Copy Markdown
Member

pratik wayase (@PratikWayase), thank you for the contribution. The linked issue (#10127) is not marked ready-for-implementation, and I cannot find an explicit maintainer comment accepting this implementation direction. Please settle the scope in the issue and wait for explicit maintainer agreement before opening a fresh implementation PR. I am closing this for now.

This branch was previously deployed

1 inactive deployment
github-app-auth — ba17df15 Deployed Nov 8, 2026 by PratikWayase via add_label #27364
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: RedisContextProvider saves the whole conversation again on every AG-UI run, so memory search returns copies and misses facts

4 participants