fix(storage): enforce local single-writer ownership
This commit is contained in:
parent
f852ef98fa
commit
2d8fc9e66d
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
[
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
Loading…
Reference in New Issue