fix: keep WeCom callback receive loop responsive - #9759
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
astrbot-docs | 09f87bd | Commit Preview URL Branch Preview URL |
Aug 21 2026, 04:42 AM |
dosubot
Bot
added
the
size:S
dosubot
Bot
added
the
area:platform
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider cancelling or awaiting any still-running
_message_handler_tasksas part of the client shutdown/cleanup path so that background callbacks don’t leak or keep the event loop alive longer than intended. - The new regression test asserts directly on the private
_message_handler_tasksattribute; relying on internal state here makes the test brittle, so it may be better to assert on externally observable behavior (e.g., that the handler completes) instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider cancelling or awaiting any still-running `_message_handler_tasks` as part of the client shutdown/cleanup path so that background callbacks don’t leak or keep the event loop alive longer than intended.
- The new regression test asserts directly on the private `_message_handler_tasks` attribute; relying on internal state here makes the test brittle, so it may be better to assert on externally observable behavior (e.g., that the handler completes) instead.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/wecom_ai_bot/wecomai_long_connection.py" line_range="162-164" />
<code_context>
+ if task.cancelled():
+ return
+ if exception := task.exception():
+ logger.error(
+ "[WecomAI][LongConn] 处理回调消息失败",
+ exc_info=exception,
+ )
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Logging exception via `exc_info=exception` is unlikely to behave as intended.
`exc_info` should be either `True` (to use the current exception) or a `(type, value, traceback)` tuple, not the exception instance. To log the traceback for this task error, either pass a proper `exc_info` tuple or call `logger.exception("[WecomAI][LongConn] 处理回调消息失败")` immediately in the except block so the current exception is captured correctly.
</issue_to_address>
### Comment 2
<location path="tests/test_wecomai_long_connection.py" line_range="30-37" />
<code_context>
+ 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)
+
+ callback = json.dumps(
</code_context>
<issue_to_address>
**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 `_message_handler_tasks` empty). Please also assert that a background task was created (e.g., `assert len(client._message_handler_tasks) == 1` after the callback is handled and before the ACK), so the test explicitly verifies that the handler runs in a separate task rather than inline.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| logger.error( | ||
| "[WecomAI][LongConn] 处理回调消息失败", | ||
| exc_info=exception, |
There was a problem hiding this comment.
issue (bug_risk): Logging exception via exc_info=exception is unlikely to behave as intended.
exc_info should be either True (to use the current exception) or a (type, value, traceback) tuple, not the exception instance. To log the traceback for this task error, either pass a proper exc_info tuple or call logger.exception("[WecomAI][LongConn] 处理回调消息失败") immediately in the except block so the current exception is captured correctly.
| 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) |
There was a problem hiding this comment.
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 _message_handler_tasks empty). Please also assert that a background task was created (e.g., assert len(client._message_handler_tasks) == 1 after the callback is handled and before the ACK), so the test explicitly verifies that the handler runs in a separate task rather than inline.
|
这类问题我之前用ruff扫出来过 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09f87bde77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| task = asyncio.create_task(self.message_handler(payload)) | ||
| self._message_handler_tasks.add(task) | ||
| task.add_done_callback(self._on_message_handler_done) |
There was a problem hiding this comment.
When the socket disconnects while a callback handler is awaiting send_command, this task survives _run_once: only the heartbeat is cancelled, and shutdown() 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_lock and delaying current callbacks. Cancel and await the handler tasks when their originating connection ends and during shutdown.
Useful? React with 👍 / 👎.
| def _on_message_handler_done(self, task: asyncio.Task[None]) -> None: | ||
| """Release a completed callback task and report its exception.""" |
There was a problem hiding this comment.
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 👍 / 👎.
| task = asyncio.create_task(self.message_handler(payload)) | ||
| self._message_handler_tasks.add(task) | ||
| task.add_done_callback(self._on_message_handler_done) |
There was a problem hiding this comment.
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 _command_lock, because one command can spend roughly 40 seconds retrying while every subsequent callback adds another pending task, allowing a burst or sustained stream to grow memory without bound. Use a bounded callback queue or concurrency limit while keeping the socket reader independent for acknowledgements.
Useful? React with 👍 / 👎.
| "[WecomAI][LongConn] 处理回调消息失败", | ||
| exc_info=exception, |
There was a problem hiding this comment.
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 👍 / 👎.
|
第一是保存了个引用防止被回收 |
Fixes #9758.
The WeCom long-connection receive loop awaited the callback handler inline. When the configured initial response was sent from that handler, it waited for a command acknowledgement that only the blocked receive loop could consume, causing repeated 10-second timeouts.
Modifications / 改动点
Dispatch WeCom callback handlers as tracked background tasks so the WebSocket receive loop remains available for command acknowledgements.
Preserve callback exceptions in logs and release completed task references.
Add a regression test that sends a response command from a callback handler and verifies that its acknowledgement is processed without blocking.
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Prevent WeCom callback processing from blocking command acknowledgements and improve Anthropic HTTP client compatibility.
Bug Fixes:
Enhancements:
Tests: