mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-11 13:41:53 +00:00
Cron jobs run through cron/scheduler.py's own ThreadPoolExecutor via a
standalone AIAgent (run_job/run_one_job), entirely outside
GatewayRunner._running_agents -- the dict _drain_active_agents() and
every other active-work check on that class reads. A gateway shutdown
(/update, /restart, and SIGUSR1 all funnel through the same stop())
could log active_at_start=0 and immediately kill tool subprocesses
while a cron job's terminal command was still running, with no wait
and no indication anything was interrupted.
Real-world impact (from the issue): a scheduled daily briefing cron
job was in flight during /update, its tool subprocess got killed
by the unconditional shutdown cleanup, and the job was never marked
failed -- it simply never completed or delivered, with no error
surfaced anywhere. A repro with a 30-minute `sleep` cron job in flight
during /update reproduced the same pattern: subprocess killed at
+0.22s of drain (active_at_start=0), the job's agent thread continued
in-process and produced a plausible-looking final response from the
truncated tool output, and the scheduler marked the run successful.
Root cause is layered, not a single line:
1. GatewayRunner._drain_active_agents() only waits on _running_agents.
Cron work was invisible to it, so drain returned instantly whenever
the only active work was a cron job.
2. Even with visibility, the shutdown's final tool-subprocess kill
(process_registry.kill_all()) is a global, unconditional sweep with
no per-job targeting -- a long-running cron job that outlives the
drain timeout still gets its subprocess killed.
3. cron/scheduler.py had no way to detect that a job's tool subprocess
was killed out from under it mid-run; the agent thread kept going
and its eventual (often degraded but plausible-looking) response
got reported as a normal successful completion.
Fix, three parts:
- cron/scheduler.py: expose get_running_job_ids() (thread-safe
snapshot of the existing _running_job_ids set, already used to
prevent double-dispatch) so the gateway can read cron's in-flight
state without reaching into private module internals.
- gateway/run.py: GatewayRunner._active_cron_job_count() reads that
snapshot. _drain_active_agents() now waits on
(_running_agents OR active cron jobs), so a cron-only workload gets
the same bounded wait chat sessions already get instead of an
instant active_at_start=0. Shutdown drain logging gains
cron_active_at_start/cron_active_now fields alongside the existing
ones (unchanged, for compat).
- cron/scheduler.py: mark_running_jobs_interrupted(reason), called by
gateway/run.py's _kill_tool_subprocesses() right after
process_registry.kill_all(), marks every job still in
_running_job_ids at that instant as failed/interrupted via the
existing mark_job_run() -- and records the job IDs in
_interrupted_job_ids BEFORE writing, so run_one_job()'s own
eventual completion for the same run (racing in its own thread)
checks that flag and skips its normal write instead of clobbering
the interrupted status with a false "ok" produced from the
now-truncated tool output. This does not attempt to correlate a
killed PID to a specific job ID (process_registry tracks PIDs, not
job IDs) -- any job still dispatched at the moment of a forced kill
is treated as interrupted, matching the existing coarser precedent
set by _interrupt_running_agents(), which interrupts every entry in
_running_agents on a drain timeout without per-agent correlation
either.
Deliberately out of scope (flagged in the issue as a separate,
lower-priority concern): startup-time reconciliation of cron runs that
started but never reached a terminal status.
Testing:
- tests/cron/test_shutdown_interrupt.py (12 tests): get_running_job_ids
snapshot semantics, mark_running_jobs_interrupted marking/no-op/
partial-failure behavior, and -- the core race guard -- run_one_job
skipping its own last_status write (both the success path and the
exception path) when the shutdown path already marked the run
interrupted, with a control test proving ordinary un-interrupted
completions are unaffected.
- tests/gateway/test_cron_active_work_drain.py (9 tests):
_active_cron_job_count reading cron state and failing closed (0) if
the cron module is unavailable; _drain_active_agents waiting for an
in-flight cron job the same way it waits for chat sessions, timing
out if the job outruns the window, and leaving existing chat-session
drain behavior unchanged; a full runner.stop() integration test
(drain-timeout path) proving mark_running_jobs_interrupted actually
fires with the right job ID when a tool subprocess is force-killed,
plus a no-op control when nothing cron-related is in flight.
- tests/gateway/test_shutdown_cache_cleanup.py: added
_active_cron_job_count() to that file's hand-rolled _FakeGateway test
double, which stop() now calls -- without it those 8 pre-existing
tests AttributeError (caught by fail-then-pass below, not a
production bug).
Fail-then-pass: reverted gateway/run.py + cron/scheduler.py, all 21
new tests fail (fixture/attribute errors -- the feature doesn't exist
yet); restored, all 21 pass.
Regression check: ran the full plausibly-affected surface --
tests/gateway/{test_gateway_shutdown,test_restart_drain,
test_restart_notification,test_restart_redelivery_dedup,
test_restart_resume_pending,test_restart_service_detection,
test_shutdown_cache_cleanup,test_stuck_loop,test_clean_shutdown_marker,
test_external_drain_control,test_session_state_cleanup,
test_update_command,test_update_streaming}.py plus tests/cron/ (944
tests) -- against a clean upstream/main checkout and against this
branch. Diffed the two FAILED lists: identical, 20 pre-existing
failures on both sides (Windows-locale/cp1252 file-encoding issues and
Unix-permission-bit assertions that don't apply on this Windows dev
box), zero new failures, zero fixed-by-accident. The 8
test_shutdown_cache_cleanup.py failures found mid-development were
from the _FakeGateway gap above, fixed in the same commit and
confirmed clean on the final rerun (diff against baseline: exit 0).
Fixes #60432
232 lines
7.6 KiB
Python
232 lines
7.6 KiB
Python
"""Regression tests for gateway shutdown cleaning up cached agent memory providers (issue #11205).
|
|
|
|
When the gateway shuts down, ``stop()`` called ``_finalize_shutdown_agents()``
|
|
which only drained agents in ``_running_agents``. Idle agents sitting in
|
|
``_agent_cache`` (LRU cache) were never cleaned up, so their
|
|
``MemoryProvider.on_session_end()`` hooks never fired.
|
|
|
|
The fix adds an explicit sweep of ``_agent_cache`` after
|
|
``_finalize_shutdown_agents`` in the ``_stop_impl`` coroutine.
|
|
"""
|
|
|
|
import asyncio
|
|
import threading
|
|
from collections import OrderedDict
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
# Import the module (not the class) to reach stop() and helpers
|
|
import gateway.run as gw_mod
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Helpers
|
|
# ---------------------------------------------------------------------------
|
|
|
|
class _FakeGateway:
|
|
"""Minimal stand-in with just enough state for ``stop()`` to run."""
|
|
|
|
def __init__(self):
|
|
self._running = True
|
|
self._draining = False
|
|
self._restart_requested = False
|
|
self._restart_detached = False
|
|
self._restart_via_service = False
|
|
self._stop_task = None
|
|
self._exit_cleanly = False
|
|
self._exit_with_failure = False
|
|
self._exit_reason = None
|
|
self._exit_code = None
|
|
self._restart_drain_timeout = 0.01
|
|
self._running_agents = {}
|
|
self._running_agents_ts = {}
|
|
self._agent_cache = OrderedDict()
|
|
self._agent_cache_lock = threading.Lock()
|
|
self.adapters = {}
|
|
self._background_tasks = set()
|
|
self._failed_platforms = []
|
|
self._shutdown_event = asyncio.Event()
|
|
self._pending_messages = {}
|
|
self._pending_approvals = {}
|
|
self._busy_ack_ts = {}
|
|
|
|
def _running_agent_count(self):
|
|
return len(self._running_agents)
|
|
|
|
def _active_cron_job_count(self):
|
|
# stop() reads this alongside _running_agent_count when logging the
|
|
# drain snapshot (#60432) -- this fake has no cron scheduler, so
|
|
# there's never in-flight cron work to report.
|
|
return 0
|
|
|
|
def _update_runtime_status(self, *_a, **_kw):
|
|
pass
|
|
|
|
async def _run_in_executor_with_context(self, func, *args):
|
|
# stop() offloads agent-resource cleanup off the loop (#53175); run
|
|
# inline in tests so the bounded-cleanup path is exercised.
|
|
return func(*args)
|
|
|
|
async def _cleanup_agent_resources_off_loop(self, agent, *, context=""):
|
|
# Mirror the real bounded helper, inline (no executor/timeout) so the
|
|
# fake exercises the same call shape stop() now uses.
|
|
self._cleanup_agent_resources(agent)
|
|
|
|
async def _notify_active_sessions_of_shutdown(self):
|
|
pass
|
|
|
|
async def _drain_active_agents(self, timeout):
|
|
return {}, False
|
|
|
|
async def _finalize_shutdown_agents(self, agents):
|
|
for agent in agents.values():
|
|
self._cleanup_agent_resources(agent)
|
|
|
|
def _cleanup_agent_resources(self, agent):
|
|
if agent is None:
|
|
return
|
|
try:
|
|
if hasattr(agent, "shutdown_memory_provider"):
|
|
agent.shutdown_memory_provider()
|
|
except Exception:
|
|
pass
|
|
try:
|
|
if hasattr(agent, "close"):
|
|
agent.close()
|
|
except Exception:
|
|
pass
|
|
|
|
def _evict_cached_agent(self, key):
|
|
pass
|
|
|
|
def _release_running_agent_state(self, session_key, **_kwargs):
|
|
agent = self._running_agents.pop(session_key, None)
|
|
self._running_agents_ts.pop(session_key, None)
|
|
self._cleanup_agent_resources(agent)
|
|
return agent is not None
|
|
|
|
|
|
def _make_mock_agent():
|
|
a = MagicMock()
|
|
a.shutdown_memory_provider = MagicMock()
|
|
a.close = MagicMock()
|
|
return a
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Tests
|
|
# ---------------------------------------------------------------------------
|
|
|
|
class TestCachedAgentCleanupOnShutdown:
|
|
"""Verify that ``stop()`` calls ``_cleanup_agent_resources`` on idle
|
|
cached agents, triggering ``shutdown_memory_provider()`` (which calls
|
|
``on_session_end``)."""
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_cached_agent_memory_provider_shut_down(self):
|
|
"""A cached agent's shutdown_memory_provider is called during gateway stop."""
|
|
gw = _FakeGateway()
|
|
agent = _make_mock_agent()
|
|
gw._agent_cache["session-1"] = (agent, "sig-123")
|
|
|
|
# Call the real stop() from GatewayRunner
|
|
await gw_mod.GatewayRunner.stop(gw)
|
|
|
|
agent.shutdown_memory_provider.assert_called_once()
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_cache_cleared_after_shutdown(self):
|
|
"""The _agent_cache dict is cleared after stop."""
|
|
gw = _FakeGateway()
|
|
agent = _make_mock_agent()
|
|
gw._agent_cache["s1"] = (agent, "sig1")
|
|
|
|
await gw_mod.GatewayRunner.stop(gw)
|
|
|
|
assert len(gw._agent_cache) == 0
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_no_cached_agents_no_error(self):
|
|
"""stop() works fine when _agent_cache is empty."""
|
|
gw = _FakeGateway()
|
|
|
|
await gw_mod.GatewayRunner.stop(gw) # Should not raise
|
|
|
|
assert len(gw._agent_cache) == 0
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_multiple_cached_agents_all_cleaned(self):
|
|
"""All cached agents get cleaned up."""
|
|
gw = _FakeGateway()
|
|
agents = []
|
|
for i in range(5):
|
|
a = _make_mock_agent()
|
|
agents.append(a)
|
|
gw._agent_cache[f"s{i}"] = (a, f"sig{i}")
|
|
|
|
await gw_mod.GatewayRunner.stop(gw)
|
|
|
|
for a in agents:
|
|
a.shutdown_memory_provider.assert_called_once()
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_cleanup_survives_agent_exception(self):
|
|
"""An exception from one agent's shutdown doesn't prevent others."""
|
|
gw = _FakeGateway()
|
|
|
|
bad = _make_mock_agent()
|
|
bad.shutdown_memory_provider.side_effect = RuntimeError("boom")
|
|
bad.close.side_effect = RuntimeError("boom")
|
|
|
|
good = _make_mock_agent()
|
|
|
|
gw._agent_cache["bad"] = (bad, "sig-bad")
|
|
gw._agent_cache["good"] = (good, "sig-good")
|
|
|
|
await gw_mod.GatewayRunner.stop(gw)
|
|
|
|
# The good agent should still be cleaned up
|
|
good.shutdown_memory_provider.assert_called_once()
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_plain_agent_not_tuple(self):
|
|
"""Cache entries that aren't tuples (just bare agents) are also cleaned."""
|
|
gw = _FakeGateway()
|
|
agent = _make_mock_agent()
|
|
gw._agent_cache["s1"] = agent # Not a tuple
|
|
|
|
await gw_mod.GatewayRunner.stop(gw)
|
|
|
|
agent.shutdown_memory_provider.assert_called_once()
|
|
assert len(gw._agent_cache) == 0
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_none_entry_skipped(self):
|
|
"""A None cache entry doesn't cause errors."""
|
|
gw = _FakeGateway()
|
|
gw._agent_cache["s1"] = None
|
|
|
|
await gw_mod.GatewayRunner.stop(gw)
|
|
|
|
assert len(gw._agent_cache) == 0
|
|
|
|
|
|
class TestRunningAgentsNotDoubleCleaned:
|
|
"""Verify behavior when agents appear in both _running_agents and _agent_cache."""
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_running_and_cached_agent_cleaned_at_least_once(self):
|
|
"""An agent in both _running_agents and _agent_cache gets
|
|
shutdown_memory_provider called at least once."""
|
|
gw = _FakeGateway()
|
|
shared = _make_mock_agent()
|
|
|
|
gw._running_agents["s1"] = shared
|
|
gw._agent_cache["s1"] = (shared, "sig1")
|
|
|
|
await gw_mod.GatewayRunner.stop(gw)
|
|
|
|
# Called at least once — either from _finalize_shutdown_agents
|
|
# or from the cache sweep (or both)
|
|
assert shared.shutdown_memory_provider.call_count >= 1
|