Merge pull request #1023 from jphein/pr/pid-file-guard

fix(hooks): PID file guard prevents stacking mine processes

Author: bensig

mempalace/hooks_cli.py

@@ -150,20 +150,70 @@ def _get_mine_dir(transcript_path: str = "") -> str:
     return ""
 
 
+_MINE_PID_FILE = STATE_DIR / "mine.pid"
+
+
+def _pid_alive(pid: int) -> bool:
+    """Cross-platform existence check for a PID.
+
+    On POSIX, ``os.kill(pid, 0)`` is the well-known no-op existence probe.
+    On Windows, ``os.kill`` maps to ``TerminateProcess(handle, sig)`` and
+    would *terminate* the target process with exit code ``sig`` — using
+    it here would kill our own mine child (or worse, the caller itself).
+    Use ``OpenProcess`` + ``GetExitCodeProcess`` via ctypes instead.
+    """
+    if sys.platform == "win32":
+        import ctypes
+        from ctypes import wintypes
+
+        PROCESS_QUERY_LIMITED_INFORMATION = 0x1000
+        STILL_ACTIVE = 259
+        kernel32 = ctypes.windll.kernel32
+        handle = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, pid)
+        if not handle:
+            return False
+        try:
+            code = wintypes.DWORD()
+            if not kernel32.GetExitCodeProcess(handle, ctypes.byref(code)):
+                return False
+            return code.value == STILL_ACTIVE
+        finally:
+            kernel32.CloseHandle(handle)
+    try:
+        os.kill(pid, 0)
+        return True
+    except (OSError, ValueError):
+        return False
+
+
+def _mine_already_running() -> bool:
+    """Return True if a background mine process from a previous hook fire is still alive."""
+    try:
+        pid = int(_MINE_PID_FILE.read_text().strip())
+    except (OSError, ValueError):
+        return False
+    return _pid_alive(pid)
+
+
+def _spawn_mine(cmd: list) -> None:
+    """Spawn a mine subprocess, write its PID to the lock file, log to hook.log."""
+    STATE_DIR.mkdir(parents=True, exist_ok=True)
+    log_path = STATE_DIR / "hook.log"
+    with open(log_path, "a") as log_f:
+        proc = subprocess.Popen(cmd, stdout=log_f, stderr=log_f)
+    _MINE_PID_FILE.write_text(str(proc.pid))
+
+
 def _maybe_auto_ingest(transcript_path: str = ""):
     """Run mempalace mine in background if a mine directory is available."""
     mine_dir = _get_mine_dir(transcript_path)
     if not mine_dir:
         return
+    if _mine_already_running():
+        _log("Skipping auto-ingest: mine already running")
+        return
     try:
-        STATE_DIR.mkdir(parents=True, exist_ok=True)
-        log_path = STATE_DIR / "hook.log"
-        with open(log_path, "a") as log_f:
-            subprocess.Popen(
-                [sys.executable, "-m", "mempalace", "mine", mine_dir],
-                stdout=log_f,
-                stderr=log_f,
-            )
+        _spawn_mine([sys.executable, "-m", "mempalace", "mine", mine_dir])
     except OSError:
         pass
 

tests/test_hooks_cli.py

@@ -1,6 +1,7 @@
 import contextlib
 import io
 import json
+import os
 import subprocess
 from pathlib import Path
 from unittest.mock import patch
@@ -14,6 +15,7 @@
     _get_mine_dir,
     _log,
     _maybe_auto_ingest,
+    _mine_already_running,
     _parse_harness_input,
     _sanitize_session_id,
     _validate_transcript_path,
@@ -250,9 +252,10 @@ 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.subprocess.Popen") as mock_popen:
-                _maybe_auto_ingest()
-                mock_popen.assert_called_once()
+            with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
+                with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
+                    _maybe_auto_ingest()
+                    mock_popen.assert_called_once()
 
 
 def test_maybe_auto_ingest_with_transcript(tmp_path):
@@ -261,9 +264,10 @@ def test_maybe_auto_ingest_with_transcript(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.subprocess.Popen") as mock_popen:
-                _maybe_auto_ingest(str(transcript))
-                mock_popen.assert_called_once()
+            with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
+                with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
+                    _maybe_auto_ingest(str(transcript))
+                    mock_popen.assert_called_once()
 
 
 def test_maybe_auto_ingest_oserror(tmp_path):
@@ -272,8 +276,54 @@ 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.subprocess.Popen", side_effect=OSError("fail")):
-                _maybe_auto_ingest()  # should not raise
+            with patch("mempalace.hooks_cli._MINE_PID_FILE", tmp_path / "mine.pid"):
+                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."""
+    mempal_dir = tmp_path / "project"
+    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_already_running", return_value=True):
+                with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen:
+                    _maybe_auto_ingest()
+                    mock_popen.assert_not_called()
+
+
+# --- _mine_already_running ---
+
+
+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
+
+
+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
+
+
+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
+
+
+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
 
 
 # --- _get_mine_dir ---