Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cron/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,8 @@ def run_job(job: dict) -> tuple[bool, str, str, Optional[str]]:
logger.info("Running job '%s' (ID: %s)", job_name, job_id)
logger.info("Prompt: %s", prompt[:100])

agent = None

# Mark this as a cron session so the approval system can apply cron_mode.
# This env var is process-wide and persists for the lifetime of the
# scheduler process — every job this process runs is a cron job.
Expand Down Expand Up @@ -1032,6 +1034,24 @@ def run_job(job: dict) -> tuple[bool, str, str, Optional[str]]:
_session_db.close()
except (Exception, KeyboardInterrupt) as e:
logger.debug("Job '%s': failed to close SQLite session store: %s", job_id, e)
# Release subprocesses, terminal sandboxes, browser daemons, and the
# main OpenAI/httpx client held by this ephemeral cron agent. Without
# this, a gateway that ticks cron every N minutes leaks fds per job
# until it hits EMFILE (#10200 / "too many open files").
try:
if agent is not None:
agent.close()
except (Exception, KeyboardInterrupt) as e:
logger.debug("Job '%s': failed to close agent resources: %s", job_id, e)
# Each cron run spins up a short-lived worker thread whose event loop
# dies as soon as the ``ThreadPoolExecutor`` shuts down. Any async
# httpx clients cached under that loop are now unusable — reap them
# so their transports don't accumulate in the process-global cache.
try:
from agent.auxiliary_client import cleanup_stale_async_clients
cleanup_stale_async_clients()
except Exception as e:
logger.debug("Job '%s': failed to reap stale auxiliary clients: %s", job_id, e)


def tick(verbose: bool = True, adapters=None, loop=None) -> int:
Expand Down
14 changes: 14 additions & 0 deletions gateway/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -1754,6 +1754,15 @@ def _cleanup_agent_resources(self, agent: Any) -> None:
agent.close()
except Exception:
pass
# Auxiliary async clients (session_search/web/vision/etc.) live in a
# process-global cache and are created inside worker threads. Clean up
# any entries whose event loop is now dead so their httpx transports do
# not accumulate across gateway turns.
try:
from agent.auxiliary_client import cleanup_stale_async_clients
cleanup_stale_async_clients()
except Exception:
pass

_STUCK_LOOP_THRESHOLD = 3 # restarts while active before auto-suspend
_STUCK_LOOP_FILE = ".restart_failure_counts"
Expand Down Expand Up @@ -2653,6 +2662,11 @@ async def _stop_impl() -> None:
cleanup_all_browsers()
except Exception:
pass
try:
from agent.auxiliary_client import shutdown_cached_clients
shutdown_cached_clients()
except Exception:
pass

# Close SQLite session DBs so the WAL write lock is released.
# Without this, --replace and similar restart flows leave the
Expand Down
14 changes: 10 additions & 4 deletions gateway/status.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,19 @@ def _read_pid_record(pid_path: Optional[Path] = None) -> Optional[dict]:


def _cleanup_invalid_pid_path(pid_path: Path, *, cleanup_stale: bool) -> None:
# Callers only reach this helper after ``get_running_pid`` has already
# determined the file's record is stale (dead pid, unparseable record,
# or mismatched start_time). At that point the file never belongs to a
# live process, so it must always be unlinked — ``remove_pid_file``'s
# "only touch files that belong to me" safety check is for the atexit
# --replace handoff and would here leave the stale file in place,
# causing the next ``write_pid_file()`` (O_EXCL) to fail with
# ``FileExistsError`` and the gateway to exit with
# "PID file race lost to another gateway instance".
if not cleanup_stale:
return
try:
if pid_path == _get_pid_path():
remove_pid_file()
else:
pid_path.unlink(missing_ok=True)
pid_path.unlink(missing_ok=True)
except Exception:
pass

Expand Down
73 changes: 73 additions & 0 deletions tests/cron/test_scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,79 @@ def test_run_job_passes_session_db_and_cron_platform(self, tmp_path):
assert call_args[0][0].startswith("cron_test-job_")
assert call_args[0][1] == "cron_complete"
fake_db.close.assert_called_once()
mock_agent.close.assert_called_once()

def test_run_job_closes_agent_on_failure_to_prevent_fd_leak(self, tmp_path):
# Regression: if ``run_conversation`` raises, the ephemeral cron
# agent was previously leaked — over days of ticks this accumulated
# httpx transports and hit EMFILE / "too many open files".
job = {
"id": "failing-job",
"name": "failing",
"prompt": "hello",
}
fake_db = MagicMock()

with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
return_value={
"api_key": "***",
"base_url": "https://example.invalid/v1",
"provider": "openrouter",
"api_mode": "chat_completions",
},
), \
patch("run_agent.AIAgent") as mock_agent_cls:
mock_agent = MagicMock()
mock_agent.run_conversation.side_effect = RuntimeError("boom")
mock_agent_cls.return_value = mock_agent

success, output, final_response, error = run_job(job)

assert success is False
assert final_response == ""
assert "RuntimeError: boom" in error
mock_agent.close.assert_called_once()

def test_run_job_reaps_stale_auxiliary_clients_per_tick(self, tmp_path):
# Regression: auxiliary clients bound to the cron worker's dead
# event loop must be reaped each tick. Without this, ``_client_cache``
# holds onto transports whose underlying sockets can no longer be
# closed (their loop is gone), leaking one fd batch per cron run.
job = {
"id": "aux-clean-job",
"name": "aux-clean",
"prompt": "hello",
}
fake_db = MagicMock()

with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
return_value={
"api_key": "***",
"base_url": "https://example.invalid/v1",
"provider": "openrouter",
"api_mode": "chat_completions",
},
), \
patch("run_agent.AIAgent") as mock_agent_cls, \
patch("agent.auxiliary_client.cleanup_stale_async_clients") as cleanup_mock:
mock_agent = MagicMock()
mock_agent.run_conversation.return_value = {"final_response": "ok"}
mock_agent_cls.return_value = mock_agent

success, _output, _final_response, _error = run_job(job)

assert success is True
cleanup_mock.assert_called_once()

def test_run_job_empty_response_returns_empty_not_placeholder(self, tmp_path):
"""Empty final_response should stay empty for delivery logic (issue #2234).
Expand Down
19 changes: 18 additions & 1 deletion tests/gateway/test_gateway_shutdown.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ async def block_forever(_event):
assert adapter._pending_messages == {}


def test_cleanup_agent_resources_reaps_stale_aux_clients():
runner, _adapter = make_restart_runner()
agent = MagicMock()

with patch("agent.auxiliary_client.cleanup_stale_async_clients") as cleanup_mock:
runner._cleanup_agent_resources(agent)

agent.shutdown_memory_provider.assert_called_once()
agent.close.assert_called_once()
cleanup_mock.assert_called_once()


@pytest.mark.asyncio
async def test_gateway_stop_interrupts_running_agents_and_cancels_adapter_tasks():
runner, adapter = make_restart_runner()
Expand All @@ -60,11 +72,16 @@ async def block_forever(_event):
running_agent = MagicMock()
runner._running_agents = {session_key: running_agent}

with patch("gateway.status.remove_pid_file"), patch("gateway.status.write_runtime_status"):
with (
patch("gateway.status.remove_pid_file"),
patch("gateway.status.write_runtime_status"),
patch("agent.auxiliary_client.shutdown_cached_clients") as shutdown_cached_clients,
):
await runner.stop()

running_agent.interrupt.assert_called_once_with("Gateway shutting down")
disconnect_mock.assert_awaited_once()
shutdown_cached_clients.assert_called_once()
assert runner.adapters == {}
assert runner._running_agents == {}
assert runner._pending_messages == {}
Expand Down
23 changes: 23 additions & 0 deletions tests/gateway/test_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,29 @@ def test_get_running_pid_rejects_live_non_gateway_pid(self, tmp_path, monkeypatc
assert status.get_running_pid() is None
assert not pid_path.exists()

def test_get_running_pid_cleans_stale_record_from_dead_process(self, tmp_path, monkeypatch):
# Simulates the aftermath of a crash: the PID file still points at a
# process that no longer exists. The next gateway startup must be
# able to unlink it so ``write_pid_file``'s O_EXCL create succeeds —
# otherwise systemd's restart loop hits "PID file race lost" forever.
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
pid_path = tmp_path / "gateway.pid"
dead_pid = 999999 # not our pid, and below we simulate it's dead
pid_path.write_text(json.dumps({
"pid": dead_pid,
"kind": "hermes-gateway",
"argv": ["python", "-m", "hermes_cli.main", "gateway", "run"],
"start_time": 111,
}))

def _dead_process(pid, sig):
raise ProcessLookupError

monkeypatch.setattr(status.os, "kill", _dead_process)

assert status.get_running_pid() is None
assert not pid_path.exists()

def test_get_running_pid_accepts_gateway_metadata_when_cmdline_unavailable(self, tmp_path, monkeypatch):
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
pid_path = tmp_path / "gateway.pid"
Expand Down