diff --git a/mempalace/backends/chroma.py b/mempalace/backends/chroma.py index ee8f10b384..24d2ad6a48 100644 --- a/mempalace/backends/chroma.py +++ b/mempalace/backends/chroma.py @@ -1311,6 +1311,33 @@ def _close_client(client) -> None: logger.debug("client.close() unavailable or failed", exc_info=True) +def _clear_chroma_system_cache() -> None: + """Drop chromadb's process-global ``SharedSystemClient`` cache. + + chromadb caches its ``System`` (and the live HNSW segment) keyed by path. + A bare ``chromadb.PersistentClient(path=...)`` reopen reuses that cached + System, so after a peer/rebuild has changed ``chroma.sqlite3`` on disk we + would rebuild against the stale in-memory segment and persist an outdated + index over the on-disk changes -- the same data-loss class as #2002, + reached via :meth:`ChromaBackend._client` instead of + ``mcp_server._get_client``. This mirrors the reset already performed by + ``mcp_server._force_chroma_cache_reset`` and ``repair._close_chroma_handles``. + + The clear is process-global (it evicts every palace's cached System, not + just this path); chromadb exposes no per-path eviction. It only fires on the + inode/mtime-change branch of ``_client``, never the steady-state hot path, + so the redundant rebuild cost is bounded to genuine external-change reopens. + """ + try: + from chromadb.api.client import SharedSystemClient + + clear = getattr(SharedSystemClient, "clear_system_cache", None) + if callable(clear): + clear() + except Exception: + logger.debug("Failed to clear chromadb SharedSystemClient cache", exc_info=True) + + class ChromaCollection(BaseCollection): """Thin adapter translating ChromaDB dict returns into typed results. @@ -2054,6 +2081,14 @@ def _client(self, palace_path: str): or (mtime_appeared and palace_path in self._freshness) ): ChromaBackend._quarantined_paths.discard(palace_path) + # #2028: the same external change means chromadb's path-keyed + # System cache is now stale. Reconstructing PersistentClient + # below would reuse the cached System (and its in-memory HNSW + # segment), so drop the shared cache first -- otherwise the + # rebuilt client persists an outdated index over the on-disk + # change. Gated on genuine external change (not first open) so + # cold opens never pay the global-evict cost. + _clear_chroma_system_cache() ChromaBackend._prepare_palace_for_open(palace_path) cached = chromadb.PersistentClient(path=palace_path) self._clients[palace_path] = cached diff --git a/tests/test_backends.py b/tests/test_backends.py index 3168ec73dc..262d43d286 100644 --- a/tests/test_backends.py +++ b/tests/test_backends.py @@ -1827,6 +1827,67 @@ class DummyClient: ] +def test_chroma_backend_resets_system_cache_on_inode_change(tmp_path, monkeypatch): + """#2028: ``_client`` must drop chromadb's path-keyed ``SharedSystemClient`` + cache *before* reconstructing ``PersistentClient`` on an inode/mtime change. + + chromadb caches its ``System`` (and live HNSW segment) keyed by path, so a + bare reopen reuses the stale segment and persists an outdated index over a + peer/rebuild's on-disk changes -- the #2002 data-loss class reached via + ``_client`` instead of ``mcp_server._get_client``. The reset must fire only + on a genuine external change (not first open) and must precede the reopen. + """ + palace = tmp_path / "palace" + palace.mkdir() + (palace / "chroma.sqlite3").write_text("") + + events = [] + + # Neutralize the on-disk HNSW pre-checks so the test exercises only the + # cache-reset / client-rebuild ordering. + for _name in ( + "_fix_missing_collection_type", + "_fix_blob_seq_ids", + "quarantine_invalid_hnsw_metadata", + "quarantine_stale_hnsw", + ): + monkeypatch.setattr(f"mempalace.backends.chroma.{_name}", lambda path, *a, **k: []) + + monkeypatch.setattr(ChromaBackend, "_quarantined_paths", set()) + + class DummyClient: + pass + + def _record_open(path): + events.append(("open", path)) + return DummyClient() + + monkeypatch.setattr("mempalace.backends.chroma.chromadb.PersistentClient", _record_open) + + from chromadb.api.client import SharedSystemClient + + def _record_clear(*args, **kwargs): + events.append(("clear", None)) + + monkeypatch.setattr(SharedSystemClient, "clear_system_cache", _record_clear) + + backend = ChromaBackend() + # ``_db_stat`` is called twice per ``_client`` call (freshness check, then + # re-stat after reopen). Same inode on the first call (first open, no prior + # freshness -> no reset), changed inode on the second (external change). + stats = iter([(1, 1.0), (1, 1.0), (2, 2.0), (2, 2.0)]) + monkeypatch.setattr(backend, "_db_stat", lambda path: next(stats)) + + backend._client(str(palace)) # first open: no external change -> no clear + backend._client(str(palace)) # inode 1 -> 2: clear, then reopen + + assert events == [ + ("open", str(palace)), # first open, no cache reset + ("clear", None), # #2028: reset fires on the inode change... + ("open", str(palace)), # ...strictly before the PersistentClient reopen + ], events + + def test_explain_ef_mismatch_recognizes_chromadb_conflict(): """When ChromaDB rejects a collection read due to an EF-name mismatch (user changed MEMPALACE_EMBEDDING_MODEL on an existing palace), the