mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(cron): prevent multi-target delivery loop crash on per-target failure
The standalone thread-pool fallback in _deliver_result() runs inside the `except RuntimeError:` block (taken when asyncio.run() sees a running loop). When future.result() raised there (SMTP ConnectionError, timeout, etc.), the exception was NOT caught by the sibling `except Exception:` — it escaped _deliver_result() and crashed the whole delivery loop, silently skipping every remaining target. Multi-target delivery (e.g. deliver: 'email:a,email:b') is a documented feature, so this broke a promised contract. Wrap the fallback in its own try/except so a per-target failure is logged with exc_info and the loop continues to the next target. Fixes #47163
This commit is contained in:
parent
d3010b74db
commit
242c9639a8
3 changed files with 99 additions and 4 deletions
|
|
@ -1696,12 +1696,30 @@ def _deliver_result(job: dict, content: str, adapters=None, loop=None) -> Option
|
|||
# prevent "coroutine was never awaited" RuntimeWarning, then retry in a
|
||||
# fresh thread that has no running loop.
|
||||
coro.close()
|
||||
with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
|
||||
future = pool.submit(asyncio.run, _send_to_platform(platform, pconfig, chat_id, cleaned_delivery_content, thread_id=thread_id, media_files=media_files))
|
||||
result = future.result(timeout=30)
|
||||
# The thread-pool fallback can itself raise (SMTP ConnectionError,
|
||||
# future.result timeout, etc.). An exception raised inside this
|
||||
# `except RuntimeError` block is NOT caught by the sibling
|
||||
# `except Exception` below — it would escape _deliver_result()
|
||||
# and crash the whole delivery loop, silently skipping every
|
||||
# remaining target (#47163). Wrap the fallback in its own
|
||||
# try/except so a per-target failure is logged and the loop
|
||||
# continues to the next target.
|
||||
try:
|
||||
pool = concurrent.futures.ThreadPoolExecutor(max_workers=1)
|
||||
try:
|
||||
future = pool.submit(asyncio.run, _send_to_platform(platform, pconfig, chat_id, cleaned_delivery_content, thread_id=thread_id, media_files=media_files))
|
||||
result = future.result(timeout=30)
|
||||
finally:
|
||||
pool.shutdown(wait=False)
|
||||
except Exception as e:
|
||||
msg = f"delivery to {platform_name}:{chat_id} failed: {e}"
|
||||
logger.error("Job '%s': %s", job["id"], msg, exc_info=True)
|
||||
target_errors.extend([msg])
|
||||
delivery_errors.extend(target_errors)
|
||||
continue
|
||||
except Exception as e:
|
||||
msg = f"delivery to {platform_name}:{chat_id} failed: {e}"
|
||||
logger.error("Job '%s': %s", job["id"], msg)
|
||||
logger.error("Job '%s': %s", job["id"], msg, exc_info=True)
|
||||
target_errors.extend([msg])
|
||||
delivery_errors.extend(target_errors)
|
||||
continue
|
||||
|
|
|
|||
|
|
@ -45,6 +45,7 @@ ACP_REGISTRY_MANIFEST = REPO_ROOT / "acp_registry" / "agent.json"
|
|||
|
||||
# Auto-extracted from noreply emails + manual overrides
|
||||
AUTHOR_MAP = {
|
||||
"swissly@users.noreply.github.com": "swissly", # PR #47167 salvage (wrap cron delivery thread-pool fallback in its own try/except so a per-target failure can't escape the except-RuntimeError block and crash the multi-target delivery loop; #47163)
|
||||
"30854794+YLChen-007@users.noreply.github.com": "YLChen-007", # PR #27289 salvage (case-insensitive streaming reasoning-tag filter in cli.py _stream_delta + gateway stream_consumer so mixed-case variants like <Think>/<ThInK> are suppressed, not just the hardcoded case literals)
|
||||
"27672904+kangsoo-bit@users.noreply.github.com": "kangsoo-bit", # PR #47508 salvage (keep Telegram gateway alive on transient bootstrap network errors: best-effort deleteWebhook + resilient start_polling degrade to background recovery instead of failing startup)
|
||||
"259353979+testingbuddies24@users.noreply.github.com": "testingbuddies24", # PR #43192 salvage (strip orphan think-tag close tags in progressive gateway stream so a bare </think> whose open was dropped upstream can't leak to the user)
|
||||
|
|
|
|||
|
|
@ -4391,3 +4391,79 @@ class TestCronContinuableSurfaceInChannel:
|
|||
store.get_or_create_session.assert_not_called()
|
||||
mirror_mock.assert_not_called()
|
||||
|
||||
|
||||
class TestMultiTargetDeliveryContinuesOnFailure:
|
||||
"""When delivery to one target fails inside the standalone thread-pool
|
||||
fallback, the loop must continue to the remaining targets (#47163).
|
||||
|
||||
The fallback runs inside the `except RuntimeError` block of
|
||||
`_deliver_result`. Before the fix, an exception raised there (SMTP
|
||||
ConnectionError, future.result timeout) escaped the function entirely —
|
||||
it is NOT caught by the sibling `except Exception` — crashing the loop
|
||||
and silently dropping every subsequent target.
|
||||
"""
|
||||
|
||||
def _email_cfg(self):
|
||||
from gateway.config import Platform
|
||||
|
||||
pconfig = MagicMock()
|
||||
pconfig.enabled = True
|
||||
mock_cfg = MagicMock()
|
||||
mock_cfg.platforms = {Platform.EMAIL: pconfig}
|
||||
return mock_cfg
|
||||
|
||||
def test_first_target_failure_does_not_crash_loop(self):
|
||||
"""First email target fails in the fallback; the second is still attempted."""
|
||||
job = {
|
||||
"id": "multi-email-job",
|
||||
"deliver": "email:a@example.com,email:b@example.com",
|
||||
}
|
||||
|
||||
with patch("gateway.config.load_gateway_config", return_value=self._email_cfg()), \
|
||||
patch("cron.scheduler.load_config", return_value={"cron": {"wrap_response": False}}), \
|
||||
patch("asyncio.run", side_effect=RuntimeError("no running loop")), \
|
||||
patch("concurrent.futures.ThreadPoolExecutor") as mock_pool_cls:
|
||||
mock_pool = MagicMock()
|
||||
mock_pool_cls.return_value = mock_pool
|
||||
|
||||
fail_future = MagicMock()
|
||||
fail_future.result.side_effect = ConnectionError("SMTP connection refused")
|
||||
ok_future = MagicMock()
|
||||
ok_future.result.return_value = {"success": True}
|
||||
mock_pool.submit.side_effect = [fail_future, ok_future]
|
||||
|
||||
result = _deliver_result(job, "Report content")
|
||||
|
||||
# Both targets attempted — the loop did not crash after the first failure.
|
||||
assert mock_pool.submit.call_count == 2, (
|
||||
f"expected 2 delivery attempts, got {mock_pool.submit.call_count}"
|
||||
)
|
||||
# First target's failure is surfaced in the returned error string.
|
||||
assert result is not None
|
||||
assert "a@example.com" in result
|
||||
assert "SMTP connection refused" in result
|
||||
|
||||
def test_all_targets_fail_returns_combined_errors(self):
|
||||
"""When every target fails, the result reports all of them."""
|
||||
job = {
|
||||
"id": "all-fail-job",
|
||||
"deliver": "email:a@example.com,email:b@example.com",
|
||||
}
|
||||
|
||||
with patch("gateway.config.load_gateway_config", return_value=self._email_cfg()), \
|
||||
patch("cron.scheduler.load_config", return_value={"cron": {"wrap_response": False}}), \
|
||||
patch("asyncio.run", side_effect=RuntimeError("no running loop")), \
|
||||
patch("concurrent.futures.ThreadPoolExecutor") as mock_pool_cls:
|
||||
mock_pool = MagicMock()
|
||||
mock_pool_cls.return_value = mock_pool
|
||||
|
||||
fail_future = MagicMock()
|
||||
fail_future.result.side_effect = ConnectionError("connection refused")
|
||||
mock_pool.submit.return_value = fail_future
|
||||
|
||||
result = _deliver_result(job, "Report content")
|
||||
|
||||
assert result is not None
|
||||
assert "a@example.com" in result
|
||||
assert "b@example.com" in result
|
||||
assert mock_pool.submit.call_count == 2
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue