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.
This commit is contained in:
@@ -676,6 +676,20 @@ def _as_list(v: Any) -> list:
|
|||||||
return [v]
|
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):
|
class ChromaCollection(BaseCollection):
|
||||||
"""Thin adapter translating ChromaDB dict returns into typed results."""
|
"""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_path = os.path.join(palace_path, "chroma.sqlite3")
|
||||||
# DB was present when cache was built but is now missing → invalidate.
|
# DB was present when cache was built but is now missing → invalidate.
|
||||||
if cached is not None and not os.path.isfile(db_path):
|
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)
|
self._freshness.pop(palace_path, None)
|
||||||
cached = None
|
cached = None
|
||||||
cached_inode, cached_mtime = 0, 0.0
|
cached_inode, cached_mtime = 0, 0.0
|
||||||
@@ -1134,14 +1148,22 @@ class ChromaBackend(BaseBackend):
|
|||||||
return ChromaCollection(collection)
|
return ChromaCollection(collection)
|
||||||
|
|
||||||
def close_palace(self, palace) -> None:
|
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
|
path = palace.local_path if isinstance(palace, PalaceRef) else palace
|
||||||
if path is None:
|
if path is None:
|
||||||
return
|
return
|
||||||
self._clients.pop(path, None)
|
_close_client(self._clients.pop(path, None))
|
||||||
self._freshness.pop(path, None)
|
self._freshness.pop(path, None)
|
||||||
|
|
||||||
def close(self) -> None:
|
def close(self) -> None:
|
||||||
|
for client in self._clients.values():
|
||||||
|
_close_client(client)
|
||||||
self._clients.clear()
|
self._clients.clear()
|
||||||
self._freshness.clear()
|
self._freshness.clear()
|
||||||
self._closed = True
|
self._closed = True
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import os
|
import os
|
||||||
|
import shutil
|
||||||
import sqlite3
|
import sqlite3
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
@@ -206,6 +207,52 @@ def test_query_empty_preserves_embeddings_outer_shape_when_requested():
|
|||||||
assert not_requested.embeddings is None
|
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):
|
def test_chroma_cache_invalidates_when_db_file_missing(tmp_path):
|
||||||
"""A palace rebuild that removes chroma.sqlite3 must drop the stale cache.
|
"""A palace rebuild that removes chroma.sqlite3 must drop the stale cache.
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user