diff --git a/semantica/context/agent_memory.py b/semantica/context/agent_memory.py index 03d144e3..85a7860f 100644 --- a/semantica/context/agent_memory.py +++ b/semantica/context/agent_memory.py @@ -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 diff --git a/semantica/context/erasure.py b/semantica/context/erasure.py index 366e777d..04f7fa72 100644 --- a/semantica/context/erasure.py +++ b/semantica/context/erasure.py @@ -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": }`` #: and Pinecone ``{"deleted": True}``; neither is a bool, so a bare diff --git a/tests/context/test_erasure_coordinator.py b/tests/context/test_erasure_coordinator.py index ea338288..e4dcb21a 100644 --- a/tests/context/test_erasure_coordinator.py +++ b/tests/context/test_erasure_coordinator.py @@ -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.