fix(gateway): size systemd TimeoutStopSec from the full stop budget
The generated unit only counted restart_drain_timeout, so a default cron drain (30s + 10s cleanup) could still be inside budget when systemd SIGKILLed the cgroup. Size TimeoutStopSec from max(drain, cron floor + reserve) plus headroom so an in-budget stop is not killed.
This commit is contained in:
@@ -51,6 +51,12 @@ DEFAULT_GATEWAY_CRON_DRAIN_TIMEOUT = float(
|
||||
# SIGKILLed mid-write and stays wedged at ``last_status=running`` forever.
|
||||
CRON_DRAIN_CLEANUP_RESERVE_S = 10.0
|
||||
|
||||
# systemd TimeoutStopSec headroom after the stop-path drain budget, and the
|
||||
# floor used when that budget is still the default immediate (0s) chat drain.
|
||||
# Keep these in lockstep with generate_systemd_unit() / #94759.
|
||||
SYSTEMD_STOP_HEADROOM_S = 30.0
|
||||
SYSTEMD_TIMEOUT_STOP_SEC_FLOOR = 60.0
|
||||
|
||||
|
||||
def is_gateway_supervisor_process(
|
||||
environ: Mapping[str, str] | None = None,
|
||||
@@ -140,10 +146,12 @@ def resolve_cron_drain_budget(
|
||||
|
||||
The configured floor is clamped to what this process can actually honour.
|
||||
The shutdown watchdog hard-exits at ``watchdog_delay`` and the service
|
||||
manager's ``TimeoutStopSec`` is sized from the same drain timeout, so
|
||||
waiting past that leash (minus ``cleanup_reserve_s`` for the teardown that
|
||||
follows the drain) would swap a cleanly-interrupted job for a SIGKILL that
|
||||
leaves it wedged mid-run — strictly worse than the bug being fixed.
|
||||
manager's ``TimeoutStopSec`` is sized from the full stop budget (drain
|
||||
vs cron floor + cleanup reserve, plus headroom — see
|
||||
``resolve_systemd_timeout_stop_sec``), so waiting past that leash
|
||||
(minus ``cleanup_reserve_s`` for the teardown that follows the drain)
|
||||
would swap a cleanly-interrupted job for a SIGKILL that leaves it
|
||||
wedged mid-run — strictly worse than the bug being fixed.
|
||||
|
||||
Never returns less than ``drain_timeout``: the cron floor only ever
|
||||
extends the wait, so an operator who deliberately configured a long
|
||||
@@ -168,6 +176,42 @@ def resolve_cron_drain_budget(
|
||||
return max(drain, min(floor, ceiling))
|
||||
|
||||
|
||||
def resolve_systemd_timeout_stop_sec(
|
||||
drain_timeout: float,
|
||||
cron_drain_timeout: float = DEFAULT_GATEWAY_CRON_DRAIN_TIMEOUT,
|
||||
*,
|
||||
cleanup_reserve_s: float = CRON_DRAIN_CLEANUP_RESERVE_S,
|
||||
headroom_s: float = SYSTEMD_STOP_HEADROOM_S,
|
||||
floor_s: float = SYSTEMD_TIMEOUT_STOP_SEC_FLOOR,
|
||||
) -> int:
|
||||
"""Seconds systemd ``TimeoutStopSec`` must cover the full stop budget.
|
||||
|
||||
``restart_drain_timeout`` is only the chat-turn interrupt budget (default
|
||||
0). The stop path may wait longer for in-flight cron work —
|
||||
``cron_drain_timeout`` plus ``cleanup_reserve_s`` — before it even starts
|
||||
interrupting. Sizing the unit from drain alone lets systemd SIGKILL an
|
||||
in-budget drain (#94759).
|
||||
|
||||
A zero ``cron_drain_timeout`` is a deliberate opt-out and does not extend
|
||||
the budget. Non-numeric inputs degrade to 0 rather than raising.
|
||||
"""
|
||||
|
||||
def _seconds(value: object) -> float:
|
||||
try:
|
||||
return max(float(value), 0.0) # type: ignore[arg-type]
|
||||
except (TypeError, ValueError):
|
||||
return 0.0
|
||||
|
||||
drain = _seconds(drain_timeout)
|
||||
cron = _seconds(cron_drain_timeout)
|
||||
reserve = _seconds(cleanup_reserve_s)
|
||||
headroom = _seconds(headroom_s)
|
||||
floor = _seconds(floor_s)
|
||||
cron_budget = (cron + reserve) if cron > 0.0 else 0.0
|
||||
stop_budget = max(drain, cron_budget)
|
||||
return int(max(floor, stop_budget + headroom))
|
||||
|
||||
|
||||
def resolve_restart_exit_wait_budget(
|
||||
drain_timeout: float,
|
||||
after_turn_timeout: float,
|
||||
|
||||
@@ -12818,16 +12818,23 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
||||
# a phantom kill in the journal. Best-effort, never raises.
|
||||
try:
|
||||
from gateway.shutdown_forensics import check_systemd_timing_alignment
|
||||
_alignment = check_systemd_timing_alignment(self._restart_drain_timeout)
|
||||
_alignment = check_systemd_timing_alignment(
|
||||
self._restart_drain_timeout,
|
||||
getattr(self, "_cron_drain_timeout", DEFAULT_GATEWAY_CRON_DRAIN_TIMEOUT),
|
||||
)
|
||||
if _alignment is not None and _alignment.get("mismatch"):
|
||||
logger.warning(
|
||||
"Stale systemd unit detected: %s has TimeoutStopSec=%.0fs but "
|
||||
"drain_timeout=%.0fs (expected >=%.0fs). systemd may SIGKILL the "
|
||||
"gateway mid-drain. Run `hermes gateway install --force` "
|
||||
"to regenerate the unit, or shorten agent.restart_drain_timeout.",
|
||||
"drain_timeout=%.0fs cron_drain_timeout=%.0fs (expected >=%.0fs). "
|
||||
"systemd may SIGKILL the gateway mid-drain. Run "
|
||||
"`hermes gateway install --force` to regenerate the unit, or "
|
||||
"shorten agent.restart_drain_timeout / agent.cron_drain_timeout.",
|
||||
_alignment.get("unit", "(unknown)"),
|
||||
_alignment["timeout_stop_sec"],
|
||||
_alignment["drain_timeout"],
|
||||
_alignment.get(
|
||||
"cron_drain_timeout", DEFAULT_GATEWAY_CRON_DRAIN_TIMEOUT
|
||||
),
|
||||
_alignment["expected_min"],
|
||||
)
|
||||
except Exception as _e:
|
||||
|
||||
@@ -26,6 +26,11 @@ import time
|
||||
from pathlib import Path
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
from gateway.restart import (
|
||||
DEFAULT_GATEWAY_CRON_DRAIN_TIMEOUT,
|
||||
resolve_systemd_timeout_stop_sec,
|
||||
)
|
||||
|
||||
|
||||
_SIGNAL_NAME_BY_NUM: Dict[int, str] = {}
|
||||
for _name in ("SIGTERM", "SIGINT", "SIGHUP", "SIGQUIT", "SIGUSR1", "SIGUSR2"):
|
||||
@@ -319,15 +324,19 @@ def context_as_json(ctx: Dict[str, Any]) -> str:
|
||||
return "{}"
|
||||
|
||||
|
||||
def check_systemd_timing_alignment(drain_timeout: float) -> Optional[Dict[str, Any]]:
|
||||
"""At startup, sanity-check that systemd's TimeoutStopSec >= drain_timeout.
|
||||
def check_systemd_timing_alignment(
|
||||
drain_timeout: float,
|
||||
cron_drain_timeout: float = DEFAULT_GATEWAY_CRON_DRAIN_TIMEOUT,
|
||||
) -> Optional[Dict[str, Any]]:
|
||||
"""At startup, sanity-check that systemd's TimeoutStopSec covers stop.
|
||||
|
||||
When the gateway is run under a stale systemd unit file (e.g. the user
|
||||
upgraded hermes-agent but never re-ran ``hermes setup`` to regenerate
|
||||
the unit), ``TimeoutStopSec`` can be smaller than the configured
|
||||
``restart_drain_timeout``. Result: SIGTERM arrives, the drain starts,
|
||||
and systemd SIGKILLs the cgroup mid-drain — looks like a phantom kill
|
||||
in the journal because the journal only logs ``code=killed status=9``.
|
||||
the unit), ``TimeoutStopSec`` can be smaller than the full stop budget
|
||||
(``restart_drain_timeout`` vs ``cron_drain_timeout`` + cleanup reserve,
|
||||
plus headroom). Result: SIGTERM arrives, the drain starts, and systemd
|
||||
SIGKILLs the cgroup mid-drain — looks like a phantom kill in the journal
|
||||
because the journal only logs ``code=killed status=9``.
|
||||
|
||||
Returns ``None`` when the alignment is fine OR we can't determine it
|
||||
(not running under systemd, ``systemctl`` unavailable, etc.). Returns
|
||||
@@ -392,15 +401,14 @@ def check_systemd_timing_alignment(drain_timeout: float) -> Optional[Dict[str, A
|
||||
return None
|
||||
|
||||
timeout_stop_sec = timeout_us / 1_000_000.0
|
||||
# systemd needs headroom for: post-interrupt kill, adapter disconnect,
|
||||
# SessionDB close, file unlinks, etc. 30s matches the unit-template
|
||||
# constant in hermes_cli/gateway.py.
|
||||
headroom = 30.0
|
||||
expected = drain_timeout + headroom
|
||||
expected = float(
|
||||
resolve_systemd_timeout_stop_sec(drain_timeout, cron_drain_timeout)
|
||||
)
|
||||
return {
|
||||
"unit": unit_name,
|
||||
"timeout_stop_sec": timeout_stop_sec,
|
||||
"drain_timeout": drain_timeout,
|
||||
"cron_drain_timeout": cron_drain_timeout,
|
||||
"expected_min": expected,
|
||||
"mismatch": timeout_stop_sec < expected,
|
||||
}
|
||||
|
||||
@@ -39,9 +39,11 @@ from gateway.restart import (
|
||||
GATEWAY_FATAL_CONFIG_EXIT_CODE,
|
||||
GATEWAY_SERVICE_RESTART_EXIT_CODE,
|
||||
is_gateway_supervisor_process,
|
||||
parse_cron_drain_timeout,
|
||||
parse_restart_after_turn_timeout,
|
||||
parse_restart_drain_timeout,
|
||||
resolve_restart_exit_wait_budget,
|
||||
resolve_systemd_timeout_stop_sec,
|
||||
)
|
||||
from hermes_cli.config import (
|
||||
get_env_value,
|
||||
@@ -3684,12 +3686,15 @@ def generate_systemd_unit(system: bool = False, run_as_user: str | None = None)
|
||||
"/sbin",
|
||||
"/bin",
|
||||
]
|
||||
# Preserve 30s for post-drain cleanup before systemd escalates, with a
|
||||
# 60s minimum for installs that use the default immediate drain. Positive
|
||||
# drain values extend the deadline directly instead of inheriting a second
|
||||
# 60s floor, so a configured 45s drain yields 75s rather than 90s.
|
||||
_drain_timeout = int(_get_restart_drain_timeout() or 0)
|
||||
restart_timeout = max(60, _drain_timeout + 30)
|
||||
# TimeoutStopSec must cover the full stop budget, not just
|
||||
# restart_drain_timeout. Cron work can legally wait cron_drain_timeout
|
||||
# plus cleanup reserve before interrupt/teardown, and systemd SIGKILLs
|
||||
# if the unit's deadline is shorter (#94759). 30s of post-drain headroom
|
||||
# is preserved on top, with a 60s floor.
|
||||
restart_timeout = resolve_systemd_timeout_stop_sec(
|
||||
_get_restart_drain_timeout(),
|
||||
_get_cron_drain_timeout(),
|
||||
)
|
||||
|
||||
if system:
|
||||
username, group_name, home_dir = _system_service_identity(run_as_user)
|
||||
@@ -4110,6 +4115,18 @@ def _get_restart_drain_timeout() -> float:
|
||||
return parse_restart_drain_timeout(raw)
|
||||
|
||||
|
||||
def _get_cron_drain_timeout() -> float:
|
||||
"""Return the configured cron-only drain floor in seconds (#82161)."""
|
||||
env_raw = os.getenv("HERMES_CRON_DRAIN_TIMEOUT")
|
||||
if env_raw is not None and str(env_raw).strip() != "":
|
||||
return parse_cron_drain_timeout(env_raw)
|
||||
cfg = read_raw_config()
|
||||
agent_cfg = cfg.get("agent", {}) if isinstance(cfg, dict) else {}
|
||||
if isinstance(agent_cfg, dict) and "cron_drain_timeout" in agent_cfg:
|
||||
return parse_cron_drain_timeout(agent_cfg.get("cron_drain_timeout"))
|
||||
return parse_cron_drain_timeout(None)
|
||||
|
||||
|
||||
def _get_restart_after_turn_timeout() -> float:
|
||||
"""Return the in-band restart wait-for-idle timeout in seconds (#77184)."""
|
||||
env_raw = os.getenv("HERMES_RESTART_AFTER_TURN_TIMEOUT")
|
||||
|
||||
Reference in New Issue
Block a user