Skip to content

fix(weixin): make rate-limited sends observable and recoverable (#94146) - #94423

Open
Finn763 wants to merge 1 commit into
NousResearch:mainfrom
Finn763:fix/issue-94146-weixin-silent-drop
Open

Finn763 wants to merge 1 commit into
NousResearch:mainfrom
Finn763:fix/issue-94146-weixin-silent-drop

Conversation

@Finn763

@Finn763 Finn763 commented Aug 25, 2026 •

Copy link
Copy Markdown

Summary

This PR hardens the Weixin adapter's outbound path against client-side false success: five defects in gateway/platforms/weixin.py that let refused, throttled, or empty sends be reported as delivered (or swallowed silently) are fixed and covered by regression tests.

It does not claim to fix the reported outage itself. The #94146 report describes replies that were dropped server-side with no error code at all (no send failed, rate limited, session expired, -14, or -2 log lines). That suppression is not reproduced or root-caused here — see "What this PR does not fix".

What this PR fixes (client-side defects, all confirmed by code inspection; red→green tests)

  1. Media sends ignored iLink responses. send_document / send_video / send_voice (and the caption send) returned SendResult(success=True, ...) even for ret=-2 / -14 responses — a literal false success with no log line (the issue's "no -14 or -2 log").
  2. Typing signals were throttled invisibly. _send_typing() never inspected its response, and throttled typing bursts (a suspected suppression trigger in [Bug]: Weixin live replies are silently dropped after rate-limit incidents, even after fresh QR login and on latest main #94146) left no trace — now they surface at WARNING instead of a debug-level swallow.
  3. Empty replies reported success. A reply that formatted/split to nothing returned SendResult(success=True, message_id=None); send_weixin_direct skipped the send and still returned success.
  4. Non-dict / absent responses fell through as success. Any non-dict sendmessage response was treated as delivered; now an empty response is a WARNING + classified failure and non-dict responses raise.
  5. Rate-limited results were not classified. send() returned SendResult(success=False, error=...) with no retryable / retry_after / error_kind, so BasePlatformAdapter._send_with_retry() matched it against connection-error patterns, treated a throttle as a formatting failure, and re-sent a mangled "(Response formatting failed, plain text:)... " duplicate instead of honoring the cooldown. Now classify_send_error() marks rate_limited with retryable=True + retry_after (remaining cooldown, 5 s fallback).

Plus: every sendmessage response (text, caption, media) is now logged sanitized at INFO/WARNING (ret, errcode, errmsg truncated, client_id prefix, context_token used), so an accepted-but-dropped send is at least distinguishable in production logs.

What this PR does not fix (the reported case)

The #94146 repro shows a reply that produced no refusal code at all: the gateway logged the send as completed and no -2 / -14 / rate-limit / session-expired line appeared, while the message never arrived. The client-side defects above do not cover that: a ret=0/errcode=0 (or absent) response is still treated as accepted and the send still reports success. If iLink suppresses delivery without an error code, this PR cannot detect it. The server-side suppression is not reproduced (needs-repro) — the new per-send log lines are the instrumentation to capture it, and an evidence request has been left on #94146. The protocol-level gaps from #24989 also remain open.

Hypothesized recovery — unverified, not a fix

The circuit-open path in this PR (invalidate the peer's cached context_token so the post-cooldown retry starts from a tokenless send) is a hypothesis that a stale/poisoned context token suppresses server-side delivery. The report directly weakens it: the reporter states that a fresh QR login that produced a new bot account/token did not recover outbound delivery (#94146). A fresh login cannot carry a stale cached token, so a token-invalidation recovery cannot explain that case. This path is therefore labeled unverified: it may mitigate future incidents where a genuinely stale token matters, but it is not claimed to fix #94146.

Rate-limit retry: double-send uncertainty

ret=-2 semantics are ambiguous. In-repo, RATE_LIMIT_ERRCODE is documented as "iLink frequency limit — backoff and retry", while _is_stale_session_ret() treats ret=-2 with errmsg == "unknown error" as a stale-session signal (same as -14). If -2 instead means "message accepted but delivery throttled", a post-cooldown retry double-sends. The handling here is conservative but cannot rule that out:

  • the in-flight chunk loop retries at most WEIXIN_SEND_CHUNK_RETRIES (default 4) times with 3x backoff, then stops;
  • the rate-limit circuit breaker (_record_rate_limit_event()) aborts the in-flight send and surfaces a classified failure instead of hammering the endpoint;
  • _send_with_retry() re-invokes the whole send at most 2 further times, honoring retry_after when present.

The double-send possibility is documented rather than asserted away; distinguishing "refused-and-retryable" from "accepted-but-throttled" needs a wire-level answer from iLink (see the evidence request on #94146).

Tests

8 new regression tests in tests/gateway/test_weixin.py (TestWeixinRateLimitRecovery) using the fake-transport/stub-session pattern: circuit-open send returns rate_limited kind + retryable + retry_after; per-chunk loop exhaustion keeps rate_limited semantics; circuit open invalidates the cached context token; successful send logs a sanitized response line (accepted-but-dropped distinguishable); non-dict response is not a silent success; blank reply is a visible failure with no _send_message call; send_document surfaces a -2 media response (was success=True); typing rate limit logs at WARNING. All 8 were red pre-fix and green post-fix.

Test results

  • tests/gateway/test_weixin.py + test_weixin_secret_scope.py + test_weixin_typing.py + test_poller_fd_lifecycle.py: 57 passed; 2 failures are pre-existing on main in this environment (test_qr_login_timeout_uses_monotonic_clock, test_recycle_closes_old_and_installs_fresh_session — both fail identically with weixin.aiohttp unpatched/None in the test process, unrelated to this change).
  • test_send_error_classification.py, test_send_retry.py, test_retry_response.py, test_completion_delivery.py: pass.
  • tests/tools/test_send_message_tool.py: same result as main in this environment (12 pre-existing Discord-forum failures, 94 passed).

Status

Related: #94146 (stays open pending the evidence; the server-side root cause is not resolved by this PR).

…Research#94146)

After a rate-limit episode, Weixin live replies were silently lost: the
adapter reported success while nothing reached the client. Multiple
false-success/silent-drop defects in the send path:

- Rate-limited send results carried no retryable/retry_after/error_kind,
  so the gateway retry layer misclassified them as formatting failures and
  re-sent a mangled '(plain text:)' duplicate instead of honoring cooldown.
- iLink sendmessage responses in the media path (caption + item sends)
  were never inspected: ret=-2/-14 came back and send_document/send_video/
  send_voice still returned success=True with no error logged.
- send_typing responses were never inspected either; throttled typing
  signals (a suspected trigger of server-side suppression) were swallowed
  at debug level - completely invisible.
- Non-dict responses and empty/whitespace replies fell through as
  SendResult(success=True, message_id=None).
- Successful sends logged nothing, so an accepted-but-dropped send was
  indistinguishable from a healthy one.

Fixes:
- Classify send failures via classify_send_error(); rate_limited results
  carry retryable=True and retry_after (cooldown remaining) so
  _send_with_retry retries after the cooldown instead of mangling.
- Inspect + surface all sendmessage responses (text, caption, media) and
  typing responses; log every sendmessage response sanitized (ret,
  errcode, errmsg truncated, client_id prefix, context_token used).
- Invalidate the cached context_token when the rate-limit circuit opens so
  the post-cooldown retry falls back to the tokenless degraded path.
- Return an explicit failure for no-deliverable-content sends instead of
  success=True with message_id=None (live and direct paths).

Regression tests (8, fake transport) were red pre-fix and green post-fix.

Closes NousResearch#94146
@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 sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 25, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

This addresses #94146's real defects precisely: classified/retryable SendResults instead of bare failures, inspected responses on every path that previously faked success (media captions, non-dict payloads, blank replies), sanitized per-send logging, and token invalidation on circuit-open — all pinned by solid tests. Concerns:

  1. _send_typing now raises for any non-zero ret (gateway/platforms/weixin.py:586-593). send_typing/stop_typing catch it, but please verify every other _send_typing( call site does too — a typing-signal failure propagating into the reply flow would convert an observability improvement into dropped messages. If any uncaught caller exists, either keep the raise local to wrapped paths or return the error instead.

  2. Classifier coupling via prose — _sendmessage_response_error deliberately words its message so classify_send_error pattern-matches "rate limited" back out (weixin.py:143-148, acknowledged in-comment). That's fragile: someone editing either string silently breaks cooldown semantics. Prefer structure over wording — a dedicated ILinkRateLimitError(RuntimeError) subclass (or passing the matched code alongside) makes the mapping type-based and unbreakable by copy edits.

  3. Empty response still counts as accepted (weixin.py:1951-1958) — warning-logged, but it remains a false-success channel of exactly the class [Bug]: Weixin live replies are silently dropped after rate-limit incidents, even after fresh QR login and on latest main #94146 describes, and an "accepted-but-dropped" suppression may manifest as empty acks. Consider feeding consecutive-empty-response occurrences into _record_rate_limit_event() so the existing circuit/cooldown machinery reacts to that signature too.

  4. Reaching into ContextTokenStore privates — _invalidate_context_token uses _token_store._key(...), .pop(key) and _persist(...) (weixin.py:1862-1866). Add a public invalidate(account_id, chat_id) on the store; the current version also skips persistence errors with a bare except Exception: pass while claiming "persist removal" in the docstring — at least debug-log it.

Minor: ret not in {0,} reads oddly next to plain equality (weixin.py:139) — use ret != 0; and the media payload diff introduces a stray space in **( {"context_token"...}) (weixin.py:2449). Test suite here is exemplary — especially pinning both circuit-open and loop-exhaustion retryable semantics.

This branch has not been deployed

No deployments
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:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Weixin live replies are silently dropped after rate-limit incidents, even after fresh QR login and on latest main

3 participants