mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-09-13 04:04:09 +00:00
fix(context): vector_store=False must suppress AgentMemory's internal vector cascade in ErasureCoordinator (#1395)
* fix(erasure): ensure vector_store=False disables internal vector cascade in AgentMemory * fix(erasure): ensure skip_vector=True does not orphan local vector ID tracking
This commit is contained in:
@@ -588,16 +588,15 @@ class AgentMemory:
|
||||
return False
|
||||
|
||||
# Remove from vector store unless a caller is staging an atomic local update.
|
||||
if not skip_vector:
|
||||
if self.vector_store:
|
||||
try:
|
||||
vector_ids = list(self._vector_ids.get(memory_id, [])) or [
|
||||
memory_id
|
||||
]
|
||||
self._delete_vector_ids(vector_ids)
|
||||
except Exception as e:
|
||||
self.logger.warning(f"Failed to delete from vector store: {e}")
|
||||
self._vector_ids.pop(memory_id, None)
|
||||
if not skip_vector and self.vector_store:
|
||||
try:
|
||||
vector_ids = list(self._vector_ids.get(memory_id, [])) or [memory_id]
|
||||
self._delete_vector_ids(vector_ids)
|
||||
except Exception as e:
|
||||
self.logger.warning(f"Failed to delete from vector store: {e}")
|
||||
# Bookkeeping runs unconditionally: a skip_vector delete still removes the
|
||||
# item, so leaving its tracked ids behind would orphan them permanently.
|
||||
self._vector_ids.pop(memory_id, None)
|
||||
|
||||
memory_item = self.memory_items[memory_id]
|
||||
|
||||
@@ -1588,12 +1587,16 @@ class AgentMemory:
|
||||
memory_ids.append(memory_id)
|
||||
return memory_ids
|
||||
|
||||
def batch_delete(self, memory_ids: List[str]) -> int:
|
||||
def batch_delete(self, memory_ids: List[str], *, skip_vector: bool = False) -> int:
|
||||
"""
|
||||
Batch delete.
|
||||
|
||||
Args:
|
||||
memory_ids: List of memory IDs to delete
|
||||
skip_vector: If True, skip each item's own vector-store cascade
|
||||
(see ``delete_memory``). A caller that is already erasing these
|
||||
ids' vectors itself passes this to avoid a redundant,
|
||||
best-effort delete against the vector store.
|
||||
|
||||
Returns:
|
||||
Number of memories deleted
|
||||
@@ -1603,7 +1606,7 @@ class AgentMemory:
|
||||
"""
|
||||
deleted = 0
|
||||
for memory_id in memory_ids:
|
||||
if self.delete_memory(memory_id):
|
||||
if self.delete_memory(memory_id, skip_vector=skip_vector):
|
||||
deleted += 1
|
||||
return deleted
|
||||
|
||||
|
||||
@@ -34,6 +34,7 @@ Example:
|
||||
'unsupported'
|
||||
"""
|
||||
|
||||
import inspect
|
||||
from dataclasses import dataclass, field
|
||||
from datetime import datetime, timezone
|
||||
from typing import Any, Dict, Iterable, List, Optional, Sequence, Tuple, Union
|
||||
@@ -150,6 +151,24 @@ class ErasureCoordinator:
|
||||
more than actually occurred. Erasing the graph last means a partial
|
||||
failure leaves the node present and the receipt incomplete, which is
|
||||
recoverable and honest.
|
||||
|
||||
Note:
|
||||
An explicit ``vector_store=False`` also suppresses ``AgentMemory``'s
|
||||
own internal vector cascade, not just the coordinator's leg (#1378).
|
||||
``AgentMemory.delete_memory()`` deletes an item's vectors best-effort:
|
||||
it catches a vector-store failure, logs it, and still returns ``True``,
|
||||
so without this a caller who opted out of the vector leg could still
|
||||
have ``memory.vector_store`` mutated underneath them while the receipt
|
||||
read ``vectors: not_configured``. ``vector_store=False`` is taken to
|
||||
mean "no vector activity at all", so the coordinator passes
|
||||
``skip_vector=True`` through to ``memory.batch_delete()`` in that case,
|
||||
and ``receipt.stores["vectors"]["status"]`` stays ``"not_configured"``
|
||||
honestly -- the caller opted the vector store out entirely, rather than
|
||||
the coordinator having erased it. This only applies when
|
||||
``vector_store=False`` was passed explicitly; when no vector store
|
||||
exists anywhere (no ``memory`` was supplied, or ``memory`` has no
|
||||
``vector_store`` attribute), there is nothing to suppress and
|
||||
``memory.batch_delete()`` is called as before.
|
||||
"""
|
||||
|
||||
def __init__(
|
||||
@@ -170,6 +189,12 @@ class ErasureCoordinator:
|
||||
|
||||
self.graph = graph
|
||||
self.memory = memory
|
||||
# Distinct from `self.vector_store is None`: that's also true when no
|
||||
# vector store exists anywhere (no memory, or memory with no
|
||||
# vector_store attribute), where there is nothing to suppress and
|
||||
# forcing skip_vector onto a duck-typed memory would break callers
|
||||
# whose batch_delete() doesn't accept that kwarg.
|
||||
self._vector_leg_disabled = vector_store is False
|
||||
if vector_store is False:
|
||||
self.vector_store: Optional[Any] = None
|
||||
elif vector_store is not None:
|
||||
@@ -424,6 +449,20 @@ class ErasureCoordinator:
|
||||
return {"status": STATUS_NOT_CONFIGURED}
|
||||
|
||||
deleted = 0
|
||||
skip_vector = self._vector_leg_disabled and _accepts_skip_vector(
|
||||
self.memory.batch_delete
|
||||
)
|
||||
if self._vector_leg_disabled and not skip_vector:
|
||||
# The class docstring only requires find_by_entity/batch_delete; a
|
||||
# duck-typed adapter is not required to support skip_vector. Falling
|
||||
# back to the plain call keeps the memory leg working -- the
|
||||
# adapter's own cascade (if it has one) just can't be suppressed.
|
||||
self.logger.warning(
|
||||
"Memory adapter %r has no skip_vector support; its own vector "
|
||||
"cascade (if any) could not be suppressed for %r",
|
||||
type(self.memory).__name__,
|
||||
entity_id,
|
||||
)
|
||||
try:
|
||||
# Sweep in pages until dry rather than passing one large limit:
|
||||
# ``find_by_entity`` has historically defaulted to ``limit=10`` and
|
||||
@@ -454,7 +493,10 @@ class ErasureCoordinator:
|
||||
"detail": "memory items carry no 'memory_id'",
|
||||
}
|
||||
|
||||
removed = self.memory.batch_delete(memory_ids)
|
||||
if skip_vector:
|
||||
removed = self.memory.batch_delete(memory_ids, skip_vector=True)
|
||||
else:
|
||||
removed = self.memory.batch_delete(memory_ids)
|
||||
deleted += removed
|
||||
if removed == 0:
|
||||
# No progress: another page would return the same items.
|
||||
@@ -564,6 +606,25 @@ def _memory_item_id(item: Any) -> Optional[str]:
|
||||
return str(memory_id) if memory_id else None
|
||||
|
||||
|
||||
def _accepts_skip_vector(batch_delete: Any) -> bool:
|
||||
"""True when ``batch_delete`` takes a ``skip_vector`` keyword.
|
||||
|
||||
``skip_vector`` is an ``AgentMemory``-specific extension, not part of the
|
||||
duck-typed contract the class docstring promises (``find_by_entity`` and
|
||||
``batch_delete`` only). Passing it to an adapter that doesn't accept it
|
||||
would raise ``TypeError`` and fail the whole memory leg, so this is
|
||||
checked before ever passing the kwarg.
|
||||
"""
|
||||
try:
|
||||
signature = inspect.signature(batch_delete)
|
||||
except (TypeError, ValueError):
|
||||
return False
|
||||
for parameter in signature.parameters.values():
|
||||
if parameter.name == "skip_vector" or parameter.kind == inspect.Parameter.VAR_KEYWORD:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
#: Dict keys a backend uses to report whether a delete succeeded, and the
|
||||
#: values that mean it did not. Qdrant returns ``{"status": <UpdateStatus>}``
|
||||
#: and Pinecone ``{"deleted": True}``; neither is a bool, so a bare
|
||||
|
||||
@@ -678,7 +678,7 @@ class TestSeparateVectorStoreHandling(unittest.TestCase):
|
||||
"""
|
||||
|
||||
def test_vector_store_false_disables_vector_leg_entirely(self):
|
||||
"""vector_store=False must disable the vector leg, not try memory.vector_store."""
|
||||
"""vector_store=False must disable the vector leg AND memory's own cascade (#1378)."""
|
||||
memory_store = _SelectiveDeleteStore()
|
||||
memory = _memory_with_embedding("customer-4471", memory_store)
|
||||
|
||||
@@ -689,8 +689,91 @@ class TestSeparateVectorStoreHandling(unittest.TestCase):
|
||||
|
||||
# Vector leg should report not_configured, not attempt deletion
|
||||
self.assertEqual(receipt.stores["vectors"]["status"], STATUS_NOT_CONFIGURED)
|
||||
# Memory's own cascade still runs, but coordinator doesn't track it
|
||||
self.assertTrue(receipt.complete)
|
||||
# Memory's own internal vector cascade must be suppressed too, not just
|
||||
# unreported: the embedding memory owns is left untouched, and the
|
||||
# backend's delete method is never even called.
|
||||
self.assertEqual(memory_store.attempts, [])
|
||||
self.assertTrue(memory_store.live)
|
||||
|
||||
def test_vector_store_false_regression_refusing_backend_never_called(self):
|
||||
"""Regression for #1378: a refusing backend must not be called at all.
|
||||
|
||||
Reproduces the exact bug report -- a vector store whose delete_vectors()
|
||||
always returns False (refuses) bound as memory.vector_store, with the
|
||||
coordinator's own vector leg disabled via vector_store=False. Before the
|
||||
fix, delete_memory()'s internal cascade would still call the refusing
|
||||
store, catch the failure, log a warning, and return True regardless --
|
||||
so receipt.complete read True while the embedding stayed live and the
|
||||
backend had in fact been asked to delete it. Pinned here so the delete
|
||||
method call count can't silently regress back to nonzero.
|
||||
"""
|
||||
refusing_store = _SelectiveDeleteStore(refuse={"vec-0"})
|
||||
memory = _memory_with_embedding("customer-4471", refusing_store)
|
||||
|
||||
receipt = ErasureCoordinator(
|
||||
memory=memory, vector_store=False
|
||||
).erase_entity("customer-4471")
|
||||
|
||||
self.assertTrue(receipt.complete)
|
||||
self.assertEqual(receipt.stores["vectors"]["status"], STATUS_NOT_CONFIGURED)
|
||||
self.assertEqual(len(refusing_store.attempts), 0) # delete_calls == 0
|
||||
|
||||
def test_skip_vector_deletion_does_not_orphan_local_vector_id_tracking(self):
|
||||
"""skip_vector=True must still pop the item's own _vector_ids entry.
|
||||
|
||||
Regression: delete_memory(skip_vector=True) used to leave the item's
|
||||
entry in AgentMemory._vector_ids behind since the pop() lived inside
|
||||
the `if not skip_vector` block alongside the actual vector-store
|
||||
delete. That orphaned entry never got cleaned up and leaked into
|
||||
to_dict()/from_dict() snapshots.
|
||||
"""
|
||||
memory = _memory_with_embedding("customer-4471", _SelectiveDeleteStore())
|
||||
memory_id = next(iter(memory.memory_items))
|
||||
self.assertIn(memory_id, memory._vector_ids)
|
||||
|
||||
ErasureCoordinator(memory=memory, vector_store=False).erase_entity(
|
||||
"customer-4471"
|
||||
)
|
||||
|
||||
self.assertNotIn(memory_id, memory.memory_items)
|
||||
self.assertNotIn(memory_id, memory._vector_ids)
|
||||
|
||||
def test_memory_adapter_without_skip_vector_support_is_not_broken(self):
|
||||
"""A duck-typed memory whose batch_delete() lacks skip_vector must still work.
|
||||
|
||||
The class docstring only requires find_by_entity and batch_delete; an
|
||||
adapter is not obligated to support skip_vector. The coordinator must
|
||||
detect that and fall back to the plain call rather than raising
|
||||
TypeError and failing the whole memory leg.
|
||||
"""
|
||||
|
||||
class _PlainAdapter:
|
||||
def __init__(self):
|
||||
self.items = {"m1": {"memory_id": "m1", "entities": [{"id": "customer-4471"}]}}
|
||||
|
||||
def find_by_entity(self, entity_id, limit=None):
|
||||
return [
|
||||
item
|
||||
for item in self.items.values()
|
||||
if any(e.get("id") == entity_id for e in item.get("entities", []))
|
||||
]
|
||||
|
||||
def batch_delete(self, memory_ids):
|
||||
removed = 0
|
||||
for memory_id in memory_ids:
|
||||
if self.items.pop(memory_id, None) is not None:
|
||||
removed += 1
|
||||
return removed
|
||||
|
||||
adapter = _PlainAdapter()
|
||||
|
||||
receipt = ErasureCoordinator(
|
||||
memory=adapter, vector_store=False
|
||||
).erase_entity("customer-4471")
|
||||
|
||||
self.assertEqual(receipt.stores["memory"]["status"], STATUS_ERASED)
|
||||
self.assertEqual(adapter.items, {})
|
||||
|
||||
def test_separate_vector_store_only_handles_coordinator_store(self):
|
||||
"""When coordinator has a different vector_store, it only handles that one.
|
||||
|
||||
Reference in New Issue
Block a user