fix(update): break the cron-update three-way restart deadlock (#100179)
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
This commit is contained in:
@@ -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"
|
||||
Reference in New Issue
Block a user