Skip to content

fix(weixin): clear content dedup key after text-batch dispatch - #36758

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/weixin-content-dedup-lifecycle
Open

fix(weixin): clear content dedup key after text-batch dispatch#36758
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/weixin-content-dedup-lifecycle

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented Jun 1, 2026

Copy link
Copy Markdown

What does this PR do?

Clears the content fingerprint dedup key from MessageDeduplicator after the Weixin text-batch is dispatched, so the same text sent again after the previous turn completes is accepted instead of silently dropped.

Related Issue

Fixes #36750

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/platforms/helpers.py: Added MessageDeduplicator.remove(key) method to evict a single key from the TTL cache, enabling lifecycle-aware dedup patterns.
  • gateway/platforms/weixin.py: Store the content dedup key on the event (_dedup_content_key) and remove it from the dedup cache in _flush_text_batch's finally block after dispatch. In-flight dedup (same text arriving while the first is still queued) is unaffected.
  • tests/gateway/test_weixin.py: Added test_same_text_accepted_after_previous_processing_completes regression test — sends the same text twice with a flush in between and asserts both reach handle_message.

How to Test

  1. Run python3 -m pytest tests/gateway/test_weixin.py::TestWeixinContentDedup -v — all 3 tests should pass.
  2. Run python3 -m pytest tests/gateway/test_weixin.py -x -q — all 59 tests should pass.
  3. Manual: with a Weixin/WeChat gateway configured, send "怎么样了", wait for the response, then send "怎么样了" again — the second message should be processed normally instead of being silently dropped.

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

Documentation & Housekeeping

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

Code Intelligence

  • Analyzed: MessageDeduplicator.remove() (new method), _flush_text_batch (callers: 1, flows: text-batch dispatch)
  • Blast radius: LOW — change is scoped to Weixin adapter's content dedup lifecycle; MessageDeduplicator is a shared helper but remove() is additive (no existing callers affected)
  • Related patterns: TextBatchAggregator uses similar lifecycle pattern; other adapters using MessageDeduplicator (discord, slack, feishu) only use is_duplicate() and clear() — unaffected by remove() addition

@dsameer0-code

dsameer0-code commented Jun 1, 2026 via email

Copy link
Copy Markdown

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists platform/feishu Feishu / Lark adapter comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter and removed platform/feishu Feishu / Lark adapter labels Jun 1, 2026
mxnstrexgl
mxnstrexgl approved these changes Jun 1, 2026
View reviewed changes

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

🤖 Automated PR Review

Security Scan

  • ✓ No hardcoded secrets, injection sinks, unsafe deserialization, or dependency red flags found by this automated scan.

Code Quality

  • ✓ No blocking code-quality issues found by this automated scan.

Summary

Status: APPROVE — security findings: 0, quality suggestions: 0.

Automated review; raw diff content intentionally omitted.

@liuhao1024 liuhao1024 mentioned this pull request Jun 8, 2026
Open
1 task
This was referenced Jul 11, 2026
Open
Open
@teknium1

teknium1 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for isolating the Weixin content-dedup lifecycle issue; the current-main premise is real at gateway/platforms/weixin.py:1415-1422.

Problems

  • gateway/platforms/weixin.py:1529-1540 merges later text into the first pending event. The proposed _dedup_content_key carries only the first event's key, so a second distinct text merged into that batch remains claimed after the flush and is still suppressed for the 300-second TTL.
  • The proposed cleanup runs after handle_message() returns. On current main, gateway/platforms/base.py:4608-4614 and :4795-4802 show that handle_message() starts background processing and returns before the turn completes, so this does not implement the body’s stated completion boundary.

Suggested changes

  • Preserve all accepted content keys across batch merges and release the complete set at a deliberately chosen lifecycle boundary.
  • Add a merged-batch regression: send distinct A and B before one flush, then verify both A and B are accepted according to the selected completion contract.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@aldoeliacim aldoeliacim mentioned this pull request Aug 21, 2026
Open
@liuhao1024 liuhao1024 closed this Sep 22, 2026
@liuhao1024
liuhao1024 force-pushed the fix/weixin-content-dedup-lifecycle branch from 09f10b2 to f5ed909 Compare September 22, 2026 07:39
@liuhao1024
fix(weixin): release content dedup keys for a dispatched text batch
5946e45 
Rework of the NousResearch#36758 fix onto current main's shared batching, per review:

- The content fingerprint is now a per-event key set: chunks merged into a
  pending batch contribute their keys to the surviving event, so a flushed
  batch releases every accepted fingerprint, not just the first chunk's.
- Release runs in _dispatch_text_batch's finally — the boundary is now
  honest: the batch has been handed to the pipeline (handle_message
  returning only marks background start, not turn completion). An
  upstream retry racing this boundary still hits the message_id dedup.
- Reuses MessageDeduplicator.discard() already on main.
- Adds the merged-batch regression requested by review.
@liuhao1024

liuhao1024 commented Sep 22, 2026

Copy link
Copy Markdown
Author

Rebased onto current main and reworked per the review, since the batching machinery moved into the shared base adapter (_enqueue_text_event / _flush_text_batch / _dispatch_text_batch are now override seams in gateway/platforms/base.py):

  • Merged batches now release every content key. The fingerprint is a per-event key set: each accepted text contributes content:{sender}:{md5} to event._dedup_content_keys, and the weixin _enqueue_text_event override unions the incoming keys into the surviving pending event before super() merges the chunk in. _dispatch_text_batch's finally then releases the complete set.
  • Honest lifecycle boundary. Release now happens at the dispatch seam — the point where the batch is handed to the pipeline — instead of the old "after handle_message() returns" spot the review correctly flagged as only marking background start. Documented as such in the code: an upstream retry racing this boundary is still caught by the message_id dedup; only a fresh send of the same text under a new message_id is re-accepted, which is the [Bug]: Weixin content dedup can drop legitimate repeated messages after the previous turn has completed #36750 symptom. (Turn-completion itself has no adapter-visible signal on main, so I chose the boundary that is both real and testable rather than overstating one.)
  • MessageDeduplicator.discard() (already on main for cancelled handoffs) is reused instead of adding a parallel remove().
  • Added the requested merged-batch regression: distinct A and B accepted in one window dispatch as a single batch (A\nB), then both A and B are re-accepted under fresh message_ids — asserting the second batch carries both texts, which fails under the single-key shape.

Tests: tests/gateway/test_weixin.py 40/40 (including the #16182 in-window suppression regression, unchanged), test_text_batch_flush_invariants.py + test_text_batching.py 11/11, ruff clean.

@liuhao1024 liuhao1024 reopened this Sep 22, 2026
@Wenfengcheng Wenfengcheng mentioned this pull request Sep 30, 2026
Closed

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mxnstrexgl mxnstrexgl mxnstrexgl approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Weixin content dedup can drop legitimate repeated messages after the previous turn has completed

5 participants

@liuhao1024 @dsameer0-code @teknium1 @mxnstrexgl @alt-glitch