[simpleqt]

Fix #2331: purge embeddings when delete_node_by_prams removes memories

Fixes #2331.

Neo4jCommunityGraphDB.delete_node_by_prams() deletes the matching graph nodes
but never purges their embeddings from vec_db. Retrieval reads the vector store
first, so a deleted memory stays retrievable while the delete API still answers
success — the symptom in the report. The sibling paths already do this:
clear() and delete_node_by_mem_cube_id() both purge vec_db, and
delete_node_by_prams is the one that was missed.

delete_node_by_prams(memory_ids=['mem-1','mem-2'])

base:  returned deleted_count = 2
       graph store now        = ['mem-3']
       vec_db now             = ['mem-1', 'mem-2', 'mem-3']   <- stale
       vec_db.delete calls    = 0

fixed: returned deleted_count = 2
       graph store now        = ['mem-3']
       vec_db now             = ['mem-3']
       vec_db.delete calls    = 1

With a filter, the stale points also accumulate: they keep occupying the vector
top-k window, which is how a commenter saw search degrade to zero results after
roughly 520 orphans.

Change

Both implementations of delete_node_by_prams now collect the matching node ids
inside the same session, before the DELETE, and purge them afterwards through a
new _delete_vectors_for_deleted_nodes() helper on Neo4jGraphDB:

• Failure is logged, not raised. The graph deletion is already committed, so
  a vector-store outage must not turn a successful delete into "Failed to delete
  memories". This matches what clear() already does.
• The enterprise Neo4jGraphDB has no vec_db (vec_config lives only on
  the community config), so the helper resolves it with getattr(self, "vec_db", None) and degrades to a no-op instead of raising AttributeError.
• Return value unchanged. The community path now returns the number of ids it
  collected rather than running a separate count(n) query — the same number,
  one query fewer.
• The count query is replaced by the id-collecting query, so this does not add a
  round trip.

Testing

tests/graph_dbs/test_delete_vec_cleanup.py (new, no Neo4j, Qdrant or network —
the driver and the vector store are fakes):

• with the test but the source at the base revision: 3 failed, 5 passed —
  assert {'mem-1','mem-2','mem-3'} == {'mem-2'} in the filter path, and the same
  shape for the memory-id path and the return-value assertion
• with the fix: 8 passed, and tests/graph_dbs → 39 passed, 3 skipped
• the 5 tests that pass on both are the graceful-degradation guards (no vec_db,
  nothing matched, vector-store failure) which legitimately hold before and after
• tests/memories tests/api tests/mem_reader → 256 passed, 5 failed, and the same
  5 fail on the untouched base tree (Ollama/environment), so they are pre-existing
• ruff at the pinned 0.11.8 passes check and format --check on all three
  files; repo-wide format --check reports 631 files already formatted

Callers checked

Every caller of the changed method was grepped and checked for a changed
contract: memories/textual/tree.py:415,435 (ignore the return value),
api/handlers/memory_handler.py:441-462 (the delete API path — returns success
or failure purely on exception, which is exactly what the fix makes truthful),
and mem_reader/read_skill_memory/process_skill_memory.py:1184 (deletes a
replaced memory, now also purged). vec_db.delete accepts an unbounded id list
(vec_dbs/qdrant.py:347, and the base class takes the same list signature), which
the new caller relies on.

Two other methods share this gap — the inherited Neo4jGraphDB.delete_node() and
remove_oldest_memory(), used by L3 maintenance and eviction. They are outside
the delete-memory API path this issue reports, so they are deliberately not
touched here; happy to file that separately if you want it.

AI assistance: this change was produced with AI assistance (an autonomous coding
agent) from the linked issue; the analysis, the reproduction, the before/after
test runs and the lint runs were executed locally, and a human reviewed the
result before submission.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

[Memtensor-AI]

🤖 Open Code Review

Target: PR #2533
Task: db7368d49c15b3ed
Base: main
Head: fix/delete-purges-vectors

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

⚠️ 1 warning(s) occurred during review.

---

1. src/memos/graph_dbs/neo4j.py (L2027-L2030)

