fix(mcp): refuse second writer for same palace - #1823
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an MCP peer-writer guard to prevent multiple long-lived Chroma clients from using stale in-memory state by enforcing a per-palace writer lease. Mutating tools are restricted to read-only mode if the writer lock cannot be acquired, and corresponding unit tests have been added. Feedback on these changes highlights that an unexpected exception during lock acquisition is not cached, which would cause subsequent mutating tool calls to repeatedly attempt to acquire the lock and flood the logs; introducing a state variable to cache this failure is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an MCP peer-writer guard to prevent concurrent mutating operations across multiple processes, protecting against stale in-memory Chroma/FTS state. It defines a set of mutating tools, implements a lock acquisition mechanism with caching for failures, and refuses mutating requests when the server is in read-only mode. Additionally, corresponding unit tests have been added to verify this behavior. I have no feedback to provide as there are no review comments to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
|
Adds startup SQLite/FTS5 integrity gate (#1818, gap flagged by @99LevelsUp) The writer guard above stops new concurrent corruption. This closes the remaining gap: a palace that's already corrupt when the server starts (e.g. from a prior killed writer) was served silently with no integrity check on open. Read-only integrity probe (repair.sqlite_integrity_errors()) runs once on startup, cached in module state. |
There was a problem hiding this comment.
Code Review
This pull request introduces an SQLite integrity gate and a peer-writer guard to the MemPalace MCP server to prevent concurrent writes and handle database corruption gracefully. It adds several helper functions for checking integrity and acquiring locks, updates existing tools like tool_status and tool_reconnect to utilize these guards, and includes comprehensive unit tests. The feedback highlights potential TypeError crashes if _config.palace_path is None when constructing file paths or performing integrity checks, and suggests registering an atexit handler to ensure the acquired palace lock is properly released on process termination.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…efusal. Added one regression test calling the function directly with palace_path=None.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an SQLite integrity gate and a peer-writer guard to the MemPalace MCP server. The integrity gate runs a quick check on startup to prevent serving malformed indexes, while the peer-writer guard holds a per-palace lock to prevent concurrent mutating operations. Comprehensive unit tests have been added to verify these behaviors, including handling edge cases like unconfigured palace paths. I have no further feedback to provide as no review comments were submitted.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The bulk upsert can leave the rebuilt palace's FTS5 inverted index malformed (observed at ~92k rows in a full-palace rehearsal); the v3.5.0 MCP startup integrity gate (MemPalace#1823) then refuses every tool call — a completed rebuild that strands the palace. quick_check the destination after 'Rebuild complete' and run the MemPalace#1596 isolated-FTS5 heal in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wf9YWhaRobuwy3rKkGLpd
What does this PR do?
Closes #1818
Why
Two long-lived MCP stdio servers can point at the same Chroma palace. Per-operation write locks serialize individual operations, but they do not invalidate another process's already-open Chroma PersistentClient and in-memory HNSW/FTS state. Holding the per-palace writer lease for the MCP server lifetime prevents a second MCP writer from silently corrupting the same palace.
How to test
python3 -m py_compile mempalace/mcp_server.py tests/test_mcp_server.pypython3 -m ruff format --check mempalace/mcp_server.py tests/test_mcp_server.pypython3 -m pytest tests/test_mcp_server.pypython -m pytest tests/ -vChecklist
python -m pytest tests/ -v)ruff check .)