mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(telegram): shut down abandoned init app + AUTHOR_MAP + cover the deadline helper
Follow-up to @msh01's wall-deadline init-timeout fix. - Resource leak: on timeout the initialize() task is abandoned without awaiting its (shielded, possibly-never-completing) cancellation, so the half-built PTB app's httpx client / connection pool was never closed — up to 8x across the retry ladder. Add an optional on_abandon cleanup to _await_with_thread_deadline that best-effort app.shutdown()s the abandoned app, run detached + exception-swallowed so it can never re-block or re-hang the ladder (mirrors _close_client_on_timeout in agent/auxiliary_client.py). - Cover the helper itself: the salvaged test monkeypatched out the real _await_with_thread_deadline, so its abandonment/cleanup path was untested. Add direct tests for happy-path return, prompt-timeout-with-cleanup, and cleanup-error-swallowed; the wedged coroutines swallow cancellation for a bounded window (proving the helper returns before cancellation completes, the #58236 shielded-scope behavior) without leaving an immortal task that would wedge pytest teardown. Widen the salvaged stub to accept on_abandon. - Attribution: add yingwaizhiying@gmail.com -> msh01 to AUTHOR_MAP (bare gmail does not auto-resolve the check-attribution gate). Known follow-up (not addressed here): the retry ladder reuses the same self._app across all 8 attempts; a fresh app per attempt would fully close the coherence risk if an abandoned initialize() completes in the background. That is a larger restructure of the ~130-line builder+handler setup, left for a separate change.
This commit is contained in:
parent
d50aae0e35
commit
a37fd66dec
3 changed files with 197 additions and 2 deletions
|
|
@ -54,7 +54,7 @@ async def test_connect_retries_when_initialize_wall_deadline_expires(monkeypatch
|
|||
|
||||
deadline_calls = 0
|
||||
|
||||
async def _fake_deadline(awaitable, timeout):
|
||||
async def _fake_deadline(awaitable, timeout, *, on_abandon=None):
|
||||
nonlocal deadline_calls
|
||||
deadline_calls += 1
|
||||
if deadline_calls == 1:
|
||||
|
|
@ -79,3 +79,123 @@ async def test_connect_retries_when_initialize_wall_deadline_expires(monkeypatch
|
|||
assert deadline_calls == 2
|
||||
tg_adapter.asyncio.sleep.assert_awaited_once_with(1)
|
||||
fake_app.start.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_await_with_thread_deadline_returns_value_on_happy_path():
|
||||
"""The real helper returns the awaited result and raises no timeout."""
|
||||
async def _ok():
|
||||
return 42
|
||||
|
||||
result = await tg_adapter._await_with_thread_deadline(_ok(), timeout=5.0)
|
||||
assert result == 42
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_await_with_thread_deadline_abandons_and_runs_cleanup_on_timeout():
|
||||
"""A wedged awaitable must raise TimeoutError promptly AND trigger the
|
||||
best-effort on_abandon cleanup (the httpx-pool-leak guard).
|
||||
|
||||
This exercises the REAL _await_with_thread_deadline (not a monkeypatched
|
||||
stub), covering the abandonment + cleanup mechanism directly.
|
||||
"""
|
||||
import asyncio as _asyncio
|
||||
import time as _time
|
||||
|
||||
cleanup_ran = _asyncio.Event()
|
||||
|
||||
async def _wedged():
|
||||
# Swallows cancellation for a bounded window — long enough that the
|
||||
# helper must return control BEFORE this finishes (proving it doesn't
|
||||
# await cancellation, the #58236 shielded-scope behavior), but bounded
|
||||
# so the abandoned task can't outlive the test and wedge teardown.
|
||||
for _ in range(20):
|
||||
try:
|
||||
await _asyncio.sleep(0.05)
|
||||
except _asyncio.CancelledError:
|
||||
# Keep going despite cancellation, like the shielded scope.
|
||||
pass
|
||||
|
||||
async def _cleanup():
|
||||
cleanup_ran.set()
|
||||
|
||||
started = _time.monotonic()
|
||||
with pytest.raises(_asyncio.TimeoutError):
|
||||
await tg_adapter._await_with_thread_deadline(
|
||||
_wedged(), timeout=0.2, on_abandon=_cleanup
|
||||
)
|
||||
elapsed = _time.monotonic() - started
|
||||
|
||||
# Returned control promptly — well before the wedged coroutine's ~1s span.
|
||||
assert elapsed < 0.8
|
||||
# The detached cleanup was scheduled; give the loop a tick to run it.
|
||||
await _asyncio.wait_for(cleanup_ran.wait(), timeout=2.0)
|
||||
assert cleanup_ran.is_set()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_await_with_thread_deadline_cleanup_error_is_swallowed():
|
||||
"""A cleanup that raises must not surface as an unhandled task error."""
|
||||
import asyncio as _asyncio
|
||||
|
||||
async def _wedged():
|
||||
for _ in range(20):
|
||||
try:
|
||||
await _asyncio.sleep(0.05)
|
||||
except _asyncio.CancelledError:
|
||||
pass
|
||||
|
||||
def _boom():
|
||||
raise RuntimeError("cleanup blew up")
|
||||
|
||||
# Must still raise TimeoutError (not the cleanup error) and not crash.
|
||||
with pytest.raises(_asyncio.TimeoutError):
|
||||
await tg_adapter._await_with_thread_deadline(
|
||||
_wedged(), timeout=0.2, on_abandon=_boom
|
||||
)
|
||||
# Let the detached cleanup task run and be observed (no unraised error).
|
||||
await _asyncio.sleep(0.05)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_shutdown_abandoned_app_closes_request_transports_when_uninitialized():
|
||||
"""The leak fix must release the httpx transports even when PTB's own
|
||||
Application.shutdown()/Bot.shutdown() no-op because the wedged initialize()
|
||||
never flipped _initialized. _shutdown_abandoned_app falls back to closing
|
||||
each bot._request transport directly (HTTPXRequest.shutdown gates only on
|
||||
client.is_closed, not on an init flag)."""
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
# A half-built app: shutdown() is a no-op (uninitialized), but the request
|
||||
# transports still hold open httpx clients that must be closed.
|
||||
req0 = MagicMock()
|
||||
req0.shutdown = AsyncMock()
|
||||
req1 = MagicMock()
|
||||
req1.shutdown = AsyncMock()
|
||||
bot = MagicMock()
|
||||
bot._request = (req0, req1)
|
||||
app = MagicMock()
|
||||
app.bot = bot
|
||||
app.shutdown = AsyncMock(return_value=None) # PTB no-op on uninitialized app
|
||||
|
||||
await tg_adapter._shutdown_abandoned_app(app)
|
||||
|
||||
app.shutdown.assert_awaited_once()
|
||||
# Fell back to closing the transports directly — the actual leak fix.
|
||||
req0.shutdown.assert_awaited_once()
|
||||
req1.shutdown.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_shutdown_abandoned_app_handles_none_and_missing_requests():
|
||||
"""Robust against app=None and an app whose bot/_request aren't present."""
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
# None app -> no-op, no crash.
|
||||
await tg_adapter._shutdown_abandoned_app(None)
|
||||
|
||||
# app.shutdown() raising must be swallowed, and missing _request tolerated.
|
||||
app = MagicMock()
|
||||
app.shutdown = AsyncMock(side_effect=RuntimeError("still running"))
|
||||
app.bot = None
|
||||
await tg_adapter._shutdown_abandoned_app(app) # must not raise
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue