Skip to content

fix(gateway): recover from stale Weixin context tokens - #74572

Open
kabuda123 wants to merge 2 commits into
NousResearch:mainfrom
kabuda123:fix/weixin-stale-context-token
Open

fix(gateway): recover from stale Weixin context tokens#74572
kabuda123 wants to merge 2 commits into
NousResearch:mainfrom
kabuda123:fix/weixin-stale-context-token

Conversation

@kabuda123

@kabuda123 kabuda123 commented Jul 30, 2026

Copy link
Copy Markdown

What does this PR do?

Recovers scheduled Weixin text delivery when iLink reports a stale context_token as ret=-2 with errmsg="prepare failed".

The fix is intentionally limited to the text sendmessage path. It does not change media delivery.

The adapter previously recognized only the unknown error stale-session variant and otherwise treated prepare failed as rate limiting. The initial fix also left edge cases where recursive recovery could reset the retry budget, reuse the original stale token after a tokenless failure, delete a newer token received concurrently, or read a token before the outbound lock was acquired.

This revision:

  • classifies prepare failed only for outbound text context-token recovery;
  • deletes the stored token only if it still matches the token that failed;
  • loads the token after acquiring the outbound text gate;
  • performs one extra tokenless recovery send without consuming the normal transient retry budget;
  • keeps all later retries tokenless; and
  • leaves getUpdates, media delivery, and genuine rate-limit behavior unchanged.

Related Issue

Follow-up to #17228.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • gateway/platforms/weixin.py: add outbound-only stale-token classification, compare-and-delete token invalidation, lock-time token lookup, and explicit bounded retry state for text sends.
  • tests/gateway/test_weixin.py: cover zero-retry recovery, bounded failure after stale recovery, concurrent fresh-token preservation, lock-time lookup, persistence/account isolation, and multi-chunk behavior.

How to Test

  1. Run scripts/run_tests.sh tests/gateway/test_weixin.py -q.
  2. Run python -m ruff check gateway/platforms/weixin.py tests/gateway/test_weixin.py.
  3. Run python scripts/check-windows-footguns.py gateway/platforms/weixin.py tests/gateway/test_weixin.py.
  4. Run git diff --check.

Expected result: all 37 Weixin tests pass, lint passes, no Windows footguns are reported, and the patch has no whitespace errors.

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: native Windows using PowerShell/Git Bash

Documentation & Housekeeping

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

Screenshots / Logs

Follow-up verification:

  • Regression RED check: 32 passed, 5 failed at the five expected safety gaps before the production revision.
  • Weixin suite after the revision: 37 passed, 0 failed.
  • Ruff: passed.
  • Windows footgun check: passed.
  • git diff --check: passed.

A repository-wide suite was attempted earlier in this native Windows checkout: 21,821 tests passed and 462 failed outside the touched Weixin test file, predominantly around native-Windows path/permission assumptions or unavailable optional dependencies. The scoped Weixin suite was green in that run; GitHub CI remains the authoritative full-suite result.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 30, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused recovery hardening. The reported failure remains present on current main: _is_stale_session_ret() only recognizes unknown error at gateway/platforms/weixin.py:100-108, so ret=-2, errmsg="prepare failed" reaches rate-limit handling at gateway/platforms/weixin.py:1794-1828. Also, the existing stale-token continue at gateway/platforms/weixin.py:1800-1810 consumes the sole loop iteration when _send_chunk_retries == 0 (for loop at line 1776), preventing the intended tokenless fallback.

Verified current-main behavior

  • The PR's explicit retry counter preserves the normal transient retry budget while permitting one tokenless recovery attempt.
  • The compare-and-delete store operation avoids removing a newer token received while the outbound request was in flight.
  • The PR is limited to the text send path; current media paths remain unchanged as stated.
  • The PR base is an ancestor of current main, with no target-file changes since its base, so salvage should be mechanical.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026

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

This was generated by AI during triage.

Summary

