fix(palace): reap orphaned per-source-file mine locks - #2200
Merged
Conversation
igorls
approved these changes
Aug 11, 2026
igorls
left a comment
Member
There was a problem hiding this comment.
Wave 2 for 3.7.0: LGTM after review. Merging into develop for the release train.
_cleanup_mine_lock_file reclaims a lock correctly on the happy path (see its own docstring for the flock-based rendezvous safety it already handles) — but only for the specific lock a mine_lock context manager just released. A process that dies before reaching its own finally block (SIGKILL, force-quit, host crash) never runs that cleanup, and nothing else in the codebase later revisits that lock file. Found in the wild: one long-lived installation had 5,636 stale lock files in ~/.mempalace/locks/, the oldest several months old, none held by any live process (confirmed via lsof before cleanup). This is distinct from the MemPalace#1264 lock-holder-diagnostics fix (identifies who holds a live lock) and the MemPalace#1299 mcp_server embedding-function fix (unrelated code path) — neither addresses orphan reclamation, and the 2026-07-10 outage postmortem comment in mcp_server.py's stdio loop covers graceful client disconnection, not abrupt process death. Adds reap_stale_mine_locks(), which reuses _cleanup_mine_lock_file itself for the actual removal — same nonblocking-flock-reacquire safety mechanism, same Windows/POSIX handling already tested in this file, no duplicated locking logic. A lock is only ever removed after this process re-acquires it, so anything genuinely held by a live process is left untouched regardless of age. Wired into mine_lock() via a throttled opportunistic call (_maybe_reap_stale_mine_locks, at most once per 15 minutes) rather than a new background thread, scheduled task, or CLI surface — it piggybacks on the natural cadence of mining rather than adding new infrastructure. mine_palace_*.lock (the newer per-palace lock added for the MemPalace#974/MemPalace#965 fan-out fix) is explicitly skipped — it has its own lifecycle and holder-identity tracking and doesn't have this failure mode. Tests: 6 new cases in test_palace_locks.py covering removal of a genuinely stale+unheld lock, preservation of a young lock regardless of hold state, the core safety property (a lock held by another process is never removed even when backdated past the age threshold), skipping mine_palace_*-prefixed locks, a missing-lock-dir no-op, and the throttle itself. Full existing test_palace_locks.py suite (19 tests) passes unchanged. Broader tests/ -k 'palace or mine' run clean (730 passed) aside from two pre-existing failures confirmed unrelated and present on an unmodified checkout (test_hnsw_capacity.py SQLite WAL signature caching, test_repair.py FTS5 shadow-table write restriction — both environment/SQLite-build-specific, neither touches locking).
igorls
force-pushed
the
fix/reap-orphaned-mine-locks
branch
from
August 11, 2026 10:54
2e0ab20 to
27212e5
Compare
pull Bot
pushed a commit
to FaZios/mempalace
that referenced
this pull request
Aug 11, 2026
The MemPalace#2200 reap tests only monkeypatched HOME. On Windows expanduser("~") reads USERPROFILE, so the reaper scanned the real home and the suite failed on test-windows after Wave 2. Share _isolate_home() that sets both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_cleanup_mine_lock_filereclaims a lock correctly on the happy path — but only for the specific lock amine_lockcontext manager just released. A process that dies before reaching its ownfinallyblock (SIGKILL, force-quit, host crash) never runs that cleanup, and nothing else in the codebase later revisits the lock file.Found in the wild: one long-lived installation had 5,636 stale lock files in
~/.mempalace/locks/, the oldest several months old, none held by any live process (confirmed vialsofbefore manual cleanup).I checked this wasn't already covered before writing a fix:
mine: silent exit when concurrent writer holds chroma lock; should detect livemcp_serverand back off with clear error #1264 (lock-holder-diagnostics, merged) — identifies who holds a live lock and fails loudly on contention. Doesn't reclaim orphans.mcp_server.py's stdio loop — covers graceful client disconnection (stdin EOF/error → clean shutdown → the process's own lock-release runs). Doesn't cover abrupt process death, where that shutdown code never gets to run at all.None of the three address orphan reclamation for a process that's simply gone.
Fix
reap_stale_mine_locks()reuses_cleanup_mine_lock_fileitself for the actual removal — same nonblocking-flock-reacquire safety mechanism, same Windows/POSIX handling already tested in this file, no duplicated locking logic. A lock is only ever removed after this process re-acquires it via the existing flock, so anything genuinely held by a live process is left untouched regardless of how old it looks by mtime — age is a courtesy throttle (avoids racing a lock that was just released and may still be mid-rendezvous with a waiter on the same pathname), not the safety mechanism.Wired into
mine_lock()via a throttled opportunistic call (_maybe_reap_stale_mine_locks, at most once per 15 minutes) rather than a new background thread, scheduled task, or CLI surface — it piggybacks on the natural cadence of mining rather than adding new infrastructure.mine_palace_*.lock(the newer per-palace lock from the #974/#965 fan-out fix) is explicitly skipped — separate lifecycle, separate holder-identity tracking, not affected by this failure mode.Testing
6 new cases in
test_palace_locks.py:multiprocessing) is never removed, even when its mtime is backdated past the age thresholdmine_palace_*-prefixed locksFull existing
test_palace_locks.pysuite (19 tests) passes unchanged. Broadertests/ -k 'palace or mine'run clean — 730 passed, 10 skipped — aside from two pre-existing failures I confirmed are unrelated and present on an unmodifieddevelopcheckout before touching anything (test_hnsw_capacity.pySQLite WAL signature caching,test_repair.pyFTS5 shadow-table write restriction — both environment/SQLite-build-specific, neither touches locking).ruff checkclean on both modified files.Compatibility
Purely additive — no existing function signatures changed, no new required config, no new CLI surface. The one behavioral change is that
mine_lock()now does a cheap, throttled, best-effort directory scan at most once per 15 minutes; failures in that scan are swallowed (logged at debug level) and never propagate to the caller's actual mine operation.