fix(palace): clean source mine locks safely - #1803
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the file locking mechanism in mempalace/palace.py to address POSIX advisory lock stale-handle issues, introducing a retry loop and safe cleanup logic. It also adds comprehensive unit tests in tests/test_mine_lock_lifecycle.py to verify lock lifecycle behavior and prevent regressions. Feedback on the changes points out a potential redundant unlock attempt in the Windows-specific cleanup block of _cleanup_mine_lock_file if the initial unlock fails, suggesting setting acquired = False in the exception handler to prevent a duplicate call in the finally block.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| _unlock_mine_lock_file(lf) | ||
| except Exception: | ||
| logger.debug("Mine-lock cleanup release failed", exc_info=True) | ||
| return |
There was a problem hiding this comment.
In the Windows-specific cleanup block, if _unlock_mine_lock_file(lf) raises an exception, the except block logs the error and returns immediately. However, because closed remains False and acquired remains True, the outer finally block will execute and attempt to call _unlock_mine_lock_file(lf) a second time.
To avoid this redundant unlock attempt (which is guaranteed to fail again), we should set acquired = False inside the except block before returning.
| try: | |
| _unlock_mine_lock_file(lf) | |
| except Exception: | |
| logger.debug("Mine-lock cleanup release failed", exc_info=True) | |
| return | |
| try: | |
| _unlock_mine_lock_file(lf) | |
| except Exception: | |
| logger.debug("Mine-lock cleanup release failed", exc_info=True) | |
| acquired = False | |
| return |
There was a problem hiding this comment.
Pull request overview
This PR updates mine_lock() to safely clean up per-source lock files after use while preserving correct flock rendezvous semantics under contention, and adds regression tests for the stale-inode waiter race described in #1800.
Changes:
- Refactors
mine_lock()into helper functions to (a) remove uncontended lock files and (b) detect/retry when a locked handle no longer matches the lock path’s inode. - Adds best-effort lock-file cleanup that avoids the POSIX “waiter wakes on unlinked inode” race.
- Introduces new regression tests covering uncontended cleanup and the stale-inode waiter scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| mempalace/palace.py | Refactors mine_lock() acquisition/release and adds safe cleanup + stale-inode detection/retry helpers. |
| tests/test_mine_lock_lifecycle.py | Adds lifecycle/regression tests for lock file cleanup and the stale-inode waiter race. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lf.close() | ||
| with public_mine_lock(source_file): | ||
| Path(entered_flag).touch() | ||
| _wait_for_path(Path(release_flag)) |
| assert result_q.get(timeout=10) == ("first-acquire-current", False) | ||
| time.sleep(0.2) | ||
| assert not entered_flag.exists(), "waiter entered while replacement lock was held" | ||
|
|
Summary
mine_lock()exitsCloses #1800.
Validation
uv run --python 3.13 pytest tests/test_mine_lock_lifecycle.py tests/test_closets.py::TestMineLock tests/test_palace_locks.py tests/test_chroma_collection_lock.py -quv run --python 3.13 ruff check mempalace/palace.py tests/test_mine_lock_lifecycle.pyuv run --python 3.13 ruff format --check mempalace/palace.py tests/test_mine_lock_lifecycle.pygit diff --checkRelease note
Candidate for v3.4.1 if we want to clean up stale
~/.mempalace/locks/*.lockfiles without breaking flock rendezvous semantics under contention.