fix(palace): reap orphaned per-source-file mine locks
_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 #1264 lock-holder-diagnostics fix (identifies who holds a live lock) and the #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 #974/#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).
This commit is contained in:
parent
7eaa3bc3de
commit
27212e5c62
|
|
@ -12,6 +12,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
|
|||
|
||||
- **Embeddings via any OpenAI-compatible `/v1/embeddings` endpoint.** New `embedding_model: "openai-compat"` option computes embeddings on a server (LM Studio, llama.cpp, vLLM, Ollama's OpenAI shim, or a self-hosted endpoint) instead of a local ONNX model — useful for larger/multilingual embedders such as Qwen3-Embedding, or GPU offload. New `OpenAICompatEmbeddingFunction` in [`mempalace/embedding.py`](mempalace/embedding.py) speaks the standard `/v1/embeddings` protocol over stdlib `urllib` (no new dependency), batches requests, re-sorts the response by `index`, and L2-normalizes for the cosine collection. Endpoint settings are resolved by `MempalaceConfig` as a single source of truth — `embedding_api_url` / `embedding_api_model` / `embedding_api_key` in `config.json`, each overridable via the matching `MEMPALACE_EMBEDDING_API_*` env var. The embedding function's `name()` encodes the model id so changing it forces `mempalace repair rebuild-index` (different vector space). Mirrors the existing `openai-compat` LLM provider naming; stays local when the endpoint is on your machine/LAN. (#1559)
|
||||
|
||||
### Bug Fixes
|
||||
|
||||
- **Orphaned per-source-file mine locks are reaped instead of accumulating forever.** `_cleanup_mine_lock_file` reclaims a lock correctly on the happy path, but only for the specific lock its own `mine_lock` context manager just released — a process killed abruptly (SIGKILL, force-quit, host crash) never reaches that cleanup, and nothing else revisited the file afterward. One long-lived installation was found with 5,636 stale entries in `~/.mempalace/locks/`, the oldest several months old, none held by any live process. `mine_lock` now opportunistically reaps locks older than an hour via the same nonblocking-flock-reacquire safety check `_cleanup_mine_lock_file` already uses, throttled to once per 15 minutes so it costs nothing on the common path. `mine_palace_*.lock` (the newer per-palace lock) is untouched — it has its own lifecycle and holder tracking.
|
||||
|
||||
---
|
||||
|
||||
## [3.7.0] — 2026-08-02
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ import os
|
|||
import re
|
||||
import sys
|
||||
import threading
|
||||
import time
|
||||
from typing import Optional
|
||||
|
||||
from .backends import (
|
||||
|
|
@ -828,6 +829,7 @@ def mine_lock(source_file: str):
|
|||
Prevents multiple agents from mining the same file simultaneously,
|
||||
which causes duplicate drawers when the delete+insert cycle interleaves.
|
||||
"""
|
||||
_maybe_reap_stale_mine_locks()
|
||||
lock_path = _mine_lock_path(source_file)
|
||||
lf = _acquire_mine_lock_file(lock_path)
|
||||
try:
|
||||
|
|
@ -1007,6 +1009,92 @@ def _cleanup_mine_lock_file(lock_path: str) -> None:
|
|||
lf.close()
|
||||
|
||||
|
||||
def reap_stale_mine_locks(*, min_age_seconds: int = 3600) -> tuple[int, int]:
|
||||
"""Best-effort garbage collection for orphaned per-source-file mine locks.
|
||||
|
||||
``_cleanup_mine_lock_file`` reclaims a lock file correctly on the happy
|
||||
path (see its docstring) — but only for the *specific* lock a
|
||||
:func:`mine_lock` context manager just released. A process that dies
|
||||
before reaching its own ``finally`` block (killed, crashed, force-quit,
|
||||
host reboot) never runs that cleanup, and nothing else in this codebase
|
||||
later revisits that lock file. Locks in ``~/.mempalace/locks/`` can
|
||||
accumulate unboundedly over time as a result — one long-lived
|
||||
installation was found with 5,636 stale entries, the oldest several
|
||||
months old, none held by any live process (confirmed via ``lsof``).
|
||||
|
||||
This reuses :func:`_cleanup_mine_lock_file` itself for the actual
|
||||
removal — same nonblocking-flock-reacquire safety mechanism, same
|
||||
Windows/POSIX handling, 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
|
||||
``min_age_seconds``. ``min_age_seconds`` is a courtesy throttle only —
|
||||
it avoids racing a lock that was *just* released and may still be
|
||||
mid-rendezvous with a waiter on the same pathname; it is not a
|
||||
substitute for the flock check, which is what actually makes removal
|
||||
safe.
|
||||
|
||||
Skips ``mine_palace_*.lock`` files — those belong to the newer
|
||||
palace-level :func:`mine_palace_lock` and have their own
|
||||
lifecycle/holder tracking; this targets only the per-source-file locks
|
||||
:func:`mine_lock` creates via :func:`_mine_lock_path`.
|
||||
|
||||
Returns ``(reaped, skipped)`` counts, for logging/testing — callers
|
||||
don't need to act on them.
|
||||
"""
|
||||
lock_dir = os.path.join(os.path.expanduser("~"), ".mempalace", "locks")
|
||||
try:
|
||||
entries = os.listdir(lock_dir)
|
||||
except OSError:
|
||||
return 0, 0
|
||||
|
||||
now = time.time()
|
||||
reaped = 0
|
||||
skipped = 0
|
||||
for name in entries:
|
||||
if not name.endswith(".lock") or name.startswith("mine_palace_"):
|
||||
continue
|
||||
lock_path = os.path.join(lock_dir, name)
|
||||
try:
|
||||
if now - os.path.getmtime(lock_path) < min_age_seconds:
|
||||
continue
|
||||
except OSError:
|
||||
continue
|
||||
_cleanup_mine_lock_file(lock_path)
|
||||
if os.path.exists(lock_path):
|
||||
skipped += 1
|
||||
else:
|
||||
reaped += 1
|
||||
return reaped, skipped
|
||||
|
||||
|
||||
_LOCK_REAP_INTERVAL_SECONDS = 900 # 15 minutes between opportunistic sweeps
|
||||
|
||||
|
||||
def _maybe_reap_stale_mine_locks() -> None:
|
||||
"""Throttled, opportunistic call site for :func:`reap_stale_mine_locks`.
|
||||
|
||||
Runs at most once per ``_LOCK_REAP_INTERVAL_SECONDS``, piggybacking on
|
||||
the natural cadence of mine operations rather than requiring a
|
||||
background thread, a scheduled task, or any new CLI surface. Failures
|
||||
are swallowed — lock maintenance must never be allowed to break an
|
||||
actual mine.
|
||||
"""
|
||||
lock_dir = os.path.join(os.path.expanduser("~"), ".mempalace", "locks")
|
||||
marker = os.path.join(lock_dir, ".last_reap")
|
||||
try:
|
||||
if (
|
||||
os.path.exists(marker)
|
||||
and time.time() - os.path.getmtime(marker) < _LOCK_REAP_INTERVAL_SECONDS
|
||||
):
|
||||
return
|
||||
os.makedirs(lock_dir, exist_ok=True)
|
||||
open(marker, "a").close()
|
||||
os.utime(marker, None)
|
||||
reap_stale_mine_locks()
|
||||
except Exception:
|
||||
logger.debug("Opportunistic mine-lock reap failed", exc_info=True)
|
||||
|
||||
|
||||
class MineAlreadyRunning(RuntimeError):
|
||||
"""Raised when another `mempalace mine` already holds the per-palace lock."""
|
||||
|
||||
|
|
|
|||
|
|
@ -22,7 +22,9 @@ from mempalace.palace import (
|
|||
_write_lock_holder,
|
||||
MineAlreadyRunning,
|
||||
mine_global_lock,
|
||||
mine_lock,
|
||||
mine_palace_lock,
|
||||
reap_stale_mine_locks,
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -439,3 +441,155 @@ def test_holder_set_not_orphaned_by_interrupt_after_mark_held(tmp_path, monkeypa
|
|||
# The flock was freed and no stale hold remains, so the lock is reusable.
|
||||
with mine_palace_lock(palace):
|
||||
pass
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# reap_stale_mine_locks — orphaned per-source-file lock garbage collection
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# mine_lock's own finally-block cleanup (_cleanup_mine_lock_file) only runs
|
||||
# for the specific lock a process just released, and only if that process
|
||||
# reaches its own finally block at all. A process killed abruptly (SIGKILL,
|
||||
# force-quit, host crash) never runs it, and nothing else in the codebase
|
||||
# later revisits that lock file — it orphans permanently. One long-lived
|
||||
# installation was found with 5,636 such orphaned lock files, the oldest
|
||||
# several months old, none held by any live process. These tests cover the
|
||||
# reaper added to reclaim them safely.
|
||||
|
||||
|
||||
def _hold_source_lock(source_file: str, ready_flag: str, release_flag: str) -> int:
|
||||
"""Acquire mine_lock(source_file), signal readiness, wait for release.
|
||||
|
||||
Runs in a child process for true cross-process locking semantics,
|
||||
mirroring _hold_lock above but for the per-source-file mine_lock
|
||||
rather than the per-palace mine_palace_lock.
|
||||
"""
|
||||
with mine_lock(source_file):
|
||||
open(ready_flag, "w").close()
|
||||
for _ in range(500):
|
||||
if os.path.exists(release_flag):
|
||||
return 0
|
||||
time.sleep(0.01)
|
||||
return 0
|
||||
|
||||
|
||||
def test_reap_removes_stale_unlocked_lock(tmp_path, monkeypatch):
|
||||
"""A lock file that's old and held by nobody is safe to remove."""
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
lock_dir = tmp_path / ".mempalace" / "locks"
|
||||
lock_dir.mkdir(parents=True)
|
||||
stale = lock_dir / "0000000000000000.lock"
|
||||
stale.write_bytes(b"")
|
||||
old_time = time.time() - 7200 # 2h old, past the default 1h threshold
|
||||
os.utime(stale, (old_time, old_time))
|
||||
|
||||
reaped, skipped = reap_stale_mine_locks(min_age_seconds=3600)
|
||||
|
||||
assert reaped == 1
|
||||
assert skipped == 0
|
||||
assert not stale.exists()
|
||||
|
||||
|
||||
def test_reap_leaves_recently_touched_lock_alone(tmp_path, monkeypatch):
|
||||
"""A lock younger than the age threshold is left alone even if unheld.
|
||||
|
||||
The flock check is what makes removal *safe*; the age threshold exists
|
||||
only to avoid racing a lock that was just released and may still be
|
||||
mid-rendezvous with a waiter on the same pathname.
|
||||
"""
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
lock_dir = tmp_path / ".mempalace" / "locks"
|
||||
lock_dir.mkdir(parents=True)
|
||||
fresh = lock_dir / "1111111111111111.lock"
|
||||
fresh.write_bytes(b"") # mtime is "now" — well under the threshold
|
||||
|
||||
reaped, skipped = reap_stale_mine_locks(min_age_seconds=3600)
|
||||
|
||||
assert reaped == 0
|
||||
assert fresh.exists()
|
||||
|
||||
|
||||
def test_reap_never_removes_a_lock_held_by_another_process(tmp_path, monkeypatch):
|
||||
"""The core safety property: a lock genuinely held by a live process,
|
||||
however old it looks by mtime, is never removed — the age threshold is
|
||||
a courtesy throttle, the flock check is the actual safety mechanism."""
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
source_file = str(tmp_path / "some_source.py")
|
||||
ready = str(tmp_path / "ready")
|
||||
release = str(tmp_path / "release")
|
||||
|
||||
ctx = _get_mp_context()
|
||||
holder = ctx.Process(target=_hold_source_lock, args=(source_file, ready, release))
|
||||
holder.start()
|
||||
try:
|
||||
for _ in range(500):
|
||||
if os.path.exists(ready):
|
||||
break
|
||||
time.sleep(0.01)
|
||||
assert os.path.exists(ready), "holder failed to acquire lock in time"
|
||||
|
||||
lock_dir = tmp_path / ".mempalace" / "locks"
|
||||
lock_files = list(lock_dir.glob("*.lock"))
|
||||
assert lock_files, "expected the held lock file to exist"
|
||||
# Backdate mtime so it would be a reap candidate by age alone —
|
||||
# the flock held by the child process must still protect it.
|
||||
old_time = time.time() - 7200
|
||||
os.utime(lock_files[0], (old_time, old_time))
|
||||
|
||||
reaped, skipped = reap_stale_mine_locks(min_age_seconds=3600)
|
||||
|
||||
assert reaped == 0
|
||||
assert skipped == 1
|
||||
assert lock_files[0].exists(), "a held lock must never be removed by the reaper"
|
||||
finally:
|
||||
open(release, "w").close()
|
||||
holder.join(timeout=5)
|
||||
assert holder.exitcode == 0
|
||||
|
||||
|
||||
def test_reap_skips_mine_palace_prefixed_locks(tmp_path, monkeypatch):
|
||||
"""mine_palace_*.lock belongs to the newer per-palace lock (mine_palace_lock)
|
||||
with its own lifecycle and holder tracking — this reaper targets only the
|
||||
per-source-file locks mine_lock creates, and must not touch those."""
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
lock_dir = tmp_path / ".mempalace" / "locks"
|
||||
lock_dir.mkdir(parents=True)
|
||||
palace_lock = lock_dir / "mine_palace_deadbeefdeadbeef.lock"
|
||||
palace_lock.write_bytes(b"")
|
||||
old_time = time.time() - 7200
|
||||
os.utime(palace_lock, (old_time, old_time))
|
||||
|
||||
reaped, skipped = reap_stale_mine_locks(min_age_seconds=3600)
|
||||
|
||||
assert reaped == 0
|
||||
assert palace_lock.exists()
|
||||
|
||||
|
||||
def test_reap_missing_lock_dir_is_a_noop(tmp_path, monkeypatch):
|
||||
"""No ~/.mempalace/locks directory yet (fresh install) must not raise."""
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
reaped, skipped = reap_stale_mine_locks()
|
||||
assert (reaped, skipped) == (0, 0)
|
||||
|
||||
|
||||
def test_maybe_reap_is_throttled(tmp_path, monkeypatch):
|
||||
"""The opportunistic call site runs at most once per interval — a stale
|
||||
lock created between two rapid-fire mine_lock calls must survive the
|
||||
second call because the reap itself was skipped, not because reaping
|
||||
failed."""
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
monkeypatch.setattr(palace_mod, "_LOCK_REAP_INTERVAL_SECONDS", 3600)
|
||||
|
||||
with mine_lock(str(tmp_path / "a.py")):
|
||||
pass # first call: no marker exists yet, this call creates it
|
||||
|
||||
lock_dir = tmp_path / ".mempalace" / "locks"
|
||||
stale = lock_dir / "2222222222222222.lock"
|
||||
stale.write_bytes(b"")
|
||||
old_time = time.time() - 7200
|
||||
os.utime(stale, (old_time, old_time))
|
||||
|
||||
with mine_lock(str(tmp_path / "b.py")):
|
||||
pass # second call: marker is fresh, reap should be skipped this time
|
||||
|
||||
assert stale.exists(), "reap should have been throttled on the second mine_lock call"
|
||||
|
|
|
|||
Loading…
Reference in New Issue