From e594bad9d69a2bf8b57e51074463e53bc71f0687 Mon Sep 17 00:00:00 2001 From: fatkobra <55045047+fatkobra@users.noreply.github.com> Date: Thu, 18 Jun 2026 11:24:18 +0000 Subject: [PATCH 1/5] fix(mcp): refuse second writer for same palace --- mempalace/mcp_server.py | 110 +++++++++++++++++++++++++++++++++++++++ tests/test_mcp_server.py | 82 +++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 9e2b3c6ab4..483ea922ab 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -224,6 +224,109 @@ def _parse_args(): _MCP_IDLE_HOURS_DEFAULT = 8.0 _last_request_time: float = time.monotonic() +# MCP peer-writer guard (#1818). +# +# The existing per-operation palace lock serializes individual writes, but it +# cannot make another long-lived Chroma PersistentClient forget stale in-memory +# HNSW/FTS state. Hold the same per-palace mine lock for this MCP process +# lifetime. A peer MCP process can still serve read tools, but mutating tools +# refuse before touching Chroma or the knowledge graph. +_MCP_WRITER_LOCK_CM = None +_MCP_WRITER_READ_ONLY = False +_MCP_WRITER_LOCK_ERROR = "" +_MCP_ALLOW_PEER_WRITER_ENV = "MEMPALACE_MCP_ALLOW_PEER_WRITER" + +_MUTATING_TOOLS = frozenset( + { + "mempalace_kg_add", + "mempalace_kg_invalidate", + "mempalace_create_tunnel", + "mempalace_delete_tunnel", + "mempalace_delete_hallway", + "mempalace_add_drawer", + "mempalace_delete_drawer", + "mempalace_mine", + "mempalace_sync", + "mempalace_update_drawer", + "mempalace_diary_write", + } +) + + +def _truthy_env(name: str) -> bool: + return os.environ.get(name, "").strip().lower() in {"1", "true", "yes", "on"} + + +def _acquire_mcp_writer_lock() -> tuple[bool, str]: + """Acquire this process's per-palace MCP writer lease. + + Returns (True, "") when this process may write. Returns (False, reason) + when another live writer already owns the per-palace lease. Once a server + starts read-only it stays read-only for its lifetime; restarting is the + safe way to become the writer after the original holder exits. + """ + + global _MCP_WRITER_LOCK_CM, _MCP_WRITER_READ_ONLY, _MCP_WRITER_LOCK_ERROR + + if _truthy_env(_MCP_ALLOW_PEER_WRITER_ENV): + return True, "" + + if _MCP_WRITER_LOCK_CM is not None: + return True, "" + + if _MCP_WRITER_READ_ONLY: + return False, _MCP_WRITER_LOCK_ERROR + + try: + from .palace import MineAlreadyRunning, mine_palace_lock + + lock_cm = mine_palace_lock(_config.palace_path) + lock_cm.__enter__() + except MineAlreadyRunning as exc: + _MCP_WRITER_READ_ONLY = True + _MCP_WRITER_LOCK_ERROR = ( + "another mempalace writer already holds the palace lock for " + f"{_config.palace_path!r}: {exc}" + ) + return False, _MCP_WRITER_LOCK_ERROR + except Exception as exc: + _MCP_WRITER_LOCK_ERROR = ( + "could not acquire MCP peer-writer lock for " + f"{_config.palace_path!r}: {exc!r}; continuing without " + "peer-writer protection" + ) + logger.warning(_MCP_WRITER_LOCK_ERROR) + return True, _MCP_WRITER_LOCK_ERROR + + _MCP_WRITER_LOCK_CM = lock_cm + _MCP_WRITER_READ_ONLY = False + _MCP_WRITER_LOCK_ERROR = "" + return True, "" + + +def _mcp_peer_writer_refusal(req_id, tool_name: str): + if tool_name not in _MUTATING_TOOLS: + return None + + ok, reason = _acquire_mcp_writer_lock() + if ok: + return None + + return { + "jsonrpc": "2.0", + "id": req_id, + "error": { + "code": -32001, + "message": "Peer MCP writer active; this server is read-only for mutating tools", + "data": { + "tool": tool_name, + "palace": _config.palace_path, + "reason": reason, + "override_env": _MCP_ALLOW_PEER_WRITER_ENV, + }, + }, + } + def _mcp_idle_timeout_secs() -> float: """Return the configured MCP idle timeout in seconds (0 = disabled).""" @@ -1051,6 +1154,9 @@ def tool_status(): # is detected so status stays reachable. db_exists = _backend_db_exists() _refresh_vector_disabled_flag() + writer_ok, writer_reason = _acquire_mcp_writer_lock() + if not writer_ok: + logger.warning("%s; mutating MCP tools will run read-only", writer_reason) if _vector_disabled: return _tool_status_via_sqlite() @@ -3483,6 +3589,10 @@ def handle_request(request): "error": {"code": -32602, "message": f"Invalid value for parameter '{key}'"}, } tool_args.pop("wait_for_previous", None) + peer_writer_error = _mcp_peer_writer_refusal(req_id, tool_name) + if peer_writer_error is not None: + return peer_writer_error + # 'content' is an accepted alias for diary_write's 'entry' (callers often # reuse add_drawer's 'content' name). Map it in here, before dispatch, so a # content-only call still satisfies the required 'entry' param while the diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 27f4251c95..b5da9a081d 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -3834,3 +3834,85 @@ def passthrough(**kwargs): ) assert "error" not in resp assert "result" in resp + + +def test_peer_writer_guard_refuses_mutating_tool_before_handler(monkeypatch): + from mempalace import mcp_server + + called = {"value": False} + + def handler(**kwargs): + called["value"] = True + return {"ok": True} + + monkeypatch.setitem( + mcp_server.TOOLS, + "mempalace_add_drawer", + { + "description": "test write tool", + "input_schema": { + "type": "object", + "properties": { + "wing": {"type": "string"}, + "room": {"type": "string"}, + "content": {"type": "string"}, + }, + }, + "handler": handler, + }, + ) + monkeypatch.setattr( + mcp_server, + "_acquire_mcp_writer_lock", + lambda: (False, "busy writer"), + ) + + response = mcp_server.handle_request( + { + "jsonrpc": "2.0", + "id": 7, + "method": "tools/call", + "params": { + "name": "mempalace_add_drawer", + "arguments": { + "wing": "wing_test", + "room": "room_test", + "content": "hello", + }, + }, + } + ) + + assert called["value"] is False + assert response["error"]["code"] == -32001 + assert "read-only" in response["error"]["message"] + assert response["error"]["data"]["tool"] == "mempalace_add_drawer" + + +def test_peer_writer_guard_does_not_gate_read_tool(monkeypatch): + from mempalace import mcp_server + + def forbidden_lock(): + raise AssertionError("read tools should not acquire the peer-writer lock") + + monkeypatch.setitem( + mcp_server.TOOLS, + "mempalace_status", + { + "description": "test read tool", + "input_schema": {"type": "object", "properties": {}}, + "handler": lambda: {"ok": True}, + }, + ) + monkeypatch.setattr(mcp_server, "_acquire_mcp_writer_lock", forbidden_lock) + + response = mcp_server.handle_request( + { + "jsonrpc": "2.0", + "id": 8, + "method": "tools/call", + "params": {"name": "mempalace_status", "arguments": {}}, + } + ) + + assert '"ok": true' in response["result"]["content"][0]["text"] From f0b5cbf37401d1d2a806352fd1cb8a38228e1651 Mon Sep 17 00:00:00 2001 From: fatkobra <55045047+fatkobra@users.noreply.github.com> Date: Thu, 18 Jun 2026 12:47:17 +0000 Subject: [PATCH 2/5] fix(mcp): cache writer lock setup failures --- mempalace/mcp_server.py | 9 ++++++++- tests/test_mcp_server.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 483ea922ab..a3d52f6dcb 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -233,6 +233,7 @@ def _parse_args(): # refuse before touching Chroma or the knowledge graph. _MCP_WRITER_LOCK_CM = None _MCP_WRITER_READ_ONLY = False +_MCP_WRITER_LOCK_FAILED = False _MCP_WRITER_LOCK_ERROR = "" _MCP_ALLOW_PEER_WRITER_ENV = "MEMPALACE_MCP_ALLOW_PEER_WRITER" @@ -266,7 +267,8 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: safe way to become the writer after the original holder exits. """ - global _MCP_WRITER_LOCK_CM, _MCP_WRITER_READ_ONLY, _MCP_WRITER_LOCK_ERROR + global _MCP_WRITER_LOCK_CM, _MCP_WRITER_READ_ONLY, _MCP_WRITER_LOCK_FAILED + global _MCP_WRITER_LOCK_ERROR if _truthy_env(_MCP_ALLOW_PEER_WRITER_ENV): return True, "" @@ -277,6 +279,9 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: if _MCP_WRITER_READ_ONLY: return False, _MCP_WRITER_LOCK_ERROR + if _MCP_WRITER_LOCK_FAILED: + return True, _MCP_WRITER_LOCK_ERROR + try: from .palace import MineAlreadyRunning, mine_palace_lock @@ -290,6 +295,7 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: ) return False, _MCP_WRITER_LOCK_ERROR except Exception as exc: + _MCP_WRITER_LOCK_FAILED = True _MCP_WRITER_LOCK_ERROR = ( "could not acquire MCP peer-writer lock for " f"{_config.palace_path!r}: {exc!r}; continuing without " @@ -300,6 +306,7 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: _MCP_WRITER_LOCK_CM = lock_cm _MCP_WRITER_READ_ONLY = False + _MCP_WRITER_LOCK_FAILED = False _MCP_WRITER_LOCK_ERROR = "" return True, "" diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index b5da9a081d..50d68429f5 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -3916,3 +3916,31 @@ def forbidden_lock(): ) assert '"ok": true' in response["result"]["content"][0]["text"] + + +def test_peer_writer_lock_setup_failure_is_cached(monkeypatch): + from mempalace import mcp_server, palace + + calls = {"count": 0} + + def broken_mine_palace_lock(palace_path): + calls["count"] += 1 + raise RuntimeError(f"permission denied for {palace_path}") + + monkeypatch.delenv(mcp_server._MCP_ALLOW_PEER_WRITER_ENV, raising=False) + monkeypatch.setattr(palace, "mine_palace_lock", broken_mine_palace_lock) + + monkeypatch.setattr(mcp_server, "_MCP_WRITER_LOCK_CM", None) + monkeypatch.setattr(mcp_server, "_MCP_WRITER_READ_ONLY", False) + monkeypatch.setattr(mcp_server, "_MCP_WRITER_LOCK_FAILED", False) + monkeypatch.setattr(mcp_server, "_MCP_WRITER_LOCK_ERROR", "") + + ok_first, reason_first = mcp_server._acquire_mcp_writer_lock() + ok_second, reason_second = mcp_server._acquire_mcp_writer_lock() + + assert ok_first is True + assert ok_second is True + assert calls["count"] == 1 + assert mcp_server._MCP_WRITER_LOCK_FAILED is True + assert "continuing without peer-writer protection" in reason_first + assert reason_second == reason_first From 63a382fc6d5cec854a221ca5438db19facb0df0e Mon Sep 17 00:00:00 2001 From: fatkobra <55045047+fatkobra@users.noreply.github.com> Date: Fri, 19 Jun 2026 06:42:36 +0000 Subject: [PATCH 3/5] fix(mcp): gate startup on sqlite integrity failures --- mempalace/mcp_server.py | 177 ++++++++++++++++++++++++++++++++++++++- tests/test_mcp_server.py | 123 +++++++++++++++++++++++++++ 2 files changed, 296 insertions(+), 4 deletions(-) diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index a3d52f6dcb..a33143ffe0 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -224,6 +224,24 @@ def _parse_args(): _MCP_IDLE_HOURS_DEFAULT = 8.0 _last_request_time: float = time.monotonic() +# MCP startup/open SQLite integrity gate (#1818). +# +# The peer-writer guard prevents new concurrent writers, but an MCP server can +# still start against a palace that was already left corrupt by a prior writer +# crash/kill. Run the existing read-only SQLite quick_check once on startup/open +# and fail loudly instead of silently serving a malformed FTS5/HNSW index. +_sqlite_integrity_checked = False +_sqlite_integrity_errors: list[str] = [] +_sqlite_integrity_check_error = "" +_SQLITE_INTEGRITY_ERROR_CODE = -32002 +_SQLITE_INTEGRITY_ALLOWED_TOOLS = frozenset( + { + "mempalace_status", + "mempalace_reconnect", + } +) + + # MCP peer-writer guard (#1818). # # The existing per-operation palace lock serializes individual writes, but it @@ -335,6 +353,108 @@ def _mcp_peer_writer_refusal(req_id, tool_name: str): } +def _refresh_sqlite_integrity_status() -> None: + """Refresh the MCP startup SQLite/FTS5 integrity gate. + + Uses repair.sqlite_integrity_errors(), which is read-only and already backs + repair preflight. A failure here is treated as an integrity failure so the + server does not proceed silently after a malformed FTS5 index or other + SQLite-layer corruption (#1818). + """ + + global _sqlite_integrity_checked + global _sqlite_integrity_errors + global _sqlite_integrity_check_error + + if not _is_chroma_backend(): + _sqlite_integrity_checked = True + _sqlite_integrity_errors = [] + _sqlite_integrity_check_error = "" + return + + try: + from .repair import sqlite_integrity_errors + + errors = sqlite_integrity_errors(_config.palace_path) + except Exception as exc: + _sqlite_integrity_check_error = ( + f"sqlite integrity probe failed: {type(exc).__name__}: {exc}" + ) + _sqlite_integrity_errors = [_sqlite_integrity_check_error] + else: + _sqlite_integrity_errors = [str(error) for error in errors if str(error)] + _sqlite_integrity_check_error = "" + + _sqlite_integrity_checked = True + + if _sqlite_integrity_errors: + logger.error( + "SQLite integrity check failed for palace=%s: %s", + _config.palace_path, + "; ".join(_sqlite_integrity_errors[:3]), + ) + + +def _ensure_sqlite_integrity_status() -> None: + if not _sqlite_integrity_checked: + _refresh_sqlite_integrity_status() + + +def _sqlite_integrity_payload() -> dict: + _ensure_sqlite_integrity_status() + + payload = { + "checked": _sqlite_integrity_checked, + "ok": not _sqlite_integrity_errors, + "palace": _config.palace_path, + "sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3"), + "error_count": len(_sqlite_integrity_errors), + "errors": _sqlite_integrity_errors[:10], + } + + if len(_sqlite_integrity_errors) > 10: + payload["truncated"] = len(_sqlite_integrity_errors) - 10 + + if _sqlite_integrity_check_error: + payload["check_error"] = _sqlite_integrity_check_error + + return payload + + +def _mcp_sqlite_integrity_refusal(req_id, tool_name: str): + if tool_name in _SQLITE_INTEGRITY_ALLOWED_TOOLS: + return None + + _ensure_sqlite_integrity_status() + + if not _sqlite_integrity_errors: + return None + + return { + "jsonrpc": "2.0", + "id": req_id, + "error": { + "code": _SQLITE_INTEGRITY_ERROR_CODE, + "message": ( + "Palace SQLite integrity check failed; refusing tool call " + "until the palace is repaired" + ), + "data": { + "tool": tool_name, + "palace": _config.palace_path, + "sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3"), + "errors": _sqlite_integrity_errors[:10], + "error_count": len(_sqlite_integrity_errors), + "hint": ( + "Stop all MemPalace MCP clients/writers, back up the palace, " + "repair the SQLite/FTS5 corruption offline, then run " + "mempalace_reconnect or restart the MCP server." + ), + }, + }, + } + + def _mcp_idle_timeout_secs() -> float: """Return the configured MCP idle timeout in seconds (0 = disabled).""" raw = os.environ.get(_MCP_IDLE_HOURS_ENV, "") @@ -1155,6 +1275,16 @@ def _tool_status_via_sqlite() -> dict: def tool_status(): + _ensure_sqlite_integrity_status() + if _sqlite_integrity_errors: + result = _tool_status_via_sqlite() + if isinstance(result, dict): + result["sqlite_integrity"] = _sqlite_integrity_payload() + result["sqlite_integrity_failed"] = True + result["error"] = "SQLite integrity check failed" + result["partial"] = True + return result + # Run the safe sqlite/pickle probe before we touch chromadb. In the # #1222 failure mode, opening the persistent client to call .count() # can segfault — short-circuit to a pure-sqlite path when divergence @@ -2874,6 +3004,24 @@ def tool_reconnect(): except Exception: pass _kg_by_path.clear() + _refresh_sqlite_integrity_status() + if _sqlite_integrity_errors: + result = { + "success": False, + "message": "SQLite integrity check failed after reconnect", + "sqlite_integrity": _sqlite_integrity_payload(), + "vector_disabled": _vector_disabled, + "vector_disabled_reason": _vector_disabled_reason, + "hint": ( + "Stop all MemPalace MCP clients/writers, back up the palace, " + "repair the SQLite/FTS5 corruption offline, then run " + "mempalace_reconnect or restart the MCP server." + ), + } + if close_errors: + result["error"] = "; ".join(close_errors) + return result + try: col = _get_collection() if col is None: @@ -3478,6 +3626,25 @@ def _internal_tool_error(req_id, tool_name: str, exc: BaseException = None) -> d } +def _mcp_tool_preflight_refusal(req_id, tool_name: str): + """Run MCP request preflight gates outside handle_request complexity.""" + + sqlite_integrity_error = _mcp_sqlite_integrity_refusal(req_id, tool_name) + if sqlite_integrity_error is not None: + return sqlite_integrity_error + + return _mcp_peer_writer_refusal(req_id, tool_name) + + +def _decorate_mcp_tool_result(tool_name: str, result): + """Attach MCP transport-only diagnostics outside handle_request complexity.""" + + if tool_name == "mempalace_status" and isinstance(result, dict): + result.setdefault("sqlite_integrity", _sqlite_integrity_payload()) + + return result + + def handle_request(request): global _last_request_time if not isinstance(request, dict): @@ -3596,9 +3763,9 @@ def handle_request(request): "error": {"code": -32602, "message": f"Invalid value for parameter '{key}'"}, } tool_args.pop("wait_for_previous", None) - peer_writer_error = _mcp_peer_writer_refusal(req_id, tool_name) - if peer_writer_error is not None: - return peer_writer_error + preflight_error = _mcp_tool_preflight_refusal(req_id, tool_name) + if preflight_error is not None: + return preflight_error # 'content' is an accepted alias for diary_write's 'entry' (callers often # reuse add_drawer's 'content' name). Map it in here, before dispatch, so a @@ -3612,7 +3779,8 @@ def handle_request(request): if "entry" not in tool_args or tool_args["entry"] is None: tool_args["entry"] = content_val try: - result = TOOLS[tool_name]["handler"](**tool_args) + result = _decorate_mcp_tool_result(tool_name, TOOLS[tool_name]["handler"](**tool_args)) + return { "jsonrpc": "2.0", "id": req_id, @@ -3898,6 +4066,7 @@ def main(): # Pre-flight: probe HNSW capacity before any tool call so the warning # is visible at startup rather than on first use (#1222). Pure # filesystem read; never opens a chromadb client. + _refresh_sqlite_integrity_status() _refresh_vector_disabled_flag() # Opt-in: pre-load the embedder so the first chromadb-write tool call # does not pay the ONNX/CoreML cold-load tax under the MCP client diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 50d68429f5..b54a36b3df 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -3944,3 +3944,126 @@ def broken_mine_palace_lock(palace_path): assert mcp_server._MCP_WRITER_LOCK_FAILED is True assert "continuing without peer-writer protection" in reason_first assert reason_second == reason_first + + +def test_sqlite_integrity_gate_refuses_non_status_tool(monkeypatch): + from mempalace import mcp_server + + monkeypatch.setattr(mcp_server, "_sqlite_integrity_checked", True) + monkeypatch.setattr( + mcp_server, + "_sqlite_integrity_errors", + ["malformed inverted index for FTS5 table main.embedding_fulltext_search"], + ) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_check_error", "") + + response = mcp_server.handle_request( + { + "jsonrpc": "2.0", + "id": 1818, + "method": "tools/call", + "params": {"name": "mempalace_list_wings", "arguments": {}}, + } + ) + + assert response["error"]["code"] == mcp_server._SQLITE_INTEGRITY_ERROR_CODE + assert "integrity check failed" in response["error"]["message"] + assert response["error"]["data"]["tool"] == "mempalace_list_wings" + assert "malformed inverted index" in response["error"]["data"]["errors"][0] + + +def test_sqlite_integrity_status_surfaces_payload_without_chroma(monkeypatch): + import json + + from mempalace import mcp_server + + monkeypatch.setattr(mcp_server, "_sqlite_integrity_checked", True) + monkeypatch.setattr( + mcp_server, + "_sqlite_integrity_errors", + ["malformed inverted index for FTS5 table main.embedding_fulltext_search"], + ) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_check_error", "") + monkeypatch.setattr( + mcp_server, + "_tool_status_via_sqlite", + lambda: {"total_drawers": 123, "backend": "chroma"}, + ) + + response = mcp_server.handle_request( + { + "jsonrpc": "2.0", + "id": 1819, + "method": "tools/call", + "params": {"name": "mempalace_status", "arguments": {}}, + } + ) + + payload = json.loads(response["result"]["content"][0]["text"]) + + assert payload["total_drawers"] == 123 + assert payload["sqlite_integrity_failed"] is True + assert payload["sqlite_integrity"]["ok"] is False + assert payload["sqlite_integrity"]["error_count"] == 1 + assert "malformed inverted index" in payload["sqlite_integrity"]["errors"][0] + + +def test_sqlite_integrity_reconnect_allowed_when_corrupt(monkeypatch): + from mempalace import mcp_server + + called = {"value": False} + + def fake_reconnect(): + called["value"] = True + return {"success": True} + + monkeypatch.setattr(mcp_server, "_sqlite_integrity_checked", True) + monkeypatch.setattr( + mcp_server, + "_sqlite_integrity_errors", + ["malformed inverted index for FTS5 table main.embedding_fulltext_search"], + ) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_check_error", "") + monkeypatch.setitem( + mcp_server.TOOLS, + "mempalace_reconnect", + { + "description": "test reconnect", + "input_schema": {"type": "object", "properties": {}}, + "handler": fake_reconnect, + }, + ) + + response = mcp_server.handle_request( + { + "jsonrpc": "2.0", + "id": 1820, + "method": "tools/call", + "params": {"name": "mempalace_reconnect", "arguments": {}}, + } + ) + + assert called["value"] is True + assert '"success": true' in response["result"]["content"][0]["text"] + + +def test_refresh_sqlite_integrity_status_records_quick_check_errors(monkeypatch): + from mempalace import mcp_server, repair + + monkeypatch.setattr(mcp_server, "_is_chroma_backend", lambda: True) + monkeypatch.setattr( + repair, + "sqlite_integrity_errors", + lambda palace_path: [ + "malformed inverted index for FTS5 table main.embedding_fulltext_search" + ], + ) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_checked", False) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_errors", []) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_check_error", "") + + mcp_server._refresh_sqlite_integrity_status() + + assert mcp_server._sqlite_integrity_checked is True + assert len(mcp_server._sqlite_integrity_errors) == 1 + assert "malformed inverted index" in mcp_server._sqlite_integrity_errors[0] From 4c46290b80bbd7eb86bea99df508144d31c69653 Mon Sep 17 00:00:00 2001 From: fatkobra <55045047+fatkobra@users.noreply.github.com> Date: Fri, 19 Jun 2026 08:59:25 +0000 Subject: [PATCH 4/5] fix(mcp): applied 3 of the 4 reviewer suggestions --- mempalace/mcp_server.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index a33143ffe0..98fa1d06a3 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -323,6 +323,9 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: return True, _MCP_WRITER_LOCK_ERROR _MCP_WRITER_LOCK_CM = lock_cm + import atexit + + atexit.register(lambda: lock_cm.__exit__(None, None, None)) _MCP_WRITER_READ_ONLY = False _MCP_WRITER_LOCK_FAILED = False _MCP_WRITER_LOCK_ERROR = "" @@ -366,7 +369,7 @@ def _refresh_sqlite_integrity_status() -> None: global _sqlite_integrity_errors global _sqlite_integrity_check_error - if not _is_chroma_backend(): + if not _config.palace_path or not _is_chroma_backend(): _sqlite_integrity_checked = True _sqlite_integrity_errors = [] _sqlite_integrity_check_error = "" @@ -407,7 +410,9 @@ def _sqlite_integrity_payload() -> dict: "checked": _sqlite_integrity_checked, "ok": not _sqlite_integrity_errors, "palace": _config.palace_path, - "sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3"), + "sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3") + if _config.palace_path + else "", "error_count": len(_sqlite_integrity_errors), "errors": _sqlite_integrity_errors[:10], } From 3c7808e1bcd633e897af53dc39e57baccd241959 Mon Sep 17 00:00:00 2001 From: fatkobra <55045047+fatkobra@users.noreply.github.com> Date: Fri, 19 Jun 2026 09:51:02 +0000 Subject: [PATCH 5/5] fix(mcp): guard remaining None palace_path in _mcp_sqlite_integrity_refusal. Added one regression test calling the function directly with palace_path=None. --- mempalace/mcp_server.py | 8 ++++++-- tests/test_mcp_server.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 98fa1d06a3..1409b7271d 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -446,8 +446,12 @@ def _mcp_sqlite_integrity_refusal(req_id, tool_name: str): ), "data": { "tool": tool_name, - "palace": _config.palace_path, - "sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3"), + "palace": _config.palace_path or "", + "sqlite_path": ( + os.path.join(_config.palace_path, "chroma.sqlite3") + if _config.palace_path + else "" + ), "errors": _sqlite_integrity_errors[:10], "error_count": len(_sqlite_integrity_errors), "hint": ( diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index b54a36b3df..8f8f6d640f 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -4067,3 +4067,31 @@ def test_refresh_sqlite_integrity_status_records_quick_check_errors(monkeypatch) assert mcp_server._sqlite_integrity_checked is True assert len(mcp_server._sqlite_integrity_errors) == 1 assert "malformed inverted index" in mcp_server._sqlite_integrity_errors[0] + + +def test_sqlite_integrity_refusal_handles_none_palace_path(monkeypatch): + """ + Regression test for Gemini review feedback on PR #1823 (lines 433-455). + + _mcp_sqlite_integrity_refusal() must not raise TypeError when + _config.palace_path is None — os.path.join(None, "chroma.sqlite3") + would otherwise crash the server on every mutating tool call while + the palace is unconfigured and integrity errors are present. + """ + from mempalace import mcp_server + + # palace_path is a read-only @property on MempalaceConfig (no setter), + # so monkeypatch.setattr on the instance fails. Patch the class-level + # property instead -- monkeypatch restores it automatically on teardown. + monkeypatch.setattr(type(mcp_server._config), "palace_path", property(lambda self: None)) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_checked", True) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_errors", ["malformed inverted index"]) + monkeypatch.setattr(mcp_server, "_sqlite_integrity_check_error", "") + + # Must not raise + result = mcp_server._mcp_sqlite_integrity_refusal(req_id=1, tool_name="mempalace_kg_add") + + assert result is not None + assert result["error"]["data"]["palace"] == "" + assert result["error"]["data"]["sqlite_path"] == "" + assert result["error"]["data"]["tool"] == "mempalace_kg_add"