fix(platforms): treat ret=-2 as stale session when using context_token - #62386
fix(platforms): treat ret=-2 as stale session when using context_token#62386liuhao1024 wants to merge 4 commits into
Conversation
When a context_token expires, the iLink API can return ret=-2 with errmsg='rate limited' instead of the expected errcode=-14. The code previously only recognized 'unknown error' as a stale-session signal, causing cron deliveries to fail with repeated rate-limit retries. This change broadens the _is_stale_session_ret() helper to treat any ret=-2/errcode=-2 as a possible stale-session signal when the request used a context_token, regardless of the errmsg string. The original behavior is preserved for requests without a context_token (e.g., getUpdates long-polling). Fixes NousResearch#62383
Duplicate of #35714 — both are your own fix PRs for the same bug (stale Weixin/iLink |
These two PRs address different bugs with different solutions:
The approaches are complementary, not duplicates. This PR's Both should remain open for review. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the verified stale-token delivery failure. Current main still recognizes only errmsg="unknown error" in _is_stale_session_ret() (gateway/platforms/weixin.py:99-107), so the reported "rate limited" response still reaches the retry/circuit-breaker path (gateway/platforms/weixin.py:1772-1813).
Problems
- The new
used_context_tokencondition atgateway/platforms/weixin.py:1793turns every-2response with a token into a stale session. That bypasses_record_rate_limit_event()and clears the stored token even for the adapter's documented genuine frequency-limit code. Existing tests intentionally model a connected adapter withctx-token(tests/gateway/test_weixin.py:372) and expecterrmsg="frequency limit"to open the breaker (tests/gateway/test_weixin.py:424-441); this PR changes those tests to remove the token rather than retain that case.
Suggested changes
- Preserve the genuine rate-limit path for token-bearing sends; use a verified discriminator for stale-token
-2responses rather than treating all of them as stale. - Add a token-present
frequency limitregression test alongside the new stale-session case.
Automated hermes-sweeper review.
| ret == SESSION_EXPIRED_ERRCODE | ||
| or errcode == SESSION_EXPIRED_ERRCODE | ||
| or _is_stale_session_ret(ret, errcode, resp.get("errmsg")) | ||
| or _is_stale_session_ret(ret, errcode, resp.get("errmsg"), used_context_token=(context_token is not None)) |
There was a problem hiding this comment.
This makes every -2 response with a stored context token take the token-clearing retry path, so genuine frequency limit responses never reach _record_rate_limit_event(). Current tests model a connected adapter with ctx-token (tests/gateway/test_weixin.py:372) and expect that response to open the circuit (:424-441); this PR removes the token from those tests. Please preserve the genuine rate-limit path for token-bearing sends.
What does this PR do?
When a
context_tokenexpires, the iLink API can returnret=-2witherrmsg="rate limited"instead of the expectederrcode=-14(session expired). The code previously only recognized"unknown error"as a stale-session signal in_is_stale_session_ret(), causing cron deliveries to fail with repeated rate-limit retries when tokens expire.This change broadens the
_is_stale_session_ret()helper to treat anyret=-2/errcode=-2as a possible stale-session signal when the request used acontext_token, regardless of theerrmsgstring. The original behavior is preserved for requests without acontext_token(e.g.,getUpdateslong-polling), which prevents false positives in polling scenarios.Related Issue
Fixes #62383
Type of Change
Changes Made
_is_stale_session_ret()ingateway/platforms/weixin.pyto accept a keyword-onlyused_context_tokenparameter (defaultFalse)used_context_token=True, anyret=-2/errcode=-2returnsTrue(treated as stale session)_poll_loop()(line 1378) to passused_context_token=False(preserves original behavior)_send_text_chunk()(line 1793) to passused_context_token=(context_token is not None)tests/gateway/test_weixin.py:test_ret_minus_2_with_rate_limited_and_context_token_is_stale: Tests the new behaviortest_ret_minus_2_with_freq_limit_without_context_token_is_not_stale: Ensures backward compatibilitytest_ret_minus_2_without_context_token_preserves_original_behavior: Validates original behavior is preservedtest_ret_minus_2_with_any_errmsg_and_context_token_is_stale: Tests that any errmsg with context_token is treated as staleHow to Test
Run the updated test suite for
TestIsStaleSessionRet:All 11 tests should pass, including the 4 new tests.
Verify backward compatibility with existing behavior:
pytest tests/gateway/test_weixin.py::TestIsStaleSessionRet::test_ret_minus_2_with_freq_limit_is_not_stalepassespytest tests/gateway/test_weixin.py::TestIsStaleSessionRet::test_ret_minus_2_with_unknown_error_is_stalepassesVerify the fix addresses the reported issue:
pytest tests/gateway/test_weixin.py::TestIsStaleSessionRet::test_ret_minus_2_with_rate_limited_and_context_token_is_stalepassesret=-2 errmsg="rate limited"case described in [Bug]: Weixin iLink cron delivery fails with ret=-2 'rate limited' when context_token is stale #62383Checklist
Code
pytest tests/ -qand all tests passDocumentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A