fix(bluebubbles): dedup inbound webhook events by message GUID - #30996
fix(bluebubbles): dedup inbound webhook events by message GUID#30996briandevans wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds inbound message GUID deduplication to the BlueBubbles webhook handler and introduces regression tests to ensure new-message + updated-message events for the same iMessage do not create duplicate deliveries/sessions.
Changes:
- Add
MessageDeduplicatorusage inBlueBubblesAdapter._handle_webhookkeyed by message GUID. - Add regression tests covering same-GUID dedup and different-GUID delivery behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/gateway/test_bluebubbles.py | Adds async regression tests validating same-GUID webhook events are deduped while distinct GUIDs are delivered. |
| gateway/platforms/bluebubbles.py | Deduplicates inbound webhook processing by message GUID prior to session-key derivation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Suppress duplicate inbound webhooks for the same message GUID. | ||
| # BlueBubbles fires both `new-message` and `updated-message` on | ||
| # delivered/read/edit echoes for the same message; without dedup the | ||
| # second event normalizes to a different chat key (chatIdentifier vs | ||
| # chatGuid) and spins up a parallel session for the same chat. | ||
| self._dedup = MessageDeduplicator() |
| r1 = await adapter._handle_webhook(_FakeRequest(new_msg, "secret")) | ||
| await asyncio.sleep(0) | ||
| r2 = await adapter._handle_webhook(_FakeRequest(upd_msg, "secret")) | ||
| await asyncio.sleep(0) |
| class _FakeRequest: | ||
| def __init__(self, payload, password): | ||
| self._payload = payload | ||
| self.query = {"password": password} | ||
| self.headers = {} | ||
|
|
||
| async def read(self): | ||
| return json.dumps(self._payload).encode("utf-8") |
|
Came to the repo to fix this same issue. Thanks @briandevans |
5ba088d to
4121a31
Compare
|
@copilot All three findings addressed in c166153e6:
The slice-3 CI failure ( |
c166153 to
4823b42
Compare
4823b42 to
189668a
Compare
BlueBubbles fires both `new-message` and `updated-message` for the same iMessage (the second on delivered/read/edit echoes). The adapter currently has no inbound dedup — unlike slack/dingtalk/wecom/weixin/ mattermost/feishu — so both events are processed and, because the two payloads carry the chat reference differently (`new-message` includes `chatGuid`, `updated-message` typically omits it and falls back to `chatIdentifier`), the gateway derives two distinct session keys (`any;-;<addr>` vs bare `<addr>`) and spins up two parallel sessions for one chat. Wire in the shared `MessageDeduplicator` helper at the top of `_handle_webhook`, keyed by the message GUID resolved with the same priority already used for `MessageEvent.message_id` (`guid`/`messageGuid`/`id`). This drops the duplicate before session-key derivation, so both the double-processing and the dual-session symptoms disappear, without changing the event subscription (`updated-message` still flows through for the edits/retractions tracked in NousResearch#8513). Regression coverage in `tests/gateway/test_bluebubbles.py`: - Same GUID via `new-message` then `updated-message` → one delivery. - Two distinct GUIDs → both delivered (no false positives). Refs: NousResearch#30708
Address Copilot review on NousResearch#30996: - Pass `max_size=2000, ttl_seconds=300` to `MessageDeduplicator()` explicitly so the long-running cache cap is visible at the call site instead of relying on the helper's defaults. - Replace `await asyncio.sleep(0)` synchronization in the dedup tests with an `asyncio.Event` set inside the patched `handle_message`, awaited via `asyncio.wait_for(..., timeout=2.0)`. Eliminates the slow-CI race where the background task scheduled by `_handle_webhook` had not yet run when the assertions executed. - Hoist the inline `_FakeRequest` helper to module-level `_FakeWebhookRequest` so both tests share one definition.
189668a to
92cbf4a
Compare
|
I opened #38379 as a narrower replacement/complement after testing this approach against the BlueBubbles group/DM alias ordering case. This PR is directionally right for exact GUID replay dedup, but it uses first-seen semantics before chat routing. That means it fixes “two replies” but can still keep the wrong route if BlueBubbles delivers the sparse
#38379 instead ignores So: #30996 removes duplicate replies, but #38379 closes the routing-correctness hole too. |
|
Closing to focus the queue on security/file-safety work where civilian merges are landing. Happy to reopen if maintainers want this picked up. |
What does this PR do?
BlueBubbles fires both
new-messageandupdated-messagefor the same iMessage (the second on delivered/read/edit echoes). The adapter currently has no inbound dedup — unlike slack / dingtalk / wecom / weixin / mattermost / feishu, all of which use the sharedMessageDeduplicatorhelper. As a result the same inbound message is processed twice, and because the two payloads carry the chat reference differently (new-messageincludeschatGuid,updated-messagetypically omits it and falls back tochatIdentifier), the gateway derives two distinct session keys (any;-;<addr>vs bare<addr>) and spins up two parallel sessions for one chat — producing interleaved/duplicate replies and, e.g., two "Session reset" confirmations for a single/new.Fix: wire in the shared
MessageDeduplicatorat the top of_handle_webhook, keyed by the message GUID resolved with the same priority already used forMessageEvent.message_id(guid→messageGuid→id). The dedup runs before session-key derivation, so both the double-processing and the dual-session symptoms disappear without changing the event subscription —updated-messagestill flows through for the edits/retractions work tracked in #8513.This is intentionally scoped to the exact-GUID echo case from #30708. The complementary text-then-attachment two-event coalescing described in #30989 is orthogonal (different GUIDs, requires a debounce-and-merge step modeled on Telegram's media-group handling) and is intentionally left out of this PR.
Related Issue
Fixes #30708
Type of Change
Changes Made
gateway/platforms/bluebubbles.py— importMessageDeduplicatoralongsidestrip_markdown, instantiateself._dedup = MessageDeduplicator()in__init__, and callself._dedup.is_duplicate(msg_guid)near the top of_handle_webhook(after theis_from_meguard, before any chat-key derivation). The GUID is resolved with the same priority list already used forMessageEvent.message_id.tests/gateway/test_bluebubbles.py— newTestBlueBubblesInboundDedupclass:test_same_guid_new_then_updated_message_dedups— drives the exactnew-message→updated-messagesequence from the issue and assertshandle_messageis called once, not twice. Pre-fix this test fails with two delivered events whosesource.chat_idisany;-;+15551234567and+15551234567respectively (the two parallel sessions the issue describes).test_different_guids_both_delivered— negative case proving distinct GUIDs still both reachhandle_message.How to Test
uv run --with pytest --with pytest-xdist --with pytest-asyncio --with pytest-timeout --with aiohttp python3 -m pytest tests/gateway/test_bluebubbles.py -vtest_same_guid_new_then_updated_message_dedupsfails withassert 2 == 1and a diff showing the two distinctsource.chat_idvalues from the issue body.Checklist
Code
fix(bluebubbles): …)Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/ASalvage-with-widening