Skip to content

fix(wecom): treat errcode 846609 as subscription-level failure — force reconnect + retry - #92804

Open
huanglu00-lang wants to merge 2 commits into
NousResearch:mainfrom
huanglu00-lang:fix/wecom-846609-subscription-reconnect
Open

huanglu00-lang wants to merge 2 commits into
NousResearch:mainfrom
huanglu00-lang:fix/wecom-846609-subscription-reconnect

Conversation

@huanglu00-lang

Copy link
Copy Markdown

Summary

Fixes the WeCom outbound message loss described in #29667 and #47564 (both still open; earlier fix PRs #24790 / #31393 / #37257 were not merged).

Problem

When the WeCom server invalidates a bot's subscription while the WebSocket remains locally OPEN, every outbound send fails with errcode 846609 ("aibot websocket not subscribed"). The adapter treated this as a plain send error:

  • the reply was silently dropped (no retry, no reconnect)
  • the stale connection persisted until the server eventually closed the TCP socket, creating a 57–79s dead window where all outbound messages were lost
  • REQUEST_TIMEOUT_SECONDS only wrapped the response wait, not the frame write — a wedged WS write could hang a send coroutine forever (observed in production: a send with no return and no timeout)
  • heartbeat failures were logged at debug level with no consequence, so functionally-dead connections survived indefinitely

Root cause

ws.closed == False does not imply the subscription is still valid. The adapter's readiness check (if not self._ws or self._ws.closed) only covers the transport layer, not the WeCom subscription lifecycle.

Fixes

  1. send(): on 846609 — both the response path and the raised-exception path from _raise_for_wecom_error() — invalidate the connection, let _listen_loop reconnect + resubscribe, then retry once on the fresh connection
  2. _invalidate_connection() (new): fail all pending futures, close the WS with code 1012, wait (bounded) until the socket is usable again
  3. _send_json(): wrap the frame write in SEND_FRAME_TIMEOUT_SECONDS (3s) so wedged writes can no longer hang callers forever
  4. _heartbeat_loop(): count consecutive failures; after 3, force-close the socket (code 1011) to trigger reconnect instead of silently logging

Production verification

Before the fix: ~10-minute disconnect cycles, every reply sent during the disconnect window silently lost (24 × errcode 846609 in a single day, one send hung forever).

After the fix: 2+ hours with zero disconnects, zero 846609 errors, and one forced-reconnect path exercised without message loss.

Test plan

  • Syntax + import verification on the patched adapter
  • Production soak: 2+ hours stable on a live WeCom bot
  • Unit test for the 846609 retry path (happy to add if maintainers want it in this PR)

…e reconnect + retry

Problem
-------
When the WeCom server invalidates a bot's subscription while the WebSocket
remains locally OPEN, every outbound send fails with errcode 846609
('aibot websocket not subscribed'). The adapter treated this as a plain
send error: the reply was silently dropped and no reconnect was triggered.
The stale connection persisted until the server eventually closed the TCP
socket, creating a dead window where all outbound messages were lost.

Additionally, REQUEST_TIMEOUT_SECONDS only wrapped the response wait, not
the frame write itself — a wedged WS write could hang a send coroutine
forever. And heartbeat failures were logged at debug level without any
consequences, so functionally-dead connections survived indefinitely.

Fixes
-----
1. send(): on 846609 (both the response path and the raised-exception
   path from _raise_for_wecom_error), invalidate the connection, let
   _listen_loop reconnect + resubscribe, then retry once on the fresh
   connection.
2. _invalidate_connection(): fail all pending futures, close the WS with
   code 1012, wait (bounded) until the socket is usable again.
3. _send_json(): wrap the frame write in SEND_FRAME_TIMEOUT_SECONDS (3s)
   so wedged writes can no longer hang callers forever.
4. _heartbeat_loop(): count consecutive failures; after 3, force-close
   the socket (code 1011) to trigger reconnect instead of logging at
   debug level.

Verified in production: before the fix, ~10-minute disconnect cycles with
every reply sent during the disconnect window silently lost (24 x errcode
846609 in one day); after the fix, 2+ hours with zero disconnects, zero
846609 errors.

