WPB-28422 reconcile stale local memberships for deleted remote conversations - #5504
Conversation
zebot
added
the
ok-to-test
There was a problem hiding this comment.
Pull request overview
This PR introduces reconciliation logic to clean up stale local membership records when a remote (federated) conversation is definitively absent from the owning backend’s successful response, and adds integration coverage for this behavior.
Changes:
- Reconcile (delete) stale local memberships when a remote “get conversations” call succeeds but omits locally-tracked remote conversation IDs.
- Add integration tests that validate reconciliation behavior (including idempotency) and ensure memberships are preserved on federation failures.
- Update an existing integration test to tolerate “no-conversation” results for a one-to-one conversation after remote user deletion.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs | Adds reconciliation side-effect when remote conversations are missing from a successful federated response. |
| integration/test/Test/Conversation.hs | Adds integration tests for stale membership reconciliation and adjusts expectations for one-to-one conversation retrieval after remote user deletion. |
| changelog.d/6-federation/WPB-28422 | Documents the new stale-membership cleanup behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
256eb56 to
0caac72
Compare
| handleFailure (Right c) = pure . Right . traverse (.convs) $ c | ||
| handleFailure locallyFound (Right response) = do | ||
| let locallyFoundForDomain = filter ((== tDomain response) . tDomain) locallyFound | ||
| returnedIds = Set.fromList $ map (qualifyAs response . (.id)) (tUnqualified response).convs |
There was a problem hiding this comment.
Looks too complicated to unqualified and then qualify, doesn't Remote a have a functor?
There was a problem hiding this comment.
yeah, the problem is, we need a Set (Remote ConvId) and not Remote (Set ConvId), and I think there is no built in generic operation that does this.
| handleFailure locallyFound (Right response) = do | ||
| let locallyFoundForDomain = filter ((== tDomain response) . tDomain) locallyFound | ||
| returnedIds = Set.fromList $ map (qualifyAs response . (.id)) (tUnqualified response).convs | ||
| missingConversations = filter (`Set.notMember` returnedIds) locallyFoundForDomain |
There was a problem hiding this comment.
| missingConversations = filter (`Set.notMember` returnedIds) locallyFoundForDomain | |
| missingConversations = locallyFoundForDomain `Set.difference` returnedIds |
There was a problem hiding this comment.
yeah, locallyFoundForDomain is not a set, unfortunately
| unless (null remoteNotFoundRemoteIds) $ | ||
| -- FUTUREWORK: This implies that the backends are out of sync. Maybe the | ||
| -- current user should be considered removed from this conversation at this | ||
| -- point. | ||
| P.warn $ | ||
| Logger.msg ("Some locally found conversation ids were not returned by remotes" :: ByteString) | ||
| . Logger.field "convIds" (show remoteNotFoundRemoteIds) |
There was a problem hiding this comment.
Do we need any of this again here?
There was a problem hiding this comment.
This is calling the code above.
| for_ missingConversations $ \conv -> | ||
| ConversationStore.deleteMembersInRemoteConversation conv [tUnqualified lusr] |
There was a problem hiding this comment.
Should we also notify the user about this?
https://wearezeta.atlassian.net/browse/WPB-28422
Checklist
changelog.d