Fix /sync missing membership in state_after#19463
Conversation
| elif (EventTypes.Member, event.sender) not in timeline_state: | ||
| members_to_fetch.add(event.sender) | ||
| # FIXME: we also care about invite targets etc. | ||
|
|
||
| if event.is_state(): | ||
| timeline_state[(event.type, event.state_key)] = event.event_id |
There was a problem hiding this comment.
Yes, there is another bug here, #19464
We should be adding to the timeline_state right away before checking (EventTypes.Member, event.sender) not in timeline_state
state_after/sync state_after
/sync state_after/sync missing membership in state_after
|
What are the odds of this making the next Synapse release on the 24th? |
| @@ -0,0 +1 @@ | |||
| Fix `/sync` missing membership event in `state_after` (experimental [MSC4222](https://github.com/matrix-org/matrix-spec-proposals/pull/4222) implementation) in some scenarios. | |||
There was a problem hiding this comment.
What are the odds of this making the next Synapse release on the 24th?
Low based on current review rate. Would need review today in order to make the RC tomorrow (204-02-17) and then the full release on 2026-02-24.
I also don't want to rush anyone as although I think this is a sane change, could introduce a new problem that the maintainer has to then spend even more effort to put out a new RC about. Feel free to make your case in the backend team lobby room and someone may be sympathetic enough to review.
There was a problem hiding this comment.
Turned into some trouble: #19474 (comment)
(only because of CI though)
anoadragon453
left a comment
There was a problem hiding this comment.
Looks reasonable. Thanks for providing Complement tests, along with detailed context!
|
Merging so we can get tests passing on #19460. |
Reverts #19463 The complement tests haven't been reviewed and require more testing. Discussed in the internal [backend team lobby](https://matrix.to/#/!SGNQGPGUwtcPBUotTL:matrix.org/$XDARK2u2iLL5wWaxiL6tJYkLg80Sn6yWWEQib8ahl5Q?via=jki.re&via=element.io&via=matrix.org) room.
|
For reference, this PR was reverted, see #19474 (comment) Fixes being re-introduced in #19460 |
*This PR was originally only to enable [MSC4222](matrix-org/matrix-spec-proposals#4222) Complement tests (`/sync` `state_after`) but after merging the [fix PR](#19463), we discovered that while the tests pass locally, [fail in CI](#19460 (comment)). To unblock the RC, we decided to revert the fix PR (see #19474 (comment) for more info). To better ensure tests actually pass in CI, we're re-introducing the fix here in the same PR that we enable the tests in.* --- Fix `/sync` missing membership in `state_after`. This applies to any scenario where the first membership has a different `sender` compared to the `state_key` and then the second membership has the same `sender`/`state_key`. Like someone inviting another person and then them joining. Or someone being kicked and then they leave. This bug has been present since the MSC4222 implementation was introduced into the codebase (#17888). --- Fix #19455 Fix element-hq/customer-success#656 I have a feeling, this might also fix these issues (will close and see how people report back): Fix #18182 Fix #19478 ### Testing strategy Complement tests: matrix-org/complement#842 We will need #19460 to merge in order to enable the Complement tests in Synapse but this PR should be merged first so they pass in the first place. I've tested locally that the Complement tests pass with this fix. ### Dev notes [MSC4222](matrix-org/matrix-spec-proposals#4222) has already been merged into the spec and is already part of Matrix v1.16 but we haven't [stabilized support in Synapse yet](#19414). --- In the same ballpark: - #19455 - #17050 - #17430 - #16940 - #18182 - #18793 - #19478 --- Docker builds preferring remote image over the local image we just built, #19460 (comment) `containerd` image store (storage driver, driver type) -> #19475 ### Todo - [x] Wait for #19463 to merge so the Complement tests all pass - [x] Wait for #19475 to merge ### Pull Request Checklist <!-- Please read https://element-hq.github.io/synapse/latest/development/contributing_guide.html before submitting your pull request --> * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Co-authored-by: Andrew Ferrazzutti <andrewf@element.io>
Fix
/syncmissing membership instate_after.This applies to any scenario where the first membership has a different
sendercompared to thestate_keyand then the second membership has the samesender/state_key. Like someone inviting another person and then them joining. Or someone being kicked and then they leave.This bug has been present since the MSC4222 implementation was introduced into the codebase (#17888).
Fix #19455
Fix https://github.com/element-hq/customer-success/issues/656
I have a feeling, this might also fix these issues (will close and see how people report back):
Fix #18182
Testing strategy
Complement tests: #19460 (matrix-org/complement#842)
We will need #19460 to merge in order to enable the Complement tests in Synapse but this PR should be merged first so they pass in the first place. I've tested locally that the Complement tests pass with this fix.
Dev notes
In the same ballpark:
state_after#19455/syncincorrectly calculates state changes for non-gappy syncs with lazy-loading #17050/synccalls #17430/syncand/membersdo not return "current state" #16940Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.