Eighteen PRs address or reference this Weixin reliability cluster, covering several distinct causes: stale context_token variants (unknown error, empty, rate limited, and prepare failed), tokenless-retry mechanics, genuine rate-limit handling, long-poll recovery, and ret=-3. For the target PR, #74572 specifically fixes outbound text sends returning ret=-2, errmsg="prepare failed" and hardens token deletion and retry accounting; it does not replace the narrower variant work in #35714 or the separate polling and genuine-throttling fixes.

Related pull requests

  • #17287 [closed] fixes — (+15/-1) — incorporated reference: introduced the shared ret=-2, errmsg="unknown error" stale-session classifier and connected it to polling and outbound sends. Although closed, it remains relevant because its commit was cherry-picked with preserved authorship into #17432 as e9b96fd05.
  • #17432 [merged] best fix — (+45/-1) — merged reference implementation: landed #17287's unknown error classifier and seven truth-table tests, establishing the tokenless-recovery baseline. It does not cover empty, rate limited, or prepare failed responses.
  • #18105 partial — (+16/-7) — keep open with a salvage path: its empty/None-errmsg classifier addresses #18100, but the keep_open review requires normalizing the alternate msg field and adding outbound tokenless-retry and genuine-rate-limit regressions so a populated msg="frequency limit" is not misclassified.
  • #20797 [closed] partial — (+27/-8) — duplicate of #18105: it makes the same empty/None-errmsg helper change and equivalent classification tests. It remains relevant only as closed corroborating evidence because its author explicitly closed it in favor of #18105.
  • #22661 partial — (+34/-1709) — keep open only for focused salvage: the Weixin portion broadens stale-session matching and attempts token stripping, but the diff also deletes twelve unrelated workflows, lacks matching tests, and places fallback after a circuit-breaker point that can suppress it. This follows the maintainer-bot keep_open verdict: retain only the reordered, tested Weixin logic on current main.
  • #23529 partial — (+27/-11) — keep open with a broader reconnect salvage path: propagating asyncio.TimeoutError exposes the zombie-poll symptom, but the keep_open review notes that the generic poll handler still neither reconnects nor transitions health state. The fix needs a failure-threshold lifecycle change and replacement of the current contradictory sentinel test.
  • #23534 [closed] related — (+1/-1) — rejected polling approach: it converts every long-poll timeout into ret=-3, while the blocking contributor decision documents such idle timeouts as normal and warns that this would produce false failures and backoff. It remains relevant as a closed design boundary, not as a recovery implementation.
  • #27234 [closed] best fix — (+8/-3) — deferred genuine-rate-limit experiment: replaces fixed waits with a long exponential schedule and raises the retry count, but does not correct stale-token classification. It remains relevant after closure only as an author-deferred backoff alternative overlapping #31132.
  • #31132 best fix — (+59/-10) — keep open with a current-main salvage path: adds retry hints, jittered exponential backoff, and chat cooldown for genuine throttling, but the keep_open review shows the default circuit opens before that wait can run; the diff also removes attachment path filters and adds untested configuration. Reconcile the circuit, retain path filtering, and add deterministic tests.
  • #34216 [closed] partial — (+4/-2) — duplicate of #23529: it converts client timeout into synthetic -999 and also changes empty-errmsg classification, but provides no actual poll-session rebuild. It remains relevant as a closed competing timeout approach, not as a complete fix for #23523.
  • #35066 best fix — (+65/-1) — keep open for the one-line guard salvage: removing and context_token permits one tokenless recovery attempt when no cached token exists. The keep_open review requires replacing the invalid ret=-3 fixture with a recognized ret=-2, errmsg="unknown error" or errcode=-14 case, and field testing says this alone does not solve the platform-level ret=-3 failure.
  • #35283 [closed] best fix — (+2927/-29) — closed mixed superset requiring focused splits: it contains potentially useful timeout/reconnect and typing-cadence work, but its final send code contradicts its empty-errmsg classifier and tests, adds noncompliant behavioral environment knobs, and conflicts with the current circuit. Despite the earlier keep_open review, the author accepted those findings and closed it to split the salvage into focused PRs.
  • #35666 [closed] best fix — (+45/-34) — closed mixed ret=-3 experiment: adds -3 stale-session handling and removes the no-token retry guard, but also changes unrelated launchd and gateway self-stop protections, while field evidence says tokenless retry did not resolve the observed ret=-3 platform behavior. Preserve the ret=-3 evidence for a focused split rather than reviving this diff.
  • #35714 best fix — (+38/-8) — keep open with a focused classifier salvage path: recognizes normalized rate limited, empty/None, and unknown error variants while retaining freq limit as genuine throttling, with focused tests. Its maintainer-bot keep_open review reports no blocking static issue, but it does not include #74572's compare-and-delete, under-lock token load, or retry-budget hardening.
  • #35983 partial — (+20/-5) — close as duplicate of #35714 after preserving its send/poll test idea: both add rate limited to stale-token classification, but this diff changes the shared helper without the requested path-level tests and can route polling into a 600-second session-expiry sleep. Despite the keep_open review on #35983, the diff shows that its stated send-only retry guard does not protect the shared polling call site.
  • #62386 partial — (+45/-5) — close as duplicate of #35714: it reaches tokenless recovery by treating every token-bearing -2 as stale, including documented genuine frequency-limit responses, and alters circuit tests to remove their tokens. Despite the keep_open review on #62386, that concrete overbroad predicate is weaker than #35714's message discriminator; retain only useful context-token path tests.
  • #73908 [closed] fixes — (+63/-13) — closed overlapping empty-errmsg implementation: adds durable token deletion and an end-to-end tokenless retry test, but duplicates the empty/None coverage now present in #35714. Its persistence work remains useful evidence, although #74572 improves it with compare-and-delete so a concurrently refreshed token is preserved.
  • #74572 partial — (+222/-18) — keep open with a salvage path: adds an outbound-only prepare failed classifier, compare-and-delete persistence, token loading under the outbound gate, a recovery attempt outside the normal retry budget, and continued tokenless retries. This matches its high-salvageability keep_open verdict and directly fixes the reported target cause without changing polling, media, or genuine rate-limit behavior.

Duplicates

#17287 was incorporated into #17432. #20797 duplicates #18105, and closed #73908 overlaps their empty/None-errmsg change while contributing persistence tests. #35983 and #62386 target the same ret=-2, errmsg="rate limited" stale-token failure as the earlier #35714, but #35983 leaks the classification into polling and #62386 overgeneralizes every token-bearing -2. #34216 overlaps #23529 on zombie polling; #27234 overlaps #31132 on genuine-rate-limit backoff. #35283 bundles pieces of several groups but was closed for focused rework. #74572 is not a duplicate of #35714: it covers the outbound-only prepare failed variant and stronger recovery mechanics.

Suggested consolidation

Keep #74572 open with a salvage path: retain its outbound-only prepare failed classifier, compare-and-delete token persistence, under-gate token load, and explicit tokenless recovery attempt, and keep its path-level regressions. Keep #35714 as the focused home for the distinct normalized rate limited and empty/None variants, while adding an outbound tokenless-retry regression and confirming that polling behavior remains intentional. Close #35983 as duplicate of #35714 despite its keep_open review because its shared-helper diff exposes polling to the new classification; close #62386 as duplicate of #35714 despite its keep_open review because treating every token-bearing -2 as stale bypasses genuine frequency-limit handling. Leave #18105, #22661, #23529, #31132, and #35066 open only with the concrete review-requested salvage actions above; the remaining closed PRs should stay closed as incorporated references, duplicates, rejected designs, or sources for focused future splits.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I62383(["issue #62383 (open)"])
    P74572["PR #74572 (open)"]
    P74572 -.->|partial| I62383
    class I62383 open
    class P74572 open
    class P74572 target
    click I62383 "https://github.com/NousResearch/hermes-agent/issues/62383"
    click P74572 "https://github.com/NousResearch/hermes-agent/pull/74572"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 18 pull requests and 9 issues in this complex. Each diff was read against this issue; Assessment working set: 277 kB of PR diffs, 67 kB of issue/PR text, 38 kB of discussion (51 comments), 49 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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: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.

4 participants