fix(gateway): deduplicate BlueBubbles inbound messages by GUID + content hash - #19976
fix(gateway): deduplicate BlueBubbles inbound messages by GUID + content hash#19976alexmacarthur wants to merge 1 commit into
Conversation
|
Likely duplicate of #18395 |
|
Good callout, @alt-glitch. I weighed whether this is its own PR, but the other MR has been out for a few weeks, and the issue is impacting people right now. I'm hoping that one can build upon the deduplication handling contained here, while fixing the problem more quickly for users. |
| # BlueBubbles can dispatch requests for the same message ~500-900ms apart, | ||
| # particularly when SIP is disabled. De-duplciation tracks by (guid, text_hash) | ||
| # to avoid sending the same message multiple times. | ||
| _MAX_DEDUP_CACHE = 1000 |
There was a problem hiding this comment.
This is a largely arbitrary number that felt fine. Up for debate, tho.
| # Inbound message dedup (BlueBubbles fires each message twice) | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| def _check_dedup(self, guid: Optional[str], text: str) -> bool: |
There was a problem hiding this comment.
There's a case to be made for separating writing & reading to the dictionary, rather than co-locate them here.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the duplicate-webhook problem. The premise is still present on current main: every accepted BlueBubbles webhook reaches asyncio.create_task(self.handle_message(event)) at gateway/platforms/bluebubbles.py:1040, while the adapter subscribes to both new-message and updated-message at gateway/platforms/bluebubbles.py:374-377.
Problems
- The proposed
(guid, text_hash)key can drop a later same-GUID, same-text attachment/lifecycle update. Current main builds attachment media atgateway/platforms/bluebubbles.py:934-968; the PR's key does not include event type or attachment state. - Current main already provides bounded TTL deduplication in
gateway/platforms/helpers.py:27-75. The PR adds a second size-only implementation instead of extending that helper. - The new tests call
_check_dedupdirectly, but do not verify duplicate payloads through_handle_webhookresult in onehandle_messagedispatch.
Suggested changes
- Use
MessageDeduplicatorwith a canonical replay fingerprint including GUID, event type, text, and relevant attachment/update metadata. - Add async webhook-path tests for exact replay suppression and same-GUID lifecycle updates.
- Remove the unused
timeimport.
Automated hermes-sweeper review.
| import logging | ||
| import os | ||
| import re | ||
| import time |
There was a problem hiding this comment.
time is not referenced by this PR's implementation. Please remove the unused import.
BlueBubbles surfaces a single 1:1 conversation under more than one chat_id,
and build_session_key used the raw value, so one thread split across several
session keys:
1. The adapter sets `session_chat_id = chat_guid or chat_identifier`
(gateway/platforms/bluebubbles.py), so a webhook carrying no chat GUID
falls back to the bare handle. The two forms key differently:
`any;-;+1555…` vs `+1555…`.
2. The GUID form recorded for one conversation is not stable over time. On
the deployment this was found on, sessions carry `iMessage;-;+1555…` from
May and `any;-;+1555…` since July, while the server today reports exactly
one chat for that handle (`any;-;+1555…`, chatIdentifier `+1555…`) and
uses the `any` prefix for every chat it knows about. Whatever drove that
change server-side, the routing key should not depend on it.
The usual report of this is duplicate replies (NousResearch#30708, NousResearch#34372): two chat-id
variants defeat the in-flight guard, so a message gets answered twice. The
split has a second and worse consequence that has not been reported. Each
variant is a separate SessionEntry with its own updated_at, so a variant that
has not been messaged recently goes stale while the conversation continues
under another. When a webhook eventually routes to the stale variant,
_should_reset() finds it idle and clears an actively-used conversation.
Observed in production: a thread whose live session held 298 messages was
reset because a GUID-less webhook landed on a sibling key last touched 20 days
earlier. The notice reads "inactive for 3h" because it renders
policy.idle_minutes rather than measured elapsed time, so it does not point at
the real cause.
Canonicalize the DM chat_id the way WhatsApp already canonicalizes JID/LID
aliases: unwrap the `<service>;-;` prefix so every form of one conversation
maps to the bare handle. Group GUIDs use `;+;` and carry an opaque chat id
rather than a participant handle, so they are returned untouched, as is every
other platform. BlueBubbles needs no group-participant equivalent of the
WhatsApp fix: the adapter already sets user_id from handle.address, which is
a bare handle.
Where a deployment does have distinct iMessage and SMS chats for the same
handle, those now share one session key. That is intended — one human, one
agent conversation — and replies to an inbound message are unaffected, since
they route on the live event's source.chat_id rather than on the key.
Existing sessions are not orphaned. Canonicalization rewrites only the routing
key, never source.chat_id, so when the exact-key lookup misses after upgrade,
find_latest_gateway_session_for_peer's peer-tuple fallback still matches the
stored row on (source, user_id, chat_id, chat_type, thread_id) and adopts the
transcript under the new key. The regression test drives build_session_key
rather than hardcoding the key, so it fails both if the canonicalization is
dropped and if source.chat_id is ever canonicalized too.
This is the session-key half of NousResearch#30708, complementary to the open adapter-side
PRs (NousResearch#45717, NousResearch#34378, NousResearch#18395, NousResearch#19976, NousResearch#27985) that suppress the duplicate-event
trigger. Those do not make the key stable on their own: the form drift in (2)
puts one conversation under two keys with no duplicate event involved, so the
reset stays reachable with any of them merged.
What does this PR do?
After disabling SIP in BlueBubbles, which Hermes requires in order to access the Private API, I began getting two independent responses for any given message (see screenshot below).
Digging through the BlueBubbles logs and Hermes integration, I discovered it's due to BlueBubbles dispatching webhook requests after both the incoming new message as well as the "read" event:
The first fires from the private API (phone helper) on immediate detection. The second fires ~800ms later when the Mac Messages DB sync confirms the message. Both carry the same guid and dateCreated — the second is simply a read-receipt-triggered re-dispatch.
The fix adds a bounded OrderedDict cache of (guid, text_hash) pairs that suppresses the duplicate webhook before it reaches the agent loop. The cache is size-bounded (max 1000). I considered making eviction time-bound, but doing so by size is simpler and fully deterministic.
Related Issue
I consider this to be a small slice for the feature discussed here.
#8513
This PR solely focuses on deduplicating message requests, which will arguably remain a concern even after a more mature feature is fleshed out.
Fixes #
Type of Change
Changes Made
How to Test
Checklist
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 or workflows — or N/AScreenshots
Example of receiving duplicate messages:
