mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 16:18:55 +00:00
fix(cli): arm exit watchdog on shutdown signal, not at chat startup (#66278)
A hermes --tui session whose main thread wedges before app.run() returns never executes the finally that calls _run_cleanup — the only place the exit watchdog was armed — so a dead CLI lingered indefinitely (observed ~47 min at 4% CPU, the #65998 class). Arm the backstop from the SIGTERM/SIGHUP handlers instead (both the interactive and single-query paths), the earliest moment shutdown intent is unambiguous. The signal-armed leash is 2x HERMES_EXIT_WATCHDOG_S so a slow-but-progressing _run_cleanup (which still arms its own tighter timer) is never cut short; the outer timer only wins when cleanup was never reached. Idempotent across repeated signals; never raises from a handler. Deliberately NOT armed at startup: the watchdog thread calls os._exit(0) unconditionally after its sleep, so a startup-armed timer (the #65998 approach) would hard-kill every session that outlives the timeout. Supersedes #65998; thanks @JeffStone69 for the report and root-cause gap analysis.
This commit is contained in:
parent
348e9912ff
commit
c66891db08
2 changed files with 178 additions and 0 deletions
55
cli.py
55
cli.py
|
|
@ -1056,6 +1056,50 @@ def _arm_exit_watchdog(timeout_s: float | None = None) -> None:
|
|||
pass # best-effort — never block shutdown on watchdog setup
|
||||
|
||||
|
||||
_signal_watchdog_armed = False
|
||||
|
||||
|
||||
def _arm_exit_watchdog_on_shutdown_signal() -> None:
|
||||
"""Arm the exit backstop the moment a termination signal arrives.
|
||||
|
||||
SIGTERM/SIGHUP establish unambiguous shutdown intent, but the graceful
|
||||
path from signal → ``agent.interrupt()`` → ``app.exit()`` /
|
||||
``KeyboardInterrupt`` → ``finally`` → ``_run_cleanup`` has several wedge
|
||||
points BEFORE ``_run_cleanup`` arms the normal watchdog: a main thread
|
||||
parked in a syscall that never observes the unwind, a prompt_toolkit
|
||||
teardown that never returns, or an agent worker blocking the ``finally``.
|
||||
When that happens the process has NO backstop and a "dead" CLI lingers
|
||||
(observed: ``hermes --tui`` alive ~47 min at 4% CPU after terminal close —
|
||||
the #65998 class).
|
||||
|
||||
Arming at signal time closes that window. The leash is 2× the normal
|
||||
cleanup timeout so a slow-but-progressing ``_run_cleanup`` (which arms
|
||||
its own tighter timer when it starts) is never cut short by this outer
|
||||
backstop — this timer only wins when cleanup was never reached at all.
|
||||
|
||||
Deliberately NOT armed at chat startup: the watchdog thread calls
|
||||
``os._exit(0)`` unconditionally after its sleep, so arming without
|
||||
shutdown intent would hard-kill every session that outlives the timeout.
|
||||
|
||||
Idempotent (module flag) so repeated signals don't stack timer threads.
|
||||
Never raises — safe to call from a signal handler.
|
||||
"""
|
||||
global _signal_watchdog_armed
|
||||
if _signal_watchdog_armed:
|
||||
return
|
||||
_signal_watchdog_armed = True
|
||||
try:
|
||||
base = float(os.getenv("HERMES_EXIT_WATCHDOG_S", "30"))
|
||||
except (TypeError, ValueError):
|
||||
base = 30.0
|
||||
if base <= 0:
|
||||
return # explicitly disabled
|
||||
try:
|
||||
_arm_exit_watchdog(timeout_s=base * 2)
|
||||
except Exception:
|
||||
pass # never let the backstop break signal handling
|
||||
|
||||
|
||||
def _run_cleanup(*, notify_session_finalize: bool = True):
|
||||
"""Run resource cleanup exactly once."""
|
||||
global _cleanup_done
|
||||
|
|
@ -15574,6 +15618,13 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin):
|
|||
logger.debug("Received signal %s, triggering graceful shutdown", signum)
|
||||
except Exception:
|
||||
pass # never let logging raise from a signal handler (#13710 regression)
|
||||
# Shutdown intent is now unambiguous — arm the exit backstop
|
||||
# IMMEDIATELY, before the graceful unwind below. If any step of
|
||||
# that unwind wedges (main thread parked in a syscall, prompt_toolkit
|
||||
# teardown never returning), _run_cleanup never runs and would
|
||||
# never arm its own watchdog — leaving a "dead" CLI alive for
|
||||
# minutes (#65998 class). Never raises.
|
||||
_arm_exit_watchdog_on_shutdown_signal()
|
||||
try:
|
||||
if getattr(self, "agent", None) and getattr(self, "_agent_running", False):
|
||||
self.agent.interrupt(f"received signal {signum}")
|
||||
|
|
@ -16178,6 +16229,10 @@ def main(
|
|||
# default for debugging.
|
||||
def _signal_handler_q(signum, frame):
|
||||
logger.debug("Received signal %s in single-query mode", signum)
|
||||
# Arm the exit backstop now that shutdown intent is unambiguous —
|
||||
# covers wedges in the unwind below that would otherwise leave the
|
||||
# process alive with no watchdog (#65998 class). Never raises.
|
||||
_arm_exit_watchdog_on_shutdown_signal()
|
||||
try:
|
||||
_agent = getattr(cli, "agent", None)
|
||||
if _agent is not None:
|
||||
|
|
|
|||
123
tests/cli/test_exit_watchdog_signal_arm.py
Normal file
123
tests/cli/test_exit_watchdog_signal_arm.py
Normal file
|
|
@ -0,0 +1,123 @@
|
|||
"""Exit watchdog: arm on shutdown *intent* (signal), never at chat startup.
|
||||
|
||||
Regression coverage for the #65998 class: a ``hermes --tui`` process whose
|
||||
main thread wedges before ``app.run()`` returns never executes the ``finally``
|
||||
that calls ``_run_cleanup`` — the only place the exit watchdog used to be
|
||||
armed — so a "dead" CLI lingered indefinitely (observed ~47 min at 4% CPU).
|
||||
|
||||
The fix arms the backstop from the SIGTERM/SIGHUP handlers via
|
||||
``_arm_exit_watchdog_on_shutdown_signal()``. Arming at *startup* (the
|
||||
rejected #65998 approach) is specifically forbidden: the watchdog thread
|
||||
calls ``os._exit(0)`` unconditionally after its sleep, so a startup-armed
|
||||
timer hard-kills every session that outlives the timeout.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import signal
|
||||
import subprocess
|
||||
import sys
|
||||
import time
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
import cli
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _reset_arm_flag(monkeypatch):
|
||||
"""Each test starts with the idempotency flag clear."""
|
||||
monkeypatch.setattr(cli, "_signal_watchdog_armed", False)
|
||||
|
||||
|
||||
class TestSignalArmLogic:
|
||||
def test_arms_with_double_cleanup_timeout(self, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_EXIT_WATCHDOG_S", "7")
|
||||
with patch.object(cli, "_arm_exit_watchdog") as arm:
|
||||
cli._arm_exit_watchdog_on_shutdown_signal()
|
||||
arm.assert_called_once_with(timeout_s=14.0)
|
||||
|
||||
def test_idempotent_across_repeated_signals(self, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_EXIT_WATCHDOG_S", "7")
|
||||
with patch.object(cli, "_arm_exit_watchdog") as arm:
|
||||
cli._arm_exit_watchdog_on_shutdown_signal()
|
||||
cli._arm_exit_watchdog_on_shutdown_signal()
|
||||
cli._arm_exit_watchdog_on_shutdown_signal()
|
||||
assert arm.call_count == 1
|
||||
|
||||
def test_disabled_via_env_zero(self, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_EXIT_WATCHDOG_S", "0")
|
||||
with patch.object(cli, "_arm_exit_watchdog") as arm:
|
||||
cli._arm_exit_watchdog_on_shutdown_signal()
|
||||
arm.assert_not_called()
|
||||
|
||||
def test_bad_env_value_falls_back_to_default(self, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_EXIT_WATCHDOG_S", "not-a-number")
|
||||
with patch.object(cli, "_arm_exit_watchdog") as arm:
|
||||
cli._arm_exit_watchdog_on_shutdown_signal()
|
||||
arm.assert_called_once_with(timeout_s=60.0)
|
||||
|
||||
def test_never_raises_even_if_arm_explodes(self, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_EXIT_WATCHDOG_S", "7")
|
||||
with patch.object(cli, "_arm_exit_watchdog", side_effect=RuntimeError("boom")):
|
||||
cli._arm_exit_watchdog_on_shutdown_signal() # must not raise
|
||||
|
||||
|
||||
_REPO_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
|
||||
|
||||
# A minimal stand-in for the wedged-CLI shape: signal handlers mirror the
|
||||
# production wiring (arm-on-signal, then a graceful unwind that wedges), and
|
||||
# the main thread parks the way a stuck app.run() does.
|
||||
_WEDGE_SRC = """
|
||||
import os, signal, sys, time
|
||||
sys.path.insert(0, {repo!r})
|
||||
import cli
|
||||
|
||||
def _handler(signum, frame):
|
||||
# Production wiring: arm the backstop the moment shutdown intent exists,
|
||||
# then attempt a graceful unwind — which, in this repro, wedges (the
|
||||
# KeyboardInterrupt lands in a frame that swallows it).
|
||||
cli._arm_exit_watchdog_on_shutdown_signal()
|
||||
|
||||
signal.signal(signal.SIGTERM, _handler)
|
||||
print("READY", flush=True)
|
||||
while True: # the wedge: never observes any unwind
|
||||
time.sleep(0.2)
|
||||
"""
|
||||
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="POSIX signals")
|
||||
def test_sigterm_on_wedged_process_forces_exit_within_leash():
|
||||
"""E2E: a wedged process armed via the signal path self-exits at ~2×
|
||||
HERMES_EXIT_WATCHDOG_S; without the signal it would live forever."""
|
||||
env = dict(os.environ, HERMES_EXIT_WATCHDOG_S="1", PYTHONPATH=_REPO_ROOT)
|
||||
# _arm_exit_watchdog refuses to arm under pytest (it would kill the test
|
||||
# worker); the subprocess must look like a real CLI.
|
||||
env.pop("PYTEST_CURRENT_TEST", None)
|
||||
src = _WEDGE_SRC.format(repo=_REPO_ROOT)
|
||||
p = subprocess.Popen(
|
||||
[sys.executable, "-c", src],
|
||||
env=env,
|
||||
stdout=subprocess.PIPE,
|
||||
text=True,
|
||||
)
|
||||
try:
|
||||
assert p.stdout is not None
|
||||
assert p.stdout.readline().strip() == "READY"
|
||||
# Wedged, no signal yet: must still be alive well past the leash
|
||||
# (proves we did NOT arm at startup — the #65998 regression).
|
||||
time.sleep(3.0)
|
||||
assert p.poll() is None, "watchdog fired without shutdown intent"
|
||||
|
||||
p.send_signal(signal.SIGTERM)
|
||||
t0 = time.time()
|
||||
rc = p.wait(timeout=10)
|
||||
elapsed = time.time() - t0
|
||||
assert rc == 0
|
||||
# Leash is 2×1s; generous CI slack.
|
||||
assert elapsed < 8.0, f"exit took {elapsed:.1f}s; leash should be ~2s"
|
||||
finally:
|
||||
if p.poll() is None:
|
||||
p.kill()
|
||||
Loading…
Add table
Add a link
Reference in a new issue