Skip to content

Commit d5a5daf

Browse files
committed
fix(chroma): release SQLite lock on cache invalidation, not just close_palace
`close_palace` already routed through `_close_client` to release chromadb's rust-side SQLite WAL/journal lock before next open. The `_client` cache- rebuild path (inode/mtime change OR DB-appeared-after-open) was the second lifecycle point but did not match: it overwrote the dict slot directly, leaving the stale handle alive until GC. Under that branch, a concurrent reader could hit "database is locked" mid-rebuild because the OS-level lock was still held by the previous PersistentClient instance, even though MemPalace had stopped using it. Now matches close_palace: `_close_client(cached)` runs before the slot is overwritten on every invalidation path. Pinned with a test that seeds a sentinel into `_clients`, materializes `chroma.sqlite3` to trip `mtime_appeared`, and asserts the sentinel was passed to `_close_client`.
1 parent 45df1a2 commit d5a5daf

2 files changed

Lines changed: 53 additions & 0 deletions

File tree

mempalace/backends/chroma.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1007,6 +1007,15 @@ def _client(self, palace_path: str):
10071007
)
10081008

10091009
if cached is None or inode_changed or mtime_changed or mtime_appeared:
1010+
# When invalidating an existing cached client (inode/mtime change
1011+
# or DB-file-appeared), close it before overwriting the slot so
1012+
# the SQLite WAL/journal lock is released up front. Otherwise the
1013+
# stale handle lingers until GC and a concurrent reader can hit
1014+
# "database is locked" mid-rebuild — the same lifecycle hazard
1015+
# close_palace() guards via _close_client().
1016+
if cached is not None:
1017+
_close_client(cached)
1018+
self._clients.pop(palace_path, None)
10101019
ChromaBackend._prepare_palace_for_open(palace_path)
10111020
cached = chromadb.PersistentClient(path=palace_path)
10121021
self._clients[palace_path] = cached

tests/test_backends.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,50 @@ def test_chroma_cache_picks_up_db_created_after_first_open(tmp_path):
315315
assert backend._freshness[str(palace_path)] != (0, 0.0)
316316

317317

318+
def test_chroma_cache_invalidation_closes_old_client(tmp_path, monkeypatch):
319+
"""Cache invalidation (DB inode/mtime change OR DB-appeared-after-open)
320+
must release the old client's SQLite WAL/journal lock before replacing
321+
the cache slot. Otherwise the stale handle lingers until GC and a
322+
concurrent reader can hit "database is locked" mid-rebuild — the same
323+
lifecycle hazard `close_palace()` already guards via `_close_client()`."""
324+
from mempalace.backends import chroma as chroma_mod
325+
326+
backend = chroma_mod.ChromaBackend()
327+
palace_path = tmp_path / "palace"
328+
palace_path.mkdir()
329+
330+
closed = []
331+
real_close = chroma_mod._close_client
332+
333+
def _spy(client):
334+
closed.append(client)
335+
real_close(client)
336+
337+
monkeypatch.setattr(chroma_mod, "_close_client", _spy)
338+
339+
# Seed the cache as if a prior _client() call had opened the palace
340+
# before chroma.sqlite3 existed (freshness (0, 0.0) is the absent-DB
341+
# signal). _close_client swallows AttributeError on non-Client objects,
342+
# so a sentinel is fine here — we just need an identity to spot.
343+
sentinel = object()
344+
backend._clients[str(palace_path)] = sentinel
345+
backend._freshness[str(palace_path)] = (0, 0.0)
346+
347+
# Materialize chroma.sqlite3 so the next _client() trips mtime_appeared
348+
# and rebuilds the cache.
349+
import chromadb as _chromadb
350+
351+
_chromadb.PersistentClient(path=str(palace_path)).get_or_create_collection("seed")
352+
353+
refreshed = backend._client(str(palace_path))
354+
355+
assert refreshed is not sentinel
356+
assert sentinel in closed, (
357+
"old cached client must be _close_client'd on cache invalidation, "
358+
"not silently overwritten"
359+
)
360+
361+
318362
def test_base_collection_update_default_rejects_mismatched_lengths():
319363
"""The ABC default update() raises ValueError rather than silently misaligning."""
320364
from mempalace.backends.base import BaseCollection

0 commit comments

Comments
 (0)