fix(hooks): detach Popen children so the hook can exit on Windows
The Stop hook spawns mining subprocesses via subprocess.Popen and then returns. On Windows the parent stays blocked at session end because the child inherits stdout/stderr handles and the OS waits for them to release before the parent can exit — the user-visible symptom is the "running stop hooks... 3/3" spinner hanging for minutes (#1268). Add _detached_popen_kwargs() helper that returns the right detach knobs per platform: - POSIX: start_new_session=True, stdin=DEVNULL, close_fds=True - Windows: creationflags=DETACHED_PROCESS|CREATE_NEW_PROCESS_GROUP| CREATE_BREAKAWAY_FROM_JOB, stdin=DEVNULL, close_fds=True Apply to all three fire-and-forget Popen sites in hooks_cli: _spawn_mine, _ingest_transcript, _desktop_toast. Leave _mine_sync's subprocess.run alone — that path is intentionally synchronous (the precompact hook must wait for the mine to finish). Note: the issue body references mempalace-stop.js, which does not exist in this repo (the plugin ships shell wrappers calling Python). The mechanism described — child holds parent open via inherited handles — is universal, so this fix targets the equivalent symptom in our Python hook path. Will follow up on the upstream JS file with the reporter.
This commit is contained in:
+24
-1
@@ -19,6 +19,27 @@ STATE_DIR = Path.home() / ".mempalace" / "hook_state"
|
|||||||
PALACE_ROOT = Path.home() / ".mempalace"
|
PALACE_ROOT = Path.home() / ".mempalace"
|
||||||
|
|
||||||
|
|
||||||
|
def _detached_popen_kwargs() -> dict:
|
||||||
|
"""Kwargs that fully detach a Popen child so the hook process can exit.
|
||||||
|
|
||||||
|
Without these, Windows holds the parent open until the child closes the
|
||||||
|
inherited stdout/stderr handles — manifesting as "Stop hook hangs" at
|
||||||
|
session end (#1268). On POSIX the parent can already exit (orphan
|
||||||
|
reparents to init), but ``start_new_session`` makes the boundary
|
||||||
|
explicit so signals to the hook don't propagate to the background mine.
|
||||||
|
"""
|
||||||
|
kwargs: dict = {"stdin": subprocess.DEVNULL, "close_fds": True}
|
||||||
|
if os.name == "nt":
|
||||||
|
flags = 0
|
||||||
|
for name in ("DETACHED_PROCESS", "CREATE_NEW_PROCESS_GROUP", "CREATE_BREAKAWAY_FROM_JOB"):
|
||||||
|
flags |= getattr(subprocess, name, 0)
|
||||||
|
if flags:
|
||||||
|
kwargs["creationflags"] = flags
|
||||||
|
else:
|
||||||
|
kwargs["start_new_session"] = True
|
||||||
|
return kwargs
|
||||||
|
|
||||||
|
|
||||||
def _palace_root_exists() -> bool:
|
def _palace_root_exists() -> bool:
|
||||||
"""User-removable kill-switch.
|
"""User-removable kill-switch.
|
||||||
|
|
||||||
@@ -285,7 +306,7 @@ def _spawn_mine(cmd: list) -> None:
|
|||||||
STATE_DIR.mkdir(parents=True, exist_ok=True)
|
STATE_DIR.mkdir(parents=True, exist_ok=True)
|
||||||
log_path = STATE_DIR / "hook.log"
|
log_path = STATE_DIR / "hook.log"
|
||||||
with open(log_path, "a") as log_f:
|
with open(log_path, "a") as log_f:
|
||||||
proc = subprocess.Popen(cmd, stdout=log_f, stderr=log_f)
|
proc = subprocess.Popen(cmd, stdout=log_f, stderr=log_f, **_detached_popen_kwargs())
|
||||||
_MINE_PID_FILE.write_text(str(proc.pid))
|
_MINE_PID_FILE.write_text(str(proc.pid))
|
||||||
|
|
||||||
|
|
||||||
@@ -350,6 +371,7 @@ def _desktop_toast(body: str, title: str = "MemPalace"):
|
|||||||
["notify-send", "--app-name=MemPalace", "--icon=brain", title, body],
|
["notify-send", "--app-name=MemPalace", "--icon=brain", title, body],
|
||||||
stdout=subprocess.DEVNULL,
|
stdout=subprocess.DEVNULL,
|
||||||
stderr=subprocess.DEVNULL,
|
stderr=subprocess.DEVNULL,
|
||||||
|
**_detached_popen_kwargs(),
|
||||||
)
|
)
|
||||||
except OSError:
|
except OSError:
|
||||||
pass
|
pass
|
||||||
@@ -513,6 +535,7 @@ def _ingest_transcript(transcript_path: str):
|
|||||||
],
|
],
|
||||||
stdout=log_f,
|
stdout=log_f,
|
||||||
stderr=log_f,
|
stderr=log_f,
|
||||||
|
**_detached_popen_kwargs(),
|
||||||
)
|
)
|
||||||
_log(f"Transcript ingest started: {path.name}")
|
_log(f"Transcript ingest started: {path.name}")
|
||||||
except OSError:
|
except OSError:
|
||||||
|
|||||||
@@ -560,6 +560,78 @@ def test_maybe_auto_ingest_skips_when_mine_running(tmp_path):
|
|||||||
mock_popen.assert_not_called()
|
mock_popen.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
|
# --- _detached_popen_kwargs ---
|
||||||
|
|
||||||
|
|
||||||
|
def test_detached_popen_kwargs_posix(monkeypatch):
|
||||||
|
"""On POSIX, kwargs include start_new_session so the child detaches."""
|
||||||
|
from mempalace.hooks_cli import _detached_popen_kwargs
|
||||||
|
|
||||||
|
monkeypatch.setattr("mempalace.hooks_cli.os.name", "posix")
|
||||||
|
kwargs = _detached_popen_kwargs()
|
||||||
|
assert kwargs.get("start_new_session") is True
|
||||||
|
assert kwargs.get("stdin") is subprocess.DEVNULL
|
||||||
|
assert kwargs.get("close_fds") is True
|
||||||
|
assert "creationflags" not in kwargs
|
||||||
|
|
||||||
|
|
||||||
|
def test_detached_popen_kwargs_windows(monkeypatch):
|
||||||
|
"""On Windows, kwargs include creationflags that fully detach the child.
|
||||||
|
|
||||||
|
Without these, the parent hook hangs at session end on Windows because
|
||||||
|
the child's inherited stdout/stderr handles keep the parent's exit
|
||||||
|
blocked (#1268 root cause for the Python hook path).
|
||||||
|
"""
|
||||||
|
from mempalace.hooks_cli import _detached_popen_kwargs
|
||||||
|
|
||||||
|
monkeypatch.setattr("mempalace.hooks_cli.os.name", "nt")
|
||||||
|
# Simulate Windows-only Popen flag constants. Patch on the imported
|
||||||
|
# subprocess module within hooks_cli so getattr() picks them up.
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"mempalace.hooks_cli.subprocess.DETACHED_PROCESS", 0x00000008, raising=False
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"mempalace.hooks_cli.subprocess.CREATE_NEW_PROCESS_GROUP", 0x00000200, raising=False
|
||||||
|
)
|
||||||
|
kwargs = _detached_popen_kwargs()
|
||||||
|
assert kwargs.get("stdin") is subprocess.DEVNULL
|
||||||
|
assert kwargs.get("close_fds") is True
|
||||||
|
flags = kwargs.get("creationflags", 0)
|
||||||
|
assert flags & 0x00000008, "DETACHED_PROCESS must be set"
|
||||||
|
assert flags & 0x00000200, "CREATE_NEW_PROCESS_GROUP must be set"
|
||||||
|
|
||||||
|
|
||||||
|
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.subprocess.Popen") as mock_popen:
|
||||||
|
mock_popen.return_value.pid = 9999
|
||||||
|
from mempalace.hooks_cli import _spawn_mine
|
||||||
|
|
||||||
|
_spawn_mine(["mempalace", "mine", "/tmp/x"])
|
||||||
|
kwargs = mock_popen.call_args.kwargs
|
||||||
|
# The exact key set varies by platform; assert on the
|
||||||
|
# shared invariants that protect against the Windows hang.
|
||||||
|
assert kwargs.get("stdin") is subprocess.DEVNULL
|
||||||
|
assert kwargs.get("close_fds") is True
|
||||||
|
|
||||||
|
|
||||||
|
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
|
||||||
|
|
||||||
|
_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
|
||||||
|
|
||||||
|
|
||||||
# --- _mine_already_running ---
|
# --- _mine_already_running ---
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user