Skip to content

fix(weixin): per-chat-id rate-limit isolation - #69932

Open
soddy022 wants to merge 1 commit into
NousResearch:mainfrom
soddy022:upgrade-v4-merge
Open

soddy022 wants to merge 1 commit into
NousResearch:mainfrom
soddy022:upgrade-v4-merge

Conversation

@soddy022

Copy link
Copy Markdown

Summary

Changes the Weixin adapter's rate-limit circuit breaker from global (one breaker for all chats) to per-chat-id isolation.

Problem

Currently _rate_limit_circuit_until and _rate_limit_events are single scalars shared across all chat IDs. When chat A triggers the breaker (e.g., a burst of cron push messages to one user), all other chats are blocked — even if they have no rate-limit history.

In multi-profile/multi-user environments, this means one user's cron jobs can silently block another user's message delivery.

Fix

  • _rate_limit_circuit_until: floatDict[str, float] backed by defaultdict(float)
  • _rate_limit_events: List[float]Dict[str, List[float]] backed by defaultdict(list)
  • All rate-limit methods (_rate_limit_cooldown_remaining, _rate_limit_error, _open_rate_limit_circuit, _record_rate_limit_event, _reset_rate_limit_circuit) now accept a chat_id parameter
  • Call sites in _send_text_chunk_locked pass chat_id through

The threshold logic is unchanged — each chat still independently tracks events in a rolling window and opens its own circuit when the threshold is reached.

Testing

  • Gateway restarts cleanly with the change
  • Standalone adapter path (send_weixin_direct) also uses the per-chat-id defaults correctly
  • Backward compatible: methods default chat_id="" so any external callers without a chat_id still work

Related

This is a follow-up to the multiplex isolation work — profile-level isolation prevents credential cross-talk, but rate limiting also needs to be scoped to the chat/channel level.

Each chat_id now tracks its own rate-limit events and cooldown,
so bursts to one chat do not trigger the global breaker for others.

- _rate_limit_circuit_until: float -> Dict[str, float] (defaultdict)
- _rate_limit_events: List[float] -> Dict[str, List[float]] (defaultdict)
- All rate-limit methods now accept chat_id parameter
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter area/profiles Multi-profile isolation, HERMES_HOME scoping sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: this active reimplementation overlaps closed #65319/#65364 and the rate-limit family in #31132, but its per-chat circuit state plus profile-scoped Weixin secret reads are a distinct current patch.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for isolating a concrete cross-chat delivery behavior: current main's breaker is adapter-wide (gateway/platforms/weixin.py:1210-1211, 1777-1778).

Problems

  • The proposed partitioning allows a different chat to send after iLink returned -2 for the same adapter token (gateway/platforms/weixin.py:1780-1787). The adapter also serializes all text sends through one lock (gateway/platforms/weixin.py:1757). The diff provides no evidence that iLink's quota is recipient-scoped rather than token/account-scoped.
  • No tests are changed. Existing coverage only exercises the same chat ID (tests/gateway/test_weixin.py:246-271), not the proposed cross-chat behavior.
  • The profile-scoped get_secret() substitutions in the diff already landed in 6160a8025327112c507e49dd3f7f6c669220a105; salvage should exclude those duplicate hunks.

Suggested changes

  • Establish the iLink quota scope and add a two-chat adapter send-path regression test. Preserve the shared circuit if the provider limits by credential; otherwise assert that chat B remains deliverable while chat A is cooling down.

Automated hermes-sweeper review.

@@ -1816,7 +1822,7 @@ async def _send_text_chunk_locked(
raise RuntimeError(
f"iLink sendmessage error: ret={ret} errcode={errcode} errmsg={errmsg}"
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This partitions a breaker for a shared adapter/token, so a send to chat B can proceed immediately after iLink rate-limits chat A. Please establish that iLink's -2 quota is recipient-scoped and add a two-chat send-path test; otherwise the circuit should remain credential-scoped.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping 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-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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