Skip to content

fix(serve): restart-friendly exit after update stops a supervised backend - #69046

Closed
bounce12340 wants to merge 1 commit into
NousResearch:mainfrom
bounce12340:fix/remote-update-restart-serve-68934
Closed

fix(serve): restart-friendly exit after update stops a supervised backend#69046
bounce12340 wants to merge 1 commit into
NousResearch:mainfrom
bounce12340:fix/remote-update-restart-serve-68934

Conversation

@bounce12340

Copy link
Copy Markdown
Contributor

What does this PR do?

When a backend update is applied (including remotely from Desktop), hermes update terminates hermes serve with SIGTERM. Under systemd Restart=on-failure, a graceful SIGTERM exit counts as a clean stop, so the service is never restarted — Desktop then polls for ~60 s and reports "Backend did not come back online" (#68934).

This PR makes the update-triggered stop restart-friendly without changing the deliberate "Hermes doesn't guess original launch args" design (_kill_stale_dashboard_processes docstring): instead of restarting anything itself, it lets the supervisor do it. The update path writes a restart marker before terminating serve/dashboard pids; on graceful shutdown the server consumes its own fresh marker and exits with code 75 (EX_TEMPFAIL — same semantics the gateway subsystem already uses in gateway/restart.py), which Restart=on-failure treats as a failure and restarts. No marker (normal shutdown, --stop, no supervisor) → behavior unchanged.

Known limitation: if several supervised serve processes are killed by one update and restart concurrently, a sibling's startup cleanup can consume the shared marker first (the survivor then exits 0). Single-server setups — the normal case — are unaffected; noted in case maintainers want per-pid marker files instead.

Related Issue

Fixes #68934

Type of Change

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

Changes Made

  • hermes_cli/serve_restart_marker.py (new) — marker read/write/consume helpers, RESTART_EXIT_CODE = 75; marker lives at $HERMES_HOME/runtime/serve_restart.json (same runtime-state convention as active_sessions.py), written with the existing atomic_json_write.
  • hermes_cli/main.py_kill_stale_dashboard_processes(restart_hint=...): writes markers before any termination signal (both the Windows taskkill and POSIX SIGTERM/SIGKILL branches). ZIP and git-pull update call sites pass restart_hint=True; the --stop call site keeps the default False so a manual stop stays clean.
  • hermes_cli/web_server.py — startup clears stale marker entries for a reborn pid (Windows taskkill /F skips graceful exit); after uvicorn returns, a consumed fresh marker exits 75 on both the POSIX and Windows runner paths.
  • website/docs/user-guide/desktop.md — systemd section now recommends Restart=on-failure/always and documents the exit-75 contract.
  • Tests: marker-before-SIGTERM ordering (fails on main), --stop writes no marker, consume hit/mismatch/expiry/missing-file cases, exit-75 vs normal-exit paths, plus a class-local POSIX ps parser fixture for the pre-existing TestFindStaleDashboardPids failures on Windows runners (the Windows wmic coverage is untouched).

How to Test

  1. python -m pytest tests/hermes_cli/test_update_stale_dashboard.py tests/hermes_cli/test_serve_command.py -q → 32 passed, 5 skipped (POSIX-only kill tests on Windows).
  2. Red/green: revert hermes_cli/main.py + hermes_cli/web_server.py only → the ordering test and exit-path tests fail; restore → green.
  3. Manual (systemd): run hermes serve under a unit with Restart=on-failure, apply a backend update from Desktop → service now restarts and Desktop reconnects; hermes serve --stop still stops it for good.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the affected test suites (test_update_stale_dashboard.py, test_serve_command.py, test_web_server.py, test_dashboard_lifecycle_flags.py — all green); full pytest tests/ -q on this machine stops at 12 pre-existing collection errors from missing optional acp deps, unrelated to this change
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11 (POSIX kill-path tests run under skipif; logic verified via mocked-signal tests)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A (no config keys added)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A (no architecture change)
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — Windows taskkill /F stale-marker hygiene; POSIX + Windows exit paths both covered
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

🤖 Generated with Claude Code

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades needs-decision Awaiting maintainer decision before any implementation labels Jul 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #69029 and #39166 address the supervised-update restart family. This patch asks the supervisor to restart through an exit marker; #69029 restarts the detected service directly. The documented shared-marker race makes this a maintainer mechanism decision, not a duplicate.

@bounce12340

Copy link
Copy Markdown
Contributor Author

Mechanism comparison for the needs-decision call, since #69029 addresses the same failure from the other side:

  • fix(update): restart systemd-supervised remote backends after killing stale processes #69029 resolves the unit via /proc/<pid>/cgroup and calls systemctl restart directly. That covers systemd-on-Linux only, and restarting a system-scope unit from the updating user's context can hit polkit/permission walls. One small correction to its description: it states there are no existing unit tests for _kill_stale_dashboard_processestests/hermes_cli/test_update_stale_dashboard.py covers that function extensively (and this PR extends it); fix(update): restart systemd-supervised remote backends after killing stale processes #69029 currently ships no tests.
  • This PR delegates the restart to whatever supervisor watches the process, via marker + exit 75 (the contract gateway/restart.py already established). That covers systemd user/system units, s6, runit, docker restart policies, etc., needs no elevated privileges, and preserves the existing "Hermes doesn't restart what it didn't launch" design (_kill_stale_dashboard_processes docstring). Trade-offs on this side: the exit-75 logic ships in the new server code, so the first update from a pre-fix version still stops cleanly (it takes effect from the following update on), plus the shared-marker race already noted in the description.
  • The two are composable if belt-and-suspenders is wanted: cgroup detection to know a process is supervised, exit-code contract to do the restart portably.

Happy to adapt this PR either way once the mechanism is picked.

…kend

- hermes update (ZIP and git-pull paths) writes a restart marker before
  terminating serve/dashboard pids; `hermes serve --stop` does not.
- serve/dashboard consumes its own fresh marker on graceful shutdown and
  exits 75 (EX_TEMPFAIL, same semantics as the gateway subsystem) so
  systemd Restart=on-failure brings it back; without a marker, or
  without a supervisor, behavior is unchanged.
- stale-marker hygiene: startup clears leftovers (Windows taskkill /F
  skips graceful exit), entries expire after 10 minutes, corrupt files
  are discarded.
- docs: recommend Restart=on-failure/always in the desktop systemd
  section.

Fixes NousResearch#68934

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bounce12340
bounce12340 force-pushed the fix/remote-update-restart-serve-68934 branch from e0d284c to a006117 Compare July 26, 2026 01:28
@bounce12340

Copy link
Copy Markdown
Contributor Author

Rebased onto current main, which now carries #39166's managed-service restart. The two compose rather than compete, so this PR is now scoped to the gap that one leaves behind.

_restart_managed_dashboard_service() runs first and returns early when it finds the unit, so on a standard hermes-dashboard.service install nothing here fires at all. The marker is only written once that lookup comes back empty:

if restart_managed and _restart_managed_dashboard_service(reason):
    return          # #39166 handled it via systemctl
...
if restart_hint:
    write_restart_markers(pids)   # only reached when it did not

That residual set is: a systemd unit under any other name (the report in #68934 describes hermes serve under a user-managed unit, and the lookup is hard-coded to hermes-dashboard.service), non-systemd supervisors (s6, runit, a container restart policy), and Windows, where the systemd path returns False immediately. The exit-75 contract needs no unit name and no privilege escalation, so it covers those without duplicating what #39166 already does.

hermes dashboard --stop requests neither flag, so a manual stop stays a clean stop.

Affected suites: 32 passed, 9 skipped (the new skips are #39166's POSIX-only systemd tests on a Windows runner).

Happy to narrow this further — or close it — if the remaining coverage isn't worth the extra surface now that the common case is handled.

@teknium1

Copy link
Copy Markdown
Contributor

Closing in favor of PR #72192 (merged), which resolves #68934 from the updater side: it snapshots each killed PID's owning systemd unit from /proc/<pid>/cgroup before the kill and restarts the unit directly — no Restart= configuration required on the unit, and it also covers manually-started (unsupervised) backends by respawning their captured argv. The exit-75/EX_TEMPFAIL marker design here was a clean supervisor-side alternative and the test coverage was solid — appreciated.

@teknium1 teknium1 closed this Jul 26, 2026
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 sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remote backend update can stop hermes serve without restarting it

3 participants