fix(cli): restart managed dashboard service after update - #39166
Conversation
dc41497 to
1a8b0d8
Compare
|
Independently verified this is a real problem. Reproduction: My dashboard runs as Why this matters alongside #44165: The detection gap (PR #44165, issue #44035) and this service-restart gap compound. If the dashboard is launched with Verified locally: After manually restarting my dashboard service post-update, the API correctly reported the new version. The One note: the |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the correct failure mode: current main still reaches raw SIGTERM for dashboard PIDs in hermes_cli/main.py:6164-6168 after update (:11042).
Problems
hermes_cli/main.py:7816builds only system-scopesystemctlcommands, and:7852restarts only system-scope units. The reported reproduction in the PR discussion is asystemctl --userunit; that unit is not discovered here, so_restart_managed_dashboard_service()returns false and the existing raw-kill path still runs.- The new tests at
tests/hermes_cli/test_update_stale_dashboard.py:348-364assert only plainsystemctlcalls, so this user-scope gap is untested.
Suggested changes
- Probe/restart user and system scopes deliberately, reusing the scope-aware convention in
hermes_cli/gateway.py:1999-2019where appropriate. - Add a
systemctl --userregression case and retain a system-scope case if both are supported.
Automated hermes-sweeper review.
|
|
||
| def _systemctl(*args: str, timeout: int = 10) -> subprocess.CompletedProcess: | ||
| return subprocess.run( | ||
| ["systemctl", *args], |
There was a problem hiding this comment.
The reported reproduction is a systemctl --user unit, but this helper always selects the system manager. It will not discover or restart ~/.config/systemd/user/hermes-dashboard.service, then returns False and the caller still SIGTERMs the dashboard PID. Probe/restart the user scope (and system scope if intended) explicitly.
1a8b0d8 to
9a11529
Compare
austinpickett
left a comment
There was a problem hiding this comment.
Hermes Agent Review — Approve
Verified against PR head 9a11529c3. Mergeable/clean, all required CI green.
The July 14 blocking concern (helper only probed the system scope, so a systemctl --user dashboard unit was never discovered and the raw-kill path still ran) is fixed by the July 19 commit. _restart_managed_dashboard_service now probes (("--user",), ()), pins the discovered scope for is-active/is-enabled/restart, and only appends the sudo -n fallback for system-scope units so a user unit never prompts for sudo or leaks to the system manager.
Confirmed the behavior directly by driving the real helper with mocked systemctl:
- user unit present -> restarted via
systemctl --user, no sudo, no system-scope call - user probe empty, system unit present -> falls back to system scope
- no unit anywhere -> returns False so the caller still stops a manually-started dashboard
- win32 -> short-circuits False
Correctness notes:
- Returning True on a printed restart failure is intentional and correct: it blocks the
os.killfallback so systemd doesn't record a clean stop that defeatsRestart=on-failure. The failure path prints the manual fix. - The safety rule from #30271 is preserved: a failed Node refresh still leaves the running dashboard untouched.
- New tests cover user-scope success, user-scope failure (no sudo/system fallback), and restart-failure-does-not-raw-kill.
Windows note: the 8 TestFindStaleDashboardPids failures I saw locally reproduce on main too (POSIX ps path), unrelated to this PR. The new systemd tests are correctly gated under TestKillStaleDashboardPosix (skipif win32) and run green on the Linux CI slices.
The prior lgtm approve (June 21) predates these commits and the user-scope fix, so it's stale; this review supersedes it.
…h#39166) * Keep systemd dashboard alive after update * fix: restart managed dashboard in owning systemd scope
Summary
hermes-dashboard.servicethrough systemd duringhermes updateRestart=on-failurehermes dashboard --stopContext
The stale-dashboard cleanup added for
hermes updateis right for manually launched dashboards, but it is the wrong primitive when the dashboard is supervised by systemd.If update sends SIGTERM directly to the service main PID, systemd reports the unit as cleanly deactivated. With
Restart=on-failure, the dashboard stays down after a successful update even though it was configured as a durable service.This is separate from:
Test plan
/home/pi/.hermes/hermes-agent/venv/bin/python -m pytest tests/hermes_cli/test_update_stale_dashboard.py -q -o 'addopts='git diff --check upstream/main..HEAD