fix(mcp): release profile stderr handles before deletion
This commit is contained in:
@@ -1302,6 +1302,12 @@ def delete_profile(name: str, yes: bool = False) -> Path:
|
||||
# into the directory before we remove it.
|
||||
_notify_multiplexer(canon)
|
||||
|
||||
# The main serve process survives this deletion. Stop only this profile's MCP
|
||||
# transports and release cached stderr handles, including completed probes.
|
||||
from hermes_constants import hermes_home_key
|
||||
from tools.mcp_tool_lifecycle import shutdown_mcp_servers
|
||||
shutdown_mcp_servers(scope=hermes_home_key(profile_dir))
|
||||
|
||||
# Release this process's holographic memory-store connections into the profile. The
|
||||
# Desktop's main serve process opens memory_store.db for every profile and is
|
||||
# deliberately not stopped above; on Windows its handles fail rmtree with WinError 32.
|
||||
|
||||
@@ -0,0 +1,55 @@
|
||||
"""Profile deletion after a real stdio MCP probe must release its log file."""
|
||||
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from hermes_cli import profiles
|
||||
from hermes_cli.mcp_config import _probe_single_server
|
||||
from hermes_constants import reset_hermes_home_override, set_hermes_home_override
|
||||
from tools import mcp_tool_config
|
||||
|
||||
|
||||
@pytest.mark.windows_only
|
||||
def test_delete_profile_after_stdio_probe(tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(Path, "home", lambda: tmp_path)
|
||||
home = tmp_path / ".hermes"
|
||||
home.mkdir()
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
# Do not manage host services or scan unrelated developer processes in this test.
|
||||
monkeypatch.setattr(profiles, "_cleanup_gateway_service", lambda *_: None)
|
||||
monkeypatch.setattr(profiles, "_stop_profile_backends", lambda *_: None)
|
||||
monkeypatch.setattr(profiles, "_notify_multiplexer", lambda *_: None)
|
||||
profile = profiles.create_profile("mcp-probe-delete", no_alias=True)
|
||||
server = tmp_path / "stdio_server.py"
|
||||
server.write_text('''import json
|
||||
import sys
|
||||
for line in sys.stdin:
|
||||
request = json.loads(line)
|
||||
if "id" not in request:
|
||||
continue
|
||||
if request["method"] == "initialize":
|
||||
result = {"protocolVersion": request["params"]["protocolVersion"],
|
||||
"capabilities": {"tools": {}},
|
||||
"serverInfo": {"name": "probe-fixture", "version": "1"}}
|
||||
elif request["method"] == "tools/list":
|
||||
result = {"tools": [{"name": "echo", "description": "Echo",
|
||||
"inputSchema": {"type": "object"}}]}
|
||||
else:
|
||||
result = {}
|
||||
print(json.dumps({"jsonrpc": "2.0", "id": request["id"], "result": result}), flush=True)
|
||||
''', encoding="utf-8")
|
||||
token = set_hermes_home_override(profile)
|
||||
try:
|
||||
assert _probe_single_server("fixture", {"command": sys.executable, "args": [str(server)]}) == [("echo", "Echo")]
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
try:
|
||||
profiles.delete_profile("mcp-probe-delete", yes=True)
|
||||
assert not profile.exists()
|
||||
finally:
|
||||
# Keep the pre-fix failure from leaking a Windows handle into tempdir cleanup.
|
||||
for handle in list(mcp_tool_config._mcp_stderr_log_fh.values()):
|
||||
if getattr(handle, "name", None) == str(profile / "logs" / "mcp-stderr.log"):
|
||||
handle.close()
|
||||
@@ -0,0 +1,38 @@
|
||||
"""MCP shutdown releases only the selected profile's cached stderr handle."""
|
||||
|
||||
from hermes_constants import (
|
||||
hermes_home_key,
|
||||
reset_hermes_home_override,
|
||||
set_hermes_home_override,
|
||||
)
|
||||
from tools.mcp_tool_config import _get_mcp_stderr_log
|
||||
from tools.mcp_tool_lifecycle import shutdown_mcp_servers
|
||||
|
||||
|
||||
def test_scoped_shutdown_releases_log_and_preserves_other_profile(tmp_path):
|
||||
handles = []
|
||||
for name in ("first", "second"):
|
||||
token = set_hermes_home_override(tmp_path / name)
|
||||
try:
|
||||
handles.append(_get_mcp_stderr_log())
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
first, second = handles
|
||||
try:
|
||||
shutdown_mcp_servers(scope=hermes_home_key(tmp_path / "first"))
|
||||
assert first.closed
|
||||
second.write("other profile remains usable\n")
|
||||
second.flush()
|
||||
token = set_hermes_home_override(tmp_path / "first")
|
||||
try:
|
||||
reopened = _get_mcp_stderr_log()
|
||||
handles.append(reopened)
|
||||
reopened.write("reload can reopen the log\n")
|
||||
reopened.flush()
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
shutdown_mcp_servers()
|
||||
assert second.closed and reopened.closed
|
||||
finally:
|
||||
for handle in handles:
|
||||
handle.close()
|
||||
@@ -20,17 +20,17 @@ _mcp_stderr_log_lock = threading.Lock()
|
||||
|
||||
|
||||
def _get_mcp_stderr_log() -> Any:
|
||||
"""Shared append-mode handle for MCP subprocess stderr, opened once per process PER PROFILE HOME (a
|
||||
"""Shared append-mode handle for MCP subprocess stderr, cached until shutdown PER PROFILE HOME (a
|
||||
multiplexed gateway's secondary profile must log under ITS ``logs/``, not the launch profile's). Must
|
||||
expose a real fd (asyncio wires the child's stderr to it); falls back to ``/dev/null``, then real stderr."""
|
||||
from hermes_constants import get_hermes_home, hermes_home_key
|
||||
from hermes_constants import get_hermes_home, hermes_home_key, mkdir_under_hermes_home
|
||||
home_key = hermes_home_key()
|
||||
with _mcp_stderr_log_lock:
|
||||
fh = _mcp_stderr_log_fh.get(home_key)
|
||||
if fh is None:
|
||||
if fh is None or fh.closed:
|
||||
try:
|
||||
log_dir = get_hermes_home() / "logs"
|
||||
log_dir.mkdir(parents=True, exist_ok=True)
|
||||
mkdir_under_hermes_home(log_dir)
|
||||
# Line-buffered so output lands promptly; errors="replace" tolerates garbled binary.
|
||||
fh = open(log_dir / "mcp-stderr.log", "a", encoding="utf-8", errors="replace", buffering=1)
|
||||
fh.fileno() # confirm a real fd before committing
|
||||
@@ -44,6 +44,21 @@ def _get_mcp_stderr_log() -> Any:
|
||||
return fh
|
||||
|
||||
|
||||
def _close_mcp_stderr_logs(*, scope: Optional[str] = None) -> None:
|
||||
"""Release cached parent handles after the selected MCP transports have stopped."""
|
||||
with _mcp_stderr_log_lock:
|
||||
keys = list(_mcp_stderr_log_fh) if scope is None else [scope]
|
||||
for key in keys:
|
||||
fh = _mcp_stderr_log_fh.pop(key, None)
|
||||
# The last-resort fallback is borrowed, not owned by MCP.
|
||||
if fh is None or fh is sys.stderr or fh is sys.__stderr__:
|
||||
continue
|
||||
try:
|
||||
fh.close()
|
||||
except OSError:
|
||||
logger.warning("Could not close MCP stderr log for %s", key, exc_info=True)
|
||||
|
||||
|
||||
def _write_stderr_log_header(server_name: str) -> None:
|
||||
"""Session marker so operators can find each server's output in the shared log
|
||||
(per-line prefixes would need a pipe + reader thread)."""
|
||||
|
||||
@@ -201,6 +201,11 @@ def shutdown_mcp_servers(*, scope: Optional[str] = None, names: Optional[set] =
|
||||
clear_selected_status()
|
||||
_clear_connect_cooldowns(None if scope is None and names is None else selected_status)
|
||||
_loop._stop_mcp_loop(only_if_idle=scope is not None or names is not None)
|
||||
# A removed subset still shares its profile's log with the remaining servers.
|
||||
# Full/profile shutdown must also release handles left by completed CLI/UI probes.
|
||||
if names is None:
|
||||
from tools.mcp_tool_config import _close_mcp_stderr_logs
|
||||
_close_mcp_stderr_logs(scope=scope)
|
||||
|
||||
|
||||
def _take_reapable_pids(include_active: bool, server_name: Optional[str]) -> tuple[Dict[int, str], Dict[int, int]]:
|
||||
|
||||
Reference in New Issue
Block a user