Skip to content

Commit fa103c0

Browse files
committed
Fix #2331: purge embeddings when delete_node_by_prams removes memories
delete_node_by_prams deleted the graph nodes but never purged the matching vectors from vec_db, so a deleted memory stayed retrievable through vector search while the delete API answered success. The sibling paths clear() and delete_node_by_mem_cube_id() already purge vec_db; this one omitted the step. The node ids are collected and the nodes deleted in a single statement, so the ids that are purged are exactly the nodes this call removed - two separate auto-commit statements leave a window in which a concurrent writer adds nodes matching the same filter, whose vectors would then be orphaned. Also drops the debug print() calls from this method (the repo logs via get_logger).
1 parent a7367d0 commit fa103c0

3 files changed

Lines changed: 291 additions & 26 deletions

File tree

‎src/memos/graph_dbs/neo4j.py‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1893,6 +1893,24 @@ def _parse_node(self, node_data: dict[str, Any]) -> dict[str, Any]:
18931893
node["sources"][idx] = json.loads(node["sources"][idx])
18941894
return {"id": node.pop("id"), "memory": node.pop("memory", ""), "metadata": node}
18951895

1896+
def _delete_vectors_for_deleted_nodes(self, node_ids: list[str]) -> None:
1897+
"""Purge the embeddings of just-deleted graph nodes from the vector store.
1898+
1899+
Deleting a graph node alone leaves its vector behind, and vector search is
1900+
what retrieval reads first, so the deleted memory stays retrievable. The
1901+
vector store is an external dependency, so a failure here is logged rather
1902+
than raised: the graph deletion is already committed and must not be
1903+
reported as failed.
1904+
"""
1905+
vec_db = getattr(self, "vec_db", None)
1906+
if not node_ids or vec_db is None:
1907+
return
1908+
try:
1909+
vec_db.delete(node_ids)
1910+
logger.info("[delete_node_by_prams] Deleted %d vectors from VecDB", len(node_ids))
1911+
except Exception as e:
1912+
logger.warning("[delete_node_by_prams] Failed to delete vectors from VecDB: %s", e)
1913+
18961914
def delete_node_by_prams(
18971915
self,
18981916
writable_cube_ids: list[str] | None = None,
@@ -1994,15 +2012,24 @@ def delete_node_by_prams(
19942012
else:
19952013
final_where = data_conditions
19962014

1997-
# Delete directly without pre-counting
1998-
delete_query = f"MATCH (n:Memory) WHERE {final_where} DETACH DELETE n"
2015+
# Collect the matching node IDs and delete the nodes in ONE statement, so
2016+
# the IDs that are purged from the vector store are exactly the nodes this
2017+
# call removed: two separate auto-commit statements would let a concurrent
2018+
# writer slip nodes in between them, and those nodes' vectors would be
2019+
# left behind. RETURN rides along with DETACH DELETE, and the deleted
2020+
# count comes from the same summary the deletion produced.
2021+
delete_query = (
2022+
f"MATCH (n:Memory) WHERE {final_where} WITH n DETACH DELETE n RETURN n.id AS id"
2023+
)
19992024
logger.info(f"[delete_node_by_prams] delete_query: {delete_query}")
20002025

20012026
deleted_count = 0
2027+
deleted_ids: list[str] = []
20022028
try:
20032029
with self.driver.session(database=self.db_name) as session:
2004-
# Execute delete query
2030+
# Execute the collect-and-delete query
20052031
result = session.run(delete_query, **params)
2032+
deleted_ids = [record["id"] for record in result]
20062033
# Consume the result to ensure deletion completes and get the summary
20072034
summary = result.consume()
20082035
# Get the count from the result summary
@@ -2016,6 +2043,7 @@ def delete_node_by_prams(
20162043
logger.error(f"[delete_node_by_prams] Failed to delete nodes: {e}", exc_info=True)
20172044
raise
20182045

2046+
self._delete_vectors_for_deleted_nodes(deleted_ids)
20192047
logger.info(f"[delete_node_by_prams] Successfully deleted {deleted_count} nodes")
20202048
return deleted_count
20212049

‎src/memos/graph_dbs/neo4j_community.py‎

Lines changed: 13 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1034,40 +1034,30 @@ def delete_node_by_prams(
10341034
logger.info(
10351035
f"[delete_node_by_prams] Deleting nodes - memory_ids: {memory_ids}, file_ids: {file_ids}, filter: {filter}"
10361036
)
1037-
print(
1038-
f"[delete_node_by_prams] Deleting nodes - memory_ids: {memory_ids}, file_ids: {file_ids}, filter: {filter}"
1039-
)
10401037

1041-
# First count matching nodes to get accurate count
1042-
count_query = f"MATCH (n:Memory) WHERE {ids_where} RETURN count(n) AS node_count"
1043-
logger.info(f"[delete_node_by_prams] count_query: {count_query}")
1044-
print(f"[delete_node_by_prams] count_query: {count_query}")
1045-
1046-
# Then delete nodes
1047-
delete_query = f"MATCH (n:Memory) WHERE {ids_where} DETACH DELETE n"
1038+
# Collect the matching node IDs and delete the nodes in ONE statement, so
1039+
# the IDs that are purged from the vector store are exactly the nodes this
1040+
# call removed: two separate auto-commit statements would let a concurrent
1041+
# writer slip nodes in between them, and those nodes' vectors would be
1042+
# left behind.
1043+
delete_query = (
1044+
f"MATCH (n:Memory) WHERE {ids_where} WITH n DETACH DELETE n RETURN n.id AS id"
1045+
)
10481046
logger.info(f"[delete_node_by_prams] delete_query: {delete_query}")
1049-
print(f"[delete_node_by_prams] delete_query: {delete_query}")
1050-
print(f"[delete_node_by_prams] params: {params}")
10511047

10521048
deleted_count = 0
1049+
deleted_ids: list[str] = []
10531050
try:
10541051
with self.driver.session(database=self.db_name) as session:
1055-
# Count nodes before deletion
1056-
count_result = session.run(count_query, **params)
1057-
count_record = count_result.single()
1058-
expected_count = 0
1059-
if count_record:
1060-
expected_count = count_record["node_count"] or 0
1061-
1062-
# Delete nodes
1063-
session.run(delete_query, **params)
1064-
# Use the count from before deletion as the actual deleted count
1065-
deleted_count = expected_count
1052+
deleted_ids = [record["id"] for record in session.run(delete_query, **params)]
1053+
# Use the count from the deletion itself
1054+
deleted_count = len(deleted_ids)
10661055

10671056
except Exception as e:
10681057
logger.error(f"[delete_node_by_prams] Failed to delete nodes: {e}", exc_info=True)
10691058
raise
10701059

1060+
self._delete_vectors_for_deleted_nodes(deleted_ids)
10711061
logger.info(f"[delete_node_by_prams] Successfully deleted {deleted_count} nodes")
10721062
return deleted_count
10731063

Lines changed: 247 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,247 @@
1+
"""
2+
Regression test for issue #2331: deleting memories must also purge their vectors.
3+
4+
`Neo4jCommunityGraphDB.delete_node_by_prams` removed the Neo4j nodes but never
5+
removed the matching embeddings from `vec_db`, so a deleted memory stayed
6+
retrievable through vector search while the delete API still answered "success".
7+
`clear()` and `delete_node_by_mem_cube_id()` already purged `vec_db`;
8+
`delete_node_by_prams` simply omitted that step.
9+
10+
The Neo4j driver and the vector store are both faked, so these tests need no
11+
Neo4j, no Qdrant, and no network.
12+
"""
13+
14+
from types import SimpleNamespace
15+
from unittest.mock import MagicMock, patch
16+
17+
import pytest
18+
19+
from memos.configs.graph_db import Neo4jCommunityGraphDBConfig, Neo4jGraphDBConfig
20+
21+
22+
class _FakeNeo4jSession:
23+
"""Minimal stand-in for a neo4j session that tracks a set of live node ids."""
24+
25+
def __init__(self, live_ids: set[str], issued_queries: list[str]):
26+
self.live_ids = live_ids
27+
self.issued_queries = issued_queries
28+
29+
def run(self, query, params=None, **kwargs):
30+
self.issued_queries.append(query)
31+
params = params or kwargs
32+
ids = params.get("memory_ids") or params.get("filter_ids")
33+
matched = sorted(i for i in self.live_ids if ids is None or i in ids)
34+
if "count(n) AS node_count" in query:
35+
return _FakeResult([{"node_count": len(matched)}])
36+
if "DETACH DELETE" in query and "RETURN n.id AS id" in query:
37+
# The collect-and-delete statement: it removes the nodes AND returns
38+
# their ids, in one round trip.
39+
self.live_ids.difference_update(matched)
40+
return _FakeResult([{"id": i} for i in matched], deleted=len(matched))
41+
if "DETACH DELETE" in query:
42+
self.live_ids.difference_update(matched)
43+
return _FakeResult([], deleted=len(matched))
44+
if "RETURN n.id AS id" in query:
45+
return _FakeResult([{"id": i} for i in matched])
46+
return _FakeResult([])
47+
48+
def __enter__(self):
49+
return self
50+
51+
def __exit__(self, *exc):
52+
return False
53+
54+
55+
class _FakeResult:
56+
"""Minimal stand-in for a neo4j Result (iterable, plus .single()/.consume())."""
57+
58+
def __init__(self, records, deleted: int = 0):
59+
self._records = records
60+
self._deleted = deleted
61+
62+
def __iter__(self):
63+
return iter(self._records)
64+
65+
def single(self):
66+
return self._records[0] if self._records else None
67+
68+
def consume(self):
69+
return SimpleNamespace(counters=SimpleNamespace(nodes_deleted=self._deleted))
70+
71+
72+
def _base_config_kwargs() -> dict:
73+
return {
74+
"uri": "bolt://localhost:7687",
75+
"user": "neo4j",
76+
"password": "test",
77+
"db_name": "test_memory_db",
78+
"auto_create": False,
79+
"use_multi_db": False,
80+
"user_name": "alice",
81+
"embedding_dimension": 3,
82+
}
83+
84+
85+
@pytest.fixture
86+
def community_config():
87+
"""`neo4j-community` is the backend that owns a `vec_db` mirror."""
88+
return Neo4jCommunityGraphDBConfig(
89+
**_base_config_kwargs(),
90+
vec_config={
91+
"backend": "qdrant",
92+
"config": {
93+
"collection_name": "test_vectors",
94+
"path": "/tmp/memos_test_vectors",
95+
},
96+
},
97+
)
98+
99+
100+
@pytest.fixture
101+
def enterprise_config():
102+
"""`neo4j` has no `vec_db`; it searches the native vector index instead."""
103+
return Neo4jGraphDBConfig(**_base_config_kwargs())
104+
105+
106+
@pytest.fixture
107+
def community_db(community_config):
108+
"""Community backend with a faked driver and a faked vector store."""
109+
from memos.graph_dbs.neo4j_community import Neo4jCommunityGraphDB
110+
111+
live_ids = {"mem-1", "mem-2", "mem-3"}
112+
issued_queries: list[str] = []
113+
114+
with patch("neo4j.GraphDatabase") as mock_gd:
115+
mock_gd.driver.return_value = MagicMock()
116+
with patch("memos.graph_dbs.neo4j_community.VecDBFactory.from_config") as mock_factory:
117+
mock_factory.return_value = MagicMock()
118+
db = Neo4jCommunityGraphDB(community_config)
119+
120+
db.driver = MagicMock()
121+
db.driver.session.side_effect = lambda **kwargs: _FakeNeo4jSession(live_ids, issued_queries)
122+
123+
vector_ids = set(live_ids)
124+
db.vec_db = MagicMock()
125+
db.vec_db.delete.side_effect = lambda ids: [vector_ids.discard(i) for i in ids]
126+
127+
db.live_ids = live_ids
128+
db.vector_ids = vector_ids
129+
db.issued_queries = issued_queries
130+
return db
131+
132+
133+
class TestCommunityDeletePurgesVectors:
134+
"""Regression: the community backend deletes vectors together with nodes."""
135+
136+
def test_delete_by_memory_ids_purges_vectors(self, community_db):
137+
deleted = community_db.delete_node_by_prams(memory_ids=["mem-1", "mem-2"])
138+
139+
assert deleted == 2
140+
assert community_db.live_ids == {"mem-3"}
141+
assert community_db.vector_ids == {"mem-3"}, "vectors of deleted nodes must not survive"
142+
assert community_db.vec_db.delete.call_count == 1
143+
assert sorted(community_db.vec_db.delete.call_args[0][0]) == ["mem-1", "mem-2"]
144+
145+
def test_graph_deletion_and_return_value_unchanged(self, community_db):
146+
"""The fix must not change which nodes the graph store deletes."""
147+
deleted = community_db.delete_node_by_prams(memory_ids=["mem-3"])
148+
149+
assert deleted == 1
150+
assert community_db.live_ids == {"mem-1", "mem-2"}
151+
assert community_db.vector_ids == {"mem-1", "mem-2"}
152+
assert any("DETACH DELETE" in q for q in community_db.issued_queries)
153+
154+
def test_vec_db_failure_does_not_mask_successful_delete(self, community_db):
155+
"""A vector store outage must not turn a committed delete into a failure."""
156+
community_db.vec_db.delete.side_effect = RuntimeError("qdrant unreachable")
157+
158+
deleted = community_db.delete_node_by_prams(memory_ids=["mem-1"])
159+
160+
assert deleted == 1
161+
assert community_db.live_ids == {"mem-2", "mem-3"}
162+
163+
def test_delete_by_filter_purges_vectors(self, community_db):
164+
"""Deleting by metadata filter must purge vectors too."""
165+
community_db.get_by_metadata = MagicMock(return_value=["mem-1", "mem-3"])
166+
167+
deleted = community_db.delete_node_by_prams(filter={"user_id": "u_1"})
168+
169+
assert deleted == 2
170+
assert community_db.live_ids == {"mem-2"}
171+
assert community_db.vector_ids == {"mem-2"}
172+
assert sorted(community_db.vec_db.delete.call_args[0][0]) == ["mem-1", "mem-3"]
173+
174+
def test_no_match_does_not_touch_vec_db(self, community_db):
175+
"""An empty match set must not issue a vector delete call."""
176+
deleted = community_db.delete_node_by_prams(memory_ids=["does-not-exist"])
177+
178+
assert deleted == 0
179+
assert community_db.live_ids == {"mem-1", "mem-2", "mem-3"}
180+
assert community_db.vector_ids == {"mem-1", "mem-2", "mem-3"}
181+
community_db.vec_db.delete.assert_not_called()
182+
183+
def test_clear_still_purges_all_vectors(self, community_db):
184+
"""The neighbouring `clear()` path keeps working after the refactor."""
185+
community_db.vec_db.get_by_filter.return_value = [
186+
SimpleNamespace(id="mem-1"),
187+
SimpleNamespace(id="mem-2"),
188+
]
189+
190+
community_db.clear(user_name="alice")
191+
192+
community_db.vec_db.get_by_filter.assert_called_once_with({"user_name": "alice"})
193+
community_db.vec_db.delete.assert_called_once_with(["mem-1", "mem-2"])
194+
assert community_db.live_ids == set()
195+
assert any("DETACH DELETE" in q for q in community_db.issued_queries)
196+
197+
def test_collect_and_delete_are_one_statement(self, community_db):
198+
"""The ids and the deletion must come from a single statement.
199+
200+
Two separate auto-commit statements leave a window in which a concurrent
201+
writer can add nodes matching the same filter: those nodes are deleted
202+
without their ids ever being collected, so their vectors stay behind and
203+
the memory keeps surfacing in vector search.
204+
"""
205+
deleted = community_db.delete_node_by_prams(memory_ids=["mem-1", "mem-2"])
206+
207+
assert deleted == 2
208+
deletes = [q for q in community_db.issued_queries if "DETACH DELETE" in q]
209+
assert len(deletes) == 1, f"expected one delete statement, got {deletes}"
210+
assert "RETURN n.id AS id" in deletes[0], (
211+
"the delete statement must return the deleted ids, "
212+
"so no separate id-collection round trip is needed"
213+
)
214+
215+
216+
class TestBackendWithoutVecDB:
217+
"""`neo4j` sets up no `vec_db`, so the vector purge must degrade to a no-op."""
218+
219+
@pytest.fixture
220+
def enterprise_db(self, enterprise_config):
221+
from memos.graph_dbs.neo4j import Neo4jGraphDB
222+
223+
live_ids = {"mem-1", "mem-2", "mem-3"}
224+
issued_queries: list[str] = []
225+
226+
with patch("neo4j.GraphDatabase") as mock_gd:
227+
mock_gd.driver.return_value = MagicMock()
228+
db = Neo4jGraphDB(enterprise_config)
229+
230+
db.driver = MagicMock()
231+
db.driver.session.side_effect = lambda **kwargs: _FakeNeo4jSession(live_ids, issued_queries)
232+
db.live_ids = live_ids
233+
db.issued_queries = issued_queries
234+
return db
235+
236+
def test_delete_without_vec_db_succeeds(self, enterprise_db):
237+
deleted = enterprise_db.delete_node_by_prams(memory_ids=["mem-1", "mem-2"])
238+
239+
assert deleted == 2
240+
assert enterprise_db.live_ids == {"mem-3"}
241+
assert any("DETACH DELETE" in q for q in enterprise_db.issued_queries)
242+
243+
def test_no_match_without_vec_db(self, enterprise_db):
244+
deleted = enterprise_db.delete_node_by_prams(memory_ids=["does-not-exist"])
245+
246+
assert deleted == 0
247+
assert enterprise_db.live_ids == {"mem-1", "mem-2", "mem-3"}

0 commit comments

Comments
 (0)