diff --git a/mem0/memory/main.py b/mem0/memory/main.py index 0cd636e91..84b760790 100644 --- a/mem0/memory/main.py +++ b/mem0/memory/main.py @@ -644,6 +644,9 @@ class Memory(MemoryBase): if memory_id and (metadata.get("agent_id") or metadata.get("run_id")): # Update only the session identifiers, keep content the same existing_memory = self.vector_store.get(vector_id=memory_id) + if existing_memory is None: + logger.warning(f"Memory {memory_id} not found for session ID update, skipping") + continue updated_metadata = deepcopy(existing_memory.payload) if metadata.get("agent_id"): updated_metadata["agent_id"] = metadata["agent_id"] @@ -1226,6 +1229,9 @@ class Memory(MemoryBase): logger.error(f"Error getting memory with ID {memory_id} during update.") raise ValueError(f"Error getting memory with ID {memory_id}. Please provide a valid 'memory_id'") + if existing_memory is None: + raise ValueError(f"Memory with id {memory_id} not found. Please provide a valid 'memory_id'") + prev_value = existing_memory.payload.get("data") new_metadata = deepcopy(metadata) if metadata is not None else {} @@ -1274,6 +1280,8 @@ class Memory(MemoryBase): def _delete_memory(self, memory_id): logger.info(f"Deleting memory with {memory_id=}") existing_memory = self.vector_store.get(vector_id=memory_id) + if existing_memory is None: + raise ValueError(f"Memory with id {memory_id} not found") prev_value = existing_memory.payload.get("data", "") self.vector_store.delete(vector_id=memory_id) self.db.add_history( @@ -1671,6 +1679,9 @@ class AsyncMemory(MemoryBase): # Create async task to update only the session identifiers async def update_session_ids(mem_id, meta): existing_memory = await asyncio.to_thread(self.vector_store.get, vector_id=mem_id) + if existing_memory is None: + logger.warning(f"Memory {mem_id} not found for session ID update, skipping") + return updated_metadata = deepcopy(existing_memory.payload) if meta.get("agent_id"): updated_metadata["agent_id"] = meta["agent_id"] @@ -2313,6 +2324,9 @@ class AsyncMemory(MemoryBase): logger.error(f"Error getting memory with ID {memory_id} during update.") raise ValueError(f"Error getting memory with ID {memory_id}. Please provide a valid 'memory_id'") + if existing_memory is None: + raise ValueError(f"Memory with id {memory_id} not found. Please provide a valid 'memory_id'") + prev_value = existing_memory.payload.get("data") new_metadata = deepcopy(metadata) if metadata is not None else {} @@ -2364,6 +2378,8 @@ class AsyncMemory(MemoryBase): async def _delete_memory(self, memory_id): logger.info(f"Deleting memory with {memory_id=}") existing_memory = await asyncio.to_thread(self.vector_store.get, vector_id=memory_id) + if existing_memory is None: + raise ValueError(f"Memory with id {memory_id} not found") prev_value = existing_memory.payload.get("data", "") await asyncio.to_thread(self.vector_store.delete, vector_id=memory_id) diff --git a/tests/test_memory.py b/tests/test_memory.py index ed98e6ab8..14214dcb5 100644 --- a/tests/test_memory.py +++ b/tests/test_memory.py @@ -320,3 +320,121 @@ def test_normalize_facts_mixed(): def test_normalize_facts_filters_empty_strings(): assert normalize_facts(["", "valid", ""]) == ["valid"] + + +@patch('mem0.utils.factory.EmbedderFactory.create') +@patch('mem0.utils.factory.VectorStoreFactory.create') +@patch('mem0.utils.factory.LlmFactory.create') +@patch('mem0.memory.storage.SQLiteManager') +def test_delete_nonexistent_memory_raises_error(mock_sqlite, mock_llm_factory, mock_vector_factory, mock_embedder_factory): + """ + Test that delete() raises ValueError when memory_id does not exist + and does not attempt to delete from the vector store. + + Issue #3849: memory.delete() fails with AttributeError when memory not found. + Should raise a clear ValueError instead. + """ + mock_embedder_factory.return_value = MagicMock() + mock_vector_store = MagicMock() + mock_vector_factory.return_value = mock_vector_store + mock_llm_factory.return_value = MagicMock() + mock_sqlite.return_value = MagicMock() + + from mem0.memory.main import Memory as MemoryClass + config = MemoryConfig() + memory = MemoryClass(config) + + mock_vector_store.get.return_value = None + + with pytest.raises(ValueError, match="Memory with id non-existent-id not found"): + memory.delete("non-existent-id") + + mock_vector_store.delete.assert_not_called() + + +@pytest.mark.asyncio +@patch('mem0.utils.factory.EmbedderFactory.create') +@patch('mem0.utils.factory.VectorStoreFactory.create') +@patch('mem0.utils.factory.LlmFactory.create') +@patch('mem0.memory.storage.SQLiteManager') +async def test_async_delete_nonexistent_memory_raises_error(mock_sqlite, mock_llm_factory, mock_vector_factory, mock_embedder_factory): + """ + Test that async delete() raises ValueError when memory_id does not exist + and does not attempt to delete from the vector store. + + Issue #3849: memory.delete() fails with AttributeError when memory not found. + Should raise a clear ValueError instead. + """ + mock_embedder_factory.return_value = MagicMock() + mock_vector_store = MagicMock() + mock_vector_store.get.return_value = None + mock_vector_factory.return_value = mock_vector_store + mock_llm_factory.return_value = MagicMock() + mock_sqlite.return_value = MagicMock() + + from mem0.memory.main import AsyncMemory + config = MemoryConfig() + memory = AsyncMemory(config) + + with pytest.raises(ValueError, match="Memory with id non-existent-id not found"): + await memory.delete("non-existent-id") + + mock_vector_store.delete.assert_not_called() + + +@patch('mem0.utils.factory.EmbedderFactory.create') +@patch('mem0.utils.factory.VectorStoreFactory.create') +@patch('mem0.utils.factory.LlmFactory.create') +@patch('mem0.memory.storage.SQLiteManager') +def test_update_nonexistent_memory_raises_error(mock_sqlite, mock_llm_factory, mock_vector_factory, mock_embedder_factory): + """ + Test that _update_memory() raises ValueError when memory_id does not exist. + + Same class of bug as #3849 — vector_store.get() returns None and code + accesses .payload without a null check. + """ + mock_embedder_factory.return_value = MagicMock() + mock_vector_store = MagicMock() + mock_vector_factory.return_value = mock_vector_store + mock_llm_factory.return_value = MagicMock() + mock_sqlite.return_value = MagicMock() + + from mem0.memory.main import Memory as MemoryClass + config = MemoryConfig() + memory = MemoryClass(config) + + mock_vector_store.get.return_value = None + + with pytest.raises(ValueError, match="Memory with id non-existent-id not found"): + memory._update_memory("non-existent-id", "new data", {"new data": [0.1, 0.2]}) + + mock_vector_store.update.assert_not_called() + + +@pytest.mark.asyncio +@patch('mem0.utils.factory.EmbedderFactory.create') +@patch('mem0.utils.factory.VectorStoreFactory.create') +@patch('mem0.utils.factory.LlmFactory.create') +@patch('mem0.memory.storage.SQLiteManager') +async def test_async_update_nonexistent_memory_raises_error(mock_sqlite, mock_llm_factory, mock_vector_factory, mock_embedder_factory): + """ + Test that async _update_memory() raises ValueError when memory_id does not exist. + + Same class of bug as #3849 — vector_store.get() returns None and code + accesses .payload without a null check. + """ + mock_embedder_factory.return_value = MagicMock() + mock_vector_store = MagicMock() + mock_vector_store.get.return_value = None + mock_vector_factory.return_value = mock_vector_store + mock_llm_factory.return_value = MagicMock() + mock_sqlite.return_value = MagicMock() + + from mem0.memory.main import AsyncMemory + config = MemoryConfig() + memory = AsyncMemory(config) + + with pytest.raises(ValueError, match="Memory with id non-existent-id not found"): + await memory._update_memory("non-existent-id", "new data", {"new data": [0.1, 0.2]}) + + mock_vector_store.update.assert_not_called()