From 49a71c9727d73eb303ac09d2c0800a069cd4c302 Mon Sep 17 00:00:00 2001 From: salch-cred Date: Tue, 1 Sep 2026 15:40:08 +0530 Subject: [PATCH] fix(update): break the cron-update three-way restart deadlock (#100179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When hermes-auto-update runs \hermes update\ from cron, the update process lives INSIDE the gateway's own process tree. Waiting for that gateway to exit is a circular wait: gateway waits on all in-flight work units (#77184 don't-amputate) -> cron agent session waits on the \hermes update\ process to exit -> \hermes update\ waits on the gateway to exit [back to A] The wedged-loop probe (#81642) cannot break it: the cron session posts activity every ~180s (process-tool poll return), so it is 'actively waiting forever' and never marked wedged. The gateway logs 'Restart deferred: waiting on 1 active work unit(s)' every 30s until the 1800s force-drain cap amputates its own updater's session — reported as a 5+ minute hang with gateway_state.json stuck at draining + restart_requested (v0.21.0, main @ 530aa7b10f). Fix (the issue's recommended option 1): at both drain sites in update_cmd.py — systemd (line ~9862) and the bare-process/launchd path (line ~10203) — check \_is_pid_ancestor_of_current_process(pid)\ before drain-waiting. When the target gateway IS an ancestor, use \_request_gateway_self_restart\ (SIGUSR1, no exit-wait) and return: the gateway's own restart flow completes normally once this process, and therefore the cron work unit holding it, exits. Both helpers already exist in hermes_cli/gateway.py (277-304) and \_request_gateway_self_restart\ already refuses non-ancestor PIDs, so a normal out-of-tree \hermes update\ keeps its full drain semantics (including the #86684 cron floor) untouched. Tests (tests/hermes_cli/test_update_cron_deadlock_guard.py, 6): - own PID / parent PID are ancestors; 0 and negative are not - self-restart refuses a non-ancestor PID [linux] - ancestor path sends SIGUSR1 and NEVER calls _wait_for_pid_exit (the deadlock witness — a wait there is the bug) [linux] - non-ancestor path still drain-waits with the given budget [linux] Existing graceful/sigusr1/restart tests pass unchanged (9 passed). Fixes #100179 --- hermes_cli/update_cmd.py | 56 +++++++++- .../test_update_cron_deadlock_guard.py | 105 ++++++++++++++++++ 2 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 tests/hermes_cli/test_update_cron_deadlock_guard.py diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 85dc437f25..0d5450a192 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -9921,10 +9921,47 @@ def _cmd_update_impl(args, gateway_mode: bool): from hermes_cli.gateway import ( GATEWAY_LOOP_WEDGED, _escalate_wedged_gateway, + _is_pid_ancestor_of_current_process, + _request_gateway_self_restart, probe_gateway_loop_liveness, ) - if ( + if _is_pid_ancestor_of_current_process(_main_pid): + # THREE-WAY DEADLOCK BREAK (#100179). + # + # When `hermes update` runs INSIDE the gateway's + # own process tree — the hermes-auto-update cron + # job is the canonical case — waiting for that + # gateway to exit is a circular wait: + # + # gateway waits on all in-flight work units + # (#77184 don't-amputate-turns) + # └─ cron agent session waits on the + # `hermes update` process to exit + # └─ `hermes update` waits on the + # gateway to exit ← back to A + # + # The wedged-loop probe cannot break it: the cron + # session posts activity every ~180s (process-tool + # poll return), so it is "actively waiting + # forever" and never marked wedged. The gateway + # then burns the full force-drain cap (1800s) + # before killing its own updater's session. + # + # Fire-and-forget instead: signal the restart and + # return immediately. The gateway's own restart + # flow completes normally once THIS process (and + # therefore the cron work unit holding it) exits. + print( + f" → {svc_name}: update is running inside " + "this gateway's process tree — signalling " + "restart and letting the gateway drain " + "itself (avoids the cron-update deadlock)" + ) + _graceful_ok = _request_gateway_self_restart( + _main_pid + ) + elif ( probe_gateway_loop_liveness(_main_pid) == GATEWAY_LOOP_WEDGED ): @@ -10234,10 +10271,25 @@ def _cmd_update_impl(args, gateway_mode: bool): from hermes_cli.gateway import ( GATEWAY_LOOP_WEDGED, _escalate_wedged_gateway, + _is_pid_ancestor_of_current_process, + _request_gateway_self_restart, probe_gateway_loop_liveness, ) - if probe_gateway_loop_liveness(pid) == GATEWAY_LOOP_WEDGED: + if _is_pid_ancestor_of_current_process(pid): + # Same three-way deadlock break as the systemd path + # (#100179): this update is running inside that gateway's + # process tree (hermes-auto-update cron), so waiting for it + # to exit is a circular wait — the gateway waits on the + # cron work unit, the cron session waits on this process, + # this process waits on the gateway. Signal and return. + print( + f" → {proc.profile}: update runs inside this gateway's " + "process tree — signalling restart without waiting " + "(avoids the cron-update deadlock)" + ) + drained = _request_gateway_self_restart(pid) + elif probe_gateway_loop_liveness(pid) == GATEWAY_LOOP_WEDGED: # Loop-liveness probe: this gateway's event loop is # provably dead (#81642) — SIGUSR1/SIGTERM shutdown can # never run, so the drain wait would burn the full budget diff --git a/tests/hermes_cli/test_update_cron_deadlock_guard.py b/tests/hermes_cli/test_update_cron_deadlock_guard.py new file mode 100644 index 0000000000..da0d202702 --- /dev/null +++ b/tests/hermes_cli/test_update_cron_deadlock_guard.py @@ -0,0 +1,105 @@ +"""Regression tests for #100179: cron-update three-way restart deadlock. + +When `hermes update` runs INSIDE the gateway's own process tree (the +hermes-auto-update cron job), waiting for that gateway to exit is a +circular wait: + + gateway waits on all in-flight work units (#77184) + -> cron agent session waits on the `hermes update` process + -> `hermes update` waits on the gateway to exit [back to A] + +The wedged-loop probe cannot break it: the cron session posts activity +every ~180s (process-tool poll return), so it is never marked wedged and +the gateway burns the full 1800s force-drain cap. + +The fix: when the target gateway PID is an ancestor of this process, +fire-and-forget (SIGUSR1 + return) instead of drain-waiting. +""" + +from unittest.mock import patch + +import pytest + +linux_only = pytest.mark.linux_only + + +class TestAncestorDetectionGuard: + """_is_pid_ancestor_of_current_process is the deadlock discriminator.""" + + def test_own_pid_is_ancestor(self): + import os + + from hermes_cli.gateway import _is_pid_ancestor_of_current_process + + assert _is_pid_ancestor_of_current_process(os.getpid()) is True + + def test_parent_pid_is_ancestor(self): + import os + + from hermes_cli.gateway import _is_pid_ancestor_of_current_process + + ppid = os.getppid() + if ppid <= 1: + pytest.skip("no meaningful parent in this environment") + assert _is_pid_ancestor_of_current_process(ppid) is True + + def test_unrelated_pid_is_not_ancestor(self): + from hermes_cli.gateway import _is_pid_ancestor_of_current_process + + # PID 0 / negative are never ancestors; a very high unlikely PID isn't + # either. Use the documented zero/negative contract for determinism. + assert _is_pid_ancestor_of_current_process(0) is False + assert _is_pid_ancestor_of_current_process(-5) is False + + +@linux_only +class TestSelfRestartFireAndForget: + """_request_gateway_self_restart signals without waiting for exit.""" + + def test_refuses_non_ancestor_pid(self): + from hermes_cli.gateway import _request_gateway_self_restart + + # A non-ancestor must be refused — signalling an unrelated gateway + # and returning immediately would skip its drain entirely. + assert _request_gateway_self_restart(0) is False + + def test_signals_ancestor_and_returns_immediately(self): + """The ancestor path sends SIGUSR1 and does NOT poll for exit.""" + import os + import signal as _signal + + from hermes_cli import gateway as gw + + sent = [] + + def _fake_kill(pid, sig): + sent.append((pid, sig)) + + with patch.object(gw.os, "kill", side_effect=_fake_kill), patch.object( + gw, "_wait_for_pid_exit", + side_effect=AssertionError( + "fire-and-forget must NOT wait for the gateway to exit — " + "that wait is the #100179 deadlock" + ), + ): + ok = gw._request_gateway_self_restart(os.getpid()) + + assert ok is True + assert sent == [(os.getpid(), _signal.SIGUSR1)] + + def test_graceful_restart_does_wait(self): + """Contrast: the non-ancestor path DOES drain-wait (unchanged).""" + import signal as _signal + + from hermes_cli import gateway as gw + + waited = [] + + with patch.object(gw.os, "kill"), patch.object( + gw, "_wait_for_pid_exit", + side_effect=lambda pid, t: waited.append((pid, t)) or True, + ): + ok = gw._graceful_restart_via_sigusr1(4242, drain_timeout=7.0) + + assert ok is True + assert waited == [(4242, 7.0)], "drain path must still wait for exit"