fix(send): close LID↔PN zombie path for group prekey 406 latency spikes

src/client/device_registry.rs

@@ -168,6 +168,18 @@ impl Client {
             .context("Failed to update device list in backend")?;
 
         if canonical_key != original_user {
+            // Invalidate before + after delete so a concurrent reader that
+            // resurrects the cache from the about-to-be-deleted DB row still
+            // gets cleared. Run the second invalidate unconditionally: even
+            // if delete fails, the cache may have been repopulated with data
+            // that no longer reflects our intent.
+            self.device_registry_cache.invalidate(&original_user).await;
+            if let Err(e) = backend.delete_devices(&original_user).await {
+                warn!(
+                    "Failed to delete stale device row under {} after canonical flip: {e}",
+                    original_user
+                );
+            }
             self.device_registry_cache.invalidate(&original_user).await;
             debug!(
                 "Device registry: stored under LID {} (resolved from {})",
@@ -528,7 +540,15 @@ impl Client {
                     .insert(lid.to_string(), record)
                     .await;
 
-                // Clean up stale PN-keyed entry without touching the fresh LID entry.
+                // Drop the PN-keyed row in both cache and DB. Invalidate
+                // twice (before + after delete) so a concurrent reader can't
+                // resurrect the cache from the DB row between the two calls.
+                // Always run the second invalidate; even if delete fails, the
+                // cache may carry resurrected data that shouldn't stick.
+                self.device_registry_cache.invalidate(pn).await;
+                if let Err(e) = backend.delete_devices(pn).await {
+                    warn!("Failed to delete PN-keyed device row during LID migration: {e}");
+                }
                 self.device_registry_cache.invalidate(pn).await;
             }
             Ok(None) => {}
@@ -1214,4 +1234,207 @@ mod tests {
             "sender key cache should have been invalidated after device removal"
         );
     }
+
+    // ── LID↔PN zombie-path regression tests (PR #579) ───────────────────
+
+    /// U1 — `update_device_list` deletes the stale DB row when the canonical
+    /// key flips (e.g. the LID↔PN mapping is learned between two writes).
+    /// Without this, the old PN-keyed row lingers and re-surfaces as a zombie
+    /// through alias lookup, causing 406s on group sends.
+    #[tokio::test]
+    async fn test_update_device_list_canonical_flip_deletes_old_db_row() {
+        use wacore::store::traits::{DeviceInfo, DeviceListRecord};
+
+        let client = create_test_client().await;
+        let pn = "15550000011";
+        let lid = "100000000000011";
+        let backend = client.persistence_manager.backend();
+
+        // Legacy state: DB row stored under PN (mapping wasn't known yet).
+        backend
+            .update_device_list(DeviceListRecord {
+                user: pn.to_string(),
+                devices: vec![DeviceInfo {
+                    device_id: 5,
+                    key_index: None,
+                }],
+                timestamp: wacore::time::now_secs(),
+                phash: None,
+                raw_id: None,
+            })
+            .await
+            .unwrap();
+
+        setup_lid_pn(&client, lid, pn).await;
+
+        // New write: `update_device_list` with original_user = PN, canonical
+        // now resolves to LID because the mapping is known.
+        client
+            .update_device_list(DeviceListRecord {
+                user: pn.to_string(),
+                devices: vec![DeviceInfo {
+                    device_id: 7,
+                    key_index: None,
+                }],
+                timestamp: wacore::time::now_secs(),
+                phash: None,
+                raw_id: None,
+            })
+            .await
+            .unwrap();
+
+        assert!(
+            backend.get_devices(pn).await.unwrap().is_none(),
+            "old PN-keyed DB row must be deleted after canonical flip"
+        );
+        let lid_row = backend.get_devices(lid).await.unwrap();
+        assert!(lid_row.is_some(), "new LID-keyed DB row must exist");
+        assert_eq!(lid_row.unwrap().devices[0].device_id, 7);
+    }
+
+    /// U2 — `migrate_device_registry_on_lid_discovery` deletes the PN-keyed DB
+    /// row, not just the cache entry. Without this the PN row stayed around
+    /// as a zombie that surfaced via alias lookup on future sends.
+    #[tokio::test]
+    async fn test_migrate_device_registry_deletes_pn_db_row() {
+        use wacore::store::traits::{DeviceInfo, DeviceListRecord};
+
+        let client = create_test_client().await;
+        let pn = "15550000022";
+        let lid = "100000000000022";
+        let backend = client.persistence_manager.backend();
+
+        backend
+            .update_device_list(DeviceListRecord {
+                user: pn.to_string(),
+                devices: vec![DeviceInfo {
+                    device_id: 0,
+                    key_index: None,
+                }],
+                timestamp: wacore::time::now_secs(),
+                phash: None,
+                raw_id: None,
+            })
+            .await
+            .unwrap();
+
+        setup_lid_pn(&client, lid, pn).await;
+
+        client
+            .migrate_device_registry_on_lid_discovery(pn, lid)
+            .await;
+
+        assert!(
+            backend.get_devices(pn).await.unwrap().is_none(),
+            "PN-keyed DB row must be gone after migration"
+        );
+        assert!(
+            backend.get_devices(lid).await.unwrap().is_some(),
+            "LID-keyed DB row must exist after migration"
+        );
+    }
+
+    /// U3 — `invalidate_device_cache` with a known LID↔PN mapping clears both
+    /// aliases from the DB (not only the cache). This is the primary fix for
+    /// the 23-batches-in-3h45m zombie loop from the field report.
+    #[tokio::test]
+    async fn test_invalidate_device_cache_clears_both_aliases_from_db() {
+        use wacore::store::traits::{DeviceInfo, DeviceListRecord};
+
+        let client = create_test_client().await;
+        let pn = "15550000033";
+        let lid = "100000000000033";
+        let backend = client.persistence_manager.backend();
+
+        // Seed DB under BOTH aliases (simulating split-brain legacy state).
+        for user in [pn, lid] {
+            backend
+                .update_device_list(DeviceListRecord {
+                    user: user.to_string(),
+                    devices: vec![DeviceInfo {
+                        device_id: 1,
+                        key_index: None,
+                    }],
+                    timestamp: wacore::time::now_secs(),
+                    phash: None,
+                    raw_id: None,
+                })
+                .await
+                .unwrap();
+        }
+        setup_lid_pn(&client, lid, pn).await;
+
+        client.invalidate_device_cache(lid).await;
+
+        assert!(
+            backend.get_devices(pn).await.unwrap().is_none(),
+            "PN DB row must be deleted via alias resolution"
+        );
+        assert!(
+            backend.get_devices(lid).await.unwrap().is_none(),
+            "LID DB row must be deleted"
+        );
+        assert!(
+            client.device_registry_cache.get(pn).await.is_none(),
+            "PN cache entry must be gone"
+        );
+        assert!(
+            client.device_registry_cache.get(lid).await.is_none(),
+            "LID cache entry must be gone"
+        );
+    }
+
+    /// U4 — TOCTOU regression: even if the cache got repopulated between the
+    /// pre-delete `invalidate` and the DB `delete_devices`, the post-delete
+    /// `invalidate` wipes it out. Simulated by pre-seeding both cache and DB
+    /// under PN, then running the canonical-flip write.
+    #[tokio::test]
+    async fn test_update_device_list_toctou_second_invalidate_clears_resurrected_cache() {
+        use wacore::store::traits::{DeviceInfo, DeviceListRecord};
+
+        let client = create_test_client().await;
+        let pn = "15550000044";
+        let lid = "100000000000044";
+        let backend = client.persistence_manager.backend();
+
+        let legacy = DeviceListRecord {
+            user: pn.to_string(),
+            devices: vec![DeviceInfo {
+                device_id: 9,
+                key_index: None,
+            }],
+            timestamp: wacore::time::now_secs(),
+            phash: None,
+            raw_id: None,
+        };
+        backend.update_device_list(legacy.clone()).await.unwrap();
+        // Pre-populate cache[PN] directly to emulate a concurrent reader that
+        // loaded from the DB row between the two invalidate calls.
+        client.device_registry_cache.insert(pn.into(), legacy).await;
+
+        setup_lid_pn(&client, lid, pn).await;
+
+        client
+            .update_device_list(DeviceListRecord {
+                user: pn.to_string(),
+                devices: vec![DeviceInfo {
+                    device_id: 10,
+                    key_index: None,
+                }],
+                timestamp: wacore::time::now_secs(),
+                phash: None,
+                raw_id: None,
+            })
+            .await
+            .unwrap();
+
+        assert!(
+            client.device_registry_cache.get(pn).await.is_none(),
+            "second invalidate must clear the pre-populated cache entry"
+        );
+        assert!(
+            backend.get_devices(pn).await.unwrap().is_none(),
+            "PN DB row must still be gone"
+        );
+    }
 }

src/features/groups.rs

@@ -209,6 +209,27 @@ impl<'a> Groups<'a> {
             participants.push(p.jid);
         }
 
+        // Populate lid_pn_cache so silent-observer participants (no messages
+        // from them) get their mapping; otherwise `invalidate_device_cache`
+        // can't resolve the PN alias and leaves zombie registry entries.
+        if !lid_to_pn_map.is_empty()
+            && let Some(client_arc) = self.client.self_weak.get().and_then(|w| w.upgrade())
+        {
+            for (lid_user, pn_jid) in &lid_to_pn_map {
+                if !pn_jid.is_pn() {
+                    continue;
+                }
+                client_arc
+                    .learn_lid_pn_mapping_fast(
+                        lid_user.as_str(),
+                        &pn_jid.user,
+                        crate::lid_pn_cache::LearningSource::Other,
+                        false,
+                    )
+                    .await;
+            }
+        }
+
         let mut info = GroupInfo::new(participants, group.addressing_mode);
         if !lid_to_pn_map.is_empty() {
             info.set_lid_to_pn_map(lid_to_pn_map);

tests/e2e/tests/groups.rs

@@ -713,3 +713,52 @@ async fn test_per_device_sender_key_tracking() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+/// E1 — regression test for PR #579 Fix 1: after `query_info` on an LID-mode
+/// group, each LID participant's PN must be present in `lid_pn_cache`.
+/// This closes the silent-observer zombie loop where `invalidate_device_cache`
+/// couldn't resolve a participant's PN alias because the mapping was never
+/// learned from a message (matches WA Web's `CreateOrReplaceDisplayNamesAndLidPnMappings`).
+#[tokio::test]
+async fn test_query_info_populates_lid_pn_cache_for_participants() -> anyhow::Result<()> {
+    let _ = env_logger::builder().is_test(true).try_init();
+
+    let client_a = TestClient::connect("e2e_grp_lidpn_a").await?;
+    let client_b = TestClient::connect("e2e_grp_lidpn_b").await?;
+
+    let jid_b_pn = client_b.jid().await;
+    let jid_b_lid = client_b
+        .client
+        .get_lid()
+        .await
+        .expect("B must have a LID after pairing")
+        .to_non_ad();
+    info!("B pn={jid_b_pn} lid={jid_b_lid}");
+
+    let group_jid = client_a
+        .client
+        .groups()
+        .create_group(GroupCreateOptions {
+            subject: "LID-PN mapping test".to_string(),
+            participants: vec![GroupParticipantOptions::new(jid_b_pn.clone())],
+            ..Default::default()
+        })
+        .await?
+        .gid;
+
+    // create_group doesn't populate the group cache, so the first query_info
+    // hits the network and runs the lid_pn_cache populate loop.
+    let _info = client_a.client.groups().query_info(&group_jid).await?;
+
+    let entry = client_a
+        .client
+        .get_lid_pn_entry(&jid_b_lid)
+        .await?
+        .expect("lid_pn_cache must have B's mapping after query_info");
+    assert_eq!(entry.lid, jid_b_lid.user);
+    assert_eq!(entry.phone_number, jid_b_pn.user);
+
+    client_a.disconnect().await;
+    client_b.disconnect().await;
+    Ok(())
+}