fix(gateway): stabilize BlueBubbles DM session keys - #67105
Closed
JoshHobbs wants to merge 3 commits into
Closed
Conversation
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.
Canonicalizing the DM key fixes new routing, but it does not rescue installs
that already split one conversation across several keys — and those are
exactly the installs the change is for. On upgrade the canonical key
(`…:dm:+1555…`) often already exists: it is the stray session created by a
GUID-less webhook mid-thread, typically near-empty. The routing index would
resolve straight onto that stray and leave the real transcript orphaned under
`…:dm:any;-;+1555…`.
Collapse the aliases when the routing index loads:
1. Group BlueBubbles DM routes by the key build_session_key would produce
now, keep the most recently updated entry, and rewrite its state.db peer
row to the canonical key. Recency picks the live conversation rather than
whichever variant happens to own the canonical key.
2. End the retired siblings with `session_key_migration`. Dropping them from
the index alone is not enough: they stay live in state.db under the
canonical key, and a stray is usually *started* later than the real
conversation (it is born from a stray webhook mid-thread), so a later
`ORDER BY started_at DESC` peer lookup reopens the stray and orphans the
transcript. `session_key_migration` is not one of the reasons
find_latest_gateway_session_for_peer treats as recoverable, so the row
can never be reopened while its transcript stays readable via /resume.
3. Add an opt-in `match_by_participant_identity` peer lookup for DM
transports whose session key is derived from participant identity rather
than from the transport chat id. The strict peer tuple matches on chat_id, so it
misses when the server reports a DM under a different service prefix than
the one stored (BlueBubbles does this: a session stored as
`iMessage;-;+1555…` is reported as `any;-;+1555…` later). The fallback
keeps the routing namespace, source, chat type and thread, and requires a
non-empty participant id — it only ignores chat_id. It stays off for every
other caller, so no other transport's lookup changes.
Tested: aliases collapse to one durable route; a collision keeps the most
recent session; a retired sibling is ended and cannot be reopened by recovery;
identity-matched recovery requires a stable user_id; and the relaxation is
opt-in, so the strict peer tuple is unchanged for every other transport.
The alias migration and the stale-route prune both run in _ensure_loaded_locked, and their order matters. Prune does more than drop ended entries: when a canonical route points at an ended session, it calls _recover_session_from_db and can *repoint* that route to a still-live session recovered for the same peer (via the match_by_participant_identity fallback for BlueBubbles DMs). With the migration running first, it grouped the raw index and picked its winner purely by updated_at. A newer-but-ended canonical entry could win over an older-but-live alias, and the migration would then retire the live alias — ending a session prune was about to rescue. Run prune first, so the index is already healed (ended canonical entries repointed to their live sessions, genuinely dead ones dropped) before the migration computes winners and retirements. The migration then only collapses what actually remains. Adds a regression test: a live `any;-;` alias plus a newer, ended canonical entry. Before the reorder the migration retires the live session; after it, the live session is kept and the canonical key resolves to it.
alt-glitch
added
type/bug
Collaborator
Author
|
Withdrawing in favor of #45717, which fixes this at the right layer. This PR canonicalizes the BlueBubbles DM Two things worth recording for anyone working this area:
|
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Makes the BlueBubbles DM session key stable, so one iMessage conversation maps to one session.
BlueBubbles surfaces a single 1:1 conversation under more than one
chat_id, andbuild_session_keykeyed on the raw value:session_chat_id = chat_guid or chat_identifier, so a webhook with no chat GUID falls back to the bare handle —any;-;+1555…and+1555…key differently.iMessage;-;+1555…→any;-;+1555…for the same conversation).The known symptom is duplicate replies (#30708, #34372). But the split has a worse, unreported consequence: each variant is its own
SessionEntrywith its ownupdated_at, so a variant goes stale while the conversation continues under another. When a webhook later routes to the stale variant,_should_reset()finds it idle and clears an active conversation — and the notice readsinactive for 3h(it renderspolicy.idle_minutes, not real elapsed time). Observed in production: a live 298-message thread reset because a GUID-less webhook hit a sibling key idle for 20 days (see Logs).Fixes #30708 (session-key half). Complementary to the open adapter PRs (#45717, #34378, #18395, #19976, #27985), which fix the duplicate-event trigger in
gateway/platforms/bluebubbles.py; none make the key stable, and cause (2) reproduces with no duplicate event at all. This PR touches onlygateway/session.pyandhermes_state.py.Type of Change
Changes
canonical_bluebubbles_identifier()unwraps the<service>;-;DM prefix;build_session_keyroutes the DMchat_idthrough it, right beside the existingcanonical_whatsapp_identifierbranch. Groups (;+;) and all other platforms untouched.state.dbpeer row, and end retired siblings withsession_key_migration.match_by_participant_identityonfind_latest_gateway_session_for_peermatches source + non-empty participant id + chat type + thread, ignoring onlychat_id. Off for every other caller.Why the migration is required (not just the key fix)
Canonicalization alone would orphan exactly the installs it targets. On a split install the canonical key is usually already taken by the stray session a GUID-less webhook created mid-thread — routing resolves onto that near-empty stray and abandons the real transcript. The migration collapses by recency instead.
Ending the siblings matters: dropped-but-live siblings stay in
state.dbunder the canonical key, and a stray is typically started later than the real conversation, so anORDER BY started_at DESClookup would reopen it.session_key_migrationisn't a recoverableend_reason, so the row can't be reopened, while/resumecan still read it. Verified on a live install: the 298-message session survived and continued.Design notes
ws_transportpath rebuildsSessionSourcewithout the adapter, so the key builder is the only chokepoint both paths cross.chat_id_alt? It holds the bare handle but is unreliable (only set when the webhook carrieschatIdentifier; omitted on the relay path) and overloaded (declared for Signal's group id).build_session_keyand the peer lookup reference it zero times today. Canonicalizing thechat_idwe always have is robust and mirrors WhatsApp.user_idfromhandle.address, already a bare handle.canonical_whatsapp_identifier(mirrored here) and Photon's_normalize_chat_key— a separate platform, never on the BlueBubbles path, and weaker (any;-;+ E.164 only). A follow-up could point Photon at this helper.Known pre-existing interaction
_enrich_async_delegation_routing(gateway/run.py) rebuildschat_idfrom the key when no persisted origin exists. It's now the bare handle, which takes_resolve_chat_guid'schat/querypath (single page,limit: 100) instead of returning a raw GUID as-is. Narrow case (gateway restarted, no origin, backgrounddelegate_taskcompletes, DM not in the 100 most-recent chats) could fail to resolve. Left as-is to keep this PR scoped; happy to paginate_resolve_chat_guidor preferoriginin a follow-up.Also flagging for merge order: #66264 (open) edits the same
find_latest_gateway_session_for_peer. No conflict — this PR adds an opt-in fallback and reorders passes; it doesn't change the recovery-policy logic #66264 targets.How to Test
pytest tests/gateway/test_bluebubbles_session_key.py -q→ 17 passed;pytest tests/test_hermes_state.py -q→ 372 passed.tests/gatewayfailure set is identical with/without the change. Verified on currentmain— our 17 plus 560 tests across the reset/recovery suites upstream recently churned (test_session,test_multiplex_phase0,test_session_reset_notify,test_session_store_stale_prune,test_hermes_state) all pass.ulimit -n 8192first, or the single-processpytest tests/reports spuriousErrno 24failures unrelated to this change.Checklist
main; added-test coverage passes (see How to Test)Logs
Three keys, one conversation, from a live
state.db:The reset — stale bare-handle key ended while the
any;-;key was mid-conversation:After: one key, one live session.