fix(update): recover the hermes serve generation after an aborted restart phase (salvage #96235) - #100174
Closed
kshitijk4poor wants to merge 4 commits into
Closed
fix(update): recover the hermes serve generation after an aborted restart phase (salvage #96235)#100174kshitijk4poor wants to merge 4 commits into
kshitijk4poor wants to merge 4 commits into
Conversation
The fresh-process recovery boundary added for NousResearch#92145 only reaches gateway profiles. `hermes serve` -- the runtime that hosts `tui_gateway.server`, and the process the original report saw failing every chat turn -- is not a gateway profile, so no `gateway restart` command can reach it and the gateway-only `collect_fleet_versions` read-back cannot see it either. The spawn-ledger collector classifies serve/dashboard runtimes purely by spawner liveness, and a systemd-launched `hermes serve` sets neither HERMES_SPAWN nor HERMES_PARENT_PID, so it is recorded as `manual-serve` and the recovery partition skips it as unrecoverable. The result is an update that clears its incomplete flag on gateway coverage alone while a live serve process keeps serving the pre-update module graph. - restart active `hermes-serve*` systemd units from the fresh child, enumerated from systemd rather than from the misclassifying inventory, and verify a changed MainPID on an active unit before claiming coverage; - report any pre-update serve/dashboard process that is still the same process, and never kill one -- a manual or Desktop-owned serve has no relaunch authority; - require every runtime family, not just the gateway leg, before a fresh-process recovery may clear the incomplete flag; - persist serve-unit outcomes and surviving runtimes in the update receipt.
…ackends `_kill_stale_dashboard_processes(restart_managed=True)` returned as soon as `_restart_managed_dashboard_service()` handled `hermes-dashboard.service`. On a host that runs both that unit and `hermes-serve.service` -- the exact unit set in NousResearch#92145 -- the serve backend hosting `tui_gateway` was never scanned, never stopped and never restarted, so it kept its pre-update `sys.modules` after the checkout advanced. The early return exists so the dashboard's own PID is not raw-killed (systemd reads our SIGTERM as a clean stop). That only requires excluding the unit, which the `already_restarted_units` filter below already does. Record the unit as handled and continue the pass instead of ending it.
Review on NousResearch#96235: discovery distinguished `(scope, unit)`, but the skip payload and the reported outcomes reduced that to the bare service name. `user/hermes-serve.service` and `system/hermes-serve.service` are two different processes, so a single unqualified token could suppress recovery of both: if the user-scope unit was already settled when the restart phase aborted, the stale system-scope unit was never restarted and nothing downstream reported it. Scope now travels with the unit end to end: - the in-process systemd loop records a scope-qualified twin of `restarted_services` (`restarted_scoped_units`) while the bare-name list keeps its existing vocabulary for the fleet probe and the receipt; - the recovery payload carries `{"scope", "unit"}` objects, and the child keys discovery, skips, outcomes and accounting by `(scope, base)`; - `verified` / `failed` — and therefore the receipt and the completion predicate — report `user/hermes-serve`, never a bare name; - an entry with no scope (a payload written by a pre-update interpreter) stays unqualified and is read as scope-agnostic, and an unrecognized scope drops the skip rather than honouring it: dropping a skip can only cost one more restart-and-verify, honouring an unreadable one can leave a stale generation running. Also from review: the survivor probe compared PIDs alone while the plan discarded the process incarnation, so a new serve that reused the planned PID read as the pre-update survivor. The inventory now records the ledger's `create_time` in the serve/dashboard runtime detail and the probe compares `(pid, create_time)`, still failing closed when either side has none. Finally, abort recovery moves out of the update monolith into `hermes_cli/update_abort_recovery.py` (417 lines) with `update_cmd` re-exporting the names `hermes_cli.main` and the update flow address. `update_cmd.py` ends up 75 lines smaller than the PR's base commit instead of 249 lines larger. Tests: dual-scope same-name regressions in both directions, proof that no systemctl verb reaches an already-settled scope, per-scope outcomes, the legacy unqualified shape, the qualified payload shape, scope-qualified completion accounting, PID-reuse vs. same-incarnation survivors, and the inventory carrying `create_time`. Refs NousResearch#92145 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YQ9oCBKgAMHSGG8CLEHLMC
alt-glitch
added
type/bug
Contributor
Duplicate of #100165: both are the same salvage for recovering hermes-serve after an aborted update restart. |
… tests Two CI failures against current main: - test_abort_recovery_does_not_restart_manual_only_fleet: capability alone (linux + systemctl) made recover_serve True, so a manual-only fleet spawned the fresh recovery child. Gate the serve pass on the plan actually containing a serve/dashboard runtime (_plan_has_serve_runtimes) — restores the pre-NousResearch#92145 no-spawn contract for hosts with nothing to recover, and unchanged behavior when a serve backend exists. - test_user_scope_restart_never_falls_back_to_system_or_sudo: pinned the OLD early-return contract this PR deliberately removes (the scan must keep running to find serve backends the unit restart didn't cover). Updated to stub the scan's read-only per-PID probes and assert the scan runs while nothing is killed and no sudo/system fallback occurs.
Contributor
Author
|
Pushed a follow-up commit fixing both CI failures: (1) serve-unit recovery now gated on the plan actually containing a serve/dashboard runtime — manual-only fleets keep the no-spawn contract; (2) the stale-dashboard test updated for this PR's intentional removal of the early return (scan continues after the managed restart, dashboard PID excluded via already_restarted_units). 152/152 targeted tests green locally. |
Contributor
Author
|
Closing in favor of the maintainer's competing salvage #100165 (created first, green, and it takes the deliberate opposite stance on the manual-only-fleet question — serve authority spawns the recovery child regardless of plan contents, pinned by test_serve_only_fleet_still_spawns_the_recovery_child). One note worth carrying: this branch's follow-up also aligned test_user_scope_restart_never_falls_back_to_system_or_sudo with the removed early-return in dashboard_procs — worth checking that #100165 covers the same sibling test. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Salvage of #96235 by @JoaoMarcos44 onto current main; all three commits cherry-picked cleanly with authorship preserved.
After an aborted
hermes updaterestart phase, recovery now also restartshermes-serve*systemd units. Previously the abort-recovery path only restarted gateway profiles via the fresh child —hermes serve(which hoststui_gateway.serverand is not a gateway profile) kept running pre-updatesys.modulesagainst the new checkout, producing the mixed-module ImportError class from #92145.Changes
hermes_cli/update_restart_recovery.py: serve-unit restart pass — scope-qualified user/system identities, MainPID-change verification (rc 0 alone is not proof of replacement; polls bounded 10×1s).hermes_cli/update_abort_recovery.py(new, ~423 lines of which ~200 are MOVED from update_cmd.py): abort-recovery logic relocated into a bounded module so update_cmd.py doesn't grow another authority surface; survivor detection via process incarnation (pid+create_time, same 2.0s threshold asprocess_identity); stdlib-only module imports (fresh-child safe).hermes_cli/dashboard_procs.py: managed-dashboard early return now records the unit inalready_restarted_unitsso serve backends aren't skipped.tests/hermes_cli/test_update_serve_generation_recovery.py(new, ~1228 lines).Validation
Size note: +2157 is ~1228 test lines + ~200 moved lines; net new logic ≈450 lines for a crash-safety boundary.
Closes #96235