mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-27 17:58:07 +00:00
fix(computer_use): stop the cua-driver child on exit
CuaDriverBackend caches a long-lived cua-driver subprocess for the life of the Hermes process, and stop() was never called from anywhere — the driver outlived the session that spawned it. #69903 stopped the orphan from pegging a core by disabling the cursor overlay, but left the process behind; this is item 3 of #28152 ("Hermes does not keep the driver alive after tool completion"). Register an atexit hook, mirroring browser_tool's atexit.register(_emergency_cleanup_all_sessions). atexit only, no signal handlers, for the prompt_toolkit reason documented there. reset_backend_for_tests now reuses the same teardown instead of repeating it.
This commit is contained in:
parent
d9f1043c33
commit
4e49af94be
2 changed files with 107 additions and 8 deletions
75
tests/computer_use/test_cua_atexit_teardown.py
Normal file
75
tests/computer_use/test_cua_atexit_teardown.py
Normal file
|
|
@ -0,0 +1,75 @@
|
|||
"""Tests for cua-driver subprocess teardown on interpreter exit.
|
||||
|
||||
``CuaDriverBackend`` spawns a long-lived ``cua-driver`` child process and
|
||||
caches it for the life of the Hermes process. Nothing ever tore it down, so
|
||||
the driver outlived the session that started it (#28152 item 3 — "Hermes does
|
||||
not keep the driver alive after tool completion"). #69903 fixed the *overlay*
|
||||
redraw loop that made the lingering process burn CPU, but not the lingering
|
||||
process itself.
|
||||
|
||||
``tools/computer_use/tool.py`` registers an ``atexit`` hook that stops the
|
||||
cached backend, mirroring ``browser_tool``'s
|
||||
``atexit.register(_emergency_cleanup_all_sessions)``.
|
||||
|
||||
These assert the behavior contract — the hook is registered, it stops a live
|
||||
backend, it is a no-op when nothing was ever started, and it never raises out
|
||||
of ``atexit`` — not a snapshot of the module's source.
|
||||
"""
|
||||
|
||||
import atexit
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from tools.computer_use import tool as cu_tool
|
||||
|
||||
|
||||
class TestAtexitTeardown:
|
||||
def test_shutdown_stops_a_live_backend(self):
|
||||
"""A cached backend is stopped when the interpreter exits."""
|
||||
fake = MagicMock()
|
||||
with patch.object(cu_tool, "_backend", fake):
|
||||
cu_tool._shutdown_backend_atexit()
|
||||
fake.stop.assert_called_once()
|
||||
|
||||
def test_shutdown_clears_the_cached_backend(self):
|
||||
"""After teardown the module cache is empty, so a later call re-spawns."""
|
||||
fake = MagicMock()
|
||||
with patch.object(cu_tool, "_backend", fake):
|
||||
cu_tool._shutdown_backend_atexit()
|
||||
assert cu_tool._backend is None
|
||||
|
||||
def test_shutdown_is_a_noop_when_never_started(self):
|
||||
"""No backend was ever created => nothing to stop, no error."""
|
||||
with patch.object(cu_tool, "_backend", None):
|
||||
cu_tool._shutdown_backend_atexit() # must not raise
|
||||
assert cu_tool._backend is None
|
||||
|
||||
def test_shutdown_swallows_backend_errors(self):
|
||||
"""A failing stop() must not raise out of an atexit hook.
|
||||
|
||||
Exceptions escaping atexit print a traceback on every exit and can
|
||||
mask the real exit status.
|
||||
"""
|
||||
fake = MagicMock()
|
||||
fake.stop.side_effect = RuntimeError("driver already dead")
|
||||
with patch.object(cu_tool, "_backend", fake):
|
||||
cu_tool._shutdown_backend_atexit() # must not raise
|
||||
assert cu_tool._backend is None
|
||||
|
||||
def test_hook_is_registered_with_atexit(self):
|
||||
"""Importing the tool module registers the teardown hook.
|
||||
|
||||
Verified by unregistering and re-registering: atexit.unregister only
|
||||
removes a function that was actually registered, so a successful
|
||||
round-trip proves the import-time registration happened.
|
||||
"""
|
||||
atexit.unregister(cu_tool._shutdown_backend_atexit)
|
||||
try:
|
||||
with patch.object(cu_tool, "_backend", MagicMock()) as fake:
|
||||
# Re-register and fire the full atexit chain the way the
|
||||
# interpreter would, then confirm our hook ran.
|
||||
atexit.register(cu_tool._shutdown_backend_atexit)
|
||||
atexit._run_exitfuncs()
|
||||
fake.stop.assert_called_once()
|
||||
finally:
|
||||
atexit.unregister(cu_tool._shutdown_backend_atexit)
|
||||
atexit.register(cu_tool._shutdown_backend_atexit)
|
||||
|
|
@ -38,6 +38,7 @@ For captures / actions with `capture_after=True`:
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import atexit
|
||||
import base64
|
||||
import json
|
||||
import logging
|
||||
|
|
@ -177,16 +178,39 @@ def _get_backend() -> ComputerUseBackend:
|
|||
return _backend
|
||||
|
||||
|
||||
def _shutdown_backend_atexit() -> None:
|
||||
"""Stop the cached backend so the cua-driver child doesn't outlive us.
|
||||
|
||||
The backend is cached per-process and holds a long-lived ``cua-driver``
|
||||
subprocess, so without this the driver survives the Hermes process that
|
||||
spawned it (#28152 item 3). #69903 kept the orphan from burning a core by
|
||||
disabling the cursor overlay; the process itself still lingered.
|
||||
|
||||
Mirrors ``browser_tool``'s ``atexit.register(_emergency_cleanup_all_sessions)``
|
||||
— same spawn-and-drive-a-subprocess shape. atexit only, no signal handlers:
|
||||
a ``SystemExit`` raised from a prompt_toolkit key binding corrupts its
|
||||
coroutine state and makes the process unkillable. Never raises, since an
|
||||
exception escaping atexit prints a traceback on every exit.
|
||||
"""
|
||||
global _backend
|
||||
# Drop the lock before stop() — teardown budgets 5s and shouldn't block
|
||||
# an unrelated caller waiting to spawn.
|
||||
with _backend_lock:
|
||||
backend, _backend = _backend, None
|
||||
if backend is None:
|
||||
return
|
||||
try:
|
||||
backend.stop()
|
||||
except Exception as e:
|
||||
logger.debug("cua-driver atexit teardown failed: %s", e)
|
||||
|
||||
|
||||
atexit.register(_shutdown_backend_atexit)
|
||||
|
||||
|
||||
def reset_backend_for_tests() -> None: # pragma: no cover
|
||||
"""Test helper — tear down the cached backend and per-session state."""
|
||||
global _backend
|
||||
with _backend_lock:
|
||||
if _backend is not None:
|
||||
try:
|
||||
_backend.stop()
|
||||
except Exception:
|
||||
pass
|
||||
_backend = None
|
||||
_shutdown_backend_atexit()
|
||||
_AUX_VISION_ROUTE_CACHE.clear()
|
||||
with _approval_lock:
|
||||
_session_auto_approve.clear()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue