fix(mcp_server): address copilot review on #1303

- Resolve the EF inside the two reopen branches that actually call
  `client.get_collection` / `client.create_collection`, so warm-cache
  reads stay zero-cost (no `MempalaceConfig()` / `_resolve_providers`
  on every tool call).
- Reuse `ChromaBackend._resolve_embedding_function()` instead of
  duplicating its try/except + log message + None-fallback.
- Reword the inline + CHANGELOG explanation to clarify that ChromaDB 1.x
  persists the EF *identity* (its `name()`) but not the *instance/
  configuration* — `mempalace.embedding` documents this and spoofs
  `name()` to `"default"` precisely so the identity check passes; the
  bug was the *provider list* (lazy ONNX selection) silently differing.
This commit is contained in:
Igor Lins e Silva
2026-05-01 19:46:59 -03:00
parent ac6c2b6af6
commit cd98d6674e
2 changed files with 20 additions and 17 deletions
+19 -16
View File
@@ -66,7 +66,6 @@ from .backends.chroma import ( # noqa: E402
_pin_hnsw_threads,
hnsw_capacity_status,
)
from .embedding import get_embedding_function # noqa: E402
from .query_sanitizer import sanitize_query # noqa: E402
from .searcher import search_memories # noqa: E402
from .palace_graph import ( # noqa: E402
@@ -279,23 +278,25 @@ def _get_collection(create=False):
global _collection_cache, _metadata_cache, _metadata_cache_time
try:
client = _get_client()
# ChromaDB 1.x does not persist the embedding function with the
# collection, so a reader/writer that omits ``embedding_function=``
# silently gets the chromadb-built-in default. On bleeding-edge
# ChromaDB 1.x persists the EF *identity* (its ``name()``) with the
# collection but not the EF *instance/configuration*. So a reader or
# writer that omits ``embedding_function=`` silently gets chromadb's
# built-in ``DefaultEmbeddingFunction`` — its ``name()`` matches the
# one we spoof in ``mempalace.embedding`` (both report ``"default"``,
# the identity check passes), but the *provider list* is chromadb's
# default rather than the user's resolved device. On bleeding-edge
# interpreters (#1299: python 3.14 + chromadb 1.5.x on Apple Silicon)
# the default's lazy ONNX provider selection can SIGSEGV the host
# process on first ``col.add()``. The miner / Stop hook ingest path
# avoids this because it routes through ``ChromaBackend.get_collection``
# which resolves the EF via ``mempalace.embedding.get_embedding_function``.
# The MCP server bypassed that abstraction; mirror its behaviour so
# ``tool_diary_write`` / ``tool_add_drawer`` get the same EF as mining.
try:
ef = get_embedding_function()
except Exception:
logger.exception("Failed to build embedding function; using chromadb default")
ef = None
ef_kwargs = {"embedding_function": ef} if ef is not None else {}
# that default provider selection can SIGSEGV the host process on
# first ``col.add()``. The miner / Stop hook ingest path avoids this
# because it routes through ``ChromaBackend.get_collection``, which
# resolves the EF via ``ChromaBackend._resolve_embedding_function``;
# the MCP server bypassed that abstraction. Resolve the EF inside the
# branches that actually open a collection so warm-cache reads stay
# zero-cost. Reuse the backend helper so the two call sites can't
# drift on logging or fallback semantics.
if create:
ef = ChromaBackend._resolve_embedding_function()
ef_kwargs = {"embedding_function": ef} if ef is not None else {}
# hnsw:num_threads=1 disables ChromaDB's multi-threaded ParallelFor
# HNSW insert path, which has a race in repairConnectionsForUpdate /
# addPoint (see issues #974, #965). Set via metadata on fresh
@@ -325,6 +326,8 @@ def _get_collection(create=False):
_metadata_cache = None
_metadata_cache_time = 0
elif _collection_cache is None:
ef = ChromaBackend._resolve_embedding_function()
ef_kwargs = {"embedding_function": ef} if ef is not None else {}
raw = client.get_collection(_config.collection_name, **ef_kwargs)
_pin_hnsw_threads(raw)
_collection_cache = ChromaCollection(raw)