fix(memory): rollover no longer silently loses rolled-off learnings — restore pre-trim backup on empty-embeddings path + honor skipped-extraction flag to close the concurrent-rollover race (orchestrator.py + extractor.py, +4 tests). Verified by artifact (seedgo 100%, 876 tests) + live (drone @memory search returns a rolled-off item at 91%)
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user