From 17a137d8a3d4dbaa5081d558bfd271eeead9c58a Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 23 Sep 2026 09:47:05 -0700 Subject: [PATCH] fix(tui-gateway): a SIGTERM-ignoring command no longer survives the gateway's SIGTERM exit The SIGTERM handler arms a 1s os._exit timer, then runs _shutdown_sessions: a flush of up to 5s, then _stop_turns_before_exit, whose kill was the graceful TERM, wait 1s, KILL. A command that ignores SIGTERM was still alive when the timer fired, and os._exit left it reparented to init (live: `trap '' TERM; sleep 3600` survived a SIGTERM to `python -m tui_gateway.entry`). - kill_live_foreground_processes(now=True): SIGKILL each in-flight foreground tree at once, no TERM grace, no wait (BaseEnvironment._force_kill_process; LocalEnvironment kills the recorded process group, never our own). - The grace timer's exit (entry._hard_exit) runs it before os._exit. - _stop_turns_before_exit SIGKILLs whatever is still alive halfway through its settle budget (it ignored the interrupt's TERM), so the tool call still ends with a result the teardown persists instead of a dangling tool_call in state.db. - The other hard exits that skip cleanup do the same before os._exit: the serve parent-death watchdog, the CLI exit watchdog, the kanban worker's SIGTERM path, and the messaging gateway's shutdown and loop-liveness watchdogs. - Deflake test_shutdown_mid_tool_kills_the_command_and_keeps_its_result: the 0.5s settle budget was too tight under -n 40 (1 red in 9 runs); the join returns when the turn ends. --- gateway/shutdown_watchdog.py | 13 +++++-- hermes_cli/cli_shutdown.py | 4 +++ hermes_cli/cli_single_query.py | 4 +++ hermes_cli/web_server_lifecycle.py | 6 ++++ tests/tui_gateway/test_serve_exit_flush.py | 42 ++++++++++++++++++++++ tools/environments/base.py | 13 +++++-- tools/environments/local.py | 11 ++++++ tui_gateway/entry.py | 12 ++++++- tui_gateway/session_reaper.py | 20 +++++++---- 9 files changed, 113 insertions(+), 12 deletions(-) diff --git a/gateway/shutdown_watchdog.py b/gateway/shutdown_watchdog.py index 1db12ea15c..db15623742 100644 --- a/gateway/shutdown_watchdog.py +++ b/gateway/shutdown_watchdog.py @@ -135,7 +135,7 @@ def start_loop_liveness_watchdog( if stop_event.is_set(): return _mark_exited_quietly(exit_code, "loop_liveness_watchdog") - os._exit(exit_code) + _hard_exit(exit_code) thread = threading.Thread(target=_watchdog, daemon=True, name="gateway-loop-liveness-watchdog") try: thread.start() @@ -145,6 +145,15 @@ def start_loop_liveness_watchdog( return _LoopLivenessWatchdogHandle(stop_event, thread) +def _hard_exit(exit_code: int) -> None: + """``os._exit`` skips every cleanup: SIGKILL in-flight foreground commands first, they run in their + own process group and would outlive the gateway, reparented to init.""" + with contextlib.suppress(Exception): + from tools.environments.base import kill_live_foreground_processes + kill_live_foreground_processes(now=True) + os._exit(exit_code) + + def _mark_exited_quietly(exit_code: int, reason: str) -> None: """Best-effort terminal stamp on BOTH lifecycle records before ``os._exit`` skips teardown: the lifecycle ledger (so the next boot names the watchdog, not SIGKILL/OOM) and @@ -293,7 +302,7 @@ def arm_shutdown_watchdog( from hermes_logging import drain_log_queue drain_log_queue(timeout=1.0) _mark_exited_quietly(exit_code, "shutdown_watchdog") - os._exit(exit_code) + _hard_exit(exit_code) try: threading.Thread(target=_watchdog, daemon=True, name=name).start() except Exception: diff --git a/hermes_cli/cli_shutdown.py b/hermes_cli/cli_shutdown.py index 4fda381bb0..0a6fa48f10 100644 --- a/hermes_cli/cli_shutdown.py +++ b/hermes_cli/cli_shutdown.py @@ -89,6 +89,10 @@ def _arm_exit_watchdog(timeout_s: float | None = None, *, from_signal: bool = Fa except Exception: pass _flush_logging_and_stdio() + # os._exit skips cleanup: a foreground command in its own process group would outlive us. + with suppress(Exception): + from tools.environments.base import kill_live_foreground_processes + kill_live_foreground_processes(now=True) os._exit(0) with suppress(Exception): # never block shutdown on watchdog setup diff --git a/hermes_cli/cli_single_query.py b/hermes_cli/cli_single_query.py index ed35d53ee5..eb1b6546a0 100644 --- a/hermes_cli/cli_single_query.py +++ b/hermes_cli/cli_single_query.py @@ -399,6 +399,10 @@ def _install_single_query_signal_handlers(cli): # store here or the worker's turn (and its usage deltas) never become durable (#88583 / # #50881 class). Best-effort under the SIGALRM deadman above. _flush_one_shot_session_store(cli) + # The worker's command runs in its own process group: SIGKILL it or it outlives os._exit. + with suppress(Exception): + from tools.environments.base import kill_live_foreground_processes + kill_live_foreground_processes(now=True) _flush_logging_and_stdio() os._exit(0) raise KeyboardInterrupt() diff --git a/hermes_cli/web_server_lifecycle.py b/hermes_cli/web_server_lifecycle.py index 792f96fa5f..89f59725e8 100644 --- a/hermes_cli/web_server_lifecycle.py +++ b/hermes_cli/web_server_lifecycle.py @@ -340,6 +340,12 @@ def _start_parent_death_watchdog() -> None: ) except Exception: pass + # os._exit skips every cleanup: a foreground command in its own process group would outlive us. + try: + from tools.environments.base import kill_live_foreground_processes + kill_live_foreground_processes(now=True) + except Exception: + pass os._exit(0) threading.Thread(target=_loop, daemon=True, name="serve-parent-watchdog").start() diff --git a/tests/tui_gateway/test_serve_exit_flush.py b/tests/tui_gateway/test_serve_exit_flush.py index 2a5e520d1e..44e7278309 100644 --- a/tests/tui_gateway/test_serve_exit_flush.py +++ b/tests/tui_gateway/test_serve_exit_flush.py @@ -197,6 +197,9 @@ def test_shutdown_mid_tool_kills_the_command_and_keeps_its_result(monkeypatch): monkeypatch.setattr(server, "_release_gateway_wake_owner", lambda: None, raising=False) monkeypatch.setattr(server, "_flush_sessions_before_exit", lambda budget_s=None: 0) monkeypatch.setattr(server, "_close_session_by_id", lambda sid, **kw: at_teardown.append(list(messages))) + # The join returns as soon as the turn ends; 0.5s is too tight for the kill + bookkeeping under -n 40. + from tui_gateway import session_reaper + monkeypatch.setattr(session_reaper, "_EXIT_TURN_SETTLE_S", 10.0) session = {"agent": _Agent(), "session_key": "sess-mid-tool", "running": True, "_run_thread": run_thread, "history_lock": threading.RLock()} with server._sessions_lock: @@ -216,6 +219,45 @@ def test_shutdown_mid_tool_kills_the_command_and_keeps_its_result(monkeypatch): env.cleanup() +@pytest.mark.skipif(os.name == "nt", reason="POSIX process groups + trap") +def test_sigterm_grace_hard_exit_kills_a_sigterm_ignoring_command(monkeypatch): + """The SIGTERM path os._exit()s after a ~1s grace, while the graceful foreground kill runs after a + flush of up to 5s and then waits 1s between TERM and KILL. The grace timer's exit must SIGKILL the + tree itself, at once, or a command that ignores SIGTERM survives, reparented to init.""" + import threading + + import psutil + + from tools.environments.local import LocalEnvironment + from tui_gateway import entry + + env = LocalEnvironment(cwd=os.getcwd()) + run_thread = threading.Thread( + target=lambda: env.execute("trap '' TERM; sleep 3522", timeout=600), daemon=True) + run_thread.start() + deadline = time.monotonic() + 20.0 + sleeper = None + while sleeper is None and time.monotonic() < deadline: + sleeper = next((p for p in psutil.Process().children(recursive=True) + if p.name() == "sleep" and "3522" in " ".join(p.cmdline())), None) + time.sleep(0.05) + assert sleeper is not None, "test setup: foreground sleep never started" + exits: list = [] + monkeypatch.setattr(entry.os, "_exit", exits.append) + try: + t0 = time.monotonic() + entry._hard_exit() + elapsed = time.monotonic() - t0 + _gone, alive = psutil.wait_procs([sleeper], timeout=5.0) + assert not alive, "SIGTERM-ignoring foreground command survived the hard exit" + assert exits == [0] and elapsed < 0.9, f"hard exit waited {elapsed:.2f}s (a TERM grace) first" + finally: + if sleeper.is_running(): + sleeper.kill() + run_thread.join(5.0) + env.cleanup() + + def test_periodic_flush_respects_interval_with_fake_clock( registered_session, monkeypatch ): diff --git a/tools/environments/base.py b/tools/environments/base.py index c68c542b35..9795ab6db2 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -59,13 +59,16 @@ _live_foreground: dict[int, tuple["BaseEnvironment", "ProcessHandle"]] = {} _live_foreground_lock = threading.Lock() -def kill_live_foreground_processes() -> int: - """Kill every in-flight foreground command's process tree; returns how many were signalled.""" +def kill_live_foreground_processes(*, now: bool = False) -> int: + """Kill every in-flight foreground command's process tree; returns how many were signalled. + + ``now=True`` is for a caller about to ``os._exit``: the graceful kill TERMs, waits and only then + KILLs, so a SIGTERM-ignoring command outlives a hard exit that lands inside that window.""" with _live_foreground_lock: live = list(_live_foreground.values()) for env, proc in live: try: - env._kill_process(proc) + (env._force_kill_process if now else env._kill_process)(proc) except Exception: logger.debug("exit-time kill of a foreground command failed", exc_info=True) return len(live) @@ -468,6 +471,10 @@ class BaseEnvironment(ABC): except (ProcessLookupError, PermissionError, OSError): pass + def _force_kill_process(self, proc: ProcessHandle): + """Kill without waiting, for a host that hard-exits next. Subclasses kill the whole tree.""" + self._kill_process(proc) + # --- CWD extraction --- def _update_cwd(self, result: dict): """Extract CWD from command output. Override for local file-based read.""" diff --git a/tools/environments/local.py b/tools/environments/local.py index fb0ea98835..89a06eec33 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -963,6 +963,17 @@ class LocalEnvironment(BaseEnvironment): with contextlib.suppress(Exception): proc.kill() + def _force_kill_process(self, proc): + """SIGKILL the whole group with no TERM grace or wait: the caller os._exit()s next.""" + if _IS_WINDOWS: # already a forced tree kill + return self._kill_process(proc) + with contextlib.suppress(OSError): + pgid = getattr(proc, "_hermes_pgid", None) or os.getpgid(proc.pid) + if pgid != os.getpgrp(): # never our own group (see _kill_process_group_posix) + os.killpg(pgid, signal.SIGKILL) # windows-footgun: ok — POSIX only (_IS_WINDOWS returned above) + with contextlib.suppress(OSError): + proc.kill() + def _extract_cwd_from_output(self, result: dict): """Base semantics plus: Git Bash ``pwd -P`` emits MSYS form on Windows — normalize to native and require the dir to exist, else ``_run_bash`` would diff --git a/tui_gateway/entry.py b/tui_gateway/entry.py index 2d948c25d9..aebfbeb802 100644 --- a/tui_gateway/entry.py +++ b/tui_gateway/entry.py @@ -100,6 +100,16 @@ def _append_crash_log(header: str, dump=None) -> None: dump(f) +def _hard_exit() -> None: + """The grace timer's ``os._exit``. The flush runs first and the graceful foreground kill + (TERM, wait, KILL) after it, so a SIGTERM-ignoring command is usually still alive here: + SIGKILL its tree now or it outlives us, reparented to init.""" + with suppress(Exception): + from tools.environments.base import kill_live_foreground_processes + kill_live_foreground_processes(now=True) + os._exit(0) + + def _log_signal(signum: int, frame) -> None: """Capture WHICH thread and WHERE a termination signal hit us, then exit. ``sys.exit(0)`` alone raced the worker pool (a thread holding ``_stdout_lock`` mid-flush blocks interpreter @@ -121,7 +131,7 @@ def _log_signal(signum: int, frame) -> None: _append_crash_log(f"{name} received · {time.strftime('%Y-%m-%d %H:%M:%S')}", _dump) print(f"[gateway-signal] {name}", file=sys.stderr, flush=True) # ``os._exit`` skips atexit but breaks the mid-flush deadlock; the crash log is the trail. - timer = threading.Timer(_shutdown_grace_seconds(), lambda: os._exit(0)) + timer = threading.Timer(_shutdown_grace_seconds(), _hard_exit) timer.daemon = True timer.start() # atexit (_shutdown_sessions) can be blocked past the grace window by a worker holding diff --git a/tui_gateway/session_reaper.py b/tui_gateway/session_reaper.py index 43e7b679ef..21e92dad12 100644 --- a/tui_gateway/session_reaper.py +++ b/tui_gateway/session_reaper.py @@ -119,8 +119,10 @@ _EXIT_TURN_SETTLE_S = 0.5 def _stop_turns_before_exit(budget_s: float | None = None) -> None: """Interrupt every in-flight turn and give it ``budget_s`` to settle, so a running tool call ends - with a result the teardown's final persist records, then kill any foreground command still alive: - it runs in its own process group and would otherwise outlive the gateway, reparented to init.""" + with a result the teardown's final persist records. A foreground command runs in its own process + group and would outlive the gateway, reparented to init. One still alive halfway through the budget + ignored the interrupt's SIGTERM: SIGKILL it then, early enough for its result to land as well (the + interrupt's own TERM, 1s, KILL outlasts the SIGTERM path's ~1s grace).""" with _sessions_lock: running = [(sid, s) for sid, s in _sessions.items() if s.get("running")] threads = [] @@ -129,11 +131,17 @@ def _stop_turns_before_exit(budget_s: float | None = None) -> None: _interrupt_session_turn(sid, session) if (t := session.get("_run_thread")) is not None and t is not threading.current_thread(): threads.append(t) - deadline = time.monotonic() + (_EXIT_TURN_SETTLE_S if budget_s is None else max(0.0, budget_s)) - for t in threads: - t.join(max(0.0, deadline - time.monotonic())) + budget = _EXIT_TURN_SETTLE_S if budget_s is None else max(0.0, budget_s) + deadline = time.monotonic() + budget + + def _join(until: float) -> None: + for t in threads: + t.join(max(0.0, until - time.monotonic())) + + _join(deadline - budget / 2) from tools.environments.base import kill_live_foreground_processes - kill_live_foreground_processes() + kill_live_foreground_processes(now=True) + _join(deadline) _exit_flush_prev_handlers: dict[int, Any] = {}