From 8130274b3770d9d15244330d5d67882a013fa92e Mon Sep 17 00:00:00 2001 From: linmukong Date: Tue, 8 Sep 2026 02:38:40 +0800 Subject: [PATCH] fix(update): discharge fleet_restart_pending when the fleet provably serves expected_sha MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _pending_fleet_restart_needed() returns True on marker existence alone. A supervisor-level gateway restart (`systemctl --user restart`, launchctl, an ops script) never passes through _clear_fleet_restart_pending_marker(), so a marker written by a pulled update survives a restart that DID bring every live gateway to the new code — and every later CLI call prints the interrupted-update warning forever, a permanent false positive that trains operators to ignore it. The comment on that branch is right that "an older receipt cannot discharge that unknown obligation" — but the obligation is not unknown. The marker records its own expected_sha, so it can be verified against the live fleet directly, with no receipt involved. _live_fleet_covers_receipt() cannot substitute: it is receipt-anchored and returns False when `owed` is empty, which is exactly the marker-only case. Hold the marker to the same evidence bar _live_fleet_covers_receipt() applies to a receipt: at least one row, and every row a `current` gateway under a known profile whose code_sha equals the marker's expected_sha, with the checkout HEAD not moved past the marker. Still keeps the marker (warns) on: - stale / down rows — the restart genuinely is owed - an all-`unknown` fleet — pre-code-identity gateways cannot prove currency (same conservatism as #88848/#74973) - a marker with no expected_sha — nothing to verify against - a newer pull that moved the checkout — it owns a fresh obligation - a probe that raises or answers empty — no proof either way Discharging deletes the marker only after every row passes; the historical receipt is left intact, since a supervisor restart is still not a successful update. Tests: discharge on a provably-current fleet; keep on stale, empty probe, unknown identity, and newer-pull-moved-checkout (which asserts the fleet is never probed once HEAD has moved). --- hermes_cli/update_cmd_fleet.py | 60 ++++++++++ .../test_update_fleet_restart_pending.py | 107 ++++++++++++++++++ 2 files changed, 167 insertions(+) diff --git a/hermes_cli/update_cmd_fleet.py b/hermes_cli/update_cmd_fleet.py index 0ffa745af2..43373a3aed 100644 --- a/hermes_cli/update_cmd_fleet.py +++ b/hermes_cli/update_cmd_fleet.py @@ -183,6 +183,64 @@ def _live_fleet_covers_receipt(expected_sha: str | None) -> bool: return False +def _read_fleet_marker_expected_sha() -> str: + """``expected_sha`` recorded in the pending marker ("" when absent/unreadable).""" + with suppress(OSError): + for line in _fleet_restart_pending_marker_path().read_text(encoding="utf-8").splitlines(): + if line.startswith("expected_sha="): + return line.split("=", 1)[1].strip() + return "" + + +def _marker_only_restart_obsolete() -> bool: + """True when the pending marker is a leftover: the fleet already runs the expected code. + + A supervisor-level restart (``systemctl --user restart``, launchctl, ops scripts) never + goes through this module's clear path, so the marker survives a restart that DID bring + every live gateway to the pulled code — and every later CLI call then prints the + interrupted-update warning forever (false positive). + + The marker is not an *unknown* obligation: it records its own ``expected_sha``, so it can + be checked against the live fleet directly — no receipt required. Hold it to the same + evidence bar ``_live_fleet_covers_receipt`` applies to one: at least one row, and every + row a ``current`` gateway under a known profile whose ``code_sha`` equals that + ``expected_sha``, with the checkout HEAD not moved past the marker. + + Keep the marker on stale/down rows, on an all-``unknown`` fleet (pre-code-identity + gateways cannot prove currency — same conservatism as the silent-failure class + #88848/#74973), on a marker with no ``expected_sha``, when a newer pull moved the + checkout, and when the probe fails or answers empty. + """ + expected_sha = _read_fleet_marker_expected_sha() + if not expected_sha: + return False # pre-expected_sha marker: nothing to verify against + checkout_sha = _current_checkout_sha() + if checkout_sha and checkout_sha != expected_sha: + return False # a newer pull moved HEAD; it owns a fresh obligation + try: + from hermes_cli.update_receipt import collect_fleet_versions + fleet = collect_fleet_versions() + except Exception as exc: + logger.debug("Fleet probe failed; keeping fleet-restart-pending marker: %s", exc) + return False + if not fleet: + return False # probe answered empty: no proof either way + for row in fleet: + if not isinstance(row, dict): + return False + profile = row.get("profile") + if not profile or profile == "unknown": + return False # unidentified runtime: the matrix cannot vouch for it + if row.get("state") != "current" or str(row.get("code_sha")) != expected_sha: + return False # stale / down / unknown-identity row still owes the restart + _clear_fleet_restart_pending_marker() + logger.debug( + "Fleet-restart-pending marker discharged: %d gateway(s) already serve %s", + len(fleet), expected_sha[:10], + ) + return True + + def _pending_fleet_restart_needed() -> bool: """Reconcile old restart obligations against current, identity-matched gateways.""" from hermes_cli.update_cmd import _current_checkout_sha @@ -191,6 +249,8 @@ def _pending_fleet_restart_needed() -> bool: # than latest.json. An older receipt cannot discharge that unknown obligation. with suppress(OSError): if _fleet_restart_pending_marker_path().is_file(): + if _marker_only_restart_obsolete(): + return False return True if not _receipt_reports_stale_runtime(): return False diff --git a/tests/hermes_cli/test_update_fleet_restart_pending.py b/tests/hermes_cli/test_update_fleet_restart_pending.py index 8ccbaa004f..95c6758690 100644 --- a/tests/hermes_cli/test_update_fleet_restart_pending.py +++ b/tests/hermes_cli/test_update_fleet_restart_pending.py @@ -609,3 +609,110 @@ def test_startup_warn_silent_when_nothing_pending(capsys): captured = capsys.readouterr() assert captured.err == "" assert captured.out == "" + + +# ── Self-heal: marker left behind by a supervisor-level restart (TRA-1180 class) ── +# +# `systemctl --user restart hermes-gateway` (weekly-update fallback, manual ops) never +# runs this module's clear path, so the marker survives a restart that DID bring the +# fleet to the pulled code — and every later CLI call warns forever. The marker must be +# discharged when (and only when) the fleet provably serves expected_sha. + + +def _patch_marker_sha(monkeypatch, disk_sha): + monkeypatch.setattr(update_cmd, "_current_checkout_sha", lambda: disk_sha) + monkeypatch.setattr(update_cmd_fleet, "_current_checkout_sha", lambda: disk_sha) + + +def test_startup_warn_discharged_when_fleet_current(monkeypatch, capsys): + disk_sha = "e" * 40 + update_cmd._write_fleet_restart_pending_marker(expected_sha=disk_sha) + _patch_marker_sha(monkeypatch, disk_sha) + monkeypatch.setattr( + update_cmd_fleet, + "_marker_only_restart_obsolete", + update_cmd_fleet._marker_only_restart_obsolete, + ) + monkeypatch.setattr( + "hermes_cli.update_receipt.collect_fleet_versions", + lambda **kwargs: [ + {"profile": "default", "pid": 42, "code_sha": disk_sha, "code_version": "0.21.0", "state": "current"} + ], + ) + + update_cmd._warn_pending_fleet_restart_on_startup() + + captured = capsys.readouterr() + assert captured.err == "" + assert not update_cmd._fleet_restart_pending_marker_path().exists() + + +def test_startup_warn_kept_when_fleet_stale(monkeypatch, capsys): + disk_sha = "e" * 40 + update_cmd._write_fleet_restart_pending_marker(expected_sha=disk_sha) + _patch_marker_sha(monkeypatch, disk_sha) + monkeypatch.setattr( + "hermes_cli.update_receipt.collect_fleet_versions", + lambda **kwargs: [ + {"profile": "default", "pid": 42, "code_sha": "7" * 40, "code_version": "0.20.0", "state": "stale"} + ], + ) + + update_cmd._warn_pending_fleet_restart_on_startup() + + err = capsys.readouterr().err + assert "did not restart running gateways" in err + assert update_cmd._fleet_restart_pending_marker_path().exists() + + +def test_startup_warn_kept_when_fleet_probe_empty(monkeypatch, capsys): + disk_sha = "e" * 40 + update_cmd._write_fleet_restart_pending_marker(expected_sha=disk_sha) + _patch_marker_sha(monkeypatch, disk_sha) + monkeypatch.setattr( + "hermes_cli.update_receipt.collect_fleet_versions", + lambda **kwargs: [], + ) + + update_cmd._warn_pending_fleet_restart_on_startup() + + err = capsys.readouterr().err + assert "did not restart running gateways" in err + assert update_cmd._fleet_restart_pending_marker_path().exists() + + +def test_startup_warn_kept_when_fleet_identity_unknown(monkeypatch, capsys): + disk_sha = "e" * 40 + update_cmd._write_fleet_restart_pending_marker(expected_sha=disk_sha) + _patch_marker_sha(monkeypatch, disk_sha) + monkeypatch.setattr( + "hermes_cli.update_receipt.collect_fleet_versions", + lambda **kwargs: [ + {"profile": "default", "pid": 42, "code_sha": None, "code_version": None, "state": "unknown"} + ], + ) + + update_cmd._warn_pending_fleet_restart_on_startup() + + err = capsys.readouterr().err + assert "did not restart running gateways" in err + assert update_cmd._fleet_restart_pending_marker_path().exists() + + +def test_marker_kept_when_newer_pull_moved_checkout_past_marker(monkeypatch, capsys): + update_cmd._write_fleet_restart_pending_marker(expected_sha="d" * 40) + _patch_marker_sha(monkeypatch, "e" * 40) # checkout advanced after the marker + seen = {"called": False} + + def _collect(**kwargs): + seen["called"] = True + return [{"profile": "default", "pid": 42, "code_sha": "d" * 40, "code_version": None, "state": "current"}] + + monkeypatch.setattr("hermes_cli.update_receipt.collect_fleet_versions", _collect) + + update_cmd._warn_pending_fleet_restart_on_startup() + + err = capsys.readouterr().err + assert "did not restart running gateways" in err + assert update_cmd._fleet_restart_pending_marker_path().exists() + assert seen["called"] is False # short-circuits before probing the fleet