Skip to content

fix(update): terminate stray venv holder processes on unattended Windows update - #67229

Closed
Dolverin wants to merge 1 commit into
NousResearch:mainfrom
Dolverin:dolverin/pr-update-venv-holder-quiesce
Closed

fix(update): terminate stray venv holder processes on unattended Windows update#67229
Dolverin wants to merge 1 commit into
NousResearch:mainfrom
Dolverin:dolverin/pr-update-venv-holder-quiesce

Conversation

@Dolverin

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a permanent dead-end of unattended Windows updates at the venv-process guard.

Today the update flow pauses gateways and then refuses (exit 2) when any other process runs from the install venv. On a busy install that refusal is effectively unconditional: cron job runs spawn pythonw from venv\Scripts at all hours, and abandoned CLI sessions linger. The desktop one-click handoff exits cleanly and launches the updater, the updater dies at the guard, and the next retry simply collides with the next job — the update can never converge without manual process hunting.

Key observation: once _pause_windows_gateways_for_update() has run, the in-gateway cron ticker is stopped too, so nothing spawns replacements. The remaining holders are strays that nothing supervises — killing them is safe, and that is exactly what an unattended update should do instead of dead-ending.

This PR teaches hermes update to converge on unattended runs:

  • When holders are detected and --yes is set (the desktop bootstrap updater always passes --yes), terminate them gracefully (psutil terminate(), wait up to 20 s, then kill()), logging each PID + cmdline, then re-scan.
  • The refusal still fires when holders persist or reappear (e.g. a supervised desktop backend that respawned) — the re-scan is authoritative, so the safety property of the guard is unchanged.
  • Interactive runs (no --yes) keep the plain refusal; the message now hints that --yes clears strays automatically.
  • --force-venv semantics are untouched, and the new code path is Windows-gated (_is_windows()), no-op without psutil.

Scope note / related PRs: #61515 reaps gateway-spawned venv children inside the gateway pause (complementary — this PR covers everything the pause legitimately can't: cron job runs, stray CLIs). #62445 kills external holders from the desktop handoff (desktop path only; this PR fixes it in the update flow itself, so plain hermes update CLI runs converge too). #63318 surfaces blockers in the UI (orthogonal).

Related Issue

No tracking issue; reproduction and a live run are included below.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/main.py
    • new _terminate_venv_python_holders() (graceful → force, bounded waits, never raises)
    • guard wiring: --yes runs terminate strays and re-scan before refusing
    • refusal message gains the --yes hint; _detect_venv_python_processes docstring updated to match the new contract
  • tests/hermes_cli/test_update_venv_health.py
    • harness now returns mocks for the detect/terminate helpers
    • new cases: --yes + strays cleared → past guard; holders persist → still exit 2; interactive → never terminates; terminate → force-kill only survivors
    • two older venv-health tests made host-aware (they built a POSIX-layout fake venv without patching _is_windows, so they could only pass off-Windows)

How to Test

  1. Start a stray holder: venv\Scripts\pythonw.exe -c "import time; time.sleep(600)"
  2. Run hermes update --yes --force --branch main
  3. The log shows → Terminating venv holder PID …, the holder is gone, and the update proceeds instead of exiting 2.

Unit: pytest tests/hermes_cli/test_update_venv_health.py -q → 22 passed.

Checklist

Code

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A — docstrings updated; no user-facing docs affected
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — new behavior is Windows-gated; other platforms unchanged
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Live unattended update on an install that had been dead-ended for two days (stray holder planted deliberately):

→ Stopping Windows gateway process(es) before updating Hermes...
  ✓ Paused gateway profile(s): default
→ Clearing stray Hermes venv process(es) before the dependency sync…
  → Terminating venv holder PID 59476 (pythonw.exe): C:\...\hermes-agent\venv\Scripts\pythonw.exe -c import time; time.sleep(1800)
→ Fetching updates...
→ Found 43 new commit(s)
...
✓ Update complete!
  ✓ Restarting Windows gateway profile(s): default

…ows update

Unattended updates (bootstrap one-click passes --yes) dead-ended whenever a
transient process held the install venv: cron job runs spawn pythonw from
venv\Scripts at all hours, the venv-holder guard exited 2, and the retry on
the next click simply collided with the next job. The update flow pauses the
gateways — and with them the in-gateway cron ticker — before this guard, so
no replacements can spawn; terminate the remaining strays (graceful, then
force), re-scan, and only refuse when holders persist or reappear (e.g. a
supervised desktop backend). Interactive runs keep the plain refusal, now
with a hint that --yes clears strays automatically.

Also makes two venv-health tests host-aware (they built a POSIX-layout fake
venv but never patched _is_windows, so they only passed off-Windows).
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #61515 and #62445: this targets the hermes update --yes post-pause lifecycle, while those repair gateway-child and Desktop-handoff holder paths. The termination-policy scope needs a maintainer choice.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the post-pause venv-holder path. Current main does retain the reported refusal at hermes_cli/main.py:9847-9851.

Problems

  • hermes_cli/main.py:9916 terminates every detector match, but the detector includes the Desktop serve backend (hermes_cli/main.py:9262-9276). Current main deliberately refuses that backend because it is supervised and can respawn (hermes_cli/main.py:9836-9845). The sole immediate re-scan at hermes_cli/main.py:9917 can be clean before a later respawn, reopening the .pyd-lock race the guard protects against. The new test at tests/hermes_cli/test_update_venv_health.py:376-381 covers only a holder still present at that immediate scan.
  • This makes --yes process-destructive, while the unchanged parser help (hermes_cli/subcommands/update.py:47-51) and updating guide (website/docs/getting-started/updating.md:105) describe it as prompt automation and instruct users to close holders or use --force-venv.

Suggested changes

  • Preserve refusal for Desktop-supervised/external holders, or add a tested stable-quiescence lifecycle before update mutation.
  • Document any approved destructive --yes behavior.

Automated hermes-sweeper review.

Comment thread hermes_cli/main.py
# then re-scan. Anything that reappears (a supervised desktop
# backend) still trips the refusal below.
print("→ Clearing stray Hermes venv process(es) before the dependency sync…")
_terminate_venv_python_holders(_venv_holders)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_detect_venv_python_processes() includes the Desktop serve backend, while current main explicitly treats killing that supervised process as a race because it can respawn. A single immediate re-scan can be clean before the supervisor restarts it. Exclude supervised/external holders or add a bounded, tested quiescence check before allowing dependency mutation.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 19, 2026
@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
@Dolverin

Copy link
Copy Markdown
Contributor Author

Closing this PR because the current implementation broadens --yes into terminating every detected venv holder, including the Desktop-supervised serve backend. The immediate re-scan does not prove stable quiescence before mutation, so it can reopen the lock race the existing fail-closed guard prevents. The safer contract remains: refuse supervised/external holders, use the external Desktop handoff, and require the explicit --force-venv override when the operator accepts that risk. Any future automatic-quiescence work should be a separate, lifecycle-proven design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants