fix(weixin): treat empty rate-limit message as stale session - #18105
liuhao1024 wants to merge 1 commit into
Conversation
|
Arriving here from #20797 (duplicate, now closed). Two things I'd like to contribute so they don't get lost: 1. Real-world repro log for the PR description / commit trailer Captured on prod before the fix — shows the exact symptom this patch cures (adapter burning retries against a dead context_token because After a user-initiated inbound message refreshed the session, the next send went through immediately — confirming stale-session, not a genuine frequency cap. 2. Tiny test-symmetry nit
def test_errcode_minus_2_with_no_errmsg_is_stale(self):
assert weixin._is_stale_session_ret(None, -2, None) is True
assert weixin._is_stale_session_ret(None, -2, "") is TrueLGTM otherwise — happy to see this land. |
|
@XIYBHK Thanks for the detailed repro log — great to have real-world confirmation that On the test symmetry nit: the current diff actually does assert both def test_errcode_minus_2_with_no_errmsg_is_stale(self):
assert weixin._is_stale_session_ret(None, -2, None) is True
assert weixin._is_stale_session_ret(None, -2, "") is TrueSo the two test methods are already symmetric. Appreciate the careful review though! |
…tream NousResearch#18105) iLink returns ret=-2 with errmsg=None or '' for stale context_token, not just 'unknown error'. The current check missed this variant and burned all retries against the dead token. Fix: treat empty/None errmsg as stale-session signal (same as 'unknown error'). Genuine rate limits carry a populated errmsg ('frequency limit' / 'too frequently' / similar). Ported from NousResearch#18105.
|
Hi @teknium1 — hope you're well! Would you be able to take a look at this one when you get a chance? It's a minimal fix (16 lines changed, 2 files) with full regression coverage. We hit this issue again in production today — the No rush at all, just wanted to make sure it wasn't lost in the queue. Happy to rebase or adjust anything if needed. Thanks so much! 🙏 |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused regression fix. The current helper still returns False for empty or missing messages (gateway/platforms/weixin.py:99-107), and the outbound path therefore reaches rate-limit backoff instead of the tokenless retry (gateway/platforms/weixin.py:1771-1813).
Problems
- The new classifier receives only
errmsg, but the existing rate-limit path treatsmsgas an alternate populated message (gateway/platforms/weixin.py:1795). A response witherrmsg=Noneandmsg="frequency limit"would become stale-session handling rather than remain on the rate-limit path.
Suggested changes
- Normalize
errmsg/msgbefore both_is_stale_session_retcalls (gateway/platforms/weixin.py:1360,gateway/platforms/weixin.py:1775), then add an outbound regression test for tokenless retry and an alternate-msgrate-limit case.
Automated hermes-sweeper review.
| if ret != RATE_LIMIT_ERRCODE and errcode != RATE_LIMIT_ERRCODE: | ||
| return False | ||
| return (errmsg or "").lower() == "unknown error" | ||
| message = (errmsg or "").strip().lower() |
There was a problem hiding this comment.
Please ensure callers pass the same errmsg/msg fallback used by the existing rate-limit path. Otherwise errmsg=None, msg="frequency limit" is classified as stale despite a populated rate-limit message.
What does this PR do?
ret=-2/errcode=-2with an empty or missingerrmsgas a stale Weixin session signal.errmsgon bothretanderrcodevariants.Related Issue
N/A
Type of Change
Changes Made
How to Test
pytest tests/ -q— all tests should passChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture and workflows — or N/A