Skip to content

fix(weixin): wrap aiohttp requests in asyncio.create_task - #33445

Closed
AaronWong1999 wants to merge 1 commit into
NousResearch:mainfrom
AaronWong1999:fix/weixin-timeout-context-manager
Closed

fix(weixin): wrap aiohttp requests in asyncio.create_task#33445
AaronWong1999 wants to merge 1 commit into
NousResearch:mainfrom
AaronWong1999:fix/weixin-timeout-context-manager

Conversation

@AaronWong1999

Copy link
Copy Markdown
Contributor

What does this PR do?

When running the Weixin platform gateway, sending messages can occasionally fail with:
RuntimeError: Timeout context manager should be used inside a task

This happens because the aiohttp.ClientSession requests (session.post and session.get) use asyncio.timeout internally, but they were being awaited directly in the call stack outside of a proper asyncio.Task context. This PR wraps the inner HTTP calls within asyncio.create_task() so that the timeout context manager is safely scoped within a task, preventing the crash.

Why is it needed?

This bug prevents messages from being reliably delivered via Weixin, especially if there's a slight network delay triggering the timeout evaluation path. We noticed this in production where cron-delivered Weixin messages would repeatedly fail to send due to this exception.

How was it fixed?

Wrapped the core HTTP logic inside _api_post and _api_get inside an _inner() async function, and returned await asyncio.create_task(_inner()).

@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 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate — this is the same Weixin aiohttp Timeout context manager should be used inside a task issue tracked across #13281, #17347, #18791, and many others. Multiple competing fix PRs already open: #14530, #17911, #18417. The asyncio.create_task() wrapper approach is one of several proposed solutions.

@AaronWong1999

Copy link
Copy Markdown
Contributor Author

Hey @alt-glitch, thanks for connecting all these related issues and PRs — it's helpful to see the full picture.

I took some time today to read through all the competing fixes (#14530, #17911, #18417, and mine) carefully, and I wanted to share my honest take on the landscape in case it helps move this forward. I have no strong attachment to any particular approach, but after comparing them I think #33445 might be the safest candidate for a first merge. Here's my reasoning:

vs #14530 — The asyncio.run() approach creates and tears down an event loop on every call, which the author noted can break cached httpx/AsyncOpenAI clients whose lifetime expects the loop to persist. #33445 avoids that entirely by not touching the loop lifecycle.

vs #17911 — Swapping aiohttp.ClientTimeout out for asyncio.wait_for() means cancelled tasks lose the chance to clean up connections gracefully. ClientTimeout is tightly integrated with aiohttp's connection pool — it does more than just timing, and keeping it in place feels like the right call.

vs #18417 — This one's actually conceptually right: ensuring coroutines run inside a proper Task so asyncio.current_task() isn't None. That's the real fix. The trade-off is where to apply it. #18417 puts it in _run_async(), which is the central sync bridge that every tool execution path goes through, for every platform. That makes it a comprehensive fix, but also a high blast-radius change — if something subtle breaks there, it breaks everywhere. #33445 applies the same idea (create_task wrapping) scoped down to only the two Weixin API functions where the error actually fires. Same guarantee, much smaller surface area.

The two aren't mutually exclusive — #18417 could absolutely land as a follow-up root-cause hardening after it's had more soak time, and I'd support that. But for an immediate fix that's been running in production without issues and won't accidentally regress Slack/Discord/Telegram/etc., I think the surgical approach is the right first step.

Happy to take any feedback or adjust the patch if there's a preferred style. Appreciate you taking the time to look at this.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Weixin fix. The reported timeout-context failure is already addressed on current main by a merged alternative, so this PR is now redundant.

  • gateway/platforms/weixin.py:381-390 and gateway/platforms/weixin.py:405-414 replace aiohttp.ClientTimeout with asyncio.wait_for() in both helpers targeted here, explicitly for the reported Timeout context manager should be used inside a task failure.
  • Commit 566669013f3f9c0b52cb1392250b76d510d99dc7 merged that fix and is included in v2026.6.5.
  • tests/gateway/test_weixin.py:1106-1207 covers the no-aiohttp-timeout contract and timeout behavior.
  • The duplicate discussion and competing approaches noted in the PR comments were considered; main now provides the requested behavioral guarantee.

This is an automated hermes-sweeper review.

@teknium1 teknium1 closed this Jul 13, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label 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 duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants