Skip to content

fix(backends/chroma): release SQLite file lock on close_palace/close (#1067)#1105

Merged
igorls merged 1 commit intoMemPalace:developfrom
mvalentsev:fix/chroma-backend-close-releases-lock
May 6, 2026
Merged

fix(backends/chroma): release SQLite file lock on close_palace/close (#1067)#1105
igorls merged 1 commit intoMemPalace:developfrom
mvalentsev:fix/chroma-backend-close-releases-lock

Conversation

@mvalentsev
Copy link
Copy Markdown
Contributor

@mvalentsev mvalentsev commented Apr 22, 2026

Problem

ChromaBackend.close_palace() and close() evict cached PersistentClients from self._clients without calling client.close(). chromadb 1.5.x retains the rust-side SQLite file lock until GC, so reopening the same palace path after shutil.rmtree + re-create within one process fails:

chromadb.errors.InternalError: Query error: Database error:
error returned from database: (code: 1032) attempt to write a readonly database

SQLite code 1032 is SQLITE_READONLY_DBMOVED. Reported and diagnosed in #1067.

Fix

_close_client() helper calls PersistentClient.close() with a try/except fallback for older chromadb that does not expose it. Three sites route through it:

  • close_palace(palace): explicit per-palace teardown
  • close(): whole-backend shutdown, iterates cached clients before clearing
  • _client() invalidation on missing chroma.sqlite3: palace rebuilt under us

The _client() auto-invalidation branch on mtime/inode change is left alone. Callers there may still hold a live ChromaCollection backed by the outgoing client; closing it would clear the rust bindings mid-use.

Test plan

  • test_chroma_close_palace_releases_sqlite_lock_for_reopen: close, rmtree, reopen same path
  • test_chroma_close_releases_all_cached_clients: two palaces, whole-backend close, each reopenable
  • Both new tests fail on current develop with SQLite code 1032 and pass with the fix
  • Repro from the issue body runs to completion after the fix

Credits @xelauvas for the diagnosis and proposed direction.

Fixes #1067.

@mvalentsev mvalentsev marked this pull request as ready for review April 22, 2026 11:05
@mvalentsev mvalentsev force-pushed the fix/chroma-backend-close-releases-lock branch from c8a6938 to 195eed2 Compare April 22, 2026 11:10
@igorls igorls added bug Something isn't working storage labels Apr 24, 2026
@mvalentsev mvalentsev force-pushed the fix/chroma-backend-close-releases-lock branch from 195eed2 to 4260706 Compare April 26, 2026 18:59
@igorls igorls added this to the v3.3.5 milestone May 2, 2026
…emPalace#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.
@mvalentsev mvalentsev force-pushed the fix/chroma-backend-close-releases-lock branch 2 times, most recently from d5a5daf to 45df1a2 Compare May 3, 2026 14:23
@igorls igorls merged commit ef0e45a into MemPalace:develop May 6, 2026
6 checks passed
igorls added a commit that referenced this pull request May 6, 2026
…1282 #1167 #1160

Bundled CHANGELOG entries for the seven Tier-1 PRs merged today, including
the behavior-change call-out for #1167 (KG date validators now reject
non-ISO inputs that previously produced silent empty results).
xcarbo added a commit to xcarbo/mempalace that referenced this pull request May 7, 2026
Catches up on a heavy upstream day — 22 fixes merged in 24h plus prior
backlog. Highlights pulled in:

- MemPalace#1305 hooks: ~/.mempalace/ deletion is now a stable kill-switch (hooks
  no longer rebuild the dir hierarchy on Stop/PreCompact/SessionStart)
- MemPalace#1214 KG: reject inverted intervals (valid_to < valid_from) at write time —
  prevents silently invisible triples
- MemPalace#1067/MemPalace#1105 chroma: ChromaBackend.close_palace() now actually releases
  the SQLite file lock (PersistentClient.close() on evict + invalidation)
- MemPalace#1215 entity_registry: atomic save (tmp+fsync+rename) — no more
  corruption on crash mid-write
- MemPalace#1073/MemPalace#1107 mempalace compress: paginated drawer fetch — no longer
  trips SQLITE_MAX_VARIABLE_NUMBER on palaces >32k drawers
- MemPalace#1282 stdio: Windows console UTF-8 reconfig for cli/mcp_server/hooks_cli
- MemPalace#1164/MemPalace#1167 mcp KG: sanitize_iso_date() blocks malformed date strings
  silently producing empty result sets
- MemPalace#1136/MemPalace#1160 mcp: per-path KG cache for multi-tenant hosts that rotate
  MEMPALACE_PALACE_PATH between tool calls
- MemPalace#1286 mcp: retry _get_collection() once on transient failure
- MemPalace#1138 lint cleanup, MemPalace#1019 search-crash fix
- 4 new tools/ scripts (backup_claude_jsonls, find_orphan_claude_jsonls,
  render_jsonl, save.md)

Conflict resolution (CHANGELOG.md only — code files all auto-merged):

- 3.3.5 section: untouched (already merged in our prior commit; upstream
  added several new bug-fix entries which auto-merged cleanly)
- 3.3.4 Bug Fixes: kept upstream's new MemPalace#1305 entry; preserved our richer
  detail on topic-tunnels (MemPalace#1194/MemPalace#1195/MemPalace#1197), HNSW-bloat (MemPalace#1191),
  max_seq_id (MemPalace#1135), and auto-ingest (MemPalace#1230/MemPalace#1231) — upstream's shorter
  topic-tunnels entry was a strict subset of ours.

xdev patches preserved (still on this branch, untouched by merge):
- 6ef44cb fix(hooks): route CC transcripts via convo_miner with cwd-based wings
- 3fad61d fix(config): allow leading dash in wing names
- 3fc821a fix(config): tighten leading-char to allow dash but not underscore

Tests: 1557 passed, 1 skipped (full unit suite excluding benchmarks).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChromaBackend._clients.pop() does not close() underlying PersistentClient; causes SQLITE_READONLY_DBMOVED on reopen

2 participants