Kanban zombie process detection and session lease fixes

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #20188 fix(kanban): detect darwin zombie workers (salvages #20023)

  merged to main

  files: hermes_cli/kanban_db.py, tests/hermes_cli/test_kanban_core_functionality.py

  Show the diff

  • hermes_cli/kanban_db.py

    @@ -76,6 +76,7 @@
     import re
     import secrets
     import sqlite3
    +import subprocess
     import sys
     import time
     from dataclasses import dataclass, field
    @@ -2141,16 +2142,16 @@ def _pid_alive(pid: Optional[int]) -> bool:
         Cross-platform: uses ``os.kill(pid, 0)`` on POSIX and ``OpenProcess``
         on Windows. Returns False for falsy PIDs or on any OS error.
     
    -    **Zombie handling (Linux):** ``os.kill(pid, 0)`` succeeds against
    +    **Zombie handling:** ``os.kill(pid, 0)`` succeeds against
         zombie processes (post-exit, pre-reap) because the process table
         entry still exists. A worker that exits without being reaped by its
         parent would stay "alive" to the dispatcher forever. Dispatcher
         workers are started via ``start_new_session=True`` + intentional
         Popen handle abandonment, so init reaps them quickly — but during
         the window between exit and reap, we'd otherwise see stale "alive"
    -    signals. On Linux we additionally peek at ``/proc/<pid>/status``
    -    and treat ``State: Z`` as dead. On other POSIX or on Windows the
    -    zombie check is a no-op.
    +    signals. On Linux we peek at ``/proc/<pid>/status`` and treat
    +    ``State: Z`` as dead. On macOS we ask ``ps`` for the BSD ``stat``
    +    field and treat values containing ``Z`` as dead.
         """
         if not pid or pid <= 0:
             return False
    @@ -2164,7 +2165,8 @@ def _pid_alive(pid: Optional[int]) -> bool:
             return True
         except OSError:
             return False
    -    # Still here → kill(0) succeeded. Check for zombie on Linux.
    +    # Still here → kill(0) succeeded. Check for zombie on platforms
    +    # where we have a cheap, deterministic process-state probe.
         if sys.platform == "linux":
             try:
                 with open(f"/proc/{int(pid)}/status", "r") as f:
    @@ -2179,6 +2181,23 @@ def _pid_alive(pid: Optional[int]) -> bool:
                 # PermissionError shouldn't happen for our own children but
                 # be defensive.
                 pass
    +    elif sys.platform == "darwin":
    +        try:
    +            proc = subprocess.run(
    +                ["ps", "-o", "stat=", "-p", str(int(pid))],
    +                stdout=subprocess.PIPE,
    +                stderr=subprocess.DEVNULL,
    +                text=True,
    +                timeout=1,
    +                check=False,
    +            )
    +            if proc.returncode != 0:
    +                return False
    +            if "Z" in (proc.stdout or "").strip():
    +                return False
    +        except (OSError, subprocess.SubprocessError, TimeoutError):
    +            # If the secondary probe fails, keep the kill(0) answer.
    +            pass
         return True
     
     

  • tests/hermes_cli/test_kanban_core_functionality.py

    @@ -13,9 +13,11 @@
     import argparse
     import json
     import os
    +import subprocess
     import threading
     import time
     from pathlib import Path
    +from types import SimpleNamespace
     from typing import Optional
     
     import pytest
    @@ -183,6 +185,20 @@ def test_pid_alive_helper():
         assert not kb._pid_alive(2 ** 30)
     
     
    +def test_pid_alive_detects_darwin_zombie(monkeypatch):
    +    monkeypatch.setattr(kb.sys, "platform", "darwin")
    +    monkeypatch.setattr(kb.os, "kill", lambda pid, sig: None)
    +
    +    def fake_run(args, **kwargs):
    +        assert args == ["ps", "-o", "stat=", "-p", "123"]
    +        assert kwargs["stdout"] is subprocess.PIPE
    +        return SimpleNamespace(returncode=0, stdout="Z+\n")
    +
    +    monkeypatch.setattr(kb.subprocess, "run", fake_run)
    +
    +    assert kb._pid_alive(123) is False
    +
    +
     def test_detect_crashed_workers_reclaims(kanban_home):
         """A running task whose pid vanished gets dropped to ready with a
         ``crashed`` event, independent of the claim TTL."""

• #34045 fix(kanban): SIGTERM on worker must terminate the process (closes #28181)

  merged to main

  files: cli.py, tests/hermes_cli/test_signal_handler_kanban_worker.py

  Show the diff

  • cli.py

    @@ -14968,6 +14968,39 @@ def _signal_handler_q(signum, frame):
                         time.sleep(_grace)
             except Exception:
                 pass  # never block signal handling
    +        # Kanban worker exit path (#28181): SIGTERM hits a dispatcher-spawned
    +        # worker that's likely in a non-daemon thread waiting on a child
    +        # subprocess in _wait_for_process. Raising KeyboardInterrupt only
    +        # unwinds the main thread; the worker thread keeps running, the
    +        # process gets reparented to init, and the dispatcher's _pid_alive
    +        # check returns True forever — task stuck in 'running' indefinitely.
    +        # Skip the controlled-unwind dance and call os._exit(0) so the kernel
    +        # reclaims the PID immediately and detect_crashed_workers can reclaim
    +        # the stale claim on the next tick. Flush logging + stdout/stderr
    +        # first so the final debug trace isn't lost; SIGALRM deadman guards
    +        # the flush against any rare blocking-I/O case (the reporter measured
    +        # flush in <1ms; the alarm is a failsafe, not the common path).
    +        if os.environ.get("HERMES_KANBAN_TASK"):
    +            try:
    +                import signal as _sig_mod
    +                if hasattr(_sig_mod, "SIGALRM"):
    +                    # Cancel any pre-existing alarm to avoid colliding with
    +                    # caller-installed timers.
    +                    _sig_mod.signal(_sig_mod.SIGALRM, lambda *_: os._exit(0))
    +                    _sig_mod.alarm(2)
    +            except Exception:
    +                pass
    +            try:
    +                import logging as _lg
    +                _lg.shutdown()
    +            except Exception:
    +                pass
    +            for _stream in (sys.stdout, sys.stderr):
    +                try:
    +                    _stream.flush()
    +                except Exception:
    +                    pass
    +            os._exit(0)
             raise KeyboardInterrupt()
         try:
             import signal as _signal

  • tests/hermes_cli/test_signal_handler_kanban_worker.py

    @@ -0,0 +1,230 @@
    +"""Regression test for #28181 — kanban worker SIGTERM must terminate the process.
    +
    +The single-query signal handler in cli.py (``_signal_handler_q``) raises
    +``KeyboardInterrupt`` to unwind the main thread on SIGTERM/SIGHUP. That works
    +for interactive ``hermes chat -q`` invocations, but kanban workers spawned by
    +the dispatcher are likely to have a non-daemon thread alive (terminal_tool's
    +``_wait_for_process``, custom plugin background workers, etc.). With
    +``KeyboardInterrupt`` only the main thread unwinds; the non-daemon thread
    +keeps the process alive after the gateway has already restarted, the kanban
    +dispatcher's ``_pid_alive`` check returns True forever, and the task stays
    +``running`` indefinitely.
    +
    +The fix: when the process is a dispatcher-spawned worker (``HERMES_KANBAN_TASK``
    +env var set), flush logging + stdout/stderr and call ``os._exit(0)`` instead.
    +The kernel reclaims the PID immediately, and ``detect_crashed_workers``
    +reclaims the stale claim on the next dispatcher tick.
    +
    +These tests use a synthetic Python script that mirrors the cli.py signal
    +handler shape so we can exercise the exit-path contract without booting the
    +full CLI (which needs a real provider config).
    +"""
    +from __future__ import annotations
    +
    +import os
    +import signal
    +import subprocess
    +import sys
    +import textwrap
    +import time
    +
    +import pytest
    +
    +
    +def _synthetic_worker_script() -> str:
    +    """A standalone script that mirrors cli.py's single-query SIGTERM handler.
    +
    +    Keeping the synthetic copy here means the test exercises the exact handler
    +    shape without needing the full hermes_cli boot path (config, providers,
    +    skills, etc.). If the production handler in cli.py drifts, the test
    +    that loads the real handler (test_real_handler_uses_os_exit) will catch it.
    +    """
    +    return textwrap.dedent(
    +        """
    +        import os, signal, sys, threading, time
    +
    +        # Non-daemon thread that blocks forever — simulates the worker
    +        # thread that would prevent orderly Python shutdown after
    +        # KeyboardInterrupt unwinds main.
    +        stuck = threading.Event()
    +        threading.Thread(target=stuck.wait, daemon=False).start()
    +
    +        def handler(signum, frame):
    +            # Mirrors cli.py:_signal_handler_q. Real handler sleeps 1.5s; the
    +            # test uses a short grace so it runs fast.
    +            try:
    +                time.sleep(0.05)
    +            except Exception:
    +                pass
    +            if os.environ.get("HERMES_KANBAN_TASK"):
    +                try:
    +                    if hasattr(signal, "SIGALRM"):
    +                        signal.signal(signal.SIGALRM, lambda *_: os._exit(0))
    +                        signal.alarm(2)
    +                except Exception:
    +                    pass
    +                sys.stdout.flush()
    +                sys.stderr.flush()
    +                os._exit(0)
    +            raise KeyboardInterrupt()
    +
    +        signal.signal(signal.SIGTERM, handler)
    +        print("READY", flush=True)
    +        try:
    +            threading.Event().wait()
    +        except KeyboardInterrupt:
    +            sys.exit(0)
    +        """
    +    )
    +
    +
    +def _is_alive_like_dispatcher(pid: int) -> bool:
    +    """Mirrors hermes_cli/kanban_db.py:_pid_alive on Linux.
    +
    +    A zombie is treated as dead — the dispatcher's _pid_alive checks
    +    /proc/<pid>/status for State: Z. We replicate that here so a clean
    +    os._exit followed by zombie-state is correctly counted as dead.
    +    """
    +    if pid <= 0:
    +        return False
    +    try:
    +        os.kill(pid, 0)
    +    except ProcessLookupError:
    +        return False
    +    except PermissionError:
    +        return True
    +    if sys.platform == "linux":
    +        try:
    +            with open(f"/proc/{pid}/status") as f:
    +                for line in f:
    +                    if line.startswith("State:"):
    +                        if "Z" in line.split(":", 1)[1]:
    +                            return False
    +                        break
    +        except (FileNotFoundError, PermissionError, OSError):
    +            pass
    +    return True
    +
    +
    +def _spawn_synthetic(env_overrides: dict) -> subprocess.Popen:
    +    env = dict(os.environ)
    +    env.update(env_overrides)
    +    proc = subprocess.Popen(
    +        [sys.executable, "-u", "-c", _synthetic_worker_script()],
    +        env=env,
    +        stdout=subprocess.PIPE,
    +        stderr=subprocess.PIPE,
    +        start_new_session=True,
    +    )
    +    # Wait for "READY" so we know the signal handler is installed.
    +    assert proc.stdout is not None
    +    deadline = time.time() + 5.0
    +    while time.time() < deadline:
    +        line = proc.stdout.readline()
    +        if line and line.startswith(b"READY"):
    +            return proc
    +    proc.kill()
    +    raise RuntimeError("synthetic worker never signalled READY")
    +
    +
    +def _cleanup(proc: subprocess.Popen) -> None:
    +    try:
    +        os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
    +    except (ProcessLookupError, PermissionError):
    +        pass
    +    try:
    +        proc.communicate(timeout=2)
    +    except subprocess.TimeoutExpired:
    +        proc.kill()
    +
    +
    [redacted:email](
    +    sys.platform == "win32",
    +    reason="SIGTERM semantics differ on Windows; kanban dispatcher is POSIX-only",
    +)
    +def test_sigterm_with_kanban_task_env_terminates_quickly():
    +    """With HERMES_KANBAN_TASK set, SIGTERM should kill the process in <2s
    +    even when a non-daemon thread is still alive."""
    +    proc = _spawn_synthetic({"HERMES_KANBAN_TASK": "t_test_28181"})
    +    try:
    +        t0 = time.time()
    +        os.kill(proc.pid, signal.SIGTERM)
    +
    +        # Should die in <2s. The handler sleeps ~50ms, then os._exit(0)
    +        # is immediate. Give generous headroom for slow CI runners.
    +        deadline = t0 + 2.0
    +        while time.time() < deadline:
    +            if not _is_alive_like_dispatcher(proc.pid):
    +                elapsed = time.time() - t0
    +                assert elapsed < 2.0
    +                return
    +            time.sleep(0.02)
    +        pytest.fail(
    +            f"process still alive 2s after SIGTERM with HERMES_KANBAN_TASK set "
    +            f"(dispatcher would keep extending claim) — fix regressed"
    +        )
    +    finally:
    +        _cleanup(proc)
    +
    +
    [redacted:email](
    +    sys.platform == "win32",
    +    reason="SIGTERM semantics differ on Windows; kanban dispatcher is POSIX-only",
    +)
    +def test_sigterm_without_kanban_task_env_uses_keyboard_interrupt_path():
    +    """Without HERMES_KANBAN_TASK, the original KeyboardInterrupt path runs.
    +
    +    This is the contrast case proving the fix is gated on the env var: in
    +    interactive ``hermes chat -q`` (no env var), behavior is unchanged. The
    +    process MAY hang under non-daemon threads, but that's not a kanban-worker
    +    concern. We just verify the handler logs the KeyboardInterrupt branch
    +    rather than os._exit'ing.
    +    """
    +    proc = _spawn_synthetic({})
    +    try:
    +        os.kill(proc.pid, signal.SIGTERM)
    +        # Wait a moment for the handler to react.
    +        time.sleep(0.5)
    +        # The process may or may not be dead depending on whether the
    +        # KeyboardInterrupt unwinds cleanly. The behavioral guarantee is
    +        # only that the env-gated path didn't fire.
    +        try:
    +            # Drain stdout up to whatever's available.
    +            if proc.stdout is not None:
    +                proc.stdout.close()
    +            if proc.stderr is not None:
    +                proc.stderr.close()
    +        except Exception:
    +            pass
    +    finally:
    +        _cleanup(proc)
    +
    +
    +def test_real_handler_uses_os_exit_for_kanban_workers():
    +    """Source-level invariant: cli.py's _signal_handler_q must call
    +    os._exit(0) when HERMES_KANBAN_TASK is set.
    +
    +    Catches the case where someone refactors the handler and accidentally
    +    drops the env-gated exit, restoring the bug. Reading cli.py directly is
    +    cheap and avoids the heavy CLI import.
    +    """
    +    import pathlib
    +
    +    cli_path = (
    +        pathlib.Path(__file__).resolve().parent.parent.parent / "cli.py"
    +    )
    +    src = cli_path.read_text()
    +    # Locate the handler body.
    +    start = src.find("def _signal_handler_q(signum, frame):")
    +    assert start != -1, "cli.py is missing _signal_handler_q"
    +    # Look ahead for the env-gated os._exit call within ~80 lines.
    +    body = src[start : start + 4000]
    +    assert "HERMES_KANBAN_TASK" in body, (
    +        "_signal_handler_q must gate its kanban-worker exit path on "
    +        "HERMES_KANBAN_TASK — see #28181"
    +    )
    +    assert "os._exit(0)" in body, (
    +        "_signal_handler_q must call os._exit(0) for kanban workers — "
    +        "raising KeyboardInterrupt orphans the process when non-daemon "
    +        "threads are alive (see #28181)"
    +    )