Support MSC4140: Delayed events (Futures)
See: MSC4140
Pull Request Checklist
- [x] Pull request is based on the develop branch
- [x] Pull request includes a changelog file. 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
EventStoretoEventWorkerStore.". - 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.
- 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
- [x] Code style is correct (run the linters)
Ironically, the reason I had squashed the commits was to try make it easier to review :upside_down_face:
I've now split them up to isolate what should be the most relevant components of the PR.
I've gone through all of @anoadragon453's comments so far, so anyone else can feel free to pick this up :slightly_smiling_face:
Despite the failures in Sytest, it passes when I run it locally. EDIT: as of the last CI run, Sytest passes now; if it continues to do so after further commits, I will remove this message :slightly_smiling_face:
I see you have a matching Complement branch here: matrix-org/complement#734. I assume you would like to hold off on merging this PR until those tests pass?
Yes, as long as it's permissible to merge Complement tests before any implementations are merged.
Finally, once this is merged and done, if you have some time to document how the whole mechanism works (however detailed or briefly) in https://element-hq.github.io/synapse/latest/development/internal_documentation/index.html, that would help future development efforts.
Noted!
Typically we'll merge them at the same time. However, yes, technically the Complement tests can be merged beforehand, as they won't be enabled on develop until this PR is merged.
(Complement CI is expected to fail until https://github.com/matrix-org/complement/pull/734 is merged.)
https://github.com/matrix-org/complement/pull/734 has been merged. I've re-run the failed Complement jobs: https://github.com/element-hq/synapse/actions/runs/10891540744?pr=17326
assuming the Complement tests are happy
They aren't :upside_down_face: Looking into it now. The failures reproduce locally, so I should have a fix ready very soon.
@AndrewFerr looks like there's complement failures on the delayed event tests: https://github.com/element-hq/synapse/actions/runs/10891540744/job/30325234632?pr=17326
Edit: GitHub failed to show your last comment 🤦
Looks like the Complement failures are false-positives when running in Postgres. I'm updating the Complement tests to account for that.
The Postgres tests should be fixed by https://github.com/matrix-org/complement/pull/737. They pass when I run them locally, in either monolith or worker mode.
Alright, I added an extra commit to satisfy what the Complement test was failing on, and now we don't have to worry about merging https://github.com/matrix-org/complement/pull/737 first.
The test expects GET /delayed_events to return events in the order they'll be sent, but no such order was guaranteed when using Postgres. dfde3c2 applies the ordering the test expects.
Strictly speaking, the test doesn't have to cover ordering on GET /delayed_events because the MSC doesn't (yet) define an ordering, so I wrote https://github.com/matrix-org/complement/pull/737/commits/279d1b3724cef49ed8f8357900ef7ddb59bce73c to remove that part of the test.
The other test failure was simply due to leftover data after the first test failed, so I wrote https://github.com/matrix-org/complement/pull/737/commits/6f86880ab2ae6f394146d3b2c15721b2eef15745 to make tests do some teardown to prevent unrelated failures later on.
Note that all tests will continue to pass with https://github.com/matrix-org/complement/pull/737, so it can still be merged safely, but now it is optional to do so :slightly_smiling_face: