Skip to content

fix(lock): recover same-process stale palace lock - #1977

Closed
fatkobra wants to merge 2 commits into
MemPalace:developfrom
fatkobra:fix/1924-recover-self-deadlocked-palace-lock
Closed

fix(lock): recover same-process stale palace lock#1977
fatkobra wants to merge 2 commits into
MemPalace:developfrom
fatkobra:fix/1924-recover-self-deadlocked-palace-lock

Conversation

@fatkobra

@fatkobra fatkobra commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes and Fixes #1924 by adding narrow same-process stale-lock recovery to mine_palace_lock().

When lock acquisition fails and the lock file says the holder PID is this same process, the process has self-deadlocked on a stale/leaked flock FD. The patch unlinks that pathname and retries once. This matches the verified workaround from the issue while keeping the safety boundary tight.

Safety properties

  • Only recovers when the recorded holder PID equals os.getpid().
  • Never clears a separate process's lock.
  • Retries only once.
  • Declines pathname-unlink recovery on Windows, where open byte-locked files cannot be handled the same way.
  • Preserves existing process-wide re-entrancy for normal nested writes.

How to test

Added tests/test_palace_lock_self_deadlock.py covering:

  • holder PID parsing;
  • POSIX stale same-process inode replacement and retry;
  • refusal to clear another process's holder identity.

Run:

    python3 -m ruff format mempalace/palace.py tests/test_palace_lock_self_deadlock.py
    python3 -m ruff check mempalace/palace.py tests/test_palace_lock_self_deadlock.py
    python3 -m pytest tests/test_palace_lock_self_deadlock.py -q
    python -m pytest tests/ -v

Checklist

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

@fatkobra

fatkobra commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@mvalentsev

Copy link
Copy Markdown
Contributor

@fatkobra the failure mode this recovers should already be gone on develop: #1859 made the re-entrancy marker process-wide, so a second acquire in the same process passes through instead of failing with its own PID. I couldn't reproduce the self-deadlock on current develop; the new test only hits it by taking the flock through a handle opened outside mine_palace_lock. Is there still a path to "flock fails and holder PID == getpid()" on current code?

The unlink also worries me: after os.unlink the old inode stays locked while a new open creates a fresh one, so a process that opened the path before the unlink ends up locking a different file, and two mines can write the same palace at once. And since _write_lock_holder is best-effort, with PID reuse the same-PID check can match a lock held by a live foreign process.

Also heads-up: this rewrites the same lines as #1971 (which moves _mark_held inside the inner try), so whichever lands second needs a rebase that keeps that.

@fatkobra

fatkobra commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @mvalentsev — I agree with this concern too.

I’m going to stop this PR unless I can reproduce the self-deadlock on current develop through a real mine_palace_lock() path. The test I added forces the condition by taking the flock outside mine_palace_lock(), which does not prove the current code can still hit holder PID == os.getpid() after #1859 made the re-entrancy marker process-wide.

I also agree that unlinking the lock file is too risky because it can create two lock inodes for the same palace. That is worse than the original failure because it can allow two writers.

I’ll close this instead of trying to land the unlink approach.

@fatkobra fatkobra closed this Jul 9, 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.

mine_palace_lock flock FD leak under long-lived MCP server → writes self-deadlock (MineAlreadyRunning cites own PID)

2 participants