From a6b6e552471f57c82394d107eb4aac668e9a95c9 Mon Sep 17 00:00:00 2001 From: jp Date: Sat, 18 Apr 2026 20:27:56 -0700 Subject: [PATCH 1/3] fix: PID file guard prevents stacking mine processes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- mempalace/hooks_cli.py | 34 ++++++++++++++++----- tests/test_hooks_cli.py | 66 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 84 insertions(+), 16 deletions(-) diff --git a/mempalace/hooks_cli.py b/mempalace/hooks_cli.py index 97832dc..f325d80 100644 --- a/mempalace/hooks_cli.py +++ b/mempalace/hooks_cli.py @@ -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 diff --git a/tests/test_hooks_cli.py b/tests/test_hooks_cli.py index 7bf6cf3..2113c2d 100644 --- a/tests/test_hooks_cli.py +++ b/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 @@ 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 --- From fe6b8899bc29dcb40a919b7bb45916a44bd83bd5 Mon Sep 17 00:00:00 2001 From: jp Date: Sat, 18 Apr 2026 21:13:37 -0700 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20broaden=20=5Fmine=5Falready=5Frunnin?= =?UTF-8?q?g=20catch=20=E2=80=94=20Windows=20os.kill=20raises=20plain=20OS?= =?UTF-8?q?Error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, os.kill(bogus_pid, 0) raises OSError[WinError 87] "The parameter is incorrect" — NOT ProcessLookupError. The old except tuple missed it, so test_mine_already_running_dead_pid failed on Windows CI. Catching OSError covers ProcessLookupError + PermissionError + FileNotFoundError on POSIX and WinError 87 on Windows. ValueError still guards the int() parse. Co-Authored-By: Claude Opus 4.7 (1M context) --- mempalace/hooks_cli.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mempalace/hooks_cli.py b/mempalace/hooks_cli.py index f325d80..ce73fbf 100644 --- a/mempalace/hooks_cli.py +++ b/mempalace/hooks_cli.py @@ -159,7 +159,10 @@ def _mine_already_running() -> bool: 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): + except (OSError, ValueError): + # OSError covers: FileNotFoundError (no pid file), ProcessLookupError + # (dead PID on POSIX), PermissionError (not our process), and + # WinError 87 / "invalid parameter" (dead or unknown PID on Windows). return False From dfba247454384e659bfcc580fc2d76f9bb252986 Mon Sep 17 00:00:00 2001 From: jp Date: Sat, 18 Apr 2026 21:19:52 -0700 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20cross-platform=20PID=20check=20?= =?UTF-8?q?=E2=80=94=20os.kill(pid,=200)=20TERMINATES=20on=20Windows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real bug surfaced on CI for this PR. On POSIX, os.kill(pid, 0) is the canonical no-op existence probe. On Windows, Python's os.kill maps to TerminateProcess(handle, sig), which *terminates* the target with exit code sig. os.kill(pid, 0) therefore kills the target with exit code 0 — silently destroying our mine child (or, as happened in test_mine_already_running_live_pid, the pytest process itself). Fix: split into _pid_alive(pid) helper with a Windows branch using ctypes.windll.kernel32.OpenProcess + GetExitCodeProcess. PROCESS_QUERY_LIMITED_INFORMATION opens a handle only if the PID exists; STILL_ACTIVE (259) distinguishes running from exited processes. No new dependencies — stdlib ctypes. Co-Authored-By: Claude Opus 4.7 (1M context) --- mempalace/hooks_cli.py | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/mempalace/hooks_cli.py b/mempalace/hooks_cli.py index ce73fbf..92184f5 100644 --- a/mempalace/hooks_cli.py +++ b/mempalace/hooks_cli.py @@ -153,17 +153,46 @@ def _get_mine_dir(transcript_path: str = "") -> str: _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()) - os.kill(pid, 0) # signal 0 = existence check, no actual signal sent - return True except (OSError, ValueError): - # OSError covers: FileNotFoundError (no pid file), ProcessLookupError - # (dead PID on POSIX), PermissionError (not our process), and - # WinError 87 / "invalid parameter" (dead or unknown PID on Windows). return False + return _pid_alive(pid) def _spawn_mine(cmd: list) -> None: