diff --git a/hermes_cli/update_restart_recovery.py b/hermes_cli/update_restart_recovery.py index 3b92865c69..547b7bb06e 100644 --- a/hermes_cli/update_restart_recovery.py +++ b/hermes_cli/update_restart_recovery.py @@ -70,24 +70,12 @@ _UNIT_SETTLE_ATTEMPTS = 10 _UNIT_SETTLE_DELAY = 1.0 -def _run_quiet( - run: Callable[..., Any], - argv: list[str], - *, - timeout: int, - **extra: Any, -) -> Any | None: +def _run_quiet(run: Callable[..., Any], argv: list[str], *, timeout: int, **extra: Any) -> Any | None: """Run ``argv`` capturing text output; ``None`` when it errors or times out.""" try: return run( - argv, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - check=False, - timeout=timeout, - **extra, + argv, capture_output=True, text=True, encoding="utf-8", errors="replace", + check=False, timeout=timeout, **extra, ) except (OSError, subprocess.TimeoutExpired): return None @@ -114,10 +102,7 @@ def _run_profile_restart(profile: str, *, run: Callable[..., Any]) -> bool: """Run one profile restart without inheriting the updater's process state.""" kwargs: dict[str, Any] = {"stdin": subprocess.DEVNULL, "env": _child_environment()} if os.name == "nt": - kwargs["creationflags"] = ( - getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) - | getattr(subprocess, "DETACHED_PROCESS", 0) - ) + kwargs["creationflags"] = getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) | getattr(subprocess, "DETACHED_PROCESS", 0) else: kwargs["start_new_session"] = True argv = [sys.executable, "-m", "hermes_cli.main", "-p", profile, "gateway", "restart"] @@ -132,26 +117,18 @@ def _systemd_unit_candidates(profile: str) -> tuple[str, ...]: def _systemd_verified_active(profile: str, *, run: Callable[..., Any]) -> bool: - """Return True only when systemd itself reports the profile's unit active. - - This is the observation that separates ``verified`` from ``relaunch_attempted``. Any failure - here (no ``systemctl``, probe error, unit not ``active``) means we could NOT verify — never that - the restart failed. - """ + """True only when systemd itself reports the profile's unit active — the observation separating + ``verified`` from ``relaunch_attempted``. Any failure (no ``systemctl``, probe error, unit not + ``active``) means we could NOT verify, never that the restart failed.""" systemctl = shutil.which("systemctl") - if not systemctl: - return False - return any( + return bool(systemctl) and any( _unit_is_active([systemctl, "--user"], unit, run=run, require_rc0=True) for unit in _systemd_unit_candidates(profile) ) def restart_profiles( - profiles: Iterable[str], - *, - supervisors: Mapping[str, str] | None = None, - run: Callable[..., Any] = subprocess.run, + profiles: Iterable[str], *, supervisors: Mapping[str, str] | None = None, run: Callable[..., Any] = subprocess.run ) -> dict[str, list[str]]: """Restart the supplied profiles (only ones whose inventory identified a service supervisor). @@ -172,15 +149,11 @@ def restart_profiles( def _systemctl_scopes() -> list[tuple[str, list[str]]]: - """``systemctl`` invocations for the user and system scopes, or nothing. + """``(label, systemctl argv)`` for the user and system scopes (the pair the in-process phase walks), or nothing. - Mirrors the scope pair the in-process restart phase walks. ``systemctl`` is resolved through - ``shutil.which`` so this module never has to import any Hermes platform helper — importing the - freshly pulled tree is exactly what aborted the phase that called us. - - Each scope is returned with its label because ``hermes-serve.service`` in the user manager and - ``hermes-serve.service`` in the system manager are two different processes. Every identity this - module produces or consumes stays qualified by that label; the bare unit name is never the key. + ``systemctl`` comes from ``shutil.which`` so this module never imports a Hermes platform helper — + importing the freshly pulled tree is exactly what aborted the phase that called us. Scopes carry + their label because the same unit name in both managers is two different processes. """ systemctl = shutil.which("systemctl") if not systemctl or sys.platform != "linux": @@ -209,18 +182,14 @@ def _listed_serve_units(scope: list[str], *, run: Callable[..., Any]) -> list[st def _unit_main_pid(scope: list[str], unit: str, *, run: Callable[..., Any]) -> int: """The unit's ``MainPID`` via ``systemctl show``; ``0`` when absent or unreadable.""" - result = _run_quiet( - run, scope + ["show", unit, "--property=MainPID", "--value"], timeout=_VERIFY_TIMEOUT - ) + result = _run_quiet(run, scope + ["show", unit, "--property=MainPID", "--value"], timeout=_VERIFY_TIMEOUT) try: return int(_stdout(result) or 0) if _succeeded(result) else 0 except ValueError: return 0 -def _unit_is_active( - scope: list[str], unit: str, *, run: Callable[..., Any], require_rc0: bool = False -) -> bool: +def _unit_is_active(scope: list[str], unit: str, *, run: Callable[..., Any], require_rc0: bool = False) -> bool: result = _run_quiet(run, scope + ["is-active", unit], timeout=_VERIFY_TIMEOUT) if result is None or (require_rc0 and not _succeeded(result)): return False @@ -228,12 +197,7 @@ def _unit_is_active( def _serve_unit_replaced( - scope: list[str], - unit: str, - previous_pid: int, - *, - run: Callable[..., Any], - sleep: Callable[[float], Any], + scope: list[str], unit: str, previous_pid: int, *, run: Callable[..., Any], sleep: Callable[[float], Any] ) -> bool: """Did the unit come back on a NEW main process? @@ -244,11 +208,10 @@ def _serve_unit_replaced( for attempt in range(_UNIT_SETTLE_ATTEMPTS): if attempt: sleep(_UNIT_SETTLE_DELAY) - if not _unit_is_active(scope, unit, run=run): - continue - current = _unit_main_pid(scope, unit, run=run) - if current > 0 and current != previous_pid: - return True + if _unit_is_active(scope, unit, run=run): + current = _unit_main_pid(scope, unit, run=run) + if current > 0 and current != previous_pid: + return True return False @@ -264,38 +227,25 @@ def _split_skip_entry(entry: Any) -> tuple[str, str]: return scope_label, unit.removesuffix(".service") -def _normalized_skips( - skip_units: Iterable[Any], -) -> tuple[set[tuple[str, str]], set[str]]: - """Split already-settled units into scope-qualified and legacy entries. - - A bare ``"hermes-serve"`` carries no scope and therefore cannot say WHICH of two same-named - processes was settled. - """ +def _normalized_skips(skip_units: Iterable[Any]) -> tuple[set[tuple[str, str]], set[str]]: + """Split already-settled units into scope-qualified and legacy entries (a bare ``"hermes-serve"`` + carries no scope and cannot say WHICH of two same-named processes was settled).""" qualified: set[tuple[str, str]] = set() legacy: set[str] = set() for entry in skip_units or (): scope_label, base = _split_skip_entry(entry) - if not base: - continue - if scope_label in _SCOPE_LABELS: - qualified.add((scope_label, base)) - else: - legacy.add(base) + if base: + (qualified.add((scope_label, base)) if scope_label in _SCOPE_LABELS else legacy.add(base)) return qualified, legacy def restart_serve_units( - *, - skip_units: Iterable[Any] = (), - run: Callable[..., Any] = subprocess.run, - sleep: Callable[[float], Any] = time.sleep, + *, skip_units: Iterable[Any] = (), run: Callable[..., Any] = subprocess.run, sleep: Callable[[float], Any] = time.sleep ) -> dict[str, list[str]]: """Restart every active ``hermes-serve*`` systemd unit from this process. - Units are enumerated from systemd, never from the update inventory. That keeps the relaunch - authority requirement structural: a manually launched or Desktop-owned ``hermes serve`` owns no - unit and therefore cannot be touched here. + Units are enumerated from systemd, never from the update inventory, so a manually launched or + Desktop-owned ``hermes serve`` (no unit) structurally cannot be touched here. """ skipped_qualified, skipped_legacy = _normalized_skips(skip_units) # (scope, base unit) -> replaced? The same unit name in the user and the system @@ -333,10 +283,7 @@ def _parse_payload(stream) -> tuple[list[str], dict[str, str], bool, list[str]]: profiles = payload.get("profiles") if not isinstance(profiles, list): raise ValueError("recovery payload must contain a profiles list") - if any( - not isinstance(profile, str) or not _PROFILE_ID_RE.fullmatch(profile) - for profile in profiles - ): + if any(not isinstance(profile, str) or not _PROFILE_ID_RE.fullmatch(profile) for profile in profiles): raise ValueError("recovery profiles contain an invalid profile id") raw_supervisors = payload.get("supervisors") supervisors: dict[str, str] = {} @@ -361,27 +308,20 @@ def _parse_payload(stream) -> tuple[list[str], dict[str, str], bool, list[str]]: if not isinstance(raw_skip, list) or any(not isinstance(entry, (str, dict)) for entry in raw_skip): raise ValueError("recovery serve_units skip list is invalid") for entry in raw_skip: - # A skip entry names one already-settled process. The qualified - # shape carries the systemd scope, because `hermes-serve.service` - # can exist in BOTH managers and settling one says nothing about - # the other. A bare string is the legacy shape a pre-update - # interpreter can still send; it is kept, and read as - # scope-agnostic by `restart_serve_units`. + # A skip entry names one already-settled process. The qualified shape carries the systemd + # scope (`hermes-serve.service` can exist in BOTH managers; settling one says nothing about + # the other). A bare string is the legacy shape a pre-update interpreter can still send, + # read as scope-agnostic by `restart_serve_units`. if isinstance(entry, dict): if not isinstance(entry.get("unit"), str): raise ValueError("recovery serve_units skip list is invalid") if not isinstance(entry.get("scope"), str): entry = {"unit": entry["unit"]} scope_label, base = _split_skip_entry(entry) - # Only the shapes systemd can actually produce for this family; a - # skip entry is a name filter, never a command argument. An - # unrecognized scope drops the entry rather than raising: dropping - # a skip can only ever cause one more restart-and-verify, while - # honouring an unreadable one could suppress recovery of a stale - # process. - if scope_label and scope_label not in _SCOPE_LABELS: - continue - if not _UNIT_RE.fullmatch(f"{base}.service"): + # Only shapes systemd can produce for this family (a skip is a name filter, never a command + # argument). An unrecognized scope DROPS the entry rather than raising: a dropped skip costs at + # most one extra restart-and-verify; honouring an unreadable one could suppress recovery. + if (scope_label and scope_label not in _SCOPE_LABELS) or not _UNIT_RE.fullmatch(f"{base}.service"): continue skip_units.append(f"{scope_label}/{base}" if scope_label else base) return profiles, supervisors, recover_serve, skip_units @@ -402,10 +342,7 @@ def main(argv: list[str] | None = None) -> int: ) except (ValueError, json.JSONDecodeError) as exc: print(json.dumps({ - "error": str(exc), - "verified": [], - "relaunch_attempted": [], - "failed": [], + "error": str(exc), "verified": [], "relaunch_attempted": [], "failed": [], "serve_units": {"verified": [], "failed": []}, })) return 2