From 97fc8a4a3c6a5a2bc7a1a0d09c11b5c803bd03f2 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 20 Jul 2026 12:37:59 +0530 Subject: [PATCH] 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) --- hermes_cli/main.py | 34 ++++++++++-------------- hermes_cli/oneshot.py | 11 ++++---- tests/hermes_cli/test_tui_resume_flow.py | 2 ++ 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 3982d0e86c94..7ce7ece58d65 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -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() diff --git a/hermes_cli/oneshot.py b/hermes_cli/oneshot.py index d600c0a9f297..e0c1337698ce 100644 --- a/hermes_cli/oneshot.py +++ b/hermes_cli/oneshot.py @@ -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() diff --git a/tests/hermes_cli/test_tui_resume_flow.py b/tests/hermes_cli/test_tui_resume_flow.py index 184aa7d6e20a..d171ab3d14a4 100644 --- a/tests/hermes_cli/test_tui_resume_flow.py +++ b/tests/hermes_cli/test_tui_resume_flow.py @@ -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