Skip to content

fix(weixin): retry on session/rate-limit errors in send_weixin_direct - #35065

Open
hanhan-tg wants to merge 1 commit into
NousResearch:mainfrom
hanhan-tg:fix/weixin-retry-only
Open

fix(weixin): retry on session/rate-limit errors in send_weixin_direct#35065
hanhan-tg wants to merge 1 commit into
NousResearch:mainfrom
hanhan-tg:fix/weixin-retry-only

Conversation

@hanhan-tg

Copy link
Copy Markdown

What does this PR do?

When send_weixin_direct() hits rate-limit errors (ret=-2) or session timeout errors (errcode=-14), it now retries once with a 3-second backoff. The stale context_token is cleared from the token store before retrying, allowing a tokenless fallback send.

Previously, these errors caused immediate failure — after a gateway restart, stale context_tokens caused persistent delivery failures until manual intervention.

Background

This is a partial cherry-pick from the now-superseded PR #32604. The MEDIA regex whitelist changes from that PR were consolidated into #34844 and have already landed. The WeChat retry logic was orthogonal to #34844 and is still unaddressed on main.

Changes Made

  • gateway/platforms/weixin.py — wrap the send call in a retry loop (up to 2 attempts) that:
    • Checks for rate-limit (ret=-2, "rate limited") and session error patterns
    • Clears stale context_token from the token store before retry
    • Falls back to tokenless send on retry
    • Waits 3 seconds between attempts
    • Returns an actionable error if both attempts fail

Checklist

Code

  • My commit messages follow Conventional Commits (fix(weixin): ...)
  • My PR contains only changes related to this fix (no unrelated commits)
  • I have tested on my platform: macOS 15.x

Documentation

  • I have updated relevant documentation — N/A (internal implementation change)

When send_weixin_direct() hits rate-limit errors (ret=-2) or session
timeout errors (errcode=-14), it now retries once with a 3-second
backoff after clearing the stale context_token, falling back to a
tokenless send attempt.

Fixes the silent delivery failure after gateway restart where stale
context_tokens caused persistent failures.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels May 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing with #35066 — both fix Weixin tokenless retry paths. #35066 fixes the _send_text_chunk guard (closes #35062). This PR fixes send_weixin_direct retry (cherry-pick from superseded #32604). Different code paths but overlapping concern — both may be needed.

@hanhan-tg

Copy link
Copy Markdown
Author

@alt-glitch -- I looked at #35066 and I think these two PRs are complementary rather than competing. Here's my analysis:

#35066 fixes _send_text_chunk's guard condition — when no cached context_token exists, the tokenless fallback path is blocked by an extraneous and context_token check. That's a narrow fix to one retry condition in one method.

#35065 wraps send_weixin_direct() with a top-level retry loop that:

  1. Detects rate-limit (ret=-2) and stale-session (errcode=-14) errors
  2. Clears the stale context_token from the store before retrying
  3. Adds a 3-second backoff
  4. Returns an actionable error if both attempts fail

These target different layers: #35066 removes a guard inside _send_text_chunk, while #35065 adds a retry wrapper in send_weixin_direct. A cron push to an inactive chat would benefit from #35066 (so the first attempt doesn't silently fail at the chunk level) and from #35065 (so transient session errors get a retry with token refresh at the higher level).

Both could land independently. I've updated the PR description to clarify this scope.

Speaking of which — this is also a first-time contributor PR (hanhan-tg). The 7 CI workflows are waiting on maintainer approval to run. Could you approve them when you get a chance? 🙇

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating the former #32604 retry portion. The stale-context_token session fallback is already on current main: send_weixin_direct() delegates to WeixinAdapter.send() (gateway/platforms/weixin.py:2315, 2358-2362), which reaches _send_text_chunk_locked() and retries errcode=-14 without the token (gateway/platforms/weixin.py:1772-1788; commit e105b7ac).

Problems

  • The new "session" in err.lower() check does not itself recognize the current formatted errcode=-14 failure form (gateway/platforms/weixin.py:1814-1816).
  • The proposed _cache.pop() only removes in-memory state; restored tokens come back from disk after restart unless persistence is updated (gateway/platforms/weixin.py:281-315).
  • Current main's genuine -2 path opens a rate-limit circuit instead of re-probing (gateway/platforms/weixin.py:1802-1804), so an outer retry needs a conscious reconciliation with that policy.

Suggested changes

  • Re-scope any salvage around the current circuit-breaker semantics and add direct-send regression coverage for the chosen behavior.

Automated hermes-sweeper review.

for _retry in range(2):
last_result = await _try_send_with_refresh()
if last_result and last_result.success:
break

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This only recognizes an error string containing session, but the existing adapter formats an unhandled -14 response as iLink sendmessage error: ret=... errcode=-14 errmsg=.... Inspect the structured response or explicitly recognize errcode=-14; otherwise the claimed session-error retry can be skipped.

is_rate = "rate limited" in err or "ret=-2" in err
is_session = "session" in err.lower()
if not is_rate and not is_session:
break

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This removes only the in-memory cache entry. ContextTokenStore.restore() reloads the account token file after restart, and this path does not call persistence, so the stale token described in the PR will return on the next process start.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/sessions Session lifecycle, resume, persistence, history labels Jul 13, 2026
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 sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

3 participants