Tighten issue 59 review follow-ups

Author: Atharva-Kanherkar

api/app.py

@@ -2,6 +2,7 @@
 
 import asyncio
 import json
+import logging
 import mimetypes
 import os
 import tempfile
@@ -34,6 +35,7 @@
 DEFAULT_MEDIA_DIR = Path(os.getenv("MEMORY_MEDIA_DIR", config.MEDIA_STORAGE_PATH))
 _SUPPORTED_FILE_MODALITIES = {"audio", "image", "video", "multimodal"}
 _ALLOWED_MEDIA_TYPES = {"image", "audio", "video", "pdf"}
+logger = logging.getLogger(__name__)
 
 
 def _normalise_origins(origins: list[str] | None = None) -> list[str]:
@@ -344,6 +346,7 @@ def _safe_contradiction_lookup(
     except ValueError as exc:
         return ContradictionCheckResult(status="skipped", candidates=[], detail=str(exc))
     except Exception as exc:
+        logger.exception("Contradiction lookup failed for semantic record %s", record.id)
         return ContradictionCheckResult(status="error", candidates=[], detail=str(exc))
 
 
@@ -418,6 +421,9 @@ def service() -> MemoryAPIService:
 
     async def run_forgetting_cycle(*, dry_run: bool) -> dict[str, Any]:
         lock = app.state.forgetting_lock
+        # This is intentionally single-flight per app process. Multi-worker
+        # deployments still need an external coordinator if cross-process
+        # forgetting exclusivity becomes a requirement.
         if lock.locked():
             return {
                 "status": "already_running",

stores/semantic_store.py

@@ -112,6 +112,11 @@ def retrieve_by_vector(
     ) -> list[tuple[SemanticMemory, float]]:
         n_results = max(1, top_k)
         if not include_superseded:
+            # We need enough raw candidates to still return `top_k` active
+            # records after filtering superseded ones, and Chroma cannot apply
+            # that predicate natively for us. This makes default semantic recall
+            # O(N) in collection size, which is acceptable for the current
+            # scale but should be revisited if the store grows substantially.
             n_results = max(1, self._collection.count())
         result = self._collection.query(
             query_embeddings=[vector],

tests/test_forgetting_service.py

@@ -73,6 +73,24 @@ def replace(self, record) -> None:
         self._records[record.id] = record
 
 
+class TrackingSemanticStore(FakeStore):
+    def __init__(self, records=None):
+        super().__init__(records)
+        self.last_include_superseded = None
+
+    def get_all_records(
+        self,
+        include_embeddings: bool = False,
+        *,
+        include_superseded: bool = False,
+    ):
+        self.last_include_superseded = include_superseded
+        return super().get_all_records(
+            include_embeddings=include_embeddings,
+            include_superseded=include_superseded,
+        )
+
+
 class EventRecorder:
     def __init__(self, bus: EventBus, *event_types: str):
         self.events = []
@@ -114,6 +132,31 @@ def _ids_for(report):
     return {decision.record_id: decision for decision in report.decisions}
 
 
+def test_scan_records_includes_superseded_semantic_rows():
+    superseded = SemanticMemory(
+        id="semantic-old",
+        content="Old fact",
+        superseded_by="semantic-new",
+    )
+    active = SemanticMemory(
+        id="semantic-new",
+        content="New fact",
+    )
+    semantic_store = TrackingSemanticStore([superseded, active])
+    service = ForgettingService(
+        semantic_store=semantic_store,
+        episodic_store=FakeStore(),
+        procedural_store=FakeStore(),
+        media_store=MediaStore(tempfile.mkdtemp(prefix="forgetting_media_scan_")),
+        contradiction_detector=_NoopDuplicateDetector(),
+    )
+
+    records = service._scan_records()
+
+    assert semantic_store.last_include_superseded is True
+    assert {"semantic-old", "semantic-new"} <= set(records)
+
+
 def test_dry_run_resolves_duplicate_clusters_component_wise(monkeypatch):
     now = datetime(2026, 3, 1, 12, 0, tzinfo=timezone.utc)
     first = SemanticMemory(