Refs NousResearch#29667, NousResearch#47564
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/wecom WeCom / WeChat Work adapter labels Aug 23, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related: #24790 and #51801 are open fixes for the same 846609 reconnect/retry family. This PR also covers bounded frame writes and subscription invalidation; maintainers should choose or consolidate the competing implementations.

… retry

Covers the four fix surfaces in the 846609 reconnect PR:
- response-path 846609 forces reconnect + retry once
- raised-exception path (reply markdown) forces reconnect + retry once
- retry failure returns the original subscription error
- non-subscription errors (40001) do not trigger reconnect
- frame write is bounded by SEND_FRAME_TIMEOUT_SECONDS (no hung sends)
- heartbeat force-closes wedged WS after 3 consecutive failures (code 1011)
@huanglu00-lang

Copy link
Copy Markdown
Author

Update: regression tests added + why this PR covers more than the competing fixes

Thanks to triage for flagging #24790 / #51801. Since they target the same 846609 reconnect/retry family, here is what this PR adds on top:

1. Bounded frame writes (SEND_FRAME_TIMEOUT_SECONDS = 3.0)
A wedged WS write used to hang the send coroutine forever — observed in production (one send with no return and no timeout). REQUEST_TIMEOUT_SECONDS only wraps the response wait, not the frame write itself. Neither of the other PRs addresses this.

2. Heartbeat-driven dead-connection detection
3 consecutive heartbeat failures force-close the socket (code 1011) so _listen_loop reconnects + resubscribes. This eliminates the 57–79s dead window before the server tears the TCP socket down — the send-path-only recovery leaves that window in place.

3. _invalidate_connection() fails pending futures + bounded wait
All pending response futures are failed so no caller hangs on them, and the code waits (bounded) until the socket is usable again before retrying.

Production verification

  • Before: ~10-minute disconnect cycles, 24 × errcode 846609 in a single day, one send hung forever.
  • After: 2+ hours with zero disconnects, zero 846609, forced-reconnect path exercised without message loss.

Tests (tests/gateway/test_wecom.py, 6 new cases; full file 25 passed)

  • response-path reconnect + retry once
  • raised-exception path (reply markdown) reconnect + retry once
  • retry failure propagates the original subscription error
  • non-subscription errors (40001) do NOT trigger reconnect
  • frame write is bounded by SEND_FRAME_TIMEOUT_SECONDS
  • heartbeat force-closes the wedged WS after 3 consecutive failures

Happy to coordinate with maintainers on consolidating with #51801 — the reconnect/retry core here is the complete fix for the message-loss bug class.

@huanglu00-lang

Copy link
Copy Markdown
Author

Consolidation proposal — how to unblock this PR cluster

Three PRs (#24790#61984, #51801, #92804) have been sitting open since May on the same 846609 bug class. Nobody has merged any of them, and I think the blocker isn't the code — it's that each PR approaches the problem from a different layer, and reviewing all three means figuring out overlap by hand. Let me make that decision cheaper.

What each PR actually adds (no overlap):

Layer #61984 (xchliu) #51801 (Jeven35) #92804 (mine)
Heartbeat failure → force reconnect ✅ (same fix, +code 1011)
send() marks 846609 retryable ✅ (via base retry) ✅ (manual dc+reconnect) ✅ (_invalidate_connection)
Bounded frame write (3s timeout) ✅ only
Fail pending futures on invalidation ✅ only
Regression tests ✅ (20) ✅ (6 new, 25 total)
Unrelated scope ⚠️ image-cache change

Proposed path:

  1. Merge the reconnect/retry core from fix(wecom): treat errcode 846609 as subscription-level failure — force reconnect + retry #92804 (it's the only one covering both the response path and the raised-exception reply path, with _invalidate_connection failing pending futures so nothing hangs).
  2. Take xchliu's heartbeat approach from fix(wecom): heartbeat-triggered reconnect + 846609 retryable #61984 if you prefer its minimal shape over my code 1011 variant — functionally equivalent.
  3. Jeven35's image-cache improvement should be its own PR; it's a different bug and including it here is what makes fix(gateway): recover WeCom sends after lost subscription #51801 look bigger than it is.

I'm happy to rebase #92804 onto current main and drop/adapt anything you'd rather take from the other two — one clean PR beats three stale ones. ping @teknium1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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.

2 participants