fix(chroma): stop the client cache invalidating on its own writes

ChromaBackend._client caches a PersistentClient per palace and keys the
cache on chroma.sqlite3's (inode, mtime) so that a write by another
process forces a rebuild rather than serving a stale HNSW segment.

Constructing a PersistentClient writes to chroma.sqlite3, and so do the
collection opens that follow it. The stamp was taken immediately after
the constructor, so it was already behind by the time the surrounding
operation finished its own writes, and the next _client() call read that
footprint as an external change.

A search opens mempalace_drawers and then mempalace_closets, so the
first open moved the mtime the second one checked and the cache missed
on essentially every request. Each miss rebuilt the client and reloaded
both HNSW segments, and the client being displaced was overwritten in
the dict without being closed, so its native index allocation
(max_elements * size_data_per_element, ~440 MB per collection on a 165k
vector palace) was never released.

Re-baseline the stat once the backend's own operation completes, so the
recorded value means "chroma.sqlite3 as this backend last left it" and a
later difference is genuinely somebody else's write. Cover writes made
through ChromaCollection as well, via its existing _write_lock context
manager, so the file-a-drawer-then-search cycle stops reloading the
index. Close the displaced client on the rebuild path.

An external write that lands while one of our own operations is in
flight is folded into the new stamp and picked up on the next change.
mtime cannot distinguish writers on its own, and PRAGMA data_version
does not help: it reports writes by any other connection, and chromadb's
connection is foreign to a probe connection, so our own opens would
register as external there too.

Eight searches against the same palace, HTTP transport:
  before  951 MB -> 3522 MB
  after   940 MB -> 965 MB

Author: igorls

CHANGELOG.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ### Bug Fixes
 
+- **A long-running MCP server no longer grows by ~440 MB per collection open.** `ChromaBackend._client` keys its client cache on `chroma.sqlite3`'s inode and mtime to detect writes by another process, but constructing a `chromadb.PersistentClient` writes to that file itself, and so do the collection opens that follow — so the stamp taken at construction time was already stale by the time the next open compared against it. Every search opens `mempalace_drawers` and then `mempalace_closets`, and the first open moved the mtime the second one checked, so the cache missed on essentially every request: each search rebuilt the client and reloaded both HNSW segments, and the displaced client was dropped from the dict without being closed, so its native index memory was never returned. On a palace of ~165k vectors that was ~650 MB of resident growth per search; a server left running for an afternoon reached 3.9 GB. The freshness stat is now re-baselined once the backend's own operation finishes — including writes through `ChromaCollection`, so the file-a-drawer-then-search cycle stops reloading the index too — which makes the recorded value mean "`chroma.sqlite3` as this backend last left it", and a genuine external change still rebuilds. The rebuild path also closes the client it replaces. Measured over eight searches on the same palace: 951 MB → 3522 MB before, 940 MB → 965 MB after. An external write landing *while* one of our own operations is in flight is absorbed into the new stamp and picked up on the next change; mtime cannot distinguish writers, and `PRAGMA data_version` does not help because chromadb's own connection reads as foreign to a probe connection. (#2307)
 - **`sweep` books a failed `stat` as a failure again.** The non-regular-file gate added in 3.7.1 reads `stat.S_ISREG(f.stat().st_mode)` inside a `try`, and its `except OSError` printed `SKIP` and moved on. A dangling symlink, a symlink loop and a file unlinked between `rglob` and the gate all raise there, and before the gate existed every one of them reached `sweep()` and was booked in `failures` — so `sweep` went from reporting a transcript it could not read to reporting success. A failed probe is now an error, not a benign file type: it is logged, printed as `WARNING`, and appended to `failures`, while a probe that succeeds and says "not regular" still skips silently. (#2221)
 - **`mempalace init` no longer tracebacks on a directory it cannot enter.** `_parse_gradle`'s `is_file()` gate sat in front of the `try` that the parser's own `except OSError` provides, so a manifest under a directory with `r` but no `x` raised `PermissionError` out of a call that used to answer "no manifest name". The gate moved inside that `try`, and `_collect_manifest_names` stats through `os.path.isfile`, which reports rather than raises. (#2221)
 - **`split` no longer blocks on a FIFO at its own output name, nor writes through a broken link.** The type gate in `main` covers the files the glob listed; `split_file` builds its output names itself, so a pre-existing named pipe at one of them wedged `write_text` in the kernel waiting for a reader. The check asks about the link itself rather than its target, because a dangling symlink at an output name reads as "nothing there" and `write_text` would create the target — landing a chunk outside the output directory. Output names that are anything but a regular file are now skipped with a `SKIP` line. (#2221)

mempalace/backends/chroma.py

@@ -1595,15 +1595,27 @@ class ChromaCollection(BaseCollection):
     directly without going through ``ChromaBackend``.
     """
 
-    def __init__(self, collection, palace_path: Optional[str] = None):
+    def __init__(self, collection, palace_path: Optional[str] = None, backend=None):
         self._collection = collection
         self._palace_path = palace_path
+        # Owning ChromaBackend, when this collection came through one. Used
+        # only to re-baseline that backend's freshness stat after our writes
+        # (see _write_lock). None for directly-constructed test doubles.
+        self._backend = backend
 
     @contextlib.contextmanager
     def _write_lock(self):
         """Acquire ``mine_palace_lock`` for the configured palace, if any.
 
         No-op (yields immediately) when ``self._palace_path`` is None.
+
+        On exit, re-baselines the owning backend's client-cache freshness stat.
+        A write moves ``chroma.sqlite3``'s mtime, and that stat is how
+        :meth:`ChromaBackend._client` detects an *external* change; without the
+        re-baseline our own upsert looks like somebody else's write and the
+        next collection open rebuilds the client, reloading every HNSW segment
+        it had already paid for. That made the file-a-drawer-then-search cycle
+        reload the whole index each time.
         """
         if self._palace_path is None:
             yield
@@ -1612,7 +1624,11 @@ def _write_lock(self):
         from ..palace import mine_palace_lock
 
         with mine_palace_lock(self._palace_path):
-            yield
+            try:
+                yield
+            finally:
+                if self._backend is not None:
+                    self._backend._restamp(self._palace_path)
 
     # ------------------------------------------------------------------
     # Writes
@@ -2327,6 +2343,18 @@ def _client(self, palace_path: str):
                 # change. Gated on genuine external change (not first open) so
                 # cold opens never pay the global-evict cost.
                 _clear_chroma_system_cache()
+            # Release the client we are about to displace. Each live
+            # PersistentClient pins its own copy of every HNSW segment it has
+            # opened (``max_elements * size_data_per_element`` bytes -- ~440 MB
+            # per collection on a 165k-drawer palace), and neither dict
+            # eviction nor _clear_chroma_system_cache() returns that native
+            # memory. Dropping it here keeps a long-lived server flat across
+            # rebuilds instead of accumulating one orphaned index set per
+            # external change. Any ChromaCollection handed out before this
+            # point is invalidated -- which is the intent: the rebuild only
+            # fires when the palace changed underneath us, and serving the
+            # pre-change segment is the stale-index class of #2002/#2028.
+            _close_client(self._clients.pop(palace_path, None))
             ChromaBackend._prepare_palace_for_open(palace_path)
             cached = chromadb.PersistentClient(path=palace_path)
             self._clients[palace_path] = cached
@@ -2336,6 +2364,40 @@ def _client(self, palace_path: str):
             self._freshness[palace_path] = self._db_stat(palace_path)
         return cached
 
+    def _restamp(self, palace_path: str) -> None:
+        """Re-baseline the freshness stat after this backend's own writes.
+
+        Opening a ``chromadb.PersistentClient`` writes to ``chroma.sqlite3``,
+        and so do the collection opens that follow it, so the mtime this cache
+        keys on moves *while we are using it*. Stamping only at
+        client-construction time (as :meth:`_client` does above) therefore left
+        the recorded value stale the moment the surrounding operation finished
+        its own writes, and the next ``_client()`` call read our own footprint
+        as an external change.
+
+        The effect was a cache that essentially never hit: a single search
+        opens ``mempalace_drawers`` and then ``mempalace_closets``, and the
+        first open bumped the mtime that the second one checked, so every
+        search rebuilt the client and reloaded both HNSW segments.
+
+        Stamping again once the operation is done makes the recorded value mean
+        "``chroma.sqlite3`` as this backend last left it", so a later
+        difference is genuinely somebody else's write.
+
+        Trade-off: an external write that lands *while* one of our operations
+        is in flight is absorbed into the new stamp and will not trigger a
+        rebuild until the next change. That window is one collection open wide.
+        It cannot be closed with mtime alone, and ``PRAGMA data_version`` does
+        not help -- it reports writes by any other *connection*, and chromadb's
+        own connection is foreign to a probe connection, so our own opens would
+        register as external there too.
+
+        No-ops when the path has no cached client, so an eviction that races
+        the operation (``close_palace``) is not resurrected as a stale stamp.
+        """
+        if palace_path in self._freshness:
+            self._freshness[palace_path] = self._db_stat(palace_path)
+
     # ------------------------------------------------------------------
     # Public static helpers (legacy; prefer :meth:`get_collection`)
     # ------------------------------------------------------------------
@@ -2476,7 +2538,11 @@ def get_collection(
                     raise ValueError(explanation) from e
                 raise
         _pin_hnsw_threads(collection)
-        return ChromaCollection(collection, palace_path=palace_path)
+        # Our own client construction and collection open just wrote to
+        # chroma.sqlite3; re-baseline so the next _client() call does not read
+        # that as an external change and rebuild the client.
+        self._restamp(palace_path)
+        return ChromaCollection(collection, palace_path=palace_path, backend=self)
 
     def close_palace(self, palace) -> None:
         """Drop cached handles for ``palace`` and release its SQLite file lock.
@@ -2538,6 +2604,7 @@ def get_or_create_collection(self, palace_path: str, collection_name: str) -> Ch
     def delete_collection(self, palace_path: str, collection_name: str) -> None:
         """Delete ``collection_name`` from the palace at ``palace_path``."""
         self._client(palace_path).delete_collection(collection_name)
+        self._restamp(palace_path)
 
     def create_collection(
         self, palace_path: str, collection_name: str, hnsw_space: str = "cosine"
@@ -2550,7 +2617,8 @@ def create_collection(
             metadata=_hnsw_creation_metadata({"hnsw_space": hnsw_space}),
             **ef_kwargs,
         )
-        return ChromaCollection(collection, palace_path=palace_path)
+        self._restamp(palace_path)
+        return ChromaCollection(collection, palace_path=palace_path, backend=self)
 
 
 def _normalize_get_collection_args(args, kwargs):

tests/test_backends.py

@@ -18,6 +18,7 @@
     available_backends,
     get_backend,
 )
+from mempalace.backends import chroma as chroma_module
 from mempalace.backends.chroma import (
     ChromaBackend,
     ChromaCollection,
@@ -455,6 +456,174 @@ def test_chroma_cache_picks_up_db_created_after_first_open(tmp_path):
     assert backend._freshness[str(palace_path)] != (0, 0.0)
 
 
+def _count_persistent_clients(monkeypatch):
+    """Patch ``chromadb.PersistentClient`` with a counting passthrough.
+
+    Returns the list whose length is the number of constructions so far.
+    """
+    built = []
+    real = chromadb.PersistentClient
+
+    def counting(*args, **kwargs):
+        built.append(1)
+        return real(*args, **kwargs)
+
+    monkeypatch.setattr(chromadb, "PersistentClient", counting)
+    return built
+
+
+def test_chroma_client_cache_survives_its_own_writes(tmp_path, monkeypatch):
+    """Opening several collections must not rebuild the client each time.
+
+    Regression for the unbounded-memory bug: ``PersistentClient(...)`` writes
+    to ``chroma.sqlite3``, so an mtime stamp taken at construction time was
+    already stale by the time the *next* open checked it. A search opens
+    ``mempalace_drawers`` then ``mempalace_closets``, so the first open
+    invalidated the cache for the second, and every search rebuilt the client
+    and reloaded both HNSW segments -- ~440 MB per collection on a large
+    palace, never released, until the server sat on multiple GB.
+
+    The invariant that matters is construction *count*: one client for a
+    palace that nothing else is writing to, no matter how many opens.
+    """
+    palace_path = tmp_path / "palace"
+    palace_path.mkdir()
+    ref = PalaceRef(id=str(palace_path), local_path=str(palace_path))
+    backend = ChromaBackend()
+    built = _count_persistent_clients(monkeypatch)
+
+    # A tmp palace with no drawers opens in well under the 0.01s epsilon, so
+    # its own writes land inside a single mtime tick and the bug stays hidden.
+    # Model the real palace instead: push the mtime forward from inside
+    # get_collection (_pin_hnsw_threads is the last step before the restamp)
+    # so every open leaves the DB visibly newer than the stamp that preceded
+    # it -- which is exactly what a 165k-drawer palace does on its own.
+    real_pin = chroma_module._pin_hnsw_threads
+
+    def pin_and_touch(collection):
+        result = real_pin(collection)
+        db_file = palace_path / "chroma.sqlite3"
+        if db_file.is_file():
+            st = db_file.stat()
+            os.utime(db_file, (st.st_atime, st.st_mtime + 1))
+        return result
+
+    monkeypatch.setattr(chroma_module, "_pin_hnsw_threads", pin_and_touch)
+
+    try:
+        for _ in range(3):
+            for name in ("mempalace_drawers", "mempalace_closets"):
+                backend.get_collection(palace=ref, collection_name=name, create=True)
+        assert len(built) == 1, f"rebuilt the client {len(built)} times for one palace"
+    finally:
+        backend.close()
+
+
+def test_chroma_collection_write_does_not_rebuild_client(tmp_path, monkeypatch):
+    """Filing a drawer must not make the next open reload the index.
+
+    A write through ChromaCollection moves chroma.sqlite3's mtime, which is
+    the same signal _client() reads to detect an external change. Without a
+    re-baseline the file-then-search cycle -- the hot path for hook-driven
+    filing -- rebuilt the client on every iteration and reloaded every HNSW
+    segment it had already paid for.
+    """
+    palace_path = tmp_path / "palace"
+    palace_path.mkdir()
+    ref = PalaceRef(id=str(palace_path), local_path=str(palace_path))
+    backend = ChromaBackend()
+    built = _count_persistent_clients(monkeypatch)
+
+    try:
+        col = backend.get_collection(palace=ref, collection_name="mempalace_drawers", create=True)
+        assert len(built) == 1
+
+        for i in range(3):
+            col.upsert(documents=[f"doc {i}"], ids=[f"id{i}"], metadatas=[{"k": "v"}])
+            backend.get_collection(palace=ref, collection_name="mempalace_drawers", create=True)
+        assert len(built) == 1, f"a write forced {len(built) - 1} client rebuild(s)"
+    finally:
+        backend.close()
+
+
+def test_chroma_client_cache_still_rebuilds_on_external_write(tmp_path, monkeypatch):
+    """Re-stamping our own writes must not blind the cache to somebody else's.
+
+    The memory fix works by treating the freshness stat as "the DB as this
+    backend last left it". That is only safe if a genuine external change --
+    a peer sync, a concurrent ``mine``, a restore -- still forces the rebuild
+    that #2002/#2028 rely on to avoid serving a stale HNSW segment.
+    """
+    palace_path = tmp_path / "palace"
+    palace_path.mkdir()
+    ref = PalaceRef(id=str(palace_path), local_path=str(palace_path))
+    backend = ChromaBackend()
+    built = _count_persistent_clients(monkeypatch)
+
+    try:
+        backend.get_collection(palace=ref, collection_name="mempalace_drawers", create=True)
+        assert len(built) == 1
+
+        # Somebody else writes the palace. Move the mtime well past the 0.01s
+        # epsilon so the change is unambiguous on coarse-grained filesystems.
+        db_file = palace_path / "chroma.sqlite3"
+        st = db_file.stat()
+        os.utime(db_file, (st.st_atime, st.st_mtime + 60))
+
+        backend.get_collection(palace=ref, collection_name="mempalace_drawers", create=True)
+        assert len(built) == 2, "external write did not invalidate the cached client"
+    finally:
+        backend.close()
+
+
+def test_chroma_restamp_ignores_uncached_palace(tmp_path):
+    """``_restamp`` must not resurrect a stamp for an evicted palace.
+
+    ``close_palace`` can race an in-flight ``get_collection``; if the restamp
+    that follows re-added a freshness entry with no client behind it, the next
+    open would compare against a stamp it never earned.
+    """
+    palace_path = tmp_path / "palace"
+    palace_path.mkdir()
+    (palace_path / "chroma.sqlite3").write_bytes(b"")
+
+    backend = ChromaBackend()
+    backend._restamp(str(palace_path))
+    assert str(palace_path) not in backend._freshness
+
+
+def test_chroma_client_rebuild_closes_displaced_client(tmp_path, monkeypatch):
+    """A rebuild must close the client it replaces, not just drop the reference.
+
+    The displaced client pins its own copy of every HNSW segment it opened;
+    dict eviction alone leaves that native memory allocated for the life of
+    the process.
+    """
+    palace_path = tmp_path / "palace"
+    palace_path.mkdir()
+    db_file = palace_path / "chroma.sqlite3"
+    db_file.write_bytes(b"")
+    st = db_file.stat()
+
+    closed = []
+
+    class _Sentinel:
+        def close(self):
+            closed.append(1)
+
+    backend = ChromaBackend()
+    backend._clients[str(palace_path)] = _Sentinel()
+    backend._freshness[str(palace_path)] = (st.st_ino, st.st_mtime)
+    # External write forces the rebuild branch.
+    os.utime(db_file, (st.st_atime, st.st_mtime + 60))
+
+    try:
+        backend._client(str(palace_path))
+        assert closed == [1], "displaced client was dropped without being closed"
+    finally:
+        backend.close()
+
+
 def test_base_collection_update_default_rejects_mismatched_lengths():
     """The ABC default update() raises ValueError rather than silently misaligning."""
     from mempalace.backends.base import BaseCollection