diff --git a/mempalace/backends/chroma.py b/mempalace/backends/chroma.py index fe36f34257..499d000dea 100644 --- a/mempalace/backends/chroma.py +++ b/mempalace/backends/chroma.py @@ -873,10 +873,30 @@ def _write_lock(self): # Writes # ------------------------------------------------------------------ + @staticmethod + def _sanitize_metadatas_for_chromadb(metadatas): + """chromadb 1.5.x rejects None and empty-dict entries in the metadatas + list (ValueError: Expected metadata to be a non-empty dict, got 0 + metadata attributes in add). Coerce any such entry to a sentinel so + the write succeeds. Operators can later locate coerced drawers via + ``where={"_repaired_empty_meta": True}``. + + This is the chokepoint catch-all: even if a caller's own sanitizer + misses a case (or skips for performance), reaching the chromadb + client always goes through here first. + """ + if metadatas is None: + return None + return [ + m if (isinstance(m, dict) and len(m) > 0) else {"_repaired_empty_meta": True} + for m in metadatas + ] + def add(self, *, documents, ids, metadatas=None, embeddings=None): kwargs: dict[str, Any] = {"documents": documents, "ids": ids} - if metadatas is not None: - kwargs["metadatas"] = metadatas + sanitized = self._sanitize_metadatas_for_chromadb(metadatas) + if sanitized is not None: + kwargs["metadatas"] = sanitized if embeddings is not None: kwargs["embeddings"] = embeddings with self._write_lock(): @@ -884,8 +904,9 @@ def add(self, *, documents, ids, metadatas=None, embeddings=None): def upsert(self, *, documents, ids, metadatas=None, embeddings=None): kwargs: dict[str, Any] = {"documents": documents, "ids": ids} - if metadatas is not None: - kwargs["metadatas"] = metadatas + sanitized = self._sanitize_metadatas_for_chromadb(metadatas) + if sanitized is not None: + kwargs["metadatas"] = sanitized if embeddings is not None: kwargs["embeddings"] = embeddings with self._write_lock(): diff --git a/mempalace/repair.py b/mempalace/repair.py index dd4c46ac65..7a90dbca59 100644 --- a/mempalace/repair.py +++ b/mempalace/repair.py @@ -139,7 +139,17 @@ def _extract_drawers(col, total: int, batch_size: int): break all_ids.extend(batch["ids"]) all_docs.extend(batch["documents"]) - all_metas.extend(batch["metadatas"]) + # chromadb 1.5.x's upsert validates that every metadatas[i] is a + # non-empty dict (chromadb/api/types.py:validate_metadata). Drawers + # extracted from sqlite ground truth can come back with None or {} + # for sparse historical writes — coerce those to a sentinel so the + # rebuild upsert can complete instead of raising ValueError ~80% + # through a multi-hour run. See #1458 for full context. + sanitized_metas = [ + m if (isinstance(m, dict) and len(m) > 0) else {"_repaired_empty_meta": True} + for m in batch["metadatas"] + ] + all_metas.extend(sanitized_metas) offset += len(batch["ids"]) return all_ids, all_docs, all_metas @@ -806,11 +816,14 @@ def _flush() -> int: for emb_id, doc, meta in extract_via_sqlite(source_palace, collection_name): ids.append(emb_id) docs.append(doc or "") - # chromadb 1.5.x rejects None entries in the metadatas list - # but accepts empty dicts. Mempalace drawers always carry at - # least wing/room, so this branch is defensive — corruption - # in embedding_metadata could yield an emb_id with no rows. - metas.append(meta if meta else {}) + # chromadb 1.5.x rejects both None and empty-dict entries in + # the metadatas list (ValueError: Expected metadata to be a + # non-empty dict). Mempalace drawers always carry at least + # wing/room, so this branch is defensive — corruption in + # embedding_metadata could yield an emb_id with no rows. + # Coerce to a sentinel that satisfies validation and is + # discoverable later via `where={"_repaired_empty_meta": True}`. + metas.append(meta if (meta and len(meta) > 0) else {"_repaired_empty_meta": True}) if len(ids) >= batch_size: _flush() _flush() diff --git a/tests/test_repair.py b/tests/test_repair.py index 9507c5dc53..144d90735e 100644 --- a/tests/test_repair.py +++ b/tests/test_repair.py @@ -76,6 +76,94 @@ def test_paginate_ids_offset_exception_fallback(): assert "id1" in ids +# ── _extract_drawers ────────────────────────────────────────────────── + + +def test_extract_drawers_preserves_valid_metadata(): + """Non-empty dict metadata passes through unchanged.""" + col = MagicMock() + col.get.return_value = { + "ids": ["id1", "id2"], + "documents": ["doc1", "doc2"], + "metadatas": [{"wing": "a", "room": "1"}, {"wing": "b", "room": "2"}], + } + all_ids, all_docs, all_metas = repair._extract_drawers(col, total=2, batch_size=2) + assert all_ids == ["id1", "id2"] + assert all_docs == ["doc1", "doc2"] + assert all_metas == [{"wing": "a", "room": "1"}, {"wing": "b", "room": "2"}] + + +def test_extract_drawers_sanitizes_none_metadata(): + """None entries in metadatas are coerced to the sentinel dict. + + chromadb 1.5.x's `validate_metadata` raises `ValueError: Expected metadata + to be a non-empty dict, got 0 metadata attributes in add.` if it sees a + None entry; the sanitizer keeps the rebuild upsert from crashing. + """ + col = MagicMock() + col.get.return_value = { + "ids": ["id1", "id2", "id3"], + "documents": ["doc1", "doc2", "doc3"], + "metadatas": [{"wing": "a"}, None, {"wing": "c"}], + } + _, _, all_metas = repair._extract_drawers(col, total=3, batch_size=3) + assert all_metas[0] == {"wing": "a"} + assert all_metas[1] == {"_repaired_empty_meta": True} + assert all_metas[2] == {"wing": "c"} + + +def test_extract_drawers_sanitizes_empty_dict_metadata(): + """Empty dict {} entries are coerced to the sentinel dict. + + chromadb 1.5.x rejects `{}` the same way it rejects `None`. The comment + in the previous code path mistakenly assumed otherwise. + """ + col = MagicMock() + col.get.return_value = { + "ids": ["id1", "id2"], + "documents": ["doc1", "doc2"], + "metadatas": [{}, {"wing": "b"}], + } + _, _, all_metas = repair._extract_drawers(col, total=2, batch_size=2) + assert all_metas[0] == {"_repaired_empty_meta": True} + assert all_metas[1] == {"wing": "b"} + + +def test_extract_drawers_sanitization_preserves_alignment(): + """Sanitized output keeps the same length and ordering as input. + + Critical invariant: ids[i] / documents[i] / metadatas[i] must stay in + lockstep through the sanitizer; otherwise the rebuild upsert mis-pairs + documents with metadata. + """ + col = MagicMock() + col.get.return_value = { + "ids": ["id1", "id2", "id3", "id4"], + "documents": ["d1", "d2", "d3", "d4"], + "metadatas": [None, {"k": "v"}, {}, None], + } + all_ids, all_docs, all_metas = repair._extract_drawers(col, total=4, batch_size=4) + assert len(all_ids) == len(all_docs) == len(all_metas) == 4 + assert all_ids == ["id1", "id2", "id3", "id4"] + assert all_metas[0] == {"_repaired_empty_meta": True} + assert all_metas[1] == {"k": "v"} + assert all_metas[2] == {"_repaired_empty_meta": True} + assert all_metas[3] == {"_repaired_empty_meta": True} + + +def test_extract_drawers_multiple_batches(): + """Pagination handles batch boundaries without losing/duplicating rows.""" + col = MagicMock() + col.get.side_effect = [ + {"ids": ["id1", "id2"], "documents": ["d1", "d2"], "metadatas": [{"a": 1}, None]}, + {"ids": ["id3"], "documents": ["d3"], "metadatas": [{}]}, + {"ids": [], "documents": [], "metadatas": []}, + ] + all_ids, all_docs, all_metas = repair._extract_drawers(col, total=3, batch_size=2) + assert all_ids == ["id1", "id2", "id3"] + assert all_metas == [{"a": 1}, {"_repaired_empty_meta": True}, {"_repaired_empty_meta": True}] + + # ── scan_palace ───────────────────────────────────────────────────────