Skip to content

fix(palace): pair the mine_palace_lock holder-set update with its release (#1970) - #1971

Merged
igorls merged 1 commit into
MemPalace:developfrom
mvalentsev:fix/palace-lock-holder-set-async-safety
Jul 14, 2026
Merged

fix(palace): pair the mine_palace_lock holder-set update with its release (#1970)#1971
igorls merged 1 commit into
MemPalace:developfrom
mvalentsev:fix/palace-lock-holder-set-async-safety

Conversation

@mvalentsev

Copy link
Copy Markdown
Contributor

Fixes #1970.

What does this PR do?

mine_palace_lock() recorded the process-wide hold with _mark_held(palace_key) before the try: whose finally: runs _mark_released(). An async exception (a KeyboardInterrupt/SIGINT, or one injected into a thread) landing in the gap after _mark_held() and before the try: skips _mark_released(), so palace_key is left in the _palace_lock_keys set. The outer finally: still frees the flock, so the in-memory holder set outlives the OS lock: _held_by_this_process() then reports a hold the flock no longer backs, the next re-entrant acquire in this process passes through and writes without the flock, and another process is free to acquire it at the same time. That is two writers into one palace, the HNSW-corruption race the lock is there to stop.

The fix moves _mark_held() to the first line inside the try:, so it pairs with _mark_released() on every exit. _mark_released() is a set.discard, so releasing a key that was never added is a no-op, and the normal acquire/release path is unchanged.

How to test

tests/test_palace_locks.py::test_holder_set_not_orphaned_by_interrupt_after_mark_held runs the real _mark_held, then raises in the window as a signal would, and asserts the holder set is unchanged afterwards. It fails on the current code (the key is stranded) and passes with the fix.

uv run pytest tests/test_palace_locks.py tests/test_chroma_collection_lock.py

Checklist

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

@mvalentsev

Copy link
Copy Markdown
Contributor Author

The red lint check isn't from this PR. ruff format --check . flags tests/test_mcp_server.py, which went in unformatted with #1960 and isn't touched here. #1965 fixes that, so this should go green once it lands.

…ease

_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.
@mvalentsev
mvalentsev force-pushed the fix/palace-lock-holder-set-async-safety branch from 3cf9338 to 4b98dd7 Compare July 11, 2026 10:22
@igorls
igorls merged commit 3da735e into MemPalace:develop Jul 14, 2026
8 checks passed
@igorls igorls mentioned this pull request Jul 14, 2026
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.

bug: mine_palace_lock holder set can outlive the flock on an async interrupt (silent two-writer window)

2 participants