-
-
Notifications
You must be signed in to change notification settings - Fork 127
fix(groups): keep persisted group metadata in sync on membership change #761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
6f69026
c87a59a
8695ace
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -415,6 +415,17 @@ impl<'a> Groups<'a> { | |
| pub async fn leave(&self, jid: &Jid) -> Result<(), anyhow::Error> { | ||
| self.client.execute(LeaveGroupIq::new(jid)).await?; | ||
| self.client.get_group_cache().await.invalidate(jid).await; | ||
| // Drop the persisted blob too: we're no longer in the group, so a stale | ||
| // phash from it would only force a needless full re-query if ever read. | ||
| if let Err(e) = self | ||
| .client | ||
| .persistence_manager | ||
| .backend() | ||
| .delete_group_metadata(&jid.to_string()) | ||
| .await | ||
| { | ||
| log::warn!("Failed to delete persisted group metadata for {jid}: {e}"); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
|
|
@@ -446,7 +457,11 @@ impl<'a> Groups<'a> { | |
| .filter(|r| r.is_ok()) | ||
| .map(|r| (&r.jid, r.phone_number.as_ref())), | ||
| ); | ||
| self.client.persist_group_metadata(jid, &info).await; | ||
| group_cache.insert(jid.clone(), Arc::new(info)).await; | ||
| } else { | ||
| // Cache expired: can't patch in place, so drop the now-stale blob. | ||
| self.client.invalidate_persisted_group_metadata(jid).await; | ||
| } | ||
| } | ||
| Ok(result) | ||
|
|
@@ -471,7 +486,11 @@ impl<'a> Groups<'a> { | |
| if let Some(info) = group_cache.get(jid).await { | ||
| let mut info = Arc::unwrap_or_clone(info); | ||
| info.remove_participants(&accepted); | ||
| self.client.persist_group_metadata(jid, &info).await; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a participant removal succeeds while the group cache is warm, this new awaited persistence write runs before Useful? React with 👍 / 👎. |
||
| group_cache.insert(jid.clone(), Arc::new(info)).await; | ||
| } else { | ||
| // Cache expired: can't patch in place, so drop the now-stale blob. | ||
| self.client.invalidate_persisted_group_metadata(jid).await; | ||
| } | ||
| self.client | ||
| .rotate_sender_key_on_participant_remove(&jid.to_string(), &accepted) | ||
|
|
@@ -994,6 +1013,38 @@ impl Client { | |
| pub fn groups(&self) -> Groups<'_> { | ||
| Groups::new(self) | ||
| } | ||
|
|
||
| /// Re-serialize and persist a group's metadata after a local membership change | ||
| /// so the phash fast-path stays consistent: the in-memory cache expires after | ||
| /// ~1h, after which a stale persisted blob would force a needless full re-query | ||
| /// (or be compared against the server as an out-of-date phash). Shared by the | ||
| /// participant-mutation API and the inbound group-notification handler. | ||
| pub(crate) async fn persist_group_metadata(&self, jid: &Jid, info: &GroupInfo) { | ||
| let backend = self.persistence_manager.backend(); | ||
| match serde_json::to_vec(info) { | ||
| Ok(blob) => { | ||
| if let Err(e) = backend.put_group_metadata(&jid.to_string(), &blob).await { | ||
| log::warn!("Failed to persist group metadata for {jid}: {e}"); | ||
| } | ||
| } | ||
| Err(e) => log::warn!("Failed to serialize group metadata for {jid}: {e}"), | ||
| } | ||
| } | ||
|
|
||
| /// Drop the persisted group metadata on a membership change we can't patch in | ||
| /// place (the in-memory cache had already expired), so the next query re-fetches | ||
| /// fresh instead of comparing a now-stale phash. Without this, persisting only on | ||
| /// a cache hit would miss the exact post-expiry case this fix targets. | ||
| pub(crate) async fn invalidate_persisted_group_metadata(&self, jid: &Jid) { | ||
| if let Err(e) = self | ||
| .persistence_manager | ||
| .backend() | ||
| .delete_group_metadata(&jid.to_string()) | ||
| .await | ||
| { | ||
| log::warn!("Failed to invalidate persisted group metadata for {jid}: {e}"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Extract the invite code from any supported invite URL format. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1380,6 +1380,9 @@ async fn handle_group_notification(client: &Arc<Client>, node: Arc<OwnedNodeRef> | |
| .iter() | ||
| .map(|p| (&p.jid, p.phone_number.as_ref())), | ||
| ); | ||
| client | ||
| .persist_group_metadata(¬ification.group_jid, &info) | ||
| .await; | ||
|
Comment on lines
+1383
to
+1385
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a participant notification arrives after the in-memory group cache has expired (the default TTL is one hour) but a persisted metadata blob still exists, this block is skipped entirely, so the new Useful? React with 👍 / 👎. |
||
| group_cache | ||
| .insert(notification.group_jid.clone(), Arc::new(info)) | ||
| .await; | ||
|
|
@@ -1388,6 +1391,11 @@ async fn handle_group_notification(client: &Arc<Client>, node: Arc<OwnedNodeRef> | |
| "Patched group cache for {}: added {} participants", | ||
| notification.group_jid.observe(), participants.len() | ||
| ); | ||
| } else { | ||
| // Cache expired: can't patch in place, so drop the now-stale blob. | ||
| client | ||
| .invalidate_persisted_group_metadata(¬ification.group_jid) | ||
| .await; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| GroupNotificationAction::Remove { participants, .. } => { | ||
|
|
@@ -1396,6 +1404,9 @@ async fn handle_group_notification(client: &Arc<Client>, node: Arc<OwnedNodeRef> | |
| if let Some(info) = group_cache.get(¬ification.group_jid).await { | ||
| let mut info = Arc::unwrap_or_clone(info); | ||
| info.remove_participants(&users); | ||
| client | ||
| .persist_group_metadata(¬ification.group_jid, &info) | ||
| .await; | ||
| group_cache | ||
| .insert(notification.group_jid.clone(), Arc::new(info)) | ||
| .await; | ||
|
|
@@ -1404,6 +1415,11 @@ async fn handle_group_notification(client: &Arc<Client>, node: Arc<OwnedNodeRef> | |
| "Patched group cache for {}: removed {} participants", | ||
| notification.group_jid.observe(), participants.len() | ||
| ); | ||
| } else { | ||
| // Cache expired: can't patch in place, so drop the now-stale blob. | ||
| client | ||
| .invalidate_persisted_group_metadata(¬ification.group_jid) | ||
| .await; | ||
| } | ||
| client | ||
| .rotate_sender_key_on_participant_remove( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.