mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 16:18:55 +00:00
refactor(cli): apply /simplify-code findings to oneshot teardown
- Add idempotency guard (_oneshot_cleanup_done) to _cleanup_oneshot_runtime, matching cli.py:_run_cleanup's pattern - Trim _exit_after_oneshot docstring from 22 to 8 lines (per-resource ownership enumeration already documented in _run_agent) - Add comment clarifying cleanup ordering mirrors gateway/run.py, not cli.py (oneshot has no _active_agent_ref) - Clarify session_db.close() comment: agent.close() calls end_session() but leaves the connection open Findings skipped (follow-up scope): - Extract shared 5-step cleanup helper from cli.py:_run_cleanup (widens scope into critical file) - Extract _hard_exit helper (3 copies across cli.py) - Extract shutdown_agent_resources helper (touches run_agent.py + cli.py + gateway/run.py) - Test boilerplate dedup (test-only, non-blocking)
This commit is contained in:
parent
2de60a3a7e
commit
97fc8a4a3c
3 changed files with 22 additions and 25 deletions
|
|
@ -68,26 +68,13 @@ import sys
|
|||
def _exit_after_oneshot(rc: object) -> None:
|
||||
"""Exit one-shot mode without letting late native finalizers change rc.
|
||||
|
||||
Once ``run_oneshot`` has returned, it has emitted any response or diagnostic
|
||||
and ``_run_agent`` has already run the stateful agent cleanup
|
||||
(memory-provider shutdown, ``agent.close()``, recall-store close). The
|
||||
SIGABRT this guards against (#43055) fires in a native-extension finalizer
|
||||
during CPython's ``Py_FinalizeEx``, *after* the response has printed, so we
|
||||
flush user-visible streams, shut down file logging, then ``os._exit`` past
|
||||
the interpreter finalization that aborts.
|
||||
|
||||
We deliberately do *not* drain the Python ``atexit`` chain here. The
|
||||
aborting finalizer has not been confirmed on the reporter's AL2023 host,
|
||||
and several registered handlers (browser/LSP emergency sweeps) re-enter
|
||||
native code and subprocess teardown — the exact class of code that may be
|
||||
the abort source — so running them just before the hard exit risks
|
||||
re-arming the crash this routine exists to contain. The stateful cleanup
|
||||
that actually matters for one-shot is closed explicitly in ``_run_agent``:
|
||||
the agent owns its task-scoped processes, terminal/browser sessions, child
|
||||
agents, and clients, while the recall SQLite store is checkpointed there as
|
||||
well. Process-wide orphan sweeps and other general ``atexit`` handlers are
|
||||
intentionally left to a later normal interpreter lifetime; daemon workers
|
||||
and remaining file descriptors are reclaimed when this process exits.
|
||||
The SIGABRT this guards against (#30387, #43055) fires in a
|
||||
native-extension finalizer during CPython's ``Py_FinalizeEx``, *after*
|
||||
the response has printed. Flush streams, shut down file logging, then
|
||||
``os._exit`` past interpreter finalization. The ``atexit`` chain is
|
||||
deliberately skipped — several handlers re-enter native code that may
|
||||
be the abort source. Stateful cleanup is handled in ``_run_agent`` and
|
||||
``_cleanup_oneshot_runtime``.
|
||||
"""
|
||||
for stream in (sys.stdout, sys.stderr):
|
||||
try:
|
||||
|
|
@ -108,6 +95,9 @@ def _exit_after_oneshot(rc: object) -> None:
|
|||
os._exit(exit_code)
|
||||
|
||||
|
||||
_oneshot_cleanup_done = False
|
||||
|
||||
|
||||
def _cleanup_oneshot_runtime() -> None:
|
||||
"""Best-effort process-global cleanup before one-shot hard exit.
|
||||
|
||||
|
|
@ -116,6 +106,10 @@ def _cleanup_oneshot_runtime() -> None:
|
|||
process-global pieces from ``cli.py:_run_cleanup()`` that would otherwise
|
||||
be skipped by ``os._exit``.
|
||||
"""
|
||||
global _oneshot_cleanup_done
|
||||
if _oneshot_cleanup_done:
|
||||
return
|
||||
_oneshot_cleanup_done = True
|
||||
try:
|
||||
from tools.terminal_tool import cleanup_all_environments
|
||||
cleanup_all_environments()
|
||||
|
|
|
|||
|
|
@ -442,6 +442,9 @@ def _run_agent(
|
|||
result = agent.run_conversation(prompt)
|
||||
return (result.get("final_response") or "", result)
|
||||
finally:
|
||||
# Ordering deliberately mirrors gateway/run.py:_cleanup_agent_resources,
|
||||
# NOT cli.py:_run_cleanup — oneshot has no _active_agent_ref and must
|
||||
# close the agent explicitly because the hard-exit path skips finalizers.
|
||||
if agent is not None:
|
||||
try:
|
||||
session_messages = getattr(agent, "_session_messages", None)
|
||||
|
|
@ -455,11 +458,9 @@ def _run_agent(
|
|||
agent.close()
|
||||
except Exception:
|
||||
logging.debug("oneshot agent cleanup failed", exc_info=True)
|
||||
# Close the recall SQLite store we opened for this run. Message rows
|
||||
# are committed synchronously during the turn, so nothing is lost, but
|
||||
# the one-shot exit path hard-exits via os._exit and skips finalizers
|
||||
# — close here so the connection (and its WAL) is checkpointed cleanly
|
||||
# instead of relying on interpreter teardown.
|
||||
# agent.close() calls session_db.end_session() but leaves the connection
|
||||
# open; close it here to checkpoint the WAL before os._exit skips
|
||||
# finalizers.
|
||||
if session_db is not None:
|
||||
try:
|
||||
session_db.close()
|
||||
|
|
|
|||
|
|
@ -32,6 +32,8 @@ def main_mod(monkeypatch):
|
|||
import hermes_cli.main as mod
|
||||
|
||||
monkeypatch.setattr(mod, "_has_any_provider_configured", lambda: True)
|
||||
# Reset the idempotency guard so each test starts fresh.
|
||||
monkeypatch.setattr(mod, "_oneshot_cleanup_done", False)
|
||||
return mod
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue