MSC4140: put delay_id in unsigned data for sender

[AndrewFerr]

Implements matrix-org/matrix-spec-proposals@49b200d

Pull Request Checklist

• Pull request is based on the develop branch
• 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.
• Code style is correct (run the linters)

[reivilibre]

Seems reasonable overall

[reivilibre]

I'm not sure everything else in this file does it correctly right now, but I would be in favour of making this optional

Suggested change

	delay_id: str
	delay_id: str | None

This will also involve changing the Rust part to return Nones when needed.

I'm not dead-set on this, so open to your thoughts? But it seems like it is more typechecker-friendly this way.

[AndrewFerr]

My vote is to leave this as it is for now, as much as I appreciate proper typing, because there appears to be some nuance to this that I don't fully grasp at the moment & am hesitant to muck around with.

From what I can gather from the Rust code, there are some fields in the metadata object that are always present but may set to None (like stream_ordering and instance_name), and others that may be absent but must always be non-None when present (like txn_id and device_id).

So, it looks like there is a functional difference between the not-typed-as-optional fields that Python code looks up with getattr, and ones that are typed as optional. Since delay_id is used in much the same way as txn_id, IMO it's sufficient for the time being to use the same code patterns of the latter for the former.

[reivilibre]

(we could then avoid this getattr call if we did make it natively str | None)

[reivilibre]

Arguably this should have been done earlier, but now we're adding more things that use this logic, it feels even stronger that it would be worth pulling out a _should_include_same_session_metadata() -> bool
function (or something along those lines) that would let us isolate the condition logic from the actual addition of the fields.

[AndrewFerr]

Part of the condition is the presence of the fields to be added to the event, which IMO makes it not worth splitting this out into a standalone function, lest it just moves the data/logic coupling somewhere else rather than decoupling them.

[reivilibre]

Hmm, not necessarily standalone.

I guess what I was going for was to replace the if-else block starting R483 with one that, instead of setting the fields directly, sets should_include_same_session_metadata = True and have a single block to populate that metadata afterwards (i.e. separate condition from action, also meaning we don't have to write the action twice).

[AndrewFerr]

With d699205, the code that adds delay_id is pulled out from the pre-existing code and is much simpler than before, so IMO there's less of a need to refactor the code for adding the transaction_id.

774e1ff applies some refactorings that I felt are now appropriate considering the new changes.

[reivilibre]

Overall looks good. Some MSC divergences I spotted though, I think.

[reivilibre]

I'm re-reading the MSC now and noticing that the MSC says we should include the delay_id to the event sender, but here we only include when the person is using the exact same device.

Ref: https://github.com/matrix-org/matrix-spec-proposals/blob/1e527c3d880135a4975b56f700adf007e2914565/proposals/4140-delayed-events-futures.md#L296

Which one is correct?

The MSC also says it's only included on /sync (same ref), yet at first glance it seems like this is included on any event-returning CSAPI request.

[AndrewFerr]

I'm re-reading the MSC now and noticing that the MSC says we should include the delay_id to the event sender, but here we only include when the person is using the exact same device. ... Which one is correct?

It should be included for all sessions. Will write a change for this.

The MSC also says it's only included on /sync (same ref), yet at first glance it seems like this is included on any event-returning CSAPI request.

Ah, I had put this code here to do the same as what's done for including the transaction_id in an event, thinking that it only came through /synced events as well. Does the transaction_id get included in any event-returning request? If so, then I'd argue for delay_id to follow suit, and would adjust the MSC accordingly.

[AndrewFerr]

Actually, including delay_id for more than just /sync would definitely be useful, regardless of what is done for transaction_id (or any other unsigned data, for that matter). MSC discussion is here: matrix-org/matrix-spec-proposals#4140 (comment)

As for including delay_id for all sessions, see d699205

[AndrewFerr]

For the record, the MSC discussion settled on including delay_id in unsigned data for all requests by the event's sender, not just /sync.

[reivilibre]

Makes sense. I think returning metadata differently in different endpoints would be a strange choice because it makes it hard for clients to be able to reliably cache events.

[reivilibre]

Thanks!