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.
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
):
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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] = {}
|
||||
|
||||
Reference in New Issue
Block a user