Skip to content

fix(lock): wait before failing palace writer lock - #1978

Closed
fatkobra wants to merge 2 commits into
MemPalace:developfrom
fatkobra:fix/1920-bounded-palace-lock-wait
Closed

fix(lock): wait before failing palace writer lock#1978
fatkobra wants to merge 2 commits into
MemPalace:developfrom
fatkobra:fix/1920-bounded-palace-lock-wait

Conversation

@fatkobra

@fatkobra fatkobra commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes and Fixes #1920 by making mine_palace_lock() bounded-wait instead of immediate fail-fast.

The previous behavior raised MineAlreadyRunning immediately whenever a transient mine/hook/MCP holder had the palace lock. That turns ordinary overlap into lost writes and retry storms.

This PR changes the lock behavior to:

  • wait up to MEMPALACE_MINE_PALACE_LOCK_WAIT_SECONDS, default 30s;
  • retry at MEMPALACE_MINE_PALACE_LOCK_POLL_SECONDS, default 250ms;
  • keep process-wide re-entrancy for the current process;
  • remove stale lock files only when the recorded holder PID is no longer alive;
  • refuse to steal locks from a genuinely alive external writer;
  • raise a timeout-specific MineAlreadyRunning message after the bounded wait expires.

Why this shape

The issue proposed bounded wait plus liveness checking. This implements the safe part of liveness recovery: dead-PID cleanup. It deliberately does not steal an alive-but-idle process because local Chroma/HNSW writes remain unsafe under concurrent multi-process access.

How to test

Added tests/test_palace_lock_wait.py covering:

  • retrying a transient holder;
  • timeout behavior for an alive holder;
  • conservative dead-holder detection.

Run:

    python3 -m ruff format mempalace/palace.py tests/test_palace_lock_wait.py
    python3 -m ruff check mempalace/palace.py tests/test_palace_lock_wait.py
    python3 -m pytest tests/test_palace_lock_wait.py -q
    python -m pytest tests/ -v

Checklist

  • Tests pass (python -m pytest tests/ -v)
  • No hardcoded paths
  • Linter passes (ruff check .)

@mvalentsev

Copy link
Copy Markdown
Contributor

@fatkobra the acquired path never releases the flock or closes the file: the old outer finally did LOCK_UN plus lf.close(), the new one only closes when not acquired. So the first successful write leaks a locked fd for the life of the process; the process's own next acquire then conflicts with that fd, sees its own PID as holder (_holder_pid_is_dead returns False for self) and waits the full 30s before raising, and every other process is starved until the holder exits. That's the exact symptom this PR is trying to fix.

The dead-PID remove is racy too: a dead holder's flock is already released by the OS, so the file can be locked as-is and the stale identity just gets overwritten by the next _write_lock_holder. Removing by pathname can unlink the file under a contender that acquired between the holder read and the remove, leaving two writers on different inodes.

Most of the CI failures are existing tests pinning the documented fail-fast contract ("exit cleanly instead of piling up as a waiting worker"), so default-on waiting is a behavior change for every caller, including hook-spawned mines. That trade-off is an open discussion in #1888, and #1826 added the opt-in queued-writes daemon for it. This also rewrites the same lines as #1971 and your #1977, so these can't land independently. (The 3.9 collection errors: int | None in _holder_pid_from_message, same fix as your second commit on #1977.)

@fatkobra

fatkobra commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @mvalentsev — I agree with this review.

I’m going to stop pushing fixes to this PR in its current form. The patch changes mine_palace_lock() from fail-fast to default waiting, and the existing lock tests are clearly pinning fail-fast as the safety contract for direct Chroma/MCP writers. The Windows hang and earlier macOS failures line up with that: callers that should raise MineAlreadyRunning are instead waiting and sometimes entering after the holder exits.

I also agree that removing the lock file by pathname is unsafe because it can split contenders across different inodes. And the acquired-path release bug means this PR can create the same starvation/self-deadlock symptom it is trying to fix.

I’ll close this rather than keep patching around CI. The safer direction for #1920 seems to be queueing through the existing opt-in daemon path, not changing the global lock semantics for every caller.

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.

mine_palace_lock is non-blocking with no liveness check — idle/long holders starve all writers and self-deadlock with MCP server lifetime hold

2 participants