Conversation
Related: this joins the WeCom errcode 846609 silent-delivery cluster around issue #29667 (also #47564). Existing open fix PRs taking the reconnect-retry approach: #24790 (earliest), #37257, #31393; #44632 prevents duplicate gateway instances. This PR adds file-wrapped image caching on top of the 846609 reconnect-retry, so it is broader in scope than the pure-846609 PRs. Flagging the cluster for a maintainer to pick the canonical fix — not auto-closing any. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the documented WeCom 846609 delivery gap. Current main still returns failures from send() without a reconnect (plugins/platforms/wecom/adapter.py:1415-1423), so the premise is valid; the related #29667/#47564 discussion is relevant.
Problems
- The new retry is only reached from markdown
send()(plugins/platforms/wecom/adapter.py:1408-1465in the PR). Media delivery still runs through_send_media_source()and returns errors from its upload/media operations (plugins/platforms/wecom/adapter.py:1203-1355on current main), without this recovery. _recover_lost_subscription()callsdisconnect()on shared adapter state (plugins/platforms/wecom/adapter.py:1067in the PR). Currentdisconnect()cancels shared listener/heartbeat tasks, fails pending responses, and closes the socket/session (plugins/platforms/wecom/adapter.py:241-267), but the PR has no lock to prevent concurrent send recoveries from interleaving.
Suggested changes
- Use a lock-guarded, once-only recovery helper shared by markdown and media outbound paths; recheck connection health after acquiring the lock.
- Add media and concurrent-recovery tests rather than mocking only independent
disconnect()/connect()calls.
Automated hermes-sweeper review.
| reason, | ||
| ) | ||
| try: | ||
| await self.disconnect() |
There was a problem hiding this comment.
disconnect() cancels shared listener/heartbeat tasks, fails all pending response futures, and closes the shared socket/session. Guard this recovery with an asyncio.Lock and recheck whether another sender already restored the subscription after acquiring it; concurrent 846609 sends can otherwise interleave lifecycle operations.
| return SendResult(success=False, error="chat_id is required") | ||
|
|
||
| try: | ||
| async def _send_once() -> Dict[str, Any]: |
There was a problem hiding this comment.
This recovery path only wraps markdown send(). Native media uses _send_media_source() plus upload and media-send helpers, which remain unmodified and still return 846609 failures without reconnecting. Please route all outbound WeCom paths through the same serialized recovery policy.
|
Verification update against current The current adapter path is
The candidate fix is broader than a single reconnect-and-retry call:
This appears to be the current-path continuation of #29667/#47564. Please reconcile this PR with the current |
Summary
Testing
Note: full pytest was not run locally because pytest was not installed in the local Python environment.