Repository navigation
fix(memory): isolate ingested events and returned search content - #7566
hackermengzhi wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates the in-memory memory service to deep-copy events and content when storing or searching memories, ensuring that ingested events and search results are detached from the caller. Unit tests are also added to verify this behavior. The review feedback correctly points out that using Pydantic's model_copy(deep=True) on Event and Session instances bypasses their custom __deepcopy__ implementations. It is recommended to use copy.deepcopy() instead to ensure the custom deep-copy logic is properly executed.
| self._session_events[user_key] = self._session_events.get(user_key, {}) | ||
| self._session_events[user_key][session.id] = [ | ||
| event | ||
| event.model_copy(deep=True) |
There was a problem hiding this comment.
The Event class explicitly overrides __deepcopy__ with a custom implementation (_model_copy.deep_copy_model) to handle its copying logic correctly. Using event.model_copy(deep=True) bypasses this custom __deepcopy__ because Pydantic's model_copy deep-copies the underlying dictionary directly rather than calling copy.deepcopy on the model instance.\n\nTo ensure the custom deep copy logic is executed, use copy.deepcopy(event) instead.\n\nNote: You will need to import copy at the top of the file.
| event.model_copy(deep=True) | |
| copy.deepcopy(event) |
There was a problem hiding this comment.
Verified against the actual Pydantic implementation: BaseModel.model_copy(deep=True) without update calls self.__deepcopy__(), so these calls do invoke Event/Session's custom copier. ADK's utils/_model_copy.py::deep_copy_model also explicitly documents support for both copy.deepcopy and model_copy(deep=True).
I ran an additional probe using that production ADK helper as a custom model copier: model_copy(deep=True) invoked the override once and editing nested copied content left the source unchanged. The existing four public-service regressions and full memory-service tests already pass (53 tests). Keeping the current calls avoids a behavior-neutral rewrite; the bypass described in these comments does not occur for these calls.
| for event in events_to_add: | ||
| if event.id not in existing_ids and event.id not in deleted_ids: | ||
| existing_events.append(event) | ||
| existing_events.append(event.model_copy(deep=True)) |
There was a problem hiding this comment.
Similarly to add_session_to_memory, using event.model_copy(deep=True) bypasses the custom __deepcopy__ implementation defined on the Event class. Use copy.deepcopy(event) to ensure the custom copy logic is respected.
| existing_events.append(event.model_copy(deep=True)) | |
| existing_events.append(copy.deepcopy(event)) |
| ) -> None: | ||
| """Changing an ingested event must not rewrite stored memory.""" | ||
| service = InMemoryMemoryService() | ||
| session = MOCK_SESSION_1.model_copy(deep=True) |
There was a problem hiding this comment.
The Session class overrides __deepcopy__ with _model_copy.deep_copy_model. Using model_copy(deep=True) bypasses this custom copy logic. It is safer to use copy.deepcopy(MOCK_SESSION_1) instead.\n\nNote: You will need to import copy in this test file.
| session = MOCK_SESSION_1.model_copy(deep=True) | |
| session = copy.deepcopy(MOCK_SESSION_1) |
| ) -> None: | ||
| """Editing returned content must not affect memory or the source session.""" | ||
| service = InMemoryMemoryService() | ||
| session = MOCK_SESSION_1.model_copy(deep=True) |
There was a problem hiding this comment.
Similarly, use copy.deepcopy(MOCK_SESSION_1) to respect the custom __deepcopy__ implementation of the Session class.
| session = MOCK_SESSION_1.model_copy(deep=True) | |
| session = copy.deepcopy(MOCK_SESSION_1) |
Summary
Fixes #7565.
Detach ingested events and returned search content in
InMemoryMemoryService. Editing an event after ingestion or nested content in a search result no longer rewrites stored memory or the source session. Keyword ranking, duplicate handling and deletion logic stay unchanged.Testing plan
tests/unittests/memory/test_in_memory_memory_service.py: 53 passed.Event, search, mutate returned text and nested arguments, then search again and verify original content. No cloud services or LLM calls.Verified on macOS arm64, Python 3.12.13. Repository-wide CI and live-provider integration are not claimed.