fix(storage): enforce local single-writer ownership

[igorls]

Summary

• enforce one process-lifetime writer owner for file-backed and unknown backends
• make the daemon hold the palace lease for its full lifetime
• make writable MCP HTTP acquire ownership before binding and refuse startup if ownership is unavailable
• keep read-only MCP HTTP and MCP stdio reads available beside the active writer
• document safe routing and recovery without unlinking or reclaiming a live lock

Root cause

The reported MCP HTTP + daemon serve + external mine topology could create multiple independent long-lived clients for the same local SQLite-backed palace:

• the daemon only participated in per-operation locking and did not own a lifetime lease
• writable MCP HTTP acquired its lease lazily on the first mutation rather than before startup
• MEMPALACE_MCP_ALLOW_PEER_WRITER=1 bypassed protection for local file-backed backends
• failures to establish the MCP lock mechanism failed open

Serializing individual calls is insufficient when separate processes retain SQLite/WAL, FTS, or vector-index state between operations.

Behavior after this change

• chroma, sqlite_exact, Milvus Lite, and unknown/plugin backends conservatively require one writer process per palace
• qdrant and pgvector retain service-managed multi-process concurrency
• a writable daemon owns the palace lock until shutdown cleanup completes
• writable MCP HTTP exits with status 2 before binding when it cannot establish ownership
• MCP stdio remains available for reads and retries ownership after the active writer exits
• read-only MCP HTTP may coexist with the writer
• lock setup failures refuse writes instead of continuing unprotected
• the peer-writer override cannot bypass safety for local backends

No unsafe lock unlinking or reclamation is introduced. Operators should stop the owning process cleanly and perform integrity or repair work offline.

Validation

uv run pytest tests/test_daemon.py tests/test_mcp_server.py tests/test_mcp_http_transport.py tests/test_palace.py tests/test_palace_locks.py tests/test_sqlite_exact_backend.py -q
393 passed in 25.97s

Focused Ruff checks and git diff --check also pass.

Fixes #2045

[Copilot]

Pull request overview

This PR hardens MemPalace’s local-backend safety guarantees by enforcing a single process-lifetime writer owner for file-backed (and unknown) backends, preventing long-lived co-writer processes from corrupting SQLite/WAL/FTS/vector state.

Changes:

• Introduces a backend-level policy (backend_requires_single_writer) to conservatively require single-writer ownership for local/unknown backends while allowing service-managed concurrency for qdrant/pgvector.
• Makes the daemon acquire and hold the palace writer lease for its full lifetime, refusing startup if another writer already owns it.
• Makes writable MCP HTTP acquire the writer lease before binding, refusing startup early when ownership can’t be established; keeps read-only modes available alongside the active writer.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
mempalace/palace.py	Adds backend classification helper to determine when single-writer enforcement is required.
mempalace/mcp_server.py	Tightens writer-lease acquisition semantics, disallows “fail open” writes, and enforces pre-bind ownership for writable HTTP.
mempalace/daemon.py	Acquires and holds the palace lease across the daemon’s lifetime for single-writer backends; improves startup refusal behavior and state restoration.
tests/test_mcp_server.py	Updates/extends tests for refusal-on-lock-failure and for peer-writer override behavior (local vs remote backends).
tests/test_mcp_http_transport.py	Adds coverage ensuring writable HTTP refuses startup without a writer lease while read-only HTTP skips lease acquisition.
tests/test_daemon.py	Adds a lifecycle test verifying the daemon holds and releases the writer lease appropriately.
docs/write-routing-policy.md	Documents the single-writer model and safe operational routing/recovery for local backends.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d8fc9e66d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Hold the writer lease until the active job stops

When a mine or sync job runs longer than SHUTDOWN_DRAIN_SECONDS, _drain_and_cleanup returns while the daemon worker is still executing, and this line releases the writer lease immediately. A new daemon, MCP server, or direct writer can then acquire the lease and mutate the same local backend concurrently with the timed-out worker; this is deterministic when run_server is hosted in an existing process and also creates a shutdown race in the normal daemon process. Keep ownership until the worker has actually stopped or until process termination so shutdown cannot expose a still-active writer.

AGENTS.md reference: AGENTS.md:L23-L23

Useful? React with 👍 / 👎.

[fatkobra]

Thanks for tackling this. The process-lifetime ownership direction is the
right one, and acquiring writable MCP HTTP ownership before binding is a
meaningful improvement.

I am requesting changes because the current patch does not yet enforce the
stated single-writer invariant across all local-backend access paths.

1. Keep daemon ownership while a timed-out worker is still alive

The existing Codex comment is correct.

_drain_and_cleanup() waits only for SHUTDOWN_DRAIN_SECONDS. If the worker
is still executing after that bounded join, run_server() nevertheless
closes writer_lease. A new daemon, MCP process, or direct writer can then
acquire the palace lock while the original worker continues mutating the
backend.

Please retain ownership until the worker has actually stopped or until the
process terminates.

The regression test should block a worker beyond the drain timeout, initiate
shutdown, and verify that a contender still receives MineAlreadyRunning
until the original worker truly exits.

2. The closet-regen-full path from #2045 still bypasses the palace lease

closet_llm.regenerate_closets() opens the drawers and closets collections
directly and performs its purge/upsert cycle under only:

with mine_lock(source):

It never contends for mine_palace_lock(palace_path).

That means the exact long-running closet-regeneration writer identified in
#2045 can still run concurrently with the daemon or writable MCP owner on a
local backend. The per-source lock prevents two operations on one source
from interleaving, but it does not establish exclusive ownership of the
palace.

Please make the complete non-dry-run regeneration operation participate in
the same palace ownership policy. The lease should be acquired before
opening the local-backend collections and before making potentially
expensive LLM calls, so a conflicting process fails immediately rather than
running for an extended period before reaching the writes.

mine_palace_lock is process-wide and re-entrant, so this can safely execute
inside the daemon or MCP owner process without conflicting with itself.

A regression test should:

1. hold the palace lease in process A;
2. invoke regenerate_closets() for sqlite_exact in process B;
3. verify that process B refuses before the LLM, delete, or upsert path is
   reached.

Please also audit other direct local-backend write entry points rather than
assuming that daemon and MCP startup ownership covers every writer.

3. Read-only MCP is not storage-read-only for sqlite_exact

The PR allows read-only HTTP and stdio readers to coexist with the active
writer, but an actual read eventually calls _get_collection(create=False).

For sqlite_exact, create=False still opens a normal read/write SQLite
connection and always executes _init_schema(). That includes
PRAGMA journal_mode=WAL, DDL, an FTS-table creation attempt, a metadata
upsert, and a commit.

Therefore, a process presented as read-only can still create a write
transaction while another process owns the lifetime writer lease.

The new read-only HTTP test only proves that lease acquisition is skipped
and the HTTP server starts. It does not execute a real read or open the
backend.

Please either:

• add a genuinely read-only backend-open path for sqlite_exact, using a
  read-only SQLite URI or query-only connection and skipping schema
  initialization, migrations, and metadata writes; or
• do not permit read-only sqlite_exact processes to coexist with the
  writer until such an open path exists.

The regression coverage should execute a real read tool against
sqlite_exact, not only mock _serve_http, and prove that opening the
collection performs no schema or metadata mutation.

Once these ownership gaps and the shutdown race are addressed, the overall
approach looks sound.