feat(client): opt-in force_pn_addressing to keep outbound DMs on PN JIDs - #940
juanlotito wants to merge 1 commit into
Conversation
Some companion registrations observe that DMs addressed via LID are accepted by the server but never delivered to the recipient (no Delivered receipt ever arrives), while the same message sent with pre-0.6 PN addressing is delivered within seconds. The unified LID addressing introduced in 0.6 upgrades PN targets to LID whenever a mapping is known (and the DM send path learns mappings via usync on first contact), which leaves such deployments with no way to reach affected chats: the first message to a fresh PN goes through, every subsequent one is silently dropped. Add Client::set_force_pn_addressing(true) as a runtime opt-out scoped to the outbound path only, so all four addressing decisions a DM send makes stay on the namespace the caller provided: recipient fanout keys (send_message_impl), session creation (resolve_lid_mappings via ensure_e2e_sessions), stanza prep participants/enc nodes (SendContextResolver::get_lid_for_phone), and session lock keys (build_session_lock_keys, which must key the same namespace the encryption uses to preserve the per-session lock invariant against concurrent inbound decrypt). Inbound decrypt addressing (resolve_encryption_jid on sender JIDs), LID mapping learning, and inbound session migration are deliberately untouched.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a ChangesForce PN Addressing
Estimated code review effort: 3 (Moderate) | ~25 minutes Look, this is the kind of precise, surgical control I want to see across all our messaging infra — a clean flag, wired consistently through every addressing decision point, with a test that actually proves the boundary holds. Companion registration edge cases matter, and this closes the gap without breaking inbound decrypt paths. That's the level of rigor we need. Ship it, but I want the send.rs overlap with those other PRs resolved before it goes out — can't have two teams fighting over the same lock-key logic. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/context_impl.rs">
<violation number="1" location="src/client/context_impl.rs:51">
P2: The four outbound `force_pn_addressing` gate points read the flag independently with `Ordering::Relaxed`, and there are `.await` suspension points between them in `send_message_impl` (e.g., between the `recipient_bare` computation at line ~1175, `ensure_e2e_sessions` at ~1387, `build_session_lock_keys` at ~1406, and `prepare_dm_stanza` which calls `get_lid_for_phone`). If `set_force_pn_addressing` is toggled concurrently during a send, different stages of the same send may observe opposite values. This recreates the namespace/session-lock divergence the PR explicitly tries to avoid. Snap-shotting the flag once at the start of each send and passing that decision through all four call sites would actually preserve the invariant.</violation>
<violation number="2" location="src/client/context_impl.rs:51">
P2: The `force_pn_addressing_enabled` gate in `get_lid_for_phone` is applied unconditionally inside the shared `SendContextResolver` implementation, which is consumed by `encrypt_for_devices` for any list of device JIDs. Because the method receives only a `phone_user` string with no send-context (e.g., DM vs. group vs. broadcast), enabling the flag will suppress LID resolution for every outbound encrypted send, not just direct messages. The PR describes this as a DM workaround, so the scope should be verified: if group or broadcast flows still require LID addressing, this global suppression could emit PN-addressed `enc` nodes in contexts that expect LID. Consider scoping the gate to DM-only sends or confirming that all callers of `get_lid_for_phone` are safe to receive PN addressing when the flag is on.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| async fn get_lid_for_phone(&self, phone_user: &str) -> Option<String> { | ||
| // Reporting no mapping keeps stanza prep (participants, enc nodes) on | ||
| // the PN namespace (see Client::set_force_pn_addressing). | ||
| if self.force_pn_addressing_enabled() { |
There was a problem hiding this comment.
P2: The four outbound force_pn_addressing gate points read the flag independently with Ordering::Relaxed, and there are .await suspension points between them in send_message_impl (e.g., between the recipient_bare computation at line ~1175, ensure_e2e_sessions at ~1387, build_session_lock_keys at ~1406, and prepare_dm_stanza which calls get_lid_for_phone). If set_force_pn_addressing is toggled concurrently during a send, different stages of the same send may observe opposite values. This recreates the namespace/session-lock divergence the PR explicitly tries to avoid. Snap-shotting the flag once at the start of each send and passing that decision through all four call sites would actually preserve the invariant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/context_impl.rs, line 51:
<comment>The four outbound `force_pn_addressing` gate points read the flag independently with `Ordering::Relaxed`, and there are `.await` suspension points between them in `send_message_impl` (e.g., between the `recipient_bare` computation at line ~1175, `ensure_e2e_sessions` at ~1387, `build_session_lock_keys` at ~1406, and `prepare_dm_stanza` which calls `get_lid_for_phone`). If `set_force_pn_addressing` is toggled concurrently during a send, different stages of the same send may observe opposite values. This recreates the namespace/session-lock divergence the PR explicitly tries to avoid. Snap-shotting the flag once at the start of each send and passing that decision through all four call sites would actually preserve the invariant.</comment>
<file context>
@@ -46,6 +46,11 @@ impl SendContextResolver for Client {
async fn get_lid_for_phone(&self, phone_user: &str) -> Option<String> {
+ // Reporting no mapping keeps stanza prep (participants, enc nodes) on
+ // the PN namespace (see Client::set_force_pn_addressing).
+ if self.force_pn_addressing_enabled() {
+ return None;
+ }
</file context>
| async fn get_lid_for_phone(&self, phone_user: &str) -> Option<String> { | ||
| // Reporting no mapping keeps stanza prep (participants, enc nodes) on | ||
| // the PN namespace (see Client::set_force_pn_addressing). | ||
| if self.force_pn_addressing_enabled() { |
There was a problem hiding this comment.
P2: The force_pn_addressing_enabled gate in get_lid_for_phone is applied unconditionally inside the shared SendContextResolver implementation, which is consumed by encrypt_for_devices for any list of device JIDs. Because the method receives only a phone_user string with no send-context (e.g., DM vs. group vs. broadcast), enabling the flag will suppress LID resolution for every outbound encrypted send, not just direct messages. The PR describes this as a DM workaround, so the scope should be verified: if group or broadcast flows still require LID addressing, this global suppression could emit PN-addressed enc nodes in contexts that expect LID. Consider scoping the gate to DM-only sends or confirming that all callers of get_lid_for_phone are safe to receive PN addressing when the flag is on.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/context_impl.rs, line 51:
<comment>The `force_pn_addressing_enabled` gate in `get_lid_for_phone` is applied unconditionally inside the shared `SendContextResolver` implementation, which is consumed by `encrypt_for_devices` for any list of device JIDs. Because the method receives only a `phone_user` string with no send-context (e.g., DM vs. group vs. broadcast), enabling the flag will suppress LID resolution for every outbound encrypted send, not just direct messages. The PR describes this as a DM workaround, so the scope should be verified: if group or broadcast flows still require LID addressing, this global suppression could emit PN-addressed `enc` nodes in contexts that expect LID. Consider scoping the gate to DM-only sends or confirming that all callers of `get_lid_for_phone` are safe to receive PN addressing when the flag is on.</comment>
<file context>
@@ -46,6 +46,11 @@ impl SendContextResolver for Client {
async fn get_lid_for_phone(&self, phone_user: &str) -> Option<String> {
+ // Reporting no mapping keeps stanza prep (participants, enc nodes) on
+ // the PN namespace (see Client::set_force_pn_addressing).
+ if self.force_pn_addressing_enabled() {
+ return None;
+ }
</file context>
|
Update — new evidence, and rebase incoming. I cherry-picked #731 onto v0.6.0 plus this branch's flag to isolate variables, and re-tested on the affected companion registration with the ack logging #731 added:
This PR currently conflicts with |
1 participant
Summary
Adds an opt-in
Client::set_force_pn_addressing(true)that keeps outbound DM addressing on the namespace the caller provided instead of upgrading PN → LID. The flag is scoped to the four addressing decisions the outbound path makes — recipient fanout keys (send_message_impl), session creation (resolve_lid_mappingsviaensure_e2e_sessions), stanza prep (SendContextResolver::get_lid_for_phone) and session lock keys (build_session_lock_keys, kept aligned with the namespace encryption actually uses so the per-session lock invariant against concurrent inbound decrypt holds) — while inbound decrypt addressing (resolve_encryption_jidon sender JIDs), LID↔PN mapping learning, and inbound session migration stay untouched. Mirrors the existingskip_history_syncruntime flag pattern.Motivation
On at least one companion registration (personal account, client linked as a companion device), DMs sent with LID addressing are accepted by the server but never delivered: no
Deliveredreceipt ever arrives and the recipient never sees the message. The same message sent with pre-0.6 PN addressing is delivered within ~2 seconds (verified side by side against 0.5.0).With 0.6's unified addressing there is currently no way to work around this from the caller side:
send_messagedoesn't help, because the DM path re-upgrades it internally as soon as the LID mapping is in the cache, andTimeline observed live (numbers redacted):
send_message(pn_jid)→ delivered ✔send_message(pn_jid)→ accepted by server, never delivered, no receipt ✘I don't know how widespread this server behavior is (possibly related to account type or registration age), so an opt-in escape hatch seemed like the minimal-surface fix rather than changing default behavior. Happy to rework into a
BotBuilderoption or config field if you prefer.Why the flag gates four call sites instead of just
resolve_encryption_jidA first iteration gated only
resolve_encryption_jid. That turned out to be both insufficient —prepare_dm_stanza/encrypt_for_devicesupgrade PN → LID through a separate path (SendContextResolver::get_lid_for_phone), so participants and enc nodes still went out LID-addressed — and unsafe: session lock keys would have diverged from the namespace encryption actually used, breaking the documented per-session lock invariant (concurrent outbound encrypt and inbound decrypt of the same peer would no longer serialize on the same mutex). The final shape keeps every outbound addressing decision consistent on PN while leaving the inbound half of the pipeline stock.Validation
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings(0 warnings),cargo test --workspace --exclude e2e-tests(all suites green) — same commands as theRust CIworkflow.test_force_pn_addressing_scopes_to_outbound_pathsasserting the outbound paths keep PN with the flag on, that inboundresolve_encryption_jidis deliberately NOT gated, and that turning the flag off restores stock behavior.Deliveredreceipts within ~2s, including full inbound → reply round-trips; without it, silent drops as described above.Default behavior is unchanged — the flag is off unless a caller opts in.