From 2a980fbcbd19b06f4f4bb782c698bb50fd315531 Mon Sep 17 00:00:00 2001 From: doryani-agent Date: Mon, 7 Sep 2026 01:58:31 -0700 Subject: [PATCH] fix(update): let systemd clients outwait legitimate unit transactions Salvage the unit-budget implementation from #104745, replacing its test matrix with two invariant tests and covering the sibling graceful start. Keep unprivileged property reads, finite fallbacks, real manager errors, and post-restart health verification. Native disposable user unit: old client timed out after 15.03 seconds; new client completed the same 16-second stop transaction in 16.13 seconds. The unit stayed active with a new PID; missing-unit errors stayed errors. Co-authored-by: Teknium <127238744+teknium1@users.noreply.github.com> --- gateway/shutdown_forensics.py | 9 ++- hermes_cli/update_cmd_fleet.py | 55 +++++++++++++++-- .../test_update_unit_client_budget.py | 61 +++++++++++++++++++ website/docs/developer-guide/cli-internals.md | 16 +++++ 4 files changed, 133 insertions(+), 8 deletions(-) create mode 100644 tests/hermes_cli/test_update_unit_client_budget.py diff --git a/gateway/shutdown_forensics.py b/gateway/shutdown_forensics.py index 3ef96d7779..8c790a392e 100644 --- a/gateway/shutdown_forensics.py +++ b/gateway/shutdown_forensics.py @@ -236,13 +236,16 @@ def _systemd_timeout_stop_us(unit_name: str) -> Optional[int]: def parse_systemd_duration_to_us(raw: str) -> Optional[int]: """Parse 'TimeoutStopUSec=1min 30s' / '90s' style values to microseconds. Covers us, ms, s, min, - h; a bare number is seconds. None on anything unexpected; never raises. Public: also consumed by + h, d, w, month, y; a bare number is seconds. None on anything unexpected; never raises. Public: also consumed by hermes_cli.gateway's restart-wait sizing. """ if not raw: return None units = {"us": 1, "ms": 1_000, "s": 1_000_000, "sec": 1_000_000, - "min": 60_000_000, "h": 3_600_000_000, "hr": 3_600_000_000} + "min": 60_000_000, "h": 3_600_000_000, "hr": 3_600_000_000, + # Fixed systemd time-util.h constants, not variable calendar months/years. + "d": 86_400_000_000, "w": 604_800_000_000, + "month": 2_629_800_000_000, "y": 31_557_600_000_000} total_us, token, digits = 0, "", "" def _flush() -> bool: # fold the pending digits/token pair into total_us @@ -252,7 +255,7 @@ def parse_systemd_duration_to_us(raw: str) -> Optional[int]: return False try: total_us += int(float(digits) * multiplier) - except ValueError: + except (ValueError, OverflowError): return False digits = token = "" return True diff --git a/hermes_cli/update_cmd_fleet.py b/hermes_cli/update_cmd_fleet.py index daa53f29fd..1a2ba0ab45 100644 --- a/hermes_cli/update_cmd_fleet.py +++ b/hermes_cli/update_cmd_fleet.py @@ -324,11 +324,53 @@ def _systemctl(cmd: list, *, timeout: float): return subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout) -def _systemctl_reset_and_restart(manage_cmd: list, svc_name: str): +# poll() takes signed 32-bit milliseconds; keep headroom for rounding in communicate(). +_SYSTEMCTL_RESTART_TIMEOUT_MAX = (2**31 - 1) // 1000 - 1 + + +def _systemd_restart_timeout(scope_cmd: list, svc_name: str, *, start_only: bool = False) -> float: + """Outwait the unit's stop + start budgets, not just the systemctl client. + + A client timeout does not cancel the manager's queued restart. Unknown or + infinite limits use systemd's usual 90s per phase so automation stays bounded. + Custom ExecStop chains or EXTEND_TIMEOUT_USEC can still exceed this budget; + genuine timeouts must continue through the existing per-unit failure path. + """ + from gateway.shutdown_forensics import parse_systemd_duration_to_us + + budgets = {"TimeoutStartUSec": 90.0} + if not start_only: + budgets["TimeoutStopUSec"] = 90.0 + try: + show = _systemctl( + scope_cmd + ["show", svc_name, "--property=TimeoutStopUSec,TimeoutStartUSec"], + timeout=5, + ) + except (FileNotFoundError, subprocess.TimeoutExpired): + return sum(budgets.values()) + 15.0 + if show.returncode == 0: + for line in (show.stdout or "").splitlines(): + key, _, raw = line.partition("=") + if key in budgets: + # The shared parser returns None for infinity/unrecognized units. + try: + raw = raw.strip() + duration = int(raw) if raw.isascii() and raw.isdigit() else parse_systemd_duration_to_us(raw) + if duration is not None and duration > 0: + budgets[key] = duration / 1_000_000 + except (ValueError, OverflowError): + pass + return min(sum(budgets.values()) + 15.0, _SYSTEMCTL_RESTART_TIMEOUT_MAX) + + +def _systemctl_reset_and_restart(manage_cmd: list, svc_name: str, *, scope_cmd: list | None = None): """``reset-failed`` then ``restart``: a unit parked in failed state by systemd's own auto-restart can wedge a plain ``restart`` against RestartSec backoff and stay dead.""" + # Property reads need no manage-units privileges: narrow sudoers may permit + # restart/reset-failed but deny show. Keep the same user/system manager scope. + timeout = _systemd_restart_timeout(scope_cmd if scope_cmd is not None else manage_cmd, svc_name) _systemctl(manage_cmd + ["reset-failed", svc_name], timeout=10) - return _systemctl(manage_cmd + ["restart", svc_name], timeout=15) + return _systemctl(manage_cmd + ["restart", svc_name], timeout=timeout) def _is_hermes_gateway_unit(unit: str) -> bool: @@ -735,7 +777,10 @@ def _restart_one_systemd_gateway_unit( # privileges; without them auto-restart still fires after RestartSec. if _manage_cmd is not None: _systemctl(_manage_cmd + ["reset-failed", svc_name], timeout=10) - _systemctl(_manage_cmd + ["start", svc_name], timeout=15) + _systemctl( + _manage_cmd + ["start", svc_name], + timeout=_systemd_restart_timeout(scope_cmd, svc_name, start_only=True), + ) if _wait_for_service_active(scope_cmd, svc_name, timeout=10.0): restarted_services.append(svc_name) return @@ -770,7 +815,7 @@ def _restart_one_systemd_gateway_unit( # Blunt restart — only when the graceful path failed (no SIGUSR1 wiring, drain over # budget, restart-policy mismatch). Mirrors `hermes gateway restart` (`systemd_restart()`). - restart = _systemctl_reset_and_restart(_manage_cmd, svc_name) + restart = _systemctl_reset_and_restart(_manage_cmd, svc_name, scope_cmd=scope_cmd) if restart.returncode != 0: failed_or_stale_units.append(svc_name) print(f" ⚠ Failed to restart {svc_name}: {restart.stderr.strip()}") @@ -782,7 +827,7 @@ def _restart_one_systemd_gateway_unit( # Retry once — transient startup failures (stale module cache, # import race) often clear; reset-failed so the retry isn't blocked. print(f" ⚠ {svc_name} died after restart, retrying...") - _systemctl_reset_and_restart(_manage_cmd, svc_name) + _systemctl_reset_and_restart(_manage_cmd, svc_name, scope_cmd=scope_cmd) if _wait_for_service_active(scope_cmd, svc_name, timeout=10.0): restarted_services.append(svc_name) print(f" ✓ {svc_name} recovered on retry") diff --git a/tests/hermes_cli/test_update_unit_client_budget.py b/tests/hermes_cli/test_update_unit_client_budget.py new file mode 100644 index 0000000000..9d540d1a38 --- /dev/null +++ b/tests/hermes_cli/test_update_unit_client_budget.py @@ -0,0 +1,61 @@ +"""Update clients cover unit transactions without hiding manager failures.""" +import subprocess + +import pytest + +from hermes_cli import update_cmd_fleet as fleet + + +@pytest.mark.parametrize("graceful,retry", [(False, False), (False, True), (True, False)]) +def test_unit_transaction_budget_preserves_scope_and_health(monkeypatch, graceful, retry): + scope = ["systemctl", "--no-ask-password"] + manage = ["sudo", "-n", *scope] + calls = [] + + def systemctl(cmd, *, timeout): + calls.append((cmd, timeout)) + if "show" in cmd: + assert cmd[:len(scope)] == scope + output = "42" if "--property=MainPID" in cmd else "TimeoutStopUSec=70s\nTimeoutStartUSec=90s" + return subprocess.CompletedProcess(cmd, 0, output, "") + if "restart" in cmd or "start" in cmd: + assert cmd[:len(manage)] == manage + assert timeout > (90 if graceful else 160) + return subprocess.CompletedProcess(cmd, 0, "active", "") + + monkeypatch.setattr(fleet, "_systemctl", systemctl) + monkeypatch.setattr(fleet, "_drain_or_signal_gateway_for_update", lambda *a: True) + health = iter([False, True] if retry else [True]) + monkeypatch.setattr(fleet, "_wait_for_service_active", lambda *a, **kw: next(health)) + name = "hermes-gateway-test" if graceful else "hermes-serve-test" + restarted, failed = [], [] + fleet._restart_one_systemd_gateway_unit( + name, scope="system", scope_cmd=scope, drain_budget=45, + _manage_cmd_cache={"system": manage}, restarted_services=restarted, + failed_or_stale_units=failed, + ) + assert restarted == [name] and not failed + assert sum("restart" in cmd or "start" in cmd for cmd, _ in calls) == (2 if retry else 1) + + +@pytest.mark.parametrize("limits", ["", "TimeoutStopUSec=infinity\nTimeoutStartUSec=invalid", "TimeoutStopUSec=70000000\nTimeoutStartUSec=90s"]) +@pytest.mark.parametrize("outcome", [0, 7, "timeout"]) +def test_budget_fallback_keeps_real_errors(monkeypatch, limits, outcome): + def systemctl(cmd, *, timeout): + if "show" in cmd: + return subprocess.CompletedProcess(cmd, 0, limits, "") + if "restart" in cmd: + assert 160 < timeout < 2**31 / 1000 + if outcome == "timeout": + raise subprocess.TimeoutExpired(cmd, timeout) + return subprocess.CompletedProcess(cmd, outcome, "", "manager diagnostic") + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr(fleet, "_systemctl", systemctl) + if outcome == "timeout": + with pytest.raises(subprocess.TimeoutExpired): + fleet._systemctl_reset_and_restart(["systemctl"], "hermes-serve-test") + else: + result = fleet._systemctl_reset_and_restart(["systemctl"], "hermes-serve-test") + assert result.returncode == outcome + assert result.stderr == "manager diagnostic" diff --git a/website/docs/developer-guide/cli-internals.md b/website/docs/developer-guide/cli-internals.md index 8742ad3623..1f7b6469d8 100644 --- a/website/docs/developer-guide/cli-internals.md +++ b/website/docs/developer-guide/cli-internals.md @@ -14,6 +14,22 @@ The stage-by-stage contract (`plan → snapshot → apply → restart-per-kind field failure each stage guards are documented in `hermes_cli/AGENTS.md`; user-facing behaviour (receipts, `--plan`, snapshot modes) is in [Updating](../getting-started/updating.md). +The systemd blunt-restart fallback waits for the unit's `TimeoutStopUSec` plus +`TimeoutStartUSec`, with 15 seconds of client-side slack. It reads the target unit +in the same manager scope as the restart; both the initial attempt and retry use +this budget. A start after a graceful drain uses only the start budget plus slack. +A missing, unparseable, or infinite phase limit falls back to 90 seconds +for that phase, keeping unattended updates bounded. Timing out the `systemctl` +client does **not** cancel the manager's transaction. Custom multi-command stop +chains or `EXTEND_TIMEOUT_USEC` can still outlast this estimate; a real timeout +remains an incomplete restart, and successful commands still require the existing +service-health and fleet-version verification. Raw numeric `*USec` values are +microseconds, while formatted values use systemd's fixed units, including days, +weeks, months and years. The combined timeout is capped below the native signed +32-bit millisecond poll limit (with rounding headroom), so exceptionally long +unit limits cannot overflow subprocess polling. Zero/unknown/infinite phase +limits use the bounded fallback. This does not change active-turn drain settings. + ## Process identity: never infer it from argv substrings The bug class behind ~10 fleet-update issues (#90778, #87594, #78089, #76129, #91964, ...):