Conversation
|
✅ Verified — BlueBubbles duplicate-message and DM-to-group misrouting fix Reviewed the diff for webhook dedup, chat GUID resolution, and event subscription safety.
The fix is correct and addresses a real message-fanout bug. The stale webhook cleanup ensures the event-list change takes effect even when an old registration exists. No issues found. |
8ce10b9 to
86be136
Compare
|
Was this implemented? I am getting this error (DM to group misrouting) |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for consolidating the BlueBubbles duplicate-delivery work. The outbound participant-fallback portion is already on current main via c279706d3, while the inbound event-registration issue remains present in gateway/platforms/bluebubbles.py:71,376,907.
Problems
gateway/platforms/bluebubbles.py:372returns when it finds one compliant registration._find_registered_webhooks()returns every same-URL registration, so a later staleupdated-messageregistration is left active and can still deliver duplicates.gateway/platforms/bluebubbles.py:160adds an unbounded five-minute GUID dictionary, despite the bounded sharedMessageDeduplicatoringateway/platforms/helpers.py:27-71.- The diff changes no tests, leaving replay, TTL, bare-address, and multi-registration cleanup behavior unverified.
Suggested changes
- Clean all stale/duplicate same-URL registrations before retaining or creating one desired registration, and cover both list orderings.
- Reuse
MessageDeduplicator(or enforce an equivalent size bound) and add focused adapter regression tests.
Automated hermes-sweeper review.
| return True | ||
| desired_events = ["new-message"] | ||
| for wh in existing: | ||
| if wh.get("events") == desired_events: |
There was a problem hiding this comment.
_find_registered_webhooks() returns every same-URL registration. Returning on the first desired entry leaves a later stale updated-message registration active, so cleanup depends on API list order and duplicates can persist. Scan/delete all stale or duplicate entries before returning.
| @@ -151,6 +157,8 @@ def __init__(self, config: PlatformConfig): | |||
| self._private_api_enabled: Optional[bool] = None | |||
| self._helper_connected: bool = False | |||
| self._guid_cache: OrderedDict[str, str] = OrderedDict() | |||
| self._recent_message_guids: Dict[str, float] = {} | |||
There was a problem hiding this comment.
This cache is only TTL-pruned and has no cardinality bound. Please reuse the bounded MessageDeduplicator from gateway.platforms.helpers or add an explicit maximum size.
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.
…ting BlueBubbles delivers one iMessage more than once, which makes Hermes reply twice — and sometimes leaks a DM reply into a group thread. 1. Remove updated-message from webhook subscription and _MESSAGE_EVENTS. BlueBubbles emits updated-message for delivery receipts, read state, and attachment finalization, each carrying a slightly different chatGuid. (Fixes NousResearch#34372) 2. Purge stale webhook registrations on startup. _find_registered_webhooks returns every registration for this URL, so keep at most one compliant registration and delete all others. An earlier revision returned as soon as it found a compliant entry, which left a later stale updated-message registration active and preserved the duplicate delivery. A failed DELETE now aborts rather than adding a second live registration. (Fixes NousResearch#33327) 3. Dedup inbound events by message GUID using the shared bounded MessageDeduplicator from gateway/platforms/helpers.py instead of an unbounded local dict, so replays after reconnect/retry are dropped without unbounded memory growth. (Fixes NousResearch#30708) 4. Normalize bare-address session_chat_id to 'any;-;{sender}' when payloads omit chatGuid, so outbound resolution cannot pick a group containing the handle. The outbound participant-address fallback from the original revision is dropped here: it already landed on main via c279706. Tests: tests/gateway/test_bluebubbles.py adds coverage for stale-registration cleanup in both list orderings, failed-DELETE abort, GUID replay, TTL expiry, distinct-GUID passthrough, and the dedup cache size bound.
86be136 to
d8f97b6
Compare
|
Thanks for the detailed review — rebased onto current 1. Stale registrations after a compliant one — fixed. 2. Unbounded GUID dict — replaced with the shared bounded 3. No test coverage — added I also dropped the outbound participant-address fallback change, since that already landed on Verification: One note: |
|
Reproduced on Hermes v0.20.6 at current A current-main local carry of this PR’s layered approach restored one dispatch in the alias replay regression: The adapted carry also hardens failure paths found during independent review: invalid, captioned-download-failed, partial multi-attachment, or missing-attachment-GUID deliveries do not claim the message GUID before a complete agent handoff; cleanup preserves one exact-good webhook; and connection fails closed if registration cannot be established. This is production reproduction evidence that the defect remains present on current Prepared with AI assistance under human direction; no private logs or identifiers are included. |
Summary
Four layered fixes for BlueBubbles webhook and routing bugs that cause duplicate replies, cross-chat message leaks, and DM-to-group misrouting.
Problem
BlueBubbles can deliver a single inbound iMessage twice under different chat identifiers, causing Hermes to spin up two parallel sessions and reply in both a DM and a group chat. Separately, outbound DM resolution can fall back to participant-address matching, which can route a private reply into a family/group thread.
Fixes
Remove
updated-messagefrom webhook subscription — BlueBubbles emitsupdated-messagefor delivery receipts, read state, and attachment finalization, each with a slightly differentchatGuidformat. Registering onlynew-messageprevents the duplicate dispatch at the source. (Fixes BlueBubbles: webhook auto-registration includes 'updated-message', causing every iMessage to be processed twice (with different chat-id variants) #34372)Drop participant-address fallback in
_resolve_chat_guid— The same contact can appear in both a 1:1 DM and group chats. The old participant-based fallback could resolve an outbound DM target to a group GUID, leaking private replies into family/group threads. Now matches strictly onchatIdentifieronly; unresolved targets fall through to_create_chat_for_handle. (Fixes [Bug]: BlueBubbles outbound chat resolution can misroute DM replies to group chats. Privacy leak! #24157)Add GUID-based inbound message dedup with 5-minute TTL — Even with
updated-messageremoved, BlueBubbles can emit duplicate events on reconnect or retry. A receiver-side dedup layer drops the second delivery regardless of event type or chat ID variant. (Fixes BlueBubbles adapter lacks inbound dedup → duplicate processing + two parallel sessions per message #30708)Clean up stale webhook registrations on startup — When an existing webhook still subscribes to
updated-message, it is removed and re-registered with onlynew-messagebefore processing begins. Prevents the duplicate-delivery bug from persisting across gateway restarts. (Fixes BlueBubbles webhook conflicts can duplicate or interrupt replies #33327)Normalize bare-address
session_chat_id— Some BlueBubbles payloads omitchatGuidand expose only the sender address. The adapter now normalizes these toany;-;{sender}format so outbound resolution does not pick a group that happens to contain that handle.Testing
All 59 existing BlueBubbles tests pass. The fix has been running in production on a personal deployment for 24+ hours with no duplicate deliveries or cross-chat routing.
Related issues
Closes #34372, closes #24157, closes #30708, closes #33327.
Partially addresses #33489 (group chat filtering — separate feature).