-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix: keep WeCom callback receive loop responsive #9759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ def __init__( | |
| self._send_lock = asyncio.Lock() | ||
| self._command_lock = asyncio.Lock() | ||
| self._response_waiters: dict[str, asyncio.Future[dict[str, Any]]] = {} | ||
| self._message_handler_tasks: set[asyncio.Task[None]] = set() | ||
|
|
||
| @staticmethod | ||
| def gen_req_id() -> str: | ||
|
|
@@ -138,7 +139,11 @@ async def _handle_text_message(self, text: str) -> None: | |
|
|
||
| cmd = payload.get("cmd") | ||
| if cmd in {"aibot_msg_callback", "aibot_event_callback"}: | ||
| await self.message_handler(payload) | ||
| # Keep the receive loop available for command acknowledgements sent by | ||
| # the callback handler, such as the configured initial response. | ||
| task = asyncio.create_task(self.message_handler(payload)) | ||
| self._message_handler_tasks.add(task) | ||
| task.add_done_callback(self._on_message_handler_done) | ||
|
Comment on lines
+144
to
+146
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When callbacks arrive faster than handlers finish, this creates and strongly retains one task per incoming frame without any queue or concurrency limit. This is especially problematic when initial responses serialize behind Useful? React with 👍 / 👎. |
||
| return | ||
|
|
||
| if payload.get("errcode") not in (None, 0): | ||
|
|
@@ -148,6 +153,17 @@ async def _handle_text_message(self, text: str) -> None: | |
| payload.get("errmsg"), | ||
| ) | ||
|
|
||
| def _on_message_handler_done(self, task: asyncio.Task[None]) -> None: | ||
| """Release a completed callback task and report its exception.""" | ||
|
Comment on lines
+156
to
+157
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This class-level helper is used by only one callback registration and performs a small discard/cancellation/exception check, so it meets neither the three-location reuse threshold nor the extreme-complexity exception required for extracting helpers. Keep this completion handling local to the scheduling block rather than adding a separate method. AGENTS.md reference: AGENTS.md:L67-L73 Useful? React with 👍 / 👎. |
||
| self._message_handler_tasks.discard(task) | ||
| if task.cancelled(): | ||
| return | ||
| if exception := task.exception(): | ||
| logger.error( | ||
| "[WecomAI][LongConn] 处理回调消息失败", | ||
| exc_info=exception, | ||
|
Comment on lines
+162
to
+164
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): Logging exception via
Comment on lines
+163
to
+164
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The newly added callback failure message is written in Chinese, so this change violates the repository requirement that all logs use English. Translate the message so operational output remains consistent with the documented convention. AGENTS.md reference: AGENTS.md:L46-L50 Useful? React with 👍 / 👎. |
||
| ) | ||
|
|
||
| async def send_command( | ||
| self, | ||
| cmd: str, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| """Tests for the WeCom AI Bot long-connection client.""" | ||
|
|
||
| import asyncio | ||
| import json | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| import pytest | ||
|
|
||
| from astrbot.core.platform.sources.wecom_ai_bot.wecomai_long_connection import ( | ||
| WecomAIBotLongConnectionClient, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_callback_handler_can_receive_command_ack() -> None: | ||
| """A callback response must not block the socket's ACK receive path.""" | ||
| handler_started = asyncio.Event() | ||
| handler_finished = asyncio.Event() | ||
|
|
||
| async def message_handler(_: dict) -> None: | ||
| handler_started.set() | ||
| sent = await client.send_command( | ||
| "aibot_respond_msg", | ||
| "response-request", | ||
| {"msgtype": "stream"}, | ||
| ) | ||
| assert sent is True | ||
| handler_finished.set() | ||
|
|
||
| client = WecomAIBotLongConnectionClient( | ||
| bot_id="bot-id", | ||
| secret="secret", | ||
| ws_url="wss://example.com", | ||
| heartbeat_interval=30, | ||
| message_handler=message_handler, | ||
| ) | ||
| client._ws = AsyncMock(closed=False) | ||
|
Comment on lines
+30
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Assert that a background message handler task is created before processing the acknowledgement to make the regression intent clearer. Currently the test only checks the final state (ACK processed and |
||
|
|
||
| callback = json.dumps( | ||
| { | ||
| "cmd": "aibot_msg_callback", | ||
| "headers": {"req_id": "callback-request"}, | ||
| "body": {}, | ||
| } | ||
| ) | ||
| await asyncio.wait_for(client._handle_text_message(callback), timeout=0.1) | ||
| await asyncio.wait_for(handler_started.wait(), timeout=0.1) | ||
|
|
||
| acknowledgement = json.dumps( | ||
| {"headers": {"req_id": "response-request"}, "errcode": 0} | ||
| ) | ||
| await client._handle_text_message(acknowledgement) | ||
|
|
||
| await asyncio.wait_for(handler_finished.wait(), timeout=0.1) | ||
| await asyncio.sleep(0) | ||
| assert not client._message_handler_tasks | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the socket disconnects while a callback handler is awaiting
send_command, this task survives_run_once: only the heartbeat is cancelled, andshutdown()does not cancel these tasks either. After its 10-second wait expires, the stale handler can retry using the newly assigned_ws, sending an old callback response over the replacement connection while retaining_command_lockand delaying current callbacks. Cancel and await the handler tasks when their originating connection ends and during shutdown.Useful? React with 👍 / 👎.