From 45df1a265748200f709a1d737bfb8003061cbb32 Mon Sep 17 00:00:00 2001 From: mvalentsev Date: Wed, 22 Apr 2026 15:59:43 +0500 Subject: [PATCH] fix(backends/chroma): release SQLite file lock on close_palace/close (#1067) ChromaBackend.close_palace() and close() evicted cached PersistentClients from self._clients without calling client.close(), so chromadb 1.5.x kept the rust-side SQLite file lock until GC. Reopening the same palace path after shutil.rmtree + re-create within one process then failed with SQLITE_READONLY_DBMOVED (SQLite code 1032). Add _close_client() helper with a try/except fallback for older chromadb, and route close_palace(), close(), and the DB-file-missing invalidation branch of _client() through it. The mtime/inode auto-invalidation branch is left as-is: callers there may still hold a live ChromaCollection handle, and closing out from under them clears the rust bindings mid-use. Regression tests cover close_palace reopen-same-path and whole-backend close for multiple palaces. --- mempalace/backends/chroma.py | 28 ++++++++++++++++++--- tests/test_backends.py | 47 ++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/mempalace/backends/chroma.py b/mempalace/backends/chroma.py index d9b99a4..e7c2e6f 100644 --- a/mempalace/backends/chroma.py +++ b/mempalace/backends/chroma.py @@ -676,6 +676,20 @@ def _as_list(v: Any) -> list: return [v] +def _close_client(client) -> None: + """Call ``PersistentClient.close()`` if available, swallow otherwise. + + chromadb 1.5.x exposes ``Client.close()`` to release rust-side SQLite + file locks; older versions relied on GC. Try/except keeps forward-compat. + """ + if client is None: + return + try: + client.close() + except Exception: + logger.debug("client.close() unavailable or failed", exc_info=True) + + class ChromaCollection(BaseCollection): """Thin adapter translating ChromaDB dict returns into typed results.""" @@ -977,7 +991,7 @@ class ChromaBackend(BaseBackend): db_path = os.path.join(palace_path, "chroma.sqlite3") # DB was present when cache was built but is now missing → invalidate. if cached is not None and not os.path.isfile(db_path): - self._clients.pop(palace_path, None) + _close_client(self._clients.pop(palace_path, None)) self._freshness.pop(palace_path, None) cached = None cached_inode, cached_mtime = 0, 0.0 @@ -1134,14 +1148,22 @@ class ChromaBackend(BaseBackend): return ChromaCollection(collection) def close_palace(self, palace) -> None: - """Drop cached handles for ``palace``. Accepts ``PalaceRef`` or legacy path str.""" + """Drop cached handles for ``palace`` and release its SQLite file lock. + + Accepts ``PalaceRef`` or legacy path str. chromadb's rust-side file + lock is held until ``PersistentClient.close()`` is called, so plain + dict eviction would leave the palace path unreopenable and + unremovable in the same process. + """ path = palace.local_path if isinstance(palace, PalaceRef) else palace if path is None: return - self._clients.pop(path, None) + _close_client(self._clients.pop(path, None)) self._freshness.pop(path, None) def close(self) -> None: + for client in self._clients.values(): + _close_client(client) self._clients.clear() self._freshness.clear() self._closed = True diff --git a/tests/test_backends.py b/tests/test_backends.py index 4ddfe12..06998c5 100644 --- a/tests/test_backends.py +++ b/tests/test_backends.py @@ -1,4 +1,5 @@ import os +import shutil import sqlite3 from pathlib import Path @@ -206,6 +207,52 @@ def test_query_empty_preserves_embeddings_outer_shape_when_requested(): assert not_requested.embeddings is None +def test_chroma_close_palace_releases_sqlite_lock_for_reopen(tmp_path): + """close_palace must release chromadb's rust-side SQLite file lock so + a fresh PersistentClient on the same path after shutil.rmtree can + write without hitting SQLITE_READONLY_DBMOVED.""" + backend = ChromaBackend() + palace_path = tmp_path / "palace-a" + ref = PalaceRef(id=str(palace_path), local_path=str(palace_path)) + + col = backend.get_collection(palace=ref, collection_name="mempalace_drawers", create=True) + col.upsert(documents=["hello"], ids=["a"], metadatas=[{"k": "v"}]) + + backend.close_palace(ref) + shutil.rmtree(palace_path) + + col = backend.get_collection(palace=ref, collection_name="mempalace_drawers", create=True) + col.upsert(documents=["world"], ids=["b"], metadatas=[{"k": "v2"}]) + assert col.count() == 1 + + +def test_chroma_close_releases_all_cached_clients(tmp_path): + """close() must release every cached client's SQLite file lock so any + of their palace paths can be reopened by a fresh backend in the same + process.""" + backend = ChromaBackend() + palace_a = tmp_path / "palace-a" + palace_b = tmp_path / "palace-b" + ref_a = PalaceRef(id=str(palace_a), local_path=str(palace_a)) + ref_b = PalaceRef(id=str(palace_b), local_path=str(palace_b)) + + for ref in (ref_a, ref_b): + backend.get_collection(palace=ref, collection_name="mempalace_drawers", create=True).upsert( + documents=["x"], ids=["x"], metadatas=[{"k": "v"}] + ) + + backend.close() + + for path in (palace_a, palace_b): + shutil.rmtree(path) + ref = PalaceRef(id=str(path), local_path=str(path)) + fresh = ChromaBackend() + col = fresh.get_collection(palace=ref, collection_name="mempalace_drawers", create=True) + col.upsert(documents=["y"], ids=["y"], metadatas=[{"k": "v2"}]) + assert col.count() == 1 + fresh.close() + + def test_chroma_cache_invalidates_when_db_file_missing(tmp_path): """A palace rebuild that removes chroma.sqlite3 must drop the stale cache.