jason/mempalace
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(hooks): per-target PID guard with atomic claim (#1212, #1206)

Browse Source 
The hook PID guard used a single global ``~/.mempalace/hook_state/mine.pid``
file, which failed two ways:

1. ``_mine_already_running`` read-then-spawn was a TOCTOU race. Two
   near-simultaneous Stop hook fires both passed the existence/liveness
   check before either wrote — so both ended up calling
   ``_spawn_mine``.

2. ``_spawn_mine`` unconditionally overwrote the global PID file with
   the new child's PID. The first PID was lost, orphaning the first
   child. The user-visible result in #1212 was two concurrent
   ``mempalace mine`` processes running against the same source, both
   driving HNSW inserts in parallel — exactly the corruption pattern
   the guard was meant to prevent. #1206 reported the same shape from
   the perspective of the user (two mines hung on a 350MB folder).

Replace the global file with per-target slots under
``~/.mempalace/hook_state/mine_pids/``, keyed by sha256 of the mine
sub-arguments (everything after ``mine``). The slot is claimed via
``O_CREAT | O_EXCL`` so the claim is atomic — two simultaneous fires
can never both pass.  Stale slots (PID exists but is dead) are
reclaimed transparently. Different targets (e.g. project mine vs
transcript ingest, or two different MEMPAL_DIRs) get independent
slots and run in parallel.

The mine subprocess receives its slot path via
``MEMPALACE_MINE_PID_FILE`` env var; ``miner._cleanup_mine_pid_file``
reads that var on exit and removes the slot if it points at our PID,
so orphaned slots from crashed mines don't accumulate.

Also routes ``_ingest_transcript`` through ``_spawn_mine`` so the
transcript ingest path now participates in the same dedup — repeated
Stop fires for the same transcript no longer stack parallel mines.

Closes #1212
Closes #1206
This commit is contained in:
Igor Lins e Silva
2026-05-08 02:09:00 -03:00
parent c70d5182dd
commit 3a76360301
4 changed files with 366 additions and 92 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/test_hooks_cli.py
+196 -35
View File
@@ -3,6 +3,7 @@ import io
import json
import os
import subprocess
import sys
from pathlib import Path
from unittest.mock import MagicMock, patch
@@ -441,7 +442,7 @@ def test_maybe_auto_ingest_with_env(tmp_path):
mempal_dir.mkdir()
with patch.dict("os.environ", {"MEMPAL_DIR": str(mempal_dir)}):
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
with patch("mempalace.hooks_cli._MINE_PID_DIR", tmp_path / "mine_pids"):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
_maybe_auto_ingest()
mock_popen.assert_called_once()
@@ -463,7 +464,7 @@ def test_maybe_auto_ingest_uses_mempalace_python(tmp_path):
mempal_dir.mkdir()
with patch.dict("os.environ", {"MEMPAL_DIR": str(mempal_dir)}):
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
with patch("mempalace.hooks_cli._MINE_PID_DIR", tmp_path / "mine_pids"):
with patch(
"mempalace.hooks_cli._mempalace_python", return_value="/fake/venv/python"
):
@@ -513,7 +514,7 @@ def test_maybe_auto_ingest_ignores_transcript_arg_path(tmp_path):
transcript.write_text("")
with patch.dict("os.environ", {}, clear=True):
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
with patch("mempalace.hooks_cli._MINE_PID_DIR", tmp_path / "mine_pids"):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
_maybe_auto_ingest()
mock_popen.assert_not_called()
@@ -543,21 +544,38 @@ def test_maybe_auto_ingest_oserror(tmp_path):
mempal_dir.mkdir()
with patch.dict("os.environ", {"MEMPAL_DIR": str(mempal_dir)}):
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
with patch("mempalace.hooks_cli._MINE_PID_DIR", tmp_path / "mine_pids"):
with patch("mempalace.hooks_cli.subprocess.Popen", side_effect=OSError("fail")):
_maybe_auto_ingest() # should not raise
def test_maybe_auto_ingest_skips_when_mine_running(tmp_path):
"""Does not spawn a new mine process if one is already running."""
"""Does not spawn a new mine process if a mine for the same target is alive."""
mempal_dir = tmp_path / "project"
mempal_dir.mkdir()
pid_dir = tmp_path / "mine_pids"
with patch.dict("os.environ", {"MEMPAL_DIR": str(mempal_dir)}):
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._mine_already_running", return_value=True):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
_maybe_auto_ingest()
mock_popen.assert_not_called()
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
# Pre-populate the per-target slot with a live PID (our own).
from mempalace.hooks_cli import _pid_file_for_cmd
cmd = [
sys.executable,
"-m",
"mempalace",
"mine",
str(mempal_dir.resolve()),
"--mode",
"projects",
]
pid_file = _pid_file_for_cmd(cmd)
pid_file.parent.mkdir(parents=True, exist_ok=True)
pid_file.write_text(str(os.getpid()))
with patch("mempalace.hooks_cli._mempalace_python", return_value=sys.executable):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
_maybe_auto_ingest()
mock_popen.assert_not_called()
# --- _detached_popen_kwargs ---
@@ -604,7 +622,7 @@ def test_detached_popen_kwargs_windows(monkeypatch):
def test_spawn_mine_uses_detached_kwargs(tmp_path):
"""_spawn_mine forwards detached kwargs so the hook can exit cleanly."""
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
with patch("mempalace.hooks_cli._MINE_PID_DIR", tmp_path / "mine_pids"):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
mock_popen.return_value.pid = 9999
from mempalace.hooks_cli import _spawn_mine
@@ -617,52 +635,195 @@ def test_spawn_mine_uses_detached_kwargs(tmp_path):
assert kwargs.get("close_fds") is True
def test_spawn_mine_skips_when_target_running(tmp_path):
"""A second spawn for the same cmd target while the first is alive must skip."""
pid_dir = tmp_path / "mine_pids"
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
from mempalace.hooks_cli import _pid_file_for_cmd, _spawn_mine
cmd = ["mempalace", "mine", "/tmp/proj", "--mode", "projects"]
pid_file = _pid_file_for_cmd(cmd)
pid_file.parent.mkdir(parents=True, exist_ok=True)
pid_file.write_text(str(os.getpid())) # live PID
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
_spawn_mine(cmd)
mock_popen.assert_not_called()
def test_spawn_mine_distinct_targets_dont_block_each_other(tmp_path):
"""Two spawn calls for *different* targets both proceed."""
pid_dir = tmp_path / "mine_pids"
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
from mempalace.hooks_cli import _spawn_mine
mock_popen.return_value.pid = 1111
_spawn_mine(["mempalace", "mine", "/tmp/a", "--mode", "projects"])
mock_popen.return_value.pid = 2222
_spawn_mine(["mempalace", "mine", "/tmp/b", "--mode", "projects"])
assert mock_popen.call_count == 2
def test_spawn_mine_reclaims_stale_slot(tmp_path):
"""A slot pointing at a dead PID is reclaimed silently."""
pid_dir = tmp_path / "mine_pids"
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
from mempalace.hooks_cli import _pid_file_for_cmd, _spawn_mine
cmd = ["mempalace", "mine", "/tmp/proj", "--mode", "projects"]
pid_file = _pid_file_for_cmd(cmd)
pid_file.parent.mkdir(parents=True, exist_ok=True)
pid_file.write_text("999999999") # dead PID
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
mock_popen.return_value.pid = 4242
_spawn_mine(cmd)
mock_popen.assert_called_once()
# New PID is recorded in the reclaimed slot.
assert pid_file.read_text().strip() == "4242"
def test_spawn_mine_releases_slot_on_oserror(tmp_path):
"""If Popen raises OSError, the claimed slot must be released."""
pid_dir = tmp_path / "mine_pids"
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
from mempalace.hooks_cli import _pid_file_for_cmd, _spawn_mine
cmd = ["mempalace", "mine", "/tmp/proj", "--mode", "projects"]
pid_file = _pid_file_for_cmd(cmd)
with patch("mempalace.hooks_cli.subprocess.Popen", side_effect=OSError("spawn fail")):
with pytest.raises(OSError):
_spawn_mine(cmd)
assert (
not pid_file.exists()
), "slot must be released so the next hook fire isn't permanently blocked"
def test_spawn_mine_passes_pid_file_env_var(tmp_path):
"""The child inherits MEMPALACE_MINE_PID_FILE so its cleanup hook can find the slot."""
pid_dir = tmp_path / "mine_pids"
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
mock_popen.return_value.pid = 5555
from mempalace.hooks_cli import _pid_file_for_cmd, _spawn_mine
cmd = ["mempalace", "mine", "/tmp/x", "--mode", "projects"]
_spawn_mine(cmd)
child_env = mock_popen.call_args.kwargs.get("env", {})
expected = str(_pid_file_for_cmd(cmd))
assert child_env.get("MEMPALACE_MINE_PID_FILE") == expected
def test_ingest_transcript_uses_detached_kwargs(tmp_path):
"""_ingest_transcript spawns the convos mine with detach kwargs."""
transcript = tmp_path / "session.jsonl"
transcript.write_text("x" * 200) # > 100 byte gate
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
from mempalace.hooks_cli import _ingest_transcript
with patch("mempalace.hooks_cli._MINE_PID_DIR", tmp_path / "mine_pids"):
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
from mempalace.hooks_cli import _ingest_transcript
_ingest_transcript(str(transcript))
assert mock_popen.called
kwargs = mock_popen.call_args.kwargs
assert kwargs.get("stdin") is subprocess.DEVNULL
assert kwargs.get("close_fds") is True
_ingest_transcript(str(transcript))
assert mock_popen.called
kwargs = mock_popen.call_args.kwargs
assert kwargs.get("stdin") is subprocess.DEVNULL
assert kwargs.get("close_fds") is True
def test_ingest_transcript_skips_when_target_running(tmp_path):
"""Repeated transcript ingests for the same transcript should dedup."""
transcript = tmp_path / "session.jsonl"
transcript.write_text("x" * 200)
pid_dir = tmp_path / "mine_pids"
with patch("mempalace.hooks_cli.STATE_DIR", tmp_path):
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
with patch("mempalace.hooks_cli._mempalace_python", return_value=sys.executable):
from mempalace.hooks_cli import _ingest_transcript, _pid_file_for_cmd
expected_cmd = [
sys.executable,
"-m",
"mempalace",
"mine",
str(transcript.parent),
"--mode",
"convos",
"--wing",
"sessions",
]
pid_file = _pid_file_for_cmd(expected_cmd)
pid_file.parent.mkdir(parents=True, exist_ok=True)
pid_file.write_text(str(os.getpid())) # live target
with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
_ingest_transcript(str(transcript))
mock_popen.assert_not_called()
# --- _mine_already_running ---
def _seed_slot(pid_dir, cmd, body: str):
"""Write ``body`` into the per-target slot for ``cmd`` under ``pid_dir``."""
from mempalace.hooks_cli import _pid_file_for_cmd
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
slot = _pid_file_for_cmd(cmd)
slot.parent.mkdir(parents=True, exist_ok=True)
slot.write_text(body)
return slot
def test_mine_already_running_no_file(tmp_path):
"""Returns False when no PID file exists."""
with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
assert _mine_already_running() is False
"""Returns False when no per-target slot exists."""
cmd = ["mempalace", "mine", "/tmp/x", "--mode", "projects"]
with patch("mempalace.hooks_cli._MINE_PID_DIR", tmp_path / "mine_pids"):
assert _mine_already_running(cmd) is False
def test_mine_already_running_dead_pid(tmp_path):
"""Returns False when PID file contains a PID that no longer exists."""
pid_file = tmp_path / "mine.pid"
pid_file.write_text("999999999") # almost certainly not a real PID
with patch("mempalace.hooks_cli._MINE_PID_FILE", pid_file):
assert _mine_already_running() is False
"""Returns False when the slot's recorded PID is no longer alive."""
pid_dir = tmp_path / "mine_pids"
cmd = ["mempalace", "mine", "/tmp/x", "--mode", "projects"]
_seed_slot(pid_dir, cmd, "999999999") # almost certainly not a real PID
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
assert _mine_already_running(cmd) is False
def test_mine_already_running_live_pid(tmp_path):
"""Returns True when PID file contains the current process's own PID."""
pid_file = tmp_path / "mine.pid"
pid_file.write_text(str(os.getpid())) # current process is definitely alive
with patch("mempalace.hooks_cli._MINE_PID_FILE", pid_file):
assert _mine_already_running() is True
"""Returns True when the slot's recorded PID is alive."""
pid_dir = tmp_path / "mine_pids"
cmd = ["mempalace", "mine", "/tmp/x", "--mode", "projects"]
_seed_slot(pid_dir, cmd, str(os.getpid())) # current process is alive
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
assert _mine_already_running(cmd) is True
def test_mine_already_running_corrupt_file(tmp_path):
"""Returns False when PID file contains non-integer content."""
pid_file = tmp_path / "mine.pid"
pid_file.write_text("not-a-pid")
with patch("mempalace.hooks_cli._MINE_PID_FILE", pid_file):
assert _mine_already_running() is False
"""Returns False when the slot contains non-integer content."""
pid_dir = tmp_path / "mine_pids"
cmd = ["mempalace", "mine", "/tmp/x", "--mode", "projects"]
_seed_slot(pid_dir, cmd, "not-a-pid")
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
assert _mine_already_running(cmd) is False
def test_mine_already_running_distinct_cmds_independent(tmp_path):
"""Slots are keyed per cmd; an alive entry for cmd A doesn't shadow cmd B."""
pid_dir = tmp_path / "mine_pids"
cmd_a = ["mempalace", "mine", "/tmp/a", "--mode", "projects"]
cmd_b = ["mempalace", "mine", "/tmp/b", "--mode", "projects"]
_seed_slot(pid_dir, cmd_a, str(os.getpid()))
with patch("mempalace.hooks_cli._MINE_PID_DIR", pid_dir):
assert _mine_already_running(cmd_a) is True
assert _mine_already_running(cmd_b) is False
# --- _get_mine_targets ---
tests/test_miner.py
+11 -9
View File
@@ -777,7 +777,7 @@ def test_mine_arbitrary_exception_prints_summary_and_reraises(tmp_path, capsys):
def test_mine_cleans_up_pid_file_on_interrupt(tmp_path):
"""Our own PID entry in mine.pid is removed in the finally clause."""
"""Our own per-target PID slot is removed in the finally clause."""
import pytest
from unittest.mock import patch
@@ -786,14 +786,16 @@ def test_mine_cleans_up_pid_file_on_interrupt(tmp_path):
_make_minable_project(project_root, n_files=2)
palace_path = project_root / "palace"
pid_file = tmp_path / "mine.pid"
pid_file = tmp_path / "mine_abc.pid"
pid_file.write_text(str(os.getpid()))
def fake_process_file(*args, **kwargs):
raise KeyboardInterrupt
# The mine subprocess receives its slot path via env var; the cleanup
# hook in miner.py reads that var and removes the slot if it matches.
with (
patch("mempalace.hooks_cli._MINE_PID_FILE", pid_file),
patch.dict(os.environ, {"MEMPALACE_MINE_PID_FILE": str(pid_file)}),
patch("mempalace.miner.process_file", side_effect=fake_process_file),
):
with pytest.raises(SystemExit):
@@ -803,7 +805,7 @@ def test_mine_cleans_up_pid_file_on_interrupt(tmp_path):
def test_mine_cleans_up_pid_file_on_clean_exit(tmp_path):
"""Successful mine also removes its own PID entry in the finally clause."""
"""Successful mine also removes its own per-target PID slot."""
from unittest.mock import patch
project_root = tmp_path / "proj"
@@ -811,17 +813,17 @@ def test_mine_cleans_up_pid_file_on_clean_exit(tmp_path):
_make_minable_project(project_root, n_files=1)
palace_path = project_root / "palace"
pid_file = tmp_path / "mine.pid"
pid_file = tmp_path / "mine_abc.pid"
pid_file.write_text(str(os.getpid()))
with patch("mempalace.hooks_cli._MINE_PID_FILE", pid_file):
with patch.dict(os.environ, {"MEMPALACE_MINE_PID_FILE": str(pid_file)}):
mine(str(project_root), str(palace_path))
assert not pid_file.exists()
def test_mine_does_not_remove_other_processes_pid_file(tmp_path):
"""A PID file pointing at someone else's PID is left untouched."""
"""A PID slot pointing at someone else's PID is left untouched."""
from unittest.mock import patch
project_root = tmp_path / "proj"
@@ -830,10 +832,10 @@ def test_mine_does_not_remove_other_processes_pid_file(tmp_path):
palace_path = project_root / "palace"
other_pid = os.getpid() + 999_999 # a PID that isn't us
pid_file = tmp_path / "mine.pid"
pid_file = tmp_path / "mine_abc.pid"
pid_file.write_text(str(other_pid))
with patch("mempalace.hooks_cli._MINE_PID_FILE", pid_file):
with patch.dict(os.environ, {"MEMPALACE_MINE_PID_FILE": str(pid_file)}):
mine(str(project_root), str(palace_path))
assert pid_file.exists(), "Foreign PID entries must not be removed"