Skip to content

fix: stop get_cache handing out the stored KV cache by reference - #2303

Open
jeojdi1 wants to merge 1 commit into
MemTensor:mainfrom
jeojdi1:fix/kv-cache-mutated-in-place-by-generate
Open

fix: stop get_cache handing out the stored KV cache by reference#2303
jeojdi1 wants to merge 1 commit into
MemTensor:mainfrom
jeojdi1:fix/kv-cache-mutated-in-place-by-generate

Conversation

@jeojdi1

@jeojdi1 jeojdi1 commented Aug 28, 2026

Copy link
Copy Markdown

Description

_concat_caches returns caches[0] unchanged when a single cache id is requested, so get_cache() hands the caller the stored KVCacheItem.memory object itself. The caller passes that cache to generate(), which appends to it in place — so the saved activation memory grows on every chat turn.

Measured across three turns, the stored cache length went 6 → 19 → 32 → 45. Nothing in the API suggests that retrieving a memory mutates it, and the multi-cache path already builds a fresh container, so only this early return leaks the reference.

This PR returns a new DynamicCache sharing the same tensors. The tensors are deliberately not cloned: generate() appends along the sequence axis rather than writing into existing rows, so a fresh container is sufficient to protect the stored item without paying to duplicate the cache.

Related Issue (Required): #2301

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Unit Test

test_get_cache_does_not_alias_stored_memory asserts the returned object is not the stored one, then simulates generate() by appending a step to the returned cache and asserts the stored length is unchanged.

Verified fails-before / passes-after on transformers 5.16.1:

# upstream
FAILED tests/memories/activation/test_kv.py::test_get_cache_does_not_alias_stored_memory
1 failed

# with this change
1 passed

Checklist

A note on the target branch

CONTRIBUTING.md says to open PRs against dev, but no dev branch exists — only main and dev-v2.0.28dev-v2.0.32. This is against main (185ebdb, "Dev v2.0.32"). Happy to retarget.

`_concat_caches` returns `caches[0]` unchanged when a single cache id is
requested, so `get_cache` hands the caller the stored `KVCacheItem.memory`
object itself. The caller passes that cache to `generate`, which appends to it
in place, so the saved activation memory grows on every chat turn -- measured
stored length 6 -> 19 -> 32 -> 45 over three turns.

Nothing in the API suggests that retrieving a memory mutates it, and the
multi-cache path already builds a fresh container, so only this early return
leaked the reference.

Returns a new `DynamicCache` sharing the same tensors instead. The tensors are
not cloned: `generate` appends along the sequence axis rather than writing into
existing rows, so a fresh container is enough to protect the stored item without
paying to duplicate the cache.

Adds `test_get_cache_does_not_alias_stored_memory`, which simulates `generate`
by appending to the returned cache and asserts the stored length is unchanged.
It fails on the current code and passes with this change.
@Memtensor-AI Memtensor-AI added area:memory 记忆存储、检索、更新、召回逻辑 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Aug 28, 2026
@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2303
Task: c9892125fee30ed9
Base: main
Head: fix/kv-cache-mutated-in-place-by-generate

🔍 OpenCodeReview found 2 issue(s) in this PR.


1. src/memos/memories/activation/kv.py (L92)

torch is imported but never referenced anywhere in _copy_cache. The # noqa: F401 suppressor confirms the linter already caught this. Unlike _concat_caches which actually calls torch.cat, this method performs only Python-level attribute copies. Remove the import.

💡 Suggested Change

Before:

        import torch  # noqa: F401  (kept local, matching this module's style)

After:

        # torch not needed here; tensors are shared by reference, not constructed

2. src/memos/memories/activation/kv.py (L103-L108)

dst.lazy_initialization(src.keys, src.values) is called conditionally, but dst.keys = src.keys and dst.values = src.values are then assigned unconditionally regardless of whether lazy_initialization ran. This has two problems:

  1. If lazy_initialization only sets keys and values, the call is always redundant (the unconditional assignments below handle it).
  2. If lazy_initialization does additional setup beyond setting those fields (e.g., allocating internal buffers or registering tensors), the unconditional overwrites on the next two lines may leave dst in an inconsistent internal state.

Consider one of:

  • Remove the lazy_initialization call and rely solely on the unconditional assignments (correct for the shallow-copy intent documented in the docstring).
  • Use an else branch so the assignments only happen when lazy_initialization was NOT called, keeping both paths mutually exclusive.
💡 Suggested Change

Before:

                if not getattr(dst, "is_initialized", True) and hasattr(
                    dst, "lazy_initialization"
                ):
                    dst.lazy_initialization(src.keys, src.values)
                dst.keys = src.keys
                dst.values = src.values

After:

                # Unconditional shallow assignment is sufficient for a
                # shallow-structure copy; lazy_initialization is not needed.
                dst.keys = src.keys
                dst.values = src.values

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Test collection failed because the torch module is not installed in the test environment, preventing the test file from being imported. [advisory, non-gating] AI-generated tests on branch test/auto-gen-c9892125fee30ed9-20260829013054: 36/36 passed — these do NOT affect the PR verdict; review the branch manually.
Branch: fix/kv-cache-mutated-in-place-by-generate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:memory 记忆存储、检索、更新、召回逻辑 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants