Skip to content

fix(weixin): treat 'rate limited' errmsg as stale-session signal in _is_stale_session_ret - #35714

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/weixin-rate-limit-stale-session
Open

fix(weixin): treat 'rate limited' errmsg as stale-session signal in _is_stale_session_ret#35714
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/weixin-rate-limit-stale-session

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a bug where the Weixin iLink adapter's _is_stale_session_ret() helper fails to recognise errmsg="rate limited" as a stale-session signal. When the context_token is stale, iLink returns ret=-2 with errmsg="rate limited" — identical to the "unknown error" variant (#17228) — but the existing check only matched "unknown error", causing all retries to burn against the dead token.

Related Issue

Fixes #35713

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/platforms/weixin.py: Widened _is_stale_session_ret() to treat "rate limited" as a stale-session signal alongside "unknown error". Updated docstring to document all recognised variants. Added .strip() to handle whitespace-padded errmsg values.
  • tests/gateway/test_weixin.py: Added 5 regression tests for the "rate limited" variant (ret=-2, errcode=-2, case-insensitive, whitespace handling). Updated class docstring to reference weixin: ret=-2 errmsg="rate limited" also indicates stale context_token (follow-up to #18100) #35713.

How to Test

  1. Run pytest tests/gateway/test_weixin.py -q -k TestIsStaleSessionRet — all 11 tests should pass (6 existing + 5 new)
  2. Run pytest tests/gateway/test_weixin.py -q — full file should pass (62 tests)
  3. Verify that weixin._is_stale_session_ret(-2, None, "rate limited") returns True
  4. Verify that weixin._is_stale_session_ret(-2, None, "freq limit") still returns False (genuine rate limit)

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter P3 Low — cosmetic, nice to have labels May 31, 2026

@tonydwb tonydwb 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.

Code Review Summary

Verdict: Approved

Overview

Well-documented fix for Weixin iLink stale-session detection. Discovers that errmsg="rate limited" with ret=-2 is a stale-session signal (not a genuine rate limit), and extends _is_stale_session_ret() to recognize it alongside "unknown error".

Looks Good

  • Clear distinction from genuine rate limits ("freq limit" excluded)
  • Comprehensive tests: 5 new regression tests covering all variants
  • Case-insensitive matching with .strip() for whitespace robustness
  • Low blast radius (single helper, 2 call sites)
  • Detailed PR body with related issue references

Reviewed by Hermes Agent

@zujh

zujh commented Jun 18, 2026

Copy link
Copy Markdown

This PR fixes the "rate limited" errmsg variant, but there is a third variant still missing: when iLink returns {"ret": -2} with no errmsg field (None/empty string), _is_stale_session_ret() still returns False, causing the code to enter the rate-limit backoff path instead of clearing the token and retrying.

The three known stale-session variants (ret=-2) are:

  1. errmsg="unknown error" — already handled (weixin: cron-initiated push fails with iLink ret=-2 when context_token is stale (not recognized as session-expired) #17228)
  2. errmsg="rate limited" — this PR
  3. errmsg=None / errmsg=""still broken (weixin: ret=-2 with empty errmsg also indicates stale context_token (follow-up to #17228) #18100 / duplicate of weixin: ret=-2 errmsg="rate limited" also indicates stale context_token (follow-up to #18100) #35713)

A more complete fix that wraps all three:

msg = (errmsg or "").strip().lower()
if not msg:
    return True
return msg in ("unknown error", "rate limited")

Alternatively, defining a named constant makes future extensions cleaner:

STALE_SESSION_ERRMSGS = frozenset({"unknown error", "rate limited"})

…is_stale_session_ret

iLink returns ret=-2 with errmsg='rate limited' when the context_token
is stale — identical recovery pattern to 'unknown error' (NousResearch#17228).
Previously this fell through to the genuine rate-limit backoff path,
burning all retries against the dead token.

Widen the check to include 'rate limited' alongside 'unknown error'.
Genuine rate limits use 'freq limit' (iLink's wording) and are not
affected.

Fixes NousResearch#35713
iLink returns ret=-2 with no errmsg field (None or empty string) when
the context_token is stale. Previously only 'unknown error' and 'rate
limited' variants were recognized, causing the empty-errmsg case to
enter the rate-limit backoff path instead of clearing the token and
retrying.

Ref: NousResearch#18100, NousResearch#35713
Suggested-by: zujh
@liuhao1024
liuhao1024 force-pushed the fix/weixin-rate-limit-stale-session branch from a8280df to 20e6dc1 Compare June 18, 2026 13:19
@liuhao1024

Copy link
Copy Markdown
Contributor Author

@zujh Great catch! I've pushed an update that handles the third variant — ret=-2 with None or empty errmsg is now treated as a stale session signal.

Changes:

  • gateway/platforms/weixin.py: if not msg: return True before the frozenset check
  • tests/gateway/test_weixin.py: Updated test_ret_minus_2_with_no_errmsg_is_stale to assert True for both None and "", plus added errcode variant test

All 73 weixin tests pass. Thanks for the detailed analysis!

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for extending the existing stale-session discriminator. The premise remains current: gateway/platforms/weixin.py:107 recognizes only "unknown error", while outbound delivery consults that helper at gateway/platforms/weixin.py:1775 before falling into the -2 retry/backoff path at gateway/platforms/weixin.py:1789.

The proposed normalization and recognized variants fit the shared helper, so they cover both _poll_loop() (gateway/platforms/weixin.py:1360) and outbound sends. The diff also adds focused regression coverage for the documented rate limited, case, whitespace, empty-message, and errcode cases in tests/gateway/test_weixin.py.

No blocking issues found in static review.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists and removed P3 Low — cosmetic, nice to have labels Jul 13, 2026
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
@Visol-456

Copy link
Copy Markdown

+1.Hope it could be merged. When I wants to run a long-term mission background over wechat, It ALWAYS stucked halfway after 10 messages. It has confused me for a long time.

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

weixin: ret=-2 errmsg="rate limited" also indicates stale context_token (follow-up to #18100)

6 participants