fix(send): gate DM LID wire addressing on the account's 1:1 migration state - #943
Merged
Conversation
… state The server 400-nacks LID-addressed DMs from accounts that are not 1:1-LID-migrated, and the send path upgraded PN to LID unconditionally whenever a mapping was cached, so every DM after the first was dropped on such accounts (#941). WA Web only addresses 1:1 wire traffic by LID once Lid1X1MigrationUtils.isLidMigrated(); the Signal session layer stays LID-first regardless (WAWebSignalAddress). Mirror that split: - persist a lid_migrated device flag, set from the pair-success client-props (ClientPairingProps.isChatDbLidMigrated) and from the primary's lid_migration_mapping_sync protocol message (self-only, applied once the lid_one_on_one_migration_enabled ab prop allows it) - for sessions paired before the flag existed, fall back to that same ab prop, matching how WA Web migrates already-linked clients - resolve the DM wire namespace through the new gate; an unmigrated account keeps 1:1 chats on PN even with a cached mapping, including mapping a caller-supplied LID back to the PN chat Fixes #941
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds persisted LID migration state, learns it from pairing and sync paths, and uses it to choose PN or LID DM wire addressing. ChangesLID migration state and DM wire addressing
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/send/mod.rs (1)
1693-1696: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWarm the same namespace that the send path reads.
After
resolve_dm_wire_jid(),recipient_baremay be PN while the caller passed a LID. The miss path still callsget_user_devices(&to), then re-readsget_devices_from_registry(&recipient_bare), so a cold PN registry can stay cold and fall back to a bare-JID fanout.Suggested fix
let mut recipient_cached = self.get_devices_from_registry(&recipient_bare).await; if recipient_cached.is_none() { - let _ = self.get_user_devices(std::slice::from_ref(&to)).await; + let warm_target = if recipient_bare.is_pn() { + recipient_bare.clone() + } else { + to.to_non_ad() + }; + let _ = self.get_user_devices(std::slice::from_ref(&warm_target)).await; recipient_cached = self.get_devices_from_registry(&recipient_bare).await; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/send/mod.rs` around lines 1693 - 1696, The cache warm-up in the send path is using the wrong namespace: after resolve_dm_wire_jid(), recipient_bare may differ from the caller’s to value, so get_user_devices(std::slice::from_ref(&to)) warms one key while get_devices_from_registry(&recipient_bare) reads another. Update the miss path in send/mod.rs to warm the same resolved recipient namespace that the send logic later checks, using recipient_bare consistently in the get_user_devices call and the subsequent registry re-read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/lid_pn.rs`:
- Around line 440-463: The migration sync in `lid_pn.rs` can persist invalid
`"0"` PN/LID pairs because protobuf scalar fields may decode to zero when
absent; update the loop in the `payload.pn_to_lid_mappings` handling to skip any
entry where `mapping.pn == 0` or the resolved `lid` is zero before calling
`add_lid_pn_mapping()`. Keep the existing `MigrationSyncLatest` persistence flow
and the `SetLidMigrated` flag logic, but ensure zero-value mappings are filtered
out so only valid pairs are written and the migrated state is not flipped based
on empty data.
In `@src/pair.rs`:
- Around line 273-284: Reset the stored lid_migrated flag when unlinking or
before starting a new pairing so a reused device row cannot carry stale LID
state into the next account. Update the pairing/logout flow around
PairUtils::extract_pairing_props, DeviceCommand::SetLidMigrated, and logout() so
the persistence layer clears or recreates the account row before the next
pair-success is processed, ensuring a false pair-success does not inherit a
previous true value.
In `@src/send/mod.rs`:
- Around line 2015-2020: The dm_stanza_to() helper is preserving a
device-qualified PN target when the caller passes a device JID, so normalize the
returned outer stanza target to the bare chat JID for PN→PN paths. Update
dm_stanza_to() to strip any device qualifier from the selected JID (including
the current to.clone() branch) while still keeping the existing LID behavior
based on recipient_bare.is_lid() and to.is_lid().
---
Outside diff comments:
In `@src/send/mod.rs`:
- Around line 1693-1696: The cache warm-up in the send path is using the wrong
namespace: after resolve_dm_wire_jid(), recipient_bare may differ from the
caller’s to value, so get_user_devices(std::slice::from_ref(&to)) warms one key
while get_devices_from_registry(&recipient_bare) reads another. Update the miss
path in send/mod.rs to warm the same resolved recipient namespace that the send
logic later checks, using recipient_bare consistently in the get_user_devices
call and the subsequent registry re-read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0eef1147-fecc-4b0f-813e-f1fc09aeeb07
📒 Files selected for processing (13)
src/client/lid_pn.rssrc/message/receive.rssrc/message/tests.rssrc/pair.rssrc/send/mod.rsstorages/sqlite-storage/migrations/2026-07-02-000000_add_lid_migrated/down.sqlstorages/sqlite-storage/migrations/2026-07-02-000000_add_lid_migrated/up.sqlstorages/sqlite-storage/src/schema.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/iq/props.rswacore/src/pair.rswacore/src/store/commands.rswacore/src/store/device.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 13 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="storages/sqlite-storage/src/sqlite_store.rs">
<violation number="1" location="storages/sqlite-storage/src/sqlite_store.rs:566">
P2: The persisted LID migration flag can revert to false on an upsert, which breaks the one-way account migration state and can send later DMs with PN addressing again. The conflict update should preserve an existing true value instead of replacing it with `excluded(device::lid_migrated)`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ings Review follow-ups on #943: a fresh pairing now always writes the migration state from the pair-success client-props, so reusing a device store for a different account cannot inherit a stale migrated flag; and migration-sync entries whose pn or lid decode as 0 (absent scalar fields) are skipped instead of poisoning the LID-PN cache.
…anza_to Review follow-ups on #943: the primary's migration mapping push now goes through learn_lid_pn_mappings_batch (one cache pass plus a single backend transaction instead of one awaited persist per entry), and dm_stanza_to normalizes a device-qualified caller jid to the bare chat jid, matching WA Web's CHAT_JID which is always a bare user wid.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="storages/sqlite-storage/src/sqlite_store.rs">
<violation number="1" location="storages/sqlite-storage/src/sqlite_store.rs:566">
P2: The persisted LID migration flag can revert to false on an upsert, which breaks the one-way account migration state and can send later DMs with PN addressing again. The conflict update should preserve an existing true value instead of replacing it with `excluded(device::lid_migrated)`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Review follow-up on #943: the mapping-sync handler now awaits the batched persist (extracted record_lid_pn_batch_in_memory keeps the single-transaction path shared with the fire-and-forget learn), so a crash between the two writes cannot leave an account durably marked migrated without its mapping rows.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/send/mod.rs (1)
1683-1696: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWarm the device registry using the resolved wire JID.
recipient_bareis now the authoritative wire namespace, but on a cache miss the warm-up still queriesto. For LID→PN downgrade or PN→LID upgrade paths, that can populate the wrong namespace and still leaveget_devices_from_registry(&recipient_bare)empty.Suggested fix
- let _ = self.get_user_devices(std::slice::from_ref(&to)).await; + let _ = self + .get_user_devices(std::slice::from_ref(&recipient_bare)) + .await; recipient_cached = self.get_devices_from_registry(&recipient_bare).await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/send/mod.rs` around lines 1683 - 1696, The cache warm-up in the DM send flow is still using the original `to` JID instead of the resolved wire JID. Update the warm-miss path in `src/send/mod.rs` so `get_user_devices` is called with `recipient_bare` (the value returned by `resolve_dm_wire_jid`) rather than `to`, keeping the namespace consistent with the later `get_devices_from_registry(&recipient_bare)` lookup.src/client/lid_pn.rs (1)
469-487: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t flip
lid_migratedafter mapping persistence fails.Right now a failed
persist_and_migrate_lid_pn_batch()only logs, then the code can still persistSetLidMigrated(true). That can switch DM wire addressing to LID after a restart without durable PN↔LID rows.Suggested fix
- if !entries.is_empty() - && let Err(e) = self - .persist_and_migrate_lid_pn_batch(entries, is_new_flags) - .await - { - log::warn!("Failed to persist migration mappings: {e:?}"); + if !entries.is_empty() + && let Err(e) = self + .persist_and_migrate_lid_pn_batch(entries, is_new_flags) + .await + { + log::warn!("Failed to persist migration mappings: {e:?}"); + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/lid_pn.rs` around lines 469 - 487, The logic in `lid_pn.rs` should not mark the device as migrated when `persist_and_migrate_lid_pn_batch()` fails. In the block that handles the `entries` batch and the subsequent `SetLidMigrated(true)` path, make the migration flag update conditional on successful persistence (for example, only proceed to `process_command(DeviceCommand::SetLidMigrated(true))` when the batch call succeeds, or return early on error). Use the existing `persist_and_migrate_lid_pn_batch`, `persistence_manager.get_device_snapshot()`, and `process_command` flow to ensure `lid_migrated` is never flipped after a failed mapping write.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/lid_pn.rs`:
- Around line 469-487: The logic in `lid_pn.rs` should not mark the device as
migrated when `persist_and_migrate_lid_pn_batch()` fails. In the block that
handles the `entries` batch and the subsequent `SetLidMigrated(true)` path, make
the migration flag update conditional on successful persistence (for example,
only proceed to `process_command(DeviceCommand::SetLidMigrated(true))` when the
batch call succeeds, or return early on error). Use the existing
`persist_and_migrate_lid_pn_batch`, `persistence_manager.get_device_snapshot()`,
and `process_command` flow to ensure `lid_migrated` is never flipped after a
failed mapping write.
In `@src/send/mod.rs`:
- Around line 1683-1696: The cache warm-up in the DM send flow is still using
the original `to` JID instead of the resolved wire JID. Update the warm-miss
path in `src/send/mod.rs` so `get_user_devices` is called with `recipient_bare`
(the value returned by `resolve_dm_wire_jid`) rather than `to`, keeping the
namespace consistent with the later `get_devices_from_registry(&recipient_bare)`
lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d9bf62d-158e-43eb-b180-fbeda066f062
📒 Files selected for processing (2)
src/client/lid_pn.rssrc/send/mod.rs
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Review follow-up on #943: a failed batch save no longer advances the migration state, closing the same flag-without-mappings inconsistency through the error path that the previous commit closed for crashes. Addressing stays correct through the ab prop until the mappings are re-learned.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/lid_pn.rs (1)
776-777: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse clearly reserved fictional PNs in tests.
These test PNs look like dialable E.164-style numbers. Please switch them to reserved fictional phone numbers and keep the string and protobuf numeric values aligned. As per coding guidelines, “Use fictitious phone numbers and JIDs in test code; never commit real user numbers (no real PII in tests)”.
Also applies to: 805-806, 827-828, 861-863, 876-879
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/lid_pn.rs` around lines 776 - 777, Replace the test phone numbers with reserved fictional values instead of E.164-looking real numbers, and keep each string PN paired with the matching protobuf numeric value. Update the PN/LID fixtures used in this test area (including the repeated pn/lid assignments around the cited blocks) so they use clearly fictitious data consistently across all assertions and serialization checks.Source: Coding guidelines
♻️ Duplicate comments (1)
src/client/lid_pn.rs (1)
447-481: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t mark the account migrated when the sync has no valid mappings.
After zero PN/LID pairs are filtered,
entriescan be empty because every mapping was invalid, but Line 481 can still persistSetLidMigrated(true)when the AB prop is on. Track whether the decoded payload contained at least one valid mapping and gate the flag on that; durable duplicate pushes still work because the payload was valid even ifentriesis empty.Proposed fix
let mappings: Vec<(String, String)> = payload .pn_to_lid_mappings .iter() .filter_map(|mapping| { let lid = mapping.latest_lid.unwrap_or(mapping.assigned_lid); // Absent scalar fields decode as 0; a "0" user would poison the cache. if mapping.pn == 0 || lid == 0 { log::warn!("Skipping migration mapping with zero pn/lid"); return None; } Some((lid.to_string(), mapping.pn.to_string())) }) .collect(); + let has_valid_mappings = !mappings.is_empty(); + // Awaited (unlike the fire-and-forget learn path) so the mappings are // durable before the migrated flag below is; a crash in between must // not leave a migrated account without its mapping rows. let (entries, is_new_flags) = self .record_lid_pn_batch_in_memory( @@ - if !self.persistence_manager.get_device_snapshot().lid_migrated + if has_valid_mappings + && !self.persistence_manager.get_device_snapshot().lid_migrated && self .ab_props() .is_enabled(wacore::iq::abprops::web::LID_ONE_ON_ONE_MIGRATION_ENABLED) .await🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/lid_pn.rs` around lines 447 - 481, The migration sync in `record_lid_pn_batch_in_memory`/`persist_and_migrate_lid_pn_batch` can still set `SetLidMigrated(true)` even when all decoded PN/LID mappings were filtered out, so add a validity check for the payload before advancing migration state. Track whether `payload.pn_to_lid_mappings` produced at least one non-zero mapping while building `mappings`, and only allow the `lid_migrated` update in the `self.persistence_manager.get_device_snapshot().lid_migrated` path when that flag is true; keep the existing persistence failure early-return behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/lid_pn.rs`:
- Around line 776-777: Replace the test phone numbers with reserved fictional
values instead of E.164-looking real numbers, and keep each string PN paired
with the matching protobuf numeric value. Update the PN/LID fixtures used in
this test area (including the repeated pn/lid assignments around the cited
blocks) so they use clearly fictitious data consistently across all assertions
and serialization checks.
---
Duplicate comments:
In `@src/client/lid_pn.rs`:
- Around line 447-481: The migration sync in
`record_lid_pn_batch_in_memory`/`persist_and_migrate_lid_pn_batch` can still set
`SetLidMigrated(true)` even when all decoded PN/LID mappings were filtered out,
so add a validity check for the payload before advancing migration state. Track
whether `payload.pn_to_lid_mappings` produced at least one non-zero mapping
while building `mappings`, and only allow the `lid_migrated` update in the
`self.persistence_manager.get_device_snapshot().lid_migrated` path when that
flag is true; keep the existing persistence failure early-return behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dddc29b7-1500-41ee-8e79-c5c5249a837e
📒 Files selected for processing (1)
src/client/lid_pn.rs
…uard pair reset Adversarial-review follow-ups on #943: - persist SetLidMigrated(true) whenever a props fetch observes lid_one_on_one_migration_enabled on; the props cache is not persisted, so without the latch a prop-only-migrated account re-entered PN wire addressing on every process start until the fetch landed, flapping the DM namespace - the mapping-sync handler now awaits only the batch persist and defers the per-mapping registry/session migrations to a detached task; they walk up to MIGRATION_DEVICE_RANGE locks per mapping and ran under the message pipeline's processing permit, which is a single permit during offline sync, exactly when a replayed migration push is likely - pair-success only resets lid_migrated when a different account is being paired onto the store; a same-account relink whose pair-success omitted client-props (the child is optional) no longer loses the flag, matching WA Web's HandlePairSuccess which never lowers the pref - an explicit latest_lid of 0 now falls back to assigned_lid instead of dropping the mapping, and resolve_encryption_jid's doc points wire addressing at resolve_dm_wire_jid
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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/lid_pn.rs">
<violation number="1" location="src/client/lid_pn.rs:530">
P2: `handle_lid_migration_mapping_sync` now sets the `lid_migrated` flag after awaiting persistence but without awaiting the detached `migrate_lid_pn_batch` task. If the process shuts down after the flag is persisted but before the detached task completes, the account is durably marked as migrated while device-registry and Signal-session migrations were never applied.
Because `warm_up_lid_pn_cache` does not re-run migrations and `record_lid_pn_batch_in_memory` skips already-persisted mappings via `can_skip_relearn`, there is no guaranteed retry path for the missed migration work on next startup. Although Signal sessions have a lazy fallback (`try_pn_to_lid_migration_decrypt` on first decrypt), there is no equivalent eager or startup recovery for `migrate_device_registry_on_lid_discovery`.
Consider either moving the migrated-flag persistence inside the spawned task (so it is only set after migrations complete) or adding a startup scan that detects persisted but un-migrated mappings and finishes the registry/session migration before the client begins sending.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A shutdown can drop the detached migration task after the mappings and the migrated flag are durable; both migration halves self-heal lazily (decrypt-side session migration, registry re-warm on the next send), so the window is accepted rather than retried.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/lid_pn.rs (1)
495-563: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t latch migration state from an empty valid-mapping set.
After zero PN/LID entries are filtered out,
mappingscan be empty but Lines 553-562 can still persistlid_migrated=true. That can make a malformed-but-decodable sync permanently advance DM wire addressing without any durable PN↔LID rows.Proposed fix
let mappings: Vec<(String, String)> = payload .pn_to_lid_mappings .iter() .filter_map(|mapping| { @@ Some((lid.to_string(), mapping.pn.to_string())) }) .collect(); + if mappings.is_empty() { + log::warn!("lid_migration_mapping_sync contained no valid PN-LID mappings"); + return; + } // The persist is awaited (unlike the fire-and-forget learn path) so🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/lid_pn.rs` around lines 495 - 563, In the migration sync flow in the handler that builds `mappings`, avoid setting `DeviceCommand::SetLidMigrated(true)` when all decoded PN/LID entries were filtered out and `mappings` is empty. Add a guard around the `lid_migrated` update so it only happens after at least one valid mapping was actually accepted and persisted through `persist_lid_pn_batch`. Use the `record_lid_pn_batch_in_memory`, `persist_lid_pn_batch`, and `self.persistence_manager.process_command` paths to keep the migrated flag consistent with durable PN↔LID rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/lid_pn.rs`:
- Around line 804-808: The new test fixture currently uses a phone number that
resembles a real E.164 user number; update the PN/LID-PN test data to use
clearly fictitious reserved values instead. In client_with_peer_mapping and the
related test cases around the cited assertions, replace 5511987650001 with a
test-only number such as 15555550101 and keep all fixture, payload, and
expected-value strings consistent across the affected tests. Ensure any other
new PN/JID literals in lid_pn.rs follow the same pattern of non-real, reserved
test identifiers.
---
Duplicate comments:
In `@src/client/lid_pn.rs`:
- Around line 495-563: In the migration sync flow in the handler that builds
`mappings`, avoid setting `DeviceCommand::SetLidMigrated(true)` when all decoded
PN/LID entries were filtered out and `mappings` is empty. Add a guard around the
`lid_migrated` update so it only happens after at least one valid mapping was
actually accepted and persisted through `persist_lid_pn_batch`. Use the
`record_lid_pn_batch_in_memory`, `persist_lid_pn_batch`, and
`self.persistence_manager.process_command` paths to keep the migrated flag
consistent with durable PN↔LID rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9e6f159-3c3f-44b2-8eb2-d7931e17a50b
📒 Files selected for processing (4)
src/client/iq_ops.rssrc/client/lid_pn.rssrc/pair.rswacore/src/pair.rs
jlucaso1
added a commit
that referenced
this pull request
Jul 2, 2026
Resolve the pair.rs test-module conflict by keeping both sides' tests (buffa's do_pair_crypto_rejects_missing_key_index and main's #943 extract_pairing_props / lid_migrated_update tests). Adapt the merged #943 code to the buffa API: - prost PascalCase-normalizes LIDMigration* to LidMigration*, buffa keeps the proto name verbatim, so the types stay LIDMigrationMapping, LIDMigrationMappingSyncMessage, LIDMigrationMappingSyncPayload. - ClientPairingProps::decode and LIDMigrationMappingSyncPayload::decode become decode_from_slice; use prost::Message becomes use buffa::Message. - the optional-bool getter is_chat_db_lid_migrated() becomes the field accessed as .unwrap_or(false). - protocol_message and lid_migration_mapping_sync_message are MessageField, read via .as_option() and built via MessageField::some(..) not Some(Box::new).
Merged
7 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
1 participant
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.
Fixes #941.
Problem
On some companion registrations, every DM sent with LID addressing is rejected by the server with
ack error="400"and never delivered, while the same message with PN addressing is delivered normally. The failure pattern reported in #941 (first DM to a fresh PN goes through, every subsequent one is dropped) is exactly the unconditional PN-to-LID upgrade: the first send learns the mapping via usync, and from then on every DM is addressed to@lid.The ab props dump from the affected account confirmed the root cause: nearly every LID flag is enabled there except
lid_one_on_one_migration_enabled(code 9435, default false, absent from the dump). The account is not 1:1-LID-migrated, and the server refuses LID-addressed DMs from unmigrated accounts.What WA Web does (docs/captured-js)
The DM wire namespace is an account-level decision, not a per-peer one:
WAWebSendMsgCreateFanoutStanzabuilds the whole DM stanza from the chat wid, and the chat wid is LID only whenLid1X1MigrationUtils.isLidMigrated()is true (WAWebLid1X1MigrationGating,WAWebMessageDestinationChat).ClientPairingProps.isChatDbLidMigratedin the<client-props>child ofpair-success(WAWebHandlePairSuccess), or on an already-linked client when the primary pushesProtocolMessage.lidMigrationSyncMessageand thelid_one_on_one_migration_enabledab prop lets the migration state machine proceed (WAWebLid1X1ThreadAccountMigrations). Once set it never reverts.WAWebSignalAddress.toString()upgrades PN to LID unconditionally whenever a mapping is known.@lidin the first place.Our lib was missing the gate entirely:
resolve_encryption_jidupgraded PN to LID for the wire whenever a mapping was cached.Changes
Device::lid_migratedflag (one-way, like the WA Web pref), with aSetLidMigrateddevice command and a sqlite migration.pair-successnow decodes the<client-props>protobuf and persists the flag whenisChatDbLidMigratedis true, so new pairings get the account state directly from the primary.ProtocolMessage.lid_migration_mapping_sync_message: learns the pushed PN-LID mappings and persists the flag once thelid_one_on_one_migration_enabledab prop allows it, mirroring the WA Web state machine parking at WAITING_PROP.Client::is_lid_migrated(): persisted flag OR thelid_one_on_one_migration_enabledab prop. The prop fallback covers sessions paired before this flag existed, which is also how WA Web migrates already-linked clients. Healthy accounts (prop on) keep today's LID addressing with no behavior change; unmigrated accounts automatically fall back to PN.resolve_dm_wire_jid: a migrated account upgrades PN to LID as before; an unmigrated account keeps 1:1 chats on PN even with a cached mapping, including mapping a caller-supplied LID back to the PN chat. Thestanza_todecision moved into a puredm_stanza_tohelper so the mixed-namespace guard is unit-tested.resolve_encryption_jid), inbound decrypt, mapping learning and session migration are deliberately untouched, matching the WA Web split above.Overhead on the send hot path is one cached device-snapshot read plus one hashmap lookup in the ab props cache per DM, only the watched prop is retained from the props fetch.
Relation to #942
This makes the addressing decision automatic from server-provided state, which is the direction discussed in #941/#942 as the alternative to a manual flag. The escape hatch in #942 becomes unnecessary for the reported deployment, though it can still be layered on top if a manual override is wanted.
Validation
cargo fmt --all,cargo clippy --all --testsandcargo clippy --all-targets -- -D warningsclean,cargo test --workspace --exclude e2e-testsall green.dm_from_unmigrated_account_addresses_outer_to_by_pncaptures the actual outbound stanza and asserts the outertoand every participant stay PN on an unmigrated account, while the Signal session underneath is the LID one. The existingsend_messageto a LID-mapped peer is rejected with ACK error 400 whentois the PN form #730 regression test now sets the migrated flag and still asserts uniform LID addressing.lid_one_on_one_migration_enabledis on for currently-working deployments would also be good confirmation.