diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index e23b07a16e..9bbfa17693 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -10815,7 +10815,18 @@ def _cmd_update_impl(args, gateway_mode: bool): ) # Check if any pre-update serve/dashboard runtimes survived on - # pre-update code generations (#100479). + # pre-update code generations (#100479). This is the SUCCESS-path + # twin of the abort-recovery probe above: the restart phase only + # restarts units, so an sshd-spawned `serve --isolated` or a manual + # `hermes serve` (no unit) is left running its pre-update + # sys.modules graph — and its cron ticker keeps firing agent jobs + # that ImportError on every symbol added in the pulled range. Runs + # AFTER the dashboard cleanup so a manual dashboard that cleanup + # killed and respawned is (correctly) not a survivor. The rows also + # feed the plan-vs-execution reconciliation below, so a survivor is + # escalated (exit 1) instead of merely printed. ``None`` means the + # probe itself failed; the reconciliation then stays fail-closed. + _stale_serve_rows: "list | None" = None try: _stale_serve_rows = _surviving_pre_update_serve_runtimes(_pre_update_plan) if _stale_serve_rows: @@ -10932,6 +10943,13 @@ def _cmd_update_impl(args, gateway_mode: bool): externally_supervised_profiles=externally_supervised_profiles, killed_pids=killed_pids, failed_units=failed_or_stale_units, + # Serve/dashboard runtimes reconcile by incarnation + # liveness, not by the gateway's unit names (#100479). + stale_serve_pids=( + {row.get("pid") for row in _stale_serve_rows} + if _stale_serve_rows is not None + else None + ), ) if report_unaccounted_runtimes(_runtime_outcomes): gateway_fleet_restart_incomplete = True diff --git a/hermes_cli/update_inventory.py b/hermes_cli/update_inventory.py index be2889189e..1e2528e86c 100644 --- a/hermes_cli/update_inventory.py +++ b/hermes_cli/update_inventory.py @@ -425,6 +425,49 @@ def print_update_plan(plan: UpdatePlan) -> None: ) +_SERVE_KINDS = ("serve", "dashboard") + + +def _serve_unit_matches_profile(profile: str, unit: object) -> bool: + """Does *unit* name a ``hermes-serve*``/``hermes-dashboard*`` unit for *profile*? + + Serve/dashboard runtimes have their OWN unit vocabulary; the gateway's + ``hermes-gateway*`` names never cover them (#100479). Exact names only — + ``work`` must not claim ``hermes-serve-workbench`` — and a scope prefix + (``user/hermes-serve``) is tolerated because the restart phase records + scope-qualified identities in some lists. + """ + name = str(unit).removesuffix(".service") + if "/" in name: + name = name.rsplit("/", 1)[-1] + if profile == "default": + return name in {"hermes-serve", "hermes-dashboard"} + return name in {f"hermes-serve-{profile}", f"hermes-dashboard-{profile}"} + + +def _serve_runtime_outcome( + r: RuntimeRecord, + *, + killed: set, + failed_set: set, + restarted_set: set, + stale_serves: "set | None", +) -> str: + """Outcome for one serve/dashboard runtime — never the gateway's.""" + if r.pid is not None and r.pid in killed: + return "stopped" + if any(_serve_unit_matches_profile(r.profile, u) for u in failed_set): + return "failed" + if stale_serves is not None: + # Incarnation-verified: the pre-update process is gone (replaced by + # its unit / the dashboard cleanup respawn / the Desktop app) or it + # is still alive on pre-update code. + return "unaccounted" if r.pid in stale_serves else "restarted" + if any(_serve_unit_matches_profile(r.profile, s) for s in restarted_set): + return "restarted" + return "unaccounted" + + def match_runtime_outcomes( plan: "UpdatePlan", *, @@ -433,6 +476,7 @@ def match_runtime_outcomes( externally_supervised_profiles: list, killed_pids: set, failed_units: list, + stale_serve_pids: "set | None" = None, ) -> list[dict[str, Any]]: """Reconcile the plan's runtimes against what the restart phase DID. @@ -450,6 +494,18 @@ def match_runtime_outcomes( ``unaccounted`` — the plan saw it and NO bookkeeping mentions it: the blind-spot tripwire (same philosophy as the fleet matrix's DOWN row). Never raises; on any probe error returns what it has. + + Serve/dashboard runtimes are reconciled in their OWN vocabulary + (#100479): a ``hermes-serve*``/``hermes-dashboard*`` unit, a killed + PID, or — when the caller passes ``stale_serve_pids`` (the + ``(pid, create_time)``-verified survivor probe, + :func:`hermes_cli.update_abort_recovery._surviving_pre_update_serve_runtimes`) + — liveness: a pre-update serve whose incarnation is gone was replaced + (unit restart, dashboard cleanup respawn, Desktop respawn) and counts as + ``restarted``; one still alive is ``unaccounted``. They never borrow the + gateway's outcome: ``relaunched_profiles`` and ``hermes-gateway*`` name a + different process that shares the profile, nothing more. Without the + probe result, an untouched serve stays ``unaccounted`` (fail closed). """ outcomes: list[dict[str, Any]] = [] try: @@ -458,11 +514,31 @@ def match_runtime_outcomes( relaunched = set(relaunched_profiles or []) external = set(externally_supervised_profiles or []) killed = {int(p) for p in (killed_pids or set())} + stale_serves = ( + {int(p) for p in stale_serve_pids} if stale_serve_pids is not None else None + ) for runtime in plan.runtimes: r = runtime if isinstance(runtime, RuntimeRecord) else None if r is None: continue + if r.kind in _SERVE_KINDS: + outcomes.append( + { + "kind": r.kind, + "profile": r.profile, + "pid": r.pid, + "mechanism": r.restart_via, + "outcome": _serve_runtime_outcome( + r, + killed=killed, + failed_set=failed_set, + restarted_set=restarted_set, + stale_serves=stale_serves, + ), + } + ) + continue outcome = "unaccounted" # The bare "hermes-gateway" unit name is gateway-specific: a # serve/dashboard runtime that merely shares the default @@ -525,8 +601,14 @@ def report_unaccounted_runtimes(outcomes: list[dict[str, Any]]) -> bool: f" — planned mechanism: {o['mechanism']}" ) print(" Restart them manually, then verify:") - print(" hermes gateway restart # active profile") - print(" hermes -p gateway restart # named profile") + if any(o.get("kind") not in _SERVE_KINDS for o in missed): + print(" hermes gateway restart # active profile") + print(" hermes -p gateway restart # named profile") + if any(o.get("kind") in _SERVE_KINDS for o in missed): + # A serve/dashboard is not reachable by any `gateway restart` + # command (#100479): name the process, not the wrong verb. + print(" systemctl --user restart hermes-serve.service # unit-managed serve") + print(" relaunch `hermes serve` / `hermes dashboard` / the Desktop app") return True diff --git a/tests/hermes_cli/test_restart_plan_reconciliation.py b/tests/hermes_cli/test_restart_plan_reconciliation.py index 15d53cf175..b87cc98f7a 100644 --- a/tests/hermes_cli/test_restart_plan_reconciliation.py +++ b/tests/hermes_cli/test_restart_plan_reconciliation.py @@ -181,6 +181,102 @@ def test_unmanaged_serve_runtime_under_default_profile_is_unaccounted(): assert report_unaccounted_runtimes(outcomes) is True +def _serve(profile: str, pid: int, kind: str = "serve") -> RuntimeRecord: + return RuntimeRecord( + kind=kind, + profile=profile, + pid=pid, + supervisor="manual-serve", + restart_via=_restart_mechanism("manual-serve", profile), + ) + + +def test_serve_never_borrows_relaunched_or_external_gateway_profile(): + """Sibling site of #100479: the relaunched_profiles / external-supervisor + bookkeeping is gateway vocabulary too. A manual gateway relaunch under + ``default`` (or a named profile) says nothing about a serve that shares + the profile name.""" + outcomes = match_runtime_outcomes( + _plan(_rt("default", 100), _serve("default", 900), + _rt("work", 101), _serve("work", 901, kind="dashboard")), + restarted_services=[], relaunched_profiles=["default"], + externally_supervised_profiles=["work"], killed_pids=set(), failed_units=[], + ) + by_pid = {o["pid"]: o["outcome"] for o in outcomes} + assert by_pid == { + 100: "restarted", 900: "unaccounted", 101: "restarted", 901: "unaccounted" + } + + +def test_named_profile_serve_does_not_match_gateway_profile_unit(): + """``hermes-gateway-work.service`` restarted must not credit the ``work`` + serve — the old substring match (``"work" in unit``) did exactly that.""" + outcomes = match_runtime_outcomes( + _plan(_rt("work", 101, supervisor="systemd"), _serve("work", 901)), + restarted_services=["hermes-gateway-work.service"], relaunched_profiles=[], + externally_supervised_profiles=[], killed_pids=set(), failed_units=[], + ) + by_pid = {o["pid"]: o["outcome"] for o in outcomes} + assert by_pid == {101: "restarted", 901: "unaccounted"} + + +def test_serve_reconciles_against_its_own_unit_vocabulary(): + """A serve IS covered when a ``hermes-serve*`` unit for its profile was + restarted (or failed) — scope-qualified identities included.""" + outcomes = match_runtime_outcomes( + _plan(_serve("default", 900), _serve("work", 901), + _serve("ops", 902, kind="dashboard"), _serve("qa", 903)), + restarted_services=["hermes-gateway", "user/hermes-serve", + "hermes-serve-work.service", "hermes-dashboard-ops"], + relaunched_profiles=[], externally_supervised_profiles=[], + killed_pids=set(), failed_units=["hermes-serve-qa.service"], + ) + by_pid = {o["pid"]: o["outcome"] for o in outcomes} + assert by_pid == {900: "restarted", 901: "restarted", 902: "restarted", 903: "failed"} + # exact names: ``work`` must not claim ``hermes-serve-workbench`` + outcomes = match_runtime_outcomes( + _plan(_serve("work", 901)), + restarted_services=["hermes-serve-workbench.service"], relaunched_profiles=[], + externally_supervised_profiles=[], killed_pids=set(), failed_units=[], + ) + assert outcomes[0]["outcome"] == "unaccounted" + + +def test_serve_outcome_follows_incarnation_probe_when_provided(): + """With the (pid, create_time) survivor probe result, liveness decides: + a pre-update serve that is gone was replaced (restarted); one still + alive is unaccounted — even when a hermes-serve unit was restarted.""" + plan = _plan(_serve("default", 900), _serve("default", 901, kind="dashboard")) + outcomes = match_runtime_outcomes( + plan, restarted_services=["hermes-serve.service"], relaunched_profiles=[], + externally_supervised_profiles=[], killed_pids=set(), failed_units=[], + stale_serve_pids={900}, + ) + by_pid = {o["pid"]: o["outcome"] for o in outcomes} + assert by_pid == {900: "unaccounted", 901: "restarted"} + # killed pid still wins as "stopped"; probe None => fail closed + outcomes = match_runtime_outcomes( + plan, restarted_services=[], relaunched_profiles=[], + externally_supervised_profiles=[], killed_pids={901}, failed_units=[], + stale_serve_pids=None, + ) + by_pid = {o["pid"]: o["outcome"] for o in outcomes} + assert by_pid == {900: "unaccounted", 901: "stopped"} + + +def test_unaccounted_serve_report_names_serve_remedy_not_gateway_restart(capsys): + outcomes = match_runtime_outcomes( + _plan(_serve("default", 900)), + restarted_services=["hermes-gateway"], relaunched_profiles=[], + externally_supervised_profiles=[], killed_pids=set(), failed_units=[], + ) + assert report_unaccounted_runtimes(outcomes) is True + out = capsys.readouterr().out + assert "serve [default] pid 900" in out + assert "hermes-serve.service" in out + assert "hermes gateway restart" not in out + + def test_mixed_fleet_only_the_missed_one_escalates(capsys): outcomes = match_runtime_outcomes( _plan( diff --git a/tests/hermes_cli/test_update_fleet_restart_pending.py b/tests/hermes_cli/test_update_fleet_restart_pending.py index 9e5a52f208..d5da6456e9 100644 --- a/tests/hermes_cli/test_update_fleet_restart_pending.py +++ b/tests/hermes_cli/test_update_fleet_restart_pending.py @@ -334,6 +334,70 @@ def test_clean_update_warns_about_surviving_pre_update_serve_runtime( assert "pre-update code" in out +def test_clean_update_escalates_surviving_serve_as_unaccounted( + monkeypatch, tmp_path, capsys +): + """#100479 end to end: the plan inventoried a gateway (restarted through + ``hermes-gateway.service``) and an unmanaged ``serve`` on the same + default profile. The serve survives the update as the SAME process, so + the update must (1) warn, (2) reconcile it as ``unaccounted`` instead of + borrowing the gateway's restart, and (3) exit 1 with a ``partial`` + receipt — not print a clean success.""" + from hermes_cli.update_inventory import ( + RuntimeRecord, UpdatePlan, _restart_mechanism, + ) + import hermes_cli.update_inventory as ui + + args = _update_args() + _patch_update_deps(monkeypatch, tmp_path, _make_head_moved_side_effect()) + + plan = UpdatePlan() + plan.runtimes = [ + RuntimeRecord(kind="gateway", profile="default", pid=4444, + supervisor="systemd", + restart_via=_restart_mechanism("systemd", "default")), + RuntimeRecord(kind="serve", profile="default", pid=5555, + supervisor="manual-serve", + restart_via=_restart_mechanism("manual-serve", "default"), + detail={"create_time": 1000.0}), + ] + monkeypatch.setattr(ui, "collect_runtime_inventory", lambda: plan) + # The restart phase's own bookkeeping says the gateway unit restarted + # (systemd branch is stubbed off in _patch_update_deps, so feed it here). + real_match = ui.match_runtime_outcomes + + def _match(p, **kw): + kw["restarted_services"] = list(kw.get("restarted_services") or []) + [ + "hermes-gateway.service" + ] + return real_match(p, **kw) + + monkeypatch.setattr(ui, "match_runtime_outcomes", _match) + # Real survivor probe semantics against a fake ledger: pid 5555 is still + # the same incarnation the plan recorded. + import hermes_cli.process_identity as pi + + monkeypatch.setattr( + pi, "ledger_entries", + lambda **_k: [{"pid": 5555, "purpose": "serve", "create_time": 1000.0}], + ) + + with pytest.raises(SystemExit) as excinfo: + hermes_main.cmd_update(args) + assert excinfo.value.code == 1 + + out = capsys.readouterr().out + assert "pid 5555" in out and "pre-update code" in out + assert "Planned runtimes the restart phase never touched" in out + assert "serve [default] pid 5555" in out + + latest = get_hermes_home() / "logs" / "update_receipts" / "latest.json" + receipt = json.loads(latest.read_text(encoding="utf-8")) + assert receipt["outcome"] == "partial" + by_pid = {o["pid"]: o["outcome"] for o in receipt["runtime_outcomes"]} + assert by_pid == {4444: "restarted", 5555: "unaccounted"} + + def test_interrupt_between_pull_and_restart_leaves_marker( monkeypatch, tmp_path ):