Merge pull request #1412 from MemPalace/fix/1268-popen-detach-windows

fix(hooks): detach Popen children so hook exits cleanly on Windows (#1268)

mempalace/hooks_cli.py

@@ -19,6 +19,27 @@ STATE_DIR = Path.home() / ".mempalace" / "hook_state"
 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:
     """User-removable kill-switch.
 
@@ -285,7 +306,7 @@ def _spawn_mine(cmd: list) -> None:
     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)
+        proc = subprocess.Popen(cmd, stdout=log_f, stderr=log_f, **_detached_popen_kwargs())
     _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],
             stdout=subprocess.DEVNULL,
             stderr=subprocess.DEVNULL,
+            **_detached_popen_kwargs(),
         )
     except OSError:
         pass
@@ -513,6 +535,7 @@ def _ingest_transcript(transcript_path: str):
                 ],
                 stdout=log_f,
                 stderr=log_f,
+                **_detached_popen_kwargs(),
             )
         _log(f"Transcript ingest started: {path.name}")
     except OSError:

tests/test_hooks_cli.py

@@ -560,6 +560,78 @@ def test_maybe_auto_ingest_skips_when_mine_running(tmp_path):
                     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 ---