fix: deduplicate BlueBubbles webhook messages - #33337
Conversation
ed30c7f to
d934f5c
Compare
|
Updated this PR to address the scope concern:
The new head is |
|
Thanks for working on this — deduping repeated BlueBubbles webhook deliveries is definitely useful. One caution from testing a similar path: deduping solely by message GUID can be a little too broad unless the key also accounts for the webhook lifecycle state. BlueBubbles may deliver multiple events for the same GUID as a message transitions or gets enriched, and some of those same-GUID updates can still be meaningful. Cases worth guarding with regression tests before this lands:
A possible safer shape is to dedupe on a small event fingerprint rather than GUID alone, e.g. message GUID plus event/update type plus the relevant attachment/reaction/edit marker. That still suppresses true webhook replays while preserving legitimate same-message lifecycle updates. Happy to help adapt tests around those lifecycle cases if useful. Disclosure: This comment was prepared with AI assistance under human direction and reviewed before posting. |
|
Follow-up on the same-GUID lifecycle concern: updated the branch so webhook dedup no longer keys solely on the BlueBubbles message GUID. It now fingerprints the event shape (event type, text, associated/reply fields, item type, and attachment metadata), so exact replay payloads are still suppressed while same-GUID text updates and attachment completion events are allowed through. Added regression coverage for both same-GUID text updates and same-GUID attachment completion.\n\nVerification:\n- |
Duplicate of #18395 — the earliest still-open PR wiring |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the verified replay-dispatch gap and for updating the key after the same-GUID lifecycle feedback.
Problems
gateway/platforms/bluebubbles.py:1056checks dedup only after the attachment loop at lines 962-996. An exact replay with an attachment still performs the second download/cache side effect before being suppressed. Move the check before that loop and add a replay-with-attachment assertion that_download_attachmentruns once.scripts/ci/classify_changes.py:89is unrelated to BlueBubbles and reverses current main's explicit MCP-catalog skip at line 90. Please remove this hunk from the focused fix.
Suggested changes
- Preserve the raw event fingerprint before attachment processing, then gate all downstream attachment and dispatch work on it.
Automated hermes-sweeper review.
| record.get("messageGuid"), | ||
| record.get("id"), | ||
| ) | ||
| dedup_key = self._webhook_dedup_key(payload, record, message_id, text) |
There was a problem hiding this comment.
This guard is reached only after the attachment loop at lines 962-996, so an exact replay still downloads and caches every attachment a second time. Build/check the fingerprint after raw text extraction and before attachment processing; add a replay-with-attachment test asserting one download and one dispatch.
| ret["deps"] = True | ||
|
|
||
| # explicitly skip mcp catalog here. it's not needed unless those files are modified. | ||
| ret["mcp_catalog"] = True |
There was a problem hiding this comment.
This unrelated CI behavior change contradicts the current-main comment that empty/.github diffs should skip the MCP catalog lane unless catalog files changed. Please drop it from this focused BlueBubbles fix.
Summary
MessageDeduplicatorin the BlueBubbles adapterFixes #33327
Validation
uv run --extra dev --extra messaging python -m pytest tests/gateway/test_bluebubbles.py -q -o addopts=(61 passed)uv run --extra dev --extra messaging ruff check gateway/platforms/bluebubbles.py tests/gateway/test_bluebubbles.py scripts/ci/classify_changes.pygit diff --check