diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fe03b48..fbf2fcad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,19 @@ PyPI version — not the changelog header. ### Fixed +- **Memory rollover no longer silently loses rolled-off learnings ("No embeddings + generated").** A capped `.trinity` file rolls its excess entries out to vectors; + two combined bugs dropped them on the floor instead. (1) On the "embedding returned + empty but success=True" path the orchestrator logged the error and continued — but + the source file was *already* trimmed, so the entry was lost from both the file and + ChromaDB; it now restores the pre-trim backup before continuing (fail-honest). + (2) A concurrent-rollover race (two runs ~33ms apart) let the second run extract + nothing yet still report success → empty embeddings → bug #1; `extract_with_metadata` + now honors the `skipped` flag and the orchestrator skips no-op extractions before the + embedding stage. Verified by artifact + live: a 25/25-capped test file rolls over → + embeds (384-dim) → `drone @memory search` returns it at 91% similarity; audit 100%, + 876 tests (+4). + - **Backup rich CLI output restored end-to-end (FPLAN-0263 + drone passthrough).** `drone @backup snapshot|versioned|all` rendered a flat text block instead of the original rich output. Two independent causes, both closed: (1) the rich rendering diff --git a/src/aipass/memory/apps/handlers/rollover/extractor.py b/src/aipass/memory/apps/handlers/rollover/extractor.py index eb17565e..65a85525 100644 --- a/src/aipass/memory/apps/handlers/rollover/extractor.py +++ b/src/aipass/memory/apps/handlers/rollover/extractor.py @@ -494,6 +494,17 @@ def extract_with_metadata(file_path: Path, percentage: int | None = None) -> Dic if not result["success"]: return result + if result.get("skipped"): + return { + "success": True, + "skipped": True, + "message": result.get("message", "Extraction skipped"), + "entries": [], + "count": 0, + "branch": result.get("branch"), + "type": result.get("type"), + } + # Enrich extracted items with metadata extracted = result.get("extracted", []) branch = result.get("branch") diff --git a/src/aipass/memory/apps/handlers/rollover/orchestrator.py b/src/aipass/memory/apps/handlers/rollover/orchestrator.py index 97e87df8..3cd310c8 100644 --- a/src/aipass/memory/apps/handlers/rollover/orchestrator.py +++ b/src/aipass/memory/apps/handlers/rollover/orchestrator.py @@ -341,6 +341,10 @@ def execute_rollover() -> Dict[str, Any]: failed.append({"trigger": str(trigger), "stage": "extraction", "error": error_msg}) continue + if extract_result.get("skipped"): + logger.info(f"[rollover] Extraction skipped for {trigger}: {extract_result.get('message', 'no excess')}") + continue + memories = extract_result.get("entries", []) branch = extract_result.get("branch", "") or trigger.branch memory_type = extract_result.get("type", "unknown") or trigger.memory_type @@ -375,6 +379,14 @@ def execute_rollover() -> Dict[str, Any]: embeddings = embed_result.get("embeddings", []) if not embeddings: logger.error(f"[rollover] No embeddings generated for {trigger}") + + # RESTORE from backup (file was already trimmed but data not vectorized) + restore_result = extractor.restore_from_backup(trigger.file_path) + if restore_result["success"]: + logger.info("[rollover] Restored from backup after empty embeddings") + else: + logger.error(f"[rollover] CRITICAL: Failed to restore from backup: {restore_result.get('error')}") + failed.append({"trigger": str(trigger), "stage": "embedding", "error": "No embeddings in result"}) continue diff --git a/src/aipass/memory/tests/test_orchestrator_exec.py b/src/aipass/memory/tests/test_orchestrator_exec.py index 478c8cd9..c6c08662 100644 --- a/src/aipass/memory/tests/test_orchestrator_exec.py +++ b/src/aipass/memory/tests/test_orchestrator_exec.py @@ -176,6 +176,67 @@ class TestExecuteRolloverExtraction: assert result["failed"][0]["error"] == "No branch in result" +class TestExecuteRolloverExtractionSkipped: + """Tests for skipped extraction (race condition / no excess entries).""" + + def test_skipped_extraction_skips_trigger(self, monkeypatch, tmp_path): + """Extraction returns skipped=True — trigger is skipped, not failed.""" + orch, mocks = _import_orchestrator(monkeypatch) + trigger = _make_trigger(tmp_path) + mocks["detector"].check_all_branches.return_value = { + "success": True, + "triggers": [trigger], + } + mocks["extractor"].create_rollover_backup.return_value = { + "success": True, + "message": "ok", + } + mocks["extractor"].extract_with_metadata.return_value = { + "success": True, + "skipped": True, + "message": "No entries exceed v2 limits", + "entries": [], + "count": 0, + } + + result = orch.execute_rollover() + assert result["success_count"] == 0 + assert len(result["failed"]) == 0 + + def test_skipped_extraction_no_embedding_attempted(self, monkeypatch, tmp_path): + """Skipped extraction does not call encode_batch_subprocess.""" + orch, mocks = _import_orchestrator(monkeypatch) + trigger = _make_trigger(tmp_path) + mocks["detector"].check_all_branches.return_value = { + "success": True, + "triggers": [trigger], + } + mocks["extractor"].create_rollover_backup.return_value = { + "success": True, + "message": "ok", + } + mocks["extractor"].extract_with_metadata.return_value = { + "success": True, + "skipped": True, + "message": "File under limit", + "entries": [], + "count": 0, + } + + embed_called = {"called": False} + original_encode = orch.encode_batch_subprocess + + def tracking_encode(texts): + """Wrap encode to track whether it was called.""" + embed_called["called"] = True + return original_encode(texts) + + monkeypatch.setattr(orch, "encode_batch_subprocess", tracking_encode) + + orch.execute_rollover() + assert embed_called["called"] is False + + class TestExecuteRolloverEmbedding: """Tests for the embedding phase.""" @@ -218,8 +279,8 @@ class TestExecuteRolloverEmbedding: assert result["failed"][0]["stage"] == "embedding" mocks["extractor"].restore_from_backup.assert_called_once() - def test_no_embeddings_returned(self, monkeypatch, tmp_path): - """Embed succeeds but returns empty embeddings list.""" + def test_no_embeddings_returned_restores_backup(self, monkeypatch, tmp_path): + """Embed succeeds but returns empty embeddings — must restore from backup.""" orch, mocks = _import_orchestrator(monkeypatch) self._setup_to_embedding(monkeypatch, tmp_path, mocks) @@ -233,6 +294,27 @@ class TestExecuteRolloverEmbedding: assert len(result["failed"]) == 1 assert result["failed"][0]["stage"] == "embedding" assert "No embeddings" in result["failed"][0]["error"] + mocks["extractor"].restore_from_backup.assert_called_once() + + def test_no_embeddings_restore_fails(self, monkeypatch, tmp_path): + """Empty embeddings + restore failure — CRITICAL data loss path.""" + orch, mocks = _import_orchestrator(monkeypatch) + self._setup_to_embedding(monkeypatch, tmp_path, mocks) + mocks["extractor"].restore_from_backup.return_value = { + "success": False, + "error": "backup file missing", + } + + monkeypatch.setattr( + orch, + "encode_batch_subprocess", + lambda texts: {"success": True, "embeddings": []}, + ) + + result = orch.execute_rollover() + assert len(result["failed"]) == 1 + assert result["failed"][0]["stage"] == "embedding" + mocks["extractor"].restore_from_backup.assert_called_once() class TestExecuteRolloverStorage: