fix(discord): slash registration and /skill refresh scan the catalog off the event loop
connect() called _register_slash_commands inline on every (re)connect, and /reload-skills called refresh_skill_group inline; both run discord_skill_commands_by_category, the same per-skill path-resolution walk the Telegram menu paid in #110707. On a 1.5k-skill install that holds the loop past the liveness watchdog. Registration now hops through asyncio.to_thread from connect(); refresh_skill_group is a coroutine that hops the rescan the same way (the reload handler already awaits an awaitable result). Contextvar-scoped profile overrides propagate through to_thread. One invariant test: the loop keeps ticking while the scan blocks, from both sites. Red on the PR head, green here. Review finding: Discord _register_slash_commands/refresh_skill_group ran the skill catalog disk scan synchronously on the event loop.
This commit is contained in:
@@ -1346,7 +1346,8 @@ class DiscordAdapter(DiscordMediaMixin, BasePlatformAdapter):
|
||||
guild_id,
|
||||
)
|
||||
if self._slash_commands:
|
||||
self._register_slash_commands()
|
||||
# Registration walks the skill catalog on disk (#110707); keep the loop free.
|
||||
await asyncio.to_thread(self._register_slash_commands)
|
||||
self._disconnecting = False
|
||||
self._bot_task = asyncio.create_task(self._client.start(self.config.token))
|
||||
self._bot_task.add_done_callback(self._handle_bot_task_done)
|
||||
@@ -4486,11 +4487,11 @@ class DiscordAdapter(DiscordMediaMixin, BasePlatformAdapter):
|
||||
self._skill_lookup = {n: (d, k) for n, d, k in entries}
|
||||
self._skill_group_hidden_count = hidden
|
||||
|
||||
def refresh_skill_group(self) -> tuple[int, int]:
|
||||
async def refresh_skill_group(self) -> tuple[int, int]:
|
||||
"""Rescan skills and refresh live ``/skill`` autocomplete; returns ``(new_count, hidden_count)``.
|
||||
Called after ``reload_skills``; no ``tree.sync()`` since autocomplete options are dynamic."""
|
||||
try:
|
||||
self._refresh_skill_catalog_state()
|
||||
await asyncio.to_thread(self._refresh_skill_catalog_state)
|
||||
except Exception as exc:
|
||||
logger.warning(
|
||||
"[%s] Failed to refresh /skill autocomplete after reload: %s", self.name, exc,
|
||||
|
||||
@@ -752,3 +752,38 @@ class TestPrivilegedIntentsRequiredFatal:
|
||||
assert "discord.com/developers/applications" in (adapter.fatal_error_message or "")
|
||||
assert adapter._bot_task is None
|
||||
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_catalog_scan_runs_off_the_event_loop(monkeypatch):
|
||||
"""A slow skill scan (#110707) must not block the gateway loop from either Discord site:
|
||||
slash registration inside connect() (every reconnect) and /reload-skills' refresh_skill_group."""
|
||||
import threading
|
||||
|
||||
adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token"))
|
||||
monkeypatch.setattr("gateway.status.acquire_scoped_lock", lambda scope, identity, metadata=None: (True, None))
|
||||
monkeypatch.setattr("gateway.status.release_scoped_lock", lambda scope, identity: None)
|
||||
monkeypatch.setattr(discord_platform.Intents, "default", lambda: SimpleNamespace(
|
||||
message_content=False, dm_messages=False, guild_messages=False, members=False, voice_states=False))
|
||||
monkeypatch.setattr(discord_platform.commands, "Bot", lambda **kw: FakeBot(intents=kw["intents"]))
|
||||
monkeypatch.setattr(adapter, "_resolve_allowed_usernames", AsyncMock())
|
||||
|
||||
async def _scan_leaves_loop_free(run_site):
|
||||
scan_started, loop_ticked = threading.Event(), threading.Event()
|
||||
|
||||
def _blocking_scan(*, reserved_names):
|
||||
scan_started.set()
|
||||
# Only a free loop can set loop_ticked while this wait is in progress.
|
||||
return ({}, [("x", "desc", "/x")], 0) if loop_ticked.wait(timeout=1) else ({}, [], 0)
|
||||
|
||||
monkeypatch.setattr("hermes_cli.commands_platforms.discord_skill_commands_by_category", _blocking_scan)
|
||||
task = asyncio.create_task(run_site())
|
||||
await asyncio.to_thread(scan_started.wait, 1)
|
||||
loop_ticked.set()
|
||||
await task
|
||||
return [n for n, _d, _k in adapter._skill_entries] == ["x"]
|
||||
|
||||
assert await _scan_leaves_loop_free(adapter.connect)
|
||||
adapter._skill_entries = []
|
||||
assert await _scan_leaves_loop_free(adapter.refresh_skill_group)
|
||||
await adapter.disconnect()
|
||||
|
||||
@@ -22,6 +22,7 @@ data the live callbacks already read from.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
|
||||
@@ -73,7 +74,7 @@ class TestRefreshSkillGroup:
|
||||
fake_collector,
|
||||
)
|
||||
|
||||
new_count, hidden = adapter.refresh_skill_group()
|
||||
new_count, hidden = asyncio.run(adapter.refresh_skill_group())
|
||||
|
||||
assert new_count == 1
|
||||
assert hidden == 0
|
||||
|
||||
Reference in New Issue
Block a user