fix: PID file guard prevents stacking mine processes
Every stop hook fire spawned a new background `mempalace mine` via subprocess.Popen with no dedup — 4 concurrent mines at ~770% CPU observed in production. Add `_mine_already_running()` (reads `hook_state/mine.pid`, uses `os.kill(pid, 0)` as an existence check) and `_spawn_mine()` (writes the child PID to the lock file after Popen returns). `_maybe_auto_ingest` bails early when the guard reports True. Tests: 4 new unit tests for `_mine_already_running` (no file, dead PID, live PID using `os.getpid()`, corrupt file), 1 new test covering the skip-when-running branch of `_maybe_auto_ingest`, and existing spawn tests patched to redirect `_MINE_PID_FILE` into tmp_path so they don't touch the real state dir. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
+26
-8
@@ -150,20 +150,38 @@ def _get_mine_dir(transcript_path: str = "") -> str:
|
||||
return ""
|
||||
|
||||
|
||||
_MINE_PID_FILE = STATE_DIR / "mine.pid"
|
||||
|
||||
|
||||
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())
|
||||
os.kill(pid, 0) # signal 0 = existence check, no actual signal sent
|
||||
return True
|
||||
except (FileNotFoundError, ValueError, ProcessLookupError, PermissionError):
|
||||
return False
|
||||
|
||||
|
||||
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
|
||||
|
||||
|
||||
+58
-8
@@ -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 @@ from mempalace.hooks_cli import (
|
||||
_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 ---
|
||||
|
||||
Reference in New Issue
Block a user