The #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.
_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).
_mark_held(palace_key) ran before the try: whose finally runs
_mark_released(). An async exception (SIGINT/KeyboardInterrupt) landing
after _mark_held() and before the try: skips _mark_released(), stranding
the key in the process-wide _palace_lock_keys set while the outer finally
frees the flock. The in-memory hold then outlives the OS lock: a later
re-entrant acquire passes through and writes without the flock while
another process can acquire it, i.e. two writers into one palace.
Move _mark_held() inside the try so it pairs with _mark_released() on
every exit. Add a regression test that injects the interrupt in the
window and asserts the holder set is not stranded.
* fix(palace): process-wide mine_palace_lock re-entrancy for threaded HTTP transport
The MCP HTTP transport (ThreadingHTTPServer) acquires the long-lived
writer-lease on one thread (_acquire_mcp_writer_lock) but dispatches each
write request on a different worker thread. The lock re-entrancy guard was
thread-local, so write handlers (add_drawer/update_drawer) failed to see the
process-held lease, re-acquired the flock, and self-conflicted with
"palace ... is held by PID <self>". Reads worked (no lock); writes over the
HTTP transport were impossible.
Make the re-entrancy record process-wide (pid-tagged, guarded by a
threading.Lock) so a write from any thread of the process that already holds
the lease passes through. Safe: flock is per-process and HTTP writes are
serialized by _HTTP_REQUEST_LOCK. Preserves fork-safety, same-thread nesting
(miner.mine -> ChromaCollection.upsert), and cross-process protection
(MineAlreadyRunning still raised between processes).
Add cross-thread same-process regression test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(palace): reset lock guard on fork to avoid inherited-locked deadlock
Address review (PR #1859): `_palace_lock_guard` is a threading.Lock, so a child
forked while another thread held it would inherit it locked (the holder thread
is gone in the child) and deadlock on the next acquire. Register an
os.register_at_fork(after_in_child=...) handler that replaces the guard with a
fresh unlocked lock and clears state; the child must reacquire the flock anyway.
Guarded by hasattr(os, "register_at_fork") for Windows.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
One-time mechanical reformat so `ruff format --check .` passes under the
newly pinned ruff. Layout only (assert-message parenthesization etc.),
no behavior change. 29 files: 28 under tests/ + 1 tools helper, no core
mempalace/ modules. Produced by `ruff format .`.
The PR reformatted two pre-existing assertions with a newer ruff
(0.5+ `assert X, (\n msg\n)` style) that CI's pinned ruff
(>=0.4.0,<0.5) rejects, failing `ruff format --check .`. Revert those
two unrelated blocks to develop's 0.4.x form; the genuine #1435 fix and
its new regression tests are untouched.
test_palace_locks.py and test_chroma_collection_lock.py spawned child
processes with the ``fork`` start method on POSIX. Under Python 3.13
this deadlocks reliably enough to hang the Linux 3.13 and macOS CI jobs
indefinitely while Linux 3.9 / 3.11 / Windows complete normally.
Root cause: by the time these tests run, the pytest parent process is
multi-threaded — chromadb and onnxruntime both spawn background threads
on import. ``fork`` snapshots the parent's address space into the
child without those threads, so any lock another thread held at fork
time stays locked in the child forever. Python 3.13 widened the window
where Python's own internal threads can be holding locks (hence the new
DeprecationWarning that fired ten times in our local 3.13 run).
macOS hits a related but distinct issue: Apple's CoreFoundation
explicitly forbids fork-without-exec; once anything in the parent has
loaded a CF-using library (ONNX, anything via Objective-C bridges) a
forked child will silently hang the moment it touches the same
library.
Switching to ``spawn`` re-imports modules in the child (~0.5s overhead
per Process — measurable but bounded), which is the standard fix for
both classes of bug. Lock-file semantics are unchanged: ``spawn``
inherits ``os.environ`` (including monkeypatched ``HOME``), which is
all these tests need from the parent.
Locally on Python 3.13: all 14 lock tests pass in 6.58s.
os.path.expanduser("~") reads HOME on POSIX but USERPROFILE on Windows;
the lock-body bound test was monkeypatching HOME only, so on
test-windows the lock file landed in the runner's real ~/.mempalace
and the tmp_path glob found nothing.
Patch USERPROFILE in addition to HOME, and read the body as bytes so
the byte-0 sentinel doesn't trip a UTF-8 decode warning. Assertion
shifts from line-count to size-bound (still detects unbounded growth
across re-acquires).
When a `mempalace mine` collided with another writer (live mcp_server,
another mine, anything taking mine_palace_lock), the operator saw a
generic "another `mempalace mine` is already running" message and the
CLI exited 0 — making the contention invisible to nohup or scripts
checking $?. The reporter ran a `nohup mempalace mine ... & disown`
and got a 200-byte log with only the auto-defaults warning, no clue
that an MCP server was holding the store.
palace.py: the lock file now records the holder's PID + first three
argv tokens on acquire. A failed acquire reads the file and surfaces
"palace <path> is held by PID N (mempalace mcp_server); wait for it
to finish or stop the holder before retrying" in the
MineAlreadyRunning message. Open mode changes from "w" to "a+" so the
prior holder's identity survives long enough to be read.
miner.mine() now lets MineAlreadyRunning propagate. cmd_mine catches
it, prints the holder-aware message to stderr, and exits non-zero so
shell wrappers detect the contention.
Note: this is a behavior change for in-process callers that depended
on miner.mine() silently swallowing MineAlreadyRunning. The silent
swallow was the bug.
Closes#1264
#976 protects `mempalace mine`, but MCP/direct backend writers still call
ChromaCollection.add/upsert/update/delete without the palace lock. This
moves the lock boundary to the Chroma backend seam so all Chroma writes
share the same palace-level serialization, with a re-entrant guard for
miner paths that already hold the lock.
mine_palace_lock(palace_path) gains a per-thread re-entrant guard
(threading.local + pid-tag against fork inheritance) so
ChromaCollection write methods can take the lock without
self-deadlocking when called from inside miner.mine()'s outer hold.
ChromaCollection.__init__ accepts an optional palace_path; when set,
add/upsert/update/delete wrap their underlying chromadb call with
mine_palace_lock(palace_path). palace_path=None preserves the legacy
no-lock behaviour for direct callers and tests. ChromaBackend's
get_collection/create_collection pass palace_path through;
mcp_server._get_collection forwards _config.palace_path so all MCP
write tools inherit the wrapping.
Tests: 5 new in tests/test_chroma_collection_lock.py covering opt-in,
writer-blocks-during-mine, re-entrant-inside-mine, two-process
serialization, and a source-level read-path-not-locked pin. Plus 1 new
+ 1 rewritten in tests/test_palace_locks.py for the re-entrant
semantics. 52 passed in 1.01s including the existing test_backends.py
regression suite.
Refs #1161.
Addresses the two actionable Copilot comments from the 2nd review pass.
tests/test_palace_locks.py (#7, #8)
multiprocessing.get_context("fork") is unavailable on Windows, so the
cross-process tests would crash the Windows CI runner. Added
`_get_mp_context()` that picks "spawn" on Windows and "fork" elsewhere.
Spawn re-imports the module in the child; it inherits os.environ
(including the monkeypatched HOME), which is all these tests need.
mempalace/palace.py (#10)
The per-palace lock key was computed from os.path.abspath(palace_path).
On Windows the filesystem is case-insensitive, so `C:\\Palace` and
`c:\\palace` would hash to different keys and two concurrent mines
could touch the same on-disk palace. Switched to
`os.path.normcase(os.path.realpath(...))` so:
* realpath resolves symlinks and `..` segments
* normcase folds case on Windows (no-op on POSIX)
Testing
pytest tests/test_palace_locks.py tests/test_hooks_cli.py
tests/test_backends.py tests/test_cli.py
→ 98 passed, 0 failed.
Addresses the six Copilot review comments on the initial commit.
1) #6 (critical) — mcp_server.py `_get_collection` bypassed ChromaBackend
The MCP server creates its palace collection directly via
`chromadb.PersistentClient.get_or_create_collection` in `_get_collection`,
not through `ChromaBackend.get_collection`. That path was missing the
`hnsw:num_threads=1` metadata, so the primary crash surface for #974
and #965 was untouched by the original patch. Fixed by passing
`hnsw:num_threads=1` at the mcp_server create site too. Documented
in a code comment that the setting is only honored at creation
time — existing palaces created before this fix still need a
`mempalace nuke` + re-mine to gain the protection.
2) #3 — mine_global_lock over-serialized mines across unrelated palaces
Replaced the single global lock file `mine_global.lock` with a
per-palace lock keyed by `sha256(os.path.abspath(palace_path))`
(`mine_palace_<hash>.lock`). Mines against the same palace still
collapse to a single runner (the correctness boundary), but mines
against *different* palaces are now free to run in parallel.
`mine_global_lock` is kept as a backward-compatible alias for
`mine_palace_lock` so any external callers that imported the
previous name keep working.
3) #1 — hook_precompact swallowed OSError but not subprocess.TimeoutExpired
`subprocess.run(..., timeout=60)` raises `TimeoutExpired` on slow
palaces. The previous `except OSError` clause didn't catch it, so
the hook could raise and fail to emit any JSON decision — leaving
the harness without a block/passthrough signal. Fixed by catching
`(OSError, subprocess.TimeoutExpired)` together and always falling
through to the block decision so the hook reliably emits a response.
4) #2 + #4 — tests
- tests/test_hooks_cli.py: added
`test_precompact_first_two_attempts_block`,
`test_precompact_passes_through_after_cap`, and
`test_precompact_counter_is_per_session` to lock in the #955
deadlock fix.
- tests/test_palace_locks.py (new): covers `mine_palace_lock`
single-acquire, reuse-after-release, cross-process serialization
on the same palace, non-interference across different palaces,
path normalization, and the `mine_global_lock` back-compat alias.
5) #5 — known limitation, documented but not auto-fixed
Copilot suggested detecting collections missing `hnsw:num_threads=1`
and calling `collection.modify(metadata=...)` to retrofit existing
palaces. Verified against chromadb 1.5.7: `modify(metadata=...)`
replaces metadata rather than merging, and re-passing
`hnsw:space="cosine"` then raises `ValueError: Changing the
distance function of a collection once it is created is not
supported currently.` The HNSW runtime configuration
(`configuration_json`) also does not expose `num_threads` in
chromadb 1.5.x, so the flag appears to be read only at creation
time. Rather than paper over the limitation with a best-effort
`modify` that silently drops `hnsw:space`, documented in the
mcp_server comment that pre-existing palaces need a
`mempalace nuke` + re-mine to gain the protection. Fresh palaces
are always protected.
Testing
- pytest tests/test_palace_locks.py tests/test_hooks_cli.py
tests/test_backends.py tests/test_cli.py → **98 passed, 0 failed**.
- Runtime validation with two concurrent `mempalace mine` calls:
- Different palaces → both complete in parallel ✓
- Same palace → one completes, the other exits with
"another `mine` is already running against <palace> — exiting
cleanly." ✓
Igor Lins e Silva
Offbeat-Breed
mvalentsev
Trọng Nguyễn
fatkobra
imtylervo
Felipe Truman