fix(bluebubbles): stop double-delivering messages via updated-message webhook - #32313
fix(bluebubbles): stop double-delivering messages via updated-message webhook#32313dandomin wants to merge 1 commit into
Conversation
hclsys
left a comment
There was a problem hiding this comment.
The registration fix is correct — dropping updated-message from _DESIRED_WEBHOOK_EVENTS plus the migration path (re-register when the existing set doesn't match, and _unregister_webhook removes all registrations) cleanly stops BlueBubbles from sending the duplicate.
But the fix is registration-side only — the receive-side filter still accepts updated-message, so the dedup isn't defense-in-depth:
gateway/platforms/bluebubbles.py:57:
_MESSAGE_EVENTS = {"new-message", "message", "updated-message"}and the inbound gate at :815:
if event_type and event_type not in _MESSAGE_EVENTS:
# skippedSo _MESSAGE_EVENTS is what decides whether an inbound webhook is processed, and it still contains updated-message (unchanged by this PR). That means if an updated-message webhook ever still reaches Hermes — a stray old registration the migration didn't catch, an out-of-band/manual registration, a BlueBubbles server shared with another client that requested it, or the server emitting it regardless — line 815 still lets it through and the double-delivery you're fixing recurs.
Since you've decided updated-message is noise for this adapter (and nothing here consumes edit/tapback/read-state from it — those are handled separately), I'd also drop it from _MESSAGE_EVENTS so the receive side ignores it no matter what's registered:
_MESSAGE_EVENTS = {"new-message", "message"}That makes the dedup robust to registration drift rather than relying solely on the server honoring the requested event set. A test feeding an updated-message payload and asserting it's skipped would lock it in.
Migration logic and _unregister_webhook (removes all) otherwise look right.
|
Complementary with #30996 — both address #30708 (BlueBubbles duplicate processing). This PR prevents double-delivery at the source by removing |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the source subscription and stale-registration path. Current main still sends new-message plus updated-message in gateway/platforms/bluebubbles.py:374-377, so the premise remains valid.
Problems
gateway/platforms/bluebubbles.py:293discards the result of_unregister_webhook(). That helper catches a failed DELETE and returnsFalse(gateway/platforms/bluebubbles.py:428-433on current main), but this code would still POST a new registration at line 301. The stale registration can remain active while a new one is added, preserving duplicateupdated-messagedelivery and potentially duplicatingnew-messagedelivery too.
Suggested changes
- Return
Falsewhen stale cleanup fails, and add a test that a failed DELETE performs no replacement POST.
Automated hermes-sweeper review.
| self._webhook_register_url_for_log, | ||
| ) | ||
| return True | ||
| await self._unregister_webhook() |
There was a problem hiding this comment.
_unregister_webhook() catches DELETE errors and returns False; continuing to POST here can leave the stale registration active and add a second one. Return failure when cleanup fails, and cover that no POST occurs after a failed stale DELETE.
… webhook
The adapter registered events=['new-message', 'updated-message']. BlueBubbles
emits 'updated-message' whenever message metadata changes (read receipts, edit
state, tapbacks), so a single inbound text could trigger two webhook
deliveries to the gateway.
The crash-resilience path also reused any existing registration verbatim, so
older Hermes builds that registered the two-event set kept their stale
registration across restarts even after a code fix.
Fix:
1. Register only ['new-message'] (single _DESIRED_WEBHOOK_EVENTS constant
keeps payload + equality check in lockstep).
2. On startup, reuse an existing registration only when its event list
exactly matches the desired set; otherwise unregister and re-create.
3. Abort when stale cleanup fails. _unregister_webhook catches a failed
DELETE and returns False; the previous revision discarded that result and
POSTed a replacement anyway, leaving the stale registration live next to
the new one and preserving the duplicate delivery it was meant to fix.
Tests:
- test_register_fresh asserts POST payload events == ['new-message']
- test_register_replaces_stale_event_registration — stale
[new-message, updated-message] registration is DELETEd and re-POSTed
- test_failed_delete_performs_no_replacement_post (new) — a failing DELETE
returns False and performs no POST
- test_successful_delete_registers_replacement (new) — happy path still
replaces the stale registration
02ce2a0 to
f0acc97
Compare
|
Thanks for the review — rebased onto current
Added the test you asked for plus its happy-path counterpart:
Verification: Note there's overlap with #45717, which consolidates this and the GUID-dedup work. Happy to close whichever you prefer; this one is the narrower, single-concern change. ( |
Summary
The BlueBubbles gateway adapter was registering its webhook with
events=["new-message", "updated-message"]. BlueBubbles firesupdated-messagewhenever message metadata changes (read receipts, edit state, tapback updates), so any inbound message produced two webhook deliveries to Hermes:new-message(the inbound message)updated-messagefor the same messageFor text inbounds this could double-trigger gateway processing of the same message.
Additionally, on adapter startup the crash-resilience path reused any existing registration verbatim. Older Hermes builds had registered the
[new-message, updated-message]event set, so the bug persisted across restarts even after a code fix — the stale registration was never replaced.Fix
["new-message"]going forward.updated-messageregistrations get cleaned up automatically the next time the gateway restarts.A single-element
_DESIRED_WEBHOOK_EVENTSconstant keeps the registration payload and the equality check in lockstep.Tests
Three test changes/additions in
tests/gateway/test_bluebubbles.py::TestBlueBubblesWebhookRegistration:test_register_fresh— now also asserts the POST payloadevents == ["new-message"].test_register_replaces_stale_event_registration(new) — an existing[new-message, updated-message]registration is DELETEd and re-POSTed with[new-message].test_register_reuses_existing— unchanged behavior for the matching-events case.All three pass: