Skip to content

fix: bounded retry before MCP writer lease goes permanently read-only - #1956

Closed
KeilerHirsch wants to merge 1 commit into
MemPalace:developfrom
KeilerHirsch:fix/mcp-writer-lock-grace-period
Closed

fix: bounded retry before MCP writer lease goes permanently read-only#1956
KeilerHirsch wants to merge 1 commit into
MemPalace:developfrom
KeilerHirsch:fix/mcp-writer-lock-grace-period

Conversation

@KeilerHirsch

Copy link
Copy Markdown
Contributor

Problem

_acquire_mcp_writer_lock() decides read-write vs read-only for an entire MCP server process on its very first mutating tool call, and per its own docstring that decision is permanent for the process lifetime once it goes read-only ("Once a server starts read-only it stays read-only for its lifetime; restarting is the safe way to become the writer").

In practice, a peer writer is very often a short-lived mempalace mine kicked off by a SessionStart/ingest hook — not a competing interactive session. If the very first mutating tool call in a session happens to land inside that mine's lock window, the entire session goes read-only for mempalace_kg_add / mempalace_add_drawer / mempalace_diary_write / etc., even though the peer releases the lock moments later. The only way back is a full client reconnect.

I hit this live: a normal search-heavy session raced a background session-ingest mine on its first knowledge-graph write and got permanently locked out for the rest of the session, well after the ingest had already finished.

Fix

Add a short, bounded retry around the first acquisition attempt only: default 5s, polled every 0.5s, configurable via MEMPALACE_MCP_WRITER_LOCK_RETRY_SECONDS. Setting it to 0 reproduces the exact pre-existing immediate-fail behavior (single probe, no sleep).

Scope, deliberately narrow:

  • Only affects the MineAlreadyRunning branch (a transient peer-writer collision). Setup failures (any other exception acquiring the lock) still fail open immediately, unchanged.
  • Does not touch the "once read-only, stays read-only for the rest of the process lifetime" contract — that's still exactly as strict as before. This only widens the window for the initial decision.
  • Does not change mine_palace_lock() itself or its use by the mine CLI path, which is intentionally non-blocking for a different reason (so hook-spawned miners don't pile up as waiting workers).

Tests

4 new tests in tests/test_mcp_server.py, using a fake monotonic clock (no real sleeping in the suite):

  • retry succeeds when the peer releases within the grace window
  • retry window expires and the process still goes permanently read-only (existing contract preserved)
  • RETRY_SECONDS=0 reproduces the old single-probe, no-sleep behavior exactly
  • env var parsing (default / valid float / negative clamps to 0 / invalid falls back to default)

Full suite: pytest tests/test_mcp_server.py tests/test_palace_locks.py → 280 passed, 0 failed, 3 skipped (pre-existing, unrelated). ruff check + ruff format --check clean on both touched files.

Related

Not a fix for #1908 (that was the startup connection-refusal / compactor path) — this is a separate, narrower issue in the peer-writer guard added for #1818.

_acquire_mcp_writer_lock() decides read-write vs read-only for a whole
MCP server process on its very first mutating tool call, and that
decision is permanent for the process lifetime (by design, per its own
docstring) once it goes read-only.

A peer writer is very often a short-lived `mempalace mine` kicked off
by a SessionStart/ingest hook, not a competing interactive session. If
the first mutating call happens to land inside that mine's lock
window, the whole session goes read-only for tools like
mempalace_kg_add/add_drawer/diary_write even though the peer releases
the lock a moment later -- the only way back is a full client
reconnect.

Add a short, bounded retry (default 5s, polled every 0.5s) around the
first acquisition attempt, configurable via
MEMPALACE_MCP_WRITER_LOCK_RETRY_SECONDS (0 reproduces the exact
pre-existing immediate-fail behavior). Only widens the window for the
initial decision -- the "once read-only, stays read-only" contract for
the rest of the process lifetime is unchanged, and setup failures
(non-MineAlreadyRunning exceptions) still fail open immediately as
before.

4 new tests cover: retry succeeds when the peer releases in time,
retry window expires and the process still goes permanently read-only,
retry=0 preserves the old single-probe behavior exactly, and env-var
parsing. Full suite (tests/test_mcp_server.py + test_palace_locks.py):
280 passed, 0 failed, 3 skipped.
@KeilerHirsch

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #1960, which removes the sticky read-only latch entirely: _acquire_mcp_writer_lock now re-attempts the non-blocking flock on every mutating call, so a server that starts read-only self-heals into the writer the moment the peer exits.

I verified #1960 also covers the scenario that motivated this PR (first mutating call colliding with a short-lived session-ingest mine): the call that hits the collision window is refused transiently, and the next mutating call after the mine exits promotes the server to writer — no restart, no permanently read-only session. A bounded retry on top of the per-call re-attempt would actually regress the legitimate peer-writer case (every refused write would stall for the grace period), so this approach is obsolete. Thanks to @Evgen197310 for the cleaner fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant