fix(bluebubbles): avoid stale duplicate webhook events - #27985
fix(bluebubbles): avoid stale duplicate webhook events#27985jeremyknows wants to merge 2 commits into
Conversation
|
Thanks for the focused BlueBubbles fix. I verified the core premise still holds on current main: gateway/platforms/bluebubbles.py:367 registers both Problems
Suggested changes
This is an automated hermes-sweeper review. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused BlueBubbles fix. The core premise remains valid on current main: gateway/platforms/bluebubbles.py:365-377 reuses same-URL webhooks without validating event sets and registers both message event types.
Problems
- PR head regresses the password-redaction fix: its
gateway/platforms/bluebubbles.py:279,:298, and:338log the password-bearingwebhook_url. Current main commit514f5020cuses_webhook_register_url_for_logfor these sites. - At PR-head
gateway/platforms/bluebubbles.py:282, a failed_unregister_webhook()is ignored before the replacement POST. Since_unregister_webhook()suppresses delete exceptions and returnsFalse(:327-345), this can leave the stale webhook in place and add another duplicate. The new test covers only successful deletion (tests/gateway/test_bluebubbles.py:602-626).
Suggested changes
- Preserve current main's masked log URL at all registration and unregistration log sites.
- Do not POST a replacement when stale-registration deletion fails; add a failed-delete/no-POST regression test.
This is an automated hermes-sweeper review.
| "[bluebubbles] webhook already registered: %s", webhook_url | ||
| ) | ||
| return True | ||
| await self._unregister_webhook() |
There was a problem hiding this comment.
_unregister_webhook() catches delete failures and returns False, but this result is ignored before creating a new registration. If deletion fails, the old stale webhook remains and this POST creates the duplicate this change is meant to avoid. Return failure/retry without POSTing, and add a failed-delete regression test.
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.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Three PRs address overlapping BlueBubbles duplicate-delivery and webhook-lifecycle causes. #27985 narrowly changes subscriptions and stale-registration replacement, #34183 combines event-aware registration with broader inbound deduplication and listener ownership, and #43672 separates outbound-only lifecycle while adding a dedupe implementation and an unrelated title-warning change.
Related pull requests
- #27985
related— (+69/-7) — keep open, but update before merge: the diff directly removesupdated-messagefrom the desired subscription and replaces stale same-URL registrations, matching the reported duplicate-event cause. Consistent with the keep_open reviews, it must preserve current main's redacted webhook logging and must not POST a replacement when stale-webhook deletion fails; add coverage for that failure path. - #34183 [closed]
related— (+535/-34) — closed, useful as a broader reference implementation superseded by #45317: the diff covers new-message-only defaults, event-aware registration, listener ownership, status-update filtering, and DM-variant deduplication. Its scope substantially exceeds the focused registration cause, and its stale-webhook deletion path also continues after deletion failures. - #43672
related— (+117/-9) — keep open only for selective salvage: the outbound-onlyconnect(start_webhook=False)path directly prevents standalone sends from registering or unregistering the gateway webhook. Despite the keep_open review on #43672, the full diff should not be consolidated as-is because its sender/text fallback can discard distinct same-text messages with different stable IDs, it lacks dedupe behavior tests, and its title-generation warning suppression contradicts existing failure-visibility behavior.
Duplicates
#27985, #34183, and #43672 overlap on BlueBubbles duplicate delivery, but they are not exact duplicates: #27985 is the focused stale-event registration fix, #34183 is a closed broader hardening implementation, and #43672 uniquely contains the explicit outbound-only lifecycle path.
Suggested consolidation
Merge #27985 after rebasing it onto current main, preserving masked log URLs, aborting replacement when stale-webhook deletion fails, and adding the failed-delete/no-POST regression test. Use closed #34183 as reference and selectively port #43672's outbound-only lifecycle fix with tests, but do not port its lossy fallback dedupe or title-warning suppression; #34183 requires no further closure, and #43672 should only be closed as superseded once its valid lifecycle change is represented in the consolidated implementation.
Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 45 kB of PR diffs, 5 kB of issue/PR text, 5 kB of discussion (7 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
BlueBubbles webhook registration now subscribes only to
new-messageevents and replaces existing registrations for the same URL when their event set is stale. This avoids duplicate inbound handling fromupdated-messageregistrations while still reusing an already-correct webhook after restart.Diff scope
gateway/platforms/bluebubbles.py— register withnew-messageonly; compare existing webhook events; delete/recreate stale registrations.tests/gateway/test_bluebubbles.py— regression coverage for fresh registration, stale-event replacement, existing correct registration reuse, and duplicate cleanup.Verification
Real-behavior proof
new-messageandupdated-message, and an old same-URL webhook with stale events could be reused instead of corrected.tests/gateway/test_bluebubbles.py.HOME=/Users/watson scripts/run_tests.sh tests/gateway/test_bluebubbles.py50 passed in 1.00s.events == ["new-message"]; stale same-URL registrations containingupdated-messageare deleted and recreated; already-correct same-URLnew-messageregistrations are reused.Regression check against pre-fix base
I copied the updated BlueBubbles test file onto
origin/mainin a detached worktree and ran the two behavior tests that cover this change:Test plan
origin/mainwith the updated behavior tests.Commits
3a0c51194 fix(bluebubbles): register new-message webhooks only64fb41983 fix(bluebubbles): replace stale webhook event registrations