fix: address backend ownership edge cases
This commit is contained in:
parent
8a112a88ef
commit
524c980a17
|
|
@ -123,8 +123,10 @@ index state between calls.
|
|||
It may therefore coexist for reads; mutating tools refuse while another
|
||||
process owns the lease and reopen writable storage after that owner exits.
|
||||
- Read-only MCP HTTP may coexist with the writer.
|
||||
- Read-only `sqlite_exact` clients use a `mode=ro`, `query_only` connection
|
||||
and skip schema, WAL, FTS, migration, and metadata initialization.
|
||||
- Read-only `sqlite_exact` clients use an immutable connection for a clean
|
||||
checkpointed database, or `mode=ro` when an active writer's complete WAL
|
||||
sidecar pair must remain visible. Both paths enable `query_only` and skip
|
||||
schema, WAL, FTS, migration, and metadata initialization.
|
||||
- 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.
|
||||
|
|
@ -134,8 +136,9 @@ index state between calls.
|
|||
|
||||
`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.
|
||||
remote service backends (`qdrant`, `pgvector`, and Milvus server/Zilliz Cloud)
|
||||
that coordinate concurrent clients themselves. Milvus Lite remains protected
|
||||
as local file-backed storage.
|
||||
|
||||
Do not delete or unlink a live palace lock to recover ownership. Stop the
|
||||
owning process cleanly; the operating system releases its lock automatically.
|
||||
|
|
|
|||
|
|
@ -76,8 +76,11 @@ def _utcnow() -> str:
|
|||
return datetime.now(timezone.utc).isoformat()
|
||||
|
||||
|
||||
def _is_server_uri(uri: str) -> bool:
|
||||
normalized = uri.lower()
|
||||
def milvus_uri_is_server(uri: Optional[str]) -> bool:
|
||||
"""Return whether ``uri`` targets service-managed Milvus storage."""
|
||||
if not uri:
|
||||
return False
|
||||
normalized = uri.strip().lower()
|
||||
return normalized.startswith(("http://", "https://", "tcp://", "grpc://"))
|
||||
|
||||
|
||||
|
|
@ -358,7 +361,7 @@ class MilvusCollection(BaseCollection):
|
|||
if score is None:
|
||||
return 1.0
|
||||
value = float(score)
|
||||
if self._config.uri and _is_server_uri(self._config.uri):
|
||||
if milvus_uri_is_server(self._config.uri):
|
||||
return 1.0 - max(-1.0, min(1.0, value))
|
||||
return value
|
||||
|
||||
|
|
@ -976,7 +979,7 @@ class MilvusBackend(BaseBackend):
|
|||
collection_name: str,
|
||||
config: _MilvusConfig,
|
||||
) -> str:
|
||||
if config.uri and not _is_server_uri(config.uri) and not config.namespace:
|
||||
if config.uri and not milvus_uri_is_server(config.uri) and not config.namespace:
|
||||
return _slug(collection_name)
|
||||
prefix = self._remote_collection_prefix(palace=palace, config=config)
|
||||
return f"{prefix}_{_slug(collection_name)}"
|
||||
|
|
@ -1059,7 +1062,7 @@ class MilvusBackend(BaseBackend):
|
|||
client = self._clients.get(config)
|
||||
if client is not None:
|
||||
return client
|
||||
if config.uri and not _is_server_uri(config.uri):
|
||||
if config.uri and not milvus_uri_is_server(config.uri):
|
||||
parent = os.path.dirname(os.path.abspath(config.uri)) or "."
|
||||
os.makedirs(parent, exist_ok=True)
|
||||
try:
|
||||
|
|
@ -1089,7 +1092,7 @@ class MilvusBackend(BaseBackend):
|
|||
pass
|
||||
marker_path = self._marker_path(palace.local_path)
|
||||
local_db_exists = bool(
|
||||
config.uri and not _is_server_uri(config.uri) and os.path.exists(config.uri)
|
||||
config.uri and not milvus_uri_is_server(config.uri) and os.path.exists(config.uri)
|
||||
)
|
||||
if os.path.isfile(marker_path):
|
||||
self._validate_marker_target(palace, config)
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ import numpy as np
|
|||
|
||||
from .base import (
|
||||
BackendClosedError,
|
||||
BackendError,
|
||||
BaseBackend,
|
||||
BaseCollection,
|
||||
CollectionNotInitializedError,
|
||||
|
|
@ -884,6 +885,31 @@ class SQLiteExactBackend(BaseBackend):
|
|||
def _db_path(palace_path: str) -> str:
|
||||
return os.path.join(palace_path, _DB_FILENAME)
|
||||
|
||||
@staticmethod
|
||||
def _connect_read_only(db_path: str) -> sqlite3.Connection:
|
||||
"""Open without creating WAL files while preserving an active WAL."""
|
||||
wal_exists = os.path.isfile(f"{db_path}-wal")
|
||||
shm_exists = os.path.isfile(f"{db_path}-shm")
|
||||
if wal_exists != shm_exists:
|
||||
raise BackendError(
|
||||
"sqlite_exact read-only open found an incomplete WAL sidecar set; "
|
||||
"open the palace after its writer exits cleanly or restore both "
|
||||
"the -wal and -shm files"
|
||||
)
|
||||
|
||||
db_uri = Path(db_path).resolve().as_uri()
|
||||
if wal_exists:
|
||||
# An active writer's uncheckpointed rows live in the WAL. With both
|
||||
# sidecars already present, mode=ro can read them without creating
|
||||
# filesystem state, including on a read-only mount.
|
||||
db_uri = f"{db_uri}?mode=ro"
|
||||
else:
|
||||
# A clean WAL-mode database would otherwise make SQLite create new
|
||||
# -wal/-shm files while connecting. Immutable mode is safe here
|
||||
# because there is no WAL whose contents could be hidden.
|
||||
db_uri = f"{db_uri}?mode=ro&immutable=1"
|
||||
return sqlite3.connect(db_uri, uri=True, check_same_thread=False)
|
||||
|
||||
def _connect(self, palace_path: str, create: bool, *, read_only: bool = False):
|
||||
if self._closed:
|
||||
raise BackendClosedError("SQLiteExactBackend has been closed")
|
||||
|
|
@ -913,12 +939,7 @@ class SQLiteExactBackend(BaseBackend):
|
|||
if cached is not None and not cached.closed:
|
||||
return cached
|
||||
if read_only:
|
||||
db_uri = f"{Path(db_path).resolve().as_uri()}?mode=ro"
|
||||
conn = sqlite3.connect(
|
||||
db_uri,
|
||||
uri=True,
|
||||
check_same_thread=False,
|
||||
)
|
||||
conn = self._connect_read_only(db_path)
|
||||
else:
|
||||
conn = sqlite3.connect(db_path, check_same_thread=False)
|
||||
try:
|
||||
|
|
|
|||
|
|
@ -355,7 +355,15 @@ def backend_requires_single_writer(backend_name: str) -> bool:
|
|||
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
|
||||
normalized = backend_name.strip().lower()
|
||||
if normalized == "milvus":
|
||||
# Only embedded Milvus Lite is local single-writer storage. A remote
|
||||
# Milvus server or Zilliz Cloud coordinates concurrent clients itself.
|
||||
from .backends.milvus import milvus_uri_is_server
|
||||
from .config import MempalaceConfig
|
||||
|
||||
return not milvus_uri_is_server(MempalaceConfig().milvus_uri)
|
||||
return normalized not in _MULTI_PROCESS_WRITER_BACKENDS
|
||||
|
||||
|
||||
def get_backend_for_palace(palace_path: str, explicit: Optional[str] = None):
|
||||
|
|
|
|||
|
|
@ -5,7 +5,36 @@ import chromadb
|
|||
from _chroma_palace_helper import make_minimal_chroma_sqlite
|
||||
|
||||
from mempalace.backends import CollectionNotInitializedError, PalaceNotFoundError
|
||||
from mempalace.palace import _open_collection_or_explain, get_collection
|
||||
from mempalace.palace import (
|
||||
_open_collection_or_explain,
|
||||
backend_requires_single_writer,
|
||||
get_collection,
|
||||
)
|
||||
|
||||
|
||||
def test_backend_writer_ownership_distinguishes_milvus_lite_from_server(tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
|
||||
monkeypatch.delenv("MEMPALACE_MILVUS_URI", raising=False)
|
||||
assert backend_requires_single_writer("milvus") is True
|
||||
|
||||
monkeypatch.setenv("MEMPALACE_MILVUS_URI", str(tmp_path / "milvus.db"))
|
||||
assert backend_requires_single_writer("milvus") is True
|
||||
|
||||
for uri in (
|
||||
"https://zilliz.example",
|
||||
"http://milvus.example:19530",
|
||||
"tcp://milvus.example:19530",
|
||||
"grpc://milvus.example:19530",
|
||||
):
|
||||
monkeypatch.setenv("MEMPALACE_MILVUS_URI", uri)
|
||||
assert backend_requires_single_writer("milvus") is False
|
||||
|
||||
|
||||
def test_backend_writer_ownership_remains_conservative_for_unknown_backend():
|
||||
assert backend_requires_single_writer("plugin_backend") is True
|
||||
assert backend_requires_single_writer("qdrant") is False
|
||||
assert backend_requires_single_writer("pgvector") is False
|
||||
|
||||
|
||||
def _capture():
|
||||
|
|
|
|||
|
|
@ -402,23 +402,84 @@ def test_sqlite_exact_read_only_open_skips_schema_init_and_refuses_writes(tmp_pa
|
|||
|
||||
db_path = tmp_path / "sqlite_exact.sqlite3"
|
||||
before = db_path.read_bytes()
|
||||
read_only = backend.get_collection(
|
||||
palace=palace,
|
||||
collection_name="mempalace_drawers",
|
||||
create=False,
|
||||
options={"read_only": True},
|
||||
)
|
||||
assert not (tmp_path / "sqlite_exact.sqlite3-wal").exists()
|
||||
assert not (tmp_path / "sqlite_exact.sqlite3-shm").exists()
|
||||
db_path.chmod(0o400)
|
||||
tmp_path.chmod(0o500)
|
||||
try:
|
||||
read_only = backend.get_collection(
|
||||
palace=palace,
|
||||
collection_name="mempalace_drawers",
|
||||
create=False,
|
||||
options={"read_only": True},
|
||||
)
|
||||
|
||||
assert read_only.count() == 1
|
||||
assert read_only._handle.read_only is True
|
||||
assert read_only._handle.conn.execute("PRAGMA query_only").fetchone()[0] == 1
|
||||
with pytest.raises(sqlite3.OperationalError):
|
||||
read_only.add(ids=["b"], documents=["blocked"], metadatas=[{}], embeddings=[[1, 0]])
|
||||
|
||||
backend.close_palace(palace)
|
||||
assert read_only.count() == 1
|
||||
assert read_only._handle.read_only is True
|
||||
assert read_only._handle.conn.execute("PRAGMA query_only").fetchone()[0] == 1
|
||||
with pytest.raises(sqlite3.OperationalError):
|
||||
read_only.add(
|
||||
ids=["b"],
|
||||
documents=["blocked"],
|
||||
metadatas=[{}],
|
||||
embeddings=[[1, 0]],
|
||||
)
|
||||
assert not (tmp_path / "sqlite_exact.sqlite3-wal").exists()
|
||||
assert not (tmp_path / "sqlite_exact.sqlite3-shm").exists()
|
||||
finally:
|
||||
tmp_path.chmod(0o700)
|
||||
db_path.chmod(0o600)
|
||||
backend.close_palace(palace)
|
||||
assert db_path.read_bytes() == before
|
||||
|
||||
|
||||
def test_sqlite_exact_read_only_open_sees_active_writer_wal(tmp_path):
|
||||
writer_backend, writer = _collection(tmp_path)
|
||||
palace = PalaceRef(id=str(tmp_path), local_path=str(tmp_path))
|
||||
writer.add(ids=["wal-row"], documents=["uncheckpointed"], metadatas=[{}], embeddings=[[1, 0]])
|
||||
|
||||
db_path = tmp_path / "sqlite_exact.sqlite3"
|
||||
wal_path = tmp_path / "sqlite_exact.sqlite3-wal"
|
||||
shm_path = tmp_path / "sqlite_exact.sqlite3-shm"
|
||||
assert wal_path.is_file()
|
||||
assert shm_path.is_file()
|
||||
before = {path: path.read_bytes() for path in (db_path, wal_path, shm_path)}
|
||||
for path in before:
|
||||
path.chmod(0o400)
|
||||
tmp_path.chmod(0o500)
|
||||
try:
|
||||
reader_code = """
|
||||
import sys
|
||||
from mempalace.backends import PalaceRef
|
||||
from mempalace.backends.sqlite_exact import SQLiteExactBackend
|
||||
|
||||
backend = SQLiteExactBackend()
|
||||
palace = PalaceRef(id=sys.argv[1], local_path=sys.argv[1])
|
||||
reader = backend.get_collection(
|
||||
palace=palace,
|
||||
collection_name="mempalace_drawers",
|
||||
create=False,
|
||||
options={"read_only": True},
|
||||
)
|
||||
print(reader.get(ids=["wal-row"]).documents[0])
|
||||
backend.close_palace(palace)
|
||||
"""
|
||||
result = subprocess.run(
|
||||
[sys.executable, "-c", reader_code, str(tmp_path)],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
)
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert result.stdout.strip() == "uncheckpointed"
|
||||
assert {path: path.read_bytes() for path in before} == before
|
||||
finally:
|
||||
tmp_path.chmod(0o700)
|
||||
for path in before:
|
||||
path.chmod(0o600)
|
||||
writer_backend.close_palace(palace)
|
||||
|
||||
|
||||
def test_sqlite_exact_direct_write_contends_with_palace_owner(tmp_path, monkeypatch):
|
||||
from mempalace.palace import MineAlreadyRunning
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue