fix(update): reap gateway venv children so the Windows venv guard self-clears - #61515
fix(update): reap gateway venv children so the Windows venv guard self-clears#61515lEWFkRAD wants to merge 2 commits into
Conversation
…f-clears On Windows, `hermes update` pauses the gateway before its venv-process guard, but pausing a gateway does not reap the helper processes it spawned from the install venv's python — stdio MCP servers and the perfmon probe. Those children survive as orphans holding native `.pyd` files mapped, so the venv-process guard (`_detect_venv_python_processes`) sees them and refuses the update indefinitely. Every blind retry hits the same wall; a killed `hermes-setup.exe` also leaves a stale `.hermes-update-in-progress` marker behind (observed July 2026 with `client_lookup_mcp` / `hermes-perfmon`). Fix (all Windows-guarded; POSIX behavior unchanged): - `_pause_windows_gateways_for_update` snapshots each gateway's venv-resident descendants BEFORE stopping it (a dead parent's dangling ppid defeats an after-the-fact walk), then reaps them once the gateways are down. They are re-spawned when the gateway restarts (resume path), so no state is lost. The supervised desktop backend (`serve`/`dashboard`) is a child of the Electron app, not a gateway, and is explicitly excluded from the reap. - `_cmd_update_impl` sweeps a stale `.hermes-update-in-progress` marker whose owning PID is dead before the guard runs, so the CLI recovery path also clears the desktop's launch gate. A live-owner marker (a genuine concurrent update, incl. our own parent setup) is never touched. - Extract the venv-holder predicate (`_venv_lock_prefixes` / `_process_holds_install_venv`) so the guard and the reaper classify holders identically. Adds tests for the predicate, snapshot (incl. desktop-backend exclusion, dedup, dead-gateway/no-psutil safety), reap (force-kill each, best-effort on error), `_pid_is_alive`, and the stale-marker sweep (dead/live/malformed/absent/POSIX). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Overview
- 2 files changed, +581/-30 lines
- Windows venv update: reap gateway child processes so the guard self-cleans
- Uses
psutilfor cross-platform process tree traversal
Looks Good
- Clean fix for a real Windows issue
psutilis already a dependency in this project- Proper process cleanup on Windows
Note
- +581 lines includes substantial process-reaping logic — worth verifying on a Windows machine
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real update-recovery gap. Current main pauses gateway PIDs but has no descendant cleanup (hermes_cli/main.py:9083-9110), then immediately applies the venv-holder guard (hermes_cli/main.py:9465-9489).
Problems
hermes_cli/main.py:9079in this PR identifies a desktop backend with"serve" in cmdline_low or "dashboard" in cmdline_low. That also matches ordinary helper commands such as the documented stdio MCP commandmcp-server-time(skills/autonomous-ai-agents/hermes-agent/references/native-mcp.md:38). Such a gateway child is excluded at PR line 9147, so it can remain locked and reproduce the guard failure.
Suggested changes
- Match the actual
hermes_cli.main serve/dashboardinvocation with token-aware parsing, not arbitrary substrings. - Add a snapshot regression test for
mcp-server-timeorserver.pybeing captured, alongside the existing exact desktop-backend exclusion test (tests/hermes_cli/test_update_venv_child_reap.py:121-137).
Automated hermes-sweeper review.
| flow (the venv guard refuses and tells the user to close the app instead). | ||
| Used as a safety exclusion when reaping gateway children. | ||
| """ | ||
| return "serve" in cmdline_low or "dashboard" in cmdline_low |
There was a problem hiding this comment.
This substring check excludes any helper whose command merely contains serve, including the documented stdio MCP command mcp-server-time. Since the snapshot then skips it, that child can retain the venv lock and defeat the purpose of this fix. Please match the tokenized hermes_cli.main serve / dashboard invocation instead, and add a mcp-server-time regression case.
|
Addressed in 9489f4f. Desktop backend detection now tokenizes the command line and requires the exact hermes_cli.main serve or hermes_cli.main dashboard module/subcommand sequence, so helpers such as mcp-server-time and server.py remain eligible for cleanup. Added regression coverage for both names; all 21 focused updater child-reap tests pass. |
|
Closing in favor of #74436, which fixes the cause this works around. Your diff treats the symptom correctly — a venv holder blocks the updater — but the holder in these reports is the dashboard's own detached The venv-holder detection you were working around is untouched and still correct; it should just rarely fire now. Thanks for digging into this — the logs and repro in here were genuinely useful in tracing the orchestration bug. |
What does this PR do?
Fixes the Windows
hermes updatepermanent dead-end at the venv-process guard._pause_windows_gateways_for_update()stops gateway PIDs before the venv guard runs, but on Windows a stopped gateway does not reap the helpers it spawned from the install venv's python (stdio MCP servers, the perfmon probe). Those orphans keep native.pydfiles mapped, so_detect_venv_python_processes()refuses the update — and since nothing ever cleans the orphans up, every retry fails identically. The guard can never self-clear.This PR makes the pause step responsible for the gateway's own venv children:
_snapshot_gateway_venv_children()walks each running gateway's descendant tree before the gateway is stopped (once the parent exits, the children's ppid dangles and a later walk misses them), keeping only processes that hold this install's venv (same three signals the guard has always used, now factored into a shared predicate_process_holds_install_venv())._terminate_gateway_venv_children()force-terminates the captured helpers (terminate_pid(force=True)→taskkill /T /F) and waits (bounded) for them to exit, so the guard that runs next sees a clean slate. The helpers respawn when the gateway restarts via the existing resume path — no state is lost.hermes_cli.main servebackend is deliberately never reaped (the app respawns it within seconds; the guard's "close the desktop app" refusal remains the right answer there)._sweep_stale_update_marker()in the update preflight removes aHERMES_HOME\.hermes-update-in-progressmarker whose owning PID is dead (left behind when a wedgedhermes-setup.exeis killed). A live-owner marker — including our own parent setup process when the desktop drives the update — is never touched.Everything is Windows-guarded (
_is_windows()), best-effort, and never raises; POSIX behavior is unchanged. If a reap fails (access denied, already gone), the venv guard still runs and still refuses — the guard remains the backstop, this just clears the self-inflicted case.Related Issue
Fixes #61514
Type of Change
Changes Made
hermes_cli/main.py_venv_lock_prefixes()/_process_holds_install_venv()— venv-holder predicate extracted from_detect_venv_python_processes()so the guard and the reaper classify holders identically (pure refactor of the guard; behavior unchanged, existing tests pass)._snapshot_gateway_venv_children()/_terminate_gateway_venv_children()— capture-before-stop + reap-after-stop, wired into_pause_windows_gateways_for_update()._process_is_desktop_backend()— safety exclusion for the supervised desktop backend._pid_is_alive()/_sweep_stale_update_marker()— dead-owner marker sweep, wired into_cmd_update_impl()preflight (mirrors the staleness self-heal the Electron launch gate already does inapps/desktop/electron/update-marker.ts).tests/hermes_cli/test_update_venv_child_reap.py— 20 new tests: predicate (venv exe / trampoline / unrelated / empty), snapshot (captures MCP+perfmon, excludes desktop backend + unrelated python, dedups across gateways, dead-gateway and no-psutil safety, off-Windows no-op), reap (force-kills each child, best-effort on per-PID failure, off-Windows/empty no-ops),_pid_is_alive(psutil path + conservative no-psutil default), marker sweep (dead-owner removed, live-owner preserved, malformed removed, absent no-op, POSIX untouched).How to Test
pytest tests/hermes_cli/test_update_venv_child_reap.py tests/hermes_cli/test_update_venv_health.py -q— new suite plus the existing guard suite (the refactored_detect_venv_python_processesbehavior).hermes update. Before this PR: after "Stopping Windows gateway process(es)…" the venv guard fires on the orphaned MCP/perfmon children and exits 2 on every retry. After: the pause step prints→ Reaped N venv helper process(es) the gateway left running (MCP servers, perfmon)and the update proceeds; the helpers are back after the gateway resumes.%LOCALAPPDATA%\hermes\.hermes-update-in-progresscontaining a dead PID + timestamp, runhermes update— it prints→ Cleared a stale update-in-progress marker …and continues. With a live PID in the marker it is left untouched.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — targeted update suites; 4 pre-existing failures reproduce identically on an unmodified checkout on this host (2 ×test_update_venv_health.pybuild a POSIXvenv/bin/pythonwithout patching_is_windows, so they fail on any real Windows host; 2 ×test_cmd_update.pyfire the real venv guard when actual venv processes are running on the test machine)Documentation & Housekeeping
docs/, docstrings) — docstrings on all new helpers; N/A otherwisecli-config.yaml.exampleif I added/changed config keys — N/A (no config changes)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A_is_windows()-gated; POSIX paths are no-ops with explicit testsScreenshots / Logs
Guard output from the wedged state this fixes (2026-07-09, live install):
The listed PIDs are children the just-paused gateway left behind — closing every Hermes window does not clear them, so the update could never proceed without manual
taskkillor--force-venv.🤖 Generated with Claude Code