synapse icon indicating copy to clipboard operation
synapse copied to clipboard

Support MSC4140: Delayed events (Futures)

Open AndrewFerr opened this issue 1 year ago • 2 comments

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 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 is correct (run the linters)

AndrewFerr avatar Jun 18 '24 21:06 AndrewFerr

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.

AndrewFerr avatar Aug 07 '24 03:08 AndrewFerr

I've gone through all of @anoadragon453's comments so far, so anyone else can feel free to pick this up :slightly_smiling_face:

AndrewFerr avatar Aug 15 '24 19:08 AndrewFerr

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:

AndrewFerr avatar Sep 13 '24 14:09 AndrewFerr

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!

AndrewFerr avatar Sep 16 '24 18:09 AndrewFerr

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.

anoadragon453 avatar Sep 17 '24 09:09 anoadragon453

(Complement CI is expected to fail until https://github.com/matrix-org/complement/pull/734 is merged.)

anoadragon453 avatar Sep 17 '24 09:09 anoadragon453

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

anoadragon453 avatar Sep 18 '24 15:09 anoadragon453

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 avatar Sep 18 '24 15:09 AndrewFerr

@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 🤦

anoadragon453 avatar Sep 18 '24 16:09 anoadragon453

Looks like the Complement failures are false-positives when running in Postgres. I'm updating the Complement tests to account for that.

AndrewFerr avatar Sep 18 '24 16:09 AndrewFerr

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.

AndrewFerr avatar Sep 18 '24 18:09 AndrewFerr

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:

AndrewFerr avatar Sep 18 '24 19:09 AndrewFerr