TOCTOU race: the ID-collection query and the delete query run as two independent auto-commit statements. Between them, a concurrent writer can insert new nodes that match the same WHERE clause — those nodes would then be deleted without their IDs being captured, leaving stale vectors in the store. Conversely, another thread could delete matching nodes between the two statements, causing deleted_ids to contain IDs that are already gone when vec_db.delete() is called (a harmless but wasteful call, only if vec_db.delete is idempotent).

The reliable fix is to run both operations in a single Cypher statement that collects the IDs and deletes atomically:

MATCH (n:Memory) WHERE {final_where}
WITH n, n.id AS id
DETACH DELETE n
RETURN id

This keeps both actions inside one auto-commit transaction, eliminating the window entirely.

💡 Suggested Change

Before:

                deleted_ids = [record["id"] for record in session.run(id_collect_query, **params)]

                # Execute delete query
                result = session.run(delete_query, **params)

After:

                # Collect IDs and delete in one atomic statement to avoid TOCTOU.
                collect_and_delete_query = (
                    f"MATCH (n:Memory) WHERE {final_where} "
                    "WITH n, n.id AS id DETACH DELETE n RETURN id"
                )
                result = session.run(collect_and_delete_query, **params)
                records = result.data()
                deleted_ids = [r["id"] for r in records]
                summary = result.consume()
                deleted_count = summary.counters.nodes_deleted if summary.counters else 0

---

2. src/memos/graph_dbs/neo4j_community.py (L1043-L1045)

Debug print() statement introduced by this PR should be removed before merging. The same information is already emitted by the logger.info call on the preceding line. print() to stdout is not appropriate in production library code and bypasses the logging configuration.

💡 Suggested Change

Before:

        id_collect_query = f"MATCH (n:Memory) WHERE {ids_where} RETURN n.id AS id"
        logger.info(f"[delete_node_by_prams] id_collect_query: {id_collect_query}")
        print(f"[delete_node_by_prams] id_collect_query: {id_collect_query}")

After:

        id_collect_query = f"MATCH (n:Memory) WHERE {ids_where} RETURN n.id AS id"
        logger.info(f"[delete_node_by_prams] id_collect_query: {id_collect_query}")

---

3. src/memos/graph_dbs/neo4j_community.py (L1058-L1063)

Same TOCTOU race as in the base class: the collect query and the delete query are two separate auto-commit statements. Nodes matching the filter could be inserted or deleted between the two round-trips, making deleted_ids inconsistent with what is actually removed. Combine both into one Cypher statement (WITH n, n.id AS id DETACH DELETE n RETURN id) for atomicity, as suggested for neo4j.py.

💡 Suggested Change

Before:

                deleted_ids = [record["id"] for record in session.run(id_collect_query, **params)]
                # Use the count from before deletion as the actual deleted count
                deleted_count = len(deleted_ids)

                # Delete nodes
                session.run(delete_query, **params)

After:

                # Collect IDs and delete in one atomic statement to avoid TOCTOU.
                collect_and_delete_query = (
                    f"MATCH (n:Memory) WHERE {ids_where} "
                    "WITH n, n.id AS id DETACH DELETE n RETURN id"
                )
                records = [record["id"] for record in session.run(collect_and_delete_query, **params)]
                deleted_ids = records
                deleted_count = len(deleted_ids)

---

4. src/memos/graph_dbs/neo4j_community.py (L1062-L1063)

The result of session.run(delete_query, **params) is never consumed (no .consume() or iteration). In the Neo4j Python driver, the result cursor is lazy: the server-side work is only guaranteed to complete once the result is fully consumed. Leaving it unconsumed means the DETACH DELETE may not have finished by the time the session closes, and any server-side error raised during execution will be silently swallowed rather than surfaced. Call session.run(...).consume() to ensure the deletion is committed and any error is propagated, consistent with the base-class implementation.

💡 Suggested Change

Before:

                # Delete nodes
                session.run(delete_query, **params)

After:

                # Delete nodes
                session.run(delete_query, **params).consume()

---

5. src/memos/graph_dbs/neo4j_community.py (L1050-L1051)

Two additional debug print() statements remain at lines 1050–1051 (for delete_query and params). These bypass the logging framework, will expose potentially sensitive filter/parameter data to raw stdout, and are inappropriate for production library code. Remove them (the corresponding logger.info call already records the delete query).

Generated by cloud-assistant via Open Code Review.