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>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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")
|
||||
|
||||
61
tests/hermes_cli/test_update_unit_client_budget.py
Normal file
61
tests/hermes_cli/test_update_unit_client_budget.py
Normal file
@@ -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"
|
||||
@@ -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, ...):
|
||||
|
||||
Reference in New Issue
Block a user