Skip to content

fix(weixin): replace per-request aiohttp ClientTimeout with asyncio.wait_for in _api_post/_api_get - #32179

Closed
ryan-flow wants to merge 2 commits into
NousResearch:mainfrom
ryan-flow:fix/weixin-api-timeout
Closed

fix(weixin): replace per-request aiohttp ClientTimeout with asyncio.wait_for in _api_post/_api_get#32179
ryan-flow wants to merge 2 commits into
NousResearch:mainfrom
ryan-flow:fix/weixin-api-timeout

Conversation

@ryan-flow

Copy link
Copy Markdown

Fix: "Timeout context manager should be used inside a task" in Weixin send

Problem

When the send_message tool sends files/media to WeChat via _run_async, a worker thread with its own event loop is created. _api_post and _api_get use aiohttp.ClientTimeout per-request timeout, but aiohttp BaseTimeout calls asyncio.current_task(loop=self._loop) — where self._loop is the gateway loop, not the worker loop — so current_task() returns None.

Fix

Replace per-request aiohttp.ClientTimeout with asyncio.wait_for(), consistent with existing _upload_ciphertext and _download_bytes.

Scope

  • Only gateway/platforms/weixin.py
  • 2 functions: _api_post, _api_get
  • No new dependencies

…ait_for in _api_post/_api_get

Fixes "Timeout context manager should be used inside a task" errors
when send_message tool invokes weixin adapter via _run_async on a
worker thread with a different event loop than the gateway's.

asyncio.wait_for() creates its own Task on the current loop, avoiding
aiohttp BaseTimeout's reliance on asyncio.current_task(loop=session._loop).

@hclsys hclsys left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct and brings these two helpers in line with the existing house pattern — I verified the RCA and the consistency claim:

  • The root cause is real: aiohttp.ClientTimeout binds its timer to the session's loop, but send_message runs _api_post/_api_get in a worker thread with a different event loop (_run_async), so aiohttp's asyncio.current_task(loop=self._loop) returns None → "Timeout context manager should be used inside a task".
  • The fix is semantically equivalent: aiohttp.ClientTimeout(total=X) is a whole-request deadline, and wrapping the entire _do_post()/_do_get() coroutine (including await response.text()) in asyncio.wait_for(..., timeout=X) covers connect+send+read the same way. On timeout, wait_for cancels the coroutine and the async with session.post(...) __aexit__ releases the connection, so no leak.
  • The consistency claim checks out: _upload_ciphertext (gateway/platforms/weixin.py:554-568) and _download_bytes (:577) on main already use this exact wait_for + inner-coroutine pattern with the same "avoid aiohttp ClientTimeout" comment. So this is bringing _api_post/_api_get in line with the established fix, not inventing a new approach.

One low-effort suggestion, not a blocker: there's no regression test. The bug is loop-context-specific and awkward to unit-test, and the sibling functions apparently lack one too — but since this failure mode has now recurred across multiple helpers, a small guard (e.g. running _api_post under a worker loop distinct from the session's and asserting it doesn't raise the "Timeout context manager…" error) would stop the next _api_* helper from quietly reintroducing aiohttp.ClientTimeout. Worth considering given it's bitten the file more than once.

Logic LGTM.

Tests that _api_post/_api_get use asyncio.wait_for (not aiohttp ClientTimeout)
and work correctly when invoked from a different event loop via
run_coroutine_threadsafe — the exact scenario that caused the original bug.
@ryan-flow

Copy link
Copy Markdown
Author

Added regression tests in tests/gateway/test_weixin_api_timeout.py per your review suggestion. The test suite covers:

  1. TestApiPostUsesWaitFor — verifies _api_post works, raises on HTTP errors, and does NOT pass a per-request ClientTimeout to session.post()
  2. TestApiGetUsesWaitFor — same for _api_get
  3. TestCrossEventLoopTimeout — the core regression test: creates an aiohttp session on one event loop, then invokes _api_post/_api_get from a different event loop via a worker thread, verifying no "Timeout context manager should be used inside a task" error
  4. TestSessionLevelTimeoutDisabled — confirms _no_aiohttp_timeout disables all timeout categories at the session level

All tests use mocked aiohttp responses (no real HTTP calls). Ready for re-review.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter duplicate This issue or pull request already exists labels May 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #31853 — same fix (replace aiohttp.ClientTimeout with asyncio.wait_for() in _api_post/_api_get). #31853 was triaged as the legitimate remaining fix after #21196 shipped.

@ryan-flow

Copy link
Copy Markdown
Author

Understood, closing in favor of #31853.

Quick note: #31853 doesn't include regression tests. If the maintainers are open to it, I'd like to contribute the test suite (tests/gateway/test_weixin_api_timeout.py) as a follow-up to #31853 — the cross-event-loop regression tests cover exactly this failure mode and could prevent the same bug from being reintroduced in future _* helpers.

Alternatively, happy to open a separate test-only PR after #31853 is merged. Let me know what works best.

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 duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants