Skip to content

fix: serialize ChromaCollection writes through palace lock - #1162

Merged
igorls merged 2 commits into
MemPalace:developfrom
imtylervo:fix/palace-write-lock-queue-pattern
May 6, 2026
Merged

fix: serialize ChromaCollection writes through palace lock#1162
igorls merged 2 commits into
MemPalace:developfrom
imtylervo:fix/palace-write-lock-queue-pattern

Conversation

@imtylervo

@imtylervo imtylervo commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

#976 protects mempalace mine, but MCP/direct backend writers still call ChromaCollection.add/upsert/update/delete without the palace lock. This PR moves the lock boundary to the Chroma backend seam so all Chroma writes share the same palace-level serialization, with a re-entrant guard for miner paths that already hold the lock.

Why this matters

mine_palace_lock(palace_path) closes the mine fan-out hole, but its current coverage is the miner.mine() pipeline. Other writers can still reach Chroma directly through the backend adapter, including MCP write tools and third-party callers that use ChromaCollection.

When one of those writers runs concurrently with mempalace mine or another direct writer, it can still enter ChromaDB's HNSW write path concurrently. That is the gap this PR closes.

What changed

  • ChromaCollection now accepts an optional palace_path.
  • ChromaCollection.add/upsert/update/delete acquire mine_palace_lock(palace_path) when a palace path is available.
  • ChromaBackend.get_collection/create_collection and MCP _get_collection() pass the palace path through.
  • mine_palace_lock is now re-entrant for the same thread and palace key, so miner.mine() can hold the outer lock while internal collection writes pass through safely.
  • The re-entrant holder state is tagged with the current PID so forked children do not inherit lock-holder credit from the parent.

palace_path=None preserves the previous no-lock behavior for legacy/tests/direct construction.

Production evidence

An equivalent local patch on a large production palace caught the gap under live load:

  • A mempalace mine subprocess held the palace lock while ingesting project transcripts.
  • A sibling MCP diary_write attempted to write during that window and was blocked by the patched ChromaCollection lock instead of entering Chroma concurrently.
  • Before this lock layer, the same workload had produced HNSW index bloat in link_lists.bin on the order of tens of GB.
  • After the patch, the palace stayed healthy under the same workload: no new Chroma/Rust segfaults, no new auto-quarantine batch, and link_lists.bin remained in the KB range.

The intended trade-off is: fail/skip a concurrent write loudly rather than corrupting the HNSW index.

Tests

$ pytest tests/test_chroma_collection_lock.py tests/test_palace_locks.py tests/test_backends.py -q
....................................................                     [100%]
52 passed in 1.09s

Coverage added:

  • palace_path=None skips locking for backward compatibility.
  • Write methods raise MineAlreadyRunning while another process holds the palace lock, without calling the underlying Chroma collection.
  • Re-entrant writes inside an existing mine_palace_lock do not deadlock.
  • Concurrent direct writers serialize through the palace lock.
  • Read paths do not acquire the write lock.

Scope

This PR intentionally only covers the correctness layer: serializing Chroma writes through the existing palace lock.

Deferred follow-up: a durable pending-write queue for MCP/autosave writes that encounter a busy palace lock, so agent autosaves can be replayed later instead of being dropped.

Refs #1161.

@bensig

bensig commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Hey @imtylervo — heads up that #976 merged to develop on 2026-04-25 and includes the chromadb 1.5.x `hnsw:num_threads=1` retrofit you correctly identified as needed. It uses `collection.modify(configuration=UpdateCollectionConfiguration(hnsw=UpdateHNSWConfiguration(num_threads=1)))` — the API that does persist on 1.5.x — so the runtime-retrofit half of this PR is now redundant.

The `palace_write_lock` half might still add value beyond #976's `mine_global_lock` — different scope (collection-level vs miner-process-wide cross-process). Worth a focused look at whether the queue-pattern lock catches anything that the global mine lock doesn't.

If you want to keep this open, suggest rebasing against develop and scoping down to just the lock layer. If the retrofit was the main motivator, fine to close — and thank you for the precise diagnosis in #1161; that's what made #976's retrofit correct.

MemPalace#976 protects `mempalace mine`, but MCP/direct backend writers still call
ChromaCollection.add/upsert/update/delete without the palace lock. This
moves the lock boundary to the Chroma backend seam so all Chroma writes
share the same palace-level serialization, with a re-entrant guard for
miner paths that already hold the lock.

mine_palace_lock(palace_path) gains a per-thread re-entrant guard
(threading.local + pid-tag against fork inheritance) so
ChromaCollection write methods can take the lock without
self-deadlocking when called from inside miner.mine()'s outer hold.

ChromaCollection.__init__ accepts an optional palace_path; when set,
add/upsert/update/delete wrap their underlying chromadb call with
mine_palace_lock(palace_path). palace_path=None preserves the legacy
no-lock behaviour for direct callers and tests. ChromaBackend's
get_collection/create_collection pass palace_path through;
mcp_server._get_collection forwards _config.palace_path so all MCP
write tools inherit the wrapping.

Tests: 5 new in tests/test_chroma_collection_lock.py covering opt-in,
writer-blocks-during-mine, re-entrant-inside-mine, two-process
serialization, and a source-level read-path-not-locked pin. Plus 1 new
+ 1 rewritten in tests/test_palace_locks.py for the re-entrant
semantics. 52 passed in 1.01s including the existing test_backends.py
regression suite.

Refs MemPalace#1161.
@imtylervo imtylervo changed the title fix: queue-pattern palace write lock + runtime HNSW num_threads retrofit fix: serialize ChromaCollection writes through palace lock Apr 27, 2026
@imtylervo
imtylervo changed the base branch from main to develop April 27, 2026 04:16
@imtylervo
imtylervo force-pushed the fix/palace-write-lock-queue-pattern branch from 300ec9e to f30fdf2 Compare April 27, 2026 04:16
@imtylervo

Copy link
Copy Markdown
Contributor Author

@bensig Updated per your review:

  • Rebased onto develop.
  • Dropped the HNSW retrofit half now covered by fix: HNSW graph corruption, PreCompact deadlock, mine fan-out (closes #974, #965, #955) #976 / develop.
  • Scoped this PR down to the backend-seam lock layer only: ChromaCollection.add/upsert/update/delete now acquire mine_palace_lock(palace_path).
  • Added a per-thread, pid-tagged re-entrant guard so miner.mine() can hold the outer lock while inner Chroma writes pass through without self-deadlock.
  • Added tests for writer-blocks-during-mine, re-entrant-inside-mine, two-process serialization, palace_path=None backward compatibility, and read paths not acquiring the write lock.

Production evidence is summarized in the PR body. Happy to iterate if anything still looks off.

@igorls

igorls commented May 2, 2026

Copy link
Copy Markdown
Member

@imtylervo please check the tests on Windows

@igorls

igorls commented May 6, 2026

Copy link
Copy Markdown
Member

Pushed a follow-up commit (d1e27b8) that runs ruff format (matching the CI-pinned 0.4.x) on the two new test files — that was the only failing check. Pure whitespace/line-collapse changes; no behavioral diff. CI should now go green; will re-review afterwards.

@igorls
igorls merged commit ea6f2c0 into MemPalace:develop May 6, 2026
adv3nt3 added a commit to adv3nt3/mempalace that referenced this pull request May 6, 2026
…error (MemPalace#1264)

When `mempalace.mcp_server` (or any other writer) holds chroma.sqlite3
open, `mempalace mine` would print only the auto-defaults stderr warning
and exit with no diagnostic visible — the chroma open hit lock contention
or a Rust-binding SIGSEGV under chromadb 1.5.x's concurrent-writer path,
and stdout buffering ate the banner that would have explained things.

Adds a pre-flight `detect_palace_holder()` to `palace.py` that uses
`lsof` on POSIX to find a non-self process holding the palace's
chroma.sqlite3, with a coarse `ps`-based classification (mcp_server vs.
mine vs. raw command name). `mine()` checks before any other output and
exits 1 with a one-line stderr error including palace path, holder PID,
process kind, and a suggested next step. The existing
`MineAlreadyRunning` catch now also exits 1 (was: clean exit 0 with
"exiting cleanly" wording, which was misleading per the issue).

Windows and hosts without `lsof` degrade silently to None — pre-flight
becomes a no-op and the existing behavior is preserved. Dry-run skips
the pre-flight (no chroma open). Complementary to PR MemPalace#1162's structural
fix at the ChromaCollection lock layer.
igorls added a commit to mjc/mempalace that referenced this pull request May 7, 2026
… conflicts)

Develop (post-MemPalace#1162 lock-plumbing era) refactored the per-open quarantine
pass into ChromaBackend._prepare_palace_for_open. This branch's
inline-expansion form added quarantine_invalid_hnsw_metadata as a third
check, plus a "discard from _quarantined_paths on inode swap" guard so
re-opens against a different physical DB re-run quarantine.

Resolution merges both:

- _prepare_palace_for_open now also calls quarantine_invalid_hnsw_metadata,
  gated by the same _quarantined_paths set.
- _client keeps the inode_changed -> _quarantined_paths.discard() guard
  before calling the helper, so a fresh inode triggers a fresh pass.
- make_client collapses to a single _prepare_palace_for_open() call.
- test_backends.py keeps both the pickle (MemPalace#1285) and shutil (develop)
  imports — both are used.
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.

3 participants