fix(update): recover the hermes serve generation after an aborted restart phase - #96235
JoaoMarcos44 wants to merge 3 commits into
Conversation
alt-glitch
added
type/bug
|
I ran the published Linux/user-systemd reproduction packet against the exact PR head. Its current revision supports explicit RED/GREEN adjudication and checks the old PID with Pins and environment
RED — pinned
|
| Observation | Result |
|---|---|
| Checkout reached synthetic generation B | yes |
| Target imports cleanly in a fresh interpreter | yes |
Injected mixed-sys.modules restart failure occurred |
yes |
| Unit remained active | yes |
| PID | 64873 -> 64873 |
| Start timestamp | unchanged |
| Effective generation after update | A |
| Old PID alive after adjudication | yes |
This reproduces the issue: disk is at B while the same active serve process continues executing A.
GREEN — main + PR head
| Observation | Result |
|---|---|
| Checkout reached synthetic generation B | yes |
| Target imports cleanly in a fresh interpreter | yes |
Same injected mixed-sys.modules failure occurred |
yes |
| Unit remained active | yes |
| PID | 65815 -> 66511 |
| Start timestamp | changed |
| Effective generation after recovery | B |
| Old PID alive after adjudication | no |
The PR therefore passes the requested real-path witness for the Linux/user-systemd hermes serve case: after the in-process restart aborts, the fresh recovery path replaces the generation-A process and the active unit serves generation B.
One non-blocking provenance note: #87859 already owns normal-path restart of hermes-serve* units. I do not read this PR as duplicating that outcome: #96235 covers fresh-process recovery after the normal in-process restart phase aborts. It does repeat some systemd enumeration/restart mechanics; stating why that logic cannot safely be reused through the potentially mixed module graph would make the distinction explicit.
|
@cervantesh — thanks for running the harness against the actual PR head, that RED→GREEN pair is exactly the witness the strict closure predicate asked for. Adding the PR's own validation data alongside it, since it corroborates the same claim from a different angle:
None of that is a live-manager run — that's exactly the gap your reproduction package closed. Good pairing: unit tests prove the predicate logic in isolation, your harness proves it against a real systemd unit through the actual On the #87859 provenance note: not reused deliberately, not by oversight. #87859's |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head d9b36438d0cf11c081236a5cab96a2701f4b2510 against live main@091cc0e8be6252d979cbf2f0875ecf3b60850056. The branch is 2 commits ahead / 35 behind its actual live-main merge base 93a29d110db7724f18959cb941aae3520fb587ac; the 35 main-only commits are path-disjoint from this PR's seven changed files, so I did not find a landing-edge semantic collision in the current drift. There were no submitted reviews on this PR when I started.
The recovery direction is strong. Moving abort recovery into a clean interpreter is the right boundary after the updater has already proven its own module graph can no longer be trusted. The changed-MainPID + active-unit witness is substantially stronger than treating systemctl restart rc=0 as proof, the stale-runtime survivor leg makes completion fail closed instead of laundering a gateway-only recovery into success, and the external cervantesh RED→GREEN systemd witness is unusually useful: it proves the actual hermes serve generation changes rather than only proving a mocked command shape. The #94392/#95930 fresh-process lineage is being extended coherently, and #87859 / chelsealong remains the normal-path serve-restart provenance rather than being silently claimed as new work here. Nice work on those boundaries. 🚀
I do have one P1 authority defect with two concrete manifestations: the fresh child weakens a qualified update plan into ambient systemd names, then uses those names as mutation authority.
P1 — ambient unit discovery can both widen and collapse recovery scope
hermes_cli/update_restart_recovery.py::restart_serve_units() says explicitly that it restarts every active hermes-serve* unit. It enumerates both the user and system managers from _systemctl_scopes(), discovers units with list-units, and then calls restart for every matching active unit. The recovery payload contains only serve_units: {recover, skip}; it carries no positive allowlist derived from the pre-update UpdatePlan, no expected old MainPID, no install/root identity, and no scope-qualified owner.
That creates the widening side of the defect. A failed update for install A can discover an active hermes-serve-lab.service in the same systemd namespace that was never present in A's pre-update plan and restart it anyway. Name matching proves "this looks like a Hermes serve unit"; it does not prove "this update owns this exact runtime." This is the same discovered_target_outside_scope / proof-scope-equals-mutation-scope class tracked by the transactional updater work in #88683/#91277. The fresh process absolutely should rediscover state after an abort, but discovery cannot manufacture new mutation authority.
The collapse side is in the same function. The implementation correctly recognizes that hermes-serve.service in the user manager and hermes-serve.service in the system manager are two different processes (seen is keyed by (scope, base), and the new test explicitly says both scopes must be proven). But skip_units is reduced to a bare base-name set, and the skip check is base in skipped. The normal in-process restart bookkeeping likewise records only svc_name. If the user-scope hermes-serve.service was already restarted before the phase aborts while the same-named system-scope unit is still stale, the fresh child receives one unqualified skip token and suppresses recovery of both processes. The dual-scope regression currently does not combine same-name scopes with an already-restarted/skip case.
Required repair: make the recovery target a qualified immutable object, not a unit string. The pre-mutation plan / restart debt should hand the fresh child an allowlist carrying at least {scope, unit, expected_old_main_pid} and the physical install/root identity (or the equivalent stable runtime owner already available in the fleet plan). Rediscovery may verify those exact targets, but a matching unit outside that allowlist must remain untouched. Carry the same qualified identity through restarted_services, the skip/already-settled receipt, verified/failed, and completion accounting; do not project user/system scope away and reconstruct it later.
Please add deterministic regressions for all three edges:
- a matching active
hermes-serve-*unit that is not in the pre-update recovery plan receives zero restart calls; - user + system managers both contain
hermes-serve.service, only the user-scope target is already settled/skipped, and the authorized stale system-scope target is still restarted and proven; - the same dual-scope shape with only one scope authorized proves the other scope is not mutated even though its name matches.
One smaller correctness note in the survivor leg: the pre-update RuntimeRecord persists PID but not process create-time, while the post-abort comparison reduces the live spawn ledger back to PID. ledger_entries() correctly validates each current (pid, create_time), but after PID reuse a newly registered serve can still reuse the planned numeric PID and be reported as the old survivor because the pre-update create-time was discarded. That is fail-safe (it can leave the update incomplete, not create a false success), so I would not block separately on it, but carrying the process incarnation in the same qualified recovery object fixes this cleanly.
Hard architecture gate — do not grow the update godfile again
This head still has live code at hermes_cli/update_cmd.py:10000+; this PR adds 249 lines there. The repository's development contract is a hard 2K ceiling and a killed/sharded file is never allowed to regrow. The new update_restart_recovery.py module is exactly the right kind of bounded owner; keep pushing the new abort-recovery orchestration/receipt logic through that or another sub-2K update-transaction module instead of adding another authority surface to the monolith. This should be a composition/restack, not a request to discard the good recovery implementation.
Interlocks / landing order
- #92145 remains the source incident. Keeping this PR as
Refs, notFixes, is correct because this slice is systemd-specific and does not claim the wider transactional updater. - #95930 (salvaged #94392) is the direct fresh-process parent and should remain the recovery-process owner.
- #87859 is complementary normal-path serve restart work; preserve
chelsealong's #83595 lineage. The two call paths are intentionally distinct because the normal updater interpreter may already be compromised after the abort. - #88683/#91277 own the broader transactional plan/fleet authority. This PR should consume their qualified-target model rather than create a second ambient inventory authority.
- #92091's control-plane direction is adjacent: as runtime ownership moves toward explicit control/state, recovery should converge on that owner rather than service-name inference.
Exact-head evidence
Docker 33058840694 and Nix 33058840706 are green. CI 33058841816 is cancelled: the affected-area job was cancelled during checkout and most required Python/JS/OS lanes were skipped. The aggregate required-check job showing success does not turn a cancelled exact-head CI object into a green commit. The subsequent label-rerun workflows are skipped. So this exact head does not yet meet the repository's all-commits-green acceptance gate; please rerun the full CI matrix after the semantic repair and use that final head as the only acceptance receipt.
The generation-replacement work itself is careful and the live serve witness is compelling. The remaining issue is making the thing that says "this process is stale" also prove "this update owns this exact process" all the way to the systemctl restart mutation. Once that scope identity is preserved, this is a much stronger recovery slice.
|
I checked the review findings against exact head Confirmed and in scope hereThe dual-scope That should be repaired on this PR by preserving scope-qualified identity through the already-settled/skip boundary and the corresponding The exact-head CI object also needs a real rerun. The aggregate check is green, but the affected-area checkout was cancelled and the Python/lint/OS lanes were consequently skipped. The final acceptance receipt should come from the repaired head. Valid concern, but broader than this incident sliceThe fresh serve pass currently discovers every active Changing that contract end to end would require widening inventory, normal restart bookkeeping, the fresh-process protocol, receipts, and completion accounting. I would route that class to the existing owners — #90144 for proof/mutation scope, #90145 for generation/incarnation fencing, and #88683 for the transactional deployment plan — rather than silently turning #96235 into the full updater-authority redesign. A concrete Likewise, carrying process create-time would remove the survivor false-positive under PID reuse, but the current behavior is fail-safe: it can retain an incomplete result rather than manufacture successful recovery. It is useful hardening, not a separate blocker for the demonstrated systemd incident. Evidence boundaryThe published RED→GREEN witness remains valid for the Linux/user-systemd case it actually exercised: pinned Finally, further extraction from My proposed disposition is therefore:
|
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
d9b3643 to
5664e45
Compare
|
@andrexibiza @cervantesh — repaired and pushed. New head 1. The dual-scope collision is fixed — scope is now the identity, everywhereYou were both right, and the collapse side was the real defect: discovery keyed Scope now travels with the unit through every boundary, and is never projected away and reconstructed:
Two deliberate decisions inside that:
2. Survivor identity is now the process incarnation, not the PID@andrexibiza's smaller note is fixed rather than deferred: 3. The godfile: this PR now shrinks
|
|
I reran the published Linux/user-systemd packet against the repaired and rebased head requested above. Exact pins and environment
main had advanced one commit beyond the PR base. That commit is #94126 and touches the dashboard reverse-proxy/configuration surface, not the updater/recovery/systemd path, but I included it in the tested composition. RED — current mainThe injected mixed-module ImportError occurred and the checkout reached synthetic generation B. The unit remained active, but PID 81620 stayed unchanged, its start timestamp stayed unchanged, effective generation remained A, and the original PID was still alive. Result: RED reproduced on current main. GREEN — current main plus PR headThe same injected ImportError occurred and the unit remained active. PID 82670 was replaced by 83203, the start timestamp changed, effective generation became B, and the original PID was no longer alive. Result: GREEN verified on the exact composed head. The repaired branch preserves the real-path Linux/user-systemd single-scope recovery outcome after the rebase and qualified-identity changes. Evidence boundaryThis run exercises the reported single user-scope systemd path. It does not independently exercise the new same-name user/system dual-scope accounting or create a pre-update allowlist for ambient unit discovery; those claims remain bounded to their dedicated tests and the stated architecture limits. This is also not yet a final acceptance receipt: CI on the same PR head is currently red. The failing jobs include test_abort_recovery_does_not_restart_manual_only_fleet and test_user_scope_restart_never_falls_back_to_system_or_sudo in run 33095734349. The real-path result above remains valid for head 5664e45, but any code change made to repair CI will require a new exact-head adjudication before merge. |
Of course, imma check this later and send one more commit so u can test here |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed current exact head 5664e4512b29bad020e3ec939c3d99cb81f3a78c after the scope-qualified identity, process-incarnation, and bounded-owner repairs.
The semantic repair still reads coherently, and the refreshed Linux/user-systemd witness proves the reported single-user-scope path on this head: the pre-update process is replaced, generation B is active, and the old PID is gone. This head is nevertheless not acceptance-ready because the full Python suite is red. The two failures are deterministic stale-test contracts, not an unexplained product regression:
-
test_abort_recovery_does_not_restart_manual_only_fleetThis older gateway-only test assumes a manual-only gateway plan launches no fresh child. That assumption is no longer globally true on Linux:
_serve_unit_recovery_available()is true whensystemctlis present, and the new #92145 contract intentionally launches the child even with zero supervised gateway profiles so it can reconcile activehermes-serve*units. The test also stubssubprocess.runwithcalls.append(...), which returnsNone, so the newly legitimate child path fails when the driver readsresult.returncode.Keep this test scoped to the invariant it actually owns—manual gateway profiles are not relaunched—by pinning serve recovery unavailable in the test:
monkeypatch.setattr( abort_recovery, "_serve_unit_recovery_available", lambda: False, )
The new suite already separately proves both sides of the serve contract: a serve-only fleet spawns the child when authority exists, and no child is spawned when neither gateway nor serve recovery authority exists.
-
test_user_scope_restart_never_falls_back_to_system_or_sudoIts
find_pids.assert_not_called()assertion now contradicts the production fix. This PR deliberately removes the managed-dashboard early return: afterhermes-dashboard.serviceis restarted, the pass must continue scanning for a separate stalehermes-servebackend. That continued scan is the barrier-5 RED→GREEN repair, and the new regression suite correctly pins it.Make the scanner return
[], assert that it was called, and retain the test's actual authority contract: only the foursystemctl --usercommands occur, no system-scope orsudocommand occurs, andos.killis never called. For deterministic isolation, patch_lock_owned_serve_pidsto return an empty set as well.
Those are test-only alignment repairs; I do not see a reason to weaken or back out the new recovery behavior to satisfy the old assertions.
After the next commit, acceptance is object-bound: run the complete CI matrix on that final SHA and rerun the published RED→GREEN packet on the same final composition. The current witness remains valid evidence for 5664e451…, but it cannot serve as the receipt for a different head. No merge disposition until that final commit is fully green.
|
too large to review safely This PR changes 1068 production lines before tests and docs. Please split it or add a focused justification if it should stay together. Signed: GPT-5.6-luna-high in Codex |
|
@egilewski, thanks for raising the reviewability concern. I checked the count: the 1,068 production-line figure is total churn — 894 additions and 174 deletions — rather than 1,068 new lines. A substantial portion is the extraction of abort recovery into the bounded 423-line My reading is that the PR already provides a focused reason for keeping its central pieces together. Strict closure requires one coherent recovery outcome across:
Splitting those contracts indiscriminately could create intermediate changes that perform recovery without proving completion, or change completion semantics without the corresponding recovery authority. A possible review order is:
There is no repository-wide source-line cap that I could find, so I would treat size as a valid reviewability signal rather than, by itself, evidence of a correctness problem. If you have a specific independently mergeable boundary in mind, could you point it out? In particular, it would help to know which part can be separated without leaving restart authority, survivor detection, completion accounting, or receipt semantics temporarily inconsistent. |
alt-glitch
added
needs-decision
Review on #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 #92145 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YQ9oCBKgAMHSGG8CLEHLMC
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
5 participants
Summary
Fixes #92145
Merged #94392 moved gateway restart recovery into a fresh interpreter. The runtime the original report actually saw failing —
hermes serve— is never reached by it.hermes servehoststui_gateway.server(hermes_cli/web_server.py:17682,19884;hermes_cli/web_routers/profiles.py:644), so the reporter'sIn-place model switch failed for TUI agent: cannot import name 'opencode_provider_family'came from the serve process, not from a gateway.This PR reconciles that runtime and closes the fail-open branch that let the updater report a clean recovery while it was still serving the pre-update module graph.
Root cause
Five independent barriers keep a stale
hermes serveinvisible on currentmain:The recovery partition is gateway-only.
_gateway_recovery_partition()hands onlykind == "gateway"runtimes to the fresh child; serve/dashboard runtimes are recorded asskipped.The inventory misclassifies a unit-owned serve.
update_inventory.pyderives the serve supervisor purely from spawner liveness:supervisor = "desktop" if spawner_is_dead(entry) is False else "manual-serve". A systemd-launchedhermes servesets neitherHERMES_SPAWNnorHERMES_PARENT_PID, sospawner_is_dead()returnsNone, theis Falsetest is false, and a unit-owned backend is labelledmanual-serve— "no relaunch authority".The authoritative read-back cannot see it.
collect_fleet_versions()inspects each profile home'sgateway_state.json/ control socket. It contains no serve, dashboard or ledger lookup, so a stale serve produces no row at all — not even adownrow.Completion is declared on the gateway leg alone.
_recovery_completeclearedgateway_fleet_restart_incompleteonce every planned gateway profile was covered.The one fallback that could reach serve short-circuits.
_kill_stale_dashboard_processes(restart_managed=True)began with:_restart_managed_dashboard_service()only ever looks at_DASHBOARD_SYSTEMD_UNIT(hermes-dashboard.service). On a host running bothhermes-dashboard.serviceandhermes-serve.service— the exact unit set in the report — the dashboard unit is restarted and the function returns before_find_stale_dashboard_pids()is ever called. The serve backend is not scanned, not stopped and not restarted.Barrier 5 reproduced directly against the unpatched
origin/mainfile:The early return exists so the dashboard's own PID is not raw-killed (systemd reads our
SIGTERMas a clean stop). That only requires excluding the unit, which the existingalready_restarted_unitsfilter already does.Implementation
Serve-unit pass in the fresh process.
update_restart_recovery.restart_serve_units()enumerates activehermes-serve*units from systemd itself — never from the misclassifying inventory — restarts them, and claims coverage only after observing a changedMainPIDon an active unit. A zero exit fromsystemctl restartis not accepted as proof. Because units are enumerated from systemd, a manually launched or Desktop-owned serve owns no unit and structurally cannot enter this path.Survivor probe.
_surviving_pre_update_serve_runtimes()reports any pre-update serve/dashboard PID still present in the spawn ledger.ledger_entries()re-verifies(pid, create_time)on every read, so a match is the original process, not PID reuse and not a successor. Survivors are named with PID, kind, profile and supervisor plus the exact command that fixes them — and are never killed: a manual or Desktop-owned backend has no relaunch authority.Fail-closed completion.
_abort_recovery_is_complete()requires every runtime family: gateway profiles fully covered with nothingfailedand nothing merelyrelaunch_attempted, no failed serve unit, and no surviving pre-update runtime. An unreadable serve block from the child is treated as failed, not as "nothing to do".No more dashboard short-circuit. The managed-dashboard branch now records its unit in
already_restarted_unitsand continues the pass. The existing filter drops PIDs owned by that unit, including the one systemd just replaced.Scope-qualified identity (review).
user/hermes-serve.serviceandsystem/hermes-serve.serviceare two different processes. Discovery already distinguished them, but the already-settled skip payload and the reported outcomes reduced that to a bare unit name, so one settled scope could suppress recovery of the other and one scope's success could describe the other's outcome. The in-process loop now recordsrestarted_scoped_units, the payload carries{scope, unit}objects, andverified/failed, the receipt and the completion predicate all report<scope>/<unit>.Process incarnation (review). The inventory records the spawn ledger's
create_timefor serve/dashboard runtimes and the survivor probe compares(pid, create_time), so a new serve that reuses the planned PID is no longer reported as the pre-update survivor. Still fail-closed when either side has no incarnation.Bounded owner (review). Abort recovery lives in
hermes_cli/update_abort_recovery.py(423 lines);update_cmdonly re-exports the nameshermes_cli.mainand the update flow address.update_cmd.pyends up 75 lines smaller thanmaininstead of 249 lines larger.Receipt.
serve_units(scope-qualified) andstale_runtimesare persisted alongside the existing per-profile recovery outcome.Validation
tests/hermes_cli/test_update_serve_generation_recovery.py— 59 behavioral tests: MainPID-change verification, unchanged-PID rejection, unit that never returns active, non-zero and timing-out restarts, inactive units left alone,hermes-server.servicenever matched by thehermes-serve*glob, gateway units untouched by the serve pass, both systemctl scopes proven independently, survivor / no-survivor / unreadable-ledger cases, and every completion-predicate branch. The review round added the dual-scope pair (same unit name in both managers, only one already settled — the other is still restarted and proven), a proof that no systemctl verb beyond discovery reaches the settled scope, per-scope outcomes, the legacy unqualified skip shape, the qualified payload shape, scope-qualified completion accounting, and the survivor-incarnation cases (PID reuse, same incarnation, missing incarnation).Focused regression batch across the update/fleet/receipt/inventory/dashboard suites: 212 passed, 13 skipped, plus 2 failures that reproduce identically on the pre-change head on this Windows host (
conftestlive-system guard; the Windows resume path) and touch none of the changed code.ruff check,compileall,git diff --check: clean.Real subprocess round-trip of the recovery protocol: passes.
External RED→GREEN witness
@cervantesh ran the published Linux/user-systemd reproduction package directly against this PR head and posted the result on the PR. Recording the data here so it isn't only in a review thread.
mainat run start:1ae2c2b17156d651a3e849d1f48c662a79320449; PR head:d9b36438d0cf11c081236a5cab96a2701f4b2510; tested composition: local merge of those two parents (0a0b34c5271b2ff2b325d0f2eb84f05004c59876).Ubuntu 24.04.4 LTS / WSL2, systemd 255, Python 3.12.3.
| Observation | RED (pinned
main) | GREEN (main+ PR head) || --- | --- | --- |
| Injected mixed-
sys.modulesrestart failure occurred | yes | yes || Unit remained active | yes | yes |
| PID |
64873 -> 64873(unchanged) |65815 -> 66511(changed) || Start timestamp | unchanged | changed |
| Effective generation after recovery | A | B |
| Old PID alive after adjudication | yes | no |
This is the real-path witness for the
hermes servecase specifically: after the in-process restart aborts, the fresh recovery path in this PR replaces the generation-A process and the active unit ends up serving generation B.Proposed limits
Stated as boundaries of the claim, not as work silently left out.
Refs, notFixes. The strict closure predicate from review asked for a RED→GREEN run of the published Linux/user-systemd harness on a composed head — I couldn't run it myself (this host is Windows, its only WSL distribution is a stoppeddocker-desktopimage with no user-systemd manager), so I asked for that run rather than claiming it. @cervantesh has since run it against this exact head (see above) and it passed. My own evidence in this PR remains behavioral (unit-level and real-subprocess), not a live-manager run — the external witness is what closes that specific gap, not this PR's own test suite.Verified coverage is systemd-only.
verifiedfor a serve unit meanssystemctlreported the unit active on a newMainPID. Nothing here claims launchd, Windows SCM, s6 orservice(8)equivalence; the macOS/launchd report in the issue is a separate contract that needs its own evidence.Unmanaged runtimes are reported, never reconciled. A manual or Desktop-owned serve/dashboard is named as an incomplete-update condition with an operator command. Killing it would trade stale code for an outage.
This is generation recovery, not a transactional updater. Pre-mutation recovery authority, crash/reboot-durable transaction state, inactive-but-enabled unit inventory, and dependency/build/assets generation proof stay with [Architecture]: make install/update/bootstrap obey one transactional deployment plan #88683 and [Tracking] Fleet update reliability: one deployment plan for local, multi-profile, remote, and image-managed installs #91277. This PR does not claim them.
Serve-unit discovery is ambient, not allowlisted. The fresh pass restarts every active
hermes-serve*unit systemd reports, rather than consuming a pre-update allowlist carrying unit, scope, incarnation and physical install identity. This is the same policy the normal in-process serve restart (fix(update): restart hermes-serve systemd units alongside gateways (#83438) #87859) already uses, and today'sRuntimeRecord/UpdatePlancarries none of that object — deriving one from the current inventory would put recovery back behind themanual-servemisclassification that is barrier 2 above. The full proof-scope-equals-mutation-scope model belongs to [Architecture] Proof scope must equal mutation scope #90144 / [Architecture]: recovery and teardown must be fenced by durable generation identity #90145 / [Architecture]: make install/update/bootstrap obey one transactional deployment plan #88683; this PR states the limit instead of claiming the model.The unattended-update notification gap stays out of scope. An update that was not started from a messaging conversation still has no delivery target; that remains its own focused issue.
One pre-existing test-isolation defect observed, not fixed. Running
tests/hermes_cli/test_update_fleet_restart_pending.pybeforetest_update_receipt.py::TestCommandBoundaryFinalization::test_cmd_update_boundary_finalizes_on_early_exitfails the latter. I reproduced this in a clean worktree of the base commit93a29d110dwith none of my changes applied — it arrives with8246c4f92aand is unrelated to this PR.Relationship to existing work
fix(update): recover aborted gateway restart in a fresh process #94392 (merged as fix(update): crashed updates no longer strand stopped gateways — a fresh process settles the restart debt (#92145, salvage #94392) #95930) built the fresh-process boundary this PR extends. It covers gateway profiles; this adds the serve runtime family and the fail-closed completion that keeps a gateway-only recovery from reporting success.
fix(update): name the stale gateway pids the update already recorded #92181 improves stale-PID naming. Observability only; no overlap.
[Architecture]: make install/update/bootstrap obey one transactional deployment plan #88683 / [Tracking] Fleet update reliability: one deployment plan for local, multi-profile, remote, and image-managed installs #91277 own the transactional updater and fleet inventory architecture. Deliberately untouched.
No open PR touches the
_kill_stale_dashboard_processesearly return (checked fix(cli): kickstart launchd-supervised dashboards on update instead of respawning #89793, fix(update): handle launchd-managed dashboards during update #87989, fix(cli): prevent dashboard orphaning on non-systemd supervisors during hermes update #73489, fix(cli): don't orphan dashboard under non-systemd supervisors during update (#73379) #73420, fix: skip supervisor wrappers in dashboard scan and respawn (#73379) #73412).fix(update): restart hermes-serve systemd units alongside gateways (#83438) #87859 already restarts
hermes-serve*units on the normal in-process update path (update_cmd.py's fleet-restart loop, straightsystemctl restart, no SIGUSR1). Not reused here, deliberately: that loop lives in the same interpreter whose module graph the aborted restart phase just left mixed — the exact condition this issue reports. Calling back into it from recovery would mean bootstrapping recovery from the tree that triggered the failure.restart_serve_units()runs in the fresh child specifically to avoid importing anything from the possibly-compromised process, so it independently re-enumerateshermes-serve*units from systemd and callssystemctl restartitself. Same systemd primitives, deliberately separate call path — fix(update): recover the hermes serve generation after an aborted restart phase #96235 covers fresh-process recovery after the in-process restart phase aborts, fix(update): restart hermes-serve systemd units alongside gateways (#83438) #87859 covers the normal path where it doesn't.Root-cause infographic
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#8b0000', 'mainBkg': '#0a0204', 'primaryTextColor': '#ffccd5', 'primaryBorderColor': '#ff0038', 'lineColor': '#ff0038'}}}%% graph TD A[Blood Core: hermes update moves checkout N to N+1] --> B[Flame Node: in-process restart phase raises ImportError] B --> C[Crimson Path: fresh-process recovery] C --> D[Gateway profiles restarted and verified] C -.->|kind is not gateway| E[Serve skipped as manual-serve] B --> F[Managed dashboard fallback] F -->|early return after hermes-dashboard.service| G[Serve backend never scanned] D --> H[collect_fleet_versions reads gateway state only] E --> I[Serve holds generation N sys.modules] G --> I H -.->|no serve row exists| J[Update reports clean recovery] I --> K[Every chat turn fails: ImportError for a symbol on disk] L[Warded Core: this PR] --> M[Serve units enumerated from systemd] M --> N[Verified only on changed MainPID of an active unit] L --> O[Survivor probe names pre-update serve PIDs] L --> P[Dashboard restart no longer ends the pass] N --> Q[Completion requires every runtime family] O --> Q P --> Q Q --> R[Unproven recovery stays explicitly incomplete]