From 7aa8f9c669992beb084b21ec64ca9ad7dcfa7d8c Mon Sep 17 00:00:00 2001 From: Julien Ellie Date: Tue, 1 Sep 2026 18:53:38 -0700 Subject: [PATCH] fix(herdr): release the pane when the process is signalled, not just on clean exit herdr keeps showing a dead code-puppy in its agent list after the process goes away. The stale entry never expires -- herdr trusts a reported agent claim indefinitely -- so the sidebar accumulates ghosts. pane.release_agent is only sent from the `shutdown`/`session_end` callbacks, which fire from a `finally:` in cli_runner. That block runs only when the interpreter unwinds. Closing a terminal pane, `kill`, logout, and service restarts all deliver SIGTERM (or SIGHUP), whose default disposition terminates the process outright: the `finally:` never runs and the release is never sent. Verified against a live herdr 0.8.2 pane: /exit -> released Ctrl-D -> released SIGTERM -> STUCK as codepuppy/idle, forever SIGKILL -> STUCK as codepuppy/idle, forever Install two guards when the plugin activates: * atexit -- covers interpreter teardown paths that bypass the callback. * a SIGTERM/SIGHUP handler -- covers the signal paths atexit misses. Both are needed. atexit does NOT run on SIGTERM (confirmed empirically), so an atexit-only fix would not have addressed the reported bug at all. The handler releases, then chains any previously-installed handler, or else restores the default disposition and re-raises. The process still dies from the signal with a 128+signum exit status; shutdown semantics are unchanged. release_and_close() is already idempotent and bounded, so double-firing is harmless and an unreachable herdr cannot delay exit. Failures to install (non-main thread) and failures to release are both swallowed -- reporting must never break the agent. SIGHUP is resolved with getattr rather than referenced directly: it does not exist on Windows, where this module is still imported because client.py speaks a named pipe there, and a bare signal.SIGHUP would raise AttributeError at import time and disable the plugin outright. SIGKILL remains uncatchable by design; reaping that case needs a herdr-side liveness check on the reporting process. Tests: 6 new cases covering registration, release-then-reraise, handler chaining, release failure, unavailable signals, and platform-safe signal resolution. The 5 behavioural cases fail without the fix. Full suite: 1983 -> 1989 passing, same 36 pre-existing failures. --- .../herdr/register_callbacks.py | 83 ++++++++++ tests/test_herdr_plugin.py | 152 ++++++++++++++++++ 2 files changed, 235 insertions(+) diff --git a/code_puppy_core_plugins/herdr/register_callbacks.py b/code_puppy_core_plugins/herdr/register_callbacks.py index b4c8011..dc203f3 100644 --- a/code_puppy_core_plugins/herdr/register_callbacks.py +++ b/code_puppy_core_plugins/herdr/register_callbacks.py @@ -32,6 +32,12 @@ * ``interactive_turn_cancel`` ........................ reset -> idle * ``awaiting_user_input`` ............................ blocked <-> not +Callbacks alone are not enough to guarantee the pane is released. They fire +from a ``finally:`` in ``cli_runner``, which the interpreter only reaches on +a graceful exit. ``_install_exit_guards`` adds an ``atexit`` hook and a +SIGTERM/SIGHUP handler so a closed pane, a plain ``kill``, or a logout also +release pane authority instead of stranding a dead agent in herdr's sidebar. + Session identity is the durable autosave (name, path) resolved by ``sources.current_session_ref`` -- NOT the per-run ``group_id`` UUID, which changes every turn. Pane metadata (model / context / tokens) and the @@ -47,7 +53,10 @@ from __future__ import annotations +import atexit import logging +import os +import signal from code_puppy.callbacks import register_callback @@ -56,6 +65,18 @@ logger = logging.getLogger(__name__) +#: Signals that terminate the process without unwinding the interpreter, so +#: neither the ``finally:`` in cli_runner nor ``atexit`` would otherwise run. +#: SIGKILL is deliberately absent -- it cannot be caught by design. SIGHUP is +#: resolved defensively because it does not exist on Windows, where this +#: module is still imported (client.py speaks a named pipe there) and a bare +#: ``signal.SIGHUP`` would raise AttributeError and break the whole plugin. +_TERMINATING_SIGNALS = tuple( + sig + for sig in (getattr(signal, name, None) for name in ("SIGTERM", "SIGHUP")) + if sig is not None +) + _client = HerdrClient() _reporter = HerdrReporter(_client) @@ -119,6 +140,67 @@ def _on_shutdown(*_args, **_kw) -> None: _reporter.on_shutdown() +def _install_exit_guards() -> None: + """Release the pane even when the interpreter never unwinds. + + The ``shutdown`` / ``session_end`` callbacks fire from a ``finally:`` in + ``cli_runner``, which only runs on a graceful exit (``/exit``, EOF). A + terminal closing its pane, a plain ``kill``, a logout, or a service + restart all send SIGTERM/SIGHUP, whose *default* disposition terminates + the process immediately -- the ``finally:`` never executes, no + ``pane.release_agent`` is ever sent, and herdr keeps showing a dead + agent forever with no mechanism to reap it. + + Two guards, because neither alone is sufficient: + + * ``atexit`` -- covers ordinary interpreter teardown paths that bypass + the callback (an unhandled exception above the ``finally:``, + ``sys.exit()`` from a nested frame). It does **not** run on SIGTERM. + * a SIGTERM/SIGHUP handler -- covers the signal paths ``atexit`` misses. + + The handler restores the previous disposition and re-raises so the + process still dies from the signal with correct ``128 + signum`` exit + status; it does not swallow the signal or alter shutdown semantics. + Any previously-installed handler is chained rather than clobbered. + + SIGKILL cannot be caught, so a ``kill -9`` still strands the pane. That + residual case needs a herdr-side liveness check on the reporting + process and is out of scope here. + """ + # release_and_close() is idempotent and bounded, so double-firing from + # both a signal and atexit is harmless. + atexit.register(_client.release_and_close) + + previous_handlers = {} + + def _release_and_reraise(signum: int, frame) -> None: + try: + _client.release_and_close() + except Exception: # never let cleanup mask the shutdown itself + logger.debug("herdr: release on signal %s failed", signum, exc_info=True) + + previous = previous_handlers.get(signum) + if callable(previous): + previous(signum, frame) + return + + # Restore the default disposition and re-raise so the exit status + # remains 128 + signum rather than a synthetic 0. + signal.signal(signum, previous if previous is not None else signal.SIG_DFL) + os.kill(os.getpid(), signum) + + for sig in _TERMINATING_SIGNALS: + try: + previous_handlers[sig] = signal.getsignal(sig) + signal.signal(sig, _release_and_reraise) + except (OSError, ValueError, AttributeError): + # signal.signal() only works on the main thread, and some signals + # are absent on some platforms (SIGHUP on Windows). Reporting must + # never break the agent, so degrade quietly. + previous_handlers.pop(sig, None) + logger.debug("herdr: could not install handler for %s", sig, exc_info=True) + + if _reporter.active: register_callback("startup", _on_startup) register_callback("user_prompt_submit", _on_user_prompt) @@ -132,6 +214,7 @@ def _on_shutdown(*_args, **_kw) -> None: register_callback("awaiting_user_input", _on_awaiting_user_input) register_callback("session_end", _on_shutdown) register_callback("shutdown", _on_shutdown) + _install_exit_guards() logger.debug("herdr plugin active for pane %s", _client._pane_id) else: logger.debug("herdr plugin inactive (not running inside a herdr pane)") diff --git a/tests/test_herdr_plugin.py b/tests/test_herdr_plugin.py index 323cee6..fd27428 100644 --- a/tests/test_herdr_plugin.py +++ b/tests/test_herdr_plugin.py @@ -444,3 +444,155 @@ def test_set_awaiting_user_input_exposes_notification_intent(): assert should_notify_awaiting_user_input() is False set_awaiting_user_input(False) assert should_notify_awaiting_user_input() is True + + +# --- exit guards (pane release on non-graceful shutdown) -------------------- + + +def test_install_exit_guards_registers_atexit_and_signal_handlers(): + """The guards must cover both interpreter teardown and terminating signals. + + The ``shutdown``/``session_end`` callbacks only fire from a ``finally:`` + that a signal-killed interpreter never reaches, so without these the pane + stays claimed by a dead process. + """ + import signal as signal_mod + + from code_puppy_core_plugins.herdr import register_callbacks as rc + + registered = [] + installed = {} + + def fake_signal(sig, handler): + installed[sig] = handler + return signal_mod.SIG_DFL + + with ( + patch.object(rc.atexit, "register", lambda fn, *a, **k: registered.append(fn)), + patch.object(rc.signal, "signal", fake_signal), + patch.object(rc.signal, "getsignal", lambda sig: signal_mod.SIG_DFL), + ): + rc._install_exit_guards() + + assert rc._client.release_and_close in registered + assert set(installed) == set(rc._TERMINATING_SIGNALS) + + +def test_exit_guard_signal_handler_releases_then_reraises(): + """Releasing must not swallow the signal: the process still dies from it.""" + import signal as signal_mod + + from code_puppy_core_plugins.herdr import register_callbacks as rc + + installed = {} + released = [] + killed = [] + + with ( + patch.object(rc.atexit, "register", lambda fn, *a, **k: None), + patch.object(rc.signal, "signal", lambda s, h: installed.__setitem__(s, h)), + patch.object(rc.signal, "getsignal", lambda sig: signal_mod.SIG_DFL), + ): + rc._install_exit_guards() + + handler = installed[signal_mod.SIGTERM] + with ( + patch.object(rc._client, "release_and_close", lambda: released.append(True)), + patch.object(rc.os, "kill", lambda pid, sig: killed.append(sig)), + patch.object(rc.signal, "signal", lambda s, h: None), + ): + handler(signal_mod.SIGTERM, None) + + assert released == [True] + assert killed == [signal_mod.SIGTERM] + + +def test_exit_guard_chains_previous_handler(): + """A pre-existing handler is chained, not clobbered.""" + import signal as signal_mod + + from code_puppy_core_plugins.herdr import register_callbacks as rc + + prior_calls = [] + + def prior_handler(signum, frame): + prior_calls.append(signum) + + installed = {} + with ( + patch.object(rc.atexit, "register", lambda fn, *a, **k: None), + patch.object(rc.signal, "signal", lambda s, h: installed.__setitem__(s, h)), + patch.object(rc.signal, "getsignal", lambda sig: prior_handler), + ): + rc._install_exit_guards() + + killed = [] + with ( + patch.object(rc._client, "release_and_close", lambda: None), + patch.object(rc.os, "kill", lambda pid, sig: killed.append(sig)), + ): + installed[signal_mod.SIGTERM](signal_mod.SIGTERM, None) + + assert prior_calls == [signal_mod.SIGTERM] + assert killed == [] # chained handler owns the exit + + +def test_exit_guard_survives_release_failure(): + """A broken socket must not stop the process from shutting down.""" + import signal as signal_mod + + from code_puppy_core_plugins.herdr import register_callbacks as rc + + installed = {} + with ( + patch.object(rc.atexit, "register", lambda fn, *a, **k: None), + patch.object(rc.signal, "signal", lambda s, h: installed.__setitem__(s, h)), + patch.object(rc.signal, "getsignal", lambda sig: signal_mod.SIG_DFL), + ): + rc._install_exit_guards() + + def boom(): + raise OSError("herdr socket gone") + + killed = [] + with ( + patch.object(rc._client, "release_and_close", boom), + patch.object(rc.os, "kill", lambda pid, sig: killed.append(sig)), + patch.object(rc.signal, "signal", lambda s, h: None), + ): + installed[signal_mod.SIGTERM](signal_mod.SIGTERM, None) + + assert killed == [signal_mod.SIGTERM] + + +def test_install_exit_guards_tolerates_unavailable_signal(): + """Non-main-thread / Windows-missing signals degrade quietly.""" + from code_puppy_core_plugins.herdr import register_callbacks as rc + + def refuse(sig, handler): + raise ValueError("signal only works in main thread") + + with ( + patch.object(rc.atexit, "register", lambda fn, *a, **k: None), + patch.object(rc.signal, "signal", refuse), + patch.object(rc.signal, "getsignal", lambda sig: None), + ): + rc._install_exit_guards() # must not raise + + +def test_terminating_signals_are_platform_safe(): + """SIGHUP is absent on Windows; resolving it must not break the import. + + ``client.py`` ships a named-pipe transport, so this module is imported on + Windows too -- a bare ``signal.SIGHUP`` reference would raise + AttributeError at import time and disable the plugin entirely. + """ + import signal as signal_mod + + from code_puppy_core_plugins.herdr import register_callbacks as rc + + assert signal_mod.SIGTERM in rc._TERMINATING_SIGNALS + assert all(s is not None for s in rc._TERMINATING_SIGNALS) + # Every entry must be a signal this platform actually knows about. + for sig in rc._TERMINATING_SIGNALS: + assert sig in set(signal_mod.Signals)