Skip to content

fix: restart dashboard service after update - #40616

Closed
mari-meiji wants to merge 1 commit into
NousResearch:mainfrom
mari-meiji:fix/restart-dashboard-systemd-after-update
Closed

fix: restart dashboard service after update#40616
mari-meiji wants to merge 1 commit into
NousResearch:mainfrom
mari-meiji:fix/restart-dashboard-systemd-after-update

Conversation

@mari-meiji

Copy link
Copy Markdown

Summary

  • restarts detected Hermes dashboard systemd units after hermes update stops stale dashboard backends
  • detects common unit names plus wildcard Hermes dashboard/webui unit files by validating ExecStart
  • falls back to the existing manual restart hint when no dashboard systemd unit is installed

Testing

  • python -m pytest tests/hermes_cli/test_update_stale_dashboard.py -q
  • python -m py_compile hermes_cli/main.py

Notes

  • Also ran python -m pytest tests/hermes_cli/test_update_stale_dashboard.py tests/hermes_cli/test_cmd_update.py -q; test_update_refreshes_repo_and_tui_node_dependencies failed because the web UI build helper was not called when the local build stamp was already current. The dashboard systemd tests passed in that run.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Jun 6, 2026

@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 addressing the dashboard-update gap; current docs explicitly support running the dashboard under systemd (website/docs/user-guide/features/web-dashboard.md:149-156). The premise is still present on current main: stale dashboard processes are stopped and only a manual restart hint is printed (hermes_cli/main.py:6101-6113, :6208-6210).

Problems

  • hermes_cli/main.py:7975 runs systemctl enable for every candidate. Candidate selection accepts known names based only on LoadState (:7898-7909), so an intentionally disabled unit becomes boot-persistent after an update.
  • Candidate selection never ties a service to a stale PID: it checks name/ExecStart (:7898-7952) and restarts after any PID was killed (:8128-8129). A dormant matching unit can therefore be started for an unrelated manual dashboard process.
  • The direct system-scope restart path omits the noninteractive privilege handling current update code uses to avoid captured polkit prompts (hermes_cli/main.py:10469-10522).

Suggested changes

  • Match services to stale PIDs via MainPID before killing them; restart only matched units.
  • Do not call enable; preserve the operator's persistence choice.
  • Reuse the existing noninteractive system-scope handling and add inactive/disabled, PID-mismatch, and unprivileged-system-scope tests.

Automated hermes-sweeper review.

Comment thread hermes_cli/main.py
timeout=10,
)
subprocess.run(
scope + ["enable", unit],

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.

enable changes persistence, not just restart state. Because known-name candidates are accepted when merely loaded (:7898-7909), this can permanently enable a unit the operator intentionally disabled. Preserve that policy; do not enable it during update.

Comment thread hermes_cli/main.py
if killed:
print(" Restart the dashboard when you're ready:")
print(" hermes dashboard --port <port>")
restart_outcomes = _restart_dashboard_systemd_services()

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.

This restart is triggered after any stale dashboard PID is killed, but candidate discovery only checks unit names/ExecStart and never verifies its MainPID was one of those PIDs. A dormant matching unit can be started for an unrelated manual dashboard. Match service MainPID to the pre-kill PID set before restarting.

@teknium1 teknium1 added 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 area/install-update Installer, updater, packaging, wheels, doctor labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing in favor of PR #72192 (merged), which restarts systemd-supervised backends by snapshotting each killed PID's owning unit from /proc/<pid>/cgroup before the kill — that's the PID↔unit matching the review here asked for, and it avoids the systemctl enable persistence change. Your PR was the first to push on the managed-unit restart gap and the wildcard-unit detection work is appreciated — credit noted in #72192's description lineage.

@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 P3 Low — cosmetic, nice to have 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants