feat(process): escalate SIGTERM→SIGKILL on host-pid termination after grace (#15008)

hermes_cli/config.py

@@ -1021,6 +1021,12 @@ def _ensure_hermes_home_managed(home: Path):
         "modal_mode": "auto",
         "cwd": ".",  # Use current directory
         "timeout": 180,
+        # Bounded grace period (seconds) between SIGTERM and an escalated
+        # SIGKILL when terminating a host process tree (browser daemons, etc.).
+        # A daemon that stalls in its SIGTERM handler is force-killed after this
+        # window so it can't leak indefinitely. 0 disables escalation (SIGTERM
+        # only — the historical behavior). Floored internally at 0.
+        "daemon_term_grace_seconds": 2.0,
         # Environment variables to pass through to sandboxed execution
         # (terminal and execute_code).  Skill-declared required_environment_variables
         # are passed through automatically; this list is for non-skill use cases.

scripts/release.py

@@ -123,6 +123,7 @@
     "290859878+synapsesx@users.noreply.github.com": "synapsesx",
     "157689911+itsflownium@users.noreply.github.com": "itsflownium",
     "dirtyren@users.noreply.github.com": "dirtyren",
+    "tkwong@inspiresynergy.com": "tkwong",
     "buihongduc132@gmail.com": "buihongduc132",
     "etheraura@protonmail.com": "EtherAura",  # PR #45205 salvage (Linux in-app update relaunch / GUI-skew terminal state)
     "valentt@users.noreply.github.com": "valentt",

tests/tools/test_process_registry.py

@@ -964,8 +964,12 @@ def terminate(self):
             # ``ProcessRegistry._is_host_pid_alive`` (→
             # ``gateway.status._pid_exists``), and the actual kill on POSIX
             # routes through ``psutil.Process(pid).terminate()``. Neither
-            # touches ``os.kill`` directly. Mock both seams.
+            # touches ``os.kill`` directly. Mock both seams.  Disable the
+            # SIGKILL-escalation step (grace=0) so it doesn't call
+            # ``psutil.wait_procs`` on the FakeProcess.
             with patch("gateway.status._pid_exists", return_value=True), \
+                 patch.object(ProcessRegistry, "_daemon_term_grace_seconds",
+                              staticmethod(lambda: 0.0)), \
                  patch.object(_psutil, "Process", side_effect=lambda pid: FakeProcess(pid)):
                 result = registry.kill_process(s.id)
 
@@ -1279,6 +1283,11 @@ def terminate(self):
 
         monkeypatch.setattr(pr, "_IS_WINDOWS", False)
         monkeypatch.setattr(psutil, "Process", _FakeParent)
+        # This test covers only the SIGTERM tree-walk ordering; disable the
+        # SIGKILL-escalation step (which would call psutil.wait_procs on the
+        # fakes) by setting the grace to 0.
+        monkeypatch.setattr(pr.ProcessRegistry, "_daemon_term_grace_seconds",
+                            staticmethod(lambda: 0.0))
 
         pr.ProcessRegistry._terminate_host_pid(12345)
 
@@ -1436,3 +1445,142 @@ def test_refresh_detached_marks_recycled_pid_exited(self, registry):
         refreshed = registry._refresh_detached_session(s)
         assert refreshed.exited is True
         assert s.id in registry._finished
+
+
+@pytest.mark.skipif(sys.platform == "win32",
+                    reason="POSIX SIGTERM→SIGKILL escalation; Windows uses taskkill /F")
+class TestSigkillEscalation:
+    """Bounded SIGTERM→SIGKILL escalation in _terminate_host_pid.
+
+    A daemon that ignores/stalls on SIGTERM must be force-killed after the
+    configured grace window so it can't leak indefinitely — while well-behaved
+    processes still exit cleanly on SIGTERM and the recycled-PID guard is never
+    bypassed.
+    """
+
+    # A process that traps SIGTERM (ignores it): only SIGKILL stops it.
+    # It prints "ready" AFTER installing the handler so the parent never
+    # signals it during the startup window (before SIG_IGN is in place).
+    _TRAP = (
+        "import signal, sys, time;"
+        "signal.signal(signal.SIGTERM, signal.SIG_IGN);"
+        "sys.stdout.write('ready\\n'); sys.stdout.flush();"
+        "[time.sleep(0.2) for _ in iter(int, 1)]"
+    )
+
+    def _spawn_trap(self):
+        proc = subprocess.Popen(
+            [sys.executable, "-c", self._TRAP],
+            stdout=subprocess.PIPE, text=True,
+        )
+        # Wait until the handler is installed before returning.
+        line = proc.stdout.readline()
+        assert line.strip() == "ready", "trap process failed to start"
+        return proc
+
+    def test_sigterm_ignoring_daemon_is_sigkilled(self, monkeypatch):
+        monkeypatch.setattr(ProcessRegistry, "_daemon_term_grace_seconds",
+                            staticmethod(lambda: 1.0))
+        proc = self._spawn_trap()
+        try:
+            ProcessRegistry._terminate_host_pid(proc.pid)
+            assert _wait_until(lambda: proc.poll() is not None, timeout=4.0), \
+                "SIGTERM-ignoring daemon should be SIGKILLed after grace"
+        finally:
+            if proc.poll() is None:
+                proc.kill()
+            proc.wait()
+
+    def test_grace_zero_disables_escalation(self, monkeypatch):
+        monkeypatch.setattr(ProcessRegistry, "_daemon_term_grace_seconds",
+                            staticmethod(lambda: 0.0))
+        proc = self._spawn_trap()
+        try:
+            ProcessRegistry._terminate_host_pid(proc.pid)
+            # No escalation → the SIGTERM-ignoring process survives.
+            assert not _wait_until(lambda: proc.poll() is not None, timeout=1.0)
+            assert proc.poll() is None
+        finally:
+            proc.kill()
+            proc.wait()
+
+    def test_well_behaved_process_dies_on_sigterm(self, monkeypatch):
+        monkeypatch.setattr(ProcessRegistry, "_daemon_term_grace_seconds",
+                            staticmethod(lambda: 2.0))
+        proc = _spawn_python_sleep(60)
+        try:
+            ProcessRegistry._terminate_host_pid(proc.pid)
+            assert _wait_until(lambda: proc.poll() is not None, timeout=3.0)
+        finally:
+            if proc.poll() is None:
+                proc.kill()
+            proc.wait()
+
+    def test_escalation_does_not_bypass_recycled_pid_guard(self, monkeypatch):
+        """A start-time mismatch must still spare the PID — no SIGTERM, no SIGKILL."""
+        monkeypatch.setattr(ProcessRegistry, "_daemon_term_grace_seconds",
+                            staticmethod(lambda: 1.0))
+        proc = self._spawn_trap()
+        try:
+            real_start = ProcessRegistry._safe_host_start_time(proc.pid)
+            ProcessRegistry._terminate_host_pid(
+                proc.pid, expected_start=(real_start or 0) + 1)
+            assert not _wait_until(lambda: proc.poll() is not None, timeout=1.5)
+            assert proc.poll() is None
+        finally:
+            proc.kill()
+            proc.wait()
+
+    def test_grace_reader_floors_at_zero(self, monkeypatch):
+        """A negative configured grace is clamped to 0 (no escalation)."""
+        import hermes_cli.config as cfg_mod
+        monkeypatch.setattr(cfg_mod, "read_raw_config",
+                            lambda: {"terminal": {"daemon_term_grace_seconds": -5}})
+        assert ProcessRegistry._daemon_term_grace_seconds() == 0.0
+
+    def test_entire_tree_is_sigkilled_not_just_parent(self, monkeypatch):
+        """A SIGTERM-ignoring parent + children are ALL force-killed.
+
+        Regression: an earlier implementation trusted psutil.wait_procs's
+        gone/alive partition, which mis-partitioned across a parent/child tree
+        and left survivors un-killed (flaky — sometimes the parent lived,
+        sometimes a child). The escalation now re-probes every target directly.
+        """
+        import psutil
+        monkeypatch.setattr(ProcessRegistry, "_daemon_term_grace_seconds",
+                            staticmethod(lambda: 1.0))
+        # Parent spawns 2 children; all trap SIGTERM. Parent prints child pids
+        # after the handler is installed.
+        parent_src = (
+            "import signal, subprocess, sys, time;"
+            "child='import signal,time\\nsignal.signal(signal.SIGTERM, signal.SIG_IGN)\\n"
+            "[time.sleep(0.2) for _ in iter(int,1)]';"
+            "kids=[subprocess.Popen([sys.executable,'-c',child]) for _ in range(2)];"
+            "signal.signal(signal.SIGTERM, signal.SIG_IGN);"
+            "sys.stdout.write(' '.join(str(k.pid) for k in kids)+'\\n'); sys.stdout.flush();"
+            "[time.sleep(0.2) for _ in iter(int,1)]"
+        )
+        parent = subprocess.Popen([sys.executable, "-c", parent_src],
+                                  stdout=subprocess.PIPE, text=True)
+        child_pids = [int(x) for x in parent.stdout.readline().split()]
+        all_pids = [parent.pid] + child_pids
+        try:
+            ProcessRegistry._terminate_host_pid(parent.pid)
+
+            def _all_dead():
+                return not any(
+                    psutil.pid_exists(p)
+                    and ProcessRegistry._proc_alive(psutil.Process(p))
+                    for p in all_pids
+                )
+
+            assert _wait_until(_all_dead, timeout=4.0), (
+                "entire SIGTERM-ignoring tree (parent + children) must be SIGKILLed"
+            )
+        finally:
+            for p in all_pids:
+                try:
+                    os.kill(p, signal.SIGKILL)
+                except (ProcessLookupError, PermissionError, OSError):
+                    pass
+            parent.wait()

tools/process_registry.py

@@ -483,6 +483,38 @@ def _refresh_detached_session(self, session: Optional[ProcessSession]) -> Option
         self._move_to_finished(session)
         return session
 
+    @staticmethod
+    def _proc_alive(proc) -> bool:
+        """True if a psutil.Process is running and not a zombie.
+
+        A zombie is already dead (just unreaped), so there's nothing to SIGKILL.
+        """
+        try:
+            import psutil
+            if not proc.is_running():
+                return False
+            return proc.status() != psutil.STATUS_ZOMBIE
+        except Exception:
+            return False
+
+    @staticmethod
+    def _daemon_term_grace_seconds() -> float:
+        """Grace window (s) between SIGTERM and escalated SIGKILL.
+
+        Read from ``terminal.daemon_term_grace_seconds`` in config.yaml; floored
+        at 0 (0 disables escalation). Falls back to the DEFAULT_CONFIG value if
+        config is unreadable, so callers always get a sane number.
+        """
+        try:
+            from hermes_cli.config import read_raw_config, cfg_get, DEFAULT_CONFIG
+            cfg = read_raw_config()
+            val = cfg_get(cfg, "terminal", "daemon_term_grace_seconds")
+            if val is None:
+                val = DEFAULT_CONFIG["terminal"]["daemon_term_grace_seconds"]
+            return max(float(val), 0.0)
+        except Exception:
+            return 2.0
+
     @classmethod
     def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> None:
         """Terminate a host-visible PID and its descendants.
@@ -496,12 +528,17 @@ def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) ->
         POSIX: walks the process tree with ``psutil`` and SIGTERMs
         children before the parent so subprocess trees (e.g. Chromium
         renderers/GPU helpers spawned by an ``agent-browser`` daemon)
-        don't get reparented to init and survive cleanup.
+        don't get reparented to init and survive cleanup.  After a bounded
+        grace window (``terminal.daemon_term_grace_seconds``) any tree member
+        that ignored SIGTERM — a daemon stalled in its signal handler — is
+        escalated to SIGKILL so it can't leak indefinitely.  Set the grace to
+        0 to disable escalation (SIGTERM only).
 
         Windows: shells out to ``taskkill /PID <pid> /T /F``. This is
         the documented Microsoft primitive for tree-kill and matches the
-        existing convention in ``gateway.status.terminate_pid``. We can't
-        reuse the POSIX psutil path on Windows because:
+        existing convention in ``gateway.status.terminate_pid``.  ``/F`` is
+        already a hard kill, so no separate escalation step is needed.  We
+        can't reuse the POSIX psutil path on Windows because:
 
           1. Windows doesn't maintain a Unix-style process tree —
              ``psutil.Process.children(recursive=True)`` walks PPID
@@ -550,19 +587,61 @@ def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) ->
         import psutil
         try:
             parent = psutil.Process(pid)
-            for child in parent.children(recursive=True):
-                try:
-                    child.terminate()
-                except psutil.NoSuchProcess:
-                    pass
-            parent.terminate()
         except psutil.NoSuchProcess:
             return
         except (OSError, PermissionError):
             try:
                 os.kill(pid, signal.SIGTERM)
             except (OSError, ProcessLookupError, PermissionError):
                 pass
+            return
+
+        # Snapshot the whole tree (children before parent) and SIGTERM each.
+        try:
+            targets = parent.children(recursive=True)
+        except (psutil.NoSuchProcess, psutil.AccessDenied, OSError):
+            targets = []
+        targets.append(parent)
+
+        for proc in targets:
+            try:
+                proc.terminate()
+            except psutil.NoSuchProcess:
+                pass
+            except (psutil.AccessDenied, OSError):
+                pass
+
+        # Escalate to SIGKILL for anything that ignored SIGTERM within the
+        # grace window — a daemon stalled in its signal handler would otherwise
+        # leak indefinitely.
+        grace = cls._daemon_term_grace_seconds()
+        if grace <= 0:
+            return
+        # Sleep out the grace window, then independently re-probe every target
+        # and SIGKILL any survivor.  We deliberately do NOT trust
+        # ``psutil.wait_procs``'s gone/alive partition here: it reaps via
+        # ``Process.wait()`` and can mis-partition when a target transitions
+        # through a zombie state or when reaping is racy across a parent/child
+        # tree, which left survivors un-killed.  A direct liveness re-probe is
+        # deterministic.
+        deadline = time.monotonic() + grace
+        while time.monotonic() < deadline:
+            if not any(cls._proc_alive(_p) for _p in targets):
+                break
+            time.sleep(0.05)
+        for proc in targets:
+            try:
+                if not cls._proc_alive(proc):
+                    continue
+                proc.kill()  # SIGKILL on POSIX
+                logger.info(
+                    "Escalated to SIGKILL for pid %d (ignored SIGTERM within "
+                    "%.1fs grace)", proc.pid, grace,
+                )
+            except psutil.NoSuchProcess:
+                pass
+            except (psutil.AccessDenied, OSError):
+                pass
 
     # ----- Spawn -----