fix(mine): identify lock holder + exit non-zero on contention (#1264) - #1413
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
Pull request overview
Improves mempalace mine lock-contention observability by recording the lock holder identity in the palace lock file, surfacing that identity in MineAlreadyRunning, and ensuring the CLI exits non-zero on contention so automation can detect it.
Changes:
- Record PID + abbreviated argv in the per-palace lock file and include it in
MineAlreadyRunningmessages. - Stop swallowing
MineAlreadyRunninginminer.mine(); handle it incli.cmd_mineandsys.exit(1). - Add regression tests covering holder-identification messaging and non-zero CLI exit on contention.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
mempalace/palace.py |
Persist lock holder identity in the lock file and surface it in contention errors. |
mempalace/miner.py |
Let MineAlreadyRunning propagate so callers can handle contention explicitly. |
mempalace/cli.py |
Catch MineAlreadyRunning, print to stderr, and exit with code 1. |
tests/test_palace_locks.py |
Add cross-process tests asserting PID appears in contention messages and lock file doesn’t grow. |
tests/test_cli.py |
Add CLI regression test asserting non-zero exit and holder identity in stderr. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+391
to
+399
| def _write_lock_holder(lock_file) -> None: | ||
| """Record this process's identity in the lock-file body. Best-effort.""" | ||
| try: | ||
| ident = f"{os.getpid()} {' '.join(sys.argv[:3])}".strip() | ||
| lock_file.seek(0) | ||
| lock_file.truncate() | ||
| lock_file.write(ident) | ||
| lock_file.flush() | ||
| except OSError: |
Comment on lines
453
to
457
| import msvcrt | ||
|
|
||
| try: | ||
| msvcrt.locking(lf.fileno(), msvcrt.LK_NBLCK, 1) | ||
| acquired = True |
Windows CI surfaced two bugs introduced by the holder-identity write: 1. msvcrt.locking(LK_NBLCK, 1) locks 1 byte at the *current* file position. Switching to "a+" mode put the position at end-of-file, so two contenders locked different bytes and silently both acquired (the test asserts saw [(ok, 1), (ok, 2)] instead of ok+busy). 2. With the byte-range lock active on Windows, the locked byte is read-blocked for other processes. A contender trying to read the holder identity from byte 0 would hit PermissionError. Switch to "r+" mode (after touch-create) and explicitly seek(0) before both lock and unlock. Then reserve byte 0 as a pure lock sentinel and write the holder identity from byte 1 onward. _read_lock_holder reads from byte 1+, so it never touches the locked byte. Also bound file growth across re-acquires: truncate to sentinel + len(ident) before writing so the file body stays the size of the current holder, never accumulating across runs. Linux fcntl.flock locks the whole file independent of byte position, so the seek(0) is harmless on POSIX. The shape works on both.
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).
4 tasks
4 tasks
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.
Summary
When
mempalace minecollided with another writer (livemcp_server, another mine, anything takingmine_palace_lock), the operator saw a generic "anothermempalace mineis already running" message and the CLI exited 0 — making the contention invisible tonohupor shell scripts checking\$?.The original reporter ran
nohup mempalace mine ... & disownagainst a palace wheremcp_serverheld the lock and got a 200-byte log with only the auto-defaults warning — no clue that the MCP server was the cause.PR #1162 (already merged) closed the structural race by routing MCP/direct writers through
mine_palace_lock. This PR fixes the observability half: when contention is detected, name the holder and exit non-zero so the operator (or their automation) can react.Changes
mempalace/palace.py:12345 mempalace mcp_server).MineAlreadyRunning:"palace /path is held by PID N (mempalace mcp_server); wait for it to finish or stop the holder before retrying"."w"to"a+"so the prior holder's identity survives long enough to be read by a failed contender.mempalace/miner.py:mine()no longer swallowsMineAlreadyRunning. The exception propagates so the CLI can render a clear message and exit non-zero.mempalace/cli.pycmd_mine: catchesMineAlreadyRunning, prints to stderr, callssys.exit(1).Tests:
tests/test_palace_locks.py:test_lock_failure_message_names_holder(cross-process repro that assertsPID Nappears in the message),test_lock_holder_identity_persists_across_release(lock body does not grow across re-acquires).tests/test_cli.py:test_cmd_mine_exits_nonzero_on_lock_holder(simulatesMineAlreadyRunning, verifiesSystemExit(1)and that the holder identity reaches stderr).Behavior change
In-process callers that called
miner.mine()and depended on it silently swallowingMineAlreadyRunningwill now see the exception. This is intentional — the silent swallow was the bug. Library users that want to coexist with another writer should handle the exception themselves.Test plan
uv run pytest tests/test_palace_locks.py tests/test_cli.py tests/test_miner.py tests/test_chroma_collection_lock.py -v— 39+ passeduvx --from 'ruff>=0.4.0,<0.5' ruff check+ format check — cleanCloses #1264