From c66891db083cd422bdecfb1e65404489128b5c9d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 17 Jul 2026 06:49:04 -0700 Subject: [PATCH] fix(cli): arm exit watchdog on shutdown signal, not at chat startup (#66278) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- cli.py | 55 +++++++++ tests/cli/test_exit_watchdog_signal_arm.py | 123 +++++++++++++++++++++ 2 files changed, 178 insertions(+) create mode 100644 tests/cli/test_exit_watchdog_signal_arm.py diff --git a/cli.py b/cli.py index 8ed35226fb6a..b7fe3261c2ec 100644 --- a/cli.py +++ b/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: diff --git a/tests/cli/test_exit_watchdog_signal_arm.py b/tests/cli/test_exit_watchdog_signal_arm.py new file mode 100644 index 000000000000..fcd482b1b282 --- /dev/null +++ b/tests/cli/test_exit_watchdog_signal_arm.py @@ -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()