From 2d8fc9e66de45058b5a3b1ea51ec09cc1d45f999 Mon Sep 17 00:00:00 2001 From: Igor Lins e Silva <4753812+igorls@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:42:28 -0300 Subject: [PATCH] fix(storage): enforce local single-writer ownership --- docs/write-routing-policy.md | 27 ++++++++++++++++ mempalace/daemon.py | 45 ++++++++++++++++++++++++-- mempalace/mcp_server.py | 40 +++++++++++++++++------ mempalace/palace.py | 15 +++++++++ tests/test_daemon.py | 35 ++++++++++++++++++++ tests/test_mcp_http_transport.py | 36 +++++++++++++++++++++ tests/test_mcp_server.py | 55 ++++++++++++++++++++++++++++++-- 7 files changed, 238 insertions(+), 15 deletions(-) diff --git a/docs/write-routing-policy.md b/docs/write-routing-policy.md index f7f9789..b5820cf 100644 --- a/docs/write-routing-policy.md +++ b/docs/write-routing-policy.md @@ -109,6 +109,33 @@ Invalid values fail with a source-specific error rather than silently falling back. This is important because silently turning a misspelled `require` into a direct write would violate the safety purpose of the policy. +## Local backend single-writer safety + +File-backed backends such as `chroma`, `sqlite_exact`, and Milvus Lite support +exactly one writable process per palace. Serializing individual calls is not +enough because each long-lived process can retain SQLite/WAL, FTS, or vector +index state between calls. + +- A writable daemon owns the palace writer lease for its full lifetime. +- Writable MCP HTTP acquires that lease before binding and refuses startup if + another process owns it. +- MCP stdio may coexist for reads, but mutating tools refuse while another + process owns the lease and become available after that owner exits. +- Read-only MCP HTTP may coexist with the writer. +- Direct CLI and hook writes must not run beside a writable daemon or MCP HTTP + owner. Route them through the daemon with `require` when the daemon owns the + palace. + +`MEMPALACE_MCP_ALLOW_PEER_WRITER` cannot bypass this protection for local +file-backed or unknown plugin backends. It is retained only for explicitly +remote service backends (`qdrant` and `pgvector`) that coordinate concurrent +clients themselves. + +Do not delete or unlink a live palace lock to recover ownership. Stop the +owning process cleanly; the operating system releases its lock automatically. +If corruption is suspected, back up the palace and run integrity/repair +operations offline, with no writable service running. + ## Follow-up PRs Hook-triggered writes now consume this policy; see diff --git a/mempalace/daemon.py b/mempalace/daemon.py index 8c92976..6951379 100644 --- a/mempalace/daemon.py +++ b/mempalace/daemon.py @@ -28,6 +28,12 @@ from urllib import request as urlrequest from urllib.parse import parse_qs, urlparse from .config import MempalaceConfig +from .palace import ( + MineAlreadyRunning, + backend_requires_single_writer, + mine_palace_lock, + resolve_backend_name, +) HOST = "127.0.0.1" STATE_ROOT_ENV = "MEMPALACE_DAEMON_STATE_ROOT" @@ -633,6 +639,15 @@ def _json_response(handler: BaseHTTPRequestHandler, status: int, payload: dict[s handler.close_connection = True +def _restore_server_process_state(previous_env: dict[str, str | None], previous_umask: int) -> None: + for key, value in previous_env.items(): + if value is None: + os.environ.pop(key, None) + else: + os.environ[key] = value + os.umask(previous_umask) + + def run_server(palace_path: str, *, backend: str | None = None, port: int = 0) -> None: palace_path = canonical_palace_path(palace_path) previous_env = { @@ -651,8 +666,31 @@ def run_server(palace_path: str, *, backend: str | None = None, port: int = 0) - # QueueStore (its _init_db opens the DB in WAL mode) — not only once the HTTP # server starts. Restored in the finally at the end of run_server. prev_umask = os.umask(0o077) - token = ensure_token(palace_path) - runtime = DaemonRuntime(palace_path, backend=backend) + runtime = None + writer_lease = contextlib.ExitStack() + try: + resolved_backend = resolve_backend_name(palace_path, explicit=backend) + if backend_requires_single_writer(resolved_backend): + try: + writer_lease.enter_context(mine_palace_lock(palace_path)) + except MineAlreadyRunning as exc: + raise DaemonError( + "writable daemon startup refused: another writer owns " + f"local backend {resolved_backend!r} for {palace_path!r}; " + "stop the existing writable MCP/direct/daemon owner, or " + "route all writes through that owner" + ) from exc + + token = ensure_token(palace_path) + # Backend resolution above is only the ownership decision. Preserve + # the caller's explicit/implicit distinction in queued payloads: + # DaemonRuntime historically injects a backend only when one was + # explicitly selected. + runtime = DaemonRuntime(palace_path, backend=backend) + except BaseException: + writer_lease.close() + _restore_server_process_state(previous_env, prev_umask) + raise class _Handler(BaseHTTPRequestHandler): protocol_version = "HTTP/1.1" @@ -803,7 +841,8 @@ def run_server(palace_path: str, *, backend: str | None = None, port: int = 0) - finally: _drain_and_cleanup(runtime, palace_path, previous_env) finally: - os.umask(prev_umask) + writer_lease.close() + _restore_server_process_state(previous_env, prev_umask) def _drain_and_cleanup( diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 6c0f16c..ef2be5e 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -426,9 +426,6 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: 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, "" - if _MCP_WRITER_LOCK_CM is not None: return True, "" @@ -437,10 +434,25 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: # the server self-heals into the writer the moment the peer exits. A broken # lock *mechanism* (below) is still cached, since retrying it can't help. if _MCP_WRITER_LOCK_FAILED: - return True, _MCP_WRITER_LOCK_ERROR + return False, _MCP_WRITER_LOCK_ERROR try: - from .palace import MineAlreadyRunning, mine_palace_lock + from .palace import ( + MineAlreadyRunning, + backend_requires_single_writer, + mine_palace_lock, + resolve_backend_name, + ) + + backend_name = resolve_backend_name(_config.palace_path) + if _truthy_env(_MCP_ALLOW_PEER_WRITER_ENV): + if not backend_requires_single_writer(backend_name): + return True, "" + logger.warning( + "%s cannot bypass the single-writer requirement for local backend %r", + _MCP_ALLOW_PEER_WRITER_ENV, + backend_name, + ) lock_cm = mine_palace_lock(_config.palace_path) lock_cm.__enter__() @@ -455,11 +467,11 @@ def _acquire_mcp_writer_lock() -> tuple[bool, str]: _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 " - "peer-writer protection" + f"{_config.palace_path!r}: {exc!r}; refusing mutating tools " + "because peer-writer protection could not be established" ) - logger.warning(_MCP_WRITER_LOCK_ERROR) - return True, _MCP_WRITER_LOCK_ERROR + logger.error(_MCP_WRITER_LOCK_ERROR) + return False, _MCP_WRITER_LOCK_ERROR _MCP_WRITER_LOCK_CM = lock_cm import atexit @@ -5499,6 +5511,16 @@ def _run_http_loop() -> None: # still cannot masquerade as an HTTP response. logger.info("MemPalace MCP HTTP server starting...") + # A writable HTTP server is a long-lived storage client, so it must own the + # local palace before it binds. Refusing at startup avoids advertising a + # writable service that will only fail (or race) on its first mutation. + # Explicit read-only HTTP remains safe to run beside the one writer owner. + if not _READ_ONLY: + writer_ok, writer_reason = _acquire_mcp_writer_lock() + if not writer_ok: + logger.error("Writable MCP HTTP startup refused: %s", writer_reason) + raise SystemExit(2) + # The HTTP transport exists for long-lived deployments. Do the cheap # filesystem-only probe before binding, but never make the listener wait on # optional embedder/HNSW warmup. Operators and tests should see /healthz as diff --git a/mempalace/palace.py b/mempalace/palace.py index 49ad06c..d022494 100644 --- a/mempalace/palace.py +++ b/mempalace/palace.py @@ -332,6 +332,21 @@ def resolve_backend_name(palace_path: str, explicit: Optional[str] = None) -> st return selected +_MULTI_PROCESS_WRITER_BACKENDS = frozenset({"pgvector", "qdrant"}) + + +def backend_requires_single_writer(backend_name: str) -> bool: + """Return whether a backend needs one process-lifetime writer owner. + + Local file-backed backends cannot safely coordinate independent long-lived + clients by serializing only individual calls: each process may retain + SQLite/WAL, FTS, or vector-index state across operations. Unknown and + plugin backends are treated conservatively. Only backends whose storage + service is explicitly responsible for cross-process concurrency opt out. + """ + return backend_name.strip().lower() not in _MULTI_PROCESS_WRITER_BACKENDS + + def get_backend_for_palace(palace_path: str, explicit: Optional[str] = None): """Return the resolved backend instance for ``palace_path``.""" return get_backend(resolve_backend_name(palace_path, explicit=explicit)) diff --git a/tests/test_daemon.py b/tests/test_daemon.py index 0cc3a43..28c9dd7 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -1,5 +1,6 @@ import os import subprocess +import sys import threading import time @@ -132,6 +133,40 @@ def test_daemon_http_lifecycle_executes_job(tmp_path, monkeypatch): _stop_server(client, thread, holders) +def test_daemon_holds_local_backend_writer_lease_for_lifetime(tmp_path, monkeypatch): + monkeypatch.setenv("HOME", str(tmp_path / "home")) + client, thread, palace, holders = _start_server( + tmp_path, monkeypatch, lambda kind, payload: {"success": True, "exit_code": 0} + ) + contender = """ +from mempalace.palace import MineAlreadyRunning, mine_palace_lock +import sys +try: + with mine_palace_lock(sys.argv[1]): + raise SystemExit(0) +except MineAlreadyRunning: + raise SystemExit(23) +""" + try: + result = subprocess.run( + [sys.executable, "-c", contender, str(palace)], + check=False, + env=os.environ.copy(), + timeout=10, + ) + assert result.returncode == 23 + finally: + _stop_server(client, thread, holders) + + released = subprocess.run( + [sys.executable, "-c", contender, str(palace)], + check=False, + env=os.environ.copy(), + timeout=10, + ) + assert released.returncode == 0 + + def test_submit_job_uses_client_and_waits(monkeypatch, tmp_path): palace = tmp_path / "palace" palace.mkdir() diff --git a/tests/test_mcp_http_transport.py b/tests/test_mcp_http_transport.py index 8c14f2f..c425772 100644 --- a/tests/test_mcp_http_transport.py +++ b/tests/test_mcp_http_transport.py @@ -235,6 +235,42 @@ def test_read_only_off_exposes_mutating_tools(http_server): assert "mempalace_add_drawer" in names +def test_writable_http_refuses_startup_without_writer_lease(monkeypatch): + monkeypatch.setattr(mcp, "_READ_ONLY", False) + monkeypatch.setattr( + mcp, + "_acquire_mcp_writer_lock", + lambda: (False, "another writer owns the palace"), + ) + monkeypatch.setattr( + mcp, + "_serve_http", + lambda *args: pytest.fail("server must not bind without the writer lease"), + ) + + with pytest.raises(SystemExit) as exc_info: + mcp._run_http_loop() + + assert exc_info.value.code == 2 + + +def test_read_only_http_skips_writer_lease(monkeypatch): + calls = [] + monkeypatch.setattr(mcp, "_READ_ONLY", True) + monkeypatch.setattr( + mcp, + "_acquire_mcp_writer_lock", + lambda: pytest.fail("read-only HTTP must not acquire the writer lease"), + ) + monkeypatch.setattr(mcp, "_refresh_vector_disabled_flag", lambda: None) + monkeypatch.setattr(mcp, "_start_idle_exit_watchdog", lambda: None) + monkeypatch.setattr(mcp, "_serve_http", lambda host, port: calls.append((host, port))) + + mcp._run_http_loop() + + assert calls == [(mcp._args.host, mcp._args.port)] + + @pytest.mark.parametrize( "disconnect_exc", [ diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 5b4eac4..db66a20 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -5072,14 +5072,63 @@ def test_peer_writer_lock_setup_failure_is_cached(monkeypatch): 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 ok_first is False + assert ok_second is False assert calls["count"] == 1 assert mcp_server._MCP_WRITER_LOCK_FAILED is True - assert "continuing without peer-writer protection" in reason_first + assert "refusing mutating tools" in reason_first assert reason_second == reason_first +def test_peer_writer_override_cannot_bypass_local_backend_lock(monkeypatch): + from mempalace import mcp_server, palace + + class _DummyLock: + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + calls = {"count": 0} + + def tracked_lock(palace_path): + calls["count"] += 1 + return _DummyLock() + + monkeypatch.setenv(mcp_server._MCP_ALLOW_PEER_WRITER_ENV, "1") + monkeypatch.setattr(palace, "resolve_backend_name", lambda path: "sqlite_exact") + monkeypatch.setattr(palace, "mine_palace_lock", tracked_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, reason = mcp_server._acquire_mcp_writer_lock() + + assert ok is True + assert reason == "" + assert calls["count"] == 1 + + +def test_peer_writer_override_remains_available_for_remote_backend(monkeypatch): + from mempalace import mcp_server, palace + + monkeypatch.setenv(mcp_server._MCP_ALLOW_PEER_WRITER_ENV, "1") + monkeypatch.setattr(palace, "resolve_backend_name", lambda path: "qdrant") + monkeypatch.setattr( + palace, + "mine_palace_lock", + lambda path: pytest.fail("remote backend should not take the local writer lease"), + ) + 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", "") + + assert mcp_server._acquire_mcp_writer_lock() == (True, "") + + def test_peer_writer_readonly_self_heals_after_peer_exits(monkeypatch): """A server that came up read-only must retry the flock and promote itself to writer once the peer holding the lease exits — no restart required."""