fix(groups): keep persisted group metadata in sync on membership change - #761
Merged
Conversation
A participant add/remove updated only the in-memory group cache, never the persisted metadata blob. The in-memory cache expires after ~1h; the next query_info then loads the stale persisted blob and sends an out-of-date participant phash, defeating the not-modified fast-path (forcing a needless full re-query) and diverging from the actual membership. leave() likewise dropped the in-memory entry but left the persisted blob behind. Re-serialize and persist the mutated GroupInfo after a successful add/remove in both the API methods (Groups::add_participants/remove_participants) and the inbound notification handler (the common path), via a shared Client::persist_group_metadata. On leave(), delete the persisted blob through a new Backend::delete_group_metadata (default no-op; sqlite + in-memory impls). Verified against WA Web (WAWebQueryGroupJob persists updated group metadata on membership mutations so the next not-modified comparison stays consistent). Test: in-memory delete_group_metadata round-trip.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds ProtocolStore::delete_group_metadata and backend implementations; introduces Client::persist_group_metadata and Client::invalidate_persisted_group_metadata; and updates Groups::leave, add_participants, remove_participants, and the group notification handler to delete or persist group-metadata so persisted phash stays consistent with in-memory GroupInfo. ChangesGroup Metadata Persistence and Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
You leave a group — we delete the persisted metadata. You mutate participants — we persist or clear the blob so phash comparisons don't force unnecessary re-queries. This must work right. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 2541-2561: Extend the SQLite round-trip test to cover the new
delete path: after inserting/upserting a group record via the existing
put_group_metadata/upsert_group_metadata calls, call
delete_group_metadata(group_jid) and then call get_group_metadata(group_jid) and
assert it returns None (or equivalent "not found"). Also add a negative case
verifying delete only removes records matching the current device_id by creating
a record for a different device_id, calling delete_group_metadata for the
current device, and asserting the other-device record still exists via
get_group_metadata for that jid/device combo. This ensures delete_group_metadata
and get_group_metadata behaviors are locked down.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34b94f80-bed0-462f-9a1a-588d0c3c5d92
📒 Files selected for processing (5)
src/features/groups.rssrc/handlers/notification.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/store/in_memory.rswacore/src/store/traits.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f69026d62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1383
to
+1385
| client | ||
| .persist_group_metadata(¬ification.group_jid, &info) | ||
| .await; |
There was a problem hiding this comment.
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 persist_group_metadata path never updates the stored participant list/phash. The same cache-hit gating exists in the add/remove API paths, which leaves the exact stale persisted blob this change is meant to avoid until a later full query_info refreshes it; consider loading/mutating the persisted GroupInfo or invalidating/deleting it on cache miss.
Useful? React with 👍 / 👎.
Benchmark Results67 unchanged benchmark(s)
|
…a cold cache Review follow-ups on the group-metadata sync PR: - Codex: persisting only on a cache hit missed the exact case this fix targets (a membership change arriving after the ~1h in-memory cache expired) — the block was skipped and the stale persisted blob was left in place. Add an else-branch on cache miss in both the API methods and the notification handler that drops the persisted blob via a new Client::invalidate_persisted_group_metadata, so the next query re-fetches fresh. - CodeRabbit: add the delete assertion to the sqlite group-metadata round-trip test.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/features/groups.rs`:
- Around line 462-464: Add a regression test that simulates an expired group
cache and exercises the add/remove code paths that call
self.client.invalidate_persisted_group_metadata(jid) (the cache-miss
invalidation branch shown in features::groups.rs around the add/remove
handlers), then assert that the persisted blob was actually deleted from the
client storage mock (or database) after the operation; specifically, create a
test that (1) seeds a persisted group metadata blob, (2) sets up the GroupCache
to appear expired for the target jid so the code takes the "Cache expired"
branch, (3) performs the add or remove operation that should call
invalidate_persisted_group_metadata, and (4) verifies the mock client received
the invalidation call and that the persisted blob no longer exists. Ensure
equivalent tests cover the other mentioned locations (around lines handling
cache-miss at the other add/remove branches) so this behavior cannot regress.
In `@src/handlers/notification.rs`:
- Around line 1394-1399: The else-branch that calls
client.invalidate_persisted_group_metadata(¬ification.group_jid) should log a
debug message before invalidation so the cache-expiration event is observable;
update both the Add and Remove branches that call
invalidate_persisted_group_metadata to emit a process/debug log including the
group identifier (notification.group_jid) and a short reason like "cache expired
- dropping persisted metadata" (and include the action type "Add" or "Remove")
immediately before calling
client.invalidate_persisted_group_metadata(¬ification.group_jid) so operators
can trace why persisted group metadata was removed.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3c26ad3-6cbc-4b3b-b501-01e4fb2670f5
📒 Files selected for processing (3)
src/features/groups.rssrc/handlers/notification.rsstorages/sqlite-storage/src/sqlite_store.rs
…invalidation Review follow-ups on the group-metadata sync PR: - CodeRabbit: add a regression test for the cache-miss invalidation path (invalidate_persisted_group_metadata deletes the persisted blob) so the stale-phash fix can't quietly regress. - CodeRabbit: the notification-handler else-branches invalidated the persisted blob silently while the if-branches logged; add matching debug logs so a cache expiry + invalidation is observable when troubleshooting stale group data.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8695ace12e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
When a participant removal succeeds while the group cache is warm, this new awaited persistence write runs before rotate_sender_key_on_participant_remove; with SQLite or a custom backend this can block on disk/network while other outgoing sends are still allowed, so a concurrent send can reuse the old sender key after the server has accepted the removal. Move the rotation ahead of this best-effort metadata persistence (the same ordering exists in the group-notification remove path) so removed members cannot retain decryptable sender-key material during a slow metadata write.
Useful? React with 👍 / 👎.
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.
Closes the features-32 gap.
A participant add/remove updated only the in-memory group cache, never the persisted metadata blob. The in-memory cache expires after ~1h; the next
query_infothen loads the stale persisted blob and sends an out-of-date participant phash, defeating the not-modified fast-path (forcing a needless full re-query) and diverging from the actual membership.leave()likewise dropped the in-memory entry but left the persisted blob behind.The fix keeps the persisted blob in sync:
GroupInfoafter a successful add/remove in both the API methods (Groups::add_participants/remove_participants) and the inbound group-notification handler (the more common path), via a sharedClient::persist_group_metadata(sameserde_jsonformatquery_infoalready uses).leave(), delete the persisted blob through a newBackend::delete_group_metadata(default no-op; implemented for sqlite + in-memory) — we're no longer in the group, so a stale phash from it would only cause a needless re-query if ever read.Verified against WA Web (
WAWebQueryGroupJobpersists updated group metadata on membership mutations so the next not-modified comparison stays consistent).Test: in-memory
delete_group_metadataround-trip (put → get → delete → get None).