Skip to content

fix(weixin): track fire-and-forget tasks and offload blocking file read - #26049

Open
frank20190926 wants to merge 1 commit into
NousResearch:mainfrom
frank20190926:fix/weixin-asyncio-task-leak-and-blocking-io
Open

fix(weixin): track fire-and-forget tasks and offload blocking file read#26049
frank20190926 wants to merge 1 commit into
NousResearch:mainfrom
frank20190926:fix/weixin-asyncio-task-leak-and-blocking-io

Conversation

@frank20190926

Copy link
Copy Markdown

Bug Description

Two async correctness bugs in the Weixin adapter that could cause silent delivery failures and event loop blocking.

Root Cause

  1. Orphan tasks on disconnect_process_message_safe and _maybe_fetch_typing_ticket were spawned via bare asyncio.create_task() with no tracking. On disconnect, only the poll loop was cancelled; child tasks kept running against closing sessions.

  2. Blocking I/O in async path_send_file called Path(path).read_bytes() synchronously, blocking the event loop while reading large media files into memory.

Fix

  1. Introduced _child_tasks tracking set + _track_task() helper. disconnect() now cancels all tracked children before closing sessions. Done callbacks auto-remove completed tasks.

  2. Replaced sync Path(path).read_bytes() with await asyncio.to_thread(Path(path).read_bytes) to offload blocking file I/O to a thread pool.

Test Plan

  • 53/53 Weixin tests pass (1 pre-existing failure unrelated)
  • 9/9 cron scheduler tests pass (weixin delivery paths)
  • Existing test coverage validates disconnect cleanup and media send paths

Risk Assessment

Low — both changes are localized to the Weixin adapter.

Two async correctness issues in the Weixin adapter:

1. **Orphan tasks on disconnect** — _process_message_safe and
   _maybe_fetch_typing_ticket were spawned via bare
   asyncio.create_task() with no tracking.  On disconnect only the
   poll loop was cancelled; child tasks continued running against
   closing sessions.

   Fix: introduce _child_tasks set + _track_task() helper.
   disconnect() now cancels all tracked children before closing
   sessions.  Done callback auto-removes completed tasks from the set.

2. **Blocking I/O in the async path** — _send_file called
   Path(path).read_bytes() synchronously, blocking the event loop
   for large media files.

   Fix: use await asyncio.to_thread(Path(path).read_bytes) to
   offload the synchronous read to a thread pool.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter P2 Medium — degraded but workaround exists labels May 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Supersedes #11998 (same Weixin fire-and-forget task tracking fix). This PR is more complete — also adds disconnect cleanup of tracked tasks and offloads blocking file I/O in _send_file.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing two concrete async hazards in the Weixin adapter. The premise is still present on current main: bare child tasks are created at gateway/platforms/weixin.py:1387 and gateway/platforms/weixin.py:1438, while _send_file synchronously reads the file at gateway/platforms/weixin.py:2112.

Problems

  • The PR changes no tests. Existing Weixin media coverage begins at tests/gateway/test_weixin.py:553, but current tests do not cover _track_task, child-task cleanup in disconnect(), or the offloaded read path.

Suggested changes

  • Add a regression test showing pending tracked children are cancelled and awaited before disconnect() closes sessions.
  • Add a focused test proving _send_file invokes asyncio.to_thread for Path(path).read_bytes.

The implementation is localized and matches the adapter's existing tracked poll/text-batch lifecycle. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants